Uh oh!
There was an error while loading. Please reload this page.
Fix charset decoding for server.request.body.files_content in commons-fileupload - #11212
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 16 commits intoApr 28, 2026
Conversation
…ileItemContentReader
… reuse across integrations
…Test Charset fallback scenarios are covered by MultipartContentDecoderTest. One integration test is kept to verify that getContentType() is passed through.
Uh oh!
There was an error while loading. Please reload this page.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 63 metrics, 8 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.064 s) : 0, 1064236
Total [baseline] (8.825 s) : 0, 8825491
Agent [candidate] (1.064 s) : 0, 1064393
Total [candidate] (8.837 s) : 0, 8837398
section iast
Agent [baseline] (1.241 s) : 0, 1241349
Total [baseline] (9.528 s) : 0, 9527842
Agent [candidate] (1.248 s) : 0, 1247917
Total [candidate] (9.545 s) : 0, 9545178
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.232 ms) : 0, 1232
crashtracking [candidate] (1.218 ms) : 0, 1218
BytebuddyAgent [baseline] (636.228 ms) : 0, 636228
BytebuddyAgent [candidate] (635.815 ms) : 0, 635815
AgentMeter [baseline] (29.472 ms) : 0, 29472
AgentMeter [candidate] (29.47 ms) : 0, 29470
GlobalTracer [baseline] (249.048 ms) : 0, 249048
GlobalTracer [candidate] (249.158 ms) : 0, 249158
AppSec [baseline] (32.761 ms) : 0, 32761
AppSec [candidate] (32.626 ms) : 0, 32626
Debugger [baseline] (60.495 ms) : 0, 60495
Debugger [candidate] (59.872 ms) : 0, 59872
Remote Config [baseline] (598.483 µs) : 0, 598
Remote Config [candidate] (598.748 µs) : 0, 599
Telemetry [baseline] (8.366 ms) : 0, 8366
Telemetry [candidate] (8.37 ms) : 0, 8370
Flare Poller [baseline] (9.929 ms) : 0, 9929
Flare Poller [candidate] (11.26 ms) : 0, 11260
section iast
crashtracking [baseline] (1.225 ms) : 0, 1225
crashtracking [candidate] (1.221 ms) : 0, 1221
BytebuddyAgent [baseline] (821.637 ms) : 0, 821637
BytebuddyAgent [candidate] (827.744 ms) : 0, 827744
AgentMeter [baseline] (11.275 ms) : 0, 11275
AgentMeter [candidate] (11.271 ms) : 0, 11271
GlobalTracer [baseline] (237.53 ms) : 0, 237530
GlobalTracer [candidate] (237.381 ms) : 0, 237381
AppSec [baseline] (30.496 ms) : 0, 30496
AppSec [candidate] (30.574 ms) : 0, 30574
Debugger [baseline] (62.45 ms) : 0, 62450
Debugger [candidate] (62.557 ms) : 0, 62557
Remote Config [baseline] (534.095 µs) : 0, 534
Remote Config [candidate] (527.302 µs) : 0, 527
Telemetry [baseline] (7.935 ms) : 0, 7935
Telemetry [candidate] (7.926 ms) : 0, 7926
Flare Poller [baseline] (3.345 ms) : 0, 3345
Flare Poller [candidate] (3.344 ms) : 0, 3344
IAST [baseline] (28.955 ms) : 0, 28955
IAST [candidate] (29.225 ms) : 0, 29225
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.071 s) : 0, 1070587
Total [baseline] (11.074 s) : 0, 11073615
Agent [candidate] (1.064 s) : 0, 1064355
Total [candidate] (11.111 s) : 0, 11111029
section appsec
Agent [baseline] (1.279 s) : 0, 1279411
Total [baseline] (11.19 s) : 0, 11189722
Agent [candidate] (1.269 s) : 0, 1268854
Total [candidate] (11.152 s) : 0, 11152139
section iast
Agent [baseline] (1.247 s) : 0, 1247351
Total [baseline] (11.263 s) : 0, 11263068
Agent [candidate] (1.243 s) : 0, 1243260
Total [candidate] (11.211 s) : 0, 11210656
section profiling
Agent [baseline] (1.19 s) : 0, 1190027
Total [baseline] (10.992 s) : 0, 10991747
Agent [candidate] (1.204 s) : 0, 1203760
Total [candidate] (11.022 s) : 0, 11021941
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.228 ms) : 0, 1228
crashtracking [candidate] (1.21 ms) : 0, 1210
BytebuddyAgent [baseline] (638.614 ms) : 0, 638614
BytebuddyAgent [candidate] (635.624 ms) : 0, 635624
AgentMeter [baseline] (29.714 ms) : 0, 29714
AgentMeter [candidate] (29.607 ms) : 0, 29607
GlobalTracer [baseline] (250.617 ms) : 0, 250617
GlobalTracer [candidate] (249.936 ms) : 0, 249936
AppSec [baseline] (33.089 ms) : 0, 33089
AppSec [candidate] (32.889 ms) : 0, 32889
Debugger [baseline] (60.753 ms) : 0, 60753
Debugger [candidate] (60.927 ms) : 0, 60927
Remote Config [baseline] (601.425 µs) : 0, 601
Remote Config [candidate] (609.0 µs) : 0, 609
Telemetry [baseline] (10.749 ms) : 0, 10749
Telemetry [candidate] (9.183 ms) : 0, 9183
Flare Poller [baseline] (9.127 ms) : 0, 9127
Flare Poller [candidate] (8.438 ms) : 0, 8438
section appsec
crashtracking [baseline] (1.24 ms) : 0, 1240
crashtracking [candidate] (1.223 ms) : 0, 1223
BytebuddyAgent [baseline] (686.073 ms) : 0, 686073
BytebuddyAgent [candidate] (674.611 ms) : 0, 674611
AgentMeter [baseline] (12.438 ms) : 0, 12438
AgentMeter [candidate] (12.237 ms) : 0, 12237
GlobalTracer [baseline] (251.247 ms) : 0, 251247
GlobalTracer [candidate] (250.792 ms) : 0, 250792
AppSec [baseline] (185.726 ms) : 0, 185726
AppSec [candidate] (186.52 ms) : 0, 186520
Debugger [baseline] (65.468 ms) : 0, 65468
Debugger [candidate] (65.283 ms) : 0, 65283
Remote Config [baseline] (589.489 µs) : 0, 589
Remote Config [candidate] (562.502 µs) : 0, 563
Telemetry [baseline] (7.988 ms) : 0, 7988
Telemetry [candidate] (7.899 ms) : 0, 7899
Flare Poller [baseline] (6.789 ms) : 0, 6789
Flare Poller [candidate] (8.145 ms) : 0, 8145
IAST [baseline] (24.874 ms) : 0, 24874
IAST [candidate] (24.951 ms) : 0, 24951
section iast
crashtracking [baseline] (1.219 ms) : 0, 1219
crashtracking [candidate] (1.246 ms) : 0, 1246
BytebuddyAgent [baseline] (825.872 ms) : 0, 825872
BytebuddyAgent [candidate] (822.708 ms) : 0, 822708
AgentMeter [baseline] (11.444 ms) : 0, 11444
AgentMeter [candidate] (11.291 ms) : 0, 11291
GlobalTracer [baseline] (237.775 ms) : 0, 237775
GlobalTracer [candidate] (237.555 ms) : 0, 237555
AppSec [baseline] (32.167 ms) : 0, 32167
AppSec [candidate] (30.661 ms) : 0, 30661
Debugger [baseline] (64.118 ms) : 0, 64118
Debugger [candidate] (63.798 ms) : 0, 63798
Remote Config [baseline] (532.535 µs) : 0, 533
Remote Config [candidate] (522.196 µs) : 0, 522
Telemetry [baseline] (7.961 ms) : 0, 7961
Telemetry [candidate] (7.909 ms) : 0, 7909
Flare Poller [baseline] (3.455 ms) : 0, 3455
Flare Poller [candidate] (3.424 ms) : 0, 3424
IAST [baseline] (26.625 ms) : 0, 26625
IAST [candidate] (28.15 ms) : 0, 28150
section profiling
crashtracking [baseline] (1.179 ms) : 0, 1179
crashtracking [candidate] (1.208 ms) : 0, 1208
BytebuddyAgent [baseline] (694.486 ms) : 0, 694486
BytebuddyAgent [candidate] (704.139 ms) : 0, 704139
AgentMeter [baseline] (8.96 ms) : 0, 8960
AgentMeter [candidate] (9.095 ms) : 0, 9095
GlobalTracer [baseline] (208.714 ms) : 0, 208714
GlobalTracer [candidate] (210.324 ms) : 0, 210324
AppSec [baseline] (32.757 ms) : 0, 32757
AppSec [candidate] (33.182 ms) : 0, 33182
Debugger [baseline] (66.122 ms) : 0, 66122
Debugger [candidate] (66.31 ms) : 0, 66310
Remote Config [baseline] (594.393 µs) : 0, 594
Remote Config [candidate] (583.26 µs) : 0, 583
Telemetry [baseline] (8.109 ms) : 0, 8109
Telemetry [candidate] (8.177 ms) : 0, 8177
Flare Poller [baseline] (3.533 ms) : 0, 3533
Flare Poller [candidate] (3.671 ms) : 0, 3671
ProfilingAgent [baseline] (93.988 ms) : 0, 93988
ProfilingAgent [candidate] (94.885 ms) : 0, 94885
Profiling [baseline] (94.544 ms) : 0, 94544
Profiling [candidate] (95.44 ms) : 0, 95440
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 21 metrics, 15 unstable metrics. Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (19.55 ms) : 19353, 19746
. : milestone, 19550,
appsec (18.735 ms) : 18546, 18924
. : milestone, 18735,
code_origins (17.975 ms) : 17798, 18152
. : milestone, 17975,
iast (17.978 ms) : 17798, 18157
. : milestone, 17978,
profiling (18.788 ms) : 18601, 18975
. : milestone, 18788,
tracing (17.933 ms) : 17754, 18111
. : milestone, 17933,
section candidate
no_agent (19.372 ms) : 19176, 19569
. : milestone, 19372,
appsec (18.68 ms) : 18492, 18868
. : milestone, 18680,
code_origins (17.776 ms) : 17599, 17952
. : milestone, 17776,
iast (17.887 ms) : 17709, 18064
. : milestone, 17887,
profiling (18.438 ms) : 18255, 18622
. : milestone, 18438,
tracing (17.694 ms) : 17519, 17869
. : milestone, 17694,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (1.27 ms) : 1258, 1282
. : milestone, 1270,
iast (3.318 ms) : 3272, 3364
. : milestone, 3318,
iast_FULL (6.068 ms) : 6006, 6130
. : milestone, 6068,
iast_GLOBAL (3.689 ms) : 3627, 3750
. : milestone, 3689,
profiling (2.267 ms) : 2246, 2289
. : milestone, 2267,
tracing (1.953 ms) : 1936, 1969
. : milestone, 1953,
section candidate
no_agent (1.237 ms) : 1225, 1249
. : milestone, 1237,
iast (3.374 ms) : 3328, 3420
. : milestone, 3374,
iast_FULL (5.983 ms) : 5922, 6045
. : milestone, 5983,
iast_GLOBAL (3.686 ms) : 3625, 3747
. : milestone, 3686,
profiling (2.152 ms) : 2133, 2172
. : milestone, 2152,
tracing (1.932 ms) : 1916, 1948
. : milestone, 1932,
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~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (14.932 s) : 14932000, 14932000
. : milestone, 14932000,
appsec (14.726 s) : 14726000, 14726000
. : milestone, 14726000,
iast (18.183 s) : 18183000, 18183000
. : milestone, 18183000,
iast_GLOBAL (18.028 s) : 18028000, 18028000
. : milestone, 18028000,
profiling (15.033 s) : 15033000, 15033000
. : milestone, 15033000,
tracing (14.76 s) : 14760000, 14760000
. : milestone, 14760000,
section candidate
no_agent (15.457 s) : 15457000, 15457000
. : milestone, 15457000,
appsec (14.523 s) : 14523000, 14523000
. : milestone, 14523000,
iast (19.089 s) : 19089000, 19089000
. : milestone, 19089000,
iast_GLOBAL (17.891 s) : 17891000, 17891000
. : milestone, 17891000,
profiling (15.069 s) : 15069000, 15069000
. : milestone, 15069000,
tracing (14.763 s) : 14763000, 14763000
. : milestone, 14763000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~33f9eb1764, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (1.486 ms) : 1475, 1498
. : milestone, 1486,
appsec (3.822 ms) : 3601, 4043
. : milestone, 3822,
iast (2.284 ms) : 2214, 2355
. : milestone, 2284,
iast_GLOBAL (2.314 ms) : 2243, 2384
. : milestone, 2314,
profiling (2.111 ms) : 2056, 2167
. : milestone, 2111,
tracing (2.074 ms) : 2020, 2128
. : milestone, 2074,
section candidate
no_agent (1.482 ms) : 1471, 1494
. : milestone, 1482,
appsec (3.784 ms) : 3567, 4001
. : milestone, 3784,
iast (2.28 ms) : 2210, 2350
. : milestone, 2280,
iast_GLOBAL (2.319 ms) : 2248, 2390
. : milestone, 2319,
profiling (2.526 ms) : 2360, 2693
. : milestone, 2526,
tracing (2.072 ms) : 2018, 2126
. : milestone, 2072,
|
…entDecoder Replaces hardcoded UTF-8 (no-charset default) and ISO-8859-1 (fallback) with Charset.defaultCharset() in both cases, per reviewer feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e74778b436
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
…partContentDecoder RFC 2045 allows quoted parameter values (charset="UTF-8"). Without stripping the quotes Charset.forName rejects the name and decodeBytes falls back to the JVM default instead of the declared charset.
…ecoder String#split is forbidden (uses regex internally). Replace with an explicit char scan to find the first ; , or space after charset=.
jandro996
commented
Apr 28, 2026
MemberAuthor
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:228e1c467a
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
…oder.extractCharset Replace toLowerCase(Locale.ROOT).indexOf with an inline ASCII case-insensitive scan to avoid allocating a full lowercase copy of the Content-Type string. Also use the already-computed end variable as the loop bound.
All files in this package (StoredByteBody, StoredBodySupplier, MultipartContentDecoder, etc.) are AppSec HTTP body inspection infrastructure.
When FileItemContentReader truncates at MAX_CONTENT_BYTES a cut in the middle of a multibyte character no longer triggers the fallback path. REPLACE substitutes the incomplete sequence with U+FFFD using the declared charset; REPORT was throwing and silently switching to the JVM default charset for the whole string.
jandro996
commented
Apr 28, 2026
MemberAuthor
@codex review |
With CodingErrorAction.REPLACE the decoder never throws CharacterCodingException, making the catch branch unreachable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:22536b19ea
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Substring search could match 'xcharset=' as 'charset=', allowing a client-controlled decoy parameter to override the real charset. Now requires the match to be at position 0 or preceded by ';' or ' '.
CharsetDecoder.decode(ByteBuffer) declares throws CharacterCodingException even though CodingErrorAction.REPLACE makes it unreachable; the compiler still requires the exception to be caught or declared.
jandro996
commented
Apr 28, 2026
MemberAuthor
@codex review |
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ 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". |
jandro996
enabled auto-merge
April 28, 2026 10:27
Contributor
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Apr 28, 2026
Uh oh!
There was an error while loading. Please reload this page.
gh-worker-dd-mergequeue-cf854dBot
merged commit Apr 28, 2026
9d73760
into
master
762 of 767 checks passed
Uh oh!
There was an error while loading. Please reload this page.
gh-worker-dd-mergequeue-cf854dBot
deleted the
alejandro.gonzalez/APPSEC-61875-files-content-encoding
branch
April 28, 2026 12:33
dougqh
reviewed
Apr 28, 2026
| try { | ||
| return Charset.forName(name); | ||
| } catch (IllegalArgumentException e) { | ||
| return null; |
Contributor
There was a problem hiding this comment.
Does this risk a NullPointerException elsewhere?
Should we fallback to a default Charset instead?
MemberAuthor
There was a problem hiding this comment.
Right now it's only used in decodeBytes method, where that is doneif (charset == null) charset = Charset.defaultCharset();
Probably It makes more sense to return it where you mention, not sure if in the future it will be necessary to know exactly if we are not able to get the charset
I could change it in my next PR related with this topic if you want :)
jandro996 added a commit
that referenced
this pull request
Apr 28, 2026
Introduces MultipartContentDecoder (internal-api) to decode multipart file bytes using the charset declared in each part's Content-Type header, with JVM-default fallback and REPLACE on malformed input. Mirrors the approach in PR #11212 for commons-fileupload so all three integrations share the same decoding logic.
gh-worker-dd-mergequeue-cf854dBot
pushed a commit
that referenced
this pull request
Apr 30, 2026
…etty multipart (#11198) refactor(appsec): extract MultipartContentDecoder to internal-api for reuse across integrations test(appsec): add missing corner cases to MultipartContentDecoderTest fix(appsec): use machine default charset as fallback in MultipartContentDecoder Replaces hardcoded UTF-8 (no-charset default) and ISO-8859-1 (fallback) with Charset.defaultCharset() in both cases, per reviewer feedback. test(appsec): migrate MultipartContentDecoderTest from Groovy/Spock to Java/JUnit 5 feat(appsec): extend files_content WAF address to Tomcat and Netty multipart 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 fix(appsec): address techdebt in Tomcat/Netty files_content instrumentation 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). fix(appsec): further cleanup of Tomcat/Netty files_content instrumentation 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. fix(appsec): align blocking paths with canonical pattern in Tomcat/Netty 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. fix(appsec): skip file content I/O in Netty when requestFilesContent 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. fix(appsec): align Netty filenames blocking and merge CommonsFileUpload 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. refactor(appsec): remove dead readContents() from FileItemContentReader 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. fix(appsec): lazy-init filesContent in Netty and align CommonsFileUpload 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. fix(appsec): skip file content I/O in Tomcat when requestFilesContent 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. fix(appsec): move BlockingException inside brf guard in all remaining 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. refactor(appsec): cache reflection and use try-with-resources in ParameterCollector - 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. test(appsec): add Netty integration tests for multipart filenames and 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. refactor(appsec): extract Netty file content reading to NettyFileUploadContentReader 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. test(appsec): add integration tests for file content size and count limits test(appsec): increase Netty test server aggregator limit to support large multipart payloads fix(appsec): do not gate Netty file content callback on requestBodyProcessed subscriber style: apply spotless formatting to HttpServerTest fix(appsec): remove effectivelyBlocked() calls in Netty filenames and content blocking paths fix(appsec): set t before effectivelyBlocked() in Tomcat filenames and content blocking paths perf(appsec): skip attribute I/O in Netty multipart when body callback is absent When only requestFilesContent is registered, avoid calling getValue() on form attributes (which may trigger disk reads for large fields). refactor(appsec): unify file content limits via Config for DD_APPSEC_MAX_FILE_CONTENT_BYTES/COUNT Replace hardcoded constants MAX_CONTENT_BYTES=4096 and MAX_FILES_TO_INSPECT=25 (duplicated across commons-fileupload, Netty, and Tomcat) with two Config-backed variables: appsec.max.file-content.bytes and appsec.max.file-content.count. chore: register DD_APPSEC_MAX_FILE_CONTENT_BYTES/COUNT in supported-configurations.json test(appsec): use Config values instead of hardcoded literals in ParameterCollectorImplTest fix(appsec): move MAX_FILES_TO_INSPECT out of advice inner class to fix muzzle validation Static fields in @RequiresRequestContext advice inner classes trigger muzzle to treat the advice class itself as a user-classpath class; moving the constant to NettyFileUploadContentReader (a helper) avoids the self-referential muzzle failure. style: apply spotless formatting to HttpPostRequestDecoderInstrumentation fix(appsec): include filenames callback in Netty multipart early-return gate requestFilesFilenames() was consulted after data collection, so when it was the only registered AppSec callback (no requestBodyProcessed, no requestFilesContent) the early-return gate fired and filenames were never published to the WAF. Fetch all three callbacks upfront and gate on all three. fix(appsec): use per-part charset for files_content in Tomcat and Netty Introduces MultipartContentDecoder (internal-api) to decode multipart file bytes using the charset declared in each part's Content-Type header, with JVM-default fallback and REPLACE on malformed input. Mirrors the approach in PR #11212 for commons-fileupload so all three integrations share the same decoding logic. refactor(appsec): extract shared InputStream read loop into MultipartContentDecoder Add readInputStream(InputStream, int, String) to eliminate the identical bounded read loop duplicated across FileItemContentReader, NettyFileUploadContentReader and ParameterCollector. Each caller still owns its own MAX_CONTENT_BYTES constant. Also consolidate the two volatile reflection caches in ParameterCollector.readContent (getInputStream + getContentType) into a single Method[] entry to halve volatile reads per invocation. test(appsec): add charset-aware decoding tests for readInputStream, Netty and Tomcat helpers test(appsec): add getContentType() to Tomcat test part doubles to fix reflective method resolution perf(appsec): skip filenames list allocation in CommonsFileUpload when filenames callback is absent refactor(appsec): extract Netty multipart data collection to NettyMultipartHelper and add unit tests test(appsec): declare ISO-8859-1 content type on RawBytesPart to match decoder charset expectation refactor(appsec): extract tryBlock helper to NettyMultipartHelper Deduplicates the identical RequestBlockingAction dispatch pattern that appeared three times in the Netty multipart advice. test(appsec): add unit tests for NettyMultipartHelper.tryBlock refactor(appsec): move MAX_FILES_TO_INSPECT to FileItemContentReader helper with addToContents() The constant now lives in the helper class (listed in helperClassNames()) where it is actually used, instead of the outer instrumentation class which is not visible from inlined advice bytecode in the app classloader. addToContents() encapsulates the limit check alongside the constant. refactor(appsec): replace Map.Entry method cache with four flat volatile fields in ParameterCollector Use separate volatile fields (cachedPartClass, cachedGetInputStream, cachedGetContentType, cachedGetFilename) instead of Map.Entry<Class<?>, Method[]> + Map.Entry<Class<?>, Method>. cachedPartClass is written last to safely publish the three methods per JMM volatile ordering. test(appsec): add unit tests for FileItemContentReader.addToContents() limit behaviour fix(appsec): preserve decoder exception when body callback does not block in Netty multipart Unconditionally assigning thr = tryBlock(...) would null out an original decoder exception (e.g. malformed multipart on an over-limit body) when tryBlock returns null for a non-blocking flow. Only overwrite thr when tryBlock returns a blocking exception. fix(appsec): make Tomcat Part method cache atomic with an immutable holder Four flat volatile fields have a TOCTOU race: thread A passes the cachedPartClass check, thread B overwrites the three method fields for a different class, and thread A invokes the wrong Method on its instance (IllegalArgumentException is swallowed, the part is silently dropped). Replace the four fields with a single volatile CachedMethods reference that holds an immutable snapshot of all three methods. Readers load the reference once into a local variable, so their snapshot stays consistent even if another thread publishes a new entry concurrently. Add a concurrency regression test that runs two threads simultaneously with different concrete Part classes; it detects dropped parts caused by the race. fix(appsec): inject CachedMethods helper class in Tomcat ParsePartsInstrumentation ByteBuddy only injects classes explicitly listed in helperClassNames(); nested inner classes are not auto-discovered. Without this entry, the first addPart() call triggers NoClassDefFoundError on CachedMethods, which is swallowed by the broad catch in addPart(), silently dropping all filenames and file contents. Co-authored-by: alejandro.gonzalez <alejandro.gonzalez@datadoghq.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
MultipartContentDecoderininternal-api/src/main/java/datadog/trace/api/http/— a shared utility that decodes multipart file content bytes using the charset declared in each part'sContent-TypeheaderFileItemContentReader.readContent()now delegates toMultipartContentDecoder.decodeBytes(buf, total, fileItem.getContentType())instead of hardcoding ISO-8859-1MultipartContentDecoder.extractCharset()is case-insensitive, handles RFC 2045 quoted values (charset="UTF-8"), and guards against parameter name substring false positives (xcharset=…no longer matches ascharset=)CodingErrorAction.REPLACEis used so that truncation at a multibyte character boundary (from theMAX_CONTENT_BYTEScap) produces U+FFFD for the incomplete sequence rather than falling back to the JVM default charset for the entire stringinternal-api/src/main/java/datadog/trace/api/http/to@DataDog/asm-javaMotivation
FileItemContentReaderwas decoding all uploaded file content with hardcoded ISO-8859-1. Files uploaded as UTF-8 or another charset arrived at the WAF with garbled non-ASCII characters, preventing detection on any content outside the ASCII range.Additional Notes
Charset fallback diverges from
StoredByteBody: when no charset is declared in the part'sContent-Type,MultipartContentDecoderfalls back toCharset.defaultCharset()(the JVM default) rather than hardcoding UTF-8 → ISO-8859-1 asStoredByteBodydoes. This was deliberately chosen after discussion with Manu — the JVM default is a safer assumption for a multi-framework utility than an opinionated constant.catch (CharacterCodingException e)is unreachable at runtime:CharsetDecoder.decode(ByteBuffer)declaresthrows CharacterCodingExceptionas a checked exception so the compiler requires the catch, butCodingErrorAction.REPLACEensures the exception is never actually thrown. The catch block rethrows asIllegalStateExceptionto make the intent clear.MultipartContentDecoderis designed for reuse: the same class is used in PR #11198 (Tomcat and Netty integrations) so the charset parsing logic lives in one place.Jira ticket: APPSEC-61875