Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(38)

Issue 2254693002: Delay generation of User-Agent header to URLRequestHttpJob and accept custom User-Agent from XHR/Fe…

Created:
2 years, 10 months ago by tyoshino (SeeGerritForStatus)
Modified:
2 years ago
Reviewers:
Nate Chapin, horo
CC:
chromium-reviews, kenjibaheux+watch_chromium.org, tzik, jsbell+serviceworker_chromium.org, cbentzel+watch_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, blink-reviews-api_chromium.org, Yoav Weiss, nhiroki, Randy Smith (Not in Mondays), loading-reviews_chromium.org, loading-reviews+fetch_chromium.org, Nate Chapin, michaeln, tyoshino+watch_chromium.org, serviceworker-reviews, falken, kinuko+serviceworker, horo+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Delay generation of User-Agent header to URLRequestHttpJob and accept custom User-Agent from XHR/Fetch To conform to the Fetch Standard, User-Agent header generation should be done after consulting a ServiceWorker (see that the step is in HTTP-network-or-cache fetch which is performed after ServiceWorker). Chromium has been violating this requirement. Renderer generates a User-Agent header and it's visible to the ServiceWorker which controls the page. This spec violation also prevented allowing for custom User-Agent header which was enabled by https://github.com/whatwg/fetch/issues/37 because a User-Agent header appended by the controlled page is considered to be a author provided header by fetch() in a ServiceWorker and may cause unnecessary wrong preflight. This change fixes both of these issues while preserving the inspector's functionality to override User-Agent, by introducing a new path in the resource loader to pass the User-Agent string generated and added by FrameFetchContext to net::URLRequest instead of embedding it in the list of headers immediately in FrameFetchContext. The logic in this CL doesn't preserve the default User-Agent value overridden by the Inspector for the controlled page if it's proxied by a ServiceWorker. But this is not working even before this CL. "request" guard and "request-no-cors" guard rejects User-Agent value added by the controlled page on fetch() call in a ServiceWorker. See http://crbug.com/638552. Note about layouttest changes: User-Agent and Authorization are removed from http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization is tested by authorization-header.html and User-Agent will be tested by setrequestheader-useragent.html introduced by this CL. print-headers.cgi should be merged with echo-headers.php. This is not done in this CL not to increase the patch size any more. See http://crbug.com/638553. BUG=595993, 571722, 638552, 638553

Patch Set 1 #

Patch Set 2 : a #

Patch Set 3 : a #

Patch Set 4 : a #

Patch Set 5 : a #

Patch Set 6 : a #

Total comments: 9

Patch Set 7 : Rebase + Address #13 #

Patch Set 8 : Addressed #13 #

Patch Set 9 : Changed Android test #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -86 lines) Patch
M android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M content/child/request_info.h View 1 chunk +4 lines, -0 lines 0 comments Download
M content/child/resource_dispatcher.cc View 1 chunk +1 line, -0 lines 0 comments Download
M content/child/web_url_loader_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M content/common/resource_messages.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/common/resource_request.h View 1 chunk +4 lines, -0 lines 0 comments Download
M net/url_request/url_request.h View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
M net/url_request/url_request.cc View 1 2 3 4 5 6 2 chunks +10 lines, -0 lines 0 comments Download
M net/url_request/url_request_http_job.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M ppapi/tests/test_url_loader.cc View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/xmlhttprequest/set-dangerous-headers-local.html View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/xmlhttprequest/set-dangerous-headers-local-expected.txt View 1 2 3 4 1 chunk +7 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js View 3 chunks +5 lines, -3 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header.js View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html View 1 2 3 4 5 6 1 chunk +22 lines, -0 lines 4 comments Download
M third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js View 1 1 chunk +1 line, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html View 1 2 3 4 5 6 1 chunk +39 lines, -0 lines 5 comments Download
A third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-forwarding-worker.html View 1 2 3 4 5 6 1 chunk +63 lines, -0 lines 2 comments Download
A third_party/WebKit/LayoutTests/http/tests/fetch/window/useragent-header.html View 1 2 3 4 5 6 1 chunk +25 lines, -0 lines 2 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/extensions-headers.html View 1 2 3 4 5 1 chunk +27 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/extensions-headers-expected.txt View 1 1 chunk +2 lines, -2 lines 0 comments Download
D third_party/WebKit/LayoutTests/http/tests/inspector/resources/echo-headers.php View 1 2 3 4 1 chunk +0 lines, -9 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/user-agent-override-worker.js View 1 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/resources/echo-headers.php View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/request-end-to-end.html View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-iframe.html View 1 2 3 4 4 chunks +4 lines, -4 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/forwarding-worker.js View 1 2 3 4 5 6 1 chunk +15 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers.html View 1 4 chunks +2 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers-expected.txt View 1 2 3 4 1 chunk +22 lines, -23 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/setrequestheader-useragent.html View 1 2 3 4 1 chunk +30 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/FetchUtils.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/FrameFetchContext.cpp View 1 2 3 4 5 6 3 chunks +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 chunk +0 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebURLRequest.cpp View 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/network/ResourceRequest.h View 3 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/network/ResourceRequest.cpp View 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/network/ResourceRequestTest.cpp View 1 2 3 4 5 6 7 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/public/platform/WebURLRequest.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (13 generated)
Nate Chapin
blink LGTM
2 years, 10 months ago (2016-08-19 21:29:01 UTC) #12
horo
Do we have layouttests that check the behavior of setting custom User-Agent in a page ...
2 years, 10 months ago (2016-08-21 08:39:37 UTC) #13
mmenke
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request.h File net/url_request/url_request.h (right): https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request.h#newcode770 net/url_request/url_request.h:770: bool has_default_user_agent_; On 2016/08/21 08:39:37, horo wrote: > You ...
2 years, 10 months ago (2016-08-21 14:37:40 UTC) #15
tyoshino (SeeGerritForStatus)
On 2016/08/21 08:39:37, horo wrote: > Do we have layouttests that check the behavior of ...
2 years, 10 months ago (2016-08-23 09:42:49 UTC) #16
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Source/platform/network/ResourceRequest.h File third_party/WebKit/Source/platform/network/ResourceRequest.h (right): https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Source/platform/network/ResourceRequest.h#newcode135 third_party/WebKit/Source/platform/network/ResourceRequest.h:135: const AtomicString& defaultHTTPUserAgent() const { return m_defaultHTTPUserAgent; } On ...
2 years, 10 months ago (2016-08-23 09:42:58 UTC) #17
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc#newcode392 net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent, request_->default_user_agent()); On 2016/08/21 14:37:39, mmenke wrote: > I ...
2 years, 10 months ago (2016-08-23 10:01:47 UTC) #18
mmenke
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc#newcode392 net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent, request_->default_user_agent()); On 2016/08/23 10:01:47, tyoshino wrote: > On ...
2 years, 10 months ago (2016-08-23 14:54:33 UTC) #19
mmenke
On 2016/08/23 14:54:33, mmenke wrote: > https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc > File net/url_request/url_request_http_job.cc (right): > > https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc#newcode392 > ...
2 years, 10 months ago (2016-08-23 15:25:22 UTC) #20
horo
> Added. For (1), is a test using the empty worker sufficient? Yes. https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html File ...
2 years, 10 months ago (2016-08-24 01:38:33 UTC) #21
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc#newcode392 net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent, request_->default_user_agent()); On 2016/08/23 14:54:33, mmenke (busy) wrote: > ...
2 years, 10 months ago (2016-08-25 04:11:34 UTC) #22
mmenke
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_request_http_job.cc#newcode392 net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent, request_->default_user_agent()); On 2016/08/25 04:11:34, tyoshino wrote: > On ...
2 years, 10 months ago (2016-08-25 04:17:02 UTC) #23
mmenke
2 years ago (2017-06-12 15:12:20 UTC) #24
On 2016/08/25 04:17:02, mmenke wrote:
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
> request_->default_user_agent());
> On 2016/08/25 04:11:34, tyoshino wrote:
> > On 2016/08/23 14:54:33, mmenke (busy) wrote:
> > > Could ServiceWorker just hide the header it passes to the Javascript API,
> and
> > > then re-set it itself?  Fetch API doesn't allow setting of this header, so
> > > there's no case in which anything else is allowed to set it.  I'm
reluctant
> to
> > > expand URLRequest's surface area here, and adding yet another complex and
> > > unexpected behavior.
> > 
> > I think embedding everything into the header holder is also source of
> complexity
> > and bugs. But I understand your sentiment. I'll check if there's any way to
> make
> > both happy.
> 
> SGTM.  I think it's also worth noting that the behavior
> URLRequest::set_default_user_agent can't really be documented, without talking
> about how it's different from setting the header directly, and ServiceWorker
> (Which is not a net/ layer concept)

Removing myself from this CL.  Feel free to add me back if you pick it up again.

Powered by Google App Engine
This is Rietveld 408576698