fix(wallet/recovery): require manual reveal before showing seed phrase (#2657)#2661
Closed
oxoxDev wants to merge 2 commits into
Closed
fix(wallet/recovery): require manual reveal before showing seed phrase (#2657)#2661oxoxDev wants to merge 2 commits into
oxoxDev wants to merge 2 commits into
Conversation
…se (tinyhumansai#2657) Blur the 12-word grid by default and disable copy until the user clicks an eye-slash overlay. Pointer-events and user-select are blocked on the hidden grid so the words can't be highlighted or copied via context menu before reveal. The reveal gesture is one-way per session — switching modes re-arms the gate. Defense in depth: `handleCopy` short-circuits if `revealed === false` even if the disabled attribute is bypassed (some AT tooling can route around `disabled`).
Five new Vitest cases covering the manual-reveal contract: - mnemonic grid is blurred + aria-hidden by default - Copy to Clipboard is disabled while phrase is hidden - click on reveal overlay un-blurs and enables Copy (one-way) - writeText is only called after reveal - mode switch re-arms the gate Locked palette token + existing trust-surface tests untouched.
Contributor
📝 WalkthroughWalkthroughThe PR implements a security-focused one-way reveal gate for the recovery phrase: the mnemonic is hidden behind a blur overlay and marked ChangesRecovery Phrase Reveal Gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![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: 1
🤖 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 `@app/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsx`:
- Around line 145-150: The test "writes the mnemonic to clipboard only after
reveal" mutates navigator.clipboard and doesn't restore it; save the original
clipboard (e.g., const originalClipboard = navigator.clipboard) before calling
Object.defineProperty, use your mocked writeText vi.fn for the test, and restore
navigator.clipboard to originalClipboard in a finally block or an afterEach
cleanup so the global is returned to its prior state (reference the test name
and the mocked writeText/navigator.clipboard setup to locate where to add the
save/restore).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a843d4d9-8e50-4a22-a8d0-571bf965f9ec
📒 Files selected for processing (2)
app/src/components/settings/panels/RecoveryPhrasePanel.tsxapp/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsx
Comment on lines
+145
to
+150
| it('writes the mnemonic to clipboard only after reveal', async () => { | ||
| const writeText = vi.fn<(text: string) => Promise<void>>(async () => { | ||
| return; | ||
| }); | ||
| Object.defineProperty(navigator, 'clipboard', { configurable: true, value: { writeText } }); | ||
| renderWithProviders(<RecoveryPhrasePanel />); |
Contributor
There was a problem hiding this comment.
Restore navigator.clipboard after this test to avoid global leakage.
This test mutates a global and never restores it, which can make later tests order-dependent.
Suggested fix
it('writes the mnemonic to clipboard only after reveal', async () => {
+ const originalClipboard = Object.getOwnPropertyDescriptor(navigator, 'clipboard');
const writeText = vi.fn<(text: string) => Promise<void>>(async () => {
return;
});
Object.defineProperty(navigator, 'clipboard', { configurable: true, value: { writeText } });
- renderWithProviders(<RecoveryPhrasePanel />);
-
- fireEvent.click(screen.getByRole('button', { name: /reveal phrase/i }));
- const copyBtn = screen
- .getAllByRole('button')
- .find(b => /copy to clipboard/i.test(b.textContent || ''))!;
- fireEvent.click(copyBtn);
-
- expect(writeText).toHaveBeenCalledTimes(1);
- const arg = writeText.mock.calls[0]?.[0] ?? '';
- // BIP-39 mnemonics are space-separated words.
- expect(arg.split(' ').length).toBeGreaterThanOrEqual(12);
+ try {
+ renderWithProviders(<RecoveryPhrasePanel />);
+ fireEvent.click(screen.getByRole('button', { name: /reveal phrase/i }));
+ const copyBtn = screen
+ .getAllByRole('button')
+ .find(b => /copy to clipboard/i.test(b.textContent || ''))!;
+ fireEvent.click(copyBtn);
+
+ expect(writeText).toHaveBeenCalledTimes(1);
+ const arg = writeText.mock.calls[0]?.[0] ?? '';
+ expect(arg.split(' ').length).toBeGreaterThanOrEqual(12);
+ } finally {
+ if (originalClipboard) {
+ Object.defineProperty(navigator, 'clipboard', originalClipboard);
+ }
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('writes the mnemonic to clipboard only after reveal', async () => { | |
| const writeText = vi.fn<(text: string) => Promise<void>>(async () => { | |
| return; | |
| }); | |
| Object.defineProperty(navigator, 'clipboard', { configurable: true, value: { writeText } }); | |
| renderWithProviders(<RecoveryPhrasePanel />); | |
| it('writes the mnemonic to clipboard only after reveal', async () => { | |
| const originalClipboard = Object.getOwnPropertyDescriptor(navigator, 'clipboard'); | |
| const writeText = vi.fn<(text: string) => Promise<void>>(async () => { | |
| return; | |
| }); | |
| Object.defineProperty(navigator, 'clipboard', { configurable: true, value: { writeText } }); | |
| try { | |
| renderWithProviders(<RecoveryPhrasePanel />); | |
| fireEvent.click(screen.getByRole('button', { name: /reveal phrase/i })); | |
| const copyBtn = screen | |
| .getAllByRole('button') | |
| .find(b => /copy to clipboard/i.test(b.textContent || ''))!; | |
| fireEvent.click(copyBtn); | |
| expect(writeText).toHaveBeenCalledTimes(1); | |
| const arg = writeText.mock.calls[0]?.[0] ?? ''; | |
| expect(arg.split(' ').length).toBeGreaterThanOrEqual(12); | |
| } finally { | |
| if (originalClipboard) { | |
| Object.defineProperty(navigator, 'clipboard', originalClipboard); | |
| } | |
| } | |
| }); |
🤖 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 `@app/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsx`
around lines 145 - 150, The test "writes the mnemonic to clipboard only after
reveal" mutates navigator.clipboard and doesn't restore it; save the original
clipboard (e.g., const originalClipboard = navigator.clipboard) before calling
Object.defineProperty, use your mocked writeText vi.fn for the test, and restore
navigator.clipboard to originalClipboard in a finally block or an afterEach
cleanup so the global is returned to its prior state (reference the test name
and the mocked writeText/navigator.clipboard setup to locate where to add the
save/restore).
Contributor
Author
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.
Summary
disabledattr +aria-disabled+ sync guard insidehandleCopyfor AT bypass).Problem
When the user navigates to Settings → Account → Recovery Phrase, the 12-word seed is rendered in plain text immediately. Anyone standing next to the user can read or photograph it before the user has any chance to hide it or move to a private location. This is a small but real shoulder-surfing exposure on a high-blast-radius secret (full wallet recovery).
Solution
revealedboolean state inRecoveryPhrasePanel, defaultfalse.blur-md,pointer-events-none,select-none, andaria-hidden="true"while not revealed — words can't be highlighted, copied via context menu, or focused.mnemonic.reveali18n key (no new strings needed;mnemonic.reveal/mnemonic.hiddenwere already inen.tsand all locale chunks).disabled,aria-disabled="true", getsopacity-50 cursor-not-allowed, and has a hover-title usingmnemonic.hiddenwhile hidden.handleCopyalso short-circuits ifrevealed === false— defence in depth in case AT or scripted activation bypasses thedisabledattribute.switchModeresetsrevealedtofalseso a re-entry through import → generate re-arms the gate.Submission Checklist
RecoveryPhrasePanel — manual reveal gate (#2657)covering hidden-default, copy-disabled, reveal-click, clipboard write only after reveal, mode-switch re-arms.RecoveryPhrasePanel.tsx: therevealedstate, the conditional grid classes, the overlay button, the disabled-copy path, thehandleCopyguard, and theswitchModereset. 11 / 11 tests pass onpnpm debug unit RecoveryPhrasePanel.docs/TEST-COVERAGE-MATRIX.md.## Related— same reason as above.Closes #NNNin the## Relatedsection — see below.Impact
mnemonic.revealandmnemonic.hiddeni18n keys already shipped in every locale chunk — zero translator work needed.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2657-recovery-phrase-blur6578565e(tip; 2 commits in the series)Validation Run
pnpm --filter openhuman-app format:check— clean (prettier --check + rust:format:check both pass).pnpm typecheck— clean (tsc --noEmit, 0 errors).pnpm debug unit RecoveryPhrasePanel— 11 passed / 0 failed, including 5 newmanual reveal gate (#2657)cases.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
if (!revealed) return;inhandleCopymirrors the standard "tool guard before action" pattern; reviewers can diff this against any other ReadOnly guard in the codebase for visual parity.Duplicate / Superseded PR Handling
Summary by CodeRabbit