fix(oauth): read the issuer's real AS metadata instead of guessing it - #37
fix(oauth): read the issuer's real AS metadata instead of guessing it#37marselsel wants to merge 2 commits into
Conversation
buildOAuthMetadata fabricated every field of the authorization-server document:
endpoints derived from the WorkOS URL layout ({issuer}/oauth2/*) plus hardcoded
grant_types_supported, code_challenge_methods_supported and scopes_supported.
For any IdP not sharing that layout the result was wrong — Entra's authorize
endpoint is /oauth2/v2.0/authorize, not the /v2.0/oauth2/authorize we derived —
and the guesses were presented to clients as fact. Same root cause as the
registration_endpoint bug fixed in 0.1.10.
The server now fetches the issuer's own metadata at startup (RFC 8414, falling
back to OIDC discovery; three URL layouts tried in order) and advertises what it
says. Precedence per field: explicit env override > discovered > derived default.
Safety properties, each with a test:
- A document whose `issuer` does not match is DISCARDED, not merged. This
document tells clients where to send users to authenticate, so honouring one
that speaks for another issuer would be a redirect-hijack primitive.
- `issuer` is never taken from the document; it must match the iss claim exactly.
- Any failure (unreachable, timeout, non-JSON, 404) falls back to the previous
derived defaults and logs a warning. Discovery can improve the document, never
prevent startup. Fetch is bounded at 5s.
- Explicit overrides still win, including OAUTH_REGISTRATION_ENDPOINT=none, so
discovery cannot silently re-advertise DCR an operator turned off.
Verified against the live WorkOS issuer: discovery=ok, and the only fields that
change are two that were previously untrue — grant_types_supported gains
device_code and scopes_supported gains offline_access, both of which WorkOS
actually supports.
Disable with OAUTH_DISCOVERY=false. Release 0.1.11.
Addresses five findings from an adversarial review of the initial discovery
implementation, which only affected the ADVERTISED metadata:
1. Registration fallback regressed the 0.1.10 fix. When discovery succeeded but
the issuer omitted registration_endpoint (WorkOS with DCR off), the code fell
back to the derived {issuer}/oauth2/register guess and re-advertised the broken
endpoint. A successful discovery is now authoritative: absent means omit; the
derived guess is used only when discovery did not run or failed.
2. Discovery was half-wired: advertised != verified. The advertised jwks_uri came
from the discovered doc, but signature verification and the userinfo email
fallback still used the derived values. On a non-WorkOS IdP we would advertise
one JWKS and verify against another. Endpoints are now resolved ONCE
(resolveOAuthEndpoints: explicit > discovered > derived) and that single set
drives both the advertised document and createAccessTokenVerifier.
3. Startup delay was up to 3x5s, not 5s. Discovery now shares one total time
budget across all candidate URLs, and the candidate list is de-duplicated (a
path-less issuer previously produced the same URL twice).
4. Discovered URLs were unvalidated. Every discovered endpoint is now checked
(https, or http only on loopback) before being advertised or used; an http
downgrade or malformed value is ignored in favour of the derived default. This
is load-bearing now that jwks_uri feeds verification.
5. Issuer match is trailing-slash-insensitive (a doc whose issuer differs only by
a trailing slash is accepted; a genuinely different issuer is still rejected).
177 tests (up from 167), each finding covered. Verified against the live WorkOS
issuer: discovery=ok, registration correctly omitted even without =none, and the
jwks_uri advertised equals the one used for verification.
|
Closing this without merging — a deliberate decision, not abandonment. Recording the reasoning here since the analysis is worth preserving. WhyThe problem this PR set out to solve is already fixed and deployed. The concrete production bug — advertising a This PR aimed to retire the class of "guessed metadata" bugs by fetching the issuer's real document at startup. Two adversarial review rounds later, that goal looks more expensive than it's worth:
When each fix for a review finding creates a new security finding, the design is fighting itself. "Fetch a remote document at boot and trust parts of it inside the auth path" is intrinsically hard to get right, and the benefit for the actual deployment (WorkOS, where discovered endpoints already equal the derived ones) is two cosmetic advertisement fields. If a concrete non-WorkOS need appearsThe right approach then is not to resurrect this implementation, but to wrap the discovery helpers the MCP SDK already ships ( The branch |
1 participant
Problem
buildOAuthMetadatainvented the entire authorization-server document — endpoints derived from the WorkOS URL layout, plus hardcodedgrant_types_supported,code_challenge_methods_supportedandscopes_supported. Those guesses were served to clients as fact.For WorkOS they happen to be right, which is why it never bit us. For Entra the authorize endpoint is
/oauth2/v2.0/authorize, not the/v2.0/oauth2/authorizewe derive. This is the root cause behind theregistration_endpointbug fixed in #36 — that PR patched the one instance that was actively wrong; this one retires the class.Solution
Fetch the issuer's own metadata at startup (RFC 8414, falling back to OIDC discovery — three URL layouts tried in order, since providers disagree). Precedence per field: explicit env override > discovered > derived default.
Safety
Each has a test:
issuerdoesn't match is discarded, not merged. This document tells clients where to send users to authenticate; honouring one that speaks for a different issuer is a redirect-hijack primitive.issueris never taken from the document — it must match theissclaim byte-for-byte.OAUTH_REGISTRATION_ENDPOINT=none, so discovery can't silently re-advertise DCR an operator disabled.Verified against the live issuer
discovery=ok, and exactly two fields change — both previously untrue:grant_types_supportedauthorization_code, refresh_tokendevice_codescopes_supportedopenid, email, profileoffline_accessEndpoints,
issuer,jwks_uri,response_types_supportedandcode_challenge_methods_supportedunchanged.Tests
167 passing (up from 155). Covers the three URL layouts incl. a path-bearing Entra-style issuer, the issuer-mismatch rejection, the never-throws fallback, and each precedence rule.
Disable with
OAUTH_DISCOVERY=false.