Parse Lambda AppSec request bodies according to their content type - #12363
Parse Lambda AppSec request bodies according to their content type#12363claponcet wants to merge 10 commits into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebad5227cd
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d casts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the surviving parts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58ba1ec8bc
ℹ️ 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".
…nside part headers Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 693dfe0be2
ℹ️ 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".
| } | ||
| final String delimiter = "--" + boundary; | ||
| final int length = body.length(); | ||
| int position = body.startsWith(delimiter) ? 0 : nextDelimiter(body, delimiter, 0); |
There was a problem hiding this comment.
Validate the initial delimiter before treating it as closing
When a multipart preamble begins with a line such as --x--not-a-close, this direct startsWith selects offset 0 without calling endsLine; the close check then stops parsing and never scans the later valid --x delimiter. The body consequently falls back to an opaque string, so key-addressed WAF rules can miss fields that a conforming multipart parser extracts after ignoring the preamble. The fresh evidence beyond the prior fix is this initial-boundary path, which still bypasses the validation now performed by nextDelimiter; validate its terminator as well.
Useful? React with 👍 / 👎.
| final String pair = tokenizer.nextToken(); | ||
| final int equals = pair.indexOf('='); | ||
| final String name = decode(equals == -1 ? pair : pair.substring(0, equals)); | ||
| if (!name.isEmpty()) { |
There was a problem hiding this comment.
Preserve form values with empty parameter names
For a valid urlencoded body such as =payload&ok=1, this condition silently drops the first value but still returns the nonempty map containing ok, so the caller does not fall back to the raw body. The Lambda handler still receives and can decode the original empty-name parameter, while AppSec sees no trace of payload; this also differs from the existing Netty body collector, which retains attributes under data.getName() without rejecting an empty key. Preserve the empty-name entry or treat the whole parse as unusable so an attack value cannot disappear from WAF inspection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The new body limit lets the WAF drop later form fields without a raw-body fallback. The multipart parser also treats a valid preamble near match as a close delimiter and misses later fields.
🤖 Datadog Autotest · Commit 693dfe0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| // These bound the work done in this parser only: exceeding any of them degrades the body to a raw | ||
| // string rather than dropping content. | ||
| static final int MAX_BYTES = 1024 * 1024; | ||
| static final int MAX_PARTS = 256; |
There was a problem hiding this comment.
WAF limit drops later form fields
An attacker can place a harmful value near the end and bypass key-based WAF rules.
Assertion details
- Input: A URL-encoded body with 85 to 256 one-value parameters, or a multipart body with more than 127 scalar fields.
- Expected:
The parser should return the full raw body before the WAF can truncate any field. - Actual:
The parser accepts the structured map. ObjectIntrospection then counts the root, keys, lists, and values against its separate 256-element limit. It drops later fields.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
| } | ||
| final String delimiter = "--" + boundary; | ||
| final int length = body.length(); | ||
| int position = body.startsWith(delimiter) ? 0 : nextDelimiter(body, delimiter, 0); |
There was a problem hiding this comment.
Preamble near match stops multipart parsing
Key-based WAF rules can miss fields that the application parses.
Assertion details
- Input: A valid multipart body starts with a preamble line such as
--boundary--not-a-close, followed by a real delimiter and form fields. - Expected:
The parser should ignore the preamble line and find the valid delimiter that follows it. - Actual:
The initial prefix check accepts the near match. The close-delimiter check then stops parsing. AppSec receives only the raw body.
| int position = body.startsWith(delimiter) ? 0 : nextDelimiter(body, delimiter, 0); | |
| int position = nextDelimiter(body, delimiter, 0); |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
What Does This Do
Dispatches Lambda AppSec request bodies on their declared
Content-Typeinstead of handing every body to a JSON parser:application/json, any+jsonsuffix,application/x-amz-json-1.1,application/javascript) or no usable type at allapplication/x-www-form-urlencodedmultipart/*Content-Type; file parts contribute their filename, not their contenttext/*and everything elseA body is never dropped. Any type we cannot structure, and any parse failure, degrades to the raw string, which the WAF can still match string rules against. In particular a
text/plainbody of12345must reach the WAF as aStringand not as theDoublea JSON parse would produce.The same rule decides what happens to a malformed multipart body. A part whose headers run into the next delimiter is not reported part-by-part: no conforming parser accepts such a body — Commons FileUpload and Netty both reject it outright — so reporting the parts that happen to survive would show the WAF less than the application receives. The whole body degrades to the raw string instead.
Parsing is bounded by three allowances, shared across nesting levels rather than re-satisfied at each one: 1 MiB of characters read, 256 parts or parameters, and 20 levels of nesting. The depth and part limits mirror the WAF's own (
WAFModule.MAX_DEPTH/MAX_ELEMENTS) — structure beyond them is discarded byObjectIntrospectionbefore the WAF sees it, so producing it would be wasted work. Exceeding an allowance degrades the body to a raw string rather than truncating it, since a truncated map would hide the dropped parameter from every rule.Behavior changes worth a reviewer's eye. The JSON-ish decision is now made in one place,
ContentTypeBodyParser.isJsonOrUntyped, which the response path inLambdaEventParsershares with the request path. It matches on the subtype alone, where the response path previously looked forjsonorjavascriptanywhere in the raw header. Three consequences:Content-Typeholding nothing but parameters —; charset=utf-8— counts as declaring no type, and so gets the same best-effort JSON parse an absent header gets. Pinned by a case indispatchesOnContentType.content-typenow gets that best-effort JSON parse too. Previously only an absent header did, since""does not containjson.multipart/form-data; boundary=--json— no longer reaches the JSON parser. This is why the gate matches on the subtype only: a client-chosen boundary must not decide how the body is read.Motivation
Peer tracers already structure urlencoded and multipart Lambda bodies. Without it, a form-encoded or multipart request reaching a Lambda is visible to the WAF only as one opaque string, so any rule addressing
server.request.bodyby key cannot match — the same request routed through a non-Lambda entrypoint would be inspected properly.Additional Notes
MultipartSplitterno longer collects aHashMapof every header per part. OnlyContent-DispositionandContent-Typeare ever read, so the others are matched and dropped as the scan passes them: a body that is nothing but header lines would otherwise allocate a map entry per line, which is why the change is here at all. The naive version of the fix — matching header names inline with a case-insensitiveregionMatches— measured slower on well-formed many-part forms, so the canonical spelling is tried with a case-sensitive compare first and the matching lives in its own small method rather than inlined intosplit.Two known flaky failures were seen in
dd-trace-corewhile validating, both unrelated to this diff and both passing on re-run in isolation:PendingTraceBufferTest.testingTracerFlareDumpWithMultipleTracesandDDAgentWriterCombinedTest.statsdCommFailure. Neither is annotated@Flaky.File parts populate
server.request.body.filenames, which comes almost free once the parts are split. The other two file addresses are left empty on purpose: the Lambda event delivers the body as one UTF-8-decoded string, so a file's bytes are already corrupted by the time we see them, andfiles_contentwould report that damage as if it were the upload.files_field_nameswaits with it.The repeated callback-registration boilerplate in
LambdaAppSecHandleris left for its own change.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
🤖 Generated with Claude Code