Harden URL handling in exported telemetry

.changeset/olive-moons-shake.md

@@ -0,0 +1,10 @@
+---
+"executor": patch
+---
+
+Stop exporting credential-bearing URLs in telemetry. Every query parameter
+value, URL fragment, and userinfo component is stripped from exported span
+URLs — no parameter name is trusted — on every exporter path: the cloud span
+processors, the self-host OTLP exporter, the browser client's OTLP exporter,
+and the forwarded browser trace batches. User-supplied MCP endpoints are
+sanitized before being stamped onto spans.

apps/cloud/src/observability/browser-traces.test.ts

@@ -53,6 +53,103 @@ describe("browserTracesResponse", () => {
     expect(await response?.text()).toBe("");
   });
 
+  it("scrubs credential-bearing URLs from the forwarded payload", async () => {
+    // The browser's OTLP batch is pre-serialized JSON; the worker must scrub
+    // the decoded payload before forwarding — the page-side exporter is not a
+    // trust boundary.
+    const SECRET = "synthetic-browser-forward-canary";
+    const payload = {
+      resourceSpans: [
+        {
+          resource: { attributes: [] },
+          scopeSpans: [
+            {
+              scope: { name: "executor-web" },
+              spans: [
+                {
+                  traceId: "0af7651916cd43dd8448eb211c80319c",
+                  spanId: "b7ad6b7169203331",
+                  name: "http.client GET",
+                  kind: 3,
+                  startTimeUnixNano: "0",
+                  endTimeUnixNano: "1",
+                  attributes: [
+                    {
+                      key: "url.full",
+                      value: {
+                        stringValue: `https://app.test/api/oauth/callback?code=${SECRET}-code#access_token=${SECRET}-fragment`,
+                        // Crafted sibling fields: the page-side exporter is not
+                        // a trust boundary, so the URL-keyed special case must
+                        // not exempt the rest of the KeyValue from the scrub.
+                        extraValue: `https://app.test/x?owner=${SECRET}-kv-inner`,
+                      },
+                      note: `retry of https://app.test/x?owner=${SECRET}-kv-sibling`,
+                    },
+                    { key: "url.query", value: { stringValue: `code=${SECRET}-code` } },
+                  ],
+                  events: [
+                    {
+                      name: "exception",
+                      timeUnixNano: "1",
+                      attributes: [
+                        {
+                          key: "exception.message",
+                          value: {
+                            stringValue: `GET https://u:${SECRET}-userinfo@api.test/x?key=${SECRET}-key failed`,
+                          },
+                        },
+                      ],
+                    },
+                  ],
+                  status: {
+                    code: 2,
+                    message: `GET https://api.test/x?key=${SECRET}-status failed`,
+                  },
+                },
+              ],
+            },
+          ],
+        },
+      ],
+    };
+    let forwarded: string | undefined;
+    const response = await browserTracesResponse(
+      makeRequest({
+        headers: { cookie: "wos-session=abc" },
+        body: JSON.stringify(payload),
+      }),
+      baseEnv,
+      (async (_url: RequestInfo | URL, init?: RequestInit) => {
+        forwarded = await new Response(init?.body as BodyInit).text();
+        return new Response(null, { status: 200 });
+      }) as typeof fetch,
+    );
+    expect(response?.status).toBe(204);
+    expect(forwarded).toBeDefined();
+    expect(forwarded).not.toContain(SECRET);
+    // Non-vacuous: the span, its route, and its trace identity survive.
+    expect(forwarded).toContain("/api/oauth/callback");
+    expect(forwarded).toContain("0af7651916cd43dd8448eb211c80319c");
+    expect(forwarded).toContain("exception");
+  });
+
+  it("rejects an unparseable batch instead of forwarding it unscrubbed", async () => {
+    let forwardedCount = 0;
+    const response = await browserTracesResponse(
+      makeRequest({
+        headers: { cookie: "wos-session=abc" },
+        body: `not-json ?token=synthetic-unparsed-secret`,
+      }),
+      baseEnv,
+      (async () => {
+        forwardedCount += 1;
+        return new Response(null, { status: 200 });
+      }) as typeof fetch,
+    );
+    expect(response?.status).toBe(400);
+    expect(forwardedCount).toBe(0);
+  });
+
   it("reports upstream failure as 502 without leaking detail", async () => {
     const response = await browserTracesResponse(
       makeRequest({ headers: { cookie: "wos-session=abc" } }),

apps/cloud/src/observability/browser-traces.ts

@@ -8,11 +8,32 @@
 // but it should not be an anonymous internet ingest — a session cookie must
 // at least be present, and bodies are capped. We deliberately do NOT verify
 // the session (that would put a WorkOS round-trip on every span batch).
+//
+// The batch is never forwarded opaquely. The page-side exporter scrubs its
+// own spans, but the page is not a trust boundary — anything with the cookie
+// can POST here — so the worker decodes the OTLP JSON payload, scrubs every
+// credential-bearing URL component server-side, and forwards the
+// re-serialized result. A batch that does not parse as JSON is refused: what
+// cannot be scrubbed is not forwarded.
+
+import { redactOtlpTraceExport } from "@executor-js/sdk";
 
 const MAX_BODY_BYTES = 2_000_000;
 
 export const BROWSER_TRACES_PATH = "/v1/traces";
 
+/** The batch re-serialized with every URL scrubbed, or `undefined` when it is
+ *  not valid JSON (or too deep to scrub) and must be refused. */
+const scrubbedBatch = (body: string): string | undefined => {
+  // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: JSON.parse throws on malformed input; the refusal path (400) is the handled outcome
+  try {
+    // oxlint-disable-next-line executor/no-json-parse -- boundary: the batch is deliberately unknown-shaped (the scrub walks arbitrary JSON); there is no schema to decode into
+    return JSON.stringify(redactOtlpTraceExport(JSON.parse(body)));
+  } catch {
+    return undefined;
+  }
+};
+
 export const browserTracesResponse = (
   request: Request,
   env: Env,
@@ -35,18 +56,34 @@ export const browserTracesResponse = (
   if (contentLength > MAX_BODY_BYTES) {
     return Promise.resolve(new Response(null, { status: 413 }));
   }
-  return fetchImpl(env.AXIOM_TRACES_URL ?? "https://api.axiom.co/v1/traces", {
-    method: "POST",
-    headers: {
-      "content-type": request.headers.get("content-type") ?? "application/json",
-      authorization: `Bearer ${env.AXIOM_TOKEN}`,
-      "x-axiom-dataset": env.AXIOM_DATASET ?? "executor-cloud",
+  return request.text().then(
+    (body) => {
+      // The content-length guard above trusts a header; re-check the bytes
+      // actually read.
+      if (body.length > MAX_BODY_BYTES) {
+        return new Response(null, { status: 413 });
+      }
+      const scrubbed = scrubbedBatch(body);
+      if (scrubbed === undefined) {
+        return new Response(null, { status: 400 });
+      }
+      return fetchImpl(env.AXIOM_TRACES_URL ?? "https://api.axiom.co/v1/traces", {
+        method: "POST",
+        headers: {
+          // Always JSON: the batch was decoded, scrubbed, and re-serialized.
+          "content-type": "application/json",
+          authorization: `Bearer ${env.AXIOM_TOKEN}`,
+          "x-axiom-dataset": env.AXIOM_DATASET ?? "executor-cloud",
+        },
+        body: scrubbed,
+      }).then(
+        // The exporter only needs success/failure; never reflect Axiom's
+        // response body (or its headers) back to an unauthenticated caller.
+        (upstream) => new Response(null, { status: upstream.ok ? 204 : 502 }),
+        () => new Response(null, { status: 502 }),
+      );
     },
-    body: request.body,
-  }).then(
-    // The exporter only needs success/failure; never reflect Axiom's
-    // response body (or its headers) back to an unauthenticated caller.
-    (upstream) => new Response(null, { status: upstream.ok ? 204 : 502 }),
-    () => new Response(null, { status: 502 }),
+    // An unreadable body cannot be scrubbed, so it is refused, not forwarded.
+    () => new Response(null, { status: 400 }),
   );
 };

apps/cloud/src/observability/oauth-callback-telemetry.test.ts

@@ -0,0 +1,114 @@
+// ---------------------------------------------------------------------------
+// OAuth callback × telemetry — the authorization code must never be exported.
+//
+// `/api/oauth/callback` is an app-owned path, so Effect's
+// `HttpMiddleware.tracer` opens its `http.server` span and stamps `url.full`
+// and `url.query` unconditionally (`effect/unstable/http/HttpMiddleware.ts`).
+// It redacts URL userinfo and configured header names — nothing else — so the
+// provider's `?code=…&state=…` reached the trace backend on every connect.
+//
+// This drives a real callback request through the real core API web handler,
+// with the real Effect→OTel tracer bridge exporting into an in-memory
+// exporter behind the production span-processor chain, and asserts that no
+// exported span attribute contains the code or the state.
+// ---------------------------------------------------------------------------
+
+import { describe, expect, it } from "@effect/vitest";
+import * as Resource from "@effect/opentelemetry/Resource";
+import * as OtelTracer from "@effect/opentelemetry/Tracer";
+import {
+  InMemorySpanExporter,
+  SimpleSpanProcessor,
+  type ReadableSpan,
+} from "@opentelemetry/sdk-trace-base";
+import { BasicTracerProvider } from "@opentelemetry/sdk-trace-base";
+import { Context, Effect, Layer } from "effect";
+import { HttpRouter, HttpServer } from "effect/unstable/http";
+import { HttpApiBuilder } from "effect/unstable/httpapi";
+
+import { ExecutorApi, observabilityMiddleware } from "@executor-js/api";
+import { CoreHandlers, ExecutionEngineService, ExecutorService } from "@executor-js/api/server";
+import { createExecutor } from "@executor-js/sdk";
+import { makeTestConfig } from "@executor-js/sdk/testing";
+
+import { UrlRedactingSpanProcessor } from "./redact-span-urls";
+
+// Synthetic placeholders — never a real authorization code or CSRF state.
+const CODE = "synthetic-authorization-code-9f2c";
+const STATE = "synthetic-csrf-state-4b7e";
+
+const makeTracing = () => {
+  const exporter = new InMemorySpanExporter();
+  const provider = new BasicTracerProvider({
+    // The same wrapper order production installs in `telemetry.ts`: the
+    // redactor sits outermost, so nothing downstream ever sees the secret.
+    spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))],
+  });
+  const tracerLayer = OtelTracer.layer.pipe(
+    Layer.provide(Layer.succeed(OtelTracer.OtelTracerProvider)(provider)),
+    Layer.provide(Resource.layer({ serviceName: "executor-cloud-test" })),
+  );
+  return { exporter, provider, tracerLayer };
+};
+
+describe("oauth callback telemetry", () => {
+  it.effect("exports no span attribute containing the authorization code or state", () =>
+    Effect.gen(function* () {
+      const { exporter, provider, tracerLayer } = makeTracing();
+      const executor = yield* createExecutor(makeTestConfig({}));
+
+      const web = yield* Effect.acquireRelease(
+        Effect.sync(() =>
+          HttpRouter.toWebHandler(
+            HttpApiBuilder.layer(ExecutorApi).pipe(
+              Layer.provide(CoreHandlers),
+              Layer.provide(observabilityMiddleware(ExecutorApi)),
+              Layer.provide(Layer.succeed(ExecutorService)(executor)),
+              Layer.provide(
+                Layer.succeed(ExecutionEngineService)({} as ExecutionEngineService["Service"]),
+              ),
+              Layer.provideMerge(HttpServer.layerServices),
+              Layer.provideMerge(Layer.succeed(HttpRouter.RouterConfig)({ maxParamLength: 1000 })),
+              // The Effect→OTel bridge: HttpMiddleware.tracer's span is created
+              // by this tracer, so it lands in the exporter below.
+              Layer.provideMerge(tracerLayer),
+            ),
+            { disableLogger: true },
+          ),
+        ),
+        (handle) => Effect.promise(() => handle.dispose()),
+      );
+
+      const context = Context.make(ExecutorService, executor).pipe(
+        Context.add(ExecutionEngineService, {} as ExecutionEngineService["Service"]),
+      );
+
+      // The provider round-trip: a real callback carries the grant and the
+      // CSRF state in the query string.
+      yield* Effect.promise(() =>
+        web.handler(
+          new Request(
+            `http://app.test/oauth/callback?code=${CODE}&state=${STATE}&domain=example.test`,
+          ),
+          context,
+        ),
+      );
+
+      yield* Effect.promise(() => provider.forceFlush());
+      const spans: readonly ReadableSpan[] = exporter.getFinishedSpans();
+
+      // The middleware must actually have opened a server span — otherwise
+      // this test would pass vacuously.
+      const serverSpan = spans.find((span) => span.name.startsWith("http.server"));
+      expect(serverSpan).toBeDefined();
+
+      const serialized = JSON.stringify(spans.map((span) => span.attributes));
+      expect(serialized).not.toContain(CODE);
+      expect(serialized).not.toContain(STATE);
+
+      // Route-level visibility survives the scrub.
+      expect(serverSpan?.attributes["url.path"]).toBe("/oauth/callback");
+      expect(String(serverSpan?.attributes["url.full"] ?? "")).toContain("/oauth/callback");
+    }),
+  );
+});