diff --git a/.changeset/oauth-token-endpoint-error-leak.md b/.changeset/oauth-token-endpoint-error-leak.md new file mode 100644 index 000000000..fe4055647 --- /dev/null +++ b/.changeset/oauth-token-endpoint-error-leak.md @@ -0,0 +1,35 @@ +--- +"executor": patch +--- + +**Keep token material out of OAuth token-endpoint error messages** + +A token-endpoint failure renders a preview of the upstream body into its +message, and that message is persisted onto connection health, returned to the +caller, and carried into telemetry. On a malformed HTTP 200 the body being +previewed is a _successful_ token response, so an access token and a refresh +token could be rendered into it. + +The preview is now built from an allowlist of fields that are safe to show +(`error`, `errors`, `error_description`, `error_uri`, plus `code`, `message`, +and `detail` nested inside them) instead of a denylist of fields to hide. A +field nobody anticipated is omitted by default rather than printed by default. +Keys stay visible and only non-allowlisted string values are replaced, so an +operator can still read the shape of what the server sent. `code` is readable +only when nested, because at the top level of a token response it is the RFC +6749 authorization code. + +Form-encoded bodies take the same allowlist, the walk over a body is +depth-bounded, and the failure summary records the token endpoint's hostname +rather than its full URL, which can carry identifiers in its path. + +On that same malformed-200 path the failure no longer keeps the underlying +rejection as its `cause`. That rejection carries the parsed token response, so +keeping it put the raw tokens back into anything that renders the whole failure +rather than only its message. Everything the path needs from the body — the +status, the error code, the redacted preview — is read before the failure is +built. A transport failure still keeps its cause, which is what tells a DNS miss +apart from a refused connection. + +No public API changes. The dead-grant classification added for HTTP 200 refresh +refusals is unaffected: it reads the HTTP status, not the rendered preview. diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index f3e7dfe75..ab5dcb903 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, @@ -21,6 +23,7 @@ import { exchangeClientCredentials, idTokenIdentityLabel, isPermanentTokenRejection, + isUnusableSuccessTokenResponse, refreshAccessToken, shouldRefreshToken, } from "./oauth-helpers"; @@ -810,6 +813,290 @@ describe("exchangeAuthorizationCode", () => { ), ); + // A malformed HTTP 200 is the worst case in this module. The OAuth library + // rejects it by handing back the PARSED BODY — the whole token response — and + // these are ordinary provider quirks, not exotic inputs. That body is the one + // the failure MESSAGE is built from, and the message is what is persisted onto + // connection health, returned to the caller, and carried into telemetry. The + // allowlist is what keeps the tokens out of it. + // + // Asserting on the message alone would not be enough. What a log line, a + // Sentry event, or a crashing host actually prints is the WHOLE failure — + // `Cause.pretty`, or a `JSON.stringify` of the cause — so a retained + // rejection would carry the raw tokens straight past a clean message. These + // assert the full rendering for that reason. + // + // The classifier has to keep working on exactly these inputs: a 2xx that + // carried no usable token is a DEAD GRANT, and mis-reading it as transient is + // what makes a connection retry forever instead of asking for re-auth. It + // reads `status`, which this module lifts out of the rejection itself — so + // redacting the rendering costs the classifier nothing. + 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 whole rendered 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; + const pretty = Cause.pretty(exit.cause); + const serialized = JSON.stringify(exit.cause); + for (const rendered of [pretty, serialized]) { + expect(rendered).not.toContain("AT-CANARY-must-not-escape"); + expect(rendered).not.toContain("RT-CANARY-must-not-escape"); + } + // Redacted, not dropped: the operator still sees which fields the + // server sent, which is the whole point of previewing at all. + expect(pretty).toContain("access_token"); + expect(pretty).toContain("[redacted]"); + // ...and the dead-grant verdict survives the redaction untouched. + const failure = Cause.squash(exit.cause) as OAuth2Error; + expect(failure).toBeInstanceOf(OAuth2Error); + expect(failure.status).toBe(200); + expect(isUnusableSuccessTokenResponse(failure)).toBe(true); + expect(isPermanentTokenRejection(failure)).toBe(true); + }), + ), + ); + } + + 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( () => diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index 207914702..53a4ce688 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -42,6 +42,22 @@ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ * because the majority of real refusals carry no RFC 6749 §5.2 code. */ readonly status?: number; + /** + * The library rejection this failure was built from, kept so a transport + * failure stays diagnosable — the chain is what separates a DNS miss from a + * refused connection, and neither is expressible in `status` or `error`. + * + * It is DROPPED on the malformed-HTTP-200 path. There the rejection carries + * the token response oauth4webapi already parsed, so retaining it would hand + * the raw access and refresh tokens to anything that renders the whole + * failure (`Cause.pretty`, `JSON.stringify`) — around the allowlist that + * keeps them out of `message`. Everything that path needs from the body is + * already lifted onto `status`, `error`, and the redacted preview, and + * nothing downstream reads this field, so nothing is lost by omitting it. + * + * Treat whatever is here as INTERNAL diagnostic input, never display + * material: anything RENDERED goes through `redactedBodyPreview` first. + */ readonly cause?: unknown; }> {} @@ -359,8 +375,112 @@ const parsedBodyFromOAuthErrorCause = (cause: unknown): unknown => { * though the Response itself never made it into the error. */ const PARSED_BODY_CAUSE_STATUS = 200; -const redactTokenEndpointBody = (body: string): string => - body +/** RFC 6749 §5.2's own error fields — the names whose STRING value is safe to + * show wherever they appear. Everything here describes a failure; none of it + * is credential material. */ +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"]); + +/** 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; +}; + +/** 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. That is not + * hypothetical on the malformed-200 path, where the body the library rejected + * IS a successful token response. 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 — and so the dead-grant classifier's + * own evidence stays legible in the message it produced it from. Non-strings + * are left alone; a number or boolean cannot carry a token. + * + * This governs only what is RENDERED. The classifier reads the parsed body + * through `cause`, unredacted, and is unaffected. */ +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", @@ -369,12 +489,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}`); @@ -565,11 +695,18 @@ const toOAuth2ErrorWithHttpSummary = ( const preview = redactedBodyPreview(safeStringify(parsedBody)); const summary = [`HTTP ${PARSED_BODY_CAUSE_STATUS}`, ...(preview ? [`body: ${preview}`] : [])]; return Effect.succeed( + // NO `cause` here, deliberately. The rejection this branch was built from + // carries the whole parsed token response — access and refresh tokens in + // the clear — and every read of that body has ALREADY happened above: + // `status`, `error`, and the redacted preview are all lifted out here. + // Keeping the rejection would only put the raw tokens back into whatever + // renders the full failure (`Cause.pretty`, `JSON.stringify`), which is + // exactly the leak the message allowlist exists to prevent. The other + // branches keep their cause because theirs is diagnostic, not a body. new OAuth2Error({ message: `${options?.fallbackMessage ?? base.message} (${summary.join("; ")})`, error: base.error ?? envelope?.error, status: PARSED_BODY_CAUSE_STATUS, - cause, }), ); }