fix(appsec): Implement limits for analyzed downstream requests#8655
fix(appsec): Implement limits for analyzed downstream requests#8655uurien wants to merge 8 commits into
Conversation
Overall package sizeSelf size: 6.05 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: bdf5880 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-05-29 10:53:56 Comparing candidate commit bdf5880 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1496 metrics, 97 unstable metrics. |
uurien
requested review from
BridgeAR and
bojbrook
and removed request for
a team
![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: 37afb8fceb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Always run handleRedirectResponse before applying body guard outcomes so sampled redirect chains still store the Location decision. Skip response_body_ignored metrics on redirect hops where body analysis is deferred to the follow-up request. Co-authored-by: Cursor <cursoragent@cursor.com>
| "implementation": "A", | ||
| "type": "int", | ||
| "internalPropertyName": "appsec.apiSecurity.maxDownstreamBodyBytes", | ||
| "default": "10485760" |
There was a problem hiding this comment.
Do we have to make this configurable? Would a default not be fine?
| */ | ||
| function getResponseBodyCollectionConfig () { | ||
| return { | ||
| maxBytes: config?.appsec?.apiSecurity?.maxDownstreamBodyBytes ?? DEFAULT_MAX_DOWNSTREAM_BODY_BYTES, |
There was a problem hiding this comment.
| maxBytes: config?.appsec?.apiSecurity?.maxDownstreamBodyBytes ?? DEFAULT_MAX_DOWNSTREAM_BODY_BYTES, | |
| maxBytes: config.appsec.apiSecurity.maxDownstreamBodyBytes |
If we access the config, it is guaranteed to already have done all of that
| return { collect: false, reason: 'content_length_too_big' } | ||
| } | ||
|
|
||
| return { collect: true } |
There was a problem hiding this comment.
I am not sure but I believe V8 would be able to detect the shape better if we define an object at the top where we just change the value before returning. I would add a undefined reason so the shape stays constant.
2 participants
What does this PR do?
Adds limits for downstream HTTP response body collection in the AppSec SSRF path:
DD_API_SECURITY_MAX_DOWNSTREAM_BODY_BYTES(default 10 MB).http/client.jsbefore buffering the response body:Content-Typeonly (application/json,text/json,application/x-www-form-urlencoded).Content-Lengthrequired and non-zero.Content-Lengthmust not exceedmaxBytes.responseBodyIgnoredReasononapm:http:client:response:finishand skips body collection.Motivation
Without limits, a sampled downstream response could buffer unbounded data in memory. This change validates headers before collection and caps the maximum analyzable body size via configuration.
Additional Notes
Content-Length.content_type_invalid,content_length_missing,content_length_too_big.http.spec.jsuse a local HTTP server for deterministic headers and body; generic finish-channel tests still hit datadoghq.System tests
APPSEC-62792