Basic Auth credentials are queried and saved without protocol decoration

RESOLVED FIXED

Status

()

Firefox for iOS
Data Storage
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sleroux, Assigned: sleroux)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios2.0+)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
STR:

1. Navigate to Login List
2. Tap Edit
3. Tap 'Select All' button

Expected:

Button title should update to 'Deselect All'

Actual:

Button title remains the same

-----

I wasn't able to reproduce this but :tecgirl was and after some debugging it looks like the condition here [1] doesn't pass. The data source is reporting a higher number of items than that are visible.

[1] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Login%20Management/LoginListViewController.swift#L146
(Assignee)

Comment 1

2 years ago
I've updated my test branch with some extensive logging to help pin down the issue. I've scoped the logging for logins only to display their hostnames and exclude anything else.

:tecgirl when you get a chance could you try running this patch and sending over the logs?

Branch: https://github.com/mozilla/firefox-ios/tree/sleroux/Bug1238103-MissingLoginsBug
New commit: https://github.com/mozilla/firefox-ios/commit/c1113ebb8bb1d9dd131399c0c39c4a33bc01f63a
Flags: needinfo?(randersen)
Created attachment 8706698 [details]
output logs from 6s sim
Flags: needinfo?(randersen)
(Assignee)

Comment 3

2 years ago
Looks like the logic for grabbing the baseDomain from the hostname URL is failing because 2 of the logins don't include the scheme:

phonebook.mozilla.org
pto.mozilla.org

:rnewman, is this normal for Logins to have their hostnames not include a scheme? These logins were pulled down from the server without modification.
Flags: needinfo?(rnewman)
I think this is how we save Basic Auth logins on iOS, actually! I'm looking on my own desktop profile, and I have two entries for pto.mo -- one saved from desktop last year, and one saved from iOS this year.

I wondered why we weren't autofilling those.
For Basic Auth, challenge.protectionSpace.host is a bare hostname, no protocol.

When we come to save the login, login.hostname is bare.

So this is definitely on us!
Flags: needinfo?(rnewman)
Proposed solution: we should be saving and querying the protocol (which is the field `protocol` on an NSURLProtectionSpace) as part of the URL for Basic Auth challenges, rather than just using the challenge host directly.

If we want to be super thorough, we can migrate existing records, but we'd have to be careful to avoid collisions with existing records.
Component: General → Data Storage
Hardware: Other → All
Summary: Tapping 'Select All' button when editing logins doesn't update button title to 'Deselect All' → Basic Auth credentials are queried and saved without protocol decoration
Proposing tracking because (a) we won't match synced basic auth logins from desktop, (b) we're polluting our logins DB, (c) it b0rks this feature.
tracking-fxios: --- → ?
tracking-fxios: ? → 2.0+
(Assignee)

Comment 8

2 years ago
I've been looking into this more closer and found that there is a a nomenclature problem between what we consider is a `hostname` and what NSURLProtectionSpace considers a 'host'. Theres a few things happening:

1. When encountering a regular login (non-Basic Auth), we create a credential using the url directly. For example, if I log into news.ycombinator.com, we invoke createWithHostname [1] and pass https://news.ycombinator.com as the 'hostname'. Technically its not actually the host but the host + protocol.

2. When encountering a basic auth login, we create a credential using only the bare 'host' where 'host' is the proper definition of a URL host [2]. In the case of https://pto.mozilla.org, the protocol is thrown away and we only store the host part.

