[PM-24971] Sanitize passkey attestation options from AliExpress #6106

SaintPatrck · 2025-10-31T19:03:24Z

🎟️ Tracking

PM-24971
#5740

📔 Objective

This commit introduces a sanitizer for PasskeyAttestationOptions to address an issue with the AliExpress Android app.

The AliExpress app incorrectly appends a newline character to the user.id field when creating a passkey. This malformed request causes the passkey creation to fail.

To work around this, a PasskeyAttestationOptionsSanitizer is introduced. This sanitizer specifically checks if the request originates from the AliExpress relying party ID (m.aliexpress.com) and if the user.id ends with a newline. If both conditions are met, it trims the user.id before the request is further processed, allowing the passkey registration to succeed.

Specific changes include:

Added PasskeyAttestationOptionsSanitizer interface and its implementation, PasskeyAttestationOptionSanitizerImpl, which contains the specific fix for AliExpress.
Injected and utilized the new sanitizer within BitwardenCredentialManagerImpl to clean the PasskeyAttestationOptions before they are used to register a FIDO2 credential.
Updated dependency injection in CredentialProviderModule to provide the sanitizer.
Added unit tests for the new sanitizer logic.

📸 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 · 2025-10-31T19:04:46Z

Claude finished @SaintPatrck's task —— View job

Code Review Complete ✅

I've reviewed this PR introducing the PasskeyAttestationOptionsSanitizer to work around the AliExpress passkey bug. Overall, the implementation is solid and follows good architectural patterns. All previous review feedback has been successfully addressed.

What Was Reviewed

Review implementation files
Review test coverage
Check previous review feedback status
Create inline comments for issues
Post summary assessment

Summary

Architecture & Design: ✅ Excellent

Clean interface/implementation separation
Proper Hilt DI integration
Follows established patterns in the codebase
Well-documented with clear KDoc

Implementation Quality: ✅ Strong

Correct use of .let() instead of .apply() (previously identified bug has been fixed)
Precise trimEnd('\n') only removes newlines, not other whitespace
Proper error handling with logging
Good test coverage for the sanitizer logic

Previous Review Issues: ✅ All Resolved

✅ Fixed .apply → .let bug (BitwardenCredentialManagerImpl.kt:367)
✅ Changed to trimEnd('\n') for precision (PasskeyAttestationOptionsSanitizerImpl.kt:18)
✅ Added integration test for sanitizer (BitwardenCredentialManagerTest.kt:437)
✅ Improved KDoc documentation (PasskeyAttestationOptionsSanitizer.kt:8)

Security Considerations: ✅ Good

Scoped specifically to AliExpress RP ID (m.aliexpress.com)
Only trims trailing newlines, preserving data integrity
Minimal attack surface for the workaround

Issues Found

I've created inline comments for the following:

⚠️ Error Handling (BitwardenCredentialManagerImpl.kt:376): Consider adding more context to the sanitization failure log message (e.g., include RP ID) to aid debugging.
📝 Test Case Issues (PasskeyAttestationOptionSanitizerTest.kt):
- Line 52: Test name says "multiple trailing newlines" but only tests single newline
- Line 62: Test name says "preceding trailing spaces and a newline" but only tests a newline

These are minor issues that don't block the PR but would improve code quality and test accuracy.

Recommendations

Consider: Add the relying party ID to the error log when sanitization fails for easier debugging
Fix: Update test cases to match their descriptions or update the test names to match what they actually test
Future: As the author mentioned, moving validation/sanitization into the SDK would be the ideal long-term solution

Verdict

Approved with minor suggestions 👍

The PR successfully addresses the AliExpress passkey issue with a clean, targeted workaround. The implementation follows project architecture guidelines, has good test coverage, and all previous review feedback has been incorporated. The issues I noted are minor and don't affect the correctness of the fix.

github-actions · 2025-10-31T19:13:41Z

Checkmarx One – Scan Summary & Details – 6bf60017-146f-45ea-bbae-e936a8e6f899

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

codecov · 2025-10-31T19:36:28Z

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.90%. Comparing base (8de3a07) to head (df37505).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...dentials/manager/BitwardenCredentialManagerImpl.kt	85.18%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6106      +/-   ##
==========================================
+ Coverage   84.30%   84.90%   +0.59%     
==========================================
  Files         863      724     -139     
  Lines       55387    52768    -2619     
  Branches     7725     7665      -60     
==========================================
- Hits        46694    44802    -1892     
+ Misses       5981     5277     -704     
+ Partials     2712     2689      -23

☔ 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.

