diff --git a/README.md b/README.md index c22a9a6..d71bd1a 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,20 @@ manager.registerGlobally() const response = await fetch(requestUri) ``` +### Pin the providers' own OIDC requests to a pristine `fetch` + +The token providers perform their own network requests (discovery, dynamic client registration, the token grant) which default to `globalThis.fetch`. If the application patches the global `fetch` with an authenticating wrapper — `registerGlobally()`, or its own wrapper that single-flights concurrent requests onto one shared authentication attempt — those OIDC requests re-enter the wrapper mid-login. A single-flighting wrapper then awaits the very authentication attempt its request is serving: a circular await that hangs login before the authorization popup/redirect ever opens. + +To avoid this, capture the pristine `fetch` before patching and pin the providers to it: + +```js +const pristineFetch = globalThis.fetch.bind(globalThis) // capture BEFORE patching + +const provider = new DPoPTokenProvider(callbackUri, getCode, getIssuer, {fetch: pristineFetch}) +const manager = new ReactiveFetchManager([provider]) +manager.registerGlobally() +``` + ## Run the demo To compile, diff --git a/package.json b/package.json index db6dd7a..82cffe6 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,8 @@ "url": "git+https://github.com/solid-contrib/reactive-authentication.git" }, "scripts": { - "build": "tsc" + "build": "tsc", + "test": "npm run build && node --test --experimental-strip-types \"test/**/*.test.ts\"" }, "license": "MIT", "dependencies": { diff --git a/src/BearerTokenProvider.ts b/src/BearerTokenProvider.ts index 051af48..f7d3a88 100644 --- a/src/BearerTokenProvider.ts +++ b/src/BearerTokenProvider.ts @@ -1,15 +1,19 @@ import * as oauth from "oauth4webapi" import { GetCodeCallback } from "./GetCodeCallback.js" import { TokenProvider } from "./TokenProvider.js" +import type { TokenProviderOptions } from "./TokenProviderOptions.js" +import { customFetchOption } from "./customFetchOption.js" // TODO: Configure properly for insecure localhost only const oauthAllowInsecureRequests = true export class BearerTokenProvider implements TokenProvider { readonly #getCode: GetCodeCallback + readonly #fetch: typeof globalThis.fetch | undefined - constructor(getCodeCallback: GetCodeCallback) { + constructor(getCodeCallback: GetCodeCallback, options?: TokenProviderOptions) { this.#getCode = getCodeCallback + this.#fetch = options?.fetch } async #getIssuer(request: Request): Promise { @@ -43,12 +47,16 @@ export class BearerTokenProvider implements TokenProvider { async upgrade(request: Request): Promise { const issuer = await this.#getIssuer(request) - const discoveryResponse = await oauth.discoveryRequest(issuer, {[oauth.allowInsecureRequests]: oauthAllowInsecureRequests}) + // Pin the provider's own OIDC requests to the configured fetch (when given) + // so they never re-enter an authenticating wrapper patched over the global. + const oidcRequestOptions = {[oauth.allowInsecureRequests]: oauthAllowInsecureRequests, ...customFetchOption(this.#fetch)} + + const discoveryResponse = await oauth.discoveryRequest(issuer, oidcRequestOptions) const authorizationServer = await oauth.processDiscoveryResponse(issuer, discoveryResponse) const callbackUri = await this.#getCallback(request) - const registrationResponse = await oauth.dynamicClientRegistrationRequest(authorizationServer, {redirect_uris: [callbackUri]}, {[oauth.allowInsecureRequests]: oauthAllowInsecureRequests}) + const registrationResponse = await oauth.dynamicClientRegistrationRequest(authorizationServer, {redirect_uris: [callbackUri]}, oidcRequestOptions) const clientRegistration = await oauth.processDynamicClientRegistrationResponse(registrationResponse) const [registeredRedirectUri] = clientRegistration.redirect_uris as string[] const [registeredResponseType] = clientRegistration.response_types as string[] @@ -84,7 +92,7 @@ export class BearerTokenProvider implements TokenProvider { clientAuth = authenticationMethod(clientSecret) } - const tokenResponse = await oauth.authorizationCodeGrantRequest(authorizationServer, clientRegistration, clientAuth, authorizationCodeParams, callbackUri, codeVerifier, {[oauth.allowInsecureRequests]: oauthAllowInsecureRequests}) + const tokenResponse = await oauth.authorizationCodeGrantRequest(authorizationServer, clientRegistration, clientAuth, authorizationCodeParams, callbackUri, codeVerifier, oidcRequestOptions) // jwt nonce missing in igrant // const tokenResult = await oauth.processAuthorizationCodeResponse(authorizationServer, clientRegistration, tokenResponse, {expectedNonce: nonce}) diff --git a/src/ClientCredentialsTokenProvider.ts b/src/ClientCredentialsTokenProvider.ts index 2bc32b5..5e7ea9f 100644 --- a/src/ClientCredentialsTokenProvider.ts +++ b/src/ClientCredentialsTokenProvider.ts @@ -2,9 +2,14 @@ import * as oauth from "oauth4webapi" import { AuthorizationServer } from "oauth4webapi" import * as DPoP from "dpop" import type { TokenProvider } from "./TokenProvider.js" +import type { TokenProviderOptions } from "./TokenProviderOptions.js" +import { customFetchOption } from "./customFetchOption.js" export class ClientCredentialsTokenProvider implements TokenProvider { - constructor(private clientId: string, private clientSecret: string) { + readonly #fetch: typeof globalThis.fetch | undefined + + constructor(private clientId: string, private clientSecret: string, options?: TokenProviderOptions) { + this.#fetch = options?.fetch } async #getIssuer(request: Request): Promise { @@ -34,9 +39,11 @@ export class ClientCredentialsTokenProvider implements TokenProvider { async upgrade(request: Request): Promise { const issuer = await this.#getIssuer(request) - const discoveryResponse = await oauth.discoveryRequest(issuer, { - signal: request.signal - }) + // Pin the provider's own OIDC requests to the configured fetch (when given) + // so they never re-enter an authenticating wrapper patched over the global. + const oidcRequestOptions = {signal: request.signal, ...customFetchOption(this.#fetch)} + + const discoveryResponse = await oauth.discoveryRequest(issuer, oidcRequestOptions) const authorizationServer = await oauth.processDiscoveryResponse(issuer, discoveryResponse) const clientRegistration: oauth.Client = {client_id: this.clientId, client_secret: this.clientSecret} @@ -46,7 +53,7 @@ export class ClientCredentialsTokenProvider implements TokenProvider { const tokenResponse = await oauth.clientCredentialsGrantRequest(authorizationServer, clientRegistration, this.getClientAuth(authorizationServer, clientRegistration), {scope: "webid"}, { DPoP: dpop, - signal: request.signal + ...oidcRequestOptions }) const tokenResult = await oauth.processClientCredentialsResponse(authorizationServer, clientRegistration, tokenResponse) diff --git a/src/DPoPTokenProvider.ts b/src/DPoPTokenProvider.ts index 4760ae2..4dcb3ce 100644 --- a/src/DPoPTokenProvider.ts +++ b/src/DPoPTokenProvider.ts @@ -3,16 +3,20 @@ import * as DPoP from "dpop" import type { GetCodeCallback } from "./GetCodeCallback.js" import type { TokenProvider } from "./TokenProvider.js" import type { GetIssuerCallback } from "./GetIssuerCallback.js" +import type { TokenProviderOptions } from "./TokenProviderOptions.js" +import { customFetchOption } from "./customFetchOption.js" export class DPoPTokenProvider implements TokenProvider { readonly #getCode: GetCodeCallback readonly #callbackUri: string readonly #getIssuer: GetIssuerCallback + readonly #fetch: typeof globalThis.fetch | undefined - constructor(callbackUri: string, getCodeCallback: GetCodeCallback, getIssuerCallback: GetIssuerCallback) { + constructor(callbackUri: string, getCodeCallback: GetCodeCallback, getIssuerCallback: GetIssuerCallback, options?: TokenProviderOptions) { this.#getCode = getCodeCallback this.#callbackUri = callbackUri this.#getIssuer = getIssuerCallback + this.#fetch = options?.fetch } async matches(request: Request): Promise { @@ -22,10 +26,14 @@ export class DPoPTokenProvider implements TokenProvider { async upgrade(request: Request): Promise { const issuer = await this.#getIssuer(request) - const discoveryResponse = await oauth.discoveryRequest(issuer, {signal: request.signal}) + // Pin the provider's own OIDC requests to the configured fetch (when given) + // so they never re-enter an authenticating wrapper patched over the global. + const oidcRequestOptions = {signal: request.signal, ...customFetchOption(this.#fetch)} + + const discoveryResponse = await oauth.discoveryRequest(issuer, oidcRequestOptions) const authorizationServer = await oauth.processDiscoveryResponse(issuer, discoveryResponse) - const registrationResponse = await oauth.dynamicClientRegistrationRequest(authorizationServer, {redirect_uris: [this.#callbackUri]}, {signal: request.signal}) + const registrationResponse = await oauth.dynamicClientRegistrationRequest(authorizationServer, {redirect_uris: [this.#callbackUri]}, oidcRequestOptions) const clientRegistration = await oauth.processDynamicClientRegistrationResponse(registrationResponse) const [registeredRedirectUri] = clientRegistration.redirect_uris as string[] const [registeredResponseType] = clientRegistration.response_types as string[] @@ -79,7 +87,7 @@ export class DPoPTokenProvider implements TokenProvider { } } - const tokenResponse = await oauth.authorizationCodeGrantRequest(authorizationServer, clientRegistration, this.getClientAuth(authorizationServer.issuer, clientRegistration), authorizationCodeParams, this.#callbackUri, authorizationServer.code_challenge_methods_supported !== undefined ? codeVerifier : oauth.nopkce, {DPoP: dpop, signal: request.signal}) + const tokenResponse = await oauth.authorizationCodeGrantRequest(authorizationServer, clientRegistration, this.getClientAuth(authorizationServer.issuer, clientRegistration), authorizationCodeParams, this.#callbackUri, authorizationServer.code_challenge_methods_supported !== undefined ? codeVerifier : oauth.nopkce, {DPoP: dpop, ...oidcRequestOptions}) const tokenResult = await oauth.processAuthorizationCodeResponse(authorizationServer, clientRegistration, tokenResponse, {expectedNonce: this.nonceVerificationOverride(authorizationServer.issuer, nonce)}) diff --git a/src/TokenProviderOptions.ts b/src/TokenProviderOptions.ts new file mode 100644 index 0000000..d626515 --- /dev/null +++ b/src/TokenProviderOptions.ts @@ -0,0 +1,23 @@ +/** + * Options accepted by the token providers. + */ +export interface TokenProviderOptions { + /** + * The fetch implementation the provider uses for its own OIDC requests — + * discovery, dynamic client registration and the token grant. + * + * @remarks + * Defaults to `globalThis.fetch`, resolved at call time (oauth4webapi's + * default behaviour). + * + * Applications that patch the global fetch with an authenticating wrapper + * (`ReactiveFetchManager.registerGlobally()`, or their own wrapper that + * single-flights concurrent requests onto one shared authentication + * attempt) should pass the pristine, pre-patch fetch here. Otherwise the + * provider's own OIDC requests re-enter the wrapper mid-upgrade: a + * single-flighting wrapper then awaits the very authentication attempt + * those requests are serving — a circular await that hangs login before + * the authorization redirect/popup ever opens. + */ + fetch?: typeof globalThis.fetch +} diff --git a/src/customFetchOption.ts b/src/customFetchOption.ts new file mode 100644 index 0000000..e133996 --- /dev/null +++ b/src/customFetchOption.ts @@ -0,0 +1,27 @@ +import * as oauth from "oauth4webapi" + +/** + * Adapts an optional Fetch API implementation to oauth4webapi's `customFetch` + * request option, as a spreadable options fragment. + * + * @remarks + * Returns an empty fragment when no fetch is given, preserving oauth4webapi's + * default (`globalThis.fetch`, resolved at call time). + * + * @see {@link TokenProviderOptions.fetch} + */ +export function customFetchOption(fetchImplementation: typeof globalThis.fetch | undefined): {[oauth.customFetch]?: (url: string, options: oauth.CustomFetchOptions) => Promise} { + if (fetchImplementation === undefined) { + return {} + } + + return { + [oauth.customFetch]: (url, options) => fetchImplementation(url, { + method: options.method, + headers: options.headers, + body: options.body ?? null, + redirect: options.redirect, + signal: options.signal ?? null, + }), + } +} diff --git a/src/mod.ts b/src/mod.ts index 0342cf6..e58915e 100644 --- a/src/mod.ts +++ b/src/mod.ts @@ -9,6 +9,7 @@ export * from "./ClientCredentialsTokenProvider.js" export * from "./GetCodeCallback.js" export * from "./issuerFrom.js" export * from "./TokenProvider.js" +export * from "./TokenProviderOptions.js" export * from "./GetIssuerCallback.js" export * from "./IdpPicker.js" export * from "./WebIdPicker.js" diff --git a/test/DPoPTokenProvider.customFetch.test.ts b/test/DPoPTokenProvider.customFetch.test.ts new file mode 100644 index 0000000..a6fe0dd --- /dev/null +++ b/test/DPoPTokenProvider.customFetch.test.ts @@ -0,0 +1,189 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { DPoPTokenProvider } from "../dist/DPoPTokenProvider.js" +import { ClientCredentialsTokenProvider } from "../dist/ClientCredentialsTokenProvider.js" +import type { GetCodeCallback } from "../dist/GetCodeCallback.js" + +// Regression tests for the OIDC re-entrancy deadlock: token providers perform +// their own network requests (discovery, dynamic client registration, the token +// grant) through oauth4webapi, which defaults to `globalThis.fetch`. When an +// application patches the global fetch with an authenticating wrapper — e.g. +// `ReactiveFetchManager.registerGlobally()`, or a wrapper that single-flights +// concurrent requests onto one shared authentication attempt — those OIDC +// requests re-enter the wrapper mid-upgrade. A single-flighting wrapper then +// awaits the very authentication attempt its request is serving: a circular +// await that hangs login before the authorization popup/redirect ever opens. +// +// The fix: `TokenProviderOptions.fetch` pins the provider's own OIDC requests +// to a caller-supplied (pristine, pre-patch) fetch via oauth4webapi's +// `customFetch`, so they never ride the patched global. + +const issuer = "https://op.example" +const callbackUri = "https://app.example/callback.html" +const clientId = "mock-client" + +function jsonResponse(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), {status, headers: {"content-type": "application/json"}}) +} + +function base64url(value: string): string { + return Buffer.from(value).toString("base64url") +} + +/** An ID Token with valid claims and a placeholder signature (oauth4webapi does not validate the signature of tokens received directly from the token endpoint). */ +function idToken(nonce: string | undefined): string { + const now = Math.floor(Date.now() / 1000) + const header = {alg: "RS256", typ: "JWT"} + const payload = { + iss: issuer, + sub: "https://alice.example/profile/card#me", + aud: clientId, + exp: now + 600, + iat: now, + webid: "https://alice.example/profile/card#me", + ...(nonce === undefined ? {} : {nonce}), + } + return `${base64url(JSON.stringify(header))}.${base64url(JSON.stringify(payload))}.${base64url("mock-signature")}` +} + +/** An in-process mock OpenID Provider, exposed as a fetch implementation. */ +function mockOpFetch(state: {nonce?: string, requests: string[]}): typeof globalThis.fetch { + return async (input, init) => { + const url = new URL(input instanceof Request ? input.url : String(input)) + state.requests.push(url.pathname) + + switch (url.pathname) { + case "/.well-known/openid-configuration": + return jsonResponse(200, { + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + registration_endpoint: `${issuer}/register`, + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["client_secret_basic"], + }) + case "/register": + return jsonResponse(201, { + client_id: clientId, + redirect_uris: [callbackUri], + response_types: ["code"], + token_endpoint_auth_method: "none", + }) + case "/token": + return jsonResponse(200, { + access_token: "mock-access-token", + token_type: "DPoP", + expires_in: 3600, + scope: "openid webid", + id_token: idToken(state.nonce), + }) + default: + throw new Error(`Unexpected request to mock OP: ${url}`) + } + } +} + +/** Echoes the authorization code straight back, capturing the nonce so the mock OP can bind it into the ID Token. */ +function getCodeCallback(state: {nonce?: string}): GetCodeCallback { + return async authorizationUri => { + state.nonce = authorizationUri.searchParams.get("nonce") ?? undefined + return `${callbackUri}?code=mock-code&state=${authorizationUri.searchParams.get("state")}` + } +} + +test("DPoP auth-code login resolves even when globalThis.fetch is patched with a deadlocking wrapper, when a pristine fetch is pinned", {timeout: 15_000}, async t => { + const state: {nonce?: string, requests: string[]} = {requests: []} + + // Simulate an app-installed single-flighting authenticated-fetch wrapper: + // mid-login, any request through it awaits the in-flight authentication + // attempt — i.e. from the provider's perspective, it never resolves. + const realFetch = globalThis.fetch + let patchedCalls = 0 + globalThis.fetch = (() => { + patchedCalls++ + return new Promise(() => {}) + }) as typeof globalThis.fetch + t.after(() => { + globalThis.fetch = realFetch + }) + + const provider = new DPoPTokenProvider( + callbackUri, + getCodeCallback(state), + async () => new URL(issuer), + {fetch: mockOpFetch(state)}, + ) + + const upgraded = await provider.upgrade(new Request("https://pod.example/private/resource")) + + assert.match(upgraded.headers.get("Authorization") ?? "", /^DPoP mock-access-token$/) + assert.ok(upgraded.headers.get("DPoP")) + assert.deepEqual(state.requests, ["/.well-known/openid-configuration", "/register", "/token"]) + assert.equal(patchedCalls, 0, "the provider's OIDC requests must not ride the patched global fetch") +}) + +test("client-credentials login resolves under a patched globalThis.fetch when a pristine fetch is pinned", {timeout: 15_000}, async t => { + const state: {nonce?: string, requests: string[]} = {requests: []} + + const realFetch = globalThis.fetch + let patchedCalls = 0 + globalThis.fetch = (() => { + patchedCalls++ + return new Promise(() => {}) + }) as typeof globalThis.fetch + t.after(() => { + globalThis.fetch = realFetch + }) + + // The client-credentials provider derives the issuer from the request URL. + const provider = new ClientCredentialsTokenProvider(clientId, "mock-secret", {fetch: mockOpFetchAt("https://solidcommunity.net", state)}) + + const upgraded = await provider.upgrade(new Request("https://alice.solidcommunity.net/private/resource")) + + assert.match(upgraded.headers.get("Authorization") ?? "", /^DPoP mock-access-token$/) + assert.equal(patchedCalls, 0, "the provider's OIDC requests must not ride the patched global fetch") +}) + +test("without options the provider still defaults to globalThis.fetch (backwards compatible)", {timeout: 15_000}, async t => { + const state: {nonce?: string, requests: string[]} = {requests: []} + + const realFetch = globalThis.fetch + globalThis.fetch = mockOpFetch(state) + t.after(() => { + globalThis.fetch = realFetch + }) + + const provider = new DPoPTokenProvider(callbackUri, getCodeCallback(state), async () => new URL(issuer)) + + const upgraded = await provider.upgrade(new Request("https://pod.example/private/resource")) + + assert.match(upgraded.headers.get("Authorization") ?? "", /^DPoP mock-access-token$/) + assert.deepEqual(state.requests, ["/.well-known/openid-configuration", "/register", "/token"]) +}) + +/** A mock OP for a different issuer (client-credentials flow: no registration, no ID Token nonce). */ +function mockOpFetchAt(opIssuer: string, state: {requests: string[]}): typeof globalThis.fetch { + return async (input, init) => { + const url = new URL(input instanceof Request ? input.url : String(input)) + state.requests.push(url.pathname) + + switch (url.pathname) { + case "/.well-known/openid-configuration": + return jsonResponse(200, { + issuer: opIssuer, + authorization_endpoint: `${opIssuer}/authorize`, + token_endpoint: `${opIssuer}/token`, + token_endpoint_auth_methods_supported: ["client_secret_basic"], + }) + case "/token": + return jsonResponse(200, { + access_token: "mock-access-token", + token_type: "DPoP", + expires_in: 3600, + scope: "webid", + }) + default: + throw new Error(`Unexpected request to mock OP: ${url}`) + } + } +}