Harden URL handling in span attributes

.changeset/olive-moons-shake.md

@@ -0,0 +1,8 @@
+---
+"executor": patch
+---
+
+Stop exporting credential-bearing URLs in span attributes. OAuth callback
+authorization codes and CSRF state are stripped from span URL attributes before
+export, and user-supplied MCP endpoints are sanitized of query-string and
+userinfo credentials before being stamped.

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");
+    }),
+  );
+});

apps/cloud/src/observability/redact-span-urls.test.ts

@@ -0,0 +1,126 @@
+import { describe, expect, it } from "@effect/vitest";
+import {
+  BasicTracerProvider,
+  InMemorySpanExporter,
+  SimpleSpanProcessor,
+  type ReadableSpan,
+} from "@opentelemetry/sdk-trace-base";
+
+import {
+  redactSpanUrlAttributes,
+  STRIPPED_QUERY_ATTRIBUTE,
+  UrlRedactingSpanProcessor,
+} from "./redact-span-urls";
+
+// Synthetic placeholders only — never a real authorization code or state.
+const CODE = "synthetic-authorization-code";
+const STATE = "synthetic-csrf-state";
+
+const callbackUrl = `https://app.test/api/oauth/callback?code=${CODE}&state=${STATE}&domain=example.test`;
+
+/** Ends one real SDK span carrying `attributes` through the redacting
+ *  processor, and returns what the exporter actually received. Using the real
+ *  provider (rather than a hand-built span) exercises the `onEnding` → `onEnd`
+ *  hook sequence exactly as production does. */
+const exportSpanWith = (attributes: Record<string, string>): ReadableSpan | undefined => {
+  const exporter = new InMemorySpanExporter();
+  const provider = new BasicTracerProvider({
+    spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))],
+  });
+  const span = provider.getTracer("test").startSpan("http.server GET");
+  span.setAttributes(attributes);
+  span.end();
+  return exporter.getFinishedSpans()[0];
+};
+
+describe("redactSpanUrlAttributes", () => {
+  it("strips the authorization code and state from url.full and url.query", () => {
+    const attributes: Record<string, unknown> = {
+      "url.full": callbackUrl,
+      "url.query": `code=${CODE}&state=${STATE}&domain=example.test`,
+      "url.path": "/api/oauth/callback",
+      "http.request.method": "GET",
+    };
+
+    const stripped = redactSpanUrlAttributes(attributes);
+
+    expect(stripped).toEqual(["code", "state"]);
+    expect(JSON.stringify(attributes)).not.toContain(CODE);
+    expect(JSON.stringify(attributes)).not.toContain(STATE);
+    // Route-level visibility is preserved.
+    expect(attributes["url.path"]).toBe("/api/oauth/callback");
+    expect(attributes["url.full"]).toBe("https://app.test/api/oauth/callback?domain=example.test");
+    expect(attributes["url.query"]).toBe("domain=example.test");
+  });
+
+  it("strips a code nested inside the login redirect's returnTo parameter", () => {
+    // `/login` is not an app-owned path, so its span comes from the worker
+    // boundary — the callback query rides along inside `returnTo`
+    // (auth/return-to.ts + the sign-in redirect in start.ts).
+    const returnTo = encodeURIComponent(`/api/oauth/callback?code=${CODE}&state=${STATE}`);
+    const attributes: Record<string, unknown> = {
+      "url.full": `https://app.test/login?returnTo=${returnTo}`,
+      "url.query": `returnTo=${returnTo}`,
+    };
+
+    const stripped = redactSpanUrlAttributes(attributes);
+
+    expect(stripped).toEqual(["returnTo.code", "returnTo.state"]);
+    expect(JSON.stringify(attributes)).not.toContain(CODE);
+    expect(JSON.stringify(attributes)).not.toContain(STATE);
+    expect(String(attributes["url.full"])).toContain("%2Fapi%2Foauth%2Fcallback");
+  });
+
+  it("leaves a span with no sensitive parameters untouched", () => {
+    const attributes: Record<string, unknown> = {
+      "url.full": "https://app.test/api/integrations?owner=org",
+      "url.query": "owner=org",
+      "url.path": "/api/integrations",
+    };
+
+    expect(redactSpanUrlAttributes(attributes)).toEqual([]);
+    expect(attributes["url.full"]).toBe("https://app.test/api/integrations?owner=org");
+    expect(attributes["url.query"]).toBe("owner=org");
+  });
+
+  it("strips the other sensitive OAuth parameters", () => {
+    const attributes: Record<string, unknown> = {
+      "url.query":
+        "id_token=synthetic-id-token&session_state=synthetic-session&error_description=synthetic-detail&error=access_denied",
+    };
+
+    expect(redactSpanUrlAttributes(attributes)).toEqual([
+      "error_description",
+      "id_token",
+      "session_state",
+    ]);
+    // `error` is an enumerable code, not a secret — it stays.
+    expect(attributes["url.query"]).toBe("error=access_denied");
+  });
+});
+
+describe("UrlRedactingSpanProcessor", () => {
+  it("scrubs the span before the exporter sees it", () => {
+    const exported = exportSpanWith({
+      "url.full": callbackUrl,
+      "url.query": `code=${CODE}&state=${STATE}`,
+      "url.path": "/api/oauth/callback",
+    });
+
+    expect(exported).toBeDefined();
+    expect(JSON.stringify(exported?.attributes)).not.toContain(CODE);
+    expect(JSON.stringify(exported?.attributes)).not.toContain(STATE);
+    expect(exported?.attributes["url.path"]).toBe("/api/oauth/callback");
+    expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBe("code,state");
+  });
+
+  it("leaves a span with no sensitive parameters unchanged", () => {
+    const exported = exportSpanWith({
+      "url.full": "https://app.test/api/integrations?owner=org",
+      "url.query": "owner=org",
+    });
+
+    expect(exported?.attributes["url.full"]).toBe("https://app.test/api/integrations?owner=org");
+    expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBeUndefined();
+  });
+});