Skip to content

fix(oauth-provider): resolve include: scopes at PAR time#186

Merged
ascorbic merged 2 commits into
mainfrom
fix/par-resolve-includes-eagerly
May 24, 2026
Merged

fix(oauth-provider): resolve include: scopes at PAR time#186
ascorbic merged 2 commits into
mainfrom
fix/par-resolve-includes-eagerly

Conversation

@ascorbic
Copy link
Copy Markdown
Owner

Summary

pdscheck's `flow.par-rejects-invalid-include` flagged Cirrus accepting a deliberately bogus permission-set NSID at PAR. Reference oauth-provider rejects this case at PAR (`request-manager.ts:297-313`); Cirrus was structurally validating the scope but deferring lexicon resolution to the authorize step, so the bad include only surfaced at consent time.

Fix

  • `PARHandler` constructor takes the `PermissionSetResolver` directly instead of a derived `allowIncludes` boolean.
  • After `parseScope()`, if the scope contains any `include:`, call `expandScope()` to resolve every NSID via the lexicon. Any `ScopeParseError` (failed network, non-permission-set lexicon, etc.) returns `400 invalid_scope` with the offending NSID in `error_description`.
  • Provider now passes the resolver through unchanged.

Test plan

  • `pnpm --filter @getcirrus/oauth-provider test` — 119 passing including two new tests ("rejects include: for a permission set that fails to resolve" and "accepts include: that the resolver successfully resolves").
  • `pnpm --filter @getcirrus/oauth-provider check` — clean.
  • `pnpm --filter @getcirrus/pds test` — 313 unit + 84 CLI passing.
  • Re-run pdscheck against the deployed PDS once this lands: expect `flow.par-rejects-invalid-include` to pass.

PAR was structurally validating `include:<nsid>` scopes (parseScope with
allowIncludes=true) but deferring lexicon resolution to the authorize
step. A request with a nonexistent permission-set NSID would get a
fresh request_uri from PAR and only fail at consent — which is the
exact dead-end the original allowIncludes flag claimed to avoid.

Reference oauth-provider resolves eagerly at PAR (request-manager.ts:
297-313). Plumb the permissionSetResolver through PARHandler and call
expandScope() after parseScope; any resolution failure returns
invalid_scope with the NSID in error_description.

Constructor now takes the resolver directly (previously a derived
allowIncludes boolean). Two new tests cover the resolver-rejects and
resolver-accepts cases.
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 24, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
pdscheck 2e0b25a May 24 2026, 10:29 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 24, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
atproto-pds 2e0b25a May 24 2026, 10:29 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 24, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
cirrusdocs 2e0b25a Commit Preview URL

Branch Preview URL
May 24 2026, 10:29 PM

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 24, 2026

Open in StackBlitz

npm i https://pkg.pr.new/create-pds@186
npm i https://pkg.pr.new/@getcirrus/oauth-provider@186
npm i https://pkg.pr.new/@getcirrus/pds@186

commit: 2e0b25a

@ascorbic ascorbic merged commit 22f09de into main May 24, 2026
7 checks passed
@ascorbic ascorbic deleted the fix/par-resolve-includes-eagerly branch May 24, 2026 22:30
@mixie-bot mixie-bot Bot mentioned this pull request May 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant