Skip to content

Commit

Permalink
Fix redirects to Google search urls not getting Safe Search policy ap…
Browse files Browse the repository at this point in the history
…plied with network service.

This adds support to change the URL of a redirect by a URLLoaderThrottle. Similar to changing the
URL at the start of a request, the new URL is restricted to the same origin.

This new functionality is only supported when the network service is enabled. This is because the
only way to change the URL that is followed after a redirect is through
NetworkDelegate::OnBeforeURLRequest. When NS is disabled the NetworkDelegate
(ChromeNetworkDelegate) and the URLLoader implementation (MojoAsyncResourceHandler) are in
different layers (chrome and content). I didn't want this functionality that is in content/ to
depend on chrome/ to make it function correctly. Since the old path is already fixed by r608599 and
that code will be deleted once network service ships, I've left it as is.

A lot of files change because two core interfaces are changing:
1) URLLoaderThrottle::WillRedirectRequest gets a non-const net::RedirectInfo
2) URLLoader::FollowRedirect gains a "url.mojom.Url? new_url"
The interesting parts in this cl are url_loader.mojom, url_loader_throttle.h,
throttling_url_loader.*, loader_browsertest.cc, network_context.cc, url_loader.* and
google_url_loader_throttle.cc.

Bug: 899268
Change-Id: I512de64600500f7e7851aaa0b57f6e46f6cb8001
Reviewed-on: https://chromium-review.googlesource.com/c/1341002
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610304}
  • Loading branch information
John Abd-El-Malek authored and Commit Bot committed Nov 22, 2018
1 parent d4fa1fc commit c16f673
Show file tree
Hide file tree
Showing 78 changed files with 487 additions and 253 deletions.
14 changes: 8 additions & 6 deletions android_webview/browser/aw_proxying_url_loader_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ class InterceptedRequest : public network::mojom::URLLoader,
void OnComplete(const network::URLLoaderCompletionStatus& status) override;

// network::mojom::URLLoader
void FollowRedirect(const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>&
modified_request_headers) override;
void FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) override;
void ProceedWithResponse() override;
void SetPriority(net::RequestPriority priority,
int32_t intra_priority_value) override;
Expand Down Expand Up @@ -291,10 +292,11 @@ void InterceptedRequest::OnComplete(
void InterceptedRequest::FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers) {
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) {
if (target_loader_) {
target_loader_->FollowRedirect(to_be_removed_request_headers,
modified_request_headers);
modified_request_headers, new_url);
}

Restart();
Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/extensions/chrome_url_request_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,11 @@ class ResourceBundleFileLoader : public network::mojom::URLLoader {
}

// mojom::URLLoader implementation:
void FollowRedirect(const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>&
modified_request_headers) override {
void FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) override {
NOTREACHED() << "No redirects for local file loads.";
}
// Current implementation reads all resource data at start of resource
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/offline_pages/offline_page_url_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ void OfflinePageURLLoader::SetTabIdGetterForTesting(
void OfflinePageURLLoader::FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers) {
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) {
NOTREACHED();
}

Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/offline_pages/offline_page_url_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@ class OfflinePageURLLoader : public network::mojom::URLLoader,
content::URLLoaderRequestInterceptor::LoaderCallback callback);

