1839873 - Credentials are not updated if imported from CSV file

Status: NEW

(Toolkit :: Password Manager, defect, P3)

Description

[Virgil Sangerean]

Found in
116.0a1 (2023-06-21)

Affected versions
116.0a1 (2023-06-21)

Tested platforms

• Affected platforms: Windows 10, Windows 11, Mac 12 ARM, Ubuntu 20, Ubuntu 22.

Preconditions
ensure signon.management.page.fileImport.enabled is set to True in about:config.
have a valid CSV file with saved credentials

Steps to reproduce

1. Open Firefox and go to about:preferences#general and click on Import Data
2. Select "Passwords from CSV files"
3. Select the valid CSV file and press Open
4. Open the already-imported CSV file and modify all passwords to something identifiable (Ex: some/all passwords set to "1")
5. Reimport the file with the modifications made

Expected result
The passwords should be updated.

Actual result
The passwords are not updated.

Regression range
Not a regression. This started with the New Migration Wizard.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Is this also what occurs if the CSV file is imported via about:logins?

Comment 2

[Neil Deakin]

I don't see this issue on Mac at least either using the migration dialog or the Import from a File menuitem in the login manager.

Comment 3

[Neil Deakin]

OK, I can reproduce the error only if the row has no guid assigned. There is some code in https://searchfox.org/mozilla-central/rev/d307d4d9f06dab6d16e963a4318e5e8ff4899141/toolkit/components/passwordmgr/LoginHelper.sys.mjs#95 which compares the guid and doesn't treat it as a duplicate when no guid is present.

To summarize, if we import an item from a csv file where a matching guid is found, we successfully import and modify the existing item's password. If the guid is not present, we treat the entries as duplicates and do not import them.

I think this matches the intent in bug 1687852 where this was implemented, specifically, quoting from that bug:

  "We explicitly support importing data with guids that match saved logins. This is a way for the user to rollback or restore a backup of logins. Import should proceed."

and:
"Any entry with exact matching fields, where modifying the first login with the values of the second would result in no change is a duplicate. This is also true where we don't have the information necessary to resolve conflicting values - e.g. 2 different passwords on an otherwise matching login, with no timestamps to know which supercedes the other. In these case we leave the first login as-is nd mark the line in the report for the 2nd login as unchanged so the user can manually update if they want to."

Sam, do you recall if seems like the intended behaviour?

Comment 4

[Virgil Sangerean]

Hello.
I can confirm that the issue is reproducing on all OSs mentioned above, also from about:logins.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Alright, thank you Virgil. In that case, I believe this falls into the domain of the credential management team.

Comment 6

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

Sam, do you recall if seems like the intended behaviour?

We don't get the file last-modified date from the .csv file, and if the dates in the data row aren't changed, we don't have a way to know if the imported data is a stale duplicate to be ignored, or an update. So I can see why we might get this result. But the expected outcome from the STR also seems like a reasonable use case. I'll leave it to :serg to make a call here, but lets not lose track of the re-import case, which I think is an important one:

"We explicitly support importing data with guids that match saved logins. This is a way for the user to rollback or restore a backup of logins. Import should proceed."

Comment 7

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

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

lets not lose track of the re-import case, which I think is an important one:

"We explicitly support importing data with guids that match saved logins. This is a way for the user to rollback or restore a backup of logins. Import should proceed."

In light of that, erring on the side of updating rather than ignoring seems like the best outcome, when we have no other cues to decide.

Comment 8

[Neil Deakin]

OK, let's see what serg thinks too before making any changes.

Comment 9

[Sergey Galich [:serg]]

We can't overwrite data in Firefox by data from CSV.
If we do that, we will lose data in Firefox when we have no idea which one user really needs. Current behavior is regretfully correct :)

This bug is valid though. We offer a poor service to users when it comes to re-import or data preservation. This will be blocked until we implement bug 1739483, then we can overwrite as much as we want.