feat(aap): In App WAF support for lambda#7783
Conversation
Overall package sizeSelf size: 5.56 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7783 +/- ##
==========================================
+ Coverage 73.76% 73.81% +0.04%
==========================================
Files 782 783 +1
Lines 36316 36384 +68
==========================================
+ Hits 26790 26857 +67
- Misses 9526 9527 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-04-27 08:30:00 Comparing candidate commit 590897a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1344 metrics, 100 unstable metrics. |
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 590897a | Docs | Datadog PR Page | Give us feedback! |
| function reportAttack ({ events: attackData, actions }, req, rootSpan) { | ||
| if (!req) { | ||
| req = storage('legacy').getStore()?.req | ||
| } |
There was a problem hiding this comment.
i'm worried that in this function, if we don't pass the empty object fake "req", it will try to get it from the store, which would make this req variable here undefined, and crash later down the function because it's trying to access props in req (req.socket, req.body, etc).
The safety mecanism that was in place before was the rootSpan check. If req was missing, then rootSpan would also be missing, and thus the if(!rootSpan) return would protect us from undefined req.
But now that we can pass the rootSpan separately from req, this safety check is inoperant. Does that make sense ?
Can you prove to me that req can never be empty here ?
There was a problem hiding this comment.
Yep, your concern is legit: decoupling rootSpan from req breaks the implicit safety mechanism. But in the lambda path, req is never null or undefined in reportAttack (it's an invocationKey ({}), a truthy object flowing from lambda.js). The only place where req is null is in finishRequest.
I've added a test to check the WAF path with a strict req object that throws on any unaudited property access, making it fail if someone adds req.something without a guard in this path.
|
Overall this PR is a bit scary because it invalidates assumptions that were made during the entire evolution of our codebase. We should really be careful about these changes, a strong test suite, and proactive monitoring. For now I feel like the test suite is a bit light no ? |
CarlesDD
force-pushed
the
ccapell/APPSEC-60752/in-app-waf-port
branch
from
f4de885 to
e0b39e1
Compare
Agree this needs careful handling. I've pushed additional changes to strengthen the safety net:
|
CarlesDD
force-pushed
the
ccapell/APPSEC-60752/in-app-waf-port
branch
from
c83559f to
590897a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 590897a5f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| rootSpan = req && web.root(req) | ||
| } | ||
|
|
||
| if (!rootSpan) return |
There was a problem hiding this comment.
| if (!rootSpan) return | |
| if (!req || !rootSpan) return |
2 participants
What does this PR do?
Adds AppSec support for AWS lambda to dd-trace-js by introducing DC handlers that allow the
datadog-lambda-jslayer to delegate WAF execution to the tracer.Changes:
datadog:lambda:start-invocationanddatadog:lambda:end-invocationdiagnostic channels with its subscribers (appsec/lambdja.js), which maps extracted HTTP data to WAF addresses, run the WAF and reports the results.reportMetrics,reportAttack,reportAttributes, andfinishRequestnow accept an optionalrootSpanparameter, falling back toweb.root(req)when not provided. This allows the Lambda handler to pass the span directly since there is noreqobject in LambdarootSpanthreading:waf/index.jsandwaf_context_wrapper.jsthreadrootSpanthrough the call chain so thatkeepTraceand reporter calls work correctly for LambdaMotivation
Porting the In-App WAF security product to AWS Lambda for the Node.js runtime. The Lambda layer extracts HTTP data from the event and dispatches it to the tracer's AppSec domain via DC.
This approach keeps all security logic (WAF execution, attack reporting, trace keep decisions) inside dd-trace-js, and the Lambda layer is only responsible for extracting raw HTTP data and publishing it.
Additional Notes
datadog-lambda-jspublishes to them.DD_APPSEC_ENABLEDenv var controls everything.APPSEC-60752