// network::mojom::URLLoader:
void FollowRedirect(const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>&
modified_request_headers) override;
void FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) override;
void ProceedWithResponse() override;
void SetPriority(net::RequestPriority priority,
int32_t intra_priority_value) override;
Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/prerender/prerender_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3074,10 +3074,11 @@ class HangingURLLoader : public network::mojom::URLLoader {
: client_(std::move(client)) {}
~HangingURLLoader() override {}
// mojom::URLLoader implementation:
void FollowRedirect(const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>&
modified_request_headers) override {}
void FollowRedirect(
const base::Optional<std::vector<std::string>>&
to_be_removed_request_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) override {}
void ProceedWithResponse() override {}
void SetPriority(net::RequestPriority priority,
int32_t intra_priority_value) override {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ class ProxyingURLLoaderFactory::InProgressRequest
// network::mojom::URLLoader:
void FollowRedirect(
const base::Optional<std::vector<std::string>>& to_be_removed_headers,
const base::Optional<net::HttpRequestHeaders>& modified_request_headers)
override;
const base::Optional<net::HttpRequestHeaders>& modified_request_headers,
const base::Optional<GURL>& new_url) override;

void ProceedWithResponse() override { target_loader_->ProceedWithResponse(); }

Expand Down Expand Up @@ -360,7 +360,8 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(

void ProxyingURLLoaderFactory::InProgressRequest::FollowRedirect(
const base::Optional<std::vector<std::string>>& opt_headers_to_remove,
const base::Optional<net::HttpRequestHeaders>& opt_modified_headers) {
const base::Optional<net::HttpRequestHeaders>& opt_modified_headers,
const base::Optional<GURL>& opt_new_url) {
std::vector<std::string> headers_to_remove;
if (opt_headers_to_remove)
headers_to_remove = *opt_headers_to_remove;
Expand All @@ -381,7 +382,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::FollowRedirect(
headers_to_remove.empty() ? base::nullopt
: base::make_optional(headers_to_remove),
modified_headers.IsEmpty() ? base::nullopt
: base::make_optional(modified_headers));
: base::make_optional(modified_headers),
opt_new_url);

request_url_ = redirect_info_.new_url;
referrer_origin_ = GURL(redirect_info_.new_referrer).GetOrigin();
Expand Down
10 changes: 5 additions & 5 deletions chrome/browser/signin/chrome_signin_url_loader_throttle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,15 @@ void URLLoaderThrottle::WillStartRequest(network::ResourceRequest* request,
}

void URLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* /* defer */,
std::vector<std::string>* to_be_removed_request_headers,
net::HttpRequestHeaders* modified_request_headers) {
ThrottleRequestAdapter request_adapter(this, request_headers_,
modified_request_headers,
to_be_removed_request_headers);
delegate_->ProcessRequest(&request_adapter, redirect_info.new_url);
delegate_->ProcessRequest(&request_adapter, redirect_info->new_url);

request_headers_.MergeFrom(*modified_request_headers);
for (const std::string& name : *to_be_removed_request_headers)
Expand All @@ -180,10 +180,10 @@ void URLLoaderThrottle::WillRedirectRequest(
// Modifications to |response_head.headers| will be passed to the
// URLLoaderClient even though |response_head| is const.
ThrottleResponseAdapter response_adapter(this, response_head.headers.get());
delegate_->ProcessResponse(&response_adapter, redirect_info.new_url);
delegate_->ProcessResponse(&response_adapter, redirect_info->new_url);

request_url_ = redirect_info.new_url;
request_referrer_ = GURL(redirect_info.new_referrer);
request_url_ = redirect_info->new_url;
request_referrer_ = GURL(redirect_info->new_referrer);
}

void URLLoaderThrottle::WillProcessResponse(
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/signin/chrome_signin_url_loader_throttle.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class URLLoaderThrottle : public content::URLLoaderThrottle {
// content::URLLoaderThrottle
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
void WillRedirectRequest(net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* headers_to_remove,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ TEST(ChromeSigninURLLoaderThrottleTest, Intercept) {

std::vector<std::string> request_headers_to_remove;
net::HttpRequestHeaders modified_request_headers;
throttle->WillRedirectRequest(redirect_info, *response_head, &defer,
throttle->WillRedirectRequest(&redirect_info, *response_head, &defer,
&request_headers_to_remove,
&modified_request_headers);

Expand Down Expand Up @@ -246,7 +246,7 @@ TEST(ChromeSigninURLLoaderThrottleTest, InterceptSubFrame) {

std::vector<std::string> request_headers_to_remove;
net::HttpRequestHeaders modified_request_headers;
throttle->WillRedirectRequest(redirect_info, response_head, &defer,
throttle->WillRedirectRequest(&redirect_info, response_head, &defer,
&request_headers_to_remove,
&modified_request_headers);
EXPECT_FALSE(defer);
Expand Down
18 changes: 14 additions & 4 deletions chrome/common/google_url_loader_throttle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "chrome/common/net/safe_search_util.h"
#include "components/variations/net/variations_http_headers.h"
#include "services/network/public/cpp/features.h"

#if BUILDFLAG(ENABLE_EXTENSIONS)
#include "extensions/common/extension_urls.h"
Expand Down Expand Up @@ -58,26 +59,35 @@ void GoogleURLLoaderThrottle::WillStartRequest(
}

void GoogleURLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& /* response_head */,
bool* /* defer */,
std::vector<std::string>* to_be_removed_headers,
net::HttpRequestHeaders* modified_headers) {
if (!variations::ShouldAppendVariationHeaders(redirect_info.new_url))
if (!variations::ShouldAppendVariationHeaders(redirect_info->new_url))
to_be_removed_headers->push_back(variations::kClientDataHeader);

// URLLoaderThrottles can only change the redirect URL when the network
// service is enabled. The non-network service path handles this in
// ChromeNetworkDelegate.
if (dynamic_params_.force_safe_search &&
base::FeatureList::IsEnabled(network::features::kNetworkService)) {
safe_search_util::ForceGoogleSafeSearch(redirect_info->new_url,
&redirect_info->new_url);
}

if (dynamic_params_.youtube_restrict >
safe_search_util::YOUTUBE_RESTRICT_OFF &&
dynamic_params_.youtube_restrict <
safe_search_util::YOUTUBE_RESTRICT_COUNT) {
safe_search_util::ForceYouTubeRestrict(
redirect_info.new_url, modified_headers,
redirect_info->new_url, modified_headers,
static_cast<safe_search_util::YouTubeRestrictMode>(
dynamic_params_.youtube_restrict));
}

if (!dynamic_params_.allowed_domains_for_apps.empty() &&
redirect_info.new_url.DomainIs("google.com")) {
redirect_info->new_url.DomainIs("google.com")) {
modified_headers->SetHeader(safe_search_util::kGoogleAppsAllowedDomains,
dynamic_params_.allowed_domains_for_apps);
}
Expand Down
2 changes: 1 addition & 1 deletion chrome/common/google_url_loader_throttle.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class GoogleURLLoaderThrottle
void DetachFromCurrentSequence() override;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
void WillRedirectRequest(net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_headers,
Expand Down
6 changes: 3 additions & 3 deletions chrome/common/prerender_url_loader_throttle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ void PrerenderURLLoaderThrottle::WillStartRequest(
}

void PrerenderURLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* /* to_be_removed_headers */,
Expand All @@ -156,12 +156,12 @@ void PrerenderURLLoaderThrottle::WillRedirectRequest(
response_head.headers->GetNormalizedHeader(
kFollowOnlyWhenPrerenderShown, &follow_only_when_prerender_shown_header);
// Abort any prerenders with requests which redirect to invalid schemes.
if (!DoesURLHaveValidScheme(redirect_info.new_url)) {
if (!DoesURLHaveValidScheme(redirect_info->new_url)) {
delegate_->CancelWithError(net::ERR_ABORTED);
canceler_getter_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(CancelPrerenderForUnsupportedScheme,
std::move(canceler_getter_), redirect_info.new_url));
std::move(canceler_getter_), redirect_info->new_url));
} else if (follow_only_when_prerender_shown_header == "1" &&
resource_type_ != content::RESOURCE_TYPE_MAIN_FRAME) {
// Only defer redirects with the Follow-Only-When-Prerender-Shown
Expand Down
2 changes: 1 addition & 1 deletion chrome/common/prerender_url_loader_throttle.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class PrerenderURLLoaderThrottle
void DetachFromCurrentSequence() override;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
void WillRedirectRequest(net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_headers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,13 @@ void DataReductionProxyURLLoaderThrottle::WillStartRequest(
}

void DataReductionProxyURLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_request_headers,
net::HttpRequestHeaders* modified_request_headers) {
url_chain_.push_back(redirect_info.new_url);
request_method_ = redirect_info.new_method;
url_chain_.push_back(redirect_info->new_url);
request_method_ = redirect_info->new_method;
}

void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class DataReductionProxyURLLoaderThrottle : public content::URLLoaderThrottle {
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_request_headers,
Expand Down
2 changes: 1 addition & 1 deletion components/download/internal/common/resource_downloader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ void ResourceDownloader::OnResponseStarted(
}

void ResourceDownloader::OnReceiveRedirect() {
url_loader_->FollowRedirect(base::nullopt, base::nullopt);
url_loader_->FollowRedirect(base::nullopt, base::nullopt, base::nullopt);
}

void ResourceDownloader::OnResponseCompleted() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,14 @@ void BaseParallelResourceThrottle::WillRedirectRequest(
}

// The safe browsing URLLoaderThrottle doesn't use the |resource_head|,
// |to_be_modified_headers| or |modified_headers| parameters, so pass
// in an empty struct to avoid changing ResourceThrottle signature.
// |to_be_modified_headers|, |modified_headers| or |new_url| parameters, so
// pass in an empty struct to avoid changing ResourceThrottle signature.
network::ResourceResponseHead resource_head;
std::vector<std::string> to_be_removed_headers;
net::HttpRequestHeaders modified_headers;
net::RedirectInfo redirect_info_copy = redirect_info;
url_loader_throttle_holder_->throttle()->WillRedirectRequest(
redirect_info, resource_head, defer, &to_be_removed_headers,
&redirect_info_copy, resource_head, defer, &to_be_removed_headers,
&modified_headers);
DCHECK(!*defer);
throttle_in_band_ = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ void BrowserURLLoaderThrottle::WillStartRequest(
}

void BrowserURLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& /* response_head */,
bool* defer,
std::vector<std::string>* /* to_be_removed_headers */,
Expand All @@ -82,7 +82,7 @@ void BrowserURLLoaderThrottle::WillRedirectRequest(

pending_checks_++;
url_checker_->CheckUrl(
redirect_info.new_url, redirect_info.new_method,
redirect_info->new_url, redirect_info->new_method,
base::BindOnce(&BrowserURLLoaderThrottle::OnCheckUrlResult,
base::Unretained(this)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class BrowserURLLoaderThrottle : public content::URLLoaderThrottle {
// content::URLLoaderThrottle implementation.
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
void WillRedirectRequest(net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_headers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void RendererURLLoaderThrottle::WillStartRequest(
}

void RendererURLLoaderThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& /* response_head */,
bool* /* defer */,
std::vector<std::string>* /* to_be_removed_headers */,
Expand All @@ -86,7 +86,7 @@ void RendererURLLoaderThrottle::WillRedirectRequest(

pending_checks_++;
url_checker_->CheckUrl(
redirect_info.new_url, redirect_info.new_method,
redirect_info->new_url, redirect_info->new_method,
base::BindOnce(&RendererURLLoaderThrottle::OnCheckUrlResult,
base::Unretained(this)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class RendererURLLoaderThrottle : public content::URLLoaderThrottle,
void DetachFromCurrentSequence() override;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override;
void WillRedirectRequest(const net::RedirectInfo& redirect_info,
void WillRedirectRequest(net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& response_head,
bool* defer,
std::vector<std::string>* to_be_removed_headers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,14 @@ void AdDelayThrottle::WillStartRequest(network::ResourceRequest* request,
}

void AdDelayThrottle::WillRedirectRequest(
const net::RedirectInfo& redirect_info,
net::RedirectInfo* redirect_info,
const network::ResourceResponseHead& /* response_head */,
bool* defer,
std::vector<std::string>* /* to_be_removed_headers */,
net::HttpRequestHeaders* /* modified_headers */) {
// Note: some MetadataProviders may not be able to distinguish requests that
// are only tagged as ads after a redirect.
*defer = MaybeDefer(redirect_info.new_url);
*defer = MaybeDefer(redirect_info->new_url);
}

bool AdDelayThrottle::MaybeDefer(const GURL& url) {
Expand Down
Loading

0 comments on commit c16f673

Please sign in to comment.