Repository navigation
Conversation
…. r?sgalich Autofill would fill password fields that exist in the DOM but are not rendered, for example type="password" inputs inside hidden containers for alternative, inactive login flows. Credentials silently written into fields the user cannot see or edit break sites that serialize every input of the form, such as S-Pankki's FTN QR login (bug 1783962). Gate the non-user-triggered fill path in _fillForm on the rendered state of the fill target: the password field, or the username field for username-only forms. When Element.checkVisibility() reports the field has no box (e.g. inside a display:none or [hidden] ancestor), bail out with a new AUTOFILL_RESULT.FIELD_NOT_RENDERED before writing any value. The gate runs after autocomplete attachment, so a field that becomes visible later still offers the autocomplete popup; user-triggered fills (doorhanger, context menu) are unaffected. FIELD_NOT_RENDERED is deliberately not counted in the pwmgr.form_autofill_result Glean labeled counter yet: adding a label to an "expires: never" metric requires data review. A follow-up can add the field_not_rendered label and drop the guard. Also render the fixture in test_input_events.html: that test asserts the input events fired by page-load autofill, which now only happens for rendered fields.
No Taskcluster jobs started for this pull requestThe |
Contributor
|
(Automated Close) Please do not file pull requests here, see https://firefox-source-docs.mozilla.org/contributing/how_to_submit_a_patch.html |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug 1247245
What
Page-load autofill writes saved credentials into password fields that are not rendered — e.g. a
type="password"username input inside a[hidden]container of an inactive "log in by username instead of QR" flow, markedautocomplete="off". Gate the non-user-triggered fill path in_fillFormon the rendered state of the fill target (passwordField ?? usernameField): whenElement.checkVisibility()reports no box, bail out with a newAUTOFILL_RESULT.FIELD_NOT_RENDEREDbefore any value is written.Untouched by design:
userTriggered=truepaths are not gated;_fillFormonly);markAsAutoCompletableField, so the autocomplete dropdown still appears on focus;checkVisibility()never consultsdocument.visibilityState(validated againstElement.cppsemantics and empirically).Why
Silent real-world breakage: S-Pankki's FTN federated login (Suomi.fi / vero.fi) serializes every form input into
POST /ftn/auth/encap/start. The autofilled hidden value binds the Encap authentication session to the wrong identity, the phone app cannot complete the handshake, and the page polls/ftn/auth/encap/finishforever withprocessStatus:"IN_PROGRESS"— no error, no timeout (see also bug 1783962). Deleting the saved login is the only fix for affected users. The same structure (single masked username field in a hidden container, no other fillable inputs) is common on bank/government login pages.Note the capture→refill loop that produces poisoned logins: banks mask the username input as
type="password"; submitting that flow makes Firefox offer to save the username's content as a password, which is later autofilled back into the hidden masked field of the other flow.Testing
username=190774in the page's serialize-all-fields payload); with the patch the field stays empty in all pref variants, includingsignon.usernameOnlyForm.enabled=false(the username-only heuristic is not involved —getUsernameFieldFromUsernameOnlyFormreturns null for any form containing a password field)../mach mochitest toolkit/components/passwordmgr/test/on the patched build: full suite; the watchlist for this change is green —test_autofill_from_bfcache.html,browser_focus_before_first_DOMContentLoaded.js,browser_glean_pwmgr_form_autofill_result.js(validates the Glean guard),browser_autofill_hidden_document.js.test_input_events.htmlasserted autofill into adisplay: nonefixture (exactly the removed behavior), so this patch renders that fixture — its purpose (autofill fires input/change events) is unchanged; it passes with the update. The two remaining unexpected results (test_LoginManagerContent_passwordEditedOrGenerated.html,browser_doorhanger_urlbar_focus.js) reproduce identically on an unpatched trunk build.FIELD_NOT_RENDEREDis deliberately not counted inpwmgr.form_autofill_resultyet: adding a label to anexpires: neverlabeled counter requires data review. The guard in the finally block makes the follow-up (label + count) trivial once approved.Lando: link
Bugzilla: bug 1247245
🚫 This pull request has 2 blockers.