diff --git a/.changeset/par-resolve-includes-eagerly.md b/.changeset/par-resolve-includes-eagerly.md new file mode 100644 index 00000000..9c945703 --- /dev/null +++ b/.changeset/par-resolve-includes-eagerly.md @@ -0,0 +1,5 @@ +--- +"@getcirrus/oauth-provider": patch +--- + +PAR (`/oauth/par`) now resolves every `include:` permission-set scope eagerly and rejects with `invalid_scope` when an include points at a nonexistent or non-permission-set lexicon. Previously the resolver only ran at the authorize step, so clients with a typo in an include scope got a fresh `request_uri` from PAR and only learned about the bad scope at consent time. Matches reference oauth-provider behaviour (`request-manager.ts:297-313`). diff --git a/packages/oauth-provider/src/par.ts b/packages/oauth-provider/src/par.ts index a918c89c..872e6312 100644 --- a/packages/oauth-provider/src/par.ts +++ b/packages/oauth-provider/src/par.ts @@ -5,10 +5,16 @@ import type { OAuthParResponse } from "@atproto/oauth-types"; import type { ClientResolver } from "./client-resolver.js"; +import type { PermissionSetResolver } from "./permission-sets.js"; import type { OAuthStorage, PARData } from "./storage.js"; import { randomString } from "./encoding.js"; import { parseRequestBody } from "./provider.js"; -import { ATPROTO_SCOPE, ScopeParseError, parseScope } from "./scopes.js"; +import { + ATPROTO_SCOPE, + ScopeParseError, + expandScope, + parseScope, +} from "./scopes.js"; export type { OAuthParResponse }; @@ -57,7 +63,7 @@ export class PARHandler { private clientResolver: ClientResolver; private issuer: string; private expiresIn: number; - private allowIncludes: boolean; + private permissionSetResolver?: PermissionSetResolver; /** * Create a PAR handler @@ -67,23 +73,24 @@ export class PARHandler { * 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 - * accepted. Should mirror the parent provider's - * `permissionSetResolver` configuration so PAR rejects upfront rather - * than letting the request fail at consent. + * @param permissionSetResolver Resolver for `include:` permission + * set scopes. When provided, PAR resolves every requested include + * eagerly and rejects with `invalid_scope` on resolution failure, + * matching the reference oauth-provider. When omitted, `include:` + * scopes are rejected outright at the parse step. */ constructor( storage: OAuthStorage, clientResolver: ClientResolver, issuer: string, expiresIn: number = DEFAULT_EXPIRES_IN, - allowIncludes: boolean = false, + permissionSetResolver?: PermissionSetResolver, ) { this.storage = storage; this.clientResolver = clientResolver; this.issuer = issuer; this.expiresIn = expiresIn; - this.allowIncludes = allowIncludes; + this.permissionSetResolver = permissionSetResolver; } /** @@ -178,12 +185,16 @@ export class PARHandler { const scope = params.scope ?? ATPROTO_SCOPE; params.scope = scope; + const allowIncludes = !!this.permissionSetResolver; try { - // PAR is a deferred authorize: when permission sets are enabled, - // includes are accepted here and resolved at the authorize step. - // When disabled, reject upfront so the client gets a clear error - // rather than a dead-end at consent time. - parseScope(scope, { allowIncludes: this.allowIncludes }); + parseScope(scope, { allowIncludes }); + // Eagerly resolve every `include:` against the lexicon so a + // nonexistent permission set fails fast with invalid_scope here + // rather than dead-ending at the consent UI. Matches reference + // oauth-provider behaviour (request-manager.ts:297). + if (allowIncludes && scope.includes("include:")) { + await expandScope(scope, this.permissionSetResolver); + } } catch (e) { if (e instanceof ScopeParseError) { return this.errorResponse("invalid_scope", e.message, 400); diff --git a/packages/oauth-provider/src/provider.ts b/packages/oauth-provider/src/provider.ts index e6f55c4a..8659c5ee 100644 --- a/packages/oauth-provider/src/provider.ts +++ b/packages/oauth-provider/src/provider.ts @@ -222,7 +222,7 @@ export class ATProtoOAuthProvider { this.clientResolver, config.issuer, undefined, - !!config.permissionSetResolver, + config.permissionSetResolver, ); this.verifyUser = config.verifyUser; this.getCurrentUser = config.getCurrentUser; diff --git a/packages/oauth-provider/test/par.test.ts b/packages/oauth-provider/test/par.test.ts index f4bf56c4..9406ede6 100644 --- a/packages/oauth-provider/test/par.test.ts +++ b/packages/oauth-provider/test/par.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect, beforeEach } from "vitest"; import { ClientResolver } from "../src/client-resolver.js"; import { PARHandler } from "../src/par.js"; +import type { + LexiconPermissionSet, + PermissionSetResolver, +} from "../src/permission-sets.js"; import { InMemoryOAuthStorage } from "../src/storage.js"; import type { ClientMetadata } from "../src/storage.js"; import { generateCodeChallenge, generateCodeVerifier } from "./helpers.js"; @@ -133,6 +137,81 @@ describe("PAR Handler", () => { expect(json.error_description).toMatch(/not registered/i); }); + it("rejects include: for a permission set that fails to resolve", async () => { + const resolver: PermissionSetResolver = { + async resolve() { + return null; + }, + }; + const handlerWithResolver = new PARHandler( + storage, + new StubResolver(REGISTERED_REDIRECT), + "https://example.com", + undefined, + resolver, + ); + + const verifier = generateCodeVerifier(); + const challenge = await generateCodeChallenge(verifier); + const request = createPARRequest({ + client_id: "did:web:client.example.com", + redirect_uri: REGISTERED_REDIRECT, + response_type: "code", + code_challenge: challenge, + code_challenge_method: "S256", + state: "random-state", + scope: "atproto include:com.example.nonexistent", + }); + + const response = await handlerWithResolver.handlePushRequest(request); + expect(response.status).toBe(400); + + const json = (await response.json()) as { + error: string; + error_description: string; + }; + expect(json.error).toBe("invalid_scope"); + expect(json.error_description).toMatch(/com\.example\.nonexistent/); + }); + + it("accepts include: that the resolver successfully resolves", async () => { + const permissionSet: LexiconPermissionSet = { + type: "permission-set", + title: "Test bundle", + detail: "for tests", + permissions: [], + }; + const resolver: PermissionSetResolver = { + async resolve() { + return permissionSet; + }, + }; + const handlerWithResolver = new PARHandler( + storage, + new StubResolver(REGISTERED_REDIRECT), + "https://example.com", + undefined, + resolver, + ); + + const verifier = generateCodeVerifier(); + const challenge = await generateCodeChallenge(verifier); + const request = createPARRequest({ + client_id: "did:web:client.example.com", + redirect_uri: REGISTERED_REDIRECT, + response_type: "code", + code_challenge: challenge, + code_challenge_method: "S256", + state: "random-state", + scope: "atproto include:com.example.real", + }); + + const response = await handlerWithResolver.handlePushRequest(request); + expect(response.status).toBe(201); + const json = (await response.json()) as { request_uri: string }; + expect(json.request_uri).toMatch(/^urn:ietf:params:oauth:request_uri:/); + }); + it("rejects unsupported response_type", async () => { const verifier = generateCodeVerifier(); const challenge = await generateCodeChallenge(verifier);