Add server.request.body.filenames support for Jetty#10988
Open
Add server.request.body.filenames support for Jetty#10988
Conversation
jandro996
added
comp: asm waf
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 petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.056 s) : 0, 1055506
Total [baseline] (11.116 s) : 0, 11115994
Agent [candidate] (1.072 s) : 0, 1071993
Total [candidate] (11.17 s) : 0, 11170313
section appsec
Agent [baseline] (1.267 s) : 0, 1266927
Total [baseline] (11.071 s) : 0, 11070808
Agent [candidate] (1.266 s) : 0, 1266035
Total [candidate] (4.42 s) : 0, 4419518
section iast
Agent [baseline] (1.24 s) : 0, 1240466
Total [baseline] (11.32 s) : 0, 11319829
Agent [candidate] (1.242 s) : 0, 1241627
Total [candidate] (11.367 s) : 0, 11367170
section profiling
Agent [baseline] (1.185 s) : 0, 1184880
Total [baseline] (11.027 s) : 0, 11026589
Agent [candidate] (1.186 s) : 0, 1185729
Total [candidate] (11.101 s) : 0, 11100718
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.215 ms) : 0, 1215
crashtracking [candidate] (1.234 ms) : 0, 1234
BytebuddyAgent [baseline] (634.844 ms) : 0, 634844
BytebuddyAgent [candidate] (644.432 ms) : 0, 644432
AgentMeter [baseline] (29.254 ms) : 0, 29254
AgentMeter [candidate] (29.594 ms) : 0, 29594
GlobalTracer [baseline] (248.682 ms) : 0, 248682
GlobalTracer [candidate] (250.77 ms) : 0, 250770
AppSec [baseline] (32.244 ms) : 0, 32244
AppSec [candidate] (32.631 ms) : 0, 32631
Debugger [baseline] (60.239 ms) : 0, 60239
Debugger [candidate] (61.406 ms) : 0, 61406
Remote Config [baseline] (594.898 µs) : 0, 595
Remote Config [candidate] (605.481 µs) : 0, 605
Telemetry [baseline] (8.029 ms) : 0, 8029
Telemetry [candidate] (8.211 ms) : 0, 8211
Flare Poller [baseline] (4.285 ms) : 0, 4285
Flare Poller [candidate] (6.632 ms) : 0, 6632
section appsec
crashtracking [baseline] (1.224 ms) : 0, 1224
crashtracking [candidate] (1.23 ms) : 0, 1230
BytebuddyAgent [baseline] (679.205 ms) : 0, 679205
BytebuddyAgent [candidate] (678.221 ms) : 0, 678221
AgentMeter [baseline] (12.31 ms) : 0, 12310
AgentMeter [candidate] (12.229 ms) : 0, 12229
GlobalTracer [baseline] (249.226 ms) : 0, 249226
GlobalTracer [candidate] (249.444 ms) : 0, 249444
IAST [baseline] (24.193 ms) : 0, 24193
IAST [candidate] (24.351 ms) : 0, 24351
AppSec [baseline] (188.03 ms) : 0, 188030
AppSec [candidate] (186.166 ms) : 0, 186166
Debugger [baseline] (64.373 ms) : 0, 64373
Debugger [candidate] (66.099 ms) : 0, 66099
Remote Config [baseline] (590.224 µs) : 0, 590
Remote Config [candidate] (557.79 µs) : 0, 558
Telemetry [baseline] (7.801 ms) : 0, 7801
Telemetry [candidate] (7.837 ms) : 0, 7837
Flare Poller [baseline] (3.423 ms) : 0, 3423
Flare Poller [candidate] (3.443 ms) : 0, 3443
section iast
crashtracking [baseline] (1.224 ms) : 0, 1224
crashtracking [candidate] (1.228 ms) : 0, 1228
BytebuddyAgent [baseline] (816.329 ms) : 0, 816329
BytebuddyAgent [candidate] (815.985 ms) : 0, 815985
AgentMeter [baseline] (11.421 ms) : 0, 11421
AgentMeter [candidate] (11.475 ms) : 0, 11475
GlobalTracer [baseline] (239.919 ms) : 0, 239919
GlobalTracer [candidate] (240.259 ms) : 0, 240259
IAST [baseline] (28.386 ms) : 0, 28386
IAST [candidate] (29.515 ms) : 0, 29515
AppSec [baseline] (28.71 ms) : 0, 28710
AppSec [candidate] (27.777 ms) : 0, 27777
Debugger [baseline] (66.47 ms) : 0, 66470
Debugger [candidate] (67.147 ms) : 0, 67147
Remote Config [baseline] (539.682 µs) : 0, 540
Remote Config [candidate] (546.423 µs) : 0, 546
Telemetry [baseline] (7.836 ms) : 0, 7836
Telemetry [candidate] (7.834 ms) : 0, 7834
Flare Poller [baseline] (3.528 ms) : 0, 3528
Flare Poller [candidate] (3.492 ms) : 0, 3492
section profiling
crashtracking [baseline] (1.181 ms) : 0, 1181
crashtracking [candidate] (1.189 ms) : 0, 1189
BytebuddyAgent [baseline] (691.48 ms) : 0, 691480
BytebuddyAgent [candidate] (693.421 ms) : 0, 693421
AgentMeter [baseline] (8.972 ms) : 0, 8972
AgentMeter [candidate] (8.978 ms) : 0, 8978
GlobalTracer [baseline] (207.707 ms) : 0, 207707
GlobalTracer [candidate] (207.521 ms) : 0, 207521
AppSec [baseline] (32.558 ms) : 0, 32558
AppSec [candidate] (32.668 ms) : 0, 32668
Debugger [baseline] (65.56 ms) : 0, 65560
Debugger [candidate] (65.124 ms) : 0, 65124
Remote Config [baseline] (566.631 µs) : 0, 567
Remote Config [candidate] (575.478 µs) : 0, 575
Telemetry [baseline] (7.807 ms) : 0, 7807
Telemetry [candidate] (7.762 ms) : 0, 7762
Flare Poller [baseline] (3.464 ms) : 0, 3464
Flare Poller [candidate] (3.531 ms) : 0, 3531
ProfilingAgent [baseline] (94.019 ms) : 0, 94019
ProfilingAgent [candidate] (93.386 ms) : 0, 93386
Profiling [baseline] (94.586 ms) : 0, 94586
Profiling [candidate] (93.943 ms) : 0, 93943
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.056 s) : 0, 1056224
Total [baseline] (8.852 s) : 0, 8852151
Agent [candidate] (1.062 s) : 0, 1062237
Total [candidate] (8.883 s) : 0, 8882842
section iast
Agent [baseline] (1.232 s) : 0, 1231982
Total [baseline] (9.579 s) : 0, 9578795
Agent [candidate] (1.234 s) : 0, 1234108
Total [candidate] (9.536 s) : 0, 9535941
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.227 ms) : 0, 1227
crashtracking [candidate] (1.229 ms) : 0, 1229
BytebuddyAgent [baseline] (636.35 ms) : 0, 636350
BytebuddyAgent [candidate] (640.171 ms) : 0, 640171
AgentMeter [baseline] (29.359 ms) : 0, 29359
AgentMeter [candidate] (29.617 ms) : 0, 29617
GlobalTracer [baseline] (248.832 ms) : 0, 248832
GlobalTracer [candidate] (249.153 ms) : 0, 249153
AppSec [baseline] (32.199 ms) : 0, 32199
AppSec [candidate] (32.296 ms) : 0, 32296
Debugger [baseline] (59.243 ms) : 0, 59243
Debugger [candidate] (59.668 ms) : 0, 59668
Remote Config [baseline] (590.726 µs) : 0, 591
Remote Config [candidate] (595.392 µs) : 0, 595
Telemetry [baseline] (8.742 ms) : 0, 8742
Telemetry [candidate] (8.809 ms) : 0, 8809
Flare Poller [baseline] (3.508 ms) : 0, 3508
Flare Poller [candidate] (4.374 ms) : 0, 4374
section iast
crashtracking [baseline] (1.216 ms) : 0, 1216
crashtracking [candidate] (1.235 ms) : 0, 1235
BytebuddyAgent [baseline] (810.616 ms) : 0, 810616
BytebuddyAgent [candidate] (813.175 ms) : 0, 813175
AgentMeter [baseline] (11.395 ms) : 0, 11395
AgentMeter [candidate] (11.366 ms) : 0, 11366
GlobalTracer [baseline] (239.1 ms) : 0, 239100
GlobalTracer [candidate] (239.429 ms) : 0, 239429
AppSec [baseline] (28.413 ms) : 0, 28413
AppSec [candidate] (26.169 ms) : 0, 26169
Debugger [baseline] (62.88 ms) : 0, 62880
Debugger [candidate] (62.853 ms) : 0, 62853
Remote Config [baseline] (520.83 µs) : 0, 521
Remote Config [candidate] (523.942 µs) : 0, 524
Telemetry [baseline] (7.785 ms) : 0, 7785
Telemetry [candidate] (7.601 ms) : 0, 7601
Flare Poller [baseline] (3.401 ms) : 0, 3401
Flare Poller [candidate] (3.365 ms) : 0, 3365
IAST [baseline] (30.597 ms) : 0, 30597
IAST [candidate] (32.393 ms) : 0, 32393
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 1 performance regressions! Performance is the same for 17 metrics, 16 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section baseline
no_agent (1.25 ms) : 1237, 1262
. : milestone, 1250,
iast (3.396 ms) : 3346, 3447
. : milestone, 3396,
iast_FULL (6.002 ms) : 5940, 6064
. : milestone, 6002,
iast_GLOBAL (3.66 ms) : 3604, 3716
. : milestone, 3660,
profiling (2.193 ms) : 2173, 2212
. : milestone, 2193,
tracing (1.915 ms) : 1898, 1931
. : milestone, 1915,
section candidate
no_agent (1.237 ms) : 1226, 1248
. : milestone, 1237,
iast (3.409 ms) : 3361, 3456
. : milestone, 3409,
iast_FULL (6.084 ms) : 6020, 6147
. : milestone, 6084,
iast_GLOBAL (3.744 ms) : 3684, 3805
. : milestone, 3744,
profiling (2.321 ms) : 2298, 2344
. : milestone, 2321,
tracing (1.901 ms) : 1885, 1917
. : milestone, 1901,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section baseline
no_agent (17.55 ms) : 17377, 17724
. : milestone, 17550,
appsec (18.517 ms) : 18335, 18698
. : milestone, 18517,
code_origins (18.749 ms) : 18564, 18935
. : milestone, 18749,
iast (18.96 ms) : 18769, 19150
. : milestone, 18960,
profiling (18.255 ms) : 18072, 18438
. : milestone, 18255,
tracing (17.881 ms) : 17703, 18059
. : milestone, 17881,
section candidate
no_agent (19.135 ms) : 18941, 19328
. : milestone, 19135,
appsec (18.806 ms) : 18616, 18996
. : milestone, 18806,
code_origins (18.064 ms) : 17883, 18245
. : milestone, 18064,
iast (17.708 ms) : 17535, 17881
. : milestone, 17708,
profiling (18.324 ms) : 18139, 18508
. : milestone, 18324,
tracing (17.821 ms) : 17644, 17998
. : milestone, 17821,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section baseline
no_agent (14.797 s) : 14797000, 14797000
. : milestone, 14797000,
appsec (15.151 s) : 15151000, 15151000
. : milestone, 15151000,
iast (18.664 s) : 18664000, 18664000
. : milestone, 18664000,
iast_GLOBAL (17.644 s) : 17644000, 17644000
. : milestone, 17644000,
profiling (14.898 s) : 14898000, 14898000
. : milestone, 14898000,
tracing (14.875 s) : 14875000, 14875000
. : milestone, 14875000,
section candidate
no_agent (15.1 s) : 15100000, 15100000
. : milestone, 15100000,
appsec (14.982 s) : 14982000, 14982000
. : milestone, 14982000,
iast (18.171 s) : 18171000, 18171000
. : milestone, 18171000,
iast_GLOBAL (18.099 s) : 18099000, 18099000
. : milestone, 18099000,
profiling (15.459 s) : 15459000, 15459000
. : milestone, 15459000,
tracing (14.992 s) : 14992000, 14992000
. : milestone, 14992000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~24e48491cd, baseline=1.62.0-SNAPSHOT~0d1c44d515b
dateFormat X
axisFormat %s
section baseline
no_agent (1.489 ms) : 1477, 1501
. : milestone, 1489,
appsec (3.845 ms) : 3622, 4069
. : milestone, 3845,
iast (2.271 ms) : 2201, 2340
. : milestone, 2271,
iast_GLOBAL (2.324 ms) : 2254, 2394
. : milestone, 2324,
profiling (2.112 ms) : 2056, 2167
. : milestone, 2112,
tracing (2.08 ms) : 2026, 2133
. : milestone, 2080,
section candidate
no_agent (1.485 ms) : 1473, 1496
. : milestone, 1485,
appsec (3.825 ms) : 3602, 4049
. : milestone, 3825,
iast (2.285 ms) : 2216, 2355
. : milestone, 2285,
iast_GLOBAL (2.314 ms) : 2244, 2384
. : milestone, 2314,
profiling (2.097 ms) : 2042, 2152
. : milestone, 2097,
tracing (2.081 ms) : 2027, 2134
. : milestone, 2081,
|
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
2 times, most recently
from
f2998c3 to
629f074
Compare
| } | ||
| } | ||
| // Fallback: parse filename from Content-Disposition header (Servlet 3.0) | ||
| if (name == null) { |
There was a problem hiding this comment.
Shouldn't this be outside of the main parts loop?
Member
Author
There was a problem hiding this comment.
Fixed. Restructured into two separate loops chosen once before iteration: if getSubmittedFileName != null (Servlet 3.1+) iterate using that method; otherwise iterate parsing the Content-Disposition header (Servlet 3.0 fallback). No per-part branching inside the loop.
| transformer.applyAdvice( | ||
| named("extractContentParameters").and(takesArguments(0)).or(named("getParts")), | ||
| getClass().getName() + "$ExtractContentParametersAdvice"); | ||
| transformer.applyAdvice(named("getParts"), getClass().getName() + "$GetFilenamesAdvice"); |
Member
Author
There was a problem hiding this comment.
Fixed. GetFilenamesAdvice now has a call-depth guard (CallDepthThreadLocalMap with Collection.class) to avoid double-firing when getParts() internally calls getParts(MultiMap)
![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: c732823549
ℹ️ 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".
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
4 times, most recently
from
3ab9ff7 to
77ec572
Compare
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
2e72584 to
d37e03e
Compare
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
d37e03e to
d8a92f8
Compare
jandro996
added a commit
to DataDog/system-tests
that referenced
this pull request
Apr 9, 2026
Jetty multipart file upload instrumentation fixed in DataDog/dd-trace-java#10988 (Jetty 8.1.3, 9.3–11).
Contributor
|
Hi! 👋 Looks like you updated a Git Submodule.
|
3 similar comments
Contributor
|
Hi! 👋 Looks like you updated a Git Submodule.
|
Contributor
|
Hi! 👋 Looks like you updated a Git Submodule.
|
Contributor
|
Hi! 👋 Looks like you updated a Git Submodule.
|
jetty-appsec-8.1.3 (covers Jetty <9) and jetty-appsec-9.2 (covers Jetty [9.2,9.3)) were reverted to master state: they report requestBodyProcessed but not requestFilesFilenames. Jetty 9.0.x and 9.2.x therefore have no active appsec module that fires the filenames event, so testBodyFilenames() must stay false for those modules.
…jetty-appsec-9.3/9.4/11.0 In Armeria + Jetty 9.4.48 (and possibly other embedded/wrapped setups), getParts() is the application entry point for multipart parsing — it internally calls extractContentParameters(), which sets _contentParameters. Without the matcher on getParts(), ExtractContentParametersAdvice.after() never fires in that code path, so the requestBodyProcessed event (and request.body.converted tag) is never reported. The call-depth guard (Request.class key) already prevents double-firing when both methods are instrumented: getParts() increments depth to 1 before extractContentParameters() is entered, so the nested advice is a no-op.
…ty-appsec-9.4 Replace the JAVAX_PART_REFERENCE muzzle check (class presence on classpath) with a field-level bytecode check on Request._dispatcherType descriptor: - Ljavax/servlet/DispatcherType; in Jetty 9.4/10 (javax namespace) - Ljakarta/servlet/DispatcherType; in Jetty 11+ (jakarta namespace) This reliably excludes Jetty 11+ even when both javax and jakarta servlet jars are on the test classpath simultaneously. Also remove assertInverse from 9_series muzzle block to avoid conflict with 10_series (both cover overlapping Jetty 10.0.x versions). Fixes armeria-jetty-1.24:jetty11Test ClassCastException where jetty94.MultipartHelper tried to cast jakarta.servlet.http.Part to javax.servlet.http.Part.
…TP 500 jetty-appsec-8.1.3 muzzle range [8.1.3, 9.2.0.RC0) includes Jetty 9.0.x. When applied, it calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x → HTTP 500 on multipart requests. Override multipart() with assumeTrue(false) in jetty-server-9.0 and jetty-server-9.0.4 test classes until the muzzle range is corrected.
…8.1.3 causes HTTP 500 Replace assumeTrue(false) with Spock's @ignore annotation — more reliable for overriding void feature methods without Spock block labels. jetty-appsec-8.1.3 muzzle range [8.1.3, 9.2.0.RC0) includes Jetty 9.0.x. When applied, it calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x → HTTP 500 on multipart requests.
Spock AST-transforms feature methods, so @OverRide on a feature method override triggers a compile error. Use a boolean hook instead: - AppSecInactiveHttpServerTest.supportsMultipart() returns true by default and is guarded with assumeTrue() at the setup: block level - Jetty9InactiveAppSecTest overrides to false in both jetty-server-9.0 and jetty-server-9.0.4, where jetty-appsec-8.1.3 range [8.1.3, 9.2.0.RC0) causes ParameterCollector.put(String, String) to fail → HTTP 500. Verified locally: tests=8, skipped=3 (multipart correctly skipped), failures=0.
…t version to 10.0.10 - jetty-server-9.0 and jetty-server-9.0.4: set testBodyMultipart() = false. jetty-appsec-8.1.3 covers [8.1.3, 9.2.0.RC0) which includes Jetty 9.0.x; its extractContentParameters() calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x, causing HTTP 500 on multipart requests. - jetty-server-10.0: bump test dependency from 10.0.0 to 10.0.10. jetty-appsec-9.4 requires _multiParts: Lorg/eclipse/jetty/server/MultiParts; but that type was only introduced in 10.0.10 (previously MultiPartFormInputStream), so the muzzle check fails for Jetty 10.0.0-10.0.9, leaving tests uninstrumented. - Revert accidental modification of agent-jmxfetch/integrations-core submodule back to its master pointer.
…ith exit-advice Replace the brittle GetPartsMethodVisitor ASM bytecode injection (intercepting MultiMap.add() calls inside getParts()) with a clean ByteBuddy exit-advice approach: - Remove ParameterCollector, GetPartsAdvice, GetPartsVisitorWrapper, RequestClassVisitor, and GetPartsMethodVisitor; drop HasTypeAdvice from the instrumentation module. - Add PartHelper with extractFormFields() (reads InputStream per part) and extractFilenames() (parses Content-Disposition manually, since Part.getSubmittedFileName() is Servlet 3.1+ and not available in Jetty 8.x). - Add GetFilenamesAdvice (@RequiresRequestContext / @ActiveRequestContext) on getParts():Collection that fires requestBodyProcessed() with form fields and requestFilesFilenames() with upload filenames. Uses CallDepthThreadLocalMap to guard against reentrant calls. - Jetty8LatestDepForkedTest: set testBodyFilenamesCalledOnce() and testBodyFilenamesCalledOnceCombined() to false because Jetty 8.x has no _multiParts / _contentParameters field guards to suppress duplicate firings.
…utStream - jetty-appsec-9.4: extend muzzle to also cover Jetty 10.0.0–10.0.9, which were previously a gap. In those versions _multiParts is typed MultiPartFormInputStream (not MultiParts); the primary Reference spec matches 9.4.10+ and 10.0.10+, and an OrReference alternative matches 10.0.0–10.0.9. The GetFilenamesAdvice already uses typing=DYNAMIC so no advice changes are needed. - jetty-appsec-8.1.3 PartHelper: wrap part.getInputStream() in try-with-resources to avoid leaking file descriptors on file-backed multipart form fields.
Splitting the header on ';' naively truncated filenames that contain semicolons inside a quoted value, e.g. filename="shell;evil.php" would produce "shell" instead of the full name. Replace the split() loop with a quote-aware state-machine parser that skips semicolons inside quoted strings and handles backslash-escaped characters. Add test cases for semicolons in filenames, escaped quotes, and filename appearing before other parameters.
…c-11.0 Replace the JAKARTA_PART_REFERENCE classpath check with a _dispatcherType field descriptor check on Request.class bytecode, mirroring the approach already used by jetty-appsec-9.4. The classpath check passes on any Jetty 9.4/10 app that has jakarta.servlet-api as a dependency, causing double-instrumentation of extractContentParameters. The bytecode check is authoritative: in Jetty 11+ Request.class carries _dispatcherType as Ljakarta/servlet/DispatcherType;, while 9.4/10 carry the javax descriptor.
…uploads In Jetty 8.x, getPart(String name) calls _multiPartInputStream.getPart(String) directly without delegating to getParts(). Applications that retrieve only one file via getPart() without ever calling getParts() would have their filename event missed. Add GetPartAdvice to cover this path. The charset fix (AI comment 2) was investigated and is not applicable: HTML5 form submissions always use UTF-8 and browsers never include charset= on individual part Content-Type headers, so the existing hardcoded UTF-8 is correct.
…ilename tests in async suite 1. Add _multiPartInputStream == null guard to GetFilenamesAdvice.before() so that repeated getParts() calls on the same request (which Jetty caches) do not re-fire requestFilesFilenames/requestBodyProcessed WAF callbacks. The field is null before the first multipart parse and non-null on all subsequent cached calls, matching the pattern used in the 9.4/11.0 advice (_multiParts guard). 2. JettyAsyncHandlerTest already disabled testBodyFilenames() but neglected to disable testBodyFilenamesCalledOnce() and testBodyFilenamesCalledOnceCombined(), which are now enabled in the Jetty11Test parent. Override both to false in the async handler suite to prevent spurious test failures.
…elper filenameFromPart() was returning null for both 'no filename parameter' and 'filename=""', causing extractFormFields() to buffer the full body of file inputs submitted with no file chosen (filename=""). An empty <input type=file> is still a file part, not a form field. Return "" instead of null so that callers using != null correctly skip those parts without reading their content. Update tests to assert "" for empty-filename cases and add regression tests for extractFormFields/extractFilenames with empty-filename parts. Note: the second AI comment about getPart(String) double-firing was not implemented. The bytecode shows the internal call is to MultiPartInputStream.getParts() (not Request.getParts()), so GetFilenamesAdvice (which instruments Request.getParts()) is never triggered during a getPart() call. There is no double-firing.
The jetty-appsec-9.4 early_10_series muzzle pass covers [10.0.0, 10.0.10) where _multiParts is MultiPartFormInputStream (before the type changed to MultiParts in 10.0.10). The existing test suite used 10.0.10, leaving the early versions untested. Add earlyDep10ForkedTest (Jetty 10.0.9) to exercise the OR-muzzle reference and verify that multipart filename events fire correctly on the old field type. All three filename tests pass: filenames, filenames-called-once, filenames-combined.
…M API constant Move duplicated IG-callback + blocking-commit blocks out of advice into helper methods (MultipartHelper.fireFilenamesEvent / PartHelper.fireBodyProcessedEvent + fireFilenamesEvent) that return BlockingException|null, exploiting @Advice.Thrown(readOnly=false) assignment behaviour. Also replace hardcoded Opcodes.ASM9 with OpenedClassReader.ASM_API in RequestGetPartsInstrumentation and fix extractFormFields to use computeIfAbsent instead of get+put.
…() delegates to getParts() In Jetty 9.0/9.1 (covered by jetty-appsec-8.1.3's [8.1.3,9.2.0.RC0) range), Request.getPart(String) delegates to getParts() internally. Without this fix, GetFilenamesAdvice fires for the full collection (via the internal getParts() call) and then GetPartAdvice fires again for the returned singleton, because the two advice classes use independent call-depth keys and cannot see each other. Fix: peek at the Part.class call depth in GetFilenamesAdvice.before(). If > 1, GetPartAdvice is active on the stack and will handle the event; skip to avoid double-firing. In Jetty 8.x (where getPart() does not delegate), the peek is always 1, so GetFilenamesAdvice continues to fire normally.
…== 1 CallDepthThreadLocalMap.incrementCallDepth() uses post-increment (depth++) so it returns the depth BEFORE the increment. In the normal case (no GetPartAdvice active), Part.class depth is 0 before the peek, making partPeek == 0. The previous guard checked partPeek == 1 which is true only when GetPartAdvice IS already on the stack — exactly backwards. Result: GetFilenamesAdvice never fired for direct getParts() calls, breaking the filenames event on Jetty 8.x. Fix: use partPeek == 0 (proceed when GetPartAdvice is not active) so the condition is: - partPeek == 0: normal direct getParts() → proceed (fire filenames event) - partPeek == 1: getParts() called by getPart() in Jetty 9.0/9.1 → skip (GetPartAdvice handles it)
…st the singleton
When getPart("field") is the app's first multipart access, Jetty 8 parses the entire
multipart stream and caches all parts in _multiPartInputStream, but only returns the
one requested part. The previous advice forwarded just that singleton to AppSec, so any
co-uploaded file parts were invisible to requestFilesFilenames — a WAF bypass if the
app never called getParts() explicitly.
Fix: read all cached parts via MultiPartInputStream.getParts() (reflected, cached handle)
and fall back to the singleton only when reflection fails. Also remove the part==null
early return: even if the requested field was not found, other file parts may have parsed.
Add PartHelper.getAllParts(Object, Part) + FakeMpi unit tests.
The Part.class depth check only prevents re-entry within a single getPart() invocation;
after the call returns the depth is 0 again, so a second getPart("file") call on the
same request would re-fire requestBodyProcessed and requestFilesFilenames with the same
cached parts. Add the same _multiPartInputStream == null guard that GetFilenamesAdvice
already uses: once the field is set the multipart body was parsed and events were already
dispatched — skip.
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
71d0ff9 to
711a52c
Compare
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: 711a52c7df
ℹ️ 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".
…ields Hard-coded UTF-8 in readPartContent() caused mojibake for fields with Content-Type: text/plain; charset=ISO-8859-1 (or any non-UTF-8 charset). Now parses the charset from the part's Content-Type header and falls back to UTF-8 only when absent or unrecognised.
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: 311e00ec81
ℹ️ 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".
…p10ForkedTest The lockfile was out of sync: testCompileClasspath was locked to 10.0.0 instead of 10.0.10, and earlyDep10ForkedTest configurations were missing entirely. Regenerated with --write-locks. Also added a resolutionStrategy to force Jetty 10.0.9 for earlyDep10ForkedTest: without it, Gradle picks 10.0.10 (inherited via testImplementation), defeating the purpose of the suite which exercises the MultiPartFormInputStream path present in [10.0.0, 10.0.10).
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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
2 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
Instruments Jetty's multipart request handling to fire both
requestFilesFilenamesandrequestBodyProcessedAppSec gateway events, enabling WAF rules that act on uploaded filenames and multipart form-field values.Jetty parses multipart bodies through two code paths depending on how the application accesses the request:
getParameterMap()→extractContentParameters()→ internalgetParts(MultiMap)getParts()/getPart(String)call from user codeBoth paths are instrumented where they exist. Guards on internal state fields (
_contentParameters,_multiParts,_multiPartInputStream) prevent double-firing when the same method is called more than once per request.Module coverage
jetty-appsec-7.0getParts()does not exist in Jetty 7.xjetty-appsec-8.1.3[8.1.3, 9.2.0.RC0)jetty-appsec-9.2[9.2, 9.3)requestBodyProcessed; no filename support needed at this rangejetty-appsec-9.3[9.3, 9.4.10)[9.3, 12). AddedGetFilenamesAdvice+GetFilenamesFromMultiPartAdvice. Double-fire guard uses_multiPartInputStream.jetty-appsec-9.4[9.4.10, 11.0)_multiParts: MultiParts(9.4.10+, 10.0.10+) and_multiParts: MultiPartFormInputStream(10.0.0–10.0.9). Bytecode discriminator:_dispatcherType: Ljavax/servlet/DispatcherType;.jetty-appsec-11.0[11.0, 12.0)_dispatcherType: Ljakarta/servlet/DispatcherType;inRequest.classbytecode — checking classpath presence ofjakarta.servletwas unreliable when bothjavaxandjakartawere present simultaneously.jetty-appsec-8.1.3 details
Replaced brittle ASM bytecode injection (
GetPartsMethodVisitor) with clean exit advice:GetFilenamesAdviceongetParts()— firesrequestBodyProcessed(form fields) +requestFilesFilenamesGetPartAdviceongetPart(String)— same, for single-part uploads that never callgetParts(); in Jetty 8.xgetPart(String)calls_multiPartInputStream.getPart()directly_multiPartInputStream == nullguard prevents re-firing on repeatedgetParts()callsRequest.classbytecode at agent startup, detecting thegetParameters()call insidegetParts()that is unique to Jetty 8.xPartHelper(replacingParameterCollector):filenameFromPart(): quote-awareContent-Dispositionparser;Part.getSubmittedFileName()is Servlet 3.1+ and unavailable hereextractFormFields(): reads form-field body values, skips file-upload partsfireBodyProcessedEvent()/fireFilenamesEvent(): encapsulate the full IG callback + blocking-commit logic, returningBlockingException|nullfor use with@Advice.Thrown(readOnly=false)Helpers and tests
PartHelper(8.1.3) andMultipartHelper(9.3/9.4/11.0) are unit-tested independently inPartHelperTest/MultipartHelperTestjetty-server-7.6,jetty-server-9.3,jetty-server-9.4.21,jetty-server-10.0,jetty-server-11.0cover both thegetParts()andgetParameterMap()code paths; a dedicatedJetty10EarlyDepForkedTestcovers Jetty 10.0.0–10.0.9Motivation
Part of APPSEC-61873 —
server.request.body.filenamesimplementation across server frameworks.Additional Notes
Bytecode discriminator rationale (9.4 vs 11.0): checking for
jakarta.servlet.http.Parton the classpath was unreliable when a Jetty 9.4/10 app ships bothjavaxandjakartaon the classpath (common with Spring Boot 3 migration tooling). The_dispatcherTypefield descriptor insideRequest.classbytecode is unambiguous regardless of classpath contents.GetFilenamesFromMultiPartAdvicevsGetFilenamesAdvice: in all 9.3/9.4/11.0 modules,extractContentParameters()sets_contentParametersbefore callinggetParts(MultiMap), so the_contentParameters != nullguard inGetFilenamesAdvicecorrectly prevents double-firing when thegetParameterMap()path is used.Part.getSubmittedFileName()availability: Servlet 3.1+ (Jetty 9.1+). For Jetty 8.x (Servlet 3.0),PartHelper.filenameFromPart()parses theContent-Dispositionheader manually with a quote-aware parser to handle filenames likefilename="shell;evil.php"correctly.Depends on #10973 (merged)
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: APPSEC-61873
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.