This commit introduces a sanitizer for `PasskeyAttestationOptions` to address an issue with the AliExpress Android app. The AliExpress app incorrectly appends a newline character to the `user.id` field when creating a passkey. This malformed request causes the passkey creation to fail. To work around this, a `PasskeyAttestationOptionsSanitizer` is introduced. This sanitizer specifically checks if the request originates from the AliExpress relying party ID (`m.aliexpress.com`) and if the `user.id` ends with a newline. If both conditions are met, it trims the `user.id` before the request is further processed, allowing the passkey registration to succeed. Specific changes include: - Added `PasskeyAttestationOptionsSanitizer` interface and its implementation, `PasskeyAttestationOptionSanitizerImpl`, which contains the specific fix for AliExpress. - Injected and utilized the new sanitizer within `BitwardenCredentialManagerImpl` to clean the `PasskeyAttestationOptions` before they are used to register a FIDO2 credential. - Updated dependency injection in `CredentialProviderModule` to provide the sanitizer. - Added unit tests for the new sanitizer logic.

This commit corrects a "santizer" to "sanitizer" typo in the package name within the `com.x8bit.bitwarden.data.credentials` path. The primary change involves renaming the directory `santizer` to `sanitizer`. All corresponding import statements and package declarations that referenced the misspelled path have been updated accordingly across multiple files.

…ionsSanitizerImpl This commit renames `PasskeyAttestationOptionSanitizerImpl` to `PasskeyAttestationOptionsSanitizerImpl` to align with the plural form used in the `PasskeyAttestationOptionsSanitizer` interface it implements.

This commit refines the workaround for a bug in the AliExpress passkey creation process. The previous implementation incorrectly used `contains` to check the Relying Party (RP) ID, which could lead to false positives if another RP ID happened to contain the AliExpress ID as a substring. This has been corrected to use a strict equality check (`==`) to ensure the workaround is applied only to AliExpress. This change makes the fix more precise and robust.

This commit improves the KDoc comments for the `PasskeyAttestationOptionsSanitizer` interface and its `sanitize` method. The goal is to provide a more detailed and clearer explanation of their purpose, particularly in the context of handling passkey options received from a server. The updated documentation clarifies that sanitization involves ensuring the options conform to expected formats, such as Base64 URL decoding fields and setting defaults, before they are used by the client. It also specifies that the `sanitize` method returns a new, sanitized instance.

This commit updates the `PasskeyAttestationOptionsSanitizer` to only trim trailing newlines from the user id. Previously, it would trim both leading and trailing whitespace, which could incorrectly alter user ids that legitimately start with whitespace characters. The `trim()` function has been replaced with `trimEnd()` to correct this behavior.

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt

...c/test/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerTest.kt

.../kotlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizer.kt

Corrected `sanitize` implementation Added test coverage for `sanitize` implementation. Updated documentation to better reflect usage. Only trim newlines from end of options.

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt

...c/test/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerTest.kt

.../kotlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizer.kt

david-livefront · 2025-11-03T16:05:50Z

...tlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionSanitizerTest.kt

+        val sanitizedOptions = sanitizer.sanitize(options)
+
+        assertEquals("user123", sanitizedOptions.user.id)
+        assertNotSame(options, sanitizedOptions)


Can we do assertNotEquals, or is the intent here to verify that they are not the same instance?

This is intentional to verify a new instance with the correct ID is returned.

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

david-livefront · 2025-11-03T16:20:55Z

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt

