1521396 - Paste event triggered on middle-clicking a link

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Johannes Pfrang [:johnp]]

Since bug 1461708 middle-clicking a link triggers a paste-event which allows websites to extract user clipboard content even when they only want to open a link in a new tab.

Kudos to redditor lihaarp for noticing it:

https://old.reddit.com/r/firefox/comments/ahwtpq/firefox_allows_sites_to_hijack_middle_clicks_and/

Comment 1

[Alice0775 White]

Attachment: sample html

Comment 2

[Ryan VanderMeulen [:RyanVM]]

We're about to build the Fx65 RC build, so it's probably too late to get a fix in there. We should definitely see what we can do for 66, though.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Interesting. Chrome never fires paste event if clicking on an anchor element which has href attribute (even if it does not work as a link due to editable).

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d24432878e975a7ddc61a685ad2d4b1ca9db68e0

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, I don't understand for some elements on Chrome. Some elements, for example, <button> sometimes fires paste event, but not always... Perhaps, related to previously focused element... So, perhaps, we should take care only <a href="..."> here.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1521396 - Make ClickHandlerChild prevent multiple action of middle click on link element for preventing middle click paste

When user middle clicks a link, most users must not expect to expose clipboard
content to the web application. Therefore, we should stop firing paste event
when user click a link with middle button.

This patch makes ClickHandlerChild.handleEvent() prevent multiple action
when it posts middle click event on a link. Note that even if middle click
event is consumed, default event handler will dispatch paste event.
Unfortunately, this is compatible behavior with the other browsers.
Therefore, we cannot change this behavior with calling preventDefault() and
this is the reason why this patch adds Event.preventMultipleActions().

Out of scope of this bug though, if there is an element which looks like a
link but implemented with JS, web apps can steal clipboard content if user
enables middle click event and user just wants to open the link in new tab.
It might be better to stop dispatching paste event in any browsers and request
to change each web apps.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81c98263341c
Make ClickHandlerChild prevent multiple action of middle click on link element for preventing middle click paste r=smaug

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

RyanVM:

What do you think we should take it in 65.0.0 or wait 65.0.1? Anyway, should I request uplift?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Looks simple enough. Please do the release approval request. I'm curious about what sort of unintended side effects this patch might have?

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/81c98263341c

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Looks simple enough. Please do the release approval request.

Thanks, I'll do it soon.

I'm curious about what sort of unintended side effects this patch might have?

Event.multipleActionsPrevented is not referred by any chrome scripts.
https://searchfox.org/mozilla-central/search?q=.multipleActionsPrevented&case=true&regexp=false&path=

One of C++ referrer is Element::CheckHandleEventForLinksPrecondition(). It's called when creating event target chain and post handling of event on a link element.
https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/dom/base/Element.cpp#2979,2987

The former case is fine because it's already done when an event listener receives an event.

One of the latter is Element::PostHandleEventForLinks().
https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/dom/base/Element.cpp#3052,3102,3104,3125-3126
but it handles only when click button is left button.

The other of the latter is HTMLLabelElement::PostHandleEvent().
https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/dom/html/HTMLLabelElement.cpp#76,84,111-112,175-176
but this also handles click event only when the button is left button.

The other referrer is PresShell::DispatchTouchEventToDOM(). But it won't handle click event.

So, the middle click event's mMultipleActionsPrevented must be referred only by EventStateManager::HandleMiddleClickPaste() which is what we need to change the behavior.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9038155 [details]
Bug 1521396 - Make ClickHandlerChild prevent multiple action of middle click on link element for preventing middle click paste

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1461708

User impact if declined: If user enables middle click paste (enabled by default only on Linux, but can be enabled on the other desktop platforms), middle clicking on a link causes leaking the clipboard content.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This marks middle click event on a link from chrome script as "multiple actions prevented" with new API.

Then, the flag of middle click event is probably referred by EventStateManager::HandleMiddleClickPaste(). See comment 11 for the detail.

String changes made/needed: none

Comment 13

[Kestrel]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #6)

Out of scope of this bug though, if there is an element which looks like a
link but implemented with JS, web apps can steal clipboard content if user
enables middle click event and user just wants to open the link in new tab.
It might be better to stop dispatching paste event in any browsers and request
to change each web apps.

This is Bug 405620.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is Bug 405620.

Thank you!

Comment 15

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/81c98263341cb237c3569dae658ef03e68bf4bad
Bug 1521396 - Make ClickHandlerChild prevent multiple action of middle click on link element for preventing middle click paste r=smaug

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9038155 [details]
Bug 1521396 - Make ClickHandlerChild prevent multiple action of middle click on link element for preventing middle click paste

[Triage Comment]
Thanks for the thorough analysis and automated test. Privacy fix which also gets us in line with what other browsers do for middle click. Approved for 65.0 RC2.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d6e9b01e0ae8