[DOM] Filter out symlinks for webkitdirectory.

This is effectively a back-out of the following patches from Bug 1274959
except we add comments and test coverage:

- Part 1 which made the DirectoryListingTask include symlinks in the
  results as exposed by Directory.getFilesAndDirectories.
- Part 3 which made GetFilesHelper include symlinks in the results.

Test coverage for getFilesAndDirectories is provided by
dom/filesystem/tests/test_basic.html by way of changes to its included
file dom/filesystem/tests/filesystem_commons.js and changes to the
createTreeFile helper in dom/filesystem/tests/script_fileList.js.
Test coverage for GetFilesHelper is provided by
dom/filesystem/tests/test_webkitdirectory.html and changes to the
createTestFile helper in dom/filesystem/tests/script_fileList.js.

Commenting out either of the `isLink` test in the relevant C++ code will
cause the given tests to fail on non-windows platforms.

6 files changed, 134 insertions, 87 deletions

diff --git a/dom/filesystem/GetDirectoryListingTask.cpp b/dom/filesystem/GetDirectoryListingTask.cpp
index 1d4f77b3b8..8fdfb4a5bb 100644
--- a/dom/filesystem/GetDirectoryListingTask.cpp
+++ b/dom/filesystem/GetDirectoryListingTask.cpp
@@ -340,9 +340,14 @@ GetDirectoryListingTaskParent::IOWork()
     nsCOMPtr<nsIFile> currFile = do_QueryInterface(supp);
     MOZ_ASSERT(currFile);
 
-    bool isSpecial, isFile;
-    if (NS_WARN_IF(NS_FAILED(currFile->IsSpecial(&isSpecial))) ||
-        isSpecial) {
+    bool isLink, isSpecial, isFile;
+    if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) ||
+                   NS_FAILED(currFile->IsSpecial(&isSpecial))) ||
+        // Although we allow explicit individual selection of symlinks via the
+        // file picker, we do not process symlinks in directory traversal.  Our
+        // specific policy decision is documented at
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1813299#c20
+        isLink || isSpecial) {
       continue;
     }
     if (NS_WARN_IF(NS_FAILED(currFile->IsFile(&isFile)) ||
diff --git a/dom/filesystem/GetFilesHelper.cpp b/dom/filesystem/GetFilesHelper.cpp
index 563ef60b4a..fb3cf7a24d 100644
--- a/dom/filesystem/GetFilesHelper.cpp
+++ b/dom/filesystem/GetFilesHelper.cpp
@@ -333,13 +333,8 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile)
     return NS_OK;
   }
 
-  nsresult rv = AddExploredDirectory(aFile);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
   nsCOMPtr<nsISimpleEnumerator> entries;
-  rv = aFile->GetDirectoryEntries(getter_AddRefs(entries));
+  nsresult rv = aFile->GetDirectoryEntries(getter_AddRefs(entries));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
@@ -361,7 +356,12 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile)
     bool isLink, isSpecial, isFile, isDir;
     if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) ||
                    NS_FAILED(currFile->IsSpecial(&isSpecial))) ||
-        isSpecial) {
+        isSpecial ||
+        // Although we allow explicit individual selection of symlinks via the
+        // file picker, we do not process symlinks in directory traversal.  Our
+        // specific policy decision is documented at
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1813299#c20
+        isLink) {
       continue;
     }
 
@@ -371,11 +371,6 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile)
       continue;
     }
 
-    // We don't want to explore loops of links.
-    if (isDir && isLink && !ShouldFollowSymLink(currFile)) {
-      continue;
-    }
-
     // The new domPath
     nsAutoString domPath;
     domPath.Assign(aDOMPath);
@@ -415,69 +410,6 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile)
   return NS_OK;
 }
 
-nsresult
-GetFilesHelperBase::AddExploredDirectory(nsIFile* aDir)
-{
-  nsresult rv;
-
-#ifdef DEBUG
-  bool isDir;
-  rv = aDir->IsDirectory(&isDir);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  MOZ_ASSERT(isDir, "Why are we here?");
-#endif
-
-  bool isLink;
-  rv = aDir->IsSymlink(&isLink);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  nsAutoCString path;
-
-  if (!isLink) {
-    nsAutoString path16;
-    rv = aDir->GetPath(path16);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-
-    path = NS_ConvertUTF16toUTF8(path16);
-  } else {
-    rv = aDir->GetNativeTarget(path);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-  }
-
-  mExploredDirectories.PutEntry(path);
-  return NS_OK;
-}
-
-bool
-GetFilesHelperBase::ShouldFollowSymLink(nsIFile* aDir)
-{
-#ifdef DEBUG
-  bool isLink, isDir;
-  if (NS_WARN_IF(NS_FAILED(aDir->IsSymlink(&isLink)) ||
-                 NS_FAILED(aDir->IsDirectory(&isDir)))) {
-    return false;
-  }
-
-  MOZ_ASSERT(isLink && isDir, "Why are we here?");
-#endif
-
-  nsAutoCString targetPath;
-  if (NS_WARN_IF(NS_FAILED(aDir->GetNativeTarget(targetPath)))) {
-    return false;
-  }
-
-  return !mExploredDirectories.Contains(targetPath);
-}
-
 void
 GetFilesHelper::ResolveOrRejectPromise(Promise* aPromise)
 {
diff --git a/dom/filesystem/GetFilesHelper.h b/dom/filesystem/GetFilesHelper.h
index 4afd41d7e0..b6c58ef396 100644
--- a/dom/filesystem/GetFilesHelper.h
+++ b/dom/filesystem/GetFilesHelper.h
@@ -9,10 +9,8 @@
 #include "mozilla/Mutex.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/dom/File.h"
-#include "nsClassHashtable.h"
 #include "nsCycleCollectionTraversalCallback.h"
 #include "nsTArray.h"
-#include "nsTHashtable.h"
 #include "nsThreadUtils.h"
 
 class nsIGlobalObject;
@@ -57,17 +55,10 @@ protected:
   nsresult
   ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile);
 
