diff --git a/extensions/browser/url_loader_factory_manager.cc b/extensions/browser/url_loader_factory_manager.cc index 9c86954db4bd11..52923d2d28262c 100644 --- a/extensions/browser/url_loader_factory_manager.cc +++ b/extensions/browser/url_loader_factory_manager.cc @@ -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; diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc index 610180ea032620..874b4e5157df87 100644 --- a/services/network/cors/cors_url_loader.cc +++ b/services/network/cors/cors_url_loader.cc @@ -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* allowed_exempt_headers, bool allow_any_cors_exempt_header, @@ -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), @@ -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; } } diff --git a/services/network/cors/cors_url_loader.h b/services/network/cors/cors_url_loader.h index 4fa196399a10a6..6b72ae211a9ce5 100644 --- a/services/network/cors/cors_url_loader.h +++ b/services/network/cors/cors_url_loader.h @@ -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* allowed_exempt_headers, bool allow_any_cors_exempt_header, @@ -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( @@ -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* allowed_exempt_headers_; diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc index 7e4d57cf7f2f8e..669995d9060d5d 100644 --- a/services/network/cors/cors_url_loader_factory.cc +++ b/services/network/cors/cors_url_loader_factory.cc @@ -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(); - 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( @@ -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(); diff --git a/services/network/cors/cors_url_loader_factory.h b/services/network/cors/cors_url_loader_factory.h index e58692fc0dd292..2400c2b96a87ff 100644 --- a/services/network/cors/cors_url_loader_factory.h +++ b/services/network/cors/cors_url_loader_factory.h @@ -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 factory_bound_origin_access_list_; - static bool allow_external_preflights_for_testing_; DISALLOW_COPY_AND_ASSIGN(CorsURLLoaderFactory); diff --git a/services/network/cors/cors_url_loader_unittest.cc b/services/network/cors/cors_url_loader_unittest.cc index ed4ecad8080a8b..e011a13c3af447 100644 --- a/services/network/cors/cors_url_loader_unittest.cc +++ b/services/network/cors/cors_url_loader_unittest.cc @@ -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, @@ -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; @@ -412,9 +390,6 @@ class CorsURLLoaderTest : public testing::Test { std::unique_ptr cors_url_loader_factory_; mojo::Remote cors_url_loader_factory_remote_; - // Factory bound origin access list for testing. - std::vector factory_bound_allow_patterns_; - std::unique_ptr test_url_loader_factory_; std::unique_ptr> test_url_loader_factory_receiver_; @@ -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"); diff --git a/services/network/public/mojom/network_context.mojom b/services/network/public/mojom/network_context.mojom index 294b04445bd375..6b6550525ef32b 100644 --- a/services/network/public/mojom/network_context.mojom +++ b/services/network/public/mojom/network_context.mojom @@ -649,11 +649,6 @@ struct URLLoaderFactoryParams { // impact because of the extra process hops, so use should be minimized. pending_remote? 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 diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc index 052e3274eea9e1..00339d4537993b 100644 --- a/services/network/url_loader.cc +++ b/services/network/url_loader.cc @@ -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).