Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature policy for client hints, used for third-party subresources. #16720

Merged
merged 2 commits into from Jun 10, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented May 7, 2019

This disables all client hints for third parties by default, except for
the simplified UA hint. Feature Policy can then be used to delegate
specific hints. Currently, only the hints which were previously being
sent to third parties on Android are available for delagation. A follow-
up CL adds the remaining ones.

Bug: 968201
Change-Id: Idea42e814078592f3bdaec67bd931a99cfaea046
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1567927
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664347}


Combined by @foolip with a follow-up change:

Add the remaining Client Hints to Feature Policy.

This allows all client hints to be potentially delegated to third-party
requests. By default, no hints (Except for the simplified UA hint) are
sent with any third party resource requests, but these can be enabled
for specific origins through the use of Feature Policy.

Bug: 968201
Change-Id: I60a94deb0e5553b85da351f08cbabfc8ae0f6e65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1584400
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664498}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1567927 branch 3 times, most recently from 87d462a to aeba71b Compare May 13, 2019 19:44
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1567927 branch 3 times, most recently from 0179b13 to 6e490cd Compare May 29, 2019 18:10
@rakuco rakuco closed this May 31, 2019
@rakuco rakuco reopened this May 31, 2019
@rakuco
Copy link
Member

rakuco commented May 31, 2019

I've pinged the CL authors to check if the flakiness is something they could look at.

@Hexcles Hexcles closed this Jun 3, 2019
@Hexcles Hexcles reopened this Jun 3, 2019
@foolip foolip force-pushed the chromium-export-cl-1567927 branch from 6e490cd to 831eb66 Compare June 4, 2019 11:55
@foolip
Copy link
Member

foolip commented Jun 4, 2019

From the logs of https://tools.taskcluster.net/groups/av_5AV-US8GCEQVMLK22Dw/tasks/V3EPGIvXSHKYoK9Bb1ommA/details:

Unstable results

Test Subtest Results Messages
/client-hints/accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html ERROR: 1/10, OK: 9/10
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 1/10, PASS: 9/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. PASS: 9/10, TIMEOUT: 1/10 Test timed out
/client-hints/http_equiv_accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html ERROR: 1/10, OK: 9/10
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 1/10, PASS: 9/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. PASS: 9/10, TIMEOUT: 1/10 Test timed out

I suspect this is like #15036, where the test isn't written to be possible to run multiple times in a row.

@foolip
Copy link
Member

foolip commented Jun 4, 2019

https://crrev.com/c/1584400 also builds on these tests and might change the results here. I wouldn't bet on it, but I'll push that commit to this PR to see if it does have an impact.

@foolip
Copy link
Member

foolip commented Jun 4, 2019

The logs now say:

Unstable results

Test Subtest Results Messages
/client-hints/accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html ERROR: 1/10, OK: 9/10
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 1/10, PASS: 9/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. PASS: 9/10, TIMEOUT: 1/10 Test timed out
/client-hints/http_equiv_accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html ERROR: 1/10, OK: 9/10
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 1/10, PASS: 9/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. PASS: 9/10, TIMEOUT: 1/10 Test timed out

That is the same as before AFAICT.

I've tested /client-hints/accept_ch_lifetime_same_origin_iframe.tentative.https.html locally and it does begin to fail after a few reloads. In an incognito window it then passes again but fails on reload.

Some of the failures seem related to pre-existing async_tests that don't wrap event handlers in t.step_func and result in a harness error, so I'll fix those and see if it affects the results. Maybe they'll be flaky with that fix too, unclear.

@foolip
Copy link
Member

foolip commented Jun 4, 2019

Sent those change in #17164.

cc @clelland on this issue.

This disables all client hints for third parties by default, except for
the simplified UA hint. Feature Policy can then be used to delegate
specific hints. Currently, only the hints which were previously being
sent to third parties on Android are available for delagation. A follow-
up CL adds the remaining ones.

Bug: 968201
Change-Id: Idea42e814078592f3bdaec67bd931a99cfaea046
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1567927
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664347}
This allows all client hints to be potentially delegated to third-party
requests. By default, no hints (Except for the simplified UA hint) are
sent with any third party resource requests, but these can be enabled
for specific origins through the use of Feature Policy.

Bug: 968201
Change-Id: I60a94deb0e5553b85da351f08cbabfc8ae0f6e65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1584400
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664498}
@foolip foolip force-pushed the chromium-export-cl-1567927 branch from 8e16855 to 932e188 Compare June 4, 2019 12:59
@foolip
Copy link
Member

foolip commented Jun 4, 2019

Now the logs say:

Unstable results

Test Subtest Results Messages
/client-hints/accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 8/10, PASS: 2/10 assert_false: device-memory-received expected false got true
/client-hints/accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. FAIL: 8/10, PASS: 2/10 assert_equals: expected "PASS" but got "FAIL"
/client-hints/http_equiv_accept_ch_lifetime_same_origin_iframe.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Precondition: Test that the browser does not have client hints preferences cached FAIL: 9/10, PASS: 1/10 assert_false: device-memory-received expected false got true
/client-hints/http_equiv_accept_ch_lifetime_subresource.tentative.https.html Loading of resources/do_not_expect_client_hints_headers.html did not finish. FAIL: 8/10, PASS: 2/10 assert_equals: expected "PASS" but got "FAIL"

That removes two lines from the previous round.

In https://wpt.fyi/results/client-hints?diff&filter=ADC&run_id=250780005&run_id=230730003 the results of these tests are unchanged.

I'll submit a PR adding whitespace to the two flaky tests to see if they were already flaky.

@foolip
Copy link
Member

foolip commented Jun 4, 2019

Probing in #17166.

@foolip
Copy link
Member

foolip commented Jun 4, 2019

From #17166 (comment) it looks like accept_ch_lifetime_same_origin_iframe.tentative.https.html and accept_ch_lifetime_subresource.tentative.https.html were already flaky, which leaves http_equiv_accept_ch_lifetime_subresource.tentative.https.html as possibly newly flaky.

@@ -5,6 +5,8 @@ def main(request, response):
"""

response.headers.append("Access-Control-Allow-Origin", "*")
response.headers.append("Access-Control-Allow-Headers", "*")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clelland this resource is used by http_equiv_accept_ch_lifetime_subresource.tentative.https.html.

Do you think these changes could have made that test flaky?

@foolip
Copy link
Member

foolip commented Jun 4, 2019

Giving it another spin in https://tools.taskcluster.net/groups/ITD387cHQ4iP6gWIa-hugg.

@foolip
Copy link
Member

foolip commented Jun 10, 2019

OK, the flakiness affects only Chrome and so I'm going to admin merge this and see how it fares. I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=972569 to enable the tests in Chromium.

@foolip foolip merged commit 519b228 into master Jun 10, 2019
@foolip foolip deleted the chromium-export-cl-1567927 branch June 10, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants