From fdea6e632942831aeee3ebf115bae661e65dad52 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:11:11 -0700 Subject: [PATCH 1/6] fix: accept token-only grants and lock authorize params Access-token responses no longer require a refresh token. Token POSTs do not follow redirects or echo response bodies. Authorize extras cannot replace PKCE fields. Hub callbacks always check a present iss, and a resource login can use a pre-registered client or client metadata instead of dynamic registration. --- README.md | 4 +- src/client.test.ts | 122 +++++++++++++++++++++++++++++++++ src/client.ts | 85 +++++++++++++++++++---- src/discovery.test.ts | 49 +++++++++++++ src/discovery.ts | 76 ++++++++++++++++++++ src/hub/credentials.test.ts | 40 +++++++++++ src/hub/credentials.ts | 47 ++++++++----- src/hub/mount.ts | 104 ++++++++++++++++++++-------- src/hub/refresh.ts | 8 ++- src/hub/route-callback.test.ts | 78 +++++++++++++++++++++ src/hub/route-callback.ts | 33 ++++++--- src/index.ts | 2 + src/session.test.ts | 38 +++++++++- src/session.ts | 10 ++- src/tokens.ts | 6 +- 15 files changed, 625 insertions(+), 77 deletions(-) create mode 100644 src/hub/credentials.test.ts create mode 100644 src/hub/route-callback.test.ts diff --git a/README.md b/README.md index 317487b..761d0c8 100644 --- a/README.md +++ b/README.md @@ -96,9 +96,9 @@ The callback server binds only loopback addresses (`127.0.0.0/8` or `::1`) on th | `GET /oauth-logins/:loginId` | Poll for completion; returns the credential id when done. | | `DELETE /oauth-logins/:loginId` | Cancel. | -A resource-URL login discovers the server's authorization server, registers a client and stores its client id, token URL and resource on the credential, so the refresher renews it without a registry entry. It needs the mount's `callbackUrl` and `store` options (below). The PKCE verifier never leaves the hub process. +A resource-URL login discovers the server's authorization server and obtains a client id from a pre-registered `clientId`, a Client ID Metadata Document URL (`clientMetadataUrl`, only when the server advertises it), or dynamic client registration. It stores that client id, the token URL and the resource on the credential, so the refresher renews a credential that has a refresh token without a registry entry. An access token issued with no refresh token is stored without one; the refresher skips that row, and a new login is required when it expires. It needs the mount's `callbackUrl` and `store` options (below). The PKCE verifier never leaves the hub process. -What the callback route relies on: PKCE S256 is required of the server (discovery refuses one that does not advertise it); the `state` is 32 random bytes, single-use and bound to the tenant and principal that started the login; the RFC 8707 `resource` goes on the authorize, exchange and refresh requests; when the server advertises RFC 9207 the redirect's `iss` must match the discovered issuer; the page is served `no-store` / `no-referrer`; and `callbackUrl` must be `https` unless it is loopback (`localhost`, `127.x`, `::1`), which is what local development uses. +What the callback route relies on: PKCE S256 is required of the server (discovery refuses one that does not advertise it); the `state` is 32 random bytes, single-use and bound to the tenant and principal that started the login; the RFC 8707 `resource` goes on the authorize, exchange and refresh requests; the discovered issuer is recorded with that state, a present `iss` must match it, and a missing `iss` is refused only when the server advertises RFC 9207; caller authorize extras cannot replace PKCE, state, or the other protected fields; token requests do not follow redirects and do not put response bodies in errors; the page is served `no-store` / `no-referrer`; and `callbackUrl` must be `https` unless it is loopback (`localhost`, `127.x`, `::1`), which is what local development uses. ## Using with Interchange diff --git a/src/client.test.ts b/src/client.test.ts index 47a9cd8..4d6ca5d 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -2,11 +2,14 @@ import { describe, expect, test } from "bun:test"; import { baseTokensFromResponse, + buildAuthorizeUrl, exchangeCode, + OAuthTokenEndpointError, OAuthTokenResponseSchemaError, type FetchLike, type OAuthClientConfig, } from "./index.js"; +import { generatePkce } from "./pkce.js"; const config: OAuthClientConfig = { clientId: "client-id", @@ -78,4 +81,123 @@ describe("OAuth client — hostile token responses", () => { ); expect(tokens.expiresAt).toBeUndefined(); }); + + test("accepts an access token with no refresh token instead of inventing one", () => { + // Load-bearing: some grants never issue a refresh token. Throwing here + // rejects a valid token response; storing a fabricated refresh token + // would later be presented to the authorization server. + const tokens = baseTokensFromResponse( + { access_token: "access-only" }, + 0, + undefined, + ); + expect(tokens.access).toBe("access-only"); + expect(tokens.refresh).toBeUndefined(); + }); +}); + +describe("buildAuthorizeUrl — protected parameters", () => { + test("ignores extras that would replace PKCE, state, or the client fields", () => { + // Load-bearing: extraAuthorizeParams are caller-controlled. Applied + // after the protected fields, they could point the code exchange at a + // challenge the caller does not hold or swap the CSRF state. + const pkce = generatePkce(); + const url = new URL( + buildAuthorizeUrl( + { + ...config, + scopes: ["openid"], + resource: "https://mcp.example.com/mcp", + extraAuthorizeParams: { + state: "attacker-state", + code_challenge: "attacker-challenge", + code_challenge_method: "plain", + client_id: "other-client", + redirect_uri: "https://evil.example/callback", + response_type: "token", + scope: "admin", + resource: "https://evil.example/resource", + prompt: "consent", + }, + }, + pkce, + "real-state", + ), + ); + expect(url.searchParams.get("state")).toBe("real-state"); + expect(url.searchParams.get("code_challenge")).toBe(pkce.challenge); + expect(url.searchParams.get("code_challenge_method")).toBe(pkce.method); + expect(url.searchParams.get("client_id")).toBe(config.clientId); + expect(url.searchParams.get("redirect_uri")).toBe(config.redirectUri); + expect(url.searchParams.get("response_type")).toBe("code"); + expect(url.searchParams.get("scope")).toBe("openid"); + expect(url.searchParams.get("resource")).toBe( + "https://mcp.example.com/mcp", + ); + expect(url.searchParams.get("prompt")).toBe("consent"); + }); + + test("omits scope when the caller has none", () => { + const url = new URL( + buildAuthorizeUrl({ ...config, scopes: [] }, generatePkce(), "state-1"), + ); + expect(url.searchParams.has("scope")).toBe(false); + }); +}); + +describe("token endpoint — redirects and error bodies", () => { + test("does not follow a redirect or copy its body into the error", async () => { + // Load-bearing: the POST body holds the code or refresh token. Following + // a 307 replays that secret at Location, and echoing the body can put a + // token into logs. + let calls = 0; + let redirect: RequestInit["redirect"]; + const impl: FetchLike = async (_input, init) => { + calls += 1; + redirect = init?.redirect; + return new Response("secret-refresh-token", { + status: 307, + headers: { + location: "https://evil.example/collect?token=secret-refresh-token", + }, + }); + }; + impl.preconnect = () => undefined; + const error = await exchangeCode( + config, + "auth-code", + "verifier", + impl, + ).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(calls).toBe(1); + expect(redirect).toBe("manual"); + expect(error).toBeInstanceOf(OAuthTokenEndpointError); + expect(String(error)).not.toContain("secret-refresh-token"); + expect(String(error)).not.toContain("evil.example"); + expect(String(error)).toContain("redirect_refused"); + }); + + test("keeps only the oauth error code from a non-2xx body", async () => { + const error = await exchangeCode( + config, + "auth-code", + "verifier", + fakeFetch( + { + error: "invalid_grant", + error_description: "leaked-access-token", + }, + 400, + ), + ).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(OAuthTokenEndpointError); + expect(String(error)).toContain("invalid_grant"); + expect(String(error)).not.toContain("leaked-access-token"); + }); }); diff --git a/src/client.ts b/src/client.ts index 575edef..c4fa826 100644 --- a/src/client.ts +++ b/src/client.ts @@ -27,10 +27,26 @@ export type TokenEndpointConfig = Pick< "clientId" | "tokenUrl" | "tokenTimeoutMs" | "resource" >; +const PROTECTED_AUTHORIZE_PARAMS = new Set([ + "response_type", + "client_id", + "redirect_uri", + "scope", + "code_challenge", + "code_challenge_method", + "state", + "resource", +]); + /** * Build the authorization URL the user opens to grant access. The challenge * binds this request to the PKCE verifier held locally; `state` is the CSRF * nonce the redirect must echo back unchanged. + * + * Caller `extraAuthorizeParams` are applied first and cannot replace the + * protected fields this function sets (PKCE, state, client, redirect, scope, + * resource, response type). An empty scope list omits `scope` rather than + * sending an empty value that would override the server default. */ export function buildAuthorizeUrl( config: OAuthClientConfig, @@ -38,21 +54,23 @@ export function buildAuthorizeUrl( state: string, ): string { const url = new URL(config.authorizeUrl); + if (config.extraAuthorizeParams !== undefined) { + for (const [key, value] of Object.entries(config.extraAuthorizeParams)) { + if (PROTECTED_AUTHORIZE_PARAMS.has(key)) continue; + url.searchParams.set(key, value); + } + } url.searchParams.set("response_type", "code"); url.searchParams.set("client_id", config.clientId); url.searchParams.set("redirect_uri", config.redirectUri); - url.searchParams.set("scope", config.scopes.join(" ")); + const scope = config.scopes.filter((item) => item.length > 0).join(" "); + if (scope.length > 0) url.searchParams.set("scope", scope); url.searchParams.set("code_challenge", pkce.challenge); url.searchParams.set("code_challenge_method", pkce.method); url.searchParams.set("state", state); if (config.resource !== undefined) { url.searchParams.set("resource", config.resource); } - if (config.extraAuthorizeParams !== undefined) { - for (const [key, value] of Object.entries(config.extraAuthorizeParams)) { - url.searchParams.set(key, value); - } - } return url.toString(); } @@ -91,12 +109,10 @@ export class OAuthTokenResponseSchemaError extends Error { } } -// No refresh token was present in the response or carried forward from a prior one. +// No refresh token was present where a refresh was required. export class OAuthMissingRefreshTokenError extends Error { constructor() { - super( - "OAuth token response carried no refresh_token and none was previously stored.", - ); + super("OAuth grant has no refresh token; a new authorization is required."); this.name = "OAuthMissingRefreshTokenError"; } } @@ -105,7 +121,9 @@ export class OAuthMissingRefreshTokenError extends Error { * Convert a token response to the shared base token fields. `now` is * injectable so callers (and tests) control the expiry baseline; * `previousRefresh` is carried forward when a refresh response omits a new - * refresh_token (servers may rotate or not). + * refresh_token (servers may rotate or not). When neither is present the + * result simply has no `refresh`: an access-token-only grant is valid and + * must not be rejected or padded with a made-up refresh token. * * `expires_in` is RECOMMENDED, not required, on a token response (RFC 6749 * §5.1). When a server omits it there is no spec-defined lifetime to fall @@ -118,10 +136,9 @@ export function baseTokensFromResponse( previousRefresh: string | undefined, ): BaseTokens { const refresh = response.refresh_token ?? previousRefresh; - if (refresh === undefined) throw new OAuthMissingRefreshTokenError(); return { access: response.access_token, - refresh, + ...(refresh !== undefined ? { refresh } : {}), ...(response.expires_in !== undefined ? { expiresAt: now + response.expires_in * 1000 } : {}), @@ -131,6 +148,36 @@ export function baseTokensFromResponse( // `fetch` implementation to use for token requests; injectable for tests. export type FetchLike = typeof fetch; +// OAuth error codes are a small token (RFC 6749 §5.2). Anything else in a +// token response body can be a secret and must not be copied into an Error. +const OAUTH_ERROR_CODE = /^[a-z0-9_]{1,64}$/; + +function oauthErrorCode(body: string): string { + try { + const parsed: unknown = JSON.parse(body); + if ( + typeof parsed === "object" && + parsed !== null && + "error" in parsed && + typeof parsed.error === "string" && + OAUTH_ERROR_CODE.test(parsed.error) + ) { + return parsed.error; + } + } catch { + // Non-JSON bodies contribute no public detail. + } + return ""; +} + +function isRedirectResponse(res: Response): boolean { + return ( + res.type === "opaqueredirect" || + res.redirected || + (res.status >= 300 && res.status < 400) + ); +} + // `fetchImpl` is a required parameter here, not defaulted to the global — // only the exported entry points (exchangeCode, refreshTokenRequest) default // to the real `fetch`; an internal helper reaching for the global itself @@ -143,6 +190,10 @@ async function postToken( // Refresh can run on a hot path outside any surrounding timer, so the // token request must abort within the bounded timeout rather than hang // the caller forever when the endpoint stalls. + // + // `redirect: "manual"` is load-bearing: the body carries an authorization + // code or refresh token, and the default follow mode would replay it at + // the Location (RFC 9700). A 3xx is a failure and its body is not read. const res = await fetchImpl(config.tokenUrl, { method: "POST", headers: { @@ -150,14 +201,18 @@ async function postToken( accept: "application/json", }, body: body.toString(), + redirect: "manual", signal: AbortSignal.timeout(config.tokenTimeoutMs), }); + if (isRedirectResponse(res)) { + throw new OAuthTokenEndpointError(res.status, "redirect_refused"); + } if (!res.ok) { let detail = ""; try { - detail = await res.text(); + detail = oauthErrorCode((await res.text()).slice(0, 4096)); } catch { - // Non-2xx is the error; the body is optional detail. + // Non-2xx is the error; the body is not public detail. } throw new OAuthTokenEndpointError(res.status, detail); } diff --git a/src/discovery.test.ts b/src/discovery.test.ts index 4de26bb..06506d1 100644 --- a/src/discovery.test.ts +++ b/src/discovery.test.ts @@ -7,6 +7,7 @@ import { generateState, mcpClientConfig, OAuthDiscoveryError, + clientRegistrationPlan, registerMcpClient, selectMcpScopes, type FetchLike, @@ -686,3 +687,51 @@ describe("mcpClientConfig scope selection", () => { expect(config.scopes).toEqual(["custom"]); }); }); + +describe("clientRegistrationPlan", () => { + const metadataUrl = "https://hub.example.com/oauth/client.json"; + + test("prefers a pre-registered client over metadata documents and dynamic registration", () => { + // Load-bearing: a hub that already has a client id must not be forced + // through dynamic registration, and must not be rewritten to a metadata URL. + expect( + clientRegistrationPlan({ + preRegisteredClientId: " fixed-client ", + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: metadataUrl, + registrationEndpoint: "https://auth.example.com/register", + }), + ).toEqual({ mechanism: "pre-registered", clientId: "fixed-client" }); + }); + + test("uses a metadata document when the server advertises it and no client id was supplied", () => { + expect( + clientRegistrationPlan({ + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: metadataUrl, + }), + ).toEqual({ mechanism: "client-id-metadata", clientId: metadataUrl }); + }); + + test("falls back to dynamic registration and otherwise reports that none is available", () => { + expect( + clientRegistrationPlan({ + registrationEndpoint: "https://auth.example.com/register", + clientMetadataUrl: metadataUrl, + }), + ).toEqual({ + mechanism: "dynamic", + registrationEndpoint: "https://auth.example.com/register", + }); + expect(clientRegistrationPlan({})).toBeUndefined(); + }); + + test("rejects a metadata document URL that is not a plain https path", () => { + expect(() => + clientRegistrationPlan({ + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: "https://hub.example.com/a/../client.json", + }), + ).toThrow(OAuthDiscoveryError); + }); +}); diff --git a/src/discovery.ts b/src/discovery.ts index 1863fbf..fe1131c 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -46,6 +46,7 @@ const AuthorizationServerMetadata = type({ "token_endpoint_auth_methods_supported?": "string[]", "code_challenge_methods_supported?": "string[]", "authorization_response_iss_parameter_supported?": "boolean", + "client_id_metadata_document_supported?": "boolean", }); // RFC 7591 §3.2.1 — registration response, narrowed to the assigned client id @@ -71,6 +72,8 @@ export type McpAuthorizationServer = { // RFC 9207: when true, every authorization response carries `iss` and the // client must reject a redirect whose `iss` is not this issuer. authorizationResponseIssParameterSupported?: boolean; + /** OAuth Client ID Metadata Documents (MCP authorization). */ + clientIdMetadataDocumentSupported?: boolean; }; // A login entry derived from a resource URL: everything the host needs to @@ -355,6 +358,12 @@ export async function discoverMcpLoginEntry( metadata.authorization_response_iss_parameter_supported, } : {}), + ...(metadata.client_id_metadata_document_supported !== undefined + ? { + clientIdMetadataDocumentSupported: + metadata.client_id_metadata_document_supported, + } + : {}), }, ...(resourceScopesSupported !== undefined ? { resourceScopesSupported } @@ -393,6 +402,73 @@ function grantedScope( return echoed ?? requested; } +function assertClientMetadataUrl(raw: string): void { + let url: URL; + try { + url = new URL(raw); + } catch { + throw new OAuthDiscoveryError("client metadata URL is not absolute"); + } + if (url.protocol !== "https:") + throw new OAuthDiscoveryError("client metadata URL must use https"); + if (url.pathname === "" || url.pathname === "/") + throw new OAuthDiscoveryError("client metadata URL must contain a path"); + if (url.hash !== "" || url.username !== "" || url.password !== "") + throw new OAuthDiscoveryError( + "client metadata URL must not contain a fragment or userinfo", + ); + const afterScheme = raw.slice(raw.indexOf("://") + 3); + const pathStart = afterScheme.indexOf("/"); + const rawPath = + pathStart === -1 + ? "" + : (afterScheme.slice(pathStart).split(/[?#]/, 1)[0] ?? ""); + if (rawPath.split("/").some((segment) => /^(?:\.|%2e){1,2}$/i.test(segment))) + throw new OAuthDiscoveryError( + "client metadata URL must not contain dot segments", + ); +} + +/** + * How a resource login obtains a client id. A pre-registered id wins, then a + * Client ID Metadata Document when the server advertises it and the host + * supplied a document URL, then RFC 7591. `undefined` means none of those + * is available — dynamic registration is not required when another applies. + */ +export type ClientRegistrationPlan = + | { readonly mechanism: "pre-registered"; readonly clientId: string } + | { readonly mechanism: "client-id-metadata"; readonly clientId: string } + | { readonly mechanism: "dynamic"; readonly registrationEndpoint: string }; + +export function clientRegistrationPlan(opts: { + readonly registrationEndpoint?: string; + readonly clientIdMetadataDocumentSupported?: boolean; + readonly clientMetadataUrl?: string; + readonly preRegisteredClientId?: string; +}): ClientRegistrationPlan | undefined { + const preRegistered = opts.preRegisteredClientId?.trim(); + if (preRegistered !== undefined && preRegistered.length > 0) { + return { mechanism: "pre-registered", clientId: preRegistered }; + } + if ( + opts.clientIdMetadataDocumentSupported === true && + opts.clientMetadataUrl !== undefined + ) { + assertClientMetadataUrl(opts.clientMetadataUrl); + return { + mechanism: "client-id-metadata", + clientId: opts.clientMetadataUrl, + }; + } + if (opts.registrationEndpoint !== undefined) { + return { + mechanism: "dynamic", + registrationEndpoint: opts.registrationEndpoint, + }; + } + return undefined; +} + /** * Register a loopback public client with the authorization server (RFC 7591). * Loopback clients cannot keep a secret, so registration requests diff --git a/src/hub/credentials.test.ts b/src/hub/credentials.test.ts new file mode 100644 index 0000000..1465a30 --- /dev/null +++ b/src/hub/credentials.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import type { CredentialCipher } from "@intx/types"; + +import { writeOAuthTokens, type OAuthCredentialWriter } from "./credentials.js"; + +const cipher: CredentialCipher = { + encrypt: (plaintext) => Promise.resolve(`sealed:${plaintext}`), + decrypt: () => Promise.reject(new Error("unused")), +}; + +describe("writeOAuthTokens", () => { + test("stores an access token without a refresh secret when none was issued", async () => { + // Load-bearing: encrypting tokens.refresh unconditionally throws on a + // valid access-token-only grant, so the login cannot be stored. + const written: { refreshSecret: string | null; secret: string }[] = []; + const db = { + update: () => ({ + set: (values: { refreshSecret: string | null; secret: string }) => ({ + where: () => { + written.push({ + refreshSecret: values.refreshSecret, + secret: values.secret, + }); + return Promise.resolve(); + }, + }), + }), + }; + await writeOAuthTokens({ + db: db as unknown as OAuthCredentialWriter, + cipher, + credentialId: "cred_1", + tokens: { access: "access-only" }, + metadata: {}, + }); + expect(written).toEqual([ + { secret: "sealed:access-only", refreshSecret: null }, + ]); + }); +}); diff --git a/src/hub/credentials.ts b/src/hub/credentials.ts index 7eb6fc7..2bac57f 100644 --- a/src/hub/credentials.ts +++ b/src/hub/credentials.ts @@ -32,6 +32,25 @@ export const OAUTH_RESOURCE_METADATA_KEY = "oauthResource"; /** Everything the token writes below need — a `db` or an open transaction. */ export type OAuthCredentialWriter = Pick; +async function sealTokenSecrets( + cipher: CredentialCipher, + credentialId: string, + tokens: BaseTokens, +): Promise<{ secret: string; refreshSecret: string | null }> { + const secret = await cipher.encrypt( + tokens.access, + credentialAad(credentialId, "secret"), + ); + if (tokens.refresh === undefined) return { secret, refreshSecret: null }; + return { + secret, + refreshSecret: await cipher.encrypt( + tokens.refresh, + credentialAad(credentialId, "refreshSecret"), + ), + }; +} + export type WriteOAuthTokensOpts = { readonly db: OAuthCredentialWriter; readonly cipher: CredentialCipher; @@ -51,13 +70,10 @@ export type WriteOAuthTokensOpts = { export async function writeOAuthTokens( opts: WriteOAuthTokensOpts, ): Promise { - const secret = await opts.cipher.encrypt( - opts.tokens.access, - credentialAad(opts.credentialId, "secret"), - ); - const refreshSecret = await opts.cipher.encrypt( - opts.tokens.refresh, - credentialAad(opts.credentialId, "refreshSecret"), + const sealed = await sealTokenSecrets( + opts.cipher, + opts.credentialId, + opts.tokens, ); await opts.db .update(credential) @@ -65,8 +81,8 @@ export async function writeOAuthTokens( ...(opts.providerId !== undefined ? { providerId: opts.providerId } : {}), ...(opts.scopes !== undefined ? { scopes: [...opts.scopes] } : {}), type: "oauth_token", - secret, - refreshSecret, + secret: sealed.secret, + refreshSecret: sealed.refreshSecret, expiresAt: opts.tokens.expiresAt === undefined ? null @@ -203,14 +219,7 @@ export async function persistOAuthCredential( return credentialId; } - const secret = await opts.cipher.encrypt( - opts.tokens.access, - credentialAad(credentialId, "secret"), - ); - const refreshSecret = await opts.cipher.encrypt( - opts.tokens.refresh, - credentialAad(credentialId, "refreshSecret"), - ); + const sealed = await sealTokenSecrets(opts.cipher, credentialId, opts.tokens); const expiresAt = opts.tokens.expiresAt === undefined ? null @@ -226,8 +235,8 @@ export async function persistOAuthCredential( name: opts.name, type: "oauth_token", description: null, - secret, - refreshSecret, + secret: sealed.secret, + refreshSecret: sealed.refreshSecret, scopes: [...opts.scopes], expiresAt, metadata, diff --git a/src/hub/mount.ts b/src/hub/mount.ts index 26f6bfe..b7b05aa 100644 --- a/src/hub/mount.ts +++ b/src/hub/mount.ts @@ -12,6 +12,7 @@ import { exchangeCode, mcpClientConfig, OAuthDiscoveryError, + clientRegistrationPlan, registerMcpClient, startCallbackServer, startOAuthLogin, @@ -51,6 +52,8 @@ const Target = { providerId: "string", credentialName: "string" } as const; const StartLogin = type({ provider: "string", ...Target }).or({ /** An MCP server URL: discovery and dynamic registration supply the rest. */ resourceUrl: "string", + /** Pre-registered client id. Skips dynamic registration when set. */ + "clientId?": "string", ...Target, }); @@ -94,12 +97,26 @@ export function mountOAuthCallback( opts: { readonly store: OAuthLoginStore; readonly path?: string }, ): void { app.get(opts.path ?? "/oauth/callback", (c) => { + const query = (key: string): string | undefined => { + const values = c.req.queries(key); + return values === undefined || values.length === 0 + ? undefined + : values[0]; + }; + const repeated = [ + "state", + "code", + "error", + "error_description", + "iss", + ].some((key) => (c.req.queries(key)?.length ?? 0) > 1); const { status, html } = opts.store.callbacks.handle({ - state: c.req.query("state"), - code: c.req.query("code"), - error: c.req.query("error"), - errorDescription: c.req.query("error_description"), - iss: c.req.query("iss"), + state: query("state"), + code: query("code"), + error: query("error"), + errorDescription: query("error_description"), + iss: query("iss"), + ...(repeated ? { repeatedParameter: true } : {}), }); // The code sits in this URL: keep it out of caches and referrers. c.header("cache-control", "no-store"); @@ -148,6 +165,13 @@ export type MountOAuthLoginOpts = { readonly fetchImpl?: FetchLike; /** The client name shown on a dynamically registered consent screen. */ readonly clientName?: string; + /** + * HTTPS URL of this hub's Client ID Metadata Document. Used as `client_id` + * when the authorization server advertises that mechanism and the login + * does not pass a pre-registered client id. The host must publish the + * document at this URL; passing it does not publish it. + */ + readonly clientMetadataUrl?: string; readonly loginTtlMs?: number; /** Reported when a login fails after the request that started it returned. */ readonly onError?: (error: unknown, context: { provider: string }) => void; @@ -226,28 +250,50 @@ export function mountOAuthLogin( resourceUrl: body.resourceUrl, ...(opts.fetchImpl !== undefined ? { fetchImpl: opts.fetchImpl } : {}), }); - const registrationEndpoint = - entry.authorizationServer.registrationEndpoint; - if (registrationEndpoint === undefined) { - return { - error: `${body.resourceUrl} does not support dynamic client registration`, - status: 502, - }; - } - const registration = await registerMcpClient({ - registrationEndpoint, - redirectUris: [redirectUri], - clientName: opts.clientName ?? "Corbits", - ...(entry.authorizationServer.grantTypesSupported !== undefined + const plan = clientRegistrationPlan({ + ...(entry.authorizationServer.registrationEndpoint !== undefined ? { - grantTypesSupported: - entry.authorizationServer.grantTypesSupported, + registrationEndpoint: + entry.authorizationServer.registrationEndpoint, } : {}), - ...(opts.fetchImpl !== undefined ? { fetchImpl: opts.fetchImpl } : {}), + ...(entry.authorizationServer.clientIdMetadataDocumentSupported === true + ? { clientIdMetadataDocumentSupported: true } + : {}), + ...(opts.clientMetadataUrl !== undefined + ? { clientMetadataUrl: opts.clientMetadataUrl } + : {}), + ...(body.clientId !== undefined + ? { preRegisteredClientId: body.clientId } + : {}), }); + if (plan === undefined) { + return { + error: `${body.resourceUrl} has no usable client registration mechanism`, + status: 502, + }; + } + const clientId = + plan.mechanism === "dynamic" + ? ( + await registerMcpClient({ + registrationEndpoint: plan.registrationEndpoint, + redirectUris: [redirectUri], + clientName: opts.clientName ?? "Corbits", + ...(entry.authorizationServer.grantTypesSupported !== undefined + ? { + grantTypesSupported: + entry.authorizationServer.grantTypesSupported, + } + : {}), + ...(opts.fetchImpl !== undefined + ? { fetchImpl: opts.fetchImpl } + : {}), + }) + ).clientId + : plan.clientId; const config = mcpClientConfig(entry, { - clientId: registration.clientId, + clientId, redirectUri, }); return { @@ -261,18 +307,18 @@ export function mountOAuthLogin( undefined, ), metadata: () => ({ - [OAUTH_CLIENT_ID_METADATA_KEY]: registration.clientId, + [OAUTH_CLIENT_ID_METADATA_KEY]: clientId, [OAUTH_TOKEN_URL_METADATA_KEY]: config.tokenUrl, [OAUTH_RESOURCE_METADATA_KEY]: entry.resourceUrl, }), startCallbackServer: (state) => - callbacks.start( - state, - entry.authorizationServer + callbacks.start(state, { + expectedIssuer: entry.authorizationServer.issuer, + ...(entry.authorizationServer .authorizationResponseIssParameterSupported === true - ? { expectedIssuer: entry.authorizationServer.issuer } - : {}, - ), + ? { requireIssuer: true } + : {}), + }), }, }; } catch (cause) { diff --git a/src/hub/refresh.ts b/src/hub/refresh.ts index 8224962..e29589d 100644 --- a/src/hub/refresh.ts +++ b/src/hub/refresh.ts @@ -6,6 +6,7 @@ import { and, eq, inArray, isNotNull, lte, or, sql } from "drizzle-orm"; import { baseTokensFromResponse, + OAuthMissingRefreshTokenError, OAuthTokenEndpointError, refreshTokenRequest, type BaseTokens, @@ -117,10 +118,13 @@ function dueAt(expiresAt: Date | null, deadline: number): boolean { } // Only `invalid_grant` (RFC 6749 §5.2) means the refresh token itself is -// dead and a fresh sign-in is the fix. Everything else — 429, 408, a 401 +// dead and a fresh sign-in is the fix. A grant that never had a refresh +// token is the same outcome. Everything else — 429, 408, a 401 // `invalid_client`, 5xx, network, decrypt — is retried on the next pass. function isReauthError(error: unknown): boolean { + if (error instanceof OAuthMissingRefreshTokenError) return true; if (!(error instanceof OAuthTokenEndpointError)) return false; + if (error.detail === "invalid_grant") return true; try { const body: unknown = JSON.parse(error.detail); return ( @@ -230,6 +234,8 @@ export function createOAuthRefreshStore(opts: { and( eq(credential.type, "oauth_token"), eq(credential.status, "active"), + // No refresh token was issued: there is nothing to redeem. The + // row stays until the host starts a new login; it is not due. isNotNull(credential.refreshSecret), isNotNull(credential.expiresAt), lte(credential.expiresAt, dueBefore), diff --git a/src/hub/route-callback.test.ts b/src/hub/route-callback.test.ts new file mode 100644 index 0000000..102ac46 --- /dev/null +++ b/src/hub/route-callback.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, test } from "bun:test"; + +import { OAuthCallbackError } from "../callback-server.js"; +import { createRouteCallbacks } from "./route-callback.js"; + +const issuer = "https://auth.example.com"; + +async function started(options: { + expectedIssuer?: string; + requireIssuer?: boolean; +}) { + const callbacks = createRouteCallbacks(); + const server = await callbacks.start("state-1", options); + return { callbacks, server }; +} + +describe("route callback issuer binding", () => { + test("rejects a present iss that is not the issuer recorded with the state", async () => { + // Load-bearing: metadata that omits authorization_response_iss_parameter_supported + // used to skip iss entirely, so a mix-up redirect carrying another issuer + // completed the login. The issuer is bound before the code is read. + const { callbacks, server } = await started({ expectedIssuer: issuer }); + const pending = server.waitForCode(new AbortController().signal); + const response = callbacks.handle({ + state: "state-1", + code: "the-code", + error: "access_denied", + iss: "https://other.example.com", + }); + expect(response.status).toBe(400); + expect(response.html).toContain("no longer the sign-in"); + expect(response.html).not.toContain("access_denied"); + await expect(pending).rejects.toBeInstanceOf(OAuthCallbackError); + }); + + test("allows a missing iss only when the server did not advertise it", async () => { + const open = await started({ expectedIssuer: issuer }); + const openCode = open.server.waitForCode(new AbortController().signal); + expect( + open.callbacks.handle({ + state: "state-1", + code: "the-code", + error: undefined, + }).status, + ).toBe(200); + expect(await openCode).toBe("the-code"); + + const required = await started({ + expectedIssuer: issuer, + requireIssuer: true, + }); + const requiredCode = required.server.waitForCode( + new AbortController().signal, + ); + expect( + required.callbacks.handle({ + state: "state-1", + code: "the-code", + error: undefined, + }).status, + ).toBe(400); + await expect(requiredCode).rejects.toBeInstanceOf(OAuthCallbackError); + }); + + test("rejects a repeated parameter before accepting its code", async () => { + const { callbacks, server } = await started({ expectedIssuer: issuer }); + const pending = server.waitForCode(new AbortController().signal); + const response = callbacks.handle({ + state: "state-1", + code: "the-code", + error: undefined, + iss: issuer, + repeatedParameter: true, + }); + expect(response.status).toBe(400); + await expect(pending).rejects.toBeInstanceOf(OAuthCallbackError); + }); +}); diff --git a/src/hub/route-callback.ts b/src/hub/route-callback.ts index 1128dda..8c27b93 100644 --- a/src/hub/route-callback.ts +++ b/src/hub/route-callback.ts @@ -14,17 +14,24 @@ export type CallbackParams = { errorDescription?: string | undefined; /** RFC 9207 issuer identifier, when the authorization server sends one. */ iss?: string | undefined; + /** + * A callback parameter was repeated. Refused before `code` or `error` is + * acted on, so a duplicated field cannot pick which value wins. + */ + repeatedParameter?: boolean; }; export type CallbackResponse = { status: 200 | 400; html: string }; export type RouteCallbackOptions = { /** - * The issuer every redirect for this login must carry as `iss` (RFC 9207). - * Set only when the server's metadata promises the parameter; a redirect - * missing or mismatching it is then refused as a mix-up attack. + * Issuer recorded with this login's state (RFC 9207). A present `iss` must + * match it exactly, whether or not the server advertised support. A missing + * `iss` is refused only when `requireIssuer` is set. */ expectedIssuer?: string; + /** Set when metadata says `authorization_response_iss_parameter_supported`. */ + requireIssuer?: boolean; }; /** @@ -43,6 +50,7 @@ export type RouteCallbacks = { type Pending = { finish: (outcome: Outcome) => void; expectedIssuer: string | undefined; + requireIssuer: boolean; }; export function createRouteCallbacks(): RouteCallbacks { @@ -66,6 +74,7 @@ export function createRouteCallbacks(): RouteCallbacks { pending.set(expectedState, { finish, expectedIssuer: options?.expectedIssuer, + requireIssuer: options?.requireIssuer === true, }); return Promise.resolve({ // Served by the hub's own listener; there is no loopback port. @@ -88,7 +97,7 @@ export function createRouteCallbacks(): RouteCallbacks { close: () => pending.delete(expectedState), }); }, - handle({ state, code, error, errorDescription, iss }) { + handle({ state, code, error, errorDescription, iss, repeatedParameter }) { const entry = state === undefined ? undefined : pending.get(state); // Not a login in flight (or a stale one): refuse without touching any. if (entry === undefined) @@ -105,10 +114,18 @@ export function createRouteCallbacks(): RouteCallbacks { }); return { status: 400, html: signInFailedHtml(failure) }; }; - // RFC 9207 §2.4: a client that knows the server sends `iss` must - // reject a response whose `iss` is absent or names another issuer. - if (entry.expectedIssuer !== undefined && iss !== entry.expectedIssuer) - return failed({ code: "state_mismatch" }); + // Provenance before the grant: a repeated parameter, or an `iss` that + // is not the issuer recorded with this state, is not this login's + // redirect. RFC 9207 §2.4 — a present `iss` is always compared; a + // missing one is refused only when the server advertised it. + if (repeatedParameter === true) return failed({ code: "state_mismatch" }); + if (entry.expectedIssuer !== undefined) { + if (iss === undefined) { + if (entry.requireIssuer) return failed({ code: "state_mismatch" }); + } else if (iss !== entry.expectedIssuer) { + return failed({ code: "state_mismatch" }); + } + } if (error !== undefined) return failed( errorDescription === undefined diff --git a/src/index.ts b/src/index.ts index d28d698..bb7f27c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -58,9 +58,11 @@ export { export { discoverMcpLoginEntry, registerMcpClient, + clientRegistrationPlan, mcpClientConfig, selectMcpScopes, OAuthDiscoveryError, + type ClientRegistrationPlan, type DiscoverMcpLoginEntryOptions, type McpAuthorizationServer, type McpClientConfigOptions, diff --git a/src/session.test.ts b/src/session.test.ts index 1ce6bcc..289f782 100644 --- a/src/session.test.ts +++ b/src/session.test.ts @@ -1,6 +1,11 @@ import { describe, expect, test } from "bun:test"; -import { createTokenSession, type BaseTokens } from "./index.js"; +import { + createTokenSession, + OAuthMissingRefreshTokenError, + OAuthRefreshFailedError, + type BaseTokens, +} from "./index.js"; describe("Token session createTokenSession — refresh coalescing", () => { test("coalesces concurrent refreshes for the same profile into one request", async () => { @@ -33,4 +38,35 @@ describe("Token session createTokenSession — refresh coalescing", () => { expect(await Promise.all([first, second])).toEqual(["new", "new"]); expect(refreshCount).toBe(1); }); + + test("uses an access-token-only grant until it expires, then asks for a new login", async () => { + // Load-bearing: a grant with no refresh token is still a usable access + // token. Refreshing it would require a refresh token this session does + // not have, so expiry is a reauthorization, not a token request. + let refreshCount = 0; + const stored: { tokens: BaseTokens } = { tokens: { access: "only" } }; + const session = createTokenSession({ + skewMs: 0, + loadProfile: async () => stored, + updateTokens: async () => undefined, + refreshTokens: async () => { + refreshCount += 1; + return { access: "should-not-run" }; + }, + toAccess: (tokens) => tokens.access, + }); + expect(await session.getValidToken("p", 500)).toBe("only"); + stored.tokens = { access: "only", expiresAt: 1_000 }; + const expiredError = await session.getValidToken("p", 1_000).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(expiredError).toBeInstanceOf(OAuthRefreshFailedError); + expect( + expiredError instanceof OAuthRefreshFailedError + ? expiredError.cause + : undefined, + ).toBeInstanceOf(OAuthMissingRefreshTokenError); + expect(refreshCount).toBe(0); + }); }); diff --git a/src/session.ts b/src/session.ts index 08ae9c6..debe7b4 100644 --- a/src/session.ts +++ b/src/session.ts @@ -1,3 +1,4 @@ +import { OAuthMissingRefreshTokenError } from "./client.js"; import type { BaseTokens } from "./tokens.js"; /** @@ -71,8 +72,15 @@ export function createTokenSession( // Re-check expiry after the I/O; another caller may have refreshed already. if (!isExpired(profile.tokens, now)) return deps.toAccess(profile.tokens); let refreshed: TTokens; + const refreshToken = profile.tokens.refresh; + if (refreshToken === undefined) { + throw new OAuthRefreshFailedError( + name, + new OAuthMissingRefreshTokenError(), + ); + } try { - refreshed = await deps.refreshTokens(profile.tokens.refresh, now); + refreshed = await deps.refreshTokens(refreshToken, now); } catch (err) { throw new OAuthRefreshFailedError(name, err); } diff --git a/src/tokens.ts b/src/tokens.ts index 72d3f8e..048abea 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -1,6 +1,10 @@ // Token shape the exchange returns. Persistence is the host's job // (Interchange `oauth_token` credential, OS vault, or otherwise). // +// `refresh` is optional because an authorization server may issue an access +// token and no refresh token (RFC 6749 §5.1). Absence means a new +// authorization is required at expiry — this package does not invent one. +// // `expiresAt` is optional because RFC 6749 §5.1 makes `expires_in` RECOMMENDED, // not required — a token endpoint may omit it, and this package refuses to // guess a lifetime on the caller's behalf (see `client.ts`). `undefined` @@ -8,7 +12,7 @@ // a token as not expired until the server rejects it (see there for why). export type BaseTokens = { access: string; - refresh: string; + refresh?: string; expiresAt?: number; }; From 64137abb47ac35f7ba98c29fb994570d1d5239d2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:36:38 -0700 Subject: [PATCH 2/6] fix: reject authorization-server metadata that splits the issuer RFC 8414 requires the document issuer to be the identifier used to fetch it. Authorization and token endpoints must share that issuer origin, so a code is not posted to another host. --- src/discovery.test.ts | 152 ++++++++++++++++++++++++++++++++++++++++-- src/discovery.ts | 58 ++++++++++++++-- src/hub/mount.ts | 15 ++++- 3 files changed, 212 insertions(+), 13 deletions(-) diff --git a/src/discovery.test.ts b/src/discovery.test.ts index 06506d1..183cd25 100644 --- a/src/discovery.test.ts +++ b/src/discovery.test.ts @@ -3,6 +3,7 @@ import { describe, expect, test } from "bun:test"; import { buildAuthorizeUrl, discoverMcpLoginEntry, + exchangeCode, generatePkce, generateState, mcpClientConfig, @@ -24,6 +25,17 @@ const asMetadata = { code_challenge_methods_supported: ["S256"], }; +// Fallback discovery fetches AS metadata using the resource URL as the issuer +// identifier, so the document's issuer must be that URL (RFC 8414 §3.3) and +// its endpoints must share the issuer origin. +const fallbackAsMetadata = { + ...asMetadata, + issuer: resourceUrl, + authorization_endpoint: "https://mcp.example.com/authorize", + token_endpoint: "https://mcp.example.com/token", + registration_endpoint: "https://mcp.example.com/register", +}; + // Real metadata (checked 2026-09-22) from two live MCP servers, used to keep // the negotiation logic honest against shapes actual authorization servers // send. No provider-specific behavior lives in src — these are fixtures only. @@ -130,12 +142,12 @@ describe("MCP OAuth discovery", () => { const { fetchImpl } = fakeFetch({ "https://mcp.example.com/.well-known/oauth-authorization-server/mcp": { status: 200, - body: asMetadata, + body: fallbackAsMetadata, }, }); const entry = await discoverMcpLoginEntry({ resourceUrl, fetchImpl }); expect(entry.authorizationServer.tokenEndpoint).toBe( - "https://auth.example.com/token", + "https://mcp.example.com/token", ); }); @@ -235,6 +247,133 @@ describe("MCP OAuth discovery", () => { ).rejects.toThrow(OAuthDiscoveryError); }); + test("discards authorization-server metadata whose issuer is not the fetch issuer", async () => { + // Load-bearing: RFC 8414 §3.3. The callback compares `iss` to the issuer + // stored from this document. A different issuer would make that check + // agree with an authorization server we did not fetch. + const { fetchImpl } = fakeFetch({ + "https://mcp.example.com/.well-known/oauth-protected-resource/mcp": { + status: 200, + body: { + resource: resourceUrl, + authorization_servers: ["https://auth.example.com"], + }, + }, + "https://auth.example.com/.well-known/oauth-authorization-server": { + status: 200, + body: { + ...asMetadata, + issuer: "https://honest.example", + authorization_endpoint: "https://honest.example/authorize", + token_endpoint: "https://honest.example/token", + }, + }, + }); + const error = await discoverMcpLoginEntry({ resourceUrl, fetchImpl }).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(OAuthDiscoveryError); + expect(String(error)).not.toContain("honest.example"); + }); + + test("does not exchange a code at a token endpoint on another origin", async () => { + // Load-bearing: a split document can name an honest issuer (so the + // callback `iss` matches) and put token_endpoint on an attacker origin. + // Accepting it would POST the code and code_verifier there. + const posts: { url: string; body: string }[] = []; + const honest = "https://honest.example"; + const fetchImpl: FetchLike = Object.assign( + async (input: Parameters[0], init?: RequestInit) => { + const url = String(input); + if (init?.method === "POST") { + posts.push({ url, body: String(init.body ?? "") }); + return new Response(JSON.stringify({ access_token: "stolen" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + if ( + url === + "https://mcp.example.com/.well-known/oauth-protected-resource/mcp" + ) { + return new Response( + JSON.stringify({ + resource: resourceUrl, + authorization_servers: [honest], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (url === `${honest}/.well-known/oauth-authorization-server`) { + return new Response( + JSON.stringify({ + issuer: honest, + authorization_endpoint: `${honest}/authorize`, + token_endpoint: "https://evil.example/token", + code_challenge_methods_supported: ["S256"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + return new Response("not found", { status: 404 }); + }, + { preconnect: () => undefined }, + ); + let discovered: McpLoginEntry | undefined; + let failure: unknown; + try { + discovered = await discoverMcpLoginEntry({ resourceUrl, fetchImpl }); + } catch (cause) { + failure = cause; + } + if (discovered !== undefined) { + await exchangeCode( + mcpClientConfig(discovered, { + clientId: "client", + redirectUri: "https://hub.example.com/callback", + }), + "the-code", + "the-verifier", + fetchImpl, + ).catch(() => undefined); + } + expect(failure).toBeInstanceOf(OAuthDiscoveryError); + expect(posts).toEqual([]); + expect(String(failure)).not.toContain("evil.example"); + expect(String(failure)).not.toContain("the-verifier"); + }); + + test("rejects an authorization endpoint on another origin than the issuer", async () => { + // Load-bearing: the same mix-up applies to the authorize URL. The user + // would grant a code at the attacker while `iss` still matches. + const honest = "https://honest.example"; + const { fetchImpl } = fakeFetch({ + "https://mcp.example.com/.well-known/oauth-protected-resource/mcp": { + status: 200, + body: { + resource: resourceUrl, + authorization_servers: [honest], + }, + }, + [`${honest}/.well-known/oauth-authorization-server`]: { + status: 200, + body: { + issuer: honest, + authorization_endpoint: "https://evil.example/authorize", + token_endpoint: `${honest}/token`, + code_challenge_methods_supported: ["S256"], + }, + }, + }); + const error = await discoverMcpLoginEntry({ resourceUrl, fetchImpl }).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(OAuthDiscoveryError); + expect(String(error)).not.toContain("evil.example"); + }); + test("refuses an authorization server that omits code_challenge_methods_supported", async () => { // Load-bearing: MCP spec "Security Considerations" — absence means the // server does not support PKCE and the client MUST refuse to proceed @@ -242,7 +381,7 @@ describe("MCP OAuth discovery", () => { const { code_challenge_methods_supported: _unused, ...metadataWithoutPkce - } = asMetadata; + } = fallbackAsMetadata; const { fetchImpl } = fakeFetch({ "https://mcp.example.com/.well-known/oauth-authorization-server/mcp": { status: 200, @@ -260,7 +399,10 @@ describe("MCP OAuth discovery", () => { const { fetchImpl } = fakeFetch({ "https://mcp.example.com/.well-known/oauth-authorization-server/mcp": { status: 200, - body: { ...asMetadata, code_challenge_methods_supported: ["plain"] }, + body: { + ...fallbackAsMetadata, + code_challenge_methods_supported: ["plain"], + }, }, }); await expect( @@ -276,7 +418,7 @@ describe("MCP OAuth discovery", () => { "https://mcp.example.com/.well-known/oauth-authorization-server/mcp": { status: 200, body: { - ...asMetadata, + ...fallbackAsMetadata, token_endpoint_auth_methods_supported: ["client_secret_basic"], }, }, diff --git a/src/discovery.ts b/src/discovery.ts index fe1131c..5d8b726 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -258,6 +258,43 @@ function assertSupportsPublicClients( ); } +// RFC 8414 §3.3: the issuer value in the document must be the identifier the +// well-known URL was built from. Anything else is discarded. +function assertFetchedIssuer( + asMetadataUrl: string, + fetchIssuer: string, + metadataIssuer: string, +): void { + if (metadataIssuer !== fetchIssuer) + throw new OAuthDiscoveryError( + `authorization-server metadata at ${asMetadataUrl} issuer does not match the issuer identifier used to retrieve it.`, + ); +} + +// The authorization and token endpoints are used with that issuer. A document +// that points either at another origin is a mix-up: the code and verifier +// would be posted to the foreign token endpoint. +export function assertEndpointOnIssuer( + issuer: string, + endpoint: string, + field: "authorization_endpoint" | "token_endpoint", +): void { + let issuerUrl: URL; + let endpointUrl: URL; + try { + issuerUrl = new URL(issuer); + endpointUrl = new URL(endpoint); + } catch { + throw new OAuthDiscoveryError( + `authorization server metadata ${field} is not a valid URL.`, + ); + } + if (endpointUrl.origin !== issuerUrl.origin) + throw new OAuthDiscoveryError( + `authorization server metadata ${field} origin does not match the issuer.`, + ); +} + // Resolve the authorization server for a resource URL: protected-resource // metadata first (RFC 9728), falling back to authorization-server metadata // served directly at the resource (RFC 8414 §3.1) for servers that skip the @@ -304,11 +341,10 @@ export async function discoverMcpLoginEntry( } // No protected-resource document: the resource itself may serve - // authorization-server metadata (some MCP servers do). - const asMetadataUrl = - issuer === undefined - ? wellKnownUrl(resourceUrl, "oauth-authorization-server") - : wellKnownUrl(issuer, "oauth-authorization-server"); + // authorization-server metadata (some MCP servers do). The URL the + // well-known suffix is inserted into is the issuer RFC 8414 compares. + const fetchIssuer = issuer ?? resourceUrl; + const asMetadataUrl = wellKnownUrl(fetchIssuer, "oauth-authorization-server"); const asRes = await getJson(asMetadataUrl, fetchImpl, timeoutMs); if (!asRes.ok) throw badStatus(asMetadataUrl, asRes.status); const metadata = AuthorizationServerMetadata( @@ -319,6 +355,18 @@ export async function discoverMcpLoginEntry( `authorization-server metadata at ${asMetadataUrl} is malformed: ${metadata.summary}`, ); + assertFetchedIssuer(asMetadataUrl, fetchIssuer, metadata.issuer); + assertEndpointOnIssuer( + metadata.issuer, + metadata.authorization_endpoint, + "authorization_endpoint", + ); + assertEndpointOnIssuer( + metadata.issuer, + metadata.token_endpoint, + "token_endpoint", + ); + assertSupportsS256(asMetadataUrl, metadata.code_challenge_methods_supported); assertSupportsPublicClients( asMetadataUrl, diff --git a/src/hub/mount.ts b/src/hub/mount.ts index b7b05aa..30c1ad1 100644 --- a/src/hub/mount.ts +++ b/src/hub/mount.ts @@ -22,6 +22,7 @@ import { type OAuthLoginHandle, type OAuthClientConfig, } from "../index.js"; +import { assertEndpointOnIssuer } from "../discovery.js"; import { checkOAuthCredentialTarget, OAUTH_CLIENT_ID_METADATA_KEY, @@ -296,16 +297,24 @@ export function mountOAuthLogin( clientId, redirectUri, }); + const issuer = entry.authorizationServer.issuer; + assertEndpointOnIssuer( + issuer, + config.authorizeUrl, + "authorization_endpoint", + ); return { flow: { provider: new URL(entry.resourceUrl).origin, config, - exchange: async (code, verifier, now) => - baseTokensFromResponse( + exchange: async (code, verifier, now) => { + assertEndpointOnIssuer(issuer, config.tokenUrl, "token_endpoint"); + return baseTokensFromResponse( await exchangeCode(config, code, verifier, opts.fetchImpl), now, undefined, - ), + ); + }, metadata: () => ({ [OAUTH_CLIENT_ID_METADATA_KEY]: clientId, [OAUTH_TOKEN_URL_METADATA_KEY]: config.tokenUrl, From 4a564a0047e3220c306d773983abec00b5e7988d Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:36:38 -0700 Subject: [PATCH 3/6] fix: treat a blank refresh token as absent An empty or whitespace refresh token is not a credential. Parse, seal, session, and the refresh ticker skip it instead of storing or redeeming it. --- src/client.test.ts | 55 +++++++++++++++++++++++++++++++++++++ src/client.ts | 26 ++++++++++++++---- src/hub/credentials.test.ts | 35 +++++++++++++++++++++++ src/hub/credentials.ts | 7 +++-- src/hub/refresh.test.ts | 34 ++++++++++++++++++++++- src/hub/refresh.ts | 23 ++++++++++++---- src/session.test.ts | 35 +++++++++++++++++++++++ src/session.ts | 4 +-- src/tokens.ts | 17 ++++++++++-- 9 files changed, 217 insertions(+), 19 deletions(-) diff --git a/src/client.test.ts b/src/client.test.ts index 4d6ca5d..009409d 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -4,8 +4,10 @@ import { baseTokensFromResponse, buildAuthorizeUrl, exchangeCode, + OAuthMissingRefreshTokenError, OAuthTokenEndpointError, OAuthTokenResponseSchemaError, + refreshTokenRequest, type FetchLike, type OAuthClientConfig, } from "./index.js"; @@ -94,6 +96,59 @@ describe("OAuth client — hostile token responses", () => { expect(tokens.access).toBe("access-only"); expect(tokens.refresh).toBeUndefined(); }); + + test("treats a blank refresh token as absent, including one carried forward", () => { + // Load-bearing: `""` and `" "` are strings, so a nullish check stores and + // later redeems them. A blank value is not a credential. + for (const refresh of ["", " "]) { + const tokens = baseTokensFromResponse( + { access_token: "access-kept", refresh_token: refresh }, + 0, + refresh, + ); + expect(tokens.refresh).toBeUndefined(); + } + const kept = baseTokensFromResponse( + { access_token: "access-kept", refresh_token: " " }, + 0, + "prior-refresh", + ); + expect(kept.refresh).toBe("prior-refresh"); + }); + + test("parses a blank refresh_token off the token response", async () => { + const response = await exchangeCode( + config, + "auth-code", + "verifier", + fakeFetch({ + access_token: "access-kept", + refresh_token: "", + expires_in: 60, + }), + ); + expect(response.refresh_token).toBeUndefined(); + expect(response.access_token).toBe("access-kept"); + }); + + test("does not post a blank refresh token", async () => { + let calls = 0; + const impl: FetchLike = async () => { + calls += 1; + return new Response( + JSON.stringify({ access_token: "access-must-not-leak" }), + { status: 200 }, + ); + }; + impl.preconnect = () => undefined; + const error = await refreshTokenRequest(config, " ", impl).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(calls).toBe(0); + expect(error).toBeInstanceOf(OAuthMissingRefreshTokenError); + expect(String(error)).not.toContain("access-must-not-leak"); + }); }); describe("buildAuthorizeUrl — protected parameters", () => { diff --git a/src/client.ts b/src/client.ts index c4fa826..1bbdd5f 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,7 +1,7 @@ import { type } from "arktype"; import type { Pkce } from "./pkce.js"; -import type { BaseTokens } from "./tokens.js"; +import { presentRefreshToken, type BaseTokens } from "./tokens.js"; // Provider-agnostic OAuth client config. Endpoints, client id, scopes, and // timeouts are supplied by the caller; this module owns only the shared @@ -121,9 +121,9 @@ export class OAuthMissingRefreshTokenError extends Error { * Convert a token response to the shared base token fields. `now` is * injectable so callers (and tests) control the expiry baseline; * `previousRefresh` is carried forward when a refresh response omits a new - * refresh_token (servers may rotate or not). When neither is present the - * result simply has no `refresh`: an access-token-only grant is valid and - * must not be rejected or padded with a made-up refresh token. + * refresh_token (servers may rotate or not). A blank refresh token is absent, + * the same as an omitted one. When neither the response nor the previous + * grant has a usable refresh token, the result simply has no `refresh`. * * `expires_in` is RECOMMENDED, not required, on a token response (RFC 6749 * §5.1). When a server omits it there is no spec-defined lifetime to fall @@ -135,7 +135,9 @@ export function baseTokensFromResponse( now: number, previousRefresh: string | undefined, ): BaseTokens { - const refresh = response.refresh_token ?? previousRefresh; + const refresh = + presentRefreshToken(response.refresh_token) ?? + presentRefreshToken(previousRefresh); return { access: response.access_token, ...(refresh !== undefined ? { refresh } : {}), @@ -225,7 +227,17 @@ async function postToken( const json = TokenResponse(payload); if (json instanceof type.errors) throw new OAuthTokenResponseSchemaError(json.summary); - return json; + return omitBlankRefresh(json); +} + +// `refresh_token: ""` parses as a string, but it is not a credential. Drop it +// at the boundary so later stages see the same shape as an omitted field. +function omitBlankRefresh(response: TokenResponse): TokenResponse { + if (presentRefreshToken(response.refresh_token) !== undefined) + return response; + if (response.refresh_token === undefined) return response; + const { refresh_token: _blank, ...rest } = response; + return rest; } /** @@ -258,6 +270,8 @@ export async function refreshTokenRequest( refreshToken: string, fetchImpl: FetchLike = fetch, ): Promise { + if (presentRefreshToken(refreshToken) === undefined) + throw new OAuthMissingRefreshTokenError(); const body = new URLSearchParams({ grant_type: "refresh_token", refresh_token: refreshToken, diff --git a/src/hub/credentials.test.ts b/src/hub/credentials.test.ts index 1465a30..417217c 100644 --- a/src/hub/credentials.test.ts +++ b/src/hub/credentials.test.ts @@ -37,4 +37,39 @@ describe("writeOAuthTokens", () => { { secret: "sealed:access-only", refreshSecret: null }, ]); }); + + test("does not seal a blank refresh token", async () => { + // Load-bearing: `""` and `" "` are strings, so they would be encrypted and + // later selected for refresh. Blank is the same as no refresh token. + const encrypted: string[] = []; + const cipherWithLog: CredentialCipher = { + encrypt: (plaintext) => { + encrypted.push(plaintext); + return Promise.resolve(`sealed:${plaintext}`); + }, + decrypt: () => Promise.reject(new Error("unused")), + }; + const written: { refreshSecret: string | null }[] = []; + const db = { + update: () => ({ + set: (values: { refreshSecret: string | null }) => ({ + where: () => { + written.push({ refreshSecret: values.refreshSecret }); + return Promise.resolve(); + }, + }), + }), + }; + for (const refresh of ["", " "]) { + await writeOAuthTokens({ + db: db as unknown as OAuthCredentialWriter, + cipher: cipherWithLog, + credentialId: "cred_1", + tokens: { access: "access-kept", refresh }, + metadata: {}, + }); + } + expect(written).toEqual([{ refreshSecret: null }, { refreshSecret: null }]); + expect(encrypted).toEqual(["access-kept", "access-kept"]); + }); }); diff --git a/src/hub/credentials.ts b/src/hub/credentials.ts index 2bac57f..9171ac2 100644 --- a/src/hub/credentials.ts +++ b/src/hub/credentials.ts @@ -8,7 +8,7 @@ import { generateId } from "@intx/hub-common"; import { credentialAad, type CredentialCipher } from "@intx/types"; import { and, eq } from "drizzle-orm"; -import type { BaseTokens } from "../index.js"; +import { presentRefreshToken, type BaseTokens } from "../tokens.js"; /** * Credential-metadata key naming the registered provider the tokens were @@ -41,11 +41,12 @@ async function sealTokenSecrets( tokens.access, credentialAad(credentialId, "secret"), ); - if (tokens.refresh === undefined) return { secret, refreshSecret: null }; + const refresh = presentRefreshToken(tokens.refresh); + if (refresh === undefined) return { secret, refreshSecret: null }; return { secret, refreshSecret: await cipher.encrypt( - tokens.refresh, + refresh, credentialAad(credentialId, "refreshSecret"), ), }; diff --git a/src/hub/refresh.test.ts b/src/hub/refresh.test.ts index 3754d2d..7b8a0ec 100644 --- a/src/hub/refresh.test.ts +++ b/src/hub/refresh.test.ts @@ -26,6 +26,7 @@ function fakeStore(row: { provider: string; expiresAt: Date | null; metadata?: Record; + refreshSecret?: string; }) { const written: { tokens: BaseTokens; metadata: Record }[] = []; @@ -40,7 +41,7 @@ function fakeStore(row: { id: row.id, tenantId: row.tenantId, expiresAt: row.expiresAt, - refreshSecret: "refresh-token", + refreshSecret: row.refreshSecret ?? "refresh-token", metadata: row.metadata ?? {}, }); if (result === null) return false; @@ -179,6 +180,37 @@ describe("createRefreshTicker", () => { expect(written).toEqual([]); expect(errors).toEqual([]); }); + + it("does not refresh a blank refresh token", async () => { + // Load-bearing: an encrypted `""` or `" "` is non-null, so listDue would + // select it and the ticker would redeem it. Blank is not a credential, + // and the skip must not disclose an access token. + const access = "access-must-not-leak"; + for (const refreshSecret of ["", " "]) { + const { store, written } = fakeStore({ + id: "cred_1", + tenantId: "tenant_1", + provider: "acme", + expiresAt: new Date(Date.now() + MINUTE), + refreshSecret, + }); + const calls: string[] = []; + const errors: unknown[] = []; + await tickOnce(store, { + providers: providers(() => { + calls.push(access); + return Promise.resolve({ access, refresh: "next" }); + }), + marginMs: 10 * MINUTE, + onError: (error) => { + errors.push(error); + }, + }); + expect(calls).toEqual([]); + expect(written).toEqual([]); + expect(errors.map(String).join("\n")).not.toContain(access); + } + }); }); describe("a credential of an unregistered provider", () => { diff --git a/src/hub/refresh.ts b/src/hub/refresh.ts index e29589d..aa5a698 100644 --- a/src/hub/refresh.ts +++ b/src/hub/refresh.ts @@ -11,6 +11,7 @@ import { refreshTokenRequest, type BaseTokens, } from "../index.js"; +import { presentRefreshToken } from "../tokens.js"; import { OAUTH_CLIENT_ID_METADATA_KEY, OAUTH_PROVIDER_METADATA_KEY, @@ -174,8 +175,9 @@ export async function refreshCredential( if (!dueAt(row.expiresAt, deadline)) return null; const refresh = provider?.refresh ?? registeredClientRefresh(row.metadata); - if (refresh === undefined) return null; - const tokens = await refresh(row.refreshSecret, Date.now()); + const refreshSecret = presentRefreshToken(row.refreshSecret); + if (refresh === undefined || refreshSecret === undefined) return null; + const tokens = await refresh(refreshSecret, Date.now()); // The prior metadata is carried forward: a refresh response may omit // what the first exchange established (an account id, say). return { @@ -264,10 +266,21 @@ export function createOAuthRefreshStore(opts: { `credential ${credentialId} has unreadable metadata: ${metadata.summary}`, ); } - const refreshSecret = await opts.cipher.decrypt( - row.refreshSecret, - credentialAad(row.id, "refreshSecret"), + const refreshSecret = presentRefreshToken( + await opts.cipher.decrypt( + row.refreshSecret, + credentialAad(row.id, "refreshSecret"), + ), ); + if (refreshSecret === undefined) { + // Blank plaintext is not a credential. Null is what listDue treats + // as absent, so this row is not selected again. + await tx + .update(credential) + .set({ refreshSecret: null, updatedAt: new Date() }) + .where(eq(credential.id, credentialId)); + return false; + } let result; try { result = await refresh({ diff --git a/src/session.test.ts b/src/session.test.ts index 289f782..2a8dd20 100644 --- a/src/session.test.ts +++ b/src/session.test.ts @@ -69,4 +69,39 @@ describe("Token session createTokenSession — refresh coalescing", () => { ).toBeInstanceOf(OAuthMissingRefreshTokenError); expect(refreshCount).toBe(0); }); + + test("treats a blank refresh token as absent and does not disclose the access token", async () => { + // Load-bearing: `""` and `" "` are present to a nullish check, so expiry + // would redeem them. The failure must not echo the access token. + const access = "access-must-not-leak"; + for (const refresh of ["", " "]) { + let refreshCount = 0; + const stored: { tokens: BaseTokens } = { + tokens: { access, refresh, expiresAt: 1_000 }, + }; + const session = createTokenSession({ + skewMs: 0, + loadProfile: async () => stored, + updateTokens: async () => undefined, + refreshTokens: async () => { + refreshCount += 1; + return { access: "should-not-run" }; + }, + toAccess: (tokens) => tokens.access, + }); + const error = await session.getValidToken("p", 1_000).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(OAuthRefreshFailedError); + expect( + error instanceof OAuthRefreshFailedError ? error.cause : undefined, + ).toBeInstanceOf(OAuthMissingRefreshTokenError); + expect(refreshCount).toBe(0); + expect(String(error)).not.toContain(access); + expect(error instanceof Error ? String(error.cause) : "").not.toContain( + access, + ); + } + }); }); diff --git a/src/session.ts b/src/session.ts index debe7b4..50bf5d3 100644 --- a/src/session.ts +++ b/src/session.ts @@ -1,5 +1,5 @@ import { OAuthMissingRefreshTokenError } from "./client.js"; -import type { BaseTokens } from "./tokens.js"; +import { presentRefreshToken, type BaseTokens } from "./tokens.js"; /** * Whether `tokens` is at or within `skewMs` of its expiry at `now`. @@ -72,7 +72,7 @@ export function createTokenSession( // Re-check expiry after the I/O; another caller may have refreshed already. if (!isExpired(profile.tokens, now)) return deps.toAccess(profile.tokens); let refreshed: TTokens; - const refreshToken = profile.tokens.refresh; + const refreshToken = presentRefreshToken(profile.tokens.refresh); if (refreshToken === undefined) { throw new OAuthRefreshFailedError( name, diff --git a/src/tokens.ts b/src/tokens.ts index 048abea..b46740f 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -2,8 +2,8 @@ // (Interchange `oauth_token` credential, OS vault, or otherwise). // // `refresh` is optional because an authorization server may issue an access -// token and no refresh token (RFC 6749 §5.1). Absence means a new -// authorization is required at expiry — this package does not invent one. +// token and no refresh token (RFC 6749 §5.1). A blank value is the same +// absence: it is not a credential and must not be stored or redeemed. // // `expiresAt` is optional because RFC 6749 §5.1 makes `expires_in` RECOMMENDED, // not required — a token endpoint may omit it, and this package refuses to @@ -21,3 +21,16 @@ export type AuthProfile = { tokens: TTokens; createdAt: number; }; + +/** + * A refresh token the caller can redeem. Missing, null, and whitespace-only + * values are absent — `""` and `" "` must not be stored or sent as if they + * were a credential. + */ +export function presentRefreshToken( + token: string | null | undefined, +): string | undefined { + if (token == null) return undefined; + if (token.trim().length === 0) return undefined; + return token; +} From 84756a931ea3920dde0d0315743cb9ea8f64f602 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:40:11 -0700 Subject: [PATCH 4/6] fix: parse oauth error codes before truncating the body A long error_description used to cut the JSON before parse, so invalid_grant was lost and the refresh row stayed active. The public detail is still only the error code. --- src/client.ts | 22 +++++++++++++++--- src/hub/refresh.test.ts | 49 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/client.ts b/src/client.ts index 1bbdd5f..c21622e 100644 --- a/src/client.ts +++ b/src/client.ts @@ -155,6 +155,11 @@ export type FetchLike = typeof fetch; const OAUTH_ERROR_CODE = /^[a-z0-9_]{1,64}$/; function oauthErrorCode(body: string): string { + const parsed = errorCodeFromJson(body) ?? errorCodeFromPrefix(body); + return parsed ?? ""; +} + +function errorCodeFromJson(body: string): string | undefined { try { const parsed: unknown = JSON.parse(body); if ( @@ -167,9 +172,20 @@ function oauthErrorCode(body: string): string { return parsed.error; } } catch { - // Non-JSON bodies contribute no public detail. + // Truncated or non-JSON bodies fall through to a prefix scan. The scan + // still returns only the error code, never error_description. } - return ""; + return undefined; +} + +// A long error_description used to be sliced to 4096 bytes before JSON.parse, +// so the object no longer parsed and invalid_grant was dropped. The code is a +// small token and can be read without the rest of the document. +function errorCodeFromPrefix(body: string): string | undefined { + const match = /"error"\s*:\s*"([a-z0-9_]{1,64})"/.exec(body); + const code = match?.[1]; + if (code !== undefined && OAUTH_ERROR_CODE.test(code)) return code; + return undefined; } function isRedirectResponse(res: Response): boolean { @@ -212,7 +228,7 @@ async function postToken( if (!res.ok) { let detail = ""; try { - detail = oauthErrorCode((await res.text()).slice(0, 4096)); + detail = oauthErrorCode(await res.text()); } catch { // Non-2xx is the error; the body is not public detail. } diff --git a/src/hub/refresh.test.ts b/src/hub/refresh.test.ts index 7b8a0ec..5a68f40 100644 --- a/src/hub/refresh.test.ts +++ b/src/hub/refresh.test.ts @@ -303,6 +303,55 @@ describe("refreshCredential", () => { expect(!result.ok && result.reason).toBe("error"); expect(written).toEqual([]); }); + + it("reports reauth when invalid_grant carries a long error_description", async () => { + // Load-bearing: slicing the body to 4096 bytes before JSON.parse drops + // invalid_grant once error_description runs past the cut, so the row stays + // active and is retried forever. The description itself is not public. + const description = "d".repeat(5000); + const { store, written } = fakeStore({ + id: "cred_1", + tenantId: "tenant_1", + provider: "https://mcp.example.com", + expiresAt: new Date(Date.now() + MINUTE), + metadata: { + oauthClientId: "dyn-client", + oauthTokenUrl: "https://auth.example.com/token", + }, + }); + const realFetch = globalThis.fetch; + globalThis.fetch = Object.assign( + () => + Promise.resolve( + new Response( + JSON.stringify({ + error: "invalid_grant", + error_description: description, + }), + { status: 400 }, + ), + ), + { preconnect: () => undefined }, + ); + let result: Awaited>; + try { + result = await refreshCredential( + store, + { providers: {}, marginMs: 10 * MINUTE }, + { + id: "cred_1", + tenantId: "tenant_1", + provider: "https://mcp.example.com", + }, + ); + } finally { + globalThis.fetch = realFetch; + } + expect(result).toMatchObject({ ok: false, reason: "reauth" }); + expect(!result.ok && result.message).toContain("invalid_grant"); + expect(!result.ok && result.message).not.toContain(description); + expect(written).toEqual([]); + }); }); describe("createRefreshTicker start()", () => { From 4ba932f1cacb3eb67bd5239ad8109bc17a7a1359 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:40:11 -0700 Subject: [PATCH 5/6] fix: canonicalize client metadata urls Reject backslash dot-segments and use the URL a fetch will request as the client id, including host case and the default https port. --- src/discovery.test.ts | 27 +++++++++++++++++++++++++++ src/discovery.ts | 16 +++++++++++----- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/discovery.test.ts b/src/discovery.test.ts index 183cd25..eb426a4 100644 --- a/src/discovery.test.ts +++ b/src/discovery.test.ts @@ -876,4 +876,31 @@ describe("clientRegistrationPlan", () => { }), ).toThrow(OAuthDiscoveryError); }); + + test("rejects a backslash dot-segment and returns one canonical client id", () => { + // Load-bearing: the URL parser treats `\` as `/`, so `a\..\client.json` + // is fetched as `/client.json`. Host case and an explicit :443 are the + // same URL a fetch requests, and must be the client id too. + expect(() => + clientRegistrationPlan({ + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: "https://hub.example.com/a\\..\\client.json", + }), + ).toThrow(OAuthDiscoveryError); + expect(() => + clientRegistrationPlan({ + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: "https://hub.example.com\\..\\client.json", + }), + ).toThrow(OAuthDiscoveryError); + expect( + clientRegistrationPlan({ + clientIdMetadataDocumentSupported: true, + clientMetadataUrl: "https://HUB.example.com:443/oauth/client.json", + }), + ).toEqual({ + mechanism: "client-id-metadata", + clientId: "https://hub.example.com/oauth/client.json", + }); + }); }); diff --git a/src/discovery.ts b/src/discovery.ts index 5d8b726..7f0b087 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -450,7 +450,7 @@ function grantedScope( return echoed ?? requested; } -function assertClientMetadataUrl(raw: string): void { +function canonicalClientMetadataUrl(raw: string): string { let url: URL; try { url = new URL(raw); @@ -466,15 +466,22 @@ function assertClientMetadataUrl(raw: string): void { "client metadata URL must not contain a fragment or userinfo", ); const afterScheme = raw.slice(raw.indexOf("://") + 3); - const pathStart = afterScheme.indexOf("/"); + // `\` is a path separator for the URL parser, same as `/`. Splitting only on + // `/` lets `a\..\client.json` through, while `new URL` fetches `/client.json`. + const pathStart = afterScheme.search(/[\\/]/); const rawPath = pathStart === -1 ? "" : (afterScheme.slice(pathStart).split(/[?#]/, 1)[0] ?? ""); - if (rawPath.split("/").some((segment) => /^(?:\.|%2e){1,2}$/i.test(segment))) + if ( + rawPath.split(/[\\/]/).some((segment) => /^(?:\.|%2e){1,2}$/i.test(segment)) + ) throw new OAuthDiscoveryError( "client metadata URL must not contain dot segments", ); + // One identifier for the client id and the URL a fetch will request: host + // case and the default https port are not distinct from the canonical URL. + return url.href; } /** @@ -502,10 +509,9 @@ export function clientRegistrationPlan(opts: { opts.clientIdMetadataDocumentSupported === true && opts.clientMetadataUrl !== undefined ) { - assertClientMetadataUrl(opts.clientMetadataUrl); return { mechanism: "client-id-metadata", - clientId: opts.clientMetadataUrl, + clientId: canonicalClientMetadataUrl(opts.clientMetadataUrl), }; } if (opts.registrationEndpoint !== undefined) { From 7dcb4f1aec182809c3e14d363f52825fb946b72f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 25 Oct 2026 16:18:21 -0700 Subject: [PATCH 6/6] refactor: simplify new grant and callback helpers --- src/client.ts | 19 +++++-------------- src/discovery.ts | 12 ++++++------ src/hub/mount.ts | 27 ++++++--------------------- src/hub/refresh.ts | 3 +-- src/hub/route-callback.ts | 11 +++-------- 5 files changed, 21 insertions(+), 51 deletions(-) diff --git a/src/client.ts b/src/client.ts index c21622e..462a9c7 100644 --- a/src/client.ts +++ b/src/client.ts @@ -42,11 +42,6 @@ const PROTECTED_AUTHORIZE_PARAMS = new Set([ * Build the authorization URL the user opens to grant access. The challenge * binds this request to the PKCE verifier held locally; `state` is the CSRF * nonce the redirect must echo back unchanged. - * - * Caller `extraAuthorizeParams` are applied first and cannot replace the - * protected fields this function sets (PKCE, state, client, redirect, scope, - * resource, response type). An empty scope list omits `scope` rather than - * sending an empty value that would override the server default. */ export function buildAuthorizeUrl( config: OAuthClientConfig, @@ -188,14 +183,6 @@ function errorCodeFromPrefix(body: string): string | undefined { return undefined; } -function isRedirectResponse(res: Response): boolean { - return ( - res.type === "opaqueredirect" || - res.redirected || - (res.status >= 300 && res.status < 400) - ); -} - // `fetchImpl` is a required parameter here, not defaulted to the global — // only the exported entry points (exchangeCode, refreshTokenRequest) default // to the real `fetch`; an internal helper reaching for the global itself @@ -222,7 +209,11 @@ async function postToken( redirect: "manual", signal: AbortSignal.timeout(config.tokenTimeoutMs), }); - if (isRedirectResponse(res)) { + if ( + res.type === "opaqueredirect" || + res.redirected || + (res.status >= 300 && res.status < 400) + ) { throw new OAuthTokenEndpointError(res.status, "redirect_refused"); } if (!res.ok) { diff --git a/src/discovery.ts b/src/discovery.ts index 7f0b087..305ce51 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -488,7 +488,7 @@ function canonicalClientMetadataUrl(raw: string): string { * How a resource login obtains a client id. A pre-registered id wins, then a * Client ID Metadata Document when the server advertises it and the host * supplied a document URL, then RFC 7591. `undefined` means none of those - * is available — dynamic registration is not required when another applies. + * is available. */ export type ClientRegistrationPlan = | { readonly mechanism: "pre-registered"; readonly clientId: string } @@ -496,13 +496,13 @@ export type ClientRegistrationPlan = | { readonly mechanism: "dynamic"; readonly registrationEndpoint: string }; export function clientRegistrationPlan(opts: { - readonly registrationEndpoint?: string; - readonly clientIdMetadataDocumentSupported?: boolean; - readonly clientMetadataUrl?: string; - readonly preRegisteredClientId?: string; + readonly registrationEndpoint?: string | undefined; + readonly clientIdMetadataDocumentSupported?: boolean | undefined; + readonly clientMetadataUrl?: string | undefined; + readonly preRegisteredClientId?: string | undefined; }): ClientRegistrationPlan | undefined { const preRegistered = opts.preRegisteredClientId?.trim(); - if (preRegistered !== undefined && preRegistered.length > 0) { + if (preRegistered) { return { mechanism: "pre-registered", clientId: preRegistered }; } if ( diff --git a/src/hub/mount.ts b/src/hub/mount.ts index 30c1ad1..4f0789e 100644 --- a/src/hub/mount.ts +++ b/src/hub/mount.ts @@ -98,12 +98,7 @@ export function mountOAuthCallback( opts: { readonly store: OAuthLoginStore; readonly path?: string }, ): void { app.get(opts.path ?? "/oauth/callback", (c) => { - const query = (key: string): string | undefined => { - const values = c.req.queries(key); - return values === undefined || values.length === 0 - ? undefined - : values[0]; - }; + const query = (key: string): string | undefined => c.req.queries(key)?.[0]; const repeated = [ "state", "code", @@ -252,21 +247,11 @@ export function mountOAuthLogin( ...(opts.fetchImpl !== undefined ? { fetchImpl: opts.fetchImpl } : {}), }); const plan = clientRegistrationPlan({ - ...(entry.authorizationServer.registrationEndpoint !== undefined - ? { - registrationEndpoint: - entry.authorizationServer.registrationEndpoint, - } - : {}), - ...(entry.authorizationServer.clientIdMetadataDocumentSupported === true - ? { clientIdMetadataDocumentSupported: true } - : {}), - ...(opts.clientMetadataUrl !== undefined - ? { clientMetadataUrl: opts.clientMetadataUrl } - : {}), - ...(body.clientId !== undefined - ? { preRegisteredClientId: body.clientId } - : {}), + registrationEndpoint: entry.authorizationServer.registrationEndpoint, + clientIdMetadataDocumentSupported: + entry.authorizationServer.clientIdMetadataDocumentSupported, + clientMetadataUrl: opts.clientMetadataUrl, + preRegisteredClientId: body.clientId, }); if (plan === undefined) { return { diff --git a/src/hub/refresh.ts b/src/hub/refresh.ts index aa5a698..94e78af 100644 --- a/src/hub/refresh.ts +++ b/src/hub/refresh.ts @@ -236,8 +236,7 @@ export function createOAuthRefreshStore(opts: { and( eq(credential.type, "oauth_token"), eq(credential.status, "active"), - // No refresh token was issued: there is nothing to redeem. The - // row stays until the host starts a new login; it is not due. + // Access-token-only rows have nothing to redeem, so they are not due. isNotNull(credential.refreshSecret), isNotNull(credential.expiresAt), lte(credential.expiresAt, dueBefore), diff --git a/src/hub/route-callback.ts b/src/hub/route-callback.ts index 8c27b93..a9703bf 100644 --- a/src/hub/route-callback.ts +++ b/src/hub/route-callback.ts @@ -14,10 +14,7 @@ export type CallbackParams = { errorDescription?: string | undefined; /** RFC 9207 issuer identifier, when the authorization server sends one. */ iss?: string | undefined; - /** - * A callback parameter was repeated. Refused before `code` or `error` is - * acted on, so a duplicated field cannot pick which value wins. - */ + /** Repeated query parameter. Refused before `code` or `error` is used. */ repeatedParameter?: boolean; }; @@ -114,10 +111,8 @@ export function createRouteCallbacks(): RouteCallbacks { }); return { status: 400, html: signInFailedHtml(failure) }; }; - // Provenance before the grant: a repeated parameter, or an `iss` that - // is not the issuer recorded with this state, is not this login's - // redirect. RFC 9207 §2.4 — a present `iss` is always compared; a - // missing one is refused only when the server advertised it. + // RFC 9207: a present `iss` must be the issuer stored with this state. + // A missing `iss` fails only when the server advertised the parameter. if (repeatedParameter === true) return failed({ code: "state_mismatch" }); if (entry.expectedIssuer !== undefined) { if (iss === undefined) {