Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion crates/bitwarden-exporters/src/cxf/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,41 @@ use crate::{
* Parse CXF payload in the format compatible with Apple (At the Account-level)
*/
pub(crate) fn parse_cxf(payload: String) -> Result<Vec<ImportingCipher>, CxfError> {
let account: CxfAccount = serde_json::from_str(&payload)?;
let sanitized = sanitize_timestamps(&payload);
let account: CxfAccount = serde_json::from_str(&sanitized)?;

let items: Vec<ImportingCipher> = account.items.into_iter().flat_map(parse_item).collect();

Ok(items)
}

/// Replace negative `creationAt` and `modifiedAt` values with 0.
///
/// Some credential managers (e.g., Google Password Manager) export timestamps
/// as the Windows FILETIME epoch (-11644473600) when no real date exists. The
/// `credential-exchange-format` crate deserializes these fields as `u64` and
/// cannot handle negative values.
pub(crate) fn sanitize_timestamps(payload: &str) -> String {
let mut value: serde_json::Value = match serde_json::from_str(payload) {
Ok(v) => v,
Err(_) => return payload.to_string(),
};

if let Some(items) = value.get_mut("items").and_then(|v| v.as_array_mut()) {
for item in items {
for key in &["creationAt", "modifiedAt"] {
if let Some(n) = item.get(key).and_then(|v| v.as_i64()) {
if n < 0 {
item[key] = serde_json::Value::Number(0.into());
}
}
}
}
}

serde_json::to_string(&value).unwrap_or_else(|_| payload.to_string())
}

Comment thread
crosenth marked this conversation as resolved.
Outdated
/// Convert a CXF timestamp to a [`DateTime<Utc>`].
///
/// If the timestamp is None, the current time is used.
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-exporters/src/cxf/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod dashlane_import_test;
mod negative_timestamp_test;
mod one_password_import_test;
mod sample_import_test;

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.

🎨 Thanks for adding comprehensive test coverage. As described in another comment, this PR does not resolve the crash when these negative values are nested within a collections array as opposed to an items array:

#[test]
fn poc_collections_negative_timestamp_still_crashes() {
    let input = r#"{
        "id": "DZSXp7iBQY-Fg-OofakQtQ", "username": "u", "email": "e",
        "collections": [{
            "id": "DZSXp7iBQY-Fg-OofakQtQ",
            "creationAt": -11644473600,
            "modifiedAt": -11644473600,
            "title": "C",
            "items": []
        }],
        "items": []
    }"#;
    let result = parse_cxf(input.to_string());
    assert!(result.is_ok(), "{:?}", result.err());
}

🎨 If you agree with the code changes I've suggested in import.rs I would also suggest adding the following test coverage:

#[test]
fn test_sanitize_valid_timestamps_unchanged_returns_borrowed() {
    let input = r#"{"id":"test","items":[{"id":"1","creationAt":1759783057,"modifiedAt":1759783057,"title":"Test","credentials":[]}]}"#;
    let result = sanitize_timestamps(input);
    assert!(matches!(result, std::borrow::Cow::Borrowed(_)));
}

