Add block_failure telemetry to jersey, resteasy, commons-fileupload and okhttp - #12519
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
type: feature
|
@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. Chef's kiss. 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. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Each changed call site reports a block failure only when the response function exists and the commit returns false. The handling is consistent across all changed integrations.
🤖 Datadog Autotest · Commit 6320ac3 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
It seems there is an opportunity to improve these changes by reducing code duplication.
Specifically, all calls to the tryCommitBlockingResponse method result in a call to reportBlockFailure if blocking fails. This raises the question: Should reportBlockFailure be part of tryCommitBlockingResponse instead?
This would also make the tests cleaner. There would be no need to verify that it's called in all cases where tryCommitBlockingResponse returns false. We would only need to verify that it's called by tryCommitBlockingResponse when blocking isn't possible, assuming that the existing tests already verify that tryCommitBlockingResponse is wired correctly.
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.
|
@codex review |
|
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". |
🛡️ 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. |
Good catch, and I agree. I'd originally kept this inlined at every call site on purpose to avoid introducing a new cross-module helper class, since a shared helper injected across instrumentation modules has classloader/muzzle implications I wanted to avoid for a telemetry-only change. |
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).
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).
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Master (#12519) added a default BlockResponseFunction.tryCommitBlockingResponse(RequestContext, RequestBlockingAction) overload alongside the existing TraceSegment-based one, making bare any() matchers ambiguous. Disambiguate with any(TraceSegment.class), matching the overload GlassFishBlockingHelper actually invokes via BlockFailureReporter.
3 participants
What Does This Do
Reports a
block_failuretag on theappsec.waf.requestsmetric whenever a WAF-decidedblock silently fails to actually commit, for jersey (2.x/3.x), resteasy (3.x),
commons-fileupload, and okhttp (client-side RASP/SSRF). This is Group 2b of a broader
effort covering all frameworks with blocking support.
The check is centralized in a new default method on
BlockResponseFunction:tryCommitBlockingResponse(RequestContext, RequestBlockingAction). It attempts the commit viathe existing
(TraceSegment, RequestBlockingAction)overload and, onfalse, resolvesAppSecContextfromRequestContextSlot.APPSECdata (instanceof AppSecContextcheck, nevercast directly to
AppSecRequestContext) and callsreportBlockFailure(). Any exception thrownby the underlying commit still propagates unchanged. The existing 2-arg overload is left
untouched so call sites outside this task's scope (RASP) keep their current behavior.
New callsites
All 13 Group 2b call sites now call the new overload with a single line,
brf.tryCommitBlockingResponse(ctx, rba):jersey2/MultiPartHelper#tryBlock(),MessageBodyReaderInstrumentation,UriRoutingContextInstrumentationjersey3/MultiPartHelper#tryBlock(),MessageBodyReaderInstrumentationresteasy/MultipartHelper#tryBlock(),DecodedFormParametersInstrumentation,MessageBodyReaderInvocationInstrumentation,MethodExpressionInstrumentationcommons-fileupload/CommonsFileUploadAppSecInstrumentation(both the filenames and file content call sites)okhttp2/AppSecInterceptor#publish(),okhttp3/AppSecInterceptor#publish()netty/NettyMultipartHelper#tryBlock()(already merged in #12316) was updated to the sameoverload for consistency, since it implements the identical pattern.
No new cross-module helper class was introduced and no
helperClassNames()changes were madeanywhere:
BlockResponseFunctionalready lives ininternal-apiand every call site alreadyinvoked its pre-existing default method via ordinary virtual dispatch, so adding a second
overload there carries none of the classloader/muzzle risk a new shared helper class would.
Tests
BlockResponseFunctionTest(hand-written test doubles, no Mockito) coversthe centralized commit-and-report logic directly.
simplified to verify wiring only, i.e. that
tryCommitBlockingResponse(ctx, rba)is called -the commit/report behavior itself is now covered once, centrally.
GlassFishBlockingHelperTest(tomcat-appsec-7.0, unrelated to this task'sscope) used an untyped Mockito
any()matcher that became ambiguous onceBlockResponseFunctiongained a second overload; retyped toany(TraceSegment.class).Motivation
Part of the block-outcome telemetry effort: without this, a block decision that silently fails to
commit (e.g. response already committed by the container) is indistinguishable from a successful
block in the
appsec.waf.requestsmetric, hiding real blocking failures from monitoring.The centralization itself was requested during review (see PR discussion): every call site
funneled into
tryCommitBlockingResponse, soreportBlockFailurebelongs there instead of beingduplicated at each site.
Additional Notes
None.
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.