diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index d61754b6290..4b99ecc5a86 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -273,6 +273,44 @@ /dd-java-agent/instrumentation/spring/spring-security/ @DataDog/asm-java @DataDog/apm-idm-java /dd-java-agent/instrumentation/velocity-1.5/ @DataDog/asm-java @DataDog/apm-idm-java +# AppSec blocking glue - exclusive asm-java ownership (must stay after the dual-owned +# /dd-java-agent/instrumentation/**/*appsec* patterns above to win last-match-wins) + +# vertx-web (3.4 / 4.0 / 5.0) +/dd-java-agent/instrumentation/vertx/vertx-web/*/src/main/java/**/RoutingContext*Advice.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/*/src/main/java/**/FileUploadHelper.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/*/src/main/java/**/PathParameterPublishingHelper.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/*/src/main/java/**/BlockingExceptionHandler.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/**/Vertx*Instrumentation.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/**/Vertx*Instrumentation.java @DataDog/asm-java +/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/**/Vertx*Instrumentation.java @DataDog/asm-java + +# ratpack-1.5 (flat package, no appsec/ subpackage) +/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/**/ContextParseAdvice.java @DataDog/asm-java +/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/**/JsonRendererAdvice.java @DataDog/asm-java +/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/**/PathBindingPublishingHandler.java @DataDog/asm-java +/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/**/RatpackRequestBody*Advice.java @DataDog/asm-java +/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/**/RequestBodyCollectionPublisher.java @DataDog/asm-java + +# jax-rs / jakarta-rs annotations +/dd-java-agent/instrumentation/rs/**/MessageBodyWriterInstrumentation.java @DataDog/asm-java + +# netty-4.1 (blocking glue already migrated by #12519, currently apm-idm-java only) +/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/**/NettyMultipartHelper.java @DataDog/asm-java +/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/**/HttpPostRequestDecoderInstrumentation.java @DataDog/asm-java +/dd-java-agent/instrumentation/netty/**/server/BlockingResponseHandler.java @DataDog/asm-java +/dd-java-agent/instrumentation/netty/**/server/MaybeBlockResponseHandler.java @DataDog/asm-java + +# cross-framework blocking helpers and BlockResponseFunction implementations +/dd-java-agent/instrumentation/**/*BlockingHelper.java @DataDog/asm-java +/dd-java-agent/instrumentation/**/*BlockResponseFunction.java @DataDog/asm-java +/dd-java-agent/instrumentation/undertow/undertow-common/src/main/java/**/UndertowBlockingHandler.java @DataDog/asm-java +/dd-java-agent/instrumentation/jetty/jetty-server/*/src/main/*/**/JettyCommitResponse*.java @DataDog/asm-java + +# bootstrap / internal-api blocking and RASP glue +/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/blocking/ @DataDog/asm-java +/internal-api/src/*/*/datadog/trace/bootstrap/instrumentation/api/java/lang/ProcessImplInstrumentationHelpers.java @DataDog/asm-java + # @DataDog/ci-app-libraries /dd-java-agent/agent-ci-visibility/ @DataDog/ci-app-libraries /dd-smoke-tests/backend-mock/ @DataDog/ci-app-libraries diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/util/BodyParser.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/util/BodyParser.java index f7f8a4ce668..159ec8c9f4b 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/util/BodyParser.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/util/BodyParser.java @@ -8,7 +8,7 @@ import com.squareup.moshi.JsonDataException; import com.squareup.moshi.JsonReader; import com.squareup.moshi.JsonWriter; -import datadog.trace.api.appsec.MediaType; +import datadog.trace.api.http.MediaType; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy index f648fabfc83..0ebdc5f6611 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy @@ -14,7 +14,7 @@ import datadog.trace.api.ProductTraceSource import datadog.trace.api.TagMap import datadog.trace.api.appsec.HttpClientRequest import datadog.trace.api.appsec.HttpClientResponse -import datadog.trace.api.appsec.MediaType +import datadog.trace.api.http.MediaType import datadog.trace.api.config.GeneralConfig import datadog.trace.api.function.TriConsumer import datadog.trace.api.function.TriFunction diff --git a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 46abd76a6da..3b5993df7dc 100644 --- a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -12,6 +12,7 @@ import datadog.trace.api.Config import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import datadog.trace.api.ProductActivation +import datadog.trace.api.appsec.AppSecContext import datadog.trace.api.config.GeneralConfig import datadog.trace.api.config.TracerConfig import datadog.trace.api.datastreams.DataStreamsContext @@ -26,6 +27,7 @@ import datadog.trace.api.gateway.RequestContext import datadog.trace.api.gateway.RequestContextSlot import datadog.trace.api.http.StoredBodySupplier import datadog.trace.api.iast.IastContext +import datadog.trace.api.internal.TraceSegment import datadog.trace.api.normalize.SimpleHttpPathNormalizer import datadog.trace.api.rum.RumInjector import datadog.trace.api.telemetry.Endpoint @@ -416,6 +418,21 @@ abstract class HttpServerTest extends WithHttpServer { true } + /** + * Whether the server instrumentation reports a block failure (see {@code + * AppSecContext#reportBlockFailure()}) when the blocking response cannot be committed. Opt in by + * overriding this once the framework call sites go through {@code + * BlockResponseFunction#tryCommitBlockingResponse(RequestContext, RequestBlockingAction)}. + */ + boolean testBlockFailure() { + false + } + + /** The blocking point exercised by the block failure test. */ + BlockFailureVariant blockFailureVariant() { + BlockFailureVariant.REQUEST_HEADERS + } + /** Tomcat 5.5 can't seem to handle the encoded URIs */ boolean testEncodedPath() { true @@ -2072,6 +2089,46 @@ abstract class HttpServerTest extends WithHttpServer { } } + def 'test block failure is reported when the blocking response cannot be committed'() { + setup: + assumeTrue(testBlockFailure()) + def variant = blockFailureVariant() + assumeTrue(variant != BlockFailureVariant.PATH_PARAMS || testPathParam() != null) + IGCallbacks.Context.blockFailureReported = false + + def request = request(variant.endpoint, 'GET', null) + .header(variant.header, variant.headerValue) + .header(IG_BLOCK_FAIL_HEADER, 'true') + .build() + + when: + def response = executeIgnoringIoErrors(request) + + then: 'no blocking response was committed' + response == null || !(response.code() in [301, 413, 418]) + + and: 'the failure to block was reported to the AppSec context' + IGCallbacks.Context.blockFailureReported + } + + /** + * Executes a request that is expected not to produce a blocking response. When the blocking + * response cannot be committed the server may have nothing left to write, so the connection can + * be closed without a complete HTTP response. + */ + protected Response executeIgnoringIoErrors(Request request) { + Response response = null + try { + response = client.newCall(request).execute() + response.body().bytes() + response + } catch (IOException ignored) { + null + } finally { + response?.close() + } + } + @Flaky(value = "https://github.com/DataDog/dd-trace-java/issues/7061", suites = ["JettyContinuationHandlerV0ForkedTest", "JettyContinuationHandlerV1ForkedTest"]) def 'test blocking of request for request body variant #variant'() { setup: @@ -2699,6 +2756,7 @@ abstract class HttpServerTest extends WithHttpServer { static final String IG_EXTRA_SPAN_NAME_HEADER = "x-ig-write-tags" static final String IG_TEST_HEADER = "x-ig-test-header" static final String IG_BLOCK_HEADER = "x-block" + static final String IG_BLOCK_FAIL_HEADER = "x-block-fail" static final String IG_BLOCK_RESPONSE_HEADER = "x-block-response" static final String IG_PARAMETERS_BLOCK_HEADER = "x-block-parameters" static final String IG_BODY_END_BLOCK_HEADER = "x-block-body-end" @@ -2714,8 +2772,60 @@ abstract class HttpServerTest extends WithHttpServer { static final String IG_PATH_PARAMS_TAG = "ig-path-params" static final String IG_SESSION_ID_TAG = "ig-session-id" + /** + * The blocking point at which a test suite wants the block failure test to be exercised. Each + * variant pairs the endpoint to hit with the instrumentation gateway header that makes the fake + * AppSec callbacks block there. + */ + static enum BlockFailureVariant { + /** Blocks on {@code requestHeaderDone}, supported by every blocking instrumentation. */ + REQUEST_HEADERS(SUCCESS, IG_BLOCK_HEADER, 'json'), + /** Blocks on {@code requestPathParams}, for instrumentations that only publish path params. */ + PATH_PARAMS(PATH_PARAM, IG_PARAMETERS_BLOCK_HEADER, 'true') + + final ServerEndpoint endpoint + final String header + final String headerValue + + private BlockFailureVariant(ServerEndpoint endpoint, String header, String headerValue) { + this.endpoint = endpoint + this.header = header + this.headerValue = headerValue + } + } + + /** Simulates a server that cannot commit the blocking response. */ + static enum FailingBlockResponseFunction implements BlockResponseFunction { + INSTANCE + + @Override + boolean tryCommitBlockingResponse(TraceSegment segment, int statusCode, + BlockingContentType templateType, Map extraHeaders, String securityResponseId) { + false + } + } + class IGCallbacks { - static class Context { + static class Context implements AppSecContext { + /** + * Set by the last request that reported a block failure. Tests that read it reset it first; + * it has to be static because the assertion happens outside the request context. + */ + static volatile boolean blockFailureReported + + /** Replaces the server's block response function with one that fails to commit. */ + boolean failBlocking + + @Override + boolean isManuallyKept() { + false + } + + @Override + void reportBlockFailure() { + blockFailureReported = true + } + String matchingHeaderValue String doneHeaderValue String extraSpanName @@ -2739,6 +2849,20 @@ abstract class HttpServerTest extends WithHttpServer { string == null ? "" : string } + /** + * Builds the blocking flow for a blocking point. When the request asked for a block failure + * (see {@link HttpServerTest#IG_BLOCK_FAIL_HEADER}), the server's block response function is + * first replaced by one that cannot commit, so the instrumentation is expected to report a + * block failure on the AppSec context. + */ + static final Flow blockingFlow(RequestContext rqCtxt, Flow.Action.RequestBlockingAction action) { + Context context = rqCtxt.getData(RequestContextSlot.APPSEC) + if (context?.failBlocking) { + rqCtxt.blockResponseFunction = FailingBlockResponseFunction.INSTANCE + } + new RbaFlow(action) + } + final Supplier> requestStartedCb = ({ -> @@ -2779,6 +2903,9 @@ abstract class HttpServerTest extends WithHttpServer { if (IG_BLOCK_HEADER.equalsIgnoreCase(key)) { context.blockingContentType = value } + if (IG_BLOCK_FAIL_HEADER.equalsIgnoreCase(key)) { + context.failBlocking = true + } if (IG_BLOCK_RESPONSE_HEADER.equalsIgnoreCase(key)) { context.responseBlock = value } @@ -2808,11 +2935,11 @@ abstract class HttpServerTest extends WithHttpServer { } if (context.blockingContentType && context.blockingContentType != 'none') { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(418, BlockingContentType.valueOf(context.blockingContentType.toUpperCase(Locale.ROOT)))) } else if (context.blockingContentType && context.blockingContentType == 'none') { - new RbaFlow( + blockingFlow(rqCtxt, Flow.Action.RequestBlockingAction.forRedirect(301, 'https://www.google.com/')) } else { Flow.ResultFlow.empty() @@ -2862,7 +2989,7 @@ abstract class HttpServerTest extends WithHttpServer { } activeSpan().localRootSpan.setTag('request.body', supplier.get() as String) if (context.bodyEndBlock) { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(413, BlockingContentType.JSON) ) } else { @@ -2892,7 +3019,7 @@ abstract class HttpServerTest extends WithHttpServer { rqCtxt.traceSegment.setTagTop('request.body.converted', obj as String) Context context = rqCtxt.getData(RequestContextSlot.APPSEC) if (context.bodyConvertedBlock) { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(413, BlockingContentType.JSON) ) } else { @@ -2936,7 +3063,7 @@ abstract class HttpServerTest extends WithHttpServer { Context context = rqCtxt.getData(RequestContextSlot.APPSEC) context.responseBody = body if (context.responseBlock) { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(413, BlockingContentType.JSON) ) } else { @@ -2971,12 +3098,12 @@ abstract class HttpServerTest extends WithHttpServer { context.tags.put(IG_RESPONSE_HEADER_TAG, context.igResponseHeaderValue) } if (context.responseBlock == 'none') { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(301, BlockingContentType.NONE, [Location: 'https://www.google.com/']) ) } else if (context.responseBlock == 'json') { - new RbaFlow( + blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(413, BlockingContentType.JSON) ) } else { @@ -2988,7 +3115,7 @@ abstract class HttpServerTest extends WithHttpServer { RequestContext rqCtxt, Map map -> Context context = rqCtxt.getData(RequestContextSlot.APPSEC) if (context.parametersBlock) { - return new RbaFlow( + return blockingFlow(rqCtxt, new Flow.Action.RequestBlockingAction(413, BlockingContentType.JSON) ) } diff --git a/dd-java-agent/instrumentation/java/java-io-1.8/src/main/java/datadog/trace/instrumentation/java/lang/FileIORaspHelper.java b/dd-java-agent/instrumentation/java/java-io-1.8/src/main/java/datadog/trace/instrumentation/java/lang/FileIORaspHelper.java index 7dd48871dfe..0d242f879e9 100644 --- a/dd-java-agent/instrumentation/java/java-io-1.8/src/main/java/datadog/trace/instrumentation/java/lang/FileIORaspHelper.java +++ b/dd-java-agent/instrumentation/java/java-io-1.8/src/main/java/datadog/trace/instrumentation/java/lang/FileIORaspHelper.java @@ -143,8 +143,10 @@ private void invokeRaspCallback( BlockResponseFunction brf = ctx.getBlockResponseFunction(); if (brf != null) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); + brf.tryCommitBlockingResponse(ctx, rba); } + // Thrown even without a BlockResponseFunction: RASP must abort the LFI attempt even when + // no blocking response can be committed. throw new BlockingException("Blocked request (for LFI attempt)"); } } catch (final BlockingException e) { diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/URLSinkCallSite.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/URLSinkCallSite.java index 7752797fa16..c160edf0a8b 100644 --- a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/URLSinkCallSite.java +++ b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/URLSinkCallSite.java @@ -85,8 +85,10 @@ private static void raspCallback(@Nonnull final URL url) { BlockResponseFunction brf = ctx.getBlockResponseFunction(); if (brf != null) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); + brf.tryCommitBlockingResponse(ctx, rba); } + // Thrown even without a BlockResponseFunction: RASP must abort the SSRF attempt even when + // no blocking response can be committed. throw new BlockingException("Blocked request (for SSRF attempt)"); } } catch (final BlockingException e) { diff --git a/dd-java-agent/instrumentation/okhttp/okhttp-2.2/src/main/java/datadog/trace/instrumentation/okhttp2/AppSecInterceptor.java b/dd-java-agent/instrumentation/okhttp/okhttp-2.2/src/main/java/datadog/trace/instrumentation/okhttp2/AppSecInterceptor.java index 82b3e0189ad..2a6a094fd90 100644 --- a/dd-java-agent/instrumentation/okhttp/okhttp-2.2/src/main/java/datadog/trace/instrumentation/okhttp2/AppSecInterceptor.java +++ b/dd-java-agent/instrumentation/okhttp/okhttp-2.2/src/main/java/datadog/trace/instrumentation/okhttp2/AppSecInterceptor.java @@ -14,12 +14,12 @@ import datadog.trace.api.appsec.HttpClientPayload; import datadog.trace.api.appsec.HttpClientRequest; import datadog.trace.api.appsec.HttpClientResponse; -import datadog.trace.api.appsec.MediaType; import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.http.MediaType; import datadog.trace.api.internal.VisibleForTesting; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; diff --git a/dd-java-agent/instrumentation/okhttp/okhttp-3.0/src/main/java/datadog/trace/instrumentation/okhttp3/AppSecInterceptor.java b/dd-java-agent/instrumentation/okhttp/okhttp-3.0/src/main/java/datadog/trace/instrumentation/okhttp3/AppSecInterceptor.java index 7cfecfbebb1..9d07c14d1f5 100644 --- a/dd-java-agent/instrumentation/okhttp/okhttp-3.0/src/main/java/datadog/trace/instrumentation/okhttp3/AppSecInterceptor.java +++ b/dd-java-agent/instrumentation/okhttp/okhttp-3.0/src/main/java/datadog/trace/instrumentation/okhttp3/AppSecInterceptor.java @@ -8,12 +8,12 @@ import datadog.trace.api.appsec.HttpClientPayload; import datadog.trace.api.appsec.HttpClientRequest; import datadog.trace.api.appsec.HttpClientResponse; -import datadog.trace.api.appsec.MediaType; import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.http.MediaType; import datadog.trace.api.internal.VisibleForTesting; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/BodyParserHelpers.java b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/BodyParserHelpers.java index 0eba7744dff..2f30e950dee 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/BodyParserHelpers.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/BodyParserHelpers.java @@ -246,13 +246,7 @@ private static void executeFilenamesCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf != null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (multipart file upload)"); - } - } + commitBlockAndThrow(reqCtx, rba, "multipart file upload"); } } @@ -331,13 +325,7 @@ private static void executeFilesContentCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf != null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (multipart file upload content)"); - } - } + commitBlockAndThrow(reqCtx, rba, "multipart file upload content"); } } @@ -368,17 +356,45 @@ private static void executeCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction != null) { - boolean success = - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (for " + details + ")"); - } + commitBlockAndThrow(reqCtx, rba, "for " + details); + } + } + + private static void commitBlockAndThrow( + RequestContext reqCtx, Flow.Action.RequestBlockingAction rba, String details) { + BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); + if (brf != null) { + // play runs on netty, which commits the blocking response synchronously and calls + // TraceSegment#effectivelyBlocked() itself: never call it here + boolean success = brf.tryCommitBlockingResponse(reqCtx, rba); + if (success) { + throw new BlockingException("Blocked request (" + details + ")"); } } } + /** + * Publishes a response body to the WAF and blocks the request if the WAF requires it. Kept here + * so that inline advices don't carry the block response function logic in their bodies. + * + * @param reqCtx the active request context + * @param body the response body, already converted to plain java objects + * @param details the call site description used in the {@link BlockingException} message + */ + public static void handleResponseBody(RequestContext reqCtx, Object body, String details) { + CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); + if (cbp == null) { + return; + } + BiFunction> callback = + cbp.getCallback(EVENTS.responseBody()); + if (callback == null) { + return; + } + + executeCallback(reqCtx, callback, body, details); + } + private static Object tryConvertingScalaContainers(Object obj, int depth) { if (depth == 0) { return obj; diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/ResultsStatusApplyAdvice.java b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/ResultsStatusApplyAdvice.java index 64e2ce7fbb5..70896264b86 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/ResultsStatusApplyAdvice.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/ResultsStatusApplyAdvice.java @@ -1,18 +1,11 @@ package datadog.trace.instrumentation.play25.appsec; -import static datadog.trace.api.gateway.Events.EVENTS; import static datadog.trace.instrumentation.play25.appsec.BodyParserHelpers.jsValueToJavaObject; -import datadog.appsec.api.blocking.BlockingException; import datadog.trace.advice.ActiveRequestContext; import datadog.trace.advice.RequiresRequestContext; -import datadog.trace.api.gateway.BlockResponseFunction; -import datadog.trace.api.gateway.CallbackProvider; -import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; import play.api.libs.json.JsValue; @@ -27,27 +20,7 @@ static void before( return; } - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - if (cbp == null) { - return; - } - BiFunction> callback = - cbp.getCallback(EVENTS.responseBody()); - if (callback == null) { - return; - } - - Flow flow = callback.apply(reqCtx, jsValueToJavaObject((JsValue) content)); - Flow.Action action = flow.getAction(); - if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction == null) { - return; - } - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - - throw new BlockingException("Blocked request (for Results$Status/apply)"); - } + BodyParserHelpers.handleResponseBody( + reqCtx, jsValueToJavaObject((JsValue) content), "Results$Status/apply"); } } diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderInstrumentation.java b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderInstrumentation.java index c3362e119a9..187da47e0cf 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderInstrumentation.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderInstrumentation.java @@ -26,6 +26,13 @@ public String instrumentedType() { return "play.mvc.StatusHeader"; } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter", + }; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderSendJsonAdvice.java b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderSendJsonAdvice.java index 51a6084c350..c127e45a17d 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderSendJsonAdvice.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/StatusHeaderSendJsonAdvice.java @@ -1,19 +1,11 @@ package datadog.trace.instrumentation.play25.appsec; -import static datadog.trace.api.gateway.Events.EVENTS; - import com.fasterxml.jackson.databind.JsonNode; -import datadog.appsec.api.blocking.BlockingException; import datadog.trace.advice.ActiveRequestContext; import datadog.trace.advice.RequiresRequestContext; -import datadog.trace.api.gateway.BlockResponseFunction; -import datadog.trace.api.gateway.CallbackProvider; -import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; import play.mvc.StatusHeader; @@ -32,28 +24,7 @@ static void before( return; } - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - if (cbp == null) { - return; - } - BiFunction> callback = - cbp.getCallback(EVENTS.responseBody()); - if (callback == null) { - return; - } - - Flow flow = callback.apply(reqCtx, json); - Flow.Action action = flow.getAction(); - if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction == null) { - return; - } - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - - throw new BlockingException("Blocked request (for StatusHeader/sendJson)"); - } + BodyParserHelpers.handleResponseBody(reqCtx, json, "StatusHeader/sendJson"); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/test/groovy/datadog/trace/instrumentation/play25/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/test/groovy/datadog/trace/instrumentation/play25/server/PlayServerTest.groovy index fd10be014f3..61903630217 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.5/src/test/groovy/datadog/trace/instrumentation/play25/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play/play-appsec-2.5/src/test/groovy/datadog/trace/instrumentation/play25/server/PlayServerTest.groovy @@ -108,6 +108,17 @@ class PlayServerTest extends HttpServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // play publishes the blocking path params callback from PathExtractionHelpers + BlockFailureVariant.PATH_PARAMS + } + @Override String testPathParam() { '/path/?/param' diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/BodyParserHelpers.java b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/BodyParserHelpers.java index 4f1fff4b5fa..078c9444cfd 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/BodyParserHelpers.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/BodyParserHelpers.java @@ -257,13 +257,7 @@ private static void executeFilenamesCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf != null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (multipart file upload)"); - } - } + commitBlockAndThrow(reqCtx, rba, "multipart file upload"); } } @@ -342,13 +336,7 @@ private static void executeFilesContentCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf != null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (multipart file upload content)"); - } - } + commitBlockAndThrow(reqCtx, rba, "multipart file upload content"); } } @@ -405,13 +393,33 @@ private static void executeCallback( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction != null) { - boolean success = - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (success) { - throw new BlockingException("Blocked request (for " + details + ")"); - } + commitBlockAndThrow(reqCtx, rba, "for " + details); + } + } + + public static void handleResponseBody(RequestContext reqCtx, Object body, String details) { + CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); + if (cbp == null) { + return; + } + BiFunction> callback = + cbp.getCallback(EVENTS.responseBody()); + if (callback == null) { + return; + } + + executeCallback(reqCtx, callback, body, details); + } + + private static void commitBlockAndThrow( + RequestContext reqCtx, Flow.Action.RequestBlockingAction rba, String details) { + BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); + if (brf != null) { + // play runs on netty, which commits the blocking response synchronously and calls + // TraceSegment#effectivelyBlocked() itself: never call it here + boolean success = brf.tryCommitBlockingResponse(reqCtx, rba); + if (success) { + throw new BlockingException("Blocked request (" + details + ")"); } } } diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/ResultsStatusInstrumentation.java b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/ResultsStatusInstrumentation.java index 659a47a02bf..ffcb98d330b 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/ResultsStatusInstrumentation.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/ResultsStatusInstrumentation.java @@ -1,24 +1,17 @@ package datadog.trace.instrumentation.play26.appsec; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.api.gateway.Events.EVENTS; import static datadog.trace.instrumentation.play26.appsec.BodyParserHelpers.jsValueToJavaObject; import com.google.auto.service.AutoService; -import datadog.appsec.api.blocking.BlockingException; import datadog.trace.advice.ActiveRequestContext; import datadog.trace.advice.RequiresRequestContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.agent.tooling.muzzle.Reference; -import datadog.trace.api.gateway.BlockResponseFunction; -import datadog.trace.api.gateway.CallbackProvider; -import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.instrumentation.play26.MuzzleReferences; -import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; import play.api.libs.json.JsValue; @@ -69,28 +62,8 @@ static void after( return; } - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - if (cbp == null) { - return; - } - BiFunction> callback = - cbp.getCallback(EVENTS.responseBody()); - if (callback == null) { - return; - } - - Flow flow = callback.apply(reqCtx, jsValueToJavaObject((JsValue) content)); - Flow.Action action = flow.getAction(); - if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction == null) { - return; - } - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - - throw new BlockingException("Blocked request (for Results$Status/apply)"); - } + BodyParserHelpers.handleResponseBody( + reqCtx, jsValueToJavaObject((JsValue) content), "Results$Status/apply"); } } } diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/StatusHeaderInstrumentation.java b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/StatusHeaderInstrumentation.java index 904c3731d3f..7cd57f39900 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/StatusHeaderInstrumentation.java +++ b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/main/java/datadog/trace/instrumentation/play26/appsec/StatusHeaderInstrumentation.java @@ -1,26 +1,19 @@ package datadog.trace.instrumentation.play26.appsec; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.api.gateway.Events.EVENTS; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.fasterxml.jackson.databind.JsonNode; import com.google.auto.service.AutoService; -import datadog.appsec.api.blocking.BlockingException; import datadog.trace.advice.ActiveRequestContext; import datadog.trace.advice.RequiresRequestContext; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.agent.tooling.muzzle.Reference; -import datadog.trace.api.gateway.BlockResponseFunction; -import datadog.trace.api.gateway.CallbackProvider; -import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.instrumentation.play26.MuzzleReferences; -import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; import play.mvc.StatusHeader; @@ -47,6 +40,13 @@ public String instrumentedType() { return "play.mvc.StatusHeader"; } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter", + }; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -70,28 +70,7 @@ static void before( return; } - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - if (cbp == null) { - return; - } - BiFunction> callback = - cbp.getCallback(EVENTS.responseBody()); - if (callback == null) { - return; - } - - Flow flow = callback.apply(reqCtx, json); - Flow.Action action = flow.getAction(); - if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction == null) { - return; - } - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - - throw new BlockingException("Blocked request (for StatusHeader/sendJson)"); - } + BodyParserHelpers.handleResponseBody(reqCtx, json, "StatusHeader/sendJson"); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/test/groovy/datadog/trace/instrumentation/play26/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/test/groovy/datadog/trace/instrumentation/play26/server/PlayServerTest.groovy index 0ac729a38ac..66433eb2a28 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-2.6/src/test/groovy/datadog/trace/instrumentation/play26/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play/play-appsec-2.6/src/test/groovy/datadog/trace/instrumentation/play26/server/PlayServerTest.groovy @@ -17,6 +17,17 @@ class PlayServerTest extends AbstractPlayServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // play publishes the blocking path params callback from PathExtractionHelpers + BlockFailureVariant.PATH_PARAMS + } + def 'test instrumentation gateway xml request body'() { setup: def request = request( diff --git a/dd-java-agent/instrumentation/play/play-appsec-common/build.gradle b/dd-java-agent/instrumentation/play/play-appsec-common/build.gradle index 3d48cbcff41..804fba845f1 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-common/build.gradle +++ b/dd-java-agent/instrumentation/play/play-appsec-common/build.gradle @@ -1,3 +1,7 @@ plugins { id 'dd-trace-java.module.instrumentation' } + +dependencies { + testImplementation libs.bundles.mockito +} diff --git a/dd-java-agent/instrumentation/play/play-appsec-common/src/main/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpers.java b/dd-java-agent/instrumentation/play/play-appsec-common/src/main/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpers.java index 8da233f0388..82936363be3 100644 --- a/dd-java-agent/instrumentation/play/play-appsec-common/src/main/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpers.java +++ b/dd-java-agent/instrumentation/play/play-appsec-common/src/main/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpers.java @@ -50,9 +50,13 @@ private static BlockingException doCallRequestPathParamsCallback( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + if (brf == null) { + // nothing can commit the blocking response, so the request must not be blocked + return null; } + // play runs on netty, which commits the blocking response synchronously and calls + // TraceSegment#effectivelyBlocked() itself: never call it here + brf.tryCommitBlockingResponse(reqCtx, rba); return new BlockingException("Blocked request (for " + origin + ")"); } } diff --git a/dd-java-agent/instrumentation/play/play-appsec-common/src/test/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpersTest.java b/dd-java-agent/instrumentation/play/play-appsec-common/src/test/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpersTest.java new file mode 100644 index 00000000000..babded1f26a --- /dev/null +++ b/dd-java-agent/instrumentation/play/play-appsec-common/src/test/java/datadog/trace/instrumentation/play/appsec/PathExtractionHelpersTest.java @@ -0,0 +1,159 @@ +package datadog.trace.instrumentation.play.appsec; + +import static datadog.trace.api.gateway.Events.EVENTS; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import datadog.appsec.api.blocking.BlockingContentType; +import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.CallbackProvider; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI; +import java.util.Collections; +import java.util.Map; +import java.util.function.BiFunction; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class PathExtractionHelpersTest { + + private static final String ORIGIN = "test.origin"; + + private TracerAPI originalTracer; + private CallbackProvider callbackProvider; + private BiFunction, Flow> callback; + + @BeforeEach + @SuppressWarnings("unchecked") + void setUp() { + originalTracer = AgentTracer.get(); + + callbackProvider = mock(CallbackProvider.class); + callback = mock(BiFunction.class); + when(callbackProvider.getCallback(EVENTS.requestPathParams())).thenReturn(callback); + + TracerAPI tracer = mock(TracerAPI.class); + when(tracer.getCallbackProvider(RequestContextSlot.APPSEC)).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDown() { + AgentTracer.forceRegister(originalTracer); + } + + @Test + void nullParams_returnsNullWithoutCallingCallback() { + RequestContext reqCtx = mock(RequestContext.class); + + assertNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, null, ORIGIN)); + + verify(callback, never()).apply(any(), any()); + } + + @Test + void emptyParams_returnsNullWithoutCallingCallback() { + RequestContext reqCtx = mock(RequestContext.class); + + assertNull( + PathExtractionHelpers.callRequestPathParamsCallback( + reqCtx, Collections.emptyMap(), ORIGIN)); + + verify(callback, never()).apply(any(), any()); + } + + @Test + void noCallbackRegistered_returnsNull() { + when(callbackProvider.getCallback(EVENTS.requestPathParams())).thenReturn(null); + RequestContext reqCtx = mock(RequestContext.class); + + assertNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN)); + + verify(reqCtx, never()).getBlockResponseFunction(); + } + + @Test + void nonBlockingAction_returnsNull() { + RequestContext reqCtx = mock(RequestContext.class); + stubCallbackAction(Flow.Action.Noop.INSTANCE); + + assertNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN)); + + verify(reqCtx, never()).getBlockResponseFunction(); + } + + /** + * Pins the intentional fail-open contract: without a {@link BlockResponseFunction} nothing can + * commit a blocking response, so a refactor flipping this to fail-closed must fail here. + */ + @Test + void blockingActionWithoutBlockResponseFunction_failsOpen() { + RequestContext reqCtx = mock(RequestContext.class); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + stubCallbackAction(blockingAction()); + + assertNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN)); + + verify(reqCtx).getBlockResponseFunction(); + } + + @Test + void blockingActionWithBlockResponseFunction_commitsAndThrows() { + RequestContext reqCtx = mock(RequestContext.class); + BlockResponseFunction brf = mock(BlockResponseFunction.class); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + Flow.Action.RequestBlockingAction rba = blockingAction(); + stubCallbackAction(rba); + + BlockingException exception = + PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN); + + assertNotNull(exception); + verify(brf).tryCommitBlockingResponse(reqCtx, rba); + } + + @Test + void blockingActionWithFailedCommit_stillThrows() { + RequestContext reqCtx = mock(RequestContext.class); + BlockResponseFunction brf = mock(BlockResponseFunction.class); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + Flow.Action.RequestBlockingAction rba = blockingAction(); + when(brf.tryCommitBlockingResponse(reqCtx, rba)).thenReturn(false); + stubCallbackAction(rba); + + assertNotNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN)); + } + + @Test + void callbackThrows_exceptionIsSwallowed() { + RequestContext reqCtx = mock(RequestContext.class); + when(callback.apply(any(), any())).thenThrow(new RuntimeException("boom")); + + assertNull(PathExtractionHelpers.callRequestPathParamsCallback(reqCtx, params(), ORIGIN)); + } + + private void stubCallbackAction(Flow.Action action) { + @SuppressWarnings("unchecked") + Flow flow = mock(Flow.class); + when(flow.getAction()).thenReturn(action); + when(callback.apply(any(), any())).thenReturn(flow); + } + + private static Flow.Action.RequestBlockingAction blockingAction() { + return new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + } + + private static Map params() { + return Collections.singletonMap("id", "1"); + } +} diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/ContextParseAdvice.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/ContextParseAdvice.java index e4b03268eff..ffaae71775c 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/ContextParseAdvice.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/ContextParseAdvice.java @@ -45,7 +45,9 @@ static void after( BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + brf.tryCommitBlockingResponse(reqCtx, rba); t = new BlockingException("Blocked request (for DefaultContext/parse)"); } diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/JsonRendererAdvice.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/JsonRendererAdvice.java index 0f1b75320f8..843ef4f43ea 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/JsonRendererAdvice.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/JsonRendererAdvice.java @@ -42,7 +42,9 @@ static void enter( BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + brf.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for JsonRenderer/render)"); } diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/PathBindingPublishingHandler.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/PathBindingPublishingHandler.java index a41a1cbfdda..cbdfaefc6a2 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/PathBindingPublishingHandler.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/PathBindingPublishingHandler.java @@ -67,7 +67,9 @@ private boolean maybePublishTokens(Context ctx) { return true; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + blockResponseFunction.tryCommitBlockingResponse(requestContext, rba); return false; } diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyCallGetBufferAdvice.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyCallGetBufferAdvice.java index 43c9de353d2..850812e3494 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyCallGetBufferAdvice.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyCallGetBufferAdvice.java @@ -45,7 +45,9 @@ static Throwable before( return null; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); return new BlockingException("Blocked request (for ByteBufBackedTypedData/getBuffer)"); } diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyGetTextCalledAdvice.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyGetTextCalledAdvice.java index 847041f83f8..af51a7ac6a5 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyGetTextCalledAdvice.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RatpackRequestBodyGetTextCalledAdvice.java @@ -36,7 +36,9 @@ static void after( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (throwable == null) { throwable = new BlockingException("Blocked request (for ByteBufBackedTypedData/getText)"); } diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RequestBodyCollectionPublisher.java b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RequestBodyCollectionPublisher.java index 6ec7376249e..b8ff0e3e36c 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RequestBodyCollectionPublisher.java +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/main/java/datadog/trace/instrumentation/ratpack/RequestBodyCollectionPublisher.java @@ -82,7 +82,9 @@ private void block(Flow.Action.RequestBlockingAction rba, Throwable t) { if (blockResponseFunction == null) { return; } - blockResponseFunction.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: Ratpack blocks through Netty's + // BlockResponseFunction, whose BlockingResponseHandler already marks the segment. + blockResponseFunction.tryCommitBlockingResponse(requestContext, rba); // we can't directly interrupt user code here by throwing an exception // user code must listen for errors and implement its own logic to prevent diff --git a/dd-java-agent/instrumentation/ratpack-1.5/src/test/groovy/server/RatpackHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.5/src/test/groovy/server/RatpackHttpServerTest.groovy index 0b665683b2b..a0ede81d695 100644 --- a/dd-java-agent/instrumentation/ratpack-1.5/src/test/groovy/server/RatpackHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/ratpack-1.5/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -108,6 +108,18 @@ class RatpackHttpServerTest extends HttpServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // Ratpack's own blocking call site for request headers lives in the Netty instrumentation; + // path params are published by PathBindingPublishingHandler, a Ratpack-specific call site. + BlockFailureVariant.PATH_PARAMS + } + @Override Serializable expectedServerSpanRoute(ServerEndpoint endpoint) { return String diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/MessageBodyWriterInstrumentation.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/MessageBodyWriterInstrumentation.java index f259d4cde5b..7fde1168665 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/MessageBodyWriterInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/MessageBodyWriterInstrumentation.java @@ -74,7 +74,7 @@ static void before( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for MessageBodyWriter)"); } diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/MessageBodyWriterInstrumentation.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/MessageBodyWriterInstrumentation.java index 8262c211420..30352bb22e0 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/MessageBodyWriterInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/MessageBodyWriterInstrumentation.java @@ -79,7 +79,7 @@ static void before( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for MessageBodyWriter)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/FileUploadHelper.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/FileUploadHelper.java index 913003e8c28..017e23d14f6 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/FileUploadHelper.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/FileUploadHelper.java @@ -22,8 +22,9 @@ public static BlockingException commitBlockingResponse( if (action instanceof Flow.Action.RequestBlockingAction) { BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse( - reqCtx.getTraceSegment(), (Flow.Action.RequestBlockingAction) action); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + brf.tryCommitBlockingResponse(reqCtx, (Flow.Action.RequestBlockingAction) action); return new BlockingException(reason); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/PathParameterPublishingHelper.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/PathParameterPublishingHelper.java index 23e2a0c5728..53f1a255065 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/PathParameterPublishingHelper.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/PathParameterPublishingHelper.java @@ -49,7 +49,10 @@ public static Throwable publishParams(Map params) { log.warn("Can't block. Don't know how to block on this server"); } else { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is + // committed. + brf.tryCommitBlockingResponse(requestContext, rba); be = new BlockingException("Blocked request (for route/matches)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextJsonAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextJsonAdvice.java index 8cb28041ec8..7bb3c5c6a74 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextJsonAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextJsonAdvice.java @@ -45,7 +45,9 @@ static void after( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (throwable == null) { throwable = new BlockingException("Blocked request (for RoutingContextImpl/getBodyAsJson)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextSessionAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextSessionAdvice.java index 5d7a0504a74..74ab1d69138 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextSessionAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/RoutingContextSessionAdvice.java @@ -41,7 +41,9 @@ static void after( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for session)"); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/WafPublishingBodyHandler.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/WafPublishingBodyHandler.java index 482ace0ef7e..821cdc12f85 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/WafPublishingBodyHandler.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/main/java/datadog/trace/instrumentation/vertx_3_4/server/WafPublishingBodyHandler.java @@ -70,7 +70,9 @@ private void publishRequestBody(Object body) { return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException( "Blocked request (for Buffer/toString or Buffer/toJson{Object,Array})"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/test/groovy/server/VertxHttpServerForkedTest.groovy b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/test/groovy/server/VertxHttpServerForkedTest.groovy index 7fadc69bac5..1e821c066ce 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/test/groovy/server/VertxHttpServerForkedTest.groovy +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-3.4/src/test/groovy/server/VertxHttpServerForkedTest.groovy @@ -144,6 +144,18 @@ class VertxHttpServerForkedTest extends HttpServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // Path params are published by the vert.x-web instrumentation itself, whereas request headers + // are published (and blocked on) by the underlying netty instrumentation. + BlockFailureVariant.PATH_PARAMS + } + @Override Class expectedExceptionType() { return RuntimeException diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/FileUploadHelper.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/FileUploadHelper.java index c0eef8cab2e..1d41c203934 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/FileUploadHelper.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/FileUploadHelper.java @@ -22,8 +22,9 @@ public static BlockingException commitBlockingResponse( if (action instanceof Flow.Action.RequestBlockingAction) { BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse( - reqCtx.getTraceSegment(), (Flow.Action.RequestBlockingAction) action); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + brf.tryCommitBlockingResponse(reqCtx, (Flow.Action.RequestBlockingAction) action); return new BlockingException(reason); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/PathParameterPublishingHelper.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/PathParameterPublishingHelper.java index 3c6f230fc01..2be6e3b248d 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/PathParameterPublishingHelper.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/PathParameterPublishingHelper.java @@ -47,7 +47,10 @@ public static Throwable publishParams(Map params) { log.warn("Can't block. Don't know how to block on this server"); } else { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is + // committed. + brf.tryCommitBlockingResponse(requestContext, rba); return new BlockingException("Blocked request (for route/matches)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonAdvice.java index b1699e5c8f7..99c968af91d 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonAdvice.java @@ -60,7 +60,9 @@ static void after( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (throwable == null) { throwable = new BlockingException("Blocked request (for RoutingContextImpl/getBodyAsJson)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonResponseAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonResponseAdvice.java index 1dfff50beef..5f51dd782d5 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonResponseAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextJsonResponseAdvice.java @@ -43,7 +43,9 @@ static void before( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for RoutingContext/json)"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextSessionAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextSessionAdvice.java index b80d8e9077f..1c9be5993ca 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextSessionAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/RoutingContextSessionAdvice.java @@ -41,7 +41,9 @@ static void after( return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException("Blocked request (for session)"); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/WafPublishingBodyHandler.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/WafPublishingBodyHandler.java index d23a70e30e3..71f7e8a6849 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/WafPublishingBodyHandler.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/main/java/datadog/trace/instrumentation/vertx_4_0/server/WafPublishingBodyHandler.java @@ -70,7 +70,9 @@ private void publishRequestBody(Object body) { return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException( "Blocked request (for Buffer/toString or Buffer/toJson{Object,Array})"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy index 62455354611..1385b59833e 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-4.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy @@ -150,6 +150,18 @@ class VertxHttpServerForkedTest extends HttpServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // Path params are published by the vert.x-web instrumentation itself, whereas request headers + // are published (and blocked on) by the underlying netty instrumentation. + BlockFailureVariant.PATH_PARAMS + } + @Override boolean isRequestBodyNoStreaming() { true diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/FileUploadHelper.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/FileUploadHelper.java index 045d825444a..a4cd402370c 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/FileUploadHelper.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/FileUploadHelper.java @@ -22,8 +22,9 @@ public static BlockingException commitBlockingResponse( if (action instanceof Flow.Action.RequestBlockingAction) { BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse( - reqCtx.getTraceSegment(), (Flow.Action.RequestBlockingAction) action); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + brf.tryCommitBlockingResponse(reqCtx, (Flow.Action.RequestBlockingAction) action); return new BlockingException(reason); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/WafPublishingBodyHandler.java b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/WafPublishingBodyHandler.java index 92364ba2cec..5c174b81021 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/WafPublishingBodyHandler.java +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/main/java/datadog/trace/instrumentation/vertx_5_0/server/WafPublishingBodyHandler.java @@ -69,7 +69,9 @@ private void publishRequestBody(Object body) { return; } Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // effectivelyBlocked() is intentionally absent: vertx-web shares Netty's block response + // function, which finishes the span synchronously when the blocking response is committed. + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); throw new BlockingException( "Blocked request (for Buffer/toString or Buffer/toJson{Object,Array})"); } diff --git a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy index cdd4bca1c19..3b784751929 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy +++ b/dd-java-agent/instrumentation/vertx/vertx-web/vertx-web-5.0/src/test/groovy/server/VertxHttpServerForkedTest.groovy @@ -145,6 +145,18 @@ class VertxHttpServerForkedTest extends HttpServerTest { true } + @Override + boolean testBlockFailure() { + true + } + + @Override + BlockFailureVariant blockFailureVariant() { + // Path params are published by the vert.x-web instrumentation itself, whereas request headers + // are published (and blocked on) by the underlying netty instrumentation. + BlockFailureVariant.PATH_PARAMS + } + @Override boolean isRequestBodyNoStreaming() { true diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java index 14a6459ca0f..ea63f83e702 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java @@ -66,7 +66,7 @@ public abstract class ContextInterpreter implements AgentPropagation.KeyClassifi private TagContext.HttpHeaders httpHeaders; private final String customIpHeaderName; private final boolean clientIpResolutionEnabled; - private final boolean clientIpWithoutAppSec; + private final boolean collectClientIp; private final boolean aiGuardEnabled; private boolean collectIpHeaders; private final boolean requestHeaderTagsCommaAllowed; @@ -83,7 +83,7 @@ protected static String toLowerCase(String key) { protected ContextInterpreter(Config config) { this.customIpHeaderName = config.getTraceClientIpHeader(); this.clientIpResolutionEnabled = config.isTraceClientIpResolverEnabled(); - this.clientIpWithoutAppSec = config.isClientIpEnabled(); + this.collectClientIp = config.isClientIpEnabled(); this.aiGuardEnabled = config.isAiGuardEnabled(); this.propagationTagsFactory = PropagationTags.factory(config); this.requestHeaderTagsCommaAllowed = config.isRequestHeaderTagsCommaAllowed(); @@ -278,7 +278,7 @@ public ContextInterpreter reset(TraceConfig traceConfig) { fullContext = true; httpHeaders = null; collectIpHeaders = - this.clientIpWithoutAppSec + this.collectClientIp || this.clientIpResolutionEnabled && (ActiveSubsystems.APPSEC_ACTIVE || this.aiGuardEnabled); headerTags = traceConfig.getRequestHeaderTags(); diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index abfc0e20585..0f869f2fd71 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -1,6 +1,6 @@ package datadog.trace.lambda; -import datadog.trace.api.appsec.MediaType; +import datadog.trace.api.http.MediaType; import datadog.trace.lambda.MultipartSplitter.Part; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index 6dfcca2729c..86aa6999eae 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -5,7 +5,7 @@ import com.squareup.moshi.JsonAdapter; import com.squareup.moshi.Moshi; import datadog.trace.api.Config; -import datadog.trace.api.appsec.MediaType; +import datadog.trace.api.http.MediaType; import datadog.trace.lambda.ContentTypeBodyParser.ParseContext; import java.io.ByteArrayInputStream; import java.io.IOException; diff --git a/gradle/spotbugFilters/exclude.xml b/gradle/spotbugFilters/exclude.xml index 2276ed212e0..22bde5b0ef0 100644 --- a/gradle/spotbugFilters/exclude.xml +++ b/gradle/spotbugFilters/exclude.xml @@ -4,6 +4,8 @@ + + diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java index 8fb38b7cebb..aee774ff6ab 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java +++ b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java @@ -386,7 +386,7 @@ private boolean dispatch( final BlockResponseFunction brf = ctx.getBlockResponseFunction(); if (brf != null) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); + brf.tryCommitBlockingResponse(ctx, rba); } return true; } diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/HttpClientPayload.java b/internal-api/src/main/java/datadog/trace/api/appsec/HttpClientPayload.java index 9be785a1037..0070750f7e7 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/HttpClientPayload.java +++ b/internal-api/src/main/java/datadog/trace/api/appsec/HttpClientPayload.java @@ -1,5 +1,6 @@ package datadog.trace.api.appsec; +import datadog.trace.api.http.MediaType; import java.io.InputStream; import java.util.List; import java.util.Map; diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/BlockResponseFunction.java b/internal-api/src/main/java/datadog/trace/api/gateway/BlockResponseFunction.java index a9c11618e11..742492e7e7a 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/BlockResponseFunction.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/BlockResponseFunction.java @@ -51,7 +51,10 @@ default boolean tryCommitBlockingResponse( * AppSecContext#reportBlockFailure()} if the commit fails. * *

