Conversation
…gration PR #12601 (APPSEC-70201) migrates ~59 AppSec blocking call sites from the (TraceSegment, RBA) overload to the (RequestContext, RBA) default added by #12519. This is a mechanical no-behavior-change migration for nearly all sites, but it currently lacks test coverage in several modules, so a silent regression there would only surface as a production incident. Adds unit and integration tests that pin the current fail-closed blocking behavior across the untested surface, so any regression introduced by the overload migration is caught by CI instead of production: - ProcessImplInstrumentationHelpers (CMDI/SHI RASP) - FileIORaspHelper (LFI RASP) and URLSinkCallSite (SSRF RASP) - AppSecEventTracker.dispatch() - StoredCharBody.maybeNotifyAndBlock() (migrated from Groovy to Java) - BlockResponseFunction's (TraceSegment, RBA) overload directly - play-appsec-common PathExtractionHelpers (deliberate tripwire: documents the fail-closed behavior expected to flip after #12601's rebase) - Play 2.5/2.6 ResultsStatus/StatusHeader advices and BodyParserHelpers - vertx-web RoutingContextSessionAdvice, FileUploadHelper and the RoutingContextJsonResponseAdvice BODY_JSON integration path - jax-rs-annotations-2.0 / jakarta-rs-annotations-3.0 MessageBodyWriter - Ratpack JsonRendererAdvice BODY_JSON integration path Also fixes FileIORaspHelperBlockingTest flakiness caused by pre-existing Groovy specs in java-io-1.8 leaking a mocked FileIORaspHelper.INSTANCE across the shared test JVM: the new test now saves and restores the real singleton around each test case via reflection.
jandro996
added
type: refactoring
comp: asm waf
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ 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 comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…S pattern - RoutingContextSessionAdviceTest -> RoutingContextSessionAdviceAppSecTest (vertx-web 3.4, 4.0) - FileUploadHelperTest -> FileUploadHelperAppSecTest (vertx-web 3.4, 4.0, 5.0) - MessageBodyWriterAdviceTest -> MessageBodyWriterAdviceAppSecTest (jakarta-rs-annotations-3.0, jax-rs-annotations-2.0)
Move the "test blocking on json response body" feature out of RatpackHttpServerTest, VertxHttpServerForkedTest (4.0, 5.0) and PlayServerTest (2.5, 2.6) into new *AppSec* test classes, so they match the CODEOWNERS *AppSec* pattern without editing CODEOWNERS or renaming the shared, general-purpose contract-test files. The new classes extend the original test class to reuse its server bootstrap and helpers, and are annotated with the new @ExcludeInheritedFeatures Spock extension (utils/test-utils) so the ~22 inherited generic contract tests are excluded at discovery time instead of re-running.
jandro996
added
the
tag: override groovy enforcement
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ 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". |
- Add cleanup: blocks restoring original IG callbacks after reset()+registerCallback() in play-appsec-2.5/2.6 and vertx-web-3.4/4.0/5.0 AppSec tests, preventing state leak across tests in the same forked JVM - Fix FQN inline okhttp3.Response -> import + Response in Play AppSec tests - Add missing blank line between Spock blocks for consistency
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ 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". |
- Rename local `request` variable to `req` in play-appsec-2.5 and play-appsec-2.6 PlayServerAppSecTest's multipart file upload test, which shadowed the inherited `request()` method once Spock's `where:` block hoisted the local variable declaration.
Local variable `request` shadowed the inherited request() method, causing Groovy to desugar the call to request.call(args) on the null local var. Unlike the earlier play-appsec fix, the trigger here is a cleanup: block (not a where: block): sibling tests without cleanup: using the same def request = request(...) pattern were unaffected. Renamed to req in vertx-web-3.4/4.0/5.0.
This branch has not been deployed
1 participant
What Does This Do
Adds unit/integration test coverage for AppSec blocking behavior that today has no test guarding it, so PR #12601's migration from the
tryCommitBlockingResponse(TraceSegment, RBA)overload to the(RequestContext, RBA)default cannot silently regress it.ProcessImplInstrumentationHelpersBlockingTest— pins the fail-closed behavior ofProcessImplInstrumentationHelpers.cmdiRaspCheck/shiRaspCheck(CMDI/SHI RASP), with and without aBlockResponseFunction.FileIORaspHelperBlockingTest— same coverage forFileIORaspHelper's entry points (LFI RASP). Also fixes test-order flakiness: pre-existing Groovy specs injava-io-1.8replaceFileIORaspHelper.INSTANCEwith a mock and never restore it, leaking it into whichever test runs next in the same JVM. The new test now saves/restores the real singleton via reflection around each case.URLSinkCallSiteBlockingTest— same coverage forURLSinkCallSite(SSRF RASP).AppSecEventTrackerTest— new case assertingdispatch()callstryCommitBlockingResponseand throws when a realBlockResponseFunctionis configured.StoredCharBodyTest— migrated from Groovy to Java (JUnit 5) and extended with a case coveringmaybeNotifyAndBlock()against a realBlockResponseFunction.BlockResponseFunctionTest— direct coverage of the(TraceSegment, RBA)overload's status code / content type / extra headers / security response id mapping.play-appsec-common'sPathExtractionHelpersTest(new test module) pins the current fail-closed behavior as a deliberate tripwire: it documents the behavior expected to flip once Migrate AppSec blocking glue to the RequestContext tryCommitBlockingResponse overload #12601 rebases onto this branch.ResultsStatus/StatusHeaderSendJsonadvices (ignoring commit result today) andBodyParserHelpers(filenames/file-content/XML blocking), plus aPlayServerTestx-block-response:bodyintegration case.RoutingContextSessionAdvice(including the fail-open path without a BRF) andFileUploadHelper.commitBlockingResponse, plus aRoutingContextJsonResponseAdviceBODY_JSON integration case (4.0/5.0).MessageBodyWriteradvice.JsonRendererAdviceBODY_JSON integration case.Motivation
#12601 migrates ~59 AppSec blocking call sites onto the
(RequestContext, RBA)overload. The migration is mechanically a no-behavior-change for nearly all of them, but several of the touched modules had no test asserting the blocking behavior at all, so a mistake in that migration would only be caught in production. This lands ahead of #12601 so CI catches any regression from that overload migration.Additional Notes
No behavior change in production code in this PR: test-only, except for the Groovy→Java migration of
StoredCharBodyTestand the newplay-appsec-commontest source set (build.gradle/lockfile additions to add amockitotest dependency where one didn't exist).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-70201
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.