brave-core icon indicating copy to clipboard operation
brave-core copied to clipboard

Published 20 hours ago • verified publisher icon brave

fix(wallet): limit account name character length

Open josheleonard opened this issue 11 months ago • 18 comments

Resolves https://github.com/brave/brave-browser/issues/23361 Resolves https://github.com/brave/brave-browser/issues/37362

Submitter Checklist:

  • [x] I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • [x] There is a ticket for my issue
  • [x] Used Github auto-closing keywords in the PR description above
  • [x] Wrote a good PR/commit description
  • [ ] Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • [x] Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • [x] Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • [ ] Ran git rebase master (if needed)

Reviewer Checklist:

  • [ ] A security review is not needed, or a link to one is included in the PR description
  • [ ] New files have MPL-2.0 license header
  • [ ] Adequate test coverage exists to prevent regressions
  • [ ] Major classes, functions and non-trivial code blocks are well-commented
  • [ ] Changes in component dependencies are properly reflected in gn
  • [ ] Code follows the style guide
  • [ ] Test plan is specified in PR before merging

After-merge Checklist:

  • [ ] The associated issue milestone is set to the smallest version that the changes has landed on
  • [ ] All relevant documentation has been updated, for instance:
    • [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
    • [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
    • [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
    • [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
    • [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
    • [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
    • [ ] https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

Scenario 1 (Create Account)

  1. Go to accounts page
  2. Click the "+" icon
  3. click "Create account"
  4. Select a coin type
  5. enter an account name over 30 characters long
  6. an error message should be shown and the submit button should be disabled until the name is 30 characters or shorter
  7. Submit button should continue to work (creates an account with the supplied name)
  8. repeat for all coin types

Scenario 2 (Import Account)

  1. Go to accounts page
  2. Click the "+" icon
  3. click "Import account"
  4. Select a coin type
  5. enter an account name over 30 characters long
  6. an error message should be shown and the submit button should be disabled until the name is 30 characters or shorter
  7. enter a private key
  8. Submit button should continue to work (imports the private key as an account with the supplied name)
  9. repeat for all coin types
  10. repeat using a JSON keystore file instead of a pasted private key (Import type)

Scenario 3 (Edit Account)

  1. Go to accounts page
  2. Click the kabab icon on an account
  3. click "Edit"
  4. enter an account name over 30 characters long
  5. an error message should be shown and the submit button should be disabled until the name is 30 characters or shorter
  6. Submit button should continue to work (edits the account to use the newly supplied name)

Scenario 4 (View Private key)

  1. Go to accounts page
  2. Click the kabab icon on an account
  3. click "Export"
  4. enter you wallet password
  5. click "show key"
  6. private key should be revealed (should not be revealed if password is incorrect, should lock wallet after 3 incorrect attempts)

Scenario 4 (Remove account)

  1. Go to accounts page
  2. Click the kabab icon on an imported account that can be removed
  3. click "Remove"
  4. enter you wallet password
  5. click "continue"
  6. the account should be removed (should not be removed if password is incorrect, should lock wallet after 3 incorrect attempts)

josheleonard avatar Apr 03 '24 18:04 josheleonard

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 03 '24 19:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 04 '24 11:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 04 '24 15:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 04 '24 18:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 04 '24 20:04 brave-builds

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password" and so security team members have been added as reviewers to take a look.
No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.
Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

github-actions[bot] avatar Apr 09 '24 13:04 github-actions[bot]

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 09 '24 14:04 brave-builds

Low risk: Account and password length limits were added. Ensure the backend also enforces these limits to prevent truncation attacks. The UI code should also gracefully handle names longer than 30 chars if they already exist in storage.

Are we checking for password length or is it an AI flutter? @josheleonard

thypon avatar Apr 09 '24 15:04 thypon

Low risk: Account and password length limits were added. Ensure the backend also enforces these limits to prevent truncation attacks. The UI code should also gracefully handle names longer than 30 chars if they already exist in storage.

Are we checking for password length or is it an AI flutter? @josheleonard

@thypon, this is AI flutter. This PR only adds checks for the account name length

josheleonard avatar Apr 09 '24 17:04 josheleonard

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 09 '24 20:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 09 '24 22:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 11 '24 12:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 12 '24 12:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 22 '24 18:04 brave-builds

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar Apr 22 '24 19:04 brave-builds

[puLL-Merge] - brave/brave-core@22907

Description

This PR adds new Brave Wallet related strings to brave_wallet_constants.h and brave_wallet.mojom. It also makes UI changes to the account settings modal, add account modal, and import account modal in the wallet extension.

The main motivation appears to be improving the UX around entering account names by adding validation and character limit checks. Some of the modal components are also refactored to use new shared button and input components.

Changes

Changes

brave_wallet_constants.h

  • Adds new string constants for private key import type label, enter password if applicable label, and account name too long error message

brave_wallet.mojom

  • Adds a new constant kAccountNameMaxCharacterLength to specify the max allowed length for account names

account-settings-modal.tsx

  • Refactors to use new shared Input and Dropdown components
  • Adds validation for account name length
  • Switches to using LeoSquaredButton instead of NavButton
  • Uses new Alert component for warnings/disclaimers

add-imported-account-modal.tsx

  • Similar changes as account settings modal - uses new input components, adds name length validation
  • Switches import type selector to use Dropdown component

create-account-modal.tsx

  • Also refactors to use new input components and adds name length validation
  • Changes network selector to Dropdown component

confirm-password-modal/remove-account-modal.tsx

  • Minor refactor and rename of component to RemoveAccountModal

Possible Issues

The max account name length is now defined in both the brave_wallet.mojom and in each UI component that uses it. This works but is a bit fragile - if the mojom constant is updated, the UI will also need to be updated in several places. Using a shared constant could help mitigate this.

github-actions[bot] avatar May 09 '24 11:05 github-actions[bot]

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password" and so security team members have been added as reviewers to take a look.
No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.
Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

github-actions[bot] avatar May 09 '24 11:05 github-actions[bot]

A Storybook has been deployed to preview UI for the latest push

brave-builds avatar May 09 '24 12:05 brave-builds