-
Notifications
You must be signed in to change notification settings - Fork 161
Recovery phrase unverified status based on last_authentication.
#3519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…recovery-phrase' into sea-snake/persistent-unverified-recovery-phrase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the recovery phrase verification system to support two verification methods: selecting words from a shuffled list (when the phrase is in memory) and typing the entire phrase (when signing in again after sign-out). The key change is using last_authentication field to determine if a recovery phrase has been verified (null = unverified).
- Renamed
Verifycomponent toVerifySelectingand created newVerifyTypingcomponent for manual phrase entry - Implemented reusable
RecoveryPhraseInputcomponent with accessibility features and validation - Updated verification logic to call the canister with the recovery phrase identity to mark it as used
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/recovery/+page.svelte |
Updates unverified status detection to use last_authentication field; implements handleVerify to authenticate with recovery phrase and mark as used |
src/frontend/src/lib/components/wizards/createRecoveryPhrase/views/VerifyTyping.svelte |
New component for manual recovery phrase entry with auto-submit and validation |
src/frontend/src/lib/components/wizards/createRecoveryPhrase/views/VerifySelecting.svelte |
Minor text formatting change adding colon to instruction |
src/frontend/src/lib/components/wizards/createRecoveryPhrase/views/Retry.svelte |
Adds verification method prop to show different error messages for selecting vs typing |
src/frontend/src/lib/components/wizards/createRecoveryPhrase/CreateRecoveryPhraseWizard.svelte |
Adds action prop and error handling; routes between VerifySelecting and VerifyTyping based on phrase availability |
src/frontend/src/lib/components/views/RecoveryPhraseInput.svelte |
New reusable component for entering 24-word recovery phrase with paste support, keyboard navigation, and validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (error) { | ||
| if ( | ||
| isCanisterError<IdentityInfoError>(error) && | ||
| error.type == "Unauthorized" |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using == for type comparison instead of ===. This should be strict equality (===) to avoid potential type coercion issues when comparing error types.
| error.type == "Unauthorized" | |
| error.type === "Unauthorized" |
| const retryVerification = () => { | ||
| isWritten = false; | ||
| isIncorrect = false; | ||
| }; |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retryVerification function sets isWritten = false which would cause the wizard to show the Write component. However, when recoveryPhrase === undefined (typing mode), there's nothing to write. This will cause unexpected behavior. Consider handling the retry differently based on verification mode or resetting incorrectRecoveryPhrase instead.
| <input | ||
| inputmode="text" | ||
| autocorrect="off" | ||
| autocomplete="off" | ||
| autocapitalize="off" | ||
| spellcheck="false" | ||
| bind:value={ | ||
| () => word, | ||
| (v) => { | ||
| words[index] = v.toLowerCase().replace(/[^a-z]/g, ""); | ||
| words = [...words]; | ||
| } | ||
| } | ||
| onkeydown={(event) => handleKeyDown(event, index)} | ||
| onpaste={(event) => handlePaste(event, index)} | ||
| pattern={dictionary?.join("|")} | ||
| {disabled} | ||
| class={[ | ||
| "peer h-7 w-full ps-8 pe-2", | ||
| "text-text-primary bg-transparent text-base ring-0 outline-none", | ||
| "border-border-primary rounded-full", | ||
| "focus:not-disabled:border-fg-primary", | ||
| "not-focus:user-invalid:!border-border-error not-focus:user-invalid:!bg-bg-error-primary/30 not-focus:user-invalid:!pe-7", | ||
| !showAll && | ||
| "not-focus:valid:!text-transparent disabled:!text-transparent", | ||
| "disabled:!text-text-disabled disabled:!bg-bg-disabled disabled:!border-border-disabled_subtle", | ||
| word.length > 7 && "tracking-tight", | ||
| ]} | ||
| data-lpignore="true" | ||
| data-1p-ignore="true" | ||
| data-bwignore="true" | ||
| data-form-type="other" | ||
| /> |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input fields lack accessible labels. While the visual labels show the word number (lines 120-124), they are marked as aria-hidden="true". Screen reader users won't know which word position they're entering. Consider adding aria-label attributes to each input, e.g., aria-label={\Word ${index + 1}`}`.
| } | ||
| onkeydown={(event) => handleKeyDown(event, index)} | ||
| onpaste={(event) => handlePaste(event, index)} | ||
| pattern={dictionary?.join("|")} |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern attribute uses dictionary?.join("|") which creates a potentially very long regex pattern (2048 words joined by |) on every render. Consider memoizing this value or using the datalist element instead for better performance and user experience with autocomplete suggestions.
| <script lang="ts"> | ||
| import Tooltip from "$lib/components/ui/Tooltip.svelte"; | ||
| import { InfoIcon } from "@lucide/svelte"; | ||
| import { t } from "$lib//stores/locale.store"; |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double slash in import path: $lib//stores/locale.store should be $lib/stores/locale.store
| import { t } from "$lib//stores/locale.store"; | |
| import { t } from "$lib/stores/locale.store"; |
| clipboard.forEach((word, i) => (words[index + i] = word)); | ||
| words = [...words]; | ||
| event.preventDefault(); | ||
| wrapperRef | ||
| ?.querySelectorAll("input") | ||
| [index + clipboard.length - 1]?.focus(); |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paste operation may write beyond the bounds of the words array if index + clipboard.length > words.length. This could lead to undefined behavior. Consider checking bounds before assignment or using slice to limit clipboard length: clipboard.slice(0, words.length - index).forEach(...)
| clipboard.forEach((word, i) => (words[index + i] = word)); | |
| words = [...words]; | |
| event.preventDefault(); | |
| wrapperRef | |
| ?.querySelectorAll("input") | |
| [index + clipboard.length - 1]?.focus(); | |
| const maxPaste = words.length - index; | |
| clipboard.slice(0, maxPaste).forEach((word, i) => (words[index + i] = word)); | |
| words = [...words]; | |
| event.preventDefault(); | |
| wrapperRef | |
| ?.querySelectorAll("input") | |
| [index + Math.min(clipboard.length, maxPaste) - 1]?.focus(); |
| event.preventDefault(); | ||
| wrapperRef | ||
| ?.querySelectorAll("input") | ||
| [index + clipboard.length - 1]?.focus(); |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After pasting, the focus position may go beyond the array bounds if index + clipboard.length > words.length. This could result in attempting to focus a non-existent input. Consider using Math.min(index + clipboard.length - 1, words.length - 1) to ensure the focus stays within bounds.
| [index + clipboard.length - 1]?.focus(); | |
| [Math.min(index + clipboard.length - 1, words.length - 1)]?.focus(); |
| const handleSubmit = () => { | ||
| isCheckingPhrase = true; | ||
| onCompleted(value); |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleSubmit function doesn't await onCompleted, which returns a Promise<void>. This means isCheckingPhrase will remain true indefinitely if the promise never resolves, and errors from onCompleted won't be caught. Consider using async/await or proper promise handling to reset isCheckingPhrase state.
| const handleSubmit = () => { | |
| isCheckingPhrase = true; | |
| onCompleted(value); | |
| const handleSubmit = async () => { | |
| isCheckingPhrase = true; | |
| try { | |
| await onCompleted(value); | |
| } catch (error) { | |
| // Optionally, handle error (e.g., show a message) | |
| console.error("Error in onCompleted:", error); | |
| } finally { | |
| isCheckingPhrase = false; | |
| } |
Recovery phrase unverified status based on
last_authentication.Changes
Verifycomponent toVerifySelecting.RecoveryPhraseInputcomponent.VerifyTyping.RecoveryPhraseWizardto use eitherVerifySelectingorVerifyTypingdepending on the availability of the valid recovery phrase in memory (e.g. user signed out and in -> not available)./manage/recoverypage to uselast_authenticationto decide if a recovery phrase is unverified (null = not used yet).Tests
No tests have been updated in this PR, existing e2e tests should pass in the CI/CD pipeline.
Notes
RecoveryPhraseInputcomponent will be used on theuse recovery phrase screenin a later PR.VerifyTypingwill be added in a later PR.