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

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<nsIHandlerApp> preferredApp;
+    mMimeInfo->GetPreferredApplicationHandler(getter_AddRefs(preferredApp));
+    nsCOMPtr<nsILocalHandlerApp> handlerApp = do_QueryInterface(preferredApp);
+    if (handlerApp) {
+      nsCOMPtr<nsIFile> executable;
+      handlerApp->GetExecutable(getter_AddRefs(executable));
+      nsCOMPtr<nsIFile> 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) {

uriloader/exthandler/nsMIMEInfoImpl.cpp

@@ -21,9 +21,20 @@
 static bool sInitializedOurData = false;
 StaticRefPtr<nsIFile> sOurAppFile;
 
-static already_AddRefed<nsIFile> GetCanonicalExecutable(nsIFile* aFile) {
+/* static */
+already_AddRefed<nsIFile> nsMIMEInfoBase::GetCanonicalExecutable(
+    nsIFile* aFile) {
   nsCOMPtr<nsIFile> 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<nsIFile> GetCanonicalExecutable(nsIFile* aFile) {
   while (binary && !StringEndsWith(leafName, u".app"_ns)) {
     nsCOMPtr<nsIFile> 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<nsIFile> binary;
   XRE_GetBinaryPath(getter_AddRefs(binary));
-  sOurAppFile = GetCanonicalExecutable(binary);
+  sOurAppFile = nsMIMEInfoBase::GetCanonicalExecutable(binary);
   ClearOnShutdown(&sOurAppFile);
 }
 

uriloader/exthandler/nsMIMEInfoImpl.h

@@ -95,6 +95,8 @@ class nsMIMEInfoBase : public nsIMIMEInfo {
    */
   bool HasExtensions() const { return mExtensions.Length() != 0; }
 
+  static already_AddRefed<nsIFile> GetCanonicalExecutable(nsIFile* aFile);
+
  protected:
   virtual ~nsMIMEInfoBase();  // must be virtual, as the the base class's
                               // Release should call the subclass's destructor

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.

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;
+  });
+});

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);
   }

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 -