diff --git a/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/baseTest/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpersBlockFailureTest.java b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/baseTest/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpersBlockFailureTest.java new file mode 100644 index 00000000000..900ac26de42 --- /dev/null +++ b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/baseTest/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpersBlockFailureTest.java @@ -0,0 +1,167 @@ +package datadog.trace.instrumentation.akkahttp.appsec; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; + +import datadog.appsec.api.blocking.BlockingContentType; +import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData; +import java.util.Map; +import java.util.function.Function; +import org.junit.jupiter.api.Test; + +/** + * Covers the {@code UnmarshallerHelpers.tryBlock() -> AppSecContext.reportBlockFailure()} path. + * Hand-written test doubles are used because Mockito is only on this module's test runtime + * classpath, not its test compile classpath (see {@code gradle/java_deps.gradle}: {@code + * testRuntimeOnly libs.mokito.core}). + */ +class UnmarshallerHelpersBlockFailureTest { + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @Test + void reportsBlockFailureWhenBlockingResponseCannotBeCommitted() { + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = + new TestRequestContext(new TestBlockResponseFunction(false), appSecCtx); + + BlockingException exception = UnmarshallerHelpers.tryBlock(ctx, RBA, "for test"); + + assertNull(exception); + assertEquals(1, appSecCtx.blockFailures); + assertSame(RBA, ctx.brf.lastAction); + assertSame(ctx.traceSegment, ctx.brf.lastSegment); + } + + @Test + void doesNotReportBlockFailureWhenBlockingResponseIsCommitted() { + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = new TestRequestContext(new TestBlockResponseFunction(true), appSecCtx); + + BlockingException exception = UnmarshallerHelpers.tryBlock(ctx, RBA, "for test"); + + assertNotNull(exception); + assertEquals("Blocked request (for test)", exception.getMessage()); + assertEquals(0, appSecCtx.blockFailures); + } + + @Test + void doesNotReportOrThrowWhenNoBlockResponseFunctionIsRegistered() { + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = new TestRequestContext(null, appSecCtx); + + BlockingException exception = UnmarshallerHelpers.tryBlock(ctx, RBA, "for test"); + + assertNull(exception); + assertEquals(0, appSecCtx.blockFailures); + } + + @Test + void doesNotThrowWhenAppSecSlotDoesNotHoldAnAppSecContext() { + TestRequestContext nullSlot = + new TestRequestContext(new TestBlockResponseFunction(false), null); + assertNull(UnmarshallerHelpers.tryBlock(nullSlot, RBA, "for test")); + + TestRequestContext foreignSlot = + new TestRequestContext(new TestBlockResponseFunction(false), "not an AppSecContext"); + assertNull(UnmarshallerHelpers.tryBlock(foreignSlot, RBA, "for test")); + } + + private static final class CountingAppSecContext implements AppSecContext { + private int blockFailures; + + @Override + public boolean isManuallyKept() { + return false; + } + + @Override + public void reportBlockFailure() { + blockFailures++; + } + } + + private static final class TestBlockResponseFunction implements BlockResponseFunction { + private final boolean committed; + private TraceSegment lastSegment; + private Flow.Action.RequestBlockingAction lastAction; + + private TestBlockResponseFunction(boolean committed) { + this.committed = committed; + } + + @Override + public boolean tryCommitBlockingResponse( + TraceSegment segment, Flow.Action.RequestBlockingAction rba) { + this.lastAction = rba; + return BlockResponseFunction.super.tryCommitBlockingResponse(segment, rba); + } + + @Override + public boolean tryCommitBlockingResponse( + TraceSegment segment, + int statusCode, + BlockingContentType templateType, + Map extraHeaders, + String securityResponseId) { + this.lastSegment = segment; + return committed; + } + } + + private static final class TestRequestContext implements RequestContext { + private final TestBlockResponseFunction brf; + private final Object appSecData; + private final TraceSegment traceSegment = TraceSegment.NoOp.INSTANCE; + + private TestRequestContext(TestBlockResponseFunction brf, Object appSecData) { + this.brf = brf; + this.appSecData = appSecData; + } + + @SuppressWarnings("unchecked") + @Override + public T getData(RequestContextSlot slot) { + return slot == RequestContextSlot.APPSEC ? (T) appSecData : null; + } + + @Override + public TraceSegment getTraceSegment() { + return traceSegment; + } + + @Override + public void setBlockResponseFunction(BlockResponseFunction blockResponseFunction) {} + + @Override + public BlockResponseFunction getBlockResponseFunction() { + return brf; + } + + @Override + public T getOrCreateMetaStructTop(String key, Function defaultValue) { + return null; + } + + @Override + public void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {} + + @Override + public ClientIpAddressData getClientIpAddressData() { + return null; + } + + @Override + public void close() {} + } +} diff --git a/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/BlockingResponseHelper.java b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/BlockingResponseHelper.java index 68d6a9d84c0..9ec7596486e 100644 --- a/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/BlockingResponseHelper.java +++ b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/BlockingResponseHelper.java @@ -42,7 +42,7 @@ public static HttpResponse handleFinishForWaf(final AgentSpan span, final HttpRe if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; if (brf instanceof AkkaBlockResponseFunction) { - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + brf.tryCommitBlockingResponse(requestContext, rba); HttpResponse altResponse = ((AkkaBlockResponseFunction) brf).maybeCreateAlternativeResponse(); if (altResponse != null) { diff --git a/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpers.java b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpers.java index 78c791612a5..1bd3779e1e2 100644 --- a/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpers.java +++ b/dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/appsec/UnmarshallerHelpers.java @@ -21,6 +21,7 @@ import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.api.http.MultipartContentDecoder; +import datadog.trace.api.internal.VisibleForTesting; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import java.lang.reflect.Field; @@ -607,13 +608,23 @@ private static void handleArbitraryPostData(Object o, String source) { executeCallback(reqCtx, callback, o, source); } - private static BlockingException tryBlock( + @VisibleForTesting + static BlockingException tryBlock( RequestContext reqCtx, Flow.Action.RequestBlockingAction rba, String details) { BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf == null) { return null; } - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // Conditional async-race gap (same class as netty-blocking.md §10/§11, but via + // Future.map/.recover/.thenApply on a Scala ExecutionContext instead of + // eventLoop().execute()): the block-failure report below is only guaranteed to run before + // GatewayBridge.onRequestEnded/end-of-request telemetry is emitted when the route's + // response Future causally depends (via flatMap) on this same unmarshalling Future - the + // idiomatic Akka HTTP usage. If the app decouples unmarshalling (used only for a side + // effect) from response production, or triggers toStrict() conversions independently of + // the main response chain, this report can arrive after end-of-request telemetry has + // already been emitted. This is not fixed here; see the KB entry for akka-http. + boolean success = brf.tryCommitBlockingResponse(reqCtx, rba); if (!success) { return null; } diff --git a/dd-java-agent/instrumentation/grizzly/grizzly-2.0/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyBlockingHelper.java b/dd-java-agent/instrumentation/grizzly/grizzly-2.0/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyBlockingHelper.java index ece9d6c4f07..a955855c5b1 100644 --- a/dd-java-agent/instrumentation/grizzly/grizzly-2.0/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyBlockingHelper.java +++ b/dd-java-agent/instrumentation/grizzly/grizzly-2.0/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyBlockingHelper.java @@ -4,7 +4,10 @@ import datadog.appsec.api.blocking.BlockingContentType; import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.blocking.BlockingActionHelper; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.io.OutputStream; @@ -55,11 +58,20 @@ public static boolean block( Map extraHeaders, String securityResponseId, Context context) { + AgentSpan span = AgentSpan.fromContext(context); if (GET_OUTPUT_STREAM == null) { + if (span != null) { + RequestContext reqCtx = span.getRequestContext(); + if (reqCtx != null) { + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + } + } return false; } - AgentSpan span = AgentSpan.fromContext(context); try { OutputStream os = (OutputStream) GET_OUTPUT_STREAM.invoke(response); response.setStatus(BlockingActionHelper.getHttpCode(statusCode)); @@ -79,13 +91,34 @@ public static boolean block( } os.close(); response.finish(); + } catch (Throwable e) { + log.info("Error committing blocking response", e); + if (span != null) { + // the response commit was attempted and failed; report it even though this method still + // returns true below (see known gap: the boolean contract can't signal this today) + RequestContext reqCtx = span.getRequestContext(); + if (reqCtx != null) { + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + } + DECORATE.onError(span, e); + DECORATE.beforeFinish(context); + span.finish(); + } + return true; + } + try { if (span != null) { span.getRequestContext().getTraceSegment().effectivelyBlocked(); } SpanClosingListener.LISTENER.onAfterService(request); } catch (Throwable e) { - log.info("Error committing blocking response", e); + // the response was already committed successfully; this is a finalization error, not a + // commit failure, so it must not be reported as a block failure + log.info("Error finalizing blocked request", e); if (span != null) { DECORATE.onError(span, e); DECORATE.beforeFinish(context); diff --git a/dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/ParsedBodyParametersInstrumentation.java b/dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/ParsedBodyParametersInstrumentation.java index aea5c512c3b..e141d7e43e8 100644 --- a/dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/ParsedBodyParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/ParsedBodyParametersInstrumentation.java @@ -111,7 +111,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for Parameters/processParameters)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/MultipartHelper.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/MultipartHelper.java index bd813c49bff..4971d42f8d0 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/MultipartHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/MultipartHelper.java @@ -115,7 +115,7 @@ public static BlockingException fireFilesContentEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file content)"); } @@ -146,7 +146,7 @@ public static BlockingException fireFilenamesEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file upload)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/RequestExtractContentParametersInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/RequestExtractContentParametersInstrumentation.java index 9660f7824a7..ed06d6201d5 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/RequestExtractContentParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/main/java/datadog/trace/instrumentation/jetty11/RequestExtractContentParametersInstrumentation.java @@ -118,7 +118,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for Request/extractContentParameters)"); reqCtx.getTraceSegment().effectivelyBlocked(); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/test/java/datadog/trace/instrumentation/jetty11/MultipartHelperTest.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/test/java/datadog/trace/instrumentation/jetty11/MultipartHelperTest.java index 6d20a79bbb4..7d747624262 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/test/java/datadog/trace/instrumentation/jetty11/MultipartHelperTest.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-11.0/src/test/java/datadog/trace/instrumentation/jetty11/MultipartHelperTest.java @@ -4,9 +4,27 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +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.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +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.EventType; +import datadog.trace.api.gateway.Events; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import jakarta.servlet.http.Part; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -14,10 +32,149 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.BiFunction; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class MultipartHelperTest { + private final AgentTracer.TracerAPI originalTracer = AgentTracer.get(); + private CallbackProvider callbackProvider; + private RequestContext reqCtx; + private BlockResponseFunction brf; + private TraceSegment traceSegment; + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @BeforeEach + void setUpBlockFailureFixtures() { + callbackProvider = mock(CallbackProvider.class); + traceSegment = mock(TraceSegment.class); + brf = mock(BlockResponseFunction.class); + reqCtx = mock(RequestContext.class); + when(reqCtx.getTraceSegment()).thenReturn(traceSegment); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + + AgentTracer.TracerAPI tracer = mock(AgentTracer.TracerAPI.class); + when(tracer.getCallbackProvider(any(RequestContextSlot.class))).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDownBlockFailureFixtures() { + AgentTracer.forceRegister(originalTracer); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private void stubCallback(EventType>> event) { + doReturn((Object) (BiFunction>) (ctx, x) -> blockingFlow()) + .when(callbackProvider) + .getCallback(event); + } + + // ── fireFilenamesEvent: wiring to BlockResponseFunction ───────────────────── + + @Test + void fireFilenamesEventCommitFails() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventCommitSucceeds() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventNoBlockResponseFunction() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilesContentEvent: wiring to BlockResponseFunction ────────────────── + + @Test + void fireFilesContentEventCommitFails() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventCommitSucceeds() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + @Test void returnsEmptyListForNull() { assertEquals(emptyList(), MultipartHelper.extractFilenames(null)); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-7.0/src/main/java/datadog/trace/instrumentation/jetty70/UrlEncodedInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-7.0/src/main/java/datadog/trace/instrumentation/jetty70/UrlEncodedInstrumentation.java index 76dc3fcca64..1d87bf32ec8 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-7.0/src/main/java/datadog/trace/instrumentation/jetty70/UrlEncodedInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-7.0/src/main/java/datadog/trace/instrumentation/jetty70/UrlEncodedInstrumentation.java @@ -100,7 +100,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for UrlEncoded/decodeTo)"); reqCtx.getTraceSegment().effectivelyBlocked(); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/main/java/datadog/trace/instrumentation/jetty8/PartHelper.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/main/java/datadog/trace/instrumentation/jetty8/PartHelper.java index 468a93a028d..d685c84fa34 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/main/java/datadog/trace/instrumentation/jetty8/PartHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/main/java/datadog/trace/instrumentation/jetty8/PartHelper.java @@ -232,7 +232,7 @@ public static BlockingException fireBodyProcessedEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart form fields)"); } @@ -262,7 +262,7 @@ public static BlockingException fireFilenamesEvent(Collection parts, RequestC Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file upload)"); } @@ -331,7 +331,7 @@ public static BlockingException fireFilesContentEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file content)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/test/java/datadog/trace/instrumentation/jetty8/PartHelperTest.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/test/java/datadog/trace/instrumentation/jetty8/PartHelperTest.java index 5826698454d..fbd57c00ea2 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/test/java/datadog/trace/instrumentation/jetty8/PartHelperTest.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-8.1.3/src/test/java/datadog/trace/instrumentation/jetty8/PartHelperTest.java @@ -5,11 +5,28 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +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.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +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.Config; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.CallbackProvider; +import datadog.trace.api.gateway.EventType; +import datadog.trace.api.gateway.Events; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.charset.Charset; @@ -19,12 +36,188 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.BiFunction; import javax.servlet.http.Part; import org.eclipse.jetty.util.MultiPartInputStream; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class PartHelperTest { + private final AgentTracer.TracerAPI originalTracer = AgentTracer.get(); + private CallbackProvider callbackProvider; + private RequestContext reqCtx; + private BlockResponseFunction brf; + private TraceSegment traceSegment; + + // Same RequestBlockingAction used across the block-failure-reporting tests below. + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @BeforeEach + void setUpBlockFailureFixtures() { + callbackProvider = mock(CallbackProvider.class); + traceSegment = mock(TraceSegment.class); + brf = mock(BlockResponseFunction.class); + reqCtx = mock(RequestContext.class); + when(reqCtx.getTraceSegment()).thenReturn(traceSegment); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + + AgentTracer.TracerAPI tracer = mock(AgentTracer.TracerAPI.class); + when(tracer.getCallbackProvider(any(RequestContextSlot.class))).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDownBlockFailureFixtures() { + AgentTracer.forceRegister(originalTracer); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private void stubCallback(EventType>> event) { + doReturn((Object) (BiFunction>) (ctx, x) -> blockingFlow()) + .when(callbackProvider) + .getCallback(event); + } + + // ── fireBodyProcessedEvent: wiring to BlockResponseFunction ───────────────── + + @Test + void fireBodyProcessedEventCommitFails() throws IOException { + stubCallback(Events.get().requestBodyProcessed()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + PartHelper.fireBodyProcessedEvent(singletonList(field("a", "x")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + verify(traceSegment, never()).effectivelyBlocked(); + } + + @Test + void fireBodyProcessedEventCommitSucceeds() throws IOException { + stubCallback(Events.get().requestBodyProcessed()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + PartHelper.fireBodyProcessedEvent(singletonList(field("a", "x")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireBodyProcessedEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.get().requestBodyProcessed()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + PartHelper.fireBodyProcessedEvent(singletonList(field("a", "x")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilenamesEvent: wiring to BlockResponseFunction ───────────────────── + + @Test + void fireFilenamesEventCommitFails() { + stubCallback(Events.get().requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + PartHelper.fireFilenamesEvent(singletonList(filePart("evil.php")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventCommitSucceeds() { + stubCallback(Events.get().requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + PartHelper.fireFilenamesEvent(singletonList(filePart("evil.php")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventNoBlockResponseFunction() { + stubCallback(Events.get().requestFilesFilenames()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + PartHelper.fireFilenamesEvent(singletonList(filePart("evil.php")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilesContentEvent: wiring to BlockResponseFunction ────────────────── + + @Test + void fireFilesContentEventCommitFails() throws IOException { + stubCallback(Events.get().requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + Part p = filePart("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = PartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventCommitSucceeds() throws IOException { + stubCallback(Events.get().requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + Part p = filePart("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = PartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.get().requestFilesContent()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + Part p = filePart("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = PartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + // ── extractFilenames ──────────────────────────────────────────────────────── @Test diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/MultipartHelper.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/MultipartHelper.java index 902db2d493a..1d9b26b67ee 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/MultipartHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/MultipartHelper.java @@ -115,7 +115,7 @@ public static BlockingException fireFilesContentEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file content)"); } @@ -146,7 +146,7 @@ public static BlockingException fireFilenamesEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file upload)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/RequestExtractContentParametersInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/RequestExtractContentParametersInstrumentation.java index dcfae4aca4c..8f6bda56141 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/RequestExtractContentParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/main/java/datadog/trace/instrumentation/jetty92/RequestExtractContentParametersInstrumentation.java @@ -96,7 +96,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); t = new BlockingException("Blocked request (for Request/extractContentParameters)"); reqCtx.getTraceSegment().effectivelyBlocked(); } @@ -137,7 +137,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for Request/getParts)"); reqCtx.getTraceSegment().effectivelyBlocked(); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/test/java/datadog/trace/instrumentation/jetty92/MultipartHelperTest.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/test/java/datadog/trace/instrumentation/jetty92/MultipartHelperTest.java index 376e1bd6305..b0c15eac33d 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/test/java/datadog/trace/instrumentation/jetty92/MultipartHelperTest.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.2/src/test/java/datadog/trace/instrumentation/jetty92/MultipartHelperTest.java @@ -4,20 +4,177 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +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.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +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.EventType; +import datadog.trace.api.gateway.Events; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.BiFunction; import javax.servlet.http.Part; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class MultipartHelperTest { + private final AgentTracer.TracerAPI originalTracer = AgentTracer.get(); + private CallbackProvider callbackProvider; + private RequestContext reqCtx; + private BlockResponseFunction brf; + private TraceSegment traceSegment; + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @BeforeEach + void setUpBlockFailureFixtures() { + callbackProvider = mock(CallbackProvider.class); + traceSegment = mock(TraceSegment.class); + brf = mock(BlockResponseFunction.class); + reqCtx = mock(RequestContext.class); + when(reqCtx.getTraceSegment()).thenReturn(traceSegment); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + + AgentTracer.TracerAPI tracer = mock(AgentTracer.TracerAPI.class); + when(tracer.getCallbackProvider(any(RequestContextSlot.class))).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDownBlockFailureFixtures() { + AgentTracer.forceRegister(originalTracer); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private void stubCallback(EventType>> event) { + doReturn((Object) (BiFunction>) (ctx, x) -> blockingFlow()) + .when(callbackProvider) + .getCallback(event); + } + + // ── fireFilenamesEvent: wiring to BlockResponseFunction ───────────────────── + + @Test + void fireFilenamesEventCommitFails() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventCommitSucceeds() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventNoBlockResponseFunction() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilesContentEvent: wiring to BlockResponseFunction ────────────────── + + @Test + void fireFilesContentEventCommitFails() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventCommitSucceeds() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + @Test void returnsEmptyListForNull() { assertEquals(emptyList(), MultipartHelper.extractFilenames(null)); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/MultipartHelper.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/MultipartHelper.java index d223d15b519..335f0e3dd2c 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/MultipartHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/MultipartHelper.java @@ -115,7 +115,7 @@ public static BlockingException fireFilesContentEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file content)"); } @@ -146,7 +146,7 @@ public static BlockingException fireFilenamesEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file upload)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/RequestExtractContentParametersInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/RequestExtractContentParametersInstrumentation.java index ed769a11d7c..6a8258dcbdf 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/RequestExtractContentParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/main/java/datadog/trace/instrumentation/jetty93/RequestExtractContentParametersInstrumentation.java @@ -112,7 +112,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for Request/extractContentParameters)"); reqCtx.getTraceSegment().effectivelyBlocked(); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/test/java/datadog/trace/instrumentation/jetty93/MultipartHelperTest.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/test/java/datadog/trace/instrumentation/jetty93/MultipartHelperTest.java index 9f580412ae8..9c58139cbd4 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/test/java/datadog/trace/instrumentation/jetty93/MultipartHelperTest.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.3/src/test/java/datadog/trace/instrumentation/jetty93/MultipartHelperTest.java @@ -4,20 +4,177 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +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.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +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.EventType; +import datadog.trace.api.gateway.Events; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.BiFunction; import javax.servlet.http.Part; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class MultipartHelperTest { + private final AgentTracer.TracerAPI originalTracer = AgentTracer.get(); + private CallbackProvider callbackProvider; + private RequestContext reqCtx; + private BlockResponseFunction brf; + private TraceSegment traceSegment; + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @BeforeEach + void setUpBlockFailureFixtures() { + callbackProvider = mock(CallbackProvider.class); + traceSegment = mock(TraceSegment.class); + brf = mock(BlockResponseFunction.class); + reqCtx = mock(RequestContext.class); + when(reqCtx.getTraceSegment()).thenReturn(traceSegment); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + + AgentTracer.TracerAPI tracer = mock(AgentTracer.TracerAPI.class); + when(tracer.getCallbackProvider(any(RequestContextSlot.class))).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDownBlockFailureFixtures() { + AgentTracer.forceRegister(originalTracer); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private void stubCallback(EventType>> event) { + doReturn((Object) (BiFunction>) (ctx, x) -> blockingFlow()) + .when(callbackProvider) + .getCallback(event); + } + + // ── fireFilenamesEvent: wiring to BlockResponseFunction ───────────────────── + + @Test + void fireFilenamesEventCommitFails() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventCommitSucceeds() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventNoBlockResponseFunction() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilesContentEvent: wiring to BlockResponseFunction ────────────────── + + @Test + void fireFilesContentEventCommitFails() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventCommitSucceeds() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + @Test void returnsEmptyListForNull() { assertEquals(emptyList(), MultipartHelper.extractFilenames(null)); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/MultipartHelper.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/MultipartHelper.java index 0f9e2b00df2..e13dd8ac139 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/MultipartHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/MultipartHelper.java @@ -115,7 +115,7 @@ public static BlockingException fireFilesContentEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file content)"); } @@ -146,7 +146,7 @@ public static BlockingException fireFilenamesEvent( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + if (brf.tryCommitBlockingResponse(reqCtx, rba)) { reqCtx.getTraceSegment().effectivelyBlocked(); return new BlockingException("Blocked request (multipart file upload)"); } diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/RequestExtractContentParametersInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/RequestExtractContentParametersInstrumentation.java index d7c810a511b..d2087d02070 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/RequestExtractContentParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/main/java/datadog/trace/instrumentation/jetty94/RequestExtractContentParametersInstrumentation.java @@ -124,7 +124,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for Request/extractContentParameters)"); reqCtx.getTraceSegment().effectivelyBlocked(); diff --git a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/test/java/datadog/trace/instrumentation/jetty94/MultipartHelperTest.java b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/test/java/datadog/trace/instrumentation/jetty94/MultipartHelperTest.java index a32058a8624..5c79fbb5341 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/test/java/datadog/trace/instrumentation/jetty94/MultipartHelperTest.java +++ b/dd-java-agent/instrumentation/jetty/jetty-appsec/jetty-appsec-9.4/src/test/java/datadog/trace/instrumentation/jetty94/MultipartHelperTest.java @@ -4,20 +4,177 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +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.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +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.EventType; +import datadog.trace.api.gateway.Events; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.BiFunction; import javax.servlet.http.Part; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class MultipartHelperTest { + private final AgentTracer.TracerAPI originalTracer = AgentTracer.get(); + private CallbackProvider callbackProvider; + private RequestContext reqCtx; + private BlockResponseFunction brf; + private TraceSegment traceSegment; + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @BeforeEach + void setUpBlockFailureFixtures() { + callbackProvider = mock(CallbackProvider.class); + traceSegment = mock(TraceSegment.class); + brf = mock(BlockResponseFunction.class); + reqCtx = mock(RequestContext.class); + when(reqCtx.getTraceSegment()).thenReturn(traceSegment); + when(reqCtx.getBlockResponseFunction()).thenReturn(brf); + + AgentTracer.TracerAPI tracer = mock(AgentTracer.TracerAPI.class); + when(tracer.getCallbackProvider(any(RequestContextSlot.class))).thenReturn(callbackProvider); + AgentTracer.forceRegister(tracer); + } + + @AfterEach + void tearDownBlockFailureFixtures() { + AgentTracer.forceRegister(originalTracer); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private void stubCallback(EventType>> event) { + doReturn((Object) (BiFunction>) (ctx, x) -> blockingFlow()) + .when(callbackProvider) + .getCallback(event); + } + + // ── fireFilenamesEvent: wiring to BlockResponseFunction ───────────────────── + + @Test + void fireFilenamesEventCommitFails() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventCommitSucceeds() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilenamesEventNoBlockResponseFunction() { + stubCallback(Events.EVENTS.requestFilesFilenames()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + BlockingException result = + MultipartHelper.fireFilenamesEvent(singletonList(part("evil.php")), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + + // ── fireFilesContentEvent: wiring to BlockResponseFunction ────────────────── + + @Test + void fireFilesContentEventCommitFails() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(false); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventCommitSucceeds() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(brf.tryCommitBlockingResponse(reqCtx, RBA)).thenReturn(true); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNotNull(result); + verify(brf, times(1)).tryCommitBlockingResponse(reqCtx, RBA); + } + + @Test + void fireFilesContentEventNoBlockResponseFunction() throws IOException { + stubCallback(Events.EVENTS.requestFilesContent()); + when(reqCtx.getBlockResponseFunction()).thenReturn(null); + + Part p = mock(Part.class); + when(p.getSubmittedFileName()).thenReturn("photo.jpg"); + when(p.getInputStream()) + .thenReturn(new ByteArrayInputStream("data".getBytes(StandardCharsets.UTF_8))); + + BlockingException result = MultipartHelper.fireFilesContentEvent(singletonList(p), reqCtx); + + assertNull(result); + verify(brf, never()).tryCommitBlockingResponse(any(RequestContext.class), any()); + } + @Test void returnsEmptyListForNull() { assertEquals(emptyList(), MultipartHelper.extractFilenames(null)); diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/JettyCommitResponseHelper.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/JettyCommitResponseHelper.java index be0be6eaa3a..597098a830b 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/JettyCommitResponseHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/JettyCommitResponseHelper.java @@ -4,8 +4,10 @@ import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_IGNORE_COMMIT_ATTRIBUTE; import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -85,6 +87,11 @@ public class JettyCommitResponseHelper { if (success) { requestContext.getTraceSegment().effectivelyBlocked(); return true; + } else { + Object rawAppSecCtx = requestContext.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } } } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-11.0/src/test/groovy/Jetty11Test.groovy b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-11.0/src/test/groovy/Jetty11Test.groovy index 6c3c72b67e9..fd80aa18479 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-11.0/src/test/groovy/Jetty11Test.groovy +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-11.0/src/test/groovy/Jetty11Test.groovy @@ -4,9 +4,16 @@ import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.instrumentation.servlet5.HtmlRumServlet import datadog.trace.instrumentation.servlet5.TestServlet5 import datadog.trace.instrumentation.servlet5.XmlRumServlet +import okhttp3.MediaType +import okhttp3.MultipartBody +import okhttp3.RequestBody import org.eclipse.jetty.server.Handler import org.eclipse.jetty.server.Server +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED +import static org.junit.jupiter.api.Assumptions.assumeTrue + abstract class Jetty11Test extends HttpServerTest { @Override HttpServer server() { @@ -115,6 +122,35 @@ abstract class Jetty11Test extends HttpServerTest { protected boolean useWebsocketPojoEndpoint() { false } + + def 'test blocking of multipart and urlencoded request body is pinned after block-telemetry-3 wiring #variant'() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request(endpoint, 'POST', body) + .header('x-block-body-converted', 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + // This pins the pre-existing blocking behavior for jetty-appsec-11.0's MultipartHelper and + // RequestExtractContentParametersInstrumentation after the block-telemetry-3 changes wired + // the reportBlockFailure() call into these advice classes. reportBlockFailure() itself remains + // unreachable through this end-to-end test: the real JettyBlockingHelper.tryCommitBlockingResponse + // always returns true for a genuine attempt (see .claude-invariants.md), so no test here can + // force that branch. + response.code() == 413 + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | executeTest | endpoint | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | RequestBody.create(MediaType.get('application/x-www-form-urlencoded'), 'a=x') + 'multipart' | testBodyMultipart() | BODY_MULTIPART | new MultipartBody.Builder().setType(MultipartBody.FORM).addFormDataPart('a', 'x').build() + } } class Jetty11V0ForkedTest extends Jetty11Test implements TestingGenericHttpNamingConventions.ServerV0 { diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/JettyCommitResponseInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/JettyCommitResponseInstrumentation.java index 85a7b630035..c66d1adca13 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/JettyCommitResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/JettyCommitResponseInstrumentation.java @@ -96,8 +96,7 @@ static class CommitResponseAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - boolean res = brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); - if (res) { + if (brf.tryCommitBlockingResponse(requestContext, rba)) { requestContext.getTraceSegment().effectivelyBlocked(); return true; } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/test/groovy/Jetty70Test.groovy b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/test/groovy/Jetty70Test.groovy index 627f379a238..fcf802d3b72 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/test/groovy/Jetty70Test.groovy +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/test/groovy/Jetty70Test.groovy @@ -2,6 +2,9 @@ import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.instrumentation.servlet3.TestServlet3 +import okhttp3.MediaType +import okhttp3.MultipartBody +import okhttp3.RequestBody import org.eclipse.jetty.server.Request import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.AbstractHandler @@ -12,8 +15,11 @@ import javax.servlet.ServletException import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.UNKNOWN +import static org.junit.jupiter.api.Assumptions.assumeTrue abstract class Jetty70Test extends HttpServerTest { @@ -131,6 +137,35 @@ abstract class Jetty70Test extends HttpServerTest { true } + def 'test blocking of multipart and urlencoded request body is pinned after block-telemetry-3 wiring #variant'() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request(endpoint, 'POST', body) + .header('x-block-body-converted', 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + // This pins the pre-existing blocking behavior for jetty-appsec-7.0's UrlEncodedInstrumentation + // (and, where enabled, MultipartHelper/RequestExtractContentParametersInstrumentation) after the + // block-telemetry-3 changes wired the reportBlockFailure() call into these advice classes. + // reportBlockFailure() itself remains unreachable through this end-to-end test: the real + // JettyBlockingHelper.tryCommitBlockingResponse always returns true for a genuine attempt + // (see .claude-invariants.md), so no test here can force that branch. + response.code() == 413 + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | executeTest | endpoint | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | RequestBody.create(MediaType.get('application/x-www-form-urlencoded'), 'a=x') + 'multipart' | testBodyMultipart() | BODY_MULTIPART | new MultipartBody.Builder().setType(MultipartBody.FORM).addFormDataPart('a', 'x').build() + } + static class TestHandler extends AbstractHandler { private static final TestHandler INSTANCE = new TestHandler() diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/JettyCommitResponseInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/JettyCommitResponseInstrumentation.java index fbe8fd6cf6f..475d2640cbb 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/JettyCommitResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/JettyCommitResponseInstrumentation.java @@ -107,7 +107,7 @@ static class CommitResponseAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - return brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + return brf.tryCommitBlockingResponse(requestContext, rba); } } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/test/groovy/Jetty76Test.groovy b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/test/groovy/Jetty76Test.groovy index daab5ac7473..28042bdc01c 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/test/groovy/Jetty76Test.groovy +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/test/groovy/Jetty76Test.groovy @@ -2,6 +2,9 @@ import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.instrumentation.servlet3.TestServlet3 +import okhttp3.MediaType +import okhttp3.MultipartBody +import okhttp3.RequestBody import org.eclipse.jetty.server.Request import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.AbstractHandler @@ -12,8 +15,11 @@ import javax.servlet.ServletException import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.UNKNOWN +import static org.junit.jupiter.api.Assumptions.assumeTrue abstract class Jetty76Test extends HttpServerTest { @@ -132,6 +138,35 @@ abstract class Jetty76Test extends HttpServerTest { true } + def 'test blocking of multipart and urlencoded request body is pinned after block-telemetry-3 wiring #variant'() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request(endpoint, 'POST', body) + .header('x-block-body-converted', 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + // This pins the pre-existing blocking behavior for jetty-appsec-7.0's UrlEncodedInstrumentation + // and jetty-appsec-8.1.3's PartHelper after the block-telemetry-3 changes wired the + // reportBlockFailure() call into these advice classes. reportBlockFailure() itself remains + // unreachable through this end-to-end test: the real JettyBlockingHelper.tryCommitBlockingResponse + // always returns true for a genuine attempt (see .claude-invariants.md), so no test here can + // force that branch. + response.code() == 413 + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | executeTest | endpoint | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | RequestBody.create(MediaType.get('application/x-www-form-urlencoded'), 'a=x') + 'multipart' | testBodyMultipart() | BODY_MULTIPART | new MultipartBody.Builder().setType(MultipartBody.FORM).addFormDataPart('a', 'x').build() + } + static class TestHandler extends AbstractHandler { private static final TestHandler INSTANCE = new TestHandler() diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0.4/src/main/java/datadog/trace/instrumentation/jetty904/JettyCommitResponseHelper.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0.4/src/main/java/datadog/trace/instrumentation/jetty904/JettyCommitResponseHelper.java index 964f6e7a524..72171517849 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0.4/src/main/java/datadog/trace/instrumentation/jetty904/JettyCommitResponseHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0.4/src/main/java/datadog/trace/instrumentation/jetty904/JettyCommitResponseHelper.java @@ -5,8 +5,10 @@ import static datadog.trace.instrumentation.jetty9.JettyDecorator.DECORATE; import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.jetty9.ExtractAdapter; import java.lang.reflect.InvocationTargetException; @@ -84,6 +86,11 @@ public class JettyCommitResponseHelper { _committed.set(true); requestContext.getTraceSegment().effectivelyBlocked(); return true; + } else { + Object rawAppSecCtx = requestContext.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } } } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/JettyCommitResponseInstrumentation.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/JettyCommitResponseInstrumentation.java index ebb0c9e1eec..01137c9ad0b 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/JettyCommitResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/JettyCommitResponseInstrumentation.java @@ -13,9 +13,11 @@ import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.agent.tooling.muzzle.Reference; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.util.concurrent.atomic.AtomicBoolean; import net.bytebuddy.asm.Advice; @@ -127,6 +129,11 @@ static class CommitResponseAdvice { if (res && _committed.get()) { requestContext.getTraceSegment().effectivelyBlocked(); return true; + } else { + Object rawAppSecCtx = requestContext.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } } } } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy index 848f551bf29..74c22f1ee9b 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy @@ -3,11 +3,18 @@ package datadog.trace.instrumentation.jetty9 import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions +import okhttp3.MediaType +import okhttp3.MultipartBody +import okhttp3.RequestBody import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.AbstractHandler import test.JettyServer import test.TestHandler +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED +import static org.junit.jupiter.api.Assumptions.assumeTrue + abstract class Jetty9Test extends HttpServerTest { @Override @@ -96,6 +103,35 @@ abstract class Jetty9Test extends HttpServerTest { boolean testWebsockets() { return super.testWebsockets() && (getServer() as JettyServer).websocketAvailable } + + def 'test blocking of multipart and urlencoded request body is pinned after block-telemetry-3 wiring #variant'() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request(endpoint, 'POST', body) + .header('x-block-body-converted', 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + // This pins the pre-existing blocking behavior for jetty-appsec-7.0's UrlEncodedInstrumentation + // (jetty-appsec-8.1.3's multipart path is disabled for this Jetty 9.0.x module, see + // testBodyMultipart() above) after the block-telemetry-3 changes wired the reportBlockFailure() + // call into these advice classes. reportBlockFailure() itself remains unreachable through this + // end-to-end test: the real JettyBlockingHelper.tryCommitBlockingResponse always returns true + // for a genuine attempt (see .claude-invariants.md), so no test here can force that branch. + response.code() == 413 + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | executeTest | endpoint | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | RequestBody.create(MediaType.get('application/x-www-form-urlencoded'), 'a=x') + 'multipart' | testBodyMultipart() | BODY_MULTIPART | new MultipartBody.Builder().setType(MultipartBody.FORM).addFormDataPart('a', 'x').build() + } } class Jetty9V0ForkedTest extends Jetty9Test implements TestingGenericHttpNamingConventions.ServerV0 { diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/main/java/datadog/trace/instrumentation/jetty93/JettyCommitResponseHelper.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/main/java/datadog/trace/instrumentation/jetty93/JettyCommitResponseHelper.java index 9eb5a286891..2cc1f6a75bc 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/main/java/datadog/trace/instrumentation/jetty93/JettyCommitResponseHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/main/java/datadog/trace/instrumentation/jetty93/JettyCommitResponseHelper.java @@ -5,8 +5,10 @@ import static datadog.trace.instrumentation.jetty9.JettyDecorator.DECORATE; import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.jetty9.ExtractAdapter; import java.lang.reflect.InvocationTargetException; @@ -74,6 +76,11 @@ public class JettyCommitResponseHelper { _committed.set(true); requestContext.getTraceSegment().effectivelyBlocked(); return true; + } else { + Object rawAppSecCtx = requestContext.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } } } diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy index 3c4c45a9d02..7fa16189108 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.3/src/test/groovy/datadog/trace/instrumentation/jetty9/Jetty9Test.groovy @@ -3,11 +3,18 @@ package datadog.trace.instrumentation.jetty9 import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions +import okhttp3.MediaType +import okhttp3.MultipartBody +import okhttp3.RequestBody import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.AbstractHandler import test.JettyServer import test.TestHandler +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED +import static org.junit.jupiter.api.Assumptions.assumeTrue + abstract class Jetty9Test extends HttpServerTest { @Override @@ -113,6 +120,35 @@ abstract class Jetty9Test extends HttpServerTest { boolean testWebsockets() { return super.testWebsockets() && (getServer() as JettyServer).websocketAvailable } + + def 'test blocking of multipart and urlencoded request body is pinned after block-telemetry-3 wiring #variant'() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request(endpoint, 'POST', body) + .header('x-block-body-converted', 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + // This pins the pre-existing blocking behavior for jetty-appsec-9.2/9.3/9.4's MultipartHelper + // and RequestExtractContentParametersInstrumentation after the block-telemetry-3 changes wired + // the reportBlockFailure() call into these advice classes. reportBlockFailure() itself remains + // unreachable through this end-to-end test: the real JettyBlockingHelper.tryCommitBlockingResponse + // always returns true for a genuine attempt (see .claude-invariants.md), so no test here can + // force that branch. + response.code() == 413 + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | executeTest | endpoint | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | RequestBody.create(MediaType.get('application/x-www-form-urlencoded'), 'a=x') + 'multipart' | testBodyMultipart() | BODY_MULTIPART | new MultipartBody.Builder().setType(MultipartBody.FORM).addFormDataPart('a', 'x').build() + } } class Jetty9V0ForkedTest extends Jetty9Test implements TestingGenericHttpNamingConventions.ServerV0 { diff --git a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.4.21/src/main/java/datadog/trace/instrumentation/jetty9421/JettyCommitResponseHelper.java b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.4.21/src/main/java/datadog/trace/instrumentation/jetty9421/JettyCommitResponseHelper.java index c51be7f1b20..57b65ad62be 100644 --- a/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.4.21/src/main/java/datadog/trace/instrumentation/jetty9421/JettyCommitResponseHelper.java +++ b/dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.4.21/src/main/java/datadog/trace/instrumentation/jetty9421/JettyCommitResponseHelper.java @@ -5,8 +5,10 @@ import static datadog.trace.instrumentation.jetty9.JettyDecorator.DECORATE; import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.jetty9.ExtractAdapter; import java.lang.reflect.InvocationTargetException; @@ -75,6 +77,11 @@ public class JettyCommitResponseHelper { if (success) { requestContext.getTraceSegment().effectivelyBlocked(); return true; + } else { + Object rawAppSecCtx = requestContext.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } } } diff --git a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/GetPartsInstrumentation.java b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/GetPartsInstrumentation.java index 102c70acd98..80ca0c333f3 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/GetPartsInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/GetPartsInstrumentation.java @@ -41,7 +41,11 @@ public String[] knownMatchingTypes() { @Override public String[] helperClassNames() { - return new String[] {"datadog.trace.instrumentation.liberty20.PartHelper"}; + return new String[] { + "datadog.trace.instrumentation.liberty20.PartHelper", + "datadog.trace.instrumentation.liberty20.LibertyBlockingHelper", + "datadog.trace.instrumentation.liberty20.LibertyBlockingHelper$WsByteBufferImpl", + }; } @Override @@ -77,8 +81,8 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (t == null) { + boolean success = LibertyBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success && t == null) { t = new BlockingException("Blocked request (multipart file upload)"); reqCtx.getTraceSegment().effectivelyBlocked(); } diff --git a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/LibertyBlockingHelper.java b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/LibertyBlockingHelper.java index 719ba39378b..b9c014b4828 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/LibertyBlockingHelper.java +++ b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/LibertyBlockingHelper.java @@ -9,7 +9,9 @@ import com.ibm.wsspi.http.channel.HttpResponseMessage; import datadog.appsec.api.blocking.BlockingContentType; import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.function.TriConsumer; +import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; @@ -30,6 +32,30 @@ public class LibertyBlockingHelper { private static final Logger log = LoggerFactory.getLogger(LibertyBlockingHelper.class); private static final WsByteBuffer[] EMPTY_BUFFER_ARRAY = new WsByteBuffer[0]; + /** + * Wraps {@link BlockResponseFunction#tryCommitBlockingResponse(RequestContext, + * Flow.Action.RequestBlockingAction)} so that an exception thrown by the commit attempt itself + * (rather than a plain {@code false} return) is still reported as a block failure. The advice + * that calls this method runs with {@code suppress = Throwable.class}, so without this guard such + * an exception would propagate out of the advice and be silently swallowed, and the + * default-method reporting inside {@code tryCommitBlockingResponse} would never run. + */ + public static boolean tryCommitBlockingResponse( + BlockResponseFunction blockResponseFunction, + RequestContext reqCtx, + Flow.Action.RequestBlockingAction rba) { + try { + return blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); + } catch (Exception e) { + log.debug("Error committing blocking response", e); + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + return false; + } + } + public static BlockingException syncBufferEnter( HttpInboundServiceContextImpl thiz, WsByteBuffer[] buffers, AgentSpan span) { if (thiz.isMessageSent() || thiz.headersSent()) { diff --git a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParseParametersInstrumentation.java b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParseParametersInstrumentation.java index dd920e9849b..428db638c21 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParseParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParseParametersInstrumentation.java @@ -54,6 +54,8 @@ public String[] helperClassNames() { packageName + ".ParameterCollector", packageName + ".ParameterCollector$ParameterCollectorNoop", packageName + ".ParameterCollector$ParameterCollectorImpl", + packageName + ".LibertyBlockingHelper", + packageName + ".LibertyBlockingHelper$WsByteBufferImpl", }; } @@ -113,9 +115,12 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - t = new BlockingException("Blocked request (for SRTServletRequest/parseParameters)"); - reqCtx.getTraceSegment().effectivelyBlocked(); + boolean success = + LibertyBlockingHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); + if (success) { + t = new BlockingException("Blocked request (for SRTServletRequest/parseParameters)"); + reqCtx.getTraceSegment().effectivelyBlocked(); + } } } } diff --git a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParsePostDataInstrumentation.java b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParsePostDataInstrumentation.java index 4fa50a6a58f..e52ffe75d40 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParsePostDataInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/ParsePostDataInstrumentation.java @@ -39,6 +39,14 @@ public String[] knownMatchingTypes() { }; } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".LibertyBlockingHelper", + packageName + ".LibertyBlockingHelper$WsByteBufferImpl", + }; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -74,9 +82,12 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - t = new BlockingException("Blocked request (for SRTServletRequest/parsePostData)"); - reqCtx.getTraceSegment().effectivelyBlocked(); + boolean success = + LibertyBlockingHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); + if (success) { + t = new BlockingException("Blocked request (for SRTServletRequest/parsePostData)"); + reqCtx.getTraceSegment().effectivelyBlocked(); + } } } } diff --git a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy index 2e2a5a93803..ed548e47682 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-20.0/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy +++ b/dd-java-agent/instrumentation/liberty/liberty-20.0/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy @@ -9,8 +9,12 @@ import datadog.trace.api.config.GeneralConfig import datadog.trace.api.env.CapturedEnvironment import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.core.DDSpan +import okhttp3.MediaType +import okhttp3.RequestBody import spock.lang.IgnoreIf +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.TIMEOUT_ERROR @@ -157,6 +161,51 @@ abstract class Liberty20Test extends HttpServerTest { it.tags['appsec.blocked'] == 'true' } != null } + + // Pins the pre-existing blocking behavior of ParsePostDataInstrumentation and + // ParseParametersInstrumentation after the block-telemetry-3 changes, which added a + // reportBlockFailure() call when BlockResponseFunction#tryCommitBlockingResponse returns + // false. The real Liberty BlockResponseFunction (LibertyDecorator$LibertyBlockResponseFunction) + // delegates to a void helper that swallows failures and then always returns true for a genuine + // commit attempt, so the reportBlockFailure() branch itself remains unreachable through this + // end-to-end test: this test only verifies the blocking response is produced exactly as before. + // + // GetPartsInstrumentation (liberty-20.0 only) is not separately covered here: the shared + // HttpServerTest file-upload callback (requestFilesFilenamesCb) never returns a blocking Flow + // action, so its blocking branch cannot be exercised through existing test infrastructure + // without changing that shared fixture, which is out of scope for this task. + @IgnoreIf({ !instance.testBlocking() }) + def 'test blocking of request body variant #variant pins block-telemetry-3 behavior'() { + setup: + def request = request( + endpoint, 'POST', + RequestBody.create(MediaType.get(contentType), body)) + .header(IG_BODY_CONVERTED_HEADER, 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + response.code() == 413 + response.header('Content-type') =~ /(?i)\Aapplication\/json(?:;\s?charset=(?:utf-8|iso-8859-1))?\z/ + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | endpoint | contentType | body + 'urlencoded' | BODY_URLENCODED | 'application/x-www-form-urlencoded' | 'a=x' + 'multipart' | BODY_MULTIPART | MULTIPART_TEST_CONTENT_TYPE | MULTIPART_TEST_BODY + } + + private static final String MULTIPART_TEST_CONTENT_TYPE = + 'multipart/form-data; charset=utf-8; boundary=------------------------943d3207457896a3' + private static final String MULTIPART_TEST_BODY = + '--------------------------943d3207457896a3\r\n' + + 'Content-Disposition: form-data; name="a"\r\n' + + '\r\n' + + 'x\r\n' + + '--------------------------943d3207457896a3--' } // make it forked because there are instrumentation errors when we shutdown and diff --git a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/LibertyBlockingHelper.java b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/LibertyBlockingHelper.java index bc35ec1ef3c..637a88634ad 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/LibertyBlockingHelper.java +++ b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/LibertyBlockingHelper.java @@ -9,7 +9,9 @@ import com.ibm.wsspi.http.channel.HttpResponseMessage; import datadog.appsec.api.blocking.BlockingContentType; import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.function.TriConsumer; +import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; @@ -30,6 +32,30 @@ public class LibertyBlockingHelper { private static final Logger log = LoggerFactory.getLogger(LibertyBlockingHelper.class); private static final WsByteBuffer[] EMPTY_BUFFER_ARRAY = new WsByteBuffer[0]; + /** + * Wraps {@link BlockResponseFunction#tryCommitBlockingResponse(RequestContext, + * Flow.Action.RequestBlockingAction)} so that an exception thrown by the commit attempt itself + * (rather than a plain {@code false} return) is still reported as a block failure. The advice + * that calls this method runs with {@code suppress = Throwable.class}, so without this guard such + * an exception would propagate out of the advice and be silently swallowed, and the + * default-method reporting inside {@code tryCommitBlockingResponse} would never run. + */ + public static boolean tryCommitBlockingResponse( + BlockResponseFunction blockResponseFunction, + RequestContext reqCtx, + Flow.Action.RequestBlockingAction rba) { + try { + return blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); + } catch (Exception e) { + log.debug("Error committing blocking response", e); + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + return false; + } + } + public static BlockingException syncBufferEnter( HttpInboundServiceContextImpl thiz, WsByteBuffer[] buffers, AgentSpan span) { if (thiz.isMessageSent() || thiz.headersSent()) { diff --git a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParseParametersInstrumentation.java b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParseParametersInstrumentation.java index d29c4fcdfb0..1541b5cb56e 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParseParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParseParametersInstrumentation.java @@ -54,6 +54,8 @@ public String[] helperClassNames() { packageName + ".ParameterCollector", packageName + ".ParameterCollector$ParameterCollectorNoop", packageName + ".ParameterCollector$ParameterCollectorImpl", + packageName + ".LibertyBlockingHelper", + packageName + ".LibertyBlockingHelper$WsByteBufferImpl", }; } @@ -113,9 +115,12 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - t = new BlockingException("Blocked request (for SRTServletRequest/parseParameters)"); - reqCtx.getTraceSegment().effectivelyBlocked(); + boolean success = + LibertyBlockingHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); + if (success) { + t = new BlockingException("Blocked request (for SRTServletRequest/parseParameters)"); + reqCtx.getTraceSegment().effectivelyBlocked(); + } } } } diff --git a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParsePostDataInstrumentation.java b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParsePostDataInstrumentation.java index 6c5cfda83d8..df50a5b6fcf 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParsePostDataInstrumentation.java +++ b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/main/java/datadog/trace/instrumentation/liberty23/ParsePostDataInstrumentation.java @@ -39,6 +39,14 @@ public String[] knownMatchingTypes() { }; } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".LibertyBlockingHelper", + packageName + ".LibertyBlockingHelper$WsByteBufferImpl", + }; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -74,9 +82,12 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - t = new BlockingException("Blocked request (for SRTServletRequest/parsePostData)"); - reqCtx.getTraceSegment().effectivelyBlocked(); + boolean success = + LibertyBlockingHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); + if (success) { + t = new BlockingException("Blocked request (for SRTServletRequest/parsePostData)"); + reqCtx.getTraceSegment().effectivelyBlocked(); + } } } } diff --git a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy index 1fd92807b6a..795d93b0f83 100644 --- a/dd-java-agent/instrumentation/liberty/liberty-23.0/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy +++ b/dd-java-agent/instrumentation/liberty/liberty-23.0/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy @@ -6,8 +6,12 @@ import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.core.DDSpan +import okhttp3.MediaType +import okhttp3.RequestBody import spock.lang.IgnoreIf +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS abstract class Liberty23Test extends HttpServerTest { @@ -147,6 +151,51 @@ abstract class Liberty23Test extends HttpServerTest { it.tags['appsec.blocked'] == 'true' } != null } + + // Pins the pre-existing blocking behavior of ParsePostDataInstrumentation and + // ParseParametersInstrumentation after the block-telemetry-3 changes, which added a + // reportBlockFailure() call when BlockResponseFunction#tryCommitBlockingResponse returns + // false. The real Liberty BlockResponseFunction (LibertyDecorator$LibertyBlockResponseFunction) + // delegates to a void helper that swallows failures and then always returns true for a genuine + // commit attempt, so the reportBlockFailure() branch itself remains unreachable through this + // end-to-end test: this test only verifies the blocking response is produced exactly as before. + // + // Guarded by testBlocking() like the inherited generic variant test: this Liberty jakarta + // module already disables request-body blocking end-to-end (testBlocking() returns false here, + // a pre-existing limitation unrelated to block-telemetry-3), so this test is currently skipped + // for this class, consistent with that existing, already-disabled behavior. + @IgnoreIf({ !instance.testBlocking() }) + def 'test blocking of request body variant #variant pins block-telemetry-3 behavior'() { + setup: + def request = request( + endpoint, 'POST', + RequestBody.create(MediaType.get(contentType), body)) + .header(IG_BODY_CONVERTED_HEADER, 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + response.code() == 413 + response.header('Content-type') =~ /(?i)\Aapplication\/json(?:;\s?charset=(?:utf-8|iso-8859-1))?\z/ + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + !handlerRan + + where: + variant | endpoint | contentType | body + 'urlencoded' | BODY_URLENCODED | 'application/x-www-form-urlencoded' | 'a=x' + 'multipart' | BODY_MULTIPART | MULTIPART_TEST_CONTENT_TYPE | MULTIPART_TEST_BODY + } + + private static final String MULTIPART_TEST_CONTENT_TYPE = + 'multipart/form-data; charset=utf-8; boundary=------------------------943d3207457896a3' + private static final String MULTIPART_TEST_BODY = + '--------------------------943d3207457896a3\r\n' + + 'Content-Disposition: form-data; name="a"\r\n' + + '\r\n' + + 'x\r\n' + + '--------------------------943d3207457896a3--' } @IgnoreIf({ diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentation.java index d170a53cc22..7c4d3a778ae 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentation.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentation.java @@ -52,6 +52,13 @@ public ElementMatcher hierarchyMatcher() { return implementsInterface(named(hierarchyMarkerType())); } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".SpringBlockingHelper", + }; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -129,9 +136,11 @@ public static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + t = new BlockingException("Blocked request (for HttpMessageConverter/read)"); + } } - t = new BlockingException("Blocked request (for HttpMessageConverter/read)"); } } } @@ -158,9 +167,11 @@ public static void before( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + throw new BlockingException("Blocked response (for HttpMessageConverter/write)"); + } } - throw new BlockingException("Blocked response (for HttpMessageConverter/write)"); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringBlockingHelper.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringBlockingHelper.java new file mode 100644 index 00000000000..fb497eb37cb --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringBlockingHelper.java @@ -0,0 +1,37 @@ +package datadog.trace.instrumentation.springweb; + +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class SpringBlockingHelper { + private static final Logger log = LoggerFactory.getLogger(SpringBlockingHelper.class); + + /** + * Wraps {@link BlockResponseFunction#tryCommitBlockingResponse(RequestContext, + * Flow.Action.RequestBlockingAction)} so that an exception thrown by the commit attempt itself + * (rather than a plain {@code false} return) is still reported as a block failure. The advice + * that calls this method runs with {@code suppress = Throwable.class}, so without this guard such + * an exception would propagate out of the advice and be silently swallowed, and the + * default-method reporting inside {@code tryCommitBlockingResponse} would never run. + */ + public static boolean tryCommitBlockingResponse( + BlockResponseFunction blockResponseFunction, + RequestContext reqCtx, + Flow.Action.RequestBlockingAction rba) { + try { + return blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); + } catch (Exception e) { + log.debug("Error committing blocking response", e); + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + return false; + } + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateAndMatrixVariablesInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateAndMatrixVariablesInstrumentation.java index e82b9b55366..d2a0d86d598 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateAndMatrixVariablesInstrumentation.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateAndMatrixVariablesInstrumentation.java @@ -82,7 +82,7 @@ public void methodAdvice(MethodTransformer transformer) { @Override public String[] helperClassNames() { return new String[] { - packageName + ".PairList", + packageName + ".PairList", packageName + ".SpringBlockingHelper", }; } @@ -167,11 +167,14 @@ public static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = + SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + t = + new BlockingException( + "Blocked request (for RequestMappingInfoHandlerMapping/handleMatch)"); + } } - t = - new BlockingException( - "Blocked request (for RequestMappingInfoHandlerMapping/handleMatch)"); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateVariablesUrlHandlerInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateVariablesUrlHandlerInstrumentation.java index cc90713ea2a..ff260d5262b 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateVariablesUrlHandlerInstrumentation.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/TemplateVariablesUrlHandlerInstrumentation.java @@ -76,6 +76,13 @@ public void methodAdvice(MethodTransformer transformer) { TemplateVariablesUrlHandlerInstrumentation.class.getName() + "$InterceptorPreHandleAdvice"); } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".SpringBlockingHelper", + }; + } + @Override public Advice.PostProcessor.Factory postProcessor() { return postProcessorFactory; @@ -127,11 +134,13 @@ public static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + t = + new BlockingException( + "Blocked request (for UriTemplateVariablesHandlerInterceptor/preHandle)"); + } } - t = - new BlockingException( - "Blocked request (for UriTemplateVariablesHandlerInterceptor/preHandle)"); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentationTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentationTest.groovy index 43309688350..a2adfec3d00 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentationTest.groovy @@ -1,9 +1,14 @@ package datadog.trace.instrumentation.springweb +import datadog.appsec.api.blocking.BlockingContentType +import datadog.appsec.api.blocking.BlockingException import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.appsec.AppSecContext +import datadog.trace.api.gateway.BlockResponseFunction import datadog.trace.api.gateway.Flow import datadog.trace.api.gateway.RequestContext import datadog.trace.api.gateway.RequestContextSlot +import datadog.trace.api.internal.TraceSegment import datadog.trace.bootstrap.instrumentation.api.AgentTracer import datadog.trace.bootstrap.instrumentation.api.TagContext import org.springframework.http.MediaType @@ -11,6 +16,7 @@ import org.springframework.http.converter.ByteArrayHttpMessageConverter import org.springframework.http.converter.FormHttpMessageConverter import org.springframework.http.converter.StringHttpMessageConverter import org.springframework.mock.http.MockHttpInputMessage +import org.springframework.mock.http.MockHttpOutputMessage import org.springframework.util.MultiValueMap import java.nio.charset.StandardCharsets @@ -90,4 +96,133 @@ class HttpMessageConverterInstrumentationTest extends InstrumentationSpecificati published.getFirst('value') == 'object' published.getFirst('another') == 'value2' } + + // Hand-written AppSecContext stub that records whether reportBlockFailure() was invoked. + private static class RecordingAppSecContext implements AppSecContext { + boolean blockFailureReported = false + + @Override + boolean isManuallyKept() { + return false + } + + @Override + void reportBlockFailure() { + blockFailureReported = true + } + } + + // Hand-written BlockResponseFunction stub whose commit outcome is controlled by the test. + private static class FixedOutcomeBlockResponseFunction implements BlockResponseFunction { + private final boolean commitSucceeds + + FixedOutcomeBlockResponseFunction(boolean commitSucceeds) { + this.commitSucceeds = commitSucceeds + } + + @Override + boolean tryCommitBlockingResponse( + TraceSegment segment, + int statusCode, + BlockingContentType templateType, + Map extraHeaders, + String securityResponseId) { + return commitSucceeds + } + } + + // Activates a span with a recording appsec context, wires the given event to a callback that + // always requests blocking, and configures the block response commit outcome. Returns the + // recording appsec context and the activated scope so the caller can assert on the former and + // close the latter. + private List setupBlockFailureScenario(def event, boolean commitSucceeds) { + def appSecContext = new RecordingAppSecContext() + TagContext ctx = new TagContext().withRequestContextDataAppSec(appSecContext) + def blockedSpan = AgentTracer.startSpan('test', 'test-blocked-span', ctx) + def blockedScope = AgentTracer.activateSpan(blockedSpan) + def blockedReqCtx = blockedSpan.spanContext() as RequestContext + blockedReqCtx.setBlockResponseFunction(new FixedOutcomeBlockResponseFunction(commitSucceeds)) + ss.reset() + ss.registerCallback(event, { RequestContext c, Object body -> + new Flow.ResultFlow(null) { + @Override + Flow.Action getAction() { + return new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO) + } + } + } as BiFunction>) + [appSecContext, blockedScope] + } + + void 'read reports block failure via stub appsec context when commit fails'() { + given: + def (appSecContext, blockedScope) = setupBlockFailureScenario(EVENTS.requestBodyProcessed(), false) + def converter = new FormHttpMessageConverter() + def raw = 'value=object' + def message = new MockHttpInputMessage(raw.getBytes(StandardCharsets.UTF_8)) + message.headers.contentType = MediaType.APPLICATION_FORM_URLENCODED + + when: + converter.read(MultiValueMap, message) + + then: + notThrown(BlockingException) + appSecContext.blockFailureReported == true + + cleanup: + blockedScope?.close() + } + + void 'read does not report block failure when commit succeeds'() { + given: + def (appSecContext, blockedScope) = setupBlockFailureScenario(EVENTS.requestBodyProcessed(), true) + def converter = new FormHttpMessageConverter() + def raw = 'value=object' + def message = new MockHttpInputMessage(raw.getBytes(StandardCharsets.UTF_8)) + message.headers.contentType = MediaType.APPLICATION_FORM_URLENCODED + + when: + converter.read(MultiValueMap, message) + + then: + thrown(BlockingException) + appSecContext.blockFailureReported == false + + cleanup: + blockedScope?.close() + } + + void 'write reports block failure via stub appsec context when commit fails'() { + given: + def (appSecContext, blockedScope) = setupBlockFailureScenario(EVENTS.responseBody(), false) + def converter = new StringHttpMessageConverter() + def message = new MockHttpOutputMessage() + + when: + converter.write('example', MediaType.TEXT_PLAIN, message) + + then: + notThrown(BlockingException) + appSecContext.blockFailureReported == true + + cleanup: + blockedScope?.close() + } + + void 'write does not report block failure when commit succeeds'() { + given: + def (appSecContext, blockedScope) = setupBlockFailureScenario(EVENTS.responseBody(), true) + def converter = new StringHttpMessageConverter() + def message = new MockHttpOutputMessage() + + when: + converter.write('example', MediaType.TEXT_PLAIN, message) + + then: + thrown(BlockingException) + appSecContext.blockFailureReported == false + + cleanup: + blockedScope?.close() + } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy index 89b0389c3da..3d25ffd2509 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -29,6 +29,7 @@ import test.SetupSpecHelper import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +import static org.junit.jupiter.api.Assumptions.assumeTrue import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.FORWARDED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.LOGIN @@ -263,6 +264,36 @@ class SpringBootBasedTest extends HttpServerTest body = null } + def 'test blocking of request for matrix parameters'() { + // This pins the unchanged blocking behavior for TemplateAndMatrixVariablesInstrumentation's + // matrix-variable path. reportBlockFailure() itself is not observable through this black-box + // HTTP test because spring-webmvc does not own its own BlockResponseFunction: it delegates to + // the underlying container's (Tomcat here), whose contract always returns true for a genuine + // attempt. + setup: + assumeTrue(testBlocking()) + + def request = request(MATRIX_PARAM, 'GET', null) + .header(IG_PARAMETERS_BLOCK_HEADER, 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + response.code() == 413 + response.header('Content-type') =~ /(?i)\Aapplication\/json(?:;\s?charset=utf-8)?\z/ + response.body().charStream().text.contains('"title":"You\'ve been blocked"') + TEST_WRITER.waitForTraces(1) + def trace = TEST_WRITER.get(0) + def rootSpan = trace.find { + it.parentId == 0 + } + rootSpan != null + rootSpan.tags['http.status_code'] == 413 + rootSpan.tags['appsec.blocked'] == 'true' + } + def 'template var is pushed to IG'() { setup: def request = request(PATH_PARAM, 'GET', null).header(IG_EXTRA_SPAN_NAME_HEADER, 'appsec-span').build() diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateAndMatrixVariablesInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateAndMatrixVariablesInstrumentation.java index 912c9d78a4a..d4fe95e8af4 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateAndMatrixVariablesInstrumentation.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateAndMatrixVariablesInstrumentation.java @@ -64,7 +64,7 @@ public void methodAdvice(MethodTransformer transformer) { @Override public String[] helperClassNames() { return new String[] { - packageName + ".PairList", + packageName + ".PairList", packageName + ".SpringBlockingHelper", }; } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateVariablesUrlHandlerInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateVariablesUrlHandlerInstrumentation.java index 6d99f64cf09..ab6e996b66c 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateVariablesUrlHandlerInstrumentation.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/TemplateVariablesUrlHandlerInstrumentation.java @@ -59,6 +59,13 @@ public void methodAdvice(MethodTransformer transformer) { packageName + ".InterceptorPreHandleAdvice"); } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".SpringBlockingHelper", + }; + } + @Override public Advice.PostProcessor.Factory postProcessor() { return postProcessorFactory; diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java index 368e91303ad..f48995d73fc 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java @@ -98,11 +98,13 @@ public static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + t = + new BlockingException( + "Blocked request (for RequestMappingInfoHandlerMapping/handleMatch)"); + } } - t = - new BlockingException( - "Blocked request (for RequestMappingInfoHandlerMapping/handleMatch)"); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/InterceptorPreHandleAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/InterceptorPreHandleAdvice.java index 89ac5f23e85..276b0136cb5 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/InterceptorPreHandleAdvice.java +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/InterceptorPreHandleAdvice.java @@ -66,11 +66,13 @@ public static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = SpringBlockingHelper.tryCommitBlockingResponse(brf, reqCtx, rba); + if (success) { + t = + new BlockingException( + "Blocked request (for UriTemplateVariablesHandlerInterceptor/preHandle)"); + } } - t = - new BlockingException( - "Blocked request (for UriTemplateVariablesHandlerInterceptor/preHandle)"); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringBlockingHelper.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringBlockingHelper.java new file mode 100644 index 00000000000..0be5861ba9a --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/SpringBlockingHelper.java @@ -0,0 +1,37 @@ +package datadog.trace.instrumentation.springweb6; + +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class SpringBlockingHelper { + private static final Logger log = LoggerFactory.getLogger(SpringBlockingHelper.class); + + /** + * Wraps {@link BlockResponseFunction#tryCommitBlockingResponse(RequestContext, + * Flow.Action.RequestBlockingAction)} so that an exception thrown by the commit attempt itself + * (rather than a plain {@code false} return) is still reported as a block failure. The advice + * that calls this method runs with {@code suppress = Throwable.class}, so without this guard such + * an exception would propagate out of the advice and be silently swallowed, and the + * default-method reporting inside {@code tryCommitBlockingResponse} would never run. + */ + public static boolean tryCommitBlockingResponse( + BlockResponseFunction blockResponseFunction, + RequestContext reqCtx, + Flow.Action.RequestBlockingAction rba) { + try { + return blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); + } catch (Exception e) { + log.debug("Error committing blocking response", e); + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + return false; + } + } +} diff --git a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataContentHelper.java b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataContentHelper.java index 9901a9b67b9..88bae3a40ef 100644 --- a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataContentHelper.java +++ b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataContentHelper.java @@ -1,6 +1,11 @@ package datadog.trace.instrumentation.undertow; import datadog.trace.api.Config; +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.api.http.MultipartContentDecoder; import datadog.trace.api.internal.VisibleForTesting; import io.undertow.server.handlers.form.FormData; @@ -39,6 +44,30 @@ public final class FormDataContentHelper { FILE_ITEM_GET_INPUT_STREAM = gis; } + /** + * Wraps {@link BlockResponseFunction#tryCommitBlockingResponse(RequestContext, + * Flow.Action.RequestBlockingAction)} so that an exception thrown by the commit attempt itself + * (rather than a plain {@code false} return) is still reported as a block failure. The advice + * that calls this method runs with {@code suppress = Throwable.class}, so without this guard such + * an exception would propagate out of the advice and be silently swallowed, and the + * default-method reporting inside {@code tryCommitBlockingResponse} would never run. + */ + public static boolean tryCommitBlockingResponse( + BlockResponseFunction blockResponseFunction, + RequestContext reqCtx, + Flow.Action.RequestBlockingAction rba) { + try { + return blockResponseFunction.tryCommitBlockingResponse(reqCtx, rba); + } catch (Exception e) { + log.debug("Error committing blocking response", e); + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + return false; + } + } + public static List collectContents(FormData attachment) { List result = new ArrayList<>(MAX_FILES_TO_INSPECT); for (String key : attachment) { diff --git a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataParserInstrumentation.java b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataParserInstrumentation.java index 8f34ab5e9f9..14123aaf6ff 100644 --- a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataParserInstrumentation.java +++ b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/FormDataParserInstrumentation.java @@ -40,7 +40,7 @@ public String instrumentedType() { @Override public String[] helperClassNames() { - return new String[] {packageName + ".FormDataMap"}; + return new String[] {packageName + ".FormDataMap", packageName + ".FormDataContentHelper"}; } private static final Reference EXCHANGE_REFERENCE = @@ -91,8 +91,9 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (t == null) { + boolean success = + FormDataContentHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); + if (success && t == null) { t = new BlockingException("Blocked request (for FormEncodedDataParser/doParse)"); } } diff --git a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/MultiPartUploadHandlerInstrumentation.java b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/MultiPartUploadHandlerInstrumentation.java index 3f0313464d3..9018763ceca 100644 --- a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/MultiPartUploadHandlerInstrumentation.java +++ b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/MultiPartUploadHandlerInstrumentation.java @@ -67,6 +67,7 @@ public void methodAdvice(MethodTransformer transformer) { @RequiresRequestContext(RequestContextSlot.APPSEC) public static class ParseBlockingAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) static boolean onEnter(@Advice.FieldValue("exchange") HttpServerExchange exchange) { return exchange.getAttachment(FORM_DATA) == null; @@ -105,7 +106,7 @@ static void after( BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { boolean success = - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + FormDataContentHelper.tryCommitBlockingResponse(blockResponseFunction, reqCtx, rba); if (success && t == null) { t = new BlockingException( @@ -133,7 +134,7 @@ static void after( (Flow.Action.RequestBlockingAction) filenamesAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null && t == null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = FormDataContentHelper.tryCommitBlockingResponse(brf, reqCtx, rba); if (success) { t = new BlockingException("Blocked request (multipart file upload)"); } @@ -152,7 +153,7 @@ static void after( (Flow.Action.RequestBlockingAction) contentAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null && t == null) { - boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + boolean success = FormDataContentHelper.tryCommitBlockingResponse(brf, reqCtx, rba); if (success) { t = new BlockingException("Blocked request (multipart file upload content)"); } diff --git a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/test/groovy/UndertowServletTest.groovy b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/test/groovy/UndertowServletTest.groovy index 846389417ba..54b39c886c0 100644 --- a/dd-java-agent/instrumentation/undertow/undertow-2.0/src/test/groovy/UndertowServletTest.groovy +++ b/dd-java-agent/instrumentation/undertow/undertow-2.0/src/test/groovy/UndertowServletTest.groovy @@ -2,7 +2,9 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.agent.test.base.WebsocketServer import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions +import datadog.trace.bootstrap.blocking.BlockingActionHelper import datadog.trace.bootstrap.instrumentation.api.Tags +import datadog.trace.core.DDSpan import io.undertow.Handlers import io.undertow.Undertow import io.undertow.UndertowOptions @@ -11,11 +13,17 @@ import io.undertow.servlet.api.DeploymentManager import io.undertow.servlet.api.ServletContainer import io.undertow.servlet.api.ServletInfo import io.undertow.websockets.jsr.WebSocketDeploymentInfo +import okhttp3.MediaType +import okhttp3.RequestBody import spock.lang.IgnoreIf import javax.servlet.MultipartConfigElement import java.nio.ByteBuffer +import static datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType.JSON +import static java.nio.charset.StandardCharsets.UTF_8 +import static org.junit.jupiter.api.Assumptions.assumeTrue + import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CREATED @@ -38,6 +46,15 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.WEBSOC abstract class UndertowServletTest extends HttpServerTest { private static final CONTEXT = "ctx" + private final static String MULTIPART_CONTENT_TYPE = + 'multipart/form-data; charset=utf-8; boundary=------------------------943d3207457896a3' + private final static String MULTIPART_BODY = + '--------------------------943d3207457896a3\r\n' + + 'Content-Disposition: form-data; name="a"\r\n' + + '\r\n' + + 'x\r\n' + + '--------------------------943d3207457896a3--' + class UndertowServer implements WebsocketServer { def port = 0 Undertow undertowServer @@ -314,6 +331,56 @@ abstract class UndertowServletTest extends HttpServerTest { body = null } + // Pins the block-telemetry-3 wiring added to FormDataParserInstrumentation (urlencoded body, + // DoParseAdvice.after) and MultiPartUploadHandlerInstrumentation (multipart body, + // ParseBlockingAdvice.after): both now check the boolean returned by + // BlockResponseFunction#tryCommitBlockingResponse and report a block failure to AppSecContext + // when it returns false. In this test the reportBlockFailure() branch itself stays unreachable, + // because UndertowBlockResponseFunction#tryCommitBlockingResponse always returns true + // unconditionally, even on its async dispatch path (see .claude-invariants.md). So this test + // only pins the unchanged, already-passing blocking behavior for both request body variants + // post block-telemetry-3, the same kind of documented gap as Netty's block-failure tests. + def "test blocking of request body parsed by undertow for variant #variant"() { + setup: + assumeTrue(testBlocking()) + assumeTrue(executeTest) + + def request = request( + endpoint, 'POST', + RequestBody.create(MediaType.get(contentType), body)) + .header(IG_BODY_CONVERTED_HEADER, 'true') + .build() + + when: + def response = client.newCall(request).execute() + + then: + response.code() == 413 + response.header('Content-type') =~ /(?i)\Aapplication\/json(?:;\s?charset=(?:utf-8|iso-8859-1))?\z/ + + def text = response.body().charStream().text + text.contains('"title":"You\'ve been blocked"') + text.getBytes(UTF_8).length == BlockingActionHelper.getTemplate(JSON).length + + !handlerRan + + TEST_WRITER.waitForTraces(1) + + then: + List spans = TEST_WRITER.flatten() + spans.find { + it.tags['http.status_code'] == 413 + } != null + spans.find { + it.tags['appsec.blocked'] == 'true' + } != null + + where: + variant | executeTest | endpoint | contentType | body + 'urlencoded' | testBodyUrlencoded() | BODY_URLENCODED | 'application/x-www-form-urlencoded' | 'a=x' + 'multipart' | testBodyMultipart() | BODY_MULTIPART | MULTIPART_CONTENT_TYPE | MULTIPART_BODY + } + @Override void handlerSpan(TraceAssert trace, ServerEndpoint endpoint = SUCCESS) { trace.span {