1700981 - Removing the timestamps from a login CSV file and importing it to a Firefox profile that already had those logins saved will delete the dates

Status: ASSIGNED

(Firefox :: about:logins, defect, P3)

Description

[Maria Heres, inactive, contact Ecosystem QA]

Attachment: existing logins updated.gif

[Affected Versions]:

• Firefox Beta 88.0b2 (Build ID: 20210323190052)
• Firefox Nightly 89.0a1 (Build ID: 20210324084848)

[Affected Platforms]:

• Windows 10
• Linux Mint 20
• macOS 10.15

[Prerequisites]:

• Have Firefox open.
• Have logins saved.

[Steps to reproduce]:

1. Navigate to “about:logins”.
2. Click the meatball menu button (“...”).
3. Choose “Export Logins” and complete the export process.
4. Open the CSV file and delete the timestamps.
5. Return to Firefox and open the meatball menu again.
6. Choose the “Import from a File..." option.
7. Select the edited CSV file and import it.
8. Observe the “Import Complete” modal.
9. Observe the dates of the login entries.

[Expected result]:

• The modal only detects duplicate logins.
• The existing entries are not updated.

[Actual result]:

• The modal detects that logins have been updated.
• The existing logins are updated to no longer have dates.

[Notes]:

• Attached is a recording of the issue.

Comment 1

[Sam Foster [:sfoster] (he/him)]

i'm not sure what the expected result would be in this case. The logins should be updated, but at that point, the timestamps are no longer accurate.
We could use new timestamps from the time of import perhaps? Or try and get the last-modified time from the file itself? Or just leave the possibly-inacurate timestamps from the initial import as-is.

Comment 2

[superstuff]

Is there a reason we're letting users control the timestamps at all? To me that seems like metadata that the user shouldn't need to control directly. For sure we should be exporting these fields to the CSV, but should we be importing them as well? In my opinion I would propose the following flow:

• Is the domain name new?
  • If so, then this is a new entry:
    • DateCreated = now
    • DateModified = now
    • LastUsed = empty (or now?)
  • Else, this entry exists:
    • DateCreated = no change
    • Has the username or password changed?
      • If so, DateModified = now
      • Else, there is no change so this is a duplicate (not updated), DateModified = no change
    • LastUsed = no change

Comment 3

[superstuff]

And if it's possible to have more than one username per domain (I'm not sure whether this is allowed but I also think it might be), then domain + username should be the identifying factor to determine new vs. existing. But the rest of the flow would be similar based on the password changing (or not).

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to superstuff from comment #2)

Is there a reason we're letting users control the timestamps at all? To me that seems like metadata that the user shouldn't need to control directly. For sure we should be exporting these fields to the CSV, but should we be importing them as well?

The CSV import was originally conceived as both a way to move saved passwords between firefox profiles as well as a way to import data from other password management software. I'm not sure if that changes the reasoning behind the proposed flow - or in fact if we have users relying on this for any reason - but that was this historical context at least.

Comment 5

[superstuff]

Alright, in that case it sounds like someone importing new entries from another account probably wants their metadata to be imported unchanged.

That much is probably straightforward, but then the controversial case is if the entry already exists, you could argue over whether to use the existing metadata or the new metadata.

I think the simplest thing that will still fix the bug is, "trust the user, unless the user is wrong", meaning we always prefer the CSV timestamps unless they are empty or invalid. If they are empty/invalid, we use the existing timestamp (if it exists) or write a new one if it doesn't.

Then lastly, it sounds like this user wants us to go a step further and say, "If all 3 CSV timestamps were empty/invalid AND all 3 had existing values to fallback on, then we didn't actually change or write any data, hence this is a duplicate and not an updated entry."

Comment 6

[superstuff]

There's a rudimentary way to fix this bug (and marks the login as a duplicate entry the way the user wants), but it only works in the case where all 3 timestamps are empty.

We can change the logic here: https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.sys.mjs#121-127

to make it so that not only does ${loginData.timeCreated} need to be different from ${existingLogin.timeCreated}, it also needs to be not empty. And we do a similar change for ${loginData.timePasswordChanged} and ${loginData.timeLastUsed}.

That way if all 3 are empty (and if no other field differs from existingLogin), it won't even enter the code block for an updated entry, and will instead be routed to the duplicate entry code block.

However, the solution is incomplete because if a single timestamp is present (and different), but the other 2 are empty, it will still enter the update code block and will change the one timestamp and set the other two to empty. We probably don't want this.

That seems to happen here: https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.sys.mjs#366

where for all logins marked "modified", we call Services.logins.modifyLogin(), only here's where I'm stuck because I can't figure out how to get inside this function and change it so that only non-empty timestamps can overwrite an existing timestamp, which I think is what we need to do next. But I just can't seem to find the code that it's calling, I see the definition of the function but I can't locate the implementation. If anyone can help out I would greatly appreciate it.

Comment 7

[superstuff]

Attachment: Bug 1700981 - Removing timestamps from login CSV no longer affects existing logins r=issammani

Comment 8

[superstuff]

I think I found a way around having to modify Services.logins.modifyLogin(). We can just make the property bag (which contains the new field data to apply) only accept non-empty values. So an empty timestamp won't get added to the bag, and won't be applied.

There is one other reference to newPropertyBag() however I don't believe it to be affected by this change.