[FS] Pass PathType to ConfirmSensitiveDirectoryAccess.

External paths aren't absolute paths, and thus need to be treated
differently. Actually blocking paths (like the root of the Downloads
directory) seems to have been broken already, so for now this just
allows all external paths. We should add special chrome-os logic to
block what we do want to block in a follow-up.

Bug: 1009970, 1134029
Change-Id: I8e5e437698b619d9df643706119c8b3492f58476
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2442901Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820277}

chrome/browser/native_file_system/chrome_native_file_system_permission_context.cc

@@ -350,25 +350,29 @@ bool ChromeNativeFileSystemPermissionContext::CanObtainWritePermission(
 
 void ChromeNativeFileSystemPermissionContext::ConfirmSensitiveDirectoryAccess(
     const url::Origin& origin,
-    const std::vector<base::FilePath>& paths,
+    PathType path_type,
+    const base::FilePath& path,
     HandleType handle_type,
     content::GlobalFrameRoutingId frame_id,
-
     base::OnceCallback<void(SensitiveDirectoryResult)> callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (paths.empty()) {
+
+  // TODO(https://crbug.com/1009970): Figure out what external paths should be
+  // blocked. We could resolve the external path to a local path, and check for
+  // blocked directories based on that, but that doesn't work well. Instead we
+  // should have a separate Chrome OS only code path to block for example the
+  // root of certain external file systems.
+  if (path_type == PathType::kExternal) {
     std::move(callback).Run(SensitiveDirectoryResult::kAllowed);
     return;
   }
-  // It is enough to only verify access to the first path, as multiple
-  // file selection is only supported if all files are in the same
-  // directory.
+
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&ShouldBlockAccessToPath, paths[0], handle_type),
+      base::BindOnce(&ShouldBlockAccessToPath, path, handle_type),
       base::BindOnce(&ChromeNativeFileSystemPermissionContext::
                          DidConfirmSensitiveDirectoryAccess,
-                     GetWeakPtr(), origin, paths, handle_type, frame_id,
+                     GetWeakPtr(), origin, path, handle_type, frame_id,
                      std::move(callback)));
 }
 
@@ -398,7 +402,7 @@ void ChromeNativeFileSystemPermissionContext::PerformAfterWriteChecks(
 void ChromeNativeFileSystemPermissionContext::
     DidConfirmSensitiveDirectoryAccess(
         const url::Origin& origin,
-        const std::vector<base::FilePath>& paths,
+        const base::FilePath& path,
         HandleType handle_type,
         content::GlobalFrameRoutingId frame_id,
         base::OnceCallback<void(SensitiveDirectoryResult)> callback,
@@ -415,7 +419,7 @@ void ChromeNativeFileSystemPermissionContext::
   content::GetUIThreadTaskRunner({})->PostTask(
       FROM_HERE,
       base::BindOnce(&ShowNativeFileSystemRestrictedDirectoryDialogOnUIThread,
-                     frame_id, origin, paths[0], handle_type,
+                     frame_id, origin, path, handle_type,
                      std::move(result_callback)));
 }
 

chrome/browser/native_file_system/chrome_native_file_system_permission_context.h

@@ -48,7 +48,8 @@ class ChromeNativeFileSystemPermissionContext
   // content::NativeFileSystemPermissionContext:
   void ConfirmSensitiveDirectoryAccess(
       const url::Origin& origin,
-      const std::vector<base::FilePath>& paths,
+      PathType path_type,
+      const base::FilePath& path,
       HandleType handle_type,
       content::GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(SensitiveDirectoryResult)> callback) override;
@@ -96,7 +97,7 @@ class ChromeNativeFileSystemPermissionContext
  private:
   void DidConfirmSensitiveDirectoryAccess(
       const url::Origin& origin,
-      const std::vector<base::FilePath>& paths,
+      const base::FilePath& path,
       HandleType handle_type,
       content::GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(SensitiveDirectoryResult)> callback,

content/browser/file_system_access/file_system_chooser_browsertest.cc

@@ -474,18 +474,23 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, OpenDirectory_DenyAccess) {
   auto write_grant = base::MakeRefCounted<FixedNativeFileSystemPermissionGrant>(
       PermissionStatus::ASK, base::FilePath());
 
-  EXPECT_CALL(permission_context,
-              CanObtainReadPermission(url::Origin::Create(
-                  embedded_test_server()->GetURL("/title1.html"))))
+  auto origin =
+      url::Origin::Create(embedded_test_server()->GetURL("/title1.html"));
+  auto frame_id = GlobalFrameRoutingId(
+      shell()->web_contents()->GetMainFrame()->GetProcess()->GetID(),
+      shell()->web_contents()->GetMainFrame()->GetRoutingID());
+
+  EXPECT_CALL(permission_context, CanObtainReadPermission(origin))
       .WillOnce(testing::Return(true));
 
-  EXPECT_CALL(permission_context,
-              ConfirmSensitiveDirectoryAccess_(
-                  testing::_, testing::_, testing::_, testing::_, testing::_))
-      .WillOnce(RunOnceCallback<4>(SensitiveDirectoryResult::kAllowed));
+  EXPECT_CALL(
+      permission_context,
+      ConfirmSensitiveDirectoryAccess_(
+          origin, NativeFileSystemPermissionContext::PathType::kLocal, test_dir,
+          NativeFileSystemPermissionContext::HandleType::kDirectory, frame_id,
+          testing::_))
+      .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAllowed));
 
-  auto origin =
-      url::Origin::Create(embedded_test_server()->GetURL("/title1.html"));
   EXPECT_CALL(permission_context,
               GetReadPermissionGrant(
                   origin, test_dir,
@@ -502,9 +507,7 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, OpenDirectory_DenyAccess) {
   EXPECT_CALL(
       *read_grant,
       RequestPermission_(
-          GlobalFrameRoutingId(
-              shell()->web_contents()->GetMainFrame()->GetProcess()->GetID(),
-              shell()->web_contents()->GetMainFrame()->GetRoutingID()),
+          frame_id,
           NativeFileSystemPermissionGrant::UserActivationState::kNotRequired,
           testing::_))
       .WillOnce(RunOnceCallback<2>(NativeFileSystemPermissionGrant::
@@ -536,10 +539,19 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
           ->GetNativeFileSystemEntryFactory())
       ->SetPermissionContextForTesting(&permission_context);
 
-  EXPECT_CALL(permission_context,
-              ConfirmSensitiveDirectoryAccess_(
-                  testing::_, testing::_, testing::_, testing::_, testing::_))
-      .WillOnce(RunOnceCallback<4>(SensitiveDirectoryResult::kAbort));
+  auto origin =
+      url::Origin::Create(embedded_test_server()->GetURL("/title1.html"));
+  auto frame_id = GlobalFrameRoutingId(
+      shell()->web_contents()->GetMainFrame()->GetProcess()->GetID(),
+      shell()->web_contents()->GetMainFrame()->GetRoutingID());
+
+  EXPECT_CALL(
+      permission_context,
+      ConfirmSensitiveDirectoryAccess_(
+          origin, NativeFileSystemPermissionContext::PathType::kLocal,
+          test_file, NativeFileSystemPermissionContext::HandleType::kFile,
+          frame_id, testing::_))
+      .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
 
   EXPECT_CALL(permission_context,
               CanObtainReadPermission(url::Origin::Create(
@@ -587,18 +599,23 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
           ->GetNativeFileSystemEntryFactory())
       ->SetPermissionContextForTesting(&permission_context);
 
-  EXPECT_CALL(permission_context,
-              ConfirmSensitiveDirectoryAccess_(
-                  testing::_, testing::_, testing::_, testing::_, testing::_))
-      .WillOnce(RunOnceCallback<4>(SensitiveDirectoryResult::kAbort));
+  auto origin =
+      url::Origin::Create(embedded_test_server()->GetURL("/title1.html"));
+  auto frame_id = GlobalFrameRoutingId(
+      shell()->web_contents()->GetMainFrame()->GetProcess()->GetID(),
+      shell()->web_contents()->GetMainFrame()->GetRoutingID());
 
-  EXPECT_CALL(permission_context,
-              CanObtainReadPermission(url::Origin::Create(
-                  embedded_test_server()->GetURL("/title1.html"))))
+  EXPECT_CALL(
+      permission_context,
+      ConfirmSensitiveDirectoryAccess_(
+          origin, NativeFileSystemPermissionContext::PathType::kLocal,
+          test_file, NativeFileSystemPermissionContext::HandleType::kFile,
+          frame_id, testing::_))
+      .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
+
+  EXPECT_CALL(permission_context, CanObtainReadPermission(origin))
       .WillOnce(testing::Return(true));
-  EXPECT_CALL(permission_context,
-              CanObtainWritePermission(url::Origin::Create(
-                  embedded_test_server()->GetURL("/title1.html"))))
+  EXPECT_CALL(permission_context, CanObtainWritePermission(origin))
       .WillOnce(testing::Return(true));
 
   ASSERT_TRUE(

content/browser/file_system_access/mock_native_file_system_permission_context.cc

@@ -13,12 +13,13 @@ MockNativeFileSystemPermissionContext::
 
 void MockNativeFileSystemPermissionContext::ConfirmSensitiveDirectoryAccess(
     const url::Origin& origin,
-    const std::vector<base::FilePath>& paths,
+    PathType path_type,
+    const base::FilePath& path,
     HandleType handle_type,
     GlobalFrameRoutingId frame_id,
     base::OnceCallback<void(SensitiveDirectoryResult)> callback) {
-  ConfirmSensitiveDirectoryAccess_(origin, paths, handle_type, frame_id,
-                                   callback);
+  ConfirmSensitiveDirectoryAccess_(origin, path_type, path, handle_type,
+                                   frame_id, callback);
 }
 
 void MockNativeFileSystemPermissionContext::PerformAfterWriteChecks(

content/browser/file_system_access/mock_native_file_system_permission_context.h

@@ -32,14 +32,16 @@ class MockNativeFileSystemPermissionContext
 
   void ConfirmSensitiveDirectoryAccess(
       const url::Origin& origin,
-      const std::vector<base::FilePath>& paths,
+      PathType path_type,
+      const base::FilePath& path,
       HandleType handle_type,
       GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(SensitiveDirectoryResult)> callback) override;
-  MOCK_METHOD5(
+  MOCK_METHOD6(
       ConfirmSensitiveDirectoryAccess_,
       void(const url::Origin& origin,
-           const std::vector<base::FilePath>& paths,
+           PathType path_type,
+           const base::FilePath& path,
            HandleType handle_type,
            GlobalFrameRoutingId frame_id,
            base::OnceCallback<void(SensitiveDirectoryResult)>& callback));

content/browser/file_system_access/native_file_system_manager_impl.cc

@@ -832,7 +832,7 @@ void NativeFileSystemManagerImpl::DidChooseEntries(
     std::vector<FileSystemChooser::ResultEntry> entries) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (result->status != NativeFileSystemStatus::kOk) {
+  if (result->status != NativeFileSystemStatus::kOk || entries.empty()) {
     std::move(callback).Run(
         std::move(result),
         std::vector<blink::mojom::NativeFileSystemEntryPtr>());
@@ -846,15 +846,14 @@ void NativeFileSystemManagerImpl::DidChooseEntries(
     return;
   }
 
-  std::vector<base::FilePath> paths;
-  paths.reserve(entries.size());
-  for (const auto& entry : entries)
-    paths.push_back(entry.path);
-
+  // It is enough to only verify access to the first path, as multiple
+  // file selection is only supported if all files are in the same
+  // directory.
+  FileSystemChooser::ResultEntry first_entry = entries.front();
   const bool is_directory =
       options.type() == blink::mojom::ChooseFileSystemEntryType::kOpenDirectory;
   permission_context_->ConfirmSensitiveDirectoryAccess(
-      binding_context.origin, std::move(paths),
+      binding_context.origin, first_entry.type, first_entry.path,
       is_directory ? HandleType::kDirectory : HandleType::kFile,
       binding_context.frame_id,
       base::BindOnce(

content/public/browser/native_file_system_entry_factory.h

@@ -25,6 +25,7 @@ class CONTENT_EXPORT NativeFileSystemEntryFactory
                                         BrowserThread::DeleteOnUIThread> {
  public:
   using UserAction = NativeFileSystemPermissionContext::UserAction;
+  using PathType = NativeFileSystemPermissionContext::PathType;
 
   // Context from which a created handle is going to be used. This is used for
   // security and permission checks. Pass in the URL most relevant as the url
@@ -48,19 +49,6 @@ class CONTENT_EXPORT NativeFileSystemEntryFactory
     int process_id() const { return frame_id.child_id; }
   };
 
-  enum class PathType {
-    // A path on the local file system. Files with these paths can be operated
-    // on by base::File.
-    kLocal,
-
-    // A path on an "external" file system. These paths can only be accessed via
-    // the filesystem abstraction in //storage/browser/file_system, and a
-    // storage::kFileSystemTypeExternal storage::FileSystemURL.
-    // This path type should be used for paths retrieved via the `virtual_path`
-    // member of a ui::SelectedFileInfo struct.
-    kExternal
-  };
-
   // Creates a new NativeFileSystemEntryPtr from the path to a file. Assumes the
   // passed in path is valid and represents a file.
   virtual blink::mojom::NativeFileSystemEntryPtr CreateFileEntryFromPath(

content/public/browser/native_file_system_permission_context.h

@@ -46,6 +46,19 @@ class NativeFileSystemPermissionContext {
   // handles.
   enum class HandleType { kFile, kDirectory };
 
+  enum class PathType {
+    // A path on the local file system. Files with these paths can be operated
+    // on by base::File.
+    kLocal,
+
+    // A path on an "external" file system. These paths can only be accessed via
+    // the filesystem abstraction in //storage/browser/file_system, and a
+    // storage::FileSystemURL of type storage::kFileSystemTypeExternal.
+    // This path type should be used for paths retrieved via the `virtual_path`
+    // member of a ui::SelectedFileInfo struct.
+    kExternal
+  };
+
   // Returns the read permission grant to use for a particular path.
   virtual scoped_refptr<NativeFileSystemPermissionGrant> GetReadPermissionGrant(
       const url::Origin& origin,
@@ -72,14 +85,15 @@ class NativeFileSystemPermissionContext {
     kAbort = 2,     // Abandon entirely, as if picking was cancelled.
     kMaxValue = kAbort
   };
-  // Checks if access to the given |paths| should be allowed or blocked. This is
+  // Checks if access to the given |path| should be allowed or blocked. This is
   // used to implement blocks for certain sensitive directories such as the
   // "Windows" system directory, as well as the root of the "home" directory.
   // Calls |callback| with the result of the check, after potentially showing
   // some UI to the user if the path should not be accessed.
   virtual void ConfirmSensitiveDirectoryAccess(
       const url::Origin& origin,
-      const std::vector<base::FilePath>& paths,
+      PathType path_type,
+      const base::FilePath& path,
       HandleType handle_type,
       GlobalFrameRoutingId frame_id,
       base::OnceCallback<void(SensitiveDirectoryResult)> callback) = 0;