Ensure servlet.path starts with /

DataDog · Dec 17, 2024 · 883f492 · 883f492
1 parent 4dc68bb
commit 883f492
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 61 deletions.
diff --git a/...a-agent/instrumentation/cxf-2.1/src/latestDepTest/groovy/CxfContextPropagationTest.groovy b/...a-agent/instrumentation/cxf-2.1/src/latestDepTest/groovy/CxfContextPropagationTest.groovy
@@ -2,6 +2,7 @@ import datadog.trace.agent.test.AgentTestRunner
 import datadog.trace.agent.test.utils.OkHttpUtils
 import datadog.trace.agent.test.utils.TraceUtils
 import datadog.trace.api.DDSpanTypes
+import datadog.trace.bootstrap.instrumentation.api.InstrumentationTags
 import datadog.trace.bootstrap.instrumentation.api.Tags
 import okhttp3.Request
 import org.apache.cxf.endpoint.Server
@@ -70,7 +71,8 @@ class CxfContextPropagationTest extends AgentTestRunner {
             "$Tags.HTTP_METHOD" "GET"
             "$Tags.HTTP_STATUS" 200
             "$Tags.HTTP_ROUTE" String
-            "servlet.path" "/test"
+            "$InstrumentationTags.SERVLET_CONTEXT" "/"
+            "$InstrumentationTags.SERVLET_PATH" "/test"
             "$Tags.HTTP_USER_AGENT" String
             "$Tags.HTTP_CLIENT_IP" "127.0.0.1"
             defaultTags()

diff --git a/.../jetty-12/src/main/java17/datadog/trace/instrumentation/jetty12/SetContextPathAdvice.java b/.../jetty-12/src/main/java17/datadog/trace/instrumentation/jetty12/SetContextPathAdvice.java
@@ -36,10 +36,16 @@ public static void updateContextPath(
         ((AgentSpan) span).setTag(SERVLET_CONTEXT, servletContext);
         req.setAttribute(DD_CONTEXT_PATH_ATTRIBUTE, servletContext);
         if (pathInContext != null) {
-          final String relativePath =
+          // the following can be cached however than can be issues for application having
+          // dynamically generated URL
+          // since a bounded cache might collide
+          String relativePath =
               pathInContext.startsWith(servletContext)
                   ? pathInContext.substring(servletContext.length())
                   : pathInContext;
+          if (!relativePath.isEmpty() && relativePath.charAt(0) != '/') {
+            relativePath = "/" + relativePath;
+          }
           ((AgentSpan) span).setTag(SERVLET_PATH, relativePath);
           req.setAttribute(DD_SERVLET_PATH_ATTRIBUTE, relativePath);
         }

diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy
@@ -1,5 +1,44 @@
 package datadog.trace.agent.test.base
 
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_JSON
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CREATED
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CREATED_IS
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CUSTOM_EXCEPTION
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.FORWARDED
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_ENCODED_BOTH
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_ENCODED_QUERY
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SESSION_ID
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.TIMEOUT
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.TIMEOUT_ERROR
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.UNKNOWN
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.USER_BLOCK
+import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
+import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
+import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_RAW_QUERY_STRING
+import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_RAW_RESOURCE
+import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_TAG_QUERY_STRING
+import static datadog.trace.api.config.TraceInstrumentationConfig.SERVLET_ASYNC_TIMEOUT_ERROR
+import static datadog.trace.api.config.TracerConfig.HEADER_TAGS
+import static datadog.trace.api.config.TracerConfig.REQUEST_HEADER_TAGS
+import static datadog.trace.api.config.TracerConfig.RESPONSE_HEADER_TAGS
+import static datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType.JSON
+import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope
+import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan
+import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.get
+import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan
+import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.SERVER_PATHWAY_EDGE_TAGS
+import static java.nio.charset.StandardCharsets.UTF_8
+import static org.junit.Assume.assumeTrue
+
 import ch.qos.logback.classic.Level
 import datadog.appsec.api.blocking.Blocking
 import datadog.appsec.api.blocking.BlockingContentType
@@ -28,6 +67,7 @@ import datadog.trace.api.iast.IastContext
 import datadog.trace.api.normalize.SimpleHttpPathNormalizer
 import datadog.trace.bootstrap.blocking.BlockingActionHelper
 import datadog.trace.bootstrap.instrumentation.api.AgentTracer
+import datadog.trace.bootstrap.instrumentation.api.InstrumentationTags
 import datadog.trace.bootstrap.instrumentation.api.Tags
 import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter
 import datadog.trace.bootstrap.instrumentation.api.URIUtils
@@ -50,45 +90,6 @@ import java.util.function.BiFunction
 import java.util.function.Function
 import java.util.function.Supplier
 
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_JSON
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CREATED
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CREATED_IS
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CUSTOM_EXCEPTION
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.FORWARDED
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_ENCODED_BOTH
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_ENCODED_QUERY
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SESSION_ID
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.TIMEOUT
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.TIMEOUT_ERROR
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.UNKNOWN
-import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.USER_BLOCK
-import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
-import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
-import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_RAW_QUERY_STRING
-import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_RAW_RESOURCE
-import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_SERVER_TAG_QUERY_STRING
-import static datadog.trace.api.config.TraceInstrumentationConfig.SERVLET_ASYNC_TIMEOUT_ERROR
-import static datadog.trace.api.config.TracerConfig.HEADER_TAGS
-import static datadog.trace.api.config.TracerConfig.REQUEST_HEADER_TAGS
-import static datadog.trace.api.config.TracerConfig.RESPONSE_HEADER_TAGS
-import static datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType.JSON
-import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope
-import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan
-import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.get
-import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan
-import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.SERVER_PATHWAY_EDGE_TAGS
-import static java.nio.charset.StandardCharsets.UTF_8
-import static org.junit.Assume.assumeTrue
-
 abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
 
   public static final Logger SERVER_LOGGER = LoggerFactory.getLogger("http-server")
@@ -208,9 +209,9 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
   // Only used if hasExtraErrorInformation is true
   Map<String, Serializable> expectedExtraErrorInformation(ServerEndpoint endpoint) {
     if (endpoint.errored) {
-      ["error.message"  : { it == null || it == EXCEPTION.body },
-        "error.type" : { it == null || it == Exception.name },
-        "error.stack": { it == null || it instanceof String }]
+      ["error.message": { it == null || it == EXCEPTION.body },
+        "error.type"   : { it == null || it == Exception.name },
+        "error.stack"  : { it == null || it instanceof String }]
     } else {
       Collections.emptyMap()
     }
@@ -492,8 +493,8 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     }
 
     private static final Map<String, ServerEndpoint> PATH_MAP = {
-      Map<String, ServerEndpoint> map = values().collectEntries { [it.path, it]}
-      map.putAll(values().collectEntries { [it.rawPath, it]})
+      Map<String, ServerEndpoint> map = values().collectEntries { [it.path, it] }
+      map.putAll(values().collectEntries { [it.rawPath, it] })
       map
     }.call()
 
@@ -701,9 +702,9 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     }
 
     where:
-    method | body | header                           | value | tags
-    'GET'  | null | 'x-datadog-test-both-header'     | 'foo' | [ 'both_header_tag': 'foo' ]
-    'GET'  | null | 'x-datadog-test-request-header'  | 'bar' | [ 'request_header_tag': 'bar' ]
+    method | body | header                          | value | tags
+    'GET'  | null | 'x-datadog-test-both-header'    | 'foo' | ['both_header_tag': 'foo']
+    'GET'  | null | 'x-datadog-test-request-header' | 'bar' | ['request_header_tag': 'bar']
   }
 
   @Flaky(value = "https://github.com/DataDog/dd-trace-java/issues/4690", suites = ["MuleHttpServerForkedTest"])
