Commit 72e9685e authored by Takashi Toyoshima's avatar Takashi Toyoshima Committed by Commit Bot

OOR-CORS: Support cross-origin redirect on webRequest::onBeforeRequest

Chrome Extensions can generate internal redirects in
webRequest::onBeforeRequest event handler.
See Life cycle of requests below.
https://developer.chrome.com/extensions/webRequest

Without OOR-CORS, WebRequestProxyingURLLoaderFactory generates virtual
response for Blink, and Blink-CORS handles the redirects. Simply said,
it rewrites the Origin header to be 'null' for cross-origin redirects.

Detailed steps are:
 1. A certain request is made by Blink.
 2. webRequest::onBeforeRequest intercepts the request, and generates
    an internal redirect response with status 307.
 3. Blink receives the generated response and Blink-CORS handles
    cross-origin redirects if it is needed, e.g. using Origin: null

But, if OOR-CORS is enabled, Blink does nothing. Detailed steps are:
 1. and 2. is ditto
 3. Blink receives the generated response and Blink-CORS is disabled
    and does nothing. Just new request for the redirect is made.
 4. The request is handled in NetworkService, with OOR-CORS. It sets
    Origin header for such cross-origin request, but the value is
    based on the request initiator's origin.

So the proxy needs to craft its ResourceRequest so that the OOR-CORS
can set a proper Origin header, null for such internal redirect cases.

My approach in this patch set is:
 1. Set null origin to the ResourceRequest.request_initiator to pretend
    the retained origin flag is set.
    See https://fetch.spec.whatwg.org/#concept-request-tainted-origin
 2. But WebRequestInfo is initialized with copied ResourceRequest that
    has the original request_initiator.

2. is needed for webRequest events. See API document below.
https://developer.chrome.com/extensions/webRequest#event-onBeforeRequest
That says the initiator does not change through redirects.

Following tests in ExtensionWebRequestApiTest.WebRequestBlocking failed
if OOR-CORS is enabled without this patch.
- crossOriginAnonymousRedirect()
- crossOriginCredentialedRedirect()
- syncXhrsFromOurselfAreInvisible()

And this patch fixes them to pass.

