Keep token material out of OAuth token-endpoint errors - #1567
Open
GeiserX wants to merge 5 commits into
Open
Conversation
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.
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.
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.
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.
This was referenced Aug 12, 2026
Author
|
Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there. This PR stands alone and doesn't depend on any of the others. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
When a token endpoint replied in a way the OAuth library could not parse, the resulting error carried the parsed response body as its
cause. On a malformed200that body is a successful token response — so an access token, and sometimes a refresh token, travelled inside an error object into whatever logged it.This drops the raw
causeand rebuilds the body preview from an allowlist of fields that are safe to show, instead of a denylist of fields to hide.What happens today
OAuth2Erroris constructed with the parsed body attached. Any handler that logs the error, serialises it, or reports it to an error tracker writes token material to that sink. Nothing in the type says the payload is sensitive, so there is no reason for a caller to suspect it.The preview that was redacted used a denylist — it removed the field names we thought of. A token under any other name went straight through.
What this changes
causeis no longer attached. The library's own parsed body never leaves the boundary.The body preview is an allowlist:
error,errors,error_description,error_uri, pluscode/message/detailnested inside those. A field nobody anticipated is now omitted by default rather than printed by default.codeis deliberately allowed only when nested — at the top level of a token response,codeis the RFC 6749 authorization code.Form-encoded bodies take the same allowlist. They previously had a separate, weaker path.
Previews are depth-bounded, so a deeply nested or self-referential body cannot exhaust the stack.
The failure summary records the token endpoint's hostname, not its full URL — paths routinely carry tenant and account identifiers.
Why the allowlist direction matters
A denylist has to predict every name an authorization server might use for a secret; it fails silently and in the unsafe direction when it guesses wrong. An allowlist fails in the safe direction: an unfamiliar field is dropped, and the cost is a slightly less descriptive error message.
Tests
The regression tests use the inputs that actually trigger the malformed-
200path —expires_in: null, an arrayscope, a non-stringtoken_type— rather than an input that merely looks similar. An earlier version of this test used an unknowntoken_type, which produces a different, harmless error shape and would have passed against the unfixed code.Each test was mutation-checked: the fix was reverted and the test confirmed to fail.
Scope
Independent of #1564 — it touches only
oauth-helpers.tsand applies to the existing host-side exchange. The two can merge in either order.