It's responsible for calling {@link TraceSegment#effectivelyBlocked()} before the span is - * finished. + * finished. Callers must never call {@link TraceSegment#effectivelyBlocked()} themselves on the + * strength of a {@code true} return value: asynchronous implementations (Netty off the event + * loop, Undertow dispatching to an IO thread) return {@code true} as soon as the blocking + * response is scheduled, and only mark the segment once the response is actually committed. * * @param ctx the request context * @param action the blocking action containing status code, content type, headers, and security diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/MediaType.java b/internal-api/src/main/java/datadog/trace/api/http/MediaType.java similarity index 98% rename from internal-api/src/main/java/datadog/trace/api/appsec/MediaType.java rename to internal-api/src/main/java/datadog/trace/api/http/MediaType.java index 979562160c8..8437f2c859b 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/MediaType.java +++ b/internal-api/src/main/java/datadog/trace/api/http/MediaType.java @@ -1,4 +1,4 @@ -package datadog.trace.api.appsec; +package datadog.trace.api.http; import java.util.Locale; diff --git a/internal-api/src/main/java/datadog/trace/api/http/StoredCharBody.java b/internal-api/src/main/java/datadog/trace/api/http/StoredCharBody.java index aee387eb5ac..cc46a41ae3b 100644 --- a/internal-api/src/main/java/datadog/trace/api/http/StoredCharBody.java +++ b/internal-api/src/main/java/datadog/trace/api/http/StoredCharBody.java @@ -180,7 +180,7 @@ public synchronized void maybeNotifyAndBlock() { BlockResponseFunction blockResponseFunction = httpContext.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(httpContext.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(httpContext, rba); } throw new BlockingException("Blocked request (for request body stream read)"); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/java/lang/ProcessImplInstrumentationHelpers.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/java/lang/ProcessImplInstrumentationHelpers.java index 97b8eb15c29..f807f6e1daa 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/java/lang/ProcessImplInstrumentationHelpers.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/java/lang/ProcessImplInstrumentationHelpers.java @@ -240,11 +240,9 @@ public static void cmdiRaspCheck(@Nonnull final String[] cmdArray) { Flow flow = execCmdCallback.apply(ctx, cmdArray); Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction brf = ctx.getBlockResponseFunction(); - if (brf != null) { - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); - } + commitBlockingResponse(ctx, (Flow.Action.RequestBlockingAction) action); + // Thrown even without a BlockResponseFunction: RASP must abort the exec attempt even when + // no blocking response can be committed. throw new BlockingException("Blocked request (for CMDI attempt)"); } } catch (final BlockingException e) { @@ -256,6 +254,14 @@ public static void cmdiRaspCheck(@Nonnull final String[] cmdArray) { } } + private static void commitBlockingResponse( + RequestContext ctx, Flow.Action.RequestBlockingAction rba) { + BlockResponseFunction brf = ctx.getBlockResponseFunction(); + if (brf != null) { + brf.tryCommitBlockingResponse(ctx, rba); + } + } + public static void resetCheckShi() { checkShi.set(false); } @@ -291,11 +297,9 @@ public static void shiRaspCheck(@Nonnull final String cmd) { Flow flow = shellCmdCallback.apply(ctx, cmd); Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { - BlockResponseFunction brf = ctx.getBlockResponseFunction(); - if (brf != null) { - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); - } + commitBlockingResponse(ctx, (Flow.Action.RequestBlockingAction) action); + // Thrown even without a BlockResponseFunction: RASP must abort the shell command attempt + // even when no blocking response can be committed. throw new BlockingException("Blocked request (for SHI attempt)"); } } catch (final BlockingException e) { diff --git a/internal-api/src/test/groovy/datadog/trace/api/appsec/MediaTypeSpecification.groovy b/internal-api/src/test/groovy/datadog/trace/api/http/MediaTypeSpecification.groovy similarity index 99% rename from internal-api/src/test/groovy/datadog/trace/api/appsec/MediaTypeSpecification.groovy rename to internal-api/src/test/groovy/datadog/trace/api/http/MediaTypeSpecification.groovy index b0d7b9462ef..d7b98a9857a 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/appsec/MediaTypeSpecification.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/http/MediaTypeSpecification.groovy @@ -1,4 +1,4 @@ -package datadog.trace.api.appsec +package datadog.trace.api.http import spock.lang.Specification diff --git a/internal-api/src/test/java/datadog/trace/api/gateway/BlockResponseFunctionTest.java b/internal-api/src/test/java/datadog/trace/api/gateway/BlockResponseFunctionTest.java index e07f49ceb12..bcb3dbf88aa 100644 --- a/internal-api/src/test/java/datadog/trace/api/gateway/BlockResponseFunctionTest.java +++ b/internal-api/src/test/java/datadog/trace/api/gateway/BlockResponseFunctionTest.java @@ -16,7 +16,8 @@ /** * Covers the {@code tryCommitBlockingResponse(RequestContext, RequestBlockingAction)} default * method, which reports a block failure to {@link AppSecContext#reportBlockFailure()} when the - * blocking response cannot be committed. + * blocking response cannot be committed, and leaves {@link TraceSegment#effectivelyBlocked()} to + * the implementation. */ class BlockResponseFunctionTest { @@ -57,6 +58,101 @@ void doesNotThrowWhenAppSecSlotDoesNotHoldAnAppSecContext() { assertFalse(brf.tryCommitBlockingResponse(new TestRequestContext("not an AppSecContext"), RBA)); } + @Test + void leavesEffectivelyBlockedToTheImplementationOnSuccess() { + CountingTraceSegment traceSegment = new CountingTraceSegment(); + TestRequestContext ctx = new TestRequestContext(new CountingAppSecContext(), traceSegment); + TestBlockResponseFunction brf = new TestBlockResponseFunction(true); + + assertTrue(brf.tryCommitBlockingResponse(ctx, RBA)); + + // the implementation, not the default method, owns effectivelyBlocked() + assertEquals(0, traceSegment.effectivelyBlockedCalls); + } + + @Test + void doesNotMarkTraceSegmentBlockedWhenCommitFails() { + CountingTraceSegment traceSegment = new CountingTraceSegment(); + TestRequestContext ctx = new TestRequestContext(new CountingAppSecContext(), traceSegment); + TestBlockResponseFunction brf = new TestBlockResponseFunction(false); + + assertFalse(brf.tryCommitBlockingResponse(ctx, RBA)); + + assertEquals(0, traceSegment.effectivelyBlockedCalls); + } + + /** + * Mimics Netty off the event loop or Undertow dispatching to an IO thread: the commit is only + * scheduled, so {@code true} must not be read as "the response was committed". The trace segment + * stays unmarked until the scheduled work runs, and a scheduled commit that later fails still + * gets to report the block failure. + */ + @Test + void asynchronousImplementationOwnsMarkingAndFailureReporting() { + CountingTraceSegment traceSegment = new CountingTraceSegment(); + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = new TestRequestContext(appSecCtx, traceSegment); + DeferredBlockResponseFunction brf = new DeferredBlockResponseFunction(); + + assertTrue(brf.tryCommitBlockingResponse(ctx, RBA)); + assertEquals(0, traceSegment.effectivelyBlockedCalls); + assertEquals(0, appSecCtx.blockFailures); + + brf.runScheduled(true); + assertEquals(1, traceSegment.effectivelyBlockedCalls); + + brf.runScheduled(false); + assertEquals(1, traceSegment.effectivelyBlockedCalls); + assertEquals(1, appSecCtx.blockFailures); + } + + private static final class CountingTraceSegment implements TraceSegment { + private int effectivelyBlockedCalls; + + @Override + public void setTagTop(String key, Object value, boolean sanitize) {} + + @Override + public Object getTagTop(String key, boolean sanitize) { + return null; + } + + @Override + public void setTagCurrent(String key, Object value, boolean sanitize) {} + + @Override + public Object getTagCurrent(String key, boolean sanitize) { + return null; + } + + @Override + public void setDataTop(String key, Object value) {} + + @Override + public Object getDataTop(String key) { + return null; + } + + @Override + public void effectivelyBlocked() { + effectivelyBlockedCalls++; + } + + @Override + public void setDataCurrent(String key, Object value) {} + + @Override + public Object getDataCurrent(String key) { + return null; + } + + @Override + public void setMetaStructTop(String field, Object value) {} + + @Override + public void setMetaStructCurrent(String field, Object value) {} + } + private static final class CountingAppSecContext implements AppSecContext { private int blockFailures; @@ -95,12 +191,50 @@ public boolean tryCommitBlockingResponse( } } + /** + * A {@link BlockResponseFunction} that only schedules the blocking response, the way Netty does + * when called off the event loop. {@link #runScheduled(boolean)} plays the scheduled work back. + */ + private static final class DeferredBlockResponseFunction implements BlockResponseFunction { + private RequestContext scheduledCtx; + + @Override + public boolean tryCommitBlockingResponse( + TraceSegment segment, + int statusCode, + BlockingContentType templateType, + Map extraHeaders, + String securityResponseId) { + throw new UnsupportedOperationException("the RequestContext overload is scheduled instead"); + } + + @Override + public boolean tryCommitBlockingResponse( + RequestContext ctx, Flow.Action.RequestBlockingAction action) { + this.scheduledCtx = ctx; + return true; + } + + private void runScheduled(boolean committed) { + if (committed) { + scheduledCtx.getTraceSegment().effectivelyBlocked(); + } else { + ((AppSecContext) scheduledCtx.getData(RequestContextSlot.APPSEC)).reportBlockFailure(); + } + } + } + private static final class TestRequestContext implements RequestContext { private final Object appSecData; - private final TraceSegment traceSegment = TraceSegment.NoOp.INSTANCE; + private final TraceSegment traceSegment; private TestRequestContext(Object appSecData) { + this(appSecData, TraceSegment.NoOp.INSTANCE); + } + + private TestRequestContext(Object appSecData, TraceSegment traceSegment) { this.appSecData = appSecData; + this.traceSegment = traceSegment; } @SuppressWarnings("unchecked")