Fix issuer registry lookups blocked by CORS - #51
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe registry manager now delegates legacy and OIDF lookups to ChangesRegistry lookup
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The change enables CORS-proxied OIDF registry lookups, but legacy lookup failures can still hang resolution or discard valid matches, and configured federation endpoints with existing query parameters can be treated as unavailable. These bounded correctness and availability issues should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant RegistryManager
participant RegistryClient
participant corsProxyFetch
participant RegistryEndpoint
RegistryManager->>RegistryManager: Resolve cached registry list
RegistryManager->>RegistryClient: Create client with routing fetch wrapper
RegistryClient->>corsProxyFetch: Request OIDF registry data
corsProxyFetch->>RegistryEndpoint: Fetch proxied registry data
RegistryClient-->>RegistryManager: Return LookupResult
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 🔧 Checkov (3.3.11)package.jsonCheckov timed out on this file 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ce6cb43-985b-4337-ba93-131a3aa5041f
📒 Files selected for processing (2)
src/lib/registryManager.tstests/unit/registryManager.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
0marSalah
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/registryManager.ts (2)
205-208: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBuild the federation fetch URL with
URLinstead of string concatenation.
federation_fetch_endpointis an arbitrary URL from the entity configuration. If it already carries a query string,${endpoint}?sub=...produces two?separators and the request fails. The registry is then reported as unchecked even though it is reachable.URL.searchParamshandles both cases.♻️ Proposed fix
- const lookupRes = await corsProxyFetch({ - url: `${endpoint}?sub=${encodeURIComponent(did)}`, - signal - }) + const lookupUrl = new URL(endpoint) + lookupUrl.searchParams.set('sub', did) + const lookupRes = await corsProxyFetch({ + url: lookupUrl.toString(), + signal + })Note that the test fixture at
tests/unit/registryManager.test.tsline 247 asserts the exact concatenated form, so update the expected URL if you apply this change.
296-299: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound and isolate the legacy lookup.
RegistryClient.lookupIssuersForcallsfetchwithout a signal or timeout. Its per-registrycatchhandles fetch errors, but a pending fetch keepsPromise.allpending. A rejection from the legacy promise also rejectslookupDidand discards OIDF results. Add a deadline wrapper and return an empty legacy result on rejection.tests/unit/registryManager.test.ts (1)
233-260: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a stalled-hop case to
stubOidfFetchcoverage.The suite covers a rejected trust anchor, a 404, and a missing endpoint. It does not cover a proxy hop that never settles. That path is the reason
corsProxyFetchgained thesignalparameter in this cohort, and the sharedAbortControllerinlookupOidfRegistriesis the only thing that ends the lookup. A test with fake timers, a pending route, andadvanceTimersByTimeAsync(LOAD_TIMEOUT_MS)would pin the "unchecked after deadline" behavior.💚 Sketch
it('reports a stalled oidf hop as unchecked after the deadline', async () => { vi.useFakeTimers() stubOidfFetch({ [proxied(TRUST_ANCHOR_EC)]: () => new Promise<Response>((_, reject) => { // resolve never; the shared AbortController must end this hop }) }) const { registryManager, LOAD_TIMEOUT_MS } = await loadRegistryManager() const lookup = registryManager.lookupDid('did:key:z123') await vi.advanceTimersByTimeAsync(LOAD_TIMEOUT_MS) expect((await lookup).uncheckedRegistries).toEqual([OIDF_REGISTRY]) })The stubbed
fetchmust observe the signal and reject onabort, because the stub does not inherit native abort behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bda75090-7290-4a67-b1b5-82b8327c6786
📒 Files selected for processing (6)
src/app.config.tssrc/lib/corsProxy.tssrc/lib/registryManager.tssrc/lib/verify.tstests/unit/corsProxy.test.tstests/unit/registryManager.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Signed-off-by: Omar Salah <itsdevomar@gmail.com>
Signed-off-by: Omar Salah <itsdevomar@gmail.com>
Signed-off-by: Omar Salah <itsdevomar@gmail.com>
Signed-off-by: Omar Salah <itsdevomar@gmail.com>
7a29362 to
fbdca43
Compare
Signed-off-by: Dmitri Zagidulin <dzagidulin@gmail.com>
2 participants
Bumps
issuer-registry-clientto 4.1.0 for its injectablefetch, and uses it to routeoidfregistry URLs through the CORS proxy. The wallet's own oidf lookup comes out; everything else stays direct.Summary by CodeRabbit
New Features
Bug Fixes
Documentation