From 4ee6fe71ba70c8f9314c0fbd422bdb67ee109e40 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Tue, 11 Aug 2026 14:39:23 -0400 Subject: [PATCH] fix(oauth): exchange the code with this build's own redirect uri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token exchange left redirect_uri to the OAuth client, which defaults to clientMetadata.redirect_uris[0] — the store id since 722de74. An unpacked build therefore authorized as itself and exchanged as the store, and the PDS refused it: invalid_grant, "The redirect_uri parameter must match the one used in the authorization request". Sign-in was dead for every install that is not the store one, at the last step of the flow. Both halves now take the URI from one place, the id the build is actually running under. A unit test pins it, including that the metadata's first entry is a different id, so the fallback cannot quietly come back. --- README.md | 8 +++++ src/lib/oauth.test.ts | 74 +++++++++++++++++++++++++++++++++++++++++++ src/lib/oauth.ts | 22 ++++++++++--- 3 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 src/lib/oauth.test.ts 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) } -- 2.51.2