From 514f69188058112a8eee5c4ef86d165164e68c7d Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:36:33 +0200 Subject: [PATCH 1/7] Keep token material out of token-endpoint failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent channels carried live credentials out of a failed OAuth token exchange. Both are closed at the source, in the one module that builds these errors, so every flow that uses it — authorization_code, client_credentials and refresh — is covered at once. The error cause. The OAuth library rejects a malformed HTTP 200 by attaching the PARSED BODY, and that body is the whole token response. This is not an exotic case: an `expires_in` of null, an array-valued `scope`, or a non-string `token_type` each trigger it, and those are ordinary provider quirks. That cause then rides into `Cause.pretty`, `JSON.stringify`, the tool-dispatch error log, and from there to an error-capture sink and an OTLP collector. `OAuth2Error` now has no `cause` field at all. Nothing read one — it decided nothing and only ever rode along to be rendered — while everything genuinely diagnostic is already lifted out first: `error_description` and a redacted HTTP summary into `message`, the RFC 6749 §5.2 code into `error`. The body preview. Its redactor named the four fields to hide, which silently trusted every field it had not thought of: a server returning its token under any other key, or echoing a submitted secret back inside an arbitrary error field, walked straight through. It is now an allowlist — every key stays visible and only non-allowlisted string values become `[redacted]` — so the shape of the response is still readable while an unknown field fails closed. This preview is persisted into connection health and shown to callers, so an unknown field is exactly the case that must not be trusted. The same summary also embedded the full response URL. It now reports the hostname, the discipline the token-request span already applies, and for the same reason: some providers carry tenant ids in the path. --- packages/core/sdk/src/oauth-helpers.test.ts | 74 ++++++++++++++++- packages/core/sdk/src/oauth-helpers.ts | 90 +++++++++++++++++++-- 2 files changed, 153 insertions(+), 11 deletions(-) diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index 38471bda7f..fcaefac21d 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -650,6 +650,65 @@ describe("exchangeAuthorizationCode", () => { ), ); + 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. + // Structure survives, so an operator still sees WHAT the server sent. + expect(failure).toContain("oops"); + expect(failure).toContain("[redacted]"); + }), + ), + ); + + 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 +1040,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 2961debc50..8b5de50895 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -23,6 +23,20 @@ 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`. */ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ readonly message: string; /** @@ -32,7 +46,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 +299,67 @@ 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. */ +const PREVIEWABLE_BODY_FIELDS = new Set([ + "error", + "errors", + "error_description", + "error_uri", + "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. */ +const redactJsonValues = (value: unknown, keyIsPreviewable = false): unknown => { + if (typeof value === "string") return keyIsPreviewable ? value : "[redacted]"; + if (Array.isArray(value)) return value.map((item) => redactJsonValues(item, keyIsPreviewable)); + if (typeof value === "object" && value !== null) { + return Object.fromEntries( + Object.entries(value).map(([key, item]) => [ + key, + redactJsonValues(item, PREVIEWABLE_BODY_FIELDS.has(key.toLowerCase())), + ]), + ); + } + 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; + } + })(); + if (typeof json === "object" && json !== null) { + return JSON.stringify(redactJsonValues(json)); + } + return body .replaceAll( /("(?:access_token|refresh_token|id_token|client_secret)"\s*:\s*")[^"]*(")/gi, "$1[redacted]$2", @@ -296,12 +368,17 @@ const redactTokenEndpointBody = (body: string): string => /((?:access_token|refresh_token|id_token|client_secret|code)=)[^&\s]*/gi, "$1[redacted]", ); +}; 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 +464,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 +495,6 @@ const toOAuth2ErrorWithHttpSummary = (cause: unknown): Effect.Effect Date: Wed, 12 Aug 2026 15:45:58 +0200 Subject: [PATCH 2/7] Pin the malformed-200 leak with the inputs that actually trigger it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first canary used a non-conform 400, which never reaches the code path that attaches the parsed body — so it passed whether or not the cause was attached. These use the three shapes confirmed to leak both tokens: a null expires_in, an array-valued scope, and a non-string token_type. --- packages/core/sdk/src/oauth-helpers.test.ts | 44 ++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index fcaefac21d..d1d62294c2 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -6,7 +6,7 @@ // --------------------------------------------------------------------------- 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 { @@ -650,6 +650,48 @@ 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 From 01930b8d7b54f63addd77a970bfe13a9f5dac5c6 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:17:22 +0200 Subject: [PATCH 3/7] Close the remaining preview leaks a fresh review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the previous commit's redactor, two of them live leaks. Each was confirmed by running before it was fixed. A pathologically nested body overflowed the stack. Because the walk runs inside `Effect.promise`, that surfaced as a DEFECT rather than a failure — which bypasses the caller's error mapping entirely, so an `invalid_grant` would never be classified and the connection would never be marked as needing re-auth. It turned a leak into a worse bug. The walk now stops at a bounded depth. Only JSON took the allowlist. A form-encoded body — the other shape a token endpoint answers in — still took the old name-based scrub, so a server returning `session_token=…`, or any name nobody had enumerated, rendered it verbatim into a message that is persisted onto the connection. Form bodies now take the same allowlist, and a body that is valid JSON but not an object no longer falls through to a scrub that cannot match a value with no field name. `code` was previewable anywhere, while the scrub four lines away had always redacted `code=` because an OAuth authorization code IS credential material. The name alone cannot distinguish the two meanings, so nesting now does: the RFC 6749 error fields are readable wherever they appear, and `code`/`message`/`detail` only inside one of them. Dropping the cause also took the whole rejection chain with it, and a network failure is the most common way this call fails: connection-refused and DNS-not-found had collapsed to the same three words. The innermost machine readable code is lifted back into the message — `ECONNREFUSED`, `ENOTFOUND`, and nothing that is prose, a URL, or a body. --- packages/core/sdk/src/oauth-helpers.test.ts | 143 +++++++++++++++++++- packages/core/sdk/src/oauth-helpers.ts | 96 +++++++++++-- 2 files changed, 224 insertions(+), 15 deletions(-) diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index d1d62294c2..88702f2760 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -719,13 +719,154 @@ describe("exchangeAuthorizationCode", () => { 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. - // 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.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 }), diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index 8b5de50895..6cce7db0a4 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -305,15 +305,17 @@ const responseFromOAuthErrorCause = (cause: unknown): Response | undefined => { * 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. */ -const PREVIEWABLE_BODY_FIELDS = new Set([ - "error", - "errors", - "error_description", - "error_uri", - "code", - "message", - "detail", -]); +/** RFC 6749 §5.2's own error fields — safe to show wherever they appear. */ +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. */ +const PREVIEWABLE_WITHIN_ERROR_FIELDS = new Set(["code", "message", "detail"]); /** Redact a token-endpoint body for display. * @@ -328,14 +330,31 @@ const PREVIEWABLE_BODY_FIELDS = new Set([ * 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. */ -const redactJsonValues = (value: unknown, keyIsPreviewable = false): unknown => { +/** 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)); + 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, PREVIEWABLE_BODY_FIELDS.has(key.toLowerCase())), + redactJsonValues(item, isPreviewableKey(key, keyIsPreviewable), depth + 1), ]), ); } @@ -356,9 +375,28 @@ const redactTokenEndpointBody = (body: string): string => { return undefined; } })(); - if (typeof json === "object" && json !== null) { + // 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, @@ -370,6 +408,11 @@ const redactTokenEndpointBody = (body: string): string => { ); }; +/** `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"); @@ -461,8 +504,11 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { : typeof c.message === "string" ? c.message : undefined; + const reason = innermostFailureReason(cause); return new OAuth2Error({ - message: `OAuth token exchange failed: ${description ?? code ?? "unknown error"}`, + message: `OAuth token exchange failed: ${description ?? code ?? "unknown error"}${ + reason ? ` (${reason})` : "" + }`, error: code, }); } @@ -471,6 +517,28 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { }); }; +/** The innermost machine-readable reason from a rejection chain — `ECONNREFUSED`, + * `ENOTFOUND`, `OAUTH_INVALID_RESPONSE`. + * + * Dropping the `cause` object closed a leak but took the whole chain with it, + * and a network failure is by far the most common way this call fails. Without + * this, connection-refused and DNS-not-found render as the same three words and + * an operator cannot tell them apart. Only the `code` is lifted — a short + * screaming-snake identifier from the runtime, never a message, a URL, or a + * response body — so the diagnosis comes back without the payload. */ +const innermostFailureReason = (cause: unknown): string | undefined => { + let reason: string | undefined; + let current: unknown = cause; + for (let depth = 0; depth < 8 && typeof current === "object" && current !== null; depth++) { + const code = (current as { readonly code?: unknown }).code; + // Codes are identifiers like ECONNREFUSED; anything longer or containing + // spaces is prose, and prose is where secrets hide. + if (typeof code === "string" && /^[A-Z][A-Z0-9_]{2,39}$/.test(code)) reason = code; + current = (current as { readonly cause?: unknown }).cause; + } + return reason; +}; + const toOAuth2ErrorWithHttpSummary = (cause: unknown): Effect.Effect => { if (isOAuth2Error(cause)) return Effect.succeed(cause); const base = toOAuth2Error(cause); From cb6e8552495aadbabbb1ead8b74a0be684babf1b Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:41:11 +0200 Subject: [PATCH 4/7] Pin the allowlist contents, and drop an inert diagnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allowlist's CONTENTS were unpinned: adding a field to it would have been invisible to every test, and `token_type` and `scope` sit directly beside the tokens in a real response. A test now asserts both lists exactly, so widening them is a deliberate act with a failing test attached. Also removes the attempt to lift a transport error code back into the message. It was written to recover the network diagnosability that dropping the cause lost, but a probe showed this runtime's fetch rejection carries no `code` at any depth — the helper could never fire. Inert code that looks like a safeguard is worse than none, so it is gone and the cost is documented on the error type instead: transport failures genuinely lose detail, and recovering it safely needs a signal this module does not receive. --- packages/core/sdk/src/oauth-helpers.test.ts | 20 +++++++++ packages/core/sdk/src/oauth-helpers.ts | 47 ++++++++------------- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index 88702f2760..08117af092 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -13,6 +13,8 @@ import { OAUTH2_DEFAULT_TIMEOUT_MS, OAUTH2_REFRESH_SKEW_MS, OAuth2Error, + PREVIEWABLE_BODY_FIELDS, + PREVIEWABLE_WITHIN_ERROR_FIELDS, buildAuthorizationUrl, providerAuthorizeExtras, createPkceCodeChallenge, @@ -844,6 +846,24 @@ describe("exchangeAuthorizationCode", () => { ), ); + 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" }), diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index 6cce7db0a4..6f7b455687 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -36,7 +36,16 @@ import * as oauth from "oauth4webapi"; * * 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`. */ + * 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; /** @@ -306,7 +315,12 @@ const responseFromOAuthErrorCause = (cause: unknown): Response | undefined => { * 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. */ -const PREVIEWABLE_BODY_FIELDS = new Set(["error", "errors", "error_description", "error_uri"]); +export const PREVIEWABLE_BODY_FIELDS = new Set([ + "error", + "errors", + "error_description", + "error_uri", +]); /** Safe only INSIDE one of the fields above. * @@ -315,7 +329,7 @@ const PREVIEWABLE_BODY_FIELDS = new Set(["error", "errors", "error_description", * 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. */ -const PREVIEWABLE_WITHIN_ERROR_FIELDS = new Set(["code", "message", "detail"]); +export const PREVIEWABLE_WITHIN_ERROR_FIELDS = new Set(["code", "message", "detail"]); /** Redact a token-endpoint body for display. * @@ -504,11 +518,8 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { : typeof c.message === "string" ? c.message : undefined; - const reason = innermostFailureReason(cause); return new OAuth2Error({ - message: `OAuth token exchange failed: ${description ?? code ?? "unknown error"}${ - reason ? ` (${reason})` : "" - }`, + message: `OAuth token exchange failed: ${description ?? code ?? "unknown error"}`, error: code, }); } @@ -517,28 +528,6 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { }); }; -/** The innermost machine-readable reason from a rejection chain — `ECONNREFUSED`, - * `ENOTFOUND`, `OAUTH_INVALID_RESPONSE`. - * - * Dropping the `cause` object closed a leak but took the whole chain with it, - * and a network failure is by far the most common way this call fails. Without - * this, connection-refused and DNS-not-found render as the same three words and - * an operator cannot tell them apart. Only the `code` is lifted — a short - * screaming-snake identifier from the runtime, never a message, a URL, or a - * response body — so the diagnosis comes back without the payload. */ -const innermostFailureReason = (cause: unknown): string | undefined => { - let reason: string | undefined; - let current: unknown = cause; - for (let depth = 0; depth < 8 && typeof current === "object" && current !== null; depth++) { - const code = (current as { readonly code?: unknown }).code; - // Codes are identifiers like ECONNREFUSED; anything longer or containing - // spaces is prose, and prose is where secrets hide. - if (typeof code === "string" && /^[A-Z][A-Z0-9_]{2,39}$/.test(code)) reason = code; - current = (current as { readonly cause?: unknown }).cause; - } - return reason; -}; - const toOAuth2ErrorWithHttpSummary = (cause: unknown): Effect.Effect => { if (isOAuth2Error(cause)) return Effect.succeed(cause); const base = toOAuth2Error(cause); From 4cf7dfab39a7ff5f6abec0ab8955d54534e7ff59 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:56:33 +0200 Subject: [PATCH 5/7] Add a changeset for the token-endpoint error leak fix --- .changeset/oauth-token-endpoint-error-leak.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/oauth-token-endpoint-error-leak.md diff --git a/.changeset/oauth-token-endpoint-error-leak.md b/.changeset/oauth-token-endpoint-error-leak.md new file mode 100644 index 0000000000..64cada46ee --- /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. From 64fe758a7c83c871613efdda8fccfdf3e4cf92e8 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:49:53 -0700 Subject: [PATCH 6/7] Re-land the token-endpoint allowlist redaction on the dead-grant classifier Scope the malformed-200 canary tests to the rendered failure message, which is what the allowlist governs, and assert the dead-grant verdict survives it. --- .changeset/oauth-token-endpoint-error-leak.md | 2 +- packages/core/sdk/src/oauth-helpers.test.ts | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.changeset/oauth-token-endpoint-error-leak.md b/.changeset/oauth-token-endpoint-error-leak.md index 5118363e62..1b3c6b9b47 100644 --- a/.changeset/oauth-token-endpoint-error-leak.md +++ b/.changeset/oauth-token-endpoint-error-leak.md @@ -7,7 +7,7 @@ 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 +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 diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index 5f2ae53db0..d8d3d71760 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -841,7 +841,7 @@ describe("exchangeAuthorizationCode", () => { }), ({ tokenUrl }) => Effect.gen(function* () { - const error = yield* Effect.flip( + const failure = yield* Effect.flip( exchangeAuthorizationCode({ tokenUrl, clientId: "cid", @@ -850,17 +850,17 @@ describe("exchangeAuthorizationCode", () => { code: "c", }), ); - expect(error).toBeInstanceOf(OAuth2Error); - expect(error.message).not.toContain("AT-CANARY-must-not-escape"); - expect(error.message).not.toContain("RT-CANARY-must-not-escape"); + expect(failure).toBeInstanceOf(OAuth2Error); + expect(failure.message).not.toContain("AT-CANARY-must-not-escape"); + expect(failure.message).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(error.message).toContain("access_token"); - expect(error.message).toContain("[redacted]"); + expect(failure.message).toContain("access_token"); + expect(failure.message).toContain("[redacted]"); // ...and the dead-grant verdict survives the redaction untouched. - expect(error.status).toBe(200); - expect(isUnusableSuccessTokenResponse(error)).toBe(true); - expect(isPermanentTokenRejection(error)).toBe(true); + expect(failure.status).toBe(200); + expect(isUnusableSuccessTokenResponse(failure)).toBe(true); + expect(isPermanentTokenRejection(failure)).toBe(true); }), ), ); From c3aae2c253b195126cf95d84177de0c945657021 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:30:05 -0700 Subject: [PATCH 7/7] Drop the parsed token body from a malformed-200 OAuth failure The allowlist kept tokens out of the message, but the same failure still carried the library rejection as its cause, so Cause.pretty or a serialised cause rendered the raw access and refresh tokens. Nothing read the cause on that path. The canary tests now assert the whole rendered failure. --- .changeset/oauth-token-endpoint-error-leak.md | 8 +++++ packages/core/sdk/src/oauth-helpers.test.ts | 29 +++++++++++++----- packages/core/sdk/src/oauth-helpers.ts | 30 ++++++++++++++----- 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/.changeset/oauth-token-endpoint-error-leak.md b/.changeset/oauth-token-endpoint-error-leak.md index 1b3c6b9b47..fe4055647c 100644 --- a/.changeset/oauth-token-endpoint-error-leak.md +++ b/.changeset/oauth-token-endpoint-error-leak.md @@ -23,5 +23,13 @@ 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 d8d3d71760..ab5dcb9038 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -6,7 +6,7 @@ // --------------------------------------------------------------------------- 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 { @@ -820,6 +820,12 @@ describe("exchangeAuthorizationCode", () => { // 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 @@ -830,7 +836,7 @@ describe("exchangeAuthorizationCode", () => { ["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 message when ${label}`, () => + it.effect(`keeps tokens out of the whole rendered failure when ${label}`, () => withTokenEndpoint( () => json(200, { @@ -841,7 +847,7 @@ describe("exchangeAuthorizationCode", () => { }), ({ tokenUrl }) => Effect.gen(function* () { - const failure = yield* Effect.flip( + const exit = yield* Effect.exit( exchangeAuthorizationCode({ tokenUrl, clientId: "cid", @@ -850,14 +856,21 @@ describe("exchangeAuthorizationCode", () => { code: "c", }), ); - expect(failure).toBeInstanceOf(OAuth2Error); - expect(failure.message).not.toContain("AT-CANARY-must-not-escape"); - expect(failure.message).not.toContain("RT-CANARY-must-not-escape"); + 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(failure.message).toContain("access_token"); - expect(failure.message).toContain("[redacted]"); + 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); diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index 38cbd0d4c2..53a4ce688c 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -43,13 +43,20 @@ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ */ readonly status?: number; /** - * The library rejection this failure was built from. INTERNAL diagnostic - * input, not display material: `parsedBodyFromOAuthErrorCause` reads the body - * oauth4webapi already parsed off a malformed HTTP 200 through it, which is - * how a 2xx that carries a refusal is classified as a dead grant at all, and - * the transport rejection chain (DNS miss vs refused connection) survives - * here too. Anything RENDERED from it goes through `redactedBodyPreview` - * first, whose allowlist is what keeps token material out of the message. + * 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; }> {} @@ -688,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, }), ); }