Open Bug 1702149 Opened 4 years ago Updated 1 years ago

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

Categories

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

Desktop
All
enhancement

Tracking

()

ASSIGNED
Tracking Status
firefox87 --- disabled
firefox88 --- affected
firefox89 --- affected

People

(Reporter: srosu, Assigned: sy.fen0)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached image MissingProtocol_MissingURL.gif (deleted) —

[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.
Mentor: sfoster
Severity: N/A → S3
Priority: -- → P2

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.

Flags: needinfo?(sfoster)

(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)

Flags: needinfo?(sfoster)

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

Flags: needinfo?(sfoster)

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.

Flags: needinfo?(sfoster)

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

Flags: needinfo?(sfoster)

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

Assignee: nobody → anshusinghjsk
Flags: needinfo?(sfoster)

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

Assignee: anshusinghjsk → nobody

Can I work on this?

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.

Assignee: nobody → sy.fen0
Status: NEW → ASSIGNED
Attachment #9328704 - Attachment is obsolete: true

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

Mentor: sfoster
Flags: needinfo?(sgalich)

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

Flags: needinfo?(sgalich)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: