Skip to content

Commit

Permalink
Reland "Fetch: Plumb request initiator through passthrough service wo…
Browse files Browse the repository at this point in the history
…rkers."

This is a reland of da0a6501cf321579bd46a27ff9fba1bb8ea910bb

This CL also includes a change to mark the two WPT tests as requiring
long timeout durations.  On my fast build machine with an opt build
they take ~5 seconds each to complete and the default timeout is 10
seconds.  On slower bots with debug builds its highly likely that these
tests would be marked as timing out.  This change gives them a 60 second
timeout instead.

Original change's description:
> Fetch: Plumb request initiator through passthrough service workers.
>
> This CL contains essentially two changes:
>
> 1. The request initiator origin is plumbed through service workers
>    that do `fetch(evt.request)`.  In addition to plumbing, this
>    requires changes to how we validate navigation requests in the
>    CorsURLLoaderFactory.
> 2. Tracks the original destination of a request passed through a
>    service worker.  This is then used in the network service to force
>    SameSite=Lax cookies to treat the request as a main frame navigation
>    where appropriate.
>
> For more detailed information about these changes please see the
> internal design doc at:
>
> https://docs.google.com/document/d/1KZscujuV7bCFEnzJW-0DaCPU-I40RJimQKoCcI0umTQ/edit?usp=sharing
>
> In addition, there is some discussion of these features in the following
> spec issues:
>
> whatwg/fetch#1321
> whatwg/fetch#1327
>
> The test includes WPT tests that verify navigation headers and SameSite
> cookies.  Note, chrome has a couple expected failures in the SameSite
> cookie tests because of the "lax-allowing-unsafe" intervention that is
> currently enabled.  See:
>
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/TestExpectations;l=4635;drc=e8133cbf2469adb99c6610483ab78bcfb8cc4c76
>
> Bug: 1115847,1241188
> Change-Id: I7e236fa20aeabb705aef40fcf8d5c36da6d2798c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3115917
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
> Commit-Queue: Ben Kelly <wanderview@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#936029}

Bug: 1115847,1241188
Change-Id: Ia26acbdd0d7ce6583d9a44f83ed086708657b8bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3251368
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Auto-Submit: Ben Kelly <wanderview@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936560}
NOKEYCHECK=True
GitOrigin-RevId: 2d916566085e4f09bca93021f2b1650ea6237077
  • Loading branch information
wanderview authored and copybara-github committed Oct 29, 2021
1 parent 913596c commit e169e97
Show file tree
Hide file tree
Showing 18 changed files with 952 additions and 23 deletions.
9 changes: 9 additions & 0 deletions blink/public/mojom/fetch/fetch_api_request.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import "services/network/public/mojom/url_request.mojom";
import "third_party/blink/public/mojom/blob/serialized_blob.mojom";
import "third_party/blink/public/mojom/loader/request_context_frame_type.mojom";
import "third_party/blink/public/mojom/loader/referrer.mojom";
import "url/mojom/origin.mojom";
import "url/mojom/url.mojom";


Expand Down Expand Up @@ -162,6 +163,14 @@ struct FetchAPIRequest {
SerializedBlob? blob;
FetchAPIRequestBody? body;

// `request_initiator` indicates the origin that initiated the request. See
// also `network::ResourceRequest::request_initiator`, and the doc comment
// for `request_initiator` in services/network/public/mojom/url_request.mojom.
//
// Note that the origin may be missing for browser-initiated navigations
// (e.g. ones initiated from the Omnibox).
url.mojom.Origin? request_initiator;

Referrer? referrer;
network.mojom.CredentialsMode credentials_mode =
network.mojom.CredentialsMode.kOmit;
Expand Down
19 changes: 3 additions & 16 deletions blink/renderer/core/fetch/fetch_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -762,22 +762,7 @@ void FetchManager::Loader::PerformHTTPFetch() {
request.SetHttpMethod(fetch_request_data_->Method());
request.SetFetchWindowId(fetch_request_data_->WindowId());
request.SetTrustTokenParams(fetch_request_data_->TrustTokenParams());

switch (fetch_request_data_->Mode()) {
case RequestMode::kSameOrigin:
case RequestMode::kNoCors:
case RequestMode::kCors:
case RequestMode::kCorsWithForcedPreflight:
request.SetMode(fetch_request_data_->Mode());
break;
case RequestMode::kNavigate:
// NetworkService (i.e. CorsURLLoaderFactory::IsSane) rejects kNavigate
// requests coming from renderers, so using kSameOrigin here.
// TODO(lukasza): Tweak CorsURLLoaderFactory::IsSane to accept kNavigate
// if request_initiator and the target are same-origin.
request.SetMode(RequestMode::kSameOrigin);
break;
}
request.SetMode(fetch_request_data_->Mode());

request.SetCredentialsMode(fetch_request_data_->Credentials());
for (const auto& header : fetch_request_data_->HeaderList()->List()) {
Expand Down Expand Up @@ -825,6 +810,8 @@ void FetchManager::Loader::PerformHTTPFetch() {
UseCounter::Count(execution_context_, mojom::WebFeature::kFetchKeepalive);
}

request.SetOriginalDestination(fetch_request_data_->OriginalDestination());

// "3. Append `Host`, ..."
// FIXME: Implement this when the spec is fixed.

Expand Down
4 changes: 4 additions & 0 deletions blink/renderer/core/fetch/fetch_request_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ FetchRequestData* FetchRequestData::Create(
// we deprecate SetContext.

request->SetDestination(fetch_api_request->destination);
if (fetch_api_request->request_initiator)
request->SetOrigin(fetch_api_request->request_initiator);
request->SetReferrerString(AtomicString(Referrer::NoReferrer()));
if (fetch_api_request->referrer) {
if (!fetch_api_request->referrer->url.IsEmpty()) {
Expand All @@ -184,6 +186,7 @@ FetchRequestData* FetchRequestData::Create(
fetch_api_request->priority));
if (fetch_api_request->fetch_window_id)
request->SetWindowId(fetch_api_request->fetch_window_id.value());

return request;
}

Expand All @@ -205,6 +208,7 @@ FetchRequestData* FetchRequestData::CloneExceptBody() {
request->integrity_ = integrity_;
request->priority_ = priority_;
request->importance_ = importance_;
request->original_destination_ = original_destination_;
request->keepalive_ = keepalive_;
request->is_history_navigation_ = is_history_navigation_;
request->window_id_ = window_id_;
Expand Down
11 changes: 11 additions & 0 deletions blink/renderer/core/fetch/fetch_request_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ class CORE_EXPORT FetchRequestData final
void SetIntegrity(const String& integrity) { integrity_ = integrity; }
ResourceLoadPriority Priority() const { return priority_; }
void SetPriority(ResourceLoadPriority priority) { priority_ = priority; }

// The original destination of a request passed through by a service worker.
void SetOriginalDestination(network::mojom::RequestDestination value) {
original_destination_ = value;
}
network::mojom::RequestDestination OriginalDestination() const {
return original_destination_;
}

bool Keepalive() const { return keepalive_; }
void SetKeepalive(bool b) { keepalive_ = b; }
bool IsHistoryNavigation() const { return is_history_navigation_; }
Expand Down Expand Up @@ -174,6 +183,8 @@ class CORE_EXPORT FetchRequestData final
String mime_type_;
String integrity_;
ResourceLoadPriority priority_;
network::mojom::RequestDestination original_destination_ =
network::mojom::RequestDestination::kEmpty;
bool keepalive_;
bool is_history_navigation_ = false;
// A specific factory that should be used for this request instead of whatever
Expand Down
19 changes: 18 additions & 1 deletion blink/renderer/core/fetch/request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ FetchRequestData* CreateCopyOfFetchRequestDataForFetch(
request->SetURL(original->Url());
request->SetMethod(original->Method());
request->SetHeaderList(original->HeaderList()->Clone());
request->SetOrigin(context->GetSecurityOrigin());
request->SetOrigin(original->Origin() ? original->Origin()
: context->GetSecurityOrigin());
// FIXME: Set client.
DOMWrapperWorld& world = script_state->World();
if (world.IsIsolatedWorld()) {
Expand All @@ -97,6 +98,18 @@ FetchRequestData* CreateCopyOfFetchRequestDataForFetch(
}
request->SetWindowId(original->WindowId());
request->SetTrustTokenParams(original->TrustTokenParams());

// When a new request is created from another the destination is always reset
// to be `kEmpty`. In order to facilitate some later checks when a service
// worker forwards a navigation request we want to keep track of the
// destination of the original request. Therefore record the original
// request's destination if its non-empty, otherwise just carry forward
// whatever "original destination" value was already set.
if (original->Destination() != network::mojom::RequestDestination::kEmpty)
request->SetOriginalDestination(original->Destination());
else
request->SetOriginalDestination(original->OriginalDestination());

return request;
}

Expand Down Expand Up @@ -319,6 +332,9 @@ Request* Request::CreateRequestWithRequestOrString(

// "If any of |init|'s members are present, then:"
if (AreAnyMembersPresent(init)) {
request->SetOrigin(execution_context->GetSecurityOrigin());
request->SetOriginalDestination(network::mojom::RequestDestination::kEmpty);

// "If |request|'s |mode| is "navigate", then set it to "same-origin".
if (request->Mode() == network::mojom::RequestMode::kNavigate)
request->SetMode(network::mojom::RequestMode::kSameOrigin);
Expand Down Expand Up @@ -982,6 +998,7 @@ mojom::blink::FetchAPIRequestPtr Request::CreateFetchAPIRequest() const {
fetch_api_request->integrity = request_->Integrity();
fetch_api_request->is_history_navigation = request_->IsHistoryNavigation();
fetch_api_request->destination = request_->Destination();
fetch_api_request->request_initiator = request_->Origin();

// Strip off the fragment part of URL. So far, all callers expect the fragment
// to be excluded.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,12 @@ class ResponsesAccumulator : public RefCounted<ResponsesAccumulator> {
auto request_clone_without_body = mojom::blink::FetchAPIRequest::New(
request->mode, request->is_main_resource_load, request->destination,
request->frame_type, request->url, request->method, request->headers,
nullptr /* blob */, ResourceRequestBody(), request->referrer.Clone(),
request->credentials_mode, request->cache_mode,
request->redirect_mode, request->integrity, request->priority,
request->fetch_window_id, request->keepalive, request->is_reload,
request->is_history_navigation, request->devtools_stack_id);
nullptr /* blob */, ResourceRequestBody(), request->request_initiator,
request->referrer.Clone(), request->credentials_mode,
request->cache_mode, request->redirect_mode, request->integrity,
request->priority, request->fetch_window_id, request->keepalive,
request->is_reload, request->is_history_navigation,
request->devtools_stack_id);
cache_remote_->Match(
std::move(request), mojom::blink::CacheQueryOptions::New(),
/*in_related_fetch_event=*/false, /*in_range_fetch_event=*/false,
Expand Down
11 changes: 11 additions & 0 deletions blink/renderer/platform/loader/fetch/resource_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,14 @@ class PLATFORM_EXPORT ResourceRequestHead {
return allowHTTP1ForStreamingUpload_;
}

// The original destination of a request passed through by a service worker.
network::mojom::RequestDestination GetOriginalDestination() const {
return original_destination_;
}
void SetOriginalDestination(network::mojom::RequestDestination value) {
original_destination_ = value;
}

const absl::optional<ResourceRequestHead::WebBundleTokenParams>&
GetWebBundleTokenParams() const {
return web_bundle_token_params_;
Expand Down Expand Up @@ -603,6 +611,9 @@ class PLATFORM_EXPORT ResourceRequestHead {

base::UnguessableToken fetch_window_id_;

network::mojom::RequestDestination original_destination_ =
network::mojom::RequestDestination::kEmpty;

uint64_t inspector_id_ = 0;

bool is_from_origin_dirty_style_sheet_ = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,8 @@ void PopulateResourceRequest(const ResourceRequestHead& src,
dest->headers.SetHeaderIfMissing(net::HttpRequestHeaders::kAccept,
network::kDefaultAcceptHeaderValue);
}

dest->original_destination = src.GetOriginalDestination();
}

} // namespace blink
Loading

0 comments on commit e169e97

Please sign in to comment.