fix(bluesky) layer 2: sanitize redirect:'error' at both construction + direct-fetch seams#48
Merged
Conversation
…-fetch seams (Workers) PR #47's edgeFetch cleared the XRPC handle resolver but Bluesky sign-in still failed on prod for every input (the [bsky] log — now shipped — showed the same "Invalid redirect value ... at the edge" cause for did:web, did:plc, and handle). Why #47 was incomplete: @atproto's resolvers reach the runtime two different ways, and #47 only covered one. - DID + AS/PDS metadata resolvers build the Request EXPLICITLY via @atproto-labs/fetch's `asRequest()` (`new Request(url, {redirect:'error'})`), which workerd rejects at CONSTRUCTION — upstream of any injected fetch, so the #47 wrapper never ran for them. - XrpcHandleResolver instead calls `fetch(url, {redirect:'error'})` DIRECTLY, and the runtime's own fetch() builds that Request with its internal native constructor — which a globalThis.Request shim can't reach. Upstream is unfixed: @atproto-labs/did-resolver@0.3.4 / fetch@0.3.3 still hardcode redirect:'error', so a version bump wouldn't help (see #3292). Fix: one sanitizer (sanitizeEdgeRequestInit: redirect:'error'→'manual') wired into BOTH seams — a lazily-installed, idempotent Request-constructor shim for the explicit-`new Request` sites, and the injected edgeFetch for the direct fetch(url,init) sites. Together they cover every call site reachable from authorize()/callback(): DID-doc fetch, handle resolution, AS/PDS metadata, PAR. Anti-redirect-hijack semantics are preserved — the resolvers reject any non-2xx, and a manual-mode redirect surfaces as a non-2xx opaque response, so a DID doc served via a redirect still fails closed and is never followed to another origin. One-pass sweep: redirect:'error' is the only hostile option; `cache:'no-cache'` was empirically confirmed accepted by workerd (compat 2026-06-01), so no other normalization is needed. Zero new dependencies. Verified under wrangler dev with all three inputs: the "Invalid redirect value" throw is gone for every one; each now proceeds to the real network call (blocked here only by the diagnosis container's egress allowlist, which prod lacks). The full authorize() round-trip on the Workers runtime is the operator's prod re-test. Unit tests pin the sanitizer, the shim (idempotent + maps error→manual), and the injected fetch; the runtime's construction-time rejection isn't reproducible in the node:test harness (Node accepts redirect:'error') and is stated as such. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVPyHAtQ1cdK76RpFasafP
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
wecanjustbuildthings | a14122f | Jul 04 2026, 06:27 AM |
MartinMontero
marked this pull request as ready for review
July 4, 2026 06:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Completes the Bluesky-on-Workers fix. PR #47 was necessary but incomplete — it cleared the XRPC handle resolver but prod sign-in still failed for every input. This covers the sites #47 missed, via one sanitizer wired into two seams.
The verbatim prod log line that reopened it
Delivered from Workers Logs (post-#47 deploy), identical cause for
bsky.appanddid:plc:…:(The
[bsky]logging shipped in #47 is what surfaced this — it earned its place.)Revised verdict — why #47 missed it (case b: construction upstream of any fetch)
@atproto's resolvers reach the runtime two different ways; #47's injected fetch only covered one:did:plc/did:webbindFetch→asRequest()→new Request(url, {redirect:'error'})(explicit, pre-fetch)asRequest()explicitnew Requestfetch(url, {redirect:'error'})directly; runtime'sfetch()builds the Request with its internal native constructoredgeFetchthis.fetchand/ornew RequestA
globalThis.Requestshim can't reach the handle resolver (native-internal construction insidefetch()), and an injected fetch can't reach the DID resolvers (the throw is atasRequest'snew Request, before any fetch). Both seams are required, feeding one sanitizer.Upstream is unfixed: latest
@atproto-labs/did-resolver@0.3.4/@atproto-labs/fetch@0.3.3still hardcoderedirect:'error'and stillnew Request(input, init)— so a version bump (preference 2) does not help. This is the in-repo fix (preference 1), zero dependency churn.One-pass sweep (so there is no layer 3)
Every fetch/Request reachable from
authorize()/callback(), and how each hostile option is handled:DidPlcMethod.resolve/DidWebMethod.resolveredirect:'error'manualredirect:'error'(explicitnew Request)manualXrpcHandleResolver.resolveredirect:'error'+cache: noCache?'no-cache':undefinededgeFetch→manual;cacheempirically OK on workerd (probe:new Request({cache:'no-cache'})did not throw, compat 2026-06-01) → not normalizedredirect:'error'(explicitnew Request)manualOAuthServerAgent)this.fetch/new RequestedgeFetchor shimredirect:'error'is the only runtime-hostile option;cacheis accepted. No layer 3.Semantics — not weakened
redirect:'error'on DID-doc fetches is an anti-hijack control (adid:webdoc served via redirect must not hand resolution to another origin). It is emulated, never remapped tofollow:'manual'+ the resolvers' existing non-2xx rejection means a redirected doc surfaces as a non-2xx opaque response and still fails closed.Verification
wrangler dev, all three inputs (did:web,did:plc,bsky.apphandle): the "Invalid redirect value" throw is gone for every one (0 occurrences); each proceeds to the real network call — blocked here only by the diagnosis container's egress allowlist, which prod lacks. That transition is the fix confirmation.error→manual, passthrough, no mutation), the shim (idempotent, maps at construction), and the injected fetch. The runtime's construction-time rejection is not reproducible innode:test(Node acceptsredirect:'error') — stated plainly; the live round-trip is your prod re-test.verify:allgreen;enforcegreen; 287 tests, 0 fail.Type of change
Required checks
npm run check· [x]npm run enforce· [x]npm test(287) · [x]npm run buildDeploy semantics & your post-merge re-test
Merging IS a production deploy. After merge, please re-test on prod:
I'll poll the deploy and probe the start route for all three input types (the flip from
reason=authorizeto an outbound PDS redirect is the layer-cleared signal), then re-run the read-only regression set. If a new distinct failure appears past this layer, I'll STOP and report.Draft on purpose — do not mark ready-for-review, do not merge; merge is yours alone.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LVPyHAtQ1cdK76RpFasafP
Generated by Claude Code