1702149 - [Enhancement] Consider automatically adding the protocol for URLs that don’t have it when importing a CSV file

Status: NEW

(Firefox :: about:logins, enhancement, P2)

Description

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: MissingProtocol_MissingURL.gif

[Versions]:

• Firefox Beta 88.0b4 (Build ID: 20210328185936)
• Firefox Nightly 89.0a1 (Build ID: 20210330215136)

[Platforms]:

• Windows 10
• Ubuntu 20.04
• macOS 11.2.3

[Prerequisites]:

• Have Firefox Beta 88/Nightly 89 open.
• Have a CSV file saved on your machine that contains logins with URLs without the “https://” protocol (eg: www.facebook.com).

[Steps to reproduce]:

1. Navigate to the “about:logins” page.
2. Click on the Menu button.
3. Select the “Import from a File…” option.
4. Select the CSV file from prerequisites and click on the “Open” button from the “Import Logins File” picker.
5. Click on the “View detailed Import Summary” link.
6. Observe the logins.

[Proposed result]:

• The logins with URLs that don't have the “https://” protocol are imported and the missing protocols are automatically added to the URL.

[Current result]:

• The logins with URLs that don't contain “https://” protocol are not imported and are recognized as “Missing URL” errors.

[Notes]:

• When creating a new login in “about:logins” page, the “https://” protocol is automatically added to the URL if it is missing and this behavior should also happen for logins that are imported.
• Also, certain password manager apps allow users to save logins with URLs without a protocol.
• Attached is a screen recording of the current behavior.

Comment 1

[Sarah Ukoha]

One possible misgiving:
Some URLs are locally domained, and may not be secured. Hence, wouldn't it be a bit on the tentative side if we presumably auto-fill "https://" at all times?

Meanwhile, I will be much obliged to give this a go.

Comment 2

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

(In reply to Sarah Ukoha from comment #1)

One possible misgiving:
Some URLs are locally domained, and may not be secured. Hence, wouldn't it be a bit on the tentative side if we presumably auto-fill "https://" at all times?

Yes that's true, "https://" would be a guess and might not be the right value, but in the absence of any other information, this seems better than doing nothing (the URL would simply not work and the entry would fail to import). This is already an edge-case, so I'm comfortable with this trade-off.

Do you have the information you need to develop a patch for this? Its looks like you've worked on a couple of bugs already. Let me know if you have any questions and feel free to attach a work-in-progress patch if you want feedback ahead of actual review (moz-phab submit --wip)

Comment 3

[Sarah Ukoha]

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

(In reply to Sarah Ukoha from comment #1)

One possible misgiving:
Some URLs are locally domained, and may not be secured. Hence, wouldn't it be a bit on the tentative side if we presumably auto-fill "https://" at all times?

Yes that's true, "https://" would be a guess and might not be the right value, but in the absence of any other information, this seems better than doing nothing (the URL would simply not work and the entry would fail to import). This is already an edge-case, so I'm comfortable with this trade-off.

Do you have the information you need to develop a patch for this? Its looks like you've worked on a couple of bugs already. Let me know if you have any questions and feel free to attach a work-in-progress patch if you want feedback ahead of actual review (moz-phab submit --wip)

Oh, yes!
I didn't keep a track of this bug, just saw your message.
Yes, I would love to give it a shot. I just need to know/find the js file that prompts the "Error: Missing URL"
As per the files to modify, I have traced about-logins-import-dialog-items-error to browser/components/aboutlogins/content/components/import-summary-dialog.js
I think I need help to find the exact js file that processed each column of the datarow.

Comment 4

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

The code to read the file and parse out the column headers and data is in/around https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginCSVImport.jsm#141 and that raw data is then passed to LoginHelper.maybeImportLogins. I think that's where I would expect cleanup of the URL field to happen and this prepending of the default scheme - you'll see we do some similar cleanup of the data there already for other fields.

Most of the test coverage for login import is xpcshell unit tests, see: https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js, I would expect a couple new tests to be a part of the patch to implement this.

Comment 5

[kaira [:anshukaira]]

I would like to take upon this bug if no one has already been working on it.

Comment 6

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

(In reply to kaira [:anshukaira] from comment #5)

I would like to take upon this bug if no one has already been working on it.

Hi :anshukaira, thanks for looking at this one. You know the drill, I'll assign the bug to you for now. You can find this team in the #lockwise-desktop:mozilla.org channel on matrix If you have questions, feel free to ask on this bug or in the channel.

Comment 7

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

I don't think kaira is actively working on this. Resetting to unassigned.

Comment 8

[Riya]

Can I work on this?

Comment 9

[Stefan Yohansson]

Attachment: Bug 1702149 - Add default scheme when importing login from CSV.

When creating a new login from the UI in about:logins, the form auto adds the https:// scheme as default. Using the same behavior for CSV import.

Comment 10

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

Unassigning myself as mentor. :serg, are you still looking for a patch for this bug?

Comment 11

[Sergey Galich [:serg]]

Yep, would accept the patch. The https://phabricator.services.mozilla.com/D175564 got abandoned due to some unrelated issue I think.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.