Feat/eid wallet redesign#961
Open
Bekiboo wants to merge 46 commits into
Open
Conversation
Allows running the wallet via `pnpm tauri dev` on a desktop machine for the redesign work. Previously the only registered capability targeted iOS/Android only, so on desktop the WebView had zero IPC permissions and the global-state init via @tauri-apps/plugin-store crashed at first paint. - Cargo.toml: add `devtools` feature on the tauri crate (Windows debug builds don't expose devtools without it) - capabilities/desktop.json: new capability granting the non-mobile plugin defaults (core, opener, store, deep-link, notifications, process) on windows/macOS/linux - tauri.conf.json: register `desktop-capability` alongside the existing `mobile-capability` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 35 minutes and 38 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughLarge frontend refactor of the eID Wallet app (navigation, onboarding/login with PIN, main page cards, personal/social bindings, settings, notifications, UI components) plus backend eVault Core additions for security questions (types, Argon2 hashing/verification, GraphQL mutations, service, entity, migration). Also Tauri capabilities/config updates. ChangesEnd-to-end wallet UX, bindings, and backend security questions
Sequence Diagram(s)(skipped) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Bekiboo
force-pushed
the
feat/eid-wallet-redesign
branch
from
bd20cb2 to
6eb980f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/eid-wallet/src/routes/+layout.svelte (1)
187-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid a hand-maintained allowlist for authenticated routes.
This helper only recognizes three exact paths, so deep links received from newly added protected screens in this PR (
/personal,/notifications,/social-bindings,/settings/*, etc.) will send already-authenticated users back to/login. Derive this from actual auth state or the(app)route shape instead of a small pathname list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`+layout.svelte around lines 187 - 192, The isAuthenticatedRoute function uses a brittle hand-maintained list of exact paths; replace it so protected status is derived from real auth state or route structure instead of matching specific paths. Update isAuthenticatedRoute to consult the authentication store/flag (e.g., an exported isAuthenticated boolean or session store) or to test route shape/prefix (e.g., check a common layout group or pathname.startsWith('/app') or '/settings/') so deep links like /personal, /notifications, /social-bindings or /settings/* are treated as protected; keep the function name isAuthenticatedRoute and remove the static authenticatedRoutes array, returning true based on the auth state or route-group/prefix logic instead.
🟡 Minor comments (12)
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte-201-203 (1)
201-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winURL-encode
w3idin registry resolve requests.Both resolve calls interpolate
enamedirectly into the query string. UseencodeURIComponent(ename)to avoid malformed requests when identifiers contain reserved characters.Suggested patch
- const resolveResp = await axios.get( - new URL(`resolve?w3id=${ename}`, PUBLIC_REGISTRY_URL).toString(), - ); + const resolveResp = await axios.get( + new URL( + `resolve?w3id=${encodeURIComponent(ename)}`, + PUBLIC_REGISTRY_URL, + ).toString(), + ); ... - const resolveResp = await axios.get( - new URL(`resolve?w3id=${ename}`, PUBLIC_REGISTRY_URL).toString(), - ); + const resolveResp = await axios.get( + new URL( + `resolve?w3id=${encodeURIComponent(ename)}`, + PUBLIC_REGISTRY_URL, + ).toString(), + );Also applies to: 615-617
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte around lines 201 - 203, The registry resolve calls interpolate ename directly into the query string which can break for reserved characters; update both axios.get usages that build the URL with new URL(`resolve?w3id=${ename}`, PUBLIC_REGISTRY_URL) to URL-encode the identifier by using encodeURIComponent(ename) when constructing the query (refer to the resolveResp axios.get usage and the second occurrence around lines 615-617), ensuring the w3id query parameter is properly encoded before calling axios.get.infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte-79-79 (1)
79-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid first-render step flash on
?upgrade=1.
stepalways starts at"pin-create"and only switches to"kyc-panel"inonMount, so upgrade-entry can briefly render the PIN flow before redirecting state. Initialize with a neutral/loading gate until query-param bootstrap completes.Also applies to: 935-961
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte at line 79, The initial value for step ("pin-create") causes a UI flash before the query-param bootstrap runs; change the initialization of the step state (the let step = $state<Step>("pin-create") usage) to a neutral/loading sentinel (e.g., "loading" or undefined) and update the onMount bootstrap that detects ?upgrade=1 to set step to "kyc-panel" or the normal initial step once the query param handling completes; also adjust any conditional rendering that assumes a concrete Step to gate UI while step is the loading sentinel. Ensure you modify the $state generic/type to accept the sentinel (or widen Step) and update any checks that render the PIN flow so they only run when step is a real Step value.infrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/NameInput.svelte-43-47 (1)
43-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssociate the label with the input via
id.The label points to
for="name", but the input only hasname="name"and no matchingid, so the association is broken.Suggested patch
- <input - name="name" + <input + id="name" + name="name" bind:this={inputEl}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/steps/NameInput.svelte around lines 43 - 47, The label uses for="name" but the corresponding input lacks an id, so screen readers and clicks won't focus the input; add id="name" (or a matching unique id) to the input element that currently has name="name" and bind:this={inputEl} so the label and input are properly associated.infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte-75-81 (1)
75-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a fallback when popping browser history.
Using only
window.history.back()can strand users who enter this route directly (no previous entry). Add a fallback to/settingsin both back paths.Proposed patch
function handleClose() { showSuccess = false; @@ - window.history.back(); + if (window.history.length > 1) { + window.history.back(); + } else { + void goto("/settings"); + } } @@ } else { - window.history.back(); + if (window.history.length > 1) { + window.history.back(); + } else { + void goto("/settings"); + } } };Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/settings/pin/+page.svelte around lines 75 - 81, handleClose currently calls window.history.back() which can strand users who navigated directly; update handleClose (and the other back-path at the other section referenced) to attempt history.back() but fall back to programmatic navigation to "/settings" when there is no prior entry (e.g., check window.history.length <= 1 or detect no navigation change) — ensure both the function handleClose and the other back-path use the same fallback so direct-entry users are redirected to "/settings".infrastructure/eid-wallet/src/routes/(app)/personal/+page.svelte-39-39 (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign knowledge completion logic with the marks counter.
knowledgeFilledcurrently treats any non-null object as complete, butmarksAchievedrequires a non-empty trimmed question. This can show a completed card while the subtitle still counts fewer marks.💡 Proposed fix
-const knowledgeFilled = $derived(!!binding.knowledge); +const knowledgeFilled = $derived( + !!binding.knowledge && binding.knowledge.question.trim().length > 0, +);Also applies to: 428-430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/personal/+page.svelte at line 39, The derived store knowledgeFilled is currently set to true for any non-null binding.knowledge, which mismatches the marksAchieved logic that requires a non-empty trimmed question; update the derived to check that binding.knowledge exists and that binding.knowledge.question is a string with trimmed length > 0 (same condition used by marksAchieved) so the card completion state aligns with the marks counter; apply the same change to the other occurrences of knowledgeFilled at the other noted locations (lines ~428-430) to keep behavior consistent.infrastructure/eid-wallet/src/routes/(app)/social-bindings/+page.svelte-126-143 (1)
126-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-functional row buttons should not be exposed as actionable controls.
At Line 126, each row is a
<button>but has no action. This announces an interactive element that does nothing.♻️ Proposed fix
- <button - type="button" - class="w-full flex items-center gap-3 py-3 active:opacity-70 text-left" - > + <div + class="w-full flex items-center gap-3 py-3 text-left" + > <div class="flex-1 min-w-0"> <p class="font-semibold text-black-900 leading-tight truncate"> {binding.counterpartyName} </p> <p class="text-black-500 leading-tight"> {formatTime(binding.completedAt)} </p> </div> <ChevronIcon size={13} class="rotate-180 text-black-500 shrink-0" /> - </button> + </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/social-bindings/+page.svelte around lines 126 - 143, The rows are rendered as interactive <button> elements but have no click handlers; replace the <button> wrapper around each binding item with a non-interactive element (e.g., a <div> with the same classes) so screen readers and keyboard users are not misled; keep the inner content (binding.counterpartyName, formatTime(binding.completedAt), and the ChevronIcon) and preserve styling and layout classes so appearance doesn't change.infrastructure/eid-wallet/src/routes/(app)/main/components/Lasso.svelte-45-55 (1)
45-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the documented default with the actual
drawDurationdefault.The prop comment says
Default 800ms, but the code defaults to500. Please make these consistent.Proposed fix
- /** ms duration of the draw animation. Default 800ms. */ + /** ms duration of the draw animation. Default 500ms. */ drawDuration?: number;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/main/components/Lasso.svelte around lines 45 - 55, The JSDoc for drawDuration is out of sync with the default used in the code: update the comment above the prop/interface (the line with "ms duration of the draw animation. Default 800ms.") to reflect the actual default used in the destructuring (drawDuration = 500) by changing the documented default to 500ms; ensure the symbol drawDuration in the props/interface and the destructuring block remain consistent and update any other inline docs that mention "800ms".infrastructure/eid-wallet/src/routes/(app)/main/legacy/MarketplaceBanner.svelte-29-40 (1)
29-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix invalid HTML nesting in the “Explore” row (div inside span).
The outer wrapper is a
<span>that contains a nested<div>, which is invalid HTML and can cause inconsistent semantics/layout. Switch the outer wrapper to a block element.Proposed fix
- <span + <div class="text-sm opacity-90 relative z-10 drop-shadow-md flex gap-1 items-center" > @@ - </span> + </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/main/legacy/MarketplaceBanner.svelte around lines 29 - 40, The outer wrapper in MarketplaceBanner.svelte currently uses a <span> with class "text-sm opacity-90 relative z-10 drop-shadow-md flex gap-1 items-center" and contains a nested <div>, which is invalid HTML; change the outer <span> to a block element (e.g., replace the <span> with a <div> while keeping the same class and children including the inner <div> and <img src="/images/W3DSLogoBlack.svg" alt="W3DS Logo" class="h-4" />) so the DOM is valid and layout is preserved.infrastructure/eid-wallet/src/lib/ui/icons/PinIcon.svelte-16-23 (1)
16-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark decorative SVG as hidden from assistive tech.
This icon is decorative and should be excluded from the accessibility tree.
💡 Suggested fix
<svg width={size} height={size} viewBox="0 0 24 24" fill="none" class={classes} xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/lib/ui/icons/PinIcon.svelte` around lines 16 - 23, The SVG in PinIcon.svelte is decorative and must be removed from the accessibility tree: update the <svg> element (the one that uses size, viewBox and class={classes}) to include aria-hidden="true" and focusable="false" (and ensure there is no title/role that exposes it to assistive tech); this will hide the decorative icon from screen readers while keeping the existing props (size, classes) intact.infrastructure/eid-wallet/src/lib/ui/icons/ChevronIcon.svelte-17-24 (1)
17-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark decorative SVG as hidden from assistive tech.
This icon component is decorative-only and should not be exposed to screen readers.
💡 Suggested fix
<svg width={size} height={size} viewBox="0 0 8 13" fill="none" class={classes} xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/lib/ui/icons/ChevronIcon.svelte` around lines 17 - 24, The SVG in ChevronIcon.svelte is decorative and should be hidden from assistive tech; update the <svg> element (the one using the size and classes props) to include accessibility attributes such as aria-hidden="true" and focusable="false" (and remove any title/desc if present) so screen readers ignore the decorative icon while preserving visuals.infrastructure/eid-wallet/src/lib/ui/icons/PrivacyIcon.svelte-15-22 (1)
15-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark decorative SVG as hidden from assistive tech.
Without
aria-hidden, screen readers may announce this purely visual icon.💡 Suggested fix
<svg width={size} height={size} viewBox="0 0 24 24" fill="none" class={classes} xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/lib/ui/icons/PrivacyIcon.svelte` around lines 15 - 22, The SVG in the PrivacyIcon.svelte component is decorative and should be hidden from assistive tech; update the <svg> element inside PrivacyIcon.svelte (the one using size and classes props) to include aria-hidden="true" (and optionally focusable="false" for legacy browsers) so screen readers ignore the visual icon..vscode/settings.json-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t standardize Biome as the default
.svelteformatter without accounting for partial support.Line 24 routes Svelte formatting to
biomejs.biome, but Biome’s own language-support docs list Svelte formatting as partial/limited (not fully/stably supported). This can lead to incomplete/inconsistent format-on-save results for.sveltefiles—either scope the setting to what’s actually supported in your setup or remove the[svelte]override unless you’ve confirmed the output is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.vscode/settings.json at line 24, The settings file currently forces Biome as the default formatter via the "editor.defaultFormatter": "biomejs.biome" setting under the [svelte] override; update this to avoid forcing Biome for .svelte files—either remove the [svelte] override entirely or replace the value with a safer option (e.g., unset the formatter or use a Svelte-first formatter like "svelte.svelte-vscode") and document that Biome is only used if you have confirmed full Svelte formatting support; locate the "[svelte]" block and the "editor.defaultFormatter": "biomejs.biome" entry to apply the change.
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(app)/settings/+layout.svelte (1)
14-18: ⚡ Quick winAvoid hardcoding the app version in this layout.
This value will eventually drift from the actual shipped version and can surface incorrect info in Settings. Prefer a single source of truth from build/runtime metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/settings/+layout.svelte around lines 14 - 18, Replace the hardcoded VERSION constant used by subtitleAtMount with a single source-of-truth build/runtime value (e.g. an env var injected at build time such as import.meta.env.VITE_APP_VERSION or a value defined from package.json via your bundler) so the Settings UI reflects the shipped app version; update the code that computes subtitleAtMount to read that injected version (and provide a safe fallback/undefined if missing) instead of using the literal "0.7.1". Ensure symbols referenced are VERSION (remove or replace) and subtitleAtMount so the change is limited to this layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.local.json:
- Around line 1-7: The committed .claude/settings.local.json contains a
user-specific absolute path and session IDs in the permissions.allow array;
remove that sensitive entry from the tracked file (edit the permissions.allow
array in settings.local.json to eliminate the absolute path/session identifiers)
and move the local session permission into an untracked local config or an
environment-specific file; also add an appropriate ignore rule (e.g., ignore
.claude/settings.local.json or the exact local file) to git so future local
Claude session permissions aren’t committed.
In `@infrastructure/eid-wallet/src-tauri/capabilities/desktop.json`:
- Around line 8-16: Remove the unused Tauri permissions from desktop.json by
deleting "process:default" (tauri_plugin_process::init() exists but there are no
frontend/Rust callsites using the process plugin) and removing the redundant
"opener:allow-default-urls" since it is covered by "opener:default"; keep
"opener:default" but scope it to schemes as before. Additionally, add app-layer
validation/allowlist checks wherever openUrl(...) is called (e.g., in
scanLogic.ts handling the deep-link/QR-derived redirect and in the open-message
route handlers) to validate the target host/URL against an approved list before
calling openUrl. Ensure changes reference the permissions array in desktop.json
and the callsites in scanLogic.ts and the open-message route.
In `@infrastructure/eid-wallet/src-tauri/Cargo.toml`:
- Line 21: The tauri crate currently always enables the "devtools" feature;
remove "devtools" from the tauri dependency line and instead expose it behind a
Cargo feature so it can be enabled only in debug/dev builds. Concretely: change
the tauri dependency (the tauri = { version = "2", features = ["devtools"] }
entry) to not include "devtools", add a [features] section with a feature name
like "devtools" that maps to the dependency feature ("devtools" =
["tauri/devtools"]), and document/enable that feature only for development (e.g.
build/run with --features devtools or enable it in your local dev scripts),
keeping release builds free of the devtools feature.
In `@infrastructure/eid-wallet/src/lib/ui/Button/ButtonIcon.svelte`:
- Around line 106-114: resolvedBgSize currently maps numeric bgSize to Tailwind
class names (h-#/w-#), which breaks the documented numeric=px contract; change
the logic so resolvedBgSize only returns Tailwind class names for named variants
(or "" when bgSize is undefined) and does NOT compose classes for numbers, and
create a separate reactive (e.g., bgSizeStyle or resolvedBgSizeStyle) that when
typeof bgSize === "number" returns an inline style string/object like "width:
Xpx; height: Xpx;" (or { width: X + 'px', height: X + 'px' }) while returning
undefined/"" for non-numeric cases; keep the existing sizeVariant lookup for
string keys (sizeVariant and bgSize) and update the component template to apply
both the class from resolvedBgSize and the inline style from the new bgSizeStyle
when present.
In `@infrastructure/eid-wallet/src/lib/utils/socialBinding.ts`:
- Around line 520-550: The createOwnSocialBindingMirror flow currently only
checks result.createBindingDocument.errors but doesn't verify that
result.createBindingDocument.metaEnvelopeId is present; update
createOwnSocialBindingMirror to validate metaEnvelopeId after the GraphQL call
(inspect result.createBindingDocument.metaEnvelopeId) and throw a clear Error if
it's null/undefined (include context like "missing metaEnvelopeId for social
binding mirror"), so a silent success cannot occur when the envelope ID wasn't
returned.
In `@infrastructure/eid-wallet/src/routes/`(app)/main/+page.svelte:
- Around line 473-476: The early return when globalState is null prevents
pageReady from being set and can leave the UI blank; in the block that checks if
(!gs) update pageReady (or set a fallback ready state) before returning so the
page can render an error/placeholder instead of staying blank. Locate the check
around globalState (variable gs) and ensure you call the pageReady setter (or
assign pageReady = true) and optionally set an error flag or message so the UI
shows a non-empty fallback when initialization misses its retry window.
In
`@infrastructure/eid-wallet/src/routes/`(app)/main/components/LegalIdAccordion.svelte:
- Around line 45-54: The accordion row is focusable but only responds to mouse
clicks; add keyboard activation by handling Enter and Space keys on the same
interactive div (in LegalIdAccordion.svelte) so keyboard users can toggle the
panel; implement an on:keydown handler that checks hasDoc, listens for 'Enter'
or ' ' (Space), calls the existing toggle() function, and prevents default for
Space to avoid scrolling, leaving the current onclick={toggle} intact for
pointer users.
In
`@infrastructure/eid-wallet/src/routes/`(app)/main/components/PersonalBindingAccordion.svelte:
- Around line 45-54: The accordion trigger div currently only responds to clicks
(onclick={toggle}) and needs keyboard activation; add a keydown handler (e.g.,
handleKeydown) on the same element that, when hasAny is true and the pressed key
is Enter or Space, prevents default and calls toggle; implement handleKeydown in
the component to check event.key (or keyCode 13/32) and only invoke toggle when
interactive, keeping existing tabindex={hasAny ? 0 : -1}, role="button",
aria-expanded={expanded} and aria-disabled={!hasAny} intact so the element is
both accessible and keyboard-operable.
In
`@infrastructure/eid-wallet/src/routes/`(app)/main/components/SocialBindingAccordion.svelte:
- Around line 56-65: The div uses role="button" and onclick={toggle} but ignores
keyboard events; add a keydown handler (e.g., on:keydown or a handleKeyDown
function referenced in the template) that listens for Enter (Enter/Return) and
Space keys and calls toggle() (preventDefault for Space to avoid scrolling) only
when hasBindings is true; ensure the handler respects hasBindings and updates
aria-expanded via the existing expanded prop so keyboard users can activate the
accordion.
In
`@infrastructure/eid-wallet/src/routes/`(app)/main/legacy/KycUpgradeOverlay.svelte:
- Around line 209-212: The code sets kycError and immediately calls resetKyc(),
which clears the error before the user can see it; update the logic in the
handler(s) that set kycError (references: kycError variable and resetKyc()
function) so you do not call resetKyc() right after assigning kycError—either
remove the immediate resetKyc() call in those branches, trigger resetKyc() only
after a short timeout or after the user dismisses the error, or have resetKyc()
skip clearing kycError so the message remains visible until acknowledged.
- Around line 98-106: The effect currently only starts the KYC flow when open
becomes true (using $effect with open, kycStep and startKycUpgrade), but never
stops or resets it when open becomes false; update the reactive effect to handle
both branches: when open is true and kycStep === "idle" call startKycUpgrade()
as before, and when open is false perform cleanup by calling the existing
stop/cleanup function (or set kycStep back to "idle" if no stopper exists) and
cancel any pending timers/promises/subscriptions started by startKycUpgrade();
keep using untrack for state mutations and apply the same fix for the other
occurrence that references kycStep (the block around startKycUpgrade).
- Around line 12-14: The import of PUBLIC_PROVISIONER_SHARED_SECRET from
$env/static/public in KycUpgradeOverlay.svelte exposes the secret to the client;
remove any use/import of PUBLIC_PROVISIONER_SHARED_SECRET from this component
(and the other occurrences referenced) and instead implement the auth logic on
the server: create a server-only endpoint or module (e.g., a +server.ts handler
or API route) that reads the secret from $env/static/private or
$env/dynamic/private and performs the x-shared-secret check, then have
KycUpgradeOverlay.svelte call that server endpoint for any operations requiring
the secret; update references to functions that previously relied on the
client-side secret to call the new server endpoint instead (search for
PUBLIC_PROVISIONER_SHARED_SECRET usages in this file and at the other noted
locations).
In
`@infrastructure/eid-wallet/src/routes/`(app)/personal/components/AddKnowledgeSheet.svelte:
- Around line 81-86: The answer input in AddKnowledgeSheet.svelte
(id="kn-answer", bind:value={answer}) is currently plaintext; change it to a
masked sensitive field by switching it to a password-style input and disable
helper/auto features: set type to "password", add attributes like
autocomplete="new-password" (or "off"), autocorrect="off", autocapitalize="off",
spellcheck="false" and consider preventing automatic suggestions/pasting if
desired; update any UI/labels to reflect it's a secret and ensure any form
handling (the answer variable and submit logic) continues to work with the
masked input.
In
`@infrastructure/eid-wallet/src/routes/`(app)/personal/components/AddPhotoSheet.svelte:
- Around line 76-80: The sheet closing can race with a late getUserMedia
resolution so add a closed/active boolean flag (e.g., isOpen or isMounted) that
close() sets to false before calling stopStream() and resetState(), and have
your camera-start/async getUserMedia handler check that flag before assigning
stream to the component; if the flag indicates closed, immediately stop the
returned MediaStream (or call stopStream()) instead of setting state. Update
stopStream(), resetState(), and the async media assignment locations (where
navigator.mediaDevices.getUserMedia is awaited and where stream is assigned) to
consult this flag to prevent retaining an orphaned active stream.
In `@infrastructure/evault-core/src/core/utils/security-answer.ts`:
- Around line 39-42: The current verifyAnswer function swallows all exceptions
from argon2.verify(storedHash, normalised) and treats infrastructure/native
failures as a wrong answer; change it so only genuine verification failures map
to false and all other unexpected errors are rethrown. In practice, update the
try/catch around argon2.verify in verifyAnswer to inspect the caught error: if
it clearly indicates a verification/mismatch result (the argon2 "incorrect
password/hash" case) then return false, otherwise rethrow the error so calling
code (e.g., SecurityQuestionService.validate) can handle infrastructure errors;
reference the verifyAnswer function and the argon2.verify(storedHash,
normalised) call when making this change.
In `@infrastructure/evault-core/src/entities/SecurityAnswerAttempt.ts`:
- Around line 22-26: The entity SecurityAnswerAttempt uses timezone-naive
columns for lockout checks; change the `@Column` decorators for lockedUntil and
lastAttemptAt to use type: "timestamptz" and add a new DB migration that ALTERs
the existing columns from TIMESTAMP to TIMESTAMPTZ so stored values remain
timezone-aware; ensure SecurityQuestionService.validate() continues to compare
attempt.lockedUntil against now() correctly after the migration and include both
up and down SQL migration steps to convert the columns safely.
In
`@infrastructure/evault-core/src/migrations/1779517729310-SecurityAnswerAttempt.ts`:
- Around line 10-11: The migration is destructively dropping and re-adding
notifications.body; instead change the two queryRunner.query calls that DROP/ADD
"body" to use ALTER TABLE "notifications" ALTER COLUMN "body" TYPE text USING
"body"::text (i.e., perform an ALTER COLUMN ... TYPE ... USING conversion) so
existing notification bodies are preserved; update both occurrences in the
1779517729310-SecurityAnswerAttempt migration where "notifications" "body" is
handled.
In `@infrastructure/evault-core/src/services/BindingDocumentService.ts`:
- Around line 121-139: The security_question branch in BindingDocumentService
currently allows any non-empty answerHash string; trim d.answerHash before
checking emptiness and reject whitespace-only values by throwing
ValidationError, and preferably validate the hash format using the shared
utility from core/utils/security-answer (the same validation used by
SecurityQuestionService.validate) so only hashes produced by the shared hashing
flow are accepted; update the returned BindingDocumentSecurityQuestionData to
use the trimmed/validated answerHash.
In `@infrastructure/evault-core/src/services/SecurityQuestionService.ts`:
- Around line 54-61: When detecting that an existing attempt's lockedUntil is in
the past, reset the stale failure counter so the user gets a fresh window: set
attempt.failedCount = 0 (and optionally clear attempt.lockedUntil) and persist
that change using the same save/update code path used elsewhere in
SecurityQuestionService (i.e., update the same repository/save call that handles
attempt changes). Apply the same reset logic in both the early-return check (the
block checking attempt.lockedUntil > now) and the later handling around lines
82–97 so expired locks always clear the failedCount before further increments.
- Around line 52-90: The current validate flow (loadOrCreateAttempt →
verifyAnswer → update attempt → attemptRepository.save) is vulnerable to race
conditions; make the failed-attempt updates atomic by wrapping the verify+update
in a DB transaction or by performing an atomic DB update instead of a
read-modify-write. Concretely, either: (A) start a transaction, reload the
attempt with a row lock (FOR UPDATE) before checking/setting
failedCount/lockedUntil and commit after attemptRepository.save, or (B) replace
the read-modify-write with a single atomic update query (e.g., UPDATE ... SET
failedCount = failedCount + 1, lastAttemptAt = ?, lockedUntil = CASE WHEN
failedCount + 1 >= :max THEN ? ELSE lockedUntil END WHERE id = :id RETURNING *),
then use that returned row to decide whether to call recordSuccess or return
locked/failed results; apply this change around loadOrCreateAttempt,
verifyAnswer, recordSuccess and attemptRepository.save to ensure correct
concurrent behavior.
---
Outside diff comments:
In `@infrastructure/eid-wallet/src/routes/`+layout.svelte:
- Around line 187-192: The isAuthenticatedRoute function uses a brittle
hand-maintained list of exact paths; replace it so protected status is derived
from real auth state or route structure instead of matching specific paths.
Update isAuthenticatedRoute to consult the authentication store/flag (e.g., an
exported isAuthenticated boolean or session store) or to test route shape/prefix
(e.g., check a common layout group or pathname.startsWith('/app') or
'/settings/') so deep links like /personal, /notifications, /social-bindings or
/settings/* are treated as protected; keep the function name
isAuthenticatedRoute and remove the static authenticatedRoutes array, returning
true based on the auth state or route-group/prefix logic instead.
---
Minor comments:
In @.vscode/settings.json:
- Line 24: The settings file currently forces Biome as the default formatter via
the "editor.defaultFormatter": "biomejs.biome" setting under the [svelte]
override; update this to avoid forcing Biome for .svelte files—either remove the
[svelte] override entirely or replace the value with a safer option (e.g., unset
the formatter or use a Svelte-first formatter like "svelte.svelte-vscode") and
document that Biome is only used if you have confirmed full Svelte formatting
support; locate the "[svelte]" block and the "editor.defaultFormatter":
"biomejs.biome" entry to apply the change.
In `@infrastructure/eid-wallet/src/lib/ui/icons/ChevronIcon.svelte`:
- Around line 17-24: The SVG in ChevronIcon.svelte is decorative and should be
hidden from assistive tech; update the <svg> element (the one using the size and
classes props) to include accessibility attributes such as aria-hidden="true"
and focusable="false" (and remove any title/desc if present) so screen readers
ignore the decorative icon while preserving visuals.
In `@infrastructure/eid-wallet/src/lib/ui/icons/PinIcon.svelte`:
- Around line 16-23: The SVG in PinIcon.svelte is decorative and must be removed
from the accessibility tree: update the <svg> element (the one that uses size,
viewBox and class={classes}) to include aria-hidden="true" and focusable="false"
(and ensure there is no title/role that exposes it to assistive tech); this will
hide the decorative icon from screen readers while keeping the existing props
(size, classes) intact.
In `@infrastructure/eid-wallet/src/lib/ui/icons/PrivacyIcon.svelte`:
- Around line 15-22: The SVG in the PrivacyIcon.svelte component is decorative
and should be hidden from assistive tech; update the <svg> element inside
PrivacyIcon.svelte (the one using size and classes props) to include
aria-hidden="true" (and optionally focusable="false" for legacy browsers) so
screen readers ignore the visual icon.
In `@infrastructure/eid-wallet/src/routes/`(app)/main/components/Lasso.svelte:
- Around line 45-55: The JSDoc for drawDuration is out of sync with the default
used in the code: update the comment above the prop/interface (the line with "ms
duration of the draw animation. Default 800ms.") to reflect the actual default
used in the destructuring (drawDuration = 500) by changing the documented
default to 500ms; ensure the symbol drawDuration in the props/interface and the
destructuring block remain consistent and update any other inline docs that
mention "800ms".
In
`@infrastructure/eid-wallet/src/routes/`(app)/main/legacy/MarketplaceBanner.svelte:
- Around line 29-40: The outer wrapper in MarketplaceBanner.svelte currently
uses a <span> with class "text-sm opacity-90 relative z-10 drop-shadow-md flex
gap-1 items-center" and contains a nested <div>, which is invalid HTML; change
the outer <span> to a block element (e.g., replace the <span> with a <div> while
keeping the same class and children including the inner <div> and <img
src="/images/W3DSLogoBlack.svg" alt="W3DS Logo" class="h-4" />) so the DOM is
valid and layout is preserved.
In `@infrastructure/eid-wallet/src/routes/`(app)/personal/+page.svelte:
- Line 39: The derived store knowledgeFilled is currently set to true for any
non-null binding.knowledge, which mismatches the marksAchieved logic that
requires a non-empty trimmed question; update the derived to check that
binding.knowledge exists and that binding.knowledge.question is a string with
trimmed length > 0 (same condition used by marksAchieved) so the card completion
state aligns with the marks counter; apply the same change to the other
occurrences of knowledgeFilled at the other noted locations (lines ~428-430) to
keep behavior consistent.
In `@infrastructure/eid-wallet/src/routes/`(app)/settings/pin/+page.svelte:
- Around line 75-81: handleClose currently calls window.history.back() which can
strand users who navigated directly; update handleClose (and the other back-path
at the other section referenced) to attempt history.back() but fall back to
programmatic navigation to "/settings" when there is no prior entry (e.g., check
window.history.length <= 1 or detect no navigation change) — ensure both the
function handleClose and the other back-path use the same fallback so
direct-entry users are redirected to "/settings".
In `@infrastructure/eid-wallet/src/routes/`(app)/social-bindings/+page.svelte:
- Around line 126-143: The rows are rendered as interactive <button> elements
but have no click handlers; replace the <button> wrapper around each binding
item with a non-interactive element (e.g., a <div> with the same classes) so
screen readers and keyboard users are not misled; keep the inner content
(binding.counterpartyName, formatTime(binding.completedAt), and the ChevronIcon)
and preserve styling and layout classes so appearance doesn't change.
In `@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/+page.svelte:
- Around line 201-203: The registry resolve calls interpolate ename directly
into the query string which can break for reserved characters; update both
axios.get usages that build the URL with new URL(`resolve?w3id=${ename}`,
PUBLIC_REGISTRY_URL) to URL-encode the identifier by using
encodeURIComponent(ename) when constructing the query (refer to the resolveResp
axios.get usage and the second occurrence around lines 615-617), ensuring the
w3id query parameter is properly encoded before calling axios.get.
- Line 79: The initial value for step ("pin-create") causes a UI flash before
the query-param bootstrap runs; change the initialization of the step state (the
let step = $state<Step>("pin-create") usage) to a neutral/loading sentinel
(e.g., "loading" or undefined) and update the onMount bootstrap that detects
?upgrade=1 to set step to "kyc-panel" or the normal initial step once the query
param handling completes; also adjust any conditional rendering that assumes a
concrete Step to gate UI while step is the loading sentinel. Ensure you modify
the $state generic/type to accept the sentinel (or widen Step) and update any
checks that render the PIN flow so they only run when step is a real Step value.
In
`@infrastructure/eid-wallet/src/routes/`(auth)/onboarding/steps/NameInput.svelte:
- Around line 43-47: The label uses for="name" but the corresponding input lacks
an id, so screen readers and clicks won't focus the input; add id="name" (or a
matching unique id) to the input element that currently has name="name" and
bind:this={inputEl} so the label and input are properly associated.
---
Nitpick comments:
In `@infrastructure/eid-wallet/src/routes/`(app)/settings/+layout.svelte:
- Around line 14-18: Replace the hardcoded VERSION constant used by
subtitleAtMount with a single source-of-truth build/runtime value (e.g. an env
var injected at build time such as import.meta.env.VITE_APP_VERSION or a value
defined from package.json via your bundler) so the Settings UI reflects the
shipped app version; update the code that computes subtitleAtMount to read that
injected version (and provide a safe fallback/undefined if missing) instead of
using the literal "0.7.1". Ensure symbols referenced are VERSION (remove or
replace) and subtitleAtMount so the change is limited to this layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bd1dc5d-962d-4291-a0c9-a7c86bfb8193
⛔ Files ignored due to path filters (14)
infrastructure/eid-wallet/static/fonts/Archivo-VariableFont_wdth,wght.ttfis excluded by!**/*.ttfinfrastructure/eid-wallet/static/images/LegalID.pngis excluded by!**/*.pnginfrastructure/eid-wallet/static/images/Logo-Blabsy.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/Logo-Dreamsync.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/Logo-Pictique.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/Logo-eCurrency.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/Logo-eVoting.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/Personal.pngis excluded by!**/*.pnginfrastructure/eid-wallet/static/images/SocialBinding.pngis excluded by!**/*.pnginfrastructure/eid-wallet/static/images/eVault-kid-drawing.pngis excluded by!**/*.pnginfrastructure/eid-wallet/static/images/message.svgis excluded by!**/*.svginfrastructure/eid-wallet/static/images/w3ds-logo-main.svgis excluded by!**/*.svgplatforms/marketplace/client/assets/pictique-logo.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (84)
.claude/settings.local.json.vscode/settings.jsoninfrastructure/eid-wallet/package.jsoninfrastructure/eid-wallet/src-tauri/Cargo.tomlinfrastructure/eid-wallet/src-tauri/capabilities/desktop.jsoninfrastructure/eid-wallet/src-tauri/tauri.conf.jsoninfrastructure/eid-wallet/src/app.cssinfrastructure/eid-wallet/src/app.htmlinfrastructure/eid-wallet/src/lib/actions/keyboardInset.tsinfrastructure/eid-wallet/src/lib/fragments/AppNav/AppNav.svelteinfrastructure/eid-wallet/src/lib/fragments/SettingsNavigationBtn/SettingsNavigationBtn.svelteinfrastructure/eid-wallet/src/lib/fragments/SplashScreen/SplashScreen.svelteinfrastructure/eid-wallet/src/lib/global/runtime.svelte.tsinfrastructure/eid-wallet/src/lib/global/state.tsinfrastructure/eid-wallet/src/lib/stores/language.tsinfrastructure/eid-wallet/src/lib/stores/notifications.tsinfrastructure/eid-wallet/src/lib/stores/personalBinding.tsinfrastructure/eid-wallet/src/lib/ui/BottomSheet/BottomSheet.svelteinfrastructure/eid-wallet/src/lib/ui/Button/ButtonIcon.svelteinfrastructure/eid-wallet/src/lib/ui/PinDots/PinDots.svelteinfrastructure/eid-wallet/src/lib/ui/icons/ChevronIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/EditIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/GearIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/MessageIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/PinIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/PrivacyIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/QRIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/index.tsinfrastructure/eid-wallet/src/lib/ui/index.tsinfrastructure/eid-wallet/src/lib/utils/index.tsinfrastructure/eid-wallet/src/lib/utils/personalBinding.tsinfrastructure/eid-wallet/src/lib/utils/portal.tsinfrastructure/eid-wallet/src/lib/utils/socialBinding.tsinfrastructure/eid-wallet/src/routes/(app)/+layout.svelteinfrastructure/eid-wallet/src/routes/(app)/main/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/AppsMarketplace.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/BindingDocuments.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/ENameCard.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/EVaultCard.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/Greeting.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/InfoDrawer.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/Lasso.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/LegalIdAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/PersonalBindingAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/ScanFAB.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/SocialBindingAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/SocialBindingDrawer.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/WelcomeTour.svelteinfrastructure/eid-wallet/src/routes/(app)/main/legacy/EPassportSection.svelteinfrastructure/eid-wallet/src/routes/(app)/main/legacy/KycUpgradeOverlay.svelteinfrastructure/eid-wallet/src/routes/(app)/main/legacy/MarketplaceBanner.svelteinfrastructure/eid-wallet/src/routes/(app)/notifications/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/components/AddKnowledgeSheet.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/components/AddParametersSheet.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/components/AddPhotoSheet.svelteinfrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.tsinfrastructure/eid-wallet/src/routes/(app)/settings/+layout.svelteinfrastructure/eid-wallet/src/routes/(app)/settings/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/settings/language/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/social-bindings/+page.svelteinfrastructure/eid-wallet/src/routes/(auth)/+layout.svelteinfrastructure/eid-wallet/src/routes/(auth)/login/+page.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/NameInput.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/PinCreate.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/PinRepeat.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/StepHeader.svelteinfrastructure/eid-wallet/src/routes/+layout.svelteinfrastructure/eid-wallet/src/routes/+page.svelteinfrastructure/evault-core/package.jsoninfrastructure/evault-core/src/config/database.tsinfrastructure/evault-core/src/core/protocol/graphql-server.tsinfrastructure/evault-core/src/core/protocol/typedefs.tsinfrastructure/evault-core/src/core/types/binding-document.tsinfrastructure/evault-core/src/core/utils/security-answer.spec.tsinfrastructure/evault-core/src/core/utils/security-answer.tsinfrastructure/evault-core/src/entities/SecurityAnswerAttempt.tsinfrastructure/evault-core/src/migrations/1779517729310-SecurityAnswerAttempt.tsinfrastructure/evault-core/src/services/BindingDocumentService.tsinfrastructure/evault-core/src/services/SecurityQuestionService.spec.tsinfrastructure/evault-core/src/services/SecurityQuestionService.tspackage.json
💤 Files with no reviewable changes (1)
- infrastructure/eid-wallet/src/app.html
Comment on lines
+52
to
+90
| const attempt = await this.loadOrCreateAttempt(eName); | ||
|
|
||
| const now = new Date(); | ||
| if (attempt.lockedUntil && attempt.lockedUntil > now) { | ||
| return { | ||
| success: false, | ||
| reason: "locked", | ||
| lockedUntil: attempt.lockedUntil, | ||
| }; | ||
| } | ||
|
|
||
| const doc = await this.bindingDocumentService.getBindingDocument( | ||
| metaEnvelopeId, | ||
| eName, | ||
| ); | ||
| if (!doc) { | ||
| return { success: false, reason: "not_found" }; | ||
| } | ||
| if (doc.type !== "security_question") { | ||
| return { success: false, reason: "invalid_doc" }; | ||
| } | ||
| const data = doc.data as BindingDocumentSecurityQuestionData; | ||
|
|
||
| const matched = await verifyAnswer(data.answerHash, candidate); | ||
|
|
||
| if (matched) { | ||
| await this.recordSuccess(attempt); | ||
| return { success: true }; | ||
| } | ||
|
|
||
| const newCount = attempt.failedCount + 1; | ||
| attempt.failedCount = newCount; | ||
| attempt.lastAttemptAt = now; | ||
| let lockedUntil: Date | null = null; | ||
| if (newCount >= this.maxFailedAttempts) { | ||
| lockedUntil = new Date(now.getTime() + this.lockoutDurationMs); | ||
| attempt.lockedUntil = lockedUntil; | ||
| } | ||
| await this.attemptRepository.save(attempt); |
Contributor
There was a problem hiding this comment.
Make failed-attempt updates atomic.
Parallel validate() calls can all load the same row, verify a wrong answer, and each save the same failedCount + 1. That undercounts failures and lets concurrent guesses bypass the intended lockout threshold. This needs a transaction/row lock or another atomic update strategy around the attempt record.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@infrastructure/evault-core/src/services/SecurityQuestionService.ts` around
lines 52 - 90, The current validate flow (loadOrCreateAttempt → verifyAnswer →
update attempt → attemptRepository.save) is vulnerable to race conditions; make
the failed-attempt updates atomic by wrapping the verify+update in a DB
transaction or by performing an atomic DB update instead of a read-modify-write.
Concretely, either: (A) start a transaction, reload the attempt with a row lock
(FOR UPDATE) before checking/setting failedCount/lockedUntil and commit after
attemptRepository.save, or (B) replace the read-modify-write with a single
atomic update query (e.g., UPDATE ... SET failedCount = failedCount + 1,
lastAttemptAt = ?, lockedUntil = CASE WHEN failedCount + 1 >= :max THEN ? ELSE
lockedUntil END WHERE id = :id RETURNING *), then use that returned row to
decide whether to call recordSuccess or return locked/failed results; apply this
change around loadOrCreateAttempt, verifyAnswer, recordSuccess and
attemptRepository.save to ensure correct concurrent behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
infrastructure/evault-core/src/services/SecurityQuestionService.ts (1)
52-97:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMake failed-attempt updates atomic.
loadOrCreateAttempt()plus in-memory increment/reset plussave()is still a read-modify-write sequence. Parallelvalidate()calls can observe the samefailedCount, overwrite each other, and delay lockout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/evault-core/src/services/SecurityQuestionService.ts` around lines 52 - 97, The read-modify-write in validate (using loadOrCreateAttempt, in-memory edits, then attemptRepository.save) is not atomic and allows race conditions; replace this with a single atomic update via the persistence layer (e.g., use an atomic findOneAndUpdate/UPDATE ... WHERE ... RETURNING or repository.increment with a conditional set) to increment failedCount, set lastAttemptAt, and set lockedUntil when threshold reached, and use a separate atomic path for success (recordSuccess) so concurrent validate() calls cannot overwrite each other; locate usages around loadOrCreateAttempt, attemptRepository.save, recordSuccess, and the maxFailedAttempts/lockoutDurationMs logic to implement the atomic DB operation.infrastructure/evault-core/src/services/BindingDocumentService.ts (1)
127-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
answerHashvalues that did not come from the shared hashing flow.This still only enforces “trimmed non-empty string”, so callers can persist a
security_questiondocument with an arbitraryanswerHash. That pushes bad data into storage and leaves recovery validation to fail later instead of being rejected here at write time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/evault-core/src/services/BindingDocumentService.ts` around lines 127 - 147, The code currently only checks that d.answerHash is a trimmed non-empty string; instead, import and call the shared security-answer validation helper (the util in core/utils/security-answer) to assert the hash was produced by the shared hashing flow (e.g., use the util's isValid/validate function) and throw a ValidationError if that helper rejects it; keep using the trimmed value for the returned BindingDocumentSecurityQuestionData (answerHash) and reference d.answerHash and the BindingDocumentSecurityQuestionData shape when making the change.
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(app)/settings/+layout.svelte (1)
14-16: ⚡ Quick winAvoid hardcoding the app version in the layout.
This will drift on the next release and show users the wrong version. Please source it from shared build/runtime metadata instead of a literal in the component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/eid-wallet/src/routes/`(app)/settings/+layout.svelte around lines 14 - 16, Replace the hardcoded VERSION constant and usage in subtitleAtMount with a runtime/build-supplied value: remove const VERSION = "0.7.1" and instead import the app version from the shared build/runtime metadata module or environment variable used by the project (e.g., the shared version export or import.meta.env variable), then use that imported symbol in subtitleAtMount (the same variable name you import, e.g., BUILD_VERSION) so the layout reads the real build version at runtime rather than a literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/eid-wallet/src/routes/`(app)/settings/pin/+page.svelte:
- Around line 76-86: The header back handler runtime.header.onback currently
calls window.history.back() unconditionally; change it to mirror handleClose()
by checking if (window.history.length > 1) then window.history.back() else
goto("/settings") so direct-entry/deep-link flows return to /settings instead of
leaving nothing to pop; apply the same fallback update to the other header/back
handler block referenced (the one around the 94-107 region) so both
runtime.header.onback and any duplicate back handlers use the same
history-length check and goto("/settings") fallback.
---
Duplicate comments:
In `@infrastructure/evault-core/src/services/BindingDocumentService.ts`:
- Around line 127-147: The code currently only checks that d.answerHash is a
trimmed non-empty string; instead, import and call the shared security-answer
validation helper (the util in core/utils/security-answer) to assert the hash
was produced by the shared hashing flow (e.g., use the util's isValid/validate
function) and throw a ValidationError if that helper rejects it; keep using the
trimmed value for the returned BindingDocumentSecurityQuestionData (answerHash)
and reference d.answerHash and the BindingDocumentSecurityQuestionData shape
when making the change.
In `@infrastructure/evault-core/src/services/SecurityQuestionService.ts`:
- Around line 52-97: The read-modify-write in validate (using
loadOrCreateAttempt, in-memory edits, then attemptRepository.save) is not atomic
and allows race conditions; replace this with a single atomic update via the
persistence layer (e.g., use an atomic findOneAndUpdate/UPDATE ... WHERE ...
RETURNING or repository.increment with a conditional set) to increment
failedCount, set lastAttemptAt, and set lockedUntil when threshold reached, and
use a separate atomic path for success (recordSuccess) so concurrent validate()
calls cannot overwrite each other; locate usages around loadOrCreateAttempt,
attemptRepository.save, recordSuccess, and the
maxFailedAttempts/lockoutDurationMs logic to implement the atomic DB operation.
---
Nitpick comments:
In `@infrastructure/eid-wallet/src/routes/`(app)/settings/+layout.svelte:
- Around line 14-16: Replace the hardcoded VERSION constant and usage in
subtitleAtMount with a runtime/build-supplied value: remove const VERSION =
"0.7.1" and instead import the app version from the shared build/runtime
metadata module or environment variable used by the project (e.g., the shared
version export or import.meta.env variable), then use that imported symbol in
subtitleAtMount (the same variable name you import, e.g., BUILD_VERSION) so the
layout reads the real build version at runtime rather than a literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91b16c51-c555-4825-abc8-b198d2338148
📒 Files selected for processing (30)
.gitignoreinfrastructure/eid-wallet/src-tauri/Cargo.tomlinfrastructure/eid-wallet/src/lib/stores/notifications.tsinfrastructure/eid-wallet/src/lib/ui/BottomSheet/BottomSheet.svelteinfrastructure/eid-wallet/src/lib/ui/Button/ButtonIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/ChevronIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/PinIcon.svelteinfrastructure/eid-wallet/src/lib/ui/icons/PrivacyIcon.svelteinfrastructure/eid-wallet/src/lib/utils/personalBinding.tsinfrastructure/eid-wallet/src/lib/utils/socialBinding.tsinfrastructure/eid-wallet/src/routes/(app)/main/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/ENameCard.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/Lasso.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/LegalIdAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/PersonalBindingAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/SocialBindingAccordion.svelteinfrastructure/eid-wallet/src/routes/(app)/main/components/SocialBindingDrawer.svelteinfrastructure/eid-wallet/src/routes/(app)/main/legacy/MarketplaceBanner.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/components/AddKnowledgeSheet.svelteinfrastructure/eid-wallet/src/routes/(app)/personal/components/AddPhotoSheet.svelteinfrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.tsinfrastructure/eid-wallet/src/routes/(app)/settings/+layout.svelteinfrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelteinfrastructure/eid-wallet/src/routes/(app)/social-bindings/+page.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelteinfrastructure/eid-wallet/src/routes/(auth)/onboarding/steps/NameInput.svelteinfrastructure/eid-wallet/src/routes/+layout.svelteinfrastructure/evault-core/src/services/BindingDocumentService.tsinfrastructure/evault-core/src/services/SecurityQuestionService.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
This branch is the eid-wallet redesign (Figma-driven UI overhaul) plus the first iteration of the Personal binding documents feature end-to-end.
Wallet redesign (UI)
Onboarding + recovery scaffolding (existing flows, polished)
/passphrase/set,/passphrase/compare, IP-rate-limited).Personal binding documents — new feature
personal_parameters(free-text card),security_question(user-authored question + Argon2id-hashed answer); optionaldescriptionfield on the existingphotographtype.SecurityQuestionServicewith eName-keyed lockout state machine (configurablemaxFailedAttempts/lockoutDurationMs).security-answerutil: NFKC → trim → collapse whitespace → lowercase → strip punctuation → Argon2id. Unicode-safe (accents + non-Latin scripts preserved).SecurityAnswerAttemptand auto-generated TypeORM migration. Migration is additive for the new table and also bundles the pre-existing drift cleanup (notifications.bodyvarchar → text,device_tokenindex rename) since drift was already present and harmless to fix here.createBindingDocumentaccepts the three new types via the existingaccessGuard.middleware;hashSecurityAnswer(answer)andvalidateSecurityAnswer(metaEnvelopeId, candidate)wrap the util and the service./personalroute with three cards (Distinctive photos, Personal parameters, Unique knowledge) backed by a sharedmarkCardsnippet.AddPhotoSheetwith camera/gallery sources, live viewfinder, capture preview, description field.AddParametersSheetandAddKnowledgeSheet.PersonalBindingAccordionon the main page (replaces the inline placeholder row).lib/utils/personalBinding.ts: typed parallel GraphQL queries (bindingDocuments(type: ...)) with latest-signature dedupe for single-instance types./mainand/personalmount.Issue Number
[fill in]
Type of change
descriptionargon2added topnpm.onlyBuiltDependenciesHow the change has been tested
infrastructure/evault-core/for the security-answer pipeline (NFKC, whitespace/casing/punctuation, Unicode preservation, hash round-trip, malformed-hash fallthrough) and theSecurityQuestionServicestate machine (correct answer clears prior failures, mismatch decrements remaining attempts, threshold locks, locked state refuses even correct answers until expiry, not_found / invalid_doc paths). All passing in ~1.7s.svelte-checkclean on the wallet (0 errors / 9 pre-existing warnings unrelated to this work).biome lintclean on all new/touched files.pnpm --filter evault-core migration:generate, then applied withmigration:runagainst the local Postgres — single transaction, committed cleanly./main→ photo persists; set personal parameters → reload → text persists; set a security question → reload → question text persists with the answer masked; edit each card → previous doc is replaced; delete a photo → removed from store + vault.Change checklist
⚠ Known gap — account recovery via security question
The Figma design includes new restore screens (
Restore — Unverified ID → Enter your eName → Enter answer to "Mother child surname") which are not wired up in this PR. They will be handled in a follow-up tracked by #971 (eID Wallet: Add passphrase-free eVault recovery via newly generated keys).What this PR contributes as foundation work for #971:
security_questionbinding-doc type — where the recoverable secret lives.SecurityQuestionServicewith per-eName lockout — the gate that proves "you are who you claim" before fresh keys are issued.hashSecurityAnswer/validateSecurityAnswerGraphQL mutations — the primitives the recovery flow will call.What #971 still needs to add on top:
validateSecurityAnswersits behindaccessGuard.middleware, which a recovering user can't satisfy).syncPublicKey(same final step as the existing passphrase flow).Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements