0

FS: Remove flag to treat opaque origins as null origins in comparators

This feature has been enabled by default since being added in M111 and
is removable as per the feature flag guidelines. It was added in
https://crrev.com/c/4122683

Bug: 1396116
Change-Id: I34bd1475e14b9d304a0935b19ff0bcc55c4eb2ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4762585
Reviewed-by: Christine Smith <christinesm@chromium.org>
Commit-Queue: Christine Smith <christinesm@chromium.org>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1181556}
This commit is contained in:
Austin Sullivan
2023-08-09 17:15:24 +00:00
committed by Chromium LUCI CQ
parent 1d451c12fb
commit 9406fc716f
7 changed files with 20 additions and 72 deletions

@ -51,7 +51,6 @@
#include "net/base/filename_util.h"
#include "services/network/public/cpp/is_potentially_trustworthy.h"
#include "storage/browser/file_system/file_system_context.h"
#include "storage/browser/file_system/file_system_features.h"
#include "storage/browser/file_system/file_system_operation_runner.h"
#include "storage/browser/file_system/file_system_url.h"
#include "storage/browser/file_system/file_system_util.h"
@ -1571,11 +1570,7 @@ storage::FileSystemURL FileSystemAccessManagerImpl::CreateFileSystemURLFromPath(
const base::FilePath& path) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return context()->CreateCrackedFileSystemURL(
base::FeatureList::IsEnabled(
storage::features::
kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin)
? blink::StorageKey()
: opaque_origin_for_non_sandboxed_filesystemurls_,
blink::StorageKey(),
path_type == PathType::kLocal ? storage::kFileSystemTypeLocal
: storage::kFileSystemTypeExternal,
path);

@ -630,19 +630,6 @@ class CONTENT_EXPORT FileSystemAccessManagerImpl
absl::optional<FileSystemChooser::ResultEntry>
auto_file_picker_result_for_test_ GUARDED_BY_CONTEXT(sequence_checker_);
// TODO(https://crbug.com/1396116): Remove this hack when removing the
// `kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin` feature flag.
//
// A StorageKey containing an arbitrary, unique, randomly-generated opaque
// origin. ChromeOS file system backends run security checks on the assumption
// that all FileSystemURLs of non-sandboxed file systems must have an opaque
// origin. Using a default-constructed StorageKey will create a random nonce,
// making origin comparison checks between two FileSystemURLs with
// default-constructed StorageKeys fail. Always using this StorageKey ensures
// that FileSystemURL::operator== will always return true for two
// FileSystemURLs which point to the same file.
blink::StorageKey opaque_origin_for_non_sandboxed_filesystemurls_;
base::WeakPtrFactory<FileSystemAccessManagerImpl> weak_factory_
GUARDED_BY_CONTEXT(sequence_checker_){this};
};

@ -17,11 +17,4 @@ BASE_FEATURE(kIncognitoFileSystemContextForTesting,
"IncognitoFileSystemContextForTesting",
base::FEATURE_DISABLED_BY_DEFAULT);
// TODO(https://crbug.com/1396116): Remove this eventually.
// When enabled, FileSystemURL comparators will treat opaque origins as a null
// state. See https://crbug.com/1396116.
BASE_FEATURE(kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin,
"FileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin",
base::FEATURE_ENABLED_BY_DEFAULT);
} // namespace storage::features

@ -16,9 +16,6 @@ BASE_DECLARE_FEATURE(kEnablePersistentFilesystemInIncognito);
COMPONENT_EXPORT(STORAGE_BROWSER)
BASE_DECLARE_FEATURE(kIncognitoFileSystemContextForTesting);
COMPONENT_EXPORT(STORAGE_BROWSER)
BASE_DECLARE_FEATURE(kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin);
} // namespace storage::features
#endif // STORAGE_BROWSER_FILE_SYSTEM_FILE_SYSTEM_FEATURES_H_

