Scrub credentials from span attributes

Strip OAuth authorization codes and CSRF state from exported span URL attributes, and sanitize user-supplied MCP endpoints before stamping them.

Rhys Sullivan committed 3 months ago

Commit 114d8661 parent fc1e58912 files · +666 −13Browse files

Showing the changes to apps/cloud/src (4 of 12 files). Show all

Unified · Split

4 files

apps/cloud/src/observability/oauth-callback-telemetry.test.ts +114 −0
apps/cloud/src/observability/redact-span-urls.test.ts +126 −0
apps/cloud/src/observability/redact-span-urls.ts +189 −0
apps/cloud/src/observability/telemetry.ts +7 −1

apps/cloud/src/observability/oauth-callback-telemetry.test.ts+114−0

@@ -0,0 +1,114 @@
	1	+// ---------------------------------------------------------------------------
	2	+// OAuth callback × telemetry — the authorization code must never be exported.
	3	+//
	4	+// `/api/oauth/callback` is an app-owned path, so Effect's
	5	+// `HttpMiddleware.tracer` opens its `http.server` span and stamps `url.full`
	6	+// and `url.query` unconditionally (`effect/unstable/http/HttpMiddleware.ts`).
	7	+// It redacts URL userinfo and configured header names — nothing else — so the
	8	+// provider's `?code=…&state=…` reached the trace backend on every connect.
	9	+//
	10	+// This drives a real callback request through the real core API web handler,
	11	+// with the real Effect→OTel tracer bridge exporting into an in-memory
	12	+// exporter behind the production span-processor chain, and asserts that no
	13	+// exported span attribute contains the code or the state.
	14	+// ---------------------------------------------------------------------------
	15	+
	16	+import { describe, expect, it } from "@effect/vitest";
	17	+import * as Resource from "@effect/opentelemetry/Resource";
	18	+import * as OtelTracer from "@effect/opentelemetry/Tracer";
	19	+import {
	20	+ InMemorySpanExporter,
	21	+ SimpleSpanProcessor,
	22	+ type ReadableSpan,
	23	+} from "@opentelemetry/sdk-trace-base";
	24	+import { BasicTracerProvider } from "@opentelemetry/sdk-trace-base";
	25	+import { Context, Effect, Layer } from "effect";
	26	+import { HttpRouter, HttpServer } from "effect/unstable/http";
	27	+import { HttpApiBuilder } from "effect/unstable/httpapi";
	28	+
	29	+import { ExecutorApi, observabilityMiddleware } from "@executor-js/api";
	30	+import { CoreHandlers, ExecutionEngineService, ExecutorService } from "@executor-js/api/server";
	31	+import { createExecutor } from "@executor-js/sdk";
	32	+import { makeTestConfig } from "@executor-js/sdk/testing";
	33	+
	34	+import { UrlRedactingSpanProcessor } from "./redact-span-urls";
	35	+
	36	+// Synthetic placeholders — never a real authorization code or CSRF state.
	37	+const CODE = "synthetic-authorization-code-9f2c";
	38	+const STATE = "synthetic-csrf-state-4b7e";
	39	+
	40	+const makeTracing = () => {
	41	+ const exporter = new InMemorySpanExporter();
	42	+ const provider = new BasicTracerProvider({
	43	+ // The same wrapper order production installs in `telemetry.ts`: the
	44	+ // redactor sits outermost, so nothing downstream ever sees the secret.
	45	+ spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))],
	46	+ });
	47	+ const tracerLayer = OtelTracer.layer.pipe(
	48	+ Layer.provide(Layer.succeed(OtelTracer.OtelTracerProvider)(provider)),
	49	+ Layer.provide(Resource.layer({ serviceName: "executor-cloud-test" })),
	50	+ );
	51	+ return { exporter, provider, tracerLayer };
	52	+};
	53	+
	54	+describe("oauth callback telemetry", () => {
	55	+ it.effect("exports no span attribute containing the authorization code or state", () =>
	56	+ Effect.gen(function* () {
	57	+ const { exporter, provider, tracerLayer } = makeTracing();
	58	+ const executor = yield* createExecutor(makeTestConfig({}));
	59	+
	60	+ const web = yield* Effect.acquireRelease(
	61	+ Effect.sync(() =>
	62	+ HttpRouter.toWebHandler(
	63	+ HttpApiBuilder.layer(ExecutorApi).pipe(
	64	+ Layer.provide(CoreHandlers),
	65	+ Layer.provide(observabilityMiddleware(ExecutorApi)),
	66	+ Layer.provide(Layer.succeed(ExecutorService)(executor)),
	67	+ Layer.provide(
	68	+ Layer.succeed(ExecutionEngineService)({} as ExecutionEngineService["Service"]),
	69	+ ),
	70	+ Layer.provideMerge(HttpServer.layerServices),
	71	+ Layer.provideMerge(Layer.succeed(HttpRouter.RouterConfig)({ maxParamLength: 1000 })),
	72	+ // The Effect→OTel bridge: HttpMiddleware.tracer's span is created
	73	+ // by this tracer, so it lands in the exporter below.
	74	+ Layer.provideMerge(tracerLayer),
	75	+ ),
	76	+ { disableLogger: true },
	77	+ ),
	78	+ ),
	79	+ (handle) => Effect.promise(() => handle.dispose()),
	80	+ );
	81	+
	82	+ const context = Context.make(ExecutorService, executor).pipe(
	83	+ Context.add(ExecutionEngineService, {} as ExecutionEngineService["Service"]),
	84	+ );
	85	+
	86	+ // The provider round-trip: a real callback carries the grant and the
	87	+ // CSRF state in the query string.
	88	+ yield* Effect.promise(() =>
	89	+ web.handler(
	90	+ new Request(
	91	+ `http://app.test/oauth/callback?code=${CODE}&state=${STATE}&domain=example.test`,
	92	+ ),
	93	+ context,
	94	+ ),
	95	+ );
	96	+
	97	+ yield* Effect.promise(() => provider.forceFlush());
	98	+ const spans: readonly ReadableSpan[] = exporter.getFinishedSpans();
	99	+
	100	+ // The middleware must actually have opened a server span — otherwise
	101	+ // this test would pass vacuously.
	102	+ const serverSpan = spans.find((span) => span.name.startsWith("http.server"));
	103	+ expect(serverSpan).toBeDefined();
	104	+
	105	+ const serialized = JSON.stringify(spans.map((span) => span.attributes));
	106	+ expect(serialized).not.toContain(CODE);
	107	+ expect(serialized).not.toContain(STATE);
	108	+
	109	+ // Route-level visibility survives the scrub.
	110	+ expect(serverSpan?.attributes["url.path"]).toBe("/oauth/callback");
	111	+ expect(String(serverSpan?.attributes["url.full"] ?? "")).toContain("/oauth/callback");
	112	+ }),
	113	+ );
	114	+});

