1413451 - A login's formSubmitURL should only contain the origin, no path components

Status: RESOLVED FIXED

(Firefox for iOS :: Login Management, enhancement, P2)

Description

[Matthew N. [:MattN]]

Firefox iOS clients are syncing invalid login data to other platforms since the formSubmitURL is supposed to only contain the origin. Yes, I know the naming was poor so I understand the confusion, see bug 1148205.

The problem is in _getPasswordOrigin "All of this logic is moved to swift (so that we don't need a uri parser here)"[1] but the Swift code didn't run the equivalent code on the formSubmitURL, only on the form's origin[2].

[1] https://github.com/mozilla-mobile/firefox-ios/blob/52fab258bec9100b869a386ec0399d4eab1b4c9d/Client/Assets/LoginsHelper.js#L583
[2] https://github.com/mozilla-mobile/firefox-ios/blob/917b5e8a7e59d2c2c938ad11b4d71b64949ceacc/Storage/Logins.swift#L292-L300

Comment 1

[Richard Newman [:rnewman]]

Matt, presumably this requires data cleanup and resyncing?

Comment 2

[Matthew N. [:MattN]]

(In reply to Richard Newman [:rnewman] from comment #1)
> Matt, presumably this requires data cleanup and resyncing?

Ideally, yes or we'll have to fix pwmgr code on all clients to handle this. I guess we may need data cleanup on multiple platforms anyways though (in case a user who used Fx iOS no longer uses it but uses the same FxA account).

Comment 3

[:garvan]

Matt we are trying to prioritize bugs and are unsure of the severity of this, can I get your opinion on that?

Comment 4

[Matthew N. [:MattN]]

I haven't had time to check how each of the platforms handle this kind of bad data. It's not hard to reproduce if you have sync setup and save a login on iOS. Then you can test if it autofills on desktop and Android.

Comment 5

[Matthew N. [:MattN]]

I don't really have time to investigate this right now and it doesn't require any specialized knowledge so maybe QA can help figure out the severity? I really think we should just fix this since even if it's not causing problems now, it's likely to cause problems in the future.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Matt, I have a WIP PR that strips all path components from the `formSubmitURL` going forward. Should we also run a "DB migration" that essentially goes through and corrects all the saved logins?

Comment 7

[Matthew N. [:MattN]]

Awesome!

Yes, probably but make sure you do that in a way that triggers a Sync (if Sync is setup). It would probably be useful to talk to the Sync team about that to make sure we don't DDOS the Sync server when the new FxiOS with the fix comes out but I suspect the volume is low enough to not be a big deal.

Comment 8

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-mobile/firefox-ios/commit/a880e931ff4964344a506030b7baab36b772c051

Comment 9

[Justin D'Arcangelo [:justindarc]]

To clarify, the PR that landed only corrects this behavior going forward. Cleaning up the old, existing saved logins data will happen within the Rust application-services "Logins" library.