From cf934a20a7371e022f81d0db8d96731446d6055f Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Thu, 16 Nov 2023 18:19:27 -0500 Subject: [PATCH] Fix cookie exclusion for fetch CORS pre-flight requests (fixes #3596) Cookies (and other credentials) will be excluded when appropriate by downgrading |credentials_mode| from kSameOrigin to kOmit. Improve logic for Origin header inclusion, including a fix for Referrer/Origin calculation in URLRequestJob::ComputeReferrerForPolicy when used with custom standard schemes. Specify correct CookiePartitionKeyCollection when loading cookies. To test: - Run tests from https://browseraudit.com/ with and without `--disable-request-handling-for-testing`. Results are the same. - Run `ceftests --gtest_filter=CorsTest.*`. --- libcef/browser/net_service/cookie_helper.cc | 10 +- .../net_service/proxy_url_loader_factory.cc | 99 +++++++++++++++---- libcef/common/app_manager.cc | 2 +- libcef/common/app_manager.h | 2 +- libcef/common/net/scheme_registration.cc | 26 ++--- libcef/common/scheme_registrar_impl.cc | 4 + patch/patch.cfg | 6 ++ patch/patches/net_url_request_3596.patch | 39 ++++++++ tests/ceftests/cors_unittest.cc | 9 ++ tests/ceftests/server_unittest.cc | 1 + 10 files changed, 166 insertions(+), 32 deletions(-) create mode 100644 patch/patches/net_url_request_3596.patch diff --git a/libcef/browser/net_service/cookie_helper.cc b/libcef/browser/net_service/cookie_helper.cc index ba7e8e68a..77b95375c 100644 --- a/libcef/browser/net_service/cookie_helper.cc +++ b/libcef/browser/net_service/cookie_helper.cc @@ -249,11 +249,19 @@ void LoadCookies(const CefBrowserContext::Getter& browser_context_getter, return; } + net::CookiePartitionKeyCollection partition_key_collection; + if (request.trusted_params.has_value() && + !request.trusted_params->isolation_info.IsEmpty()) { + partition_key_collection = net::CookiePartitionKeyCollection::FromOptional( + net::CookiePartitionKey::FromNetworkIsolationKey( + request.trusted_params->isolation_info.network_isolation_key())); + } + CEF_POST_TASK( CEF_UIT, base::BindOnce(LoadCookiesOnUIThread, browser_context_getter, request.url, GetCookieOptions(request, /*for_loading_cookies=*/true), - net::CookiePartitionKeyCollection(), allow_cookie_callback, + std::move(partition_key_collection), allow_cookie_callback, std::move(done_callback))); } diff --git a/libcef/browser/net_service/proxy_url_loader_factory.cc b/libcef/browser/net_service/proxy_url_loader_factory.cc index 9394bbbbf..b52d03ad4 100644 --- a/libcef/browser/net_service/proxy_url_loader_factory.cc +++ b/libcef/browser/net_service/proxy_url_loader_factory.cc @@ -22,6 +22,7 @@ #include "content/public/browser/render_frame_host.h" #include "content/public/browser/resource_context.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/referrer.h" #include "mojo/public/cpp/base/big_buffer.h" #include "mojo/public/cpp/bindings/receiver.h" #include "net/http/http_status_code.h" @@ -30,6 +31,7 @@ #include "services/network/public/cpp/cors/cors.h" #include "services/network/public/cpp/features.h" #include "services/network/public/mojom/early_hints.mojom.h" +#include "third_party/blink/public/common/loader/referrer_utils.h" namespace net_service { @@ -65,6 +67,21 @@ bool DisableRequestHandlingForTesting() { return disabled; } +// Match logic in devtools_url_loader_interceptor.cc +// InterceptionJob::CalculateResponseTainting. +network::mojom::FetchResponseType CalculateResponseTainting( + bool should_check_cors, + network::mojom::RequestMode mode, + bool tainted_origin) { + if (should_check_cors) { + return network::mojom::FetchResponseType::kCors; + } + if (mode == network::mojom::RequestMode::kNoCors && tainted_origin) { + return network::mojom::FetchResponseType::kOpaque; + } + return network::mojom::FetchResponseType::kBasic; +} + } // namespace // Owns all of the ProxyURLLoaderFactorys for a given BrowserContext. Since @@ -451,25 +468,71 @@ void InterceptedRequest::Restart() { current_request_uses_header_client_ = factory_->url_loader_header_client_receiver_.is_bound(); - if (request_.request_initiator && - network::cors::ShouldCheckCors(request_.url, request_.request_initiator, - request_.mode)) { - if (scheme::IsCorsEnabledScheme(request_.url.scheme())) { - // Add the Origin header for CORS-enabled scheme requests. - request_.headers.SetHeaderIfMissing( - net::HttpRequestHeaders::kOrigin, - request_.request_initiator->Serialize()); - } else if (!HasCrossOriginWhitelistEntry( - *request_.request_initiator, - url::Origin::Create(request_.url))) { - // Fail requests if a CORS check is required and the scheme is not CORS - // enabled. This matches the error condition that would be generated by - // CorsURLLoader::StartRequest in the network process. - SendErrorStatusAndCompleteImmediately( - network::URLLoaderCompletionStatus(network::CorsErrorStatus( - network::mojom::CorsError::kCorsDisabledScheme))); - return; + const bool is_cross_origin = + request_.request_initiator && + !request_.request_initiator->IsSameOriginWith(request_.url); + const bool is_cors_enabled_scheme = + scheme::IsCorsEnabledScheme(request_.url.scheme()); + + // Match logic in network::cors::ShouldCheckCors. + bool should_check_cors = + is_cross_origin && + request_.mode != network::mojom::RequestMode::kNavigate && + request_.mode != network::mojom::RequestMode::kNoCors; + + if (should_check_cors && !is_cors_enabled_scheme && + !HasCrossOriginWhitelistEntry(*request_.request_initiator, + url::Origin::Create(request_.url))) { + // Fail requests if a CORS check is required and the scheme is not CORS + // enabled. This matches the error condition that would be generated by + // CorsURLLoader::StartRequest in the network process. + SendErrorStatusAndCompleteImmediately( + network::URLLoaderCompletionStatus(network::CorsErrorStatus( + network::mojom::CorsError::kCorsDisabledScheme))); + return; + } + + // Maybe update |credentials_mode| for fetch requests. + if (request_.credentials_mode == + network::mojom::CredentialsMode::kSameOrigin) { + // Match logic in devtools_url_loader_interceptor.cc + // InterceptionJob::FollowRedirect. + bool tainted_origin = false; + if (redirect_in_progress_ && request_.request_initiator && + !url::IsSameOriginWith(request_.url, original_url_) && + !request_.request_initiator->IsSameOriginWith(original_url_)) { + tainted_origin = true; } + + // Match logic in CorsURLLoader::StartNetworkRequest. + const auto response_tainting = CalculateResponseTainting( + should_check_cors, request_.mode, tainted_origin); + request_.credentials_mode = + network::cors::CalculateCredentialsFlag(request_.credentials_mode, + response_tainting) + ? network::mojom::CredentialsMode::kInclude + : network::mojom::CredentialsMode::kOmit; + } + + const bool should_add_origin_header = + // Cross-origin requests that are not kNavigate nor kNoCors. + should_check_cors || + // Same-origin requests except for GET and HEAD. + (!is_cross_origin && + request_.method != net::HttpRequestHeaders::kGetMethod && + request_.method != net::HttpRequestHeaders::kHeadMethod); + + if (should_add_origin_header) { + // Match logic in navigation_request.cc AddAdditionalRequestHeaders. + url::Origin origin_header_value = + request_.request_initiator.value_or(url::Origin()); + origin_header_value = content::Referrer::SanitizeOriginForRequest( + request_.url, origin_header_value, + blink::ReferrerUtils::NetToMojoReferrerPolicy( + request_.referrer_policy)); + + request_.headers.SetHeaderIfMissing(net::HttpRequestHeaders::kOrigin, + origin_header_value.Serialize()); } const GURL original_url = request_.url; diff --git a/libcef/common/app_manager.cc b/libcef/common/app_manager.cc index 167393a7e..0eadb2443 100644 --- a/libcef/common/app_manager.cc +++ b/libcef/common/app_manager.cc @@ -39,7 +39,7 @@ CefAppManager::~CefAppManager() { g_manager = nullptr; } -void CefAppManager::AddCustomScheme(CefSchemeInfo* scheme_info) { +void CefAppManager::AddCustomScheme(const CefSchemeInfo* scheme_info) { DCHECK(!scheme_info_list_locked_); scheme_info_list_.push_back(*scheme_info); diff --git a/libcef/common/app_manager.h b/libcef/common/app_manager.h index 94ea96794..e0e3e9aaf 100644 --- a/libcef/common/app_manager.h +++ b/libcef/common/app_manager.h @@ -36,7 +36,7 @@ class CefAppManager { // (url/url_util.h) via ContentClient::AddAdditionalSchemes which calls // AddCustomScheme, and second with Blink (SchemeRegistry) via // ContentRendererClient::WebKitInitialized which calls GetCustomSchemes. - void AddCustomScheme(CefSchemeInfo* scheme_info); + void AddCustomScheme(const CefSchemeInfo* scheme_info); bool HasCustomScheme(const std::string& scheme_name); using SchemeInfoList = std::list; diff --git a/libcef/common/net/scheme_registration.cc b/libcef/common/net/scheme_registration.cc index 75569767e..60f78fa54 100644 --- a/libcef/common/net/scheme_registration.cc +++ b/libcef/common/net/scheme_registration.cc @@ -40,22 +40,26 @@ void AddInternalSchemes(content::ContentClient::Schemes* schemes) { // with Blink only. for (size_t i = 0; i < sizeof(internal_schemes) / sizeof(internal_schemes[0]); ++i) { - if (internal_schemes[i].is_standard) { - schemes->standard_schemes.push_back(internal_schemes[i].scheme_name); + const auto& scheme = internal_schemes[i]; + if (scheme.is_standard) { + schemes->standard_schemes.push_back(scheme.scheme_name); + if (!scheme.is_local && !scheme.is_display_isolated) { + schemes->referrer_schemes.push_back(scheme.scheme_name); + } } - if (internal_schemes[i].is_local) { - schemes->local_schemes.push_back(internal_schemes[i].scheme_name); + if (scheme.is_local) { + schemes->local_schemes.push_back(scheme.scheme_name); } - if (internal_schemes[i].is_secure) { - schemes->secure_schemes.push_back(internal_schemes[i].scheme_name); + if (scheme.is_secure) { + schemes->secure_schemes.push_back(scheme.scheme_name); } - if (internal_schemes[i].is_cors_enabled) { - schemes->cors_enabled_schemes.push_back(internal_schemes[i].scheme_name); + if (scheme.is_cors_enabled) { + schemes->cors_enabled_schemes.push_back(scheme.scheme_name); } - if (internal_schemes[i].is_csp_bypassing) { - schemes->csp_bypassing_schemes.push_back(internal_schemes[i].scheme_name); + if (scheme.is_csp_bypassing) { + schemes->csp_bypassing_schemes.push_back(scheme.scheme_name); } - CefAppManager::Get()->AddCustomScheme(&internal_schemes[i]); + CefAppManager::Get()->AddCustomScheme(&scheme); } } diff --git a/libcef/common/scheme_registrar_impl.cc b/libcef/common/scheme_registrar_impl.cc index a31734f79..e33e5020d 100644 --- a/libcef/common/scheme_registrar_impl.cc +++ b/libcef/common/scheme_registrar_impl.cc @@ -49,6 +49,9 @@ bool CefSchemeRegistrarImpl::AddCustomScheme(const CefString& scheme_name, // with Blink only. if (is_standard) { schemes_.standard_schemes.push_back(scheme); + if (!is_local && !is_display_isolated) { + schemes_.referrer_schemes.push_back(scheme); + } } if (is_local) { schemes_.local_schemes.push_back(scheme); @@ -74,6 +77,7 @@ bool CefSchemeRegistrarImpl::AddCustomScheme(const CefString& scheme_name, void CefSchemeRegistrarImpl::GetSchemes( content::ContentClient::Schemes* schemes) { AppendArray(schemes_.standard_schemes, &schemes->standard_schemes); + AppendArray(schemes_.referrer_schemes, &schemes->referrer_schemes); AppendArray(schemes_.local_schemes, &schemes->local_schemes); AppendArray(schemes_.secure_schemes, &schemes->secure_schemes); AppendArray(schemes_.cors_enabled_schemes, &schemes->cors_enabled_schemes); diff --git a/patch/patch.cfg b/patch/patch.cfg index e451e8ae4..a5de37909 100644 --- a/patch/patch.cfg +++ b/patch/patch.cfg @@ -678,5 +678,11 @@ patches = [ # https://chromium-review.googlesource.com/c/chromium/src/+/4829483 # https://bugs.chromium.org/p/chromium/issues/detail?id=1470837#c22 'name': 'rfh_navigation_4829483' + }, + { + # Fix Referrer & Origin calculation for secure referrer (custom standard + # scheme) with insecure destination. + # https://github.com/chromiumembedded/cef/issues/3596 + 'name': 'net_url_request_3596' } ] diff --git a/patch/patches/net_url_request_3596.patch b/patch/patches/net_url_request_3596.patch new file mode 100644 index 000000000..0b92b43ec --- /dev/null +++ b/patch/patches/net_url_request_3596.patch @@ -0,0 +1,39 @@ +diff --git net/url_request/url_request_job.cc net/url_request/url_request_job.cc +index 0e585570d3fa6..7158d4e8df44e 100644 +--- net/url_request/url_request_job.cc ++++ net/url_request/url_request_job.cc +@@ -34,6 +34,7 @@ + #include "net/ssl/ssl_private_key.h" + #include "net/url_request/redirect_util.h" + #include "net/url_request/url_request_context.h" ++#include "url/url_util.h" + + namespace net { + +@@ -46,6 +47,16 @@ base::Value::Dict SourceStreamSetParams(SourceStream* source_stream) { + return event_params; + } + ++bool IsSecureScheme(const GURL& url) { ++ if (!url.has_scheme()) { ++ return false; ++ } ++ if (GURL::SchemeIsCryptographic(url.scheme_piece())) { ++ return true; ++ } ++ return base::Contains(url::GetSecureSchemes(), url.scheme_piece()); ++} ++ + } // namespace + + // Each SourceStreams own the previous SourceStream in the chain, but the +@@ -334,8 +345,7 @@ GURL URLRequestJob::ComputeReferrerForPolicy( + } + + bool secure_referrer_but_insecure_destination = +- original_referrer.SchemeIsCryptographic() && +- !destination.SchemeIsCryptographic(); ++ IsSecureScheme(original_referrer) && !IsSecureScheme(destination); + + switch (policy) { + case ReferrerPolicy::CLEAR_ON_TRANSITION_FROM_SECURE_TO_INSECURE: diff --git a/tests/ceftests/cors_unittest.cc b/tests/ceftests/cors_unittest.cc index 515f2f7e8..9cb06b71a 100644 --- a/tests/ceftests/cors_unittest.cc +++ b/tests/ceftests/cors_unittest.cc @@ -4,6 +4,7 @@ #include #include +#include #include #include "include/base/cef_callback.h" @@ -67,6 +68,14 @@ enum class HandlerType { std::string GetOrigin(HandlerType handler) { switch (handler) { case HandlerType::SERVER: + // TODO: Only call test_server::GetOrigin() after test server + // initialization. + if (!kUseHttpsServerScheme) { + std::stringstream ss; + ss << "http://" << test_server::kHttpServerAddress << ":" + << test_server::kHttpServerPort; + return ss.str(); + } return test_server::GetOrigin(kUseHttpsServerScheme); case HandlerType::HTTP_SCHEME: // Use HTTPS because requests from HTTP to the loopback address will be diff --git a/tests/ceftests/server_unittest.cc b/tests/ceftests/server_unittest.cc index 96642e05e..b260fa5be 100644 --- a/tests/ceftests/server_unittest.cc +++ b/tests/ceftests/server_unittest.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include "include/base/cef_callback.h" #include "include/base/cef_ref_counted.h"