apps/cloud/src/observability/redact-span-urls.test.ts+126−0

@@ -0,0 +1,126 @@
	1	+import { describe, expect, it } from "@effect/vitest";
	2	+import {
	3	+ BasicTracerProvider,
	4	+ InMemorySpanExporter,
	5	+ SimpleSpanProcessor,
	6	+ type ReadableSpan,
	7	+} from "@opentelemetry/sdk-trace-base";
	8	+
	9	+import {
	10	+ redactSpanUrlAttributes,
	11	+ STRIPPED_QUERY_ATTRIBUTE,
	12	+ UrlRedactingSpanProcessor,
	13	+} from "./redact-span-urls";
	14	+
	15	+// Synthetic placeholders only — never a real authorization code or state.
	16	+const CODE = "synthetic-authorization-code";
	17	+const STATE = "synthetic-csrf-state";
	18	+
	19	+const callbackUrl = `https://app.test/api/oauth/callback?code=${CODE}&state=${STATE}&domain=example.test`;
	20	+
	21	+/** Ends one real SDK span carrying `attributes` through the redacting
	22	+ * processor, and returns what the exporter actually received. Using the real
	23	+ * provider (rather than a hand-built span) exercises the `onEnding` → `onEnd`
	24	+ * hook sequence exactly as production does. */
	25	+const exportSpanWith = (attributes: Record<string, string>): ReadableSpan \| undefined => {
	26	+ const exporter = new InMemorySpanExporter();
	27	+ const provider = new BasicTracerProvider({
	28	+ spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))],
	29	+ });
	30	+ const span = provider.getTracer("test").startSpan("http.server GET");
	31	+ span.setAttributes(attributes);
	32	+ span.end();
	33	+ return exporter.getFinishedSpans()[0];
	34	+};
	35	+
	36	+describe("redactSpanUrlAttributes", () => {
	37	+ it("strips the authorization code and state from url.full and url.query", () => {
	38	+ const attributes: Record<string, unknown> = {
	39	+ "url.full": callbackUrl,
	40	+ "url.query": `code=${CODE}&state=${STATE}&domain=example.test`,
	41	+ "url.path": "/api/oauth/callback",
	42	+ "http.request.method": "GET",
	43	+ };
	44	+
	45	+ const stripped = redactSpanUrlAttributes(attributes);
	46	+
	47	+ expect(stripped).toEqual(["code", "state"]);
	48	+ expect(JSON.stringify(attributes)).not.toContain(CODE);
	49	+ expect(JSON.stringify(attributes)).not.toContain(STATE);
	50	+ // Route-level visibility is preserved.
	51	+ expect(attributes["url.path"]).toBe("/api/oauth/callback");
	52	+ expect(attributes["url.full"]).toBe("https://app.test/api/oauth/callback?domain=example.test");
	53	+ expect(attributes["url.query"]).toBe("domain=example.test");
	54	+ });
	55	+
	56	+ it("strips a code nested inside the login redirect's returnTo parameter", () => {
	57	+ // `/login` is not an app-owned path, so its span comes from the worker
	58	+ // boundary — the callback query rides along inside `returnTo`
	59	+ // (auth/return-to.ts + the sign-in redirect in start.ts).
	60	+ const returnTo = encodeURIComponent(`/api/oauth/callback?code=${CODE}&state=${STATE}`);
	61	+ const attributes: Record<string, unknown> = {
	62	+ "url.full": `https://app.test/login?returnTo=${returnTo}`,
	63	+ "url.query": `returnTo=${returnTo}`,
	64	+ };
	65	+
	66	+ const stripped = redactSpanUrlAttributes(attributes);
	67	+
	68	+ expect(stripped).toEqual(["returnTo.code", "returnTo.state"]);
	69	+ expect(JSON.stringify(attributes)).not.toContain(CODE);
	70	+ expect(JSON.stringify(attributes)).not.toContain(STATE);
	71	+ expect(String(attributes["url.full"])).toContain("%2Fapi%2Foauth%2Fcallback");
	72	+ });
	73	+
	74	+ it("leaves a span with no sensitive parameters untouched", () => {
	75	+ const attributes: Record<string, unknown> = {
	76	+ "url.full": "https://app.test/api/integrations?owner=org",
	77	+ "url.query": "owner=org",
	78	+ "url.path": "/api/integrations",
	79	+ };
	80	+
	81	+ expect(redactSpanUrlAttributes(attributes)).toEqual([]);
	82	+ expect(attributes["url.full"]).toBe("https://app.test/api/integrations?owner=org");
	83	+ expect(attributes["url.query"]).toBe("owner=org");
	84	+ });
	85	+
	86	+ it("strips the other sensitive OAuth parameters", () => {
	87	+ const attributes: Record<string, unknown> = {
	88	+ "url.query":
	89	+ "id_token=synthetic-id-token&session_state=synthetic-session&error_description=synthetic-detail&error=access_denied",
	90	+ };
	91	+
	92	+ expect(redactSpanUrlAttributes(attributes)).toEqual([
	93	+ "error_description",
	94	+ "id_token",
	95	+ "session_state",
	96	+ ]);
	97	+ // `error` is an enumerable code, not a secret — it stays.
	98	+ expect(attributes["url.query"]).toBe("error=access_denied");
	99	+ });
	100	+});
	101	+
	102	+describe("UrlRedactingSpanProcessor", () => {
	103	+ it("scrubs the span before the exporter sees it", () => {
	104	+ const exported = exportSpanWith({
	105	+ "url.full": callbackUrl,
	106	+ "url.query": `code=${CODE}&state=${STATE}`,
	107	+ "url.path": "/api/oauth/callback",
	108	+ });
	109	+
	110	+ expect(exported).toBeDefined();
	111	+ expect(JSON.stringify(exported?.attributes)).not.toContain(CODE);
	112	+ expect(JSON.stringify(exported?.attributes)).not.toContain(STATE);
	113	+ expect(exported?.attributes["url.path"]).toBe("/api/oauth/callback");
	114	+ expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBe("code,state");
	115	+ });
	116	+
	117	+ it("leaves a span with no sensitive parameters unchanged", () => {
	118	+ const exported = exportSpanWith({
	119	+ "url.full": "https://app.test/api/integrations?owner=org",
	120	+ "url.query": "owner=org",
	121	+ });
	122	+
	123	+ expect(exported?.attributes["url.full"]).toBe("https://app.test/api/integrations?owner=org");
	124	+ expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBeUndefined();
	125	+ });
	126	+});

