Conversation
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. |
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.
1 participant
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.3,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
Known gap: several
BlockResponseFunctionimplementations cannot signal a genuine commit failureBlockResponseFunctionimplementationalways returns
trueafter a genuine commit attempt, regardless of whether the commit actuallysucceeded. The only reachable
falsevalues are pre-attempt guards (missing reflection handle, noactive span, wrong request/response type, process-wide init state), never a post-attempt outcome.
Undertow's async
exchange.dispatch(...)path is fire-and-forget, so there is no reachablefalseat all on that path (the real outcome is only logged vialog.warn). Practically, thereportBlockFailure()branch wired by this PR will rarely or never fire for these fourframeworks today.
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 these underlying implementations is out of scope for this PR - it only wires telemetry on
top of the existing
BlockResponseFunctioncontract and does not change any commit/blockingbehavior. 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.