Page MenuHomePhabricator

Add 'autocomplete="off"' to MFA TOTP inputs
ClosedPublic

Authored by epriestley on Oct 1 2018, 5:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 1:10 AM
Unknown Object (File)
Sun, Nov 24, 12:53 PM
Unknown Object (File)
Oct 14 2024, 11:23 PM
Unknown Object (File)
Oct 2 2024, 10:57 PM
Unknown Object (File)
Oct 1 2024, 3:10 PM
Unknown Object (File)
Sep 23 2024, 5:35 PM
Unknown Object (File)
Sep 18 2024, 2:23 AM
Unknown Object (File)
Sep 6 2024, 4:07 AM
Subscribers
None

Details

Summary

Ref T13202. See https://discourse.phabricator-community.org/t/2fa-input-box-isnt-hinted-as-a-password-so-browsers-suggest-auto-fills/1959.

If browsers are autofilling this, I think browser behavior here is bad, but behavior is probably better on the balance if we hint this as autocomplete="off" and this is a minor concesssion.

Test Plan
  • I couldn't immediately get any browser to try to autofill this field (perhaps I've disabled autofill, or just not enabled it aggressively?), but this change didn't break anything.
  • After the change, answered a TOTP prompt normally.
  • After the change, inspected page content and saw autocomplete="off" on the <input /> node.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley edited the test plan for this revision. (Show Details)

While you're here, it always bugs me that the 2FA page doesn't automatically put the cursor in the input box. Is that a one-line change?

This revision is now accepted and ready to land.Oct 1 2018, 7:37 PM

The dialog version of the TOTP page does autofocus for me in Safari and Chrome, at least (Workflow.js near line 338 focuses the most-upper input, text area, or button after showing a dialog).

We don't currently ever focus inputs on standalone pages on load. We could, but it's possible this would do some weird stuff, so I'd be slightly hesitant to just start doing it without some testing/consideration. Offhand, maybe there'd be a weird jumpy behavior on pages like Conduit API pages, where the first input that gets focused is way way down the page? For those pages, at least, it seems preferable not to focus anything by default.

A safe change might to be to run the same focus code when dialogs render as standalone pages (that is, only on the "render dialog as standalone" pathway, not the "render any standalone page" pathway). Since we know these pages also render as dialogs sometimes and thus get the focus code run on them, this should always be safe, although the code will need to be careful to focus only inputs in the dialog itself, not inputs on the whole page (otherwise, it will always just focus the global menu bar search field, I think). We might also need to do a little planning around what happens with the persistent chat thing when other stuff tries to steal focus -- it seems like we currently de-focus it even if you're in the middle of typing something, which seems not-so-great.

Upshot: probably not one line, but maybe a tractable change? Happy to review either the "focus standalone dialog pages" or "focus every page" changes, the latter just might need some babysitting and a mess of a test plan involving Quicksand, persistent chat, etc.

This revision was automatically updated to reflect the committed changes.