#[test]
fn test_sanitize_negative_timestamp_in_collection() {
    let input = r#"{"id":"test","items":[],"collections":[{"id":"c1","creationAt":-11644473600,"modifiedAt":-11644473600,"title":"C","items":[]}]}"#;
    let result = sanitize_timestamps(input);
    assert!(result.contains(r#""creationAt":null"#));
    assert!(result.contains(r#""modifiedAt":null"#));
}

#[test]
fn test_sanitize_negative_timestamp_in_sub_collection() {
    let input = r#"{"id":"test","items":[],"collections":[{"id":"c1","title":"C","items":[],
        "subCollections":[{"id":"c2","creationAt":-11644473600,"title":"Sub","items":[]}]}]}"#;
    let result = sanitize_timestamps(input);
    assert!(result.contains(r#""creationAt":null"#));
}

#[test]
fn test_parse_cxf_with_negative_timestamps_falls_back_to_current_time() {
    let input = r#"{
        "id": "DZSXp7iBQY-Fg-OofakQtQ", "username": "u", "email": "e",
        "collections": [{"id":"c1","creationAt":-11644473600,"modifiedAt":-11644473600,"title":"C","items":[]}],
        "items": [{
            "id": "9OF-QjVDQo2Wp2xWPw6ZhA",
            "creationAt": -11644473600, "modifiedAt": -11644473600,
            "title": "Test Entry",
            "credentials": [{"type":"basic-auth","username":{"id":"u","fieldType":"string","value":"testuser"},
                              "password":{"id":"p","fieldType":"concealed-string","value":"testpass"},
                              "urls":["https://example.com"]}]
        }]
    }"#;
    let ciphers = parse_cxf(input.to_string()).expect("should not error on negative timestamps");
    let cipher = ciphers.first().unwrap();
    assert!(cipher.creation_date > Utc::now() - chrono::Duration::seconds(5));
}

🎨 You will also want to update existing tests in this file to expect null instead of 0

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @harr1424 - Merged in your updates and fixed the null instead of 0 issue

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
//! Tests for handling negative timestamps in CXF import.
//!
//! Some credential managers (e.g., Google Password Manager) export timestamps
//! as the Windows FILETIME epoch (-11644473600) when no real date exists.

#[cfg(test)]
mod tests {
use crate::cxf::import::{parse_cxf, sanitize_timestamps};

#[test]
fn test_sanitize_negative_creation_at() {
let input = r#"{"id":"test","items":[{"id":"1","creationAt":-11644473600,"modifiedAt":1759783057,"title":"Test","credentials":[]}]}"#;
let result = sanitize_timestamps(input);
assert!(result.contains(r#""creationAt":0"#));
assert!(result.contains(r#""modifiedAt":1759783057"#));
}

#[test]
fn test_sanitize_negative_modified_at() {
let input = r#"{"id":"test","items":[{"id":"1","creationAt":1759783057,"modifiedAt":-11644473600,"title":"Test","credentials":[]}]}"#;
let result = sanitize_timestamps(input);
assert!(result.contains(r#""creationAt":1759783057"#));
assert!(result.contains(r#""modifiedAt":0"#));
}

#[test]
fn test_sanitize_both_negative() {
let input = r#"{"id":"test","items":[{"id":"1","creationAt":-11644473600,"modifiedAt":-11644473600,"title":"Test","credentials":[]}]}"#;
let result = sanitize_timestamps(input);
assert!(result.contains(r#""creationAt":0"#));
assert!(result.contains(r#""modifiedAt":0"#));
}

#[test]
fn test_sanitize_valid_timestamps_unchanged() {
let input = r#"{"id":"test","items":[{"id":"1","creationAt":1759783057,"modifiedAt":1759783057,"title":"Test","credentials":[]}]}"#;
let result = sanitize_timestamps(input);
assert!(result.contains(r#""creationAt":1759783057"#));
assert!(result.contains(r#""modifiedAt":1759783057"#));
}

#[test]
fn test_parse_cxf_with_negative_timestamps_does_not_error() {
let input = r#"{
"id": "DZSXp7iBQY-Fg-OofakQtQ",
"username": "user@example.com",
"email": "user@example.com",
"fullName": "Test User",
"collections": [],
"items": [{
"id": "9OF-QjVDQo2Wp2xWPw6ZhA",
"creationAt": -11644473600,
"modifiedAt": -11644473600,
"title": "Test Entry",
"credentials": [{
"type": "basic-auth",
"username": {
"id": "-eZX0Gw-TzOsBFwt67N7ZA",
"fieldType": "string",
"value": "testuser"
},
"password": {
"id": "wgu3wTcXSYawrGMWMtaANg",
"fieldType": "concealed-string",
"value": "testpass"
},
"urls": ["https://example.com"]
}]
}]
}"#;
let result = parse_cxf(input.to_string());
assert!(result.is_ok(), "parse_cxf should not error on negative timestamps: {:?}", result.err());
}
}