@@ -715,7 +716,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     def body = null
     def header = IG_RESPONSE_HEADER
     def mapping = 'mapped_response_header_tag'
-    def tags = ['mapped_response_header_tag': "$IG_RESPONSE_HEADER_VALUE" ]
+    def tags = ['mapped_response_header_tag': "$IG_RESPONSE_HEADER_VALUE"]
 
     injectSysConfig(HTTP_SERVER_TAG_QUERY_STRING, "true")
     injectSysConfig(RESPONSE_HEADER_TAGS, "$header:$mapping")
@@ -798,13 +799,13 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     }
 
     where:
-    rawQuery | endpoint               | encoded
-    true     | SUCCESS                | false
-    true     | QUERY_PARAM            | false
-    true     | QUERY_ENCODED_QUERY    | true
-    false    | SUCCESS                | false
-    false    | QUERY_PARAM            | false
-    false    | QUERY_ENCODED_QUERY    | true
+    rawQuery | endpoint            | encoded
+    true     | SUCCESS             | false
+    true     | QUERY_PARAM         | false
+    true     | QUERY_ENCODED_QUERY | true
+    false    | SUCCESS             | false
+    false    | QUERY_PARAM         | false
+    false    | QUERY_ENCODED_QUERY | true
 
     method = "GET"
     body = null
@@ -917,7 +918,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     }
 
     then:
-    DDSpan span = TEST_WRITER.flatten().find {it.operationName =='appsec-span' }
+    DDSpan span = TEST_WRITER.flatten().find { it.operationName == 'appsec-span' }
     span.getTag(IG_PATH_PARAMS_TAG) == expectedIGPathParams()
 
     and:
@@ -1611,7 +1612,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     then:
     TEST_WRITER.waitForTraces(1)
     def trace = TEST_WRITER.get(0)
-    assert trace.find {it.isError() } == null
+    assert trace.find { it.isError() } == null
   }
 
   def 'test blocking of request for path parameters'() {
@@ -1692,7 +1693,8 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     spans.find { it.tags['appsec.blocked'] == 'true' } != null
     spans.find {
       it.error &&
-      it.tags['error.type'] == BlockingException.name } != null
+      it.tags['error.type'] == BlockingException.name
+    } != null
 
     and:
     if (isDataStreamsEnabled()) {
@@ -1872,7 +1874,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     if (isDataStreamsEnabled()) {
       TEST_DATA_STREAMS_WRITER.waitForGroups(1)
     }
-    DDSpan span = TEST_WRITER.flatten().find {it.operationName =='appsec-span' }
+    DDSpan span = TEST_WRITER.flatten().find { it.operationName == 'appsec-span' }
     span != null
     final sessionId = span.tags[IG_SESSION_ID_TAG]
     sessionId != null
@@ -1966,6 +1968,9 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
         if (null != expectedServerSpanRoute) {
           "$Tags.HTTP_ROUTE" expectedServerSpanRoute
         }
+        if (span.getTag(InstrumentationTags.SERVLET_PATH) != null) {
+          assert span.getTag(InstrumentationTags.SERVLET_PATH).toString().startsWith("/")
+        }
         if (null != expectedExtraErrorInformation) {
           addTags(expectedExtraErrorInformation)
         }