From a8eecb05740c5185a3d6a9fc7f05e786db2f04f8 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Wed, 26 Jan 2022 14:54:34 +0000 Subject: [PATCH] Bug 1750253 - stop infinite file handling loops by checking if we're about to pass files to ourselves via the OS, r=mhowell Differential Revision: https://phabricator.services.mozilla.com/D136693 --- .../exthandler/nsExternalHelperAppService.cpp | 29 +++++- uriloader/exthandler/nsMIMEInfoImpl.cpp | 17 +++- uriloader/exthandler/nsMIMEInfoImpl.h | 2 + .../exthandler/tests/mochitest/browser.ini | 1 + .../mochitest/browser_filehandling_loop.js | 93 +++++++++++++++++++ ...owser_local_files_open_doesnt_duplicate.js | 21 +++-- .../mochitest/browser_protocolhandler_loop.js | 6 +- 7 files changed, 154 insertions(+), 15 deletions(-) create mode 100644 uriloader/exthandler/tests/mochitest/browser_filehandling_loop.js diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp index 208f219504fd..9386f8a720d5 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.cpp +++ b/uriloader/exthandler/nsExternalHelperAppService.cpp @@ -63,6 +63,7 @@ #include "nsDSURIContentListener.h" #include "nsMimeTypes.h" +#include "nsMIMEInfoImpl.h" // used for header disposition information. #include "nsIHttpChannel.h" #include "nsIHttpChannelInternal.h" @@ -76,7 +77,6 @@ # include "nsILocalFileMac.h" #endif -#include "nsPluginHost.h" #include "nsEscape.h" #include "nsIStringBundle.h" // XXX needed to localize error msgs @@ -1955,6 +1955,33 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) { !shouldAutomaticallyHandleInternally; } + // If we're handling with the OS default and we are that default, force + // asking, so we don't end up in an infinite loop: + if (!alwaysAsk && action == nsIMIMEInfo::useSystemDefault) { + bool areOSDefault = false; + alwaysAsk = NS_SUCCEEDED(mMimeInfo->IsCurrentAppOSDefault(&areOSDefault)) && + areOSDefault; + } else if (!alwaysAsk && action == nsIMIMEInfo::useHelperApp) { + nsCOMPtr preferredApp; + mMimeInfo->GetPreferredApplicationHandler(getter_AddRefs(preferredApp)); + nsCOMPtr handlerApp = do_QueryInterface(preferredApp); + if (handlerApp) { + nsCOMPtr executable; + handlerApp->GetExecutable(getter_AddRefs(executable)); + nsCOMPtr ourselves; + if (executable && + // Despite the name, this really just fetches an nsIFile... + NS_SUCCEEDED(NS_GetSpecialDirectory(XRE_EXECUTABLE_FILE, + getter_AddRefs(ourselves)))) { + ourselves = nsMIMEInfoBase::GetCanonicalExecutable(ourselves); + executable = nsMIMEInfoBase::GetCanonicalExecutable(executable); + bool isSameApp = false; + alwaysAsk = + NS_FAILED(executable->Equals(ourselves, &isSameApp)) || isSameApp; + } + } + } + // if we were told that we _must_ save to disk without asking, all the stuff // before this is irrelevant; override it if (mForceSave) { diff --git a/uriloader/exthandler/nsMIMEInfoImpl.cpp b/uriloader/exthandler/nsMIMEInfoImpl.cpp index 2c278043277a..f0b3b48244e0 100644 --- a/uriloader/exthandler/nsMIMEInfoImpl.cpp +++ b/uriloader/exthandler/nsMIMEInfoImpl.cpp @@ -21,9 +21,20 @@ static bool sInitializedOurData = false; StaticRefPtr sOurAppFile; -static already_AddRefed GetCanonicalExecutable(nsIFile* aFile) { +/* static */ +already_AddRefed nsMIMEInfoBase::GetCanonicalExecutable( + nsIFile* aFile) { nsCOMPtr binary = aFile; #ifdef XP_MACOSX + nsAutoString path; + if (binary) { + binary->GetPath(path); + } + if (!StringEndsWith(path, u".app"_ns) && path.RFind(u".app/"_ns) == -1) { + // This shouldn't ever happen with Firefox's own binary, tracked in + // sOurAppFile, but might happen when called with other files. + return binary.forget(); + } nsAutoString leafName; if (binary) { binary->GetLeafName(leafName); @@ -31,7 +42,7 @@ static already_AddRefed GetCanonicalExecutable(nsIFile* aFile) { while (binary && !StringEndsWith(leafName, u".app"_ns)) { nsCOMPtr parent; binary->GetParent(getter_AddRefs(parent)); - binary = parent; + binary = std::move(parent); if (binary) { binary->GetLeafName(leafName); } @@ -47,7 +58,7 @@ static void EnsureAppDetailsAvailable() { sInitializedOurData = true; nsCOMPtr binary; XRE_GetBinaryPath(getter_AddRefs(binary)); - sOurAppFile = GetCanonicalExecutable(binary); + sOurAppFile = nsMIMEInfoBase::GetCanonicalExecutable(binary); ClearOnShutdown(&sOurAppFile); } diff --git a/uriloader/exthandler/nsMIMEInfoImpl.h b/uriloader/exthandler/nsMIMEInfoImpl.h index f048c43b75dc..9e7c4a479915 100644 --- a/uriloader/exthandler/nsMIMEInfoImpl.h +++ b/uriloader/exthandler/nsMIMEInfoImpl.h @@ -95,6 +95,8 @@ class nsMIMEInfoBase : public nsIMIMEInfo { */ bool HasExtensions() const { return mExtensions.Length() != 0; } + static already_AddRefed GetCanonicalExecutable(nsIFile* aFile); + protected: virtual ~nsMIMEInfoBase(); // must be virtual, as the the base class's // Release should call the subclass's destructor diff --git a/uriloader/exthandler/tests/mochitest/browser.ini b/uriloader/exthandler/tests/mochitest/browser.ini index a823682e26a7..650360833d36 100644 --- a/uriloader/exthandler/tests/mochitest/browser.ini +++ b/uriloader/exthandler/tests/mochitest/browser.ini @@ -54,6 +54,7 @@ support-files = support-files = file_as.exe file_as.exe^headers^ +[browser_filehandling_loop.js] [browser_launched_app_save_directory.js] # This test checks the save destination of the # open with app download on Windows, Linux and OS X. diff --git a/uriloader/exthandler/tests/mochitest/browser_filehandling_loop.js b/uriloader/exthandler/tests/mochitest/browser_filehandling_loop.js new file mode 100644 index 000000000000..70eb0f68d8a0 --- /dev/null +++ b/uriloader/exthandler/tests/mochitest/browser_filehandling_loop.js @@ -0,0 +1,93 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +/** + * If the user has set Firefox itself as a helper app, + * we should force prompting what to do, rather than ending up + * in an infinite loop. + * In an ideal world, we'd also test the case where we are the OS + * default handler app, but that would require test infrastructure + * to make ourselves the OS default (or at least fool ourselves into + * believing we are) which we don't have... + */ +add_task(async function test_helperapp() { + // Set up the test infrastructure: + const mimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); + const handlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].getService( + Ci.nsIHandlerService + ); + let handlerInfo = mimeSvc.getFromTypeAndExtension("application/x-foo", "foo"); + registerCleanupFunction(() => { + handlerSvc.remove(handlerInfo); + }); + // Say we want to use a specific app: + handlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp; + handlerInfo.alwaysAskBeforeHandling = false; + + // Say it's us: + let selfFile = Services.dirsvc.get("XREExeF", Ci.nsIFile); + // Make sure it's the .app + if (AppConstants.platform == "macosx") { + while ( + !selfFile.leafName.endsWith(".app") && + !selfFile.leafName.endsWith(".app/") + ) { + selfFile = selfFile.parent; + } + } + let selfHandlerApp = Cc[ + "@mozilla.org/uriloader/local-handler-app;1" + ].createInstance(Ci.nsILocalHandlerApp); + selfHandlerApp.executable = selfFile; + handlerInfo.possibleApplicationHandlers.appendElement(selfHandlerApp); + handlerInfo.preferredApplicationHandler = selfHandlerApp; + handlerSvc.store(handlerInfo); + + await BrowserTestUtils.withNewTab("about:blank", async browser => { + // Now, do some safety stubbing. If we do end up recursing we spawn + // infinite tabs. We definitely don't want that. Avoid it by stubbing + // our external URL handling bits: + let oldAddTab = gBrowser.addTab; + registerCleanupFunction(() => (gBrowser.addTab = oldAddTab)); + let wrongThingHappenedPromise = new Promise(resolve => { + gBrowser.addTab = function(aURI) { + ok(false, "Tried to open unexpected URL in a tab: " + aURI); + resolve(null); + // Pass a dummy object to avoid upsetting BrowserContentHandler - + // if it thinks opening the tab failed, it tries to open a window instead, + // which we can't prevent as easily, and at which point we still end up + // with runaway tabs. + return {}; + }; + }); + + let askedUserPromise = BrowserTestUtils.domWindowOpenedAndLoaded(); + + info("Clicking a link that should open the unknown content type dialog"); + await SpecialPowers.spawn(browser, [], () => { + let link = content.document.createElement("a"); + link.download = "foo.foo"; + link.textContent = "Foo file"; + link.href = "data:application/x-foo,hello"; + content.document.body.append(link); + link.click(); + }); + let dialog = await Promise.race([ + wrongThingHappenedPromise, + askedUserPromise, + ]); + ok(dialog, "Should have gotten a dialog"); + Assert.stringContains( + dialog.document.location.href, + "unknownContentType", + "Should have opened correct dialog." + ); + + let closePromise = BrowserTestUtils.windowClosed(dialog); + dialog.close(); + await closePromise; + askedUserPromise = null; + }); +}); diff --git a/uriloader/exthandler/tests/mochitest/browser_local_files_open_doesnt_duplicate.js b/uriloader/exthandler/tests/mochitest/browser_local_files_open_doesnt_duplicate.js index d0db9d743f27..0b2493430013 100644 --- a/uriloader/exthandler/tests/mochitest/browser_local_files_open_doesnt_duplicate.js +++ b/uriloader/exthandler/tests/mochitest/browser_local_files_open_doesnt_duplicate.js @@ -26,7 +26,8 @@ add_task(async function setup() { let handlerApp = Cc[ "@mozilla.org/uriloader/local-handler-app;1" ].createInstance(Ci.nsILocalHandlerApp); - handlerApp.executable = Services.dirsvc.get("XCurProcD", Ci.nsIFile); + handlerApp.executable = Services.dirsvc.get("TmpD", Ci.nsIFile); + handlerApp.executable.append("foopydoo.exe"); mimeInfo.possibleApplicationHandlers.appendElement(handlerApp); mimeInfo.preferredApplicationHandler = handlerApp; } @@ -51,7 +52,10 @@ add_task(async function open_from_dialog() { let dialogWindowPromise = BrowserTestUtils.domWindowOpenedAndLoaded(); let openedFile = getChromeDir(getResolvedURI(gTestPath)); openedFile.append("file_pdf_binary_octet_stream.pdf"); - let loadingTab = BrowserTestUtils.addTab(gBrowser, openedFile.path); + let expectedPath = openedFile.isSymlink() + ? openedFile.target + : openedFile.path; + let loadingTab = BrowserTestUtils.addTab(gBrowser, expectedPath); let dialogWindow = await dialogWindowPromise; is( @@ -70,10 +74,10 @@ add_task(async function open_from_dialog() { let [, openedPath] = await openingPromise; is( openedPath, - openedFile.path, + expectedPath, "Should have opened file directly (not created a copy)." ); - if (openedPath != openedFile.path) { + if (openedPath != expectedPath) { await IOUtils.setPermissions(openedPath, 0o666); await IOUtils.remove(openedPath); } @@ -98,15 +102,18 @@ add_task(async function open_directly() { let openedFile = getChromeDir(getResolvedURI(gTestPath)); openedFile.append("file_pdf_binary_octet_stream.pdf"); - let loadingTab = BrowserTestUtils.addTab(gBrowser, openedFile.path); + let expectedPath = openedFile.isSymlink() + ? openedFile.target + : openedFile.path; + let loadingTab = BrowserTestUtils.addTab(gBrowser, expectedPath); let [, openedPath] = await openingPromise; is( openedPath, - openedFile.path, + expectedPath, "Should have opened file directly (not created a copy)." ); - if (openedPath != openedFile.path) { + if (openedPath != expectedPath) { await IOUtils.setPermissions(openedPath, 0o666); await IOUtils.remove(openedPath); } diff --git a/uriloader/exthandler/tests/mochitest/browser_protocolhandler_loop.js b/uriloader/exthandler/tests/mochitest/browser_protocolhandler_loop.js index b9ba4a795503..b4fc0b020b10 100644 --- a/uriloader/exthandler/tests/mochitest/browser_protocolhandler_loop.js +++ b/uriloader/exthandler/tests/mochitest/browser_protocolhandler_loop.js @@ -44,11 +44,9 @@ add_task(async function test_helperapp() { // infinite tabs. We definitely don't want that. Avoid it by stubbing // our external URL handling bits: let oldAddTab = gBrowser.addTab; - registerCleanupFunction( - () => (gBrowser.addTab = gBrowser.loadOneTab = oldAddTab) - ); + registerCleanupFunction(() => (gBrowser.addTab = oldAddTab)); let wrongThingHappenedPromise = new Promise(resolve => { - gBrowser.addTab = gBrowser.loadOneTab = function(aURI) { + gBrowser.addTab = function(aURI) { ok(false, "Tried to open unexpected URL in a tab: " + aURI); resolve(null); // Pass a dummy object to avoid upsetting BrowserContentHandler -