Skip to content

Commit

Permalink
[reland] Removing URLLoaderFactoryParams::factory_bound_access_patterns.
Browse files Browse the repository at this point in the history 
[This is a reland of https://crrev.com/c/2573677 after rebasing]

After https://crrev.com/c/2566090, the per-NetworkContext
OriginAccessList and the per-factory OriginAccessList cover the same
permissions.  This means that there is no need to maintain both and
therefore this CL removes the factory-bound access list.

Fixed: 936310
Tbr: Nasko Oskov <nasko@chromium.org>
Tbr: Devlin <rdevlin.cronin@chromium.org>
Tbr: Takashi Toyoshima <toyoshim@chromium.org>
Change-Id: I2d62beefdce2d913d61a185f8439841cac2b4288
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2644393
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846828}
  • Loading branch information
@anforowicz
anforowicz authored and Chromium LUCI CQ committed Jan 25, 2021
1 parent 45d22fc commit 631b644
Show file tree
Hide file tree
Showing 8 changed files with 5 additions and 144 deletions.
20 changes: 1 addition & 19 deletions extensions/browser/url_loader_factory_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,27 +95,9 @@ bool ShouldCreateSeparateFactoryForContentScripts(const Extension& extension) {
void OverrideFactoryParams(const Extension& extension,
FactoryUser factory_user,
network::mojom::URLLoaderFactoryParams* params) {
if (ShouldDisableCorb(extension, factory_user)) {
// TODO(lukasza): https://crbug.com/1016904: Use more granular CORB
// enforcement based on the specific |extension|'s permissions reflected
// in the |factory_bound_access_patterns| above.
if (ShouldDisableCorb(extension, factory_user))
params->is_corb_enabled = false;

// Setup factory bound allow list that overwrites per-profile common list to
// allow tab specific permissions only for this newly created factory.
//
// TODO(lukasza): Setting |factory_bound_access_patterns| together with
// |is_corb_enabled| seems accidental.
params->factory_bound_access_patterns =
network::mojom::CorsOriginAccessPatterns::New();
params->factory_bound_access_patterns->source_origin =
url::Origin::Create(extension.url());
params->factory_bound_access_patterns->allow_patterns =
CreateCorsOriginAccessAllowList(extension);
params->factory_bound_access_patterns->block_patterns =
CreateCorsOriginAccessBlockList(extension);
}

if (ShouldInspectIsolatedWorldOrigin(extension, factory_user))
params->ignore_isolated_world_origin = false;

Expand Down
6 changes: 1 addition & 5 deletions services/network/cors/cors_url_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ CorsURLLoader::CorsURLLoader(
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
mojom::URLLoaderFactory* network_loader_factory,
const OriginAccessList* origin_access_list,
const OriginAccessList* factory_bound_origin_access_list,
PreflightController* preflight_controller,
const base::flat_set<std::string>* allowed_exempt_headers,
bool allow_any_cors_exempt_header,
Expand All @@ -87,7 +86,6 @@ CorsURLLoader::CorsURLLoader(
forwarding_client_(std::move(client)),
traffic_annotation_(traffic_annotation),
origin_access_list_(origin_access_list),
factory_bound_origin_access_list_(factory_bound_origin_access_list),
preflight_controller_(preflight_controller),
allowed_exempt_headers_(allowed_exempt_headers),
skip_cors_enabled_scheme_check_(skip_cors_enabled_scheme_check),
Expand Down Expand Up @@ -576,10 +574,8 @@ bool CorsURLLoader::HasSpecialAccessToDestination() const {
case OriginAccessList::AccessState::kAllowed:
return true;
case OriginAccessList::AccessState::kBlocked:
return false;
case OriginAccessList::AccessState::kNotListed:
return factory_bound_origin_access_list_->CheckAccessState(request_) ==
OriginAccessList::AccessState::kAllowed;
return false;
}
}

Expand Down
6 changes: 2 additions & 4 deletions services/network/cors/cors_url_loader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoader
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
mojom::URLLoaderFactory* network_loader_factory,
const OriginAccessList* origin_access_list,
const OriginAccessList* factory_bound_origin_access_list,
PreflightController* preflight_controller,
const base::flat_set<std::string>* allowed_exempt_headers,
bool allow_any_cors_exempt_header,
Expand Down Expand Up @@ -118,8 +117,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoader

void SetCorsFlagIfNeeded();

// Returns true if request's origin has special access to the destination
// URL (via |origin_access_list_| and |factory_bound_origin_access_list_|).
// Returns true if request's origin has special access to the destination URL
// (via |origin_access_list_|).
bool HasSpecialAccessToDestination() const;

bool PassesTimingAllowOriginCheck(
Expand Down Expand Up @@ -184,7 +183,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoader

// Outlives |this|.
const OriginAccessList* const origin_access_list_;
const OriginAccessList* const factory_bound_origin_access_list_;
PreflightController* preflight_controller_;
const base::flat_set<std::string>* allowed_exempt_headers_;

Expand Down
12 changes: 1 addition & 11 deletions services/network/cors/cors_url_loader_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,6 @@ CorsURLLoaderFactory::CorsURLLoaderFactory(
// assigned IsolationInfo, to prevent cross-site information leaks.
DCHECK_EQ(mojom::kBrowserProcessId, process_id_);
}
factory_bound_origin_access_list_ = std::make_unique<OriginAccessList>();
if (params->factory_bound_access_patterns) {
factory_bound_origin_access_list_->SetAllowListForOrigin(
params->factory_bound_access_patterns->source_origin,
params->factory_bound_access_patterns->allow_patterns);
factory_bound_origin_access_list_->SetBlockListForOrigin(
params->factory_bound_access_patterns->source_origin,
params->factory_bound_access_patterns->block_patterns);
}

auto factory_override = std::move(params->factory_override);
auto network_loader_factory = std::make_unique<network::URLLoaderFactory>(
Expand Down Expand Up @@ -276,8 +267,7 @@ void CorsURLLoaderFactory::CreateLoaderAndStart(
factory_override_ &&
factory_override_->ShouldSkipCorsEnabledSchemeCheck(),
std::move(client), traffic_annotation, inner_url_loader_factory,
origin_access_list_, factory_bound_origin_access_list_.get(),
context_->cors_preflight_controller(),
origin_access_list_, context_->cors_preflight_controller(),
context_->cors_exempt_header_list(),
GetAllowAnyCorsExemptHeaderForBrowser(), isolation_info_);
auto* raw_loader = loader.get();
Expand Down
4 changes: 0 additions & 4 deletions services/network/cors/cors_url_loader_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
// it's safe.
const OriginAccessList* const origin_access_list_;

// Owns factory bound OriginAccessList that to have factory specific
// additional allowed access list.
std::unique_ptr<OriginAccessList> factory_bound_origin_access_list_;

static bool allow_external_preflights_for_testing_;

DISALLOW_COPY_AND_ASSIGN(CorsURLLoaderFactory);
Expand Down
79 changes: 0 additions & 79 deletions services/network/cors/cors_url_loader_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,18 +312,6 @@ class CorsURLLoaderTest : public testing::Test {
mojom::CorsOriginAccessMatchPriority::kHighPriority);
}

void AddFactoryBoundAllowListEntryForOrigin(
const url::Origin& source_origin,
const std::string& protocol,
const std::string& domain,
const mojom::CorsDomainMatchMode mode) {
factory_bound_allow_patterns_.push_back(mojom::CorsOriginPattern::New(
protocol, domain, /*port=*/0, mode,
mojom::CorsPortMatchMode::kAllowAnyPort,
mojom::CorsOriginAccessMatchPriority::kDefaultPriority));
ResetFactory(source_origin, kRendererProcessId);
}

static net::RedirectInfo CreateRedirectInfo(
int status_code,
base::StringPiece method,
Expand Down Expand Up @@ -357,16 +345,6 @@ class CorsURLLoaderTest : public testing::Test {
auto factory_params = network::mojom::URLLoaderFactoryParams::New();
if (initiator) {
factory_params->request_initiator_origin_lock = *initiator;
if (!initiator->opaque()) {
factory_params->factory_bound_access_patterns =
network::mojom::CorsOriginAccessPatterns::New();
factory_params->factory_bound_access_patterns->source_origin =
*initiator;
for (const auto& item : factory_bound_allow_patterns_) {
factory_params->factory_bound_access_patterns->allow_patterns
.push_back(item.Clone());
}
}
}
factory_params->is_trusted = is_trusted;
factory_params->process_id = process_id;
Expand Down Expand Up @@ -412,9 +390,6 @@ class CorsURLLoaderTest : public testing::Test {
std::unique_ptr<mojom::URLLoaderFactory> cors_url_loader_factory_;
mojo::Remote<mojom::URLLoaderFactory> cors_url_loader_factory_remote_;

// Factory bound origin access list for testing.
std::vector<mojom::CorsOriginPatternPtr> factory_bound_allow_patterns_;

std::unique_ptr<TestURLLoaderFactory> test_url_loader_factory_;
std::unique_ptr<mojo::Receiver<mojom::URLLoaderFactory>>
test_url_loader_factory_receiver_;
Expand Down Expand Up @@ -1618,60 +1593,6 @@ TEST_F(CorsURLLoaderTest, OriginAccessList_Blocked) {
EXPECT_EQ(net::ERR_FAILED, client().completion_status().error_code);
}

// CorsURLLoader manages two lists, per-NetworkContext list and
// per-URLLoaderFactory list. This test verifies if per-URLLoaderFactory list
// works.
TEST_F(CorsURLLoaderTest, OriginAccessList_AllowedByFactoryList) {
const GURL origin("https://example.com");
const GURL url("http://other.example.com/foo.png");

AddFactoryBoundAllowListEntryForOrigin(
url::Origin::Create(origin), url.scheme(), url.host(),
mojom::CorsDomainMatchMode::kDisallowSubdomains);

CreateLoaderAndStart(origin, url, mojom::RequestMode::kCors);
RunUntilCreateLoaderAndStartCalled();

NotifyLoaderClientOnReceiveResponse();
NotifyLoaderClientOnComplete(net::OK);

RunUntilComplete();

EXPECT_TRUE(IsNetworkLoaderStarted());
EXPECT_FALSE(client().has_received_redirect());
EXPECT_TRUE(client().has_received_response());
EXPECT_EQ(network::mojom::FetchResponseType::kBasic,
client().response_head()->response_type);
EXPECT_TRUE(client().has_received_completion());
EXPECT_EQ(net::OK, client().completion_status().error_code);
}

// Checks if CorsURLLoader can respect the per-NetworkContext block list.
TEST_F(CorsURLLoaderTest, OriginAccessList_AllowedByFactoryListButBlocked) {
const GURL origin("https://example.com");
const GURL url("http://other.example.com/foo.png");

AddFactoryBoundAllowListEntryForOrigin(
url::Origin::Create(origin), url.scheme(), url.host(),
mojom::CorsDomainMatchMode::kDisallowSubdomains);
AddBlockListEntryForOrigin(url::Origin::Create(origin), url.scheme(),
url.host(),
mojom::CorsDomainMatchMode::kDisallowSubdomains);

CreateLoaderAndStart(origin, url, mojom::RequestMode::kCors);
RunUntilCreateLoaderAndStartCalled();

NotifyLoaderClientOnReceiveResponse();

RunUntilComplete();

EXPECT_TRUE(IsNetworkLoaderStarted());
EXPECT_FALSE(client().has_received_redirect());
EXPECT_FALSE(client().has_received_response());
EXPECT_TRUE(client().has_received_completion());
EXPECT_EQ(net::ERR_FAILED, client().completion_status().error_code);
}

// Tests if OriginAccessList is actually used to decide response tainting.
TEST_F(CorsURLLoaderTest, OriginAccessList_NoCors) {
const GURL origin("https://example.com");
Expand Down
5 changes: 0 additions & 5 deletions services/network/public/mojom/network_context.mojom
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,11 +649,6 @@ struct URLLoaderFactoryParams {
// impact because of the extra process hops, so use should be minimized.
pending_remote<TrustedURLLoaderHeaderClient>? header_client;

// |factory_bound_access_patterns| are used for CORS checks in addition to
// the per-context allow patterns that is managed via NetworkContext
// interface. This still respects the per-context block lists.
CorsOriginAccessPatterns? factory_bound_access_patterns;

// Information used restrict access to identity information (like SameSite
// cookies) and to shard network resources, like the cache. If set, takes
// precedence over ResourceRequest::TrustedParams::IsolationInfo field
Expand Down
17 changes: 0 additions & 17 deletions services/network/url_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2191,23 +2191,6 @@ bool URLLoader::ShouldForceIgnoreSiteForCookies(
request.request_initiator.value(), request.url)) {
return true;
}
// Try again against the `factory_bound_access_patterns` to also cover the
// ActiveTab permission that the extension might have been granted.
if (factory_params_->factory_bound_access_patterns) {
cors::OriginAccessList factory_bound_origin_access_list;
factory_bound_origin_access_list.SetAllowListForOrigin(
factory_params_->factory_bound_access_patterns->source_origin,
factory_params_->factory_bound_access_patterns->allow_patterns);
factory_bound_origin_access_list.SetBlockListForOrigin(
factory_params_->factory_bound_access_patterns->source_origin,
factory_params_->factory_bound_access_patterns->block_patterns);
if (request.request_initiator.has_value() &&
cors::OriginAccessList::AccessState::kAllowed ==
factory_bound_origin_access_list.CheckAccessState(
request.request_initiator.value(), request.url)) {
return true;
}
}

// Convert `site_for_cookies` into an origin (an opaque origin if
// `net::SiteForCookies::IsNull()` returns true).
Expand Down

0 comments on commit 631b644

Please sign in to comment.