diff --git a/browser/actors/ClickHandlerChild.jsm b/browser/actors/ClickHandlerChild.jsm index 22f11081b9bd..0f3bf42e2290 100644 --- a/browser/actors/ClickHandlerChild.jsm +++ b/browser/actors/ClickHandlerChild.jsm @@ -57,13 +57,6 @@ class ClickHandlerChild extends JSWindowActorChild { return; } - // For untrusted events, require a valid transient user gesture activation. - // consumeTransientUserGestureActivation returns false if there isn't one, - // and consumes it otherwise. - if (!event.isTrusted && !ownerDoc.consumeTransientUserGestureActivation()) { - return; - } - // Handle click events from about pages if (event.button == 0) { if (ownerDoc.documentURI.startsWith("about:blocked")) { @@ -71,6 +64,11 @@ class ClickHandlerChild extends JSWindowActorChild { } } + // For untrusted events, require a valid transient user gesture activation. + if (!event.isTrusted && !ownerDoc.hasValidTransientUserGestureActivation) { + return; + } + let [href, node, principal] = BrowserUtils.hrefAndLinkNodeForClickEvent( event ); @@ -119,6 +117,26 @@ class ClickHandlerChild extends JSWindowActorChild { return; } + if ( + !event.isTrusted && + BrowserUtils.whereToOpenLink(event) != "current" + ) { + // If we'll open the link, we want to consume the user gesture + // activation to ensure that we don't allow multiple links to open + // from one user gesture. + // Avoid doing so for links opened in the current tab, which get + // handled later, by gecko, as otherwise its popup blocker will stop + // the link from opening. + // We will do the same check (whereToOpenLink) again in the parent and + // avoid handling the click for such links... but we still need the + // click information in the parent because otherwise places link + // tracking breaks. (bug 1742894 tracks improving this.) + ownerDoc.consumeTransientUserGestureActivation(); + // We don't care about the return value because we already checked that + // hasValidTransientUserGestureActivation was true earlier in this + // function. + } + json.href = href; if (node) { json.title = node.getAttribute("title"); diff --git a/browser/actors/test/browser/browser_untrusted_click_event.js b/browser/actors/test/browser/browser_untrusted_click_event.js index afb162520cc8..7bb579223da0 100644 --- a/browser/actors/test/browser/browser_untrusted_click_event.js +++ b/browser/actors/test/browser/browser_untrusted_click_event.js @@ -23,9 +23,85 @@ add_task(async function test_untrusted_click_opens_tab() { let eventObj = {}; eventObj[AppConstants.platform == "macosx" ? "metaKey" : "ctrlKey"] = true; await BrowserTestUtils.synthesizeMouseAtCenter("#test", eventObj, browser); - info("Waiting for new tab to open; if we timeout the test is broken."); + info( + "Waiting for new tab to open from frontend; if we timeout the test is broken." + ); let newTab = await newTabOpened; ok(newTab, "New tab should be opened."); BrowserTestUtils.removeTab(newTab); }); }); + +/** + * Ensure that click handlers that redirect a modifier-less click into + * an untrusted event without modifiers don't run afoul of the popup + * blocker by having their transient user gesture bit consumed in the + * child by the handling code for modified clicks. + */ +add_task(async function test_unused_click_doesnt_consume_activation() { + // Enable the popup blocker. + await SpecialPowers.pushPrefEnv({ + set: [ + ["dom.disable_open_during_load", true], + ["dom.block_multiple_popups", true], + ], + }); + await BrowserTestUtils.withNewTab(TEST_PATH + "click.html", async browser => { + let newTabOpened = BrowserTestUtils.waitForNewTab( + gBrowser, + "https://example.com/", + true + ); + info("clicking button that generates link press."); + await BrowserTestUtils.synthesizeMouseAtCenter( + "#fire-untrusted", + {}, + browser + ); + info( + "Waiting for new tab to open through gecko; if we timeout the test is broken." + ); + let newTab = await newTabOpened; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); + }); +}); + +/** + * Ensure that click handlers redirecting to elements without href don't + * trigger user gesture consumption either. + */ +add_task(async function test_click_without_href_doesnt_consume_activation() { + // Enable the popup blocker. + await SpecialPowers.pushPrefEnv({ + set: [ + ["dom.disable_open_during_load", true], + ["dom.block_multiple_popups", true], + ], + }); + await BrowserTestUtils.withNewTab(TEST_PATH + "click.html", async browser => { + let newTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, "about:blank"); + info("clicking link that generates link click on target-less link."); + await BrowserTestUtils.synthesizeMouseAtCenter( + "#fire-targetless-link", + {}, + browser + ); + info( + "Waiting for new tab to open after targetless link; if we timeout the test is broken." + ); + let newTab = await newTabOpened; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); + + newTabOpened = BrowserTestUtils.waitForNewTab(gBrowser, "about:blank"); + info("clicking link that generates button click."); + await BrowserTestUtils.synthesizeMouseAtCenter("#fire-button", {}, browser); + info( + "Waiting for new tab to open after button redirect; if we timeout the test is broken." + ); + newTab = await newTabOpened; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); + }); +}); diff --git a/browser/actors/test/browser/click.html b/browser/actors/test/browser/click.html index 366e9c924a35..42afdf8050a4 100644 --- a/browser/actors/test/browser/click.html +++ b/browser/actors/test/browser/click.html @@ -6,7 +6,13 @@ -click me +click me

+ +This is a link, works only with untrusted events
+

+ +Click me (dispatch untrusted event to link without href)
+Click me (dispatch untrusted event to button) diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 85a8398873b9..0f686ff1e5a1 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -24,7 +24,6 @@ XPCOMUtils.defineLazyModuleGetters(this, { BrowserUsageTelemetry: "resource:///modules/BrowserUsageTelemetry.jsm", BrowserTelemetryUtils: "resource://gre/modules/BrowserTelemetryUtils.jsm", BrowserUIUtils: "resource:///modules/BrowserUIUtils.jsm", - BrowserUtils: "resource://gre/modules/BrowserUtils.jsm", BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm", CFRPageActions: "resource://activity-stream/lib/CFRPageActions.jsm", Color: "resource://gre/modules/Color.jsm", diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js index d827de09f231..5d999d12969f 100644 --- a/browser/base/content/utilityOverlay.js +++ b/browser/base/content/utilityOverlay.js @@ -14,6 +14,7 @@ var { XPCOMUtils } = ChromeUtils.import( XPCOMUtils.defineLazyModuleGetters(this, { AboutNewTab: "resource:///modules/AboutNewTab.jsm", + BrowserUtils: "resource://gre/modules/BrowserUtils.jsm", BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm", ContextualIdentityService: "resource://gre/modules/ContextualIdentityService.jsm", @@ -161,93 +162,14 @@ function openUILink( openUILinkIn(url, where, params); } -// Utility function to check command events for potential middle-click events -// from checkForMiddleClick and unwrap them. +// This is here for historical reasons. bug 1742889 covers cleaning this up. function getRootEvent(aEvent) { - // Part of the fix for Bug 1523813. - // Middle-click events arrive here wrapped in different numbers (1-2) of - // command events, depending on the button originally clicked. - if (!aEvent) { - return aEvent; - } - let tempEvent = aEvent; - while (tempEvent.sourceEvent) { - if (tempEvent.sourceEvent.button == 1) { - aEvent = tempEvent.sourceEvent; - break; - } - tempEvent = tempEvent.sourceEvent; - } - return aEvent; + return BrowserUtils.getRootEvent(aEvent); } -/** - * whereToOpenLink() looks at an event to decide where to open a link. - * - * The event may be a mouse event (click, double-click, middle-click) or keypress event (enter). - * - * On Windows, the modifiers are: - * Ctrl new tab, selected - * Shift new window - * Ctrl+Shift new tab, in background - * Alt save - * - * Middle-clicking is the same as Ctrl+clicking (it opens a new tab). - * - * Exceptions: - * - Alt is ignored for menu items selected using the keyboard so you don't accidentally save stuff. - * (Currently, the Alt isn't sent here at all for menu items, but that will change in bug 126189.) - * - Alt is hard to use in context menus, because pressing Alt closes the menu. - * - Alt can't be used on the bookmarks toolbar because Alt is used for "treat this as something draggable". - * - The button is ignored for the middle-click-paste-URL feature, since it's always a middle-click. - * - * @param e {Event|Object} Event or JSON Object - * @param ignoreButton {Boolean} - * @param ignoreAlt {Boolean} - * @returns {"current" | "tabshifted" | "tab" | "save" | "window"} - */ +// This is here for historical reasons. bug 1742889 covers cleaning this up. function whereToOpenLink(e, ignoreButton, ignoreAlt) { - // This method must treat a null event like a left click without modifier keys (i.e. - // e = { shiftKey:false, ctrlKey:false, metaKey:false, altKey:false, button:0 }) - // for compatibility purposes. - if (!e) { - return "current"; - } - - e = getRootEvent(e); - - var shift = e.shiftKey; - var ctrl = e.ctrlKey; - var meta = e.metaKey; - var alt = e.altKey && !ignoreAlt; - - // ignoreButton allows "middle-click paste" to use function without always opening in a new window. - let middle = !ignoreButton && e.button == 1; - let middleUsesTabs = Services.prefs.getBoolPref( - "browser.tabs.opentabfor.middleclick", - true - ); - let middleUsesNewWindow = Services.prefs.getBoolPref( - "middlemouse.openNewWindow", - false - ); - - // Don't do anything special with right-mouse clicks. They're probably clicks on context menu items. - - var metaKey = AppConstants.platform == "macosx" ? meta : ctrl; - if (metaKey || (middle && middleUsesTabs)) { - return shift ? "tabshifted" : "tab"; - } - - if (alt && Services.prefs.getBoolPref("browser.altClickSave", false)) { - return "save"; - } - - if (shift || (middle && !middleUsesTabs && middleUsesNewWindow)) { - return "window"; - } - - return "current"; + return BrowserUtils.whereToOpenLink(e, ignoreButton, ignoreAlt); } /* openTrustedLinkIn will attempt to open the given URI using the SystemPrincipal diff --git a/toolkit/modules/BrowserUtils.jsm b/toolkit/modules/BrowserUtils.jsm index ae23c213d47d..3de35d228149 100644 --- a/toolkit/modules/BrowserUtils.jsm +++ b/toolkit/modules/BrowserUtils.jsm @@ -7,6 +7,9 @@ var EXPORTED_SYMBOLS = ["BrowserUtils"]; +const { AppConstants } = ChromeUtils.import( + "resource://gre/modules/AppConstants.jsm" +); const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); const { XPCOMUtils } = ChromeUtils.import( "resource://gre/modules/XPCOMUtils.jsm" @@ -205,6 +208,95 @@ var BrowserUtils = { node && node.ownerDocument.nodePrincipal, ]; }, + + /** + * whereToOpenLink() looks at an event to decide where to open a link. + * + * The event may be a mouse event (click, double-click, middle-click) or keypress event (enter). + * + * On Windows, the modifiers are: + * Ctrl new tab, selected + * Shift new window + * Ctrl+Shift new tab, in background + * Alt save + * + * Middle-clicking is the same as Ctrl+clicking (it opens a new tab). + * + * Exceptions: + * - Alt is ignored for menu items selected using the keyboard so you don't accidentally save stuff. + * (Currently, the Alt isn't sent here at all for menu items, but that will change in bug 126189.) + * - Alt is hard to use in context menus, because pressing Alt closes the menu. + * - Alt can't be used on the bookmarks toolbar because Alt is used for "treat this as something draggable". + * - The button is ignored for the middle-click-paste-URL feature, since it's always a middle-click. + * + * @param e {Event|Object} Event or JSON Object + * @param ignoreButton {Boolean} + * @param ignoreAlt {Boolean} + * @returns {"current" | "tabshifted" | "tab" | "save" | "window"} + */ + whereToOpenLink(e, ignoreButton, ignoreAlt) { + // This method must treat a null event like a left click without modifier keys (i.e. + // e = { shiftKey:false, ctrlKey:false, metaKey:false, altKey:false, button:0 }) + // for compatibility purposes. + if (!e) { + return "current"; + } + + e = this.getRootEvent(e); + + var shift = e.shiftKey; + var ctrl = e.ctrlKey; + var meta = e.metaKey; + var alt = e.altKey && !ignoreAlt; + + // ignoreButton allows "middle-click paste" to use function without always opening in a new window. + let middle = !ignoreButton && e.button == 1; + let middleUsesTabs = Services.prefs.getBoolPref( + "browser.tabs.opentabfor.middleclick", + true + ); + let middleUsesNewWindow = Services.prefs.getBoolPref( + "middlemouse.openNewWindow", + false + ); + + // Don't do anything special with right-mouse clicks. They're probably clicks on context menu items. + + var metaKey = AppConstants.platform == "macosx" ? meta : ctrl; + if (metaKey || (middle && middleUsesTabs)) { + return shift ? "tabshifted" : "tab"; + } + + if (alt && Services.prefs.getBoolPref("browser.altClickSave", false)) { + return "save"; + } + + if (shift || (middle && !middleUsesTabs && middleUsesNewWindow)) { + return "window"; + } + + return "current"; + }, + + // Utility function to check command events for potential middle-click events + // from checkForMiddleClick and unwrap them. + getRootEvent(aEvent) { + // Part of the fix for Bug 1523813. + // Middle-click events arrive here wrapped in different numbers (1-2) of + // command events, depending on the button originally clicked. + if (!aEvent) { + return aEvent; + } + let tempEvent = aEvent; + while (tempEvent.sourceEvent) { + if (tempEvent.sourceEvent.button == 1) { + aEvent = tempEvent.sourceEvent; + break; + } + tempEvent = tempEvent.sourceEvent; + } + return aEvent; + }, }; XPCOMUtils.defineLazyPreferenceGetter(