From 5084380441eb9374454580333b988e40f711a6df Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Wed, 29 Jul 2026 20:15:21 +0100 Subject: [PATCH 1/6] refactor(auth): resolve Cloudflare credentials through provider --- AGENTS.md | 15 +- package-lock.json | 6 +- package.json | 2 +- src/auth/api-token-mode.ts | 216 +++++++-------- src/auth/oauth-handler.ts | 123 ++++++--- src/index.ts | 15 +- tests/auth/api-token-mode.test.ts | 421 ++++++++++++++++++++---------- tests/auth/oauth-routes.test.ts | 13 +- tests/helpers/cloudflare-api.ts | 4 +- tests/mcp-modern.test.ts | 1 + tests/worker.test.ts | 20 +- 11 files changed, 519 insertions(+), 317 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ce3c245..95acabd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,8 +10,8 @@ When modifying MCP or OAuth functionality, **always check the latest published MCP specification**: -- **Specification:** https://modelcontextprotocol.io/specification/2025-11-25 -- **Authorization section:** https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization +- **Specification:** https://modelcontextprotocol.io/specification/2026-07-28 +- **Authorization section:** https://modelcontextprotocol.io/specification/2026-07-28/basic/authorization ## Repository structure @@ -27,7 +27,8 @@ cloudflare-mcp/ │ ├── metrics.ts # Analytics Engine metrics (auth_user/tool_call) │ ├── auth/ │ │ ├── types.ts # Auth props schemas (Zod discriminated union) -│ │ ├── api-token-mode.ts # Direct Cloudflare API token support +│ │ ├── api-token-mode.ts # Prefix classification & external resolver +│ │ ├── cloudflare-identity.ts # Owner-aware Cloudflare API identity probes │ │ ├── cloudflare-auth.ts # PKCE & OAuth utilities │ │ ├── oauth-handler.ts # OAuth authorization flow │ │ ├── derived-oauth-scopes.ts # Canonical production OAuth scope API metadata @@ -120,10 +121,12 @@ Code execution uses Cloudflare's Worker Loader API to dynamically create isolate ### Authentication -Two modes via Zod discriminated union (`AuthProps`): +Two credential paths produce the same Zod-validated `AuthProps` union: -- **OAuth mode** (default): Uses `@cloudflare/workers-oauth-provider` with PKCE. Supports both account-scoped and user-scoped tokens. -- **API token mode**: Direct Cloudflare API tokens bypass OAuth. Detection: OAuth tokens have 3 colon-separated parts; API tokens don't. +- **OAuth mode** (default): `@cloudflare/workers-oauth-provider` validates its own access tokens and restores encrypted Cloudflare OAuth props. The upstream authorization-code and refresh flows use PKCE and remain separate from direct credential resolution. +- **Direct Cloudflare credential mode**: after the provider's internal lookup misses, `resolveExternalToken` validates the bearer against the Cloudflare API and returns request-local props. Prefixes are owner hints: `cfat_` account tokens query only `/accounts`; `cfut_` user API tokens and `cfoat_` Cloudflare OAuth credentials query `/user` and `/accounts`; unprefixed legacy tokens retain response-based inference. Expected failures use the provider's `ExternalTokenError`, which generates RFC 6750/9728 `401`/`403` challenges and preserves retry guidance. + +Validated direct-credential identity is cached by token hash in `OAUTH_KV`; provider-issued MCP tokens never invoke the external resolver. The consent picker uses the production catalog returned by `GET /oauth/scopes` in every deployment. Staging may register additional scopes, but the MCP picker exposes them only after they reach production. Only the user, account, and offline-access OAuth bootstrap scopes sit outside the API catalog. Terraform registration must land before deploying picker additions. The app does not impose a scope-count cap. diff --git a/package-lock.json b/package-lock.json index 723ea21..202f1ad 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,7 +8,7 @@ "name": "cloudflare-mcp", "version": "0.1.0", "dependencies": { - "@cloudflare/workers-oauth-provider": "^0.8.3", + "@cloudflare/workers-oauth-provider": "https://pkg.pr.new/cloudflare/workers-oauth-provider/@cloudflare/workers-oauth-provider@272", "@modelcontextprotocol/server": "2.0.0", "hono": "^4.12.25", "zod": "^4.3.5" @@ -168,8 +168,8 @@ }, "node_modules/@cloudflare/workers-oauth-provider": { "version": "0.8.3", - "resolved": "https://registry.npmjs.org/@cloudflare/workers-oauth-provider/-/workers-oauth-provider-0.8.3.tgz", - "integrity": "sha512-5dIeQlJTDKAQvy3GcrtWBhPvTb9lHeRp/m8MoostTkcrbrGImH8zKSdJhogQ5YSTFUCg+b7FBg38b7KCM/x3VQ==", + "resolved": "https://pkg.pr.new/cloudflare/workers-oauth-provider/@cloudflare/workers-oauth-provider@272", + "integrity": "sha512-WCPwKtejzaj1VvidZ5K9VKnFicVEGABPi9hcjW4slBt0w9Ukqi80MJURwyNKAEljQhjpnPOAtXuC2ucuO4eMow==", "license": "MIT" }, "node_modules/@cspotcode/source-map-support": { diff --git a/package.json b/package.json index 9832127..1e3607c 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ "seed:prod": "tsx scripts/seed-r2.ts production" }, "dependencies": { - "@cloudflare/workers-oauth-provider": "^0.8.3", + "@cloudflare/workers-oauth-provider": "https://pkg.pr.new/cloudflare/workers-oauth-provider/@cloudflare/workers-oauth-provider@272", "@modelcontextprotocol/server": "2.0.0", "hono": "^4.12.25", "zod": "^4.3.5" diff --git a/src/auth/api-token-mode.ts b/src/auth/api-token-mode.ts index f4a069e..e4a6989 100644 --- a/src/auth/api-token-mode.ts +++ b/src/auth/api-token-mode.ts @@ -1,17 +1,38 @@ -import { env as cloudflareEnv } from 'cloudflare:workers' - -import { getUserAndAccounts } from './oauth-handler' +import { + ExternalTokenError, + type OAuthTokenErrorCode, + type ResolveExternalTokenInput, + type ResolveExternalTokenResult +} from '@cloudflare/workers-oauth-provider' +import { z } from 'zod' + +import { getUserAndAccounts, type CloudflareTokenOwner } from './oauth-handler' import { OAuthError } from './workers-oauth-utils' -import { AUTH_PROPS_VERSION, type AccountSchema, type AuthProps, type UserSchema } from './types' +import { + AccountsSchema, + AUTH_PROPS_VERSION, + UserSchema, + type AccountSchema, + type AuthProps, + type UserSchema as UserIdentity +} from './types' -const env = cloudflareEnv as Env const API_TOKEN_IDENTITY_CACHE_TTL_SECONDS = 2_592_000 -type ApiTokenIdentity = { - user: UserSchema | null - accounts: AccountSchema[] - accountCount?: number +const ApiTokenIdentitySchema = z.object({ + user: UserSchema.nullable(), + accounts: AccountsSchema, + accountCount: z.number().int().nonnegative().optional() +}) + +type ApiTokenIdentity = z.infer + +/** Prefixes are trusted only as owner hints; unprefixed legacy credentials remain supported. */ +export function cloudflareTokenOwner(token: string): CloudflareTokenOwner { + if (token.startsWith('cfat_')) return 'account' + if (token.startsWith('cfut_') || token.startsWith('cfoat_')) return 'user' + return 'unknown' } async function hashApiToken(token: string): Promise { @@ -19,24 +40,29 @@ async function hashApiToken(token: string): Promise { return [...new Uint8Array(digest)].map((byte) => byte.toString(16).padStart(2, '0')).join('') } -async function getCachedApiTokenIdentity(token: string): Promise { +async function getCachedApiTokenIdentity( + token: string, + tokenOwner: CloudflareTokenOwner, + kv: KVNamespace +): Promise { const tokenHash = await hashApiToken(token) - // v2 namespace: abandons pre-versioning entries that may hold a truncated - // first page of accounts rather than the full (or count-only) list. - const cacheKey = `api-token-identity:v2:${tokenHash}` + const cacheKey = `api-token-identity:v3:${tokenHash}` + try { - const cached = await env.OAUTH_KV.get(cacheKey, 'json') - if (cached) { - return cached + const cachedValue = await kv.get(cacheKey, 'json') + if (cachedValue !== null) { + const cached = ApiTokenIdentitySchema.safeParse(cachedValue) + if (cached.success) return cached.data + console.warn('api_token_identity_probe ignored invalid cache entry') } } catch (error) { console.warn('api_token_identity_probe kv-cache read failed', error) } - const identity = await getUserAndAccounts(token, 'api_token_identity_probe') + const identity = await getUserAndAccounts(token, 'api_token_identity_probe', tokenOwner) try { - await env.OAUTH_KV.put(cacheKey, JSON.stringify(identity), { + await kv.put(cacheKey, JSON.stringify(identity), { expirationTtl: API_TOKEN_IDENTITY_CACHE_TTL_SECONDS }) } catch (error) { @@ -46,97 +72,39 @@ async function getCachedApiTokenIdentity(token: string): Promise Promise -): Promise { - if (!isDirectApiToken(request)) { - return null - } - - const token = extractBearerToken(request) - if (!token) { - return new Response(JSON.stringify({ error: 'Authorization header required' }), { - status: 401, - headers: { 'Content-Type': 'application/json' } - }) - } - - try { - const { user, accounts, accountCount } = await getCachedApiTokenIdentity(token) - - // Account-scoped token - if (!user) { - if (accounts.length === 0) { - return new Response(JSON.stringify({ error: 'Invalid token' }), { - status: 401, - headers: { 'Content-Type': 'application/json' } - }) - } - if (accounts.length > 1) { - return new Response( - JSON.stringify({ - error: 'Token has access to multiple accounts - use account_id parameter' - }), - { status: 400, headers: { 'Content-Type': 'application/json' } } - ) +function externalTokenError( + error: OAuthError, + tokenOwner: CloudflareTokenOwner +): { + code: OAuthTokenErrorCode + statusCode: number + requiredScopes?: string[] +} { + switch (error.code) { + case 'invalid_token': + return { code: 'invalid_token', statusCode: 401 } + case 'insufficient_scope': + return { + code: 'insufficient_scope', + statusCode: 403, + requiredScopes: tokenOwner === 'account' ? ['account:read'] : ['user:read', 'account:read'] } - const props = buildAuthProps(token, null, accounts) - return createMcpResponse(props) - } - - // User token - const props = buildAuthProps(token, user, accounts, accountCount) - return createMcpResponse(props) - } catch (err) { - if (err instanceof OAuthError) { - return err.toResponse() - } - return new Response(JSON.stringify({ error: 'Token verification failed' }), { - status: 401, - headers: { 'Content-Type': 'application/json' } - }) + case 'temporarily_unavailable': + return { code: 'temporarily_unavailable', statusCode: error.statusCode } + case 'server_error': + return { code: 'server_error', statusCode: error.statusCode } + default: + return error.statusCode >= 500 + ? { code: 'server_error', statusCode: 502 } + : { code: 'invalid_token', statusCode: 401 } } } -/** - * Build AuthProps from verified token info - */ +/** Build the request props consumed by the MCP tool layer from a verified Cloudflare credential. */ export function buildAuthProps( token: string, - user?: { id: string; email: string } | null, - accounts?: Array<{ id: string; name: string }>, + user: UserIdentity | null, + accounts: AccountSchema[], accountCount?: number ): AuthProps { if (user) { @@ -144,14 +112,18 @@ export function buildAuthProps( type: 'user_token', accessToken: token, user, - accounts: accounts || [], + accounts, accountCount, version: AUTH_PROPS_VERSION } } - if (!accounts || accounts.length === 0) { - throw new Error('Cannot build auth props: no user or account information') + if (accounts.length !== 1) { + throw new OAuthError( + 'invalid_token', + 'Account token must resolve to exactly one Cloudflare account', + 401 + ) } return { @@ -160,3 +132,35 @@ export function buildAuthProps( account: accounts[0] } } + +/** + * Resolve direct Cloudflare API and OAuth credentials after the provider's own + * access-token lookup misses. workers-oauth-provider owns bearer parsing, + * standards-compliant error responses, and injection of the returned props. + */ +export async function resolveCloudflareToken({ + token, + env +}: ResolveExternalTokenInput): Promise { + const tokenOwner = cloudflareTokenOwner(token) + try { + const { user, accounts, accountCount } = await getCachedApiTokenIdentity( + token, + tokenOwner, + env.OAUTH_KV + ) + + return { props: buildAuthProps(token, user, accounts, accountCount) } + } catch (error) { + if (error instanceof OAuthError) { + const { code, statusCode, requiredScopes } = externalTokenError(error, tokenOwner) + throw new ExternalTokenError(code, { + description: error.description, + statusCode, + headers: error.headers, + requiredScopes + }) + } + throw error + } +} diff --git a/src/auth/oauth-handler.ts b/src/auth/oauth-handler.ts index 8418676..7f43bcc 100644 --- a/src/auth/oauth-handler.ts +++ b/src/auth/oauth-handler.ts @@ -316,17 +316,20 @@ function getRetryAfterHeader(...responses: Response[]): Record { } } -function throwCombinedCloudflareApiError(userResp: Response, accountsResp: Response): never { - const statuses = [userResp.status, accountsResp.status] +function throwCloudflareApiError(...responses: [Response, ...Response[]]): never { + const statuses = responses.map((response) => response.status) if (statuses.some((status) => status >= 500)) { throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) } if (statuses.includes(429)) { - throw new OAuthError('temporarily_unavailable', 'Rate limited, try again later', 429, { - ...getRetryAfterHeader(userResp, accountsResp) - }) + throw new OAuthError( + 'temporarily_unavailable', + 'Rate limited, try again later', + 429, + getRetryAfterHeader(...responses) + ) } if (statuses.includes(401)) { @@ -334,27 +337,47 @@ function throwCombinedCloudflareApiError(userResp: Response, accountsResp: Respo } if (statuses.includes(403)) { - throw new OAuthError('insufficient_scope', 'Insufficient permissions', 403) + throw new OAuthError( + 'insufficient_scope', + 'Token lacks required user:read or account:read scope', + 403 + ) + } + + if (statuses.includes(400)) { + throw new OAuthError( + 'invalid_token', + 'Access token appears malformed; reauthenticate and try again', + 401 + ) } - throw new OAuthError('invalid_token', 'Failed to verify token', userResp.status) + throw new OAuthError('invalid_token', 'Access token is invalid or expired', statuses[0]) } +export type CloudflareTokenOwner = 'account' | 'unknown' | 'user' + async function fetchCloudflareProbes( accessToken: string, - caller = 'oauth_callback_identity_probe' -): Promise<[Response, Response]> { + caller: string, + tokenOwner: CloudflareTokenOwner +): Promise<{ user?: Response; accounts: Response }> { const headers = { Authorization: `Bearer ${accessToken}` } try { - return await Promise.all([ - fetchWithRetry(`${env.CLOUDFLARE_API_BASE}/user`, { headers }, { caller }), + const userRequest = + tokenOwner === 'account' + ? Promise.resolve(undefined) + : fetchWithRetry(`${env.CLOUDFLARE_API_BASE}/user`, { headers }, { caller }) + const [user, accounts] = await Promise.all([ + userRequest, fetchWithRetry( `${env.CLOUDFLARE_API_BASE}/accounts?per_page=${ACCOUNTS_PROBE_PAGE_SIZE}`, { headers }, { caller } ) ]) + return { user, accounts } } catch (error) { console.error('Cloudflare API request failed', error) throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) @@ -362,43 +385,38 @@ async function fetchCloudflareProbes( } /** - * Fetch user and accounts from Cloudflare API + * Resolve the identity and account access of a Cloudflare credential. Prefixes + * are owner hints: account-owned `cfat_` tokens skip `/user`; user-owned + * `cfut_` and `cfoat_` credentials require user identity; legacy unprefixed + * values retain response-based owner inference. */ export async function getUserAndAccounts( accessToken: string, - caller = 'oauth_callback_identity_probe' + caller = 'oauth_callback_identity_probe', + tokenOwner: CloudflareTokenOwner = 'unknown' ): Promise<{ user: UserSchema | null accounts: AccountSchema[] accountCount?: number }> { - const [userResp, accountsResp] = await fetchCloudflareProbes(accessToken, caller) + const { user: userResp, accounts: accountsResp } = await fetchCloudflareProbes( + accessToken, + caller, + tokenOwner + ) - // Check for upstream errors before parsing - if (!userResp.ok && !accountsResp.ok) { - console.error(`Cloudflare API error: user=${userResp.status}, accounts=${accountsResp.status}`) - throwCombinedCloudflareApiError(userResp, accountsResp) + if (!accountsResp.ok && (!userResp || !userResp.ok)) { + console.warn( + `Cloudflare API identity probe failed: user=${userResp?.status ?? 'skipped'}, accounts=${accountsResp.status}` + ) + if (userResp) throwCloudflareApiError(userResp, accountsResp) + throwCloudflareApiError(accountsResp) } - // Parse user from response - let user: UserSchema | null = null - if (userResp.ok) { - try { - const json = (await userResp.json()) as { success?: boolean; result?: unknown } - if (json.success && json.result) { - const parsed = UserSchema.safeParse(json.result) - if (parsed.success) { - user = parsed.data - } else { - console.error('Cloudflare API /user payload did not match expected shape', parsed.error) - } - } - } catch (error) { - console.error('Cloudflare API /user response is not valid JSON', error) - } + if (!accountsResp.ok && userResp?.ok && tokenOwner === 'user') { + throwCloudflareApiError(accountsResp) } - // Parse accounts from response let accounts: AccountSchema[] = [] let totalAccountCount: number | undefined if (accountsResp.ok) { @@ -438,17 +456,44 @@ export async function getUserAndAccounts( } } + if (!userResp) { + if (accounts.length === 1) return { user: null, accounts } + throw new OAuthError( + 'invalid_token', + 'Account token must resolve to exactly one Cloudflare account', + 401 + ) + } + + let user: UserSchema | null = null + if (userResp.ok) { + try { + const json = (await userResp.json()) as { success?: boolean; result?: unknown } + if (json.success && json.result) { + const parsed = UserSchema.safeParse(json.result) + if (parsed.success) { + user = parsed.data + } else { + console.error('Cloudflare API /user payload did not match expected shape', parsed.error) + } + } + } catch (error) { + console.error('Cloudflare API /user response is not valid JSON', error) + } + } else if (tokenOwner === 'user' || userResp.status >= 429) { + throwCloudflareApiError(userResp) + } else if (accounts.length > 0) { + return { user: null, accounts } + } + if (user) { - // Don't persist a long account list. Prefer the API-reported total; the - // page count fallback preserves availability if pagination metadata drifts. if (totalAccountCount !== undefined && totalAccountCount > MAX_STORED_ACCOUNTS) { return { user, accounts: [], accountCount: totalAccountCount } } return { user, accounts } } - // Account-scoped token - user will be null - if (accounts.length > 0) { + if (tokenOwner === 'unknown' && accounts.length > 0) { return { user: null, accounts } } diff --git a/src/index.ts b/src/index.ts index ae5dba9..dea4595 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,10 +3,9 @@ import OAuthProvider, { type OAuthProviderOptions } from '@cloudflare/workers-oauth-provider' import { createAuthHandlers, handleTokenExchangeCallback } from './auth/oauth-handler' -import { isDirectApiToken, handleApiTokenRequest } from './auth/api-token-mode' +import { resolveCloudflareToken } from './auth/api-token-mode' import { MCP_ROUTE, - handleAuthenticatedMcpRequest, handleMcpPreflight, oauthMcpHandler, rejectInvalidMcpRequest @@ -34,15 +33,8 @@ export default { if (request.method === 'OPTIONS') return handleMcpPreflight(request) } - // Check for direct API token first (like GitHub MCP's PAT support) - if (isMcpRoute && isDirectApiToken(request)) { - const response = await handleApiTokenRequest(request, (props) => - handleAuthenticatedMcpRequest(request, props) - ) - if (response) return response - } - - // OAuth mode - handle via workers-oauth-provider + // workers-oauth-provider resolves its own access tokens first, then delegates + // direct Cloudflare API/OAuth credentials to resolveExternalToken. const oauthOptions: OAuthProviderOptions = { apiHandlers: { [MCP_ROUTE]: oauthMcpHandler @@ -52,6 +44,7 @@ export default { authorizeEndpoint: '/authorize', tokenEndpoint: '/token', clientRegistrationEndpoint: '/register', + resolveExternalToken: resolveCloudflareToken, tokenExchangeCallback: (options) => handleTokenExchangeCallback( options, diff --git a/tests/auth/api-token-mode.test.ts b/tests/auth/api-token-mode.test.ts index f31d509..dcc8d6e 100644 --- a/tests/auth/api-token-mode.test.ts +++ b/tests/auth/api-token-mode.test.ts @@ -1,203 +1,342 @@ -import { env } from 'cloudflare:workers' +import { env, exports } from 'cloudflare:workers' +import { ExternalTokenError } from '@cloudflare/workers-oauth-provider' import { http, HttpResponse } from 'msw' -import { describe, it, expect, vi, afterEach } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { - isDirectApiToken, - extractBearerToken, buildAuthProps, - handleApiTokenRequest + cloudflareTokenOwner, + resolveCloudflareToken } from '../../src/auth/api-token-mode' import { AUTH_PROPS_VERSION } from '../../src/auth/types' import { API_BASE, cfAccountsSuccess, cfError, cfSuccess } from '../helpers/cloudflare-api' import { clearKv } from '../helpers/kv' +import { MCP_URL, modernMcpRequest } from '../helpers/mcp' import { server } from '../setup/msw' -/** - * Helper to create a mock Request with given Authorization header - */ -function mockRequest(authHeader?: string): Request { - const headers = new Headers() - if (authHeader) { - headers.set('Authorization', authHeader) +vi.mock('../../src/utils/fetch-retry', async (importOriginal) => { + const original = await importOriginal() + return { + ...original, + fetchWithRetry: (input: RequestInfo, init?: RequestInit) => + original.fetchWithRetry(input, init, { maxRetries: 0 }) + } +}) + +const USER = { id: 'user-1', email: 'user@example.com' } +const ACCOUNT = { id: 'account-1', name: 'Account One' } + +function resolverInput(token: string) { + return { + token, + request: new Request(MCP_URL), + env } - return new Request('https://example.com', { headers }) } -afterEach(() => clearKv(env.OAUTH_KV)) +async function sha256Hex(value: string): Promise { + const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(value)) + return [...new Uint8Array(digest)].map((byte) => byte.toString(16).padStart(2, '0')).join('') +} -describe('isDirectApiToken', () => { - it('should return false for requests without Authorization header', () => { - const request = mockRequest() - expect(isDirectApiToken(request)).toBe(false) - }) +function mockIdentity(opts: { user?: () => Response; accounts?: () => Response }): { + userCalls: () => number + accountCalls: () => number +} { + let userCalls = 0 + let accountCalls = 0 + if (opts.user) { + server.use( + http.get(`${API_BASE}/user`, () => { + userCalls++ + return opts.user!() + }) + ) + } + if (opts.accounts) { + server.use( + http.get(`${API_BASE}/accounts`, () => { + accountCalls++ + return opts.accounts!() + }) + ) + } + return { userCalls: () => userCalls, accountCalls: () => accountCalls } +} - it('should return false for non-Bearer auth schemes', () => { - const request = mockRequest('Basic dXNlcjpwYXNz') - expect(isDirectApiToken(request)).toBe(false) - }) +async function expectExternalTokenError( + promise: Promise, + code: string, + statusCode: number +): Promise { + try { + await promise + throw new Error('Expected ExternalTokenError') + } catch (error) { + expect(error).toBeInstanceOf(ExternalTokenError) + expect(error).toMatchObject({ code, statusCode }) + return error as ExternalTokenError + } +} - it('should return false for OAuth tokens (3 colon-separated parts)', () => { - // OAuth tokens from workers-oauth-provider have format: userId:grantId:secret - const request = mockRequest('Bearer user123:grant456:secretabc') - expect(isDirectApiToken(request)).toBe(false) - }) +afterEach(() => clearKv(env.OAUTH_KV)) - it('should return true for Cloudflare API tokens (no colons)', () => { - // Cloudflare API tokens are typically long alphanumeric strings - const request = mockRequest('Bearer abcdef1234567890abcdef1234567890') - expect(isDirectApiToken(request)).toBe(true) +describe('Cloudflare token ownership', () => { + it.each([ + ['cfat_account-token', 'account'], + ['cfut_user-token', 'user'], + ['cfoat_wrangler-token', 'user'], + ['legacy-unprefixed-token', 'unknown'] + ] as const)('classifies %s as %s', (token, owner) => { + expect(cloudflareTokenOwner(token)).toBe(owner) }) +}) - it('should return true for tokens with 1 colon (not OAuth format)', () => { - const request = mockRequest('Bearer part1:part2') - expect(isDirectApiToken(request)).toBe(true) +describe('buildAuthProps', () => { + it('builds versioned user props', () => { + expect(buildAuthProps('token', USER, [ACCOUNT], 1)).toEqual({ + type: 'user_token', + accessToken: 'token', + user: USER, + accounts: [ACCOUNT], + accountCount: 1, + version: AUTH_PROPS_VERSION + }) }) - it('should return true for tokens with 4+ colons (not OAuth format)', () => { - const request = mockRequest('Bearer a:b:c:d:e') - expect(isDirectApiToken(request)).toBe(true) + it('builds account props only for exactly one account', () => { + expect(buildAuthProps('token', null, [ACCOUNT])).toEqual({ + type: 'account_token', + accessToken: 'token', + account: ACCOUNT + }) + expect(() => buildAuthProps('token', null, [])).toThrow( + 'Account token must resolve to exactly one Cloudflare account' + ) + expect(() => + buildAuthProps('token', null, [ACCOUNT, { id: 'account-2', name: 'Two' }]) + ).toThrow('Account token must resolve to exactly one Cloudflare account') }) }) -describe('extractBearerToken', () => { - it('should return null for requests without Authorization header', () => { - const request = mockRequest() - expect(extractBearerToken(request)).toBeNull() - }) +describe('resolveCloudflareToken', () => { + it('uses only /accounts for prefixed account tokens', async () => { + const calls = mockIdentity({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - it('should return null for non-Bearer auth schemes', () => { - const request = mockRequest('Basic dXNlcjpwYXNz') - expect(extractBearerToken(request)).toBeNull() + await expect(resolveCloudflareToken(resolverInput('cfat_account-token'))).resolves.toEqual({ + props: { + type: 'account_token', + accessToken: 'cfat_account-token', + account: ACCOUNT + } + }) + expect(calls.userCalls()).toBe(0) + expect(calls.accountCalls()).toBe(1) }) - it('should extract token from valid Bearer header', () => { - const request = mockRequest('Bearer my-secret-token') - expect(extractBearerToken(request)).toBe('my-secret-token') - }) + it.each(['cfut_user-token', 'cfoat_wrangler-token'])( + 'uses /user and /accounts for prefixed user credential %s', + async (token) => { + const calls = mockIdentity({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - it('should handle tokens with special characters', () => { - const request = mockRequest('Bearer abc:def:ghi') - expect(extractBearerToken(request)).toBe('abc:def:ghi') - }) + await expect(resolveCloudflareToken(resolverInput(token))).resolves.toMatchObject({ + props: { type: 'user_token', accessToken: token, user: USER, accounts: [ACCOUNT] } + }) + expect(calls.userCalls()).toBe(1) + expect(calls.accountCalls()).toBe(1) + } + ) + + it('retains response-based account inference for legacy credentials', async () => { + const calls = mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 403 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - it('should handle tokens with spaces after Bearer', () => { - // "Bearer token" - double space, should return " token" - const request = mockRequest('Bearer token-with-leading-space') - expect(extractBearerToken(request)).toBe(' token-with-leading-space') + await expect( + resolveCloudflareToken(resolverInput('legacy-account-token')) + ).resolves.toMatchObject({ + props: { type: 'account_token', account: ACCOUNT } + }) + expect(calls.userCalls()).toBe(1) + expect(calls.accountCalls()).toBe(1) }) -}) -describe('buildAuthProps', () => { - const mockToken = 'test-token-123' - const mockUser = { id: 'user-1', email: 'test@example.com' } - const mockAccounts = [ - { id: 'acc-1', name: 'Account One' }, - { id: 'acc-2', name: 'Account Two' } - ] + it('does not infer account ownership for a known user token', async () => { + mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 403 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - it('should build user_token props when user is provided', () => { - const props = buildAuthProps(mockToken, mockUser, mockAccounts) + const error = await expectExternalTokenError( + resolveCloudflareToken(resolverInput('cfut_user-token-no-user-scope')), + 'insufficient_scope', + 403 + ) + expect(error.requiredScopes).toEqual(['user:read', 'account:read']) + }) - expect(props).toEqual({ - type: 'user_token', - accessToken: mockToken, - user: mockUser, - accounts: mockAccounts, - accountCount: undefined, - version: AUTH_PROPS_VERSION + it('uses account-only scope guidance for account token failures', async () => { + const calls = mockIdentity({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfError([], null), { status: 403 }) }) - }) - it('should build user_token props with empty accounts if not provided', () => { - const props = buildAuthProps(mockToken, mockUser) + const error = await expectExternalTokenError( + resolveCloudflareToken(resolverInput('cfat_account-token-no-read-scope')), + 'insufficient_scope', + 403 + ) + expect(error.requiredScopes).toEqual(['account:read']) + expect(calls.userCalls()).toBe(0) + expect(calls.accountCalls()).toBe(1) + }) - expect(props).toEqual({ - type: 'user_token', - accessToken: mockToken, - user: mockUser, - accounts: [], - accountCount: undefined, - version: AUTH_PROPS_VERSION + it('caches a verified identity by token hash', async () => { + const calls = mockIdentity({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) + const input = resolverInput('cfut_cached-user-token') + + await resolveCloudflareToken(input) + await resolveCloudflareToken(input) + + expect(calls.userCalls()).toBe(1) + expect(calls.accountCalls()).toBe(1) }) - it('should build account_token props when no user but has accounts', () => { - const props = buildAuthProps(mockToken, null, mockAccounts) + it('ignores malformed cached identity data and revalidates upstream', async () => { + const token = 'cfut_invalid-cache-token' + await env.OAUTH_KV.put( + `api-token-identity:v3:${await sha256Hex(token)}`, + JSON.stringify({ user: 'not-an-object', accounts: [] }) + ) + const calls = mockIdentity({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - expect(props).toEqual({ - type: 'account_token', - accessToken: mockToken, - account: mockAccounts[0] // Uses first account + await expect(resolveCloudflareToken(resolverInput(token))).resolves.toMatchObject({ + props: { type: 'user_token', user: USER, accounts: [ACCOUNT] } }) + expect(calls.userCalls()).toBe(1) + expect(calls.accountCalls()).toBe(1) }) - it('should throw error when no user and no accounts', () => { - expect(() => buildAuthProps(mockToken, null, [])).toThrow( - 'Cannot build auth props: no user or account information' + it('maps malformed credentials to invalid_token', async () => { + mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 400 }), + accounts: () => HttpResponse.json(cfError([], null), { status: 400 }) + }) + + const error = await expectExternalTokenError( + resolveCloudflareToken(resolverInput('malformed-token')), + 'invalid_token', + 401 ) + expect(error.description).toContain('appears malformed') }) - it('should throw error when no user and accounts undefined', () => { - expect(() => buildAuthProps(mockToken, null, undefined)).toThrow( - 'Cannot build auth props: no user or account information' + it('preserves rate-limit backoff', async () => { + mockIdentity({ + user: () => + HttpResponse.json(cfError([], null), { + status: 429, + headers: { 'Retry-After': '17' } + }), + accounts: () => HttpResponse.json(cfError([], null), { status: 429 }) + }) + + const error = await expectExternalTokenError( + resolveCloudflareToken(resolverInput('cfoat_rate-limited-token')), + 'temporarily_unavailable', + 429 ) + expect(error.headers).toEqual({ 'Retry-After': '17' }) }) - it('should treat undefined user same as null', () => { - const props = buildAuthProps(mockToken, undefined, mockAccounts) + it('maps upstream failures to server_error', async () => { + mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 502 }), + accounts: () => HttpResponse.json(cfError([], null), { status: 401 }) + }) - expect(props.type).toBe('account_token') + await expectExternalTokenError( + resolveCloudflareToken(resolverInput('legacy-upstream-failure')), + 'server_error', + 502 + ) }) }) -describe('handleApiTokenRequest identity probe caching', () => { - const token = 'api-token-123' - const tokenHash = '9bdb81d121b42d1c7819c816fa3cfbb6ee109726f9ed2475edb169374881d7b3' - const cacheKey = `api-token-identity:v2:${tokenHash}` - const user = { id: 'user-1', email: 'test@example.com' } - const accounts = [{ id: 'acc-1', name: 'Account One' }] - - // Count upstream identity probes so we can assert cache hits/misses without - // mocking our own getUserAndAccounts — the REAL function runs against MSW. - function mockUserProbe(): () => number { - let calls = 0 - server.use( - http.get(`${API_BASE}/user`, () => { - calls++ - return HttpResponse.json(cfSuccess(user)) - }), - http.get(`${API_BASE}/accounts`, () => HttpResponse.json(cfAccountsSuccess(accounts))) +describe('external token resource responses', () => { + it('lets the provider construct the RFC 6750/9728 invalid-token response', async () => { + mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 400 }), + accounts: () => HttpResponse.json(cfError([], null), { status: 400 }) + }) + + const response = await exports.default.fetch( + modernMcpRequest('malformed-provider-token', 'server/discover') ) - return () => calls - } - it('stores API token identity lookups in KV by token hash', async () => { - const probeCalls = mockUserProbe() - const createMcpResponse = vi.fn().mockResolvedValue(new Response('ok')) + expect(response.status).toBe(401) + expect(response.headers.get('cache-control')).toBe('no-store') + expect(response.headers.get('www-authenticate')).toContain('error="invalid_token"') + expect(response.headers.get('www-authenticate')).toContain( + `resource_metadata="${new URL(MCP_URL).origin}/.well-known/oauth-protected-resource/mcp"` + ) + await expect(response.json()).resolves.toEqual({ + error: 'invalid_token', + error_description: 'Access token appears malformed; reauthenticate and try again' + }) + }) + + it('lets the provider construct an insufficient-scope challenge', async () => { + mockIdentity({ + user: () => HttpResponse.json(cfError([], null), { status: 403 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) - await handleApiTokenRequest(mockRequest(`Bearer ${token}`), createMcpResponse) + const response = await exports.default.fetch( + modernMcpRequest('cfut_provider-insufficient-scope', 'server/discover') + ) - // The real probe ran once and the identity was written to real KV. - expect(probeCalls()).toBe(1) - expect(await env.OAUTH_KV.get(cacheKey, 'json')).toEqual({ user, accounts }) - expect(createMcpResponse).toHaveBeenCalledWith(buildAuthProps(token, user, accounts)) + expect(response.status).toBe(403) + expect(response.headers.get('www-authenticate')).toContain('error="insufficient_scope"') + expect(response.headers.get('www-authenticate')).toContain('scope="user:read account:read"') + await expect(response.json()).resolves.toEqual({ + error: 'insufficient_scope', + error_description: 'Token lacks required user:read or account:read scope' + }) }) - it('uses cached API token identity from KV without probing upstream', async () => { - // Seed the real cache; any upstream /user hit would throw (unhandled). - await env.OAUTH_KV.put(cacheKey, JSON.stringify({ user, accounts })) - let probed = false - server.use( - http.get(`${API_BASE}/user`, () => { - probed = true - return HttpResponse.json(cfError([], null)) - }) - ) - const createMcpResponse = vi.fn().mockResolvedValue(new Response('ok')) + it('lets the provider preserve a retryable rate-limit response', async () => { + mockIdentity({ + user: () => + HttpResponse.json(cfError([], null), { + status: 429, + headers: { 'Retry-After': '23' } + }), + accounts: () => HttpResponse.json(cfError([], null), { status: 429 }) + }) - await handleApiTokenRequest(mockRequest(`Bearer ${token}`), createMcpResponse) + const response = await exports.default.fetch( + modernMcpRequest('cfoat_provider-rate-limit', 'server/discover') + ) - expect(probed).toBe(false) - expect(createMcpResponse).toHaveBeenCalledWith(buildAuthProps(token, user, accounts)) + expect(response.status).toBe(429) + expect(response.headers.get('retry-after')).toBe('23') + expect(response.headers.get('cache-control')).toBe('no-store') + await expect(response.json()).resolves.toEqual({ + error: 'temporarily_unavailable', + error_description: 'Rate limited, try again later' + }) }) }) diff --git a/tests/auth/oauth-routes.test.ts b/tests/auth/oauth-routes.test.ts index 1c5dda7..1975c48 100644 --- a/tests/auth/oauth-routes.test.ts +++ b/tests/auth/oauth-routes.test.ts @@ -20,6 +20,8 @@ import { server } from '../setup/msw' const REDIRECT_URI = 'https://app.example.com/cb' const MCP_ORIGIN = 'https://mcp.cloudflare.com' +const DOWNSTREAM_CODE_VERIFIER = 'test-downstream-code-verifier' +const DOWNSTREAM_CODE_CHALLENGE = 'I4fhllfHqqQsgap17V2SDI0scSei8H7U0e0rZBDIcbo' /** Register a client via the provider's RFC 7591 endpoint; returns its id. */ async function registerClient(): Promise { @@ -62,6 +64,8 @@ async function beginAuthorization(options: { state?: string; scopes?: string } = response_type: 'code', client_id: clientId, redirect_uri: REDIRECT_URI, + code_challenge: DOWNSTREAM_CODE_CHALLENGE, + code_challenge_method: 'S256', scope: 'user:read', ...(options.state === undefined ? {} : { state: options.state }) }) @@ -152,6 +156,8 @@ describe('GET /authorize', () => { response_type: 'code', client_id: clientId, redirect_uri: REDIRECT_URI, + code_challenge: DOWNSTREAM_CODE_CHALLENGE, + code_challenge_method: 'S256', scope: 'user:read' }) ) @@ -232,7 +238,9 @@ describe('GET /authorize', () => { authorizeUrl({ response_type: 'code', client_id: 'does-not-exist', - redirect_uri: REDIRECT_URI + redirect_uri: REDIRECT_URI, + code_challenge: DOWNSTREAM_CODE_CHALLENGE, + code_challenge_method: 'S256' }) ) ) @@ -306,7 +314,8 @@ describe('GET /oauth/callback', () => { grant_type: 'authorization_code', code: code!, client_id: clientId, - redirect_uri: REDIRECT_URI + redirect_uri: REDIRECT_URI, + code_verifier: DOWNSTREAM_CODE_VERIFIER }).toString() }) ) diff --git a/tests/helpers/cloudflare-api.ts b/tests/helpers/cloudflare-api.ts index 8bebb15..dd8f5ac 100644 --- a/tests/helpers/cloudflare-api.ts +++ b/tests/helpers/cloudflare-api.ts @@ -28,8 +28,8 @@ export function cfError(errors: Array<{ code: number; message: string }>, result } /** - * Register MSW handlers for the API-token identity probe (`/user` + `/accounts`) - * so a direct token resolves through the REAL `getUserAndAccounts` code path. + * Register MSW handlers for legacy/user credential identity probes + * (`/user` + `/accounts`) through the real external-token resolver. * * - `user: null` (default) -> account-scoped token (single account pinned) * - `user` provided -> user token diff --git a/tests/mcp-modern.test.ts b/tests/mcp-modern.test.ts index b57b888..cf40706 100644 --- a/tests/mcp-modern.test.ts +++ b/tests/mcp-modern.test.ts @@ -256,6 +256,7 @@ describe('MCP 2026-07-28 stateless handler', () => { expect(response.headers.get('www-authenticate')).toMatch( /^Bearer .*resource_metadata="https:\/\/mcp\.cloudflare\.com\/\.well-known\/oauth-protected-resource\/mcp"/ ) + expect(await response.text()).toBe('') }) }) diff --git a/tests/worker.test.ts b/tests/worker.test.ts index 1d8f9e2..578196b 100644 --- a/tests/worker.test.ts +++ b/tests/worker.test.ts @@ -1,7 +1,7 @@ import { env } from 'cloudflare:workers' import { http, HttpResponse } from 'msw' import { afterEach, describe, expect, it } from 'vitest' -import { API_BASE, cfSuccess, mockIdentityProbe } from './helpers/cloudflare-api' +import { API_BASE, cfAccountsSuccess, cfSuccess } from './helpers/cloudflare-api' import { clearKv } from './helpers/kv' import { callTool, toolText } from './helpers/mcp' import { server } from './setup/msw' @@ -21,16 +21,23 @@ import { server } from './setup/msw' const ACCOUNT_ID = '00000000000000000000000000000001' -// A direct (non-OAuth) API token: NOT 3 colon-separated parts, so the worker -// treats it as a direct Cloudflare API token rather than an OAuth bearer. -const API_TOKEN = 'test-api-token-e2e' +// A prefixed account-owned Cloudflare API token. The OAuth provider tries its +// own token store first, then delegates the miss to the external token resolver. +const API_TOKEN = 'cfat_test-api-token-e2e' afterEach(() => clearKv(env.OAUTH_KV)) describe('worker: execute tool call', () => { it('runs code in a Worker Loader isolate and returns the mocked API result', async () => { - // Real getUserAndAccounts resolves this token to a single-account token. - mockIdentityProbe({ accounts: [{ id: ACCOUNT_ID, name: 'E2E Test Account' }] }) + // Account prefixes skip /user. MSW fails this test if an unregistered /user + // request occurs, so only register the one expected identity probe. + let accountProbeCalls = 0 + server.use( + http.get(`${API_BASE}/accounts`, () => { + accountProbeCalls++ + return HttpResponse.json(cfAccountsSuccess([{ id: ACCOUNT_ID, name: 'E2E Test Account' }])) + }) + ) // The Cloudflare API call the executed code makes, forwarded by GlobalOutbound. let verifyCalled = false @@ -59,6 +66,7 @@ describe('worker: execute tool call', () => { expect(text).toContain('"status": "active"') expect(text).toContain('token-1') + expect(accountProbeCalls).toBe(1) // The forwarded API call really went through GlobalOutbound -> MSW. expect(verifyCalled).toBe(true) }) From 6379beb41c9458e868b2fc89804709721348634a Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Wed, 29 Jul 2026 20:43:16 +0100 Subject: [PATCH 2/6] refactor(auth): name external token resolver after provider hook --- src/auth/api-token-mode.ts | 2 +- src/index.ts | 4 ++-- tests/auth/api-token-mode.test.ts | 26 +++++++++++++------------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/auth/api-token-mode.ts b/src/auth/api-token-mode.ts index e4a6989..271a448 100644 --- a/src/auth/api-token-mode.ts +++ b/src/auth/api-token-mode.ts @@ -138,7 +138,7 @@ export function buildAuthProps( * access-token lookup misses. workers-oauth-provider owns bearer parsing, * standards-compliant error responses, and injection of the returned props. */ -export async function resolveCloudflareToken({ +export async function resolveExternalToken({ token, env }: ResolveExternalTokenInput): Promise { diff --git a/src/index.ts b/src/index.ts index dea4595..7f7f8f9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,7 +3,7 @@ import OAuthProvider, { type OAuthProviderOptions } from '@cloudflare/workers-oauth-provider' import { createAuthHandlers, handleTokenExchangeCallback } from './auth/oauth-handler' -import { resolveCloudflareToken } from './auth/api-token-mode' +import { resolveExternalToken } from './auth/api-token-mode' import { MCP_ROUTE, handleMcpPreflight, @@ -44,7 +44,7 @@ export default { authorizeEndpoint: '/authorize', tokenEndpoint: '/token', clientRegistrationEndpoint: '/register', - resolveExternalToken: resolveCloudflareToken, + resolveExternalToken, tokenExchangeCallback: (options) => handleTokenExchangeCallback( options, diff --git a/tests/auth/api-token-mode.test.ts b/tests/auth/api-token-mode.test.ts index dcc8d6e..b0ed13d 100644 --- a/tests/auth/api-token-mode.test.ts +++ b/tests/auth/api-token-mode.test.ts @@ -5,7 +5,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { buildAuthProps, cloudflareTokenOwner, - resolveCloudflareToken + resolveExternalToken } from '../../src/auth/api-token-mode' import { AUTH_PROPS_VERSION } from '../../src/auth/types' import { API_BASE, cfAccountsSuccess, cfError, cfSuccess } from '../helpers/cloudflare-api' @@ -118,14 +118,14 @@ describe('buildAuthProps', () => { }) }) -describe('resolveCloudflareToken', () => { +describe('resolveExternalToken', () => { it('uses only /accounts for prefixed account tokens', async () => { const calls = mockIdentity({ user: () => HttpResponse.json(cfSuccess(USER)), accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) - await expect(resolveCloudflareToken(resolverInput('cfat_account-token'))).resolves.toEqual({ + await expect(resolveExternalToken(resolverInput('cfat_account-token'))).resolves.toEqual({ props: { type: 'account_token', accessToken: 'cfat_account-token', @@ -144,7 +144,7 @@ describe('resolveCloudflareToken', () => { accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) - await expect(resolveCloudflareToken(resolverInput(token))).resolves.toMatchObject({ + await expect(resolveExternalToken(resolverInput(token))).resolves.toMatchObject({ props: { type: 'user_token', accessToken: token, user: USER, accounts: [ACCOUNT] } }) expect(calls.userCalls()).toBe(1) @@ -159,7 +159,7 @@ describe('resolveCloudflareToken', () => { }) await expect( - resolveCloudflareToken(resolverInput('legacy-account-token')) + resolveExternalToken(resolverInput('legacy-account-token')) ).resolves.toMatchObject({ props: { type: 'account_token', account: ACCOUNT } }) @@ -174,7 +174,7 @@ describe('resolveCloudflareToken', () => { }) const error = await expectExternalTokenError( - resolveCloudflareToken(resolverInput('cfut_user-token-no-user-scope')), + resolveExternalToken(resolverInput('cfut_user-token-no-user-scope')), 'insufficient_scope', 403 ) @@ -188,7 +188,7 @@ describe('resolveCloudflareToken', () => { }) const error = await expectExternalTokenError( - resolveCloudflareToken(resolverInput('cfat_account-token-no-read-scope')), + resolveExternalToken(resolverInput('cfat_account-token-no-read-scope')), 'insufficient_scope', 403 ) @@ -204,8 +204,8 @@ describe('resolveCloudflareToken', () => { }) const input = resolverInput('cfut_cached-user-token') - await resolveCloudflareToken(input) - await resolveCloudflareToken(input) + await resolveExternalToken(input) + await resolveExternalToken(input) expect(calls.userCalls()).toBe(1) expect(calls.accountCalls()).toBe(1) @@ -222,7 +222,7 @@ describe('resolveCloudflareToken', () => { accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) - await expect(resolveCloudflareToken(resolverInput(token))).resolves.toMatchObject({ + await expect(resolveExternalToken(resolverInput(token))).resolves.toMatchObject({ props: { type: 'user_token', user: USER, accounts: [ACCOUNT] } }) expect(calls.userCalls()).toBe(1) @@ -236,7 +236,7 @@ describe('resolveCloudflareToken', () => { }) const error = await expectExternalTokenError( - resolveCloudflareToken(resolverInput('malformed-token')), + resolveExternalToken(resolverInput('malformed-token')), 'invalid_token', 401 ) @@ -254,7 +254,7 @@ describe('resolveCloudflareToken', () => { }) const error = await expectExternalTokenError( - resolveCloudflareToken(resolverInput('cfoat_rate-limited-token')), + resolveExternalToken(resolverInput('cfoat_rate-limited-token')), 'temporarily_unavailable', 429 ) @@ -268,7 +268,7 @@ describe('resolveCloudflareToken', () => { }) await expectExternalTokenError( - resolveCloudflareToken(resolverInput('legacy-upstream-failure')), + resolveExternalToken(resolverInput('legacy-upstream-failure')), 'server_error', 502 ) From 5e67ddcac0d56fd34990095d1923ac4e8276fe90 Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Wed, 29 Jul 2026 20:55:19 +0100 Subject: [PATCH 3/6] refactor(auth): isolate owner-aware identity probes --- AGENTS.md | 31 +-- src/auth/api-token-mode.ts | 4 +- src/auth/cloudflare-identity.ts | 206 +++++++++++++++++ src/auth/oauth-handler.ts | 210 +---------------- tests/auth/cloudflare-identity.test.ts | 298 +++++++++++++++++++++++++ tests/auth/oauth-handler.test.ts | 277 ----------------------- 6 files changed, 525 insertions(+), 501 deletions(-) create mode 100644 src/auth/cloudflare-identity.ts create mode 100644 tests/auth/cloudflare-identity.test.ts diff --git a/AGENTS.md b/AGENTS.md index 95acabd..7241418 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,21 +62,21 @@ Node 22+ required. ## Commands -| Command | What it does | -| ---------------------- | --------------------------------------------- | -| `npm run dev` | Start local dev server (wrangler dev) | -| `npm run deploy` | Deploy to staging | -| `npm run deploy:prod` | Deploy to production | -| `npm run types` | Generate worker type definitions | -| `npm run typecheck` | TypeScript type checking (no emit) | -| `npm run lint` | Lint with oxlint | -| `npm run format` | Format with oxfmt | -| `npm run format:check` | Check formatting without modifying | -| `npm run test` | Run vitest test suite | -| `npm run test:watch` | Run vitest in watch mode | -| `npm run check` | Run all checks (format, lint, typecheck, test)| -| `npm run seed:staging` | Seed OpenAPI spec to staging R2 | -| `npm run seed:prod` | Seed OpenAPI spec to production R2 | +| Command | What it does | +| ---------------------- | ---------------------------------------------- | +| `npm run dev` | Start local dev server (wrangler dev) | +| `npm run deploy` | Deploy to staging | +| `npm run deploy:prod` | Deploy to production | +| `npm run types` | Generate worker type definitions | +| `npm run typecheck` | TypeScript type checking (no emit) | +| `npm run lint` | Lint with oxlint | +| `npm run format` | Format with oxfmt | +| `npm run format:check` | Check formatting without modifying | +| `npm run test` | Run vitest test suite | +| `npm run test:watch` | Run vitest in watch mode | +| `npm run check` | Run all checks (format, lint, typecheck, test) | +| `npm run seed:staging` | Seed OpenAPI spec to staging R2 | +| `npm run seed:prod` | Seed OpenAPI spec to production R2 | ## Code standards @@ -172,6 +172,7 @@ npm run test:watch # Watch mode ``` **Unit/integration coverage areas:** + - Scheduled handler (spec fetching & processing) - Auth token detection and parsing - Auth props building and validation diff --git a/src/auth/api-token-mode.ts b/src/auth/api-token-mode.ts index 271a448..13af489 100644 --- a/src/auth/api-token-mode.ts +++ b/src/auth/api-token-mode.ts @@ -6,7 +6,7 @@ import { } from '@cloudflare/workers-oauth-provider' import { z } from 'zod' -import { getUserAndAccounts, type CloudflareTokenOwner } from './oauth-handler' +import { getCloudflareIdentity, type CloudflareTokenOwner } from './cloudflare-identity' import { OAuthError } from './workers-oauth-utils' import { @@ -59,7 +59,7 @@ async function getCachedApiTokenIdentity( console.warn('api_token_identity_probe kv-cache read failed', error) } - const identity = await getUserAndAccounts(token, 'api_token_identity_probe', tokenOwner) + const identity = await getCloudflareIdentity(token, 'api_token_identity_probe', tokenOwner) try { await kv.put(cacheKey, JSON.stringify(identity), { diff --git a/src/auth/cloudflare-identity.ts b/src/auth/cloudflare-identity.ts new file mode 100644 index 0000000..66c49f1 --- /dev/null +++ b/src/auth/cloudflare-identity.ts @@ -0,0 +1,206 @@ +import { env as cloudflareEnv } from 'cloudflare:workers' + +import { fetchWithRetry } from '../utils/fetch-retry' +import { + AccountsSchema, + ACCOUNTS_PROBE_PAGE_SIZE, + MAX_STORED_ACCOUNTS, + UserSchema, + type AccountSchema, + type UserSchema as UserIdentity +} from './types' +import { OAuthError } from './workers-oauth-utils' + +const env = cloudflareEnv as Env + +export type CloudflareTokenOwner = 'account' | 'unknown' | 'user' + +function retryAfterHeaders(...responses: Response[]): Record { + return { + 'Retry-After': + responses.find((response) => response.status === 429)?.headers.get('Retry-After') ?? '30' + } +} + +function throwIdentityProbeError(...responses: [Response, ...Response[]]): never { + const statuses = responses.map((response) => response.status) + + if (statuses.some((status) => status >= 500)) { + throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) + } + if (statuses.includes(429)) { + throw new OAuthError( + 'temporarily_unavailable', + 'Rate limited, try again later', + 429, + retryAfterHeaders(...responses) + ) + } + if (statuses.includes(401)) { + throw new OAuthError('invalid_token', 'Access token is invalid or expired', 401) + } + if (statuses.includes(403)) { + throw new OAuthError( + 'insufficient_scope', + 'Token lacks required user:read or account:read scope', + 403 + ) + } + if (statuses.includes(400)) { + throw new OAuthError( + 'invalid_token', + 'Access token appears malformed; reauthenticate and try again', + 401 + ) + } + + throw new OAuthError('invalid_token', 'Access token is invalid or expired', statuses[0]) +} + +async function fetchIdentityProbes( + accessToken: string, + caller: string, + tokenOwner: CloudflareTokenOwner +): Promise<{ user?: Response; accounts: Response }> { + const headers = { Authorization: `Bearer ${accessToken}` } + + try { + // The cfat_ prefix proves this is account-owned, so /user cannot add + // identity information and would only spend another upstream request. + const userRequest = + tokenOwner === 'account' + ? Promise.resolve(undefined) + : fetchWithRetry(`${env.CLOUDFLARE_API_BASE}/user`, { headers }, { caller }) + const [user, accounts] = await Promise.all([ + userRequest, + fetchWithRetry( + `${env.CLOUDFLARE_API_BASE}/accounts?per_page=${ACCOUNTS_PROBE_PAGE_SIZE}`, + { headers }, + { caller } + ) + ]) + return { user, accounts } + } catch (error) { + console.error('Cloudflare API request failed', error) + throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) + } +} + +/** Resolve one Cloudflare credential into the identity stored in request props. */ +export async function getCloudflareIdentity( + accessToken: string, + caller = 'oauth_callback_identity_probe', + tokenOwner: CloudflareTokenOwner = 'unknown' +): Promise<{ + user: UserIdentity | null + accounts: AccountSchema[] + accountCount?: number +}> { + const { user: userResponse, accounts: accountsResponse } = await fetchIdentityProbes( + accessToken, + caller, + tokenOwner + ) + + if (!accountsResponse.ok && (!userResponse || !userResponse.ok)) { + console.warn( + `Cloudflare API identity probe failed: user=${userResponse?.status ?? 'skipped'}, accounts=${accountsResponse.status}` + ) + if (userResponse) throwIdentityProbeError(userResponse, accountsResponse) + throwIdentityProbeError(accountsResponse) + } + if (!accountsResponse.ok && userResponse?.ok && tokenOwner === 'user') { + throwIdentityProbeError(accountsResponse) + } + + let accounts: AccountSchema[] = [] + let totalAccountCount: number | undefined + if (accountsResponse.ok) { + try { + const json = (await accountsResponse.json()) as { + success?: boolean + result?: unknown + result_info?: { count?: unknown; total_count?: unknown } + } + if (json.success && json.result) { + const parsed = AccountsSchema.safeParse(json.result) + if (parsed.success) { + accounts = parsed.data + const reported = json.result_info?.total_count + if (typeof reported === 'number' && Number.isFinite(reported) && reported >= 0) { + totalAccountCount = reported + } else { + const pageCount = json.result_info?.count + totalAccountCount = + typeof pageCount === 'number' && Number.isFinite(pageCount) && pageCount >= 0 + ? Math.max(pageCount, accounts.length) + : accounts.length + console.warn( + `Cloudflare API /accounts response is missing a valid result_info.total_count; ` + + `falling back to page count ${totalAccountCount}` + ) + } + } else { + console.error( + 'Cloudflare API /accounts payload did not match expected shape', + parsed.error + ) + } + } + } catch (error) { + console.error('Cloudflare API /accounts response is not valid JSON', error) + } + } + + // A prefixed account token must resolve to its one owning account. It never + // uses response-based inference and never calls /user. + if (!userResponse) { + if (accounts.length === 1) return { user: null, accounts } + throw new OAuthError( + 'invalid_token', + 'Account token must resolve to exactly one Cloudflare account', + 401 + ) + } + + let user: UserIdentity | null = null + if (userResponse.ok) { + try { + const json = (await userResponse.json()) as { success?: boolean; result?: unknown } + if (json.success && json.result) { + const parsed = UserSchema.safeParse(json.result) + if (parsed.success) { + user = parsed.data + } else { + console.error('Cloudflare API /user payload did not match expected shape', parsed.error) + } + } + } catch (error) { + console.error('Cloudflare API /user response is not valid JSON', error) + } + } else if (tokenOwner === 'user' || userResponse.status >= 429) { + // Known user credentials can never become account credentials. Transient + // failures also cannot be used as evidence of account ownership. + throwIdentityProbeError(userResponse) + } else if (accounts.length > 0) { + // Only unprefixed legacy credentials use response-based owner inference. + return { user: null, accounts } + } + + if (user) { + if (totalAccountCount !== undefined && totalAccountCount > MAX_STORED_ACCOUNTS) { + return { user, accounts: [], accountCount: totalAccountCount } + } + return { user, accounts } + } + + if (tokenOwner === 'unknown' && accounts.length > 0) { + return { user: null, accounts } + } + + throw new OAuthError( + 'invalid_token', + 'Failed to verify token: no user or account information', + 401 + ) +} diff --git a/src/auth/oauth-handler.ts b/src/auth/oauth-handler.ts index 7f43bcc..b76c284 100644 --- a/src/auth/oauth-handler.ts +++ b/src/auth/oauth-handler.ts @@ -8,16 +8,7 @@ import { refreshAuthToken } from './cloudflare-auth' import { DEFAULT_TEMPLATE, REQUIRED_SCOPES, SCOPE_DEFINITIONS, SCOPE_TEMPLATES } from './scopes' -import { - UserSchema, - AccountsSchema, - AuthProps as AuthPropsSchema, - AUTH_PROPS_VERSION, - ACCOUNTS_PROBE_PAGE_SIZE, - MAX_STORED_ACCOUNTS, - type AuthProps, - type AccountSchema -} from './types' +import { AuthProps as AuthPropsSchema, AUTH_PROPS_VERSION, type AuthProps } from './types' import { clientIdAlreadyApproved, createOAuthState, @@ -29,7 +20,7 @@ import { validateOAuthState, OAuthError } from './workers-oauth-utils' -import { fetchWithRetry } from '../utils/fetch-retry' +import { getCloudflareIdentity } from './cloudflare-identity' import { MetricsTracker, AuthUser } from '../metrics' import { SERVER_INFO } from '../constants' @@ -309,201 +300,6 @@ export async function guardRefreshTokenExchange( return refreshPromise } -function getRetryAfterHeader(...responses: Response[]): Record { - return { - 'Retry-After': - responses.find((response) => response.status === 429)?.headers.get('Retry-After') ?? '30' - } -} - -function throwCloudflareApiError(...responses: [Response, ...Response[]]): never { - const statuses = responses.map((response) => response.status) - - if (statuses.some((status) => status >= 500)) { - throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) - } - - if (statuses.includes(429)) { - throw new OAuthError( - 'temporarily_unavailable', - 'Rate limited, try again later', - 429, - getRetryAfterHeader(...responses) - ) - } - - if (statuses.includes(401)) { - throw new OAuthError('invalid_token', 'Access token is invalid or expired', 401) - } - - if (statuses.includes(403)) { - throw new OAuthError( - 'insufficient_scope', - 'Token lacks required user:read or account:read scope', - 403 - ) - } - - if (statuses.includes(400)) { - throw new OAuthError( - 'invalid_token', - 'Access token appears malformed; reauthenticate and try again', - 401 - ) - } - - throw new OAuthError('invalid_token', 'Access token is invalid or expired', statuses[0]) -} - -export type CloudflareTokenOwner = 'account' | 'unknown' | 'user' - -async function fetchCloudflareProbes( - accessToken: string, - caller: string, - tokenOwner: CloudflareTokenOwner -): Promise<{ user?: Response; accounts: Response }> { - const headers = { Authorization: `Bearer ${accessToken}` } - - try { - const userRequest = - tokenOwner === 'account' - ? Promise.resolve(undefined) - : fetchWithRetry(`${env.CLOUDFLARE_API_BASE}/user`, { headers }, { caller }) - const [user, accounts] = await Promise.all([ - userRequest, - fetchWithRetry( - `${env.CLOUDFLARE_API_BASE}/accounts?per_page=${ACCOUNTS_PROBE_PAGE_SIZE}`, - { headers }, - { caller } - ) - ]) - return { user, accounts } - } catch (error) { - console.error('Cloudflare API request failed', error) - throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) - } -} - -/** - * Resolve the identity and account access of a Cloudflare credential. Prefixes - * are owner hints: account-owned `cfat_` tokens skip `/user`; user-owned - * `cfut_` and `cfoat_` credentials require user identity; legacy unprefixed - * values retain response-based owner inference. - */ -export async function getUserAndAccounts( - accessToken: string, - caller = 'oauth_callback_identity_probe', - tokenOwner: CloudflareTokenOwner = 'unknown' -): Promise<{ - user: UserSchema | null - accounts: AccountSchema[] - accountCount?: number -}> { - const { user: userResp, accounts: accountsResp } = await fetchCloudflareProbes( - accessToken, - caller, - tokenOwner - ) - - if (!accountsResp.ok && (!userResp || !userResp.ok)) { - console.warn( - `Cloudflare API identity probe failed: user=${userResp?.status ?? 'skipped'}, accounts=${accountsResp.status}` - ) - if (userResp) throwCloudflareApiError(userResp, accountsResp) - throwCloudflareApiError(accountsResp) - } - - if (!accountsResp.ok && userResp?.ok && tokenOwner === 'user') { - throwCloudflareApiError(accountsResp) - } - - let accounts: AccountSchema[] = [] - let totalAccountCount: number | undefined - if (accountsResp.ok) { - try { - const json = (await accountsResp.json()) as { - success?: boolean - result?: unknown - result_info?: { count?: unknown; total_count?: unknown } - } - if (json.success && json.result) { - const parsed = AccountsSchema.safeParse(json.result) - if (parsed.success) { - accounts = parsed.data - const reported = json.result_info?.total_count - if (typeof reported === 'number' && Number.isFinite(reported) && reported >= 0) { - totalAccountCount = reported - } else { - const pageCount = json.result_info?.count - totalAccountCount = - typeof pageCount === 'number' && Number.isFinite(pageCount) && pageCount >= 0 - ? Math.max(pageCount, accounts.length) - : accounts.length - console.warn( - `Cloudflare API /accounts response is missing a valid result_info.total_count; ` + - `falling back to page count ${totalAccountCount}` - ) - } - } else { - console.error( - 'Cloudflare API /accounts payload did not match expected shape', - parsed.error - ) - } - } - } catch (error) { - console.error('Cloudflare API /accounts response is not valid JSON', error) - } - } - - if (!userResp) { - if (accounts.length === 1) return { user: null, accounts } - throw new OAuthError( - 'invalid_token', - 'Account token must resolve to exactly one Cloudflare account', - 401 - ) - } - - let user: UserSchema | null = null - if (userResp.ok) { - try { - const json = (await userResp.json()) as { success?: boolean; result?: unknown } - if (json.success && json.result) { - const parsed = UserSchema.safeParse(json.result) - if (parsed.success) { - user = parsed.data - } else { - console.error('Cloudflare API /user payload did not match expected shape', parsed.error) - } - } - } catch (error) { - console.error('Cloudflare API /user response is not valid JSON', error) - } - } else if (tokenOwner === 'user' || userResp.status >= 429) { - throwCloudflareApiError(userResp) - } else if (accounts.length > 0) { - return { user: null, accounts } - } - - if (user) { - if (totalAccountCount !== undefined && totalAccountCount > MAX_STORED_ACCOUNTS) { - return { user, accounts: [], accountCount: totalAccountCount } - } - return { user, accounts } - } - - if (tokenOwner === 'unknown' && accounts.length > 0) { - return { user: null, accounts } - } - - throw new OAuthError( - 'invalid_token', - 'Failed to verify token: no user or account information', - 401 - ) -} - /** * Handle token refresh for workers-oauth-provider. * @@ -749,7 +545,7 @@ export function createAuthHandlers() { ]) // Fetch user and accounts - const { user, accounts, accountCount } = await getUserAndAccounts(access_token) + const { user, accounts, accountCount } = await getCloudflareIdentity(access_token) // Account-scoped tokens (user: null) are only supported via API token mode // (see api-token-mode.ts). The OAuth flow always requires a user identity. diff --git a/tests/auth/cloudflare-identity.test.ts b/tests/auth/cloudflare-identity.test.ts new file mode 100644 index 0000000..6a48dc6 --- /dev/null +++ b/tests/auth/cloudflare-identity.test.ts @@ -0,0 +1,298 @@ +import { OAuthError as ProviderOAuthError } from '@cloudflare/workers-oauth-provider' +import { http, HttpResponse } from 'msw' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { getCloudflareIdentity } from '../../src/auth/cloudflare-identity' +import { OAuthError } from '../../src/auth/workers-oauth-utils' +import { API_BASE, cfAccountsSuccess, cfSuccess } from '../helpers/cloudflare-api' +import { server } from '../setup/msw' + +/** Register MSW handlers for the identity-probe endpoints by path. */ +function mockProbes(opts: { user?: () => Response; accounts?: () => Response }) { + if (opts.user) server.use(http.get(`${API_BASE}/user`, opts.user)) + if (opts.accounts) server.use(http.get(`${API_BASE}/accounts`, opts.accounts)) +} + +vi.mock('../../src/utils/fetch-retry', async (importOriginal) => { + const original = await importOriginal() + return { + ...original, + fetchWithRetry: (input: RequestInfo, init?: RequestInit) => + original.fetchWithRetry(input, init, { maxRetries: 0 }) + } +}) + +async function expectOAuthError( + promise: Promise, + code: string, + statusCode: number +): Promise { + try { + await promise + throw new Error('Expected OAuthError to be thrown') + } catch (error) { + expect(error).toBeInstanceOf(OAuthError) + expect(error).toBeInstanceOf(ProviderOAuthError) + expect(error).toMatchObject({ code, statusCode }) + return error as OAuthError + } +} + +afterEach(() => vi.restoreAllMocks()) + +describe('getCloudflareIdentity', () => { + it('requests enough accounts for the prompt-list cutoff to be detected', async () => { + let accountsUrl = '' + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })) + }) + server.use( + http.get(`${API_BASE}/accounts`, ({ request }) => { + accountsUrl = request.url + return HttpResponse.json(cfAccountsSuccess([])) + }) + ) + + await getCloudflareIdentity('test-token') + + expect(new URL(accountsUrl).searchParams.get('per_page')).toBe('31') + }) + + it('records the count but omits the records when a user has too many accounts', async () => { + const accounts = Array.from({ length: 31 }, (_, index) => ({ + id: `account-${index + 1}`, + name: `Account ${index + 1}` + })) + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + accounts: () => + HttpResponse.json({ + ...cfSuccess(accounts), + result_info: { page: 1, per_page: 31, count: 31, total_count: 137 } + }) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: { id: 'user-1', email: 'user@example.com' }, + accounts: [], + accountCount: 137 + }) + }) + + it('warns and falls back to page count when total_count is missing', async () => { + const accounts = Array.from({ length: 31 }, (_, index) => ({ + id: `account-${index + 1}`, + name: `Account ${index + 1}` + })) + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + accounts: () => + HttpResponse.json({ + ...cfSuccess(accounts), + result_info: { page: 1, per_page: 31, count: 31 } + }) + }) + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: { id: 'user-1', email: 'user@example.com' }, + accounts: [], + accountCount: 31 + }) + expect(warning).toHaveBeenCalledWith( + expect.stringContaining('missing a valid result_info.total_count') + ) + }) + + it('keeps the full list when a user is under the cutoff', async () => { + const accounts = Array.from({ length: 25 }, (_, index) => ({ + id: `account-${index + 1}`, + name: `Account ${index + 1}` + })) + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + accounts: () => + HttpResponse.json({ + ...cfSuccess(accounts), + result_info: { page: 1, per_page: 31, count: 25, total_count: 25 } + }) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: { id: 'user-1', email: 'user@example.com' }, + accounts + }) + }) + + it('accepts account-scoped token when /user fails but /accounts succeeds', async () => { + mockProbes({ + user: () => new HttpResponse('Forbidden', { status: 403 }), + accounts: () => + HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: null, + accounts: [{ id: 'acc-1', name: 'Primary Account' }] + }) + }) + + it('accepts user tokens when /accounts fails but /user succeeds', async () => { + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + accounts: () => new HttpResponse('Forbidden', { status: 403 }) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: { id: 'user-1', email: 'user@example.com' }, + accounts: [] + }) + }) + + it('throws insufficient_scope when both endpoints fail with 403', async () => { + mockProbes({ + user: () => new HttpResponse('Forbidden', { status: 403 }), + accounts: () => new HttpResponse('Forbidden', { status: 403 }) + }) + + await expectOAuthError(getCloudflareIdentity('test-token'), 'insufficient_scope', 403) + }) + + it.each([ + { + userStatus: 401, + accountsStatus: 401, + code: 'invalid_token', + statusCode: 401 + }, + { + userStatus: 429, + accountsStatus: 429, + code: 'temporarily_unavailable', + statusCode: 429 + }, + { + userStatus: 500, + accountsStatus: 500, + code: 'server_error', + statusCode: 502 + }, + { + userStatus: 418, + accountsStatus: 418, + code: 'invalid_token', + statusCode: 418 + }, + { + userStatus: 403, + accountsStatus: 500, + code: 'server_error', + statusCode: 502 + } + ])( + 'maps dual endpoint failures to OAuthError for /user=$userStatus /accounts=$accountsStatus', + async ({ userStatus, accountsStatus, code, statusCode }) => { + mockProbes({ + user: () => new HttpResponse('upstream error', { status: userStatus }), + accounts: () => new HttpResponse('upstream error', { status: accountsStatus }) + }) + + await expectOAuthError(getCloudflareIdentity('test-token'), code, statusCode) + } + ) + + it('preserves Retry-After from Cloudflare API 429 responses', async () => { + mockProbes({ + user: () => + new HttpResponse('rate limited', { status: 429, headers: { 'Retry-After': '17' } }), + accounts: () => new HttpResponse('rate limited', { status: 429 }) + }) + + const error = await expectOAuthError( + getCloudflareIdentity('test-token'), + 'temporarily_unavailable', + 429 + ) + expect(error.headers).toEqual({ 'Retry-After': '17' }) + }) + + it('defaults Retry-After when Cloudflare API 429 responses omit it', async () => { + mockProbes({ + user: () => new HttpResponse('rate limited', { status: 429 }), + accounts: () => new HttpResponse('rate limited', { status: 429 }) + }) + + const error = await expectOAuthError( + getCloudflareIdentity('test-token'), + 'temporarily_unavailable', + 429 + ) + expect(error.headers).toEqual({ 'Retry-After': '30' }) + }) + + it('falls back to account-scoped auth when /user is 200 but invalid JSON', async () => { + mockProbes({ + user: () => new HttpResponse('not-json', { status: 200 }), + accounts: () => + HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: null, + accounts: [{ id: 'acc-1', name: 'Primary Account' }] + }) + }) + + it('falls back to account-scoped auth when /user is 200 with success=false', async () => { + mockProbes({ + user: () => HttpResponse.json({ success: false }), + accounts: () => + HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: null, + accounts: [{ id: 'acc-1', name: 'Primary Account' }] + }) + }) + + it('keeps user auth when /accounts is 200 but invalid JSON', async () => { + mockProbes({ + user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + accounts: () => new HttpResponse('not-json', { status: 200 }) + }) + + await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ + user: { id: 'user-1', email: 'user@example.com' }, + accounts: [] + }) + }) + + it('rejects when /accounts returns empty result and /user fails', async () => { + mockProbes({ + user: () => new HttpResponse('Forbidden', { status: 403 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([])) + }) + + await expectOAuthError(getCloudflareIdentity('test-token'), 'invalid_token', 401) + }) + + it('rejects when /accounts payload shape is invalid and /user fails', async () => { + mockProbes({ + user: () => new HttpResponse('Forbidden', { status: 403 }), + accounts: () => HttpResponse.json(cfSuccess([{ id: 'acc-1' }])) + }) + + await expectOAuthError(getCloudflareIdentity('test-token'), 'invalid_token', 401) + }) + + it('maps a network failure to server_error', async () => { + // A transport-level failure (fetch rejecting) is BELOW MSW's HTTP + // abstraction — HttpResponse.error() leaks an unhandled rejection through + // @mswjs/interceptors. The fetch primitive is the correct seam for a + // network error, so spy it here. (vi.spyOn restored in afterEach.) + vi.spyOn(globalThis, 'fetch').mockRejectedValue(new TypeError('network failed')) + + await expectOAuthError(getCloudflareIdentity('test-token'), 'server_error', 502) + }) +}) diff --git a/tests/auth/oauth-handler.test.ts b/tests/auth/oauth-handler.test.ts index 95aa4c4..f83b942 100644 --- a/tests/auth/oauth-handler.test.ts +++ b/tests/auth/oauth-handler.test.ts @@ -8,32 +8,12 @@ import { http, HttpResponse } from 'msw' import { afterEach, describe, expect, it, vi } from 'vitest' import { - getUserAndAccounts, guardRefreshTokenExchange, handleTokenExchangeCallback } from '../../src/auth/oauth-handler' import { OAuthError } from '../../src/auth/workers-oauth-utils' -import { API_BASE, cfAccountsSuccess, cfSuccess } from '../helpers/cloudflare-api' import { server } from '../setup/msw' -/** Register MSW handlers for the two identity-probe endpoints by path. */ -function mockProbes(opts: { user?: () => Response; accounts?: () => Response }) { - if (opts.user) server.use(http.get(`${API_BASE}/user`, opts.user)) - if (opts.accounts) server.use(http.get(`${API_BASE}/accounts`, opts.accounts)) -} - -// Use minimal retry config so tests don't wait for real backoff delays. This is -// the only mock of our own modules left here: it just tightens retry timing for -// the identity-probe tests; the actual fetch boundary is mocked with MSW. -vi.mock('../../src/utils/fetch-retry', async (importOriginal) => { - const original = await importOriginal() - return { - ...original, - fetchWithRetry: (input: RequestInfo, init?: RequestInit) => - original.fetchWithRetry(input, init, { maxRetries: 0 }) - } -}) - function deferred(): { promise: Promise resolve: (value: T) => void @@ -101,263 +81,6 @@ afterEach(async () => { await Promise.all(keys.map((k) => kv.delete(k.name))) }) -describe('getUserAndAccounts', () => { - it('requests enough accounts for the prompt-list cutoff to be detected', async () => { - let accountsUrl = '' - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })) - }) - server.use( - http.get(`${API_BASE}/accounts`, ({ request }) => { - accountsUrl = request.url - return HttpResponse.json(cfAccountsSuccess([])) - }) - ) - - await getUserAndAccounts('test-token') - - expect(new URL(accountsUrl).searchParams.get('per_page')).toBe('31') - }) - - it('records the count but omits the records when a user has too many accounts', async () => { - const accounts = Array.from({ length: 31 }, (_, index) => ({ - id: `account-${index + 1}`, - name: `Account ${index + 1}` - })) - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => - HttpResponse.json({ - ...cfSuccess(accounts), - result_info: { page: 1, per_page: 31, count: 31, total_count: 137 } - }) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts: [], - accountCount: 137 - }) - }) - - it('warns and falls back to page count when total_count is missing', async () => { - const accounts = Array.from({ length: 31 }, (_, index) => ({ - id: `account-${index + 1}`, - name: `Account ${index + 1}` - })) - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => - HttpResponse.json({ - ...cfSuccess(accounts), - result_info: { page: 1, per_page: 31, count: 31 } - }) - }) - const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts: [], - accountCount: 31 - }) - expect(warning).toHaveBeenCalledWith( - expect.stringContaining('missing a valid result_info.total_count') - ) - }) - - it('keeps the full list when a user is under the cutoff', async () => { - const accounts = Array.from({ length: 25 }, (_, index) => ({ - id: `account-${index + 1}`, - name: `Account ${index + 1}` - })) - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => - HttpResponse.json({ - ...cfSuccess(accounts), - result_info: { page: 1, per_page: 31, count: 25, total_count: 25 } - }) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts - }) - }) - - it('accepts account-scoped token when /user fails but /accounts succeeds', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('accepts user tokens when /accounts fails but /user succeeds', async () => { - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => new HttpResponse('Forbidden', { status: 403 }) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts: [] - }) - }) - - it('throws insufficient_scope when both endpoints fail with 403', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => new HttpResponse('Forbidden', { status: 403 }) - }) - - await expectOAuthError(getUserAndAccounts('test-token'), 'insufficient_scope', 403) - }) - - it.each([ - { - userStatus: 401, - accountsStatus: 401, - code: 'invalid_token', - statusCode: 401 - }, - { - userStatus: 429, - accountsStatus: 429, - code: 'temporarily_unavailable', - statusCode: 429 - }, - { - userStatus: 500, - accountsStatus: 500, - code: 'server_error', - statusCode: 502 - }, - { - userStatus: 418, - accountsStatus: 418, - code: 'invalid_token', - statusCode: 418 - }, - { - userStatus: 403, - accountsStatus: 500, - code: 'server_error', - statusCode: 502 - } - ])( - 'maps dual endpoint failures to OAuthError for /user=$userStatus /accounts=$accountsStatus', - async ({ userStatus, accountsStatus, code, statusCode }) => { - mockProbes({ - user: () => new HttpResponse('upstream error', { status: userStatus }), - accounts: () => new HttpResponse('upstream error', { status: accountsStatus }) - }) - - await expectOAuthError(getUserAndAccounts('test-token'), code, statusCode) - } - ) - - it('preserves Retry-After from Cloudflare API 429 responses', async () => { - mockProbes({ - user: () => - new HttpResponse('rate limited', { status: 429, headers: { 'Retry-After': '17' } }), - accounts: () => new HttpResponse('rate limited', { status: 429 }) - }) - - const error = await expectOAuthError( - getUserAndAccounts('test-token'), - 'temporarily_unavailable', - 429 - ) - expect(error.headers).toEqual({ 'Retry-After': '17' }) - }) - - it('defaults Retry-After when Cloudflare API 429 responses omit it', async () => { - mockProbes({ - user: () => new HttpResponse('rate limited', { status: 429 }), - accounts: () => new HttpResponse('rate limited', { status: 429 }) - }) - - const error = await expectOAuthError( - getUserAndAccounts('test-token'), - 'temporarily_unavailable', - 429 - ) - expect(error.headers).toEqual({ 'Retry-After': '30' }) - }) - - it('falls back to account-scoped auth when /user is 200 but invalid JSON', async () => { - mockProbes({ - user: () => new HttpResponse('not-json', { status: 200 }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('falls back to account-scoped auth when /user is 200 with success=false', async () => { - mockProbes({ - user: () => HttpResponse.json({ success: false }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('keeps user auth when /accounts is 200 but invalid JSON', async () => { - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => new HttpResponse('not-json', { status: 200 }) - }) - - await expect(getUserAndAccounts('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts: [] - }) - }) - - it('rejects when /accounts returns empty result and /user fails', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => HttpResponse.json(cfAccountsSuccess([])) - }) - - await expectOAuthError(getUserAndAccounts('test-token'), 'invalid_token', 401) - }) - - it('rejects when /accounts payload shape is invalid and /user fails', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => HttpResponse.json(cfSuccess([{ id: 'acc-1' }])) - }) - - await expectOAuthError(getUserAndAccounts('test-token'), 'invalid_token', 401) - }) - - it('maps a network failure to server_error', async () => { - // A transport-level failure (fetch rejecting) is BELOW MSW's HTTP - // abstraction — HttpResponse.error() leaks an unhandled rejection through - // @mswjs/interceptors. The fetch primitive is the correct seam for a - // network error, so spy it here. (vi.spyOn restored in afterEach.) - vi.spyOn(globalThis, 'fetch').mockRejectedValue(new TypeError('network failed')) - - await expectOAuthError(getUserAndAccounts('test-token'), 'server_error', 502) - }) -}) - describe('guardRefreshTokenExchange', () => { it('singleflights concurrent refreshes for the same upstream token in one isolate', async () => { const kv = env.OAUTH_KV From ee4146e2c248894a00c1abfe1bde941004af4897 Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Wed, 29 Jul 2026 21:18:08 +0100 Subject: [PATCH 4/6] refactor(auth): model identity probes by credential owner --- src/auth/api-token-mode.ts | 146 ++++------- src/auth/cloudflare-identity.ts | 331 ++++++++++++++--------- src/auth/oauth-handler.ts | 26 +- tests/auth/api-token-mode.test.ts | 21 +- tests/auth/cloudflare-identity.test.ts | 349 ++++++++++++++----------- tests/auth/oauth-routes.test.ts | 30 ++- 6 files changed, 491 insertions(+), 412 deletions(-) diff --git a/src/auth/api-token-mode.ts b/src/auth/api-token-mode.ts index 13af489..d3f04f2 100644 --- a/src/auth/api-token-mode.ts +++ b/src/auth/api-token-mode.ts @@ -1,34 +1,21 @@ import { ExternalTokenError, - type OAuthTokenErrorCode, type ResolveExternalTokenInput, type ResolveExternalTokenResult } from '@cloudflare/workers-oauth-provider' -import { z } from 'zod' - -import { getCloudflareIdentity, type CloudflareTokenOwner } from './cloudflare-identity' -import { OAuthError } from './workers-oauth-utils' import { - AccountsSchema, - AUTH_PROPS_VERSION, - UserSchema, - type AccountSchema, - type AuthProps, - type UserSchema as UserIdentity -} from './types' + CloudflareIdentitySchema, + resolveCloudflareCredential, + type CloudflareIdentity, + type CloudflareTokenOwner +} from './cloudflare-identity' +import { AUTH_PROPS_VERSION, type AuthProps } from './types' +import { OAuthError } from './workers-oauth-utils' const API_TOKEN_IDENTITY_CACHE_TTL_SECONDS = 2_592_000 -const ApiTokenIdentitySchema = z.object({ - user: UserSchema.nullable(), - accounts: AccountsSchema, - accountCount: z.number().int().nonnegative().optional() -}) - -type ApiTokenIdentity = z.infer - -/** Prefixes are trusted only as owner hints; unprefixed legacy credentials remain supported. */ +/** Prefixes are ownership hints; unprefixed legacy credentials remain supported. */ export function cloudflareTokenOwner(token: string): CloudflareTokenOwner { if (token.startsWith('cfat_')) return 'account' if (token.startsWith('cfut_') || token.startsWith('cfoat_')) return 'user' @@ -40,18 +27,17 @@ async function hashApiToken(token: string): Promise { return [...new Uint8Array(digest)].map((byte) => byte.toString(16).padStart(2, '0')).join('') } -async function getCachedApiTokenIdentity( +async function getCachedIdentity( token: string, tokenOwner: CloudflareTokenOwner, kv: KVNamespace -): Promise { - const tokenHash = await hashApiToken(token) - const cacheKey = `api-token-identity:v3:${tokenHash}` +): Promise { + const cacheKey = `api-token-identity:v4:${await hashApiToken(token)}` try { const cachedValue = await kv.get(cacheKey, 'json') if (cachedValue !== null) { - const cached = ApiTokenIdentitySchema.safeParse(cachedValue) + const cached = CloudflareIdentitySchema.safeParse(cachedValue) if (cached.success) return cached.data console.warn('api_token_identity_probe ignored invalid cache entry') } @@ -59,7 +45,7 @@ async function getCachedApiTokenIdentity( console.warn('api_token_identity_probe kv-cache read failed', error) } - const identity = await getCloudflareIdentity(token, 'api_token_identity_probe', tokenOwner) + const identity = await resolveCloudflareCredential(token, tokenOwner) try { await kv.put(cacheKey, JSON.stringify(identity), { @@ -75,92 +61,68 @@ async function getCachedApiTokenIdentity( function externalTokenError( error: OAuthError, tokenOwner: CloudflareTokenOwner -): { - code: OAuthTokenErrorCode - statusCode: number - requiredScopes?: string[] -} { +): ExternalTokenError { + const common = { description: error.description, headers: error.headers } + switch (error.code) { case 'invalid_token': - return { code: 'invalid_token', statusCode: 401 } + return new ExternalTokenError('invalid_token', { ...common, statusCode: 401 }) case 'insufficient_scope': - return { - code: 'insufficient_scope', + return new ExternalTokenError('insufficient_scope', { + ...common, statusCode: 403, requiredScopes: tokenOwner === 'account' ? ['account:read'] : ['user:read', 'account:read'] - } + }) case 'temporarily_unavailable': - return { code: 'temporarily_unavailable', statusCode: error.statusCode } + return new ExternalTokenError('temporarily_unavailable', { + ...common, + statusCode: error.statusCode + }) case 'server_error': - return { code: 'server_error', statusCode: error.statusCode } + return new ExternalTokenError('server_error', { + ...common, + statusCode: error.statusCode + }) default: - return error.statusCode >= 500 - ? { code: 'server_error', statusCode: 502 } - : { code: 'invalid_token', statusCode: 401 } + return new ExternalTokenError(error.statusCode >= 500 ? 'server_error' : 'invalid_token', { + ...common, + statusCode: error.statusCode >= 500 ? 502 : 401 + }) } } -/** Build the request props consumed by the MCP tool layer from a verified Cloudflare credential. */ -export function buildAuthProps( - token: string, - user: UserIdentity | null, - accounts: AccountSchema[], - accountCount?: number -): AuthProps { - if (user) { - return { - type: 'user_token', - accessToken: token, - user, - accounts, - accountCount, - version: AUTH_PROPS_VERSION - } - } - - if (accounts.length !== 1) { - throw new OAuthError( - 'invalid_token', - 'Account token must resolve to exactly one Cloudflare account', - 401 - ) - } - - return { - type: 'account_token', - accessToken: token, - account: accounts[0] +/** Convert a verified Cloudflare identity into request-local tool props. */ +export function buildAuthProps(token: string, identity: CloudflareIdentity): AuthProps { + switch (identity.type) { + case 'account': + return { + type: 'account_token', + accessToken: token, + account: identity.account + } + case 'user': + return { + type: 'user_token', + accessToken: token, + user: identity.user, + accounts: identity.accounts, + accountCount: identity.accountCount, + version: AUTH_PROPS_VERSION + } } } -/** - * Resolve direct Cloudflare API and OAuth credentials after the provider's own - * access-token lookup misses. workers-oauth-provider owns bearer parsing, - * standards-compliant error responses, and injection of the returned props. - */ +/** Resolve direct Cloudflare credentials after the provider's internal token lookup misses. */ export async function resolveExternalToken({ token, env }: ResolveExternalTokenInput): Promise { const tokenOwner = cloudflareTokenOwner(token) try { - const { user, accounts, accountCount } = await getCachedApiTokenIdentity( - token, - tokenOwner, - env.OAUTH_KV - ) - - return { props: buildAuthProps(token, user, accounts, accountCount) } + const identity = await getCachedIdentity(token, tokenOwner, env.OAUTH_KV) + return { props: buildAuthProps(token, identity) } } catch (error) { - if (error instanceof OAuthError) { - const { code, statusCode, requiredScopes } = externalTokenError(error, tokenOwner) - throw new ExternalTokenError(code, { - description: error.description, - statusCode, - headers: error.headers, - requiredScopes - }) - } + if (error instanceof OAuthError) throw externalTokenError(error, tokenOwner) throw error } } diff --git a/src/auth/cloudflare-identity.ts b/src/auth/cloudflare-identity.ts index 66c49f1..9a1d8ac 100644 --- a/src/auth/cloudflare-identity.ts +++ b/src/auth/cloudflare-identity.ts @@ -1,20 +1,66 @@ import { env as cloudflareEnv } from 'cloudflare:workers' +import { z } from 'zod' import { fetchWithRetry } from '../utils/fetch-retry' import { + AccountSchema, AccountsSchema, ACCOUNTS_PROBE_PAGE_SIZE, MAX_STORED_ACCOUNTS, - UserSchema, - type AccountSchema, - type UserSchema as UserIdentity + UserSchema } from './types' import { OAuthError } from './workers-oauth-utils' const env = cloudflareEnv as Env +const API_TOKEN_IDENTITY_CALLER = 'api_token_identity_probe' +const OAUTH_IDENTITY_CALLER = 'oauth_callback_identity_probe' +// /accounts enforces a minimum page size of five. That is still enough to +// prove the account-token invariant without fetching the 31-record user page. +const ACCOUNT_TOKEN_PROBE_PAGE_SIZE = 5 export type CloudflareTokenOwner = 'account' | 'unknown' | 'user' +const AccountIdentitySchema = z.object({ + type: z.literal('account'), + account: AccountSchema +}) + +const UserIdentitySchema = z.object({ + type: z.literal('user'), + user: UserSchema, + accounts: AccountsSchema, + accountCount: z.number().int().nonnegative().optional() +}) + +export const CloudflareIdentitySchema = z.discriminatedUnion('type', [ + AccountIdentitySchema, + UserIdentitySchema +]) + +export type CloudflareIdentity = z.infer +export type CloudflareUserIdentity = z.infer + +type AccountPage = { + accounts: z.infer + totalCount?: number +} + +const UserResponseSchema = z.object({ + success: z.boolean().optional(), + result: UserSchema.nullable().optional() +}) + +const AccountsResponseSchema = z.object({ + success: z.boolean().optional(), + result: AccountsSchema.optional(), + result_info: z + .object({ + count: z.number().optional(), + total_count: z.number().optional() + }) + .optional() +}) + function retryAfterHeaders(...responses: Response[]): Record { return { 'Retry-After': @@ -57,150 +103,185 @@ function throwIdentityProbeError(...responses: [Response, ...Response[]]): never throw new OAuthError('invalid_token', 'Access token is invalid or expired', statuses[0]) } -async function fetchIdentityProbes( - accessToken: string, - caller: string, - tokenOwner: CloudflareTokenOwner -): Promise<{ user?: Response; accounts: Response }> { - const headers = { Authorization: `Bearer ${accessToken}` } - +async function fetchProbe(url: string, accessToken: string, caller: string): Promise { try { - // The cfat_ prefix proves this is account-owned, so /user cannot add - // identity information and would only spend another upstream request. - const userRequest = - tokenOwner === 'account' - ? Promise.resolve(undefined) - : fetchWithRetry(`${env.CLOUDFLARE_API_BASE}/user`, { headers }, { caller }) - const [user, accounts] = await Promise.all([ - userRequest, - fetchWithRetry( - `${env.CLOUDFLARE_API_BASE}/accounts?per_page=${ACCOUNTS_PROBE_PAGE_SIZE}`, - { headers }, - { caller } - ) - ]) - return { user, accounts } + return await fetchWithRetry( + url, + { headers: { Authorization: `Bearer ${accessToken}` } }, + { caller } + ) } catch (error) { console.error('Cloudflare API request failed', error) throw new OAuthError('server_error', 'Cloudflare API is temporarily unavailable', 502) } } -/** Resolve one Cloudflare credential into the identity stored in request props. */ -export async function getCloudflareIdentity( +function fetchUser(accessToken: string, caller: string): Promise { + return fetchProbe(`${env.CLOUDFLARE_API_BASE}/user`, accessToken, caller) +} + +function fetchAccounts( accessToken: string, - caller = 'oauth_callback_identity_probe', - tokenOwner: CloudflareTokenOwner = 'unknown' -): Promise<{ - user: UserIdentity | null - accounts: AccountSchema[] - accountCount?: number -}> { - const { user: userResponse, accounts: accountsResponse } = await fetchIdentityProbes( - accessToken, - caller, - tokenOwner - ) - - if (!accountsResponse.ok && (!userResponse || !userResponse.ok)) { - console.warn( - `Cloudflare API identity probe failed: user=${userResponse?.status ?? 'skipped'}, accounts=${accountsResponse.status}` - ) - if (userResponse) throwIdentityProbeError(userResponse, accountsResponse) - throwIdentityProbeError(accountsResponse) - } - if (!accountsResponse.ok && userResponse?.ok && tokenOwner === 'user') { - throwIdentityProbeError(accountsResponse) + caller: string, + pageSize = ACCOUNTS_PROBE_PAGE_SIZE +): Promise { + return fetchProbe(`${env.CLOUDFLARE_API_BASE}/accounts?per_page=${pageSize}`, accessToken, caller) +} + +async function parseUser(response: Response): Promise { + try { + const parsed = UserResponseSchema.safeParse(await response.json()) + if (parsed.success && parsed.data.success && parsed.data.result) return parsed.data.result + if (!parsed.success) { + console.error('Cloudflare API /user payload did not match expected shape', parsed.error) + } + } catch (error) { + console.error('Cloudflare API /user response is not valid JSON', error) } + return null +} - let accounts: AccountSchema[] = [] - let totalAccountCount: number | undefined - if (accountsResponse.ok) { - try { - const json = (await accountsResponse.json()) as { - success?: boolean - result?: unknown - result_info?: { count?: unknown; total_count?: unknown } - } - if (json.success && json.result) { - const parsed = AccountsSchema.safeParse(json.result) - if (parsed.success) { - accounts = parsed.data - const reported = json.result_info?.total_count - if (typeof reported === 'number' && Number.isFinite(reported) && reported >= 0) { - totalAccountCount = reported - } else { - const pageCount = json.result_info?.count - totalAccountCount = - typeof pageCount === 'number' && Number.isFinite(pageCount) && pageCount >= 0 - ? Math.max(pageCount, accounts.length) - : accounts.length - console.warn( - `Cloudflare API /accounts response is missing a valid result_info.total_count; ` + - `falling back to page count ${totalAccountCount}` - ) - } - } else { - console.error( - 'Cloudflare API /accounts payload did not match expected shape', - parsed.error - ) - } - } - } catch (error) { - console.error('Cloudflare API /accounts response is not valid JSON', error) +async function parseAccounts(response: Response): Promise { + try { + const parsed = AccountsResponseSchema.safeParse(await response.json()) + if (!parsed.success) { + console.error('Cloudflare API /accounts payload did not match expected shape', parsed.error) + return { accounts: [] } } + if (!parsed.data.success || !parsed.data.result) return { accounts: [] } + + const accounts = parsed.data.result + const reported = parsed.data.result_info?.total_count + if (reported !== undefined && Number.isFinite(reported) && reported >= 0) { + return { accounts, totalCount: reported } + } + + const pageCount = parsed.data.result_info?.count + const totalCount = + pageCount !== undefined && Number.isFinite(pageCount) && pageCount >= 0 + ? Math.max(pageCount, accounts.length) + : accounts.length + console.warn( + `Cloudflare API /accounts response is missing a valid result_info.total_count; ` + + `falling back to page count ${totalCount}` + ) + return { accounts, totalCount } + } catch (error) { + console.error('Cloudflare API /accounts response is not valid JSON', error) + return { accounts: [] } } +} - // A prefixed account token must resolve to its one owning account. It never - // uses response-based inference and never calls /user. - if (!userResponse) { - if (accounts.length === 1) return { user: null, accounts } +function accountIdentity(page: AccountPage): CloudflareIdentity { + if (page.accounts.length !== 1 || (page.totalCount !== undefined && page.totalCount !== 1)) { throw new OAuthError( 'invalid_token', 'Account token must resolve to exactly one Cloudflare account', 401 ) } + return { type: 'account', account: page.accounts[0] } +} - let user: UserIdentity | null = null - if (userResponse.ok) { - try { - const json = (await userResponse.json()) as { success?: boolean; result?: unknown } - if (json.success && json.result) { - const parsed = UserSchema.safeParse(json.result) - if (parsed.success) { - user = parsed.data - } else { - console.error('Cloudflare API /user payload did not match expected shape', parsed.error) - } - } - } catch (error) { - console.error('Cloudflare API /user response is not valid JSON', error) - } - } else if (tokenOwner === 'user' || userResponse.status >= 429) { - // Known user credentials can never become account credentials. Transient - // failures also cannot be used as evidence of account ownership. - throwIdentityProbeError(userResponse) - } else if (accounts.length > 0) { - // Only unprefixed legacy credentials use response-based owner inference. - return { user: null, accounts } - } - - if (user) { - if (totalAccountCount !== undefined && totalAccountCount > MAX_STORED_ACCOUNTS) { - return { user, accounts: [], accountCount: totalAccountCount } - } - return { user, accounts } +function userIdentity( + user: CloudflareUserIdentity['user'], + page: AccountPage +): CloudflareUserIdentity { + if (page.totalCount !== undefined && page.totalCount > MAX_STORED_ACCOUNTS) { + return { type: 'user', user, accounts: [], accountCount: page.totalCount } + } + return { type: 'user', user, accounts: page.accounts } +} + +async function resolveAccountCredential( + accessToken: string, + caller: string +): Promise { + // One minimum-size page is enough to prove whether an account-owned token + // resolves to exactly one account without fetching the larger user page. + const accountsResponse = await fetchAccounts(accessToken, caller, ACCOUNT_TOKEN_PROBE_PAGE_SIZE) + if (!accountsResponse.ok) throwIdentityProbeError(accountsResponse) + return accountIdentity(await parseAccounts(accountsResponse)) +} + +async function resolveUserCredential( + accessToken: string, + caller: string +): Promise { + const [userResponse, accountsResponse] = await Promise.all([ + fetchUser(accessToken, caller), + fetchAccounts(accessToken, caller) + ]) + const failures = [userResponse, accountsResponse].filter((response) => !response.ok) + if (failures.length > 0) { + throwIdentityProbeError(failures[0], ...failures.slice(1)) + } + + const [user, accounts] = await Promise.all([ + parseUser(userResponse), + parseAccounts(accountsResponse) + ]) + if (!user) { + throw new OAuthError('invalid_token', 'Failed to verify token: no user information', 401) } + return userIdentity(user, accounts) +} - if (tokenOwner === 'unknown' && accounts.length > 0) { - return { user: null, accounts } +async function resolveLegacyCredential( + accessToken: string, + caller: string +): Promise { + const [userResponse, accountsResponse] = await Promise.all([ + fetchUser(accessToken, caller), + fetchAccounts(accessToken, caller) + ]) + if (!accountsResponse.ok) { + if (!userResponse.ok) throwIdentityProbeError(userResponse, accountsResponse) + throwIdentityProbeError(accountsResponse) } - throw new OAuthError( - 'invalid_token', - 'Failed to verify token: no user or account information', - 401 - ) + const [user, accounts] = await Promise.all([ + userResponse.ok ? parseUser(userResponse) : Promise.resolve(null), + accountsResponse.ok ? parseAccounts(accountsResponse) : Promise.resolve({ accounts: [] }) + ]) + if (user) return userIdentity(user, accounts) + + // A transient /user failure cannot prove account ownership. + if (!userResponse.ok && userResponse.status >= 429) throwIdentityProbeError(userResponse) + return accountIdentity(accounts) +} + +/** Resolve a direct Cloudflare API or OAuth credential using its ownership hint. */ +export function resolveCloudflareCredential( + accessToken: string, + tokenOwner: CloudflareTokenOwner +): Promise { + switch (tokenOwner) { + case 'account': + return resolveAccountCredential(accessToken, API_TOKEN_IDENTITY_CALLER) + case 'user': + return resolveUserCredential(accessToken, API_TOKEN_IDENTITY_CALLER) + case 'unknown': + return resolveLegacyCredential(accessToken, API_TOKEN_IDENTITY_CALLER) + } +} + +/** Resolve the upstream Cloudflare OAuth grant used by this server's OAuth flow. */ +export async function getCloudflareOAuthUser(accessToken: string): Promise { + const [userResponse, accountsResponse] = await Promise.all([ + fetchUser(accessToken, OAUTH_IDENTITY_CALLER), + fetchAccounts(accessToken, OAUTH_IDENTITY_CALLER) + ]) + if (!userResponse.ok && !accountsResponse.ok) { + throwIdentityProbeError(userResponse, accountsResponse) + } + + const [user, accounts] = await Promise.all([ + userResponse.ok ? parseUser(userResponse) : Promise.resolve(null), + accountsResponse.ok ? parseAccounts(accountsResponse) : Promise.resolve({ accounts: [] }) + ]) + if (!user) { + throw new OAuthError('server_error', 'Failed to fetch user information from Cloudflare', 500) + } + return userIdentity(user, accounts) } diff --git a/src/auth/oauth-handler.ts b/src/auth/oauth-handler.ts index b76c284..1f84e66 100644 --- a/src/auth/oauth-handler.ts +++ b/src/auth/oauth-handler.ts @@ -20,7 +20,7 @@ import { validateOAuthState, OAuthError } from './workers-oauth-utils' -import { getCloudflareIdentity } from './cloudflare-identity' +import { getCloudflareOAuthUser } from './cloudflare-identity' import { MetricsTracker, AuthUser } from '../metrics' import { SERVER_INFO } from '../constants' @@ -544,36 +544,26 @@ export function createAuthHandlers() { }) ]) - // Fetch user and accounts - const { user, accounts, accountCount } = await getCloudflareIdentity(access_token) - - // Account-scoped tokens (user: null) are only supported via API token mode - // (see api-token-mode.ts). The OAuth flow always requires a user identity. - if (!user) { - return new OAuthError( - 'server_error', - 'Failed to fetch user information from Cloudflare' - ).toHtmlResponse() - } + const identity = await getCloudflareOAuthUser(access_token) // Complete authorization const { redirectTo } = await env.OAUTH_PROVIDER.completeAuthorization({ request: oauthReqInfo, - userId: user.id, - metadata: { label: user.email }, + userId: identity.user.id, + metadata: { label: identity.user.email }, scope: oauthReqInfo.scope, props: { type: 'user_token', - user, - accounts, - accountCount, + user: identity.user, + accounts: identity.accounts, + accountCount: identity.accountCount, version: AUTH_PROPS_VERSION, accessToken: access_token, refreshToken: refresh_token } satisfies AuthProps }) - metrics.logEvent(new AuthUser({ userId: user.id })) + metrics.logEvent(new AuthUser({ userId: identity.user.id })) return new Response(null, { status: 302, diff --git a/tests/auth/api-token-mode.test.ts b/tests/auth/api-token-mode.test.ts index b0ed13d..8d25938 100644 --- a/tests/auth/api-token-mode.test.ts +++ b/tests/auth/api-token-mode.test.ts @@ -93,7 +93,14 @@ describe('Cloudflare token ownership', () => { describe('buildAuthProps', () => { it('builds versioned user props', () => { - expect(buildAuthProps('token', USER, [ACCOUNT], 1)).toEqual({ + expect( + buildAuthProps('token', { + type: 'user', + user: USER, + accounts: [ACCOUNT], + accountCount: 1 + }) + ).toEqual({ type: 'user_token', accessToken: 'token', user: USER, @@ -103,18 +110,12 @@ describe('buildAuthProps', () => { }) }) - it('builds account props only for exactly one account', () => { - expect(buildAuthProps('token', null, [ACCOUNT])).toEqual({ + it('builds account props from an account identity', () => { + expect(buildAuthProps('token', { type: 'account', account: ACCOUNT })).toEqual({ type: 'account_token', accessToken: 'token', account: ACCOUNT }) - expect(() => buildAuthProps('token', null, [])).toThrow( - 'Account token must resolve to exactly one Cloudflare account' - ) - expect(() => - buildAuthProps('token', null, [ACCOUNT, { id: 'account-2', name: 'Two' }]) - ).toThrow('Account token must resolve to exactly one Cloudflare account') }) }) @@ -214,7 +215,7 @@ describe('resolveExternalToken', () => { it('ignores malformed cached identity data and revalidates upstream', async () => { const token = 'cfut_invalid-cache-token' await env.OAUTH_KV.put( - `api-token-identity:v3:${await sha256Hex(token)}`, + `api-token-identity:v4:${await sha256Hex(token)}`, JSON.stringify({ user: 'not-an-object', accounts: [] }) ) const calls = mockIdentity({ diff --git a/tests/auth/cloudflare-identity.test.ts b/tests/auth/cloudflare-identity.test.ts index 6a48dc6..9c53219 100644 --- a/tests/auth/cloudflare-identity.test.ts +++ b/tests/auth/cloudflare-identity.test.ts @@ -2,12 +2,17 @@ import { OAuthError as ProviderOAuthError } from '@cloudflare/workers-oauth-prov import { http, HttpResponse } from 'msw' import { afterEach, describe, expect, it, vi } from 'vitest' -import { getCloudflareIdentity } from '../../src/auth/cloudflare-identity' +import { + getCloudflareOAuthUser, + resolveCloudflareCredential +} from '../../src/auth/cloudflare-identity' import { OAuthError } from '../../src/auth/workers-oauth-utils' import { API_BASE, cfAccountsSuccess, cfSuccess } from '../helpers/cloudflare-api' import { server } from '../setup/msw' -/** Register MSW handlers for the identity-probe endpoints by path. */ +const USER = { id: 'user-1', email: 'user@example.com' } +const ACCOUNT = { id: 'account-1', name: 'Account One' } + function mockProbes(opts: { user?: () => Response; accounts?: () => Response }) { if (opts.user) server.use(http.get(`${API_BASE}/user`, opts.user)) if (opts.accounts) server.use(http.get(`${API_BASE}/accounts`, opts.accounts)) @@ -40,12 +45,130 @@ async function expectOAuthError( afterEach(() => vi.restoreAllMocks()) -describe('getCloudflareIdentity', () => { - it('requests enough accounts for the prompt-list cutoff to be detected', async () => { +describe('resolveCloudflareCredential', () => { + it('resolves an account-owned token with one minimum-size /accounts probe', async () => { + let userCalls = 0 let accountsUrl = '' mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })) + user: () => { + userCalls++ + return HttpResponse.json(cfSuccess(USER)) + } + }) + server.use( + http.get(`${API_BASE}/accounts`, ({ request }) => { + accountsUrl = request.url + return HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) + ) + + await expect(resolveCloudflareCredential('cfat_token', 'account')).resolves.toEqual({ + type: 'account', + account: ACCOUNT + }) + expect(userCalls).toBe(0) + expect(new URL(accountsUrl).searchParams.get('per_page')).toBe('5') + }) + + it('requires both identity probes for a known user credential', async () => { + mockProbes({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) + + await expect(resolveCloudflareCredential('cfut_token', 'user')).resolves.toEqual({ + type: 'user', + user: USER, + accounts: [ACCOUNT] + }) + }) + + it('does not infer account ownership for a known user credential', async () => { + mockProbes({ + user: () => new HttpResponse('Forbidden', { status: 403 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) + + await expectOAuthError( + resolveCloudflareCredential('cfut_token', 'user'), + 'insufficient_scope', + 403 + ) + }) + + it.each([ + ['forbidden user probe', () => new HttpResponse('Forbidden', { status: 403 })], + ['invalid user JSON', () => new HttpResponse('not-json', { status: 200 })], + ['unsuccessful user envelope', () => HttpResponse.json({ success: false })] + ] as const)('retains legacy account inference for an %s', async (_, userResponse) => { + mockProbes({ + user: userResponse, + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) + }) + + await expect(resolveCloudflareCredential('legacy-token', 'unknown')).resolves.toEqual({ + type: 'account', + account: ACCOUNT + }) + }) + + it('requires account discovery even when a legacy user probe succeeds', async () => { + mockProbes({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => new HttpResponse('Forbidden', { status: 403 }) + }) + + await expectOAuthError( + resolveCloudflareCredential('legacy-token', 'unknown'), + 'insufficient_scope', + 403 + ) + }) + + it('never infers account ownership from a transient /user failure', async () => { + mockProbes({ + user: () => new HttpResponse('rate limited', { status: 429 }), + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) + + await expectOAuthError( + resolveCloudflareCredential('legacy-token', 'unknown'), + 'temporarily_unavailable', + 429 + ) + }) + + it('requires an account token to resolve to exactly one account', async () => { + mockProbes({ accounts: () => HttpResponse.json(cfAccountsSuccess([])) }) + + await expectOAuthError( + resolveCloudflareCredential('cfat_token', 'account'), + 'invalid_token', + 401 + ) + }) + + it('rejects a truncated account-token page whose total count exceeds one', async () => { + mockProbes({ + accounts: () => + HttpResponse.json({ + ...cfSuccess([ACCOUNT]), + result_info: { page: 1, per_page: 5, count: 1, total_count: 2 } + }) + }) + + await expectOAuthError( + resolveCloudflareCredential('cfat_token', 'account'), + 'invalid_token', + 401 + ) + }) +}) + +describe('user account metadata', () => { + it('requests one account past the prompt-list cutoff', async () => { + let accountsUrl = '' + mockProbes({ user: () => HttpResponse.json(cfSuccess(USER)) }) server.use( http.get(`${API_BASE}/accounts`, ({ request }) => { accountsUrl = request.url @@ -53,18 +176,18 @@ describe('getCloudflareIdentity', () => { }) ) - await getCloudflareIdentity('test-token') + await resolveCloudflareCredential('cfut_token', 'user') expect(new URL(accountsUrl).searchParams.get('per_page')).toBe('31') }) - it('records the count but omits the records when a user has too many accounts', async () => { + it('stores only the count when the complete account list is too large', async () => { const accounts = Array.from({ length: 31 }, (_, index) => ({ id: `account-${index + 1}`, name: `Account ${index + 1}` })) mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + user: () => HttpResponse.json(cfSuccess(USER)), accounts: () => HttpResponse.json({ ...cfSuccess(accounts), @@ -72,20 +195,34 @@ describe('getCloudflareIdentity', () => { }) }) - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, + await expect(resolveCloudflareCredential('cfut_token', 'user')).resolves.toEqual({ + type: 'user', + user: USER, accounts: [], accountCount: 137 }) }) - it('warns and falls back to page count when total_count is missing', async () => { + it('keeps a verified user when an accounts payload is malformed', async () => { + mockProbes({ + user: () => HttpResponse.json(cfSuccess(USER)), + accounts: () => new HttpResponse('not-json', { status: 200 }) + }) + + await expect(resolveCloudflareCredential('cfut_token', 'user')).resolves.toEqual({ + type: 'user', + user: USER, + accounts: [] + }) + }) + + it('falls back to the page count when total_count is missing', async () => { const accounts = Array.from({ length: 31 }, (_, index) => ({ id: `account-${index + 1}`, name: `Account ${index + 1}` })) mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + user: () => HttpResponse.json(cfSuccess(USER)), accounts: () => HttpResponse.json({ ...cfSuccess(accounts), @@ -94,8 +231,9 @@ describe('getCloudflareIdentity', () => { }) const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, + await expect(resolveCloudflareCredential('cfut_token', 'user')).resolves.toEqual({ + type: 'user', + user: USER, accounts: [], accountCount: 31 }) @@ -103,196 +241,91 @@ describe('getCloudflareIdentity', () => { expect.stringContaining('missing a valid result_info.total_count') ) }) +}) - it('keeps the full list when a user is under the cutoff', async () => { - const accounts = Array.from({ length: 25 }, (_, index) => ({ - id: `account-${index + 1}`, - name: `Account ${index + 1}` - })) - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => - HttpResponse.json({ - ...cfSuccess(accounts), - result_info: { page: 1, per_page: 31, count: 25, total_count: 25 } - }) - }) - - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts - }) - }) - - it('accepts account-scoped token when /user fails but /accounts succeeds', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('accepts user tokens when /accounts fails but /user succeeds', async () => { +describe('getCloudflareOAuthUser', () => { + it('preserves OAuth availability when /accounts is forbidden', async () => { mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), + user: () => HttpResponse.json(cfSuccess(USER)), accounts: () => new HttpResponse('Forbidden', { status: 403 }) }) - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, + await expect(getCloudflareOAuthUser('oauth-token')).resolves.toEqual({ + type: 'user', + user: USER, accounts: [] }) }) - it('throws insufficient_scope when both endpoints fail with 403', async () => { + it('rejects an OAuth result without a user', async () => { mockProbes({ user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => new HttpResponse('Forbidden', { status: 403 }) + accounts: () => HttpResponse.json(cfAccountsSuccess([ACCOUNT])) }) - await expectOAuthError(getCloudflareIdentity('test-token'), 'insufficient_scope', 403) + await expectOAuthError(getCloudflareOAuthUser('oauth-token'), 'server_error', 500) }) +}) +describe('identity probe errors', () => { it.each([ - { - userStatus: 401, - accountsStatus: 401, - code: 'invalid_token', - statusCode: 401 - }, - { - userStatus: 429, - accountsStatus: 429, - code: 'temporarily_unavailable', - statusCode: 429 - }, - { - userStatus: 500, - accountsStatus: 500, - code: 'server_error', - statusCode: 502 - }, - { - userStatus: 418, - accountsStatus: 418, - code: 'invalid_token', - statusCode: 418 - }, - { - userStatus: 403, - accountsStatus: 500, - code: 'server_error', - statusCode: 502 - } - ])( - 'maps dual endpoint failures to OAuthError for /user=$userStatus /accounts=$accountsStatus', - async ({ userStatus, accountsStatus, code, statusCode }) => { + [401, 401, 'invalid_token', 401], + [429, 429, 'temporarily_unavailable', 429], + [500, 500, 'server_error', 502], + [418, 418, 'invalid_token', 418], + [403, 500, 'server_error', 502] + ] as const)( + 'maps /user=%i and /accounts=%i to %s', + async (userStatus, accountsStatus, code, statusCode) => { mockProbes({ user: () => new HttpResponse('upstream error', { status: userStatus }), accounts: () => new HttpResponse('upstream error', { status: accountsStatus }) }) - await expectOAuthError(getCloudflareIdentity('test-token'), code, statusCode) + await expectOAuthError( + resolveCloudflareCredential('legacy-token', 'unknown'), + code, + statusCode + ) } ) - it('preserves Retry-After from Cloudflare API 429 responses', async () => { + it('uses a default Retry-After when the upstream omits it', async () => { mockProbes({ - user: () => - new HttpResponse('rate limited', { status: 429, headers: { 'Retry-After': '17' } }), + user: () => new HttpResponse('rate limited', { status: 429 }), accounts: () => new HttpResponse('rate limited', { status: 429 }) }) const error = await expectOAuthError( - getCloudflareIdentity('test-token'), + resolveCloudflareCredential('legacy-token', 'unknown'), 'temporarily_unavailable', 429 ) - expect(error.headers).toEqual({ 'Retry-After': '17' }) + expect(error.headers).toEqual({ 'Retry-After': '30' }) }) - it('defaults Retry-After when Cloudflare API 429 responses omit it', async () => { + it('preserves Retry-After from a rate-limited probe', async () => { mockProbes({ - user: () => new HttpResponse('rate limited', { status: 429 }), + user: () => + new HttpResponse('rate limited', { status: 429, headers: { 'Retry-After': '17' } }), accounts: () => new HttpResponse('rate limited', { status: 429 }) }) const error = await expectOAuthError( - getCloudflareIdentity('test-token'), + resolveCloudflareCredential('legacy-token', 'unknown'), 'temporarily_unavailable', 429 ) - expect(error.headers).toEqual({ 'Retry-After': '30' }) - }) - - it('falls back to account-scoped auth when /user is 200 but invalid JSON', async () => { - mockProbes({ - user: () => new HttpResponse('not-json', { status: 200 }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('falls back to account-scoped auth when /user is 200 with success=false', async () => { - mockProbes({ - user: () => HttpResponse.json({ success: false }), - accounts: () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Primary Account' }])) - }) - - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: null, - accounts: [{ id: 'acc-1', name: 'Primary Account' }] - }) - }) - - it('keeps user auth when /accounts is 200 but invalid JSON', async () => { - mockProbes({ - user: () => HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })), - accounts: () => new HttpResponse('not-json', { status: 200 }) - }) - - await expect(getCloudflareIdentity('test-token')).resolves.toEqual({ - user: { id: 'user-1', email: 'user@example.com' }, - accounts: [] - }) - }) - - it('rejects when /accounts returns empty result and /user fails', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => HttpResponse.json(cfAccountsSuccess([])) - }) - - await expectOAuthError(getCloudflareIdentity('test-token'), 'invalid_token', 401) - }) - - it('rejects when /accounts payload shape is invalid and /user fails', async () => { - mockProbes({ - user: () => new HttpResponse('Forbidden', { status: 403 }), - accounts: () => HttpResponse.json(cfSuccess([{ id: 'acc-1' }])) - }) - - await expectOAuthError(getCloudflareIdentity('test-token'), 'invalid_token', 401) + expect(error.headers).toEqual({ 'Retry-After': '17' }) }) it('maps a network failure to server_error', async () => { - // A transport-level failure (fetch rejecting) is BELOW MSW's HTTP - // abstraction — HttpResponse.error() leaks an unhandled rejection through - // @mswjs/interceptors. The fetch primitive is the correct seam for a - // network error, so spy it here. (vi.spyOn restored in afterEach.) vi.spyOn(globalThis, 'fetch').mockRejectedValue(new TypeError('network failed')) - await expectOAuthError(getCloudflareIdentity('test-token'), 'server_error', 502) + await expectOAuthError( + resolveCloudflareCredential('legacy-token', 'unknown'), + 'server_error', + 502 + ) }) }) diff --git a/tests/auth/oauth-routes.test.ts b/tests/auth/oauth-routes.test.ts index 1975c48..47c7f3f 100644 --- a/tests/auth/oauth-routes.test.ts +++ b/tests/auth/oauth-routes.test.ts @@ -99,7 +99,12 @@ async function beginAuthorization(options: { state?: string; scopes?: string } = } } -function useCloudflareAuthSuccess(): void { +function useCloudflareAuthSuccess(): { + userCalls: () => number + accountCalls: () => number +} { + let userCalls = 0 + let accountCalls = 0 server.use( http.post('https://dash.cloudflare.com/oauth2/token', () => HttpResponse.json({ @@ -110,13 +115,16 @@ function useCloudflareAuthSuccess(): void { token_type: 'bearer' }) ), - http.get('https://api.cloudflare.com/client/v4/user', () => - HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })) - ), - http.get('https://api.cloudflare.com/client/v4/accounts', () => - HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Account One' }])) - ) + http.get('https://api.cloudflare.com/client/v4/user', () => { + userCalls++ + return HttpResponse.json(cfSuccess({ id: 'user-1', email: 'user@example.com' })) + }), + http.get('https://api.cloudflare.com/client/v4/accounts', () => { + accountCalls++ + return HttpResponse.json(cfAccountsSuccess([{ id: 'acc-1', name: 'Account One' }])) + }) ) + return { userCalls: () => userCalls, accountCalls: () => accountCalls } } /** Collapse a response's Set-Cookie header(s) into a Cookie request header. */ @@ -294,9 +302,9 @@ describe('GET /oauth/callback', () => { expect(dp.blobs?.[3]).toBeFalsy() }) - it('serves modern MCP from provider-issued authenticated props', async () => { + it('serves modern MCP without externally resolving the provider-issued token', async () => { const { clientId, state, sessionCookie } = await beginAuthorization() - useCloudflareAuthSuccess() + const probes = useCloudflareAuthSuccess() const callback = await exports.default.fetch( new Request(`${MCP_ORIGIN}/oauth/callback?code=authcode&state=${encodeURIComponent(state)}`, { headers: { Cookie: sessionCookie }, @@ -321,11 +329,15 @@ describe('GET /oauth/callback', () => { ) expect(tokenResponse.status).toBe(200) const { access_token } = (await tokenResponse.json()) as { access_token: string } + expect(probes.userCalls()).toBe(1) + expect(probes.accountCalls()).toBe(1) const mcpResponse = await exports.default.fetch(modernMcpRequest(access_token, 'tools/list')) const mcpBody = await parseMcpResult(mcpResponse) expect(mcpResponse.status).toBe(200) + expect(probes.userCalls()).toBe(1) + expect(probes.accountCalls()).toBe(1) expect(mcpBody.result?.resultType).toBe('complete') expect(mcpBody.result?.tools?.map((tool) => tool.name)).toEqual(['docs', 'search', 'execute']) }) From d400621a908bb4ece20e431429be9a3b295b223f Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Wed, 29 Jul 2026 21:20:26 +0100 Subject: [PATCH 5/6] docs: keep auth architecture update focused --- AGENTS.md | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7241418..95acabd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,21 +62,21 @@ Node 22+ required. ## Commands -| Command | What it does | -| ---------------------- | ---------------------------------------------- | -| `npm run dev` | Start local dev server (wrangler dev) | -| `npm run deploy` | Deploy to staging | -| `npm run deploy:prod` | Deploy to production | -| `npm run types` | Generate worker type definitions | -| `npm run typecheck` | TypeScript type checking (no emit) | -| `npm run lint` | Lint with oxlint | -| `npm run format` | Format with oxfmt | -| `npm run format:check` | Check formatting without modifying | -| `npm run test` | Run vitest test suite | -| `npm run test:watch` | Run vitest in watch mode | -| `npm run check` | Run all checks (format, lint, typecheck, test) | -| `npm run seed:staging` | Seed OpenAPI spec to staging R2 | -| `npm run seed:prod` | Seed OpenAPI spec to production R2 | +| Command | What it does | +| ---------------------- | --------------------------------------------- | +| `npm run dev` | Start local dev server (wrangler dev) | +| `npm run deploy` | Deploy to staging | +| `npm run deploy:prod` | Deploy to production | +| `npm run types` | Generate worker type definitions | +| `npm run typecheck` | TypeScript type checking (no emit) | +| `npm run lint` | Lint with oxlint | +| `npm run format` | Format with oxfmt | +| `npm run format:check` | Check formatting without modifying | +| `npm run test` | Run vitest test suite | +| `npm run test:watch` | Run vitest in watch mode | +| `npm run check` | Run all checks (format, lint, typecheck, test)| +| `npm run seed:staging` | Seed OpenAPI spec to staging R2 | +| `npm run seed:prod` | Seed OpenAPI spec to production R2 | ## Code standards @@ -172,7 +172,6 @@ npm run test:watch # Watch mode ``` **Unit/integration coverage areas:** - - Scheduled handler (spec fetching & processing) - Auth token detection and parsing - Auth props building and validation From 0507b03a452d2f512ff161a7cb75de90f3bea407 Mon Sep 17 00:00:00 2001 From: Matt Carey Date: Fri, 31 Jul 2026 00:37:28 +0100 Subject: [PATCH 6/6] test: keep scope catalog authorization PKCE-compliant --- tests/auth/oauth-routes.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/auth/oauth-routes.test.ts b/tests/auth/oauth-routes.test.ts index 47c7f3f..11935a3 100644 --- a/tests/auth/oauth-routes.test.ts +++ b/tests/auth/oauth-routes.test.ts @@ -205,6 +205,8 @@ describe('GET /authorize', () => { response_type: 'code', client_id: clientId, redirect_uri: REDIRECT_URI, + code_challenge: DOWNSTREAM_CODE_CHALLENGE, + code_challenge_method: 'S256', scope: 'user:read' }) )