Browser keyboard shortcuts (eg copy Ctrl+C) don't work on sites that use those keys with resistFingerprinting enabled

VERIFIED FIXED in Firefox 59

Status

()

P2
normal
VERIFIED FIXED
a year ago
11 days ago

People

(Reporter: philipp, Assigned: arthur)

Tracking

(Blocks: 1 bug, {regression})

59 Branch
mozilla60
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 59+, firefox-esr52 unaffected, firefox58 unaffected, firefox59 verified, firefox60+ verified)

Details

(Whiteboard: [fingerprinting-breakage][tor 17009])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
the keyboard shortcut Ctrl+C doesn't work reliably anymore to copy text with a german keyboard when privacy.resistFingerprinting is enabled. with that setting, the shortcut fails consistently to put text into the clipboard on many sites, like gmail for example.

this has regressed with bug 1222285. i'm not sure if this is an intended by-product of that patch, but the issue is quite noticeable and aggravating over time...
Hey Arthur, I hadn't followed Bug 122285 very closely, let's talk about this this Tuesday and see what's up with this.
Flags: needinfo?(arthuredelstein)
Whiteboard: [fingerprinting-breakage]
Were you able to figure this out, Tom/Arthur?
Flags: needinfo?(tom)
We have not yet investigated this, no. I'm afraid I don't have an ETA.
Flags: needinfo?(tom)
[Tracking Requested - why for this release]:
OK, given this pref doesn't default to enabled and my understanding is that Tor is based on ESR, I'm going to set unaffected for 59 but ask for tracking with 60 (next ESR).
status-firefox59: affected → fix-optional
tracking-firefox60: --- → ?
tracking-firefox60: ? → +
Also reported in Bug 1432859.

Updated

a year ago
Duplicate of this bug: 1432859

Updated

a year ago
Duplicate of this bug: 1436309

Comment 8

a year ago
I think Bug 1222285 Part 2 should be backed out from 59 ASAP for the following reasons:

* It breaks standard browser shortcuts (copy, paste, find etc.) for every website and extension that uses those keys as shortcuts.
* The premise of the patch that "Chrome can still get these events" is incorrect for these common cases.
* There is no ETA on this regression bug and it is marked as "fix-optional" for 59. 
* Keyboard layout is still being leaked by Bug 1438795.
* It frustrates the user and makes them want to disable resistFingerprinting.

Updated

a year ago
Duplicate of this bug: 1436057

Updated

a year ago
Summary: Ctrl+C stopped working/copying on some sites with privacy.resistFingerprinting enabled → Browser keyboard shortcuts (eg copy Ctrl+C) don't work on sites that use those keys with resistFingerprinting enabled
Tom do you agree with comment 8?
Flags: needinfo?(tom)
I spoke with Arthur about this. He thinks he can get a fix together for this, so I don't think it makes sense to back it out when we can (hopefully) uplift a fix for it instead.
Assignee: nobody → arthuredelstein
Flags: needinfo?(tom)
Flags: needinfo?(arthuredelstein)

Updated

a year ago
Duplicate of this bug: 1439533
Priority: -- → P2
(Assignee)

Updated

a year ago
Blocks: 1329996
(Assignee)

Updated

a year ago
Whiteboard: [fingerprinting-breakage] → [fingerprinting-breakage][tor 17009]
(Assignee)

Comment 13

a year ago
In bug 1222285, the control key modifier state was spoofed for ordinary combinations like Ctrl+A. This was intended to provide protection for fingerprinting of Asian keyboards, but it interferes with common command-key combinations (such as Select All for Ctrl+A).

