From 803a700cdbf2a0452814b0e309bdd51f4434e8cd Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 09:55:03 +0200 Subject: [PATCH 1/6] Add block-outcome telemetry for Tomcat blocking enforcement failures Report block_failure on appsec.waf.requests when Tomcat's blocking-commit path fails to enforce a decided block, mirroring the Netty implementation (#12316). Adds a shared TomcatBlockingHelper.tryCommitAndReport choke point used by all Tomcat/GlassFish blocking-commit call sites (5.5/6.0/7.0), and guards it against exceptions thrown by the registered BlockResponseFunction. --- .../tomcat/TomcatServerInstrumentation.java | 7 +- .../tomcat55/CommitActionInstrumentation.java | 3 +- .../ParsedBodyParametersInstrumentation.java | 3 +- .../tomcat-appsec-6.0/build.gradle | 1 + .../ParsedBodyParametersInstrumentation.java | 13 +- .../tomcat7/CommitActionInstrumentation.java | 3 +- .../tomcat7/GlassFishBlockingHelper.java | 17 +- .../GlassFishMultipartInstrumentation.java | 2 + .../tomcat7/ParsePartsInstrumentation.java | 9 +- .../tomcat7/GlassFishBlockingHelperTest.java | 96 +++++++++- .../tomcat/tomcat-common/build.gradle | 7 + .../tomcat/TomcatBlockingHelper.java | 67 +++++++ .../tomcat/TomcatBlockingHelperTest.java | 181 ++++++++++++++++++ 13 files changed, 381 insertions(+), 28 deletions(-) create mode 100644 dd-java-agent/instrumentation/tomcat/tomcat-common/src/test/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelperTest.java diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java index 14fb92e8fae..00148602615 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java @@ -186,9 +186,7 @@ public static class PostParseAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void afterParse( - @Advice.Argument(1) Request req, - @Advice.Argument(3) Response resp, - @Advice.Return(readOnly = false) Boolean ret) { + @Advice.Argument(1) Request req, @Advice.Return(readOnly = false) Boolean ret) { Object contextObj = req.getAttribute(DD_CONTEXT_ATTRIBUTE); if (contextObj instanceof Context) { Context context = (Context) contextObj; @@ -202,8 +200,7 @@ public static void afterParse( DECORATE.onRequest(span, req, req, parentContext); Flow.Action.RequestBlockingAction rba = span.getRequestBlockingAction(); if (rba != null) { - TomcatBlockingHelper.commitBlockingResponse( - span.getRequestContext().getTraceSegment(), req, resp, rba); + TomcatBlockingHelper.tryCommitAndReport(span.getRequestContext(), rba); ret = false; // skip pipeline } } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java index b5e74a8751b..0f0bd6bd395 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java @@ -15,6 +15,7 @@ import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import datadog.trace.instrumentation.tomcat.ExtractAdapter; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import datadog.trace.instrumentation.tomcat.TomcatDecorator; import net.bytebuddy.asm.Advice; import org.apache.coyote.ActionCode; @@ -114,7 +115,7 @@ static class ProcessCommitActionAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(requestContext, rba); thiz.action(ActionCode.ACTION_CLOSE, null); return true; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java index be98fe79c12..a350477b240 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java @@ -20,6 +20,7 @@ import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import java.util.Hashtable; import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; @@ -143,7 +144,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for processParameters)"); } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/build.gradle b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/build.gradle index 67cb1a7ea40..fa2240c2dcf 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/build.gradle +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/build.gradle @@ -25,6 +25,7 @@ muzzle { dependencies { compileOnly group: 'org.apache.tomcat', name: 'coyote', version: '6.0.53' + implementation project(':dd-java-agent:instrumentation:tomcat:tomcat-common') } // testing happens in tomcat-5.5 module diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java index 6a6aebeec7b..3ad8935bbf2 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java @@ -12,13 +12,13 @@ import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.agent.tooling.muzzle.Reference; -import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import java.util.ArrayList; import java.util.HashMap; import java.util.Map; @@ -129,15 +129,8 @@ static void after( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); - if (blockResponseFunction != null) { - boolean committedBlockingResponse = - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (committedBlockingResponse) { - if (t == null) { - t = new BlockingException("Blocked request (for processParameters)"); - } - } + if (TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba) && t == null) { + t = new BlockingException("Blocked request (for processParameters)"); } } } finally { diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java index 330c40a68d2..678c2935bf6 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java @@ -16,6 +16,7 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import datadog.trace.instrumentation.tomcat.ExtractAdapter; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import datadog.trace.instrumentation.tomcat.TomcatDecorator; import net.bytebuddy.asm.Advice; import org.apache.coyote.ActionCode; @@ -121,7 +122,7 @@ static class ProcessCommitActionAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(requestContext, rba); thiz.action(ActionCode.CLOSE, null); return true; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java index 78a06148a71..4bce115f855 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java @@ -7,6 +7,7 @@ import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.http.MultipartContentDecoder; import datadog.trace.bootstrap.blocking.BlockingActionHelper; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; @@ -16,9 +17,13 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.Part; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public final class GlassFishBlockingHelper { + private static final Logger log = LoggerFactory.getLogger(GlassFishBlockingHelper.class); + public static final int MAX_FILE_CONTENT_COUNT = Config.get().getAppSecMaxFileContentCount(); public static final int MAX_FILE_CONTENT_BYTES = Config.get().getAppSecMaxFileContentBytes(); @@ -38,12 +43,18 @@ public static boolean tryBlock( try { BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + // tryCommitAndReport already reports the block failure when the commit fails + if (!TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba)) { + return false; + } } else if (!commitBlocking(fallbackReq, fallbackResp, rba)) { + TomcatBlockingHelper.reportBlockFailure(reqCtx); return false; } - } catch (Exception ignored) { - // commit failed — response not sent, cannot block this request + } catch (Exception e) { + // commit failed - response not sent, cannot block this request + log.debug("Error committing blocking response", e); + TomcatBlockingHelper.reportBlockFailure(reqCtx); return false; } // Response was committed — mark as blocked on a best-effort basis. diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java index 74dde6d6b38..f31c73285b8 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java @@ -50,6 +50,8 @@ public String instrumentedType() { @Override public String[] helperClassNames() { return new String[] { + // referenced by GlassFishBlockingHelper.tryBlock() to commit the blocking response + "datadog.trace.instrumentation.tomcat.TomcatBlockingHelper", "datadog.trace.instrumentation.tomcat7.GlassFishBlockingHelper", }; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java index 1799a9cbaa9..9abac02c4ef 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java @@ -17,6 +17,7 @@ import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import java.util.List; import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; @@ -56,6 +57,8 @@ public String instrumentedType() { @Override public String[] helperClassNames() { return new String[] { + // referenced by the inlined advice below to commit the blocking response + "datadog.trace.instrumentation.tomcat.TomcatBlockingHelper", "datadog.trace.instrumentation.tomcat7.ParameterCollector", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorNoop", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorImpl", @@ -122,7 +125,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (for Request/parseParts)"); reqCtx.getTraceSegment().effectivelyBlocked(); } @@ -142,7 +145,7 @@ static void after( (Flow.Action.RequestBlockingAction) filenamesAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (multipart file upload)"); reqCtx.getTraceSegment().effectivelyBlocked(); } @@ -163,7 +166,7 @@ static void after( (Flow.Action.RequestBlockingAction) contentAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (multipart file upload content)"); reqCtx.getTraceSegment().effectivelyBlocked(); } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java index e4c4a953c17..c23aac4310e 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java @@ -5,6 +5,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.contains; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -12,9 +13,11 @@ import static org.mockito.Mockito.when; import datadog.appsec.api.blocking.BlockingContentType; +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 java.io.ByteArrayOutputStream; import java.io.IOException; @@ -120,7 +123,7 @@ void commitBlocking_ioException_returnsFalse() throws IOException { @Test void tryBlock_withBrf_commitsViaFunctionAndReturnsTrue() throws Exception { TraceSegment segment = mock(TraceSegment.class); - BlockResponseFunction brf = mock(BlockResponseFunction.class); + BlockResponseFunction brf = mockCommittingBrf(); RequestContext reqCtx = mockReqCtx(brf, segment); Flow.Action.RequestBlockingAction action = rba(403); @@ -168,13 +171,83 @@ void tryBlock_brfThrows_returnsFalse() throws Exception { @Test void tryBlock_effectivelyBlockedThrows_stillReturnsTrue() throws Exception { TraceSegment segment = mock(TraceSegment.class); - BlockResponseFunction brf = mock(BlockResponseFunction.class); + BlockResponseFunction brf = mockCommittingBrf(); RequestContext reqCtx = mockReqCtx(brf, segment); doThrow(new RuntimeException("span already finished")).when(segment).effectivelyBlocked(); assertTrue(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); } + // ------- tryBlock() block_failure telemetry ------- + + @Test + void tryBlock_brfCommitFails_reportsBlockFailure() { + TraceSegment segment = mock(TraceSegment.class); + BlockResponseFunction brf = mock(BlockResponseFunction.class); // tryCommit... returns false + RequestContext reqCtx = mockReqCtx(brf, segment); + AppSecContext appSecCtx = mockAppSecCtx(reqCtx); + + assertFalse(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); + + verify(appSecCtx).reportBlockFailure(); + } + + @Test + void tryBlock_brfCommitFails_doesNotMarkEffectivelyBlocked() { + TraceSegment segment = mock(TraceSegment.class); + BlockResponseFunction brf = mock(BlockResponseFunction.class); // tryCommit... returns false + RequestContext reqCtx = mockReqCtx(brf, segment); + mockAppSecCtx(reqCtx); + + assertFalse(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); + + verify(segment, never()).effectivelyBlocked(); + } + + @Test + void tryBlock_noBrf_fallbackCommitFails_reportsBlockFailure() { + TraceSegment segment = mock(TraceSegment.class); + RequestContext reqCtx = mockReqCtx(null, segment); + AppSecContext appSecCtx = mockAppSecCtx(reqCtx); + // an already committed response makes commitBlocking() return false + HttpServletResponse resp = mock(HttpServletResponse.class); + when(resp.isCommitted()).thenReturn(true); + + assertFalse(GlassFishBlockingHelper.tryBlock(reqCtx, null, resp, rba(403))); + + verify(appSecCtx).reportBlockFailure(); + verify(segment, never()).effectivelyBlocked(); + } + + @Test + void tryBlock_commitThrows_reportsBlockFailure() throws Exception { + TraceSegment segment = mock(TraceSegment.class); + BlockResponseFunction brf = mock(BlockResponseFunction.class); + RequestContext reqCtx = mockReqCtx(brf, segment); + AppSecContext appSecCtx = mockAppSecCtx(reqCtx); + doThrow(new RuntimeException("commit failed")) + .when(brf) + .tryCommitBlockingResponse(any(), any(Flow.Action.RequestBlockingAction.class)); + + assertFalse(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); + + verify(appSecCtx).reportBlockFailure(); + verify(segment, never()).effectivelyBlocked(); + } + + @Test + void tryBlock_success_doesNotReportBlockFailure() { + TraceSegment segment = mock(TraceSegment.class); + BlockResponseFunction brf = mockCommittingBrf(); + RequestContext reqCtx = mockReqCtx(brf, segment); + AppSecContext appSecCtx = mockAppSecCtx(reqCtx); + + assertTrue(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); + + verify(appSecCtx, never()).reportBlockFailure(); + verify(segment).effectivelyBlocked(); + } + // ------- processPartsAndBlock() ------- @Test @@ -271,7 +344,7 @@ void processPartsAndBlock_getInputStreamThrows_emptyStringFallback() throws Exce void processPartsAndBlock_filenamesCbBlocks_contentCbNotFired() throws Exception { Part filePart = mockPart("evil.exe", "application/octet-stream", "content".getBytes()); TraceSegment segment = mock(TraceSegment.class); - BlockResponseFunction brf = mock(BlockResponseFunction.class); + BlockResponseFunction brf = mockCommittingBrf(); RequestContext reqCtx = mockReqCtx(brf, segment); BiFunction, Flow> filenamesCb = mockBlockingCb(403); BiFunction, Flow> contentCb = mockPassThroughCb(); @@ -287,7 +360,7 @@ void processPartsAndBlock_filenamesCbBlocks_contentCbNotFired() throws Exception void processPartsAndBlock_contentCbBlocks_returnsTrue() throws Exception { Part filePart = mockPart("upload.bin", "application/octet-stream", "payload".getBytes()); TraceSegment segment = mock(TraceSegment.class); - BlockResponseFunction brf = mock(BlockResponseFunction.class); + BlockResponseFunction brf = mockCommittingBrf(); RequestContext reqCtx = mockReqCtx(brf, segment); BiFunction, Flow> filenamesCb = mockPassThroughCb(); BiFunction, Flow> contentCb = mockBlockingCb(403); @@ -322,6 +395,21 @@ private static RequestContext mockReqCtx(BlockResponseFunction brf, TraceSegment return reqCtx; } + /** A {@link BlockResponseFunction} whose commit attempt succeeds. */ + private static BlockResponseFunction mockCommittingBrf() { + BlockResponseFunction brf = mock(BlockResponseFunction.class); + when(brf.tryCommitBlockingResponse(any(), any(Flow.Action.RequestBlockingAction.class))) + .thenReturn(true); + return brf; + } + + /** Binds an {@link AppSecContext} mock to the APPSEC slot of the given request context. */ + private static AppSecContext mockAppSecCtx(RequestContext reqCtx) { + AppSecContext appSecCtx = mock(AppSecContext.class); + doReturn(appSecCtx).when(reqCtx).getData(RequestContextSlot.APPSEC); + return appSecCtx; + } + private static Part mockPart(String submittedFilename, String contentType, byte[] content) throws Exception { Part part = mock(Part.class); diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/build.gradle b/dd-java-agent/instrumentation/tomcat/tomcat-common/build.gradle index e6f844fb59f..100d8738bdb 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-common/build.gradle +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/build.gradle @@ -18,4 +18,11 @@ dependencies { // Version that corresponds with Tomcat 5.5 // https://tomcat.apache.org/whichversion.html compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.4' + + testImplementation group: 'tomcat', name: 'catalina', version: tomcatVersion + testImplementation group: 'tomcat', name: 'tomcat-coyote', version: tomcatVersion + testImplementation group: 'tomcat', name: 'tomcat-util', version: tomcatVersion + testImplementation group: 'javax.servlet', name: 'servlet-api', version: '2.4' + testImplementation libs.bundles.junit5 + testImplementation libs.bundles.mockito } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java index aab5ea6f0b0..38135c4b2df 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java @@ -1,10 +1,16 @@ package datadog.trace.instrumentation.tomcat; import datadog.appsec.api.blocking.BlockingContentType; +import datadog.context.Context; +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.blocking.BlockingActionHelper; import datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import java.io.IOException; import java.io.OutputStream; @@ -34,6 +40,65 @@ public class TomcatBlockingHelper { GET_OUTPUT_STREAM = mh; } + /** + * Commits a blocking response through the {@link BlockResponseFunction} registered on the given + * request context and, if the commit fails, reports the failure via {@link + * AppSecContext#reportBlockFailure()}. + * + *

This is the single choke point shared by all Tomcat blocking call sites so the {@code + * block_failure} telemetry is not duplicated inline. + * + * @return {@code true} if the blocking response was committed, {@code false} otherwise (including + * when no {@link BlockResponseFunction} is registered, in which case nothing was attempted + * and no failure is reported, or when the commit attempt threw). + */ + public static boolean tryCommitAndReport( + RequestContext reqCtx, Flow.Action.RequestBlockingAction rba) { + BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); + if (brf == null) { + // nothing was attempted, so this is not a block failure + return false; + } + try { + if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + return true; + } + } catch (Exception e) { + log.debug("Error committing blocking response", e); + reportBlockFailure(reqCtx); + return false; + } + reportBlockFailure(reqCtx); + return false; + } + + /** Reports a block failure on the AppSec context bound to the given request context, if any. */ + public static void reportBlockFailure(RequestContext reqCtx) { + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + } + + /** + * Reports a block failure on the AppSec context bound to the given Tomcat request, if the request + * still carries a datadog context with an active span. + */ + private static void reportBlockFailure(Request request) { + Object contextObj = request.getAttribute(HttpServerDecorator.DD_CONTEXT_ATTRIBUTE); + if (!(contextObj instanceof Context)) { + return; + } + AgentSpan span = AgentSpan.fromContext((Context) contextObj); + if (span == null) { + return; + } + RequestContext reqCtx = span.getRequestContext(); + if (reqCtx != null) { + reportBlockFailure(reqCtx); + } + } + public static void commitBlockingResponse( TraceSegment segment, Request request, Response resp, Flow.Action.RequestBlockingAction rba) { commitBlockingResponse( @@ -55,6 +120,8 @@ public static boolean commitBlockingResponse( Map extraHeaders, String securityResponseId) { if (GET_OUTPUT_STREAM == null) { + // a commit was genuinely attempted, but the JVM-wide reflection lookup failed at class init + reportBlockFailure(request); return false; } int httpCode = BlockingActionHelper.getHttpCode(statusCode); diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/test/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelperTest.java b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/test/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelperTest.java new file mode 100644 index 00000000000..66ff3d6cf38 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/test/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelperTest.java @@ -0,0 +1,181 @@ +package datadog.trace.instrumentation.tomcat; + +import static java.util.Collections.emptyMap; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import datadog.appsec.api.blocking.BlockingContentType; +import datadog.context.Context; +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; +import java.lang.invoke.MethodHandle; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import org.apache.catalina.connector.Request; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +/** + * Tests the {@code GET_OUTPUT_STREAM == null} degraded path of {@link TomcatBlockingHelper}, i.e. + * the case where the JVM-wide reflective lookup of {@code Response#getOutputStream()} failed at + * class initialization: a blocking commit was genuinely attempted, so a {@code block_failure} must + * be reported on the AppSec context reachable from the Tomcat request. + * + *

On a real classpath {@code Response#getOutputStream()} always exists, so the field is never + * {@code null} and the branch is unreachable by normal means. There is no production test hook (and + * adding one is out of scope), so the tests temporarily overwrite the {@code private static final} + * field and restore it afterwards. {@code Field#set} refuses static finals even after {@code + * setAccessible(true)}, and {@code Lookup#unreflectVarHandle} refuses write access to them as well, + * so the write goes through {@code sun.misc.Unsafe}, accessed purely reflectively (the same style + * already used in {@code dd-smoke-tests/crashtracking}). + */ +class TomcatBlockingHelperTest { + + private static final Field FIELD; + private static final MethodHandle ORIGINAL_VALUE; + private static final Object UNSAFE; + private static final Method PUT_OBJECT; + private static final Object STATIC_BASE; + private static final long STATIC_OFFSET; + + static { + try { + FIELD = TomcatBlockingHelper.class.getDeclaredField("GET_OUTPUT_STREAM"); + FIELD.setAccessible(true); + // also forces class initialization, so the static initializer cannot overwrite our writes + ORIGINAL_VALUE = (MethodHandle) FIELD.get(null); + + Class unsafeClass = Class.forName("sun.misc.Unsafe"); + Field theUnsafe = unsafeClass.getDeclaredField("theUnsafe"); + theUnsafe.setAccessible(true); + UNSAFE = theUnsafe.get(null); + STATIC_BASE = unsafeClass.getMethod("staticFieldBase", Field.class).invoke(UNSAFE, FIELD); + STATIC_OFFSET = + (Long) unsafeClass.getMethod("staticFieldOffset", Field.class).invoke(UNSAFE, FIELD); + PUT_OBJECT = unsafeClass.getMethod("putObject", Object.class, long.class, Object.class); + } catch (Exception e) { + throw new ExceptionInInitializerError(e); + } + } + + private static void setLookup(MethodHandle value) { + try { + PUT_OBJECT.invoke(UNSAFE, STATIC_BASE, STATIC_OFFSET, value); + } catch (Exception e) { + throw new IllegalStateException("Unable to overwrite GET_OUTPUT_STREAM", e); + } + } + + @AfterEach + void restoreLookup() { + setLookup(ORIGINAL_VALUE); + } + + @Test + void sanityCheck_lookupSucceedsOnARealClasspath() { + assertNotNull(ORIGINAL_VALUE); + } + + @Test + void commitBlockingResponse_lookupFailed_reportsBlockFailureAndReturnsFalse() { + setLookup(null); + + AppSecContext appSecCtx = mock(AppSecContext.class); + RequestContext reqCtx = mock(RequestContext.class); + when(reqCtx.getData(RequestContextSlot.APPSEC)).thenReturn(appSecCtx); + Request request = mockRequestWithSpan(mockSpan(reqCtx)); + TraceSegment segment = mock(TraceSegment.class); + + assertFalse(commitBlockingResponse(segment, request)); + + verify(appSecCtx).reportBlockFailure(); + verifyNoInteractions(segment); + } + + @Test + void commitBlockingResponse_lookupFailed_noDatadogContextOnRequest_doesNotReport() { + setLookup(null); + + Request request = mock(Request.class); + when(request.getAttribute(HttpServerDecorator.DD_CONTEXT_ATTRIBUTE)).thenReturn(null); + TraceSegment segment = mock(TraceSegment.class); + + assertFalse(commitBlockingResponse(segment, request)); + + verifyNoInteractions(segment); + } + + @Test + void commitBlockingResponse_lookupFailed_noSpanInContext_doesNotReport() { + setLookup(null); + + Request request = mockRequestWithSpan(null); + + assertFalse(commitBlockingResponse(mock(TraceSegment.class), request)); + } + + @Test + void commitBlockingResponse_lookupFailed_noRequestContextOnSpan_doesNotReport() { + setLookup(null); + + AgentSpan span = mock(AgentSpan.class); + when(span.getRequestContext()).thenReturn(null); + + assertFalse(commitBlockingResponse(mock(TraceSegment.class), mockRequestWithSpan(span))); + } + + @Test + void commitBlockingResponse_lookupFailed_appsecDataIsNotAnAppSecContext_doesNotReport() { + setLookup(null); + + RequestContext reqCtx = mock(RequestContext.class); + when(reqCtx.getData(RequestContextSlot.APPSEC)).thenReturn("not an AppSecContext"); + Request request = mockRequestWithSpan(mockSpan(reqCtx)); + + assertFalse(commitBlockingResponse(mock(TraceSegment.class), request)); + + verify(reqCtx).getData(RequestContextSlot.APPSEC); + } + + @Test + void commitBlockingResponse_lookupFailed_doesNotTouchTheResponse() { + setLookup(null); + + Request request = mock(Request.class); + + assertFalse(commitBlockingResponse(mock(TraceSegment.class), request)); + + // the early return happens before any status/header/attribute mutation + verify(request, never()).setAttribute(any(), any()); + } + + private static boolean commitBlockingResponse(TraceSegment segment, Request request) { + // the response is never touched on this path, hence null + return TomcatBlockingHelper.commitBlockingResponse( + segment, request, null, 403, BlockingContentType.AUTO, emptyMap(), null); + } + + private static AgentSpan mockSpan(RequestContext reqCtx) { + AgentSpan span = mock(AgentSpan.class); + when(span.getRequestContext()).thenReturn(reqCtx); + return span; + } + + private static Request mockRequestWithSpan(AgentSpan span) { + Context context = mock(Context.class); + when(context.get(any())).thenReturn(span); + Request request = mock(Request.class); + when(request.getAttribute(HttpServerDecorator.DD_CONTEXT_ATTRIBUTE)).thenReturn(context); + return request; + } +} From b6b1546544ff205f0dc2cc508fc4211d3398f87c Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 10:46:16 +0200 Subject: [PATCH 2/6] Split TomcatBlockingHelper to fix muzzle failures in block-outcome telemetry Extract tryCommitAndReport()/reportBlockFailure(RequestContext) into a new catalina-independent BlockFailureReporter class in tomcat-common, since helperClassNames() injects the whole class and muzzle validates all its references against every declared library version - including the catalina-independent code path that doesn't need it. --- .../tomcat/RequestInstrumentation.java | 1 + .../tomcat/ResponseInstrumentation.java | 1 + .../tomcat/TomcatServerInstrumentation.java | 3 +- .../tomcat55/CommitActionInstrumentation.java | 5 +- .../ParsedBodyParametersInstrumentation.java | 9 ++- .../ParsedBodyParametersInstrumentation.java | 9 ++- .../tomcat7/CommitActionInstrumentation.java | 5 +- .../tomcat7/GlassFishBlockingHelper.java | 8 +-- .../GlassFishMultipartInstrumentation.java | 2 +- .../tomcat7/ParsePartsInstrumentation.java | 10 +-- .../tomcat/BlockFailureReporter.java | 65 +++++++++++++++++++ .../tomcat/TomcatBlockingHelper.java | 45 +------------ 12 files changed, 100 insertions(+), 63 deletions(-) create mode 100644 dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/RequestInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/RequestInstrumentation.java index 9265724a132..781965a1207 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/RequestInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/RequestInstrumentation.java @@ -54,6 +54,7 @@ public String[] helperClassNames() { packageName + ".TomcatDecorator", packageName + ".TomcatDecorator$TomcatBlockResponseFunction", packageName + ".TomcatBlockingHelper", + packageName + ".BlockFailureReporter", packageName + ".RequestURIDataAdapter", }; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/ResponseInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/ResponseInstrumentation.java index d9acec40e32..df1f7f5d5d1 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/ResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/ResponseInstrumentation.java @@ -38,6 +38,7 @@ public String[] helperClassNames() { packageName + ".TomcatDecorator", packageName + ".TomcatDecorator$TomcatBlockResponseFunction", packageName + ".TomcatBlockingHelper", + packageName + ".BlockFailureReporter", packageName + ".RequestURIDataAdapter", }; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java index 00148602615..4235863358b 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/main/java/datadog/trace/instrumentation/tomcat/TomcatServerInstrumentation.java @@ -56,6 +56,7 @@ public String[] helperClassNames() { packageName + ".TomcatDecorator$TomcatBlockResponseFunction", packageName + ".RequestURIDataAdapter", packageName + ".TomcatBlockingHelper", + packageName + ".BlockFailureReporter", }; } @@ -200,7 +201,7 @@ public static void afterParse( DECORATE.onRequest(span, req, req, parentContext); Flow.Action.RequestBlockingAction rba = span.getRequestBlockingAction(); if (rba != null) { - TomcatBlockingHelper.tryCommitAndReport(span.getRequestContext(), rba); + BlockFailureReporter.tryCommitAndReport(span.getRequestContext(), rba); ret = false; // skip pipeline } } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java index 0f0bd6bd395..983b40b924f 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/CommitActionInstrumentation.java @@ -14,8 +14,8 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import datadog.trace.instrumentation.tomcat.ExtractAdapter; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import datadog.trace.instrumentation.tomcat.TomcatDecorator; import net.bytebuddy.asm.Advice; import org.apache.coyote.ActionCode; @@ -76,6 +76,7 @@ public String[] helperClassNames() { pkg + ".TomcatDecorator", pkg + ".TomcatDecorator$TomcatBlockResponseFunction", pkg + ".TomcatBlockingHelper", + pkg + ".BlockFailureReporter", pkg + ".RequestURIDataAdapter", }; } @@ -115,7 +116,7 @@ static class ProcessCommitActionAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - TomcatBlockingHelper.tryCommitAndReport(requestContext, rba); + BlockFailureReporter.tryCommitAndReport(requestContext, rba); thiz.action(ActionCode.ACTION_CLOSE, null); return true; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java index a350477b240..13dd6962148 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-5.5/src/main/java/datadog/trace/instrumentation/tomcat55/ParsedBodyParametersInstrumentation.java @@ -20,7 +20,7 @@ import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import java.util.Hashtable; import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; @@ -66,6 +66,11 @@ public Reference[] additionalMuzzleReferences() { return new Reference[] {PARAM_HASH_STRING_ARRAY_REFERENCE}; } + @Override + public String[] helperClassNames() { + return new String[] {"datadog.trace.instrumentation.tomcat.BlockFailureReporter"}; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -144,7 +149,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); + BlockFailureReporter.tryCommitAndReport(reqCtx, rba); if (t == null) { t = new BlockingException("Blocked request (for processParameters)"); } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java index 3ad8935bbf2..7d536179f64 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-6.0/src/main/java/datadog/trace/instrumentation/tomcat6/ParsedBodyParametersInstrumentation.java @@ -18,7 +18,7 @@ import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import java.util.ArrayList; import java.util.HashMap; import java.util.Map; @@ -53,6 +53,11 @@ public Reference[] additionalMuzzleReferences() { return new Reference[] {PARAM_HASH_VALUES_MAP_REFERENCE}; } + @Override + public String[] helperClassNames() { + return new String[] {"datadog.trace.instrumentation.tomcat.BlockFailureReporter"}; + } + @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( @@ -129,7 +134,7 @@ static void after( Flow.Action action = flow.getAction(); if (action instanceof Flow.Action.RequestBlockingAction) { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - if (TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba) && t == null) { + if (BlockFailureReporter.tryCommitAndReport(reqCtx, rba) && t == null) { t = new BlockingException("Blocked request (for processParameters)"); } } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java index 678c2935bf6..f36a55dff07 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/CommitActionInstrumentation.java @@ -15,8 +15,8 @@ import datadog.trace.api.gateway.RequestContext; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import datadog.trace.instrumentation.tomcat.ExtractAdapter; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; import datadog.trace.instrumentation.tomcat.TomcatDecorator; import net.bytebuddy.asm.Advice; import org.apache.coyote.ActionCode; @@ -77,6 +77,7 @@ public String[] helperClassNames() { pkg + ".TomcatDecorator", pkg + ".TomcatDecorator$TomcatBlockResponseFunction", pkg + ".TomcatBlockingHelper", + pkg + ".BlockFailureReporter", pkg + ".RequestURIDataAdapter", }; } @@ -122,7 +123,7 @@ static class ProcessCommitActionAdvice { Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = requestContext.getBlockResponseFunction(); if (brf != null) { - TomcatBlockingHelper.tryCommitAndReport(requestContext, rba); + BlockFailureReporter.tryCommitAndReport(requestContext, rba); thiz.action(ActionCode.CLOSE, null); return true; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java index 4bce115f855..d5af7fc0bd4 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java @@ -7,7 +7,7 @@ import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.http.MultipartContentDecoder; import datadog.trace.bootstrap.blocking.BlockingActionHelper; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; @@ -44,17 +44,17 @@ public static boolean tryBlock( BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { // tryCommitAndReport already reports the block failure when the commit fails - if (!TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba)) { + if (!BlockFailureReporter.tryCommitAndReport(reqCtx, rba)) { return false; } } else if (!commitBlocking(fallbackReq, fallbackResp, rba)) { - TomcatBlockingHelper.reportBlockFailure(reqCtx); + BlockFailureReporter.reportBlockFailure(reqCtx); return false; } } catch (Exception e) { // commit failed - response not sent, cannot block this request log.debug("Error committing blocking response", e); - TomcatBlockingHelper.reportBlockFailure(reqCtx); + BlockFailureReporter.reportBlockFailure(reqCtx); return false; } // Response was committed — mark as blocked on a best-effort basis. diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java index f31c73285b8..27d3d4376cc 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishMultipartInstrumentation.java @@ -51,7 +51,7 @@ public String instrumentedType() { public String[] helperClassNames() { return new String[] { // referenced by GlassFishBlockingHelper.tryBlock() to commit the blocking response - "datadog.trace.instrumentation.tomcat.TomcatBlockingHelper", + "datadog.trace.instrumentation.tomcat.BlockFailureReporter", "datadog.trace.instrumentation.tomcat7.GlassFishBlockingHelper", }; } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java index 9abac02c4ef..f884816cafb 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java @@ -17,7 +17,7 @@ import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import datadog.trace.instrumentation.tomcat.TomcatBlockingHelper; +import datadog.trace.instrumentation.tomcat.BlockFailureReporter; import java.util.List; import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; @@ -58,7 +58,7 @@ public String instrumentedType() { public String[] helperClassNames() { return new String[] { // referenced by the inlined advice below to commit the blocking response - "datadog.trace.instrumentation.tomcat.TomcatBlockingHelper", + "datadog.trace.instrumentation.tomcat.BlockFailureReporter", "datadog.trace.instrumentation.tomcat7.ParameterCollector", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorNoop", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorImpl", @@ -125,7 +125,7 @@ static void after( Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { - TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); + BlockFailureReporter.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (for Request/parseParts)"); reqCtx.getTraceSegment().effectivelyBlocked(); } @@ -145,7 +145,7 @@ static void after( (Flow.Action.RequestBlockingAction) filenamesAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); + BlockFailureReporter.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (multipart file upload)"); reqCtx.getTraceSegment().effectivelyBlocked(); } @@ -166,7 +166,7 @@ static void after( (Flow.Action.RequestBlockingAction) contentAction; BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { - TomcatBlockingHelper.tryCommitAndReport(reqCtx, rba); + BlockFailureReporter.tryCommitAndReport(reqCtx, rba); t = new BlockingException("Blocked request (multipart file upload content)"); reqCtx.getTraceSegment().effectivelyBlocked(); } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java new file mode 100644 index 00000000000..185c3193621 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java @@ -0,0 +1,65 @@ +package datadog.trace.instrumentation.tomcat; + +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; + +/** + * Catalina-independent counterpart of {@link TomcatBlockingHelper}: commits a blocking response + * through the {@link BlockResponseFunction} registered on a request context and reports {@code + * block_failure} telemetry when the commit fails. + * + *

Kept separate from {@link TomcatBlockingHelper} (which also carries the Servlet/Catalina + * fallback commit path) so that instrumentations whose target library version range does not + * guarantee {@code org.apache.catalina.connector.Request}/{@code Response} presence can list this + * class in {@code helperClassNames()} without pulling in {@link TomcatBlockingHelper}'s + * catalina-typed methods and static initializer, which muzzle would otherwise validate against + * every version in that range. + */ +public class BlockFailureReporter { + private static final Logger log = LoggerFactory.getLogger(BlockFailureReporter.class); + + /** + * Commits a blocking response through the {@link BlockResponseFunction} registered on the given + * request context and, if the commit fails, reports the failure via {@link + * AppSecContext#reportBlockFailure()}. + * + *

This is the single choke point shared by all Tomcat blocking call sites so the {@code + * block_failure} telemetry is not duplicated inline. + * + * @return {@code true} if the blocking response was committed, {@code false} otherwise (including + * when no {@link BlockResponseFunction} is registered, in which case nothing was attempted + * and no failure is reported, or when the commit attempt threw). + */ + public static boolean tryCommitAndReport( + RequestContext reqCtx, Flow.Action.RequestBlockingAction rba) { + BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); + if (brf == null) { + // nothing was attempted, so this is not a block failure + return false; + } + try { + if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + return true; + } + } catch (Exception e) { + log.debug("Error committing blocking response", e); + reportBlockFailure(reqCtx); + return false; + } + reportBlockFailure(reqCtx); + return false; + } + + /** Reports a block failure on the AppSec context bound to the given request context, if any. */ + public static void reportBlockFailure(RequestContext reqCtx) { + Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + } +} diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java index 38135c4b2df..86fca266e5c 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/TomcatBlockingHelper.java @@ -2,11 +2,8 @@ import datadog.appsec.api.blocking.BlockingContentType; import datadog.context.Context; -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.blocking.BlockingActionHelper; import datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType; @@ -40,46 +37,6 @@ public class TomcatBlockingHelper { GET_OUTPUT_STREAM = mh; } - /** - * Commits a blocking response through the {@link BlockResponseFunction} registered on the given - * request context and, if the commit fails, reports the failure via {@link - * AppSecContext#reportBlockFailure()}. - * - *

This is the single choke point shared by all Tomcat blocking call sites so the {@code - * block_failure} telemetry is not duplicated inline. - * - * @return {@code true} if the blocking response was committed, {@code false} otherwise (including - * when no {@link BlockResponseFunction} is registered, in which case nothing was attempted - * and no failure is reported, or when the commit attempt threw). - */ - public static boolean tryCommitAndReport( - RequestContext reqCtx, Flow.Action.RequestBlockingAction rba) { - BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); - if (brf == null) { - // nothing was attempted, so this is not a block failure - return false; - } - try { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { - return true; - } - } catch (Exception e) { - log.debug("Error committing blocking response", e); - reportBlockFailure(reqCtx); - return false; - } - reportBlockFailure(reqCtx); - return false; - } - - /** Reports a block failure on the AppSec context bound to the given request context, if any. */ - public static void reportBlockFailure(RequestContext reqCtx) { - Object rawAppSecCtx = reqCtx.getData(RequestContextSlot.APPSEC); - if (rawAppSecCtx instanceof AppSecContext) { - ((AppSecContext) rawAppSecCtx).reportBlockFailure(); - } - } - /** * Reports a block failure on the AppSec context bound to the given Tomcat request, if the request * still carries a datadog context with an active span. @@ -95,7 +52,7 @@ private static void reportBlockFailure(Request request) { } RequestContext reqCtx = span.getRequestContext(); if (reqCtx != null) { - reportBlockFailure(reqCtx); + BlockFailureReporter.reportBlockFailure(reqCtx); } } From e7944059bfec4cf2f68dcc526a07b971c86db588 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 15:11:25 +0200 Subject: [PATCH 3/6] Fix spring-webmvc test fixture clobbering Tomcat's real BlockResponseFunction TestSpringBlockResponseFunction now delegates to the previously registered BlockResponseFunction (Tomcat's own, in production tests) instead of returning true/false unconditionally when RequestContextHolder is not yet populated by Spring, fixing 10 latestDepTest failures exposed by the tryCommitAndReport refactor. --- .../groovy/test/SetupSpecHelper.groovy | 19 +++++++++++++++---- .../test/groovy/test/SetupSpecHelper.groovy | 19 +++++++++++++++---- .../springweb6/SetupSpecHelper.groovy | 19 +++++++++++++++---- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/test/SetupSpecHelper.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/test/SetupSpecHelper.groovy index 04912808d9b..639da44f3dc 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/test/SetupSpecHelper.groovy +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/latestDepTest/groovy/test/SetupSpecHelper.groovy @@ -31,14 +31,24 @@ class SetupSpecHelper { ss.registerCallback(uriEvent, new TriFunction>() { @Override Flow apply(RequestContext requestContext, String s, URIDataAdapter uriDataAdapter) { - requestContext.setBlockResponseFunction(TestSpringBlockResponseFunction.INSTANCE) + BlockResponseFunction original = requestContext.getBlockResponseFunction() + requestContext.setBlockResponseFunction(new TestSpringBlockResponseFunction(original)) origUriCallback.apply(requestContext, s, uriDataAdapter) } }) } - enum TestSpringBlockResponseFunction implements BlockResponseFunction { - INSTANCE + /** + * Commits the blocking response through Spring's request attributes when they are available, + * falling back to the block response function the server instrumentation had already registered + * (e.g. Tomcat's) for block points that happen before Spring populates RequestContextHolder. + */ + static class TestSpringBlockResponseFunction implements BlockResponseFunction { + private final BlockResponseFunction original + + TestSpringBlockResponseFunction(BlockResponseFunction original) { + this.original = original + } @Override boolean tryCommitBlockingResponse(TraceSegment segment, int statusCode, BlockingContentType templateType, Map extraHeaders, String securityResponseId) { @@ -46,8 +56,9 @@ class SetupSpecHelper { if (attributes) { ServletBlockingHelper .commitBlockingResponse(segment, attributes.request, attributes.response, statusCode, templateType, extraHeaders, securityResponseId) + return true } - true + original != null && original.tryCommitBlockingResponse(segment, statusCode, templateType, extraHeaders, securityResponseId) } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/SetupSpecHelper.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/SetupSpecHelper.groovy index 04912808d9b..639da44f3dc 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/SetupSpecHelper.groovy +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/test/groovy/test/SetupSpecHelper.groovy @@ -31,14 +31,24 @@ class SetupSpecHelper { ss.registerCallback(uriEvent, new TriFunction>() { @Override Flow apply(RequestContext requestContext, String s, URIDataAdapter uriDataAdapter) { - requestContext.setBlockResponseFunction(TestSpringBlockResponseFunction.INSTANCE) + BlockResponseFunction original = requestContext.getBlockResponseFunction() + requestContext.setBlockResponseFunction(new TestSpringBlockResponseFunction(original)) origUriCallback.apply(requestContext, s, uriDataAdapter) } }) } - enum TestSpringBlockResponseFunction implements BlockResponseFunction { - INSTANCE + /** + * Commits the blocking response through Spring's request attributes when they are available, + * falling back to the block response function the server instrumentation had already registered + * (e.g. Tomcat's) for block points that happen before Spring populates RequestContextHolder. + */ + static class TestSpringBlockResponseFunction implements BlockResponseFunction { + private final BlockResponseFunction original + + TestSpringBlockResponseFunction(BlockResponseFunction original) { + this.original = original + } @Override boolean tryCommitBlockingResponse(TraceSegment segment, int statusCode, BlockingContentType templateType, Map extraHeaders, String securityResponseId) { @@ -46,8 +56,9 @@ class SetupSpecHelper { if (attributes) { ServletBlockingHelper .commitBlockingResponse(segment, attributes.request, attributes.response, statusCode, templateType, extraHeaders, securityResponseId) + return true } - true + original != null && original.tryCommitBlockingResponse(segment, statusCode, templateType, extraHeaders, securityResponseId) } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SetupSpecHelper.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SetupSpecHelper.groovy index e409483dc6b..34f006b185e 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SetupSpecHelper.groovy +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/SetupSpecHelper.groovy @@ -31,14 +31,24 @@ class SetupSpecHelper { ss.registerCallback(uriEvent, new TriFunction>() { @Override Flow apply(RequestContext requestContext, String s, URIDataAdapter uriDataAdapter) { - requestContext.setBlockResponseFunction(TestSpringBlockResponseFunction.INSTANCE) + BlockResponseFunction original = requestContext.getBlockResponseFunction() + requestContext.setBlockResponseFunction(new TestSpringBlockResponseFunction(original)) origUriCallback.apply(requestContext, s, uriDataAdapter) } }) } - enum TestSpringBlockResponseFunction implements BlockResponseFunction { - INSTANCE + /** + * Commits the blocking response through Spring's request attributes when they are available, + * falling back to the block response function the server instrumentation had already registered + * (e.g. Tomcat's) for block points that happen before Spring populates RequestContextHolder. + */ + static class TestSpringBlockResponseFunction implements BlockResponseFunction { + private final BlockResponseFunction original + + TestSpringBlockResponseFunction(BlockResponseFunction original) { + this.original = original + } @Override boolean tryCommitBlockingResponse(TraceSegment segment, int statusCode, BlockingContentType templateType, Map extraHeaders, String securityResponseId) { @@ -46,8 +56,9 @@ class SetupSpecHelper { if (attributes) { JakartaServletBlockingHelper .commitBlockingResponse(segment, attributes.request, attributes.response, statusCode, templateType, extraHeaders, securityResponseId) + return true } - true + original != null && original.tryCommitBlockingResponse(segment, statusCode, templateType, extraHeaders, securityResponseId) } } } From f34f16feea0480ec9eb4fce524e50d2e9f426976 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 16:18:56 +0200 Subject: [PATCH 4/6] Fix: do not report block_failure when GlassFishBlockingHelper has no fallback response Guard the Servlet API fallback branch of tryBlock() so reportBlockFailure() only fires when a response was actually available to commit through (i.e. a commit was genuinely attempted). Previously it reported unconditionally whenever the fallback commit failed, including the brf == null / fallbackResp == null case where nothing was attempted at all. --- .../tomcat7/GlassFishBlockingHelper.java | 5 ++++- .../tomcat7/GlassFishBlockingHelperTest.java | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java index d5af7fc0bd4..f8acb4857ab 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelper.java @@ -48,7 +48,10 @@ public static boolean tryBlock( return false; } } else if (!commitBlocking(fallbackReq, fallbackResp, rba)) { - BlockFailureReporter.reportBlockFailure(reqCtx); + if (fallbackResp != null) { + // a commit was genuinely attempted (there was a response to write to) and failed + BlockFailureReporter.reportBlockFailure(reqCtx); + } return false; } } catch (Exception e) { diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java index c23aac4310e..a231ec9e111 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/java/datadog/trace/instrumentation/tomcat7/GlassFishBlockingHelperTest.java @@ -219,6 +219,19 @@ void tryBlock_noBrf_fallbackCommitFails_reportsBlockFailure() { verify(segment, never()).effectivelyBlocked(); } + @Test + void tryBlock_noBrf_nullFallbackResponse_doesNotReportBlockFailure() { + TraceSegment segment = mock(TraceSegment.class); + RequestContext reqCtx = mockReqCtx(null, segment); + AppSecContext appSecCtx = mockAppSecCtx(reqCtx); + + // brf == null and fallbackResp == null: no response was ever available to commit through, + // so nothing was genuinely attempted and no block_failure should be reported. + assertFalse(GlassFishBlockingHelper.tryBlock(reqCtx, null, null, rba(403))); + + verify(appSecCtx, never()).reportBlockFailure(); + } + @Test void tryBlock_commitThrows_reportsBlockFailure() throws Exception { TraceSegment segment = mock(TraceSegment.class); From f468901d7ce966a57cd94f2e80b7d88f7219c16b Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 21:24:17 +0200 Subject: [PATCH 5/6] Fix: propagate commit exceptions instead of swallowing them in BlockFailureReporter Remove the catch(Exception) around brf.tryCommitBlockingResponse() in tryCommitAndReport() so it matches the Netty reference implementation (PR #12316), which only branches on the boolean return value. Several call sites (CommitActionInstrumentation in 5.5/7.0, ParsePartsInstrumentation, ParsedBodyParametersInstrumentation) rely on their enclosing advice's suppress = Throwable.class to abort the whole method when the commit throws, gating success-path side effects (closing the connection, injecting a BlockingException, marking the segment effectively blocked). Swallowing the exception inside tryCommitAndReport made those side effects run unconditionally even when nothing was actually committed to the client. Removing the catch restores the pre-existing control flow; exception-based commit failures are now a known gap not reported to block_failure telemetry, same as Netty. Addresses Codex review comment on PR #12493. --- .../tomcat/BlockFailureReporter.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java index 185c3193621..8c9538feaf8 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/src/main/java/datadog/trace/instrumentation/tomcat/BlockFailureReporter.java @@ -5,8 +5,6 @@ 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; /** * Catalina-independent counterpart of {@link TomcatBlockingHelper}: commits a blocking response @@ -21,7 +19,6 @@ * every version in that range. */ public class BlockFailureReporter { - private static final Logger log = LoggerFactory.getLogger(BlockFailureReporter.class); /** * Commits a blocking response through the {@link BlockResponseFunction} registered on the given @@ -31,9 +28,18 @@ public class BlockFailureReporter { *

This is the single choke point shared by all Tomcat blocking call sites so the {@code * block_failure} telemetry is not duplicated inline. * + *

Exceptions thrown by the commit attempt are deliberately propagated instead of being + * converted into a {@code false} return: the calling advice methods declare {@code suppress = + * Throwable.class} and rely on the whole advice being aborted so that their blocking success-path + * side effects (closing the connection, injecting a {@link + * datadog.appsec.api.blocking.BlockingException}, marking the trace segment effectively blocked) + * are skipped when no response was committed. Such exception-based commit failures are therefore + * not reported to the {@code block_failure} telemetry, which is the same known gap as the Netty + * implementation this mirrors. + * * @return {@code true} if the blocking response was committed, {@code false} otherwise (including * when no {@link BlockResponseFunction} is registered, in which case nothing was attempted - * and no failure is reported, or when the commit attempt threw). + * and no failure is reported). */ public static boolean tryCommitAndReport( RequestContext reqCtx, Flow.Action.RequestBlockingAction rba) { @@ -42,14 +48,8 @@ public static boolean tryCommitAndReport( // nothing was attempted, so this is not a block failure return false; } - try { - if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { - return true; - } - } catch (Exception e) { - log.debug("Error committing blocking response", e); - reportBlockFailure(reqCtx); - return false; + if (brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba)) { + return true; } reportBlockFailure(reqCtx); return false; From ad61aa54a1fb5448b57ad336196d9838f7ffdb0b Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 15 Sep 2026 22:42:08 +0200 Subject: [PATCH 6/6] Update tomcat-common gradle.lockfile for new test dependencies Regenerate the lock file to reflect the testImplementation dependencies added in TODO-9 (catalina/coyote/util, servlet-api, JUnit 5, Mockito) for the sun.misc.Unsafe-based test. No change needed for tomcat-appsec-6.0's lockfile: its new project(':...:tomcat-common') dependency does not add any external dependency to its classpath. Addresses review comments from AlexeyKuznetsov-DD on PR #12493. --- .../tomcat/tomcat-common/gradle.lockfile | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-common/gradle.lockfile b/dd-java-agent/instrumentation/tomcat/tomcat-common/gradle.lockfile index c5a0975ed81..65ca4b24631 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-common/gradle.lockfile +++ b/dd-java-agent/instrumentation/tomcat/tomcat-common/gradle.lockfile @@ -53,7 +53,7 @@ de.thetaphi:forbiddenapis:3.10=compileClasspath,testCompileClasspath,testRuntime io.leangen.geantyref:geantyref:1.3.16=testRuntimeClasspath io.sqreen:libsqreen:17.5.0=testRuntimeClasspath javax.servlet:javax.servlet-api:3.1.0=testCompileClasspath,testRuntimeClasspath -javax.servlet:servlet-api:2.4=compileClasspath +javax.servlet:servlet-api:2.4=compileClasspath,testCompileClasspath,testRuntimeClasspath jaxen:jaxen:2.0.6=spotbugs junit:junit:4.13.2=testRuntimeClasspath net.bytebuddy:byte-buddy-agent:1.18.10=buildTimeInstrumentationPlugin,compileClasspath,muzzleTooling,runtimeClasspath,testCompileClasspath,testRuntimeClasspath @@ -99,7 +99,8 @@ org.junit.platform:junit-platform-suite-api:1.14.1=testRuntimeClasspath org.junit.platform:junit-platform-suite-commons:1.14.1=testRuntimeClasspath org.junit:junit-bom:5.14.1=testCompileClasspath,testRuntimeClasspath org.junit:junit-bom:6.1.2=spotbugs -org.mockito:mockito-core:4.4.0=testRuntimeClasspath +org.mockito:mockito-core:4.4.0=testCompileClasspath,testRuntimeClasspath +org.mockito:mockito-junit-jupiter:4.4.0=testCompileClasspath,testRuntimeClasspath org.objenesis:objenesis:3.3=testCompileClasspath,testRuntimeClasspath org.opentest4j:opentest4j:1.3.0=testCompileClasspath,testRuntimeClasspath org.ow2.asm:asm-analysis:9.10.1=spotbugs @@ -123,7 +124,7 @@ org.spockframework:spock-core:2.4-groovy-3.0=testCompileClasspath,testRuntimeCla org.tabletest:tabletest-junit:1.2.2=testCompileClasspath,testRuntimeClasspath org.tabletest:tabletest-parser:1.2.1=testCompileClasspath,testRuntimeClasspath org.xmlresolver:xmlresolver:5.3.3=spotbugs -tomcat:catalina:5.5.12=compileClasspath -tomcat:tomcat-coyote:5.5.12=compileClasspath -tomcat:tomcat-util:5.5.12=compileClasspath +tomcat:catalina:5.5.12=compileClasspath,testCompileClasspath,testRuntimeClasspath +tomcat:tomcat-coyote:5.5.12=compileClasspath,testCompileClasspath,testRuntimeClasspath +tomcat:tomcat-util:5.5.12=compileClasspath,testCompileClasspath,testRuntimeClasspath empty=spotbugsPlugins