feat(did): add optional fetch timeout to the did:web resolver - #129
feat(did): add optional fetch timeout to the did:web resolver#129EfeDurmaz16 wants to merge 3 commits into
Conversation
|
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)
WalkthroughThe Changesdid:web fetch timeout
Estimated code review effort: 2 (Simple) | ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/did/src/did-resolvers/web-did-resolver.ts`:
- Around line 70-78: Update getResolver() to validate timeout before calling
AbortSignal.timeout(), rejecting negative, non-integer, non-finite, and
non-number values explicitly rather than allowing them to become notFound DID
errors. Preserve valid timeout behavior and add tests covering each
invalid-value category.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a295354-8cf0-4ada-af59-bed6ec27927b
📒 Files selected for processing (3)
.changeset/did-web-resolver-fetch-timeout.mdpackages/did/src/did-resolvers/web-did-resolver.test.tspackages/did/src/did-resolvers/web-did-resolver.ts
7503cd5 to
f288d45
Compare
|
@venables when you have a moment, would appreciate a review. Small backward-compatible hardening (optional fetch timeout on the did:web resolver) I hit while building an ACK-ID + x402 demo. CodeRabbit's note is addressed; CI is waiting on a maintainer 'approve and run' for the fork branch. |
domleboss97
left a comment
domleboss97
left a comment
There was a problem hiding this comment.
😎 some comments, but looks good!
| expect(mockFetch).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("passes an abort signal built from the configured timeout", async () => { |
There was a problem hiding this comment.
not related to this pr in particular, but i think some test helpers could make each of these tests less verbose
There was a problem hiding this comment.
Agreed it would help. Left it out since it touches the pre-existing tests too; happy to do a small follow-up PR.
| * @default [] | ||
| */ | ||
| allowedHttpHosts?: string[] | ||
| /** |
There was a problem hiding this comment.
this comment seems a bit verbose - it seems like we could drop the rationale sentence and the mention of previous behavior, which one could see through changesets anyways. could also add a @default annotation
| docPath = DEFAULT_DOC_PATH, | ||
| fetch = globalThis.fetch, | ||
| allowedHttpHosts = DEFAULT_ALLOWED_HTTP_HOSTS, | ||
| timeout, |
There was a problem hiding this comment.
might make sense now that we're adding this to have a default value, perhaps 5 seconds or something?
There was a problem hiding this comment.
Done in 814f8d8: defaults to 5000ms now. While double checking the range for this I noticed values above 2^31-1 clamp to a 1ms timer in Node, and above 2^32 they throw inside the fetch, both surfacing as a misleading notFound.
So f1e9670 caps the range at 2^31 -1 . Passing the max (about 24.8 days) works as an effective opt-out. The changeset covers both.
| allowedHttpHosts = DEFAULT_ALLOWED_HTTP_HOSTS, | ||
| timeout, | ||
| }: DidWebResolverOptions = {}): { web: DIDResolver } { | ||
| // Fail fast on a bad timeout: `AbortSignal.timeout` throws on non-positive, |
There was a problem hiding this comment.
very minor - AbortSignal.timeout(0) does not actually throw
There was a problem hiding this comment.
Right, it doesn't throw natively. getResolver still rejects 0 with a RangeError since a 0ms timeout would abort every request before it starts. Fixed the comment in 814f8d8 so it no longer claims the API throws on it.
| // non-integer or non-finite values, and that would otherwise surface as a | ||
| // misleading `notFound` resolution error rather than a programmer error. | ||
| if (timeout !== undefined && (!Number.isInteger(timeout) || timeout <= 0)) { | ||
| throw new TypeError("`timeout` must be a positive integer (milliseconds)") |
There was a problem hiding this comment.
nit - RangeError might be more accurate here. the type is actually correct.
There was a problem hiding this comment.
Agreed, switched in 814f8d8, tests updated.
Address review feedback: - timeout now defaults to 5000ms instead of being opt-in - out-of-range values throw RangeError instead of TypeError - correct the validation rationale: AbortSignal.timeout(0) does not throw, 0 is rejected because it would abort every request before it starts - trim the option docs and add @default - update the changeset (the previous backward-compat wording no longer held) - tests: default-signal assertion, RangeError, exact-init updates
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/did/src/did-resolvers/web-did-resolver.ts`:
- Around line 47-54: Preserve an omitted timeout as undefined in the resolver
configuration and request flow, rather than applying a 5000ms default. Update
the timeout handling around getDidResolver/getResolver so an AbortSignal is
created and passed only when timeout is explicitly configured; keep configured
timeout behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cddb25b4-2032-4768-90c3-bbc85cfd914b
📒 Files selected for processing (3)
.changeset/did-web-resolver-fetch-timeout.mdpackages/did/src/did-resolvers/web-did-resolver.test.tspackages/did/src/did-resolvers/web-did-resolver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/did/src/did-resolvers/web-did-resolver.test.ts
Values in (2^31-1, 2^32-1] pass AbortSignal.timeout in Node but the timer clamps to 1ms (TimeoutOverflowWarning), aborting every request; values above 2^32-1 throw at fetch time. Both surfaced as a misleading notFound, which is exactly what the fail-fast guard exists to prevent. Reject timeouts above 2147483647ms up front and document that the maximum is an effective opt-out.
2 participants
What
Adds an optional
timeout(ms) toDidWebResolverOptions/getResolverforthe
did:webresolver. When set, the resolver passesAbortSignal.timeout(timeout)to the underlying fetch.Why
Resolving a
did:webDID fetches the host named in the DID. TodayfetchDidDocumentAtUrlcallsfetch(url, { mode: "cors" })with noAbortSignal, so an unresponsive or slow host hangs the resolverindefinitely. The resolver's own header comment says it exists for "additional
checks and more control for fetching and resolution", so a bounded fetch fits
its purpose. This is resource-exhaustion hardening, not an SSRF fix (did:web
inherently fetches the domain named in the DID).
Change
timeout?: numberonDidWebResolverOptionsgetResolver->resolve->fetchDidDocumentAtUrlTests
Three new tests in
web-did-resolver.test.ts: the resolver builds the signalfrom the configured timeout (
AbortSignal.timeoutis called with the value),passes no signal when the option is omitted, and surfaces an aborted/timed-out
fetch as the standard
notFoundresolution error.pnpm --filter @agentcommercekit/did test-> 73 passing. Changeset included(minor). A response-size cap is a natural follow-up but needs body streaming,
so it is out of scope here.
AI assistance disclosure
Per the repository AI policy: implemented and tested by gpt 5.6-sol and fable 5, then reviewed and understood by me. I can explain the implementation and its trade-offs without AI aid. Tests, typecheck, and formatting pass locally against the package suite.
Summary by CodeRabbit
timeoutsetting fordid:webresolution requests (defaults to 5000ms).AbortSignalto the fetch call).timeoutvalues now fail fast with a clearRangeError(valid range:1..2147483647).notFoundresolution result with timeout-related error details.