(In any case, hiding the control modifier doesn't actually provide protection if the typed key is not a character typically found on a Latin keyboard. If an Asian character is typed, we know the user is using an Asian keyboard regardless of whether the modifier is present. We can investigate in the future if there are specific key combinations that should be spoofed for particular Asian keyboards.)

It's still useful to spoof the Shift and Alt modifiers, however. For example, on a French keyboard, numeric digits [0-9] require the Shift key, whereas the same characters require no Shift on a US keyboard.
Attachment #8953689 - Flags: review?(masayuki)
Comment on attachment 8953689 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

># HG changeset patch
># User Arthur Edelstein <arthuredelstein@gmail.com>
>
>Bug 1433592 - Don't spoof ctrl key modifier for privacy.resistFingerprinting=true
>
>---
> dom/events/KeyboardEvent.cpp | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
>diff --git a/dom/events/KeyboardEvent.cpp b/dom/events/KeyboardEvent.cpp
>index 2e0aace936d5..b1527546822a 100644
>--- a/dom/events/KeyboardEvent.cpp
>+++ b/dom/events/KeyboardEvent.cpp
>@@ -1,8 +1,9 @@
>+
> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

What's this odd new line? Please remove it.

>@@ -44,25 +45,17 @@ KeyboardEvent::AltKey(CallerType aCallerType)
>   // modifier key in certain keyboard layout. For example, the '@' key for
>   // German keyboard for MAC is Alt+L.
>   return GetSpoofedModifierStates(Modifier::MODIFIER_ALT, altState);
> }
> 
> bool
> KeyboardEvent::CtrlKey(CallerType aCallerType)
> {
>-  bool ctrlState = mEvent->AsKeyboardEvent()->IsControl();
>-
>-  if (!ShouldResistFingerprinting(aCallerType)) {
>-    return ctrlState;
>-  }
>-
>-  // We need to give a spoofed state for Control key since it could be used as a
>-  // modifier key in certain asian keyboard layouts.
>-  return GetSpoofedModifierStates(Modifier::MODIFIER_CONTROL, ctrlState);
>+  return mEvent->AsKeyboardEvent()->IsControl();

I think that you should add comment here to explain why we don't need to override ctrlKey. And perhaps, you should do same thing in ::MetaKey() too for macOS.

And please add new automated test to browser_spoofing_keyboard_event.js.
Attachment #8953689 - Flags: review?(masayuki) → review-
Oh, I forgot a thing. On Windows, some keyboard layouts use Ctrl key to input character. We know Ctrl + 2 on Khmer keyboard layout inputs U+20AC.
https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/widget/tests/test_keycodes.xul#4325,4333-4334

However, currently, we unset ctrlKey before dispatching WidgetKeyboardEvent from widget. Therefore, this does NOT affect your patch directly. Although, in the future, we should not do it.

Comment 16

a year ago
So that was it… It makes GitHub barely usable since they have shortcuts on almost all common Ctrl+ combinations (like Ctrl+c to open a new issue…).
(Assignee)

Comment 17

a year ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #14)

Thank you for the review! Here's a new version with corrections made as suggested.
Attachment #8953689 - Attachment is obsolete: true
Attachment #8955359 - Flags: review?(masayuki)
Comment on attachment 8955359 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

Thank you!
Attachment #8955359 - Flags: review?(masayuki) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 19

a year ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15ab7984211
Don't spoof ctrl key modifier for privacy.resistFingerprinting=true r=masayuki
Keywords: checkin-needed

Comment 20

a year ago
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed46fff108c1
Backed out changeset f15ab7984211 for failing browser chrome at browser/components/resistfingerprinting/test/browser/browser_spoofing_keyboard_event.js on a CLOSED TREE

Updated

a year ago
Duplicate of this bug: 1442926
(Assignee)

Comment 22

a year ago
Turns out I wasn't dealing properly with "dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content".

Here's a new version of the same patch with that issue fixed.

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab9a83dbba9
Attachment #8955359 - Attachment is obsolete: true
Attachment #8956351 - Flags: review?(masayuki)
Comment on attachment 8956351 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

Approval Request Comment
[Feature/Bug causing the regression]: In 58 we tried to hide the keyboard layout in Resist Fingerprinting mode. In the process we broke common shortcuts (Copy/Paste) on a slew of websites for users without English keyboards (Gmail, Github, etc). We've had a stream of duplicate issues logged this entire release cycle.

[User impact if declined]: Users with Resist Fingerprinting turned on will need to suffer through another release cycle where Resist Fingerprinting breaks shortcuts all over the web. =(

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It affects a non-default preference.

[String changes made/needed]: None
Attachment #8956351 - Flags: approval-mozilla-beta?
To be clear, I am requesting this go into 59. If it has to wait for a dot release and be a ride-along because we're so late in the cycle, that's okay.
Comment on attachment 8956351 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

moving the uplift request to -release since we're at RC week
Attachment #8956351 - Flags: approval-mozilla-beta? → approval-mozilla-release?

Comment 26

a year ago
Hey Tom Ritter, this bug is not only on non-English keyboards. At least the Gmail one occurs when using an English keyboard as well. Turning this preference off fixes it.
(Assignee)

Comment 27

a year ago
(In reply to Gil Cottle from comment #26)
> Hey Tom Ritter, this bug is not only on non-English keyboards. At least the
> Gmail one occurs when using an English keyboard as well. Turning this
> preference off fixes it.

Yes, this patch should fix the problem on US English keyboards as well.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 28

a year ago
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ed89da3c62
Don't spoof ctrl key modifier for privacy.resistFingerprinting=true r=masayuki
Keywords: checkin-needed

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10ed89da3c62
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8956351 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

Only affects users with a non-standard pref set, so no risk to regular users. Approved for 59rc2.
Attachment #8956351 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 31

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/d52d963d1167
status-firefox59: fix-optional → fixed
Flags: in-testsuite+
Backed out for browser_spoofing_keyboard_event.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=166578993&repo=mozilla-release

https://hg.mozilla.org/releases/mozilla-release/rev/557058dc5cbf
status-firefox59: fixed → fix-optional
Attachment #8956351 - Flags: approval-mozilla-release+ → approval-mozilla-release?
(Assignee)

Comment 33

a year ago
Here's the same patch with a minor revision to the unit test so that it is compatible with Firefox 59.
Attachment #8957060 - Flags: approval-mozilla-release?
(Assignee)

Updated

a year ago
Attachment #8956351 - Flags: approval-mozilla-release?

Updated

a year ago
Duplicate of this bug: 1446986
I think we could take this as a ridealong for 59.0.2. We don't have exact timing for that, yet.
Comment on attachment 8957060 [details] [diff] [review]
0001-Bug-1433592-Don-t-spoof-ctrl-key-modifier-for-privac.patch

Fix so that keyboard shortcuts still work with resist fingerprinting, looks like it should not affect the default configuration. Let's uplift to m-r.
Attachment #8957060 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 37

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/d53e4bbdb169
status-firefox59: fix-optional → fixed
Flags: qe-verify+
Added to 59.0.2 release notes
relnote-firefox: --- → 59+

Comment 39

a year ago
Build ID: 20180323154952
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Verified as fixed on Firefox 59.0.2 and Firefox Beta 60.0b6 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

However I couldn't reproduce this issue on a build from when this issue was logged, I used a German keyboard(also tried with Firefox German version) and privacy.resistFingerprinting was enabled.

[:philipp] could you please try to see if this issue is fixed on your side also on Firefox 59.0.2?

Thanks.

Comment 40

a year ago
(In reply to Hani Yacoub from comment #39)
> However I couldn't reproduce this issue on a build from when this issue was
> logged, I used a German keyboard(also tried with Firefox German version) and
> privacy.resistFingerprinting was enabled.

It doesn't have to be a german keyboard to replicate there is an issue. I am using en-US and a standard English keyboard.

Copy some text to clipboard (from anywhere), and then go to any txt based github file and edit it. Try ctrl-V to paste that text in - the ctrl key does not fire and all you end up doing is typing the letter v. This is on FF59.0.1. Hope this helps

Comment 41

a year ago
Got it now. Thank you for your help.
I reproduced the issue on Firefox 59.0.1.

Verified as fixed on Firefox 59.0.2 and Firefox Beta 60.0b6 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: fixed → verified

Updated

a year ago
Flags: qe-verify+
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.