From af9261a07ad22fc5a83116641b5be6b546b0a214 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Sun, 24 May 2026 22:14:56 +0100 Subject: [PATCH 1/2] fix(check): preserve pre-redirect step results across OAuth redirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runPostCallback was marking every PRE_REDIRECT_STEPS entry as 'pass' regardless of its actual outcome before the redirect — so any failure in the pre-redirect leg (PAR boundary tests, scope selection, metadata validation, etc.) would silently become green once the user came back from the auth server. Snapshot each pre-redirect step (status, message, evidence, timings) into the persisted state alongside codeVerifier/stateNonce, and restore from it after the callback. If no persisted result is present (older session storage), the steps render as 'skip' with a 'no persisted result' message rather than a fabricated pass. --- apps/check/src/lib/oauth-flow.ts | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/apps/check/src/lib/oauth-flow.ts b/apps/check/src/lib/oauth-flow.ts index 57568259..eb6f0347 100644 --- a/apps/check/src/lib/oauth-flow.ts +++ b/apps/check/src/lib/oauth-flow.ts @@ -64,6 +64,7 @@ interface PersistedState { stateNonce: string; expectedIss: string; scope: string; + preRedirectSteps?: FlowStep[]; } // Scope is chosen at runtime based on the auth server's advertised scopes: @@ -1136,6 +1137,7 @@ export function startPreRedirectFlow(target: string): FlowRun { evidence: { request: { method: "GET", url: authUrl.toString() } }, })); + const preRedirectSet = new Set(PRE_REDIRECT_STEPS); await persistState({ target, handle: resolved.handle, @@ -1146,6 +1148,9 @@ export function startPreRedirectFlow(target: string): FlowRun { stateNonce, expectedIss: (state.authServer!.issuer as string) ?? "", scope: activeScope, + preRedirectSteps: state.steps + .filter((s) => preRedirectSet.has(s.id)) + .map((s) => ({ ...s })), }); setState( @@ -1199,10 +1204,23 @@ export async function runPostCallback(): Promise { if (persisted?.scope) activeScope = persisted.scope; const initial = createFlowState(persisted?.target ?? ""); - // Mark pre-redirect steps complete + // Restore the pre-redirect step results captured before the redirect. If + // we don't have them (older persisted state, or none at all), fall back + // to marking the steps "skip" rather than fabricating a "pass" — the + // alternative was to lie about what actually happened pre-redirect. + const persistedById = new Map( + (persisted?.preRedirectSteps ?? []).map((s) => [s.id, s]), + ); for (const id of PRE_REDIRECT_STEPS) { const idx = initial.steps.findIndex((s) => s.id === id); - if (idx >= 0) initial.steps[idx]!.status = "pass"; + if (idx < 0) continue; + const prior = persistedById.get(id); + if (prior) { + initial.steps[idx] = { ...initial.steps[idx]!, ...prior }; + } else { + initial.steps[idx]!.status = "skip"; + initial.steps[idx]!.message = "no persisted result"; + } } initial.phase = "post-callback"; initial.handle = persisted?.handle; From 9079d00d3c3244d19aef9ea58ebd2444a637e7b8 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Sun, 24 May 2026 22:24:17 +0100 Subject: [PATCH 2/2] fix(oauth-provider): PAR rejects unregistered redirect_uri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §3.1.2.4 requires the authorization server to reject any redirect_uri that was not registered in the client metadata. Cirrus checked this at the authorize step (provider.ts:370) but not at PAR, so a caller could obtain a valid request_uri for an unregistered redirect — defense-in-depth weakness flagged by pdscheck's flow.par-rejects-unregistered-redirect-uri check. Wire the existing ClientResolver into PARHandler and validate redirect_uri against client.redirectUris before issuing the request_uri. Adds a regression test covering the rejection. --- .changeset/par-validate-redirect-uri.md | 5 +++ packages/oauth-provider/src/par.ts | 29 ++++++++++++ packages/oauth-provider/src/provider.ts | 5 ++- packages/oauth-provider/test/par.test.ts | 56 +++++++++++++++++++++++- 4 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 .changeset/par-validate-redirect-uri.md 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);