From a48f5f1f76ae5ba45cdf1fd01b8f875e1649e4dd Mon Sep 17 00:00:00 2001
From: Bryan Oltman <bryanoltman@google.com>
Date: Thu, 14 May 2026 18:36:55 -0700
Subject: [PATCH] [FSA] Revoke descendant grants in NotifyEntryRemoved

When a directory is removed, NotifyEntryRemoved is called with the
directory path. Previously, it only revoked permissions for the
directory itself, leaving descendant file/directory grants active.

This CL updates NotifyEntryRemoved to iterate through all active and
persisted grants, revoking those that are descendants of the removed
path in addition to the removed path.

Fixed: 501810874
Change-Id: I652dc8b81ddcecf01dd499ea9b494d71fb37f916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7850274
Reviewed-by: Elias Klim <elklm@chromium.org>
Commit-Queue: Bryan Oltman <bryanoltman@google.com>
Cr-Commit-Position: refs/heads/main@{#1630980}
---

diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
index 09b470c..bd610b8 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
@@ -1202,26 +1202,16 @@
     }
   }
 
-  // Downgrades the in-memory read permission grant for the `path` if it exist
-  //  in `grants`. This is different from
+  // Downgrades the in-memory read permission grant. This is different from
   // ChromeFileSystemAccessPermissionContext::RevokeGrant in that this method
   // does not reset the persisted permission state.
-  static void DowngradeReadGrantInMemory(
-      std::map<base::FilePath, raw_ptr<PermissionGrantImpl, CtnExperimental>>&
-          grants,
-      const content::PathInfo& path) {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  void DowngradeActiveReadGrant() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-    auto entry_it = std::ranges::find_if(grants, [&path](const auto& entry) {
-      return entry.first == path.path;
-    });
-    if (entry_it == grants.end()) {
+    if (GetActivePermissionStatus() != PermissionStatus::GRANTED) {
       return;
     }
 
-    DCHECK_EQ(entry_it->second->GetActivePermissionStatus(),
-              PermissionStatus::GRANTED);
-    auto* const grant_impl = entry_it->second.get();
     // Updates the in-memory status of the grant synchronously. This ensures
     // that any existing handle instances that hold a `scoped_refptr` to this
     // grant will immediately see the updated permission status.
@@ -1234,9 +1224,8 @@
     // asynchronously after a `remove()`, a subsequent query for a new handle
     // (e.g., from IndexedDB) could read the stale on-disk state and
     // incorrectly return `GRANTED`.
-    grant_impl->SetStatus(
-        PermissionStatus::DENIED,
-        PersistedPermissionOptions::kDoNotUpdatePersistedPermission);
+    SetStatus(PermissionStatus::DENIED,
+              PersistedPermissionOptions::kDoNotUpdatePersistedPermission);
   }
 
  protected:
@@ -2617,24 +2606,41 @@
     return;
   }
 
+  auto is_path_or_descendant = [&](const base::FilePath& file_path) {
+    return file_path == path.path || path.path.IsParent(file_path);
+  };
+
   bool updated = false;
   auto it = active_permissions_map_.find(origin);
   if (it != active_permissions_map_.end()) {
-    PermissionGrantImpl::DowngradeReadGrantInMemory(it->second.read_grants,
-                                                    path);
-    // Marks the path as downgraded so that it can be restored later.
-    it->second.downgraded_read_paths.insert(path.path);
+    auto& origin_state = it->second;
+    // Always insert the removed path itself into downgraded read paths.
+    origin_state.downgraded_read_paths.insert(path.path);
     updated = true;
+
+    // Revoke active read grants for the removed entry and its descendants.
+    for (auto& [grant_path, grant] : origin_state.read_grants) {
+      if (!is_path_or_descendant(grant_path)) {
+        continue;
+      }
+      grant->DowngradeActiveReadGrant();
+      origin_state.downgraded_read_paths.insert(grant_path);
+    }
   }
 
   if (base::FeatureList::IsEnabled(
           features::kFileSystemAccessPersistentPermissions)) {
     // Active grants are a subset of persisted grants, so we also need to update
-    // persisted grants, which is not covered by
-    // `PermissionGrantImpl::DowngradeReadGrantInMemory()` above.
-    const std::unique_ptr<Object> object =
-        GetGrantedObject(origin, PathAsPermissionKey(path.path));
-    if (object) {
+    // persisted grants, which is not covered by `DowngradeActiveReadGrant()`
+    // above.
+    // Revoke persisted read grants for the removed entry and its descendants.
+    for (const auto& object : GetGrantedObjects(origin)) {
+      std::optional<base::FilePath> grant_path =
+          base::ValueToFilePath(object->value.Find(kPermissionPathKey));
+      if (!grant_path || !is_path_or_descendant(*grant_path)) {
+        continue;
+      }
+
       base::DictValue new_object = object->value.Clone();
       new_object.Set(GetGrantKeyFromGrantType(GrantType::kRead), false);
       UpdateObjectPermission(origin, object->value, std::move(new_object));
diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
index f729660..05dc903 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
@@ -3628,6 +3628,77 @@
       kTestOrigin, file_path_info.path));
 }
 
+// Tests that calling NotifyEntryRemoved with a directory path correctly revokes
+// read permission grants for all descendants of that directory.
+// Regression test for crbug.com/501810874.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryRemoved_RecursiveDir_DescendantFileGrantDowngraded) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  FileSystemAccessPermissionRequestManager::FromWebContents(web_contents())
+      ->set_auto_response_for_test(PermissionAction::GRANTED);
+
+  // Sets up a directory path and a child file path to be the test targets.
+  const auto dir_info = kTestPathInfo;
+  const auto file_info = PathInfo(dir_info.path.AppendASCII("config.json"));
+
+  // Grant a standalone read permission for the child file.
+  auto file_read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_info, HandleType::kFile, UserAction::kOpen);
+  ASSERT_EQ(file_read_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Grant read and write permission to the directory.
+  auto dir_read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, dir_info, HandleType::kDirectory, UserAction::kOpen);
+  {
+    base::test::TestFuture<PermissionRequestOutcome> f;
+    dir_read_grant->RequestPermission(
+        frame_id(), UserActivationState::kNotRequired, f.GetCallback());
+    ASSERT_EQ(f.Get(), PermissionRequestOutcome::kUserGranted);
+  }
+  auto dir_write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, dir_info, HandleType::kDirectory, UserAction::kOpen);
+  {
+    base::test::TestFuture<PermissionRequestOutcome> f;
+    dir_write_grant->RequestPermission(
+        frame_id(), UserActivationState::kNotRequired, f.GetCallback());
+    ASSERT_EQ(f.Get(), PermissionRequestOutcome::kUserGranted);
+  }
+  ASSERT_EQ(dir_read_grant->GetStatus(), PermissionStatus::GRANTED);
+  ASSERT_EQ(dir_write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Revoke permissions for the directory. This represents a recursive removal
+  // of the directory.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, dir_info);
+
+  // Verify that the directory's own read permission is downgraded.
+  EXPECT_EQ(dir_read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, dir_info.path));
+
+  // Verify that the descendant file's read permission is also downgraded.
+  EXPECT_EQ(file_read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_info.path));
+
+  // Verify that a fresh lookup also sees the downgraded status while the grant
+  // is still in memory.
+  EXPECT_EQ(permission_context()
+                ->GetReadPermissionGrant(kTestOrigin, file_info,
+                                         HandleType::kFile, UserAction::kNone)
+                ->GetStatus(),
+            PermissionStatus::DENIED);
+
+  // Once the grant is no longer in memory, its status should revert to ASK.
+  file_read_grant.reset();
+  EXPECT_EQ(permission_context()
+                ->GetReadPermissionGrant(kTestOrigin, file_info,
+                                         HandleType::kFile, UserAction::kNone)
+                ->GetStatus(),
+            PermissionStatus::ASK);
+}
+
 // Tests that moving a file to a destination with a pre-existing permission
 // grant works correctly.

diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
index f729660..05dc903 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
@@ -3628,6 +3628,77 @@
       kTestOrigin, file_path_info.path));
 }
 
+// Tests that calling NotifyEntryRemoved with a directory path correctly revokes
+// read permission grants for all descendants of that directory.
+// Regression test for crbug.com/501810874.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryRemoved_RecursiveDir_DescendantFileGrantDowngraded) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  FileSystemAccessPermissionRequestManager::FromWebContents(web_contents())
+      ->set_auto_response_for_test(PermissionAction::GRANTED);
+
+  // Sets up a directory path and a child file path to be the test targets.
+  const auto dir_info = kTestPathInfo;
+  const auto file_info = PathInfo(dir_info.path.AppendASCII("config.json"));
+
+  // Grant a standalone read permission for the child file.
+  auto file_read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_info, HandleType::kFile, UserAction::kOpen);
+  ASSERT_EQ(file_read_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Grant read and write permission to the directory.
+  auto dir_read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, dir_info, HandleType::kDirectory, UserAction::kOpen);
+  {
+    base::test::TestFuture<PermissionRequestOutcome> f;
+    dir_read_grant->RequestPermission(
+        frame_id(), UserActivationState::kNotRequired, f.GetCallback());
+    ASSERT_EQ(f.Get(), PermissionRequestOutcome::kUserGranted);
+  }
+  auto dir_write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, dir_info, HandleType::kDirectory, UserAction::kOpen);
+  {
+    base::test::TestFuture<PermissionRequestOutcome> f;
+    dir_write_grant->RequestPermission(
+        frame_id(), UserActivationState::kNotRequired, f.GetCallback());
+    ASSERT_EQ(f.Get(), PermissionRequestOutcome::kUserGranted);
+  }
+  ASSERT_EQ(dir_read_grant->GetStatus(), PermissionStatus::GRANTED);
+  ASSERT_EQ(dir_write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Revoke permissions for the directory. This represents a recursive removal
+  // of the directory.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, dir_info);
+
+  // Verify that the directory's own read permission is downgraded.
+  EXPECT_EQ(dir_read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, dir_info.path));
+
+  // Verify that the descendant file's read permission is also downgraded.
+  EXPECT_EQ(file_read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_info.path));
+
+  // Verify that a fresh lookup also sees the downgraded status while the grant
+  // is still in memory.
+  EXPECT_EQ(permission_context()
+                ->GetReadPermissionGrant(kTestOrigin, file_info,
+                                         HandleType::kFile, UserAction::kNone)
+                ->GetStatus(),
+            PermissionStatus::DENIED);
+
+  // Once the grant is no longer in memory, its status should revert to ASK.
+  file_read_grant.reset();
+  EXPECT_EQ(permission_context()
+                ->GetReadPermissionGrant(kTestOrigin, file_info,
+                                         HandleType::kFile, UserAction::kNone)
+                ->GetStatus(),
+            PermissionStatus::ASK);
+}
+
 // Tests that moving a file to a destination with a pre-existing permission
 // grant works correctly.
 TEST_F(ChromeFileSystemAccessPermissionContextTest,