Closed Bug 1646610 Opened 7 months ago Closed 5 months ago

Consider matching base domains (eTLD+1) instead of origin for formActionOrigin

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
81 Branch
Tracking Status
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- verified

People

(Reporter: bdanforth, Assigned: tgiles, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(2 files)

formActionOrigin (formerly formSubmitURL) was added in Bug 360493, and in Bug 1121119, we considered ignoring it, but ultimately decided to keep it as a precaution against a "very narrow attack scenario" given that we autofill forms without user interaction.

The telemetry that was added to support ignoring formActionOrigin in Bug 1124888 indicated that 17% of the time, we do not autofill solely due to a difference in formActionOrigin value, with plausible reasons for why this might occur.

While we may not want to ignore formActionOrigin, we may help to lower this 17% by considering the base domain (eTLD+1) instead of the origin. This would allow us to autofill on subdomains for a site, which we hypothesize is a common situation. Are there any reasons why this is not a good idea?

Flags: needinfo?(dveditz)
See Also: → 1640639
Severity: -- → S3

given that we autofill forms without user interaction.

which is, of course, password manager's original sin. I do not understand out complete rejection of addressing that point.

The biggest risk for autofill w/out interaction is when an attacker can inject script, and XSS is still one of the most common website vulnerabilities even at companies with the skills and resources of Google (we've had presentations at W3C conferences).

Using the (eTLD+1) instead of full domain in a secure context (e.g. https: page) won't make us much less secure. Will it solve your problem, though? How common are the cases ckarlof mentions in bug 1121119 comment 12? If most of that 17% turns out to be javascript: urls or no form action at all then this change won't help much at all.

Flags: needinfo?(dveditz)

(In reply to Daniel Veditz [:dveditz] from comment #1)

given that we autofill forms without user interaction.

which is, of course, password manager's original sin. I do not understand out complete rejection of addressing that point.

There isn't a complete rejection of this… that's why we added the user-visible pref in about:preferences#privacy-logins last year. I've commented somewhere else before that we want to get the password manager working well before hindering the UX by not autofilling. In order to have a decent UX comparable with other browsers it's not as simple of changing the default value of this pref… Chrome still autofills AFAICT and Safari only stopped autofilling when they added auto-submission of login forms. Implementing the latter is quite difficult (adding even more heuristics to password manager which is complicated enough) and error-prone making it not a priority at the moment.

Using the (eTLD+1) instead of full domain in a secure context (e.g. https: page) won't make us much less secure. Will it solve your problem, though?

Anecdotally I think it will help a large proportion of the 17% as I've seen subdomains changing between the following common subdomains: 'www', 'secure', 'login', & 'm'.

How common are the cases ckarlof mentions in bug 1121119 comment 12? If most of that 17% turns out to be javascript: urls or no form action at all then this change won't help much at all.

Only 0.24% of my 800+ saved logins have a formActionOrigin of javascript: and anecdotally it doesn't seem to be a common pattern nowadays. "No form action" means we use the document origin and therefore could still be helped by this change.

After landing this change we should be able to approximate the effect by looking for a decrease in AUTOFILL_RESULT.NO_SAVED_LOGINS telemetry as it's fairly stable. If you aren't objecting to this change then I'd like to try it, with a pref to revert if necessary.

Flags: qe-verify+
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All

To implement this improvement:

./mach test toolkit/components/passwordmgr/
Mentor: MattN+bmo
Whiteboard: [lang=js]
Assignee: nobody → tgiles
Status: NEW → ASSIGNED
Attachment #9169499 - Attachment description: Bug 1646610 - Match base domains (eTLD+1) instead of origin for formActionOrigin. r=bdanforth,MattN! → Bug 1646610 - Match base domains (eTLD+1) instead of origin for formActionOrigin. r=MattN
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/8cb8f7c2eee3
Match base domains (eTLD+1) instead of origin for formActionOrigin. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Attached file bug-1646610-script.js

Steps to reproduce:

  1. Enable the devtools.chrome.enabled pref in about:config
  2. Open the "Browser Console"
  • Ctrl+Shift+J on Windows
  • Cmd+Shift+J on Mac
  1. Run attached bug-1646610-script.js in the Browser Toolbox
  2. Navigate to https://twitter.com
  3. Verify test credentials have been auto filled
  • TestUser for username
  • TestPassword for password

Verified-fixed on latest Nightly 81.0a1 (2020-08-20) (64-bit) on Windows 10 x64 and MacOS 10.13 based on the steps provided in Comment 7.
Waiting for potential Uplift to Fx80.

(In reply to Timea Cernea [:tbabos] from comment #8)

Waiting for potential Uplift to Fx80.

This is new behaviour that I don't think we should uplift.

Thanks Matt, marking this as verified-fixed and removing qe+ flag.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.