Skip to content

Commit

Permalink
Origin policy fetching is now triggered from the network service
Browse files Browse the repository at this point in the history
This represents steps 7 and 8 of:
https://docs.google.com/document/d/1heiIgNdO7kbaU9BLOPO4wZ9maHScB87cGT5lyjeBVAM/edit#heading=h.4en9va43fgfj
Bug: 950905

I have combined these two steps as they make a lot of sense together.

1) A new bool `obey_origin_policy` has been added to ResourceRequest
2) Based on its presence:
 a) The `Sec-Origin-Policy` HTTP header is set
 b) The OriginPolicyManager is used to retrieve the policy
 c) The policy is saved on the ResourceRequest
3) The OriginPolicyThrottle has all the necessary information to
trigger the interstitial or allow the navigation.

Change-Id: I75b4da55f01c0eff461e005a92f24b28925eef62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706428
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686411}
  • Loading branch information
andypaicu authored and Commit Bot committed Aug 13, 2019
1 parent 0f5259c commit d669835
Show file tree
Hide file tree
Showing 24 changed files with 516 additions and 342 deletions.
18 changes: 4 additions & 14 deletions content/browser/frame_host/navigation_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,6 @@ void AddAdditionalRequestHeaders(net::HttpRequestHeaders* headers,
// Sec-Fetch-Site is covered by network::SetSecFetchSiteHeader function.
}

// Ask whether we should request a policy.
if (OriginPolicyThrottle::ShouldRequestOriginPolicy(url)) {
headers->SetHeader(net::HttpRequestHeaders::kSecOriginPolicy,
kDefaultOriginPolicyVersion);
}

// Next, set the HTTP Origin if needed.
if (!NeedsHTTPOrigin(headers, method))
return;
Expand Down Expand Up @@ -1217,10 +1211,6 @@ mojom::NavigationClient* NavigationRequest::GetCommitNavigationClient() {
return commit_navigation_client_.get();
}

void NavigationRequest::SetOriginPolicy(const network::OriginPolicy& policy) {
response_head_->head.origin_policy = policy;
}

void NavigationRequest::OnRequestRedirected(
const net::RedirectInfo& redirect_info,
const scoped_refptr<network::ResourceResponse>& response_head) {
Expand Down Expand Up @@ -1915,7 +1905,7 @@ void NavigationRequest::OnStartChecksComplete(
loader_ = NavigationURLLoader::Create(
browser_context, partition,
std::make_unique<NavigationRequestInfo>(
common_params_.Clone(), begin_params_.Clone(), site_for_cookies,
common_params_->Clone(), begin_params_.Clone(), site_for_cookies,
net::NetworkIsolationKey(top_frame_origin, frame_origin),
frame_tree_node_->IsMainFrame(), parent_is_main_frame,
IsSecureFrame(frame_tree_node_->parent()),
Expand All @@ -1926,8 +1916,8 @@ void NavigationRequest::OnStartChecksComplete(
upgrade_if_insecure_,
blob_url_loader_factory_ ? blob_url_loader_factory_->Clone()
: nullptr,
devtools_navigation_token(),
frame_tree_node_->devtools_frame_token()),
devtools_navigation_token(), frame_tree_node_->devtools_frame_token(),
OriginPolicyThrottle::ShouldRequestOriginPolicy(common_params_->url)),
std::move(navigation_ui_data), service_worker_handle_.get(),
appcache_handle_.get(), std::move(prefetched_signed_exchange_cache_),
this, is_served_from_back_forward_cache(), std::move(interceptor));
Expand Down Expand Up @@ -2199,7 +2189,7 @@ void NavigationRequest::CommitNavigation() {
render_frame_host_->GetProcess()->GetID(),
render_frame_host_->GetRoutingID(), &service_worker_provider_info);
}
auto common_params = common_params_.Clone();
auto common_params = common_params_->Clone();
auto commit_params = commit_params_.Clone();
if (subresource_loader_params_ &&
!subresource_loader_params_->prefetched_signed_exchanges.empty()) {
Expand Down
5 changes: 0 additions & 5 deletions content/browser/frame_host/navigation_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
// for commit. Only used with PerNavigationMojoInterface enabled.
mojom::NavigationClient* GetCommitNavigationClient();

// TODO(andypaicu): Currently the origin_policy_throttle is responsible for
// setting the origin policy. Remove this function after this is done inside
// the network service.
void SetOriginPolicy(const network::OriginPolicy& policy);

void set_transition(ui::PageTransition transition) {
common_params_->transition = transition;
}
Expand Down
6 changes: 4 additions & 2 deletions content/browser/frame_host/navigation_request_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ NavigationRequestInfo::NavigationRequestInfo(
std::unique_ptr<network::SharedURLLoaderFactoryInfo>
blob_url_loader_factory,
const base::UnguessableToken& devtools_navigation_token,
const base::UnguessableToken& devtools_frame_token)
const base::UnguessableToken& devtools_frame_token,
bool obey_origin_policy)
: common_params(std::move(common_params)),
begin_params(std::move(begin_params)),
site_for_cookies(site_for_cookies),
Expand All @@ -37,7 +38,8 @@ NavigationRequestInfo::NavigationRequestInfo(
upgrade_if_insecure(upgrade_if_insecure),
blob_url_loader_factory(std::move(blob_url_loader_factory)),
devtools_navigation_token(devtools_navigation_token),
devtools_frame_token(devtools_frame_token) {}
devtools_frame_token(devtools_frame_token),
obey_origin_policy(obey_origin_policy) {}

NavigationRequestInfo::~NavigationRequestInfo() {}

Expand Down
8 changes: 7 additions & 1 deletion content/browser/frame_host/navigation_request_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ struct CONTENT_EXPORT NavigationRequestInfo {
std::unique_ptr<network::SharedURLLoaderFactoryInfo>
blob_url_loader_factory,
const base::UnguessableToken& devtools_navigation_token,
const base::UnguessableToken& devtools_frame_token);
const base::UnguessableToken& devtools_frame_token,
bool obey_origin_policy);
NavigationRequestInfo(const NavigationRequestInfo& other) = delete;
~NavigationRequestInfo();

Expand Down Expand Up @@ -78,6 +79,11 @@ struct CONTENT_EXPORT NavigationRequestInfo {
const base::UnguessableToken devtools_navigation_token;

const base::UnguessableToken devtools_frame_token;

// If set, the network service will attempt to retrieve the appropriate origin
// policy, if necessary, and attach it to the ResourceResponseHead.
// Spec: https://wicg.github.io/origin-policy/
const bool obey_origin_policy;
};

} // namespace content
Expand Down
110 changes: 44 additions & 66 deletions content/browser/frame_host/origin_policy_throttle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "content/browser/frame_host/navigation_request.h"
#include "content/browser/storage_partition_impl.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/common/content_features.h"
#include "content/public/common/content_switches.h"
Expand Down Expand Up @@ -52,16 +53,9 @@ OriginPolicyThrottle::MaybeCreateThrottleFor(NavigationHandle* handle) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(handle);

// We use presence of the origin policy request header to determine
// whether we should create the throttle.
if (!handle->GetRequestHeaders().HasHeader(
net::HttpRequestHeaders::kSecOriginPolicy))
if (!ShouldRequestOriginPolicy(handle->GetURL()))
return nullptr;

// TODO(vogelheim): Rewrite & hoist up this DCHECK to ensure that ..HasHeader
// and ShouldRequestOriginPolicy are always equal on entry to the method.
// This depends on https://crbug.com/881234 being fixed.
DCHECK(OriginPolicyThrottle::ShouldRequestOriginPolicy(handle->GetURL()));
return base::WrapUnique(new OriginPolicyThrottle(handle));
}

Expand Down Expand Up @@ -93,78 +87,62 @@ NavigationThrottle::ThrottleCheckResult
OriginPolicyThrottle::WillProcessResponse() {
DCHECK(navigation_handle());

// Per spec, Origin Policies are only fetched for https:-requests. So we
// should always have HTTP headers at this point.
// However, some unit tests generate responses without headers, so we still
// need to check.
if (!navigation_handle()->GetResponseHeaders())
// If no test origin policy is set, look at the actual origin policy from the
// response.
const base::Optional<network::OriginPolicy>& origin_policy =
GetTestOriginPolicy().has_value()
? GetTestOriginPolicy()
: static_cast<NavigationHandleImpl*>(navigation_handle())
->navigation_request()
->response()
->head.origin_policy;

// If there is no origin_policy, treat this case as
// network::OriginPolicyState::kNoPolicyApplies.
if (!origin_policy.has_value()) {
return NavigationThrottle::PROCEED;
}

std::string header;
navigation_handle()->GetResponseHeaders()->GetNormalizedHeader(
net::HttpRequestHeaders::kSecOriginPolicy, &header);

network::mojom::OriginPolicyManager::RetrieveOriginPolicyCallback
origin_policy_manager_done = base::BindOnce(
&OriginPolicyThrottle::OnOriginPolicyManagerRetrieveDone,
weak_factory_.GetWeakPtr());

SiteInstance* site_instance = navigation_handle()->GetStartingSiteInstance();
StoragePartitionImpl* storage_partition =
static_cast<StoragePartitionImpl*>(BrowserContext::GetStoragePartition(
site_instance->GetBrowserContext(), site_instance));
network::mojom::OriginPolicyManager* origin_policy_manager =
storage_partition->GetOriginPolicyManagerForBrowserProcess();

origin_policy_manager->RetrieveOriginPolicy(
GetRequestOrigin(), header, std::move(origin_policy_manager_done));
switch (origin_policy->state) {
case network::OriginPolicyState::kCannotLoadPolicy:
case network::OriginPolicyState::kInvalidRedirect:
case network::OriginPolicyState::kOther:
return NavigationThrottle::ThrottleCheckResult(
NavigationThrottle::CANCEL, net::ERR_BLOCKED_BY_CLIENT,
GetContentClient()->browser()->GetOriginPolicyErrorPage(
origin_policy->state, navigation_handle()));

return NavigationThrottle::DEFER;
case network::OriginPolicyState::kNoPolicyApplies:
case network::OriginPolicyState::kLoaded:
return NavigationThrottle::PROCEED;
}
}

const char* OriginPolicyThrottle::GetNameForLogging() {
return "OriginPolicyThrottle";
}

OriginPolicyThrottle::OriginPolicyThrottle(NavigationHandle* handle)
: NavigationThrottle(handle) {}

const url::Origin OriginPolicyThrottle::GetRequestOrigin() const {
return url::Origin::Create(navigation_handle()->GetURL());
// static
void OriginPolicyThrottle::SetOriginPolicyForTesting(
const network::OriginPolicy& origin_policy) {
base::Optional<network::OriginPolicy> new_test_origin_policy = origin_policy;
GetTestOriginPolicy().swap(new_test_origin_policy);
}

void OriginPolicyThrottle::CancelNavigation(network::OriginPolicyState state,
const GURL& policy_url) {
base::Optional<std::string> error_page =
GetContentClient()->browser()->GetOriginPolicyErrorPage(
state, navigation_handle());
CancelDeferredNavigation(NavigationThrottle::ThrottleCheckResult(
NavigationThrottle::CANCEL, net::ERR_BLOCKED_BY_CLIENT, error_page));
// static
void OriginPolicyThrottle::ResetOriginPolicyForTesting() {
GetTestOriginPolicy().reset();
}

void OriginPolicyThrottle::OnOriginPolicyManagerRetrieveDone(
const network::OriginPolicy& origin_policy) {
switch (origin_policy.state) {
case network::OriginPolicyState::kCannotLoadPolicy:
case network::OriginPolicyState::kInvalidRedirect:
CancelNavigation(origin_policy.state, origin_policy.policy_url);
return;

case network::OriginPolicyState::kNoPolicyApplies:
Resume();
return;
OriginPolicyThrottle::OriginPolicyThrottle(NavigationHandle* handle)
: NavigationThrottle(handle) {}

case network::OriginPolicyState::kLoaded:
DCHECK(origin_policy.contents);
static_cast<NavigationHandleImpl*>(navigation_handle())
->navigation_request()
->SetOriginPolicy(origin_policy);
Resume();
return;

default:
NOTREACHED();
}
// static
base::Optional<network::OriginPolicy>&
OriginPolicyThrottle::GetTestOriginPolicy() {
static base::NoDestructor<base::Optional<network::OriginPolicy>>
test_origin_policy;
return *test_origin_policy;
}

} // namespace content
21 changes: 5 additions & 16 deletions content/browser/frame_host/origin_policy_throttle.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,9 @@

class GURL;

namespace url {
class Origin;
}

namespace content {
class NavigationHandle;

// Constant derived from the spec, https://github.com/WICG/origin-policy
static constexpr const char* kDefaultOriginPolicyVersion = "0";

// The OriginPolicyThrottle is responsible for deciding whether an origin
// policy should be fetched, and doing so when that is positive.
//
Expand Down Expand Up @@ -59,18 +52,14 @@ class CONTENT_EXPORT OriginPolicyThrottle : public NavigationThrottle {
ThrottleCheckResult WillProcessResponse() override;
const char* GetNameForLogging() override;

static void SetOriginPolicyForTesting(
const network::OriginPolicy& origin_policy);
static void ResetOriginPolicyForTesting();

private:
explicit OriginPolicyThrottle(NavigationHandle* handle);

const url::Origin GetRequestOrigin() const;

void CancelNavigation(network::OriginPolicyState state,
const GURL& policy_url);

void OnOriginPolicyManagerRetrieveDone(
const network::OriginPolicy& origin_policy);

base::WeakPtrFactory<OriginPolicyThrottle> weak_factory_{this};
static base::Optional<network::OriginPolicy>& GetTestOriginPolicy();

DISALLOW_COPY_AND_ASSIGN(OriginPolicyThrottle);
};
Expand Down

0 comments on commit d669835

Please sign in to comment.