From ab88b786ad83b611543df884ef7344d7ff92a906 Mon Sep 17 00:00:00 2001
From: Ming-Ying Chung <[email protected]>
Date: Tue, 11 Aug 2026 18:30:26 -0700
Subject: [PATCH] [FSA] Maintain downgraded read permissions across file moves

A state tracking flaw in NotifyEntryMoved() leaves stale entries in
downgraded_read_paths when file entries are moved. When handle.remove()
downgrades a read grant to DENIED, the path is stored in
downgraded_read_paths to track potential origin file recreations.
However, moving the handle away and back leaves the original path in
downgraded_read_paths. Consequently, MaybeRestoreReadPermission()
inappropriately restores read access to GRANTED on a file the origin did
not author.

For example, consider an origin manipulating a target file handle:

(1) Initial state: Read status is GRANTED and write status is GRANTED
    for "secret.txt".
(2) handle.remove(): Read status drops to DENIED, write status remains
    GRANTED, and "secret.txt" is added to downgraded_read_paths.
(3) Victim recreates "secret.txt": Read status for origin remains
    DENIED.
(4) handle.move('temp.txt'): Active grants migrate to "temp.txt", but
    "secret.txt" remains in downgraded_read_paths.
(5) handle.move('secret.txt'): Grants migrate back to secret.txt, and
    MaybeRestoreReadPermission() finds "secret.txt" in
    downgraded_read_paths, incorrectly restoring read status to GRANTED.

Also drop the obsolete DCHECK in PermissionGrantImpl:UpdateGrantPath():
removing it is safe because DENIED read grants now represent a valid
runtime state. PermissionGrantImpl::UpdateGrantPath() asserted that
active grants being moved held GRANTED status, which caused Debug builds
to crash when moving handles with read grants downgraded to DENIED by
handle.remove(). Prior to kFileSystemAccessRevokeReadOnRemove, active
grants being moved were assumed to be GRANTED. Under
kFileSystemAccessRevokeReadOnRemove, revoked grants are retained in
read_grants with status DENIED to track state across operations, so
removing the DCHECK allows PermissionGrantImpl::UpdateGrantPath() to
safely update grant paths without altering permission checks.

In summary, this CL updates NotifyEntryMoved() to carry
downgraded_read_paths entries alongside active grants, drops the
obsolete assertion in PermissionGrantImpl::UpdateGrantPath(), and adds
dedicated unit tests to verify state migration and assertion safety in
Debug builds.

TAG=agy
CONV=214d42c9-1a4f-4404-85d5-800d9b0885eb

Bug: 496401361
Change-Id: I7844d3e7cfe5ec9363bd16e028665a94fc79e620
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8224042
Reviewed-by: Mingyu Lei <[email protected]>
Commit-Queue: Ming-Ying Chung <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1677704}
---

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 e0d5734..f2400b7 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
@@ -1223,9 +1223,6 @@
       return;
     }
 
-    DCHECK_EQ(old_path_it->second->GetActivePermissionStatus(),
-              PermissionStatus::GRANTED);
-
     auto* const grant_to_move = old_path_it->second.get();
 
     if (allow_overwrite) {
@@ -2665,6 +2662,7 @@
       features::kFileSystemAccessMoveWithOverwrite);
 
   bool updated = false;
+  bool old_path_was_downgraded = false;
   auto it = active_permissions_map_.find(origin);
   if (it != active_permissions_map_.end()) {
     // TODO(crbug.com/40245144): Consolidate superfluous child grants.
@@ -2672,6 +2670,14 @@
                                          new_path, allow_overwrite);
     PermissionGrantImpl::UpdateGrantPath(it->second.read_grants, old_path,
                                          new_path, allow_overwrite);
+    if (base::FeatureList::IsEnabled(
+            blink::features::kFileSystemAccessRevokeReadOnRemove) &&
+        it->second.downgraded_read_paths.erase(old_path.path)) {
+      // The downgraded read grant moved along with the entry; carry the
+      // downgraded state to `new_path` so a later write there can restore it.
+      it->second.downgraded_read_paths.insert(new_path.path);
+      old_path_was_downgraded = true;
+    }
     updated = true;
   }
   if (base::FeatureList::IsEnabled(
@@ -2702,7 +2708,10 @@
   }
 
   if (base::FeatureList::IsEnabled(
-          blink::features::kFileSystemAccessRevokeReadOnRemove)) {
+          blink::features::kFileSystemAccessRevokeReadOnRemove) &&
+      !old_path_was_downgraded) {
+    // Only restore if the moved entry was readable at `old_path`; otherwise
+    // the origin has not authored the content now at `new_path`.
     MaybeRestoreReadPermission(origin, new_path.path);
   }
 }
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 337d442..67f0e25 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
@@ -3795,6 +3795,103 @@
             PermissionStatus::ASK);
 }
 
