Conversation
MediaType is a general-purpose media type parsing utility with no AppSec specific logic, so it does not belong under datadog.trace.api.appsec.
…karta-rs annotations
Switch every ratpack-1.5 blocking call site to the BlockResponseFunction.tryCommitBlockingResponse(RequestContext, RequestBlockingAction) overload so a blocking response that cannot be committed is reported via AppSecContext.reportBlockFailure(). effectivelyBlocked() is not called at these call sites: ratpack blocks through Netty's NettyBlockResponseFunction, whose BlockingResponseHandler already marks the trace segment. Enable the shared HttpServerTest block failure coverage for ratpack using the PATH_PARAMS variant, which exercises PathBindingPublishingHandler (the request header blocking point belongs to the netty instrumentation).
Switch the four blocking call sites in internal-api (StoredCharBody request-body block, AppSecEventTracker#dispatch, and the CMDI/SHI RASP checks in ProcessImplInstrumentationHelpers) from BlockResponseFunction#tryCommitBlockingResponse(TraceSegment, ...) to the (RequestContext, RequestBlockingAction) overload, so a silently failed commit is reported through AppSecContext#reportBlockFailure(). TraceSegment#effectivelyBlocked() is now called explicitly at each call site when the commit succeeds, following the jersey/resteasy pattern. StoredCharBody now throws BlockingException only when a BlockResponseFunction is present, matching the WAF request-body pattern used by jersey. The RASP checks keep throwing unconditionally: aborting the exec/shell attempt must not depend on being able to commit a blocking response.
Migrate every Play blocking call site to BlockResponseFunction#tryCommitBlockingResponse(RequestContext, RequestBlockingAction) so a failed commit is reported through AppSecContext#reportBlockFailure(). PathExtractionHelpers threw the BlockingException even when no block response function was available, blocking the request with no chance of a blocking response ever being committed; the throw now happens strictly inside the guard. Play runs on the Netty server, whose block response function commits synchronously and calls TraceSegment#effectivelyBlocked() itself, so the call sites must not call it again. Enable the shared HttpServerTest block failure coverage for the Play 2.5 and 2.6 AppSec suites on the path params blocking point.
Migrate every vertx-web 3.4/4.0/5.0 blocking call site to BlockResponseFunction#tryCommitBlockingResponse(RequestContext, RequestBlockingAction), which centralises AppSecContext#reportBlockFailure() when the blocking response cannot be committed. effectivelyBlocked() is deliberately not called at these call sites: vertx-web reuses Netty's block response function, which already finishes the span synchronously when it commits the response. Also fix RoutingContextJsonAdvice, which only assigned a BlockingException when the instrumented method had not thrown. A concurrent exception would silently swallow the block, so the BlockingException is now thrown unconditionally inside the block response function guard. Enable the shared HttpServerTest block failure test in the three vertx-web server test suites, using the path params variant since path params are published by the vertx-web instrumentation itself.
The Results$Status/apply and StatusHeader/sendJson advices carried the WAF callback lookup and the block response function call inline in their advice bodies, which get inlined into the instrumented Play classes. Both now delegate to BodyParserHelpers#handleResponseBody, reusing the existing executeCallback glue, so the blocking logic lives in a single static helper and stays testable, matching the pattern of the rest of the AppSec blocking call sites.
… CODEOWNERS Adds sole-asm-java routing for blocking-glue files across vertx-web, ratpack, jax-rs/jakarta-rs, netty, and cross-framework helpers that were previously apm-idm-java-only or shadowed by the dual-owned appsec/* patterns (last-match-wins). Appended after the existing dual block so it wins; does not touch the ~40-file dual category left as a team decision.
- FileIORaspHelper/URLSinkCallSite: throw BlockingException even when no BlockResponseFunction is available, so LFI/SSRF attempts are always aborted, matching the existing CMDI/SHI invariant - ProcessImplInstrumentationHelpers: extract shared commitBlockingResponse() helper for the CMDI/SHI call sites (pure dedup, no behavior change) - play-appsec-2.5/2.6 BodyParserHelpers: extract shared commitBlockAndThrow() helper for the three multipart call sites (pure dedup, no behavior change) - RatpackRequestBodyGetTextCalledAdvice: drop the unused @Advice.Thrown(readOnly=false) parameter and throw unconditionally, matching the RoutingContextSessionAdvice/JsonAdvice pattern - HttpServerTest.groovy: close the response in a finally block in executeIgnoringIoErrors to avoid a resource leak - CODEOWNERS: split the vertx-web wildcard glob into explicit per-version entries (3.4/4.0/5.0) for Vertx*Instrumentation.java
jandro996
added
type: refactoring
comp: asm waf
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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. |
![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: aea500b01e
ℹ️ 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".
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. |
…e analysis HttpServerTest$IGCallbacks$Context implementing AppSecContext made SpotBugs newly flag every primitive setter in the class as AT_STALE_THREAD_WRITE_OF_PRIMITIVE, failing CI. This is test infrastructure shared across instrumentation suites, not production code, matching the existing exclusion pattern for smoke test controllers.
…y-read abort Three OnMethodExit advices (Ratpack getText, vertx-web 3.4/4.0 getBodyAsJson) had suppress = Throwable.class, so a bare `throw new BlockingException(...)` inside the advice body was swallowed by ByteBuddy's suppression wrapper instead of propagating to the caller, silently disabling blocking on those call sites. Restored the @Advice.Thrown(readOnly = false) reassignment, the only mechanism ByteBuddy respects to rethrow despite suppress. StoredCharBody.maybeNotifyAndBlock() had moved the abort throw inside the blockResponseFunction != null guard, so a request with no response function available no longer aborted the body read at all. Moved the throw back outside the guard to always abort when the WAF requests blocking. Found by Codex review on PR #12601.
…mark pattern StoredCharBody and AppSecEventTracker both live in internal-api and repeated the same "commit blocking response, then mark segment on success" sequence.
…all sites FileIORaspHelper, URLSinkCallSite, and both MessageBodyWriterInstrumentation classes (jax-rs2, jakarta3) had the same inline tryCommitBlockingResponse+effectivelyBlocked pattern already deduped in internal-api. ProcessImplInstrumentationHelpers' own private helper now delegates to the shared default method instead of duplicating the logic.
ResultsStatusInstrumentation and StatusHeaderInstrumentation duplicated the same callback-lookup + block-and-throw logic inline. play25's BodyParserHelpers already extracted this to handleResponseBody() in this PR; apply the same extraction here, reusing the existing executeCallback helper.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6032cf24e
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b6032cf24e
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
- Restore the success check in play25/play26 commitBlockAndThrow before throwing BlockingException, dropped by the earlier dedup extraction - Add unit tests for BlockResponseFunction.tryCommitBlockingResponseAndMarkBlocked covering both branches (marks effectivelyBlocked on success, skips on failure), fixing internal-api jacocoTestCoverageVerification
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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. |
Empty commit to retrigger the pipeline after the transient muzzle shard [3/8] network timeout.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| RequestContext ctx, Flow.Action.RequestBlockingAction action) { | ||
| boolean committed = tryCommitBlockingResponse(ctx, action); | ||
| if (committed) { | ||
| ctx.getTraceSegment().effectivelyBlocked(); |
There was a problem hiding this comment.
AppSec telemetry can record a failed blocking attempt as successful and hide the block failure.
Assertion details
- Input: A migrated caller uses this helper with Netty or Undertow, and the response starts or becomes unavailable before the scheduled blocking handler runs.
- Expected:
Mark the trace as blocked only after the response handler commits the blocking response. Report a block failure when the handler cannot commit it. - Actual:
The helper receives true when Netty or Undertow accepts asynchronous work. It calls effectivelyBlocked() before the response handler commits the block. A later failure does not call reportBlockFailure().
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
There was a problem hiding this comment.
Good catch, thanks. Confirmed both async implementations you mentioned actually hit this: UndertowBlockResponseFunction#tryCommitBlockingResponse always returns true right after dispatching to UndertowBlockingHandler, and NettyHttpServerDecorator#tryCommitBlockingResponse (netty-4.1) returns true immediately when it schedules the commit on the event loop from a non-event-loop thread — in both cases the actual write happens later.
Fixed in 589c247 by removing tryCommitBlockingResponseAndMarkBlocked entirely rather than trying to special-case it: none of the migrated call sites called effectivelyBlocked() before this PR, so the helper was introducing the premature mark rather than deduplicating an existing pattern. The 7 call sites now call tryCommitBlockingResponse(ctx, action) directly, leaving the async implementation solely responsible for both effectivelyBlocked() and any later failure reporting, matching pre-PR semantics. Also documented on the interface javadoc that a synchronous true only means the commit was scheduled, not that it was already committed.
Added asynchronousImplementationOwnsMarkingAndFailureReporting in BlockResponseFunctionTest to cover exactly this schedule-then-commit-later scenario.
Note: UndertowBlockingHandler and the Netty scheduled runnable still don't call reportBlockFailure() if the deferred write itself fails or times out — that's the other half of this finding, but it's pre-existing behavior unrelated to this PR's migration, so I'm leaving it out of scope here and will file a follow-up.
…n helper tryCommitBlockingResponseAndMarkBlocked called effectivelyBlocked() as soon as tryCommitBlockingResponse returned true, but for async implementations (Netty off-event-loop, Undertow dispatch) true only means the commit was scheduled, not that it actually happened. A later async failure would leave the trace marked as blocked with no reportBlockFailure() call. None of the migrated call sites called effectivelyBlocked() before this PR, so the helper was introducing the premature call rather than deduplicating an existing pattern. Removes the helper, restores the 7 call sites to call tryCommitBlockingResponse(ctx, rba) directly (matching pre-PR semantics), and documents on the interface javadoc that a synchronous true does not guarantee the response was already committed. Adds a BlockResponseFunctionTest case reproducing the async schedule-then-commit-later scenario from the review comment.
PathExtractionHelpers had zero test coverage. Its behavior when BlockResponseFunction is unavailable (fail-open: return null instead of throwing BlockingException) is an intentional design decision already discussed on PR review (a WAF block cannot be committed consistently without a BlockResponseFunction, unlike the RASP case which fails closed to prevent an actual command/file/network operation), but nothing guarded it against an accidental flip in a future refactor. Adds PathExtractionHelpersTest covering: empty/null params, missing callback, non-blocking actions, the fail-open branch, the commit branch (including a failed commit still throwing, since the boolean result is intentionally ignored), and callback exceptions being swallowed. Adds the mockito test dependency the module was missing.
This branch has not been deployed
1 participant
What Does This Do
Centralizes ~59 AppSec blocking call sites onto
BlockResponseFunction#tryCommitBlockingResponse(RequestContext, RequestBlockingAction), replacing the older(TraceSegment, int, BlockingContentType, Map, String)overload, and reports block failures (a WAF-triggered block whose response could not actually be committed) consistently across frameworks.Test infrastructure
block_failuremechanism from the resteasy AppSec test fixture into the sharedHttpServerTest.groovy, so every server instrumentation under test can opt in viatestBlockFailure()/blockFailureVariant()and assert thatAppSecContext#reportBlockFailure()is called when the blocking response cannot be committed.Migrated call sites
RoutingContextJsonAdvice,RoutingContextSessionAdvice,WafPublishingBodyHandler,FileUploadHelper,PathParameterPublishingHelpernow use theRequestContextoverload and calleffectivelyBlocked()explicitly where the old overload used to do it implicitly.BodyParserHelpers,PathExtractionHelpers,ResultsStatusApplyAdvice/ResultsStatusInstrumentation,StatusHeaderInstrumentation/StatusHeaderSendJsonAdvicemigrated.PathExtractionHelpersnow returnsnull(fail-open, request proceeds unblocked) when noBlockResponseFunctionis available, instead of throwingBlockingExceptionunconditionally as before: without aBlockResponseFunctionthere is no way to commit a blocking response consistent with the rest of the pipeline (headers, status, template), so we let the request through rather than abort without a controlled response. This intentionally differs from the RASP case below, where aborting without aBlockResponseFunctionprevents an actual command/file/network operation, not just an HTTP response.ContextParseAdvice,JsonRendererAdvice,PathBindingPublishingHandler,RatpackRequestBodyCallGetBufferAdvice,RatpackRequestBodyGetTextCalledAdvice,RequestBodyCollectionPublisher.MessageBodyWriterInstrumentationin both.FileIORaspHelper(LFI) andURLSinkCallSite(SSRF) migrated to the new overload; both now throwBlockingExceptioneven when noBlockResponseFunctionis available, so the attempt is always aborted, matching the existing CMDI/SHI invariant inProcessImplInstrumentationHelpers.StoredCharBody,AppSecEventTracker,ProcessImplInstrumentationHelpers(exec/shell command blocking), with a sharedcommitBlockingResponse()helper to remove duplication between the twoProcessImplInstrumentationHelperscall sites.AppSecInterceptorupdated for theMediaTypepackage move (see below).Other changes bundled with this migration
MediaTypefromdatadog.trace.api.appsectodatadog.trace.api.http, since it's a generic HTTP concept, not AppSec-specific.ContextInterpreter#clientIpWithoutAppSectocollectClientIpfor clarity..github/CODEOWNERSvertx-web wildcard glob (vertx-web/*/.../Vertx*Instrumentation.java) into explicit per-version entries (3.4/4.0/5.0), and maximizes exclusive@DataDog/asm-javaownership across the rest of the AppSec blocking-glue domain covered by this PR (ratpack-1.5, jax-rs/jakarta-rsMessageBodyWriterInstrumentation, netty-4.1 blocking glue, cross-framework*BlockingHelper/*BlockResponseFunctionhelpers,agent-bootstrap/blocking/,ProcessImplInstrumentationHelpers.java). Scope explicitly excludes the ~40 dual-owned files under*-appsec-*/appsec/directories (team decision, would partially revert Fix CODEOWNERS routing for AppSec, IAST, and RASP instrumentation modules #12583) and 8 nested*BlockResponseFunctionclasses unreachable by path glob.Motivation
Consolidates blocking-response commit logic onto a single,
RequestContext-based API so that a failure to commit a blocking response (e.g. the response was already partially written) is observable and reported consistently (AppSecContext#reportBlockFailure()) across every server integration, instead of silently doing nothing in some frameworks and throwing unconditionally in others.Additional Notes
play-appsec-2.5/play-appsec-2.6andvertx-web-3.4/vertx-web-4.0test tasks are gated bytestJvmConstraints(max Java 8 for Play, max Java 15 for vertx-web 3.4/4.0) — verifiedBUILD SUCCESSFULfor all four under the correct JVM (-PtestJvm=8/-PtestJvm=11).dd-trace-coretest failures (TracerConnectionReliabilityTest.testLateAgentStart/testAgentRestart) were reproduced identically on a cleanmastercheckout and are unrelated to this change.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.