[core] Sanitize URLs in tracing policy (Azure#29606)

### Packages impacted by this PR

@azure/core-rest-pipeline
@typespec/ts-http-runtime

### Issues associated with this PR

Resolves Azure#29433

### Describe the problem that is addressed by this PR

Tracing spans capture the url as an attribute. A customer noticed that
URLs are not sanitized correctly in Java - specifically the QS params.

This was fixed in Java and will now be fixed in JS. It could be
considered a breaking change; however, the OTEL instrumentation 
package is still in beta and as this should have been sanitized earlier 
we can consider this a bugfix.

### What are the possible designs available to address the problem? If
there are more than one possible design, why was the one in this PR
chosen?

I could duplicate the Sanitizer#sanitizeUrl method; however, I wanted to
make sure I can take advantge of the default query params if the list is ever
updated.

Author: maorleger

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ### Bugs Fixed
 
+- Tracing spans will now correctly sanitize query parameters in the http.url span attribute. [#29606](https://github.com/Azure/azure-sdk-for-js/pull/29606)
+
 ### Other Changes
 
 ## 1.16.0 (2024-05-02)

sdk/core/core-rest-pipeline/review/core-rest-pipeline.api.md

@@ -478,6 +478,7 @@ export const tracingPolicyName = "tracingPolicy";
 
 // @public
 export interface TracingPolicyOptions {
+    additionalAllowedQueryParameters?: string[];
     userAgentPrefix?: string;
 }
 

sdk/core/core-rest-pipeline/src/createPipelineFromOptions.ts

@@ -94,7 +94,9 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip
   // properties (e.g., making the boundary constant in recorded tests).
   pipeline.addPolicy(multipartPolicy(), { afterPhase: "Deserialize" });
   pipeline.addPolicy(defaultRetryPolicy(options.retryOptions), { phase: "Retry" });
-  pipeline.addPolicy(tracingPolicy(options.userAgentOptions), { afterPhase: "Retry" });
+  pipeline.addPolicy(tracingPolicy({ ...options.userAgentOptions, ...options.loggingOptions }), {
+    afterPhase: "Retry",
+  });
   if (isNodeLike) {
     // Both XHR and Fetch expect to handle redirects automatically,
     // so only include this policy when we're in Node.

sdk/core/core-rest-pipeline/src/policies/tracingPolicy.ts

@@ -14,6 +14,7 @@ import { getUserAgentValue } from "../util/userAgent.js";
 import { logger } from "../log.js";
 import { getErrorMessage, isError } from "@azure/core-util";
 import { isRestError } from "../restError.js";
+import { Sanitizer } from "../util/sanitizer.js";
 
 /**
  * The programmatic identifier of the tracingPolicy.
@@ -30,6 +31,11 @@ export interface TracingPolicyOptions {
    * Defaults to an empty string.
    */
   userAgentPrefix?: string;
+  /**
+   * Query string names whose values will be logged when logging is enabled. By default no
+   * query string values are logged.
+   */
+  additionalAllowedQueryParameters?: string[];
 }
 
 /**
@@ -40,6 +46,9 @@ export interface TracingPolicyOptions {
  */
 export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolicy {
   const userAgent = getUserAgentValue(options.userAgentPrefix);
+  const sanitizer = new Sanitizer({
+    additionalAllowedQueryParameters: options.additionalAllowedQueryParameters,
+  });
   const tracingClient = tryCreateTracingClient();
 
   return {
@@ -49,7 +58,17 @@ export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolic
         return next(request);
       }
 
-      const { span, tracingContext } = tryCreateSpan(tracingClient, request, userAgent) ?? {};
+      const spanAttributes = {
+        "http.url": sanitizer.sanitizeUrl(request.url),
+        "http.method": request.method,
+        "http.user_agent": userAgent,
+        requestId: request.requestId,
+      };
+      if (userAgent) {
+        spanAttributes["http.user_agent"] = userAgent;
+      }
+
+      const { span, tracingContext } = tryCreateSpan(tracingClient, request, spanAttributes) ?? {};
 
       if (!span || !tracingContext) {
         return next(request);
@@ -83,7 +102,7 @@ function tryCreateTracingClient(): TracingClient | undefined {
 function tryCreateSpan(
   tracingClient: TracingClient,
   request: PipelineRequest,
-  userAgent?: string,
+  spanAttributes: Record<string, unknown>,
 ): { span: TracingSpan; tracingContext: TracingContext } | undefined {
   try {
     // As per spec, we do not need to differentiate between HTTP and HTTPS in span name.
@@ -92,11 +111,7 @@ function tryCreateSpan(
       { tracingOptions: request.tracingOptions },
       {
         spanKind: "client",
-        spanAttributes: {
-          "http.method": request.method,
-          "http.url": request.url,
-          requestId: request.requestId,
-        },
+        spanAttributes,
       },
     );
 
@@ -106,10 +121,6 @@ function tryCreateSpan(
       return undefined;
     }
 
-    if (userAgent) {
-      span.setAttribute("http.user_agent", userAgent);
-    }
-
     // set headers
     const headers = tracingClient.createRequestHeaders(
       updatedOptions.tracingOptions.tracingContext,

sdk/core/core-rest-pipeline/src/util/sanitizer.ts

@@ -132,6 +132,26 @@ export class Sanitizer {
     );
   }
 
+  public sanitizeUrl(value: string): string {
+    if (typeof value !== "string" || value === null) {
+      return value;
+    }
+
+    const url = new URL(value);
+
+    if (!url.search) {
+      return value;
+    }
+
+    for (const [key] of url.searchParams) {
+      if (!this.allowedQueryParameters.has(key.toLowerCase())) {
+        url.searchParams.set(key, RedactedString);
+      }
+    }
+
+    return url.toString();
+  }
+
   private sanitizeHeaders(obj: UnknownObject): UnknownObject {
     const sanitized: UnknownObject = {};
     for (const key of Object.keys(obj)) {
@@ -161,24 +181,4 @@ export class Sanitizer {
 
     return sanitized;
   }
-
-  private sanitizeUrl(value: string): string {
-    if (typeof value !== "string" || value === null) {
-      return value;
-    }
-
-    const url = new URL(value);
-
-    if (!url.search) {
-      return value;
-    }
-
-    for (const [key] of url.searchParams) {
-      if (!this.allowedQueryParameters.has(key.toLowerCase())) {
-        url.searchParams.set(key, RedactedString);
-      }
-    }
-
-    return url.toString();
-  }
 }

sdk/core/core-rest-pipeline/test/tracingPolicy.spec.ts

@@ -154,7 +154,7 @@ describe("tracingPolicy", function () {
 
     const createdSpan = activeInstrumenter.lastSpanCreated;
     assert.exists(createdSpan);
-    const mockSpan = createdSpan!;
+    const mockSpan = createdSpan;
     assert.isTrue(mockSpan.endCalled, "expected span to be ended");
     assert.equal(mockSpan.name, "HTTP POST");
     assert.equal(mockSpan.getAttribute("http.method"), "POST");
@@ -163,6 +163,30 @@ describe("tracingPolicy", function () {
     assert.equal(mockSpan.getAttribute("http.status_code"), 200); // createTestRequest's response will return 200 OK
   });
 
+  it("will sanitize URLs", async () => {
+    const policy = tracingPolicy({ additionalAllowedQueryParameters: ["allowedQueryParam"] });
+    const request = createPipelineRequest({
+      url: "https://bing.com/search?redactedParam=redactedValue&allowedQueryParam=allowedValue",
+      tracingOptions: { tracingContext: noopTracingContext },
+    });
+
+    const response: PipelineResponse = {
+      headers: createHttpHeaders(),
+      request: request,
+      status: 200,
+    };
+    const next = vi.fn<Parameters<SendRequest>, ReturnType<SendRequest>>();
+    next.mockResolvedValue(response);
+
+    await policy.sendRequest(request, next);
+    const createdSpan = activeInstrumenter.lastSpanCreated;
+    assert.exists(createdSpan);
+
+    const spanUrlValue = new URL(createdSpan.getAttribute("http.url") as string);
+    assert.equal(spanUrlValue.searchParams.get("redactedParam"), "REDACTED");
+    assert.equal(spanUrlValue.searchParams.get("allowedQueryParam"), "allowedValue");
+  });
+
   it("will set request headers correctly", async () => {
     vi.spyOn(activeInstrumenter, "createRequestHeaders").mockReturnValue({
       testheader: "testvalue",

sdk/core/ts-http-runtime/review/ts-http-runtime.api.md

@@ -777,6 +777,7 @@ export const tracingPolicyName = "tracingPolicy";
 
 // @public
 export interface TracingPolicyOptions {
+    additionalAllowedQueryParameters?: string[];
     userAgentPrefix?: string;
 }
 

sdk/core/ts-http-runtime/src/createPipelineFromOptions.ts

@@ -93,7 +93,9 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip
   // properties (e.g., making the boundary constant in recorded tests).
   pipeline.addPolicy(multipartPolicy(), { afterPhase: "Deserialize" });
   pipeline.addPolicy(defaultRetryPolicy(options.retryOptions), { phase: "Retry" });
-  pipeline.addPolicy(tracingPolicy(options.userAgentOptions), { afterPhase: "Retry" });
+  pipeline.addPolicy(tracingPolicy({ ...options.userAgentOptions, ...options.loggingOptions }), {
+    afterPhase: "Retry",
+  });
   if (isNodeLike) {
     // Both XHR and Fetch expect to handle redirects automatically,
     // so only include this policy when we're in Node.

sdk/core/ts-http-runtime/src/policies/tracingPolicy.ts

@@ -10,6 +10,7 @@ import { getUserAgentValue } from "../util/userAgent.js";
 import { logger } from "../log.js";
 import { getErrorMessage, isError } from "../util/error.js";
 import { isRestError } from "../restError.js";
+import { Sanitizer } from "../util/sanitizer.js";
 
 /**
  * The programmatic identifier of the tracingPolicy.
@@ -26,6 +27,11 @@ export interface TracingPolicyOptions {
    * Defaults to an empty string.
    */
   userAgentPrefix?: string;
+  /**
+   * Query string names whose values will be logged when logging is enabled. By default no
+   * query string values are logged.
+   */
+  additionalAllowedQueryParameters?: string[];
 }
 
 /**
@@ -36,6 +42,9 @@ export interface TracingPolicyOptions {
  */
 export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolicy {
   const userAgent = getUserAgentValue(options.userAgentPrefix);
+  const sanitizer = new Sanitizer({
+    additionalAllowedQueryParameters: options.additionalAllowedQueryParameters,
+  });
   const tracingClient = tryCreateTracingClient();
 
   return {
@@ -45,7 +54,17 @@ export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolic
         return next(request);
       }
 
-      const { span, tracingContext } = tryCreateSpan(tracingClient, request, userAgent) ?? {};
+      const spanAttributes = {
+        "http.url": sanitizer.sanitizeUrl(request.url),
+        "http.method": request.method,
+        "http.user_agent": userAgent,
+        requestId: request.requestId,
+      };
+      if (userAgent) {
+        spanAttributes["http.user_agent"] = userAgent;
+      }
+
+      const { span, tracingContext } = tryCreateSpan(tracingClient, request, spanAttributes) ?? {};
 
       if (!span || !tracingContext) {
         return next(request);
@@ -79,7 +98,7 @@ function tryCreateTracingClient(): TracingClient | undefined {
 function tryCreateSpan(
   tracingClient: TracingClient,
   request: PipelineRequest,
-  userAgent?: string,
+  spanAttributes: Record<string, unknown>,
 ): { span: TracingSpan; tracingContext: TracingContext } | undefined {
   try {
     // As per spec, we do not need to differentiate between HTTP and HTTPS in span name.
@@ -88,11 +107,7 @@ function tryCreateSpan(
       { tracingOptions: request.tracingOptions },
       {
         spanKind: "client",
-        spanAttributes: {
-          "http.method": request.method,
-          "http.url": request.url,
-          requestId: request.requestId,
-        },
+        spanAttributes,
       },
     );
 
@@ -102,10 +117,6 @@ function tryCreateSpan(
       return undefined;
     }
 
-    if (userAgent) {
-      span.setAttribute("http.user_agent", userAgent);
-    }
-
     // set headers
     const headers = tracingClient.createRequestHeaders(
       updatedOptions.tracingOptions.tracingContext,

sdk/core/ts-http-runtime/src/util/sanitizer.ts

@@ -132,6 +132,26 @@ export class Sanitizer {
     );
   }
 
+  public sanitizeUrl(value: string): string {
+    if (typeof value !== "string" || value === null) {
+      return value;
+    }
+
+    const url = new URL(value);
+
+    if (!url.search) {
+      return value;
+    }
+
+    for (const [key] of url.searchParams) {
+      if (!this.allowedQueryParameters.has(key.toLowerCase())) {
+        url.searchParams.set(key, RedactedString);
+      }
+    }
+
+    return url.toString();
+  }
+
   private sanitizeHeaders(obj: UnknownObject): UnknownObject {
     const sanitized: UnknownObject = {};
     for (const key of Object.keys(obj)) {
@@ -161,24 +181,4 @@ export class Sanitizer {
 
     return sanitized;
   }
-
-  private sanitizeUrl(value: string): string {
-    if (typeof value !== "string" || value === null) {
-      return value;
-    }
-
-    const url = new URL(value);
-
-    if (!url.search) {
-      return value;
-    }
-
-    for (const [key] of url.searchParams) {
-      if (!this.allowedQueryParameters.has(key.toLowerCase())) {
-        url.searchParams.set(key, RedactedString);
-      }
-    }
-
-    return url.toString();
-  }
 }