From 09cf471e57448f39a4a3aca46db4fb4c129befa8 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 18:12:50 +0100 Subject: [PATCH 1/3] feat(auth): add createDcrProvider for RFC 7591 dynamic client registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ships the missing OAuth provider factory for flows that mint a per-install `client_id` / `client_secret` via [RFC 7591](https://datatracker.ietf.org/doc/html/rfc7591). `prepare()` POSTs the client metadata, threads the issued credentials through the handshake, and `exchangeCode()` authenticates the token POST per `tokenEndpointAuthMethod` (`client_secret_basic` default, `_post`, or `none` for public-client). Mirrors `createPkceProvider`'s ergonomics — caller supplies `validate`; same `PkceLazyString` resolvers for URLs. Also: - Extract a private `_oauth.ts` with `postTokenEndpoint`, `buildPkceAuthorizeUrl`, `buildAuthError`, `resolve`, `safeReadText`. `pkce.ts` is now thin (≈115 LOC). The helper is grant-agnostic so a future refresh-token feature reuses it unchanged — `body: new URLSearchParams({grant_type: 'refresh_token', …})`. - New `errorHints?: string[]` option on both factories. Prepended to every `CliError` they throw; server-returned body text (on non-2xx) is appended after so the actionable hint stays at the top. - New `AUTH_DCR_FAILED` error code on the auth-error union. No external API change for existing `createPkceProvider` consumers. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 61 ++++-- src/auth/errors.ts | 1 + src/auth/index.ts | 2 + src/auth/providers/_oauth.test.ts | 230 ++++++++++++++++++++ src/auth/providers/_oauth.ts | 153 +++++++++++++ src/auth/providers/dcr.test.ts | 349 ++++++++++++++++++++++++++++++ src/auth/providers/dcr.ts | 240 ++++++++++++++++++++ src/auth/providers/pkce.ts | 114 +++------- 8 files changed, 1050 insertions(+), 100 deletions(-) create mode 100644 src/auth/providers/_oauth.test.ts create mode 100644 src/auth/providers/_oauth.ts create mode 100644 src/auth/providers/dcr.test.ts create mode 100644 src/auth/providers/dcr.ts diff --git a/README.md b/README.md index 8c3b595..cdc7c15 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`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`), 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). `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke backends. `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), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) 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` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | +| Module | Key exports | Purpose | +| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createDcrProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` / `SecureStore` / `UserRecordStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `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). `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), and `@napi-rs/keyring` (when using `createSecureStore` or the keyring `TokenStore`) 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` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | ## Usage @@ -124,7 +124,7 @@ The semver helpers (`parseVersion`, `compareVersions`, `isNewer`, `getInstallTag ### Auth (optional subpath) -Wire ` [auth] login` and the supporting OAuth runtime. cli-core ships the standard public-client PKCE flow (`createPkceProvider`) and the `attachLoginCommand` Commander helper that drives `runOAuthFlow` end-to-end. Bespoke flows (Dynamic Client Registration, device code, magic link, username / password) implement the `AuthProvider` interface directly — no cli-core release needed. Token storage is a `TokenStore` the consumer provides; cli-core does not ship a default. +Wire ` [auth] login` and the supporting OAuth runtime. cli-core ships the standard public-client PKCE flow (`createPkceProvider`), the RFC 7591 Dynamic Client Registration flow (`createDcrProvider`), and the `attachLoginCommand` Commander helper that drives `runOAuthFlow` end-to-end. Other bespoke flows (device code, magic link, username / password) implement the `AuthProvider` interface directly — no cli-core release needed. Token storage is a `TokenStore` the consumer provides; cli-core does not ship a default. #### Install @@ -169,6 +169,32 @@ attachLoginCommand(auth, { `attachLoginCommand` returns the new `Command` so you can chain `.description(...)` / `.option(...)` / `.addHelpText(...)`. Any consumer-attached options land in the `flags` object passed to `resolveScopes`, `onSuccess`, and the provider hooks. +#### 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` runs the registration POST in `prepare()`, then drives the standard PKCE authorize / token-exchange dance against the resulting client. + +```ts +import { attachLoginCommand, createDcrProvider } from '@doist/cli-core/auth' + +const provider = createDcrProvider({ + registrationUrl: 'https://example.com/oauth/register', + authorizeUrl: 'https://example.com/oauth/authorize', + tokenUrl: 'https://example.com/oauth/token', + clientMetadata: { + clientName: 'Example CLI', + clientUri: 'https://github.com/example/cli', + logoUri: 'https://example.com/logo.png', + applicationType: 'native', + tokenEndpointAuthMethod: 'client_secret_basic', // default + }, + validate: async ({ token }) => probeUser(token), +}) +``` + +The DCR-issued `client_id` (and `client_secret`, if returned) are stashed in the handshake and threaded through the rest of the flow. By default the token exchange uses `Authorization: Basic `; 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. + +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. + #### Sibling attachers (`logout` / `status` / `token`) The same registrar shape covers the other three auth subcommands. Each returns the new `Command` for chaining and shares the same `TokenStore` instance. @@ -423,9 +449,9 @@ Account-selection resolvers (env > `--user` > default > single-only > error), `a A `TokenStore` MAY throw `CliError('AUTH_STORE_READ_FAILED', …)` from `active(ref)` when a matching record exists but the token itself can't be read (e.g. an OS keyring backing the store is offline). `attachLogoutCommand` catches this specific code on the explicit-ref path and proceeds with `clear(ref)` — local logout doesn't need the token, and the `revokeToken` hook is skipped because there's no token to send. Every other error from `active(ref)` (notably `ACCOUNT_NOT_FOUND` from a genuine ref miss, plus any consumer-thrown code) still propagates so a real miss isn't masked. Without `--user`, the logout pre-flight swallows any snapshot read failure so the local clear always runs. `attachStatusCommand` and `attachTokenViewCommand` propagate `AUTH_STORE_READ_FAILED` since they have no way to render or print without the token. -#### Custom `AuthProvider` (non-PKCE flows) +#### Custom `AuthProvider` (non-PKCE, non-DCR flows) -Implement `AuthProvider` directly for Dynamic Client Registration, device code, magic-link, etc. The four hooks fire in this order during `runOAuthFlow`: +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 | | --------------- | ---------------------------------- | ------------------------------------------------------------------------------------------------------------- | @@ -469,6 +495,7 @@ Every failure in this subpath surfaces as a `CliError`: | `AUTH_OAUTH_FAILED` | Provider returned `?error=...`, the flow was aborted via `signal`, or the callback server stopped before completion. | | `AUTH_CALLBACK_TIMEOUT` | No valid callback within `timeoutMs` (default 3 minutes). | | `AUTH_PORT_BIND_FAILED` | Could not bind any port in `[preferredPort, preferredPort + portFallbackCount]`, or `--callback-port` was out of range. | +| `AUTH_DCR_FAILED` | `createDcrProvider` registration POST failed (network error, non-2xx, non-JSON body, or response missing `client_id`). | | `AUTH_TOKEN_EXCHANGE_FAILED` | Token endpoint network error, non-2xx response, non-JSON body, or missing `access_token`. | | `AUTH_STORE_WRITE_FAILED` | `TokenStore.set` threw a non-`CliError`. (`CliError`s thrown from `set` propagate unchanged.) | | `NOT_AUTHENTICATED` | `status` / `token` ran with an empty `TokenStore` (and no `onNotAuthenticated` callback for `status`). Default message: `'Not signed in.'`. | diff --git a/src/auth/errors.ts b/src/auth/errors.ts index bf77311..3cd5c7d 100644 --- a/src/auth/errors.ts +++ b/src/auth/errors.ts @@ -7,6 +7,7 @@ export type AuthErrorCode = | 'AUTH_OAUTH_FAILED' | 'AUTH_CALLBACK_TIMEOUT' | 'AUTH_PORT_BIND_FAILED' + | 'AUTH_DCR_FAILED' | 'AUTH_TOKEN_EXCHANGE_FAILED' | 'AUTH_STORE_WRITE_FAILED' | 'AUTH_STORE_READ_FAILED' diff --git a/src/auth/index.ts b/src/auth/index.ts index 90858c9..8ca936f 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -22,6 +22,8 @@ export { export type { GenerateVerifierOptions } from './pkce.js' export { createPkceProvider } from './providers/pkce.js' export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' +export { createDcrProvider } from './providers/dcr.js' +export type { DcrClientMetadata, DcrProviderOptions } from './providers/dcr.js' export type { AccountRef, AuthAccount, diff --git a/src/auth/providers/_oauth.test.ts b/src/auth/providers/_oauth.test.ts new file mode 100644 index 0000000..f434342 --- /dev/null +++ b/src/auth/providers/_oauth.test.ts @@ -0,0 +1,230 @@ +import { describe, expect, it } from 'vitest' + +import { buildPkceAuthorizeUrl, postTokenEndpoint, resolve, safeReadText } from './_oauth.js' + +const TOKEN_URL = 'https://example.com/oauth/token' + +describe('resolve', () => { + it('returns literal strings unchanged', () => { + expect(resolve('static', {}, {})).toBe('static') + }) + + it('passes handshake + flags to function resolvers', () => { + const got = resolve( + ({ handshake, flags }) => `${handshake.base as string}|${flags.clientId as string}`, + { base: 'https://x' }, + { clientId: 'cid' }, + ) + expect(got).toBe('https://x|cid') + }) +}) + +describe('safeReadText', () => { + it('returns trimmed body text', async () => { + const res = new Response(' body text ', { status: 400 }) + expect(await safeReadText(res)).toBe('body text') + }) + + it('returns undefined for empty bodies', async () => { + const res = new Response('', { status: 400 }) + expect(await safeReadText(res)).toBeUndefined() + }) +}) + +describe('buildPkceAuthorizeUrl', () => { + it('sets PKCE params and joins scopes with the supplied separator', () => { + const url = new URL( + buildPkceAuthorizeUrl({ + authorizeUrl: 'https://example.com/oauth/authorize', + clientId: 'cid', + redirectUri: 'http://localhost/cb', + state: 'st', + scopes: ['a', 'b'], + scopeSeparator: ',', + codeChallenge: 'challenge-1', + }), + ) + expect(url.searchParams.get('response_type')).toBe('code') + expect(url.searchParams.get('client_id')).toBe('cid') + expect(url.searchParams.get('redirect_uri')).toBe('http://localhost/cb') + expect(url.searchParams.get('state')).toBe('st') + expect(url.searchParams.get('code_challenge')).toBe('challenge-1') + expect(url.searchParams.get('code_challenge_method')).toBe('S256') + expect(url.searchParams.get('scope')).toBe('a,b') + }) + + it('omits scope when scopes is empty', () => { + const url = new URL( + buildPkceAuthorizeUrl({ + authorizeUrl: 'https://example.com/oauth/authorize', + clientId: 'cid', + redirectUri: 'http://localhost/cb', + state: 'st', + scopes: [], + scopeSeparator: ' ', + codeChallenge: 'c', + }), + ) + expect(url.searchParams.has('scope')).toBe(false) + }) +}) + +describe('postTokenEndpoint', () => { + it('POSTs the form body, returns access_token + refresh_token + expiresAt', async () => { + let captured: { url: string; init: RequestInit } | undefined + const result = await postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams({ grant_type: 'authorization_code', code: 'c' }), + fetchImpl: ((url, init = {}) => { + captured = { url: String(url), init } + return Promise.resolve( + new Response( + JSON.stringify({ + access_token: 'tok', + refresh_token: 'rtok', + expires_in: 60, + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ), + ) + }) as typeof fetch, + }) + expect(result.accessToken).toBe('tok') + expect(result.refreshToken).toBe('rtok') + expect(result.expiresAt).toBeGreaterThan(Date.now()) + expect(captured?.url).toBe(TOKEN_URL) + expect(captured?.init.method).toBe('POST') + const headers = captured?.init.headers as Record + expect(headers['Content-Type']).toBe('application/x-www-form-urlencoded') + expect(headers.Authorization).toBeUndefined() + }) + + it('adds Authorization: Basic when basicAuth is supplied', async () => { + let headers: Record | undefined + await postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams({ grant_type: 'authorization_code' }), + basicAuth: { clientId: 'cid', clientSecret: 'sec' }, + fetchImpl: ((_url, init = {}) => { + headers = init.headers as Record + return Promise.resolve( + new Response(JSON.stringify({ access_token: 'x' }), { status: 200 }), + ) + }) as typeof fetch, + }) + expect(headers?.Authorization).toBe( + `Basic ${Buffer.from('cid:sec', 'utf8').toString('base64')}`, + ) + }) + + it('refresh_token grants reuse the same helper', async () => { + let body: string | undefined + const result = await postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: 'old-rtok', + client_id: 'cid', + }), + fetchImpl: ((_url, init = {}) => { + body = init.body as string + return Promise.resolve( + new Response( + JSON.stringify({ access_token: 'new', refresh_token: 'new-rtok' }), + { status: 200 }, + ), + ) + }) as typeof fetch, + }) + expect(result.accessToken).toBe('new') + expect(result.refreshToken).toBe('new-rtok') + expect(new URLSearchParams(body).get('grant_type')).toBe('refresh_token') + }) + + it('network errors throw AUTH_TOKEN_EXCHANGE_FAILED', async () => { + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + fetchImpl: (() => Promise.reject(new Error('econnrefused'))) as typeof fetch, + }), + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + }) + + it('non-2xx surfaces user errorHints first, then the body as a hint', async () => { + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + errorHints: ['Re-run login'], + fetchImpl: (() => + Promise.resolve( + new Response('invalid_grant', { status: 400 }), + )) as typeof fetch, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: ['Re-run login', 'invalid_grant'], + }) + }) + + it('2xx HTML / non-JSON body throws AUTH_TOKEN_EXCHANGE_FAILED', async () => { + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + fetchImpl: (() => + Promise.resolve( + new Response('oops', { + status: 200, + headers: { 'Content-Type': 'text/html' }, + }), + )) as typeof fetch, + }), + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + }) + + it('errorHints are prepended to every CliError, with body text appended after on non-2xx', async () => { + // Non-2xx: both user hints AND body detail. + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + errorHints: ['Re-run login', 'Set TWIST_API_TOKEN'], + fetchImpl: (() => + Promise.resolve( + new Response('invalid_grant', { status: 400 }), + )) as typeof fetch, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: ['Re-run login', 'Set TWIST_API_TOKEN', 'invalid_grant'], + }) + + // Network failure: user hints only (no server detail to append). + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + errorHints: ['Re-run login'], + fetchImpl: (() => Promise.reject(new Error('econnrefused'))) as typeof fetch, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: ['Re-run login'], + }) + }) + + it('missing access_token throws AUTH_TOKEN_EXCHANGE_FAILED', async () => { + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + fetchImpl: (() => + Promise.resolve( + new Response(JSON.stringify({ refresh_token: 'r' }), { status: 200 }), + )) as typeof fetch, + }), + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + }) +}) diff --git a/src/auth/providers/_oauth.ts b/src/auth/providers/_oauth.ts new file mode 100644 index 0000000..6e12695 --- /dev/null +++ b/src/auth/providers/_oauth.ts @@ -0,0 +1,153 @@ +import { CliError, getErrorMessage } from '../../errors.js' +import type { AuthErrorCode } from '../errors.js' +import type { PkceLazyString } from './pkce.js' + +/** + * Build a `CliError` with user-supplied `errorHints` prepended and an optional + * server-derived `extra` detail appended. Centralises the "user-actionable + * first, diagnostic second" ordering used everywhere in this directory. + */ +export function buildAuthError( + code: AuthErrorCode, + message: string, + userHints: string[] | undefined, + extra?: string, +): CliError { + const hints = [...(userHints ?? []), ...(extra ? [extra] : [])] + return new CliError(code, message, hints.length > 0 ? { hints } : {}) +} + +/** + * Resolve a literal-or-function endpoint/clientId against the current handshake + * and runtime flags. Used by every provider in this directory. + */ +export function resolve( + resolver: PkceLazyString, + handshake: Record, + flags: Record, +): string { + return typeof resolver === 'function' ? resolver({ handshake, flags }) : resolver +} + +/** Read a response body without letting a stream error escape — used for hints. */ +export async function safeReadText(response: Response): Promise { + try { + const text = (await response.text()).trim() + return text.length > 0 ? text : undefined + } catch { + return undefined + } +} + +export type BuildPkceAuthorizeUrlInput = { + authorizeUrl: string + clientId: string + redirectUri: string + state: string + scopes: string[] + scopeSeparator: string + codeChallenge: string +} + +/** Construct the standard PKCE S256 authorize URL. */ +export function buildPkceAuthorizeUrl(input: BuildPkceAuthorizeUrlInput): string { + const url = new URL(input.authorizeUrl) + url.searchParams.set('response_type', 'code') + url.searchParams.set('client_id', input.clientId) + url.searchParams.set('redirect_uri', input.redirectUri) + url.searchParams.set('state', input.state) + url.searchParams.set('code_challenge', input.codeChallenge) + url.searchParams.set('code_challenge_method', 'S256') + if (input.scopes.length > 0) { + url.searchParams.set('scope', input.scopes.join(input.scopeSeparator)) + } + return url.toString() +} + +export type PostTokenEndpointInput = { + url: string + /** Form-encoded body. Caller owns grant_type + grant-specific params. */ + body: URLSearchParams + /** When present, sent as `Authorization: Basic base64(clientId:clientSecret)`. */ + basicAuth?: { clientId: string; clientSecret: string } + /** + * User-facing remediation hints attached to every `CliError` this helper + * throws (network failure, non-2xx, parse failure, missing access_token). + * The server-returned response body (for non-2xx) is appended after these + * so user hints stay at the top. + */ + errorHints?: string[] + fetchImpl: typeof fetch +} + +export type PostTokenEndpointResult = { + accessToken: string + refreshToken?: string + /** Unix-epoch ms. Computed from `expires_in` when the server returns it. */ + expiresAt?: number +} + +/** + * POST to an OAuth 2.0 token endpoint and parse the standard JSON response. + * The same shape covers `authorization_code` (PKCE / DCR exchange) and + * `refresh_token` grants — the caller picks the grant by populating `body`. + * + * Failures uniformly throw `CliError('AUTH_TOKEN_EXCHANGE_FAILED', …)`: + * network errors, non-2xx responses (with body text as a hint), non-JSON + * bodies (the misconfigured-proxy HTML case), and responses missing + * `access_token`. + */ +export async function postTokenEndpoint( + input: PostTokenEndpointInput, +): Promise { + const headers: Record = { + 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json', + } + if (input.basicAuth) { + const encoded = Buffer.from( + `${input.basicAuth.clientId}:${input.basicAuth.clientSecret}`, + 'utf8', + ).toString('base64') + headers.Authorization = `Basic ${encoded}` + } + + const fail = (message: string, extra?: string): CliError => + buildAuthError('AUTH_TOKEN_EXCHANGE_FAILED', message, input.errorHints, extra) + + let response: Response + try { + response = await input.fetchImpl(input.url, { + method: 'POST', + headers, + body: input.body.toString(), + }) + } catch (error) { + throw fail(`Token endpoint request failed: ${getErrorMessage(error)}`) + } + + if (!response.ok) { + const detail = await safeReadText(response) + throw fail(`Token endpoint returned HTTP ${response.status}.`, detail) + } + + // Parse defensively — a misconfigured proxy can return a 2xx HTML error + // page that would otherwise blow up with a raw SyntaxError. + let payload: { access_token?: string; refresh_token?: string; expires_in?: number } + try { + payload = (await response.json()) as typeof payload + } catch (error) { + throw fail(`Token endpoint returned non-JSON response: ${getErrorMessage(error)}`) + } + if (!payload.access_token) { + throw fail('Token endpoint response missing access_token.') + } + return { + accessToken: payload.access_token, + refreshToken: payload.refresh_token, + expiresAt: + typeof payload.expires_in === 'number' + ? Date.now() + payload.expires_in * 1000 + : undefined, + } +} diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts new file mode 100644 index 0000000..bb06bc3 --- /dev/null +++ b/src/auth/providers/dcr.test.ts @@ -0,0 +1,349 @@ +import { describe, expect, it } from 'vitest' + +import { createDcrProvider } from './dcr.js' + +type Account = { id: string; label?: string } + +const respond = (body: unknown, status = 200): Response => + new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }) + +const validate = async () => ({ id: '1' }) as Account + +const REGISTRATION_URL = 'https://example.com/oauth/register' +const AUTHORIZE_URL = 'https://example.com/oauth/authorize' +const TOKEN_URL = 'https://example.com/oauth/token' +const REDIRECT_URI = 'http://localhost:8765/callback' + +type FetchCall = { url: string; init: RequestInit } + +function makeFetchRecorder(handler: (url: string) => Response): { + calls: FetchCall[] + fetchImpl: typeof fetch +} { + const calls: FetchCall[] = [] + const fetchImpl = ((url: RequestInfo | URL, init: RequestInit = {}) => { + const u = String(url) + calls.push({ url: u, init }) + return Promise.resolve(handler(u)) + }) as typeof fetch + return { calls, fetchImpl } +} + +describe('createDcrProvider', () => { + it('prepare POSTs RFC 7591 metadata, authorize uses the issued client_id, exchangeCode sends Basic auth', async () => { + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ client_id: 'issued-id', client_secret: 'issued-secret' }) + : respond({ access_token: 'tok-1', expires_in: 3600 }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { + clientName: 'Twist CLI', + clientUri: 'https://github.com/doist/twist-cli', + logoUri: 'https://example.com/logo.png', + applicationType: 'native', + }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(prepared.handshake).toEqual({ clientId: 'issued-id', clientSecret: 'issued-secret' }) + + const regBody = JSON.parse(calls[0].init.body as string) as Record + expect(regBody).toMatchObject({ + client_name: 'Twist CLI', + client_uri: 'https://github.com/doist/twist-cli', + logo_uri: 'https://example.com/logo.png', + application_type: 'native', + redirect_uris: [REDIRECT_URI], + grant_types: ['authorization_code'], + response_types: ['code'], + token_endpoint_auth_method: 'client_secret_basic', + }) + + const authorize = await provider.authorize({ + redirectUri: REDIRECT_URI, + state: 'state-123', + scopes: ['user:read', 'threads:read'], + readOnly: false, + flags: {}, + handshake: prepared.handshake, + }) + const url = new URL(authorize.authorizeUrl) + expect(url.searchParams.get('client_id')).toBe('issued-id') + expect(url.searchParams.get('redirect_uri')).toBe(REDIRECT_URI) + expect(url.searchParams.get('state')).toBe('state-123') + expect(url.searchParams.get('code_challenge_method')).toBe('S256') + expect(url.searchParams.get('code_challenge')).toMatch(/^[A-Za-z0-9_-]+$/) + expect(url.searchParams.get('scope')).toBe('user:read threads:read') + expect(typeof authorize.handshake.codeVerifier).toBe('string') + expect(authorize.handshake.clientSecret).toBe('issued-secret') + + const result = await provider.exchangeCode({ + code: 'auth-code', + state: 'state-123', + redirectUri: REDIRECT_URI, + handshake: authorize.handshake, + }) + expect(result.accessToken).toBe('tok-1') + expect(result.expiresAt).toBeGreaterThan(Date.now()) + + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenHeaders = tokenCall.init.headers as Record + expect(tokenHeaders.Authorization).toBe( + `Basic ${Buffer.from('issued-id:issued-secret', 'utf8').toString('base64')}`, + ) + const tokenBody = new URLSearchParams(tokenCall.init.body as string) + expect(tokenBody.get('grant_type')).toBe('authorization_code') + expect(tokenBody.get('code')).toBe('auth-code') + expect(tokenBody.get('redirect_uri')).toBe(REDIRECT_URI) + expect(tokenBody.get('code_verifier')).toBe(authorize.handshake.codeVerifier as string) + expect(tokenBody.has('client_id')).toBe(false) + expect(tokenBody.has('client_secret')).toBe(false) + }) + + it('client_secret_post puts the secret in the body, omits the Authorization header, and forwards the auth method in the registration POST', async () => { + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ client_id: 'cid', client_secret: 'sec' }) + : respond({ access_token: 'tok-2' }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_post' }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + const regBody = JSON.parse(calls[0].init.body as string) as Record + expect(regBody.token_endpoint_auth_method).toBe('client_secret_post') + + await provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }) + + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenHeaders = tokenCall.init.headers as Record + const tokenBody = new URLSearchParams(tokenCall.init.body as string) + expect(tokenHeaders.Authorization).toBeUndefined() + expect(tokenBody.get('client_id')).toBe('cid') + expect(tokenBody.get('client_secret')).toBe('sec') + }) + + it('tokenEndpointAuthMethod=none (or missing client_secret) sends client_id in the body and no Authorization header', async () => { + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ client_id: 'pub-cid' }) // public-client DCR: no client_secret + : respond({ access_token: 'tok-3' }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'none' }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(prepared.handshake.clientSecret).toBeUndefined() + + await provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }) + + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenHeaders = tokenCall.init.headers as Record + const tokenBody = new URLSearchParams(tokenCall.init.body as string) + expect(tokenHeaders.Authorization).toBeUndefined() + expect(tokenBody.get('client_id')).toBe('pub-cid') + expect(tokenBody.has('client_secret')).toBe(false) + }) + + it('DCR non-2xx is AUTH_DCR_FAILED; errorHints are prepended before the body text', async () => { + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + errorHints: ['Re-run: cli auth login'], + fetchImpl: (() => + Promise.resolve( + new Response('invalid_redirect_uri', { status: 400 }), + )) as typeof fetch, + }) + await expect( + provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ + code: 'AUTH_DCR_FAILED', + hints: ['Re-run: cli auth login', 'invalid_redirect_uri'], + }) + }) + + it('DCR response missing client_id is AUTH_DCR_FAILED', async () => { + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + fetchImpl: (() => Promise.resolve(respond({ client_secret: 'sec' }))) as typeof fetch, + }) + await expect( + provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) + }) + + it('DCR non-JSON response is AUTH_DCR_FAILED', async () => { + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + fetchImpl: (() => + Promise.resolve( + new Response('oops', { + status: 200, + headers: { 'Content-Type': 'text/html' }, + }), + )) as typeof fetch, + }) + await expect( + provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) + }) + + it('token endpoint non-2xx becomes AUTH_TOKEN_EXCHANGE_FAILED', async () => { + const { fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ client_id: 'cid', client_secret: 'sec' }) + : new Response('invalid_grant', { status: 400 }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + fetchImpl, + }) + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + await expect( + provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }), + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + }) + + it('errorHints are prepended to every error (DCR failure + token-exchange failure), with body text appended on non-2xx', async () => { + const userHints = ['Re-run: cli auth login', 'Or set CLI_API_TOKEN'] + + // DCR non-2xx + const dcrProvider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + errorHints: userHints, + fetchImpl: (() => + Promise.resolve(new Response('rate_limited', { status: 429 }))) as typeof fetch, + }) + await expect( + dcrProvider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ + code: 'AUTH_DCR_FAILED', + hints: [...userHints, 'rate_limited'], + }) + + // DCR missing client_id (no server body to append) + const noClientIdProvider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + errorHints: userHints, + fetchImpl: (() => Promise.resolve(respond({}))) as typeof fetch, + }) + await expect( + noClientIdProvider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED', hints: userHints }) + + // Token endpoint failure flows through postTokenEndpoint + const exchangeProvider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + errorHints: userHints, + fetchImpl: ((url: RequestInfo | URL) => + String(url) === REGISTRATION_URL + ? Promise.resolve(respond({ client_id: 'cid', client_secret: 'sec' })) + : Promise.resolve( + new Response('invalid_grant', { status: 400 }), + )) as typeof fetch, + }) + const prepared = await exchangeProvider.prepare!({ + redirectUri: REDIRECT_URI, + flags: {}, + }) + await expect( + exchangeProvider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: [...userHints, 'invalid_grant'], + }) + }) + + it('clientMetadata.extra fields appear in the registration POST body verbatim; named fields win on collisions', async () => { + const { calls, fetchImpl } = makeFetchRecorder(() => respond({ client_id: 'cid' })) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { + clientName: 'CLI', + extra: { + software_statement: 'eyJhbGciOiJSUzI1NiJ9.test', + contacts: ['ops@example.com'], + client_name: 'should-be-overridden', + }, + }, + validate, + fetchImpl, + }) + await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + const body = JSON.parse(calls[0].init.body as string) as Record + expect(body.software_statement).toBe('eyJhbGciOiJSUzI1NiJ9.test') + expect(body.contacts).toEqual(['ops@example.com']) + expect(body.client_name).toBe('CLI') + }) +}) diff --git a/src/auth/providers/dcr.ts b/src/auth/providers/dcr.ts new file mode 100644 index 0000000..9ad17c9 --- /dev/null +++ b/src/auth/providers/dcr.ts @@ -0,0 +1,240 @@ +import { getErrorMessage } from '../../errors.js' +import { deriveChallenge, generateVerifier } from '../pkce.js' +import type { + AuthAccount, + AuthProvider, + AuthorizeInput, + AuthorizeResult, + ExchangeInput, + ExchangeResult, + PrepareInput, + PrepareResult, + ValidateInput, +} from '../types.js' +import { + buildAuthError, + buildPkceAuthorizeUrl, + postTokenEndpoint, + resolve, + safeReadText, +} from './_oauth.js' +import type { PkceLazyString } from './pkce.js' + +/** + * RFC 7591 Dynamic Client Registration metadata POSTed to the registration + * endpoint. Only fields the CLI typically cares about are named; pass anything + * else (`software_statement`, `jwks`, …) via `extra`. + */ +export type DcrClientMetadata = { + clientName: string + clientUri?: string + logoUri?: string + applicationType?: 'native' | 'web' + /** How the token endpoint will be authenticated. Defaults to `'client_secret_basic'`. */ + tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post' | 'none' + /** Defaults to `['authorization_code']`. */ + grantTypes?: string[] + /** Defaults to `['code']`. */ + responseTypes?: string[] + /** Merged verbatim into the registration POST body. */ + extra?: Record +} + +export type DcrProviderOptions = { + /** RFC 7591 registration endpoint. Function form supports per-flow base URLs. */ + registrationUrl: PkceLazyString + /** OAuth 2.0 authorize endpoint. */ + authorizeUrl: PkceLazyString + /** OAuth 2.0 token endpoint. */ + tokenUrl: PkceLazyString + clientMetadata: DcrClientMetadata + /** How to join scopes in the authorize URL. Default `' '` (RFC 6749). */ + scopeSeparator?: string + verifierAlphabet?: string + /** Default 64. */ + verifierLength?: number + /** Probe an authenticated endpoint to confirm the token works and resolve the account. */ + validate: (input: ValidateInput) => Promise + /** + * User-facing remediation hints attached to every CliError this factory + * throws (`AUTH_DCR_FAILED` from `prepare()` / `authorize()` and + * `AUTH_TOKEN_EXCHANGE_FAILED` from `exchangeCode()`). Server-returned + * response bodies are appended after these so the actionable hint stays + * first. + */ + errorHints?: string[] + /** Inject a fetch implementation (tests). */ + fetchImpl?: typeof fetch +} + +/** + * Build an `AuthProvider` for the RFC 7591 Dynamic Client Registration flow. + * + * - `prepare`: POST `clientMetadata` to `registrationUrl`. Stash the issued + * `client_id` (and `client_secret` if returned) in the handshake. + * - `authorize`: standard PKCE S256 with `client_id` read from the handshake. + * - `exchangeCode`: token endpoint POST, authenticated per the metadata's + * `tokenEndpointAuthMethod` — Basic auth header for `client_secret_basic`, + * secret in the body for `client_secret_post`, neither for `none` (or when + * the registration response carried no `client_secret`). + * - `validateToken`: caller-supplied. + */ +export function createDcrProvider( + options: DcrProviderOptions, +): AuthProvider { + const fetchImpl = options.fetchImpl ?? fetch + const scopeSeparator = options.scopeSeparator ?? ' ' + const tokenEndpointAuthMethod = + options.clientMetadata.tokenEndpointAuthMethod ?? 'client_secret_basic' + + return { + async prepare(input: PrepareInput): Promise { + const registrationUrl = resolve(options.registrationUrl, {}, input.flags) + const registrationBody = buildRegistrationBody( + options.clientMetadata, + input.redirectUri, + tokenEndpointAuthMethod, + ) + + const fail = (message: string, extra?: string) => + buildAuthError('AUTH_DCR_FAILED', message, options.errorHints, extra) + + let response: Response + try { + response = await fetchImpl(registrationUrl, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify(registrationBody), + }) + } catch (error) { + throw fail(`Registration endpoint request failed: ${getErrorMessage(error)}`) + } + + if (!response.ok) { + const detail = await safeReadText(response) + throw fail(`Registration endpoint returned HTTP ${response.status}.`, detail) + } + + let payload: { client_id?: string; client_secret?: string } + try { + payload = (await response.json()) as typeof payload + } catch (error) { + throw fail( + `Registration endpoint returned non-JSON response: ${getErrorMessage(error)}`, + ) + } + if (!payload.client_id) { + throw fail('Registration response missing client_id.') + } + + const handshake: Record = { clientId: payload.client_id } + if (payload.client_secret) handshake.clientSecret = payload.client_secret + return { handshake } + }, + + async authorize(input: AuthorizeInput): Promise { + const clientId = input.handshake.clientId + if (typeof clientId !== 'string') { + throw buildAuthError( + 'AUTH_DCR_FAILED', + 'Internal: DCR handshake missing clientId before authorize.', + options.errorHints, + ) + } + + const verifier = generateVerifier({ + alphabet: options.verifierAlphabet, + length: options.verifierLength, + }) + const challenge = deriveChallenge(verifier) + const authorizeUrl = buildPkceAuthorizeUrl({ + authorizeUrl: resolve(options.authorizeUrl, input.handshake, input.flags), + clientId, + redirectUri: input.redirectUri, + state: input.state, + scopes: input.scopes, + scopeSeparator, + codeChallenge: challenge, + }) + + return { + authorizeUrl, + handshake: { ...input.handshake, codeVerifier: verifier }, + } + }, + + async exchangeCode(input: ExchangeInput): Promise> { + const verifier = input.handshake.codeVerifier + const clientId = input.handshake.clientId + if (typeof verifier !== 'string' || typeof clientId !== 'string') { + throw buildAuthError( + 'AUTH_TOKEN_EXCHANGE_FAILED', + 'Internal: DCR handshake state lost between authorize and exchange.', + options.errorHints, + ) + } + const clientSecretRaw = input.handshake.clientSecret + const clientSecret = typeof clientSecretRaw === 'string' ? clientSecretRaw : undefined + const flags = (input.handshake.flags as Record | undefined) ?? {} + const tokenUrl = resolve(options.tokenUrl, input.handshake, flags) + + const body = new URLSearchParams({ + grant_type: 'authorization_code', + code: input.code, + redirect_uri: input.redirectUri, + code_verifier: verifier, + }) + + // Public-client fallback: a registration with no `client_secret` + // can't authenticate Basic/Post regardless of the requested method, + // so we POST `client_id` in the body like a non-confidential + // client. Otherwise honour the configured auth method. + let basicAuth: { clientId: string; clientSecret: string } | undefined + if (!clientSecret || tokenEndpointAuthMethod === 'none') { + body.set('client_id', clientId) + } else if (tokenEndpointAuthMethod === 'client_secret_post') { + body.set('client_id', clientId) + body.set('client_secret', clientSecret) + } else { + basicAuth = { clientId, clientSecret } + } + + const result = await postTokenEndpoint({ + url: tokenUrl, + body, + basicAuth, + errorHints: options.errorHints, + fetchImpl, + }) + return { + accessToken: result.accessToken, + refreshToken: result.refreshToken, + expiresAt: result.expiresAt, + } + }, + + validateToken: options.validate, + } +} + +function buildRegistrationBody( + metadata: DcrClientMetadata, + redirectUri: string, + tokenEndpointAuthMethod: NonNullable, +): Record { + const body: Record = { + ...metadata.extra, + client_name: metadata.clientName, + redirect_uris: [redirectUri], + grant_types: metadata.grantTypes ?? ['authorization_code'], + response_types: metadata.responseTypes ?? ['code'], + token_endpoint_auth_method: tokenEndpointAuthMethod, + } + if (metadata.clientUri) body.client_uri = metadata.clientUri + if (metadata.logoUri) body.logo_uri = metadata.logoUri + if (metadata.applicationType) body.application_type = metadata.applicationType + return body +} diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index 2e641a7..393a330 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -1,4 +1,3 @@ -import { CliError, getErrorMessage } from '../../errors.js' import { deriveChallenge, generateVerifier } from '../pkce.js' import type { AuthAccount, @@ -9,6 +8,7 @@ import type { ExchangeResult, ValidateInput, } from '../types.js' +import { buildAuthError, buildPkceAuthorizeUrl, postTokenEndpoint, resolve } from './_oauth.js' /** * Lazy resolver: a literal string, or a function that builds one from the @@ -33,6 +33,13 @@ export type PkceProviderOptions = { verifierLength?: number /** Probe an authenticated endpoint to confirm the token works and resolve the account. */ validate: (input: ValidateInput) => Promise + /** + * User-facing remediation hints attached to every CliError this factory + * throws (token-endpoint failures, internal handshake-state guards). + * Server-returned response bodies are appended after these so the + * actionable hint stays first. + */ + errorHints?: string[] /** Inject a fetch implementation (tests). */ fetchImpl?: typeof fetch } @@ -47,8 +54,8 @@ export type PkceProviderOptions = { * `runOAuthFlow` and arrives on `AuthorizeInput.scopes`; this factory does * not own scope resolution. * - * Flows that need DCR or HTTP Basic auth on the token endpoint implement - * the `AuthProvider` interface directly. + * Flows that need DCR or HTTP Basic auth on the token endpoint use + * `createDcrProvider` (or implement the `AuthProvider` interface directly). */ export function createPkceProvider( options: PkceProviderOptions, @@ -64,21 +71,18 @@ export function createPkceProvider( }) const challenge = deriveChallenge(verifier) const clientId = resolve(options.clientId, input.handshake, input.flags) - const authorizeUrl = resolve(options.authorizeUrl, input.handshake, input.flags) - - const url = new URL(authorizeUrl) - url.searchParams.set('response_type', 'code') - url.searchParams.set('client_id', clientId) - url.searchParams.set('redirect_uri', input.redirectUri) - url.searchParams.set('state', input.state) - url.searchParams.set('code_challenge', challenge) - url.searchParams.set('code_challenge_method', 'S256') - if (input.scopes.length > 0) { - url.searchParams.set('scope', input.scopes.join(scopeSeparator)) - } + const authorizeUrl = buildPkceAuthorizeUrl({ + authorizeUrl: resolve(options.authorizeUrl, input.handshake, input.flags), + clientId, + redirectUri: input.redirectUri, + state: input.state, + scopes: input.scopes, + scopeSeparator, + codeChallenge: challenge, + }) return { - authorizeUrl: url.toString(), + authorizeUrl, handshake: { ...input.handshake, codeVerifier: verifier, clientId }, } }, @@ -87,9 +91,10 @@ export function createPkceProvider( const verifier = input.handshake.codeVerifier const clientId = input.handshake.clientId if (typeof verifier !== 'string' || typeof clientId !== 'string') { - throw new CliError( + throw buildAuthError( 'AUTH_TOKEN_EXCHANGE_FAILED', 'Internal: PKCE handshake state lost between authorize and exchange.', + options.errorHints, ) } // `runOAuthFlow` folds the runtime `flags` into the handshake @@ -106,76 +111,19 @@ export function createPkceProvider( code_verifier: verifier, }) - let response: Response - try { - response = await fetchImpl(tokenUrl, { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - Accept: 'application/json', - }, - body: body.toString(), - }) - } catch (error) { - throw new CliError( - 'AUTH_TOKEN_EXCHANGE_FAILED', - `Token endpoint request failed: ${getErrorMessage(error)}`, - ) - } - - if (!response.ok) { - const detail = await safeReadText(response) - throw new CliError( - 'AUTH_TOKEN_EXCHANGE_FAILED', - `Token endpoint returned HTTP ${response.status}.`, - detail ? { hints: [detail] } : {}, - ) - } - - // Parse defensively — a misconfigured proxy can return a 2xx HTML - // error page that would otherwise blow up with a raw SyntaxError. - let payload: { access_token?: string; refresh_token?: string; expires_in?: number } - try { - payload = (await response.json()) as typeof payload - } catch (error) { - throw new CliError( - 'AUTH_TOKEN_EXCHANGE_FAILED', - `Token endpoint returned non-JSON response: ${getErrorMessage(error)}`, - ) - } - if (!payload.access_token) { - throw new CliError( - 'AUTH_TOKEN_EXCHANGE_FAILED', - 'Token endpoint response missing access_token.', - ) - } + const result = await postTokenEndpoint({ + url: tokenUrl, + body, + errorHints: options.errorHints, + fetchImpl, + }) return { - accessToken: payload.access_token, - refreshToken: payload.refresh_token, - expiresAt: - typeof payload.expires_in === 'number' - ? Date.now() + payload.expires_in * 1000 - : undefined, + accessToken: result.accessToken, + refreshToken: result.refreshToken, + expiresAt: result.expiresAt, } }, validateToken: options.validate, } } - -function resolve( - resolver: PkceLazyString, - handshake: Record, - flags: Record, -): string { - return typeof resolver === 'function' ? resolver({ handshake, flags }) : resolver -} - -async function safeReadText(response: Response): Promise { - try { - const text = (await response.text()).trim() - return text.length > 0 ? text : undefined - } catch { - return undefined - } -} From 445613ab1406f7b28204b762d2e5155e4b307c53 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 18:18:33 +0100 Subject: [PATCH 2/3] test(auth): trim redundant coverage in DCR + _oauth tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop tests that duplicate behaviour already exercised elsewhere or that test obvious helpers in isolation: - `_oauth.test.ts`: remove standalone describes for `resolve`, `safeReadText`, and `buildPkceAuthorizeUrl` — already covered by the provider integration tests. Consolidate the four token-endpoint error cases into a single parametrised case and merge the duplicate errorHints assertion into the non-2xx test. - `dcr.test.ts`: collapse the three DCR error tests (missing client_id + non-JSON kept as a single parametrised case; non-2xx absorbed the errorHints assertion) and drop the standalone "token endpoint non-2xx" test that overlaps with `_oauth.test.ts`. Net –231 LOC across the two test files. 374 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/providers/_oauth.test.ts | 193 +++++------------------------- src/auth/providers/dcr.test.ts | 142 ++++------------------ 2 files changed, 52 insertions(+), 283 deletions(-) diff --git a/src/auth/providers/_oauth.test.ts b/src/auth/providers/_oauth.test.ts index f434342..dd583ca 100644 --- a/src/auth/providers/_oauth.test.ts +++ b/src/auth/providers/_oauth.test.ts @@ -1,76 +1,11 @@ import { describe, expect, it } from 'vitest' -import { buildPkceAuthorizeUrl, postTokenEndpoint, resolve, safeReadText } from './_oauth.js' +import { postTokenEndpoint } from './_oauth.js' const TOKEN_URL = 'https://example.com/oauth/token' -describe('resolve', () => { - it('returns literal strings unchanged', () => { - expect(resolve('static', {}, {})).toBe('static') - }) - - it('passes handshake + flags to function resolvers', () => { - const got = resolve( - ({ handshake, flags }) => `${handshake.base as string}|${flags.clientId as string}`, - { base: 'https://x' }, - { clientId: 'cid' }, - ) - expect(got).toBe('https://x|cid') - }) -}) - -describe('safeReadText', () => { - it('returns trimmed body text', async () => { - const res = new Response(' body text ', { status: 400 }) - expect(await safeReadText(res)).toBe('body text') - }) - - it('returns undefined for empty bodies', async () => { - const res = new Response('', { status: 400 }) - expect(await safeReadText(res)).toBeUndefined() - }) -}) - -describe('buildPkceAuthorizeUrl', () => { - it('sets PKCE params and joins scopes with the supplied separator', () => { - const url = new URL( - buildPkceAuthorizeUrl({ - authorizeUrl: 'https://example.com/oauth/authorize', - clientId: 'cid', - redirectUri: 'http://localhost/cb', - state: 'st', - scopes: ['a', 'b'], - scopeSeparator: ',', - codeChallenge: 'challenge-1', - }), - ) - expect(url.searchParams.get('response_type')).toBe('code') - expect(url.searchParams.get('client_id')).toBe('cid') - expect(url.searchParams.get('redirect_uri')).toBe('http://localhost/cb') - expect(url.searchParams.get('state')).toBe('st') - expect(url.searchParams.get('code_challenge')).toBe('challenge-1') - expect(url.searchParams.get('code_challenge_method')).toBe('S256') - expect(url.searchParams.get('scope')).toBe('a,b') - }) - - it('omits scope when scopes is empty', () => { - const url = new URL( - buildPkceAuthorizeUrl({ - authorizeUrl: 'https://example.com/oauth/authorize', - clientId: 'cid', - redirectUri: 'http://localhost/cb', - state: 'st', - scopes: [], - scopeSeparator: ' ', - codeChallenge: 'c', - }), - ) - expect(url.searchParams.has('scope')).toBe(false) - }) -}) - describe('postTokenEndpoint', () => { - it('POSTs the form body, returns access_token + refresh_token + expiresAt', async () => { + it('POSTs the form body, returns access_token + refresh_token + expiresAt, no Authorization header without basicAuth', async () => { let captured: { url: string; init: RequestInit } | undefined const result = await postTokenEndpoint({ url: TOKEN_URL, @@ -84,16 +19,14 @@ describe('postTokenEndpoint', () => { refresh_token: 'rtok', expires_in: 60, }), - { status: 200, headers: { 'Content-Type': 'application/json' } }, + { status: 200 }, ), ) }) as typeof fetch, }) - expect(result.accessToken).toBe('tok') - expect(result.refreshToken).toBe('rtok') + expect(result).toMatchObject({ accessToken: 'tok', refreshToken: 'rtok' }) expect(result.expiresAt).toBeGreaterThan(Date.now()) expect(captured?.url).toBe(TOKEN_URL) - expect(captured?.init.method).toBe('POST') const headers = captured?.init.headers as Record expect(headers['Content-Type']).toBe('application/x-www-form-urlencoded') expect(headers.Authorization).toBeUndefined() @@ -117,41 +50,7 @@ describe('postTokenEndpoint', () => { ) }) - it('refresh_token grants reuse the same helper', async () => { - let body: string | undefined - const result = await postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams({ - grant_type: 'refresh_token', - refresh_token: 'old-rtok', - client_id: 'cid', - }), - fetchImpl: ((_url, init = {}) => { - body = init.body as string - return Promise.resolve( - new Response( - JSON.stringify({ access_token: 'new', refresh_token: 'new-rtok' }), - { status: 200 }, - ), - ) - }) as typeof fetch, - }) - expect(result.accessToken).toBe('new') - expect(result.refreshToken).toBe('new-rtok') - expect(new URLSearchParams(body).get('grant_type')).toBe('refresh_token') - }) - - it('network errors throw AUTH_TOKEN_EXCHANGE_FAILED', async () => { - await expect( - postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams(), - fetchImpl: (() => Promise.reject(new Error('econnrefused'))) as typeof fetch, - }), - ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) - }) - - it('non-2xx surfaces user errorHints first, then the body as a hint', async () => { + it('non-2xx wraps as AUTH_TOKEN_EXCHANGE_FAILED with user errorHints first and body text second', async () => { await expect( postTokenEndpoint({ url: TOKEN_URL, @@ -168,63 +67,29 @@ describe('postTokenEndpoint', () => { }) }) - it('2xx HTML / non-JSON body throws AUTH_TOKEN_EXCHANGE_FAILED', async () => { - await expect( - postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams(), - fetchImpl: (() => - Promise.resolve( - new Response('oops', { - status: 200, - headers: { 'Content-Type': 'text/html' }, - }), - )) as typeof fetch, - }), - ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) - }) - - it('errorHints are prepended to every CliError, with body text appended after on non-2xx', async () => { - // Non-2xx: both user hints AND body detail. - await expect( - postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams(), - errorHints: ['Re-run login', 'Set TWIST_API_TOKEN'], - fetchImpl: (() => - Promise.resolve( - new Response('invalid_grant', { status: 400 }), - )) as typeof fetch, - }), - ).rejects.toMatchObject({ - code: 'AUTH_TOKEN_EXCHANGE_FAILED', - hints: ['Re-run login', 'Set TWIST_API_TOKEN', 'invalid_grant'], - }) - - // Network failure: user hints only (no server detail to append). - await expect( - postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams(), - errorHints: ['Re-run login'], - fetchImpl: (() => Promise.reject(new Error('econnrefused'))) as typeof fetch, - }), - ).rejects.toMatchObject({ - code: 'AUTH_TOKEN_EXCHANGE_FAILED', - hints: ['Re-run login'], - }) - }) - - it('missing access_token throws AUTH_TOKEN_EXCHANGE_FAILED', async () => { - await expect( - postTokenEndpoint({ - url: TOKEN_URL, - body: new URLSearchParams(), - fetchImpl: (() => - Promise.resolve( - new Response(JSON.stringify({ refresh_token: 'r' }), { status: 200 }), - )) as typeof fetch, - }), - ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + it('network errors, non-JSON bodies, and responses missing access_token all become AUTH_TOKEN_EXCHANGE_FAILED', async () => { + const cases: Array<() => Promise> = [ + () => Promise.reject(new Error('econnrefused')), + () => + Promise.resolve( + new Response('oops', { + status: 200, + headers: { 'Content-Type': 'text/html' }, + }), + ), + () => + Promise.resolve( + new Response(JSON.stringify({ refresh_token: 'r' }), { status: 200 }), + ), + ] + for (const fetchImpl of cases) { + await expect( + postTokenEndpoint({ + url: TOKEN_URL, + body: new URLSearchParams(), + fetchImpl: fetchImpl as typeof fetch, + }), + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) + } }) }) diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts index bb06bc3..7f3ec6f 100644 --- a/src/auth/providers/dcr.test.ts +++ b/src/auth/providers/dcr.test.ts @@ -197,130 +197,34 @@ describe('createDcrProvider', () => { }) }) - it('DCR response missing client_id is AUTH_DCR_FAILED', async () => { - const provider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - fetchImpl: (() => Promise.resolve(respond({ client_secret: 'sec' }))) as typeof fetch, - }) - await expect( - provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), - ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) - }) - - it('DCR non-JSON response is AUTH_DCR_FAILED', async () => { - const provider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - fetchImpl: (() => + it('DCR response missing client_id or returning non-JSON is AUTH_DCR_FAILED', async () => { + const make = (fetchImpl: typeof fetch) => + createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI' }, + validate, + fetchImpl, + }) + const cases: Array<() => Promise> = [ + () => Promise.resolve(respond({ client_secret: 'sec' })), + () => Promise.resolve( new Response('oops', { status: 200, headers: { 'Content-Type': 'text/html' }, }), - )) as typeof fetch, - }) - await expect( - provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), - ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) - }) - - it('token endpoint non-2xx becomes AUTH_TOKEN_EXCHANGE_FAILED', async () => { - const { fetchImpl } = makeFetchRecorder((u) => - u === REGISTRATION_URL - ? respond({ client_id: 'cid', client_secret: 'sec' }) - : new Response('invalid_grant', { status: 400 }), - ) - const provider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - fetchImpl, - }) - const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) - await expect( - provider.exchangeCode({ - code: 'c', - state: 's', - redirectUri: REDIRECT_URI, - handshake: { ...prepared.handshake, codeVerifier: 'v' }, - }), - ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) - }) - - it('errorHints are prepended to every error (DCR failure + token-exchange failure), with body text appended on non-2xx', async () => { - const userHints = ['Re-run: cli auth login', 'Or set CLI_API_TOKEN'] - - // DCR non-2xx - const dcrProvider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - errorHints: userHints, - fetchImpl: (() => - Promise.resolve(new Response('rate_limited', { status: 429 }))) as typeof fetch, - }) - await expect( - dcrProvider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), - ).rejects.toMatchObject({ - code: 'AUTH_DCR_FAILED', - hints: [...userHints, 'rate_limited'], - }) - - // DCR missing client_id (no server body to append) - const noClientIdProvider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - errorHints: userHints, - fetchImpl: (() => Promise.resolve(respond({}))) as typeof fetch, - }) - await expect( - noClientIdProvider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }), - ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED', hints: userHints }) - - // Token endpoint failure flows through postTokenEndpoint - const exchangeProvider = createDcrProvider({ - registrationUrl: REGISTRATION_URL, - authorizeUrl: AUTHORIZE_URL, - tokenUrl: TOKEN_URL, - clientMetadata: { clientName: 'CLI' }, - validate, - errorHints: userHints, - fetchImpl: ((url: RequestInfo | URL) => - String(url) === REGISTRATION_URL - ? Promise.resolve(respond({ client_id: 'cid', client_secret: 'sec' })) - : Promise.resolve( - new Response('invalid_grant', { status: 400 }), - )) as typeof fetch, - }) - const prepared = await exchangeProvider.prepare!({ - redirectUri: REDIRECT_URI, - flags: {}, - }) - await expect( - exchangeProvider.exchangeCode({ - code: 'c', - state: 's', - redirectUri: REDIRECT_URI, - handshake: { ...prepared.handshake, codeVerifier: 'v' }, - }), - ).rejects.toMatchObject({ - code: 'AUTH_TOKEN_EXCHANGE_FAILED', - hints: [...userHints, 'invalid_grant'], - }) + ), + ] + for (const fetchImpl of cases) { + await expect( + make(fetchImpl as typeof fetch).prepare!({ + redirectUri: REDIRECT_URI, + flags: {}, + }), + ).rejects.toMatchObject({ code: 'AUTH_DCR_FAILED' }) + } }) it('clientMetadata.extra fields appear in the registration POST body verbatim; named fields win on collisions', async () => { From c1d9c85c105ab5b8f9756b5386a9267ad7036b02 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 18:24:48 +0100 Subject: [PATCH 3/3] =?UTF-8?q?refactor(auth):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20honour=20server=20auth=20method,=20URL-encode=20Bas?= =?UTF-8?q?ic,=20dedupe=20fetch+parse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - DCR now persists the registration response's `token_endpoint_auth_method` on the handshake and lets it override the configured one at `exchangeCode()`, per RFC 7591 §3.2.1. Validate the value against the known set so an unknown method silently falls back to the configured one. - URL-encode `client_id` and `client_secret` before joining with a colon for HTTP Basic auth per RFC 6749 §2.3.1. A literal colon (or any reserved character) in either value would otherwise corrupt the credential. - Extract `postAndParseJson` in `_oauth.ts`. Both `postTokenEndpoint` and DCR `prepare()` now share the network / non-2xx / parse error wrapping while still validating their own success shape. - Add `OAuthLazyString` as a grant-agnostic alias of `PkceLazyString` (non-breaking; existing name still exported). - Export `DcrTokenEndpointAuthMethod` from the auth subpath so callers can refer to the union directly. - Tests: cover the "no `client_secret` but `client_secret_post` requested → falls back to public-client POST" branch, the "server-issued `token_endpoint_auth_method` wins" branch, and the PKCE-level `errorHints` plumbing (both the token-endpoint path and the handshake-lost guard). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/index.ts | 8 ++- src/auth/providers/_oauth.ts | 124 +++++++++++++++++++++++--------- src/auth/providers/dcr.test.ts | 75 +++++++++++++++++++ src/auth/providers/dcr.ts | 117 ++++++++++++++++++------------ src/auth/providers/pkce.test.ts | 35 +++++++++ src/auth/providers/pkce.ts | 9 ++- 6 files changed, 284 insertions(+), 84 deletions(-) diff --git a/src/auth/index.ts b/src/auth/index.ts index 8ca936f..0589c43 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -21,9 +21,13 @@ export { } from './pkce.js' export type { GenerateVerifierOptions } from './pkce.js' export { createPkceProvider } from './providers/pkce.js' -export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' +export type { OAuthLazyString, PkceLazyString, PkceProviderOptions } from './providers/pkce.js' export { createDcrProvider } from './providers/dcr.js' -export type { DcrClientMetadata, DcrProviderOptions } from './providers/dcr.js' +export type { + DcrClientMetadata, + DcrProviderOptions, + DcrTokenEndpointAuthMethod, +} from './providers/dcr.js' export type { AccountRef, AuthAccount, diff --git a/src/auth/providers/_oauth.ts b/src/auth/providers/_oauth.ts index 6e12695..06c6bdc 100644 --- a/src/auth/providers/_oauth.ts +++ b/src/auth/providers/_oauth.ts @@ -64,6 +64,75 @@ export function buildPkceAuthorizeUrl(input: BuildPkceAuthorizeUrlInput): string return url.toString() } +/** + * Per RFC 6749 §2.3.1, the `client_id` and `client_secret` MUST be + * `application/x-www-form-urlencoded`-encoded before being concatenated with + * a colon for HTTP Basic Authentication. A literal colon (or any reserved + * character) in either value would otherwise corrupt the credential. + */ +export function encodeBasicAuth(clientId: string, clientSecret: string): string { + return Buffer.from( + `${encodeURIComponent(clientId)}:${encodeURIComponent(clientSecret)}`, + 'utf8', + ).toString('base64') +} + +export type PostAndParseJsonInput = { + url: string + headers: Record + /** Pre-encoded request body. */ + body: string + /** Error code wrapped around every failure mode. */ + errorCode: AuthErrorCode + /** Prefix for error messages, e.g. `'Token endpoint'` or `'Registration endpoint'`. */ + errorLabel: string + errorHints?: string[] + fetchImpl: typeof fetch +} + +/** + * POST a request, parse a JSON response, and wrap every failure mode as a + * typed `CliError`. Common backbone for the OAuth token endpoint and the + * RFC 7591 dynamic-client-registration endpoint — both POST a body, both + * expect a JSON reply, both want uniform error handling. + * + * Throws `errorCode` with the configured hints on: + * - network failure (fetch rejection) + * - non-2xx response (body text appended as a hint after `errorHints`) + * - non-JSON 2xx body (a misconfigured proxy returning HTML, etc.) + * + * Success-shape validation (e.g. `access_token` present) is the caller's + * job, because it differs per endpoint. + */ +export async function postAndParseJson(input: PostAndParseJsonInput): Promise { + const fail = (message: string, extra?: string): CliError => + buildAuthError(input.errorCode, message, input.errorHints, extra) + + let response: Response + try { + response = await input.fetchImpl(input.url, { + method: 'POST', + headers: input.headers, + body: input.body, + }) + } catch (error) { + throw fail(`${input.errorLabel} request failed: ${getErrorMessage(error)}`) + } + + if (!response.ok) { + const detail = await safeReadText(response) + throw fail(`${input.errorLabel} returned HTTP ${response.status}.`, detail) + } + + // Parse defensively — a misconfigured proxy can return a 2xx HTML error + // page that would otherwise blow up with a raw SyntaxError. + try { + return (await response.json()) as T + } catch (error) { + throw fail(`${input.errorLabel} returned non-JSON response: ${getErrorMessage(error)}`) + } +} + export type PostTokenEndpointInput = { url: string /** Form-encoded body. Caller owns grant_type + grant-specific params. */ @@ -94,8 +163,7 @@ export type PostTokenEndpointResult = { * * Failures uniformly throw `CliError('AUTH_TOKEN_EXCHANGE_FAILED', …)`: * network errors, non-2xx responses (with body text as a hint), non-JSON - * bodies (the misconfigured-proxy HTML case), and responses missing - * `access_token`. + * bodies, and responses missing `access_token`. */ export async function postTokenEndpoint( input: PostTokenEndpointInput, @@ -105,42 +173,28 @@ export async function postTokenEndpoint( Accept: 'application/json', } if (input.basicAuth) { - const encoded = Buffer.from( - `${input.basicAuth.clientId}:${input.basicAuth.clientSecret}`, - 'utf8', - ).toString('base64') - headers.Authorization = `Basic ${encoded}` - } - - const fail = (message: string, extra?: string): CliError => - buildAuthError('AUTH_TOKEN_EXCHANGE_FAILED', message, input.errorHints, extra) - - let response: Response - try { - response = await input.fetchImpl(input.url, { - method: 'POST', - headers, - body: input.body.toString(), - }) - } catch (error) { - throw fail(`Token endpoint request failed: ${getErrorMessage(error)}`) + headers.Authorization = `Basic ${encodeBasicAuth(input.basicAuth.clientId, input.basicAuth.clientSecret)}` } - if (!response.ok) { - const detail = await safeReadText(response) - throw fail(`Token endpoint returned HTTP ${response.status}.`, detail) - } - - // Parse defensively — a misconfigured proxy can return a 2xx HTML error - // page that would otherwise blow up with a raw SyntaxError. - let payload: { access_token?: string; refresh_token?: string; expires_in?: number } - try { - payload = (await response.json()) as typeof payload - } catch (error) { - throw fail(`Token endpoint returned non-JSON response: ${getErrorMessage(error)}`) - } + const payload = await postAndParseJson<{ + access_token?: string + refresh_token?: string + expires_in?: number + }>({ + url: input.url, + headers, + body: input.body.toString(), + errorCode: 'AUTH_TOKEN_EXCHANGE_FAILED', + errorLabel: 'Token endpoint', + errorHints: input.errorHints, + fetchImpl: input.fetchImpl, + }) if (!payload.access_token) { - throw fail('Token endpoint response missing access_token.') + throw buildAuthError( + 'AUTH_TOKEN_EXCHANGE_FAILED', + 'Token endpoint response missing access_token.', + input.errorHints, + ) } return { accessToken: payload.access_token, diff --git a/src/auth/providers/dcr.test.ts b/src/auth/providers/dcr.test.ts index 7f3ec6f..4ef6093 100644 --- a/src/auth/providers/dcr.test.ts +++ b/src/auth/providers/dcr.test.ts @@ -143,6 +143,81 @@ describe('createDcrProvider', () => { expect(tokenBody.get('client_secret')).toBe('sec') }) + it('falls back to public-client POST when registration omits client_secret even though client_secret_post was requested', async () => { + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ client_id: 'pub-cid' }) // server returned no client_secret + : respond({ access_token: 'tok' }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + // Configured method asked for client_secret_post — but the registration + // came back without a secret, so the token request must still drop to + // public-client POST instead of sending a half-baked credential. + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_post' }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(prepared.handshake.clientSecret).toBeUndefined() + + await provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }) + + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenHeaders = tokenCall.init.headers as Record + const tokenBody = new URLSearchParams(tokenCall.init.body as string) + expect(tokenHeaders.Authorization).toBeUndefined() + expect(tokenBody.get('client_id')).toBe('pub-cid') + expect(tokenBody.has('client_secret')).toBe(false) + }) + + it("honours the server's token_endpoint_auth_method from the registration response over the configured one (RFC 7591 §3.2.1)", async () => { + // Configured: client_secret_basic. Server downgrades to client_secret_post. + // Effective method on the token request must follow the server. + const { calls, fetchImpl } = makeFetchRecorder((u) => + u === REGISTRATION_URL + ? respond({ + client_id: 'cid', + client_secret: 'sec', + token_endpoint_auth_method: 'client_secret_post', + }) + : respond({ access_token: 'tok' }), + ) + const provider = createDcrProvider({ + registrationUrl: REGISTRATION_URL, + authorizeUrl: AUTHORIZE_URL, + tokenUrl: TOKEN_URL, + clientMetadata: { clientName: 'CLI', tokenEndpointAuthMethod: 'client_secret_basic' }, + validate, + fetchImpl, + }) + + const prepared = await provider.prepare!({ redirectUri: REDIRECT_URI, flags: {} }) + expect(prepared.handshake.tokenEndpointAuthMethod).toBe('client_secret_post') + + await provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: REDIRECT_URI, + handshake: { ...prepared.handshake, codeVerifier: 'v' }, + }) + + const tokenCall = calls.find((c) => c.url === TOKEN_URL)! + const tokenHeaders = tokenCall.init.headers as Record + const tokenBody = new URLSearchParams(tokenCall.init.body as string) + expect(tokenHeaders.Authorization).toBeUndefined() + expect(tokenBody.get('client_id')).toBe('cid') + expect(tokenBody.get('client_secret')).toBe('sec') + }) + it('tokenEndpointAuthMethod=none (or missing client_secret) sends client_id in the body and no Authorization header', async () => { const { calls, fetchImpl } = makeFetchRecorder((u) => u === REGISTRATION_URL diff --git a/src/auth/providers/dcr.ts b/src/auth/providers/dcr.ts index 9ad17c9..3194276 100644 --- a/src/auth/providers/dcr.ts +++ b/src/auth/providers/dcr.ts @@ -1,4 +1,3 @@ -import { getErrorMessage } from '../../errors.js' import { deriveChallenge, generateVerifier } from '../pkce.js' import type { AuthAccount, @@ -14,12 +13,14 @@ import type { import { buildAuthError, buildPkceAuthorizeUrl, + postAndParseJson, postTokenEndpoint, resolve, - safeReadText, } from './_oauth.js' import type { PkceLazyString } from './pkce.js' +export type DcrTokenEndpointAuthMethod = 'client_secret_basic' | 'client_secret_post' | 'none' + /** * RFC 7591 Dynamic Client Registration metadata POSTed to the registration * endpoint. Only fields the CLI typically cares about are named; pass anything @@ -30,8 +31,13 @@ export type DcrClientMetadata = { clientUri?: string logoUri?: string applicationType?: 'native' | 'web' - /** How the token endpoint will be authenticated. Defaults to `'client_secret_basic'`. */ - tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post' | 'none' + /** + * Requested token-endpoint auth method. Defaults to `'client_secret_basic'`. + * The registration response is authoritative per RFC 7591 §3.2.1 — when + * the server returns its own `token_endpoint_auth_method`, that value + * wins over this configured one. + */ + tokenEndpointAuthMethod?: DcrTokenEndpointAuthMethod /** Defaults to `['authorization_code']`. */ grantTypes?: string[] /** Defaults to `['code']`. */ @@ -67,16 +73,25 @@ export type DcrProviderOptions = { fetchImpl?: typeof fetch } +const VALID_AUTH_METHODS: ReadonlySet = new Set([ + 'client_secret_basic', + 'client_secret_post', + 'none', +]) + /** * Build an `AuthProvider` for the RFC 7591 Dynamic Client Registration flow. * * - `prepare`: POST `clientMetadata` to `registrationUrl`. Stash the issued - * `client_id` (and `client_secret` if returned) in the handshake. + * `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. - * - `exchangeCode`: token endpoint POST, authenticated per the metadata's - * `tokenEndpointAuthMethod` — Basic auth header for `client_secret_basic`, - * secret in the body for `client_secret_post`, neither for `none` (or when - * the registration response carried no `client_secret`). + * - `exchangeCode`: token endpoint POST, authenticated per the handshake's + * server-returned auth method (falling back to the configured one) — + * Basic auth header for `client_secret_basic`, secret in the body for + * `client_secret_post`, neither for `none` (or when the registration + * response carried no `client_secret`). * - `validateToken`: caller-supplied. */ export function createDcrProvider( @@ -84,7 +99,7 @@ export function createDcrProvider( ): AuthProvider { const fetchImpl = options.fetchImpl ?? fetch const scopeSeparator = options.scopeSeparator ?? ' ' - const tokenEndpointAuthMethod = + const configuredAuthMethod: DcrTokenEndpointAuthMethod = options.clientMetadata.tokenEndpointAuthMethod ?? 'client_secret_basic' return { @@ -93,45 +108,47 @@ export function createDcrProvider( const registrationBody = buildRegistrationBody( options.clientMetadata, input.redirectUri, - tokenEndpointAuthMethod, + configuredAuthMethod, ) - const fail = (message: string, extra?: string) => - buildAuthError('AUTH_DCR_FAILED', message, options.errorHints, extra) - - let response: Response - try { - response = await fetchImpl(registrationUrl, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Accept: 'application/json', - }, - body: JSON.stringify(registrationBody), - }) - } catch (error) { - throw fail(`Registration endpoint request failed: ${getErrorMessage(error)}`) - } - - if (!response.ok) { - const detail = await safeReadText(response) - throw fail(`Registration endpoint returned HTTP ${response.status}.`, detail) - } + const payload = await postAndParseJson<{ + client_id?: string + client_secret?: string + token_endpoint_auth_method?: string + }>({ + url: registrationUrl, + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify(registrationBody), + errorCode: 'AUTH_DCR_FAILED', + errorLabel: 'Registration endpoint', + errorHints: options.errorHints, + fetchImpl, + }) - let payload: { client_id?: string; client_secret?: string } - try { - payload = (await response.json()) as typeof payload - } catch (error) { - throw fail( - `Registration endpoint returned non-JSON response: ${getErrorMessage(error)}`, - ) - } if (!payload.client_id) { - throw fail('Registration response missing client_id.') + throw buildAuthError( + 'AUTH_DCR_FAILED', + 'Registration response missing client_id.', + options.errorHints, + ) } const handshake: Record = { clientId: payload.client_id } if (payload.client_secret) handshake.clientSecret = payload.client_secret + // Per RFC 7591 §3.2.1 the server may downgrade or override the + // requested method. Only persist values we know how to act on; + // an unknown method falls back to the configured one at exchange. + if ( + typeof payload.token_endpoint_auth_method === 'string' && + VALID_AUTH_METHODS.has( + payload.token_endpoint_auth_method as DcrTokenEndpointAuthMethod, + ) + ) { + handshake.tokenEndpointAuthMethod = payload.token_endpoint_auth_method + } return { handshake } }, @@ -178,6 +195,16 @@ export function createDcrProvider( } 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 flags = (input.handshake.flags as Record | undefined) ?? {} const tokenUrl = resolve(options.tokenUrl, input.handshake, flags) @@ -191,11 +218,11 @@ export function createDcrProvider( // Public-client fallback: a registration with no `client_secret` // can't authenticate Basic/Post regardless of the requested method, // so we POST `client_id` in the body like a non-confidential - // client. Otherwise honour the configured auth method. + // client. Otherwise honour the effective auth method. let basicAuth: { clientId: string; clientSecret: string } | undefined - if (!clientSecret || tokenEndpointAuthMethod === 'none') { + if (!clientSecret || effectiveAuthMethod === 'none') { body.set('client_id', clientId) - } else if (tokenEndpointAuthMethod === 'client_secret_post') { + } else if (effectiveAuthMethod === 'client_secret_post') { body.set('client_id', clientId) body.set('client_secret', clientSecret) } else { @@ -223,7 +250,7 @@ export function createDcrProvider( function buildRegistrationBody( metadata: DcrClientMetadata, redirectUri: string, - tokenEndpointAuthMethod: NonNullable, + tokenEndpointAuthMethod: DcrTokenEndpointAuthMethod, ): Record { const body: Record = { ...metadata.extra, diff --git a/src/auth/providers/pkce.test.ts b/src/auth/providers/pkce.test.ts index 6bfebbf..dc57bd6 100644 --- a/src/auth/providers/pkce.test.ts +++ b/src/auth/providers/pkce.test.ts @@ -128,6 +128,41 @@ describe('createPkceProvider', () => { ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) }) + it('forwards errorHints onto both the token-endpoint failure and the handshake-lost guard', async () => { + const provider = createPkceProvider({ + authorizeUrl: 'unused', + tokenUrl: 'https://example.com/oauth/token', + clientId: 'cid', + validate, + errorHints: ['Re-run login'], + fetchImpl: (() => + Promise.resolve(new Response('invalid_grant', { status: 400 }))) as typeof fetch, + }) + await expect( + provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: 'http://localhost/callback', + handshake: { codeVerifier: 'v', clientId: 'cid' }, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: ['Re-run login', 'invalid_grant'], + }) + // Same hints flow through the internal handshake-lost guard. + await expect( + provider.exchangeCode({ + code: 'c', + state: 's', + redirectUri: 'http://localhost/callback', + handshake: {}, + }), + ).rejects.toMatchObject({ + code: 'AUTH_TOKEN_EXCHANGE_FAILED', + hints: ['Re-run login'], + }) + }) + it('throws AUTH_TOKEN_EXCHANGE_FAILED when the handshake state was lost between authorize and exchange', async () => { const provider = createPkceProvider({ authorizeUrl: 'unused', diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index 393a330..bdd50b9 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -12,13 +12,18 @@ import { buildAuthError, buildPkceAuthorizeUrl, postTokenEndpoint, resolve } fro /** * Lazy resolver: a literal string, or a function that builds one from the - * current PKCE handshake (so callers can derive the URL or client_id from - * the active session's `baseUrl` / per-flow flags). + * current OAuth handshake (so callers can derive the URL or client_id from + * the active session's `baseUrl` / per-flow flags). Used by both + * `createPkceProvider` and `createDcrProvider`; prefer the grant-agnostic + * alias `OAuthLazyString` for new code. */ export type PkceLazyString = | string | ((ctx: { handshake: Record; flags: Record }) => string) +/** Grant-agnostic alias for {@link PkceLazyString}. Identical type. */ +export type OAuthLazyString = PkceLazyString + export type PkceProviderOptions = { /** OAuth 2.0 authorize endpoint. Function form supports per-flow base URLs (Outline self-hosted). */ authorizeUrl: PkceLazyString