Fix/eid onboarding #415
Fix/eid onboarding #415
Conversation
|
Warning Rate limit exceeded@sosweetham has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant GS as GlobalState
participant KS as KeyService
participant Store
participant KMF as KeyManagerFactory
App->>GS: create()
GS->>KS: new KeyService(store)
KS->>Store: load persisted contexts
GS->>KS: initialize()
KS->>Store: check READY_KEY
GS->>App: ready with keyService
App->>KS: getManager(keyId, context)
KS->>KS: compute cacheKey
alt Cache miss
KS->>KMF: getKeyManagerForContext(context)
KMF-->>KS: KeyManager instance
KS->>KS: cache manager
KS->>KS: persistContext()
KS->>Store: save context entry
end
KS-->>App: KeyManager
App->>KS: ensureKey(keyId, context)
KS->>KS: getManager(keyId, context)
KS->>KS: check key exists
alt Key missing
KS->>KS: generate key
note over KS: return created=true
end
KS->>KS: touchContext (update lastUsed)
KS-->>App: { manager, created }
sequenceDiagram
participant User
participant Page as Scan-QR Page
participant Logic as scanLogic
participant Drawer as Drawer Component
participant GS as GlobalState
User->>Page: Mount page
Page->>Logic: initialize()
Logic->>GS: check authentication/deep-link
Logic-->>Page: stores & actions ready
User->>Page: Scan QR code
Page->>Logic: startScan()
Logic->>Page: update scanning store
alt QR contains auth request
Logic->>Page: update signingData, open AuthDrawer
User->>Drawer: click Confirm
Drawer->>Page: onConfirm callback
Page->>Logic: handleAuth(data)
Logic->>GS: sign payload via keyService
Logic->>GS: submit to backend
Logic->>Page: update success state
Page->>Drawer: show success
else QR contains sign request
Logic->>Page: open SigningDrawer
User->>Drawer: click Sign
Page->>Logic: handleSignVote(data)
Logic->>GS: sign via keyService
Logic->>Page: show signing success
end
User->>Drawer: click OK/close
Page->>Logic: closeDrawer()
Logic->>Page: reset state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 3
🧹 Nitpick comments (11)
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (2)
33-38: Consider the necessity of the globalState guard.The guard check redirects to
/onboardingifglobalStateis missing, butonMount(line 56) throws an error ifglobalStateis undefined. This creates an inconsistency in error handling strategy.If handlers can be invoked before
onMountcompletes (a race condition), these guards are valuable. Otherwise, consider whether the defensive checks are needed or if the handlers should rely on theonMountguard.
42-47: Same globalState guard pattern as handleSkip.This follows the same defensive pattern as
handleSkip. Consider aligning the error handling strategy across both handlers andonMountfor consistency.infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte (1)
109-109: Consider centralizing the KEY_ID constant.The constant
KEY_ID = "default"is duplicated across multiple files (verify, onboarding). Consider extracting this to a shared constants file or the KeyService module to ensure consistency and reduce duplication.infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte (1)
26-26: KEY_ID constant duplicated across files.As noted in the verify page,
KEY_ID = "default"appears in multiple files. Consider centralizing this constant to avoid potential inconsistencies if the value needs to change.infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/RevealDrawer.svelte (1)
1-30: Consider extracting the shared state sync pattern.Both
LoggedInDrawerandRevealDraweruse identical state synchronization logic (lines 19-29). Consider extracting this into a reusable Svelte action or composable to reduce duplication and ensure consistency.Example approach:
// useDrawerSync.svelte.ts export function useDrawerSync(isOpen: boolean, onOpenChange: (value: boolean) => void) { let internalOpen = $state(isOpen); let lastReportedOpen = $state(internalOpen); $effect(() => { if (isOpen !== internalOpen) internalOpen = isOpen; }); $effect(() => { if (internalOpen !== lastReportedOpen) { lastReportedOpen = internalOpen; onOpenChange?.(internalOpen); } }); return { internalOpen }; }infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/AuthDrawer.svelte (1)
79-86: Potential null reference in conditional message.The conditional message at line 82 interpolates
{platform}, which can benullorundefined(based on the prop type at line 8). Svelte will render this as an empty string, but it may result in awkward text like "After confirmation, you may return to and continue there" when platform is null.Consider adding a fallback:
<p class="text-sm text-gray-600"> - After confirmation, you may return to {platform} and continue + After confirmation, you may return to {platform ?? "the platform"} and continue there </p>infrastructure/eid-wallet/src/lib/global/state.ts (1)
86-111: Review error handling in nested promise chains.The setter accepts
Promise<boolean>and chains multiple async operations. In the Promise branch (lines 88-102), ifthis.#store.set()rejects, the error is caught and logged. However, the subsequentthis.keyService.setReady()call will not execute, potentially leaving the keyService in an inconsistent state where the store shows onboarding complete but keyService readiness wasn't updated.Consider ensuring both operations complete or both fail:
set isOnboardingComplete(value: boolean | Promise<boolean>) { if (value instanceof Promise) { value .then((resolved) => { - this.#store - .set("isOnboardingComplete", resolved) - .then(() => this.keyService.setReady(resolved)) - .catch((error) => { - console.error( - "Failed to set onboarding status:", - error, - ); - }); + return Promise.all([ + this.#store.set("isOnboardingComplete", resolved), + this.keyService.setReady(resolved) + ]); }) .catch((error) => { console.error("Failed to set onboarding status:", error); }); } else { - this.#store - .set("isOnboardingComplete", value) - .then(() => this.keyService.setReady(value)) - .catch((error) => { - console.error("Failed to set onboarding status:", error); - }); + Promise.all([ + this.#store.set("isOnboardingComplete", value), + this.keyService.setReady(value) + ]).catch((error) => { + console.error("Failed to set onboarding status:", error); + }); } }This ensures both store and keyService updates succeed together or fail together, maintaining consistency.
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (3)
320-346: Simplify redirect method cascade.The code attempts three different redirect methods sequentially (lines 320-346), with each method wrapped in its own try/catch. In modern browsers,
window.location.hrefis the standard and widely supported method. The cascade adds unnecessary complexity and may mask actual errors.Consider simplifying to use a single method with proper error handling:
- try { - window.location.href = data.redirect; - } catch (error1) { - console.log( - "Method 1 failed, trying method 2:", - error1, - ); - try { - window.location.assign(data.redirect); - } catch (error2) { - console.log( - "Method 2 failed, trying method 3:", - error2, - ); - try { - window.location.replace(data.redirect); - } catch (error3) { - console.log( - "Method 3 failed, using fallback:", - error3, - ); - throw new Error( - "All redirect methods failed", - ); - } - } - } + try { + window.location.href = data.redirect; + } catch (error) { + console.error("Redirect failed:", error); + loggedInDrawerOpen.set(true); + startScan(); + }If the standard method fails, show the drawer instead of trying alternatives that are unlikely to work.
624-886: Consider refactoring large function into smaller units.The
handleBlindVotefunction is 262 lines long and handles multiple concerns: validation, voter registration, cryptographic vote generation, hex conversion, and submission. This makes it difficult to test, maintain, and reason about.Consider extracting logical units into separate functions:
async function validateBlindVoteData(selectedOption: number | null, signingData: SigningData | null) { ... } async function getVoterPublicKey(vault: Vault, keyService: KeyService) { ... } async function checkExistingVote(platformUrl: string, pollId: string, voterId: string) { ... } async function registerVoter(platformUrl: string, pollId: string, voterId: string) { ... } async function generateBlindVote(voterId: string, pollId: string, pollDetails: PollDetails, selectedOption: number) { ... } async function submitBlindVote(redirectUrl: string, payload: BlindVotePayload) { ... }This would improve testability and make the control flow clearer.
812-812: Verify localStorage storage limits for vote data.The function stores vote data in localStorage (line 812) which typically has a 5-10MB limit. The vote data includes commitments and anchors which could be large depending on the number of options. If the limit is exceeded,
setItemwill throw a QuotaExceededError.Add error handling:
- localStorage.setItem(`blindVote_${pollId}`, jsonString); + try { + localStorage.setItem(`blindVote_${pollId}`, jsonString); + } catch (error) { + if (error instanceof DOMException && error.name === 'QuotaExceededError') { + throw new Error('Unable to store vote data: storage quota exceeded'); + } + throw error; + }infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
174-183: Verify edge case handling in #parseCacheKey.The method splits on
:and rejoins the rest to handle keyIds that may contain colons. This is correct for the stated cache key format"${context}:${keyId}", but relies on KeyServiceContext values never containing colons.The current KeyServiceContext values ("onboarding", "signing", "verification", "pre-verification") are safe, but consider adding validation or documentation:
#parseCacheKey(cacheKey: string): { context: KeyServiceContext; keyId: string; } { + // Assumes context values never contain ':' character const [context, ...rest] = cacheKey.split(":"); return { context: context as KeyServiceContext, keyId: rest.join(":"), }; }Alternatively, use a different separator like
|or::that's less likely to appear in keyIds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/index.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/state.ts(3 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/AuthDrawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/LoggedInDrawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/RevealDrawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/SigningDrawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(6 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte(6 hunks)infrastructure/eid-wallet/src/routes/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T05:54:12.858Z
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
Applied to files:
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte
🧬 Code graph analysis (3)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
infrastructure/eid-wallet/src/lib/services/NotificationService.ts (1)
requestPermissions(43-61)
infrastructure/eid-wallet/src/lib/global/state.ts (2)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
KeyService(21-184)infrastructure/eid-wallet/src/lib/global/controllers/security.ts (1)
value(167-177)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
infrastructure/eid-wallet/src/lib/global/index.ts (2)
KeyServiceContext(4-4)KeyService(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
infrastructure/eid-wallet/src/lib/global/index.ts (1)
3-4: LGTM! Clean export additions.The new KeyService exports follow the existing pattern and provide a clear public API for the centralized key management functionality.
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte (1)
36-42: LGTM! Proper onboarding state reset.The addition of
globalState.isOnboardingComplete = falsecorrectly resets the onboarding status when the wallet is deleted, ensuring users are properly redirected through the onboarding flow.infrastructure/eid-wallet/src/routes/+page.svelte (1)
24-34: LGTM! Graceful error handling for onboarding status.The try/catch approach safely defaults to
onboardingComplete = falseif the status check fails, ensuring users are redirected to onboarding rather than encountering a hard error. The error logging provides debugging visibility while maintaining a smooth user experience.infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/LoggedInDrawer.svelte (1)
1-64: LGTM! Well-structured drawer component.The component follows a clean pattern:
- Clear prop API for parent communication
- Proper state synchronization between internal and external open state
- Conditional rendering based on redirect and platform props
The reactive statements correctly handle bidirectional state sync without causing infinite loops.
infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte (5)
149-151: Clean context selector implementation.The
getKeyContext()function provides a clear, context-specific identifier for the verification flow. This enhances traceability in the KeyService operations.
154-168: LGTM! Improved hardware key check with proper cleanup.The refactored
checkHardwareKeySupportnow:
- Uses the centralized
globalState.keyService.isHardwareAvailable()- Includes a guard for undefined globalState
- Always sets
hardwareKeyCheckComplete = truein the finally block, ensuring UI state is updated even on errors
170-182: Good refactoring to centralized key management.The
initializeKeyManagerfunction now properly delegates toglobalState.keyService.getManager()with the verification context, improving consistency across the codebase.
184-202: LGTM! Idempotent key provisioning.The new
ensureKeyForVerificationfunction provides idempotent key creation:
- Creates a key if it doesn't exist
- Returns existing key if already present
- Logs the outcome for debugging
This is a more robust approach than checking and creating separately.
204-219: Public key retrieval properly centralized.The refactored function now uses
globalState.keyService.getPublicKey()with the verification context, ensuring consistent key access patterns across the application.infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte (5)
50-52: Context-aware key management.The
getKeyContext()function properly returns different contexts based on thepreVerifiedstate, enabling appropriate key isolation between pre-verification and onboarding flows.
54-65: Consistent KeyService integration.The refactored
initializeKeyManagerfollows the same pattern as the verify page, usingglobalState.keyService.getManager()with the computed context. Good consistency across auth flows.
67-85: Idempotent key provisioning for onboarding.Similar to the verify page,
ensureKeyForContextprovides idempotent key creation with proper logging. The consistent pattern across pages improves maintainability.
87-104: Improved public key retrieval with lazy initialization.The refactored function now:
- Guards against undefined globalState
- Lazily initializes keyManager if needed
- Uses the centralized
globalState.keyService.getPublicKey()with context- Includes comprehensive error handling
151-151: Good catch on the typo fix.Correcting "pre-verificaiton" to "pre-verification" improves professionalism in user-facing error messages.
infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/RevealDrawer.svelte (1)
32-113: LGTM! Comprehensive reveal flow UI.The component effectively handles multiple states:
- Default reveal form with poll information
- Loading state during reveal operation
- Error display with clear messaging
- Success state showing revealed vote details
The informative note about local reveal (lines 76-81) sets clear expectations for users.
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
170-207: Review security and error handling in blind vote URL parsing.The blind vote URL parsing (lines 170-207) decodes base64 data from URL parameters and parses it as JSON. While there's a try/catch block, consider the following:
Input validation: The code checks for presence of parameters (lines 180-184) but doesn't validate the structure or content of
decodedDatabeyond checkingtype === "blind-vote".Base64 decode error handling:
atob()can throw on invalid base64, which is caught, but the fallback tohandleAuthRequest(content)at line 207 may process untrusted content.Verify that
handleAuthRequestproperly validates and sanitizes the content URL, as it becomes a fallback for failed blind vote parsing. Consider adding more specific validation:if (sessionId && base64Data && redirectUri && platformUrl) { const decodedString = atob(decodeURIComponent(base64Data)); const decodedData: SigningData = JSON.parse(decodedString); // Add validation if (decodedData.type === "blind-vote" && decodedData.pollId && typeof decodedData.pollId === 'string') { handleBlindVotingRequest(decodedData, platformUrl, redirectUri); return; } }
infrastructure/eid-wallet/src/routes/(app)/scan-qr/components/SigningDrawer.svelte
Show resolved
Hide resolved
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/env.d.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
infrastructure/eid-wallet/src/env.d.ts (1)
PUBLIC_PLATFORM_URL(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
478-513: Validate and normalize platform URL before fetch.Lines 489-490 interpolate
platformUrldirectly into the fetch URL without validation. WhenplatformUrlisnullor empty, this constructs an invalid URL like"null/api/polls/123", causing the fetch to fail before the drawer even opens.This is the unresolved issue flagged in the past review.
Apply this diff to validate and normalize the platform URL:
async function handleBlindVotingRequest( blindVoteData: SigningData, platformUrl: string | null, redirectUri: string | null, ) { try { const pollId = blindVoteData.pollId; if (!pollId) { throw new Error("No poll ID provided in blind vote data"); } + const resolvedPlatformUrl = + platformUrl?.trim() || PUBLIC_PLATFORM_URL?.trim(); + if (!resolvedPlatformUrl) { + throw new Error( + "Platform URL missing from blind vote request and no PUBLIC_PLATFORM_URL configured.", + ); + } + + let normalizedPlatformUrl: string; + try { + normalizedPlatformUrl = new URL(resolvedPlatformUrl) + .toString() + .replace(/\/+$/, ""); + } catch (error) { + throw new Error( + `Invalid platform URL "${resolvedPlatformUrl}": ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + const pollResponse = await fetch( - `${platformUrl}/api/polls/${pollId}`, + `${normalizedPlatformUrl}/api/polls/${pollId}`, ); if (!pollResponse.ok) { throw new Error("Failed to fetch poll details"); } const pollDetails = await pollResponse.json(); signingData.set({ pollId: pollId, pollDetails: pollDetails, redirect: redirectUri ?? undefined, sessionId: blindVoteData.sessionId, - platform_url: platformUrl ?? undefined, + platform_url: normalizedPlatformUrl, });Based on learnings
🧹 Nitpick comments (3)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (3)
813-813: Consider secure storage for blind vote data.Line 813 stores blind vote data (including the voter's choice, commitments, and anchors) in
localStorage, which is accessible to any script on the same origin. If an XSS vulnerability exists in the application, this sensitive data could be compromised.For a Tauri application, consider using the secure storage plugin (
@tauri-apps/plugin-storeor encrypted storage) to protect blind vote data at rest, especially if the app loads any external content or third-party scripts.
841-849: Consider clarifying redirect URL construction logic.Lines 841-845 construct the redirect URL with a complex ternary expression that's difficult to parse. The logic is correct (checks if redirect is absolute, else concatenates with platform_url), but readability could be improved.
Apply this diff for clearer logic:
- const redirectUrl = currentSigningData.redirect?.startsWith("http") - ? currentSigningData.redirect - : `${currentSigningData.platform_url ?? ""}${ - currentSigningData.redirect || "" - }`; - - if (!redirectUrl) { + let redirectUrl = currentSigningData.redirect || ""; + if (!redirectUrl) { + throw new Error("Redirect URL not provided"); + } + + // If redirect is relative, prepend platform URL + if (!redirectUrl.startsWith("http")) { + const platformUrl = currentSigningData.platform_url; + if (!platformUrl) { + throw new Error("Platform URL required for relative redirect"); + } + redirectUrl = `${platformUrl}${redirectUrl}`; + } + + if (!redirectUrl || !redirectUrl.startsWith("http")) { throw new Error("Redirect URL not provided"); }
312-347: Consider simplifying navigation fallback logic.Lines 312-347 attempt three different navigation methods (
window.location.href,window.location.assign,window.location.replace) with nested try-catch blocks. This defensive approach is very robust, but modern browsers reliably supportwindow.location.hreffor navigation.Unless you've encountered specific browser compatibility issues, the fallback chain could be simplified:
try { window.location.href = data.redirect; - } catch (error1) { - console.log( - "Method 1 failed, trying method 2:", - error1, - ); - try { - window.location.assign(data.redirect); - } catch (error2) { - console.log( - "Method 2 failed, trying method 3:", - error2, - ); - try { - window.location.replace(data.redirect); - } catch (error3) { - console.log( - "Method 3 failed, using fallback:", - error3, - ); - throw new Error( - "All redirect methods failed", - ); - } - } - } + } catch (error) { + console.error("Navigation failed:", error); + throw new Error("Failed to redirect after authentication"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T10:34:52.509Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 415
File: infrastructure/eid-wallet/src/env.d.ts:8-8
Timestamp: 2025-11-13T10:34:52.509Z
Learning: In infrastructure/eid-wallet, PUBLIC_PLATFORM_URL should not be added to .env.example or configured as a static environment variable. The platform URL is extracted dynamically through URI parsing according to the protocol specification, and all fallbacks for platform URL are being removed.
Applied to files:
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
infrastructure/eid-wallet/src/env.d.ts (1)
PUBLIC_PLATFORM_URL(8-8)
🔇 Additional comments (4)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (4)
1-110: LGTM! Well-structured type definitions.The imports and type definitions provide a clean, comprehensive public API for the scan logic module. The separation of stores and actions into distinct interfaces promotes maintainability.
1092-1120: Excellent URL validation implementation! ✅Lines 1092-1120 properly address the past review concern about the hardcoded platform URL. The code now:
- Falls back to
PUBLIC_PLATFORM_URLwhen needed (lines 1092-1094)- Validates the URL is non-empty (lines 1096-1101)
- Validates URL format with clear error messages (lines 1103-1120)
- Normalizes trailing slashes for consistent API calls (line 1109)
This is exactly the validation pattern that should be applied in
handleBlindVotingRequestas well.
1187-1264: LGTM! Well-structured initialization with proper cleanup.The
initializefunction properly:
- Checks authentication before proceeding (lines 1191-1204)
- Registers event listeners for deep link events (lines 1233-1235)
- Handles persisted deep link data from session storage (lines 1237-1253)
- Returns a cleanup function to remove event listeners (lines 1259-1263)
This follows React/Svelte lifecycle patterns correctly and prevents memory leaks.
670-694: Review W3ID fallback logic for blind voting.Lines 691-694 fall back to using
vault.enameif W3ID retrieval fails:voterPublicKey = vault.ename || "unknown_public_key";This fallback might compromise the cryptographic integrity of the blind voting system. If the W3ID generation fails, the blind vote should probably fail rather than use a fallback identifier.
Verify whether the blind voting protocol requires proper W3ID generation, or if using
vault.enameas a fallback is acceptable. If W3ID is cryptographically required, remove the fallback:} catch (error) { console.error("Failed to get W3ID using KeyManager:", error); - voterPublicKey = vault.ename || "unknown_public_key"; + throw new Error("Failed to retrieve voter public key for blind voting"); }
| scan({ formats, windowed }) | ||
| .then((res) => { | ||
| scannedData.set(res); | ||
| const content = res.content; | ||
| if (content.startsWith("w3ds://sign")) { | ||
| handleSigningRequest(content); | ||
| } else if (content.startsWith("w3ds://reveal")) { | ||
| handleRevealRequest(content); | ||
| } else if (content.includes("/blind-vote")) { | ||
| try { | ||
| const url = new URL(content); | ||
| const sessionId = url.searchParams.get("session"); | ||
| const base64Data = url.searchParams.get("data"); | ||
| const redirectUri = | ||
| url.searchParams.get("redirect_uri"); | ||
| const platformUrl = | ||
| url.searchParams.get("platform_url"); | ||
|
|
||
| if ( | ||
| sessionId && | ||
| base64Data && | ||
| redirectUri && | ||
| platformUrl | ||
| ) { | ||
| const decodedString = atob( | ||
| decodeURIComponent(base64Data), | ||
| ); | ||
| const decodedData: SigningData = | ||
| JSON.parse(decodedString); | ||
|
|
||
| if (decodedData.type === "blind-vote") { | ||
| handleBlindVotingRequest( | ||
| decodedData, | ||
| platformUrl, | ||
| redirectUri, | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error( | ||
| "Error parsing blind voting HTTP URL:", | ||
| error, | ||
| ); | ||
| } | ||
| handleAuthRequest(content); | ||
| } else { | ||
| handleAuthRequest(content); | ||
| } |
There was a problem hiding this comment.
Consider explicit error handling for unrecognized QR patterns.
The fallback to handleAuthRequest (lines 208, 210) for any unrecognized QR content or parsing errors might mask issues. If a QR code contains /blind-vote but fails to parse, or if it's an entirely unrecognized format, treating it as an auth request could lead to confusing errors.
Consider adding explicit logging or error handling for unrecognized patterns:
} catch (error) {
console.error(
"Error parsing blind voting HTTP URL:",
error,
);
+ // Explicitly handle parse failure
+ blindVoteError.set("Invalid blind vote QR code format");
+ return;
}
- handleAuthRequest(content);
} else {
handleAuthRequest(content);
}Alternatively, validate that the content matches expected auth URL patterns before calling handleAuthRequest.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (3)
162-210: Consider explicit error handling for unrecognized QR patterns.The fallback to
handleAuthRequestfor unrecognized QR content or parsing errors might mask issues. If a QR code fails to parse or is an unrecognized format, treating it as an auth request could lead to confusing errors downstream.
366-375: Validate redirect parameter before URL construction.Line 369 creates a URL from
url.searchParams.get("redirect") || "", which will throw aTypeErrorif the redirect parameter is missing or empty, crashing the authentication flow.Apply this diff to validate the redirect before parsing:
function handleAuthRequest(content: string) { const url = new URL(content); platform.set(url.searchParams.get("platform")); - const redirectUrl = new URL(url.searchParams.get("redirect") || ""); - redirect.set(url.searchParams.get("redirect")); + const redirectParam = url.searchParams.get("redirect"); + if (!redirectParam) { + console.error("Missing redirect parameter in auth request"); + return; + } + redirect.set(redirectParam); + const redirectUrl = new URL(redirectParam); session.set(url.searchParams.get("session")); hostname.set(redirectUrl.hostname); isSigningRequest.set(false); codeScannedDrawerOpen.set(true); }
477-512: Validate platform URL is non-null before fetch.The
platformUrlparameter is typed asstring | null(line 479), but the code uses it directly in the fetch call at line 488-490 without validation. IfplatformUrlis null, this will construct an invalid URL like"null/api/polls/..."and fail.Apply this diff to validate the platform URL:
async function handleBlindVotingRequest( blindVoteData: SigningData, platformUrl: string | null, redirectUri: string | null, ) { try { const pollId = blindVoteData.pollId; if (!pollId) { throw new Error("No poll ID provided in blind vote data"); } + + if (!platformUrl) { + throw new Error("Platform URL not provided in blind vote request"); + } const pollResponse = await fetch( `${platformUrl}/api/polls/${pollId}`, );Based on learnings
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (2)
15-30: Consider consolidating duplicate platform URL fields.The
SigningDatainterface has bothplatform_url(line 23) andplatformUrl(line 29), which appears redundant. If both are needed for backward compatibility or different data sources, consider adding a comment explaining when each is used.
624-886: Consider refactoring long function into smaller units.The
handleBlindVotefunction is 262 lines long and handles multiple distinct responsibilities: input validation, W3ID retrieval, vote generation, backend registration, vote data storage, and submission. Consider extracting helper functions for each concern to improve readability and testability.For example:
validateBlindVoteInputs()getVoterIdentity()generateLocalVoteData()submitBlindVoteToBackend()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T10:34:52.509Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 415
File: infrastructure/eid-wallet/src/env.d.ts:8-8
Timestamp: 2025-11-13T10:34:52.509Z
Learning: In infrastructure/eid-wallet, PUBLIC_PLATFORM_URL should not be added to .env.example or configured as a static environment variable. The platform URL is extracted dynamically through URI parsing according to the protocol specification, and all fallbacks for platform URL are being removed.
Applied to files:
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (2)
1091-1118: LGTM: Proper platform URL validation.The platform URL validation correctly follows the protocol specification by requiring it from the signing data without fallback, validates it's non-empty, normalizes it using URL constructor, and provides clear error messages on failure.
Based on learnings
1185-1262: LGTM: Well-structured initialization with proper cleanup.The initialize function correctly validates authentication, sets up deep link event handlers, processes stored deep link data, and returns a cleanup function to prevent memory leaks. The dual check for both
deepLinkDataandpendingDeepLinkin sessionStorage provides good backward compatibility.
Description of change
Fixes eID Wallet onboarding flow and scan issues
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Improvements