diff --git a/README.md b/README.md index 9ae1715..74f6852 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,14 @@ the consent window, watches its tab for the redirect back to the matters), then hands the redirect back to the offscreen document for the token exchange and closes both. +Both halves name the redirect URI of the ID the build is actually running +under (`chrome.runtime.id`), because the OAuth server checks the exchange +against what the authorization request registered. The client's own default +for each half is `redirect_uris[0]` in the metadata — the store ID — so +leaving either to the default breaks sign-in for every install that is not +the store one, and only at the exchange (`src/lib/oauth.ts`, +`src/lib/oauth.test.ts`). + Sessions persist in IndexedDB with a non-extractable DPoP key — the same model `@atproto/oauth-client-browser` uses for web apps — so you stay signed in across browser restarts without re-consenting. diff --git a/src/lib/oauth.test.ts b/src/lib/oauth.test.ts new file mode 100644 index 0000000..3a3868b --- /dev/null +++ b/src/lib/oauth.test.ts @@ -0,0 +1,74 @@ +// The one thing about the OAuth client that can be tested without a browser: +// that both halves of the flow name the same redirect URI, and that it is this +// build's own id rather than whichever id the client metadata happens to list +// first. Getting that wrong fails only at the token exchange, only for the ids +// that are not first, and only in a real sign-in — so it is worth pinning here. +import { beforeEach, describe, expect, it, vi } from 'vitest' + +interface RedirectOption { + redirect_uri?: string +} + +const authorize = vi.fn( + async (_input: string, _options?: RedirectOption) => + new URL('https://pds.example.com/authorize?x=1'), +) +const callback = vi.fn(async (_params: URLSearchParams, _options?: RedirectOption) => ({ + session: { sub: 'did:plc:abc123' }, +})) + +vi.mock('@atproto/oauth-client-browser', () => ({ + BrowserOAuthClient: class { + authorize = authorize + callback = callback + }, +})) + +// buildSessionInfo resolves the handle and avatar; both are cosmetic and this +// test is about the arguments, not the profile. +vi.mock('./atproto', () => ({ + resolveDid: vi.fn(async () => { + throw new Error('no network in tests') + }), + profileAvatarUrl: vi.fn(), +})) + +const EXTENSION_ID = 'degljbilkggdpbobomfbgnellecgbkjj' +const REDIRECT = `https://${EXTENSION_ID}.chromiumapp.org/oauth2` + +vi.stubGlobal('chrome', { runtime: { id: EXTENSION_ID } }) + +const { completeAuthorization, startAuthorization } = await import('./oauth') + +// The store id, listed first in oauth/client-metadata.json — what the client +// falls back to when a call does not say otherwise. +const metadata = (await import('../../oauth/client-metadata.json')).default + +beforeEach(() => { + authorize.mockClear() + callback.mockClear() +}) + +describe('the interactive sign-in flow', () => { + it('authorizes with the redirect uri of the id this build runs under', async () => { + await startAuthorization('reader.example.com') + expect(authorize).toHaveBeenCalledWith('reader.example.com', { redirect_uri: REDIRECT }) + }) + + it('exchanges the code with the same redirect uri it authorized with', async () => { + await completeAuthorization(`${REDIRECT}?code=abc&state=xyz`) + const [params, options] = callback.mock.calls[0] ?? [] + expect(params?.get('code')).toBe('abc') + expect(options).toEqual({ redirect_uri: REDIRECT }) + }) + + it('does not leave the exchange to fall back to the first listed id', async () => { + // The regression: with no options the client sends redirect_uris[0], so an + // unpacked build authorizes as itself and exchanges as the store. + expect(metadata.redirect_uris[0]).not.toBe(REDIRECT) + await completeAuthorization(`${REDIRECT}?code=abc`) + const sent = callback.mock.calls[0]?.[1]?.redirect_uri + expect(sent).toBe(REDIRECT) + expect(sent).not.toBe(metadata.redirect_uris[0]) + }) +}) diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 9c92f3a..a50f4cc 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -31,6 +31,22 @@ function getClient(): BrowserOAuthClient { return client } +/** + * The redirect URI for the id this build is actually running under, which is + * not the one the OAuth client would pick on its own. + * + * Both halves of the flow must send the same value: the authorization request + * registers it, and the token exchange is checked against what was registered. + * The client defaults each half to `clientMetadata.redirect_uris[0]`, and the + * metadata lists the store id first, so an unpacked build that authorizes as + * itself would exchange as the store — `invalid_grant`, "The redirect_uri + * parameter must match the one used in the authorization request". Same shape + * of bug as v1.2.1, on the other half of the flow. + */ +function redirectUri(): `https://${string}` { + return oauthRedirectUri(chrome.runtime.id) as `https://${string}` +} + /** * First half of interactive sign-in: resolve the handle, push the * authorization request, and return the consent URL to open. The client @@ -38,9 +54,7 @@ function getClient(): BrowserOAuthClient { * completeAuthorization can run in a later client instance. */ export async function startAuthorization(handle: string): Promise { - const url = await getClient().authorize(handle, { - redirect_uri: oauthRedirectUri(chrome.runtime.id) as `https://${string}`, - }) + const url = await getClient().authorize(handle, { redirect_uri: redirectUri() }) return url.href } @@ -51,7 +65,7 @@ export async function startAuthorization(handle: string): Promise { */ export async function completeAuthorization(callbackUrl: string): Promise { const params = new URL(callbackUrl).searchParams - const { session } = await getClient().callback(params) + const { session } = await getClient().callback(params, { redirect_uri: redirectUri() }) return buildSessionInfo(session) }