NOTE: Both these cases generate hostnames that are now synced with hostnames being valid URLS (https://new.ycombinator.com) and invalid, host-only strings (pto.mozilla.org).

3. A Login struct represents its hostname data from an instance of NSURLProtectionSpace [3]. When we construct logins from the DB using the factory in SQLiteLogins [4], we're assigning the full hostname stored in the DB as the host inside NSURLProtectionSpace. This logically doesn't make sense considering that a NSURLProtectionSpace's host property is supposed to only store the host and not the scheme/protocol + host.

To reconcile the differences between host/hostnames, I propose:

1. Modify the factory to properly set the host and protocol params of NSURLProtectionSpace by converting the hostname into a URL and extracting the scheme/host from it.

2. Update the hostname getter [3] to not just return the protectionSpace.host but prefix it with protectionSpace.protocol + ://

3. Update the hostname constructor [1] to convert the hostname into a URL and assign the scheme/host to the NSURLProtectionSpace's protocol/host properties.

This works for newly created logins but I'm not sure of a good plan yet to migrate or handle already created logins with bare hostnames.

As a side note, this same discrepancy is the cause of basic auth prompts not being auto-filled. The NSURLProtectionSpace we provide the getLoginsForProtectionSpace method contains a proper host and protocol but the arguments we pass to the DB only consider the host [5].

[1] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L181
[2] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L192
[3] https://github.com/mozilla/firefox-ios/blob/master/Storage/Logins.swift#L126
[4] https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteLogins.swift#L136
[5] https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/SQLiteLogins.swift#L215
(Assignee)

Comment 9

2 years ago
Created attachment 8707070 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1432

I've attached a WIP patch implementing the proposed solution w/o consideration for migration.
Attachment #8707070 - Flags: feedback?(rnewman)
Comment on attachment 8707070 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1432

Comments left on the PR. Note in particular that we also should include non-standard ports in the URL.

Re migration: we have a couple of options.

We can "soft-migrate":

* Don't sync rows that don't start with a protocol.
* When we try to match one during an auth challenge, match against the DB without the protocol. 
  * Example: we're on https://example.org:5050. Match "example.org" as well as "https://example.org:5050" (and perhaps "https://example.org").
* If the challenge succeeds, replace the hostname in the DB with the correct expanded hostname, and mark the record as needing to be synced. Tada! We migrated one row.
* Take extreme care: if the expanded URL exists in the DB already, simply delete this row, optionally updating the other row's password to match the one that worked.

We can "hard-migrate":

* Guess the protocol (https?), perhaps by checking history.
* Assume no custom port.
* Rewrite every row directly, marking them all as needing to be synced.
Attachment #8707070 - Flags: feedback?(rnewman) → feedback+
(Assignee)

Comment 11

2 years ago
I've updated the PR to include the review nits and also started down the soft-migrate path.

Couple of things:

1. Looks like whenever we build up the login using the factory, if no scheme is provided when building a NSURLProtectionSpace then it defaults to http. This actually works nicely because it forces Login.hostname to be a valid URL all the time. This resolves the bug where not all logins were displaying the LoginListViewController.

2. The soft-migrate works pretty well for a single entry that doesn't match. I've updated the query to search for scheme://host and just host [1]. When one entry is returned, this is great because we can just update the hostname and use that credentials. Ez-Pz. The complex part is when we find duplicate logins. You mentioned that we should simply delete the partial hostname entry if a full entry is available but how do we know that the password associated with that one is the 'correct' one? There is a chance that the locally generated one is newer/correct than the full url one, no?

[1] https://github.com/mozilla/firefox-ios/commit/71ca814ae61fe46350f846fadeec9c4a6e56a46e#diff-58cadf5237e7721e748de06632b7ebceR43
(In reply to Stephan Leroux [:sleroux] from comment #11)
> You mentioned that we should simply delete the partial hostname
> entry if a full entry is available but how do we know that the password
> associated with that one is the 'correct' one? There is a chance that the
> locally generated one is newer/correct than the full url one, no?

Yes, exactly. I wrote:

--
Take extreme care: if the expanded URL exists in the DB already, simply delete this row, optionally updating the other row's password to match the one that worked.
--

The end result should be:

* The desktop row is preserved. It probably has the oldest GUID, and it's safest to keep the row that other clients are actively using.
* The one that used to have the malformed hostname is marked as deleted for Sync.
* The last password that successfully logged in is kept.

The usual update login call path will probably take care of the last… and in the soft migration case, it's only use of a login that will cause migration, so you're at the coal face at the right time.

There's a pretty good chance that both rows will have the same password, so in that case you can just drop the malformed one altogether.
(Assignee)

Comment 13

2 years ago
Comment on attachment 8707070 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1432

I've gone ahead and finished the soft-migration path by:

1. Bubbling up information allow me to know which logins contain malformed URLS by adding the hasMalformedHostname flag. Unfortunately since malformed hostnames look default to http://, there is no way of telling the difference between a valid http:// scheme and a malformed one.

2. For duplicate entries, find the first entry that matches the protectionSpace and is not malformed. For malformed entries, trim them from the DB.
Attachment #8707070 - Flags: review?(rnewman)
Comment on attachment 8707070 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1432

Comments left. A few tweaks and at least one bug. Needs a basic test!
Attachment #8707070 - Flags: review?(rnewman)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8707070 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1432

Addressed PR comments, fixed the bug, and added unit tests for the various scenarios.
Attachment #8707070 - Flags: review?(rnewman)
One main concern — out-of-range.
(Assignee)

Comment 17

2 years ago
master 4e77934bf9081dbcc8675880b35f3eb3f23c2040
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Attachment #8707070 - Flags: review?(rnewman)
You need to log in before you can comment on or make changes to this bug.