-  nsresult
-  AddExploredDirectory(nsIFile* aDirectory);
-
-  bool
-  ShouldFollowSymLink(nsIFile* aDirectory);
-
   bool mRecursiveFlag;
 
   // We populate this array in the I/O thread with the BlobImpl.
   FallibleTArray<RefPtr<BlobImpl>> mTargetBlobImplArray;
-  nsTHashtable<nsCStringHashKey> mExploredDirectories;
 };
 
 // Retrieving the list of files can be very time/IO consuming. We use this
diff --git a/dom/filesystem/tests/filesystem_commons.js b/dom/filesystem/tests/filesystem_commons.js
index 4f7234121e..c8bd9ac9fc 100644
--- a/dom/filesystem/tests/filesystem_commons.js
+++ b/dom/filesystem/tests/filesystem_commons.js
@@ -38,6 +38,10 @@ function test_getFilesAndDirectories(aDirectory, aRecursive, aNext) {
           if (data[i] instanceof File) {
             is(data[i].webkitRelativePath, createRelativePath(dir, data[i]), "File.webkitRelativePath should be called: parentdir.path + '/' + file.name: " + data[i].webkitRelativePath);
           }
+          ok(
+            !data[i].webkitRelativePath.endsWith("symlink.txt"),
+            "We should never see a path ending with symlink.txt, our symlink sentinel."
+          );
         }
       }
     );
diff --git a/dom/filesystem/tests/script_fileList.js b/dom/filesystem/tests/script_fileList.js
index 89fd04cabe..dedc61a448 100644
--- a/dom/filesystem/tests/script_fileList.js
+++ b/dom/filesystem/tests/script_fileList.js
@@ -36,6 +36,15 @@ function createTreeFile(depth, parent) {
   if (depth == 0) {
     nextFile.append('file.txt');
     nextFile.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0o600);
+
+#ifdef XP_UNIX
+    // It's not possible to create symlinks on windows by default or on our
+    // Android platforms, so we can't create the symlink file there.  Our
+    // callers that care are aware of this.
+    var linkFile = parent.clone();
+    linkFile.append("symlink.txt");
+    createSymLink(nextFile.path, linkFile.path);
+#endif
   } else {
     nextFile.append('subdir' + depth);
     nextFile.createUnique(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0o700);
@@ -84,6 +93,15 @@ function createTestFile() {
   file2.append('bar.txt');
   file2.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0o600);
 
+#ifdef XP_UNIX
+  // It's not possible to create symlinks on windows by default or on our
+  // Android platforms, so we can't create the symlink file there.  Our
+  // callers that care are aware of this.
+  var linkFile = dir.clone();
+  linkFile.append("symlink.txt");
+  createSymLink(file1.path, linkFile.path);
+#endif 
+
   return tmpFile;
 }
 
@@ -127,3 +145,14 @@ addMessageListener("file.open", function (e) {
     file: File.createFromNsIFile(testFile)
   });
 });
+
+addMessageListener("symlink.open", function (e) {
+  let testDir = createTestFile();
+  let testFile = testDir.clone();
+  testFile.append("subdir");
+  testFile.append("symlink.txt");
+
+  File.createFromNsIFile(testFile).then(function (file) {
+    sendAsyncMessage("symlink.opened", { dir: testDir.path, file });
+  });
+});
diff --git a/dom/filesystem/tests/test_webkitdirectory.html b/dom/filesystem/tests/test_webkitdirectory.html
index 591619e45b..46c728718f 100644
--- a/dom/filesystem/tests/test_webkitdirectory.html
+++ b/dom/filesystem/tests/test_webkitdirectory.html
@@ -9,11 +9,26 @@
 <body>
 <input id="inputFileWebkitDirectory" type="file" webkitdirectory></input>
 <input id="inputFileWebkitDirectoryAndDirectory" type="file" webkitdirectory allowdirs></input>
+<input id="inputFileWebkitFile" type="file"></input>
 <input id="inputFileDirectory" type="file" allowdirs></input>
 <input id="inputFileDirectoryChange" type="file" webkitdirectory></input>
 
 <script type="application/javascript;version=1.7">
 
+// Populate the given input type=file `aInputFile`'s `files` attribute by:
+// - loading `script_fileList.js` in the parent process
+// - telling it to generate the "test" template directory pattern which will
+//   create "foo.txt", "subdir/bar.txt", and if symlinks are available on the
+//   platform, "symlink.txt" which will be a symlink to "foo.txt".  (Note that
+//   we explicitly expect the symlink to be filtered out if generated, and
+//   during the enhancement of the test we verified the file was created on
+//   linux by running the test before fixing the GetFilesHelper logic to filter
+//   the symlink out and verifying the subsequent `test_fileList` check failed.)
+// - Triggering the mock file picker with the base directory of the "test"
+//   template directory.
+//
+// It's expected that `test_fileList` will be used after this step completes in
+// order to validate the results.
 function populateInputFile(aInputFile) {
   var url = SimpleTest.getTestFileURL("script_fileList.js");
   var script = SpecialPowers.loadChromeScript(url);
@@ -52,6 +67,8 @@ function checkFile(file, fileList, dirName) {
   ok(false, "File not found.");
 }
 
+// Validate the contents of the given input type=file `aInputFile`'s' `files`
+// property against the expected list of files `aWhat`.
 function test_fileList(aInputFile, aWhat) {
   var input = document.getElementById(aInputFile);
   var fileList = input.files;
@@ -70,6 +87,65 @@ function test_fileList(aInputFile, aWhat) {
   next();
 }
 
+// Verify that we can explicitly select a symlink and it will not be filtered
+// out.  This is really a verification that GetFileHelper's file-handling logic
+// https://searchfox.org/mozilla-central/rev/065102493dfc49234120c37fc6a334a5b1d86d9e/dom/filesystem/GetFilesHelper.cpp#81-86
+// does not proactively take an action to filter out a selected symlink.
+//
+// This is a glass box test that is not entirely realistic for our actual system
+// file pickers but does reflect what will happen in the drag-and-drop case
+// for `HTMLInputElement::MozSetDndFilesAndDirectories` and this helps ensure
+// that future implementation changes will behave as expected.  Specifically,
+// the presence of webkitdirectory will result in the file picker using
+// `modeGetFolder` which will only allow selection of a directory and forbid
+// file selection.
+//
+// This test explicitly does not validate HTMLInputElement's non-webkitdirectory
+// file selection mechanism because it does not involve GetFileHelper.
+async function test_individualSymlink(aInputFile) {
+  const input = document.getElementById(aInputFile);
+
+  // -- Create the symlink and get a `File` instance pointing at it.
+  const url = SimpleTest.getTestFileURL("script_fileList.js");
+  const script = SpecialPowers.loadChromeScript(url);
+
+  let opened = new Promise(resolve => script.addMessageListener("symlink.opened", resolve));
+  script.sendAsyncMessage("symlink.open", {});
+  let { dir, file: symlinkFile } = await opened;
+  info(`symlink.open provided dir: ${dir}`)
+
+  // -- Have the picker pick it
+  var MockFilePicker = SpecialPowers.MockFilePicker;
+  MockFilePicker.init(window, "A Mock File Picker", SpecialPowers.Ci.nsIFilePicker.modeOpen);
+
+  MockFilePicker.displayDirectory = dir;
+  let pickerShown = new Promise(resolve => {
+    MockFilePicker.showCallback = function() {
+      // This is where we are diverging from a realistic scenario in order to get
+      // the expected coverage.
+      MockFilePicker.setFiles([symlinkFile]);
+      resolve();
+    }
+  });
+  MockFilePicker.returnValue = MockFilePicker.returnOK;
+
+  let changeEvent = waitForEvent(input, "change");
+
+  input.click();
+
+  await pickerShown;
+  await changeEvent;
+
+  MockFilePicker.cleanup();
+  script.destroy();
+
+  // -- Verify that we see the symlink.
+  let fileList = input.files;
+  is(fileList.length, 1, "There should be 1 file.");
+  is(fileList[0].name, "symlink.txt", "The file should be the symlink.");
+  next();
+}
+
 function test_webkitdirectory_attribute() {
   var a = document.createElement("input");
   a.setAttribute("type", "file");
@@ -169,6 +245,16 @@ var tests = [
   function() { test_fileList('inputFileWebkitDirectory', testDirData) },
   function() { test_fileList('inputFileWebkitDirectoryAndDirectory', testDirData) },
   function() { test_fileList('inputFileDirectory', null); },
+  
+  function() {
+#ifdef XP_WIN    // Symlinks are not available on Windows and so will not be created.
+    info("Skipping individual symlink check on Windows and Android.");
+    next();
+    return;
+#endif
+    test_individualSymlink("inputFileWebkitFile").catch(err => ok(false, `Problem in symlink case: ${err}`));
+  },
+
 
   test_webkitdirectory_attribute,