apps/cloud/src/observability/redact-span-urls.ts+189−0

@@ -0,0 +1,189 @@
	1	+// ---------------------------------------------------------------------------
	2	+// URL-attribute redaction for exported spans.
	3	+//
	4	+// Effect's `HttpMiddleware.tracer` stamps `url.full` and `url.query`
	5	+// unconditionally on every `http.server` span (see
	6	+// `effect/unstable/http/HttpMiddleware.ts` — it redacts URL userinfo and
	7	+// configured header names, and nothing else). `/api/oauth/callback` is an
	8	+// app-owned path, so its span carried the provider's `?code=…&state=…`
	9	+// verbatim to the trace backend on every OAuth connect.
	10	+//
	11	+// The scrub runs at the span-processor seam rather than at the route, because
	12	+// this is the only chokepoint every span in the isolate must pass through on
	13	+// its way to the exporter — worker spans, Effect spans, Durable Object spans,
	14	+// and any route added later. A per-route middleware or a `TracerDisabledWhen`
	15	+// override would only cover the routes someone remembered to wire it into, and
	16	+// overriding the middleware's attribute handling would mean forking Effect
	17	+// internals.
	18	+//
	19	+// `url.path` is deliberately preserved: route-level visibility is what makes
	20	+// these traces worth exporting at all. Only the sensitive query parameters are
	21	+// removed, and their presence is recorded as a stripped-key list so a trace
	22	+// still shows that the request carried a `code`, without its value.
	23	+// ---------------------------------------------------------------------------
	24	+
	25	+import type { Context } from "@opentelemetry/api";
	26	+import type { ReadableSpan, Span, SpanProcessor } from "@opentelemetry/sdk-trace-base";
	27	+
	28	+/** Query parameters that are credentials, single-use grants, or CSRF secrets.
	29	+ * Matched case-insensitively against the parameter name. */
	30	+const SENSITIVE_QUERY_KEYS: ReadonlySet<string> = new Set([
	31	+ "access_token",
	32	+ "client_secret",
	33	+ "code",
	34	+ "code_verifier",
	35	+ "error_description",
	36	+ "id_token",
	37	+ "refresh_token",
	38	+ "session_state",
	39	+ "state",
	40	+ "token",
	41	+]);
	42	+
	43	+/** Span attributes whose value is a whole URL. */
	44	+const URL_ATTRIBUTES = ["url.full", "http.url"] as const;
	45	+const QUERY_ATTRIBUTE = "url.query";
	46	+/** Names of the parameters removed from this span's URL attributes. Non-secret
	47	+ * by construction — it is the key list, never the values. */
	48	+export const STRIPPED_QUERY_ATTRIBUTE = "url.query.stripped_keys";
	49	+
	50	+const isSensitive = (key: string): boolean => SENSITIVE_QUERY_KEYS.has(key.toLowerCase());
	51	+
	52	+/** How deep to follow a query parameter whose value is itself a URL or path
	53	+ * with a query string. Depth 2 covers the real nesting in this app —
	54	+ * `/login?returnTo=%2Fapi%2Foauth%2Fcallback%3Fcode%3D…` (see
	55	+ * `auth/return-to.ts` and the sign-in redirect in `start.ts`) — with headroom,
	56	+ * and bounds the work per span. */
	57	+const MAX_NESTED_DEPTH = 2;
	58	+
	59	+/** The query string with every sensitive parameter dropped, plus the names
	60	+ * dropped. Parsed with `URLSearchParams` so encoding round-trips correctly.
	61	+ *
	62	+ * A parameter whose own value carries a nested query string is redacted
	63	+ * recursively rather than dropped: the login redirect round-trips the whole
	64	+ * OAuth callback path through `returnTo`, so the credential rides inside
	65	+ * another parameter's value and a top-level key check alone would miss it. */
	66	+const redactQuery = (
	67	+ query: string,
	68	+ depth = 0,
	69	+): { readonly query: string; readonly stripped: readonly string[] } => {
	70	+ const params = new URLSearchParams(query);
	71	+ const stripped = new Set<string>();
	72	+ for (const key of Array.from(new Set(params.keys()))) {
	73	+ if (isSensitive(key)) {
	74	+ stripped.add(key);
	75	+ params.delete(key);
	76	+ continue;
	77	+ }
	78	+ if (depth >= MAX_NESTED_DEPTH) continue;
	79	+ const values = params.getAll(key);
	80	+ const rewritten = values.map((value) => {
	81	+ const separator = value.indexOf("?");
	82	+ if (separator === -1) return value;
	83	+ const nested = redactQuery(value.slice(separator + 1), depth + 1);
	84	+ for (const name of nested.stripped) stripped.add(`${key}.${name}`);
	85	+ return nested.stripped.length === 0
	86	+ ? value
	87	+ : `${value.slice(0, separator)}${nested.query === "" ? "" : `?${nested.query}`}`;
	88	+ });
	89	+ if (rewritten.every((value, index) => value === values[index])) continue;
	90	+ params.delete(key);
	91	+ for (const value of rewritten) params.append(key, value);
	92	+ }
	93	+ return stripped.size === 0
	94	+ ? { query, stripped: [] }
	95	+ : { query: params.toString(), stripped: Array.from(stripped).sort() };
	96	+};
	97	+
	98	+/** The URL with every sensitive query parameter dropped. Path, host, and
	99	+ * scheme are untouched. Unparseable values are dropped entirely rather than
	100	+ * passed through — if it cannot be parsed it cannot be proven safe. */
	101	+const redactUrl = (
	102	+ value: string,
	103	+): { readonly url: string; readonly stripped: readonly string[] } => {
	104	+ if (!URL.canParse(value)) {
	105	+ const { query, stripped } = redactQuery(value);
	106	+ return { url: stripped.length === 0 ? value : query, stripped };
	107	+ }
	108	+ const url = new URL(value);
	109	+ if (url.search === "") return { url: value, stripped: [] };
	110	+ const { query, stripped } = redactQuery(url.search.slice(1));
	111	+ if (stripped.length === 0) return { url: value, stripped: [] };
	112	+ url.search = query;
	113	+ return { url: url.toString(), stripped };
	114	+};
	115	+
	116	+/** Rewrites the URL-bearing attributes of an in-flight span in place, dropping
	117	+ * sensitive query parameters. Returns the parameter names removed. */
	118	+export const redactSpanUrlAttributes = (attributes: Record<string, unknown>): readonly string[] => {
	119	+ const stripped = new Set<string>();
	120	+ for (const name of URL_ATTRIBUTES) {
	121	+ const value = attributes[name];
	122	+ if (typeof value !== "string") continue;
	123	+ const result = redactUrl(value);
	124	+ for (const key of result.stripped) stripped.add(key);
	125	+ if (result.stripped.length > 0) attributes[name] = result.url;
	126	+ }
	127	+ const query = attributes[QUERY_ATTRIBUTE];
	128	+ if (typeof query === "string") {
	129	+ const result = redactQuery(query);
	130	+ for (const key of result.stripped) stripped.add(key);
	131	+ if (result.stripped.length > 0) attributes[QUERY_ATTRIBUTE] = result.query;
	132	+ }
	133	+ return Array.from(stripped).sort();
	134	+};
	135	+
	136	+/** Wraps a span processor so every span is scrubbed of credential-bearing URL
	137	+ * query parameters before the inner processor (and therefore the exporter)
	138	+ * sees it.
	139	+ *
	140	+ * The rewrite happens in `onEnding`, the last hook the OTel SDK calls while
	141	+ * the span is still mutable (`Span.end()` runs `onEnding` before setting
	142	+ * `_ended`, so `setAttribute` still applies); `onEnd` receives a frozen
	143	+ * `ReadableSpan`. `onEnding` is optional in the SpanProcessor interface, so
	144	+ * `onEnd` re-checks the attribute bag and mutates it directly as a backstop
	145	+ * for any SDK path that skips the earlier hook. */
	146	+export class UrlRedactingSpanProcessor implements SpanProcessor {
	147	+ constructor(private readonly inner: SpanProcessor) {}
	148	+
	149	+ forceFlush(): Promise<void> {
	150	+ return this.inner.forceFlush();
	151	+ }
	152	+
	153	+ onStart(span: Span, parentContext: Context): void {
	154	+ this.inner.onStart(span, parentContext);
	155	+ }
	156	+
	157	+ onEnding(span: Span): void {
	158	+ this.redact(span.attributes, (key, value) => span.setAttribute(key, value));
	159	+ this.inner.onEnding?.(span);
	160	+ }
	161	+
	162	+ onEnd(span: ReadableSpan): void {
	163	+ const attributes = span.attributes as Record<string, unknown>;
	164	+ this.redact(attributes, (key, value) => {
	165	+ attributes[key] = value;
	166	+ });
	167	+ this.inner.onEnd(span);
	168	+ }
	169	+
	170	+ shutdown(): Promise<void> {
	171	+ return this.inner.shutdown();
	172	+ }
	173	+
	174	+ private redact(
	175	+ attributes: Record<string, unknown>,
	176	+ write: (key: string, value: string) => void,
	177	+ ): void {
	178	+ // Work on a copy so the redaction decision is made from the current values
	179	+ // and applied through the caller's writer (span API vs direct mutation).
	180	+ const draft: Record<string, unknown> = { ...attributes };
	181	+ const stripped = redactSpanUrlAttributes(draft);
	182	+ if (stripped.length === 0) return;
	183	+ for (const name of [...URL_ATTRIBUTES, QUERY_ATTRIBUTE]) {
	184	+ const value = draft[name];
	185	+ if (typeof value === "string" && value !== attributes[name]) write(name, value);
	186	+ }
	187	+ write(STRIPPED_QUERY_ATTRIBUTE, stripped.join(","));
	188	+ }
	189	+}

apps/cloud/src/observability/telemetry.ts+7−1

@@ -51,6 +51,7 @@ import { · 50 unmodified lines
51	51	OTEL_MAX_SPAN_QUEUE_SIZE,
52	52	recordForceFlush,
53	53	} from "./memory-metrics";
	54	+import { UrlRedactingSpanProcessor } from "./redact-span-urls";
54	55
55	56	const SERVICE_NAME = "executor-cloud";
56	57	const SERVICE_VERSION = "1.0.0";
@@ -93,7 +94,12 @@ const ensureGlobalTracerProvider = (): boolean => { · 36 unmodified lines
93	94	}),
94	95	OTEL_MAX_SPAN_QUEUE_SIZE,
95	96	);
96		− return [countingProcessor];
	97	+ // Outermost wrapper: every span the isolate produces passes through here
	98	+ // before it is queued for export, so credential-bearing query parameters
	99	+ // (OAuth `code`/`state` on `/api/oauth/callback`, which Effect's
	100	+ // HttpMiddleware.tracer stamps into `url.full`/`url.query`
	101	+ // unconditionally) are stripped no matter which route emitted the span.
	102	+ return [new UrlRedactingSpanProcessor(countingProcessor)];
97	103	})(),
98	104	});
99	105	// Skip `provider.register()` — its StackContextManager / W3C propagator