+/**
+ * Default implementation of [PasskeyAttestationOptionsSanitizer].
+ */
+object PasskeyAttestationOptionsSanitizerImpl : PasskeyAttestationOptionsSanitizer {


david-livefront · 2025-11-03T16:21:29Z

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt

+        // to the user.id field when creating a passkey. This causes the operation to fail
+        // downstream. As a workaround, we detect this specific scenario, trim the newline, and
+        // re-serialize the JSON request.
+        return if (options.relyingParty.id == ALIEXPRESS_RP_ID &&


Is there any downside to doing it on all relaying parties?

Performance wise, the difference is negligible. I chose this route more to retain data integrity for other RP's, and ease of removal when we no longer need the workaround.

I plan to open a PR that performs validation and sanitizes request properties in the SDK, but I don't want that to hold-up fixing this issue.

Makes snese

david-livefront · 2025-11-03T17:32:20Z

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

+                ?.runCatching { json.encodeToString(this) }
+                ?.fold(
+                    onSuccess = { it },
+                    onFailure = { null },


Do we want to propagate the throwable?

I don't believe we need to propagate it. We only show a Toast when it fails, but we can certainly log it here.

david-livefront · 2025-11-03T17:45:09Z

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

+                ?.fold(
+                    onSuccess = { it },
+                    onFailure = {
+                        Timber.e(it, "Failed to sanitize passkey attestation options.")


claude · 2025-11-03T17:46:08Z

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt

+                        null
+                    },
+                )
+                ?: return Fido2RegisterCredentialResult.Error.InternalError


⚠️ Error Handling: Consider propagating the throwable or adding more context to the log message.

Currently, if sanitization fails, we only log the error and return InternalError. This makes it difficult to diagnose issues. Consider:

Including the relying party ID in the log message for better debugging

Creating a more specific error type for sanitization failures

Or at minimum, logging which sanitizer check failed

Example:

onFailure = { Timber.e(it, "Failed to sanitize passkey attestation options for RP: ${options.relyingParty.id}") null },

This would help distinguish sanitization failures from other internal errors when investigating issues.

claude · 2025-11-03T17:46:10Z

...tlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionSanitizerTest.kt

+    }
+
+    @Test
+    fun `Sanitization with multiple trailing newlines`() {


📝 Test Case Clarification: The test name says "multiple trailing newlines" but only tests a single newline.

This test currently has userId = "user123\n" which is the same as the first test case. If you want to test multiple trailing newlines, it should be:

val options = createOptions(rpId = "m.aliexpress.com", userId = "user123\n\n")

With trimEnd('\n'), this would remove all trailing newlines, resulting in "user123".

claude · 2025-11-03T17:46:11Z

...tlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionSanitizerTest.kt

+    }
+
+    @Test
+    fun `Sanitization with preceding trailing spaces and a newline`() {


📝 Test Case Issue: Similar to the previous test, this test name says "preceding trailing spaces and a newline" but only tests "user123\n".

If you want to test spaces followed by a newline, the test should be:

val options = createOptions(rpId = "m.aliexpress.com", userId = "user123 \n")

Note: With trimEnd('\n'), this would result in "user123 " (spaces preserved), not "user123". If you want to remove both spaces and newlines, you'd need trimEnd() or trim(), but that would change the behavior discussed in earlier reviews.

SaintPatrck requested review from a team, brian-livefront and david-livefront as code owners October 31, 2025 19:03

SaintPatrck force-pushed the PM-24971/sanitize-aliexpress-fido2-attestation-options branch from 6c8a933 to f919401 Compare October 31, 2025 19:06

SaintPatrck added 7 commits November 3, 2025 09:13

Use apply instead of also for object sanitization

6544a46

SaintPatrck force-pushed the PM-24971/sanitize-aliexpress-fido2-attestation-options branch from f919401 to dd92be6 Compare November 3, 2025 14:14

claude bot reviewed Nov 3, 2025

View reviewed changes

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt Outdated Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt Outdated Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

...c/test/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerTest.kt Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

.../kotlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizer.kt Outdated Show resolved Hide resolved

Address Claude review comments

4c5023d

Corrected `sanitize` implementation Added test coverage for `sanitize` implementation. Updated documentation to better reflect usage. Only trim newlines from end of options.

claude bot reviewed Nov 3, 2025

View reviewed changes

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

...lin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizerImpl.kt Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

...c/test/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerTest.kt Show resolved Hide resolved

claude bot reviewed Nov 3, 2025

View reviewed changes

.../kotlin/com/x8bit/bitwarden/data/credentials/sanitizer/PasskeyAttestationOptionsSanitizer.kt Show resolved Hide resolved

david-livefront reviewed Nov 3, 2025

View reviewed changes

...c/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/BitwardenCredentialManagerImpl.kt Outdated Show resolved Hide resolved

david-livefront reviewed Nov 3, 2025

View reviewed changes

Remove redundant blank line

7fa03d6

david-livefront reviewed Nov 3, 2025

View reviewed changes

Log passkey attestation option sanitization failures

df37505

SaintPatrck enabled auto-merge November 3, 2025 17:43

david-livefront reviewed Nov 3, 2025

View reviewed changes

david-livefront approved these changes Nov 3, 2025

View reviewed changes

claude bot reviewed Nov 3, 2025

View reviewed changes

SaintPatrck added this pull request to the merge queue Nov 3, 2025

Merged via the queue into main with commit a631d68 Nov 3, 2025
15 of 26 checks passed

SaintPatrck deleted the PM-24971/sanitize-aliexpress-fido2-attestation-options branch November 3, 2025 18:42

[PM-24971] Sanitize passkey attestation options from AliExpress #6106

[PM-24971] Sanitize passkey attestation options from AliExpress #6106

Uh oh!

Conversation

SaintPatrck commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

claude bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

What Was Reviewed

Summary

Issues Found

Recommendations

Verdict

Uh oh!

github-actions bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

codecov bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SaintPatrck commented Oct 31, 2025 •

edited

Loading

claude bot commented Oct 31, 2025 •

edited

Loading

github-actions bot commented Oct 31, 2025 •

edited

Loading

codecov bot commented Oct 31, 2025 •

edited

Loading