+// Verifies that moving an entry with a downgraded read grant transfers the
+// downgraded state to the destination path, and moving the entry back to its
+// original path does not restore the read grant.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryMoved_DowngradedReadGrantFollowsMove) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  permission_context()->SetOriginHasExtendedPermissionForTesting(kTestOrigin);
+
+  const auto file_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("test_file.txt"));
+  const auto temp_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("temp_file.txt"));
+
+  // Initialize read and write permissions for the target file path.
+  auto read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  auto write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Calling `NotifyEntryRemoved()` downgrades the read grant and records the
+  // path in `downgraded_read_paths`.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, file_path_info);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+
+  // Calling `NotifyEntryMoved()` transfers the downgraded read state to the
+  // destination path.
+  permission_context()->NotifyEntryMoved(kTestOrigin, file_path_info,
+                                         temp_path_info);
+  EXPECT_EQ(read_grant->GetPath(), temp_path_info.path);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, temp_path_info.path));
+
+  // Moving the entry back to its original path retains the `DENIED` read grant
+  // because the origin has not authored new content at the destination.
+  permission_context()->NotifyEntryMoved(kTestOrigin, temp_path_info,
+                                         file_path_info);
+  EXPECT_EQ(read_grant->GetPath(), file_path_info.path);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, temp_path_info.path));
+
+  // Calling `NotifyEntryModified()` at the original path restores the read
+  // grant.
+  permission_context()->NotifyEntryModified(kTestOrigin, file_path_info);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+}
+
+// Verifies that moving a handle with a downgraded read grant successfully
+// updates the grant path without triggering assertion failures.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryMoved_DowngradedReadGrantMigratesWithoutAssertionFailure) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  permission_context()->SetOriginHasExtendedPermissionForTesting(kTestOrigin);
+
+  const auto file_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("test_file.txt"));
+  const auto temp_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("temp_file.txt"));
+
+  // Initialize read and write grants.
+  auto read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  auto write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Downgrade the read grant to `DENIED` via `NotifyEntryRemoved()`.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, file_path_info);

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 337d442..67f0e25 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
@@ -3795,6 +3795,103 @@
             PermissionStatus::ASK);
 }
 
+// Verifies that moving an entry with a downgraded read grant transfers the
+// downgraded state to the destination path, and moving the entry back to its
+// original path does not restore the read grant.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryMoved_DowngradedReadGrantFollowsMove) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  permission_context()->SetOriginHasExtendedPermissionForTesting(kTestOrigin);
+
+  const auto file_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("test_file.txt"));
+  const auto temp_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("temp_file.txt"));
+
+  // Initialize read and write permissions for the target file path.
+  auto read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  auto write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Calling `NotifyEntryRemoved()` downgrades the read grant and records the
+  // path in `downgraded_read_paths`.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, file_path_info);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+
+  // Calling `NotifyEntryMoved()` transfers the downgraded read state to the
+  // destination path.
+  permission_context()->NotifyEntryMoved(kTestOrigin, file_path_info,
+                                         temp_path_info);
+  EXPECT_EQ(read_grant->GetPath(), temp_path_info.path);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, temp_path_info.path));
+
+  // Moving the entry back to its original path retains the `DENIED` read grant
+  // because the origin has not authored new content at the destination.
+  permission_context()->NotifyEntryMoved(kTestOrigin, temp_path_info,
+                                         file_path_info);
+  EXPECT_EQ(read_grant->GetPath(), file_path_info.path);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_TRUE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, temp_path_info.path));
+
+  // Calling `NotifyEntryModified()` at the original path restores the read
+  // grant.
+  permission_context()->NotifyEntryModified(kTestOrigin, file_path_info);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_FALSE(permission_context()->IsPathInDowngradedReadPathsForTesting(
+      kTestOrigin, file_path_info.path));
+}
+
+// Verifies that moving a handle with a downgraded read grant successfully
+// updates the grant path without triggering assertion failures.
+TEST_F(ChromeFileSystemAccessPermissionContextTest,
+       NotifyEntryMoved_DowngradedReadGrantMigratesWithoutAssertionFailure) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      blink::features::kFileSystemAccessRevokeReadOnRemove);
+  permission_context()->SetOriginHasExtendedPermissionForTesting(kTestOrigin);
+
+  const auto file_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("test_file.txt"));
+  const auto temp_path_info =
+      PathInfo(kTestPathInfo.path.AppendASCII("temp_file.txt"));
+
+  // Initialize read and write grants.
+  auto read_grant = permission_context()->GetReadPermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  auto write_grant = permission_context()->GetWritePermissionGrant(
+      kTestOrigin, file_path_info, HandleType::kFile, UserAction::kSave);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::GRANTED);
+  EXPECT_EQ(write_grant->GetStatus(), PermissionStatus::GRANTED);
+
+  // Downgrade the read grant to `DENIED` via `NotifyEntryRemoved()`.
+  permission_context()->NotifyEntryRemoved(kTestOrigin, file_path_info);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+
+  // Moving the handle invokes `PermissionGrantImpl::UpdateGrantPath()`.
+  // This operation must successfully update the grant path for a `DENIED`
+  // grant without triggering assertion failures in debug builds.
+  permission_context()->NotifyEntryMoved(kTestOrigin, file_path_info,
+                                         temp_path_info);
+  EXPECT_EQ(read_grant->GetPath(), temp_path_info.path);
+  EXPECT_EQ(read_grant->GetStatus(), PermissionStatus::DENIED);
+}
+
 // Tests that calling NotifyEntryRemoved with a directory path also revokes
 // read permission grants for descendants whose stored path differs only in
 // case from the removed directory. Native file pickers on case-insensitive