feat(did): refuse redirects when resolving did:web by default - #133
feat(did): refuse redirects when resolving did:web by default#133EfeDurmaz16 wants to merge 4 commits into
Conversation
allowedHttpHosts is applied to the URL built from the DID, but the document fetch followed redirects, so the check only governed the first hop: a did:web served over https could redirect the resolver to plain http, or to a host the allowlist would have rejected. Because the redirect target is chosen by the DID's own host, this also turns resolution into an outbound request the issuer controls. did:web documents are served directly at a well-known path, so redirects are refused by default via redirect: "error". followRedirects: true restores the previous behavior for deployments that need it.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changesdid:web redirect handling
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
domleboss97
left a comment
There was a problem hiding this comment.
One comment on the error handling, but I think this is the right move!
| const res = await fetch(url, { mode: "cors" }) | ||
| const res = await fetch(url, { | ||
| mode: "cors", | ||
| redirect: followRedirects ? "follow" : "error", |
There was a problem hiding this comment.
i think it might be more useful to do manual here. with error this will throw with a TypeError, and then I think the resolver has nothing better ot report than notFound.
with manual, fetch resolves with the redirect repsonse (although in browsers i think it's opaque response), so we could throw a precise error - on node it could include the redirect target from the Location header.
There was a problem hiding this comment.
Done in 22f6a5d: switched to redirect: "manual". The resolver now refuses any redirect response with an error naming the Location target on Node; browsers surface an opaque redirect so the same error fires without a target. Tests cover both cases, changeset updated.
Per review: redirect: "error" rejects with a bare TypeError, which the resolver can only surface as a generic notFound. With redirect: "manual" the redirect resolves as a response, so the resolver throws a precise error naming the Location target on Node; browsers surface an opaque redirect and get the same error without a target.
| ) { | ||
| const location = res.headers.get("location") | ||
| throw new Error( | ||
| `DID resolution refused a redirect${location ? ` to ${location}` : ""}`, |
There was a problem hiding this comment.
nit - might be nice to add an action here, i.e. Set followRedirects: true to allow redirects.
|
@EfeDurmaz16 mind resolving the conflicts? |
Resolves the conflicts between the redirect refusal in this branch and the did:web fetch timeout added upstream in agentcommercekit#129. Both options now apply to the same request: the resolver sends `redirect: "manual"` (unless `followRedirects` is set) and an `AbortSignal` built from `timeout`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ScNeLEEDvtbyDg8MQwRob3
|
@venables resolved! |
What
getResolverfordid:webnow sendsredirect: "error"when fetching the DID document. A newfollowRedirects?: booleanoption (defaultfalse) restores the previous behavior.Why
allowedHttpHostsis applied to the URL built from the DID:The fetch that follows it used the platform default, which follows redirects. So the allowlist only governs the first hop: a
did:webresolved overhttpscan answer302 Location: http://…, and the resolver follows it to a scheme and host the check was there to reject.flowchart LR D["did:web:issuer.example"] --> C{"allowedHttpHosts<br/>allows http for this host?"} C -- no --> U["fetch https://issuer.example/.well-known/did.json"] U --> H{"host answers 302<br/>Location: http://internal/…"} H -->|before: redirect followed| BAD["request lands on the scheme<br/>and host the check rejected"] H -->|after: redirect error| OK["resolution fails as notFound"] classDef bad stroke-dasharray: 4 3 class BAD badBecause the redirect target is chosen by the DID's own host, this also makes resolution an outbound request an untrusted party controls. That matters for anything that resolves a DID before verifying a signature, which is the normal order: you need the document to check the signature. A server verifying an ACK-ID proof from an arbitrary issuer will fetch whatever that issuer's host points it at.
did:web documents are served directly at
/.well-known/did.json(or the configureddocPath), so refusing redirects costs legitimate resolution nothing. Deployments that genuinely sit behind a redirect can opt in.How
DidWebResolverOptionsgainsfollowRedirects?: boolean, documented with the reason, defaulting tofalse.fetchDidDocumentAtUrlpassesredirect: followRedirects ? "follow" : "error"alongside the existingmode: "cors".notFound) and the opt-in (redirect: "follow").Test plan
Found while building an ACK-ID identity-gated x402 demo, where a seller resolves buyer DIDs it has never seen before.
Implemented and tested by gpt 5.6-sol and fable 5, reviewed by me.
Summary by CodeRabbit
New Features
followRedirects: truesetting for resolvingdid:webdocuments.Documentation
followRedirects: true.