[PM-25423] Force LTR direction for password and sensitive fields

[SaintPatrck]

🎟️ Tracking

PM-25423

📔 Objective

This commit introduces a VisualTransformation to enforce Left-To-Right (LTR) text direction in password fields and other sensitive inputs. This ensures consistent layout and prevents rendering issues, particularly in Right-To-Left (RTL) locales where characters like asterisks might otherwise appear incorrectly aligned.

A CompoundVisualTransformation utility has also been added to allow chaining multiple VisualTransformations together.

Specific changes:

• Created ForceLtrVisualTransformation to wrap text with LTR Unicode control characters.
• Created CompoundVisualTransformation to combine multiple transformations, preserving correct offset mapping.
• Applied forceLtrVisualTransformation to BitwardenPasswordField and BitwardenHiddenPasswordField, combining it with the existing PasswordVisualTransformation or nonLetterColorVisualTransformation.
• Enforced LTR direction on generated passwords in the Password History screen.
• Applied LTR transformation to sensitive fields in the "Identity" and "Login" item views, such as TOTP, URI, SSN, and passport number.

📸 Screenshots

Coming soon!

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @SaintPatrck's task —— View job

---

Review Complete

I've completed a comprehensive review of the LTR visual transformation implementation. Here's my detailed feedback:

---

Summary of Changes Since Last Review

The PR has addressed several previous review comments:

• ✅ Removed unnecessary compound transformation when content is obscured (BitwardenPasswordField:145-154)
• ✅ Fixed visual transformation order in PasswordHistoryListItem (nonLetterColor → forceLtr)
• ✅ Removed unnecessary @Suppress("StringTemplate") in tests
• ✅ Fixed test failures related to LTR transformations

---

Critical Issues

1. Potential Offset Mapping Bug in CompoundVisualTransformation

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/CompoundVisualTransformation.kt:67-69

The transformedToOriginal mapping may fail with out-of-bounds offsets when transformations add characters:

override fun transformedToOriginal(offset: Int): Int {
    val resultOriginal = result.offsetMapping.transformedToOriginal(offset)
    return acc.offsetMapping.transformedToOriginal(resultOriginal)
}

Issue: When offset exceeds the transformed text length, the first transformedToOriginal call might return a value that's still out of bounds for the accumulated transformation's original text. This could cause crashes or incorrect cursor positioning.

Example scenario:

• Original text: "abc" (length 3)
• After Transform1 (add prefix "X"): "Xabc" (length 4)
• After Transform2 (add prefix "Y"): "YXabc" (length 5)
• If cursor is at position 10 (invalid), the mapping chain may fail

Recommendation: Add bounds checking similar to ForceLtrVisualTransformation:64-65:

override fun transformedToOriginal(offset: Int): Int {
    val resultOriginal = result.offsetMapping.transformedToOriginal(offset)
    return acc.offsetMapping.transformedToOriginal(resultOriginal)
        .coerceIn(0, /* original text length */)
}

However, getting the original text length requires refactoring. Consider capturing it in the fold operation.

---

2. Missing @Composable Best Practice

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ForceLtrVisualTransformation.kt:79

The remember call in forceLtrVisualTransformation() doesn't provide keys, but the object is a singleton:

@Composable
fun forceLtrVisualTransformation(): VisualTransformation = remember {
    ForceLtrVisualTransformation
}

Issue: Since ForceLtrVisualTransformation is an object (singleton), using remember without keys is fine, but it's redundant. The object is already a singleton and doesn't need to be remembered.

Recommendation: Either:

1. Remove the remember wrapper (simpler):

   @Composable
   fun forceLtrVisualTransformation(): VisualTransformation = 
       ForceLtrVisualTransformation

2. Or use remember(Unit) to be explicit about no dependencies

Justification from review comment: The author stated "Only to expose ForceLtrTransformation since it's internal and follow the existing pattern(s)." However, this doesn't require remember - a simple wrapper function suffices.

---

Suggested Improvements

3. Documentation Clarity - When NOT to Use

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ForceLtrVisualTransformation.kt:45-48

The documentation states:

 * - Only applies LTR transformation when text is **visible**
 * - Do NOT use with obscured text (e.g., password bullets) as masked characters
 *   are directionally neutral

