From f1500652941b0635716e0eb18388b0f8c6f3d10e Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Tue, 29 Sep 2026 16:01:49 -0400 Subject: [PATCH 1/2] Skip IAST session tracking check when the servlet context is null GetHttpSessionAdvice used request.getServletContext() as a context store key without a null check. Request copies such as Wicket's ServletRequestCopy and Atmosphere's NoOpsRequest return a session but no servlet context, so the weak map behind the store threw a NullPointerException on every getSession() call. Return early when the context is null, in both the javax and jakarta advice. Also document that ContextStore and WeakMap keys must not be null and mark the key parameters @Nonnull. Co-Authored-By: Claude Sonnet 5.5 --- .../java/datadog/trace/bootstrap/WeakMap.java | 21 ++++--- ...artaHttpServletRequestInstrumentation.java | 5 ++ .../servlet5/GetHttpSessionAdviceTest.java | 63 +++++++++++++++++++ ...OutHttpServletRequest3Instrumentation.java | 5 ++ .../servlet3/GetHttpSessionAdviceTest.java | 63 +++++++++++++++++++ .../datadog/trace/bootstrap/ContextStore.java | 16 +++-- 6 files changed, 160 insertions(+), 13 deletions(-) create mode 100644 dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java create mode 100644 dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index 2fedd001284..85a2ac5b879 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -2,21 +2,28 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.function.Function; - +import javax.annotation.Nonnull; + +/** + * Map with weakly referenced keys. + * + *

Keys must not be {@code null}: the backing {@code WeakConcurrentMap} rejects null keys and + * throws. + */ public interface WeakMap { int size(); - boolean containsKey(K target); + boolean containsKey(@Nonnull K target); - V get(K key); + V get(@Nonnull K key); - void put(K key, V value); + void put(@Nonnull K key, V value); - void putIfAbsent(K key, V value); + void putIfAbsent(@Nonnull K key, V value); - V computeIfAbsent(K key, Function supplier); + V computeIfAbsent(@Nonnull K key, Function supplier); - V remove(K key); + V remove(@Nonnull K key); abstract class Supplier { private static volatile Supplier SUPPLIER; diff --git a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/main/java/datadog/trace/instrumentation/servlet5/IastOptOutJakartaHttpServletRequestInstrumentation.java b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/main/java/datadog/trace/instrumentation/servlet5/IastOptOutJakartaHttpServletRequestInstrumentation.java index 2dbda417503..031ef810000 100644 --- a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/main/java/datadog/trace/instrumentation/servlet5/IastOptOutJakartaHttpServletRequestInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/main/java/datadog/trace/instrumentation/servlet5/IastOptOutJakartaHttpServletRequestInstrumentation.java @@ -84,6 +84,11 @@ public static void onExit( return; } final ServletContext context = request.getServletContext(); + if (context == null) { + // some request copies (e.g. Wicket or Atmosphere websocket requests) have no servlet + // context + return; + } if (InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class).get(context) != null) { diff --git a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java new file mode 100644 index 00000000000..e6bd3b005ef --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java @@ -0,0 +1,63 @@ +package datadog.trace.instrumentation.servlet5; + +import static java.util.Collections.emptyList; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.sink.ApplicationModule; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpSession; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class GetHttpSessionAdviceTest { + + private final List moduleCalls = new ArrayList<>(); + private ApplicationModule previousModule; + + @BeforeEach + void registerModule() { + previousModule = InstrumentationBridge.APPLICATION; + InstrumentationBridge.APPLICATION = + proxy( + ApplicationModule.class, + (proxy, method, args) -> { + moduleCalls.add(method.getName()); + return null; + }); + } + + @AfterEach + void restoreModule() { + InstrumentationBridge.APPLICATION = previousModule; + } + + /** + * Request copies such as Wicket's {@code ServletRequestCopy} or Atmosphere's {@code NoOpsRequest} + * return a session but have no servlet context. The advice must not use that null context as a + * context store key (the weak map behind it throws on null keys). + */ + @Test + void ignoresRequestWithoutServletContext() { + HttpServletRequest request = proxy(HttpServletRequest.class, (proxy, method, args) -> null); + HttpSession session = proxy(HttpSession.class, (proxy, method, args) -> null); + + assertDoesNotThrow( + () -> + IastOptOutJakartaHttpServletRequestInstrumentation.GetHttpSessionAdvice.onExit( + request, session)); + + assertEquals(emptyList(), moduleCalls); + } + + @SuppressWarnings("unchecked") + private static T proxy(Class type, InvocationHandler handler) { + return (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] {type}, handler); + } +} diff --git a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/main/java/datadog/trace/instrumentation/servlet3/IastOptOutHttpServletRequest3Instrumentation.java b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/main/java/datadog/trace/instrumentation/servlet3/IastOptOutHttpServletRequest3Instrumentation.java index 57289e26ab7..8f7797643cc 100644 --- a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/main/java/datadog/trace/instrumentation/servlet3/IastOptOutHttpServletRequest3Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/main/java/datadog/trace/instrumentation/servlet3/IastOptOutHttpServletRequest3Instrumentation.java @@ -92,6 +92,11 @@ public static void onExit( return; } final ServletContext context = request.getServletContext(); + if (context == null) { + // some request copies (e.g. Wicket or Atmosphere websocket requests) have no servlet + // context + return; + } if (InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class).get(context) != null) { return; diff --git a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java new file mode 100644 index 00000000000..9e15950c095 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java @@ -0,0 +1,63 @@ +package datadog.trace.instrumentation.servlet3; + +import static java.util.Collections.emptyList; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.sink.ApplicationModule; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class GetHttpSessionAdviceTest { + + private final List moduleCalls = new ArrayList<>(); + private ApplicationModule previousModule; + + @BeforeEach + void registerModule() { + previousModule = InstrumentationBridge.APPLICATION; + InstrumentationBridge.APPLICATION = + proxy( + ApplicationModule.class, + (proxy, method, args) -> { + moduleCalls.add(method.getName()); + return null; + }); + } + + @AfterEach + void restoreModule() { + InstrumentationBridge.APPLICATION = previousModule; + } + + /** + * Request copies such as Wicket's {@code ServletRequestCopy} or Atmosphere's {@code NoOpsRequest} + * return a session but have no servlet context. The advice must not use that null context as a + * context store key (the weak map behind it throws on null keys). + */ + @Test + void ignoresRequestWithoutServletContext() { + HttpServletRequest request = proxy(HttpServletRequest.class, (proxy, method, args) -> null); + HttpSession session = proxy(HttpSession.class, (proxy, method, args) -> null); + + assertDoesNotThrow( + () -> + IastOptOutHttpServletRequest3Instrumentation.GetHttpSessionAdvice.onExit( + request, session)); + + assertEquals(emptyList(), moduleCalls); + } + + @SuppressWarnings("unchecked") + private static T proxy(Class type, InvocationHandler handler) { + return (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] {type}, handler); + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/ContextStore.java b/internal-api/src/main/java/datadog/trace/bootstrap/ContextStore.java index 18dd8344a5c..0fe1e5149b9 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/ContextStore.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/ContextStore.java @@ -1,6 +1,7 @@ package datadog.trace.bootstrap; import java.util.function.Function; +import javax.annotation.Nonnull; import javax.annotation.Nullable; /** @@ -9,6 +10,9 @@ *

Context instances are weakly referenced and will be garbage collected when their corresponding * key instance is collected. * + *

Keys must not be {@code null}: the backing weak maps reject null keys and throw. Callers that + * take a key from application code (for example a servlet request's context) must check it first. + * * @param key type to do context lookups * @param context type */ @@ -38,7 +42,7 @@ default C apply(Object key) { * @return context instance; {@code null} if the key had no context */ @Nullable - C get(K key); + C get(@Nonnull K key); /** * Unconditionally put new context instance for the given key. @@ -46,7 +50,7 @@ default C apply(Object key) { * @param key the context key * @param context context instance to save */ - void put(K key, C context); + void put(@Nonnull K key, C context); /** * Gets the context instance for the given key. If no context exists then associate it with the @@ -56,7 +60,7 @@ default C apply(Object key) { * @param context new context instance * @return existing context instance if present; otherwise new instance */ - C getOrPut(K key, C context); + C getOrPut(@Nonnull K key, C context); /** * Gets the context instance for the given key. If no context exists then create one using the @@ -66,7 +70,7 @@ default C apply(Object key) { * @param contextFactory factory instance to produce new context instances * @return existing context instance if present; otherwise new instance */ - default C getOrCreate(K key, Factory contextFactory) { + default C getOrCreate(@Nonnull K key, Factory contextFactory) { return getOrCompute(key, contextFactory); } @@ -78,7 +82,7 @@ default C getOrCreate(K key, Factory contextFactory) { * @param contextFactory factory instance to produce new context instances * @return existing context instance if present; otherwise new instance */ - C getOrCompute(K key, Function contextFactory); + C getOrCompute(@Nonnull K key, Function contextFactory); /** * Removes the context instance for the given key. @@ -87,5 +91,5 @@ default C getOrCreate(K key, Factory contextFactory) { * @return removed context instance; {@code null} if the key had no context */ @Nullable - C remove(K key); + C remove(@Nonnull K key); } From 054f6b8a5e2d7cc3d09cbfaddd08122665c61258 Mon Sep 17 00:00:00 2001 From: Yury Gribkov Date: Thu, 1 Oct 2026 18:02:49 -0700 Subject: [PATCH 2/2] Replace direct servlet advice tests with instrumentation coverage (#12696) --- ...tpServletRequestInstrumentationTest.groovy | 18 ++++++ .../servlet5/GetHttpSessionAdviceTest.java | 63 ------------------- .../src/test/groovy/JettyServlet3Test.groovy | 27 ++++++++ .../servlet3/GetHttpSessionAdviceTest.java | 63 ------------------- 4 files changed, 45 insertions(+), 126 deletions(-) delete mode 100644 dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java delete mode 100644 dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java diff --git a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy index f8d0d1811bf..c3db1613c4f 100644 --- a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy @@ -460,6 +460,24 @@ class JakartaHttpServletRequestInstrumentationTest extends InstrumentationSpecif suite << testSuite() } + void 'getSession tolerates a null servlet context'() { + setup: + final module = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(module) + final session = Mock(HttpSession) + final delegate = Mock(HttpServletRequest) + final request = new CustomRequest(request: delegate) + + when: + final result = request.getSession() + + then: + result.is(session) + 1 * delegate.getSession() >> session + 1 * delegate.getServletContext() >> null + 0 * module._ + } + protected E runUnderIastTrace(Closure cl) { final ddctx = new TagContext().withRequestContextDataIast(iastCtx) final span = TEST_TRACER.startSpan("test", "test-iast-span", ddctx) diff --git a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java b/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java deleted file mode 100644 index e6bd3b005ef..00000000000 --- a/dd-java-agent/instrumentation/servlet/jakarta-servlet-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java +++ /dev/null @@ -1,63 +0,0 @@ -package datadog.trace.instrumentation.servlet5; - -import static java.util.Collections.emptyList; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.junit.jupiter.api.Assertions.assertEquals; - -import datadog.trace.api.iast.InstrumentationBridge; -import datadog.trace.api.iast.sink.ApplicationModule; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpSession; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Proxy; -import java.util.ArrayList; -import java.util.List; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -class GetHttpSessionAdviceTest { - - private final List moduleCalls = new ArrayList<>(); - private ApplicationModule previousModule; - - @BeforeEach - void registerModule() { - previousModule = InstrumentationBridge.APPLICATION; - InstrumentationBridge.APPLICATION = - proxy( - ApplicationModule.class, - (proxy, method, args) -> { - moduleCalls.add(method.getName()); - return null; - }); - } - - @AfterEach - void restoreModule() { - InstrumentationBridge.APPLICATION = previousModule; - } - - /** - * Request copies such as Wicket's {@code ServletRequestCopy} or Atmosphere's {@code NoOpsRequest} - * return a session but have no servlet context. The advice must not use that null context as a - * context store key (the weak map behind it throws on null keys). - */ - @Test - void ignoresRequestWithoutServletContext() { - HttpServletRequest request = proxy(HttpServletRequest.class, (proxy, method, args) -> null); - HttpSession session = proxy(HttpSession.class, (proxy, method, args) -> null); - - assertDoesNotThrow( - () -> - IastOptOutJakartaHttpServletRequestInstrumentation.GetHttpSessionAdvice.onExit( - request, session)); - - assertEquals(emptyList(), moduleCalls); - } - - @SuppressWarnings("unchecked") - private static T proxy(Class type, InvocationHandler handler) { - return (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] {type}, handler); - } -} diff --git a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/JettyServlet3Test.groovy index 3271479c39b..d3373a40a3f 100644 --- a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/JettyServlet3Test.groovy @@ -28,6 +28,7 @@ import javax.servlet.annotation.WebServlet import javax.servlet.http.HttpServlet import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse +import javax.servlet.http.HttpSession import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CUSTOM_EXCEPTION import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR @@ -570,6 +571,32 @@ class JettyServlet3ServeFromAsyncTimeout extends JettyServlet3Test { class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { + void 'getSession tolerates a null servlet context'() { + setup: + final module = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(module) + final session = Mock(HttpSession) + final delegate = Mock(HttpServletRequest) + final request = new CustomRequest(request: delegate) + + when: + final result = request.getSession() + + then: + result.is(session) + 1 * delegate.getSession() >> session + 1 * delegate.getServletContext() >> null + 0 * module._ + + cleanup: + InstrumentationBridge.clearIastModules() + } + + private static class CustomRequest implements HttpServletRequest { + @Delegate + private HttpServletRequest request + } + @Override Class servlet() { return TestServlet3.GetSession diff --git a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java b/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java deleted file mode 100644 index 9e15950c095..00000000000 --- a/dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java +++ /dev/null @@ -1,63 +0,0 @@ -package datadog.trace.instrumentation.servlet3; - -import static java.util.Collections.emptyList; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.junit.jupiter.api.Assertions.assertEquals; - -import datadog.trace.api.iast.InstrumentationBridge; -import datadog.trace.api.iast.sink.ApplicationModule; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Proxy; -import java.util.ArrayList; -import java.util.List; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpSession; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -class GetHttpSessionAdviceTest { - - private final List moduleCalls = new ArrayList<>(); - private ApplicationModule previousModule; - - @BeforeEach - void registerModule() { - previousModule = InstrumentationBridge.APPLICATION; - InstrumentationBridge.APPLICATION = - proxy( - ApplicationModule.class, - (proxy, method, args) -> { - moduleCalls.add(method.getName()); - return null; - }); - } - - @AfterEach - void restoreModule() { - InstrumentationBridge.APPLICATION = previousModule; - } - - /** - * Request copies such as Wicket's {@code ServletRequestCopy} or Atmosphere's {@code NoOpsRequest} - * return a session but have no servlet context. The advice must not use that null context as a - * context store key (the weak map behind it throws on null keys). - */ - @Test - void ignoresRequestWithoutServletContext() { - HttpServletRequest request = proxy(HttpServletRequest.class, (proxy, method, args) -> null); - HttpSession session = proxy(HttpSession.class, (proxy, method, args) -> null); - - assertDoesNotThrow( - () -> - IastOptOutHttpServletRequest3Instrumentation.GetHttpSessionAdvice.onExit( - request, session)); - - assertEquals(emptyList(), moduleCalls); - } - - @SuppressWarnings("unchecked") - private static T proxy(Class type, InvocationHandler handler) { - return (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] {type}, handler); - } -}