Skip to content

[PM-42349] fix: use fixed-length password mask - #7293

Open
488315 wants to merge 2 commits into
bitwarden:mainfrom
488315:fix/pm-42337-fixed-password-mask
Open

[PM-42349] fix: use fixed-length password mask#7293
488315 wants to merge 2 commits into
bitwarden:mainfrom
488315:fix/pm-42337-fixed-password-mask

Conversation

@488315

@488315 488315 commented Aug 24, 2026

Copy link
Copy Markdown

Tracking

Fixes #7290 (PM-42337)

Objective

Use a fixed eight-character mask for hidden login passwords and custom hidden fields so their displayed mask no longer reveals the underlying value length.

The shared password component keeps its existing length-based behavior by default. Only the affected login-password and custom-hidden-field call sites opt into the fixed-length mask, so existing behavior for card numbers, security codes, bank fields, identity fields, SSH keys, and other masked values is unchanged.

Regression coverage verifies:

  • fixed-length masking for opted-in read-only password fields;
  • the real value is shown after reveal;
  • default read-only password masking still preserves value length for non-opted-in fields;
  • non-interactable login password fields use the fixed mask.

Validation:

  • ./gradlew.bat :ui:testDebugUnitTest
  • ./gradlew.bat :ui:lintDebug
  • ./gradlew.bat detekt -Pprecommit=true --rerun-tasks
  • git diff --check

All completed checks passed with JDK 21.

Screenshots

N/A. There is no layout or styling change; affected hidden login/custom values consistently render eight mask characters.

Copilot AI lite review requested due to automatic review settings August 24, 2026 07:24
@488315
488315 requested a review from a team as a code owner August 24, 2026 07:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CLAassistant

CLAassistant commented Aug 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot

Copy link
Copy Markdown
Collaborator

Thank you for your contribution! We've added this to our internal tracking system for review.
ID: PM-42349
Link: https://bitwarden.atlassian.net/browse/PM-42349

Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process.

@bitwarden-bot bitwarden-bot changed the title [PM-42337] fix: use fixed-length password mask [PM-42349] [PM-42337] fix: use fixed-length password mask Aug 24, 2026
@488315 488315 changed the title [PM-42349] [PM-42337] fix: use fixed-length password mask [PM-42349] fix: use fixed-length password mask Aug 24, 2026
@488315
488315 force-pushed the fix/pm-42337-fixed-password-mask branch from 11b8ee8 to 839a7ad Compare August 24, 2026 07:56
@andrebispo5

Copy link
Copy Markdown
Contributor

Hi @488315
When the fixed mask is on, lastTextValue is still seeded from the raw value while the field renders displayedValue. The two disagree, so the first tap on the field makes the TextField's onValueChange fire with "********" instead of staying quiet.

It's harmless right now since all three opted-in call sites pass onValueChange = { }, but useFixedLengthMask is public on a shared :ui component. The next caller that wires a real callback would get the mask handed to it on first tap, and if that value ever gets saved the password becomes eight asterisks.

Repro (drop into BitwardenPasswordFieldTest, fails on this branch with expected:<[]> but was:<[********]>):

@Test
fun `tapping a masked read only field does not emit the mask`() {
    val emitted = mutableListOf<String>()

    setTestContent {
        BitwardenTheme {
            BitwardenPasswordField(
                label = "Password",
                value = "correct horse battery staple",
                showPassword = false,
                showPasswordChange = { },
                onValueChange = { emitted += it },
                readOnly = true,
                useFixedLengthMask = true,
                cardStyle = CardStyle.Full,
            )
        }
    }

    composeTestRule.onNodeWithText("Password").performClick()
    composeTestRule.waitForIdle()

    assertEquals(emptyList<String>(), emitted)
}

Keying the state off the displayed value fixes it, and the existing tests still pass:

var lastTextValue by remember(displayedValue) { mutableStateOf(value = displayedValue) }

Could you double check this one?

Thanks for the submission 👍

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.82%. Comparing base (d817f6b) to head (839a7ad).

Files with missing lines Patch % Lines
...ult/feature/item/component/VaultItemCustomField.kt 0.00% 2 Missing ⚠️
...den/ui/vault/feature/item/VaultItemLoginContent.kt 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (d817f6b) and HEAD (839a7ad). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (d817f6b) HEAD (839a7ad)
app-ui-auth-tools 1 0
app-ui-vault 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7293       +/-   ##
===========================================
- Coverage   86.35%   43.82%   -42.53%     
===========================================
  Files         893      896        +3     
  Lines       65488    64454     -1034     
  Branches     9861     9790       -71     
===========================================
- Hits        56550    28247    -28303     
- Misses       5464    34224    +28760     
+ Partials     3474     1983     -1491     
Flag Coverage Δ
app-data 17.89% <0.00%> (-0.01%) ⬇️
app-ui-auth-tools ?
app-ui-platform 16.37% <0.00%> (-0.01%) ⬇️
app-ui-vault ?
authenticator 6.07% <0.00%> (-0.03%) ⬇️
lib-core-network-bridge 4.10% <0.00%> (+0.01%) ⬆️
lib-data-ui 1.20% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@488315

488315 commented Aug 24, 2026

Copy link
Copy Markdown
Author

Thanks for catching that. I double checked it and you were right.

I reproduced it with the regression test you sent first, and it failed with the mask being emitted on the initial tap. I updated lastTextValue to key off displayedValue, added that regression test, and pushed the fix in 06742ac.

I also reran the focused BitwardenPasswordFieldTest suite (5/5), the full :ui:testDebugUnitTest suite, :ui:lintDebug, and staged Detekt precommit checks. All are passing on JDK 21.

Appreciate you pointing this one out.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PM-42337] Password / hidden fields revealing actual data length

5 participants