Issue: This is slightly misleading. The transformation can be used with password fields when visible (as evidenced by BitwardenPasswordField:148-154), but shouldn't be combined with PasswordVisualTransformation.

Recommendation: Clarify:

 * - Do NOT combine with masking transformations (e.g., PasswordVisualTransformation)
 *   as masked characters (bullets) are directionally neutral and don't need LTR forcing
 * - CAN be used with password fields when the password is visible

---

4. Test Coverage Gap - Round-Trip with Compound Transformations

Location: ui/src/test/kotlin/com/bitwarden/ui/platform/components/util/CompoundVisualTransformationTest.kt

Missing test: While there's a round-trip test for single transformations (line 223-237), there's no comprehensive round-trip test for compound transformations with multiple layers.

Recommendation: Add a test like:

@Test
fun `compoundVisualTransformation round trip with multiple transformations`() {
    val text = AnnotatedString("test123")
    val compound = CompoundVisualTransformation(
        PasswordVisualTransformation(),
        ForceLtrVisualTransformation
    )
    val result = compound.filter(text)
    
    for (originalOffset in 0..text.length) {
        val transformed = result.offsetMapping.originalToTransformed(originalOffset)
        val backToOriginal = result.offsetMapping.transformedToOriginal(transformed)
        assertEquals(originalOffset, backToOriginal)
    }
}

---

5. Inconsistent Application to Identity Fields

Location: app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemIdentityContent.kt

The LTR transformation is applied to: SSN (154), passport (177), license (200), email (223), phone (246), but NOT to:

• Driver's license number (should have LTR per docs:27)
• Addresses (correctly omitted, as noted in docs:40)

Question: Should driver's license number have forceLtr = true on line 200? The field appears to already have it, so this is good. Just noting for completeness.

---

6. Potential Performance Consideration

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/CompoundVisualTransformation.kt:53-74

The fold operation creates a new OffsetMapping object for each transformation, which involves object allocation in the hot path (every recomposition with text changes).

Impact: Low to medium - depends on number of transformations and recomposition frequency.

Recommendation: Consider object pooling or a more efficient composition strategy if performance issues arise. For now, this is acceptable since most use cases involve only 2 transformations.

---

7. Missing Edge Case Tests

Recommended additional test cases:

1. Unicode Emoji Handling: Test with emoji characters that have complex grapheme clusters
2. Bidirectional Text with Numbers: Test with Arabic text + numbers (e.g., "مرحبا123")
3. Zero-Width Characters: Test with zero-width joiners/non-joiners

These edge cases are important for password fields that might contain complex Unicode.

---

Good Practices Observed

• ✅ Comprehensive test coverage with 348 lines for CompoundVisualTransformation and 256 for ForceLtrVisualTransformation
• ✅ Proper offset mapping with coercion in ForceLtrVisualTransformation
• ✅ Clear, detailed KDoc documentation with usage guidelines
• ✅ Appropriate use of internal visibility for implementation details
• ✅ Consistent application across password fields, TOTP, URIs, and sensitive identity fields
• ✅ Tests include boundary conditions and edge cases (empty strings, long text, etc.)

---

Action Items for Author

High Priority

1. [CRITICAL] Investigate and fix potential offset mapping bug in CompoundVisualTransformation.transformedToOriginal (Issue Android Accessibility Service #1)

Medium Priority

2. [IMPROVEMENT] Simplify forceLtrVisualTransformation() by removing unnecessary remember (Issue Site Search #2)
3. [DOCS] Clarify documentation about when NOT to use the transformation (Issue Fully Implement Change Password Flow for Extension #3)

Low Priority

4. [TESTING] Add round-trip test for compound transformations (Issue i18n #4)
5. [TESTING] Add edge case tests for emoji and complex Unicode (Issue Alternative to using Clipboard on Android #7)

---

Verification Checklist

• ✅ Architecture compliance: Follows Compose patterns, proper use of VisualTransformation API
• ✅ Style guidelines: KDoc present, proper formatting, appropriate visibility modifiers
• ✅ Test coverage: Excellent coverage at 84.81%, comprehensive unit tests
• ⚠️ Security: LTR forcing itself is safe, but verify no sensitive data leaks through logs/debugging
• ⚠️ Edge cases: Most covered, but missing some Unicode edge cases

---

Review Methodology

This review analyzed:

• Implementation files: ForceLtrVisualTransformation.kt, CompoundVisualTransformation.kt, BitwardenPasswordField.kt
• Test files: ForceLtrVisualTransformationTest.kt, CompoundVisualTransformationTest.kt
• Usage locations: VaultItemLoginContent.kt, VaultItemIdentityContent.kt, PasswordHistoryListItem.kt
• Commit history: 6 commits addressing previous feedback
• Previous review comments: Tracked resolution of 4 comments

---

[github-actions]

Checkmarx One – Scan Summary & Details – 64fb2187-d9cf-4235-ac2d-bbf51348d30d

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.81%. Comparing base (c16d31f) to head (9d6189d).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6090   +/-   ##
=======================================
  Coverage   84.80%   84.81%           
=======================================
  Files         721      721           
  Lines       52811    52826   +15     
  Branches     7669     7670    +1     
=======================================
+ Hits        44789    44805   +16     
+ Misses       5332     5329    -3     
- Partials     2690     2692    +2     

☔ View full report in Codecov by Sentry.
📢 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.

[SaintPatrck]

@codokie We've pivoted to a different approach for handling bidi text. Would you be willing to check behavior with these changes? I'm particularly curious if the Password fields handle manual RTL input correctly.

[david-livefront]

This one can just be "$LRO${password.password}$PDF"

[david-livefront]

Does the Remember do anything here?

[SaintPatrck]

Only to expose ForceLtrTransformation since it's internal and follow the existing pattern(s).

[codokie]

@codokie We've pivoted to a different approach for handling bidi text. Would you be willing to check behavior with these changes? I'm particularly curious if the Password fields handle manual RTL input correctly.

The usage of unicode characters to force LTR direction on text does seem to fix the first and third regressions of the unicodeWrap() attempt (see this comment). However, the 2nd regression (wrong cursor position) is still very much an issue.

As for manual RTL input in password fields, there seems to be a problem with that because LTR direction is being forced regardless of the directionality of the input. For example, the password סיסמה1234* is supposed to appear as *סיסמה1234 (asterisk is shifted). Imitating the logic of TextDirection.ContentOrLtr could prove to be a solution. That said, it is not really a big deal because most services forbid passwords with RTL characters, and LTR input seems to work as expected in password fields (except for the cursor alignment issue).

Upon brief testing, I’ve noticed a few more quirks that occur when an RTL app language is selected:

• Editing a URI field does not force LTR text direction on the input (typing https://bitwarden.com/ appears as /https://bitwarden.com).
• Editing the phone number field of an identity item does not force LTR text direction on the input (typing 123 456 7890 appears as 7890 456 123).
• In the footnote of the Settings -> About page, the copyright symbol is placed on the right (Bitwarden Inc. 2015-2025 ©) instead of on the left (© Bitwarden Inc. 2015-2025).
• LTR notes written in a language such as English are uncomfortable to read/write because RTL text direction is applied to them. If any non-strong character is used at the start or end of a line, then it is placed on the other end (e.g. Hello world! appears as !Hello world). Bi-directional notes (mixture of LTR with RTL text) are also not rendered properly. In order to fully support them, the user should be able to override (through some kind of text formatting UI) the direction of each line of text separately. This should obviously not be a priority at the moment because note formatting has not been implemented yet.

It still seems to me that #5012 is a better approach than utilizing Unicode, mainly because the native Android implementation to set text direction takes care to align the text cursor correctly. The only downside is that it also aligns the text according to its direction, but it is easy to override this behavior with basically no side effects.

Lastly, maintainability is also an important factor to consider. With the changes that this PR proposes, future PRs that add new types of vault items (for example) could be prone to omitting or misusing the new forceLtr property or the forceLtrVisualTransformation() function.

Thanks for dedicating time to find the best solution to this matter, issuing a fix could greatly improve the usability of the app for users of RTL languages. Please let me know if I could help with anything.