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

Issue 816661 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocking:
issue 735518



Sign in to add a comment

Client hints requested using only Accept-CH header should be restricted

Project Member Reported by tbansal@chromium.org, Feb 26 2018

Issue description

Origins can request client hints using Accept-CH response header. In that case, client hints are attached to the request header for all subresources contained in that page.

Additionally, the origins can also specify a persistant lifetime using Accept-CH-Lifetime header (this feature is currently under experiment). In that case, client hints are attached for all fetches from that origin for the duration provided as long as certain conditions are met: (i) The origin has the permission to store cookies and run javascript (ii) The origin of the main frame request matches the origin of the subresource fetch.

These two conditions currently do not apply when the origin has specified only the Accept-CH-header.

To bring parity between the two ways of requesting client hints, for pages where the origin has only specified Accept-CH header, Chromium should attach clients iff:  (i) The origin has the permission to store cookies and run javascript (ii) The origin of the main frame request matches the origin of the subresource fetch.
 
Cc: y...@yoav.ws
/cc Yoav for sanity check.

Tarun: yep, that looks good. This may be implicit but the other requirement for both is that hints are HTTPS-only.

Comment 2 by y...@yoav.ws, Feb 27 2018

(i) makes total sense (even if not mentioned in the spec. Should it be mentioned in the spec?)

(ii) will break existing users of CH without providing them with a deprecation period nor an alternative. https://github.com/WICG/feature-policy/issues/129 seems like it still has ways to go till it's ready to land.

Looking at https://www.chromestatus.com/metrics/feature/timeline/popularity/837 usage still seems very low (and it is an *upper bound* for breakage), so breaking third party CH here will probably be fine, but it seems like a decision that requires an "intent" thread.

Comment 3 by y...@yoav.ws, Feb 27 2018

Also, agree on the HTTPS-only front, but it probably also requires an intent, or at the very least a PSA. (we could combine all these changes into a single intent though)
Re, (ii): correct, but existing adoption of CH is also well under deprecation threshold. 

We intend to do an i2s which will simultaneous deprecate previous behavior and introduce HTTPS-only + 1P scoped behavior outlined above. The FP work will follow after this and will land separately down the road. Based on the feedback to date, availability of hints to 1P has been a more pressing need than 3P, and we're prioritizing accordingly; I don't believe it makes sense to couple these two things together at this stage.

Comment 5 by y...@yoav.ws, Feb 27 2018

> We intend to do an i2s which will simultaneous deprecate previous behavior and introduce HTTPS-only + 1P scoped behavior outlined above.

Sounds good! :)

> The FP work will follow after this and will land separately down the road. Based on the feedback to date, availability of hints to 1P has been a more pressing need than 3P, and we're prioritizing accordingly; I don't believe it makes sense to couple these two things together at this stage.

I agree there's no need to couple shipping of these solutions. All I was saying is that the removal is not trivial, so should go through an i2s. 
Thanks Yoav and Ilya. We had an i2i for Accept-CH-Lifetime (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QHI3sio6--Q). However, there was no mention of HTTPS-only or breaking 3P support for existing Accept-CH users.

Would it be better to do an i2i for Accept-CH changes and then i2s for all three of them? The other option is to do i2i&s for everything at the same time.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 27 2018

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

commit 229647bd00b39414b4bc0aac87a01ba2546bad1b
Author: Tarun Bansal <tbansal@chromium.org>
Date: Tue Feb 27 17:33:36 2018

Add browser tests for client hints without Accept-CH-Lifetime

This CL introduces no functional changes, and only verifies
the current behavior of client hints when only the
Accept-CH header is specified in the response headers.

Browser tests and Blink tests are added for cases where the origin has
specified only the Accept-CH header in the response headers. The tests
ensure that currently client hints are attached even if the origin
does not have permission to run JavaScript. Additionally, the tests
also ensure that the client hints are attached for third-party origins.

The next CL will modify these behaviors, and prevent attaching of
client hints when origin does not have permission to run scripts.
The next CL would also prevent attaching of client hints to
third-party origins. The tests added in this CL would be updated
to match that behavior in the next CL.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I90d992cd456e62f36924243972430012639efde9
Bug:  816661 
Reviewed-on: https://chromium-review.googlesource.com/938541
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539485}
[modify] https://crrev.com/229647bd00b39414b4bc0aac87a01ba2546bad1b/chrome/browser/client_hints/client_hints_browsertest.cc
[add] https://crrev.com/229647bd00b39414b4bc0aac87a01ba2546bad1b/chrome/test/data/client_hints/accept_ch_without_lifetime_img_localhost.html
[add] https://crrev.com/229647bd00b39414b4bc0aac87a01ba2546bad1b/chrome/test/data/client_hints/accept_ch_without_lifetime_img_localhost.html.mock-http-headers
[modify] https://crrev.com/229647bd00b39414b4bc0aac87a01ba2546bad1b/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/229647bd00b39414b4bc0aac87a01ba2546bad1b/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp

Given that we're well on the way here, I'd suggest we do an i2s (and the sooner the better) that includes all of the above and lays out all the proposed changes. 

Tarun: I think we have most of that already in the earlier doc we were working on.

Comment 9 by y...@yoav.ws, Feb 28 2018

> Given that we're well on the way here, I'd suggest we do an i2s (and the sooner the better) that includes all of the above and lays out all the proposed changes.

+1
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 2 2018

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

commit 3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79
Author: Tarun Bansal <tbansal@chromium.org>
Date: Fri Mar 02 18:48:00 2018

Restrict client hints to 1P origins and when script and cookies permissions are available.

Prevent client hints from being sent when script or cookies
are blocked for the origin. Also, restrict client hints for
only the first party origins. This means that subresources
that are fetched from an origin different
than the main frame origin would not get client hints.

Note that currently when both Accept-CH and
Accept-CH-Lifetime headers are present,
Chromium already enforces the check for script and cookies
permission. Additionally, it also does not attach
client hints for requests to 3P origins. This CL brings the
behavior of Accept-CH header at parity with when
both Accept-CH and Accept-CH-Lifetime headers are present.

Bug:  816661 
Change-Id: I119e6d483555989212c33786a95a6e7cbd9fccaf
Reviewed-on: https://chromium-review.googlesource.com/923865
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540568}
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/chrome/browser/client_hints/client_hints.cc
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/chrome/browser/client_hints/client_hints_browsertest.cc
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/chrome/common/client_hints/client_hints.cc
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/chrome/common/client_hints/client_hints.h
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/chrome/renderer/content_settings_observer.cc
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/3f343d7c56d84ae5a2cef3a3ad3eb80e223e5e79/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp

Labels: M-67
Status: Fixed (was: Started)
This is now fully implemented behind flag.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 19 2018

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

commit 62158e3ea5680211a69999dd5f00649248130c0d
Author: Raymes Khoury <raymes@chromium.org>
Date: Mon Mar 19 08:46:49 2018

Revert "Enable ClientHintsPersistent feature (Accept-CH-Lifetime header)."

This reverts commit 6af9df3e37f4ba34a5a5a44a5245bebce7485507.

Reason for revert: Caused a regression as outlined in
https://chromium-review.googlesource.com/c/chromium/src/+/957265#message-bfd66cb6916d1397f05a74dd7c1fca4645857147

Original change's description:
> Enable ClientHintsPersistent feature (Accept-CH-Lifetime header).
>
> blink-dev i2s thread: https://groups.google.com/a/chromium.org/d/topic/blink-dev/8RBFue7RMXQ/discussion
>
> Change-Id: I5595b3aed72ea0cece88948f69f480f5808fce6b
> Bug:  735518 , 782381 , 816661 
> TBR: raymes@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/957265
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Reviewed-by: David Dorwin <ddorwin@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Tarun Bansal <tbansal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#543786}

TBR=ddorwin@chromium.org,raymes@chromium.org,kinuko@chromium.org,tbansal@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  735518 ,  782381 ,  816661 
Change-Id: I47c42057e7d3158eeb515b993266ce0ff8e937e7
Reviewed-on: https://chromium-review.googlesource.com/968081
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543992}
[modify] https://crrev.com/62158e3ea5680211a69999dd5f00649248130c0d/chrome/browser/client_hints/client_hints_browsertest.cc
[modify] https://crrev.com/62158e3ea5680211a69999dd5f00649248130c0d/chrome/browser/content_settings/content_settings_browsertest.cc
[modify] https://crrev.com/62158e3ea5680211a69999dd5f00649248130c0d/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 20 2018

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

commit 593790112b293faaabc0f692ffe225ab91c6018e
Author: Tarun Bansal <tbansal@chromium.org>
Date: Tue Mar 20 04:53:34 2018

Enable ClientHintsPersistent feature (Accept-CH-Lifetime header).

blink-dev i2s thread: https://groups.google.com/a/chromium.org/d/topic/blink-dev/8RBFue7RMXQ/discussion

Bug:  735518 , 782381 , 816661 
Change-Id: Ic7c8a6a0eaf323d3ab736ac8e3ad2d23104ae0cb
Reviewed-on: https://chromium-review.googlesource.com/969407
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544285}
[modify] https://crrev.com/593790112b293faaabc0f692ffe225ab91c6018e/chrome/browser/client_hints/client_hints_browsertest.cc
[modify] https://crrev.com/593790112b293faaabc0f692ffe225ab91c6018e/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Sign in to add a comment