diff --git a/.changeset/oauth-token-endpoint-error-leak.md b/.changeset/oauth-token-endpoint-error-leak.md new file mode 100644 index 000000000..64cada46e --- /dev/null +++ b/.changeset/oauth-token-endpoint-error-leak.md @@ -0,0 +1,9 @@ +--- +"executor": patch +--- + +**Token material no longer reaches OAuth error messages or logs** + +When a token endpoint replied in a way the OAuth library could not parse, the resulting `OAuth2Error` carried the parsed response body as its `cause`. On a malformed `200` that body is a *successful* token response — so an access token, and sometimes a refresh token, travelled inside an error object into whatever logged it. + +The body preview is now built from an allowlist of fields that are safe to show (`error`, `errors`, `error_description`, `error_uri`, and `code`/`message`/`detail` nested inside them) rather than from a denylist of fields to hide, so a field nobody anticipated is omitted by default instead of printed by default. The same allowlist applies to form-encoded bodies, previews are depth-bounded, and the failure summary records the token endpoint's hostname rather than its full URL, which can carry identifiers in its path. diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index 38471bda7..08117af09 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -6,13 +6,15 @@ // --------------------------------------------------------------------------- import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Ref } from "effect"; +import { Cause, Effect, Exit, Ref } from "effect"; import { HttpServerResponse } from "effect/unstable/http"; import { OAUTH2_DEFAULT_TIMEOUT_MS, OAUTH2_REFRESH_SKEW_MS, OAuth2Error, + PREVIEWABLE_BODY_FIELDS, + PREVIEWABLE_WITHIN_ERROR_FIELDS, buildAuthorizationUrl, providerAuthorizeExtras, createPkceCodeChallenge, @@ -650,6 +652,266 @@ describe("exchangeAuthorizationCode", () => { ), ); + // A malformed HTTP 200 is the worst case in this module. The OAuth library + // rejects it by attaching the PARSED BODY — the whole token response — and + // these are ordinary provider quirks, not exotic inputs. Each of these bodies + // was confirmed to leak both tokens before the `cause` field was removed. + for (const [label, quirk] of [ + ["expires_in is null", { expires_in: null }], + ["scope is an array", { scope: ["read"] }], + ["token_type is not a string", { token_type: 7 }], + ] as const) { + it.effect(`keeps tokens out of the failure when ${label}`, () => + withTokenEndpoint( + () => + json(200, { + access_token: "AT-CANARY-must-not-escape", + refresh_token: "RT-CANARY-must-not-escape", + token_type: "Bearer", + ...quirk, + }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + // Both renderings, because different sinks use different ones: + // structured capture serialises, console output pretty-prints. + for (const rendering of [JSON.stringify(exit.cause), Cause.pretty(exit.cause)]) { + expect(rendering).not.toContain("AT-CANARY-must-not-escape"); + expect(rendering).not.toContain("RT-CANARY-must-not-escape"); + } + }), + ), + ); + } + + it.effect("redacts a credential echoed back under a field name nobody predicted", () => + withTokenEndpoint( + // The failure the old name-based scrub could not see. It hid four known + // field names, so a server that echoes a submitted secret — or returns its + // token — under ANY other key walked straight through into the message, + // and that message is persisted into connection health and shown to the + // caller. An unknown field is exactly the case that has to fail closed. + // No `error` field: a NON-conform body, which is the shape that actually + // reaches the body preview. A conform error response is summarised from + // its typed fields instead and never renders the body at all. + () => json(400, { oops: "AT-CANARY-must-not-escape" }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const failure = JSON.stringify(exit.cause); + expect(failure).not.toContain("AT-CANARY-must-not-escape"); + // Structure survives, so an operator still sees WHAT the server sent. + expect(failure).toContain("oops"); + expect(failure).toContain("[redacted]"); + }), + ), + ); + + it.effect("keeps an error array readable — the shape real providers answer with", () => + withTokenEndpoint( + // Datadog answers a refused refresh this way. The preview has to stay + // readable through the array, or the one body that most needs explaining + // previews as nothing. + () => json(400, { errors: ["invalid_grant - Invalid or expired refresh token"] }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + expect(JSON.stringify(exit.cause)).toContain("Invalid or expired refresh token"); + }), + ), + ); + + it.effect( + "redacts an authorization code at the top level, but not an error envelope's code", + () => + withTokenEndpoint( + // `code` means two different things depending on where it sits: inside an + // error envelope it names the failure, at the top level it is the RFC 6749 + // authorization code — credential material. Name alone cannot tell them + // apart, so nesting has to. + () => + json(400, { + code: "AUTHZ-CODE-CANARY", + error: { code: "invalid_client_id", message: "Invalid client_id" }, + }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const failure = JSON.stringify(exit.cause); + expect(failure).not.toContain("AUTHZ-CODE-CANARY"); + expect(failure).toContain("invalid_client_id"); + expect(failure).toContain("Invalid client_id"); + }), + ), + ); + + it.effect("applies the allowlist to a form-encoded body too", () => + withTokenEndpoint( + // The other shape a token endpoint answers in. It used to take a + // name-based scrub that could not match a field nobody had enumerated. + () => + HttpServerResponse.text("session_token=FORM-CANARY-must-not-escape&error=invalid_request", { + status: 400, + headers: { "content-type": "application/x-www-form-urlencoded" }, + }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const failure = JSON.stringify(exit.cause); + expect(failure).not.toContain("FORM-CANARY-must-not-escape"); + expect(failure).toContain("session_token"); + expect(failure).toContain("invalid_request"); + }), + ), + ); + + it.effect("survives a pathologically nested body instead of dying", () => + withTokenEndpoint( + () => { + let nested: unknown = "AT-CANARY-must-not-escape"; + for (let i = 0; i < 10_000; i++) nested = { nest: nested }; + return json(400, nested); + }, + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + // A DEFECT here would bypass the caller's error mapping entirely, so + // the connection would never be marked as needing re-auth. The walk + // must stop, not blow the stack. + const rendered = JSON.stringify(exit.cause); + expect(rendered).not.toContain("AT-CANARY-must-not-escape"); + expect(rendered).toContain("OAuth2Error"); + expect(rendered).not.toContain("Maximum call stack"); + }), + ), + ); + + it("previews only the RFC 6749 error fields — widening this list is a security change", () => { + // Nothing else pins the allowlist's CONTENTS, so adding a field to it would + // otherwise be invisible: `token_type` and `scope` sit right beside the + // tokens in a real response, and a future `access_token` entry would defeat + // the whole redactor while every existing test stayed green. + for (const field of ["token_type", "scope", "access_token", "refresh_token", "id_token"]) { + expect(PREVIEWABLE_BODY_FIELDS.has(field)).toBe(false); + expect(PREVIEWABLE_WITHIN_ERROR_FIELDS.has(field)).toBe(false); + } + expect([...PREVIEWABLE_BODY_FIELDS].sort()).toEqual([ + "error", + "error_description", + "error_uri", + "errors", + ]); + expect([...PREVIEWABLE_WITHIN_ERROR_FIELDS].sort()).toEqual(["code", "detail", "message"]); + }); + + it.effect("matches allowlisted field names case-insensitively", () => + withTokenEndpoint( + () => json(400, { Error_Description: "Code expired upstream", Oops: "MIXED-CANARY" }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const failure = JSON.stringify(exit.cause); + expect(failure).toContain("Code expired upstream"); + expect(failure).not.toContain("MIXED-CANARY"); + }), + ), + ); + + it.effect("reports the token endpoint by hostname, never by path", () => + withTokenEndpoint( + () => HttpServerResponse.text("nope", { status: 404 }), + ({ tokenUrl }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + exchangeAuthorizationCode({ + tokenUrl, + clientId: "cid", + redirectUrl: "https://cb", + codeVerifier: "v", + code: "c", + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const failure = JSON.stringify(exit.cause); + // Persisted into connection health, so a tenant id in the path would + // outlive the request. The host is enough to identify the server. + expect(failure).toContain(new URL(tokenUrl).hostname); + expect(failure).not.toContain(`${new URL(tokenUrl).origin}/token`); + }), + ), + ); + it.effect("preserves provider error codes while redacting token endpoint secrets", () => withTokenEndpoint( () => @@ -981,12 +1243,21 @@ describe("OAuth2Error tagging", () => { }), ); - it("OAuth2Error is constructable directly with message and cause", () => { - const err = new OAuth2Error({ message: "test", cause: { foo: 1 } }); + it("OAuth2Error is constructable directly with message and code", () => { + const err = new OAuth2Error({ message: "test", error: "invalid_grant" }); expect(err).toMatchObject({ _tag: "OAuth2Error", message: "test", - cause: { foo: 1 }, + error: "invalid_grant", }); }); + + it("carries no cause, so nothing unsanitised can ride along", () => { + // The type forbids it; this pins the RUNTIME shape too. The leak this + // prevents came from an object attached at construction and rendered far + // away, so a re-added `cause` field would compile and silently reopen it. + const err = new OAuth2Error({ message: "test", error: "invalid_grant" }); + expect(Object.hasOwn(err, "cause")).toBe(false); + expect(JSON.stringify(err)).not.toContain("cause"); + }); }); diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index 2961debc5..6f7b45568 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -23,6 +23,29 @@ import * as oauth from "oauth4webapi"; // Errors // --------------------------------------------------------------------------- +/** A token-endpoint failure, carrying only values this module has inspected. + * + * There is deliberately NO `cause`. The OAuth library rejects a malformed HTTP + * 200 by attaching the PARSED BODY, and that body is the whole token response — + * a live access and refresh token. It is not an exotic case: an `expires_in` of + * null, an array-valued `scope`, or a non-string `token_type` all trigger it, + * and those are ordinary provider quirks. A cause decides nothing — no code + * reads one — it only rides along to be rendered, and it renders everywhere: + * `Cause.pretty`, `JSON.stringify`, the tool-dispatch error log, and from there + * the error-capture sink and an OTLP collector. + * + * Everything genuinely diagnostic is lifted out before that can happen: the + * `error_description` and a redacted HTTP summary into `message`, and the RFC + * 6749 §5.2 code into `error`. + * + * KNOWN COST, accepted deliberately. Transport failures lose detail: a refused + * connection and a DNS miss both arrive as "fetch failed", because the runtime + * reports them only through the rejection chain this drops. Lifting the + * innermost error code back out was tried and removed — this runtime's fetch + * rejection carries no `code` at any depth, so the code was inert, and lifting + * the innermost MESSAGE instead would put unbounded prose back on a path that + * is persisted onto the connection. Restoring that detail safely needs a + * transport-level signal this module does not currently receive. */ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ readonly message: string; /** @@ -32,7 +55,6 @@ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ * the AS no longer honours → re-auth required) from transient ones. */ readonly error?: string; - readonly cause?: unknown; }> {} // --------------------------------------------------------------------------- @@ -286,8 +308,110 @@ const responseFromOAuthErrorCause = (cause: unknown): Response | undefined => { return undefined; }; -const redactTokenEndpointBody = (body: string): string => - body +/** Field names whose STRING value is safe to show in an error preview. + * + * RFC 6749 §5.2's own error fields, plus the container names real providers + * wrap them in (`error` as an object with `code`/`message`, Datadog's `errors` + * array). Everything here describes a failure; none of it is credential + * material. */ +/** RFC 6749 §5.2's own error fields — safe to show wherever they appear. */ +export const PREVIEWABLE_BODY_FIELDS = new Set([ + "error", + "errors", + "error_description", + "error_uri", +]); + +/** Safe only INSIDE one of the fields above. + * + * Providers wrap the real error in an envelope — `{"error":{"code":…, + * "message":…}}` — so these have to be readable there. They must NOT be + * readable at the top level: `code` in particular is the RFC 6749 + * authorization code, which is credential material, and the form-encoded scrub + * in this same file has always redacted `code=` for exactly that reason. */ +export const PREVIEWABLE_WITHIN_ERROR_FIELDS = new Set(["code", "message", "detail"]); + +/** Redact a token-endpoint body for display. + * + * ALLOWLIST, deliberately. This used to name the four fields to hide, which + * silently trusted every field it had not thought of: a provider that returns + * its token under any other key — or that echoes a submitted secret back + * inside an arbitrary error field — walked straight through. This preview is + * not just a log line; it reaches persisted connection health and the caller, + * so an unknown field is exactly the case that must fail closed. + * + * Structure is preserved rather than dropped: every key stays visible and only + * non-allowlisted STRING values become `[redacted]`, so an operator can still + * see the shape of what the server sent. Non-strings are left alone — a number + * or boolean cannot carry a token. */ +/** Deepest body this walker will descend. A token endpoint's error body is a + * handful of levels; anything past this is not something an operator was going + * to read anyway. The bound exists because the walk is recursive and this runs + * on a failure path: without it a pathologically nested body turns a leak into + * an uncontained stack overflow, which is a worse bug than the one being fixed. */ +const MAX_PREVIEW_DEPTH = 32; + +const isPreviewableKey = (key: string, insideError: boolean): boolean => { + const name = key.toLowerCase(); + return ( + PREVIEWABLE_BODY_FIELDS.has(name) || (insideError && PREVIEWABLE_WITHIN_ERROR_FIELDS.has(name)) + ); +}; + +const redactJsonValues = (value: unknown, keyIsPreviewable = false, depth = 0): unknown => { + if (depth > MAX_PREVIEW_DEPTH) return "[redacted]"; + if (typeof value === "string") return keyIsPreviewable ? value : "[redacted]"; + if (Array.isArray(value)) { + return value.map((item) => redactJsonValues(item, keyIsPreviewable, depth + 1)); + } + if (typeof value === "object" && value !== null) { + return Object.fromEntries( + Object.entries(value).map(([key, item]) => [ + key, + redactJsonValues(item, isPreviewableKey(key, keyIsPreviewable), depth + 1), + ]), + ); + } + return value; +}; + +const redactTokenEndpointBody = (body: string): string => { + // A JSON body is the token-endpoint shape, so it gets the structural + // allowlist above. Anything else (an HTML error page, a plain-text 404) is + // not a token response; keep the legacy name-based scrub so those stay + // readable, which is the only thing that made them useful to begin with. + const json: unknown = (() => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: probing an untrusted upstream body for display; a parse failure just means "not a JSON token response" + try { + // oxlint-disable-next-line executor/no-json-parse -- boundary: same untrusted-body probe; the value is only re-serialised for a redacted preview, never decoded into domain types + return JSON.parse(body) as unknown; + } catch { + return undefined; + } + })(); + // Anything that parsed as JSON goes through the walker, not just an object. + // A body that is a bare JSON string is still a body the server chose to send, + // and gating on `object` let exactly that case fall through to the name-based + // scrub below — which cannot match a value that has no field name. + if (json !== undefined) { + return JSON.stringify(redactJsonValues(json)); + } + // A form-encoded body is the OTHER shape a token endpoint answers in, and it + // gets the same allowlist. It used to fall through to a name-based scrub, + // which meant a server returning its token as `session_token=…` — any name + // the scrub had not enumerated — rendered it verbatim into a message that is + // persisted onto the connection. + if (isFormEncoded(body)) { + const params = new URLSearchParams(body); + return [...params] + .map(([key, value]) => `${key}=${isPreviewableKey(key, false) ? value : "[redacted]"}`) + .join("&"); + } + // Neither shape: an HTML error page or a plain-text status line. There is no + // field structure to reason about, so keep it readable — that legibility is + // the only reason the preview earns its place for these responses — but still + // scrub the named credentials, since such a page can echo a submitted one. + return body .replaceAll( /("(?:access_token|refresh_token|id_token|client_secret)"\s*:\s*")[^"]*(")/gi, "$1[redacted]$2", @@ -296,12 +420,22 @@ const redactTokenEndpointBody = (body: string): string => /((?:access_token|refresh_token|id_token|client_secret|code)=)[^&\s]*/gi, "$1[redacted]", ); +}; + +/** `a=b&c=d` — no whitespace, at least one `key=`. Deliberately strict: a prose + * body like `route not found` must NOT be mistaken for one field. */ +const isFormEncoded = (body: string): boolean => + /^[^=&\s]+=[^&\s]*(?:&[^=&\s]+=[^&\s]*)*$/.test(body); const tokenEndpointHttpSummary = async (response: Response): Promise => { const status = `HTTP ${response.status}${response.statusText ? ` ${response.statusText}` : ""}`; const contentType = response.headers.get("content-type"); - const url = response.url ? ` from ${response.url}` : ""; - const parts = [`${status}${url}`]; + // Hostname, never the full URL — the same discipline the token-request span + // already applies, and for the same reason: some providers carry tenant ids + // in the path. This summary is persisted into connection health and shown to + // callers, so it outlives the request by far longer than a log line does. + const host = response.url ? hostnameForTelemetry(response.url) : ""; + const parts = [`${status}${host ? ` from ${host}` : ""}`]; if (contentType) parts.push(`content-type ${contentType}`); const preview = await bodyPreviewFromResponse(response); if (preview) parts.push(`body: ${preview}`); @@ -387,12 +521,10 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { return new OAuth2Error({ message: `OAuth token exchange failed: ${description ?? code ?? "unknown error"}`, error: code, - cause, }); } return new OAuth2Error({ message: "OAuth token exchange failed", - cause, }); }; @@ -420,7 +552,6 @@ const toOAuth2ErrorWithHttpSummary = (cause: unknown): Effect.Effect