Add server.request.body.filenames and files_content for GlassFish/Payara#11267
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 26 commits intoMay 21, 2026
Merged
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
…ckingException for Payara blocking ByteBuddy inlines advice into Multipart.getParts() (package org.apache.catalina.fileupload), which is a different package than GlassFishBlockingHelper. Package-private static fields caused IllegalAccessError at runtime crashing the Payara container. BlockingException thrown from getParts() propagated through Payara's Grizzly pipeline and caused an ungraceful shutdown. Replace with parts = Collections.emptyList() so the response is committed (via commitBlocking fallback) and the controller skips processing the parts.
…assFishBlockingHelper Extract the repeated brf/fallback blocking pattern from the GlassFish multipart advice into GlassFishBlockingHelper.tryBlock(). This eliminates the duplication between the filenames and content blocking paths, and fixes the ordering issue where blocked=true was assigned after effectivelyBlocked() — if effectivelyBlocked() threw, blocked would stay false and the content callback would fire after filenames already blocked. tryBlock() uses a two-phase try/catch: the outer block handles commit failures (returning false), while the inner block wraps effectivelyBlocked() so a thrown exception does not suppress the true return value when the response was already committed. Add GlassFishBlockingHelperTest covering all branches of commitBlocking() and tryBlock().
…elper Extract the part-iteration loop and IG callback dispatch from the advice into a new GlassFishBlockingHelper.processPartsAndBlock() static method. The advice now contains only the inlined reflection block (which must stay inlined due to Java module-system constraints) and a single call to the helper. Unit tests added for processPartsAndBlock(): form-field skip, empty filename, normal filename, content reading, maxFiles limit, getInputStream failure fallback, sequential callback ordering (filenames blocks → content not fired), content callback blocks → returns true, non-Part object skip.
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". |
jandro996
changed the title
jandro996
added
type: enhancement
![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: f6f2feb51c
ℹ️ 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".
…ation Use @advice.FieldValue("request") instead of getDeclaredField+setAccessible to access Multipart.request — ByteBuddy inlines direct field access, no reflection needed for the field itself. getResponse() still uses reflection since the return type is a Payara-internal class. Add explanatory comments to all catch(Exception ignored) blocks in both the advice and GlassFishBlockingHelper.
Typing the field to org.apache.catalina.connector.Request allows calling getResponse() directly, removing the Method reflection block entirely.
org.apache.catalina.connector.Request is not exported by glassfish-embedded-all, so typing the field parameter to that class adds a bytecode reference that muzzle cannot resolve, causing the AssertPass check for GlassFish 4.0 to fail. Keep @advice.FieldValue (avoids setAccessible across module boundaries) but type as Object. getResponse() stays reflective for the same reason.
Type @advice.FieldValue("request") as org.apache.catalina.connector.Request instead of Object. Pass catRequest directly to GlassFishBlockingHelper, which casts it to the org.apache.catalina.Request Catalina interface (available in GlassFish/Payara via HttpRequest) to reach HttpServletResponse without any Method.invoke() reflection. The org.apache.catalina.Request.getResponse() method returns org.apache.catalina.Response with a stable JVM descriptor shared between Tomcat and GlassFish, so no muzzle descriptor mismatch occurs. This is not possible calling connector.Request.getResponse() directly because Tomcat returns connector.Response while GlassFish returns org.apache.catalina.Response. Add glassfish-embedded-all:4.0 as compileOnly dependency so that org.apache.catalina.Request and org.apache.catalina.Response — GlassFish-specific interfaces removed from Tomcat 7.x public API — are available at compile time. Also add missing comment to the empty catch for effectivelyBlocked().
Using the Catalina Request interface (instead of connector.Request) as the @advice.FieldValue parameter type lets muzzle verify that the instrumented field type (connector.Request) still implements that interface in every supported GlassFish version, catching any future breakage at build time. The interface's getResponse() descriptor is stable across Tomcat and GlassFish, which also removes the need for the instanceof cast in the helper. Add glassfish-embedded-all:4.0 as testCompileOnly so the test sources can resolve the interface type.
This comment has been minimized.
This comment has been minimized.
Contributor
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 8649452320257527611 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Contributor
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
alejandro.gonzalez/APPSEC-61873-payara
branch
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
GlassFishMultipartInstrumentation—@OnMethodExitadvice onorg.apache.catalina.fileupload.Multipart.getParts(). Reads therequestfield via@Advice.FieldValuetyped asorg.apache.catalina.connector.Request(the exact declared field type) and delegates toGlassFishBlockingHelper.GlassFishBlockingHelper— helper injected into the app classloader:processPartsAndBlock(): iteratesjavax.servlet.http.Partobjects via interface dispatch, collects filenames and file contents, firesrequestFilesFilenamesandrequestFilesContentIG callbacks. ExtractsHttpServletResponsefor Payara via theorg.apache.catalina.RequestCatalina interface (see Additional Notes).tryBlock(): commits blocking response viaBlockResponseFunctionor Servlet API fallback, then marks the trace segment as effectively blocked.commitBlocking(): Servlet API fallback — writes the 403 response directly viaHttpServletResponsefor Payara, whereBlockResponseFunctionis never registered.Build
glassfishfororg.glassfish.main.extras:glassfish-embedded-all:[4.0, 6.1.0)withassertInverse = true. Upper bound excludes GlassFish 6.1.0+ (migrated tojakarta.*namespace).compileOnly javax.servlet:javax.servlet-api:3.1.0to accessPart.getSubmittedFileName()(Servlet 3.1, not available intomcat-catalina:7.0.4).compileOnly org.glassfish.main.extras:glassfish-embedded-all:4.0to accessorg.apache.catalina.Requestandorg.apache.catalina.ResponseCatalina interfaces, which Tomcat 7.x removed from its published API (see Additional Notes).Tests
Unit tests for all methods in
GlassFishBlockingHelperTest: form-field skip (null filename), empty filename, normal filename collection, content reading, max-files limit,getInputStream()failure fallback, sequential blocking (filenames blocks → content not fired), content blocking, non-Partobject skip,effectivelyBlocked()throws → still returns true.Motivation
GlassFish/Payara does not use Tomcat's
Request.parseParts()— it delegates multipart parsing toorg.apache.catalina.fileupload.Multipart.getParts(). Without this instrumentation, theserver.request.body.filenamesandserver.request.body.files_contentWAF addresses are never populated for GlassFish/Payara deployments.Jira ticket: APPSEC-61873
Additional Notes
Why
@Advice.FieldValueis typed asconnector.RequestandgetResponse()is called through theorg.apache.catalina.Requestinterface:Multipart.requestis declared asfinal org.apache.catalina.connector.Request request, so typing the advice parameter to match eliminates theObjectcast. However,connector.Request.getResponse()cannot be called directly: in Tomcat the method returnsorg.apache.catalina.connector.Response(descriptor()Lorg/apache/catalina/connector/Response;), while in GlassFish it returnsorg.apache.catalina.Response(descriptor()Lorg/apache/catalina/Response;). The differing JVM descriptors would cause muzzle to reject the reference when checking againstglassfish-embedded-all.The solution is to call
getResponse()through theorg.apache.catalina.RequestCatalina interface, whosegetResponse()consistently returnsorg.apache.catalina.Responsein both containers.connector.Requestin GlassFish/Payara implements this interface (viaHttpRequest), so theinstanceofcast inGlassFishBlockingHelperis always true at runtime.Why
glassfish-embedded-all:4.0is needed ascompileOnly:org.apache.catalina.Requestandorg.apache.catalina.Responseare GlassFish-specific: Tomcat 7.x removed them from the publishedtomcat-catalinaartifact. Addingglassfish-embedded-all:4.0ascompileOnlymakes them available at compile time without affecting the produced JAR.Why a Servlet API fallback for blocking:
TomcatServerInstrumentationinstrumentsCoyoteAdapter.postParseRequest(), whose 4th argument is typed asorg.apache.catalina.connector.Response. In Payara the equivalent type isPECoyoteResponse, so the method matcher never fires andBlockResponseFunctionis never registered.GlassFishBlockingHelper.commitBlocking()writes the 403 response directly viaHttpServletResponseas a fallback.Muzzle range
[4.0, 6.1.0):GlassFish 6.1.0+ migrated to the
jakarta.*namespace. This instrumentation referencesjavax.servlet.http.Part, so it must be excluded from Jakarta-namespace versions.Verified on Payara Micro 5.2022.1 via system-tests
APPSEC_BLOCKINGscenario withspring-boot-payaraweblog:Test_Blocking_request_body_filenamesandTest_Blocking_request_body_files_contentboth pass.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 issueNote: 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.