New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 845683 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Proj-Servicification

Blocking:
issue 832749



Sign in to add a comment

Support modifying headers in URLLoader::FollowRedirect

Project Member Reported by chongz@chromium.org, May 22 2018

Issue description

Problem:
Currently |network::mojom::URLLoader::FollowRedirect()| takes no argument and simply reuses existing headers for the redirect.

However consumers like DMServer ( issue 832749 ) may want to add headers based on redirected URLs.

Proposal:
Update the interface to
`FollowRedirect(network.mojom.HttpRequestHeaders? modified_request_headers)`
so consumers have a chance to modify headers.

Implementation Plan:
1. Update the interface and guard non-empty impl with DCHECKs.
2. Handle |modified_request_headers| in impls and remove DCHECKs gradually.

 

Comment 1 by chongz@chromium.org, May 22 2018

Blocking: 832749
Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2018

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

commit f19dde9e195d2810fc60ff51bdc927111ed3f794
Author: Chong Zhang <chongz@chromium.org>
Date: Wed May 23 04:33:59 2018

Add modified_request_headers parameter to URLLoader::FollowRedirect

Consumers like Device Management Service ( crbug.com/832749 ) need to
add extra headers on redirect.

This CL:
1. Adds |modified_request_headers| parameter in 'url_loader.mojom' and
   all |URLLoader::FollowRedirect| overrides.
2. Adds |DCHECK(!modified_request_headers.has_value())| in all
   non-empty override implementations.
3. Use |base::nullopt| for all call-sites.

This CL doesn't have behavior change.

Bug:  845683 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I45cae17220b1e4179d9617c802718f794918e535
Reviewed-on: https://chromium-review.googlesource.com/1065584
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560942}
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/chrome/browser/extensions/chrome_url_request_util.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/components/download/internal/common/resource_downloader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/appcache/appcache_url_loader_job.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/appcache/appcache_url_loader_job.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/devtools/devtools_url_loader_interceptor.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/file_url_loader_factory.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/fileapi/file_system_url_loader_factory.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/loader/prefetch_url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/loader/prefetch_url_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_installed_script_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_installed_script_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_navigation_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/service_worker/service_worker_new_script_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/shared_worker/shared_worker_script_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/shared_worker/shared_worker_script_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/web_package/signed_exchange_cert_fetcher_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/web_package/web_package_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/browser/web_package/web_package_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/common/throttling_url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/common/throttling_url_loader_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/renderer/fetchers/resource_fetcher_impl.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/cors/cors_url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/cors/cors_url_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/network_service_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/public/cpp/simple_url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/public/cpp/simple_url_loader_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/public/mojom/url_loader.mojom
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/url_loader.h
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/services/network/url_loader_unittest.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/storage/browser/blob/blob_url_loader.cc
[modify] https://crrev.com/f19dde9e195d2810fc60ff51bdc927111ed3f794/storage/browser/blob/blob_url_loader.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 1 2018

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

commit 7607f1ffefbd10026803a656f5ca4e722e6ed127
Author: Chong Zhang <chongz@chromium.org>
Date: Fri Jun 01 20:52:20 2018

Support add/overwrite redirect request headers in NavigationURLLoader

Previous CL (https://crrev.com/c/1065584) added
|modified_request_headers| to "url_loader.mojom" w/o behavior changes.

This CL:
* Added actual supports in "navigation_url_loader_impl.cc" as well as
  "throttling_url_loader.cc", "url_request.cc", and "url_loader.cc".
* Added unittests.

Browser tests will be added together with Device Management Service
in the followup CLs.

Note: Deleting headers are not supported yet, but shouldn't be too hard
to add.

Bug:  845683 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie07f36d3492b9b203f9da6ab67aaafb5182ab6dc
Reviewed-on: https://chromium-review.googlesource.com/1072643
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563787}
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/components/cronet/cronet_url_request.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/appcache/appcache_url_loader_request.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/navigation_url_loader.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/navigation_url_loader_impl.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/browser/shared_worker/shared_worker_script_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/common/throttling_url_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/common/throttling_url_loader.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/test/test_navigation_url_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/content/test/test_navigation_url_loader.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/spdy/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/redirect_util.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/redirect_util.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/redirect_util_unittest.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/url_request.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/url_request.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/url_request_job.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/url_request_job.h
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/services/network/url_loader.cc
[modify] https://crrev.com/7607f1ffefbd10026803a656f5ca4e722e6ed127/services/network/url_loader_unittest.cc

Cc: yhirano@chromium.org
I think having DCHECK is not a good idea, as compromised renderers can make an invalid redirect response. We should fail the request instead.
The DCHECKs are not for security purposes - the headers will just be ignored.  They're to alert if unimplemented methods are being used.
But that lets a renderer to crash the entire browser, which is not good. I heard that DCHECK-enabled build is distributed to (some?) canary users and that affects them.
I'm honestly not sure we care about such a niche case.  Compromised renderers being able to crash users with Chrome Canary with DCHECKs installed doesn't seem like something worth worrying about, particularly as these DCHECKs serve a real development purpose.
yhirano@ Thanks for the comments! Do we have a doc or discussion about why we should avoid using DCHECKs in renderer?

I can only find the following code style, and it feels to me that it's a legit usage to assert unhandled cases with DCHECK: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 22 2018

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

commit a4e12afac8d44d768890fb3b2b697341eec63861
Author: Chong Zhang <chongz@chromium.org>
Date: Fri Jun 22 01:51:30 2018

[NetworkService] Support adding policy headers for DMServer

This CL:
1. Allows chrome to add policy headers for navigations and redirects
   via content API.
2. Removes the old |PolicyHeaderIOHelper| codepath and moves policy
   header tests to a new location.
3. Adds a move constructor/assignment for |net::HttpRequestHeaders|.

Bug:  832749 , 845683 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I713ed30da33ba5feb5f437f8927b173c2f84d91a
Reviewed-on: https://chromium-review.googlesource.com/1100393
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Drew Wilson <atwilson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569503}
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client_browsertest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/loader/chrome_resource_dispatcher_host_delegate_browsertest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/BUILD.gn
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper.cc
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper.h
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper_unittest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service_unittest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/detachable_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/intercepting_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/null_resource_controller.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/null_resource_controller.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_controller.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_handler.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_loader.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/public/browser/content_browser_client.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/net/http/http_request_headers.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/net/http/http_request_headers.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Status: Fixed (was: Started)
Mark as fixed since the implementation required by DMServer is done. Will leave DCHECKs for related owners if they ever hit it.

Sign in to add a comment