From ee25637a13420c7e40d54e978ead60e148aaf392 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Wed, 10 Feb 2021 23:49:21 +0000 Subject: [PATCH] Bug 1678255 - prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r=pbz,nika This passes around the "are we external" bit of load information a bunch, such that the external protocol handling code has access to it. In this bug and bug 1667468, I think ideally I would have used a check if we're the OS default for a given protocol before continuing. However, this information is currently unavailable on Linux (bug 1599713), and worse, I believe is likely to remain unavailable in flatpak and other such restricted environments (cf. bug 1618094 - we aren't able to find out anything about protocol handlers from the OS). So instead, we prompt the user if we are about to open a link passed to us externally. There is a small chance this will be Breaking People's Workflows, where I don't know whether anyone relies on Firefox happily passing these URIs along to the relevant application (more convenient than doing all the registry/API work yourself in scripts!) or anything like that. To help with that, there's a pref, `network.protocol-handler.prompt-from-external`, that can be created and set to false to avoid prompting in this case. Differential Revision: https://phabricator.services.mozilla.com/D103967 --- docshell/base/nsDocShell.cpp | 3 +- dom/ipc/ContentParent.cpp | 6 +- dom/ipc/ContentParent.h | 3 +- dom/ipc/PContent.ipdl | 5 +- .../handling/ContentDispatchChooser.jsm | 24 +++- .../exthandler/nsExternalHelperAppService.cpp | 7 +- .../exthandler/nsExternalHelperAppService.h | 3 +- .../exthandler/nsExternalProtocolHandler.cpp | 3 +- .../exthandler/nsIContentDispatchChooser.idl | 5 +- .../exthandler/nsIExternalProtocolService.idl | 6 +- .../exthandler/tests/mochitest/browser.ini | 1 + .../browser_protocol_ask_dialog_external.js | 106 ++++++++++++++++++ uriloader/exthandler/tests/mochitest/head.js | 21 +++- 13 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 3645c2621c25..4c3798095e0e 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -12766,7 +12766,8 @@ nsresult nsDocShell::OnLinkClickSync(nsIContent* aContent, extProtService->IsExposedProtocol(scheme.get(), &isExposed); if (NS_SUCCEEDED(rv) && !isExposed) { return extProtService->LoadURI(aLoadState->URI(), triggeringPrincipal, - mBrowsingContext); + mBrowsingContext, + /* aTriggeredExternally */ false); } } } diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 3e47037f09e8..f0af44f87520 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -4473,7 +4473,8 @@ mozilla::ipc::IPCResult ContentParent::RecvAccumulateMixedContentHSTS( mozilla::ipc::IPCResult ContentParent::RecvLoadURIExternal( nsIURI* uri, nsIPrincipal* aTriggeringPrincipal, - const MaybeDiscarded& aContext) { + const MaybeDiscarded& aContext, + bool aWasExternallyTriggered) { if (aContext.IsDiscarded()) { return IPC_OK(); } @@ -4489,7 +4490,8 @@ mozilla::ipc::IPCResult ContentParent::RecvLoadURIExternal( } BrowsingContext* bc = aContext.get(); - extProtService->LoadURI(uri, aTriggeringPrincipal, bc); + extProtService->LoadURI(uri, aTriggeringPrincipal, bc, + aWasExternallyTriggered); return IPC_OK(); } diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index bd2d9f5cb599..270d4b6e0c7e 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -1087,7 +1087,8 @@ class ContentParent final mozilla::ipc::IPCResult RecvLoadURIExternal( nsIURI* uri, nsIPrincipal* triggeringPrincipal, - const MaybeDiscarded& aContext); + const MaybeDiscarded& aContext, + bool aWasExternallyTriggered); mozilla::ipc::IPCResult RecvExtProtocolChannelConnectParent( const uint64_t& registrarId); diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl index 7682f85e1b18..ac87d042041b 100644 --- a/dom/ipc/PContent.ipdl +++ b/dom/ipc/PContent.ipdl @@ -1047,7 +1047,10 @@ parent: async StartVisitedQueries(nsIURI[] uri); async SetURITitle(nsIURI uri, nsString title); - async LoadURIExternal(nsIURI uri, nsIPrincipal triggeringPrincipal, MaybeDiscardedBrowsingContext browsingContext); + async LoadURIExternal(nsIURI uri, + nsIPrincipal triggeringPrincipal, + MaybeDiscardedBrowsingContext browsingContext, + bool wasExternallyTriggered); async ExtProtocolChannelConnectParent(uint64_t registrarId); // PrefService message diff --git a/toolkit/mozapps/handling/ContentDispatchChooser.jsm b/toolkit/mozapps/handling/ContentDispatchChooser.jsm index 520189bbd5ad..aa9ea87e1037 100644 --- a/toolkit/mozapps/handling/ContentDispatchChooser.jsm +++ b/toolkit/mozapps/handling/ContentDispatchChooser.jsm @@ -19,6 +19,14 @@ var EXPORTED_SYMBOLS = [ "ContentDispatchChooserTelemetry", ]; +const gPrefs = {}; +XPCOMUtils.defineLazyPreferenceGetter( + gPrefs, + "promptForExternal", + "network.protocol-handler.prompt-from-external", + true +); + const PROTOCOL_HANDLER_OPEN_PERM_KEY = "open-protocol-handler"; const PERMISSION_KEY_DELIMITER = "^"; @@ -238,13 +246,27 @@ class nsContentDispatchChooser { * @param {nsIURI} aURI - URI to be handled. * @param {nsIPrincipal} [aPrincipal] - Principal which triggered the load. * @param {BrowsingContext} [aBrowsingContext] - Context of the load. + * @param {bool} [aTriggeredExternally] - Whether the load came from outside + * this application. */ - async handleURI(aHandler, aURI, aPrincipal, aBrowsingContext) { + async handleURI( + aHandler, + aURI, + aPrincipal, + aBrowsingContext, + aTriggeredExternally = false + ) { let callerHasPermission = this._hasProtocolHandlerPermission( aHandler.type, aPrincipal ); + // Force showing the dialog for links passed from outside the application. + // This avoids infinite loops, see bug 1678255, bug 1667468, etc. + if (aTriggeredExternally && gPrefs.promptForExternal) { + aHandler.alwaysAskBeforeHandling = true; + } + // Skip the dialog if a preferred application is set and the caller has // permission. if ( diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp index 616fff2bd181..94cc4f67de64 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.cpp +++ b/uriloader/exthandler/nsExternalHelperAppService.cpp @@ -1010,12 +1010,13 @@ static const char kExternalProtocolDefaultPref[] = NS_IMETHODIMP nsExternalHelperAppService::LoadURI(nsIURI* aURI, nsIPrincipal* aTriggeringPrincipal, - BrowsingContext* aBrowsingContext) { + BrowsingContext* aBrowsingContext, + bool aTriggeredExternally) { NS_ENSURE_ARG_POINTER(aURI); if (XRE_IsContentProcess()) { mozilla::dom::ContentChild::GetSingleton()->SendLoadURIExternal( - aURI, aTriggeringPrincipal, aBrowsingContext); + aURI, aTriggeringPrincipal, aBrowsingContext, aTriggeredExternally); return NS_OK; } @@ -1117,7 +1118,7 @@ nsExternalHelperAppService::LoadURI(nsIURI* aURI, NS_ENSURE_SUCCESS(rv, rv); return chooser->HandleURI(handler, uri, aTriggeringPrincipal, - aBrowsingContext); + aBrowsingContext, aTriggeredExternally); } ////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/uriloader/exthandler/nsExternalHelperAppService.h b/uriloader/exthandler/nsExternalHelperAppService.h index 8c87587e9ee7..5735e73bcde7 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.h +++ b/uriloader/exthandler/nsExternalHelperAppService.h @@ -82,7 +82,8 @@ class nsExternalHelperAppService : public nsIExternalHelperAppService, NS_IMETHOD GetProtocolHandlerInfo(const nsACString& aScheme, nsIHandlerInfo** aHandlerInfo) override; NS_IMETHOD LoadURI(nsIURI* aURI, nsIPrincipal* aTriggeringPrincipal, - mozilla::dom::BrowsingContext* aBrowsingContext) override; + mozilla::dom::BrowsingContext* aBrowsingContext, + bool aWasTriggeredExternally) override; NS_IMETHOD SetProtocolHandlerDefaults(nsIHandlerInfo* aHandlerInfo, bool aOSHandlerExists) override; diff --git a/uriloader/exthandler/nsExternalProtocolHandler.cpp b/uriloader/exthandler/nsExternalProtocolHandler.cpp index 30b806d7c2fd..dbb1b2d0a43e 100644 --- a/uriloader/exthandler/nsExternalProtocolHandler.cpp +++ b/uriloader/exthandler/nsExternalProtocolHandler.cpp @@ -165,7 +165,8 @@ nsresult nsExtProtocolChannel::OpenURL() { } RefPtr principal = mLoadInfo->TriggeringPrincipal(); - rv = extProtService->LoadURI(mUrl, principal, ctx); + rv = extProtService->LoadURI(mUrl, principal, ctx, + mLoadInfo->GetLoadTriggeredFromExternal()); if (NS_SUCCEEDED(rv) && mListener) { mStatus = NS_ERROR_NO_CONTENT; diff --git a/uriloader/exthandler/nsIContentDispatchChooser.idl b/uriloader/exthandler/nsIContentDispatchChooser.idl index d12c1aa90701..be4e18b89013 100644 --- a/uriloader/exthandler/nsIContentDispatchChooser.idl +++ b/uriloader/exthandler/nsIContentDispatchChooser.idl @@ -29,10 +29,13 @@ interface nsIContentDispatchChooser : nsISupports { * The principal making the request. * @param aBrowsingContext * The browsing context where the load should happen. + * @param aWasTriggeredExternally + * True if the load was tripped by an external app. */ void handleURI(in nsIHandlerInfo aHandler, in nsIURI aURI, in nsIPrincipal aTriggeringPrincipal, - in BrowsingContext aBrowsingContext); + in BrowsingContext aBrowsingContext, + [optional] in bool aWasTriggeredExternally); }; diff --git a/uriloader/exthandler/nsIExternalProtocolService.idl b/uriloader/exthandler/nsIExternalProtocolService.idl index 66d459c4913e..bfc6cec006de 100644 --- a/uriloader/exthandler/nsIExternalProtocolService.idl +++ b/uriloader/exthandler/nsIExternalProtocolService.idl @@ -109,13 +109,17 @@ interface nsIExternalProtocolService : nsISupports * handlers will fail. We need to do better than that; bug 394483 * filed in order to track. * + * @param aWasTriggeredExternally + * If true, indicates the load was initiated by an external app. + * * @note Embedders that do not expose the http protocol should not currently * use web-based protocol handlers, as handoff won't work correctly * (bug 394479). */ void loadURI(in nsIURI aURI, [optional] in nsIPrincipal aTriggeringPrincipal, - [optional] in BrowsingContext aBrowsingContext); + [optional] in BrowsingContext aBrowsingContext, + [optional] in bool aWasTriggeredExternally); /** * Gets a human-readable description for the application responsible for diff --git a/uriloader/exthandler/tests/mochitest/browser.ini b/uriloader/exthandler/tests/mochitest/browser.ini index 0d5b20bef8b2..4ce81c67ce01 100644 --- a/uriloader/exthandler/tests/mochitest/browser.ini +++ b/uriloader/exthandler/tests/mochitest/browser.ini @@ -45,6 +45,7 @@ support-files = [browser_first_prompt_not_blocked_without_user_interaction.js] support-files = file_external_protocol_iframe.html +[browser_protocol_ask_dialog_external.js] [browser_protocol_ask_dialog_permission.js] [browser_protocolhandler_loop.js] [browser_remember_download_option.js] diff --git a/uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js b/uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js new file mode 100644 index 000000000000..03c396107651 --- /dev/null +++ b/uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js @@ -0,0 +1,106 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +ChromeUtils.import( + "resource://testing-common/HandlerServiceTestUtils.jsm", + this +); + +let gHandlerService = Cc["@mozilla.org/uriloader/handler-service;1"].getService( + Ci.nsIHandlerService +); + +/** + * Creates dummy protocol handler + */ +function initTestHandlers() { + let handlerInfo = HandlerServiceTestUtils.getBlankHandlerInfo("yoink"); + + let appHandler = Cc[ + "@mozilla.org/uriloader/local-handler-app;1" + ].createInstance(Ci.nsILocalHandlerApp); + // This is a dir and not executable, but that's enough for here. + appHandler.executable = Services.dirsvc.get("XCurProcD", Ci.nsIFile); + handlerInfo.possibleApplicationHandlers.appendElement(appHandler); + handlerInfo.preferredApplicationHandler = appHandler; + handlerInfo.preferredAction = handlerInfo.useHelperApp; + handlerInfo.alwaysAskBeforeHandling = false; + gHandlerService.store(handlerInfo); + registerCleanupFunction(() => { + gHandlerService.remove(handlerInfo); + }); +} + +/** + * Check that if we get a direct request from another app / the OS to open a + * link, we always prompt, even if we think we know what the correct answer + * is. This is to avoid infinite loops in such situations where the OS and + * Firefox have conflicting ideas about the default handler, or where our + * checks with the OS don't work (Linux and/or Snap, at time of this comment). + */ +add_task(async function test_external_asks_anyway() { + await SpecialPowers.pushPrefEnv({ + set: [["network.protocol-handler.prompt-from-external", true]], + }); + initTestHandlers(); + + let cmdLineHandler = Cc["@mozilla.org/browser/final-clh;1"].getService( + Ci.nsICommandLineHandler + ); + let fakeCmdLine = { + length: 1, + _arg: "yoink:yoink", + + getArgument(aIndex) { + if (aIndex == 0) { + return this._arg; + } + throw Components.Exception("", Cr.NS_ERROR_INVALID_ARG); + }, + + findFlag() { + return -1; + }, + + handleFlagWithParam() { + if (this._argCount) { + this._argCount = 0; + return this._arg; + } + + return ""; + }, + + state: 2, + + STATE_INITIAL_LAUNCH: 0, + STATE_REMOTE_AUTO: 1, + STATE_REMOTE_EXPLICIT: 2, + + preventDefault: false, + + resolveURI() { + return Services.io.newURI(this._arg); + }, + QueryInterface: ChromeUtils.generateQI(["nsICommandLine"]), + }; + let chooserDialogOpenPromise = waitForProtocolAppChooserDialog( + gBrowser, + true + ); + cmdLineHandler.handle(fakeCmdLine); + let dialog = await chooserDialogOpenPromise; + ok(dialog, "Should have prompted."); + + let dialogClosedPromise = waitForProtocolAppChooserDialog( + gBrowser.selectedBrowser, + false + ); + let dialogEl = dialog._frame.contentDocument.querySelector("dialog"); + dialogEl.cancelDialog(); + await dialogClosedPromise; + // We will have opened a tab; close it. + BrowserTestUtils.removeTab(gBrowser.selectedTab); +}); diff --git a/uriloader/exthandler/tests/mochitest/head.js b/uriloader/exthandler/tests/mochitest/head.js index 3b0d8a4072f3..dc864179a999 100644 --- a/uriloader/exthandler/tests/mochitest/head.js +++ b/uriloader/exthandler/tests/mochitest/head.js @@ -125,11 +125,26 @@ async function openHelperAppDialog(launcher) { return dlg; } +/** + * Wait for a subdialog event indicating a dialog either opened + * or was closed. + * + * First argument is the browser in which to listen. If a tabbrowser, + * we listen to subdialogs for any tab of that browser. + */ async function waitForSubDialog(browser, url, state) { let eventStr = state ? "dialogopen" : "dialogclose"; - let tabDialogBox = gBrowser.getTabDialogBox(browser); - let dialogStack = tabDialogBox.getTabDialogManager()._dialogStack; + let eventTarget; + + // Tabbrowser? + if (browser.tabContainer) { + eventTarget = browser.tabContainer.ownerDocument.documentElement; + } else { + // Individual browser. Get its box: + let tabDialogBox = browser.ownerGlobal.gBrowser.getTabDialogBox(browser); + eventTarget = tabDialogBox.getTabDialogManager()._dialogStack; + } let checkFn; @@ -138,7 +153,7 @@ async function waitForSubDialog(browser, url, state) { } let event = await BrowserTestUtils.waitForEvent( - dialogStack, + eventTarget, eventStr, true, checkFn