From 20b42c73c5491d1c2bbf149b13fe26337b0ab86b Mon Sep 17 00:00:00 2001 From: "Moon.eth" Date: Wed, 29 Jul 2026 10:45:17 +0900 Subject: [PATCH] oauth improvements --- src/lib/api/client.ts | 23 ++++- src/lib/api/oauth.test.ts | 206 ++++++++++++++++++++++++++++++++++++++ src/lib/api/oauth.ts | 149 +++++++++++++++++++-------- src/lib/api/types.ts | 5 +- src/test/setup.ts | 43 +++++--- vite.config.ts | 3 + 6 files changed, 368 insertions(+), 61 deletions(-) create mode 100644 src/lib/api/oauth.test.ts diff --git a/src/lib/api/client.ts b/src/lib/api/client.ts index ff29e2d..8d42403 100644 --- a/src/lib/api/client.ts +++ b/src/lib/api/client.ts @@ -48,8 +48,8 @@ export interface RequestOptions { method?: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE' query?: Query body?: unknown - /** Send as multipart instead of JSON (media uploads). */ - form?: FormData + /** Send as browser-encoded form data instead of JSON. */ + form?: FormData | URLSearchParams signal?: AbortSignal /** Override the instance token for this call. */ token?: string | null @@ -216,6 +216,25 @@ export class ApiClient { return data } + /** + * Post an `application/x-www-form-urlencoded` body. + * + * OAuth endpoints are specified as form endpoints. Letting fetch set the + * header for URLSearchParams also keeps this a CORS-safelisted request. + */ + async postForm( + path: string, + fields: Record, + options: RequestOptions = {}, + ): Promise { + const form = new URLSearchParams() + for (const [key, value] of Object.entries(fields)) { + if (value !== undefined) form.set(key, value) + } + const { data } = await this.raw(path, { ...options, method: 'POST', form }) + return data + } + async patch(path: string, body?: unknown, options: RequestOptions = {}): Promise { const { data } = await this.raw(path, { ...options, method: 'PATCH', body }) return data diff --git a/src/lib/api/oauth.test.ts b/src/lib/api/oauth.test.ts new file mode 100644 index 0000000..4c0fe67 --- /dev/null +++ b/src/lib/api/oauth.test.ts @@ -0,0 +1,206 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + authorizationUrl, + beginLogin, + completeLogin, + ensureApp, + pkceChallenge, + redirectUri, + revoke, + SCOPES, +} from './oauth' +import type { OAuthApp } from './types' + +const APP_KEY = 'plspace:oauth:apps' +const PENDING_KEY = 'plspace:oauth:pending' + +function response(body: unknown, status = 200): Response { + return new Response(body === null ? null : JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }) +} + +function app(overrides: Partial = {}): OAuthApp { + return { + id: 'app-1', + name: 'plspace', + client_id: 'client-id', + client_secret: 'client-secret', + redirect_uris: [redirectUri()], + ...overrides, + } +} + +function saveApp(value = app()): void { + localStorage.setItem( + APP_KEY, + JSON.stringify({ + 'social.example': { ...value, plspace_redirect_uri: redirectUri() }, + }), + ) +} + +function savePending(overrides: Record = {}): void { + sessionStorage.setItem( + PENDING_KEY, + JSON.stringify({ + host: 'social.example', + state: 'expected-state', + verifier: 'pkce-verifier', + createdAt: Date.now(), + returnTo: '#/timeline/home', + ...overrides, + }), + ) +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('OAuth request compatibility', () => { + it('produces the RFC 7636 S256 challenge', async () => { + await expect( + pkceChallenge('dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'), + ).resolves.toBe('E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM') + }) + + it('builds a Mastodon-compatible authorization URL with state and PKCE', () => { + const url = new URL( + authorizationUrl('social.example', app(), 'oauth-state', 'pkce-challenge'), + ) + + expect(url.origin).toBe('https://social.example') + expect(url.pathname).toBe('/oauth/authorize') + expect(url.searchParams.get('client_id')).toBe('client-id') + expect(url.searchParams.get('redirect_uri')).toBe(redirectUri()) + expect(url.searchParams.get('response_type')).toBe('code') + expect(url.searchParams.get('scope')).toBe(SCOPES) + expect(url.searchParams.get('state')).toBe('oauth-state') + expect(url.searchParams.get('code_challenge')).toBe('pkce-challenge') + expect(url.searchParams.get('code_challenge_method')).toBe('S256') + }) + + it('registers once and understands the Mastodon 4.3 redirect_uris response', async () => { + const fetchMock = vi.fn().mockResolvedValue(response(app())) + vi.stubGlobal('fetch', fetchMock) + + await ensureApp('social.example') + await ensureApp('social.example') + + expect(fetchMock).toHaveBeenCalledOnce() + const [url, options] = fetchMock.mock.calls[0] as [string, RequestInit] + expect(url).toBe('https://social.example/api/v1/apps') + expect(JSON.parse(String(options.body))).toMatchObject({ + client_name: 'plspace', + redirect_uris: redirectUri(), + scopes: SCOPES, + }) + }) + + it('persists one state-bound PKCE attempt before navigating away', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response(app()))) + const navigate = vi.fn() + + await beginLogin('social.example', '#/mail', navigate) + + expect(navigate).toHaveBeenCalledOnce() + const url = new URL(navigate.mock.calls[0][0]) + const pending = JSON.parse(String(sessionStorage.getItem(PENDING_KEY))) as Record< + string, + unknown + > + expect(pending).toMatchObject({ + host: 'social.example', + returnTo: '#/mail', + state: url.searchParams.get('state'), + }) + expect(typeof pending.verifier).toBe('string') + expect(typeof pending.createdAt).toBe('number') + expect(url.searchParams.get('code_challenge')).toBeTruthy() + expect(url.searchParams.get('code_challenge_method')).toBe('S256') + }) + + it('exchanges a code as URL-encoded form data and accepts Pleroma extras', async () => { + saveApp() + savePending() + window.history.replaceState( + {}, + '', + '/?code=authorization-code&state=expected-state&iss=https%3A%2F%2Fsocial.example', + ) + const fetchMock = vi.fn().mockResolvedValue( + response({ + access_token: 'access-token', + token_type: 'Bearer', + scope: SCOPES, + created_at: 123, + id: 42, + me: 'https://social.example/users/alice', + }), + ) + vi.stubGlobal('fetch', fetchMock) + + await expect(completeLogin()).resolves.toEqual({ + host: 'social.example', + token: 'access-token', + returnTo: '#/timeline/home', + }) + + expect(fetchMock).toHaveBeenCalledOnce() + const [url, options] = fetchMock.mock.calls[0] as [string, RequestInit] + expect(url).toBe('https://social.example/oauth/token') + expect(options.body).toBeInstanceOf(URLSearchParams) + expect(Object.fromEntries(new URLSearchParams(String(options.body)))).toEqual({ + grant_type: 'authorization_code', + client_id: 'client-id', + client_secret: 'client-secret', + redirect_uri: redirectUri(), + code: 'authorization-code', + code_verifier: 'pkce-verifier', + }) + expect(window.location.search).toBe('') + }) + + it('does not retry a PKCE-bound code without its verifier', async () => { + saveApp() + savePending() + window.history.replaceState({}, '', '/?code=bad-code&state=expected-state') + const fetchMock = vi + .fn() + .mockResolvedValue(response({ error: 'invalid_grant' }, 400)) + vi.stubGlobal('fetch', fetchMock) + + await expect(completeLogin()).rejects.toThrow('invalid_grant') + expect(fetchMock).toHaveBeenCalledOnce() + }) + + it('rejects expired attempts before exchanging the code', async () => { + saveApp() + savePending({ createdAt: Date.now() - 11 * 60 * 1000 }) + window.history.replaceState({}, '', '/?code=old-code&state=expected-state') + const fetchMock = vi.fn() + vi.stubGlobal('fetch', fetchMock) + + await expect(completeLogin()).rejects.toThrow('expired') + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('revokes with URL-encoded form data', async () => { + saveApp() + const fetchMock = vi.fn().mockResolvedValue(response({})) + vi.stubGlobal('fetch', fetchMock) + + await revoke('social.example', 'access-token') + + const [url, options] = fetchMock.mock.calls[0] as [string, RequestInit] + expect(url).toBe('https://social.example/oauth/revoke') + expect(options.body).toBeInstanceOf(URLSearchParams) + expect(Object.fromEntries(new URLSearchParams(String(options.body)))).toEqual({ + client_id: 'client-id', + client_secret: 'client-secret', + token: 'access-token', + }) + }) +}) diff --git a/src/lib/api/oauth.ts b/src/lib/api/oauth.ts index 8a56f2e..053dcde 100644 --- a/src/lib/api/oauth.ts +++ b/src/lib/api/oauth.ts @@ -3,16 +3,15 @@ * * A static frontend has no backend to keep a client secret, so it registers a * throwaway app per instance (`POST /api/v1/apps`) and uses the authorization - * code flow, upgrading to PKCE when the server advertises support (Mastodon - * 4.3+). Pleroma and older Mastodon ignore the PKCE params, hence the retry - * without them rather than a hard requirement. + * code flow with S256 PKCE. Current Pleroma supports these parameters, while + * older OAuth implementations ignore unknown authorization parameters. * * The registered app credentials and the resulting token live in localStorage; * they are per-origin and per-instance, and are exactly as sensitive as being * logged in on this browser. */ -import { ApiClient, ApiError, normalizeHost } from './client' +import { ApiClient, normalizeHost } from './client' import type { OAuthApp, OAuthToken } from './types' export const APP_NAME = 'plspace' @@ -21,11 +20,18 @@ export const SCOPES = 'read write follow' const APP_KEY = 'plspace:oauth:apps' const PENDING_KEY = 'plspace:oauth:pending' +const PENDING_MAX_AGE_MS = 10 * 60 * 1000 + +interface StoredOAuthApp extends OAuthApp { + /** Redirect registered by this client, independent of server response shape. */ + plspace_redirect_uri?: string +} interface PendingAuth { host: string verifier?: string state: string + createdAt: number /** Route to land on after a successful exchange. */ returnTo: string } @@ -39,9 +45,10 @@ export function redirectUri(): string { return `${window.location.origin}${window.location.pathname}` } -function readApps(): Record { +function readApps(): Record { try { - return JSON.parse(localStorage.getItem(APP_KEY) ?? '{}') as Record + const value = JSON.parse(localStorage.getItem(APP_KEY) ?? '{}') as unknown + return value && typeof value === 'object' ? (value as Record) : {} } catch { return {} } @@ -49,17 +56,23 @@ function readApps(): Record { function writeApp(host: string, app: OAuthApp): void { const apps = readApps() - apps[host] = app + apps[host] = { ...app, plspace_redirect_uri: redirectUri() } localStorage.setItem(APP_KEY, JSON.stringify(apps)) } +function appHasRedirect(app: StoredOAuthApp, redirect: string): boolean { + if (app.plspace_redirect_uri === redirect) return true + if (app.redirect_uris?.includes(redirect)) return true + return app.redirect_uri?.split(/\s+/).includes(redirect) ?? false +} + /** Register (or reuse) an OAuth app on `host`. */ export async function ensureApp(host: string): Promise { const key = normalizeHost(host) const cached = readApps()[key] // Re-register if the deployment moved: a stale redirect_uri fails at /oauth/authorize // with an opaque error page, which is miserable to debug. - if (cached && cached.redirect_uri === redirectUri()) return cached + if (cached?.client_id && cached.client_secret && appHasRedirect(cached, redirectUri())) return cached const client = new ApiClient(key) const app = await client.post('/api/v1/apps', { @@ -68,6 +81,9 @@ export async function ensureApp(host: string): Promise { scopes: SCOPES, website: APP_WEBSITE, }) + if (!app?.client_id || !app.client_secret) { + throw new Error('The server returned an invalid OAuth app registration.') + } writeApp(key, app) return app } @@ -85,13 +101,37 @@ function base64url(buffer: ArrayBuffer | Uint8Array): string { return btoa(binary).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '') } -async function challengeFor(verifier: string): Promise { +export async function pkceChallenge(verifier: string): Promise { const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(verifier)) return base64url(digest) } +export function authorizationUrl( + host: string, + app: Pick, + state: string, + codeChallenge?: string, +): string { + const params = new URLSearchParams({ + client_id: app.client_id, + redirect_uri: redirectUri(), + response_type: 'code', + scope: SCOPES, + state, + }) + if (codeChallenge) { + params.set('code_challenge', codeChallenge) + params.set('code_challenge_method', 'S256') + } + return `https://${normalizeHost(host)}/oauth/authorize?${params.toString()}` +} + /** Kick off the redirect to the instance's consent screen. */ -export async function beginLogin(host: string, returnTo = '#/'): Promise { +export async function beginLogin( + host: string, + returnTo = '#/', + navigate: (url: string) => void = (url) => window.location.assign(url), +): Promise { const key = normalizeHost(host) const app = await ensureApp(key) @@ -100,23 +140,18 @@ export async function beginLogin(host: string, returnTo = '#/'): Promise { // authorization-code flow there rather than failing to log in at all. const canPkce = Boolean(crypto.subtle) const verifier = canPkce ? randomString(48) : undefined + const codeChallenge = verifier ? await pkceChallenge(verifier) : undefined - const pending: PendingAuth = { host: key, verifier, state, returnTo } + const pending: PendingAuth = { + host: key, + verifier, + state, + createdAt: Date.now(), + returnTo: returnTo.startsWith('#/') ? returnTo : '#/', + } sessionStorage.setItem(PENDING_KEY, JSON.stringify(pending)) - const params = new URLSearchParams({ - client_id: app.client_id, - redirect_uri: redirectUri(), - response_type: 'code', - scope: SCOPES, - state, - }) - if (verifier) { - params.set('code_challenge', await challengeFor(verifier)) - params.set('code_challenge_method', 'S256') - } - - window.location.assign(`https://${key}/oauth/authorize?${params.toString()}`) + navigate(authorizationUrl(key, app, state, codeChallenge)) } export interface CompletedLogin { @@ -141,48 +176,76 @@ export async function completeLogin(): Promise { sessionStorage.removeItem(PENDING_KEY) clearOAuthParams() - if (error) { - throw new Error(url.searchParams.get('error_description') ?? `Authorization failed: ${error}`) - } if (!rawPending) { throw new Error('No sign-in was in progress in this tab. Please start again.') } - const pending = JSON.parse(rawPending) as PendingAuth + const pending = parsePending(rawPending) if (pending.state !== state) { throw new Error('Sign-in state did not match. Please start again.') } + if (Date.now() - pending.createdAt > PENDING_MAX_AGE_MS) { + throw new Error('This sign-in attempt expired. Please start again.') + } + if (error) { + throw new Error(url.searchParams.get('error_description') ?? `Authorization failed: ${error}`) + } const app = readApps()[pending.host] if (!app) throw new Error('Lost the app registration for this server. Please start again.') const client = new ApiClient(pending.host) - const body: Record = { + const body = { grant_type: 'authorization_code', client_id: app.client_id, client_secret: app.client_secret, redirect_uri: redirectUri(), - scope: SCOPES, code: code!, + code_verifier: pending.verifier, } - if (pending.verifier) body.code_verifier = pending.verifier - let token: OAuthToken - try { - token = await client.post('/oauth/token', body) - } catch (cause) { - // Servers that don't implement PKCE reject the unexpected code_verifier. - if (pending.verifier && cause instanceof ApiError && cause.status === 400) { - delete body.code_verifier - token = await client.post('/oauth/token', body) - } else { - throw cause - } + // A code issued with a PKCE challenge is bound to its verifier. Retrying a + // failed exchange without it is both invalid and liable to consume the code. + const token = await client.postForm('/oauth/token', body) + if (!token || typeof token.access_token !== 'string' || !token.access_token) { + throw new Error('The server returned an invalid OAuth token response.') } return { host: pending.host, token: token.access_token, returnTo: pending.returnTo || '#/' } } +function parsePending(raw: string): PendingAuth { + let value: unknown + try { + value = JSON.parse(raw) + } catch { + throw new Error('The saved sign-in attempt is invalid. Please start again.') + } + if (!value || typeof value !== 'object') { + throw new Error('The saved sign-in attempt is invalid. Please start again.') + } + const pending = value as Partial + if ( + typeof pending.host !== 'string' || + !normalizeHost(pending.host) || + typeof pending.state !== 'string' || + !pending.state || + typeof pending.createdAt !== 'number' || + !Number.isFinite(pending.createdAt) || + typeof pending.returnTo !== 'string' || + (pending.verifier !== undefined && typeof pending.verifier !== 'string') + ) { + throw new Error('The saved sign-in attempt is invalid. Please start again.') + } + return { + host: normalizeHost(pending.host), + state: pending.state, + verifier: pending.verifier, + createdAt: pending.createdAt, + returnTo: pending.returnTo.startsWith('#/') ? pending.returnTo : '#/', + } +} + /** Strip `?code=…&state=…` so a refresh doesn't try to redeem a spent code. */ function clearOAuthParams(): void { const url = new URL(window.location.href) @@ -197,7 +260,7 @@ export async function revoke(host: string, token: string): Promise { const app = readApps()[normalizeHost(host)] if (!app) return try { - await new ApiClient(host).post('/oauth/revoke', { + await new ApiClient(host).postForm('/oauth/revoke', { client_id: app.client_id, client_secret: app.client_secret, token, diff --git a/src/lib/api/types.ts b/src/lib/api/types.ts index 602920e..6764553 100644 --- a/src/lib/api/types.ts +++ b/src/lib/api/types.ts @@ -283,9 +283,12 @@ export interface OAuthApp { id: string name: string website?: string | null - redirect_uri: string + /** Deprecated by Mastodon 4.3, but still returned by older servers. */ + redirect_uri?: string + redirect_uris?: string[] client_id: string client_secret: string + client_secret_expires_at?: number vapid_key?: string } diff --git a/src/test/setup.ts b/src/test/setup.ts index e9ab882..11dd063 100644 --- a/src/test/setup.ts +++ b/src/test/setup.ts @@ -1,22 +1,35 @@ import '@testing-library/jest-dom/vitest' +import { beforeEach } from 'vitest' -const values = new Map() -const memoryStorage: Storage = { - get length() { - return values.size - }, - clear: () => values.clear(), - getItem: (key) => values.get(key) ?? null, - key: (index) => [...values.keys()][index] ?? null, - removeItem: (key) => { - values.delete(key) - }, - setItem: (key, value) => { - values.set(key, String(value)) - }, +function memoryStorage(): Storage { + const values = new Map() + return { + get length() { + return values.size + }, + clear: () => values.clear(), + getItem: (key) => values.get(key) ?? null, + key: (index) => [...values.keys()][index] ?? null, + removeItem: (key) => { + values.delete(key) + }, + setItem: (key, value) => { + values.set(key, String(value)) + }, + } } Object.defineProperty(globalThis, 'localStorage', { configurable: true, - value: memoryStorage, + value: memoryStorage(), +}) +Object.defineProperty(globalThis, 'sessionStorage', { + configurable: true, + value: memoryStorage(), +}) + +beforeEach(() => { + localStorage.clear() + sessionStorage.clear() + window.history.replaceState({}, '', '/') }) diff --git a/vite.config.ts b/vite.config.ts index cbd0719..e995fec 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -22,6 +22,9 @@ export default defineConfig({ }, test: { environment: 'jsdom', + environmentOptions: { + jsdom: { url: 'https://plspace.test/' }, + }, setupFiles: ['./src/test/setup.ts'], }, })