Add null guards and secure logging in proving internals #1653
Conversation
📝 WalkthroughWalkthroughThis PR adds runtime validation guards across the proving module to enforce required parameters (secret, uuid, circuitType) and enhances error handling in document processing, payload generation, socket handling, and URL resolution pipelines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mobile-sdk-alpha/src/proving/internal/documentProcessor.ts (1)
260-260: Inconsistent: unsafeas stringcast remains in the register branch.The PR goal is to remove unsafe casts, but line 260 still uses
secret as stringwhen callingisUserRegisteredWithAlternativeCSCA. This path could receive an undefined secret, which would be silently coerced.Consider adding a similar guard before this call for consistency with the disclose path:
🔧 Suggested fix
} else { + if (!secret || typeof secret !== 'string') { + throw new Error('Secret is required and must be a string'); + } const { isRegistered, csca } = await isUserRegisteredWithAlternativeCSCA(passportData, secret as string, {Then remove the
as stringcast after the guard is in place.
🤖 Fix all issues with AI agents
In `@packages/mobile-sdk-alpha/src/proving/internal/socketIOListener.ts`:
- Around line 120-125: Prettier flagged a formatting issue in the actorEvent
handling block around the PROVE_FAILURE branch; fix the formatting
(spacing/indentation) so the if-block and calls are compliant (ensure consistent
indentation and spacing around the if, the console.error(...) line, and the
getActor()?.send(result.actorEvent) line), then run Prettier or apply the
project's code formatter to the file to clear the CI warning; the relevant
symbols to look at are result.actorEvent, 'PROVE_FAILURE', console.error(...)
and getActor()?.send(...).
| if (result.actorEvent) { | ||
| if (result.actorEvent.type === 'PROVE_FAILURE') { | ||
| console.error('Proof generation/verification failed (status 3 or 5).'); | ||
| console.error(data); | ||
| console.error('Proof generation/verification failed (status 3 or 5). Error code:', result.stateUpdate?.error_code); | ||
| } | ||
| getActor()?.send(result.actorEvent); | ||
| } |
There was a problem hiding this comment.
Good: Logging only error_code instead of full payload.
This aligns with the coding guidelines about not logging sensitive data. The status payload may contain sensitive context, so logging just the error_code is appropriate.
However, the static analysis indicates a Prettier formatting issue on line 122 that's causing the CI warning.
🔧 Fix formatting
if (result.actorEvent) {
if (result.actorEvent.type === 'PROVE_FAILURE') {
- console.error('Proof generation/verification failed (status 3 or 5). Error code:', result.stateUpdate?.error_code);
+ console.error(
+ 'Proof generation/verification failed (status 3 or 5). Error code:',
+ result.stateUpdate?.error_code,
+ );
}
getActor()?.send(result.actorEvent);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (result.actorEvent) { | |
| if (result.actorEvent.type === 'PROVE_FAILURE') { | |
| console.error('Proof generation/verification failed (status 3 or 5).'); | |
| console.error(data); | |
| console.error('Proof generation/verification failed (status 3 or 5). Error code:', result.stateUpdate?.error_code); | |
| } | |
| getActor()?.send(result.actorEvent); | |
| } | |
| if (result.actorEvent) { | |
| if (result.actorEvent.type === 'PROVE_FAILURE') { | |
| console.error( | |
| 'Proof generation/verification failed (status 3 or 5). Error code:', | |
| result.stateUpdate?.error_code, | |
| ); | |
| } | |
| getActor()?.send(result.actorEvent); | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[warning] 122-122:
Replace 'Proof·generation/verification·failed·(status·3·or·5).·Error·code:',·result.stateUpdate?.error_code with ⏎············'Proof·generation/verification·failed·(status·3·or·5).·Error·code:',⏎············result.stateUpdate?.error_code,⏎··········
🤖 Prompt for AI Agents
In `@packages/mobile-sdk-alpha/src/proving/internal/socketIOListener.ts` around
lines 120 - 125, Prettier flagged a formatting issue in the actorEvent handling
block around the PROVE_FAILURE branch; fix the formatting (spacing/indentation)
so the if-block and calls are compliant (ensure consistent indentation and
spacing around the if, the console.error(...) line, and the
getActor()?.send(result.actorEvent) line), then run Prettier or apply the
project's code formatter to the file to clear the CI warning; the relevant
symbols to look at are result.actorEvent, 'PROVE_FAILURE', console.error(...)
and getActor()?.send(...).
Motivation
Description
_generateCircuitInputsfor theregisteranddisclosecases to ensuresecretis present and a string, and removed unsafeas stringcasts (packages/mobile-sdk-alpha/src/proving/internal/payloadGenerator.ts).uuidbefore building the submit request in_generatePayloadand throw a clear error if missing (packages/mobile-sdk-alpha/src/proving/internal/payloadGenerator.ts).validatingDocumentdisclosebranch before callingisUserRegisteredto avoid unsafe casts (packages/mobile-sdk-alpha/src/proving/internal/documentProcessor.ts).statushandler and changed PROVE_FAILURE logging to avoid printing the full status payload (log only the error code), then dispatchPROVE_ERRORif circuit type is missing (packages/mobile-sdk-alpha/src/proving/internal/socketIOListener.ts).getMappingKeyto handlepassport,id_card, and explicitly disallowaadhaarfor DSC circuits with a clear error (packages/mobile-sdk-alpha/src/proving/internal/websocketUrlResolver.ts).Testing
yarn workspace @selfxyz/mobile-sdk-alpha types(passed).yarn workspace @selfxyz/mobile-sdk-alpha build(build succeeded).yarn workspace @selfxyz/mobile-sdk-alpha testand all tests passed (Test Files: 28 passed; Tests: 250 passed).Codex Task
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.