diff --git a/.changeset/par-validate-redirect-uri.md b/.changeset/par-validate-redirect-uri.md new file mode 100644 index 00000000..da6f1326 --- /dev/null +++ b/.changeset/par-validate-redirect-uri.md @@ -0,0 +1,5 @@ +--- +"@getcirrus/oauth-provider": patch +--- + +PAR (`/oauth/par`) now validates `redirect_uri` against the client's registered redirect_uris at push time. Previously the check only ran at the authorize step, which let a malicious caller obtain a `request_uri` for an unregistered redirect even though the subsequent authorize would have rejected it. Reject early per RFC 6749 §3.1.2.4. diff --git a/packages/oauth-provider/src/par.ts b/packages/oauth-provider/src/par.ts index 4a1211c0..a918c89c 100644 --- a/packages/oauth-provider/src/par.ts +++ b/packages/oauth-provider/src/par.ts @@ -4,6 +4,7 @@ */ import type { OAuthParResponse } from "@atproto/oauth-types"; +import type { ClientResolver } from "./client-resolver.js"; import type { OAuthStorage, PARData } from "./storage.js"; import { randomString } from "./encoding.js"; import { parseRequestBody } from "./provider.js"; @@ -53,6 +54,7 @@ const REQUIRED_PARAMS = [ */ export class PARHandler { private storage: OAuthStorage; + private clientResolver: ClientResolver; private issuer: string; private expiresIn: number; private allowIncludes: boolean; @@ -60,6 +62,9 @@ export class PARHandler { /** * Create a PAR handler * @param storage OAuth storage implementation + * @param clientResolver Client metadata resolver. Used to validate + * redirect_uri against the registered list at push time per + * RFC 6749 §3.1.2.4. * @param issuer The OAuth issuer URL * @param expiresIn PAR expiration time in seconds (default: 90) * @param allowIncludes Whether `include:` (permission set) scopes are @@ -69,11 +74,13 @@ export class PARHandler { */ constructor( storage: OAuthStorage, + clientResolver: ClientResolver, issuer: string, expiresIn: number = DEFAULT_EXPIRES_IN, allowIncludes: boolean = false, ) { this.storage = storage; + this.clientResolver = clientResolver; this.issuer = issuer; this.expiresIn = expiresIn; this.allowIncludes = allowIncludes; @@ -147,6 +154,28 @@ export class PARHandler { return this.errorResponse("invalid_request", "Invalid redirect_uri", 400); } + // RFC 6749 §3.1.2.4: the AS MUST reject any redirect_uri that wasn't + // registered in the client metadata. Authorize re-validates, but PAR + // must also reject here — otherwise a request_uri gets issued for a + // redirect the client never registered. + let client; + try { + client = await this.clientResolver.resolveClient(clientId); + } catch (e) { + return this.errorResponse( + "invalid_client", + `Failed to resolve client: ${e instanceof Error ? e.message : String(e)}`, + 400, + ); + } + if (!client.redirectUris.includes(params.redirect_uri!)) { + return this.errorResponse( + "invalid_request", + "redirect_uri is not registered for this client", + 400, + ); + } + const scope = params.scope ?? ATPROTO_SCOPE; params.scope = scope; try { diff --git a/packages/oauth-provider/src/provider.ts b/packages/oauth-provider/src/provider.ts index 5491c63a..91bc9b72 100644 --- a/packages/oauth-provider/src/provider.ts +++ b/packages/oauth-provider/src/provider.ts @@ -215,14 +215,15 @@ export class ATProtoOAuthProvider { this.issuer = config.issuer; this.dpopRequired = config.dpopRequired ?? true; this.enablePAR = config.enablePAR ?? true; + this.clientResolver = + config.clientResolver ?? new ClientResolver({ storage: config.storage }); this.parHandler = new PARHandler( config.storage, + this.clientResolver, config.issuer, undefined, !!config.permissionSetResolver, ); - this.clientResolver = - config.clientResolver ?? new ClientResolver({ storage: config.storage }); this.verifyUser = config.verifyUser; this.getCurrentUser = config.getCurrentUser; this.getPasskeyOptions = config.getPasskeyOptions; diff --git a/packages/oauth-provider/test/par.test.ts b/packages/oauth-provider/test/par.test.ts index 90882cd6..f4bf56c4 100644 --- a/packages/oauth-provider/test/par.test.ts +++ b/packages/oauth-provider/test/par.test.ts @@ -1,15 +1,44 @@ import { describe, it, expect, beforeEach } from "vitest"; +import { ClientResolver } from "../src/client-resolver.js"; import { PARHandler } from "../src/par.js"; import { InMemoryOAuthStorage } from "../src/storage.js"; +import type { ClientMetadata } from "../src/storage.js"; import { generateCodeChallenge, generateCodeVerifier } from "./helpers.js"; +/** + * Stub resolver for PAR tests: returns metadata with a fixed registered + * redirect_uri so PAR's RFC 6749 §3.1.2.4 check passes for the canonical + * fixture. Tests that probe rejection pass a different redirect_uri. + */ +class StubResolver extends ClientResolver { + private fixed: ClientMetadata; + constructor(redirectUri: string) { + super(); + this.fixed = { + clientId: "did:web:client.example.com", + clientName: "Test Client", + redirectUris: [redirectUri], + tokenEndpointAuthMethod: "none", + cachedAt: Date.now(), + }; + } + override async resolveClient(): Promise { + return this.fixed; + } +} + describe("PAR Handler", () => { let storage: InMemoryOAuthStorage; let handler: PARHandler; + const REGISTERED_REDIRECT = "https://client.example.com/callback"; beforeEach(() => { storage = new InMemoryOAuthStorage(); - handler = new PARHandler(storage, "https://example.com"); + handler = new PARHandler( + storage, + new StubResolver(REGISTERED_REDIRECT), + "https://example.com", + ); }); function createPARRequest(params: Record): Request { @@ -79,6 +108,31 @@ describe("PAR Handler", () => { expect(json.error).toBe("invalid_request"); }); + it("rejects redirect_uri not registered in client metadata (RFC 6749 §3.1.2.4)", async () => { + const verifier = generateCodeVerifier(); + const challenge = await generateCodeChallenge(verifier); + + const request = createPARRequest({ + client_id: "did:web:client.example.com", + redirect_uri: "https://attacker.invalid/steal-code", + response_type: "code", + code_challenge: challenge, + code_challenge_method: "S256", + state: "random-state", + scope: "atproto", + }); + + const response = await handler.handlePushRequest(request); + expect(response.status).toBe(400); + + const json = (await response.json()) as { + error: string; + error_description: string; + }; + expect(json.error).toBe("invalid_request"); + expect(json.error_description).toMatch(/not registered/i); + }); + it("rejects unsupported response_type", async () => { const verifier = generateCodeVerifier(); const challenge = await generateCodeChallenge(verifier);