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/5] 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 38471bda7..fcaefac21 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 2961debc5..8b5de5089 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/5] 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 fcaefac21..d1d62294c 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/5] 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 d1d62294c..88702f276 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 8b5de5089..6cce7db0a 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/5] 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 88702f276..08117af09 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 6cce7db0a..6f7b45568 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/5] 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 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.