Bug: 909633
Change-Id: I755db213256605c72015ab21c845e649bc35e319
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1503273
Auto-Submit: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: default avatarJohn Abd-El-Malek <jam@chromium.org>
Reviewed-by: default avatarKaran Bhatia <karandeepb@chromium.org>
Reviewed-by: default avatarYutaka Hirano <yhirano@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638724}
parent 4ac69276
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/debug/alias.h" #include "base/debug/alias.h"
#include "base/debug/dump_without_crashing.h" #include "base/debug/dump_without_crashing.h"
#include "base/feature_list.h"
#include "base/strings/stringprintf.h" #include "base/strings/stringprintf.h"
#include "base/task/post_task.h" #include "base/task/post_task.h"
#include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_task_traits.h"
...@@ -19,7 +20,9 @@ ...@@ -19,7 +20,9 @@
#include "extensions/browser/extension_navigation_ui_data.h" #include "extensions/browser/extension_navigation_ui_data.h"
#include "extensions/common/manifest_handlers/web_accessible_resources_info.h" #include "extensions/common/manifest_handlers/web_accessible_resources_info.h"
#include "net/http/http_util.h" #include "net/http/http_util.h"
#include "services/network/public/cpp/features.h"
#include "third_party/blink/public/platform/resource_request_blocked_reason.h" #include "third_party/blink/public/platform/resource_request_blocked_reason.h"
#include "url/origin.h"
namespace extensions { namespace extensions {
...@@ -36,6 +39,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( ...@@ -36,6 +39,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
network::mojom::URLLoaderClientPtr client) network::mojom::URLLoaderClientPtr client)
: factory_(factory), : factory_(factory),
request_(request), request_(request),
original_initiator_(request.request_initiator),
is_download_(is_download), is_download_(is_download),
request_id_(request_id), request_id_(request_id),
network_service_request_id_(network_service_request_id), network_service_request_id_(network_service_request_id),
...@@ -79,11 +83,16 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::Restart() { ...@@ -79,11 +83,16 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::Restart() {
request_completed_ = false; request_completed_ = false;
// Derive a new WebRequestInfo value any time |Restart()| is called, because // Derive a new WebRequestInfo value any time |Restart()| is called, because
// the details in |request_| may have changed e.g. if we've been redirected. // the details in |request_| may have changed e.g. if we've been redirected.
// |request_initiator| can be modified on redirects, but we keep the original
// for |initiator| in the event. See also
// https://developer.chrome.com/extensions/webRequest#event-onBeforeRequest.
network::ResourceRequest request_for_info = request_;
request_for_info.request_initiator = original_initiator_;
info_.emplace( info_.emplace(
request_id_, factory_->render_process_id_, request_.render_frame_id, request_id_, factory_->render_process_id_, request_.render_frame_id,
factory_->navigation_ui_data_ ? factory_->navigation_ui_data_->DeepCopy() factory_->navigation_ui_data_ ? factory_->navigation_ui_data_->DeepCopy()
: nullptr, : nullptr,
routing_id_, factory_->resource_context_, request_, is_download_, routing_id_, factory_->resource_context_, request_for_info, is_download_,
!(options_ & network::mojom::kURLLoadOptionSynchronous)); !(options_ & network::mojom::kURLLoadOptionSynchronous));
current_request_uses_header_client_ = current_request_uses_header_client_ =
...@@ -359,19 +368,37 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: ...@@ -359,19 +368,37 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::
"Location: %s\n" "Location: %s\n"
"Non-Authoritative-Reason: WebRequest API\n\n", "Non-Authoritative-Reason: WebRequest API\n\n",
kInternalRedirectStatusCode, redirect_url_.spec().c_str()); kInternalRedirectStatusCode, redirect_url_.spec().c_str());
std::string http_origin;
if (request_.headers.GetHeader("Origin", &http_origin)) { if (base::FeatureList::IsEnabled(network::features::kOutOfBlinkCors)) {
// Cross-origin requests need to modify the Origin header to 'null'. Since
// CorsURLLoader sets |request_initiator| to the Origin request header in
// NetworkService, we need to modify |request_initiator| here to craft the
// Origin header indirectly.
// Following checks implement the step 10 of "4.4. HTTP-redirect fetch",
// https://fetch.spec.whatwg.org/#http-redirect-fetch
if (request_.request_initiator &&
(!url::Origin::Create(redirect_url_)
.IsSameOriginWith(url::Origin::Create(request_.url)) &&
!request_.request_initiator->IsSameOriginWith(
url::Origin::Create(request_.url)))) {
// Reset the initiator to pretend tainted origin flag of the spec is set.
request_.request_initiator = url::Origin();
}
} else {
// If this redirect is used in a cross-origin request, add CORS headers to // If this redirect is used in a cross-origin request, add CORS headers to
// make sure that the redirect gets through. Note that the destination URL // make sure that the redirect gets through the Blink CORS. Note that the
// is still subject to the usual CORS policy, i.e. the resource will only // destination URL is still subject to the usual CORS policy, i.e. the
// be available to web pages if the server serves the response with the // resource will only be available to web pages if the server serves the
// required CORS response headers. // response with the required CORS response headers. Matches the behavior in
// Matches the behavior in url_request_redirect_job.cc. // url_request_redirect_job.cc.
headers += base::StringPrintf( std::string http_origin;
"\n" if (request_.headers.GetHeader("Origin", &http_origin)) {
"Access-Control-Allow-Origin: %s\n" headers += base::StringPrintf(
"Access-Control-Allow-Credentials: true", "\n"
http_origin.c_str()); "Access-Control-Allow-Origin: %s\n"
"Access-Control-Allow-Credentials: true",
http_origin.c_str());
}
} }
head.headers = base::MakeRefCounted<net::HttpResponseHeaders>( head.headers = base::MakeRefCounted<net::HttpResponseHeaders>(
net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.length())); net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.length()));
......
...@@ -124,6 +124,7 @@ class WebRequestProxyingURLLoaderFactory ...@@ -124,6 +124,7 @@ class WebRequestProxyingURLLoaderFactory
WebRequestProxyingURLLoaderFactory* const factory_; WebRequestProxyingURLLoaderFactory* const factory_;
network::ResourceRequest request_; network::ResourceRequest request_;
const base::Optional<url::Origin> original_initiator_;
const bool is_download_; const bool is_download_;
const uint64_t request_id_; const uint64_t request_id_;
const int32_t network_service_request_id_; const int32_t network_service_request_id_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment