Closed Bug 1521396 Opened 1 year ago Closed 1 year ago

Paste event triggered on middle-clicking a link

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- wontfix
firefox65 - fixed
firefox66 + fixed

People

(Reporter: johnp, Assigned: masayuki)

References

Details

(Keywords: privacy, regression)

Attachments

(2 files)

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/

Attached file sample html
Attachment #9037849 - Attachment mime type: text/plain → text/html
Version: 60 Branch → Trunk

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.

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).

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Priority: -- → P1

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.

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.

Pushed by masayuki@d-toybox.com:
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

RyanVM:

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

Flags: needinfo?(ryanvm)

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

Flags: needinfo?(ryanvm) → needinfo?(masayuki)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

(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.

Flags: needinfo?(masayuki)

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

Attachment #9038155 - Flags: approval-mozilla-release?

(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.

See Also: → 405620

This is Bug 405620.

Thank you!

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 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.

Attachment #9038155 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.