New issue
Advanced search Search tips

Issue 595993 link

Starred by 8 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Sign in to add a comment

User-Agent and Origin header shouldn't be generated before ServiceWorkers

Project Member Reported by tyoshino@chromium.org, Mar 18 2016

Issue description

According to the Fetch Standard, the "HTTP-network-or-cache fetch" algorithm generates the Origin header from the origin (https://fetch.spec.whatwg.org/#concept-request-origin) of the passed request (https://fetch.spec.whatwg.org/#concept-request).

But ServiceWorkers may see the header (checked by https://codereview.chromium.org/1816473002/).

 
The same about User-Agent and should also be about Referer (haven't checked Fetch API case).

All the stuffs are set in FrameFetchContext::addAdditionalRequestHeaders() and sent to a ServiceWorker.
Two options:

1. Stop adding such headers in Blink and add / use properties in ResourceRequest.
2. Clear out headers in the ServiceWorker case.

I like the first option but the second is much easier to implement.

> All the stuffs are set in FrameFetchContext::addAdditionalRequestHeaders() and sent to a ServiceWorker.

They can be set in other places.
Thanks Ben.

The files Ben listed have been upstreamed to the w3c repo:

* https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/fetch-header-visibility.https.html
* https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/resources/fetch-header-visibility-iframe.html

check-ua-header checks that no UA in SW when not specified in headers.
check-accept-header checks that there's always Accept in SW.

* https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/resources/fetch-request-no-freshness-headers-worker.js

fetch-request-no-freshness-headers.https.html checks that if-none-match and if-modified-since are not yet generated at SW point.

----

I also wrote a test for checking our behavior:
https://codereview.chromium.org/1816473002/
Maybe we can just merge Ben's.
Summary: User-Agent and Origin header shouldn't be generated before ServiceWorkers (was: Origin header is visible to ServiceWorkers)
Currently, Chrome is not conforming to the spec in that it generates the following headers before SW:
- User-Agent
- Origin
Status: Started (was: Available)
I'll keep the test I wrote for XHR. The other test which checks main resource loading by having SW handle iframe loading also doesn't overlap with Ben's work. I'll keep it, too.

Comment 8 by falken@chromium.org, Jul 19 2016

Owner: tyoshino@chromium.org
In the change in #7, I've just added a test for checking what's exposed.

Chrome is still not conforming to the spec.
Blocking: 571722
Review is ongoing: https://codereview.chromium.org/2254693002/
Blocking: 638552
Blocking: 709838
Project Member

Comment 14 by bugdroid1@chromium.org, May 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a3e413c07f490356a2a9b5cf623954ed9d35827

commit 6a3e413c07f490356a2a9b5cf623954ed9d35827
Author: mike <mike@mikepennisi.com>
Date: Tue May 09 18:51:55 2017

Upstream service worker `fetch` tests to WPT

**fetch-request-css-base-url**

The Chromium-specific version of this test is almost identical to the
version provided by the Web Platform Test suite. The upstream version
differs only in its use of Promises for control flow. However, the
upstream version also suffers from timeout issues in the build
environment.

Rename the Chromium-specific version to reflect its deprecated status,
and re-enable the upstream version as an expected timeout.

**fetch-request-xhr**

Update the upstream version to improve failure reporting and to include
novel tests from the Chromium version for HTTP header values (modified
to comply with the Fetch specification).

Update the Chromium-specific version to document its deprecated status
and invalid assertions.

Update both versions to improve state management via `Test#add_cleanup`.

**fetch-response-xhr**

Update the upstream version to avoid a race condition.

Update the Chromium-specific version to document its deprecated status.

Update both versions to improve state management via `Test#add_cleanup`.

BUG= 688116 , 595993,  658997 
R=mek@chromium.org

Review-Url: https://codereview.chromium.org/2862353002
Cr-Commit-Position: refs/heads/master@{#470395}

[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr.https.html
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-response-xhr.https.html
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-iframe.https.html
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-response-xhr-iframe.https.html
[rename] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-request-css-base-url.html
[rename] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-request-xhr.html
[rename] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.fetch-response-xhr.html
[modify] https://crrev.com/6a3e413c07f490356a2a9b5cf623954ed9d35827/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-iframe.html

Blocking: 678905
We currently have TestExpectation lines:

crbug.com/595993 external/wpt/service-workers/service-worker/fetch-header-visibility.https.html [ Failure ]
crbug.com/595993 external/wpt/service-workers/service-worker/request-end-to-end.https.html [ Failure ]

Actually instead of TestExpectation of Failure we should have -expected.txt files.
Project Member

Comment 16 by bugdroid1@chromium.org, May 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7d76092f05699bab9d8d9c3fd5433f04dfd3334

commit b7d76092f05699bab9d8d9c3fd5433f04dfd3334
Author: mike <mike@mikepennisi.com>
Date: Mon May 22 16:11:31 2017

Upstream service worker "request" tests to WPT

**request-body-blob**

Re-locate test file to Web Platform Test directory for eventual
automated upstreaming. Update resource URLs to suitable values for that
project. Schedule frame removal to occur following test completion.
Introduce a "use strict" directive.

**request-end-to-end**

This test exists in both WPT and the Chromium project tree, although the
two implementations differ in the following ways:

- Test structure - the WPT version uses `async_test` while the Chromium
  version uses `promise_test`
- Client-worker communication - the WPT version mediates communication
  between the client and the service worker via a dedicated
  MessageChannel. The Chromium version implements this communication via
  the "Handle Fetch" mechanism.
- "User-Agent" header assertion - the WPT version asserts that the
  "User-Agent" header is not set while the Chromium version asserts that
  it is set to the value of the client document's `navigator.userAgent`
  string (the former is correct)
- "Request construction assertion" - the Chromium version asserts that
  the expected exception is thrown when attempting to construct a new
  Request instance from the intercepted instance. The WPT version makes
  no such assertion.

Modify the WPT version to use the test structure from the Chromium
version, as this improves test readability. Adopt the communication
solution from the Chromium version, as this improves maintainability and
makes the test less susceptible to failure from unrelated bugs. Persist
the "User-Agent" assertion from the WPT version and incorporate the
"Request construction" assertion from the Chromium version. Add "use
strict" directives to the test files.

Because Chromium continues to fail the WPT version of the test, the
Chromium version cannot be removed without negatively effecting test
coverage in that project. Persist the Chromium version of the test, but
re-name the file, remove the faulty assertion, and add a comment
documenting its deprecated status.

BUG= 688116 , 595993
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2889153004
Cr-Commit-Position: refs/heads/master@{#473587}

[rename] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/request-body-blob.https.html
[modify] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/request-end-to-end.https.html
[rename] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/request-body-blob-iframe.html
[rename] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/request-body-blob-worker.js
[modify] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/request-end-to-end-worker.js
[rename] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.request-end-to-end.html
[modify] https://crrev.com/b7d76092f05699bab9d8d9c3fd5433f04dfd3334/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-end-to-end-worker.js

Comment 17 by leon....@intel.com, Jun 16 2017

Blocking: 658997

Comment 18 by leon....@intel.com, Jun 16 2017

Cc: leon....@intel.com
Project Member

Comment 19 by bugdroid1@chromium.org, Jun 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6357aabd4ce4e2f51b3d897f8cd3e0b2da72af4

commit e6357aabd4ce4e2f51b3d897f8cd3e0b2da72af4
Author: tansell <tansell@chromium.org>
Date: Fri Jun 23 05:02:59 2017

LayoutTests: Disabling more service-worker tests.

These tests are flaky on Mac and likely flaky everywhere.

TBR=dpranke@chromium.org,mcgreevy@chromium.org,qyearsley@chromium.org,jeffcarp@chromium.org,horo@chromium.org
BUG=595993
NOTRY=true

Review-Url: https://codereview.chromium.org/2950253003
Cr-Commit-Position: refs/heads/master@{#481807}

[modify] https://crrev.com/e6357aabd4ce4e2f51b3d897f8cd3e0b2da72af4/third_party/WebKit/LayoutTests/TestExpectations

Blockedon: 715640
I think this will just be fixed by S13nServiceWorker so it may make sense to just dupe this bug there. request-end-to-end.https.html is already passing on the Mojo Linux (NetworkService) bot now:

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_tests&tests=request-end-to-end.https.html
Issue 771815 has been merged into this issue.
Owner: falken@chromium.org
Reassigning to falken@.
Investigated a little. For reference, the fetch spec has a nice informative note here:
https://fetch.spec.whatwg.org/#http-header-layer-division

It's not really clear to me why User-Agent must be set after the SW.

Origin header I suppose makes sense since it's something about CORS which is after SW and reissuing a request with fetch() could confuse things.

Chrome also violates in the other direction because accept-language is set after the SW.
"because accept-language is set" -> "because Chrome sets accept-language after the SW, and the spec says to set it before."

Sign in to add a comment