Repository navigation
Add block_failure telemetry to jersey, resteasy, commons-fileupload and okhttp - #12519
Merged
Merged
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
Member
Author
|
@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.
Contributor
🟢 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. |
jandro996
marked this pull request as ready for review
September 16, 2026 11:58
jandro996
requested review from
claponcet,
manuel-alvarez-alvarez and
ygree
and removed request for
a team
September 16, 2026 11:58
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
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
claponcet
approved these changes
Sep 16, 2026
ygree
requested changes
Sep 16, 2026
Contributor
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.
Member
Author
|
@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. |
Member
Author
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. |
jandro996
added a commit
that referenced
this pull request
Sep 17, 2026
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).
jandro996
added a commit
that referenced
this pull request
Sep 17, 2026
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).
Contributor
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 18, 2026
jandro996
added a commit
that referenced
this pull request
Sep 22, 2026
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.
jandro996
added a commit
that referenced
this pull request
Oct 6, 2026
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.
jandro996
added a commit
that referenced
this pull request
Oct 6, 2026
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.
jandro996
added a commit
that referenced
this pull request
Oct 6, 2026
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).
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Oct 7, 2026
…12493) Add block-outcome telemetry for Tomcat blocking enforcement failures Report block_failure on appsec.waf.requests when Tomcat's blocking-commit path fails to enforce a decided block, mirroring the Netty implementation (#12316). Adds a shared TomcatBlockingHelper.tryCommitAndReport choke point used by all Tomcat/GlassFish blocking-commit call sites (5.5/6.0/7.0), and guards it against exceptions thrown by the registered BlockResponseFunction. Split TomcatBlockingHelper to fix muzzle failures in block-outcome telemetry Extract tryCommitAndReport()/reportBlockFailure(RequestContext) into a new catalina-independent BlockFailureReporter class in tomcat-common, since helperClassNames() injects the whole class and muzzle validates all its references against every declared library version - including the catalina-independent code path that doesn't need it. Fix spring-webmvc test fixture clobbering Tomcat's real BlockResponseFunction TestSpringBlockResponseFunction now delegates to the previously registered BlockResponseFunction (Tomcat's own, in production tests) instead of returning true/false unconditionally when RequestContextHolder is not yet populated by Spring, fixing 10 latestDepTest failures exposed by the tryCommitAndReport refactor. Fix: do not report block_failure when GlassFishBlockingHelper has no fallback response Guard the Servlet API fallback branch of tryBlock() so reportBlockFailure() only fires when a response was actually available to commit through (i.e. a commit was genuinely attempted). Previously it reported unconditionally whenever the fallback commit failed, including the brf == null / fallbackResp == null case where nothing was attempted at all. Fix: propagate commit exceptions instead of swallowing them in BlockFailureReporter Remove the catch(Exception) around brf.tryCommitBlockingResponse() in tryCommitAndReport() so it matches the Netty reference implementation (PR #12316), which only branches on the boolean return value. Several call sites (CommitActionInstrumentation in 5.5/7.0, ParsePartsInstrumentation, ParsedBodyParametersInstrumentation) rely on their enclosing advice's suppress = Throwable.class to abort the whole method when the commit throws, gating success-path side effects (closing the connection, injecting a BlockingException, marking the segment effectively blocked). Swallowing the exception inside tryCommitAndReport made those side effects run unconditionally even when nothing was actually committed to the client. Removing the catch restores the pre-existing control flow; exception-based commit failures are now a known gap not reported to block_failure telemetry, same as Netty. Addresses Codex review comment on PR #12493. Report block_failure for already-committed and write-failure branches in Tomcat Follow-up to PR review comment r4067498401: TomcatBlockingHelper.commitBlockingResponse() now reports block_failure when the response was already committed by something other than us, and when writing the blocking response throws. Safe against false positives from Tomcat's multiple per-request blocking evaluations via a new blockingResponseInitiated guard on TomcatBlockResponseFunction, mirroring Netty's existing pattern. Fix ambiguous tryCommitBlockingResponse mock after rebase 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. Restore manual helperClassNames override for muzzle-sensitive Tomcat7 modules Master's automatic bytecode-based helper discovery (PR #12649) does not resolve BlockFailureReporter as a dependency of GlassFishBlockingHelper or ParsePartsInstrumentation$ParsePartsAdvice, causing muzzle to fail for tomcat-catalina 7.0.0/9.0.1 and all glassfish-embedded-all versions with "Missing class datadog.trace.instrumentation.tomcat.BlockFailureReporter". Regenerate tomcat-common gradle.lockfile after third rebase onto master Picks up the testCompileClasspath/testRuntimeClasspath scope widening for javax.servlet:servlet-api, org.mockito:mockito-core (plus the new mockito-junit-jupiter dependency), and the tomcat 5.5.12 artifacts introduced upstream in #12747. Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Oct 7, 2026
…, Undertow and Akka-http (#12527) Add block-outcome telemetry to blocking response helpers - 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 Centralize block_failure reporting via BlockResponseFunction overload 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). Report block failure on unguarded exception paths in Grizzly, Jetty and 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 Guard remaining Undertow form-parse commit call and scope Grizzly failure 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. Fix block_failure reporting gaps in Undertow and Liberty exception paths - 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 Fix block_failure reporting gaps in Spring-webmvc blocking response paths - 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. Report block_failure on Liberty direct response-commit path in syncBufferEnter - 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 Merge branch 'master' into block-telemetry-3 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
zacharycmontoya
pushed a commit
that referenced
this pull request
Oct 7, 2026
…, Undertow and Akka-http (#12527) Add block-outcome telemetry to blocking response helpers - 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 Centralize block_failure reporting via BlockResponseFunction overload 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). Report block failure on unguarded exception paths in Grizzly, Jetty and 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 Guard remaining Undertow form-parse commit call and scope Grizzly failure 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. Fix block_failure reporting gaps in Undertow and Liberty exception paths - 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 Fix block_failure reporting gaps in Spring-webmvc blocking response paths - 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. Report block_failure on Liberty direct response-commit path in syncBufferEnter - 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 Merge branch 'master' into block-telemetry-3 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
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
3 participants
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
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.