fix(app): fetch protocol trees before the recovery registration check - #2272
fix(app): fetch protocol trees before the recovery registration check#2272seshanthS wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe change centralizes restored-document registration checks. Recovery screens now use this workflow, restore CSCA data only when available, and distinguish protocol-data failures from other recovery errors. ChangesRestored Document Recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to Account restore can still fail for passport or ID-card documents without an authority key identifier because a missing alternative CSCA value is dereferenced before the registration fallback runs. This is a high-impact correctness issue in the recovery flow and should be fixed before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98f2cefc-de45-41ee-a184-a8d36a73834e
📒 Files selected for processing (8)
app/src/proving/alternativeCSCA.tsapp/src/proving/checkRestoredDocumentRegistration.tsapp/src/proving/validateDocument.tsapp/src/screens/account/recovery/AccountRecoveryChoiceScreen.tsxapp/src/screens/account/recovery/RecoverWithPhraseScreen.tsxapp/tests/src/proving/checkRestoredDocumentRegistration.test.tsapp/tests/src/screens/account/recovery/AccountRecoveryChoiceScreen.test.tsxapp/tests/src/screens/account/recovery/RecoverWithPhraseScreen.test.tsx
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e59756e9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await protocolState[documentCategory].fetch_all(environment); | ||
| } catch (error) { | ||
| throw new ProtocolDataUnavailableError(documentCategory, { cause: error }); |
There was a problem hiding this comment.
When recovering an Aadhaar or KYC document, fetch_all also waits for deployed-circuit, DNS-mapping, and OFAC requests (packages/mobile-sdk-alpha/src/stores/protocolStore.ts), even though this check only needs the commitment tree and, for Aadhaar, public keys. If any unrelated endpoint rejects while those registration inputs are available, this catch converts the rejection to ProtocolDataUnavailableError, and both recovery screens prevent the user from recovering. Fetch the required slices directly or verify the required fields after a partial fetch_all failure.
Useful? React with 👍 / 👎.
| if (csca) { | ||
| await reStorePassportDataWithRightCSCA(passportDataParsed, csca); | ||
| } |
There was a problem hiding this comment.
For a registered Aadhaar document, isUserRegisteredWithAlternativeCSCA returns the matched public key through its csca field, so this newly reachable recovery path passes an Aadhaar public key to reStorePassportDataWithRightCSCA. That function treats the value as an X.509 CSCA certificate and calls parseCertificateSimple and brutforceSignatureAlgorithmDsc on passport-only fields, causing otherwise successful Aadhaar recovery to fall into the error handler. Restrict restoration to passport and ID-card categories, as validateDocument.ts already does.
Useful? React with 👍 / 👎.
| const readCommitmentTree = (category: DocumentCategory) => { | ||
| const serialized = serializeCommitmentTree( | ||
| getCommitmentTree(selfClient, category), | ||
| ); |
There was a problem hiding this comment.
If an Aadhaar commitment tree is already cached and the next identity-tree request fails, aadhaar.fetch_identity_tree catches the failure without clearing commitment_tree, while fetch_all can still resolve after the other requests finish. This read then accepts the stale tree as if the requested environment and refresh succeeded, potentially reporting a valid recovery phrase as unregistered—for example after switching between production and staging data or when the cached snapshot predates registration. Clear the slice before fetching or have the fetcher null the tree on failure so the helper raises ProtocolDataUnavailableError instead.
Useful? React with 👍 / 👎.
| const hasAlternativeCSCA = | ||
| Object.keys(readAlternativeCSCA(documentCategory)).length > 0; | ||
|
|
||
| if (!isMrzDocument || hasAlternativeCSCA) { | ||
| const { isRegistered, csca } = await isUserRegisteredWithAlternativeCSCA( |
There was a problem hiding this comment.
When the Aadhaar public-key endpoint successfully returns an empty list, this branch still calls isUserRegisteredWithAlternativeCSCA, whose Aadhaar path returns false immediately for empty keys, and the later fallback is restricted to MRZ documents. A valid Aadhaar document is therefore reported as unregistered even though isUserRegistered can derive and check its commitment directly from the restored document. Extend the single-commitment fallback to Aadhaar when no public keys are available.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/proving/checkRestoredDocumentRegistration.ts (1)
152-168: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnguarded
alternative_cscastill crashescheckRestoredDocumentRegistrationfor MRZ documents.
readAlternativeCSCAat Line 152-153 callsgetAlternativeCSCA(useProtocolStore, category)without a fallback. Forpassport/id_card,getAlternativeCSCAreturnsuseProtocolStore.getState()[docCategory].alternative_cscadirectly, and this field stays unset whenfetchProtocolDatatakes the no-AKI branch (Line 93), which only callsfetch_identity_treeand never populatesalternative_csca.At Line 167-168,
Object.keys(readAlternativeCSCA(documentCategory))then throws aTypeErroronnull. This happens afterreadCommitmentTree(documentCategory)succeeds (Line 158), because that check only validatescommitment_tree, notalternative_csca. The crash aborts the function before theisUserRegisteredfallback runs, so a restorable document fails with an unhandled exception instead of a controlled result.This is the same defect flagged in the earlier review on this file (Lines 167-168). The proposed fix was not applied.
🐛 Proposed fix
const readAlternativeCSCA = (category: DocumentCategory) => - getAlternativeCSCA(useProtocolStore, category); + getAlternativeCSCA(useProtocolStore, category) ?? {};
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 545ca37d-5e60-43d5-a5d5-566143356d19
📒 Files selected for processing (2)
app/src/proving/checkRestoredDocumentRegistration.tsapp/tests/src/proving/checkRestoredDocumentRegistration.test.ts
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
1 participant
Fixes SELF-3931.
Problem
Account restore fails for almost everyone. Mixpanel, last 30 days: 15 unique
users restored successfully, 701 hit
Cloud Restore Failed: Unknown Error, and84 more got a false "Passport Not Registered". Chronic for 4+ months, not a
regression.
Both recovery screens called
isUserRegisteredWithAlternativeCSCAwhile readingcommitment_treeandalternative_cscastraight out of the protocol store,without ever fetching them. Both default to
null, soLeanIMT.import(hash, null)threw a TypeError, or an empty commitment list produced a false "not registered".
The phrase path's telemetry confirms it:
error: TypeErroron 151 events.Nothing warmed the store on those paths —
SplashScreenonly callscheckAndUpdateRegistrationStateswhen a document hasisRegistered === undefined,which is false on any modern install.
Fix
New
app/src/proving/checkRestoredDocumentRegistration.ts, used by both screens,replacing ~20 lines of duplicated inline callbacks in each. It fetches first, then
checks:
fetchAllTreesAndCircuits.Without one → just
fetch_identity_tree, since onlyfetch_alternative_cscaconsumes the AKI and a blank ski would fire a pointless
/ski-pems/request.fetch_all(environment)directly, so they are no longerskipped for lacking a
dsc_parsed(which they never have).ProtocolDataUnavailableError. It is the only universally required input.alternative_cscaor absent AKI falls backto
isUserRegisteredon the document's own stored DSC/CSCA. Aadhaar seedsdocument_public_keyitself; kyc'spublic_keysis always null by design(
fetch_public_keyssets it unconditionally), so guarding either would rejectrestores that work today.
single-commitment path before giving up — this has strictly more recall than
either screen had before, and covers a stale
/ski-pems/response.commitment_treeis typedanyin the store; the endpoint returns it as a JSONstring today (verified against
tree.self.xyz/identity) andLeanIMT.importrequires that, so a structured value is serialized rather than reported as missing.
Also fixed
AccountRecoveryChoiceScreencalledreStorePassportDataWithRightCSCA(data, csca as string)unconditionally, butcscais null for registered KYC documents and the callee dereferences it viaparseCertificateSimple. Now guarded, matching the phrase screen.the failing branch was unknowable. All four sites now report a
reason—restore_failed,protocol_data_unavailable,backup_download_failed,unexpected_error. That last split matters for SELF-3934: it separates "nobackup / OAuth failed" from "registration check failed".
getAlternativeCSCAmoved toapp/src/proving/alternativeCSCA.ts.Importing it from
validateDocumentpulled@/services/analyticsinto thescreens' module graph, which broke their tests at import time.
validateDocumentre-exports it, so existing importers are unchanged.throw new Error('KYC is not supported yet')from bothscreens —
validate.tsreturns early for kyc beforegetAltCSCAis consulted.Deliberately out of scope
packages/mobile-sdk-alpha/src/proving/recoveryValidation.tshas the samemissing-fetch bug on the webview path. Separate issue — the app does not use it,
and fixing it here would change webview behaviour in an app PR.
validateDocument.tsskipping every aadhaar/kyc document for lacking an AKI.protocolStore.ts's error-swallowing fetchers or tofetchAllTreesAndCircuits's signature; both ripple intoprovingMachine.Validation
pnpm nice(0 errors),pnpm types,pnpm test— 111 suites / 1231 testspassing. Every file these tests depend on is identical between this base and
dev.19 new tests: 15 on the helper (fetch-before-check ordering, missing-tree error,
both fallback paths, aadhaar/kyc not skipped, empty-string AKI, staging env,
wrapped fetch rejection, tree serialization) and 4 on
AccountRecoveryChoiceScreen, which had no test file at all.Post-merge, watch
Cloud Restore SuccessvsCloud Restore Failed: *uniquesagainst the 15-vs-785 baseline.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests