Add server.request.body.filenames and files_content for GlassFish/Payara#11267
Draft
Add server.request.body.filenames and files_content 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
…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
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
Instruments
org.apache.catalina.fileupload.Multipart.getParts()in GlassFish/Payara to report uploaded file names and file contents to the AppSec WAF via therequestFilesFilenamesandrequestFilesContentIG events.New classes
GlassFishMultipartInstrumentation—@OnMethodExitadvice onMultipart.getParts(). Collectsjavax.servlet.http.Partdata via interface dispatch (no reflection), extracts the fallbackHttpServletResponsefromMultipart.requestviasetAccessible(works because the advice is inlined into the same Java module), then delegates toGlassFishBlockingHelper.GlassFishBlockingHelper— helper injected into the app classloader that centralizes:processPartsAndBlock()(part iteration, filename/content collection, IG callback dispatch),tryBlock()(commits blocking response viaBlockResponseFunctionor Servlet API fallback, callseffectivelyBlocked()best-effort),commitBlocking()(Servlet API fallback for when noBlockResponseFunctionis registered).Build
glassfishfororg.glassfish.main.extras:glassfish-embedded-all:[4.0, 6.1.0)withassertInverse = truecompileOnly javax.servlet:javax.servlet-api:3.1.0to accessPart.getSubmittedFileName()(Servlet 3.1, not intomcat-catalina:7.0.4)javax.servlet-api:3.1.0, mockitoTests
Unit tests for all three 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 interface dispatch instead of reflection for
PartItemmethods:The Java module system (Java 9+) blocks reflective
Method.invoke()from an unnamed module (the injected helper) intoPartItem(a named GlassFish module).setAccessible(true)also fails silently. The fix is to cast throughjavax.servlet.http.Part, whichPartItemimplements — interface dispatch is not restricted by the module system.Why
Collections.emptyList()instead ofBlockingException:BlockingExceptionpropagating through Payara's Grizzly pipeline causes an ungraceful container shutdown. Returning an empty parts list to the caller achieves the same effect (the controller receives nothing to process) without crashing the server.Why a Servlet API fallback for blocking:
TomcatServerInstrumentationis muzzled for Payara because Payara's response type (PECoyoteResponse) is notorg.apache.catalina.connector.Response. As a resultBlockResponseFunctionis never registered, andreqCtx.getBlockResponseFunction()always returns null in Payara.GlassFishBlockingHelper.commitBlocking()writes the 403 response directly viaHttpServletResponse(Servlet API), extracted fromMultipart.requestvia reflection that works because the advice is inlined into the same module.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.