@ -11,7 +11,6 @@
#include <tuple>
#include <utility>
#include "base/feature_list.h"
#include "base/functional/bind.h"
#include "base/functional/callback.h"
#include "base/functional/callback_helpers.h"
@ -24,7 +23,6 @@
#include "storage/browser/file_system/file_observers.h"
#include "storage/browser/file_system/file_system_backend.h"
#include "storage/browser/file_system/file_system_context.h"
#include "storage/browser/file_system/file_system_features.h"
#include "storage/browser/file_system/file_system_file_util.h"
#include "storage/browser/file_system/file_system_util.h"
#include "storage/browser/file_system/remove_operation_delegate.h"
@ -343,13 +341,8 @@ void FileSystemOperationImpl::CopyFileLocal(
// the two URLs are mounted in two different isolated file systems. As long
// as their origin and type are the same, they are part of the same file
// system, and local operations are allowed. See https://crbug.com/1396116.
if (base::FeatureList::IsEnabled(
features::kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin)) {
DCHECK(src_url.origin() == dest_url.origin() ||
(src_url.origin().opaque() && dest_url.origin().opaque()));
} else {
DCHECK_EQ(src_url.origin(), dest_url.origin());
}
DCHECK(src_url.origin() == dest_url.origin() ||
(src_url.origin().opaque() && dest_url.origin().opaque()));
DCHECK_EQ(src_url.type(), dest_url.type());
auto split_callback = base::SplitOnceCallback(std::move(callback));
@ -371,13 +364,8 @@ void FileSystemOperationImpl::MoveFileLocal(const FileSystemURL& src_url,
// the two URLs are mounted in two different isolated file systems. As long
// as their origin and type are the same, they are part of the same file
// system, and local operations are allowed. See https://crbug.com/1396116.
if (base::FeatureList::IsEnabled(
features::kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin)) {
DCHECK(src_url.origin() == dest_url.origin() ||
(src_url.origin().opaque() && dest_url.origin().opaque()));
} else {
DCHECK_EQ(src_url.origin(), dest_url.origin());
}
DCHECK(src_url.origin() == dest_url.origin() ||
(src_url.origin().opaque() && dest_url.origin().opaque()));
DCHECK_EQ(src_url.type(), dest_url.type());
auto split_callback = base::SplitOnceCallback(std::move(callback));

@ -7,11 +7,9 @@
#include <sstream>
#include "base/check.h"
#include "base/feature_list.h"
#include "base/files/safe_base_name.h"
#include "base/strings/escape.h"
#include "base/strings/string_util.h"
#include "storage/browser/file_system/file_system_features.h"
#include "storage/browser/file_system/file_system_util.h"
#include "storage/common/file_system/file_system_types.h"
#include "storage/common/file_system/file_system_util.h"
@ -30,15 +28,12 @@ bool AreSameStorageKey(const FileSystemURL& a, const FileSystemURL& b) {
// systems. This leads to unexpected behavior when comparing two non-sandboxed
// FileSystemURLs which differ only in the nonce of their default-constructed
// StorageKey.
return base::FeatureList::IsEnabled(
features::kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin)
? a.storage_key() == b.storage_key() ||
(a.type() == b.type() &&
(a.type() == storage::kFileSystemTypeExternal ||
a.type() == storage::kFileSystemTypeLocal) &&
a.storage_key().origin().opaque() &&
b.storage_key().origin().opaque())
: a.storage_key() == b.storage_key();
return a.storage_key() == b.storage_key() ||
(a.type() == b.type() &&
(a.type() == storage::kFileSystemTypeExternal ||
a.type() == storage::kFileSystemTypeLocal) &&
a.storage_key().origin().opaque() &&
b.storage_key().origin().opaque());
}
} // namespace
@ -277,11 +272,7 @@ bool FileSystemURL::IsParent(const FileSystemURL& child) const {
bool FileSystemURL::IsInSameFileSystem(const FileSystemURL& other) const {
// Invalid FileSystemURLs should never be considered of the same file system.
bool is_maybe_valid =
!base::FeatureList::IsEnabled(
features::kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin) ||
(is_valid() && other.is_valid());
return AreSameStorageKey(*this, other) && is_maybe_valid &&
return AreSameStorageKey(*this, other) && is_valid() && other.is_valid() &&
type() == other.type() && filesystem_id() == other.filesystem_id() &&
bucket() == other.bucket();
}

@ -426,18 +426,15 @@ TEST(FileSystemURLTest, IsInSameFileSystem) {
kFileSystemTypeTemporary, base::FilePath::FromUTF8Unsafe("a"));
EXPECT_FALSE(url_foo_temp_a.IsInSameFileSystem(url_foo_temp_c));
if (base::FeatureList::IsEnabled(
features::kFileSystemURLComparatorsTreatOpaqueOriginAsNoOrigin)) {
// Test that opaque origins with differing nonces are considered to be in
// the same file system.
EXPECT_NE(url_opaque_a, url_opaque_b);
EXPECT_TRUE(url_opaque_a.IsInSameFileSystem(url_opaque_b));
// Test that opaque origins with differing nonces are considered to be in
// the same file system.
EXPECT_NE(url_opaque_a, url_opaque_b);
EXPECT_TRUE(url_opaque_a.IsInSameFileSystem(url_opaque_b));
// Test that identical, invalid URLs are considered not to be in the same
// file system.
EXPECT_EQ(url_invalid_a, url_invalid_b);
EXPECT_FALSE(url_invalid_a.IsInSameFileSystem(url_invalid_b));
}
// Test that identical, invalid URLs are considered not to be in the same
// file system.
EXPECT_EQ(url_invalid_a, url_invalid_b);
EXPECT_FALSE(url_invalid_a.IsInSameFileSystem(url_invalid_b));
}
TEST(FileSystemURLTest, ValidAfterMoves) {