From 0c931f537364b51d68ee311ae172e9465faedcc2 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 5 Jun 2026 13:32:08 +0100 Subject: [PATCH 1/3] feat(auth): add resource indicator, refresh, and client caching to DCR provider Extend createDcrProvider so RFC 7591 DCR consumers can cover three cases the factory previously forced them to hand-roll a bespoke AuthProvider for: - RFC 8707 resource indicator: new `resource` option, threaded into the authorize URL and the authorization_code + refresh_token token requests so the AS issues a token whose audience targets the protected resource. - refreshToken: the DCR provider now implements it (previously only createPkceProvider did), honouring the handshake client auth (public None / client_secret_post / RFC-3986 Basic), the resource indicator, and a 10s timeout, mapping invalid_grant -> AUTH_REFRESH_EXPIRED else AUTH_REFRESH_TRANSIENT. - client caching: optional loadClient/saveClient hooks let a consumer reuse a registered client across logins (cache hit skips the registration round-trip and the oauth4webapi load); cli-core stays storage-agnostic. Factor the shared refresh-error mapping into oauth.ts (mapRefreshError) and reuse it from createPkceProvider, and add an optional additionalParameters field to buildPkceAuthorizeUrl. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/auth/index.ts | 1 + src/auth/providers/dcr.test.ts | 167 ++++++++++++++++++++++++ src/auth/providers/dcr.ts | 226 +++++++++++++++++++++++++++------ src/auth/providers/oauth.ts | 44 +++++++ src/auth/providers/pkce.ts | 38 +----- 5 files changed, 403 insertions(+), 73 deletions(-) diff --git a/src/auth/index.ts b/src/auth/index.ts index 109badc..61eada3 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -45,6 +45,7 @@ export { createDcrProvider } from './providers/dcr.js' export type { DcrClientMetadata, DcrProviderOptions, + DcrRegisteredClient, DcrTokenEndpointAuthMethod, } from './providers/dcr.js' export type { diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts index 3a252bb..d10ec66 100644 --- a/src/auth/providers/dcr.test.ts +++ b/src/auth/providers/dcr.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { createDcrProvider } from './dcr.js' +import type { DcrRegisteredClient } from './dcr.js' type Account = { id: string; label?: string } @@ -390,4 +391,170 @@ describe('createDcrProvider', () => { provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) }) + + it('threads the RFC 8707 resource indicator into the authorize URL and the token request body', async () => { + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? registration({ client_id: 'pub' }) + : token({ access_token: 'tok', expires_in: 3600 }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + resource: 'https://api.example.com', + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'none' }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + const authorize = await provider.authorize({ + redirectUri: REDIRECT_URI, + state: 'state-123', + scopes: ['user:read'], + readOnly: false, + flags: {}, + handshake: prepared.handshake, + }) + expect(new URL(authorize.authorizeUrl).searchParams.get('resource')).toBe( + 'https://api.example.com', + ) + + await provider.exchangeCode({ + code: 'auth-code', + state: 'state-123', + redirectUri: REDIRECT_URI, + handshake: authorize.handshake, + }) + const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) + expect(tokenBody.get('resource')).toBe('https://api.example.com') + }) + + it('refreshToken runs the refresh_token grant, forwarding the resource indicator', async () => { + const { calls, fetchImpl } = makeFetchRecorder(() => + token({ access_token: 'tok-2', refresh_token: 'rt-2', expires_in: 3600 }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + resource: 'https://api.example.com', + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'none' }, + validate, + fetchImpl, + }) + + const result = await provider.refreshToken!({ + refreshToken: 'rt-1', + handshake: { clientId: 'pub' }, + }) + expect(result.accessToken).toBe('tok-2') + expect(result.refreshToken).toBe('rt-2') + expect(result.expiresAt).toBeGreaterThan(Date.now()) + + const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) + expect(tokenBody.get('grant_type')).toBe('refresh_token') + expect(tokenBody.get('refresh_token')).toBe('rt-1') + expect(tokenBody.get('resource')).toBe('https://api.example.com') + }) + + it('refreshToken authenticates a confidential client per the handshake auth method', async () => { + const { calls, fetchImpl } = makeFetchRecorder(() => token({ access_token: 'tok' })) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_post' }, + validate, + fetchImpl, + }) + + await provider.refreshToken!({ + refreshToken: 'rt', + handshake: { + clientId: 'cid', + clientSecret: 'sec', + tokenEndpointAuthMethod: 'client_secret_post', + }, + }) + const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) + expect(tokenBody.get('client_id')).toBe('cid') + expect(tokenBody.get('client_secret')).toBe('sec') + }) + + it('maps an invalid_grant refresh rejection to AUTH_REFRESH_EXPIRED', async () => { + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'none' }, + validate, + fetchImpl: (() => + Promise.resolve(respond({ error: 'invalid_grant' }, 400))) as typeof fetch, + }) + await expect( + provider.refreshToken!({ refreshToken: 'rt', handshake: { clientId: 'pub' } }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_EXPIRED' }) + }) + + it('refreshToken without a clientId in the handshake is AUTH_REFRESH_UNAVAILABLE', async () => { + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + fetchImpl: (() => Promise.resolve(token({ access_token: 'x' }))) as typeof fetch, + }) + await expect( + provider.refreshToken!({ refreshToken: 'rt', handshake: {} }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) + }) + + it('reuses a cached client via loadClient without a registration POST', async () => { + const { calls, fetchImpl } = makeFetchRecorder(() => { + throw new Error('registration must not be called on a cache hit') + }) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + loadClient: () => ({ clientId: 'cached', tokenEndpointAuthMethod: 'none' }), + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(prepared.handshake).toEqual({ + clientId: 'cached', + tokenEndpointAuthMethod: 'none', + }) + expect(calls).toHaveLength(0) + }) + + it('registers and persists a fresh client via saveClient on a cache miss', async () => { + const saved: DcrRegisteredClient[] = [] + const { calls, fetchImpl } = makeFetchRecorder(() => + registration({ client_id: 'fresh', client_secret: 'sec' }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + loadClient: () => null, + saveClient: (client) => { + saved.push(client) + }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(calls).toHaveLength(1) + expect(prepared.handshake).toEqual({ clientId: 'fresh', clientSecret: 'sec' }) + expect(saved).toEqual([{ clientId: 'fresh', clientSecret: 'sec' }]) + }) }) diff --git a/src/auth/providers/dcr.ts b/src/auth/providers/dcr.ts index 43976b7..ba71465 100644 --- a/src/auth/providers/dcr.ts +++ b/src/auth/providers/dcr.ts @@ -1,4 +1,9 @@ -import type { AuthorizationServer, Client, ClientAuth } from 'oauth4webapi' +import type { + AuthorizationServer, + Client, + ClientAuth, + TokenEndpointRequestOptions, +} from 'oauth4webapi' import { getErrorMessage } from '../../errors.js' import type { CliError } from '../../errors.js' @@ -13,6 +18,7 @@ import type { ExchangeResult, PrepareInput, PrepareResult, + RefreshInput, ValidateInput, } from '../types.js' import { @@ -20,10 +26,16 @@ import { buildPkceAuthorizeUrl, expiresAtFromExpiresIn, loadOauth4webapi, + mapRefreshError, resolve, } from './oauth.js' import type { OAuthLazyString } from './pkce.js' +// Upper bound on the refresh-token POST, mirroring createPkceProvider — kept +// under the refresh helper's stale-lock threshold so a timed-out grant releases +// the lock before another invocation considers it abandoned. +const REFRESH_TIMEOUT_MS = 10_000 + export type DcrTokenEndpointAuthMethod = 'client_secret_basic' | 'client_secret_post' | 'none' /** @@ -51,6 +63,18 @@ export type DcrClientMetadata = { extra?: Record } +/** + * A registered DCR client, as stashed in the handshake by `prepare()` and + * surfaced to / supplied by the optional caching hooks. The shape a consumer + * persists to reuse a registration across logins. + */ +export type DcrRegisteredClient = { + clientId: string + clientSecret?: string + /** Server-authoritative method from the registration response, when supported. */ + tokenEndpointAuthMethod?: DcrTokenEndpointAuthMethod +} + export type DcrProviderOptions = { /** RFC 7591 registration endpoint. Function form supports per-flow base URLs. */ registrationUrl: OAuthLazyString @@ -58,12 +82,36 @@ export type DcrProviderOptions = { authorizeUrl: OAuthLazyString /** OAuth 2.0 token endpoint. */ tokenUrl: OAuthLazyString + /** + * RFC 8707 resource indicator. When set, it's appended to the authorize URL + * and added to the `authorization_code` and `refresh_token` token requests, + * so the authorization server issues a token whose audience targets this + * protected resource. Function form supports per-flow resources. + */ + resource?: OAuthLazyString clientMetadata: DcrClientMetadata /** How to join scopes in the authorize URL. Default `' '` (RFC 6749). */ scopeSeparator?: string verifierAlphabet?: string /** Default 64. */ verifierLength?: number + /** + * Return a previously-registered client to reuse instead of registering a + * fresh one on every `prepare()`. `null`/`undefined` → register anew. The + * consumer owns where the client is persisted (config file, keyring, …); + * pair with `saveClient` to populate that store. cli-core does no caching + * of its own. + */ + loadClient?: ( + input: PrepareInput, + ) => Promise | DcrRegisteredClient | null | undefined + /** + * Persist a freshly-registered client so a later `prepare()` can reuse it + * via `loadClient`. Called only after a successful registration, never on a + * cache hit. A rejection propagates — handle/swallow inside the hook if a + * persistence failure shouldn't fail the login. + */ + saveClient?: (client: DcrRegisteredClient, input: PrepareInput) => Promise | void /** Probe an authenticated endpoint to confirm the token works and resolve the account. */ validate: (input: ValidateInput) => Promise /** @@ -91,16 +139,22 @@ const VALID_AUTH_METHODS: ReadonlySet = new Set([ * driven by [`oauth4webapi`](https://github.com/panva/oauth4webapi) (an * optional peer dep — installed only by DCR/refresh consumers). * - * - `prepare`: register via `dynamicClientRegistrationRequest`. Stash the - * issued `client_id`, optional `client_secret`, and the server-returned - * `token_endpoint_auth_method` (RFC 7591 §3.2.1 — server is authoritative) - * in the handshake. - * - `authorize`: standard PKCE S256 with `client_id` read from the handshake. + * - `prepare`: reuse a cached client via `loadClient` when supplied, else + * register via `dynamicClientRegistrationRequest` (persisting through + * `saveClient`). Stash the issued `client_id`, optional `client_secret`, and + * the server-returned `token_endpoint_auth_method` (RFC 7591 §3.2.1 — server + * is authoritative) in the handshake. + * - `authorize`: standard PKCE S256 with `client_id` read from the handshake, + * plus the optional RFC 8707 `resource` indicator. * - `exchangeCode`: `authorizationCodeGrantRequest` authenticated per the * handshake's server-returned auth method (falling back to the configured * one) — HTTP Basic (RFC 3986-encoded, see `clientSecretBasicRfc3986`), * client-secret POST, or public-client `None` (the last also when the - * registration response carried no `client_secret`). + * registration response carried no `client_secret`). Threads the `resource` + * indicator into the token request. + * - `refreshToken`: `refreshTokenGrantRequest` with the same client auth and + * `resource` as the code exchange, bounded by a 10s timeout. Maps server + * rejections onto `AUTH_REFRESH_EXPIRED` / `AUTH_REFRESH_TRANSIENT`. * - `validateToken`: caller-supplied. */ export function createDcrProvider( @@ -112,6 +166,13 @@ export function createDcrProvider( return { async prepare(input: PrepareInput): Promise { + // Reuse a cached registration when the consumer supplies one — skip + // the registration round-trip (and the oauth4webapi load) entirely. + const cached = options.loadClient ? await options.loadClient(input) : undefined + if (cached?.clientId) { + return { handshake: clientHandshake(cached) } + } + const oauth = await loadOauth4webapi({ code: 'AUTH_DCR_FAILED', missingMessage: 'oauth4webapi is required for Dynamic Client Registration.', @@ -147,9 +208,9 @@ export function createDcrProvider( ) } - const handshake: Record = { clientId: client.client_id } + const registered: DcrRegisteredClient = { clientId: client.client_id } if (typeof client.client_secret === 'string') { - handshake.clientSecret = client.client_secret + registered.clientSecret = client.client_secret } // Per RFC 7591 §3.2.1 the server's chosen method is authoritative. // Honour a supported one; fail fast on a method we can't perform @@ -164,9 +225,10 @@ export function createDcrProvider( options.errorHints, ) } - handshake.tokenEndpointAuthMethod = serverMethod + registered.tokenEndpointAuthMethod = serverMethod as DcrTokenEndpointAuthMethod } - return { handshake } + if (options.saveClient) await options.saveClient(registered, input) + return { handshake: clientHandshake(registered) } }, async authorize(input: AuthorizeInput): Promise { @@ -184,6 +246,9 @@ export function createDcrProvider( length: options.verifierLength, }) const challenge = deriveChallenge(verifier) + const resource = options.resource + ? await resolve(options.resource, input.handshake, input.flags) + : undefined const authorizeUrl = buildPkceAuthorizeUrl({ authorizeUrl: await resolve(options.authorizeUrl, input.handshake, input.flags), clientId, @@ -192,6 +257,7 @@ export function createDcrProvider( scopes: input.scopes, scopeSeparator, codeChallenge: challenge, + additionalParameters: { resource }, }) return { @@ -210,18 +276,6 @@ export function createDcrProvider( options.errorHints, ) } - const clientSecretRaw = input.handshake.clientSecret - const clientSecret = typeof clientSecretRaw === 'string' ? clientSecretRaw : undefined - const issuedMethodRaw = input.handshake.tokenEndpointAuthMethod - const issuedMethod: DcrTokenEndpointAuthMethod | undefined = - typeof issuedMethodRaw === 'string' && - VALID_AUTH_METHODS.has(issuedMethodRaw as DcrTokenEndpointAuthMethod) - ? (issuedMethodRaw as DcrTokenEndpointAuthMethod) - : undefined - // Server-issued method wins (RFC 7591 §3.2.1). Fall back to the - // configured one only when the server didn't echo a known method. - const effectiveAuthMethod = issuedMethod ?? configuredAuthMethod - const oauth = await loadOauth4webapi({ code: 'AUTH_TOKEN_EXCHANGE_FAILED', missingMessage: 'oauth4webapi is required for the DCR token exchange.', @@ -230,21 +284,12 @@ export function createDcrProvider( }) const flags = (input.handshake.flags as Record | undefined) ?? {} const tokenUrl = await resolve(options.tokenUrl, input.handshake, flags) + const resource = options.resource + ? await resolve(options.resource, input.handshake, flags) + : undefined const as: AuthorizationServer = { issuer: tokenUrl, token_endpoint: tokenUrl } const client: Client = { client_id: clientId } - - // Public-client fallback: a registration with no `client_secret` - // can't authenticate Basic/Post regardless of the requested method, - // so we POST `client_id` like a non-confidential client. Otherwise - // honour the effective auth method. - let clientAuth: ClientAuth - if (!clientSecret || effectiveAuthMethod === 'none') { - clientAuth = oauth.None() - } else if (effectiveAuthMethod === 'client_secret_post') { - clientAuth = oauth.ClientSecretPost(clientSecret) - } else { - clientAuth = clientSecretBasicRfc3986(clientSecret) - } + const clientAuth = selectClientAuth(oauth, input.handshake, configuredAuthMethod) try { // The flow runtime owns CSRF state validation; skip oauth4webapi's @@ -262,7 +307,7 @@ export function createDcrProvider( callbackParameters, input.redirectUri, verifier, - customFetchOptions(oauth, options.fetchImpl), + tokenRequestOptions(oauth, options.fetchImpl, resource), ) const result = await oauth.processAuthorizationCodeResponse(as, client, response) return { @@ -282,6 +327,58 @@ export function createDcrProvider( }, validateToken: options.validate, + + async refreshToken(input: RefreshInput): Promise> { + const clientId = input.handshake.clientId + if (typeof clientId !== 'string') { + throw buildAuthError( + 'AUTH_REFRESH_UNAVAILABLE', + 'Internal: DCR refresh handshake missing clientId.', + options.errorHints, + ) + } + const oauth = await loadOauth4webapi({ + code: 'AUTH_REFRESH_UNAVAILABLE', + missingMessage: 'oauth4webapi is required for refresh-token support.', + userHints: options.errorHints, + missingHints: MISSING_PEER_HINTS, + }) + // Mirror `exchangeCode`: a resolver that reads `flags` sees the same + // view during silent refresh as it did at authorize time. + const flags = (input.handshake.flags as Record | undefined) ?? {} + const tokenUrl = await resolve(options.tokenUrl, input.handshake, flags) + const resource = options.resource + ? await resolve(options.resource, input.handshake, flags) + : undefined + const as: AuthorizationServer = { issuer: tokenUrl, token_endpoint: tokenUrl } + const client: Client = { client_id: clientId } + const clientAuth = selectClientAuth(oauth, input.handshake, configuredAuthMethod) + // Bound the network call so a hung token endpoint can't hold the + // refresh lock forever (see createPkceProvider for the rationale). + const requestOptions = tokenRequestOptions( + oauth, + options.fetchImpl, + resource, + AbortSignal.timeout(REFRESH_TIMEOUT_MS), + ) + try { + const response = await oauth.refreshTokenGrantRequest( + as, + client, + clientAuth, + input.refreshToken, + requestOptions, + ) + const result = await oauth.processRefreshTokenResponse(as, client, response) + return { + accessToken: result.access_token, + refreshToken: result.refresh_token, + expiresAt: expiresAtFromExpiresIn(result.expires_in), + } + } catch (error) { + throw mapRefreshError(error, oauth) + } + }, } } @@ -313,6 +410,61 @@ function customFetchOptions( return fetchImpl ? { [oauth.customFetch]: fetchImpl } : undefined } +/** Build the handshake fields a registered (or cached) client contributes. */ +function clientHandshake(client: DcrRegisteredClient): Record { + const handshake: Record = { clientId: client.clientId } + if (client.clientSecret !== undefined) handshake.clientSecret = client.clientSecret + if (client.tokenEndpointAuthMethod !== undefined) { + handshake.tokenEndpointAuthMethod = client.tokenEndpointAuthMethod + } + return handshake +} + +/** + * Pick the token-endpoint client authentication for `authorization_code` / + * `refresh_token` grants from the handshake. Server-issued method wins (RFC + * 7591 §3.2.1), falling back to the configured one. A registration with no + * `client_secret` can't authenticate Basic/Post regardless of the requested + * method, so it drops to public-client `None` (POST `client_id` only). + */ +function selectClientAuth( + oauth: typeof import('oauth4webapi'), + handshake: Record, + configuredAuthMethod: DcrTokenEndpointAuthMethod, +): ClientAuth { + const clientSecretRaw = handshake.clientSecret + const clientSecret = typeof clientSecretRaw === 'string' ? clientSecretRaw : undefined + const issuedMethodRaw = handshake.tokenEndpointAuthMethod + const issuedMethod: DcrTokenEndpointAuthMethod | undefined = + typeof issuedMethodRaw === 'string' && + VALID_AUTH_METHODS.has(issuedMethodRaw as DcrTokenEndpointAuthMethod) + ? (issuedMethodRaw as DcrTokenEndpointAuthMethod) + : undefined + const effectiveAuthMethod = issuedMethod ?? configuredAuthMethod + + if (!clientSecret || effectiveAuthMethod === 'none') return oauth.None() + if (effectiveAuthMethod === 'client_secret_post') return oauth.ClientSecretPost(clientSecret) + return clientSecretBasicRfc3986(clientSecret) +} + +/** + * Assemble the oauth4webapi token-request options: the injected `fetchImpl` + * (via the `customFetch` symbol), the RFC 8707 `resource` indicator (as an + * `additionalParameters` body field), and an optional abort `signal`. + */ +function tokenRequestOptions( + oauth: typeof import('oauth4webapi'), + fetchImpl: typeof fetch | undefined, + resource: string | undefined, + signal?: AbortSignal, +): TokenEndpointRequestOptions { + const opts: TokenEndpointRequestOptions = {} + if (fetchImpl) opts[oauth.customFetch] = fetchImpl + if (resource) opts.additionalParameters = { resource } + if (signal) opts.signal = signal + return opts +} + /** * Translate an oauth4webapi failure into a typed `CliError`. A `ResponseBodyError` * carries the server's OAuth error JSON (`error` / `error_description`) — surface diff --git a/src/auth/providers/oauth.ts b/src/auth/providers/oauth.ts index a4167cd..f37f192 100644 --- a/src/auth/providers/oauth.ts +++ b/src/auth/providers/oauth.ts @@ -47,6 +47,13 @@ type BuildPkceAuthorizeUrlInput = { scopes: string[] scopeSeparator: string codeChallenge: string + /** + * Extra query parameters appended to the authorize URL, e.g. the RFC 8707 + * `resource` indicator. Set after the standard PKCE params, so a caller + * can't accidentally clobber `client_id`/`state`/etc. `undefined` values + * are skipped. + */ + additionalParameters?: Record } /** Construct the standard PKCE S256 authorize URL. */ @@ -61,6 +68,9 @@ export function buildPkceAuthorizeUrl(input: BuildPkceAuthorizeUrlInput): string if (input.scopes.length > 0) { url.searchParams.set('scope', input.scopes.join(input.scopeSeparator)) } + for (const [key, value] of Object.entries(input.additionalParameters ?? {})) { + if (value !== undefined) url.searchParams.set(key, value) + } return url.toString() } @@ -190,6 +200,40 @@ export function expiresAtFromExpiresIn(expiresIn: number | undefined): number | return typeof expiresIn === 'number' ? Date.now() + expiresIn * 1000 : undefined } +/** + * Map a `refreshTokenGrantRequest` failure onto the refresh error taxonomy, + * shared by every provider that exposes `refreshToken`. A `ResponseBodyError` + * carries the server's OAuth error JSON: `invalid_grant` (any status — some + * proxies remap 400 → 401) means the refresh token itself was rejected, so + * re-login is the only recovery (`AUTH_REFRESH_EXPIRED`). Every other code is + * transient from cli-core's POV (`AUTH_REFRESH_TRANSIENT`) — but the actual + * `error`/`error_description` is surfaced so a misconfigured server is + * diagnosable rather than hidden behind a generic message. + */ +export function mapRefreshError(error: unknown, oauth: typeof import('oauth4webapi')): CliError { + if (error instanceof oauth.ResponseBodyError) { + const detail = error.error_description + ? `${error.error} (${error.error_description})` + : error.error + if (error.error === 'invalid_grant') { + return new CliError('AUTH_REFRESH_EXPIRED', `Refresh token rejected: ${detail}`, { + hints: ['Re-run the login command to reauthorize.'], + }) + } + return new CliError('AUTH_REFRESH_TRANSIENT', `Refresh request failed: ${detail}`, { + hints: ['Try again.'], + }) + } + // Network failure, non-JSON body, WWWAuthenticateChallengeError, … + return new CliError( + 'AUTH_REFRESH_TRANSIENT', + `Refresh request failed: ${getErrorMessage(error)}`, + { + hints: ['Try again.'], + }, + ) +} + // Optional peer dep — only DCR and refresh consumers install it. The dynamic // import (and a missing-peer failure) is memoised so it isn't repeated on every // call that sits on the authenticated-call path. diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index e182b10..870b243 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -1,5 +1,4 @@ import type { AuthorizationServer, Client, TokenEndpointRequestOptions } from 'oauth4webapi' -import { CliError, getErrorMessage } from '../../errors.js' import { deriveChallenge, generateVerifier } from '../pkce.js' import type { AuthAccount, @@ -16,6 +15,7 @@ import { buildPkceAuthorizeUrl, expiresAtFromExpiresIn, loadOauth4webapi, + mapRefreshError, postTokenEndpoint, resolve, } from './oauth.js' @@ -193,41 +193,7 @@ export function createPkceProvider( expiresAt: expiresAtFromExpiresIn(result.expires_in), } } catch (error) { - // A ResponseBodyError carries the server's OAuth error JSON. - // `invalid_grant` (any status — some proxies remap 400 → 401) - // means the refresh token itself was rejected; re-login is the - // only recovery. Every other code is transient from cli-core's - // POV — but surface the actual `error`/`error_description` so a - // misconfigured server (e.g. `invalid_request: Missing - // client_secret`) is diagnosable rather than hidden behind - // oauth4webapi's generic "server responded with an error". - if (error instanceof oauth.ResponseBodyError) { - const detail = error.error_description - ? `${error.error} (${error.error_description})` - : error.error - if (error.error === 'invalid_grant') { - throw new CliError( - 'AUTH_REFRESH_EXPIRED', - `Refresh token rejected: ${detail}`, - { - hints: ['Re-run the login command to reauthorize.'], - }, - ) - } - throw new CliError( - 'AUTH_REFRESH_TRANSIENT', - `Refresh request failed: ${detail}`, - { - hints: ['Try again.'], - }, - ) - } - // Network failure, non-JSON body, WWWAuthenticateChallengeError, … - throw new CliError( - 'AUTH_REFRESH_TRANSIENT', - `Refresh request failed: ${getErrorMessage(error)}`, - { hints: ['Try again.'] }, - ) + throw mapRefreshError(error, oauth) } }, } From 03ba82e328975797820f6c3a35d6232cb5ad3938 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 5 Jun 2026 13:47:28 +0100 Subject: [PATCH 2/3] fix(auth): address DCR provider review - buildPkceAuthorizeUrl: apply additionalParameters before the standard PKCE params so the latter always win (the loop previously ran after, enabling the clobbering the doc claimed to prevent). - Validate the loadClient cache shape (string clientId, supported tokenEndpointAuthMethod) so a cached client behaves like a freshly registered one instead of failing later or silently using the wrong auth method. - Resolve tokenUrl/authorizeUrl and the resource indicator concurrently (Promise.all) on the authorize, exchange, and refresh paths. - Document the loadClient redirect-URI binding (registration is bound to its redirect_uris; cache must be keyed on PrepareInput.redirectUri) and the DCR refresh handshake contract (clientId must be supplied on the refresh handshake, reconstructed from persisted account metadata). - Tests: make the refresh auth-method test prove handshake-over-config precedence (differing values), exercise async loadClient/saveClient, and cover malformed-cache rejection. - README: document resource indicators, refresh, and client caching; drop the stale "does not cache" claim. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 48 +++++++++++------ src/auth/providers/dcr.test.ts | 55 ++++++++++++++++--- src/auth/providers/dcr.ts | 96 ++++++++++++++++++++++++++++------ src/auth/providers/oauth.ts | 16 +++--- 4 files changed, 168 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 9370a76..32ae094 100644 --- a/README.md +++ b/README.md @@ -12,20 +12,20 @@ npm install @doist/cli-core ## What's in it -| Module | Key exports | Purpose | -| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `attachAccountListCommand`, `attachAccountUseCommand`, `attachAccountCurrentCommand`, `attachAccountRemoveCommand`, `runOAuthFlow`, `refreshAccessToken`, `createPkceProvider`, `createDcrProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, `bundleFromExchange`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `ActiveBundleSnapshot` / `RefreshInput` / `AccountRef` / `ClearedAccount` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` / `AttachAccountListContext` / `AttachAccountCurrentContext` / `AttachAccountRemoveContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token` and ` account list` / `use` / `current` / `remove`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), the RFC 7591 Dynamic Client Registration flow (`createDcrProvider`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). The store contract supports an optional `setBundle(account, bundle)` write method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`; `active()` stays narrow (access token + account only) so callers that don't need refresh state don't pay extra keyring IPC. `AuthProvider` and `TokenStore` remain the escape hatches for fully bespoke backends (device code, magic-link, …). `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`), and `oauth4webapi` (when a consumer opts into silent refresh or uses `createDcrProvider`) are optional peer/optional deps. | -| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | -| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | -| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | -| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | -| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | -| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | -| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | -| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | -| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | -| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | -| `testing` (subpath) | `describeEmptyMachineOutput`, `createTestProgram`, `captureConsole`, `captureStream`, `buildTokenStore`, `buildSingleEntryStore`, `ingenEntries`, `alanGrant` / `ellieSattler` / `ianMalcolm`, `TestAccount` / `StoreEntry` / `TokenStoreHarness` / `MatchAccount` types | Vitest helpers + fixtures reusable by consuming CLIs: a parametrised empty-state suite (`--json` / `--ndjson` / human modes); a Commander test-program builder (`createTestProgram`; the whole subpath **requires** `commander` since the barrel re-exports it); console / stdout-stderr spies that silence + auto-restore (`captureConsole` / `captureStream`, call inside a test or `beforeEach`); and a canonical stateful in-memory `TokenStore` mock plus shared account fixtures (`buildTokenStore` / `buildSingleEntryStore`) modelling `createKeyringTokenStore`'s default-selection contract — pass `matchAccount` to mirror a consumer's own ref-matching (numeric-id / case-insensitive label). | +| Module | Key exports | Purpose | +| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `attachAccountListCommand`, `attachAccountUseCommand`, `attachAccountCurrentCommand`, `attachAccountRemoveCommand`, `runOAuthFlow`, `refreshAccessToken`, `createPkceProvider`, `createDcrProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, `bundleFromExchange`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `ActiveBundleSnapshot` / `RefreshInput` / `AccountRef` / `ClearedAccount` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` / `AttachAccountListContext` / `AttachAccountCurrentContext` / `AttachAccountRemoveContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token` and ` account list` / `use` / `current` / `remove`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), the RFC 7591 Dynamic Client Registration flow (`createDcrProvider`, with optional RFC 8707 resource indicators, refresh-token support, and `loadClient`/`saveClient` client caching), a thin cross-platform OS-keyring wrapper (`createSecureStore`), and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). The store contract supports an optional `setBundle(account, bundle)` write method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`; `active()` stays narrow (access token + account only) so callers that don't need refresh state don't pay extra keyring IPC. `AuthProvider` and `TokenStore` remain the escape hatches for fully bespoke backends (device code, magic-link, …). `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`), and `oauth4webapi` (when a consumer opts into silent refresh or uses `createDcrProvider`) are optional peer/optional deps. | +| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | +| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | +| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | +| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | +| `global-args` | `parseGlobalArgs`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | +| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | +| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | +| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | +| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | +| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | +| `testing` (subpath) | `describeEmptyMachineOutput`, `createTestProgram`, `captureConsole`, `captureStream`, `buildTokenStore`, `buildSingleEntryStore`, `ingenEntries`, `alanGrant` / `ellieSattler` / `ianMalcolm`, `TestAccount` / `StoreEntry` / `TokenStoreHarness` / `MatchAccount` types | Vitest helpers + fixtures reusable by consuming CLIs: a parametrised empty-state suite (`--json` / `--ndjson` / human modes); a Commander test-program builder (`createTestProgram`; the whole subpath **requires** `commander` since the barrel re-exports it); console / stdout-stderr spies that silence + auto-restore (`captureConsole` / `captureStream`, call inside a test or `beforeEach`); and a canonical stateful in-memory `TokenStore` mock plus shared account fixtures (`buildTokenStore` / `buildSingleEntryStore`) modelling `createKeyringTokenStore`'s default-selection contract — pass `matchAccount` to mirror a consumer's own ref-matching (numeric-id / case-insensitive label). | ## Usage @@ -216,7 +216,7 @@ The `authorizeUrl` / `tokenUrl` / `clientId` resolvers may return `string` **or* #### Quick start (Dynamic Client Registration) -For providers that issue per-install `client_id` / `client_secret` via [RFC 7591](https://datatracker.ietf.org/doc/html/rfc7591). `createDcrProvider` registers in `prepare()`, then drives the standard PKCE authorize / token-exchange dance against the resulting client. Registration and token exchange run through [`oauth4webapi`](https://github.com/panva/oauth4webapi) (the same optional peer dep PKCE refresh uses — `npm install oauth4webapi`), so endpoints must be HTTPS and the registration endpoint must return RFC 7591-conformant `201` responses. +For providers that issue per-install `client_id` / `client_secret` via [RFC 7591](https://datatracker.ietf.org/doc/html/rfc7591). `createDcrProvider` registers in `prepare()`, then drives the standard PKCE authorize / token-exchange dance against the resulting client and (optionally) refreshes it later. Registration, token exchange, and refresh run through [`oauth4webapi`](https://github.com/panva/oauth4webapi) (the same optional peer dep PKCE refresh uses — `npm install oauth4webapi`), so endpoints must be HTTPS and the registration endpoint must return RFC 7591-conformant `201` responses. ```ts import { attachLoginCommand, createDcrProvider } from '@doist/cli-core/auth' @@ -225,6 +225,11 @@ const provider = createDcrProvider({ registrationUrl: 'https://example.com/oauth/register', authorizeUrl: 'https://example.com/oauth/authorize', tokenUrl: 'https://example.com/oauth/token', + // Optional RFC 8707 resource indicator — set when the authorization server + // and the protected resource (API) are different origins, so the issued + // token's audience targets the API. Added to the authorize URL and the + // code-exchange + refresh token requests. + resource: 'https://api.example.com', clientMetadata: { clientName: 'Example CLI', clientUri: 'https://github.com/example/cli', @@ -233,10 +238,19 @@ const provider = createDcrProvider({ tokenEndpointAuthMethod: 'client_secret_basic', // default }, validate: async ({ token }) => probeUser(token), + // Optional: reuse a registered client across logins instead of minting a + // fresh one each time. The consumer owns persistence; key the cache on + // `input.redirectUri` (the registration is bound to its redirect URIs). + loadClient: (input) => readCachedClient(input.redirectUri), + saveClient: (client, input) => writeCachedClient(client, input.redirectUri), }) ``` -The DCR-issued `client_id` (and `client_secret`, if returned) are stashed in the handshake and threaded through the rest of the flow. The server-returned `token_endpoint_auth_method` is authoritative (RFC 7591 §3.2.1) and overrides the configured one. By default the token exchange uses `Authorization: Basic` with each credential component percent-encoded via `encodeURIComponent` (RFC 3986) rather than oauth4webapi's stricter RFC 6749 §2.3.1 form-url-encoding. Both escape genuinely reserved chars (`:` / `%` / `+` / `/`) so a conformant server reconstructs them, but §2.3.1 _also_ escapes the unreserved `-` `_` `.` `~` — which breaks servers that don't url-decode the Basic credential (a DCR-issued `twd_…` would arrive as `twd%5F…` and miss the lookup). Pass `tokenEndpointAuthMethod: 'client_secret_post'` to send credentials in the body instead, or `'none'` for a public-client registration. When the registration response carries no `client_secret`, the exchange falls back to a public-client POST regardless of the requested method. Any extra registration metadata (e.g. `software_statement`) goes in `clientMetadata.extra`. cli-core does **not** cache the registered client — each login mints a fresh one. +The DCR-issued `client_id` (and `client_secret`, if returned) are stashed in the handshake and threaded through the rest of the flow. The server-returned `token_endpoint_auth_method` is authoritative (RFC 7591 §3.2.1) and overrides the configured one. By default the token exchange uses `Authorization: Basic` with each credential component percent-encoded via `encodeURIComponent` (RFC 3986) rather than oauth4webapi's stricter RFC 6749 §2.3.1 form-url-encoding. Both escape genuinely reserved chars (`:` / `%` / `+` / `/`) so a conformant server reconstructs them, but §2.3.1 _also_ escapes the unreserved `-` `_` `.` `~` — which breaks servers that don't url-decode the Basic credential (a DCR-issued `twd_…` would arrive as `twd%5F…` and miss the lookup). Pass `tokenEndpointAuthMethod: 'client_secret_post'` to send credentials in the body instead, or `'none'` for a public-client registration. When the registration response carries no `client_secret`, the exchange falls back to a public-client POST regardless of the requested method. Any extra registration metadata (e.g. `software_statement`) goes in `clientMetadata.extra`. + +**Client caching.** By default cli-core does not cache the registered client — each login mints a fresh one. Supply `loadClient` / `saveClient` to reuse one: a cache hit short-circuits `prepare()` (skipping both the registration round-trip and the `oauth4webapi` load), and the returned `DcrRegisteredClient` shape is validated before use. Because an RFC 7591 registration is bound to its `redirect_uris` and `runOAuthFlow` may pick a different callback port/path on a later login, the cache must be keyed on `PrepareInput.redirectUri`. + +**Refresh.** `createDcrProvider` implements `refreshToken`, so DCR consumers can use the standard `refreshAccessToken` silent-refresh path (same client auth and `resource` as the code exchange). Unlike `createPkceProvider` — whose `clientId` is a static provider option — a DCR `clientId` is minted at registration and cli-core does not persist the handshake (`runOAuthFlow` stores only the token bundle). So the consumer must reconstruct the refresh handshake from persisted account metadata and pass it via `refreshAccessToken({ handshake })`: at minimum `clientId`, plus `clientSecret` / `tokenEndpointAuthMethod` for a confidential client. Both `createPkceProvider` and `createDcrProvider` accept an optional `errorHints: string[]` that is prepended to every `CliError` they throw. Use it for CLI-specific remediation that should accompany every auth failure (e.g. `['Try again: tw auth login', 'Or set TWIST_API_TOKEN environment variable']`). Server-returned response bodies (for non-2xx replies) are appended after the user hints so the actionable hint stays at the top. @@ -431,7 +445,7 @@ Error contract: - `AUTH_REFRESH_TRANSIENT` — 5xx, network, non-JSON body, lock timeout. Caller may retry. - `AUTH_REFRESH_UNAVAILABLE` — refresh isn't possible in the current setup: no refresh token stored, the store doesn't implement **both** `activeBundle` and `setBundle` (a full bundle must be readable and persistable), the credential was removed mid-refresh, the provider doesn't implement `refreshToken`, or the optional `oauth4webapi` peer dep isn't installed. -The PKCE provider (`createPkceProvider`) implements `refreshToken` via the [`oauth4webapi`](https://github.com/panva/oauth4webapi) library, declared as an **optional peer dependency** — only CLIs that opt into refresh or use `createDcrProvider` need to install it (`npm install oauth4webapi`). Providers built directly against the `AuthProvider` interface (e.g. device code) implement the `refreshToken?` hook themselves; the storage and helper contract is identical. +The PKCE provider (`createPkceProvider`) and the DCR provider (`createDcrProvider`) both implement `refreshToken` via the [`oauth4webapi`](https://github.com/panva/oauth4webapi) library, declared as an **optional peer dependency** — only CLIs that opt into refresh or use `createDcrProvider` need to install it (`npm install oauth4webapi`). For DCR, reconstruct the refresh handshake (`clientId`, plus `clientSecret` / `tokenEndpointAuthMethod` for confidential clients) from persisted account metadata and pass it via `refreshAccessToken({ handshake })`, since cli-core doesn't persist the DCR handshake. Providers built directly against the `AuthProvider` interface (e.g. device code) implement the `refreshToken?` hook themselves; the storage and helper contract is identical. #### Keyring primitive (`createSecureStore`) diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts index d10ec66..df7e095 100644 --- a/src/auth/providers/dcr.test.ts +++ b/src/auth/providers/dcr.test.ts @@ -459,13 +459,17 @@ describe('createDcrProvider', () => { expect(tokenBody.get('resource')).toBe('https://api.example.com') }) - it('refreshToken authenticates a confidential client per the handshake auth method', async () => { + it('refreshToken honours the handshake auth method over the configured one', async () => { const { calls, fetchImpl } = makeFetchRecorder(() => token({ access_token: 'tok' })) + // Configured: client_secret_basic. Handshake (server-issued at + // registration): client_secret_post. The refresh must follow the + // handshake — so the secret goes in the body and no Basic header is + // sent. Differing the two values is what makes this assert precedence. const provider = createDcrProvider({ registrationUrl: REGISTRATION_URL, authorizeUrl: AUTHORIZE_URL, tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_post' }, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_basic' }, validate, fetchImpl, }) @@ -478,7 +482,9 @@ describe('createDcrProvider', () => { tokenEndpointAuthMethod: 'client_secret_post', }, }) - const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenBody = bodyOf(tokenCall) + expect(headersOf(tokenCall).has('authorization')).toBe(false) expect(tokenBody.get('client_id')).toBe('cid') expect(tokenBody.get('client_secret')).toBe('sec') }) @@ -512,7 +518,7 @@ describe('createDcrProvider', () => { ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) }) - it('reuses a cached client via loadClient without a registration POST', async () => { + it('reuses a cached client via an async loadClient without a registration POST', async () => { const { calls, fetchImpl } = makeFetchRecorder(() => { throw new Error('registration must not be called on a cache hit') }) @@ -521,7 +527,14 @@ describe('createDcrProvider', () => { authorizeUrl: AUTHORIZE_URL, tokenUrl: TOKEN_URL, clientMetadata: { clientName: 'CLI' }, - loadClient: () => ({ clientId: 'cached', tokenEndpointAuthMethod: 'none' }), + // Async hook: a dropped `await` in the provider would surface a + // Promise here instead of the resolved client and fail the assert. + loadClient: (input) => + Promise.resolve( + input.redirectUri === REDIRECT_URI + ? { clientId: 'cached', tokenEndpointAuthMethod: 'none' } + : null, + ), validate, fetchImpl, }) @@ -534,7 +547,7 @@ describe('createDcrProvider', () => { expect(calls).toHaveLength(0) }) - it('registers and persists a fresh client via saveClient on a cache miss', async () => { + it('registers and persists a fresh client via an async saveClient on a cache miss', async () => { const saved: DcrRegisteredClient[] = [] const { calls, fetchImpl } = makeFetchRecorder(() => registration({ client_id: 'fresh', client_secret: 'sec' }), @@ -544,8 +557,9 @@ describe('createDcrProvider', () => { authorizeUrl: AUTHORIZE_URL, tokenUrl: TOKEN_URL, clientMetadata: { clientName: 'CLI' }, - loadClient: () => null, - saveClient: (client) => { + loadClient: () => Promise.resolve(null), + saveClient: async (client) => { + await Promise.resolve() saved.push(client) }, validate, @@ -557,4 +571,29 @@ describe('createDcrProvider', () => { expect(prepared.handshake).toEqual({ clientId: 'fresh', clientSecret: 'sec' }) expect(saved).toEqual([{ clientId: 'fresh', clientSecret: 'sec' }]) }) + + it('rejects a malformed cached client from loadClient', async () => { + const make = (cached: unknown) => + createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + loadClient: () => cached as DcrRegisteredClient, + validate, + fetchImpl: (() => { + throw new Error('registration must not be reached') + }) as typeof fetch, + }) + + await expect( + make({ clientId: 42 }).prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) + await expect( + make({ clientId: 'cid', tokenEndpointAuthMethod: 'private_key_jwt' }).prepare!({ + redirectUri: REDIRECT_URI, + flags: {}, + }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) + }) }) diff --git a/src/auth/providers/dcr.ts b/src/auth/providers/dcr.ts index ba71465..76f0239 100644 --- a/src/auth/providers/dcr.ts +++ b/src/auth/providers/dcr.ts @@ -101,6 +101,14 @@ export type DcrProviderOptions = { * consumer owns where the client is persisted (config file, keyring, …); * pair with `saveClient` to populate that store. cli-core does no caching * of its own. + * + * IMPORTANT: an RFC 7591 registration is bound to the `redirect_uris` it + * was registered with, and `runOAuthFlow` can pick a different callback + * port/path on a later login. Key the cache on `input.redirectUri` (it's + * provided here) and return a hit only for a matching URI, or the reused + * client will send an unregistered `redirect_uri` and the authorize call + * will fail. The returned shape is validated (string `clientId`, supported + * `tokenEndpointAuthMethod`) before use. */ loadClient?: ( input: PrepareInput, @@ -168,8 +176,14 @@ export function createDcrProvider( async prepare(input: PrepareInput): Promise { // Reuse a cached registration when the consumer supplies one — skip // the registration round-trip (and the oauth4webapi load) entirely. - const cached = options.loadClient ? await options.loadClient(input) : undefined - if (cached?.clientId) { + // Validate the persisted shape so a cached client behaves exactly + // like a freshly registered one rather than failing later in + // authorize/exchange or silently using the wrong auth method. + const cached = validateCachedClient( + options.loadClient ? await options.loadClient(input) : undefined, + options.errorHints, + ) + if (cached) { return { handshake: clientHandshake(cached) } } @@ -246,11 +260,13 @@ export function createDcrProvider( length: options.verifierLength, }) const challenge = deriveChallenge(verifier) - const resource = options.resource - ? await resolve(options.resource, input.handshake, input.flags) - : undefined + // Resolve concurrently — both may be async (config read / prompt). + const [authorizeBaseUrl, resource] = await Promise.all([ + resolve(options.authorizeUrl, input.handshake, input.flags), + resolveResource(options.resource, input.handshake, input.flags), + ]) const authorizeUrl = buildPkceAuthorizeUrl({ - authorizeUrl: await resolve(options.authorizeUrl, input.handshake, input.flags), + authorizeUrl: authorizeBaseUrl, clientId, redirectUri: input.redirectUri, state: input.state, @@ -283,10 +299,10 @@ export function createDcrProvider( missingHints: MISSING_PEER_HINTS, }) const flags = (input.handshake.flags as Record | undefined) ?? {} - const tokenUrl = await resolve(options.tokenUrl, input.handshake, flags) - const resource = options.resource - ? await resolve(options.resource, input.handshake, flags) - : undefined + const [tokenUrl, resource] = await Promise.all([ + resolve(options.tokenUrl, input.handshake, flags), + resolveResource(options.resource, input.handshake, flags), + ]) const as: AuthorizationServer = { issuer: tokenUrl, token_endpoint: tokenUrl } const client: Client = { client_id: clientId } const clientAuth = selectClientAuth(oauth, input.handshake, configuredAuthMethod) @@ -329,11 +345,18 @@ export function createDcrProvider( validateToken: options.validate, async refreshToken(input: RefreshInput): Promise> { + // Unlike createPkceProvider (whose clientId is a static provider + // option), a DCR clientId is minted at registration. cli-core does + // not persist the handshake — `runOAuthFlow` stores only the token + // bundle — so the consumer must supply it on the refresh handshake, + // reconstructed from persisted account metadata, via + // `refreshAccessToken({ handshake })`. (Confidential clients pass + // `clientSecret`/`tokenEndpointAuthMethod` the same way.) const clientId = input.handshake.clientId if (typeof clientId !== 'string') { throw buildAuthError( 'AUTH_REFRESH_UNAVAILABLE', - 'Internal: DCR refresh handshake missing clientId.', + 'DCR refresh requires a clientId on the handshake (reconstruct it from the stored account and pass it via refreshAccessToken({ handshake })).', options.errorHints, ) } @@ -346,10 +369,10 @@ export function createDcrProvider( // Mirror `exchangeCode`: a resolver that reads `flags` sees the same // view during silent refresh as it did at authorize time. const flags = (input.handshake.flags as Record | undefined) ?? {} - const tokenUrl = await resolve(options.tokenUrl, input.handshake, flags) - const resource = options.resource - ? await resolve(options.resource, input.handshake, flags) - : undefined + const [tokenUrl, resource] = await Promise.all([ + resolve(options.tokenUrl, input.handshake, flags), + resolveResource(options.resource, input.handshake, flags), + ]) const as: AuthorizationServer = { issuer: tokenUrl, token_endpoint: tokenUrl } const client: Client = { client_id: clientId } const clientAuth = selectClientAuth(oauth, input.handshake, configuredAuthMethod) @@ -410,6 +433,49 @@ function customFetchOptions( return fetchImpl ? { [oauth.customFetch]: fetchImpl } : undefined } +/** + * Validate a client returned by `loadClient` before trusting it. A JSON-backed + * hook can hand back arbitrary shapes; reject a missing/empty `clientId` or an + * unsupported `tokenEndpointAuthMethod` here so a cached client can't behave + * differently from a freshly registered one (which the registration path + * already validates). Returns `undefined` for a cache miss (`null`/`undefined`). + */ +function validateCachedClient( + cached: DcrRegisteredClient | null | undefined, + errorHints: string[] | undefined, +): DcrRegisteredClient | undefined { + if (cached === null || cached === undefined) return undefined + if (typeof cached.clientId !== 'string' || cached.clientId.length === 0) { + throw buildAuthError( + 'AUTH_DCR_FAILED', + 'Cached OAuth client is missing a string clientId.', + errorHints, + ) + } + if ( + cached.tokenEndpointAuthMethod !== undefined && + !VALID_AUTH_METHODS.has(cached.tokenEndpointAuthMethod) + ) { + throw buildAuthError( + 'AUTH_DCR_FAILED', + `Cached OAuth client has an unsupported token_endpoint_auth_method: ${String( + cached.tokenEndpointAuthMethod, + )}.`, + errorHints, + ) + } + return cached +} + +/** Resolve the optional RFC 8707 resource indicator, or `undefined` when unset. */ +function resolveResource( + resource: OAuthLazyString | undefined, + handshake: Record, + flags: Record, +): Promise { + return resource ? resolve(resource, handshake, flags) : Promise.resolve(undefined) +} + /** Build the handshake fields a registered (or cached) client contributes. */ function clientHandshake(client: DcrRegisteredClient): Record { const handshake: Record = { clientId: client.clientId } diff --git a/src/auth/providers/oauth.ts b/src/auth/providers/oauth.ts index f37f192..f106f9e 100644 --- a/src/auth/providers/oauth.ts +++ b/src/auth/providers/oauth.ts @@ -48,10 +48,10 @@ type BuildPkceAuthorizeUrlInput = { scopeSeparator: string codeChallenge: string /** - * Extra query parameters appended to the authorize URL, e.g. the RFC 8707 - * `resource` indicator. Set after the standard PKCE params, so a caller - * can't accidentally clobber `client_id`/`state`/etc. `undefined` values - * are skipped. + * Extra query parameters for the authorize URL, e.g. the RFC 8707 + * `resource` indicator. Applied *before* the standard PKCE params so the + * latter always win — a caller can't accidentally clobber + * `client_id`/`state`/etc. `undefined` values are skipped. */ additionalParameters?: Record } @@ -59,6 +59,11 @@ type BuildPkceAuthorizeUrlInput = { /** Construct the standard PKCE S256 authorize URL. */ export function buildPkceAuthorizeUrl(input: BuildPkceAuthorizeUrlInput): string { const url = new URL(input.authorizeUrl) + // Set extras first; the standard params below overwrite any collision, so + // the PKCE essentials can never be displaced by a caller-supplied value. + for (const [key, value] of Object.entries(input.additionalParameters ?? {})) { + if (value !== undefined) url.searchParams.set(key, value) + } url.searchParams.set('response_type', 'code') url.searchParams.set('client_id', input.clientId) url.searchParams.set('redirect_uri', input.redirectUri) @@ -68,9 +73,6 @@ export function buildPkceAuthorizeUrl(input: BuildPkceAuthorizeUrlInput): string if (input.scopes.length > 0) { url.searchParams.set('scope', input.scopes.join(input.scopeSeparator)) } - for (const [key, value] of Object.entries(input.additionalParameters ?? {})) { - if (value !== undefined) url.searchParams.set(key, value) - } return url.toString() } From 22002f1d6295afccf2dd019b74dc88878b2bd3e9 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 5 Jun 2026 14:01:40 +0100 Subject: [PATCH 3/3] feat(auth): surface token-response scope on ExchangeResult MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an optional `scope` to `ExchangeResult` carrying the raw `scope` from the token response (RFC 6749 §5.1). Populate it from the authorization_code and refresh_token grants in both createPkceProvider and createDcrProvider (and from postTokenEndpoint). Lets a provider's validateToken record the server-authoritative granted scope instead of re-deriving it from the request — important when the server narrows scope relative to what was asked for. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 12 ++++++------ src/auth/providers/dcr.test.ts | 14 +++++++++++--- src/auth/providers/dcr.ts | 2 ++ src/auth/providers/oauth.ts | 4 ++++ src/auth/providers/pkce.ts | 2 ++ src/auth/types.ts | 7 +++++++ 6 files changed, 32 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 32ae094..af5ed04 100644 --- a/README.md +++ b/README.md @@ -586,12 +586,12 @@ A `TokenStore` MAY throw `CliError('AUTH_STORE_READ_FAILED', …)` from `active( Implement `AuthProvider` directly for device code, magic-link, username / password, or any other flow not covered by `createPkceProvider` / `createDcrProvider`. The four hooks fire in this order during `runOAuthFlow`: -| Hook | When | Purpose | -| --------------- | ---------------------------------- | ------------------------------------------------------------------------------------------------------------- | -| `prepare?` | Before the callback server binds | Pre-flight (e.g. DCR to mint a `client_id`). The returned `handshake` is threaded into every later hook. | -| `authorize` | After the callback server is up | Build the URL the user opens. Returns the URL plus any handshake state needed at exchange (PKCE verifier, …). | -| `exchangeCode` | After the browser callback fires | Trade the `code` for an `accessToken`. Optionally returns a fully-resolved `account` to skip `validateToken`. | -| `validateToken` | When `exchangeCode` had no account | Probe an authenticated endpoint to resolve the account. | +| Hook | When | Purpose | +| --------------- | ---------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `prepare?` | Before the callback server binds | Pre-flight (e.g. DCR to mint a `client_id`). The returned `handshake` is threaded into every later hook. | +| `authorize` | After the callback server is up | Build the URL the user opens. Returns the URL plus any handshake state needed at exchange (PKCE verifier, …). | +| `exchangeCode` | After the browser callback fires | Trade the `code` for an `accessToken`. May also return the server-granted `scope` (so `validateToken` records the authoritative scope) and optionally a fully-resolved `account` to skip `validateToken`. | +| `validateToken` | When `exchangeCode` had no account | Probe an authenticated endpoint to resolve the account. | Skeleton: diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts index df7e095..43ab3c0 100644 --- a/src/auth/providers/dcr.test.ts +++ b/src/auth/providers/dcr.test.ts @@ -396,7 +396,7 @@ describe('createDcrProvider', () => { const { calls, fetchImpl } = makeFetchRecorder((u) => u === REGISTRATION_URL ? registration({ client_id: 'pub' }) - : token({ access_token: 'tok', expires_in: 3600 }), + : token({ access_token: 'tok', expires_in: 3600, scope: 'user:read' }), ) const provider = createDcrProvider({ registrationUrl: REGISTRATION_URL, @@ -421,19 +421,26 @@ describe('createDcrProvider', () => { 'https://api.example.com', ) - await provider.exchangeCode({ + const result = await provider.exchangeCode({ code: 'auth-code', state: 'state-123', redirectUri: REDIRECT_URI, handshake: authorize.handshake, }) + // The server-granted scope is surfaced for validateToken to record. + expect(result.scope).toBe('user:read') const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) expect(tokenBody.get('resource')).toBe('https://api.example.com') }) it('refreshToken runs the refresh_token grant, forwarding the resource indicator', async () => { const { calls, fetchImpl } = makeFetchRecorder(() => - token({ access_token: 'tok-2', refresh_token: 'rt-2', expires_in: 3600 }), + token({ + access_token: 'tok-2', + refresh_token: 'rt-2', + expires_in: 3600, + scope: 'user:read', + }), ) const provider = createDcrProvider({ registrationUrl: REGISTRATION_URL, @@ -452,6 +459,7 @@ describe('createDcrProvider', () => { expect(result.accessToken).toBe('tok-2') expect(result.refreshToken).toBe('rt-2') expect(result.expiresAt).toBeGreaterThan(Date.now()) + expect(result.scope).toBe('user:read') const tokenBody = bodyOf(calls.find((c) => c.url === TOKEN_URL)!) expect(tokenBody.get('grant_type')).toBe('refresh_token') diff --git a/src/auth/providers/dcr.ts b/src/auth/providers/dcr.ts index 76f0239..369eb36 100644 --- a/src/auth/providers/dcr.ts +++ b/src/auth/providers/dcr.ts @@ -330,6 +330,7 @@ export function createDcrProvider( accessToken: result.access_token, refreshToken: result.refresh_token, expiresAt: expiresAtFromExpiresIn(result.expires_in), + scope: result.scope, } } catch (error) { throw mapOauthError( @@ -397,6 +398,7 @@ export function createDcrProvider( accessToken: result.access_token, refreshToken: result.refresh_token, expiresAt: expiresAtFromExpiresIn(result.expires_in), + scope: result.scope, } } catch (error) { throw mapRefreshError(error, oauth) diff --git a/src/auth/providers/oauth.ts b/src/auth/providers/oauth.ts index f106f9e..6deb6c4 100644 --- a/src/auth/providers/oauth.ts +++ b/src/auth/providers/oauth.ts @@ -151,6 +151,8 @@ type PostTokenEndpointResult = { refreshToken?: string /** Unix-epoch ms. Computed from `expires_in` when the server returns it. */ expiresAt?: number + /** Raw `scope` from the token response, when present (RFC 6749 §5.1). */ + scope?: string } /** @@ -174,6 +176,7 @@ export async function postTokenEndpoint( access_token?: string refresh_token?: string expires_in?: number + scope?: string }>({ url: input.url, headers, @@ -194,6 +197,7 @@ export async function postTokenEndpoint( accessToken: payload.access_token, refreshToken: payload.refresh_token, expiresAt: expiresAtFromExpiresIn(payload.expires_in), + scope: typeof payload.scope === 'string' ? payload.scope : undefined, } } diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index 870b243..33d7f55 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -148,6 +148,7 @@ export function createPkceProvider( accessToken: result.accessToken, refreshToken: result.refreshToken, expiresAt: result.expiresAt, + scope: result.scope, } }, @@ -191,6 +192,7 @@ export function createPkceProvider( accessToken: result.access_token, refreshToken: result.refresh_token, expiresAt: expiresAtFromExpiresIn(result.expires_in), + scope: result.scope, } } catch (error) { throw mapRefreshError(error, oauth) diff --git a/src/auth/types.ts b/src/auth/types.ts index c7cca04..b42ad9a 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -64,6 +64,13 @@ export type ExchangeResult = { expiresAt?: number /** Refresh-token expiry, unix-epoch ms. */ refreshTokenExpiresAt?: number + /** + * The raw `scope` from the token response, when the server returned one + * (RFC 6749 §5.1 — present especially when the granted scope differs from + * what was requested). Lets a provider's `validateToken` record the + * server-authoritative scope rather than re-deriving it from the request. + */ + scope?: string /** Set when the token endpoint already identifies the account; skips `validateToken`. */ account?: TAccount }