Fix PDS Moover backups (Authorization header + CORS) - #197
Conversation
The CORS middleware advertised `Access-Control-Allow-Headers: *`, but `*` wildcard does not cover the `Authorization` header. Authenticated cross-origin XRPC calls from web clients (e.g. PDS Moover's getRepoStatus) were being blocked at preflight. Lists the allowed headers explicitly instead.
9d3332f to
ad262ec
Compare
My previous commit hand-listed allowed headers (to get Authorization working), but that quietly dropped others browsers need (accept-language, x-bsky-topics, etc). Omitting allowHeaders lets Hono echo the requested headers back, matching the [reference atproto PDS](https://github.com/bluesky-social/atproto/blob/7f5c4ceb0b6872cb921ba9c2fab8c38614414f6c/packages/pds/src/index.ts#L171) and covering anything future clients send.
ad262ec to
4cd16af
Compare
There was a problem hiding this comment.
Thanks — this is right. Hono's allowHeaders: ["*"] never covered Authorization (the Fetch spec excludes it from the wildcard), so reflecting the requested headers is the only way to allow it; Firefox and Safari enforce that, Chrome historically didn't, which is probably why nobody noticed. I checked the reflection is safe for our origin: "*", no-credentials setup and that Hono adds Vary: Access-Control-Request-Headers. Approving; a few small things inline to fix before merge, and could you retitle so the squash commit doesn't claim Moover is fixed (the body says it isn't yet) — something like "fix(pds): allow Authorization header in CORS preflight". I'll approve the CI run.
|
Thanks! I moved away from cirrus back in June as this was a blocker for me; I'll happily make the changes you suggest, but I won't have a good way of manually testing (I didn't need my Cloudflare subscription without Cirrus!) I hope you're comfy with just the CI run 😊 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Inline notes for the review above.
| "@getcirrus/pds": patch | ||
| --- | ||
|
|
||
| Be explicit with CORS headers so browser-based authenticated XRPC calls work (particularly PDS Moover). |
There was a problem hiding this comment.
This says "be explicit" but the change removes the explicit list in favour of reflection. Could you make it one user-facing line, e.g.:
Allow the
Authorizationheader in cross-origin requests. Browser clients sending a bearer token were previously blocked at CORS preflight in Firefox and Safari.
The second paragraph about the Bluesky implementation is good PR-description material but doesn't need to be in the changelog.
| // Omit allowHeaders: Hono reflects the browser's | ||
| // Access-Control-Request-Headers back, matching the reference | ||
| // atproto PDS (`cors({ maxAge })`). This allows Authorization | ||
| // (a `*` wildcard wouldn't), DPoP, atproto-proxy, | ||
| // atproto-accept-labelers, accept-language, x-bsky-topics and | ||
| // any future header automatically. |
There was a problem hiding this comment.
Can be one line — the thing it needs to stop someone doing is re-adding allowHeaders: ["*"], so just say that. Something like:
// No allowHeaders: the "*" wildcard doesn't cover Authorization, so Hono reflects the requested headers instead.A preflight test would be nice too (OPTIONS with Access-Control-Request-Headers: authorization, assert it comes back in Allow-Headers) but I won't block on it.
|
No worries. Thanks for the contribution anyway! |
2 participants
Hey Matt; the CORS middleware currently advertises
Access-Control-Allow-Headers: *; as setting*wildcard does not cover theAuthorizationheader it breaks some tools (like PDS Moover's backup), which need it.I checked out the Bluesky PDS, and they just reflect all headers back by not specifying any allowed headers, so I swapped to that here.
This still doesn't fix PDS Moover, as it looks like there are other issues, but this fix removes the issue I had with the Authorization header!