diff --git a/.changeset/olive-moons-shake.md b/.changeset/olive-moons-shake.md new file mode 100644 index 000000000..d9961b1a2 --- /dev/null +++ b/.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. diff --git a/apps/cloud/src/observability/browser-traces.test.ts b/apps/cloud/src/observability/browser-traces.test.ts index ce20707b0..f35bf1811 100644 --- a/apps/cloud/src/observability/browser-traces.test.ts +++ b/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" } }), diff --git a/apps/cloud/src/observability/browser-traces.ts b/apps/cloud/src/observability/browser-traces.ts index 8505678e2..929663eef 100644 --- a/apps/cloud/src/observability/browser-traces.ts +++ b/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 }), ); }; diff --git a/apps/cloud/src/observability/oauth-callback-telemetry.test.ts b/apps/cloud/src/observability/oauth-callback-telemetry.test.ts new file mode 100644 index 000000000..d96e905ef --- /dev/null +++ b/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"); + }), + ); +}); diff --git a/apps/cloud/src/observability/redact-span-urls.test.ts b/apps/cloud/src/observability/redact-span-urls.test.ts new file mode 100644 index 000000000..288c0f65c --- /dev/null +++ b/apps/cloud/src/observability/redact-span-urls.test.ts @@ -0,0 +1,286 @@ +// The redaction rules themselves are tested in `@executor-js/sdk` +// (`telemetry-url-redaction.test.ts`). These tests cover the cloud-specific +// adapter: the span-processor seam every isolate span passes through on its +// way to the exporter. + +import * as Resource from "@effect/opentelemetry/Resource"; +import * as OtelTracer from "@effect/opentelemetry/Tracer"; +import { describe, expect, it } from "@effect/vitest"; +import { SpanStatusCode, type Span } from "@opentelemetry/api"; +import { + BasicTracerProvider, + InMemorySpanExporter, + SimpleSpanProcessor, + type ReadableSpan, +} from "@opentelemetry/sdk-trace-base"; +import { Effect, Exit, Layer } from "effect"; +import { FetchHttpClient, HttpClient } from "effect/unstable/http"; + +import { STRIPPED_QUERY_ATTRIBUTE } from "@executor-js/sdk"; + +import { 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. `configure` runs before the span + * ends, for recording exceptions and setting a status. */ +const exportSpanWith = ( + attributes: Record, + configure?: (span: Span) => void, +): 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); + configure?.(span); + span.end(); + return exporter.getFinishedSpans()[0]; +}; + +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.full"]).toBe("https://app.test/api/oauth/callback"); + expect(exported?.attributes["url.path"]).toBe("/api/oauth/callback"); + expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBe("code,domain,state"); + }); + + it("drops a query value regardless of its parameter name", () => { + // Query-auth placement names are arbitrary strings — a key configured as + // `?owner=…` is exactly as much a credential as `?key=…`. + const exported = exportSpanWith({ + "url.full": "https://app.test/api/integrations?owner=synthetic-owner-secret", + "url.query": "owner=synthetic-owner-secret", + }); + + expect(JSON.stringify(exported?.attributes)).not.toContain("synthetic-owner-secret"); + expect(exported?.attributes["url.full"]).toBe("https://app.test/api/integrations"); + expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBe("owner"); + }); + + it("leaves a query-free span unchanged", () => { + const exported = exportSpanWith({ + "url.full": "https://app.test/api/integrations", + "url.path": "/api/integrations", + }); + + expect(exported?.attributes["url.full"]).toBe("https://app.test/api/integrations"); + expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBeUndefined(); + }); + + it("scrubs URL-bearing link attributes", () => { + // Span links carry attributes exactly as spans do — `ReadableSpan.links` + // is a fourth channel to the exporter, and a link stamped with the peer's + // URL must not export the credential the span's own attributes dropped. + const exporter = new InMemorySpanExporter(); + const provider = new BasicTracerProvider({ + spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))], + }); + const tracer = provider.getTracer("test"); + const upstream = tracer.startSpan("upstream"); + const span = tracer.startSpan("http.server GET", { + links: [ + { + context: upstream.spanContext(), + attributes: { + // Malformed on purpose: the free-text regex alone would not match + // it, so the URL-aware attribute path must handle link attributes. + "url.full": "http://exa mple.test/graphql?key=synthetic-link-key-secret", + "peer.note": + "after GET https://canary:synthetic-link-userinfo-secret@api.test/graphql?owner=synthetic-link-owner-secret", + }, + }, + ], + }); + span.end(); + upstream.end(); + + const exported = exporter + .getFinishedSpans() + .find((finished) => finished.name === "http.server GET"); + const links = JSON.stringify(exported?.links); + expect(links).not.toContain("synthetic-link-key-secret"); + expect(links).not.toContain("synthetic-link-userinfo-secret"); + expect(links).not.toContain("synthetic-link-owner-secret"); + // Non-vacuous: the link, its identity, and the scrubbed URLs survive. + expect(exported?.links).toHaveLength(1); + expect(links).toContain(upstream.spanContext().spanId); + expect(links).toContain("https://api.test/graphql"); + }); + + it("scrubs a credential URL inside an array attribute on the span and on a link", () => { + // OTel attributes permit string[] values, so an array element is a fifth + // way a credential-bearing URL reaches the exporter. The arrays are + // planted by direct mutation: `setAttribute` sanitization would drop a + // mixed-type array, but the processor's contract is + // `Record` — upstream bridges and hand-built + // ReadableSpans hand it arbitrary bags. + const SECRET = "synthetic-array-canary-secret"; + const canary = () => [`https://canary:${SECRET}@api.test/graphql?key=${SECRET}`, 42]; + const exporter = new InMemorySpanExporter(); + const provider = new BasicTracerProvider({ + spanProcessors: [new UrlRedactingSpanProcessor(new SimpleSpanProcessor(exporter))], + }); + const tracer = provider.getTracer("test"); + const upstream = tracer.startSpan("upstream"); + const span = tracer.startSpan("http.server GET", { + // The placeholder attribute keeps the link's bag defined — the SDK + // normalizes an empty bag away, and the canary is planted by mutation. + links: [{ context: upstream.spanContext(), attributes: { "peer.kind": "canary" } }], + }); + // oxlint-disable-next-line executor/no-double-cast -- boundary: planting an out-of-contract attribute bag on the SDK span IS the fixture; no public API accepts a mixed-type array + const bags = span as unknown as { + attributes: Record; + links: ReadonlyArray<{ attributes?: Record }>; + }; + bags.attributes["url.full"] = canary(); + const linkAttributes = bags.links[0]?.attributes; + expect(linkAttributes).toBeDefined(); + if (linkAttributes !== undefined) linkAttributes["peer.urls"] = canary(); + span.end(); + upstream.end(); + + const exported = exporter + .getFinishedSpans() + .find((finished) => finished.name === "http.server GET"); + const serialized = JSON.stringify({ + attributes: exported?.attributes, + links: exported?.links, + }); + expect(serialized).not.toContain(SECRET); + // Non-vacuous: the scrubbed URL and the non-string element survive. + expect(exported?.attributes["url.full"]).toEqual(["https://api.test/graphql", 42]); + expect(exported?.links[0]?.attributes?.["peer.urls"]).toEqual(["https://api.test/graphql", 42]); + expect(exported?.attributes[STRIPPED_QUERY_ATTRIBUTE]).toBe("key,userinfo"); + }); + + it("scrubs a credential URL inside an array attribute on a span event", () => { + // Event attributes permit string[] exactly as span and link attributes do, + // and the event walk redacts free text — so an array element is the same + // channel one level down. Planted by direct mutation for the same reason + // as the span/link array canary: `addEvent` sanitization would drop a + // mixed-type array, but upstream bridges hand the processor arbitrary bags. + const SECRET = "synthetic-event-array-canary-secret"; + const exported = exportSpanWith({}, (span) => { + // The placeholder attribute keeps the event's bag defined; the canary is + // planted by mutation. + span.addEvent("canary", { "event.kind": "canary" }); + // oxlint-disable-next-line executor/no-double-cast -- boundary: planting an out-of-contract attribute bag on the SDK span IS the fixture; no public API accepts a mixed-type array + const bags = span as unknown as { + events: ReadonlyArray<{ attributes?: Record }>; + }; + const eventAttributes = bags.events[0]?.attributes; + expect(eventAttributes).toBeDefined(); + if (eventAttributes !== undefined) { + eventAttributes["event.urls"] = [ + `https://canary:${SECRET}@api.test/graphql?key=${SECRET}`, + 42, + ]; + } + }); + + expect(JSON.stringify(exported?.events)).not.toContain(SECRET); + // Non-vacuous: the scrubbed URL and the non-string element survive. + expect(exported?.events[0]?.attributes?.["event.urls"]).toEqual([ + "https://api.test/graphql", + 42, + ]); + }); + + it("scrubs the URL out of exception events and the status message", () => { + // The shape `@effect/opentelemetry` exports for a failed request: + // `TransportError.message` embeds the raw URL, and the bridge copies it + // into `exception.message`/`exception.stacktrace` event attributes and + // into `status.message`. + const message = `Transport: fetch failed (GET https://canary:synthetic-userinfo-secret@api.test/graphql?key=synthetic-key-secret)`; + const exported = exportSpanWith({}, (span) => { + // oxlint-disable-next-line executor/no-error-constructor -- boundary: OTel's recordException takes a plain JS Error; reproducing the bridge's exception shape IS the fixture + span.recordException(new Error(message)); + span.setStatus({ code: SpanStatusCode.ERROR, message }); + }); + + // Non-vacuous: the exception event exists and kept its scrubbed URL. + const events = JSON.stringify(exported?.events); + expect(events).toContain("exception"); + expect(events).toContain("https://api.test/graphql"); + expect(events).not.toContain("synthetic-userinfo-secret"); + expect(events).not.toContain("synthetic-key-secret"); + expect(exported?.status.message).toBe("Transport: fetch failed (GET https://api.test/graphql)"); + }); +}); + +describe("credential canary — no export channel carries the secret", () => { + const USERINFO_SECRET = "synthetic-canary-userinfo-secret"; + const KEY_SECRET = "synthetic-canary-query-key-secret"; + + it.effect( + "a failed outbound request exports no attribute, event, or status with the secret", + () => { + const exporter = new InMemorySpanExporter(); + const provider = new BasicTracerProvider({ + 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 Effect.gen(function* () { + const client = yield* HttpClient.HttpClient; + + // Port 1 refuses immediately, so both requests fail without any network + // dependency, and the failing error's message embeds the raw URL + // (`TransportError.message` is "Transport: … (GET )"). The secret + // rides in userinfo and in a query parameter named `key` — a real + // GraphQL-integration auth shape no name blocklist would catch. + const withUserinfo = yield* client + .get(`http://canary-user:${USERINFO_SECRET}@127.0.0.1:1/graphql?key=${KEY_SECRET}`) + .pipe(Effect.withSpan("canary.outbound_userinfo"), Effect.exit); + const refused = yield* client + .get(`http://127.0.0.1:1/graphql?key=${KEY_SECRET}`) + .pipe(Effect.withSpan("canary.outbound_refused"), Effect.exit); + expect(Exit.isFailure(withUserinfo)).toBe(true); + expect(Exit.isFailure(refused)).toBe(true); + + yield* Effect.promise(() => provider.forceFlush()); + const spans = exporter.getFinishedSpans(); + + // Non-vacuous: the failures produced ERROR spans that recorded + // exception events and a status message. + const errored = spans.filter((span) => span.status.code === SpanStatusCode.ERROR); + expect(errored.length).toBeGreaterThan(0); + expect(errored.some((span) => span.events.length > 0)).toBe(true); + expect(errored.some((span) => (span.status.message ?? "") !== "")).toBe(true); + + const serialized = JSON.stringify( + spans.map((span) => ({ + name: span.name, + attributes: span.attributes, + events: span.events, + status: span.status, + })), + ); + expect(serialized).not.toContain(USERINFO_SECRET); + expect(serialized).not.toContain(KEY_SECRET); + // The scrub redacts; it does not erase — the host survives for debugging. + expect(serialized).toContain("127.0.0.1"); + }).pipe(Effect.provide(Layer.mergeAll(FetchHttpClient.layer, tracerLayer))); + }, + ); +}); diff --git a/apps/cloud/src/observability/redact-span-urls.ts b/apps/cloud/src/observability/redact-span-urls.ts new file mode 100644 index 000000000..ced051321 --- /dev/null +++ b/apps/cloud/src/observability/redact-span-urls.ts @@ -0,0 +1,131 @@ +// --------------------------------------------------------------------------- +// URL redaction at the cloud span-processor seam. +// +// The redaction rules themselves live in `@executor-js/sdk` +// (`telemetry-url-redaction`) and are shared by every exporter path — this +// module only adapts them to the OpenTelemetry SDK's span-processor +// interface, which is the one chokepoint every span in a cloud isolate must +// pass through on its way to the exporter: worker spans, Effect spans, +// Durable Object spans, and any route added later. A per-route middleware or +// a `TracerDisabledWhen` override would only cover the routes someone +// remembered to wire it into. +// +// Four channels are scrubbed: attributes (`url.full` / `url.query` stamped +// unconditionally by Effect's HttpMiddleware.tracer and HttpClient), event +// attributes (`exception.message` / `exception.stacktrace` carry the raw URL +// of a failed request), link attributes (a link to a peer span carries the +// peer's own attribute bag), and the status message. `url.path` is +// deliberately preserved — route-level visibility is what makes these traces +// worth exporting at all. +// --------------------------------------------------------------------------- + +import type { Context } from "@opentelemetry/api"; +import type { ReadableSpan, Span, SpanProcessor } from "@opentelemetry/sdk-trace-base"; + +import { + redactSpanUrlAttributes, + redactStringElements, + redactUrlsInText, + STRIPPED_QUERY_ATTRIBUTE, +} from "@executor-js/sdk"; + +/** The mutable surface of a span the redactor needs: the attribute bag, the + * recorded events (exception events carry `exception.message` and + * `exception.stacktrace`), the links (each carries its own attribute bag), + * and the status (whose `message` carries the failing error's message). Both + * `Span` and `ReadableSpan` expose all four as plain mutable objects. */ +interface RedactableSpan { + readonly attributes: Record; + readonly events: ReadonlyArray<{ + name: string; + attributes?: Record | undefined; + }>; + readonly links: ReadonlyArray<{ + attributes?: Record | undefined; + }>; + readonly status: { message?: string | undefined }; +} + +/** Wraps a span processor so every span is scrubbed of credential-bearing URL + * components — in attributes, in event attributes, in link attributes, and + * in the status message — before the inner processor (and therefore the + * exporter) sees it. + * + * The attribute rewrite happens in `onEnding`, the last hook the OTel SDK + * calls while the span is still mutable (`Span.end()` runs `onEnding` before + * setting `_ended`, so `setAttribute` still applies); `onEnd` receives a + * frozen `ReadableSpan`. `onEnding` is optional in the SpanProcessor + * interface, so `onEnd` re-checks the attribute bag and mutates it directly + * as a backstop for any SDK path that skips the earlier hook. Events and + * status have no setter on an ended span in either hook, so they are always + * scrubbed by direct mutation. */ +export class UrlRedactingSpanProcessor implements SpanProcessor { + constructor(private readonly inner: SpanProcessor) {} + + forceFlush(): Promise { + return this.inner.forceFlush(); + } + + onStart(span: Span, parentContext: Context): void { + this.inner.onStart(span, parentContext); + } + + onEnding(span: Span): void { + this.redact(span, (key, value) => span.setAttribute(key, value)); + this.inner.onEnding?.(span); + } + + onEnd(span: ReadableSpan): void { + this.redact(span, (key, value) => { + span.attributes[key] = value; + }); + this.inner.onEnd(span); + } + + shutdown(): Promise { + return this.inner.shutdown(); + } + + private redact(span: RedactableSpan, write: (key: string, value: string) => void): void { + // Work on a copy so the redaction decision is made from the current values + // and applied through the caller's writer (span API vs direct mutation). + const draft: Record = { ...span.attributes }; + const stripped = new Set(redactSpanUrlAttributes(draft)); + for (const [name, value] of Object.entries(draft)) { + if (typeof value === "string" && value !== span.attributes[name]) write(name, value); + } + + // Links have no setter on an ended span, so their attribute bags are + // scrubbed by direct mutation — the same full URL-aware pass the span's + // own attributes get, since a link carries an arbitrary attribute bag. + for (const link of span.links) { + if (link.attributes === undefined) continue; + for (const key of redactSpanUrlAttributes(link.attributes)) stripped.add(key); + } + + if (stripped.size > 0) write(STRIPPED_QUERY_ATTRIBUTE, Array.from(stripped).sort().join(",")); + + for (const event of span.events) { + const name = redactUrlsInText(event.name); + if (name !== event.name) event.name = name; + if (event.attributes === undefined) continue; + for (const [key, value] of Object.entries(event.attributes)) { + // Event attributes permit string[] exactly as span attributes do, so + // array elements get the same free-text scrub, in place. + if (Array.isArray(value)) { + redactStringElements(value, redactUrlsInText); + continue; + } + if (typeof value !== "string") continue; + const redacted = redactUrlsInText(value); + if (redacted !== value) event.attributes[key] = redacted; + } + } + + const message = span.status.message; + if (typeof message === "string") { + const redacted = redactUrlsInText(message); + if (redacted !== message) span.status.message = redacted; + } + } +} diff --git a/apps/cloud/src/observability/telemetry.ts b/apps/cloud/src/observability/telemetry.ts index 3e992a215..91731b787 100644 --- a/apps/cloud/src/observability/telemetry.ts +++ b/apps/cloud/src/observability/telemetry.ts @@ -52,6 +52,7 @@ import { OTEL_MAX_SPAN_QUEUE_SIZE, recordForceFlush, } from "./memory-metrics"; +import { UrlRedactingSpanProcessor } from "./redact-span-urls"; const SERVICE_NAME = "executor-cloud"; @@ -124,7 +125,12 @@ const ensureGlobalTracerProvider = (): boolean => { }), OTEL_MAX_SPAN_QUEUE_SIZE, ); - return [countingProcessor]; + // Outermost wrapper: every span the isolate produces passes through here + // before it is queued for export, so credential-bearing query parameters + // (OAuth `code`/`state` on `/api/oauth/callback`, which Effect's + // HttpMiddleware.tracer stamps into `url.full`/`url.query` + // unconditionally) are stripped no matter which route emitted the span. + return [new UrlRedactingSpanProcessor(countingProcessor)]; })(), }); // Skip `provider.register()` — its StackContextManager / W3C propagator diff --git a/apps/host-selfhost/src/telemetry.test.ts b/apps/host-selfhost/src/telemetry.test.ts index 9cc4a9683..e0fecf246 100644 --- a/apps/host-selfhost/src/telemetry.test.ts +++ b/apps/host-selfhost/src/telemetry.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "@effect/vitest"; import { FetchHttpClient } from "effect/unstable/http"; -import { Effect, Layer } from "effect"; +import { Cause, Effect, Layer } from "effect"; import { makeTelemetryLive } from "./telemetry"; @@ -86,6 +86,101 @@ test("log export needs its own endpoint when only traces is configured", async ( expect(urls).toEqual(["http://collector.test/v1/traces"]); }); +// The self-host exporter is its own path to the wire — no cloud span processor +// runs here — so the credential scrub is asserted on the serialized OTLP +// payload the collector actually receives. +test("the exported payload carries no query values, userinfo, or fragments", async () => { + const SECRET = "synthetic-selfhost-canary"; + const seen: Array = []; + await Effect.runPromise( + Effect.gen(function* () { + // A span whose URL attributes carry every credential shape at once. + yield* Effect.void.pipe( + Effect.withSpan("canary.request", { + attributes: { + "url.full": `https://svc:${SECRET}-userinfo@api.test/graphql?owner=${SECRET}-query#access_token=${SECRET}-fragment`, + "url.query": `owner=${SECRET}-query`, + "error.message": `request to https://api.test/graphql?key=${SECRET}-text failed`, + }, + }), + ); + // A failed span: the error message (with its raw URL) becomes the + // exported status message and exception event. + yield* Effect.fail( + `Transport: fetch failed (GET https://u:${SECRET}-err@api.test/graphql?key=${SECRET}-errq)`, + ).pipe(Effect.withSpan("canary.failure"), Effect.exit); + }).pipe( + Effect.provide( + makeTelemetryLive({ OTEL_EXPORTER_OTLP_ENDPOINT: "http://collector.test" }).pipe( + Layer.provide( + stubFetch((request) => { + seen.push(request); + return new Response(null, { status: 200 }); + }), + ), + ), + ), + Effect.scoped, + ), + ); + + const body = await seen[0]?.text(); + expect(body).toBeDefined(); + // Non-vacuous: both spans made it onto the wire with their host and path. + expect(body).toContain("canary.request"); + expect(body).toContain("canary.failure"); + expect(body).toContain("/graphql"); + expect(body).not.toContain(SECRET); + // No exported url.full keeps a query string. + expect(body).not.toContain("graphql?"); +}); + +// Logs are their own OTLP signal with their own serialization path: the +// OtlpLogger exports `Cause.pretty` output and every log annotation, and the +// server logs OAuth callback failure causes whose error messages embed the raw +// URL. The scrub is asserted on the logs payload the collector receives. +test("the exported logs payload carries no credential-bearing URLs", async () => { + const SECRET = "synthetic-selfhost-log-canary"; + const seen: Array = []; + await Effect.runPromise( + Effect.gen(function* () { + yield* Effect.logError( + "oauth callback failed", + Cause.fail( + `Transport: fetch failed (GET https://u:${SECRET}-userinfo@api.test/api/oauth/callback?code=${SECRET}-code)`, + ), + ).pipe( + Effect.annotateLogs( + "request.url", + `https://api.test/api/oauth/callback?code=${SECRET}-annotation`, + ), + ); + }).pipe( + Effect.provide( + makeTelemetryLive({ + OTEL_EXPORTER_OTLP_ENDPOINT: "http://collector.test", + EXECUTOR_OTEL_EXPORT_LOGS: "true", + }).pipe( + Layer.provide( + stubFetch((request) => { + seen.push(request); + return new Response(null, { status: 200 }); + }), + ), + ), + ), + Effect.scoped, + ), + ); + + const body = await seen.find((request) => request.url.endsWith("/v1/logs"))?.text(); + expect(body).toBeDefined(); + // Non-vacuous: the record, its cause, and the scrubbed URL are on the wire. + expect(body).toContain("oauth callback failed"); + expect(body).toContain("/api/oauth/callback"); + expect(body).not.toContain(SECRET); +}); + // --- helpers --------------------------------------------------------------- // `Fetch` is a Context.Reference with `globalThis.fetch` as its default, so diff --git a/apps/host-selfhost/src/telemetry.ts b/apps/host-selfhost/src/telemetry.ts index 4b41b8033..8fed1c612 100644 --- a/apps/host-selfhost/src/telemetry.ts +++ b/apps/host-selfhost/src/telemetry.ts @@ -20,9 +20,11 @@ // --------------------------------------------------------------------------- import { FetchHttpClient } from "effect/unstable/http"; -import { OtlpLogger, OtlpSerialization, OtlpTracer } from "effect/unstable/observability"; +import { OtlpLogger, OtlpTracer } from "effect/unstable/observability"; import { Layer } from "effect"; +import { UrlRedactingOtlpSerializationJson } from "@executor-js/sdk"; + import packageJson from "../package.json" with { type: "json" }; const SERVICE_NAME = "executor-selfhost"; @@ -111,7 +113,13 @@ export const makeTelemetryLive = ( ); return Layer.merge(TracerLive, LoggerLive).pipe( - Layer.provide(OtlpSerialization.layerJson), + // The redacting serialization is the scrub for this exporter path: no + // cloud span processor runs here, so credential-bearing URL components + // (query values, userinfo, fragments) are stripped from the trace and log + // payloads at the serialization seam every exported span and log record + // passes through. Removal only — this path adds no stripped-keys + // diagnostic. + Layer.provide(UrlRedactingOtlpSerializationJson), // The exporter gets a plain fetch client, deliberately not the guarded // hosted client: the collector is an address the operator configured, so // the SSRF guard's job (stopping agent-chosen URLs from reaching the diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index 2990d605a..a69979b58 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -508,3 +508,21 @@ export { insufficientScopeFromEmbeddedJson, type InsufficientScopeDetection, } from "./insufficient-scope"; + +// Endpoint sanitization for span attributes — plugins stamping a user-supplied +// endpoint must strip its credential-bearing parts first. +export { endpointForTelemetry, endpointTelemetryAttributes } from "./telemetry-endpoint"; + +// URL redaction for exported telemetry — the shared scrub every exporter path +// (cloud span processors, self-host and browser OTLP serialization, the +// browser-traces forwarder) consumes. +export { + redactOtlpTraceExport, + redactSpanUrlAttributes, + redactStringElements, + redactUrlForTelemetry, + redactUrlsInText, + STRIPPED_QUERY_ATTRIBUTE, + UrlRedactingOtlpSerializationJson, + type RedactedUrl, +} from "./telemetry-url-redaction"; diff --git a/packages/core/sdk/src/shared.ts b/packages/core/sdk/src/shared.ts index a353500c1..d0cd24da7 100644 --- a/packages/core/sdk/src/shared.ts +++ b/packages/core/sdk/src/shared.ts @@ -210,3 +210,16 @@ export { type OAuthPopupResult, isOAuthPopupResult, } from "./oauth-popup-types"; + +// URL redaction for exported telemetry (browser-safe — pure Effect). The +// browser client provides the redacting OTLP serialization to its exporter so +// page-side spans are scrubbed before they leave the page. +export { + redactOtlpTraceExport, + redactSpanUrlAttributes, + redactUrlForTelemetry, + redactUrlsInText, + STRIPPED_QUERY_ATTRIBUTE, + UrlRedactingOtlpSerializationJson, + type RedactedUrl, +} from "./telemetry-url-redaction"; diff --git a/packages/core/sdk/src/telemetry-endpoint.test.ts b/packages/core/sdk/src/telemetry-endpoint.test.ts new file mode 100644 index 000000000..7b4fc1f17 --- /dev/null +++ b/packages/core/sdk/src/telemetry-endpoint.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { endpointForTelemetry, endpointTelemetryAttributes } from "./telemetry-endpoint"; + +// Synthetic placeholders only. +const QUERY_TOKEN = "synthetic-query-token"; +const USERINFO_PASSWORD = "synthetic-userinfo-password"; + +describe("endpointForTelemetry", () => { + it("strips a credential carried in the query string", () => { + // The shape the MCP preset list ships and the add-flow passes through raw. + expect(endpointForTelemetry(`https://mcp.example.test/mcp?token=${QUERY_TOKEN}`)).toBe( + "https://mcp.example.test/mcp", + ); + }); + + it("strips a credential carried in URL userinfo", () => { + expect(endpointForTelemetry(`https://svc-user:${USERINFO_PASSWORD}@mcp.example.test/mcp`)).toBe( + "https://mcp.example.test/mcp", + ); + }); + + it("strips query, fragment, and userinfo together", () => { + const scrubbed = endpointForTelemetry( + `https://svc-user:${USERINFO_PASSWORD}@mcp.example.test/mcp?token=${QUERY_TOKEN}#frag`, + ); + expect(scrubbed).toBe("https://mcp.example.test/mcp"); + expect(scrubbed).not.toContain(QUERY_TOKEN); + expect(scrubbed).not.toContain(USERINFO_PASSWORD); + }); + + it("leaves a credential-free endpoint intact", () => { + expect(endpointForTelemetry("https://mcp.example.test/mcp")).toBe( + "https://mcp.example.test/mcp", + ); + }); + + it("returns credential-free unparseable input as-is", () => { + expect(endpointForTelemetry("not a url")).toBe("not a url"); + }); + + it("truncates an unparseable paste at the first `?` instead of passing it through", () => { + // A space in the host makes this unparseable, but the `?token=…` shape is + // still a credential and must not be stamped verbatim. + const scrubbed = endpointForTelemetry(`http://exa mple.test/mcp?token=${QUERY_TOKEN}`); + expect(scrubbed).toBe("http://exa mple.test/mcp"); + expect(scrubbed).not.toContain(QUERY_TOKEN); + }); + + it("drops a userinfo-looking prefix from an unparseable paste", () => { + const scrubbed = endpointForTelemetry( + `http://svc-user:${USERINFO_PASSWORD}@exa mple.test/mcp?token=${QUERY_TOKEN}`, + ); + expect(scrubbed).toBe("exa mple.test/mcp"); + expect(scrubbed).not.toContain(QUERY_TOKEN); + expect(scrubbed).not.toContain(USERINFO_PASSWORD); + }); +}); + +describe("endpointTelemetryAttributes", () => { + it("keeps the endpoint debuggable without exposing the credential", () => { + const attributes = endpointTelemetryAttributes( + "mcp.endpoint", + `https://svc-user:${USERINFO_PASSWORD}@mcp.example.test/mcp?token=${QUERY_TOKEN}`, + ); + + expect(attributes).toEqual({ + "mcp.endpoint": "https://mcp.example.test/mcp", + "mcp.endpoint.origin": "https://mcp.example.test", + "mcp.endpoint.has_query": true, + "mcp.endpoint.has_fragment": false, + "mcp.endpoint.has_userinfo": true, + }); + expect(JSON.stringify(attributes)).not.toContain(QUERY_TOKEN); + expect(JSON.stringify(attributes)).not.toContain(USERINFO_PASSWORD); + }); + + it("reports absence of both credential shapes for a plain endpoint", () => { + expect(endpointTelemetryAttributes("mcp.endpoint", "https://mcp.example.test/mcp")).toEqual({ + "mcp.endpoint": "https://mcp.example.test/mcp", + "mcp.endpoint.origin": "https://mcp.example.test", + "mcp.endpoint.has_query": false, + "mcp.endpoint.has_fragment": false, + "mcp.endpoint.has_userinfo": false, + }); + }); + + it("reports a fragment-only malformed endpoint honestly", () => { + // The `#` starts a fragment, not a query — `has_query` must not claim one, + // and the fragment's presence must be recorded as its own signal. + const attributes = endpointTelemetryAttributes( + "mcp.endpoint", + `http://exa mple.test/mcp#access_token=${QUERY_TOKEN}`, + ); + + expect(attributes["mcp.endpoint"]).toBe("http://exa mple.test/mcp"); + expect(attributes["mcp.endpoint.has_query"]).toBe(false); + expect(attributes["mcp.endpoint.has_fragment"]).toBe(true); + expect(JSON.stringify(attributes)).not.toContain(QUERY_TOKEN); + }); + + it("reports a fragment on a parseable endpoint", () => { + const attributes = endpointTelemetryAttributes( + "mcp.endpoint", + `https://mcp.example.test/mcp#access_token=${QUERY_TOKEN}`, + ); + + expect(attributes["mcp.endpoint"]).toBe("https://mcp.example.test/mcp"); + expect(attributes["mcp.endpoint.has_query"]).toBe(false); + expect(attributes["mcp.endpoint.has_fragment"]).toBe(true); + expect(JSON.stringify(attributes)).not.toContain(QUERY_TOKEN); + }); + + it("degrades an unparseable paste without leaking either credential shape", () => { + const attributes = endpointTelemetryAttributes( + "mcp.endpoint", + `http://svc-user:${USERINFO_PASSWORD}@exa mple.test/mcp?token=${QUERY_TOKEN}`, + ); + + expect(attributes).toEqual({ + "mcp.endpoint": "exa mple.test/mcp", + "mcp.endpoint.has_query": true, + "mcp.endpoint.has_fragment": false, + "mcp.endpoint.has_userinfo": true, + }); + expect(JSON.stringify(attributes)).not.toContain(QUERY_TOKEN); + expect(JSON.stringify(attributes)).not.toContain(USERINFO_PASSWORD); + }); +}); diff --git a/packages/core/sdk/src/telemetry-endpoint.ts b/packages/core/sdk/src/telemetry-endpoint.ts new file mode 100644 index 000000000..749b6ee90 --- /dev/null +++ b/packages/core/sdk/src/telemetry-endpoint.ts @@ -0,0 +1,88 @@ +// --------------------------------------------------------------------------- +// Endpoint sanitization for span attributes. +// +// User-supplied endpoints are a credential carrier: `?token=…` in the query +// string and `user:pass@host` userinfo are both first-class supported input +// shapes (the MCP preset list ships a query-token URL, and the add-flow passes +// the raw paste straight through). Stamping such a URL verbatim onto a span +// ships the credential to the trace backend. +// +// Span attributes may carry hostnames, paths, and booleans; they may never +// carry the secret-bearing parts of a URL. `endpointForTelemetry` keeps the +// scheme/host/path — the parts that make a trace debuggable — and drops the +// query, fragment, and userinfo. `endpointTelemetryAttributes` adds the +// non-sensitive companions (origin, and whether a query string was present) so +// "the user pasted a URL with credentials in it" stays diagnosable without the +// credential itself. +// --------------------------------------------------------------------------- + +/** Fallback for input `URL` cannot parse. A malformed paste can still carry + * every credential shape (`user:password@`, `?token=…`, `#access_token=…`), + * so it must never pass through verbatim — degrade by truncation instead. + * Everything from the first `?` or `#` is dropped (an unparseable query + * string cannot be proven credential-free), and anything before a remaining + * `@` is dropped with it (it may be userinfo; over-stripping is the safe + * direction here). The `hadQuery` / `hadFragment` booleans are honest: a `?` + * after the `#` is fragment content, not a query, and a fragment-only paste + * reports no query. */ +const opaqueEndpoint = ( + endpoint: string, +): { + readonly sanitized: string; + readonly hadQuery: boolean; + readonly hadFragment: boolean; + readonly hadUserinfo: boolean; +} => { + const queryStart = endpoint.indexOf("?"); + const fragmentStart = endpoint.indexOf("#"); + const cut = endpoint.search(/[?#]/); + const beforeCut = cut === -1 ? endpoint : endpoint.slice(0, cut); + const userinfoEnd = beforeCut.lastIndexOf("@"); + return { + sanitized: userinfoEnd === -1 ? beforeCut : beforeCut.slice(userinfoEnd + 1), + hadQuery: queryStart !== -1 && (fragmentStart === -1 || queryStart < fragmentStart), + hadFragment: fragmentStart !== -1, + hadUserinfo: userinfoEnd !== -1, + }; +}; + +/** The endpoint with every credential-bearing component removed: query string, + * fragment, and `user:pass@` userinfo. Unparseable input degrades through the + * same textual truncation — never verbatim, because a malformed paste is + * exactly where a stray credential hides. */ +export const endpointForTelemetry = (endpoint: string): string => { + if (!URL.canParse(endpoint)) return opaqueEndpoint(endpoint).sanitized; + const url = new URL(endpoint); + url.search = ""; + url.hash = ""; + url.username = ""; + url.password = ""; + return url.toString(); +}; + +/** Span attributes describing an endpoint without exposing its credentials. + * `` is the sanitized URL, `.origin` the scheme+host, and the + * booleans record which credential-bearing components (query string, + * fragment, userinfo) were stripped. */ +export const endpointTelemetryAttributes = ( + prefix: string, + endpoint: string, +): Record => { + if (!URL.canParse(endpoint)) { + const opaque = opaqueEndpoint(endpoint); + return { + [prefix]: opaque.sanitized, + [`${prefix}.has_query`]: opaque.hadQuery, + [`${prefix}.has_fragment`]: opaque.hadFragment, + [`${prefix}.has_userinfo`]: opaque.hadUserinfo, + }; + } + const url = new URL(endpoint); + return { + [prefix]: endpointForTelemetry(endpoint), + [`${prefix}.origin`]: url.origin, + [`${prefix}.has_query`]: url.search !== "", + [`${prefix}.has_fragment`]: url.hash !== "", + [`${prefix}.has_userinfo`]: url.username !== "" || url.password !== "", + }; +}; diff --git a/packages/core/sdk/src/telemetry-url-redaction.test.ts b/packages/core/sdk/src/telemetry-url-redaction.test.ts new file mode 100644 index 000000000..c44753ddf --- /dev/null +++ b/packages/core/sdk/src/telemetry-url-redaction.test.ts @@ -0,0 +1,330 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { + redactOtlpTraceExport, + redactSpanUrlAttributes, + redactUrlForTelemetry, + redactUrlsInText, +} from "./telemetry-url-redaction"; + +// 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`; + +describe("redactSpanUrlAttributes", () => { + it("strips every query value from url.full and url.query", () => { + const attributes: Record = { + "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", "domain", "state"]); + expect(JSON.stringify(attributes)).not.toContain(CODE); + expect(JSON.stringify(attributes)).not.toContain(STATE); + // Route-level visibility is preserved; no query value is. + expect(attributes["url.path"]).toBe("/api/oauth/callback"); + expect(attributes["url.full"]).toBe("https://app.test/api/oauth/callback"); + expect(attributes["url.query"]).toBe(""); + }); + + it("drops a secret riding under any parameter name — names are never trusted", () => { + // Query-auth placement names are arbitrary strings: a key configured as + // `?owner=…` or `?error=…` is exactly as much a credential as `?key=…`. + // That is why no allowlist of "safe" names can exist. + const attributes: Record = { + "url.full": + "https://api.test/graphql?owner=synthetic-owner-secret&error=synthetic-error-secret", + "url.query": "owner=synthetic-owner-secret&error=synthetic-error-secret", + }; + + const stripped = redactSpanUrlAttributes(attributes); + + expect(stripped).toEqual(["error", "owner"]); + expect(JSON.stringify(attributes)).not.toContain("synthetic-owner-secret"); + expect(JSON.stringify(attributes)).not.toContain("synthetic-error-secret"); + expect(attributes["url.full"]).toBe("https://api.test/graphql"); + }); + + it("drops a nested returnTo value wholesale — code, state, and userinfo included", () => { + // The login redirect round-trips the whole OAuth callback URL through + // `returnTo`, so credentials ride inside another parameter's value. The + // value is dropped with every other query value; nothing nested survives. + const returnTo = encodeURIComponent( + `https://canary:synthetic-nested-userinfo-secret@app.test/api/oauth/callback?code=${CODE}&state=${STATE}`, + ); + const attributes: Record = { + "url.full": `https://app.test/login?returnTo=${returnTo}`, + "url.query": `returnTo=${returnTo}`, + }; + + const stripped = redactSpanUrlAttributes(attributes); + + expect(stripped).toEqual(["returnTo"]); + expect(JSON.stringify(attributes)).not.toContain(CODE); + expect(JSON.stringify(attributes)).not.toContain(STATE); + expect(JSON.stringify(attributes)).not.toContain("synthetic-nested-userinfo-secret"); + expect(attributes["url.full"]).toBe("https://app.test/login"); + }); + + it("reports a nameless query segment as *, never by its text", () => { + // `?` parses as a parameter NAME — echoing it would leak the token + // through the stripped-keys list. + const attributes: Record = { + "url.full": "https://api.test/hook?synthetic-bare-token-secret", + }; + + const stripped = redactSpanUrlAttributes(attributes); + + expect(stripped).toEqual(["*"]); + expect(JSON.stringify(attributes)).not.toContain("synthetic-bare-token-secret"); + expect(attributes["url.full"]).toBe("https://api.test/hook"); + }); + + it("clears a fragment-borne token from a parseable URL", () => { + const attributes: Record = { + "url.full": "https://app.test/callback#access_token=synthetic-fragment-token", + }; + + const stripped = redactSpanUrlAttributes(attributes); + + expect(stripped).toEqual(["fragment"]); + expect(JSON.stringify(attributes)).not.toContain("synthetic-fragment-token"); + expect(attributes["url.full"]).toBe("https://app.test/callback"); + }); + + it("clears a fragment-borne token from a malformed URL", () => { + const attributes: Record = { + "url.full": "http://exa mple.test/callback#access_token=synthetic-fragment-token", + }; + + redactSpanUrlAttributes(attributes); + + expect(JSON.stringify(attributes)).not.toContain("synthetic-fragment-token"); + expect(attributes["url.full"]).toBe("http://exa mple.test/callback"); + }); + + it("does not treat an @ inside the fragment as userinfo", () => { + const attributes: Record = { + "url.full": "http://exa mple.test/docs#note@anchor", + }; + + redactSpanUrlAttributes(attributes); + + expect(attributes["url.full"]).toBe("http://exa mple.test/docs"); + }); + + it("strips userinfo from a URL attribute", () => { + const attributes: Record = { + "url.full": "https://svc:synthetic-basic-password@api.test/graphql", + }; + + expect(redactSpanUrlAttributes(attributes)).toEqual(["userinfo"]); + expect(attributes["url.full"]).toBe("https://api.test/graphql"); + expect(JSON.stringify(attributes)).not.toContain("synthetic-basic-password"); + }); + + it("scrubs a URL embedded in a free-text attribute", () => { + const attributes: Record = { + "error.message": "request to https://api.test/graphql?key=synthetic-key failed", + }; + + redactSpanUrlAttributes(attributes); + + expect(attributes["error.message"]).toBe("request to https://api.test/graphql failed"); + }); + + it("degrades an unparseable URL attribute instead of passing it through", () => { + const attributes: Record = { + "url.full": "http://exa mple.test/graphql?key=synthetic-key", + }; + + expect(redactSpanUrlAttributes(attributes)).toEqual(["key"]); + expect(attributes["url.full"]).toBe("http://exa mple.test/graphql"); + }); + + it("redacts the string elements of an array attribute value in place", () => { + // OTel attributes permit string[] values — an array element carries a + // credential-bearing URL exactly as a scalar does. Non-string elements + // pass through untouched and the array keeps its identity. + const SECRET = "synthetic-array-element-secret"; + const urlArray = [`https://canary:${SECRET}@host.test/graphql?key=${SECRET}`, 42]; + const attributes: Record = { + "url.full": urlArray, + "url.query": [`key=${SECRET}`, 7], + "error.notes": [`request to https://api.test/graphql?key=${SECRET} failed`, true], + }; + + const stripped = redactSpanUrlAttributes(attributes); + + expect(stripped).toEqual(["key", "userinfo"]); + expect(JSON.stringify(attributes)).not.toContain(SECRET); + expect(attributes["url.full"]).toBe(urlArray); + expect(attributes["url.full"]).toEqual(["https://host.test/graphql", 42]); + expect(attributes["url.query"]).toEqual(["", 7]); + expect(attributes["error.notes"]).toEqual(["request to https://api.test/graphql failed", true]); + }); + + it("leaves a query-, fragment-, and userinfo-free URL untouched", () => { + const attributes: Record = { + "url.full": "https://app.test/api/integrations", + "url.path": "/api/integrations", + }; + + expect(redactSpanUrlAttributes(attributes)).toEqual([]); + expect(attributes["url.full"]).toBe("https://app.test/api/integrations"); + }); +}); + +describe("redactUrlForTelemetry", () => { + it("strips query, fragment, and userinfo together", () => { + const result = redactUrlForTelemetry( + "https://svc:synthetic-pass@api.test/x?owner=synthetic-q#access_token=synthetic-f", + ); + expect(result.url).toBe("https://api.test/x"); + expect(result.stripped).toEqual(["fragment", "owner", "userinfo"]); + }); + + it("returns a clean URL unchanged", () => { + expect(redactUrlForTelemetry("https://api.test/x")).toEqual({ + url: "https://api.test/x", + stripped: [], + }); + }); +}); + +describe("redactUrlsInText", () => { + it("scrubs userinfo, query values, and fragments from embedded URLs", () => { + const text = `Transport: fetch failed (GET https://u:synthetic-pass@api.test/x?key=synthetic-key#t=synthetic-frag)`; + expect(redactUrlsInText(text)).toBe("Transport: fetch failed (GET https://api.test/x)"); + }); +}); + +describe("redactOtlpTraceExport", () => { + it("scrubs every channel of a serialized OTLP batch", () => { + const SECRET = "synthetic-otlp-secret"; + const payload = { + resourceSpans: [ + { + resource: { attributes: [] }, + scopeSpans: [ + { + scope: { name: "executor-web" }, + spans: [ + { + traceId: "0af7651916cd43dd8448eb211c80319c", + spanId: "b7ad6b7169203331", + name: "http.client GET", + attributes: [ + { + key: "url.full", + value: { + stringValue: `https://u:${SECRET}-userinfo@api.test/x?owner=${SECRET}-query#t=${SECRET}-fragment`, + }, + }, + { key: "url.query", value: { stringValue: `owner=${SECRET}-query` } }, + { + key: "http.url", + // Malformed on purpose: the free-text regex alone would + // not match it, so the key-aware path must. + value: { stringValue: `http://exa mple.test/x?key=${SECRET}-malformed` }, + }, + ], + events: [ + { + name: "exception", + attributes: [ + { + key: "exception.message", + value: { + stringValue: `GET https://api.test/x?key=${SECRET}-event failed`, + }, + }, + ], + }, + ], + status: { + code: 2, + message: `GET https://api.test/x?key=${SECRET}-status failed`, + }, + links: [ + { + traceId: "0af7651916cd43dd8448eb211c80319c", + spanId: "00f067aa0ba902b7", + attributes: [ + { + key: "peer.url", + value: { stringValue: `https://api.test/x?key=${SECRET}-link` }, + }, + ], + }, + ], + }, + ], + }, + ], + }, + ], + }; + + const serialized = JSON.stringify(redactOtlpTraceExport(payload)); + + expect(serialized).not.toContain(SECRET); + // Non-vacuous: identity, route, and structure survive the scrub. + expect(serialized).toContain("0af7651916cd43dd8448eb211c80319c"); + expect(serialized).toContain("https://api.test/x"); + expect(serialized).toContain("exception"); + expect(serialized).toContain("http://exa mple.test/x"); + }); + + it("scrubs the sibling fields of a URL-keyed KeyValue instead of passing them through", () => { + // The URL-aware KeyValue special case must not exempt the REST of that + // object from the walk: a crafted KeyValue can carry URL-bearing text in a + // sibling of `key`/`value.stringValue`, and the browser-traces forwarder + // feeds attacker-shaped JSON straight through this function. + const SECRET = "synthetic-keyvalue-sibling-secret"; + const payload = { + resourceSpans: [ + { + scopeSpans: [ + { + spans: [ + { + attributes: [ + { + key: "url.full", + value: { + stringValue: "https://api.test/x?owner=synthetic-keyvalue-main-secret", + extraValue: `see https://api.test/x?owner=${SECRET}-inner`, + }, + note: `retry of https://api.test/x?owner=${SECRET}-sibling`, + }, + ], + }, + ], + }, + ], + }, + ], + }; + + const serialized = JSON.stringify(redactOtlpTraceExport(payload)); + + expect(serialized).not.toContain(SECRET); + expect(serialized).not.toContain("synthetic-keyvalue-main-secret"); + // Non-vacuous: the redacted URL survives in every field. + expect(serialized).toContain("https://api.test/x"); + }); + + it("drops content past the nesting bound rather than forwarding it unexamined", () => { + let deep: unknown = "https://api.test/x?key=synthetic-deep-secret"; + for (let index = 0; index < 200; index += 1) deep = [deep]; + const serialized = JSON.stringify(redactOtlpTraceExport({ resourceSpans: deep })); + expect(serialized).not.toContain("synthetic-deep-secret"); + }); +}); diff --git a/packages/core/sdk/src/telemetry-url-redaction.ts b/packages/core/sdk/src/telemetry-url-redaction.ts new file mode 100644 index 000000000..2897d31be --- /dev/null +++ b/packages/core/sdk/src/telemetry-url-redaction.ts @@ -0,0 +1,286 @@ +// --------------------------------------------------------------------------- +// URL redaction for exported telemetry — the shared implementation every +// exporter path consumes. +// +// Effect's `HttpMiddleware.tracer` stamps `url.full` and `url.query` +// unconditionally on every `http.server` span, and `HttpClient` does the same +// for outbound `http.client` spans (see `effect/unstable/http`). Those URLs +// routinely carry credentials: `/api/oauth/callback` receives the provider's +// `?code=…&state=…`, query-auth placements put API keys under arbitrary +// parameter names (`?key=…`, `?owner=…` — the NAME is operator-chosen free +// text), fragments carry implicit-grant tokens (`#access_token=…`), and +// userinfo carries basic-auth passwords. Because parameter names are +// arbitrary, no allowlist of "safe" names can exist: EVERY query parameter +// value is dropped, every fragment is dropped, and userinfo is dropped. Only +// scheme, host, and path survive — that is what makes a trace debuggable. The +// parameter NAMES (never values) are reported so a trace still shows that a +// request carried a `code`, without its value; a nameless segment (`?token` — +// indistinguishable from a bare value) is reported as `*`. That diagnostic is +// stamped only by the cloud span processor, which holds whole spans; the +// serialization-seam consumers below remove secrets and report nothing. +// +// URLs also escape the attribute bag: when a request fails, Effect's error +// types embed the raw URL in their `message` ("Transport: … (GET )"), +// which the exporters copy into exception events and the span status. So the +// scrub has a free-text form too, applied to every exported string. +// +// This module is the single source of truth. Consumers: +// - apps/cloud wraps its OTel span processors (`UrlRedactingSpanProcessor`) +// around `redactSpanUrlAttributes`/`redactUrlsInText`; +// - the self-host server and the browser client provide +// `UrlRedactingOtlpSerializationJson` to their Effect OTLP exporters, so +// the scrub runs at the serialization seam every span and log record +// passes through; +// - apps/cloud's browser-traces forwarder scrubs the decoded OTLP JSON +// batch with `redactOtlpTraceExport` before forwarding it. +// --------------------------------------------------------------------------- + +import { Layer } from "effect"; +import * as HttpBody from "effect/unstable/http/HttpBody"; +// The deep module: `effect/unstable/observability` re-exports OtlpSerialization +// as a NAMESPACE, and passing the namespace to `Layer.succeed` would key the +// context entry on `undefined`. The service class itself lives here. +import { OtlpSerialization } from "effect/unstable/observability/OtlpSerialization"; + +/** Span attributes whose value is a whole URL. */ +const URL_ATTRIBUTES = ["url.full", "http.url"] as const; +const QUERY_ATTRIBUTE = "url.query"; +/** Names of the parameters removed from this span's URL attributes, plus the + * markers `userinfo` / `fragment` when those components were dropped and `*` + * for a nameless query segment. Non-secret by construction — it is the key + * list, never the values. Stamped only by the cloud span processor; the + * serialization-seam paths remove secrets without reporting. */ +export const STRIPPED_QUERY_ATTRIBUTE = "url.query.stripped_keys"; + +const isUrlAttribute = (name: string): boolean => + (URL_ATTRIBUTES as readonly string[]).includes(name); + +/** The parameter NAMES of a query string. A segment without `=` has no name — + * it may be a bare credential (`?`), so it is reported as `*` rather + * than echoed. */ +const queryParameterNames = (query: string): readonly string[] => { + const names = new Set(); + for (const segment of query.split("&")) { + if (segment === "") continue; + const separator = segment.indexOf("="); + names.add(separator === -1 ? "*" : segment.slice(0, separator)); + } + return Array.from(names).sort(); +}; + +/** A URL reduced to its exportable parts, and what was removed. */ +export interface RedactedUrl { + readonly url: string; + readonly stripped: readonly string[]; +} + +/** The URL with userinfo, the entire query string, and the fragment removed. + * Scheme, host, and path are untouched. An unparseable value is degraded + * textually — truncated at its first `?` or `#`, then shorn of any + * `user:password@` prefix — never passed through: if it cannot be parsed it + * cannot be proven safe (over-stripping is the safe direction). */ +export const redactUrlForTelemetry = (value: string): RedactedUrl => { + const stripped = new Set(); + if (URL.canParse(value)) { + const url = new URL(value); + let changed = false; + if (url.username !== "" || url.password !== "") { + url.username = ""; + url.password = ""; + stripped.add("userinfo"); + changed = true; + } + if (url.search !== "") { + for (const name of queryParameterNames(url.search.slice(1))) stripped.add(name); + url.search = ""; + changed = true; + } + if (url.hash !== "") { + stripped.add("fragment"); + url.hash = ""; + changed = true; + } + return changed + ? { url: url.toString(), stripped: Array.from(stripped).sort() } + : { url: value, stripped: [] }; + } + // Malformed fallback. The `#` cut runs before the `@` scan so an `@` inside + // a fragment is never misread as userinfo. + const cut = value.search(/[?#]/); + let head = cut === -1 ? value : value.slice(0, cut); + if (cut !== -1) { + const tail = value.slice(cut); + const fragmentStart = tail.indexOf("#"); + if (fragmentStart !== -1) stripped.add("fragment"); + if (tail.startsWith("?")) { + const query = fragmentStart === -1 ? tail.slice(1) : tail.slice(1, fragmentStart); + for (const name of queryParameterNames(query)) stripped.add(name); + } + } + const userinfoEnd = head.lastIndexOf("@"); + if (userinfoEnd !== -1) { + stripped.add("userinfo"); + head = head.slice(userinfoEnd + 1); + } + return head === value + ? { url: value, stripped: [] } + : { url: head, stripped: Array.from(stripped).sort() }; +}; + +/** Matches URL-shaped substrings inside free text — error messages, stack + * traces, status descriptions. The character class stops at whitespace and + * common delimiters so `(GET http://…)` captures only the URL. */ +const URL_IN_TEXT = /[A-Za-z][A-Za-z0-9+.-]*:\/\/[^\s"'`<>()[\]{}]+/g; + +/** Free text with every embedded URL redacted (userinfo, query values, and + * fragments removed). This is how exception messages and status descriptions + * are scrubbed — they carry the URL mid-sentence, not as a whole attribute + * value. */ +export const redactUrlsInText = (text: string): string => + text.replace(URL_IN_TEXT, (match) => redactUrlForTelemetry(match).url); + +/** Applies `redact` to each string element of `values` in place, leaving + * non-string elements untouched. OTel attributes permit string[] values, so + * an array element carries a URL exactly as a scalar does. In-place mutation + * preserves the array's identity, which is how the change reaches a span + * whose bag was shallow-copied before redaction. Exported for the cloud + * span-processor's event-attribute walk, which redacts the same shapes. */ +export const redactStringElements = ( + values: unknown[], + redact: (value: string) => string, +): void => { + for (let index = 0; index < values.length; index += 1) { + const element = values[index]; + if (typeof element === "string") values[index] = redact(element); + } +}; + +/** Rewrites the URL-bearing attributes of a span attribute bag in place, + * dropping userinfo, every query parameter value, and the fragment. Every + * other string attribute is scrubbed as free text, so a URL embedded in an + * error-message attribute cannot slip through either. Array values get the + * same treatment element by element — OTel attributes permit string[]. + * Returns the stripped parameter names/markers. */ +export const redactSpanUrlAttributes = (attributes: Record): readonly string[] => { + const stripped = new Set(); + const redactWholeUrl = (value: string): string => { + const result = redactUrlForTelemetry(value); + for (const key of result.stripped) stripped.add(key); + return result.url; + }; + for (const name of URL_ATTRIBUTES) { + const value = attributes[name]; + if (Array.isArray(value)) { + redactStringElements(value, redactWholeUrl); + continue; + } + if (typeof value !== "string") continue; + const redacted = redactWholeUrl(value); + if (redacted !== value) attributes[name] = redacted; + } + // The raw query attribute never survives; its parameter names are already + // reported via the stripped-keys list. + const dropQuery = (value: string): string => { + for (const key of queryParameterNames(value)) stripped.add(key); + return ""; + }; + const query = attributes[QUERY_ATTRIBUTE]; + if (Array.isArray(query)) { + redactStringElements(query, dropQuery); + } else if (typeof query === "string" && query !== "") { + attributes[QUERY_ATTRIBUTE] = dropQuery(query); + } + for (const [name, value] of Object.entries(attributes)) { + if (isUrlAttribute(name) || name === QUERY_ATTRIBUTE) continue; + if (Array.isArray(value)) { + redactStringElements(value, redactUrlsInText); + continue; + } + if (typeof value !== "string") continue; + const redacted = redactUrlsInText(value); + if (redacted !== value) attributes[name] = redacted; + } + return Array.from(stripped).sort(); +}; + +// --------------------------------------------------------------------------- +// OTLP export payload scrub. +// --------------------------------------------------------------------------- + +/** Nesting bound for the payload walk. An OTLP batch is a few levels deep; + * anything deeper is not a trace batch, and its content is dropped rather + * than forwarded unexamined. */ +const MAX_SCRUB_DEPTH = 64; + +const scrubValue = (value: unknown, depth: number): unknown => { + if (typeof value === "string") return redactUrlsInText(value); + if (Array.isArray(value)) { + return depth >= MAX_SCRUB_DEPTH ? [] : value.map((item) => scrubValue(item, depth + 1)); + } + if (value !== null && typeof value === "object") { + if (depth >= MAX_SCRUB_DEPTH) return {}; + const record = value as Record; + const result: Record = {}; + for (const [name, item] of Object.entries(record)) { + result[name] = scrubValue(item, depth + 1); + } + // An OTLP KeyValue whose key names a URL attribute additionally gets the + // URL-aware scrub on its string value (free-text scrubbing alone would + // miss a malformed URL that the text regex does not match). This runs ON + // TOP of the generic walk above — never instead of it — so a crafted + // KeyValue cannot smuggle URL-bearing text through a sibling field. + const key = record["key"]; + const inner = record["value"]; + if ( + typeof key === "string" && + (isUrlAttribute(key) || key === QUERY_ATTRIBUTE) && + inner !== null && + typeof inner === "object" + ) { + const text = (inner as Record)["stringValue"]; + if (typeof text === "string") { + result["value"] = { + ...(result["value"] as Record), + stringValue: key === QUERY_ATTRIBUTE ? "" : redactUrlForTelemetry(text).url, + }; + } + } + return result; + } + return value; +}; + +/** A decoded OTLP trace-export payload (`{ resourceSpans: … }`) with every + * string scrubbed of embedded URL credentials and every `url.full` / + * `http.url` / `url.query` attribute value redacted. The walk is generic — + * every string in the tree passes through the free-text scrub — so a + * credential-bearing URL cannot hide in a field the OTLP schema does not + * name. Removal only: this path adds no `url.query.stripped_keys` + * diagnostic (that reporting exists only on the cloud span-processor path). */ +export const redactOtlpTraceExport = (payload: unknown): unknown => scrubValue(payload, 0); + +/** A decoded OTLP log-export payload (`{ resourceLogs: … }`) with every string + * scrubbed of embedded URL credentials. Log records need this as much as + * spans do: Effect's `OtlpLogger` exports `Cause.pretty` output (a failure's + * error message embeds the raw URL of the request that failed) and every log + * annotation verbatim. The walk is the same one traces get, including the + * URL-aware KeyValue handling for annotation attributes. */ +export const redactOtlpLogExport = (payload: unknown): unknown => scrubValue(payload, 0); + +/** JSON OTLP serialization with the trace and log payloads scrubbed at the + * serialization seam — the one chokepoint every exported span and log record + * passes through in Effect's OTLP exporter, regardless of which layer created + * it. Drop-in replacement for `OtlpSerialization.layerJson`. Metrics + * serialize unchanged: they are aggregated numbers under fixed metric names, + * not request-derived strings. This seam removes secrets only; the + * stripped-parameter-names diagnostic is stamped solely by the cloud span + * processor, which sees spans as attribute bags rather than serialized + * batches. */ +export const UrlRedactingOtlpSerializationJson: Layer.Layer = Layer.succeed( + OtlpSerialization, + { + traces: (spans) => HttpBody.jsonUnsafe(redactOtlpTraceExport(spans)), + metrics: (metrics) => HttpBody.jsonUnsafe(metrics), + logs: (logs) => HttpBody.jsonUnsafe(redactOtlpLogExport(logs)), + }, +); diff --git a/packages/plugins/graphql/src/sdk/invoke.ts b/packages/plugins/graphql/src/sdk/invoke.ts index b3ae55e20..bb2ee78be 100644 --- a/packages/plugins/graphql/src/sdk/invoke.ts +++ b/packages/plugins/graphql/src/sdk/invoke.ts @@ -1,6 +1,8 @@ import { Effect, Layer, Option } from "effect"; import { HttpClient, HttpClientRequest } from "effect/unstable/http"; +import { endpointForTelemetry } from "@executor-js/sdk/core"; + import { GraphqlInvocationError } from "./errors"; import { type OperationBinding, InvocationResult } from "./types"; @@ -13,14 +15,6 @@ const endpointWithQueryParams = (endpoint: string, queryParams: Record { - if (!URL.canParse(endpoint)) return endpoint; - const url = new URL(endpoint); - url.search = ""; - url.hash = ""; - return url.toString(); -}; - // Below Cloudflare's approximate 125-second subrequest limit while preserving // slow upstream requests that Executor's HTTP transports support. export const GRAPHQL_INVOCATION_TIMEOUT_MS = 110_000; diff --git a/packages/plugins/graphql/src/sdk/plugin.test.ts b/packages/plugins/graphql/src/sdk/plugin.test.ts index acc095ada..10a83add4 100644 --- a/packages/plugins/graphql/src/sdk/plugin.test.ts +++ b/packages/plugins/graphql/src/sdk/plugin.test.ts @@ -16,6 +16,7 @@ import { ProviderKey, ToolAddress, createExecutor, + endpointForTelemetry, } from "@executor-js/sdk"; import { makeTestConfig, @@ -24,7 +25,6 @@ import { } from "@executor-js/sdk/testing"; import { graphqlPlugin } from "./plugin"; -import { endpointForTelemetry } from "./invoke"; import { introspect } from "./introspect"; import type { IntrospectionResult } from "./introspect"; import { diff --git a/packages/plugins/mcp/src/sdk/plugin.test.ts b/packages/plugins/mcp/src/sdk/plugin.test.ts index 4634d2fd1..a132aace9 100644 --- a/packages/plugins/mcp/src/sdk/plugin.test.ts +++ b/packages/plugins/mcp/src/sdk/plugin.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect, Layer, Option, Predicate, Schema } from "effect"; +import { Cause, Effect, Exit, Layer, Option, Predicate, Schema, Tracer } from "effect"; import { HttpClient, HttpClientRequest, @@ -1155,3 +1155,103 @@ describe("mcpPlugin detect URL-token fallback", () => { }), ); }); + +describe("mcpPlugin endpoint telemetry", () => { + // A credential in the endpoint's query string is a first-class supported + // input shape here (the shipped preset list carries one, and the add-flow + // passes the raw paste through), so the endpoint must be sanitized before it + // is stamped onto a span. Synthetic placeholders only. + const QUERY_TOKEN = "synthetic-endpoint-token"; + const USERINFO_PASSWORD = "synthetic-endpoint-password"; + + /** Records every span the program opens, so the stamped attributes can be + * read back. Port 1 connection-refuses immediately, so detection resolves + * without any network dependency. */ + const recordingTracer = (spans: Array) => + Tracer.make({ + span: (options) => { + const span = new Tracer.NativeSpan(options); + spans.push(span); + return span; + }, + context: (primitive, fiber) => primitive["~effect/Effect/evaluate"](fiber), + }); + + /** Serializes spans the way the OTel export bridge would see them — + * attributes, events, and, for a failed span, the error channel + * (`@effect/opentelemetry` stamps each pretty error's message/stack as an + * exception EVENT and `errors[0].message` as `status.message`). A + * credential hiding in any of those channels fails the assertion, not just + * one hiding in an attribute. */ + const serializeExportChannels = (spans: ReadonlyArray): string => + JSON.stringify( + spans.map((span) => ({ + attributes: Object.fromEntries(span.attributes.entries()), + events: span.events.map(([name, , attributes]) => ({ name, attributes })), + errors: + Predicate.isTagged(span.status, "Ended") && Exit.isFailure(span.status.exit) + ? Cause.prettyErrors(span.status.exit.cause).map((prettyError) => ({ + name: prettyError.name, + message: prettyError.message, + stack: prettyError.stack ?? "", + })) + : [], + })), + ); + + it.effect("stamps a sanitized endpoint on the detect span", () => + Effect.gen(function* () { + const spans: Array = []; + const executor = yield* createExecutor(makeTestConfig({ plugins: [mcpPlugin()] as const })); + + yield* executor.integrations + .detect(`http://svc-user:${USERINFO_PASSWORD}@127.0.0.1:1/api/mcp?token=${QUERY_TOKEN}`) + .pipe(Effect.provideService(Tracer.Tracer, recordingTracer(spans))); + + const detect = spans.find((span) => span.name === "mcp.plugin.detect"); + expect(detect).toBeDefined(); + expect(detect?.attributes.get("mcp.endpoint")).toBe("http://127.0.0.1:1/api/mcp"); + // The non-sensitive companions keep the trace debuggable. + expect(detect?.attributes.get("mcp.endpoint.origin")).toBe("http://127.0.0.1:1"); + expect(detect?.attributes.get("mcp.endpoint.has_query")).toBe(true); + expect(detect?.attributes.get("mcp.endpoint.has_userinfo")).toBe(true); + + // Scoped to the plugin's own spans. Effect's HttpClient separately + // stamps `url.full`/`url.query` on its outgoing client spans + // (`effect/unstable/http/HttpClient.ts:685,690`); those are scrubbed + // downstream by the cloud export pipeline's `UrlRedactingSpanProcessor`, + // which is not installed at this level. + const serialized = serializeExportChannels( + spans.filter((span) => span.name.startsWith("mcp.plugin.")), + ); + expect(serialized).not.toContain(QUERY_TOKEN); + expect(serialized).not.toContain(USERINFO_PASSWORD); + }), + ); + + it.effect("stamps a sanitized endpoint on the probe_endpoint span", () => + Effect.gen(function* () { + const spans: Array = []; + const executor = yield* createExecutor(makeTestConfig({ plugins: [mcpPlugin()] as const })); + + yield* executor.mcp + .probeEndpoint(`http://127.0.0.1:1/mcp?token=${QUERY_TOKEN}`) + .pipe(Effect.exit, Effect.provideService(Tracer.Tracer, recordingTracer(spans))); + + const probe = spans.find((span) => span.name === "mcp.plugin.probe_endpoint"); + expect(probe).toBeDefined(); + expect(probe?.attributes.get("mcp.endpoint")).toBe("http://127.0.0.1:1/mcp"); + expect(probe?.attributes.get("mcp.endpoint.has_query")).toBe(true); + + // Scoped to the plugin's own spans. Effect's HttpClient separately + // stamps `url.full`/`url.query` on its outgoing client spans + // (`effect/unstable/http/HttpClient.ts:685,690`); those are scrubbed + // downstream by the cloud export pipeline's `UrlRedactingSpanProcessor`, + // which is not installed at this level. + const serialized = serializeExportChannels( + spans.filter((span) => span.name.startsWith("mcp.plugin.")), + ); + expect(serialized).not.toContain(QUERY_TOKEN); + }), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 1f5cd77fe..27165206a 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -10,6 +10,7 @@ import { AuthTemplateSlug, ConnectionName, definePlugin, + endpointTelemetryAttributes, IntegrationAlreadyExistsError, IntegrationSlug, mergeAuthTemplates, @@ -963,7 +964,13 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { }); }).pipe( Effect.withSpan("mcp.plugin.probe_endpoint", { - attributes: { "mcp.endpoint": typeof input === "string" ? input : input.endpoint }, + // The probed endpoint is raw user paste and routinely carries a + // credential in its query string (`?token=…`) — sanitize before + // stamping. + attributes: endpointTelemetryAttributes( + "mcp.endpoint", + typeof input === "string" ? input : input.endpoint, + ), }), ); @@ -1579,7 +1586,8 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { }).pipe( Effect.catch(() => Effect.succeed(null)), Effect.withSpan("mcp.plugin.detect", { - attributes: { "mcp.endpoint": url }, + // Same raw-paste input as probe_endpoint — sanitize before stamping. + attributes: endpointTelemetryAttributes("mcp.endpoint", url), }), ), diff --git a/packages/react/src/api/client.tsx b/packages/react/src/api/client.tsx index bdb0fc455..e519313b2 100644 --- a/packages/react/src/api/client.tsx +++ b/packages/react/src/api/client.tsx @@ -2,16 +2,15 @@ import * as Atom from "effect/unstable/reactivity/Atom"; import * as AtomHttpApi from "effect/unstable/reactivity/AtomHttpApi"; import { FetchHttpClient, HttpClient, HttpClientRequest } from "effect/unstable/http"; import * as HttpClientError from "effect/unstable/http/HttpClientError"; -import { OtlpSerialization, OtlpTracer } from "effect/unstable/observability"; import { ExecutorApi } from "@executor-js/api/client"; import * as Cause from "effect/Cause"; import * as Effect from "effect/Effect"; -import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; import { reportHandledFrontendError } from "./error-reporting"; import { notifyLocalAuthRequired } from "./local-auth"; +import { makeBrowserTracingLayer } from "./tracing"; import { EXECUTOR_ORG_HEADER, getActiveOrgSlug, @@ -85,29 +84,13 @@ const otlpSampleRatio = Number( // no export). addGlobalLayer is provideMerge'd into every runtime built by // the default factory, which is exactly what AtomHttpApi services use. // -// TracerDisabledWhen must be URL-scoped, NOT a blanket `() => true` on the -// exporter's client: addGlobalLayer leaks provided references into the -// shared runtime context, and a blanket predicate silently disables -// tracing for EVERY HttpClient — no spans, no traceparent, no export, no -// error. URL-scoped, the leak is the desired behavior: any client posting -// to the OTLP endpoint (the exporter) goes untraced, everything else is -// traced. // Browser-only (this module is also evaluated during SSR, where the worker // has its own tracer and a relative exporter URL would be meaningless). if (otlpTracesUrl && typeof document !== "undefined" && Math.random() < otlpSampleRatio) { Atom.runtime.addGlobalLayer( - Layer.mergeAll( - OtlpTracer.layer({ - // Relative paths (the prod shape: "/v1/traces" → the worker's - // forwarding route) resolve against the page's own origin. - url: new URL(otlpTracesUrl, window.location.origin).toString(), - resource: { serviceName: "executor-web" }, - // Browser sessions are short; the 5s default loses the tail spans - // when the tab closes. - exportInterval: "1 second", - }).pipe(Layer.provide(OtlpSerialization.layerJson), Layer.provide(FetchHttpClient.layer)), - Layer.succeed(HttpClient.TracerDisabledWhen, (request) => request.url.includes("/v1/traces")), - ), + // Relative paths (the prod shape: "/v1/traces" → the worker's + // forwarding route) resolve against the page's own origin. + makeBrowserTracingLayer(new URL(otlpTracesUrl, window.location.origin).toString()), ); } diff --git a/packages/react/src/api/tracing.test.ts b/packages/react/src/api/tracing.test.ts new file mode 100644 index 000000000..b0e959988 --- /dev/null +++ b/packages/react/src/api/tracing.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "@effect/vitest"; +import { FetchHttpClient } from "effect/unstable/http"; +import { Effect, Layer } from "effect"; + +import { makeBrowserTracingLayer } from "./tracing"; + +/** `Fetch` is a Context.Reference defaulting to `globalThis.fetch`, so a stub + * layer overrides it without discharging any requirement. */ +const stubFetch = (onRequest: (request: Request) => void): Layer.Layer => + Layer.succeed(FetchHttpClient.Fetch)(((input: RequestInfo | URL, init?: RequestInit) => { + onRequest(input instanceof Request ? input : new Request(String(input), init)); + return Promise.resolve(new Response(null, { status: 200 })); + }) as typeof globalThis.fetch); + +// The browser client exports its spans itself — no cloud span processor runs +// in the page — so the scrub is asserted on the serialized OTLP payload the +// exporter posts to /v1/traces. +describe("browser tracing layer", () => { + it.effect("the exported payload carries no query values, userinfo, or fragments", () => { + const SECRET = "synthetic-browser-canary"; + const seen: Array = []; + return Effect.gen(function* () { + yield* Effect.void.pipe( + Effect.withSpan("canary.request", { + attributes: { + "url.full": `https://svc:${SECRET}-userinfo@api.test/graphql?owner=${SECRET}-query#access_token=${SECRET}-fragment`, + "url.query": `owner=${SECRET}-query`, + }, + }), + ); + yield* Effect.fail( + `Transport: fetch failed (GET https://u:${SECRET}-err@api.test/graphql?key=${SECRET}-errq)`, + ).pipe(Effect.withSpan("canary.failure"), Effect.exit); + }).pipe( + Effect.provide( + makeBrowserTracingLayer("http://page.test/v1/traces").pipe( + Layer.provide(stubFetch((request) => seen.push(request))), + ), + ), + Effect.scoped, + Effect.andThen( + Effect.promise(async () => { + const body = await seen[0]?.text(); + expect(body).toBeDefined(); + // Non-vacuous: both spans made it onto the wire with host and path. + expect(body).toContain("canary.request"); + expect(body).toContain("canary.failure"); + expect(body).toContain("/graphql"); + expect(body).not.toContain(SECRET); + // No exported url.full keeps a query string. + expect(body).not.toContain("graphql?"); + }), + ), + ); + }); +}); diff --git a/packages/react/src/api/tracing.ts b/packages/react/src/api/tracing.ts new file mode 100644 index 000000000..22b09d252 --- /dev/null +++ b/packages/react/src/api/tracing.ts @@ -0,0 +1,38 @@ +// --------------------------------------------------------------------------- +// Browser tracing layer — extracted from client.tsx so the export path itself +// is testable: what leaves the page is exactly what this layer serializes. +// --------------------------------------------------------------------------- + +import { FetchHttpClient, HttpClient } from "effect/unstable/http"; +import { OtlpTracer } from "effect/unstable/observability"; +import * as Layer from "effect/Layer"; + +import { UrlRedactingOtlpSerializationJson } from "@executor-js/sdk/shared"; + +/** + * OTLP tracing for the browser client, exporting to `tracesUrl`. + * + * The serialization layer is the redacting one from `@executor-js/sdk`: the + * page exports its own spans (no server-side span processor ever sees them), + * so credential-bearing URL components — query values, userinfo, fragments, + * and URLs embedded in error text — are scrubbed at the serialization seam + * before the batch leaves the page. + * + * `TracerDisabledWhen` must be URL-scoped, NOT a blanket `() => true` on the + * exporter's client: addGlobalLayer leaks provided references into the shared + * runtime context, and a blanket predicate silently disables tracing for + * EVERY HttpClient — no spans, no traceparent, no export, no error. + * URL-scoped, the leak is the desired behavior: any client posting to the + * OTLP endpoint (the exporter) goes untraced, everything else is traced. + */ +export const makeBrowserTracingLayer = (tracesUrl: string): Layer.Layer => + Layer.mergeAll( + OtlpTracer.layer({ + url: tracesUrl, + resource: { serviceName: "executor-web" }, + // Browser sessions are short; the 5s default loses the tail spans when + // the tab closes. + exportInterval: "1 second", + }).pipe(Layer.provide(UrlRedactingOtlpSerializationJson), Layer.provide(FetchHttpClient.layer)), + Layer.succeed(HttpClient.TracerDisabledWhen, (request) => request.url.includes("/v1/traces")), + );