Conversation
…t (Group 2b) Group 2b: jersey (2.x/3.x), resteasy (3.x), commons-fileupload, and okhttp (client-side RASP/SSRF). Each call site checks the boolean return of BlockResponseFunction#tryCommitBlockingResponse(...) and, on false, reports the failure via AppSecContext#reportBlockFailure() (instanceof-checked, never cast to AppSecRequestContext). Only reported when a BlockResponseFunction was actually present. Adds JUnit5 unit tests for the extractable call sites (jersey2/3 and resteasy MultiPartHelper/MultipartHelper.tryBlock(), okhttp2/3 AppSecInterceptor) and extends existing Groovy/Spock instrumentation tests for the inline @advice sites that cannot be unit tested in isolation.
jandro996
added
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. |
![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: ff0f114442
ℹ️ 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".
🟢 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. |
Address reviewer feedback (review #5228047427) by moving the commit-and-report logic into a new default overload, tryCommitBlockingResponse(RequestContext, RequestBlockingAction), instead of duplicating the check at each of the 13 call sites plus Netty's. The existing (TraceSegment, RequestBlockingAction) overload is left untouched so out-of-scope RASP call sites keep their current behavior. Per-call-site tests now verify wiring only; the commit-and-report logic is covered once in the new BlockResponseFunctionTest.
…erTest The new BlockResponseFunction#tryCommitBlockingResponse(RequestContext, RequestBlockingAction) overload added in this branch makes the untyped any() matcher ambiguous against the pre-existing (TraceSegment, RequestBlockingAction) overload this test actually exercises.
ff0f114 to
e1564ba
Compare
- Wire reportBlockFailure() at every BlockResponseFunction.tryCommitBlockingResponse call site across akka-http, grizzly, jetty (appsec + server), liberty, spring-webmvc and undertow, resolving AppSecContext via RequestContextSlot.APPSEC instead of casting directly to AppSecRequestContext - Add UnmarshallerHelpersBlockFailureTest covering all branches of UnmarshallerHelpers.tryBlock (block committed, block failed, no BRF, foreign/null AppSec slot) - Widen UnmarshallerHelpers.tryBlock visibility via @VisibleForTesting instead of a package-private comment
Replace the repeated inline pattern (commit blocking response, then manually check AppSecContext and call reportBlockFailure() on failure) with the new BlockResponseFunction.tryCommitBlockingResponse(RequestContext, Flow.Action.RequestBlockingAction) default overload added in block-telemetry-2b (#12519). That overload already performs the AppSecContext lookup and reportBlockFailure() call internally, so call sites only need to pass the RequestContext instead of the TraceSegment. Applies to the 37 call sites identified as mechanically substitutable: akka-http-10.0, grizzly-http-2.3.20, jetty-appsec (7.0, 8.1.3, 9.2, 9.3, 9.4, 11.0), jetty-server (7.0, 7.6), liberty (20.0, 23.0), spring-webmvc (3.1, 6.0), and undertow-2.0. Excludes GrizzlyBlockingHelper (grizzly-2.0, manual reflection-based commit) and jetty-server-9.0's JettyCommitResponseInstrumentation (compound res && _committed.get() condition, not mechanically equivalent to the new overload's plain boolean return).
…nd Undertow - Move Grizzly's post-commit span/listener calls inside the try/catch so failures there also trigger reportBlockFailure() - Add missing !success reportBlockFailure() branch to Jetty's before() in 9.0.4/9.3/9.4.21/10.0 - Wrap Undertow's tryCommitBlockingResponse calls in FormDataContentHelper to catch exceptions swallowed by the advice's suppress=Throwable.class
8855f84 to
2b99081
Compare
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b99081893
ℹ️ 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".
…lure reporting to commit errors - FormDataParserInstrumentation (Undertow): wrap the doParse blocking-commit call with FormDataContentHelper.tryCommitBlockingResponse so a synchronous exception is still reported as a block failure instead of being swallowed by the advice's suppress = Throwable.class. - GrizzlyBlockingHelper: split post-commit finalization (effectivelyBlocked, SpanClosingListener.onAfterService) into its own try/catch so a failure there no longer reports block_failure for a response that was already committed successfully.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2cbed27d7
ℹ️ 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".
- Check the boolean return of FormDataContentHelper.tryCommitBlockingResponse in Undertow's FormDataParserInstrumentation before throwing BlockingException, matching the existing MultiPartUploadHandlerInstrumentation pattern - Add LibertyBlockingHelper.tryCommitBlockingResponse wrapper in liberty-20.0 and liberty-23.0 to report block_failure when the commit itself throws (mirrors Undertow's FormDataContentHelper contract) - Route ParsePostDataInstrumentation, ParseParametersInstrumentation and GetPartsInstrumentation (both Liberty modules) through the new wrapper and check its return value before treating the request as blocked
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f02c3d270
ℹ️ 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".
🛡️ 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. |
…aths - Add SpringBlockingHelper (spring-webmvc-3.1 and spring-webmvc-6.0) wrapping BlockResponseFunction#tryCommitBlockingResponse to guarantee reportBlockFailure() is invoked even when the commit call throws. - Route all 6 blocking call sites (HttpMessageConverter read/write, RequestMappingInfoHandlerMapping#handleMatch, UriTemplateVariablesHandlerInterceptor#preHandle, x2 modules) through the helper and only set/throw BlockingException when the commit actually succeeds. - Fix HttpMessageConverterInstrumentationTest assertions that expected the old, unconditional-throw behavior.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
🛡️ 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. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Blocking-response commit paths now report false results and caught exceptions. Successful blocking behavior stays unchanged.
🤖 Datadog Autotest · Commit 79478d2 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79478d24fd
ℹ️ 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".
…fferEnter - LibertyBlockingHelper (liberty-20.0, liberty-23.0): report the block failure when thiz.reinit()/thiz.finishResponseMessage() throws during syncBufferEnter, matching the existing wrapper's behavior for the parameter/body callback path
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
🛡️ 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. |
This branch has not been deployed
2 participants
What Does This Do
Wires block-outcome telemetry (
AppSecContext.reportBlockFailure()) into the six remainingframeworks: Grizzly, Jetty, Liberty, Spring-webmvc, Undertow, and Akka-http. Follows the canonical
pattern already established for Netty (#12316) and Tomcat (#12493): resolve the
BlockResponseFunction, attempt the commit throughtryCommitBlockingResponse(...), and onfalse(or on exception) report the failure viaAppSecContext- resolved throughRequestContext#getData(RequestContextSlot.APPSEC)+ aninstanceofcheck, never a direct cast toAppSecRequestContext.tryCommitBlockingResponsecall site across:grizzly-2.0,grizzly-http-2.3.20,jetty-appsec-{7.0,8.1.3,9.2,9.3,9.4,11.0},jetty-server-{7.0,7.6,9.0,9.0.4,9.3,9.4.21,10.0,11.0},liberty-{20.0,23.0},spring-webmvc-{3.1,6.0},undertow-2.0andakka-http-10.0UnmarshallerHelpersBlockFailureTest(JUnit 5) covering all branches ofUnmarshallerHelpers.tryBlock: block committed, block failed, noBlockResponseFunction,foreign/null AppSec slot
UnmarshallerHelpers.tryBlockvisibility via@VisibleForTestinginstead of apackage-private comment, matching the existing repo convention
Follow-up: try/catch wrapper helpers for Undertow, Liberty and Spring-webmvc
A round of Codex review on this PR surfaced call sites where an exception thrown while committing
the blocking response was swallowed by the advice's
suppress = Throwable.classbefore it everreached
reportBlockFailure()- so a genuine commit failure went unreported whenever the commititself threw, as opposed to returning
falsecleanly. Fixed by introducing a small per-modulewrapper (
FormDataContentHelperin Undertow,LibertyBlockingHelperin Liberty,SpringBlockingHelperin Spring-webmvc x2) that callstryCommitBlockingResponse(RequestContext, RequestBlockingAction)inside an explicittry/catch, reports the failure on either a caughtexception or a clean
false, and returns a boolean the caller can trust. Spring-webmvc'sHttpMessageConverterInstrumentationTestwas updated to assertnotThrown(BlockingException)(notthrown) on a clean commit failure, since the advice now correctly skips throwing whentryCommitBlockingResponsereturnsfalse.Known gap: several
BlockResponseFunctionimplementations still cannot signal a genuine commit failurefound call sites (Grizzly's
ParsedBodyParametersInstrumentation, six files underjetty-appsec-{7.0,9.2,9.3,9.4,11.0}) that discard the boolean returned bytryCommitBlockingResponseand unconditionally report failure regardless of the actual outcome,plus a
JettyBlockingHelper.block()that catchesThrowableand still returnstrueunconditionally. None of this has been fixed in this PR - it needs its own follow-up under
APPSEC-62696, tracked separately from the try/catch wrapper fix above.
BlockResponseFunctionimplementation still always returnstrueafter a genuine commit attempt regardless of whether the commit actually succeeded (theonly reachable
falsevalues are pre-attempt guards: missing reflection handle, no active span,wrong request/response type, process-wide init state). This PR's wrapper helpers do make
reportBlockFailure()fire correctly on any exception thrown during the commit attempt, but acommit that returns cleanly without throwing is still assumed to have succeeded.
AkkaBlockResponseFunction.tryCommitBlockingResponsereturningtruemeans theRequestBlockingActionwas stashed for later use, not that the HTTP response was committed. Theactual response substitution happens out-of-band via
maybeCreateAlternativeResponse(). Thisreturn value is structurally incapable of signaling a genuine commit failure.
Fixing the remaining Grizzly/Jetty gaps and the Undertow/Liberty/Akka-http structural limitations is
out of scope for this PR. It should be tracked as a separate tech-debt follow-up under APPSEC-62696.
Motivation
Part of APPSEC-62696: block-outcome telemetry, extending the pattern already shipped for Netty and
Tomcat to the remaining instrumented servers.
Additional Notes
reportBlockFailure()wiring duplicated across the six frameworks isintentional: each module has its own
BlockResponseFunction, classloader scope and bytecodediscriminator, so no cross-module helper is introduced by design.
instanceofcheck plus a conditional method call on the existing commit-attempt path, not a new hot path.
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-62696
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.