Repository navigation
fix(did): apply the resolver timeout to did:jwks fetches - #261
furkan3152 wants to merge 3 commits into
Conversation
getDidResolver() wraps fetch for the did:jwks resolver to inject the redirect policy, but passed no AbortSignal, so the webOptions.timeout (default 5000ms) added for did:web in agentcommercekit#129 never bounded did:jwks. A did:jwks host that stalls its response held resolution, and any verification waiting on it, open with no bound from this library. Abort each did:jwks fetch (JWKS, OpenID configuration, jwks_uri) after the same timeout.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Timeout configuration and fetch propagation packages/did/src/did-resolvers/web-did-resolver.ts, packages/did/src/did-resolvers/get-did-resolver.ts, packages/did/src/did-resolvers/get-did-resolver.test.ts, .changeset/did-jwks-fetch-timeout.md |
The resolver exports and uses a shared 5000 ms default. did:jwks fetches use the configured timeout or that default and combine the timeout signal with any caller-provided signal. Timed-out internal errors return as notFound. Tests cover abort signals, default timeouts, and a later successful resolution after a timeout. The changeset documents the timeout and custom-fetch signal requirement. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: efedurmaz16
Merge Risk: 
7ec4a
-
A later discovery failure can be reported with the wrong error when an earlier fetch’s timeout expires. Clean up the listener before merging, or accept this bounded risk.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: applying the resolver timeout to did:jwks fetches. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
did-resolver's in-memory cache stores every result except notFound, and the did:jwks resolver reports any fetch failure as internalError. With cache: true, a transient timeout was cached and the DID never resolved again. Report a timeout as notFound, as did:web does.
The timeout listener fired when a fetch's timer expired even if that fetch had already settled, so a later non-timeout failure in the same resolution could be reported as notFound. Check the timeout signal when the fetch fails instead.
getDidResolver()applieswebOptions.timeout(default 5000ms, added in #129) todid:webfetches but not todid:jwks. Thedid:jwksresolver gets its own fetch wrapper that only sets the redirect policy and passes noAbortSignal, so resolvingdid:jwks:<host>waits on that host with no bound. This sits underverifyJwt,verifyParsedCredential,verifyPaymentReceiptand the A2A verifiers, so a token whoseissis adid:jwksDID on a stalling host holds verification open, across up to three requests (JWKS, OpenID configuration,jwks_uri).Repro: run a local server that accepts connections and never responds. With the default
getDidResolver(),did:web:127.0.0.1%3A<port>returnsnotFoundafter ~5s, whiledid:jwks:127.0.0.1%3A<port>is still pending after 15s. With this change it fails with a timeout after ~5s.Changes:
get-did-resolver.ts: thedid:jwksfetch wrapper setsAbortSignal.timeout(timeout)(combined with a caller signal viaAbortSignal.any), usingwebOptions.timeout ?? DEFAULT_TIMEOUT_MS.web-did-resolver.ts: the 5000ms default is a sharedDEFAULT_TIMEOUT_MS.@agentcommercekit/didpatch changeset.pnpm run check,pnpm run knipandpnpm run check:formatpass. Independent of #193/#200, which add a URL policy to the same path but no timeout.AI usage: Claude Code assisted with this change; I reviewed it and the test results.
Summary by CodeRabbit
did:jwksresolution now uses the configured fetch timeout, defaulting to 5 seconds, for JWKS and OpenID configuration requests. Requests that exceed the timeout are aborted, and resolution reports the DID as not found.