Closed Bug 1833264 Opened 1 year ago Closed 1 year ago

Ctrl+Z (Edit|Undo) is not available & has no effect in password fields

Categories

(Core :: DOM: Forms, defect)

defect

Tracking

()

VERIFIED FIXED
115 Branch
Tracking Status
relnote-firefox --- 115+
firefox115 --- fixed

People

(Reporter: dholbert, Assigned: masayuki)

References

Details

Attachments

(2 files)

STR:

  1. Load attached testcase.
  2. Focus the password field.
  3. Type or paste in some text.
  4. Press Ctrl+Z. Or: open the edit menu, and look for "Undo".

ACTUAL RESULTS:
Ctrl Z has no effect. In the edit menu, "Undo" is grayed out (as if there's nothing to undo)

EXPECTED RESULTS:
My text entry (or paste) should be undone, just as it would be for a textfield.

I ran into this in-the-wild when I accidentally pasted two copies of my password into the same password field. I instinctively hit Ctrl+Z to undo the second paste operation, but to my surprise, it had no effect.

Attached file testcase 1

I'm using Firefox 115.0a1 (2023-05-15) (64-bit) on Ubuntu 22.04.

I also tested Firefox 115 nightly on macOS, and Firefox 113 (release) on Win11. And I tested some older builds -- e.g. Nightly 2018-04-21 -- via mozregression. All builds I've tested so far (all of the above mentioned ones) reproduce the same issue.

Chrome 115 dev and Safari 16.4 give me "expected results".

Trying some much-older builds... I can also reproduce in Nightly 2010-12-01 (4.0b8pre) and 2011-04-21 (6.0a1).

So: seems to not be a regression (though I can't believe I've never noticed this before).

Masayuki, do you know where we handle this behavior?

Severity: -- → S3
Flags: needinfo?(masayuki)

It was intentionally disabled in bug 271154.

dholbert: Do you think the change should be canceled for web-compat issue or something?

I instinctively hit Ctrl+Z to undo the second paste operation, but to my surprise, it had no effect.

Well, might be better to beep or something, although modern OSes stopped playing sounds for some UIs like menu etc.

Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(masayuki) → needinfo?(dholbert)
See Also: → 271154

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #5)

It was intentionally disabled in bug 271154.

dholbert: Do you think the change should be canceled for web-compat issue or something?

Yeah, I think we should revert that change. It looks like we disabled undo in password fields, at a UX cost (a basic keyboard function not working), for a purported security & compat benefit; however:
(1) The compat benefit didn't actually exist and was a misunderstanding (see bug 271154 comment 7 and bug 271154 comment 8), so this was and remains a Firefox-only behavior.
(2) The security benefit seems extremely marginal. The attack scenario (having typed your full password into a login form, and then deleted it, and then walked away from your keyboard) seem pretty strained -- and in that scenario, there would be plenty of other similarly-sensitive bits of information (e.g. session cookies in particular, if you forgot to log out of some website) that someone could just as easily take, which we don't bother to protect.

The "browser continuously running on a public computer where users might type in their password and then backspace it and then just walk away" attack scenario from bug 271154 perhaps made some sense for internet cafes / hotel-lobby-computers in 2004; but these days, a setup like that would almost certainly want to involve a browser restart [at a minimum] between usages. Otherwise, there's all sorts of potentially-leaked-between-users sensitive user data, and Ctrl+Z-disabling isn't a particularly robust protection.

dveditz, I should probably defer to you on a security-judgement here, both in general & since you were involved in bug 271154 -- do you agree that it'd be fine to revert bug 271154 and reenable Ctrl+Z in password fields (matching how all other browsers behave & have always behaved)?

Flags: needinfo?(dholbert) → needinfo?(dveditz)

With a simple text input, when you reload the page the undo-history is gone -- even if partial text you entered is preserved in a simple reload (ctrl-r) versus a field-clearing forced refresh (ctrl-shift-r). That should be just fine for password fields. Yeah, go ahead and revert.

Flags: needinfo?(dveditz)

Thanks. Looks like these days, this check lives here:
https://searchfox.org/mozilla-central/rev/e409c755f313f6befb2c00f260bf873ccc191924/dom/html/TextControlState.cpp#1933-1939

if (IsPasswordTextControl()) {
  // Disable undo for <input type="password">.  Note that we want to do this
  // at the very end of InitEditor(), so the calls to EnableUndoRedo() when
  // setting the default value don't screw us up.  Since changing the
  // control type does a reframe, we don't have to worry about dynamic type
  // changes here.
  DebugOnly<bool> disabledUndoRedo = newTextEditor->DisableUndoRedo();

Maybe DisableUndoRedo and the associated machinery can all disappear, with this removed? At first glance, it looks like it's only used for this password-field-disabling and also to provide a JS-exposed enableUndo API on nsIEditor, but that API seems to be unused except by tests, so I'm not sure if it's still needed:
https://searchfox.org/mozilla-central/search?q=enableUndo&path=&case=true&regexp=false

(Bouncing back to masayuki for another look - hopefully we can move this back from UNCONFIRMED to NEW, and maybe even ASSIGNED if he's up for taking it? :) )

Flags: needinfo?(masayuki)

Thank you for clarification! I'll revert the change.

Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(masayuki)

It was intentionally disabled in bug 271154 in 2004 for protecting users use
PC in public spaces. However, in these days, only protecting from undo
transactions is not enough and most users must use smart phones instead.
Therefore, let's revert the change for compatibility with the other browsers.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e6d63c2a651d
Enable undo/redo in password fields r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40242 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
Upstream PR merged by moz-wptsync-bot

:masayuki could you please consider nominating this change for a relnote? https://wiki.mozilla.org/Release_Management/Release_Notes_Nomination

Flags: needinfo?(masayuki)

Release Note Request (optional, but appreciated)
[Why is this notable]: A major behavior change of password fields
[Affects Firefox for Android]: Yes
[Suggested wording]: Undo and redo are available in password fields. They were disabled in 2004, for security on PC shared in public space like Internet Cafe etc in 2004, but in these days, it's not reasonable to protect users and the other browsers allow password fields undo-able.
[Links (documentation, blog post, etc)]: bug 271154 and bug 1833264 comment 6.

relnote-firefox: --- → ?
Flags: needinfo?(masayuki)

Thanks, added a slightly reworded note to the Nightly release notes. Keeping the relnote? flag open to keep it on the radar for inclusion in our final release notes.

Flags: qe-verify+

Reproduced the issue with Firefox 115.0a1 (2023-05-15) on Windows 10 and Ubuntu 22.
The issue is verified fixed with Firefox 116.0a1 (20230613092556) and Firefox 115.0b4 (20230611180300) on Windows 10, Ubuntu 22 and macOS 12.

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

Attachment

General

Created:
Updated:
Size: