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 1 performance improvements and 0 performance regressions! Performance is the same for 61 metrics, 9 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.064 s) : 0, 1063718
Total [baseline] (11.035 s) : 0, 11035363
Agent [candidate] (1.06 s) : 0, 1059665
Total [candidate] (11.118 s) : 0, 11118047
section appsec
Agent [baseline] (1.253 s) : 0, 1252620
Total [baseline] (11.169 s) : 0, 11168943
Agent [candidate] (1.256 s) : 0, 1255729
Total [candidate] (11.088 s) : 0, 11088142
section iast
Agent [baseline] (1.228 s) : 0, 1228265
Total [baseline] (11.314 s) : 0, 11313825
Agent [candidate] (1.236 s) : 0, 1235974
Total [candidate] (11.332 s) : 0, 11332234
section profiling
Agent [baseline] (1.187 s) : 0, 1186558
Total [baseline] (11.022 s) : 0, 11022199
Agent [candidate] (1.188 s) : 0, 1188015
Total [candidate] (11.053 s) : 0, 11053247
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.248 ms) : 0, 1248
crashtracking [candidate] (1.231 ms) : 0, 1231
BytebuddyAgent [baseline] (634.72 ms) : 0, 634720
BytebuddyAgent [candidate] (633.648 ms) : 0, 633648
AgentMeter [baseline] (29.646 ms) : 0, 29646
AgentMeter [candidate] (29.46 ms) : 0, 29460
GlobalTracer [baseline] (250.113 ms) : 0, 250113
GlobalTracer [candidate] (249.016 ms) : 0, 249016
AppSec [baseline] (32.522 ms) : 0, 32522
AppSec [candidate] (32.38 ms) : 0, 32380
Debugger [baseline] (60.266 ms) : 0, 60266
Debugger [candidate] (59.923 ms) : 0, 59923
Remote Config [baseline] (597.373 µs) : 0, 597
Remote Config [candidate] (588.654 µs) : 0, 589
Telemetry [baseline] (8.032 ms) : 0, 8032
Telemetry [candidate] (8.728 ms) : 0, 8728
Flare Poller [baseline] (10.251 ms) : 0, 10251
Flare Poller [candidate] (8.363 ms) : 0, 8363
section appsec
crashtracking [baseline] (1.231 ms) : 0, 1231
crashtracking [candidate] (1.221 ms) : 0, 1221
BytebuddyAgent [baseline] (662.94 ms) : 0, 662940
BytebuddyAgent [candidate] (666.11 ms) : 0, 666110
AgentMeter [baseline] (12.341 ms) : 0, 12341
AgentMeter [candidate] (12.255 ms) : 0, 12255
GlobalTracer [baseline] (250.201 ms) : 0, 250201
GlobalTracer [candidate] (249.245 ms) : 0, 249245
AppSec [baseline] (186.158 ms) : 0, 186158
AppSec [candidate] (186.47 ms) : 0, 186470
Debugger [baseline] (65.99 ms) : 0, 65990
Debugger [candidate] (66.651 ms) : 0, 66651
Remote Config [baseline] (609.889 µs) : 0, 610
Remote Config [candidate] (619.361 µs) : 0, 619
Telemetry [baseline] (8.472 ms) : 0, 8472
Telemetry [candidate] (8.579 ms) : 0, 8579
Flare Poller [baseline] (3.604 ms) : 0, 3604
Flare Poller [candidate] (3.565 ms) : 0, 3565
IAST [baseline] (24.612 ms) : 0, 24612
IAST [candidate] (24.548 ms) : 0, 24548
section iast
crashtracking [baseline] (1.235 ms) : 0, 1235
crashtracking [candidate] (1.234 ms) : 0, 1234
BytebuddyAgent [baseline] (803.35 ms) : 0, 803350
BytebuddyAgent [candidate] (809.129 ms) : 0, 809129
AgentMeter [baseline] (11.656 ms) : 0, 11656
AgentMeter [candidate] (11.713 ms) : 0, 11713
GlobalTracer [baseline] (239.801 ms) : 0, 239801
GlobalTracer [candidate] (240.875 ms) : 0, 240875
AppSec [baseline] (29.681 ms) : 0, 29681
AppSec [candidate] (29.118 ms) : 0, 29118
Debugger [baseline] (66.239 ms) : 0, 66239
Debugger [candidate] (67.967 ms) : 0, 67967
Remote Config [baseline] (531.873 µs) : 0, 532
Remote Config [candidate] (544.454 µs) : 0, 544
Telemetry [baseline] (9.279 ms) : 0, 9279
Telemetry [candidate] (9.373 ms) : 0, 9373
Flare Poller [baseline] (3.563 ms) : 0, 3563
Flare Poller [candidate] (3.626 ms) : 0, 3626
IAST [baseline] (26.636 ms) : 0, 26636
IAST [candidate] (25.963 ms) : 0, 25963
section profiling
ProfilingAgent [baseline] (94.037 ms) : 0, 94037
ProfilingAgent [candidate] (93.972 ms) : 0, 93972
crashtracking [baseline] (1.181 ms) : 0, 1181
crashtracking [candidate] (1.19 ms) : 0, 1190
BytebuddyAgent [baseline] (692.537 ms) : 0, 692537
BytebuddyAgent [candidate] (694.286 ms) : 0, 694286
AgentMeter [baseline] (9.233 ms) : 0, 9233
AgentMeter [candidate] (8.944 ms) : 0, 8944
GlobalTracer [baseline] (207.311 ms) : 0, 207311
GlobalTracer [candidate] (207.612 ms) : 0, 207612
AppSec [baseline] (32.843 ms) : 0, 32843
AppSec [candidate] (32.857 ms) : 0, 32857
Debugger [baseline] (65.913 ms) : 0, 65913
Debugger [candidate] (65.609 ms) : 0, 65609
Remote Config [baseline] (595.224 µs) : 0, 595
Remote Config [candidate] (568.982 µs) : 0, 569
Telemetry [baseline] (7.769 ms) : 0, 7769
Telemetry [candidate] (7.801 ms) : 0, 7801
Flare Poller [baseline] (3.516 ms) : 0, 3516
Flare Poller [candidate] (3.575 ms) : 0, 3575
Profiling [baseline] (94.611 ms) : 0, 94611
Profiling [candidate] (94.552 ms) : 0, 94552
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.059 s) : 0, 1059176
Total [baseline] (8.871 s) : 0, 8870757
Agent [candidate] (1.064 s) : 0, 1063727
Total [candidate] (8.852 s) : 0, 8852037
section iast
Agent [baseline] (1.226 s) : 0, 1226259
Total [baseline] (9.584 s) : 0, 9583563
Agent [candidate] (1.226 s) : 0, 1226029
Total [candidate] (9.571 s) : 0, 9570616
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.241 ms) : 0, 1241
crashtracking [candidate] (1.234 ms) : 0, 1234
BytebuddyAgent [baseline] (634.504 ms) : 0, 634504
BytebuddyAgent [candidate] (638.501 ms) : 0, 638501
AgentMeter [baseline] (29.624 ms) : 0, 29624
AgentMeter [candidate] (29.798 ms) : 0, 29798
GlobalTracer [baseline] (249.432 ms) : 0, 249432
GlobalTracer [candidate] (250.614 ms) : 0, 250614
AppSec [baseline] (32.517 ms) : 0, 32517
AppSec [candidate] (32.559 ms) : 0, 32559
Debugger [baseline] (59.457 ms) : 0, 59457
Debugger [candidate] (59.5 ms) : 0, 59500
Remote Config [baseline] (617.995 µs) : 0, 618
Remote Config [candidate] (594.939 µs) : 0, 595
Telemetry [baseline] (7.998 ms) : 0, 7998
Telemetry [candidate] (8.027 ms) : 0, 8027
Flare Poller [baseline] (7.408 ms) : 0, 7408
Flare Poller [candidate] (6.516 ms) : 0, 6516
section iast
crashtracking [baseline] (1.239 ms) : 0, 1239
crashtracking [candidate] (1.227 ms) : 0, 1227
BytebuddyAgent [baseline] (802.978 ms) : 0, 802978
BytebuddyAgent [candidate] (803.065 ms) : 0, 803065
AgentMeter [baseline] (11.598 ms) : 0, 11598
AgentMeter [candidate] (11.624 ms) : 0, 11624
GlobalTracer [baseline] (239.459 ms) : 0, 239459
GlobalTracer [candidate] (239.548 ms) : 0, 239548
AppSec [baseline] (33.639 ms) : 0, 33639
AppSec [candidate] (32.053 ms) : 0, 32053
Debugger [baseline] (61.908 ms) : 0, 61908
Debugger [candidate] (63.25 ms) : 0, 63250
Remote Config [baseline] (536.338 µs) : 0, 536
Remote Config [candidate] (547.122 µs) : 0, 547
Telemetry [baseline] (9.295 ms) : 0, 9295
Telemetry [candidate] (9.171 ms) : 0, 9171
Flare Poller [baseline] (3.542 ms) : 0, 3542
Flare Poller [candidate] (3.53 ms) : 0, 3530
IAST [baseline] (25.878 ms) : 0, 25878
IAST [candidate] (25.855 ms) : 0, 25855
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.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (1.256 ms) : 1243, 1268
. : milestone, 1256,
iast (3.258 ms) : 3213, 3303
. : milestone, 3258,
iast_FULL (6.005 ms) : 5944, 6067
. : milestone, 6005,
iast_GLOBAL (3.62 ms) : 3563, 3678
. : milestone, 3620,
profiling (2.327 ms) : 2306, 2349
. : milestone, 2327,
tracing (1.939 ms) : 1923, 1955
. : milestone, 1939,
section candidate
no_agent (1.245 ms) : 1233, 1258
. : milestone, 1245,
iast (3.38 ms) : 3330, 3431
. : milestone, 3380,
iast_FULL (6.13 ms) : 6067, 6193
. : milestone, 6130,
iast_GLOBAL (3.706 ms) : 3644, 3768
. : milestone, 3706,
profiling (2.184 ms) : 2163, 2205
. : milestone, 2184,
tracing (1.868 ms) : 1853, 1883
. : milestone, 1868,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (19.01 ms) : 18817, 19204
. : milestone, 19010,
appsec (18.604 ms) : 18414, 18794
. : milestone, 18604,
code_origins (18.046 ms) : 17867, 18225
. : milestone, 18046,
iast (19.26 ms) : 19061, 19458
. : milestone, 19260,
profiling (18.44 ms) : 18255, 18626
. : milestone, 18440,
tracing (17.938 ms) : 17766, 18111
. : milestone, 17938,
section candidate
no_agent (19.647 ms) : 19443, 19850
. : milestone, 19647,
appsec (18.467 ms) : 18285, 18649
. : milestone, 18467,
code_origins (17.904 ms) : 17725, 18082
. : milestone, 17904,
iast (18.075 ms) : 17895, 18254
. : milestone, 18075,
profiling (19.514 ms) : 19319, 19710
. : milestone, 19514,
tracing (17.548 ms) : 17377, 17720
. : milestone, 17548,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (15.715 s) : 15715000, 15715000
. : milestone, 15715000,
appsec (14.829 s) : 14829000, 14829000
. : milestone, 14829000,
iast (18.679 s) : 18679000, 18679000
. : milestone, 18679000,
iast_GLOBAL (18.498 s) : 18498000, 18498000
. : milestone, 18498000,
profiling (15.736 s) : 15736000, 15736000
. : milestone, 15736000,
tracing (14.907 s) : 14907000, 14907000
. : milestone, 14907000,
section candidate
no_agent (15.304 s) : 15304000, 15304000
. : milestone, 15304000,
appsec (14.716 s) : 14716000, 14716000
. : milestone, 14716000,
iast (18.695 s) : 18695000, 18695000
. : milestone, 18695000,
iast_GLOBAL (18.065 s) : 18065000, 18065000
. : milestone, 18065000,
profiling (15.214 s) : 15214000, 15214000
. : milestone, 15214000,
tracing (14.9 s) : 14900000, 14900000
. : milestone, 14900000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~ce166bef0a, baseline=1.62.0-SNAPSHOT~081af53226
dateFormat X
axisFormat %s
section baseline
no_agent (1.5 ms) : 1489, 1512
. : milestone, 1500,
appsec (3.797 ms) : 3577, 4016
. : milestone, 3797,
iast (2.289 ms) : 2219, 2359
. : milestone, 2289,
iast_GLOBAL (2.338 ms) : 2267, 2408
. : milestone, 2338,
profiling (2.118 ms) : 2062, 2173
. : milestone, 2118,
tracing (2.091 ms) : 2037, 2145
. : milestone, 2091,
section candidate
no_agent (1.493 ms) : 1481, 1505
. : milestone, 1493,
appsec (2.568 ms) : 2513, 2624
. : milestone, 2568,
iast (2.3 ms) : 2230, 2370
. : milestone, 2300,
iast_GLOBAL (2.345 ms) : 2274, 2415
. : milestone, 2345,
profiling (2.109 ms) : 2054, 2164
. : milestone, 2109,
tracing (2.095 ms) : 2041, 2149
. : milestone, 2095,
|
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
from
e3d4073 to
e2d5ed0
Compare
Add GetFilenamesAdvice to all three Jetty AppSec modules to collect uploaded file names from multipart requests and fire the requestFilesFilenames() IG callback: - jetty-appsec-8.1.3: intercepts getParts() return value; includes Content-Disposition header fallback for Servlet 3.0 (Jetty 9.0) where getSubmittedFileName() is not available - jetty-appsec-9.2: intercepts no-arg getParts() for Servlet 3.1+ - jetty-appsec-9.3: same, applies to Jetty 9.3, 10, 11 Enable testBodyFilenames() in Jetty 9.x, 10 and 11 server tests.
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
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
2 times, most recently
from
ecf65c5 to
eae08aa
Compare
…MultiMap) path - jetty-appsec-9.3: add call-depth guard (Collection.class) to GetFilenamesAdvice to prevent double callback invocation when getParts() calls getParts(MultiMap) internally - jetty-appsec-9.2: extend GetFilenamesAdvice matcher to all getParts overloads (not just no-arg) to cover getParameter*()/getParameterMap() code paths, guarded with same call-depth mechanism to avoid double-firing
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-3
branch
2 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
- Add BODY_MULTIPART_REPEATED case to TestServlet3 (javax) so Jetty 9.x/10.x test modules can exercise the repeated getParts() scenario - Enable testBodyFilenamesCalledOnce() for Jetty 9.0, 9.0.4, 9.3, 9.4.21, and 10.0
…vice path - New BODY_MULTIPART_COMBINED endpoint: calls getParameterMap() first (triggers GetFilenamesFromMultiPartAdvice via extractContentParameters -> getParts(MultiMap)), then getParts() explicitly (GetFilenamesAdvice must not double-fire since _contentParameters is already set) - New test 'file upload filenames called once via parameter map' verifies the callback fires exactly once across both advice paths - Enabled in Jetty 9.0, 9.0.4, 9.3, 9.4.21, 10.0 and 11.0
…eplaces _contentParameters as the getParts() cache In Jetty 9.3, getParts(MultiMap) sets _contentParameters, so the map==null guard prevents re-firing on repeated getParts() calls. In Jetty 9.4+, getParts() delegates to getParts(null) and caches the result in _multiParts instead, leaving _contentParameters null on every call. Add _multiParts==null as an additional guard (optional=true handles Jetty 9.3 where the field does not exist).
…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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e43466fc5b
ℹ️ 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".
…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.
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: 1f2e2b3998
ℹ️ 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".
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.
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: 0ce4bf1084
ℹ️ 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".
…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.
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: 74948a75d6
ℹ️ 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".
…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.
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: 1508fef49c
ℹ️ 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".
…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.
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: bece33644c
ℹ️ 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".
…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.
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 the
requestFilesFilenamesAppSec gateway event, enabling WAF rules that act on uploaded file names.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.jetty-appsec-7.0getParts()does not exist in Jetty 7.xjetty-appsec-8.1.3[8.1.3, 9.2.0.RC0)GetPartsMethodVisitor) with clean exit advice ongetParts()andgetPart(String). In Jetty 8.x,getPart(String)calls_multiPartInputStream.getPart()directly without going throughgetParts(), so it requires its own advice to catch single-part uploads. AddedPartHelperto parseContent-Dispositionheaders manually —Part.getSubmittedFileName()is Servlet 3.1+ and unavailable in Jetty 8.x (Servlet 3.0). The_multiPartInputStream == nullguard inGetFilenamesAdviceprevents WAF callbacks from re-firing on repeatedgetParts()calls (the field is set on first parse and non-null on cached calls).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)to[9.3, 9.4.10). AddedGetFilenamesAdvice+GetFilenamesFromMultiPartAdvice. Double-fire guard uses_multiPartInputStream(present only in this range; replaced by_multiPartsin 9.4.10). AddedMultipartHelperusingPart.getSubmittedFileName()(Servlet 3.1).jetty-appsec-9.4[9.4.10, 11.0)javax.servletversions from 9.4.10 onward:[9.4.10, 10.0)where_multiPartsisMultiParts, and[10.0.0, 11.0)(Jetty 10.0.0–10.0.9 used_multiParts: MultiPartFormInputStream; 10.0.10+ switched toMultiParts). An OR-muzzle reference covers both field types. The_dispatcherType: Ljavax/servlet/DispatcherType;bytecode field discriminates against Jetty 11+ (which usesLjakarta/servlet/DispatcherType;). Previously handled byjetty-appsec-9.3via reflection.jetty-appsec-11.0[11.0, 12.0)jakarta.servlet.http.Part). Identical structure to 9.4 but compiled againstjakarta. The muzzle discriminator uses_dispatcherType: Ljakarta/servlet/DispatcherType;inRequest.classbytecode — checking forjakarta.servlet.http.Parton the classpath was unreliable when bothjavaxandjakartawere present, causing double instrumentation. Previously handled byjetty-appsec-9.3via reflection.Motivation
Part of APPSEC-61873 —
server.request.body.filenamesimplementation across server frameworks.Additional Notes
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 issue