Closed Bug 1433592 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

59 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
relnote-firefox --- 59+
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- verified
firefox60 + verified

People

(Reporter: philipp, Assigned: arthur)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fingerprinting-breakage][tor 17009])

Attachments

(2 files, 2 obsolete files)

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)
See Also: → 1436309
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).
Also reported in Bug 1432859.
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.
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)
Priority: -- → P2
Whiteboard: [fingerprinting-breakage] → [fingerprinting-breakage][tor 17009]
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.
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…).
(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+
Keywords: checkin-needed
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
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
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)
Attachment #8956351 - Flags: review?(masayuki) → review+
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?
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.
(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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/10ed89da3c62
Status: NEW → RESOLVED
Closed: 6 years ago
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+
Attachment #8956351 - Flags: approval-mozilla-release+ → approval-mozilla-release?
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?
Attachment #8956351 - Flags: approval-mozilla-release?
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+
Flags: qe-verify+
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.
(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
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
Flags: qe-verify+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.