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 63 metrics, 8 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.057 s) : 0, 1056828
Total [baseline] (8.848 s) : 0, 8848098
Agent [candidate] (1.06 s) : 0, 1059941
Total [candidate] (8.833 s) : 0, 8832757
section iast
Agent [baseline] (1.239 s) : 0, 1238866
Total [baseline] (9.533 s) : 0, 9532679
Agent [candidate] (1.241 s) : 0, 1241157
Total [candidate] (9.539 s) : 0, 9538778
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.241 ms) : 0, 1241
crashtracking [candidate] (1.226 ms) : 0, 1226
BytebuddyAgent [baseline] (636.995 ms) : 0, 636995
BytebuddyAgent [candidate] (638.742 ms) : 0, 638742
AgentMeter [baseline] (29.285 ms) : 0, 29285
AgentMeter [candidate] (29.542 ms) : 0, 29542
GlobalTracer [baseline] (248.188 ms) : 0, 248188
GlobalTracer [candidate] (249.836 ms) : 0, 249836
AppSec [baseline] (32.327 ms) : 0, 32327
AppSec [candidate] (32.514 ms) : 0, 32514
Debugger [baseline] (59.662 ms) : 0, 59662
Debugger [candidate] (59.718 ms) : 0, 59718
Remote Config [baseline] (604.034 µs) : 0, 604
Remote Config [candidate] (597.983 µs) : 0, 598
Telemetry [baseline] (8.76 ms) : 0, 8760
Telemetry [candidate] (8.036 ms) : 0, 8036
Flare Poller [baseline] (3.623 ms) : 0, 3623
Flare Poller [candidate] (3.595 ms) : 0, 3595
section iast
crashtracking [baseline] (1.225 ms) : 0, 1225
crashtracking [candidate] (1.225 ms) : 0, 1225
BytebuddyAgent [baseline] (814.614 ms) : 0, 814614
BytebuddyAgent [candidate] (817.62 ms) : 0, 817620
AgentMeter [baseline] (11.56 ms) : 0, 11560
AgentMeter [candidate] (11.495 ms) : 0, 11495
GlobalTracer [baseline] (240.117 ms) : 0, 240117
GlobalTracer [candidate] (240.548 ms) : 0, 240548
AppSec [baseline] (29.686 ms) : 0, 29686
AppSec [candidate] (27.559 ms) : 0, 27559
Debugger [baseline] (64.384 ms) : 0, 64384
Debugger [candidate] (62.249 ms) : 0, 62249
Remote Config [baseline] (534.732 µs) : 0, 535
Remote Config [candidate] (521.719 µs) : 0, 522
Telemetry [baseline] (7.764 ms) : 0, 7764
Telemetry [candidate] (7.709 ms) : 0, 7709
Flare Poller [baseline] (3.505 ms) : 0, 3505
Flare Poller [candidate] (3.477 ms) : 0, 3477
IAST [baseline] (29.366 ms) : 0, 29366
IAST [candidate] (32.652 ms) : 0, 32652
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.065 s) : 0, 1064846
Total [baseline] (11.09 s) : 0, 11090002
Agent [candidate] (1.065 s) : 0, 1064888
Total [candidate] (11.054 s) : 0, 11054387
section appsec
Agent [baseline] (1.263 s) : 0, 1262699
Total [baseline] (11.106 s) : 0, 11106452
Agent [candidate] (1.268 s) : 0, 1268270
Total [candidate] (11.094 s) : 0, 11093620
section iast
Agent [baseline] (1.234 s) : 0, 1233762
Total [baseline] (11.313 s) : 0, 11312792
Agent [candidate] (1.234 s) : 0, 1233914
Total [candidate] (11.295 s) : 0, 11294588
section profiling
Agent [baseline] (1.186 s) : 0, 1186352
Total [baseline] (11.129 s) : 0, 11129024
Agent [candidate] (1.188 s) : 0, 1187925
Total [candidate] (11.033 s) : 0, 11033294
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.23 ms) : 0, 1230
crashtracking [candidate] (1.228 ms) : 0, 1228
BytebuddyAgent [baseline] (640.182 ms) : 0, 640182
BytebuddyAgent [candidate] (639.892 ms) : 0, 639892
AgentMeter [baseline] (29.681 ms) : 0, 29681
AgentMeter [candidate] (29.596 ms) : 0, 29596
GlobalTracer [baseline] (250.833 ms) : 0, 250833
GlobalTracer [candidate] (250.756 ms) : 0, 250756
AppSec [baseline] (32.731 ms) : 0, 32731
AppSec [candidate] (32.518 ms) : 0, 32518
Debugger [baseline] (60.819 ms) : 0, 60819
Debugger [candidate] (60.753 ms) : 0, 60753
Remote Config [baseline] (607.062 µs) : 0, 607
Remote Config [candidate] (601.103 µs) : 0, 601
Telemetry [baseline] (8.108 ms) : 0, 8108
Telemetry [candidate] (8.93 ms) : 0, 8930
Flare Poller [baseline] (4.369 ms) : 0, 4369
Flare Poller [candidate] (4.408 ms) : 0, 4408
section appsec
crashtracking [baseline] (1.218 ms) : 0, 1218
crashtracking [candidate] (1.221 ms) : 0, 1221
BytebuddyAgent [baseline] (676.125 ms) : 0, 676125
BytebuddyAgent [candidate] (679.904 ms) : 0, 679904
AgentMeter [baseline] (12.259 ms) : 0, 12259
AgentMeter [candidate] (12.359 ms) : 0, 12359
GlobalTracer [baseline] (248.921 ms) : 0, 248921
GlobalTracer [candidate] (250.454 ms) : 0, 250454
AppSec [baseline] (186.027 ms) : 0, 186027
AppSec [candidate] (185.772 ms) : 0, 185772
Debugger [baseline] (65.69 ms) : 0, 65690
Debugger [candidate] (65.906 ms) : 0, 65906
Remote Config [baseline] (573.251 µs) : 0, 573
Remote Config [candidate] (586.559 µs) : 0, 587
Telemetry [baseline] (7.898 ms) : 0, 7898
Telemetry [candidate] (7.953 ms) : 0, 7953
Flare Poller [baseline] (3.417 ms) : 0, 3417
Flare Poller [candidate] (3.467 ms) : 0, 3467
IAST [baseline] (24.201 ms) : 0, 24201
IAST [candidate] (24.194 ms) : 0, 24194
section iast
crashtracking [baseline] (1.205 ms) : 0, 1205
crashtracking [candidate] (1.208 ms) : 0, 1208
BytebuddyAgent [baseline] (810.261 ms) : 0, 810261
BytebuddyAgent [candidate] (811.077 ms) : 0, 811077
AgentMeter [baseline] (11.342 ms) : 0, 11342
AgentMeter [candidate] (11.408 ms) : 0, 11408
GlobalTracer [baseline] (239.393 ms) : 0, 239393
GlobalTracer [candidate] (239.547 ms) : 0, 239547
AppSec [baseline] (28.338 ms) : 0, 28338
AppSec [candidate] (25.678 ms) : 0, 25678
Debugger [baseline] (66.079 ms) : 0, 66079
Debugger [candidate] (66.524 ms) : 0, 66524
Remote Config [baseline] (548.188 µs) : 0, 548
Remote Config [candidate] (523.414 µs) : 0, 523
Telemetry [baseline] (7.908 ms) : 0, 7908
Telemetry [candidate] (7.759 ms) : 0, 7759
Flare Poller [baseline] (3.536 ms) : 0, 3536
Flare Poller [candidate] (3.424 ms) : 0, 3424
IAST [baseline] (29.102 ms) : 0, 29102
IAST [candidate] (30.773 ms) : 0, 30773
section profiling
crashtracking [baseline] (1.17 ms) : 0, 1170
crashtracking [candidate] (1.182 ms) : 0, 1182
BytebuddyAgent [baseline] (692.255 ms) : 0, 692255
BytebuddyAgent [candidate] (694.878 ms) : 0, 694878
AgentMeter [baseline] (8.937 ms) : 0, 8937
AgentMeter [candidate] (8.987 ms) : 0, 8987
GlobalTracer [baseline] (207.673 ms) : 0, 207673
GlobalTracer [candidate] (207.7 ms) : 0, 207700
AppSec [baseline] (32.693 ms) : 0, 32693
AppSec [candidate] (32.504 ms) : 0, 32504
Debugger [baseline] (66.026 ms) : 0, 66026
Debugger [candidate] (65.308 ms) : 0, 65308
Remote Config [baseline] (570.526 µs) : 0, 571
Remote Config [candidate] (580.99 µs) : 0, 581
Telemetry [baseline] (7.896 ms) : 0, 7896
Telemetry [candidate] (7.781 ms) : 0, 7781
Flare Poller [baseline] (3.524 ms) : 0, 3524
Flare Poller [candidate] (3.518 ms) : 0, 3518
ProfilingAgent [baseline] (94.109 ms) : 0, 94109
ProfilingAgent [candidate] (93.635 ms) : 0, 93635
Profiling [baseline] (94.697 ms) : 0, 94697
Profiling [candidate] (94.205 ms) : 0, 94205
LoadParameters
See matching parameters
SummaryFound 4 performance improvements and 2 performance regressions! Performance is the same for 14 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section baseline
no_agent (18.254 ms) : 18069, 18439
. : milestone, 18254,
appsec (18.396 ms) : 18212, 18579
. : milestone, 18396,
code_origins (17.708 ms) : 17534, 17882
. : milestone, 17708,
iast (18.127 ms) : 17946, 18308
. : milestone, 18127,
profiling (18.334 ms) : 18151, 18516
. : milestone, 18334,
tracing (17.885 ms) : 17704, 18065
. : milestone, 17885,
section candidate
no_agent (18.18 ms) : 17997, 18364
. : milestone, 18180,
appsec (18.753 ms) : 18564, 18941
. : milestone, 18753,
code_origins (17.718 ms) : 17544, 17891
. : milestone, 17718,
iast (18.278 ms) : 18093, 18462
. : milestone, 18278,
profiling (18.31 ms) : 18126, 18494
. : milestone, 18310,
tracing (17.51 ms) : 17339, 17681
. : milestone, 17510,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section baseline
no_agent (1.243 ms) : 1231, 1254
. : milestone, 1243,
iast (3.213 ms) : 3172, 3254
. : milestone, 3213,
iast_FULL (6.031 ms) : 5971, 6091
. : milestone, 6031,
iast_GLOBAL (3.847 ms) : 3784, 3911
. : milestone, 3847,
profiling (2.365 ms) : 2339, 2390
. : milestone, 2365,
tracing (1.861 ms) : 1845, 1876
. : milestone, 1861,
section candidate
no_agent (1.261 ms) : 1250, 1272
. : milestone, 1261,
iast (3.465 ms) : 3416, 3514
. : milestone, 3465,
iast_FULL (5.894 ms) : 5835, 5954
. : milestone, 5894,
iast_GLOBAL (3.575 ms) : 3516, 3634
. : milestone, 3575,
profiling (2.099 ms) : 2080, 2117
. : milestone, 2099,
tracing (1.876 ms) : 1861, 1891
. : milestone, 1876,
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~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section baseline
no_agent (15.528 s) : 15528000, 15528000
. : milestone, 15528000,
appsec (14.873 s) : 14873000, 14873000
. : milestone, 14873000,
iast (18.616 s) : 18616000, 18616000
. : milestone, 18616000,
iast_GLOBAL (18.039 s) : 18039000, 18039000
. : milestone, 18039000,
profiling (14.959 s) : 14959000, 14959000
. : milestone, 14959000,
tracing (15.009 s) : 15009000, 15009000
. : milestone, 15009000,
section candidate
no_agent (15.367 s) : 15367000, 15367000
. : milestone, 15367000,
appsec (14.642 s) : 14642000, 14642000
. : milestone, 14642000,
iast (18.204 s) : 18204000, 18204000
. : milestone, 18204000,
iast_GLOBAL (17.991 s) : 17991000, 17991000
. : milestone, 17991000,
profiling (15.138 s) : 15138000, 15138000
. : milestone, 15138000,
tracing (15.172 s) : 15172000, 15172000
. : milestone, 15172000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~3e9621e752, baseline=1.62.0-SNAPSHOT~7c1b47b5d5
dateFormat X
axisFormat %s
section baseline
no_agent (1.491 ms) : 1479, 1503
. : milestone, 1491,
appsec (3.802 ms) : 3582, 4021
. : milestone, 3802,
iast (2.278 ms) : 2208, 2347
. : milestone, 2278,
iast_GLOBAL (2.323 ms) : 2253, 2393
. : milestone, 2323,
profiling (2.513 ms) : 2289, 2737
. : milestone, 2513,
tracing (2.09 ms) : 2036, 2144
. : milestone, 2090,
section candidate
no_agent (1.495 ms) : 1483, 1506
. : milestone, 1495,
appsec (3.837 ms) : 3612, 4062
. : milestone, 3837,
iast (2.286 ms) : 2216, 2355
. : milestone, 2286,
iast_GLOBAL (2.321 ms) : 2251, 2391
. : milestone, 2321,
profiling (2.097 ms) : 2042, 2151
. : milestone, 2097,
tracing (2.086 ms) : 2033, 2140
. : milestone, 2086,
|
…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.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
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.