Add server.request.body.filenames AppSec address for Undertow and Play#11174
Open
Add server.request.body.filenames AppSec address for Undertow and Play#11174
Conversation
- Undertow: extract filenames from FormData attachments in MultiPartUploadHandlerInstrumentation - Play 2.5/2.6: extract filenames from MultipartFormData.files() in BodyParserHelpers Both implementations fire the requestFilesFilenames() IG event and support blocking on malicious filenames.
In undertow 2.2+, FormValueImpl.isFile() returns false for in-memory file uploads (file size below fileSizeThreshold) because it checks fileItem.isInMemory(). Use getFileName() to identify file uploads regardless of storage, which works across all undertow versions. Also check the filenames callback before building the list to avoid allocations on requests where the feature is inactive.
jandro996
added
type: enhancement
Member
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f340ebfab9
ℹ️ 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".
…Undertow Both callbacks are now fetched upfront; the method only returns early when both are null. Previously an early return on requestBodyProcessed==null silently skipped filename detection, breaking deployments with filename-only WAF rules.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 60 metrics, 11 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.061 s) : 0, 1060687
Total [baseline] (11.078 s) : 0, 11078058
Agent [candidate] (1.063 s) : 0, 1062986
Total [candidate] (11.163 s) : 0, 11162926
section appsec
Agent [baseline] (1.264 s) : 0, 1264329
Total [baseline] (11.071 s) : 0, 11070568
Agent [candidate] (1.268 s) : 0, 1268457
Total [candidate] (11.245 s) : 0, 11244872
section iast
Agent [baseline] (1.242 s) : 0, 1241933
Total [baseline] (11.356 s) : 0, 11356214
Agent [candidate] (1.234 s) : 0, 1234342
Total [candidate] (11.344 s) : 0, 11343791
section profiling
Agent [baseline] (1.187 s) : 0, 1186802
Total [baseline] (11.033 s) : 0, 11032564
Agent [candidate] (1.187 s) : 0, 1187070
Total [candidate] (11.102 s) : 0, 11101696
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.234 ms) : 0, 1234
crashtracking [candidate] (1.225 ms) : 0, 1225
BytebuddyAgent [baseline] (635.638 ms) : 0, 635638
BytebuddyAgent [candidate] (639.627 ms) : 0, 639627
AgentMeter [baseline] (29.591 ms) : 0, 29591
AgentMeter [candidate] (29.735 ms) : 0, 29735
GlobalTracer [baseline] (249.51 ms) : 0, 249510
GlobalTracer [candidate] (250.577 ms) : 0, 250577
AppSec [baseline] (32.341 ms) : 0, 32341
AppSec [candidate] (32.473 ms) : 0, 32473
Debugger [baseline] (59.879 ms) : 0, 59879
Debugger [candidate] (60.023 ms) : 0, 60023
Remote Config [baseline] (596.017 µs) : 0, 596
Remote Config [candidate] (595.128 µs) : 0, 595
Telemetry [baseline] (9.579 ms) : 0, 9579
Telemetry [candidate] (8.065 ms) : 0, 8065
Flare Poller [baseline] (5.899 ms) : 0, 5899
Flare Poller [candidate] (4.348 ms) : 0, 4348
section appsec
crashtracking [baseline] (1.239 ms) : 0, 1239
crashtracking [candidate] (1.241 ms) : 0, 1241
BytebuddyAgent [baseline] (676.661 ms) : 0, 676661
BytebuddyAgent [candidate] (678.366 ms) : 0, 678366
AgentMeter [baseline] (12.16 ms) : 0, 12160
AgentMeter [candidate] (12.205 ms) : 0, 12205
GlobalTracer [baseline] (249.523 ms) : 0, 249523
GlobalTracer [candidate] (250.52 ms) : 0, 250520
IAST [baseline] (24.199 ms) : 0, 24199
IAST [candidate] (24.337 ms) : 0, 24337
AppSec [baseline] (185.969 ms) : 0, 185969
AppSec [candidate] (187.926 ms) : 0, 187926
Debugger [baseline] (66.132 ms) : 0, 66132
Debugger [candidate] (65.333 ms) : 0, 65333
Remote Config [baseline] (578.49 µs) : 0, 578
Remote Config [candidate] (570.064 µs) : 0, 570
Telemetry [baseline] (7.919 ms) : 0, 7919
Telemetry [candidate] (7.955 ms) : 0, 7955
Flare Poller [baseline] (3.457 ms) : 0, 3457
Flare Poller [candidate] (3.446 ms) : 0, 3446
section iast
crashtracking [baseline] (1.261 ms) : 0, 1261
crashtracking [candidate] (1.232 ms) : 0, 1232
BytebuddyAgent [baseline] (815.78 ms) : 0, 815780
BytebuddyAgent [candidate] (810.776 ms) : 0, 810776
AgentMeter [baseline] (11.545 ms) : 0, 11545
AgentMeter [candidate] (11.419 ms) : 0, 11419
GlobalTracer [baseline] (240.939 ms) : 0, 240939
GlobalTracer [candidate] (239.669 ms) : 0, 239669
IAST [baseline] (29.331 ms) : 0, 29331
IAST [candidate] (29.205 ms) : 0, 29205
AppSec [baseline] (29.615 ms) : 0, 29615
AppSec [candidate] (29.412 ms) : 0, 29412
Debugger [baseline] (65.198 ms) : 0, 65198
Debugger [candidate] (64.782 ms) : 0, 64782
Remote Config [baseline] (547.111 µs) : 0, 547
Remote Config [candidate] (543.993 µs) : 0, 544
Telemetry [baseline] (7.792 ms) : 0, 7792
Telemetry [candidate] (7.764 ms) : 0, 7764
Flare Poller [baseline] (3.454 ms) : 0, 3454
Flare Poller [candidate] (3.396 ms) : 0, 3396
section profiling
crashtracking [baseline] (1.188 ms) : 0, 1188
crashtracking [candidate] (1.193 ms) : 0, 1193
BytebuddyAgent [baseline] (693.459 ms) : 0, 693459
BytebuddyAgent [candidate] (693.001 ms) : 0, 693001
AgentMeter [baseline] (8.975 ms) : 0, 8975
AgentMeter [candidate] (9.007 ms) : 0, 9007
GlobalTracer [baseline] (207.519 ms) : 0, 207519
GlobalTracer [candidate] (207.798 ms) : 0, 207798
AppSec [baseline] (32.565 ms) : 0, 32565
AppSec [candidate] (32.711 ms) : 0, 32711
Debugger [baseline] (65.643 ms) : 0, 65643
Debugger [candidate] (65.724 ms) : 0, 65724
Remote Config [baseline] (577.877 µs) : 0, 578
Remote Config [candidate] (577.55 µs) : 0, 578
Telemetry [baseline] (7.811 ms) : 0, 7811
Telemetry [candidate] (7.862 ms) : 0, 7862
Flare Poller [baseline] (3.501 ms) : 0, 3501
Flare Poller [candidate] (3.5 ms) : 0, 3500
ProfilingAgent [baseline] (93.977 ms) : 0, 93977
ProfilingAgent [candidate] (94.363 ms) : 0, 94363
Profiling [baseline] (94.536 ms) : 0, 94536
Profiling [candidate] (94.921 ms) : 0, 94921
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.056 s) : 0, 1055923
Total [baseline] (8.88 s) : 0, 8880287
Agent [candidate] (1.058 s) : 0, 1057820
Total [candidate] (8.898 s) : 0, 8897676
section iast
Agent [baseline] (1.234 s) : 0, 1233652
Total [baseline] (9.616 s) : 0, 9616036
Agent [candidate] (1.238 s) : 0, 1237898
Total [candidate] (9.62 s) : 0, 9619592
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.235 ms) : 0, 1235
crashtracking [candidate] (1.237 ms) : 0, 1237
BytebuddyAgent [baseline] (635.493 ms) : 0, 635493
BytebuddyAgent [candidate] (636.384 ms) : 0, 636384
AgentMeter [baseline] (29.572 ms) : 0, 29572
AgentMeter [candidate] (29.569 ms) : 0, 29569
GlobalTracer [baseline] (249.081 ms) : 0, 249081
GlobalTracer [candidate] (249.222 ms) : 0, 249222
AppSec [baseline] (32.2 ms) : 0, 32200
AppSec [candidate] (32.398 ms) : 0, 32398
Debugger [baseline] (59.052 ms) : 0, 59052
Debugger [candidate] (58.959 ms) : 0, 58959
Remote Config [baseline] (605.392 µs) : 0, 605
Remote Config [candidate] (602.19 µs) : 0, 602
Telemetry [baseline] (8.017 ms) : 0, 8017
Telemetry [candidate] (8.811 ms) : 0, 8811
Flare Poller [baseline] (4.344 ms) : 0, 4344
Flare Poller [candidate] (4.312 ms) : 0, 4312
section iast
crashtracking [baseline] (1.239 ms) : 0, 1239
crashtracking [candidate] (1.225 ms) : 0, 1225
BytebuddyAgent [baseline] (810.651 ms) : 0, 810651
BytebuddyAgent [candidate] (812.53 ms) : 0, 812530
AgentMeter [baseline] (11.39 ms) : 0, 11390
AgentMeter [candidate] (11.407 ms) : 0, 11407
GlobalTracer [baseline] (239.693 ms) : 0, 239693
GlobalTracer [candidate] (241.289 ms) : 0, 241289
IAST [baseline] (27.368 ms) : 0, 27368
IAST [candidate] (27.671 ms) : 0, 27671
AppSec [baseline] (28.662 ms) : 0, 28662
AppSec [candidate] (30.696 ms) : 0, 30696
Debugger [baseline] (66.677 ms) : 0, 66677
Debugger [candidate] (64.894 ms) : 0, 64894
Remote Config [baseline] (534.184 µs) : 0, 534
Remote Config [candidate] (552.107 µs) : 0, 552
Telemetry [baseline] (7.769 ms) : 0, 7769
Telemetry [candidate] (7.9 ms) : 0, 7900
Flare Poller [baseline] (3.479 ms) : 0, 3479
Flare Poller [candidate] (3.511 ms) : 0, 3511
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section baseline
no_agent (18.14 ms) : 17954, 18327
. : milestone, 18140,
appsec (19.395 ms) : 19202, 19589
. : milestone, 19395,
code_origins (18.74 ms) : 18553, 18927
. : milestone, 18740,
iast (17.887 ms) : 17710, 18064
. : milestone, 17887,
profiling (18.348 ms) : 18164, 18533
. : milestone, 18348,
tracing (17.832 ms) : 17656, 18008
. : milestone, 17832,
section candidate
no_agent (18.058 ms) : 17875, 18240
. : milestone, 18058,
appsec (19.138 ms) : 18945, 19332
. : milestone, 19138,
code_origins (17.972 ms) : 17795, 18150
. : milestone, 17972,
iast (17.989 ms) : 17806, 18172
. : milestone, 17989,
profiling (18.128 ms) : 17948, 18307
. : milestone, 18128,
tracing (17.841 ms) : 17666, 18017
. : milestone, 17841,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section baseline
no_agent (1.367 ms) : 1353, 1381
. : milestone, 1367,
iast (3.511 ms) : 3459, 3563
. : milestone, 3511,
iast_FULL (6.032 ms) : 5970, 6093
. : milestone, 6032,
iast_GLOBAL (3.743 ms) : 3682, 3804
. : milestone, 3743,
profiling (2.151 ms) : 2129, 2172
. : milestone, 2151,
tracing (1.847 ms) : 1831, 1863
. : milestone, 1847,
section candidate
no_agent (1.273 ms) : 1261, 1285
. : milestone, 1273,
iast (3.407 ms) : 3358, 3457
. : milestone, 3407,
iast_FULL (6.053 ms) : 5991, 6114
. : milestone, 6053,
iast_GLOBAL (3.606 ms) : 3547, 3665
. : milestone, 3606,
profiling (2.204 ms) : 2184, 2224
. : milestone, 2204,
tracing (1.869 ms) : 1854, 1884
. : milestone, 1869,
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~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section baseline
no_agent (15.013 s) : 15013000, 15013000
. : milestone, 15013000,
appsec (14.642 s) : 14642000, 14642000
. : milestone, 14642000,
iast (18.247 s) : 18247000, 18247000
. : milestone, 18247000,
iast_GLOBAL (17.752 s) : 17752000, 17752000
. : milestone, 17752000,
profiling (15.55 s) : 15550000, 15550000
. : milestone, 15550000,
tracing (14.74 s) : 14740000, 14740000
. : milestone, 14740000,
section candidate
no_agent (15.019 s) : 15019000, 15019000
. : milestone, 15019000,
appsec (14.974 s) : 14974000, 14974000
. : milestone, 14974000,
iast (18.248 s) : 18248000, 18248000
. : milestone, 18248000,
iast_GLOBAL (18.085 s) : 18085000, 18085000
. : milestone, 18085000,
profiling (15.606 s) : 15606000, 15606000
. : milestone, 15606000,
tracing (14.834 s) : 14834000, 14834000
. : milestone, 14834000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~43dc2fcc14, baseline=1.62.0-SNAPSHOT~d4d2069709
dateFormat X
axisFormat %s
section baseline
no_agent (1.487 ms) : 1476, 1499
. : milestone, 1487,
appsec (3.829 ms) : 3607, 4051
. : milestone, 3829,
iast (2.273 ms) : 2204, 2343
. : milestone, 2273,
iast_GLOBAL (2.311 ms) : 2241, 2381
. : milestone, 2311,
profiling (2.095 ms) : 2040, 2150
. : milestone, 2095,
tracing (2.075 ms) : 2022, 2129
. : milestone, 2075,
section candidate
no_agent (1.481 ms) : 1470, 1493
. : milestone, 1481,
appsec (3.817 ms) : 3594, 4039
. : milestone, 3817,
iast (2.269 ms) : 2200, 2338
. : milestone, 2269,
iast_GLOBAL (2.313 ms) : 2243, 2383
. : milestone, 2313,
profiling (2.084 ms) : 2030, 2139
. : milestone, 2084,
tracing (2.069 ms) : 2016, 2123
. : milestone, 2069,
|
…port Use reflection to invoke MultipartFormData.files() so the bytecode does not embed a hard reference to the Scala 2.11/2.12 return type (Lscala/collection/Seq;). In Scala 2.13 (Play 2.7+) the method returns scala.collection.immutable.Seq, causing muzzle to disable the entire PlayBodyParsersInstrumentation and breaking all body-parsing features. Also enable testBodyFilenames() in Play 2.5/2.6/2.7 test suites.
Avoids a redundant WAF evaluation when the request was already blocked by the requestBodyProcessed callback. The filenamesCb is now only invoked when t == null (no block committed yet), and the inner t == null guard is removed since it is now guaranteed by the outer condition.
…rd; add play-2.5 unit tests
jandro996
force-pushed
the
alejandro.gonzalez/APPSEC-61873-4-undertow-play
branch
from
c991aee to
b3a4e2a
Compare
Member
Author
|
@codex review |
jandro996
changed the title
Add getCharset(), getFileItem(), isFileItem(), and isBigField() stubs to the anonymous FormData.FormValue in addInMemoryFileValue(). These abstract methods were added in undertow 2.2.x and caused compilation failure when the latestDepForkedTest resolved the latest 2.2.x release.
…ferences FormData.FormValue gained getCharset(), getFileItem(), isFileItem(), and isBigField() in undertow 2.2.x. A static anonymous class can't implement all versions simultaneously. Using a Proxy resolves the interface at runtime, so the test compiles against undertow 2.0 and runs correctly against the latest 2.2.x dependency.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
… tests AbstractPlayServerTest is shared by both play-2.6 (no AppSec) and play-appsec-2.6 tests. Setting testBodyFilenames=true there caused the plain play-2.6 tests to check the request.body.filenames tag, which is never set without the AppSec instrumentation. Move the override into PlayServerTest and PlayAsyncServerTest in play-appsec-2.6, which are the modules where the instrumentation is active.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Undertow (
undertow-2.0, covers 2.0–2.3+)MultiPartUploadHandlerInstrumentation: Extends theparseBlocking()exit advice to fire therequestFilesFilenamesIG event alongside the existingrequestBodyProcessedevent. The two callbacks are evaluated independently — early return only when both are null, so deployments with filename-only WAF rules still work. The filenames callback is skipped when the body callback already triggered a block.tryCommitBlockingResponse()return value is now checked before assigning the thrown exception, consistent with Play behavior.FormDataMapbug fix: Switched the text-field filter from!isFile()togetFileName() == null. In Undertow 2.2+, in-memory file uploads (below the size threshold) haveisFile() == falseeven though they are file uploads — the old guard causedgetValue()to be called on file parts, throwingIllegalStateExceptionon every multipart request with small attachments.getFileName() == nullcorrectly identifies text fields in all Undertow versions.Play 2.5 / 2.6 (
play-appsec-2.5,play-appsec-2.6)BodyParserHelpers.handleMultipartFormData(): AddshandleMultipartFilenames()which iteratesMultipartFormData.FilePart.filename()values and fires therequestFilesFilenamescallback viaexecuteFilenamesCallback()with full blocking support.MultipartFormData.files()returnsscala.collection.Seqin Scala 2.11/2.12 butscala.collection.immutable.Seqin Scala 2.13 (Play 2.7+). A direct call embeds the return-type descriptor in bytecode; muzzle detects it as missing and disables the wholePlayBodyParsersInstrumentationfor Play 2.7. The fix caches theMethodin astatic final MULTIPART_FILES_METHODfield initialized once at class load — no return-type descriptor in bytecode, zero reflection cost per request.Motivation
Part of APPSEC-61873 —
server.request.body.filenamesimplementation across server frameworks.Depends on #10949 and #10973 (both merged into master).
Additional Notes
Static reflection pattern for Scala 2.13 compatibility:
MultipartFormData.files()has a binary-incompatible return type between Scala 2.11/2.12 (scala.collection.Seq) and 2.13 (scala.collection.immutable.Seq). Rather than duplicating modules or restricting the muzzle version range,MULTIPART_FILES_METHODis cached as astatic final Methodin astatic {}initializer. The initializer runs once when the helper class is loaded into the application classloader (where the correct Play version is already present), so there is no per-request reflection cost and no return-type descriptor appears in the instrumentation bytecode.Method.invokedispatches polymorphically, so the correct override is called regardless of the concreteMultipartFormDatasubclass.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.