Skip to content

Commit

Permalink
Support document.referrer in SXG
Browse files Browse the repository at this point in the history
Before this CL, CreateRedirectInfo() in signed_exchange_loader.cc doesn't set
redirect_info.new_referrer correctly. So document.referrer was always empty
string after clicking a link of signed exchange.
After this CL, CreateRedirectInfo() returns a RedirectInfo with new_referrer, so
document.referrer will be set correctly.

This CL introduces network::ResourceRequest argument in
NavigationLoaderInterceptor::MaybeCreateLoaderForResponse(),
PrefetchURLLoader, SignedExchangePrefetchHandler and SignedExchangeLoader.
This value will be used to compute a correct RedirectInfo while handling the
synthesized redirect for signed exchange.

This CL uses "ignoreErrors" flag in generate-test-sxgs.sh while creating invalid
signed exchange files. This flags was introduces by this change in webpackage
repository.
WICG/webpackage@d335219

Bug: 920905
Change-Id: I52a302d22c375b776874c996638b409cdcba8a9c
Reviewed-on: https://chromium-review.googlesource.com/c/1415245
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625094}
  • Loading branch information
horo-t authored and Commit Bot committed Jan 23, 2019
1 parent ee0c34a commit d0a953d
Show file tree
Hide file tree
Showing 34 changed files with 467 additions and 146 deletions.
2 changes: 1 addition & 1 deletion content/browser/appcache/appcache_request_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ void AppCacheRequestHandler::MaybeCreateLoader(
}

bool AppCacheRequestHandler::MaybeCreateLoaderForResponse(
const GURL& request_url,
const network::ResourceRequest& request,
const network::ResourceResponseHead& response,
network::mojom::URLLoaderPtr* loader,
network::mojom::URLLoaderClientRequest* client_request,
Expand Down
2 changes: 1 addition & 1 deletion content/browser/appcache/appcache_request_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class CONTENT_EXPORT AppCacheRequestHandler
FallbackCallback fallback_callback) override;
// MaybeCreateLoaderForResponse always returns synchronously.
bool MaybeCreateLoaderForResponse(
const GURL& request_url,
const network::ResourceRequest& request,
const network::ResourceResponseHead& response,
network::mojom::URLLoaderPtr* loader,
network::mojom::URLLoaderClientRequest* client_request,
Expand Down
2 changes: 1 addition & 1 deletion content/browser/loader/navigation_loader_interceptor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ NavigationLoaderInterceptor::MaybeCreateSubresourceLoaderParams() {
}

bool NavigationLoaderInterceptor::MaybeCreateLoaderForResponse(
const GURL& request_url,
const network::ResourceRequest& request,
const network::ResourceResponseHead& response,
network::mojom::URLLoaderPtr* loader,
network::mojom::URLLoaderClientRequest* client_request,
Expand Down
4 changes: 2 additions & 2 deletions content/browser/loader/navigation_loader_interceptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class CONTENT_EXPORT NavigationLoaderInterceptor {
MaybeCreateSubresourceLoaderParams();

// Returns true if the handler creates a loader for the |response| passed.
// |request_url| is the latest request URL including URL fragment.
// |request| is the latest request whose request URL may include URL fragment.
// An example of where this is used is AppCache, where the handler returns
// fallback content for the response passed in.
// The URLLoader interface pointer is returned in the |loader| parameter.
Expand All @@ -100,7 +100,7 @@ class CONTENT_EXPORT NavigationLoaderInterceptor {
// Remove this flag when we support service worker and signed exchange
// integration. See crbug.com/894755#c1. Nullptr is not allowed.
virtual bool MaybeCreateLoaderForResponse(
const GURL& request_url,
const network::ResourceRequest& request,
const network::ResourceResponseHead& response,
network::mojom::URLLoaderPtr* loader,
network::mojom::URLLoaderClientRequest* client_request,
Expand Down
9 changes: 4 additions & 5 deletions content/browser/loader/navigation_url_loader_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1391,8 +1391,9 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
network::mojom::URLLoaderClientRequest response_client_request;
bool skip_other_interceptors = false;
if (interceptor->MaybeCreateLoaderForResponse(
url_, response, &response_url_loader_, &response_client_request,
url_loader_.get(), &skip_other_interceptors)) {
*resource_request_, response, &response_url_loader_,
&response_client_request, url_loader_.get(),
&skip_other_interceptors)) {
if (response_loader_binding_.is_bound())
response_loader_binding_.Close();
response_loader_binding_.Bind(std::move(response_client_request));
Expand Down Expand Up @@ -1460,11 +1461,9 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
// SignedExchangeHandler which is indirectly owned by |this| until its
// header is verified and parsed, that's where the getter is used.
return std::make_unique<SignedExchangeRequestHandler>(
url::Origin::Create(request_info.common_params.url),
GetURLLoaderOptions(request_info.is_main_frame),
request_info.frame_tree_node_id, request_info.devtools_navigation_token,
request_info.devtools_frame_token, request_info.report_raw_headers,
request_info.begin_params->load_flags, std::move(url_loader_factory),
std::move(url_loader_factory),
base::BindRepeating(
&URLLoaderRequestController::CreateURLLoaderThrottles,
base::Unretained(this)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class TestNavigationLoaderInterceptor : public NavigationLoaderInterceptor {
}

bool MaybeCreateLoaderForResponse(
const GURL& request_url,
const network::ResourceRequest& request,
const network::ResourceResponseHead& response,
network::mojom::URLLoaderPtr* loader,
network::mojom::URLLoaderClientRequest* client_request,
Expand Down
36 changes: 15 additions & 21 deletions content/browser/loader/prefetch_url_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ PrefetchURLLoader::PrefetchURLLoader(
scoped_refptr<SignedExchangePrefetchMetricRecorder>
signed_exchange_prefetch_metric_recorder)
: frame_tree_node_id_getter_(frame_tree_node_id_getter),
url_(resource_request.url),
report_raw_headers_(resource_request.report_raw_headers),
load_flags_(resource_request.load_flags),
throttling_profile_id_(resource_request.throttling_profile_id),
resource_request_(resource_request),
network_loader_factory_(std::move(network_loader_factory)),
client_binding_(this),
forwarding_client_(std::move(client)),
Expand All @@ -52,16 +49,11 @@ PrefetchURLLoader::PrefetchURLLoader(
std::move(signed_exchange_prefetch_metric_recorder)) {
DCHECK(network_loader_factory_);

if (resource_request.request_initiator)
request_initiator_ = *resource_request.request_initiator;

base::Optional<network::ResourceRequest> modified_resource_request;
if (signed_exchange_utils::ShouldAdvertiseAcceptHeader(
url::Origin::Create(resource_request.url))) {
url::Origin::Create(resource_request_.url))) {
// Set the SignedExchange accept header only for the limited origins.
// (https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#internet-media-type-applicationsigned-exchange).
modified_resource_request = resource_request;
modified_resource_request->headers.SetHeader(
resource_request_.headers.SetHeader(
network::kAcceptHeader, kSignedExchangeEnabledAcceptHeaderForPrefetch);
}

Expand All @@ -71,8 +63,7 @@ PrefetchURLLoader::PrefetchURLLoader(
&PrefetchURLLoader::OnNetworkConnectionError, base::Unretained(this)));
network_loader_factory_->CreateLoaderAndStart(
mojo::MakeRequest(&loader_), routing_id, request_id, options,
modified_resource_request ? *modified_resource_request : resource_request,
std::move(network_client), traffic_annotation);
resource_request_, std::move(network_client), traffic_annotation);
}

PrefetchURLLoader::~PrefetchURLLoader() = default;
Expand All @@ -86,7 +77,6 @@ void PrefetchURLLoader::FollowRedirect(
"crbug.com/845683";
DCHECK(!new_url) << "Redirect with modified URL was not "
"supported yet. crbug.com/845683";
DCHECK(new_url_for_redirect_.is_valid());
if (signed_exchange_prefetch_handler_) {
// Rebind |client_binding_| and |loader_|.
client_binding_.Bind(signed_exchange_prefetch_handler_->FollowRedirect(
Expand All @@ -101,7 +91,7 @@ void PrefetchURLLoader::FollowRedirect(
// SignedHTTPExchange feature. So need to update the accept header by
// checking the new URL when redirected.
if (signed_exchange_utils::ShouldAdvertiseAcceptHeader(
url::Origin::Create(new_url_for_redirect_))) {
url::Origin::Create(resource_request_.url))) {
modified_request_headers_for_accept.SetHeader(
network::kAcceptHeader,
kSignedExchangeEnabledAcceptHeaderForPrefetch);
Expand Down Expand Up @@ -134,17 +124,17 @@ void PrefetchURLLoader::ResumeReadingBodyFromNet() {

void PrefetchURLLoader::OnReceiveResponse(
const network::ResourceResponseHead& response) {
if (signed_exchange_utils::ShouldHandleAsSignedHTTPExchange(url_, response)) {
if (signed_exchange_utils::ShouldHandleAsSignedHTTPExchange(
resource_request_.url, response)) {
DCHECK(!signed_exchange_prefetch_handler_);

// Note that after this point this doesn't directly get upcalls from the
// network. (Until |this| calls the handler's FollowRedirect.)
signed_exchange_prefetch_handler_ =
std::make_unique<SignedExchangePrefetchHandler>(
frame_tree_node_id_getter_, report_raw_headers_, load_flags_,
throttling_profile_id_, response, std::move(loader_),
client_binding_.Unbind(), network_loader_factory_,
request_initiator_, url_, url_loader_throttles_getter_,
frame_tree_node_id_getter_, resource_request_, response,
std::move(loader_), client_binding_.Unbind(),
network_loader_factory_, url_loader_throttles_getter_,
resource_context_, request_context_getter_, this,
signed_exchange_prefetch_metric_recorder_);
return;
Expand All @@ -155,7 +145,11 @@ void PrefetchURLLoader::OnReceiveResponse(
void PrefetchURLLoader::OnReceiveRedirect(
const net::RedirectInfo& redirect_info,
const network::ResourceResponseHead& head) {
new_url_for_redirect_ = redirect_info.new_url;
resource_request_.url = redirect_info.new_url;
resource_request_.site_for_cookies = redirect_info.new_site_for_cookies;
resource_request_.top_frame_origin = redirect_info.new_top_frame_origin;
resource_request_.referrer = GURL(redirect_info.new_referrer);
resource_request_.referrer_policy = redirect_info.new_referrer_policy;
forwarding_client_->OnReceiveRedirect(redirect_info, head);
}

Expand Down
10 changes: 3 additions & 7 deletions content/browser/loader/prefetch_url_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,9 @@ class CONTENT_EXPORT PrefetchURLLoader : public network::mojom::URLLoader,
void OnNetworkConnectionError();

const base::RepeatingCallback<int(void)> frame_tree_node_id_getter_;
const GURL url_;
const bool report_raw_headers_;
const int load_flags_;
const base::Optional<base::UnguessableToken> throttling_profile_id_;

// Set in the constructor and updated when redirected.
network::ResourceRequest resource_request_;

scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory_;

Expand All @@ -109,8 +108,6 @@ class CONTENT_EXPORT PrefetchURLLoader : public network::mojom::URLLoader,
// To be a URLLoader for the client.
network::mojom::URLLoaderClientPtr forwarding_client_;

url::Origin request_initiator_;

// |url_loader_throttles_getter_| and |resource_context_| should be
// valid as far as |request_context_getter_| returns non-null value.
URLLoaderThrottlesGetter url_loader_throttles_getter_;
Expand All @@ -122,7 +119,6 @@ class CONTENT_EXPORT PrefetchURLLoader : public network::mojom::URLLoader,
std::unique_ptr<SignedExchangePrefetchHandler>
signed_exchange_prefetch_handler_;

GURL new_url_for_redirect_;

scoped_refptr<SignedExchangePrefetchMetricRecorder>
signed_exchange_prefetch_metric_recorder_;
Expand Down
68 changes: 32 additions & 36 deletions content/browser/web_package/signed_exchange_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "net/base/net_errors.h"
#include "net/cert/cert_status_flags.h"
#include "net/http/http_util.h"
#include "net/url_request/redirect_util.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/cpp/url_loader_completion_status.h"
Expand All @@ -38,24 +39,23 @@ constexpr char kPrefetchLoadResultHistogram[] =
constexpr char kContentTypeOptionsHeaderName[] = "x-content-type-options";
constexpr char kNoSniffHeaderValue[] = "nosniff";

net::RedirectInfo CreateRedirectInfo(const GURL& new_url,
const GURL& outer_request_url) {
net::RedirectInfo redirect_info;
if (outer_request_url.has_ref()) {
// Propagate ref fragment from the outer request URL.
url::Replacements<char> replacements;
base::StringPiece ref = outer_request_url.ref_piece();
replacements.SetRef(ref.data(), url::Component(0, ref.length()));
redirect_info.new_url = new_url.ReplaceComponents(replacements);
} else {
redirect_info.new_url = new_url;
}
redirect_info.new_method = "GET";
net::RedirectInfo CreateRedirectInfo(
const GURL& new_url,
const network::ResourceRequest& outer_request,
const network::ResourceResponseHead& outer_response) {
// https://wicg.github.io/webpackage/loading.html#mp-http-fetch
// Step 3. Set actualResponse's status to 303. [spec text]
redirect_info.status_code = 303;
redirect_info.new_site_for_cookies = redirect_info.new_url;
return redirect_info;
return net::RedirectInfo::ComputeRedirectInfo(
"GET", outer_request.url, outer_request.site_for_cookies,
outer_request.top_frame_origin,
outer_request.update_first_party_url_on_redirect
? net::URLRequest::FirstPartyURLPolicy::
UPDATE_FIRST_PARTY_URL_ON_REDIRECT
: net::URLRequest::FirstPartyURLPolicy::NEVER_CHANGE_FIRST_PARTY_URL,
outer_request.referrer_policy, outer_request.referrer.spec(), 303,
new_url,
net::RedirectUtil::GetReferrerPolicyHeader(outer_response.headers.get()),
false /* insecure_scheme_was_upgraded */);
}

bool HasNoSniffHeader(const network::ResourceResponseHead& response) {
Expand Down Expand Up @@ -106,43 +106,37 @@ class SignedExchangeLoader::ResponseTimingInfo {
};

SignedExchangeLoader::SignedExchangeLoader(
const GURL& outer_request_url,
const network::ResourceRequest& outer_request,
const network::ResourceResponseHead& outer_response,
network::mojom::URLLoaderClientPtr forwarding_client,
network::mojom::URLLoaderClientEndpointsPtr endpoints,
url::Origin request_initiator,
uint32_t url_loader_options,
int load_flags,
bool should_redirect_on_failure,
const base::Optional<base::UnguessableToken>& throttling_profile_id,
std::unique_ptr<SignedExchangeDevToolsProxy> devtools_proxy,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
URLLoaderThrottlesGetter url_loader_throttles_getter,
base::RepeatingCallback<int(void)> frame_tree_node_id_getter,
scoped_refptr<SignedExchangePrefetchMetricRecorder> metric_recorder)
: outer_request_url_(outer_request_url),
: outer_request_(outer_request),
outer_response_timing_info_(
std::make_unique<ResponseTimingInfo>(outer_response)),
outer_response_(outer_response),
forwarding_client_(std::move(forwarding_client)),
url_loader_client_binding_(this),
request_initiator_(request_initiator),
url_loader_options_(url_loader_options),
load_flags_(load_flags),
should_redirect_on_failure_(should_redirect_on_failure),
throttling_profile_id_(throttling_profile_id),
devtools_proxy_(std::move(devtools_proxy)),
url_loader_factory_(std::move(url_loader_factory)),
url_loader_throttles_getter_(std::move(url_loader_throttles_getter)),
frame_tree_node_id_getter_(frame_tree_node_id_getter),
metric_recorder_(std::move(metric_recorder)),
weak_factory_(this) {
DCHECK(signed_exchange_utils::IsSignedExchangeHandlingEnabled());
DCHECK(outer_request_url_.is_valid());
DCHECK(outer_request_.url.is_valid());

if (!(load_flags_ & net::LOAD_PREFETCH)) {
if (!(outer_request_.load_flags & net::LOAD_PREFETCH)) {
metric_recorder_->OnSignedExchangeNonPrefetch(
outer_request_url_, outer_response_.response_time);
outer_request_.url, outer_response_.response_time);
}

// Can't use HttpResponseHeaders::GetMimeType() because SignedExchangeHandler
Expand Down Expand Up @@ -209,8 +203,10 @@ void SignedExchangeLoader::OnTransferSizeUpdated(int32_t transfer_size_diff) {
void SignedExchangeLoader::OnStartLoadingResponseBody(
mojo::ScopedDataPipeConsumerHandle body) {
auto cert_fetcher_factory = SignedExchangeCertFetcherFactory::Create(
std::move(request_initiator_), std::move(url_loader_factory_),
std::move(url_loader_throttles_getter_), throttling_profile_id_);
outer_request_.request_initiator ? *outer_request_.request_initiator
: url::Origin(),
std::move(url_loader_factory_), std::move(url_loader_throttles_getter_),
outer_request_.throttling_profile_id);

if (g_signed_exchange_factory_for_testing_) {
signed_exchange_handler_ = g_signed_exchange_factory_for_testing_->Create(
Expand All @@ -222,12 +218,12 @@ void SignedExchangeLoader::OnStartLoadingResponseBody(
}

signed_exchange_handler_ = std::make_unique<SignedExchangeHandler>(
IsOriginSecure(outer_request_url_), HasNoSniffHeader(outer_response_),
IsOriginSecure(outer_request_.url), HasNoSniffHeader(outer_response_),
content_type_, std::make_unique<DataPipeToSourceStream>(std::move(body)),
base::BindOnce(&SignedExchangeLoader::OnHTTPExchangeFound,
weak_factory_.GetWeakPtr()),
std::move(cert_fetcher_factory), load_flags_, std::move(devtools_proxy_),
frame_tree_node_id_getter_);
std::move(cert_fetcher_factory), outer_request_.load_flags,
std::move(devtools_proxy_), frame_tree_node_id_getter_);
}

void SignedExchangeLoader::OnComplete(
Expand Down Expand Up @@ -276,10 +272,10 @@ void SignedExchangeLoader::OnHTTPExchangeFound(
const network::ResourceResponseHead& resource_response,
std::unique_ptr<net::SourceStream> payload_stream) {
UMA_HISTOGRAM_ENUMERATION(kLoadResultHistogram, result);
if (load_flags_ & net::LOAD_PREFETCH) {
if (outer_request_.load_flags & net::LOAD_PREFETCH) {
UMA_HISTOGRAM_ENUMERATION(kPrefetchLoadResultHistogram, result);
metric_recorder_->OnSignedExchangePrefetchFinished(
outer_request_url_, outer_response_.response_time);
outer_request_.url, outer_response_.response_time);
}

if (error) {
Expand All @@ -296,7 +292,7 @@ void SignedExchangeLoader::OnHTTPExchangeFound(
fallback_url_ = request_url;
DCHECK(outer_response_timing_info_);
forwarding_client_->OnReceiveRedirect(
CreateRedirectInfo(request_url, outer_request_url_),
CreateRedirectInfo(request_url, outer_request_, outer_response_),
std::move(outer_response_timing_info_)->CreateRedirectResponseHead());
forwarding_client_.reset();
return;
Expand All @@ -306,7 +302,7 @@ void SignedExchangeLoader::OnHTTPExchangeFound(
// TODO(https://crbug.com/803774): Handle no-GET request_method as a error.
DCHECK(outer_response_timing_info_);
forwarding_client_->OnReceiveRedirect(
CreateRedirectInfo(request_url, outer_request_url_),
CreateRedirectInfo(request_url, outer_request_, outer_response_),
std::move(outer_response_timing_info_)->CreateRedirectResponseHead());
forwarding_client_.reset();

Expand Down
10 changes: 2 additions & 8 deletions content/browser/web_package/signed_exchange_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,12 @@ class SignedExchangeLoader final : public network::mojom::URLLoaderClient,
// If |should_redirect_on_failure| is true, verification failure causes a
// redirect to the fallback URL.
SignedExchangeLoader(
const GURL& outer_request_url,
const network::ResourceRequest& outer_request,
const network::ResourceResponseHead& outer_response,
network::mojom::URLLoaderClientPtr forwarding_client,
network::mojom::URLLoaderClientEndpointsPtr endpoints,
url::Origin request_initiator,
uint32_t url_loader_options,
int load_flags,
bool should_redirect_on_failure,
const base::Optional<base::UnguessableToken>& throttling_profile_id,
std::unique_ptr<SignedExchangeDevToolsProxy> devtools_proxy,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
URLLoaderThrottlesGetter url_loader_throttles_getter,
Expand Down Expand Up @@ -119,7 +116,7 @@ class SignedExchangeLoader final : public network::mojom::URLLoaderClient,

void FinishReadingBody(int result);

const GURL outer_request_url_;
const network::ResourceRequest outer_request_;

// This timing info is used to create a dummy redirect response.
std::unique_ptr<const ResponseTimingInfo> outer_response_timing_info_;
Expand Down Expand Up @@ -148,11 +145,8 @@ class SignedExchangeLoader final : public network::mojom::URLLoaderClient,
// Kept around until ProceedWithResponse is called.
mojo::ScopedDataPipeConsumerHandle pending_body_consumer_;

url::Origin request_initiator_;
const uint32_t url_loader_options_;
const int load_flags_;
const bool should_redirect_on_failure_;
const base::Optional<base::UnguessableToken> throttling_profile_id_;
std::unique_ptr<SignedExchangeDevToolsProxy> devtools_proxy_;
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
URLLoaderThrottlesGetter url_loader_throttles_getter_;
Expand Down
Loading

0 comments on commit d0a953d

Please sign in to comment.