[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).