Skip to content

fix(security): Consistent 128‑char password handling and hardened auth validation#3149

Merged
CharlVS merged 6 commits intodevfrom
fix/password-bypass-v2
Sep 18, 2025
Merged

fix(security): Consistent 128‑char password handling and hardened auth validation#3149
CharlVS merged 6 commits intodevfrom
fix/password-bypass-v2

Conversation

@CharlVS
Copy link
Copy Markdown
Collaborator

@CharlVS CharlVS commented Sep 16, 2025

Description

Security/auth hardening

  • Enforce a unified passwordMaxLength = 128 across all auth/password flows by switching from LengthLimitingTextInputFormatter(40) to maxLength with hidden counters:
    • wallet_login.dart, password_dialog_content.dart, password_update_page.dart, wallet_deleting.dart, wallet_import_by_file.dart, trezor_dialog_select_wallet.dart
  • Gate wallet creation on valid passwords/confirmation:
    • CreationPasswordFields now uses passive validation and exposes onValidityChanged
    • wallet_creation.dart disables the Create button until both fields are valid
  • Improve auth flows and logging:

UI/UX and shared components

  • UiTextFormField: simplified validation behavior; uses AutovalidateMode per InputValidationMode; surfaces external errorText directly
  • Fixes/consistency:
    • Price formatting type safety in grouped_asset_ticker_item.dart
    • Text style/readability in coin_select_item_widget.dart

Manual QA checklist

  • Create, log in, update, delete, import wallets using passwords >40 chars (up to 128) across all dialogs.
  • Verify Create button only enables when password + confirmation are valid.

- Resolved duplicate maxLength property in password_update_page.dart
- Updated validation mode from eager to passive in creation_password_fields.dart
- Added onChanged callback for password validity notification
- Removed duplicate imports in both files
- Maintained consistent password field behavior across the application
…ants\n\n- Define passwordMaxLength=128 for UI inputs\n- Add lastLoggedInWalletKey for quick login feature\n- Update matrixIdRegex for stricter validation
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 16, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/password-bypass-v2

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@CharlVS CharlVS changed the title fix(password): resolve merge conflicts and enforce password max length fix(password): enforce 128-char limit and consistent validation across dialogs Sep 16, 2025
@CharlVS CharlVS requested a review from Copilot September 16, 2025 15:26
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 enforces a global 128-character password length limit across all password input dialogs and standardizes validation behavior to improve consistency and user experience.

  • Refactored password input validation to rely on Form autovalidation instead of custom state management
  • Added password length enforcement using passwordMaxLength constant across login, creation, deletion, and change dialogs
  • Implemented callback-driven validation state to enable/disable Create button only when both password fields are valid

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/komodo_ui_kit/lib/src/inputs/ui_text_form_field.dart Simplified validation logic by removing custom error state management and relying on Form autovalidation
lib/views/wallets_manager/widgets/wallet_login.dart Added constants import and applied consistent password field formatting
lib/views/wallets_manager/widgets/wallet_deleting.dart Removed duplicate import, reordered password field properties, applied code formatting
lib/views/wallets_manager/widgets/wallet_creation.dart Added password validity tracking and callback to enable Create button only when passwords are valid
lib/views/wallets_manager/widgets/creation_password_fields.dart Implemented validity callback system and switched to passive validation mode
lib/views/settings/widgets/security_settings/password_update_page.dart Moved constants import to proper location
lib/views/common/wallet_password_dialog/password_dialog_content.dart Added constants import for password length enforcement
lib/views/common/hw_wallet_dialog/trezor_steps/trezor_dialog_select_wallet.dart Added constants import, has duplicate import issue

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@CharlVS
Copy link
Copy Markdown
Collaborator Author

CharlVS commented Sep 16, 2025

@cursor please review

@cursor
Copy link
Copy Markdown

cursor bot commented Sep 16, 2025

I streamlined the codebase by removing redundant import statements across several UI components, enhancing code cleanliness and reducing potential for confusion. This refactoring ensures each necessary dependency is imported only once per file.

  • I removed a duplicate import 'package:web_dex/shared/constants.dart'; from lib/views/common/hw_wallet_dialog/trezor_steps/trezor_dialog_select_wallet.dart.
  • I eliminated a redundant import 'package:web_dex/shared/constants.dart'; in lib/views/common/wallet_password_dialog/password_dialog_content.dart.
  • I cleaned up lib/views/wallets_manager/widgets/wallet_login.dart by removing an unnecessary second import 'package:web_dex/shared/constants.dart'; statement.

3 tasks completed

  • Remove duplicate constants imports in login, trezor, and password dialog files
  • Verify no duplicate named params in password fields across dialogs
  • Run lints on modified files and address issues
Open in Cursor Open in Web

Learn more about Cursor Agents

CharlVS and others added 2 commits September 16, 2025 17:28
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 16, 2025

Visit the preview URL for this PR (updated for commit 69b25c1):

https://walletrc--pull-3149-merge-d1fi9903.web.app

(expires Thu, 25 Sep 2025 15:39:23 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc

@CharlVS CharlVS changed the title fix(password): enforce 128-char limit and consistent validation across dialogs fix(password): fix password Sep 16, 2025
@CharlVS CharlVS changed the title fix(password): fix password fix(security): Consistent 128‑char password handling and hardened auth validation Sep 16, 2025
@CharlVS CharlVS added the QA Ready for QA Testing label Sep 16, 2025
@CharlVS CharlVS requested review from DeckerSU and smk762 September 16, 2025 15:43
Copy link
Copy Markdown
Collaborator

@smk762 smk762 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 128 char input limit confirmed (#3151 outstanding).
  • < 8 char constraint honored with disabled button and error message
  • lowercase char requirement honored with disabled button and error message
  • uppercase char requirement honored with disabled button and error message
  • numeric char requirement honored with disabled button and error message
  • special char requirement honored with disabled button and error message
  • matching value in confirmation input honored with disabled button and error message.

@CharlVS CharlVS self-assigned this Sep 17, 2025
@DeckerSU
Copy link
Copy Markdown
Contributor

Overall it is good. At least the user cannot bypass password confirmation. But there is a small UI issue with error messages. For example, enter Anakonda!567 in the password field and Anaconda!567 in the confirmation field with one letter wrong. Then accept the EULA. The Create Wallet button is disabled, but there is no error message explaining why until the user changes focus away from the confirmation text field. This happens on Android. On Linux, clicking the EULA checkbox counts as a focus change and the error message appears, but on Android it does not. So overall LGTM, but if this Android bug can be fixed quickly, it would be better to fix it.

video_2025-09-17_21-32-15.mp4

@CharlVS CharlVS merged commit 5848741 into dev Sep 18, 2025
8 of 13 checks passed
@CharlVS CharlVS deleted the fix/password-bypass-v2 branch September 18, 2025 15:36
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Validation Error Display Issue

When a UiTextFormField loses focus in eager or passive validation modes, _shouldValidate updates directly without a setState() call. This prevents the TextFormField's autovalidateMode from updating, so validation errors don't appear immediately after focus loss.

packages/komodo_ui_kit/lib/src/inputs/ui_text_form_field.dart#L140-L155

https://github.com/KomodoPlatform/komodo-wallet/blob/69b25c17a32c66fed434accf44e12bdd32fed007/packages/komodo_ui_kit/lib/src/inputs/ui_text_form_field.dart#L140-L155

Fix in Cursor Fix in Web


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA Ready for QA Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants