Add server.request.body.files_content AppSec address for Tomcat and Netty multipart#11198
Draft
jandro996 wants to merge 20 commits intoalejandro.gonzalez/APPSEC-61875-file-upload-contentfrom
Conversation
…ltipart Read up to 4096 bytes per file (ISO-8859-1, max 25 files) and fire the server.request.body.files_content callback after the filenames callback, only when the request has not already been blocked. - ParameterCollector: add readContent() via reflection on getInputStream(), getContents() getter, and MAX_FILES_TO_INSPECT guard - ParsePartsInstrumentation: dispatch requestFilesContent() after requestFilesFilenames() when t == null - HttpPostRequestDecoderInstrumentation: collect file content inline in the getBodyHttpDatas() loop (in-memory via ByteBuf, on-disk via FileInputStream) and dispatch requestFilesContent() when thr == null after filenames - Add ParameterCollectorImplTest covering truncation, 25-file cap, form-field skip, IOException fallback, and ISO-8859-1 round-trip
…tation Issue 1 — Netty: extract MAX_CONTENT_BYTES=4096 and MAX_FILES_TO_INSPECT=25 as named constants in ParseBodyAdvice, replacing raw magic literals. Issue 3 — Tomcat + Netty: call effectivelyBlocked() after tryCommitBlockingResponse in both the filenames and files_content blocking paths, matching the behaviour already present in CommonsFileUploadAppSecInstrumentation. Issue 4 — Netty: collect file content for ALL FileUpload data items regardless of whether filename is present or empty; HttpDataType.FileUpload is the file-vs-form-field discriminator in Netty's model, so the filename-empty check was incorrectly skipping content for unnamed file parts (inconsistent with FileItemContentReader which only skips isFormField() items).
…ation Issue 1 — Tomcat: inspect file content even when submitted filename is empty. getSubmittedFileName() returns null for form fields (no filename parameter) and "" for file uploads whose client explicitly sent filename="". Null is now the only skip condition, so file parts with an empty filename are inspected but not added to the filenames list, matching the commons-fileupload and Netty behaviour. Issue 2 — Netty: remove unnecessary (long) casts in Math.min; both operands are already int so Math.min(int,int) resolves directly. Issue 3 — Netty: replace manual try/finally on FileInputStream with try-with-resources, consistent with FileItemContentReader style.
…tty files_content Move BlockingException assignment inside the brf != null guard so it is only thrown when the blocking response was actually committed. Also add effectivelyBlocked() to the Tomcat body-processed path, and remove the redundant if (t == null) guard there. For the Netty body-processed path (urlencoded), effectivelyBlocked() is intentionally omitted: tryCommitBlockingResponse() finishes the Netty span synchronously, so calling effectivelyBlocked() afterwards would interact with an already-closed TraceSegment.
…callback is unregistered Pre-fetch the requestFilesContent callback before the body-data loop and guard ByteBuf/FileInputStream reads behind it, consistent with how CommonsFileUploadAppSecInstrumentation already does. Also flattens the post-loop dispatch block by one nesting level.
…ad loop Move thr = new BlockingException(...) for the filenames path in Netty inside the brf != null guard, consistent with the body-processed and content paths in the same advice and with the Tomcat/Liberty pattern. Merge the two FileItem iterations in CommonsFileUploadAppSecInstrumentation into a single loop that builds both filenames and filesContent in one pass, calling FileItemContentReader.readContent() per item instead of a separate FileItemContentReader.readContents() sweep.
The bulk readContents(List<FileItem>) method is no longer called — the advice iterates file items inline calling readContent(FileItem) per item. Remove the method, its now-unused ArrayList/List imports, and the corresponding test cases. Also fix redundant type witness on ArrayList construction in CommonsFileUploadAppSecInstrumentation.
…oad guard style Skip filesContent ArrayList allocation in Netty ParseBodyAdvice when the requestFilesContent callback is not registered, matching the null-sentinel pattern already used in CommonsFileUploadAppSecInstrumentation. Replace the early-return in CommonsFileUploadAppSecInstrumentation's filenames blocking branch with a t == null guard on the content dispatch block, aligning control flow with ParsePartsInstrumentation and HttpPostRequestDecoderInstrumentation.
… callback is unregistered Add an inspectContent boolean to ParameterCollectorImpl, set from the presence of the requestFilesContent callback in ParsePartsInstrumentation before(). When false, addPart() still collects filenames but skips the getInputStream() read, matching the lazy-init approach already used by the Netty and commons-fileupload integrations.
… paths The content path in Netty and both the filenames and content paths in Tomcat were setting the BlockingException outside the if (brf != null) block, meaning the exception would be thrown even when no blocking response had actually been committed. Align all three with the canonical pattern: tryCommitBlockingResponse + effectivelyBlocked + exception assignment all inside if (brf != null). The Netty body-processed path intentionally still omits effectivelyBlocked() because tryCommitBlockingResponse() closes the Netty span synchronously in the test environment, making a subsequent effectivelyBlocked() call fail.
…meterCollector - Replace per-call getMethod() with volatile (Class, Method) entry cache so reflection resolves once per concrete Part class rather than per file per request. - Use try-with-resources for InputStream in readContent() instead of try/finally. - Add unit test covering the Tomcat 7 getFilename() fallback path.
… file content Also fix HttpPostRequestDecoderInstrumentation to fire on PREEPILOGUE+isLastChunk so that multipart requests delivered as a FullHttpRequest (single shot) trigger the requestBodyProcessed/requestFilesFilenames/requestFilesContent callbacks. The PREEPILOGUE→EPILOGUE transition requires a second parseBody() call which never comes when the full request arrives in one shot; PREEPILOGUE+isLastChunk is equivalent to EPILOGUE in that scenario.
jandro996
added
type: enhancement
…adContentReader helper Move the in-memory/disk-backed FileUpload content reading logic out of ParseBodyAdvice into a testable static helper class. Also fixes the partial-read risk on disk-backed uploads (single fis.read() replaced by a loop) and broadens the catch to Exception to handle IllegalReferenceCountException from released ByteBufs.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 62 metrics, 9 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.067 s) : 0, 1066951
Total [baseline] (8.92 s) : 0, 8920026
Agent [candidate] (1.061 s) : 0, 1061385
Total [candidate] (8.856 s) : 0, 8855524
section iast
Agent [baseline] (1.233 s) : 0, 1233167
Total [baseline] (9.582 s) : 0, 9581557
Agent [candidate] (1.233 s) : 0, 1232990
Total [candidate] (9.567 s) : 0, 9566770
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.272 ms) : 0, 1272
crashtracking [candidate] (1.254 ms) : 0, 1254
BytebuddyAgent [baseline] (640.596 ms) : 0, 640596
BytebuddyAgent [candidate] (635.992 ms) : 0, 635992
AgentMeter [baseline] (29.825 ms) : 0, 29825
AgentMeter [candidate] (29.625 ms) : 0, 29625
GlobalTracer [baseline] (250.347 ms) : 0, 250347
GlobalTracer [candidate] (249.882 ms) : 0, 249882
AppSec [baseline] (32.631 ms) : 0, 32631
AppSec [candidate] (32.481 ms) : 0, 32481
Debugger [baseline] (59.448 ms) : 0, 59448
Debugger [candidate] (59.212 ms) : 0, 59212
Remote Config [baseline] (604.384 µs) : 0, 604
Remote Config [candidate] (590.108 µs) : 0, 590
Telemetry [baseline] (8.081 ms) : 0, 8081
Telemetry [candidate] (8.021 ms) : 0, 8021
Flare Poller [baseline] (7.604 ms) : 0, 7604
Flare Poller [candidate] (8.029 ms) : 0, 8029
section iast
crashtracking [baseline] (1.234 ms) : 0, 1234
crashtracking [candidate] (1.245 ms) : 0, 1245
BytebuddyAgent [baseline] (810.387 ms) : 0, 810387
BytebuddyAgent [candidate] (810.531 ms) : 0, 810531
AgentMeter [baseline] (11.439 ms) : 0, 11439
AgentMeter [candidate] (11.459 ms) : 0, 11459
GlobalTracer [baseline] (239.616 ms) : 0, 239616
GlobalTracer [candidate] (240.288 ms) : 0, 240288
IAST [baseline] (29.272 ms) : 0, 29272
IAST [candidate] (29.089 ms) : 0, 29089
AppSec [baseline] (31.163 ms) : 0, 31163
AppSec [candidate] (28.458 ms) : 0, 28458
Debugger [baseline] (62.151 ms) : 0, 62151
Debugger [candidate] (63.298 ms) : 0, 63298
Remote Config [baseline] (542.707 µs) : 0, 543
Remote Config [candidate] (529.079 µs) : 0, 529
Telemetry [baseline] (7.755 ms) : 0, 7755
Telemetry [candidate] (7.695 ms) : 0, 7695
Flare Poller [baseline] (3.422 ms) : 0, 3422
Flare Poller [candidate] (3.405 ms) : 0, 3405
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.067 s) : 0, 1067480
Total [baseline] (11.134 s) : 0, 11134210
Agent [candidate] (1.06 s) : 0, 1060126
Total [candidate] (11.096 s) : 0, 11096275
section appsec
Agent [baseline] (1.273 s) : 0, 1272817
Total [baseline] (11.013 s) : 0, 11012956
Agent [candidate] (1.275 s) : 0, 1275176
Total [candidate] (11.033 s) : 0, 11032719
section iast
Agent [baseline] (1.24 s) : 0, 1239852
Total [baseline] (11.413 s) : 0, 11413195
Agent [candidate] (1.235 s) : 0, 1234940
Total [candidate] (11.399 s) : 0, 11399176
section profiling
Agent [baseline] (1.205 s) : 0, 1204549
Total [baseline] (11.137 s) : 0, 11137464
Agent [candidate] (1.188 s) : 0, 1187938
Total [candidate] (11.137 s) : 0, 11137105
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.25 ms) : 0, 1250
crashtracking [candidate] (1.23 ms) : 0, 1230
BytebuddyAgent [baseline] (639.925 ms) : 0, 639925
BytebuddyAgent [candidate] (633.811 ms) : 0, 633811
AgentMeter [baseline] (29.898 ms) : 0, 29898
AgentMeter [candidate] (29.624 ms) : 0, 29624
GlobalTracer [baseline] (251.281 ms) : 0, 251281
GlobalTracer [candidate] (249.716 ms) : 0, 249716
AppSec [baseline] (32.417 ms) : 0, 32417
AppSec [candidate] (32.448 ms) : 0, 32448
Debugger [baseline] (60.001 ms) : 0, 60001
Debugger [candidate] (59.951 ms) : 0, 59951
Remote Config [baseline] (589.091 µs) : 0, 589
Remote Config [candidate] (603.571 µs) : 0, 604
Telemetry [baseline] (8.847 ms) : 0, 8847
Telemetry [candidate] (8.051 ms) : 0, 8051
Flare Poller [baseline] (6.812 ms) : 0, 6812
Flare Poller [candidate] (8.371 ms) : 0, 8371
section appsec
crashtracking [baseline] (1.242 ms) : 0, 1242
crashtracking [candidate] (1.239 ms) : 0, 1239
BytebuddyAgent [baseline] (680.972 ms) : 0, 680972
BytebuddyAgent [candidate] (680.637 ms) : 0, 680637
AgentMeter [baseline] (12.249 ms) : 0, 12249
AgentMeter [candidate] (12.32 ms) : 0, 12320
GlobalTracer [baseline] (251.363 ms) : 0, 251363
GlobalTracer [candidate] (252.455 ms) : 0, 252455
IAST [baseline] (24.547 ms) : 0, 24547
IAST [candidate] (24.657 ms) : 0, 24657
AppSec [baseline] (187.41 ms) : 0, 187410
AppSec [candidate] (188.675 ms) : 0, 188675
Debugger [baseline] (66.337 ms) : 0, 66337
Debugger [candidate] (66.646 ms) : 0, 66646
Remote Config [baseline] (583.449 µs) : 0, 583
Remote Config [candidate] (576.857 µs) : 0, 577
Telemetry [baseline] (7.975 ms) : 0, 7975
Telemetry [candidate] (7.908 ms) : 0, 7908
Flare Poller [baseline] (3.504 ms) : 0, 3504
Flare Poller [candidate] (3.452 ms) : 0, 3452
section iast
crashtracking [baseline] (1.264 ms) : 0, 1264
crashtracking [candidate] (1.241 ms) : 0, 1241
BytebuddyAgent [baseline] (813.879 ms) : 0, 813879
BytebuddyAgent [candidate] (809.34 ms) : 0, 809340
AgentMeter [baseline] (11.489 ms) : 0, 11489
AgentMeter [candidate] (11.416 ms) : 0, 11416
GlobalTracer [baseline] (241.489 ms) : 0, 241489
GlobalTracer [candidate] (241.011 ms) : 0, 241011
IAST [baseline] (30.108 ms) : 0, 30108
IAST [candidate] (29.22 ms) : 0, 29220
AppSec [baseline] (27.606 ms) : 0, 27606
AppSec [candidate] (29.852 ms) : 0, 29852
Debugger [baseline] (65.97 ms) : 0, 65970
Debugger [candidate] (64.989 ms) : 0, 64989
Remote Config [baseline] (542.007 µs) : 0, 542
Remote Config [candidate] (533.574 µs) : 0, 534
Telemetry [baseline] (7.844 ms) : 0, 7844
Telemetry [candidate] (7.802 ms) : 0, 7802
Flare Poller [baseline] (3.44 ms) : 0, 3440
Flare Poller [candidate] (3.384 ms) : 0, 3384
section profiling
crashtracking [baseline] (1.213 ms) : 0, 1213
crashtracking [candidate] (1.197 ms) : 0, 1197
BytebuddyAgent [baseline] (703.51 ms) : 0, 703510
BytebuddyAgent [candidate] (693.736 ms) : 0, 693736
AgentMeter [baseline] (9.314 ms) : 0, 9314
AgentMeter [candidate] (9.195 ms) : 0, 9195
GlobalTracer [baseline] (210.53 ms) : 0, 210530
GlobalTracer [candidate] (207.417 ms) : 0, 207417
AppSec [baseline] (33.33 ms) : 0, 33330
AppSec [candidate] (32.838 ms) : 0, 32838
Debugger [baseline] (66.915 ms) : 0, 66915
Debugger [candidate] (65.688 ms) : 0, 65688
Remote Config [baseline] (610.791 µs) : 0, 611
Remote Config [candidate] (586.511 µs) : 0, 587
Telemetry [baseline] (7.977 ms) : 0, 7977
Telemetry [candidate] (7.741 ms) : 0, 7741
Flare Poller [baseline] (3.598 ms) : 0, 3598
Flare Poller [candidate] (3.497 ms) : 0, 3497
ProfilingAgent [baseline] (95.293 ms) : 0, 95293
ProfilingAgent [candidate] (94.699 ms) : 0, 94699
Profiling [baseline] (95.864 ms) : 0, 95864
Profiling [candidate] (95.268 ms) : 0, 95268
LoadParameters
See matching parameters
SummaryFound 4 performance improvements and 3 performance regressions! Performance is the same for 12 metrics, 17 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section baseline
no_agent (17.858 ms) : 17675, 18040
. : milestone, 17858,
appsec (18.431 ms) : 18245, 18617
. : milestone, 18431,
code_origins (19.729 ms) : 19533, 19925
. : milestone, 19729,
iast (17.749 ms) : 17576, 17921
. : milestone, 17749,
profiling (18.456 ms) : 18272, 18641
. : milestone, 18456,
tracing (19.569 ms) : 19372, 19766
. : milestone, 19569,
section candidate
no_agent (18.167 ms) : 17980, 18354
. : milestone, 18167,
appsec (18.628 ms) : 18435, 18821
. : milestone, 18628,
code_origins (17.934 ms) : 17754, 18113
. : milestone, 17934,
iast (19.335 ms) : 19141, 19529
. : milestone, 19335,
profiling (19.214 ms) : 19017, 19411
. : milestone, 19214,
tracing (17.983 ms) : 17805, 18162
. : milestone, 17983,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section baseline
no_agent (1.267 ms) : 1254, 1280
. : milestone, 1267,
iast (3.398 ms) : 3351, 3445
. : milestone, 3398,
iast_FULL (6.073 ms) : 6011, 6136
. : milestone, 6073,
iast_GLOBAL (3.686 ms) : 3625, 3747
. : milestone, 3686,
profiling (2.333 ms) : 2311, 2356
. : milestone, 2333,
tracing (1.869 ms) : 1853, 1884
. : milestone, 1869,
section candidate
no_agent (1.313 ms) : 1300, 1325
. : milestone, 1313,
iast (3.403 ms) : 3356, 3451
. : milestone, 3403,
iast_FULL (6.053 ms) : 5991, 6114
. : milestone, 6053,
iast_GLOBAL (3.654 ms) : 3598, 3710
. : milestone, 3654,
profiling (2.232 ms) : 2209, 2255
. : milestone, 2232,
tracing (1.843 ms) : 1828, 1859
. : milestone, 1843,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section baseline
no_agent (14.788 s) : 14788000, 14788000
. : milestone, 14788000,
appsec (15.052 s) : 15052000, 15052000
. : milestone, 15052000,
iast (18.314 s) : 18314000, 18314000
. : milestone, 18314000,
iast_GLOBAL (17.948 s) : 17948000, 17948000
. : milestone, 17948000,
profiling (14.581 s) : 14581000, 14581000
. : milestone, 14581000,
tracing (14.957 s) : 14957000, 14957000
. : milestone, 14957000,
section candidate
no_agent (15.315 s) : 15315000, 15315000
. : milestone, 15315000,
appsec (15.098 s) : 15098000, 15098000
. : milestone, 15098000,
iast (18.108 s) : 18108000, 18108000
. : milestone, 18108000,
iast_GLOBAL (18.037 s) : 18037000, 18037000
. : milestone, 18037000,
profiling (15.129 s) : 15129000, 15129000
. : milestone, 15129000,
tracing (15.034 s) : 15034000, 15034000
. : milestone, 15034000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~cac2d3d976, baseline=1.62.0-SNAPSHOT~71f9713d93
dateFormat X
axisFormat %s
section baseline
no_agent (1.485 ms) : 1474, 1497
. : milestone, 1485,
appsec (2.547 ms) : 2491, 2603
. : milestone, 2547,
iast (2.276 ms) : 2206, 2347
. : milestone, 2276,
iast_GLOBAL (2.309 ms) : 2239, 2380
. : milestone, 2309,
profiling (2.51 ms) : 2287, 2733
. : milestone, 2510,
tracing (2.09 ms) : 2036, 2144
. : milestone, 2090,
section candidate
no_agent (1.486 ms) : 1474, 1498
. : milestone, 1486,
appsec (3.817 ms) : 3594, 4040
. : milestone, 3817,
iast (2.279 ms) : 2209, 2349
. : milestone, 2279,
iast_GLOBAL (2.313 ms) : 2243, 2384
. : milestone, 2313,
profiling (2.095 ms) : 2040, 2149
. : milestone, 2095,
tracing (2.075 ms) : 2022, 2129
. : milestone, 2075,
|
Member
Author
|
@codex review |
![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: 43d9017c46
ℹ️ 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".
…ocessed subscriber
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 510b150f9c
ℹ️ 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".
Member
Author
|
@codex review |
… content blocking paths
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…d content blocking paths
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Skip form attributes when only file content is requested
When AppSec has a server.request.body.files_content subscriber but no requestBodyProcessed subscriber, the new callback == null && contentCb == null gate lets this loop run with callback == null, yet it still calls Attribute.getValue() for every non-file multipart field even though attributes is never published. A large, disk-backed, or unreadable form field can allocate unnecessarily or set exc and make the advice throw before/after file-content inspection, so a files-content-only WAF rule can break requests that contain ordinary form fields; guard this branch with callback != null or skip building attributes unless it will be sent.
ℹ️ 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".
…k is absent When only requestFilesContent is registered, avoid calling getValue() on form attributes (which may trigger disk reads for large fields).
Member
Author
|
@codex review |
jandro996
changed the title
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Extends the
server.request.body.files_contentWAF address (introduced in #11137) to Tomcat and Netty multipart instrumentation. Each uploaded file contributes its first 4 096 bytes (ISO-8859-1), up to 25 files per request.Netty instrumentation (
HttpPostRequestDecoderInstrumentation)ParseBodyAdviceto collect file contents; addsNettyFileUploadContentReaderhelper (injected viahelperClassNames()) that reads in-memory uploads viagetByteBuf()and disk-backed uploads viaFileInputStream(fileUpload.getFile())PREEPILOGUEvsEPILOGUEstatus mismatch: when aFullHttpRequestarrives in one shot,HttpPostMultipartRequestDecoder.parseBody()leaves status atPREEPILOGUE(the secondparseBody()call that would advance toEPILOGUEnever comes); the advice now also fires onPREEPILOGUE + isLastChunk == truerequestBodyProcessedandrequestFilesContentcallbacks before the early-return guard, usingcallback == null && contentCb == nullas the exit condition; skips form attribute I/O whenrequestBodyProcessedis not registeredTomcat instrumentation (
ParameterCollector/ParsePartsInstrumentation)ParameterCollector.addPart()to read file content via reflection onPart.getInputStream()(avoids a direct bytecode reference to Tomcat version-specificParttypes); resolvesgetSubmittedFileName()(Servlet 3.1+ / Tomcat 8+) with fallback togetFilename()(Tomcat 7)Methodper concretePartclass using avolatile Map.Entry<Class<?>, Method>for safe, one-shot publication under the Java Memory Modelnullfilename = no filename parameter = form field (skip); empty string = file upload without a name (inspect content)ParsePartsInstrumentation: passesinspectContentflag (only true whenrequestFilesContentis registered) toParameterCollectorImplto avoid unnecessary I/OMotivation
Extends the AppSec file upload attack surface to cover file content (not just filenames) on Tomcat and Netty, enabling WAF rules that inspect uploaded payloads for malicious content (e.g. PHP webshells, EICAR strings).
Jira ticket: APPSEC-61875
Additional Notes
Reflection cache design for Tomcat —
ParameterCollectoruses avolatile Map.Entry<Class<?>, Method>(not a plainvolatile Method) because the Part concrete class can vary across Tomcat versions in the same JVM. The cache entry stores both the class identity and the resolved method; when the class changes the method is re-resolved. This follows the static-field pattern for multi-version library support without a direct bytecode reference to the varying type.effectivelyBlocked()absent from all three Netty blocking paths —NettyBlockResponseFunction.tryCommitBlockingResponsefinishes the Netty span synchronously (viaBlockingResponseHandler), which internally callseffectivelyBlocked(). Calling it a second time on an already-finished span throws. The Tomcat paths do not have this constraint and correctly calleffectivelyBlocked()aftert = new BlockingException(...).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 issueNote: 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.