Skip to content

[PM-41690] fix: CXF import crash on negative timestamps - #1362

Merged
harr1424 merged 13 commits into
bitwarden:mainfrom
crosenth:main
Aug 18, 2026
Merged

[PM-41690] fix: CXF import crash on negative timestamps#1362
harr1424 merged 13 commits into
bitwarden:mainfrom
crosenth:main

Conversation

@crosenth

@crosenth crosenth commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Some credential managers (e.g., Google Password Manager) export creationAt/modifiedAt as the Windows FILETIME epoch (-11644473600) when no real date exists. The credential-exchange-format crate deserializes these fields as u64 and rejects negative values.

Sanitize the JSON payload before deserialization by clamping negative timestamp values to null.

🎟️ Tracking

bitwarden/android#7140

bitwarden/android#7215

bitwarden/android#7216

https://bitwarden.atlassian.net/browse/PM-40542

📔 Objective

Summary

  • Sanitize negative creationAt/modifiedAt values in CXF payloads
    before deserialization, clamping them to null
  • Fixes import failures from Google Password Manager on devices where
    credentials have no real creation/modification date (exported as the
    Windows FILETIME epoch: -11644473600)

Root cause

The credential-exchange-format crate defines these fields as
Option<u64>, which cannot represent negative values. The fix
pre-processes the JSON in parse_cxf() before the typed
deserialization step.

Test plan

  • Added unit tests for sanitize_timestamps() covering negative,
    valid, and mixed timestamp values
  • Added integration test confirming parse_cxf() succeeds with
    negative timestamps in the payload

Some credential managers (e.g., Google Password Manager) export
creationAt/modifiedAt as the Windows FILETIME epoch (-11644473600)
when no real date exists. The credential-exchange-format crate
deserializes these fields as u64 and rejects negative values.

Sanitize the JSON payload before deserialization by clamping negative
timestamp values to 0.
@crosenth
crosenth requested a review from a team as a code owner August 7, 2026 16:02
@CLAassistant

CLAassistant commented Aug 7, 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-41690
Link: https://bitwarden.atlassian.net/browse/PM-41690

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

@harr1424 harr1424 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@crosenth This PR and the research you invested in it is greatly appreciated. I do disagree with the fundamental approach taken here, which diverges from v1.0 specifications:

creationAt
This OPTIONAL member contains the UNIX timestamp in seconds at which this Collection was originally created. If this member is not set, but the importing provider requires this member in their proprietary data model, the importer SHOULD use the current timestamp at the time the provider encounters this Collection.

modifiedAt
This OPTIONAL member contains the UNIX timestamp in seconds of the last modification brought to this Collection. If this member is not set, but the importing provider requires this member in their proprietary data model, the importer SHOULD use the current timestamp at the time the provider encounters this Collection.

The above also applies to these values nested in the items array.

Additionally, the fix has only been applied to negative values in the items array and not collection which can also introduce the negative values and cause a crash. Please see my comment in crates/bitwarden-exporters/src/cxf/tests/negative_timestamp_test.rs for a POC.

I've suggested code changes to improve the efficiency of the timestamp adjustment and ensure it results in the current timestamp.

I've also suggested some additional test coverage.

Comment thread crates/bitwarden-exporters/src/cxf/tests/negative_timestamp_test.rs
Comment thread crates/bitwarden-exporters/src/cxf/import.rs Outdated
crosenth and others added 2 commits August 8, 2026 15:27
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
  - Assert null instead of 0 for clamped timestamps
  - Add test verifying unmodified input returns borrowed reference
@harr1424
harr1424 self-requested a review August 8, 2026 22:37
Comment thread crates/bitwarden-exporters/src/cxf/tests/negative_timestamp_test.rs
crosenth and others added 2 commits August 11, 2026 11:15
  - Test negative timestamps in collections array
  - Test negative timestamps in nested subCollections
  - Test that clamped timestamps fall back to approximately current time
@harr1424
harr1424 self-requested a review August 11, 2026 22:22

@harr1424 harr1424 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've left a suggestion to update a doc comment to describe the new behavior, please also update the PR description as it still describes the old behavior (assigning 0 instead of null).

Please also be sure to run the checks described here as your PR won't be able to merge until these pass.

Lastly I left a comment in the new test file asking if there was any reason not to add a test ensuring allocation is avoided when the payload doesn't need to be modified. If you'd prefer not to add this, it's probably OK, it would be nice to document your reasoning, though.

Comment thread crates/bitwarden-exporters/src/cxf/import.rs Outdated
Comment thread crates/bitwarden-exporters/src/cxf/tests/negative_timestamp_test.rs
crosenth and others added 2 commits August 11, 2026 22:38
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
  Use matches!(Cow::Borrowed(_)) to explicitly verify no allocation
  occurs, rather than just checking content equality.
harr1424
harr1424 previously approved these changes Aug 12, 2026
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.41%. Comparing base (cc7daf1) to head (451545b).

Files with missing lines Patch % Lines
crates/bitwarden-exporters/src/cxf/import.rs 95.45% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1362   +/-   ##
=======================================
  Coverage   86.41%   86.41%           
=======================================
  Files         501      501           
  Lines       74235    74278   +43     
=======================================
+ Hits        64149    64190   +41     
- Misses      10086    10088    +2     

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

@harr1424

Copy link
Copy Markdown
Contributor

@crosenth please be sure to run the lint described here in order to format these changes against standards, right now the required CI check "Lint / Check Style (pull_request)" is failing.

You can run npm run lint:fix from the root of your cloned repository.

@harr1424
harr1424 requested a review from adudek-bw August 13, 2026 00:18
@harr1424

Copy link
Copy Markdown
Contributor

@crosenth we are waiting on a second code-review from a colleague on my team. Everything looks good on my end and I look forward to merging your changes as soon as they are reviewed by a second pair of eyes.

@adudek-bw can you please give this a review?

@crosenth

Copy link
Copy Markdown
Contributor Author

@crosenth we are waiting on a second code-review from a colleague on my team. Everything looks good on my end and I look forward to merging your changes as soon as they are reviewed by a second pair of eyes.

@adudek-bw can you please give this a review?

Thanks @harr1424 really enjoyed iterating on this with you

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: main (451545b)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ✅ No breaking changes detected Compilation passed with new SDK version - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@harr1424
harr1424 merged commit 8283c9a into bitwarden:main Aug 18, 2026
71 of 72 checks passed
bw-ghapp Bot added a commit to bitwarden/sdk-swift that referenced this pull request Aug 18, 2026
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.

5 participants