Add server.request.body.files_content AppSec address support for Jetty by jandro996 · Pull Request #11706 · DataDog/dd-trace-java

jandro996 · 2026-06-23T10:40:52Z

What Does This Do

Adds extractContents() to MultipartHelper (jetty-appsec-9.2/9.3/9.4/11.0) and PartHelper (jetty-appsec-8.1.3) to extract file content from multipart Parts, reading up to dd.appsec.max-file-content-bytes (default 4096) bytes per part, capped at dd.appsec.max-file-content-count (default 25) parts per request
Adds fireFilesContentEvent() to each helper, firing the server.request.body.files_content IG event via EVENTS.requestFilesContent()
Updates both advice classes in all five AppSec modules to fire the content event sequentially after the filenames event: t = fireFilenamesEvent(...); if (t == null) t = fireFilesContentEvent(...)
Form fields (parts with null submitted filename) are excluded; files with empty filename ("") are included for content inspection but excluded from filenames — matches the filter established in Add server.request.body.files_content AppSec address for Tomcat and Netty multipart #11198
Content is decoded via MultipartContentDecoder.readInputStream() with charset detection from the part's Content-Type header
For Jetty 8.1.3, filenameFromPart() (Content-Disposition header parsing) is used in place of getSubmittedFileName(), which was only added in Servlet 3.1
Adds unit tests for extractContents() in all five modules (null/empty, form-field skip, empty-filename include, content read, byte truncation, IOException fallback, count cap)
Enables testBodyFilesContent() in integration test classes for Jetty 8.x, 9.2, 9.3, 9.4, 10.0, and 11.0

Motivation

Extends the server.request.body.files_content WAF address to Jetty, following the Tomcat/Netty implementation in #11198 and building on the Jetty filenames support from #10988.

Additional Notes

The content event fires sequentially after the filenames event — only when filenames did not block. This matches the intent from #11137 (commons-fileupload). The MAX_CONTENT_BYTES and MAX_FILES_TO_INSPECT constants are static final fields on the helper classes (not on advice inner classes with @RequiresRequestContext) to avoid muzzle validation failures at CI.

Contributor Checklist

Format the title according to the contribution guidelines
Assign the type: and (comp: or inst:) labels in addition to any other useful labels
Avoid using close, fix, or any linking keywords when referencing an issue
Use solves instead, and assign the PR milestone to the issue
Update the CODEOWNERS file on source file addition, migration, or deletion
Update public documentation with any new configuration flags or behaviors

Jira ticket: N/A

Note: Once your PR is ready to merge, add it to the merge queue by commenting /merge. /merge -c cancels the queue request. /merge -f --reason "reason" skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.

- Add extractContents(), readFileContent(), and fireFilesContentEvent() to MultipartHelper (9.2/9.3/9.4/11.0) and PartHelper (8.1.3) - Fire files_content event sequentially after filenames event in all five Jetty AppSec advice classes - Add unit tests for extractContents in all five modules - Enable testBodyFilesContent() in integration test classes for all instrumented Jetty versions (8.x, 9.2, 9.3, 9.4, 10.0, 11.0)

jandro996 · 2026-06-23T10:47:00Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66fad6bbfd

ℹ️ 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".

dd-octo-sts · 2026-06-23T11:02:07Z

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	13.98 s	13.93 s	[-0.2%; +0.9%] (no difference)
startup:insecure-bank:tracing:Agent	12.97 s	12.97 s	[-0.7%; +0.6%] (no difference)
startup:petclinic:appsec:Agent	16.89 s	16.55 s	[+1.0%; +3.1%] (significantly worse)
startup:petclinic:iast:Agent	16.91 s	16.88 s	[-0.5%; +0.8%] (no difference)
startup:petclinic:profiling:Agent	16.78 s	16.92 s	[-1.8%; +0.1%] (no difference)
startup:petclinic:sca:Agent	16.92 s	16.61 s	[+1.0%; +2.8%] (maybe worse)
startup:petclinic:tracing:Agent	16.11 s	16.04 s	[-0.6%; +1.5%] (no difference)

Commit: 8fdf93cb · CI Pipeline · Benchmarking Platform UI

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

…ring-sensitive test - RequestGetPartsInstrumentation (Jetty 8): add bodyBlock == null guard before fireFilesContentEvent in both GetFilenamesAdvice and GetPartAdvice, consistent with Tomcat, Netty, and Jersey implementations (PR #11198, #11229) - HttpServerTest: change max-files-limit assertion to count-based check; Jetty returns parts in HashMap order (not insertion order), so checking for a specific file number by position was fragile

jandro996 · 2026-06-23T11:36:50Z

Addressed in d605e01:

Jetty 8 body-block guard: added bodyBlock == null && before fireFilesContentEvent in both GetFilenamesAdvice and GetPartAdvice — consistent with the Tomcat, Netty, and Jersey pattern (PRs Add server.request.body.files_content AppSec address for Tomcat and Netty multipart #11198, Add server.request.body.files_content AppSec address for Jersey and RESTEasy #11229) where content only fires when no prior event blocked.
Ordering-sensitive test fix: the max-files-limit assertion now counts occurrences of content_of_file_ instead of checking for a specific file number. Jetty returns parts in HashMap order rather than insertion order, so the file excluded by the limit isn't deterministically the last one sent.

amarziali

ok for apm-java (we only own the HttpServerTest) so our review is not blocking. However I left comments (not exhaustive since there are more occurrences) about the fact that there are a lot of exception logged. Those are surfaced through the telemetry and needs to get triaged. If those logs are not actionable I suggest to use the EXCLUDE_TELEMETRY marker

Add EXCLUDE_TELEMETRY marker to all log.debug calls in PartHelper/MultipartHelper that handle exceptions from user-controlled data (malformed parts, stream I/O failures). These are not actionable from the agent side and would pollute telemetry signal. The MethodHandle invocation failure in PartHelper.getAllParts is intentionally left without the marker — that failure IS actionable as it indicates an instrumentation-internal issue.

jandro996 · 2026-06-24T14:19:30Z

ok for apm-java (we only own the HttpServerTest) so our review is not blocking. However I left comments (not exhaustive since there are more occurrences) about the fact that there are a lot of exception logged. Those are surfaced through the telemetry and needs to get triaged. If those logs are not actionable I suggest to use the EXCLUDE_TELEMETRY marker

Good catch. These exceptions come from user-controlled data (malformed multipart parts, I/O failures reading temp files) and are not actionable from the agent side, so I've added EXCLUDE_TELEMETRY to all of them across the five modules. Applied in 4c96d0d.

The one exception is PartHelper.getAllParts a MethodHandle invocation failure there is instrumentation-internal and should reach telemetry, so it stays without the marker

extractFormFields() bounded the bytes read per field but not the number of fields processed, leaving total allocation unbounded as O(fields x byteCap). Cap the field count via Config.get().getAppSecMaxFileContentCount(), mirroring the MAX_FILES_TO_INSPECT pattern used by MultipartHelper#extractContents() (PR #11706) for file content.

…11876) fix: bound unbounded multipart form-field buffering in Jetty 8 AppSec advice PartHelper.readPartContent() copied a multipart Part's InputStream into an unbounded ByteArrayOutputStream before any WAF size check applied, allowing a single oversized text field to exhaust heap. Cap the read at Config.get().getAppSecMaxFileContentBytes(), reusing the existing file-content byte cap since there is no dedicated form-field knob. fix: cap number of multipart form fields inspected in Jetty 8 PartHelper extractFormFields() bounded the bytes read per field but not the number of fields processed, leaving total allocation unbounded as O(fields x byteCap). Cap the field count via Config.get().getAppSecMaxFileContentCount(), mirroring the MAX_FILES_TO_INSPECT pattern used by MultipartHelper#extractContents() (PR #11706) for file content. Merge branch 'master' into APMSP-3580 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

jandro996 added type: feature Enhancements and improvements comp: asm waf Application Security Management (WAF) labels Jun 23, 2026

jandro996 changed the title ~~Add server.request.body.files_content WAF address support to Jetty~~ Add server.request.body.files_content AppSec address support for Jetty Jun 23, 2026

chatgpt-codex-connector Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread ...8.1.3/src/main/java/datadog/trace/instrumentation/jetty8/RequestGetPartsInstrumentation.java Outdated

This comment has been minimized.

Sign in to view

jandro996 marked this pull request as ready for review June 23, 2026 13:38

jandro996 requested review from a team as code owners June 23, 2026 13:38

jandro996 requested review from amarziali, claponcet, manuel-alvarez-alvarez and vandonr and removed request for a team June 23, 2026 13:38

claponcet reviewed Jun 24, 2026

View reviewed changes

Comment thread ...ec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/MultipartHelper.java Outdated

jandro996 added 2 commits June 24, 2026 11:57

fix: correct Javadoc version in jetty92 MultipartHelper (9.3.x → 9.2.x)

8fdf93c

Merge branch 'master' into jetty-filecontent

b85e109

jandro996 requested a review from claponcet June 24, 2026 11:52

claponcet approved these changes Jun 24, 2026

View reviewed changes

amarziali approved these changes Jun 24, 2026

View reviewed changes

jandro996 mentioned this pull request Jul 7, 2026

Fix unbounded multipart form-field buffering in Jetty 8 PartHelper #11876

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add server.request.body.files_content AppSec address support for Jetty#11706

Add server.request.body.files_content AppSec address support for Jetty#11706
jandro996 wants to merge 5 commits into
masterfrom
jetty-filecontent

jandro996 commented Jun 23, 2026 •

edited

Loading

Uh oh!

jandro996 commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

This comment has been minimized.

dd-octo-sts Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

jandro996 commented Jun 23, 2026

Uh oh!

Uh oh!

amarziali left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jandro996 commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jandro996 commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Additional Notes

Contributor Checklist

Uh oh!

jandro996 commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

This comment has been minimized.

dd-octo-sts Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

jandro996 commented Jun 23, 2026

Uh oh!

Uh oh!

amarziali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jandro996 commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jandro996 commented Jun 23, 2026 •

edited

Loading

dd-octo-sts Bot commented Jun 23, 2026 •

edited

Loading