Skip to content

fix(security): prevent PIN deletion vulnerability when removing wallet + UI improvements#648

Merged
joemarct merged 4 commits intomasterfrom
improvements/general-ui-ux
Mar 19, 2026
Merged

fix(security): prevent PIN deletion vulnerability when removing wallet + UI improvements#648
joemarct merged 4 commits intomasterfrom
improvements/general-ui-ux

Conversation

@joemarct
Copy link
Copy Markdown
Member

Summary

This PR includes a critical security fix along with minor UI improvements.

Security Fix: PIN Deletion Vulnerability

Problem: When deleting an imported wallet, the app was deleting the legacy global PIN key ('pin') that may still be used by older wallets. This caused users to unexpectedly lose their PIN authentication on their original wallet.

Root Cause: Older wallets created before the wallet-specific PIN migration used a shared global PIN storage key. When any wallet was deleted, this global key was deleted along with wallet-specific keys, affecting other wallets that relied on it.

User Impact: Users with older wallets would be prompted to set up a new PIN after deleting a temporary/imported wallet, even though their original wallet already had a PIN configured.

Fix: Modified deleteAllWalletData() in src/wallet/index.js to only delete wallet-specific PIN keys (pin-{hash} and pin {mnemonic}), preserving the legacy 'pin' key for backward compatibility.

UI Improvements

  • Applied text-grad class to icons in Get BCH and Spend BCH pages for theme consistency
  • Adjusted header-nav mobile safe area padding to match home page

Files Changed

  • src/wallet/index.js - Security fix for PIN deletion
  • src/pages/apps/get-bch.vue - Theme consistency
  • src/pages/apps/spend-bch.vue - Theme consistency
  • src/components/header-nav.vue - Padding fix

Testing Notes

To test the security fix:

  1. Have an existing wallet with PIN configured
  2. Import another wallet temporarily
  3. Delete the temporary wallet
  4. Verify the original wallet still has working PIN authentication (not prompted to set up new PIN)

Fixes security vulnerability reported via user feedback.

Reduce top padding from platform-specific values (73px/29px iOS, 18px Android)
to a consistent 9px above safe area on both iOS and Android to align with
home page content spacing.
…g wallet

When deleting an imported wallet, the code was deleting the legacy 'pin'
key that may still be used by older wallets that haven't migrated to
wallet-specific PINs. This caused users to lose their existing PIN
unexpectedly and be prompted to set up a new PIN.

The fix ensures only wallet-specific PIN keys are deleted, preserving
the legacy 'pin' key for backward compatibility with older wallets.
@opencode-agent
Copy link
Copy Markdown

PR Review Summary

✅ Security Fix - APPROVED

File: src/wallet/index.js:192-200

The security fix is correct and critical. By removing the legacy 'pin' key from the deletion list, you prevent unintended PIN loss on older wallets that share this global key. The comment clearly explains the security rationale.

Before:

'pin'  // Would delete shared global PIN

After:

// REMOVED: 'pin' - legacy global key may be used by other wallets

✅ UI Improvements - APPROVED

Theme Consistency (get-bch.vue & spend-bch.vue):

  • Replaced hardcoded color="primary" with class="text-grad"
  • Removed hardcoded hex colors (#3B7BF6) with !important
  • Icons now properly adapt to light/dark themes

Header Padding (header-nav.vue):

  • Consistent padding across iOS/Android (safeTop + 9px)
  • Height calculations simplified and aligned with home page
  • Better mobile safe area handling

⚠️ Minor Suggestions

  1. Consider a migration comment: Add a TODO to eventually migrate all wallets to wallet-specific PIN keys so the legacy 'pin' key can be safely removed in the future.

  2. Testing verification: Ensure text-grad class is defined in the project's CSS/SCSS variables.

Verdict

Approve - The security fix is the priority here and is implemented correctly. UI improvements are clean and consistent.

New%20session%20-%202026-03-19T09%3A07%3A16.822Z
opencode session  |  github run

Add comment noting that after all wallets migrate to wallet-specific PIN
keys, the legacy 'pin' key can be safely removed.
@opencode-agent
Copy link
Copy Markdown

PR Review Summary

✅ Security Fix - APPROVED

File: src/wallet/index.js:192-212

The security fix correctly addresses the PIN deletion vulnerability by only removing wallet-specific PIN keys (pin-${sha256(mnemonic)} and pin ${mnemonic}) while preserving the legacy global 'pin' key. The clear comment and TODO provide excellent documentation for future migration.

✅ UI Improvements - APPROVED

Theme Consistency (get-bch.vue & spend-bch.vue):

  • Icons now use class="text-grad" instead of hardcoded color="primary"
  • Better theme adaptation for light/dark modes

Header Padding (header-nav.vue:126-150):

  • Unified safe area padding (safeTop + 9px) across iOS/Android
  • Aligns with home page styling
  • Proper safe area handling using env() and CSS variables

Verdict

Approve - The security fix is critical and correctly implemented. UI improvements are clean and follow project conventions.

New%20session%20-%202026-03-19T09%3A10%3A08.611Z
opencode session  |  github run

@joemarct joemarct merged commit 4b7ce63 into master Mar 19, 2026
1 check passed
@joemarct joemarct deleted the improvements/general-ui-ux branch March 19, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant