Add server.request.body.filenames and files_content AppSec addresses for GlassFish/Payara#11267
Draft
Add server.request.body.filenames and files_content AppSec addresses for GlassFish/Payara#11267
Conversation
…ntation GlassFish 5 / Payara 5 does not have Request.parseParts() — instead Request.getParts() delegates entirely to org.apache.catalina.fileupload.Multipart.getParts(). That class uses its own ArrayList<Part> field (INVOKEVIRTUAL, not INVOKEINTERFACE) and calls a private initParts() instead of the Tomcat parseParts() that ParsePartsInstrumentation looks for. As a result, ParsePartsInstrumentation is a complete no-op on Payara: the method matcher finds nothing, and the bytecode visitor intercepts no Collection.add() calls. File names never reach the WAF even though the request is parsed without error. GlassFishMultipartInstrumentation targets org.apache.catalina.fileupload.Multipart.getParts() — a public method that exists only in GlassFish/Payara's web-core.jar and is therefore automatically skipped by ByteBuddy on standard Tomcat. After the method returns it iterates the Collection<Part> result and uses the existing ParameterCollector reflection helpers (getSubmittedFileName()) to extract file names, then fires requestFilesFilenames (and optionally requestFilesContent) callbacks exactly as ParsePartsInstrumentation does.
…/Payara multipart instrumentation
- Add `muzzleDirective() { return "glassfish"; }` to prevent the instrumentation
from being included in Tomcat muzzle tests, which would violate the `assertInverse`
constraint of the `from703` block and fail CI
- Consolidate double `getCallbackProvider()` call into a single variable
- Add glassfish muzzle block to build.gradle for CI validation against
glassfish-embedded-all artifacts
- Remove glassfish-embedded-all from testImplementation (its bundled Guava
conflicts with the test bootstrap classpath setup)
…monsFileUpload pattern Fetch both callbacks upfront before the collection loop, derive inspectContent from the captured contentCb reference, and add early exit when neither callback is registered. Eliminates a redundant getCallback() call and matches the pattern used in CommonsFileUploadAppSecInstrumentation (PR #11137).
…sFish multipart instrumentation On Java 11+ with GlassFish/Payara, reflective Method.invoke() from the injected ParameterCollector helper (unnamed module) to PartItem (GlassFish named module) fails with IllegalAccessException — setAccessible(true) is also blocked by the module system. Replace reflection-based approach with a direct cast to javax.servlet.http.Part inside the @Advice.OnMethodExit body. Because ByteBuddy inlines the advice into the target class (Multipart), it runs in the same classloader/module context as PartItem, so virtual dispatch through the Part interface works without any reflective access. Changes: - Rewrite GetPartsAdvice to cast each part to javax.servlet.http.Part and call getSubmittedFileName()/getInputStream()/getContentType() directly on the interface - Add compileOnly javax.servlet:javax.servlet-api:3.1.0 so getSubmittedFileName() (added in Servlet 3.1) is available at compile time; tomcat-catalina:7.0.4 ships only Servlet 3.0 - Add setAccessible(true) to ParameterCollector.resolveAndCache() (defensive; used by ParsePartsInstrumentation on Tomcat where reflection still works) - Remove helperClassNames() override — no helper injection needed for GlassFish path
…-item try/catch An uncaught exception from getSubmittedFileName() on part N would short-circuit the loop silently, leaving parts N+1..M unprocessed and the WAF with partial data. suppress=Throwable.class on @Advice.OnMethodExit only swallows the exception after the loop exits — it does not protect individual iterations. Wrap the entire per-part body in try/catch(Exception ignored) so a broken PartItem skips that part without affecting the remaining ones.
These calls were added during an attempt to fix the GlassFish IllegalAccessException via reflection, but that approach was superseded by inlining the advice directly into Multipart (which avoids reflection entirely). ParameterCollector is only used by ParsePartsInstrumentation on standard Tomcat, where reflection on ApplicationPart works without setAccessible(true) — as tests confirmed before this change.
Align with CommonsFileUploadAppSecInstrumentation: only populate the filenames list when filenamesCallback is actually registered. Previously the list was created and filled regardless, then silently dropped at dispatch time. No correctness impact, but avoids unnecessary ArrayList allocation and string copies when only the content callback is registered.
…strumentation TomcatServerInstrumentation is muzzled out for Payara's response type, so BlockResponseFunction is never registered. Add GlassFishBlockingHelper that commits the blocking response directly via Servlet API, extracting the HttpServletResponse from Multipart.request (private field) via reflection. The setAccessible call works because the advice is inlined into Multipart.getParts() — the same module as the field owner. Verified against system-tests APPSEC_BLOCKING with spring-boot-payara: - Test_Blocking_request_body_filenames: passes (was missing_feature) - Test_Blocking_request_body_files_content: passes (was missing_feature)
…rumentation - Use try-with-resources for OutputStream in GlassFishBlockingHelper - Cache Config limit values as static finals in GlassFishBlockingHelper to avoid per-request Config.get() calls (consistent with ParameterCollector) - Remove Config import from GlassFishMultipartInstrumentation (now reads limits from GlassFishBlockingHelper static fields) - Add comment to skipVersions explaining the jakarta namespace reason - Add assertInverse to glassfish muzzle block for defensive CI coverage
jandro996
changed the title
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
GlassFishMultipartInstrumentation, which hooksorg.apache.catalina.fileupload.Multipart.getParts()— the GlassFish/Payara-specific multipart implementation — to publish theserver.request.body.filenamesandserver.request.body.files_contentAppSec IG events.GlassFishBlockingHelperto commit blocking responses directly via the Servlet API. This is needed becauseTomcatServerInstrumentationis muzzled out for Payara's internal response type (PECoyoteResponse), soBlockResponseFunctionis never registered on Payara. The helper extracts theHttpServletResponsefromMultipart.request(private field) via reflection — which works because the advice is inlined intoMultipart.getParts(), running in the same module as the field owner.glassfishmuzzle block inbuild.gradle(withassertInverse = true) againstorg.glassfish.main.extras:glassfish-embedded-all:[4.0,]. GlassFish 6.1.0 is excluded because it migrated to thejakarta.*namespace.compileOnly javax.servlet:javax.servlet-api:3.1.0to compile againstPart.getSubmittedFileName(), which is Servlet 3.1 and absent from thetomcat-catalina:7.0.4dependency that covers Servlet 3.0 only.muzzleDirective() = "glassfish"to exclude this instrumentation from the existing Tomcat muzzle tests (from703hasassertInverse = true).Design notes
PartItem(the concretePartimplementation in GlassFish) lives in a named module under Java 11+. ReflectiveMethod.invoke()from an unnamed-module helper would fail withIllegalAccessException. Instead the advice is kept inline (noParameterCollectorhelper) and each part is accessed through thejavax.servlet.http.Partinterface via a plain cast — dispatch goes through the interface, no reflection on GlassFish internal types.org.apache.catalina.fileupload.Multipartdoes not exist in standard Tomcat, ByteBuddy skips the instrumentation entirely on non-GlassFish containers with no extra classloader check needed.Motivation
Solves APPSEC-61873.
GlassFish/Payara does not call
Request.parseParts()(the hook used byParsePartsInstrumentation) — it routes multipart parsing throughorg.apache.catalina.fileupload.Multipart.getParts()instead. Without this instrumentation, uploaded filenames and file contents are never reported to the WAF and attacks viaserver.request.body.filenames/server.request.body.files_contentrules are not detected on Payara.Additional Notes
Verified with system-tests
APPSEC_BLOCKINGscenario againstspring-boot-payara(Payara Micro 5.2022.1, Java 11):Test_Blocking_request_body_filenames: PASSTest_Blocking_request_body_files_content: PASSThe system-tests manifest for
DataDog/system-testsis updated in a companion PR.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.