1673196 - Login autocomplete broken if aria-labelledby is used inside Shadow DOM: TypeError: can't access property "textContent", labelledBy[0] is null

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Matthew N. [:MattN]]

If aria-labelledby is used on a <input type=password> inside ShadowDOM, password autocomplete will break with a throw exception.

Example:

#shadow-root
  <label id="paper-input-label-2">Password</label>
  <input aria-labelledby="paper-input-label-2" type="password">

Stack trace:

LoginAutoComplete: TypeError: can't access property "textContent", labelledBy[0] is null
    hasLabelMatchingRegex resource://gre/modules/NewPasswordModel.jsm:143
    hasCurrentLabel resource://gre/modules/NewPasswordModel.jsm:404
    getSubfactsFromFunction resource://gre/modules/third_party/fathom/fathom.jsm:1687
    fact resource://gre/modules/third_party/fathom/fathom.jsm:1734
    updateFnode resource://gre/modules/third_party/fathom/fathom.jsm:2288
    forEach resource://gre/modules/third_party/fathom/fathom.jsm:608
    results resource://gre/modules/third_party/fathom/fathom.jsm:2278
    _execute resource://gre/modules/third_party/fathom/fathom.jsm:2683
    get resource://gre/modules/third_party/fathom/fathom.jsm:2598
    _computeType resource://gre/modules/third_party/fathom/fathom.jsm:2107
    scoreFor resource://gre/modules/third_party/fathom/fathom.jsm:2008
    _isProbablyANewPasswordField resource://gre/modules/LoginAutoComplete.jsm:730
    _requestAutoCompleteResultsFromParent resource://gre/modules/LoginAutoComplete.jsm:676
    startSearch resource://gre/modules/LoginAutoComplete.jsm:639

The code is making two bad assumptions:

1. That the label element will be found by document.getElementById
   • for ShadowDOM it should be ShadowRoot.getElementById)
2. It assumes that the id specified will always be found
   • Authors can make typos or elements can be missing and we shouldn't throw in that case

Assigning S2 since there isn't a workaround (other than manually opening about:logins and copying the password) since the login context menu doesn't work in this case and because this affects the popular Home Assistant web app.

Comment 1

[Matthew N. [:MattN]]

Attachment: Test case

STR:

1. Load the test case
2. Click into the password field
3. Clear the password field if it was autofilled
4. Hit the down arrow

Expected result:

• Login autocomplete should appear showing any saved logins for the domain along with the View Saved Logins footer

Actual result:

• No autocomplete appears
• Browser Console contains the error from comment 0.

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1673196 - NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster,bdanforth,erik

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d09f42a1b754
NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9183635 [details]
Bug 1673196 - NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster,bdanforth,erik

Beta/Release Uplift Approval Request

• User impact if declined: Password autocomplete doesn't show on fields using aria-labelledby with Shadow DOM. This affects popular apps like Home Assistant and other polymer apps.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward change to use the appropriate root node but also added a truthiness check to handle when ID reference doesn't exist.
• String changes made/needed: None

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d09f42a1b754

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9183635 [details]
Bug 1673196 - NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster,bdanforth,erik

Low risk and S2, approved for 83 beta 6, thanks.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/08a92e35a77c

Comment 8

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Firefox Release 82.0.2 (64-bit) on Windows 10 x64.
Verified-fixed on latest Nightly 84.0a1 (2020-10-29) (64-bit) on Windows 10 x64.

Hey Matt, I verified that the dropdown is indeed displayed if I click on the empty field, but you did mention to clear the field if it was autofilled and use the dropdown arrow. The password field is not getting autofilled on page load when I have only 1 credential saved for this site. I also checked other forms that use shadowDOM (not this specific scenario tho) and there the password-only form is autofilled (ex: https://bugs.mattn.ca/pwmgr/login_and_change_form.html#shadow)

Let me know if this patch handles only the dropdown issue and the autofill one should be handled in a different bug or what is going on with it. Thank you! Will verify further on the other OSes afterward.

Comment 9

[Matthew N. [:MattN]]

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

Hey Matt, I verified that the dropdown is indeed displayed if I click on the empty field, but you did mention to clear the field if it was autofilled and use the dropdown arrow. The password field is not getting autofilled on page load when I have only 1 credential saved for this site. I also checked other forms that use shadowDOM (not this specific scenario tho) and there the password-only form is autofilled (ex: https://bugs.mattn.ca/pwmgr/login_and_change_form.html#shadow)

Let me know if this patch handles only the dropdown issue and the autofill one should be handled in a different bug or what is going on with it. Thank you! Will verify further on the other OSes afterward.

Yeah, this only handles the dropdown issue as autofill for shadow dom is tracked separately. I included that step just in case some is following the STR in the future or was testing in a failure case without shadow DOM (e.g. aria-labelledby referring to a non-existing element).

Comment 10

[Timea Cernea [:tbabos][inactive]]

Thanks Matt!
Verified-fixed on latest Firefox Nightly 84.0a1 (2020-11-01) (64-bit) and Beta 83.0b6 (64-bit) on Windows 10, macOS 10.15 and Ubuntu 16.04.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR78 also?

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 9183635 [details]
Bug 1673196 - NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster,bdanforth,erik

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Trivial fix for a JS exception affecting popular apps like Home Assistant and other Polymer apps.
• User impact if declined: Password autocomplete doesn't show on fields using aria-labelledby with Shadow DOM. This affects popular apps like Home Assistant and other Polymer apps.
• Fix Landed on Version: 84
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward change to use the appropriate root node but also added a truthiness check to handle when ID reference doesn't exist.
• String or UUID changes made by this patch: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9183635 [details]
Bug 1673196 - NewPasswordModel: Don't assume @aria-labelledBy refers to an ID present in the document. r=sfoster,bdanforth,erik

Approved for 78.5esr, thanks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/ad575a040c53

Comment 15

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest 78.5.0esr (32-bit) (buildID: 20201110001500) on Windows 10 x64 and macOS 10.15.