Bug 201353

Summary: Resource Load Statistics: Downgrade all third-party referrer headers
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, cgarcia, commit-queue, dbates, ews-watchlist, katherine_cheney, mcatanzaro, mkwst, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201319
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future
none
Patch
none
Archive of layout-test-results from ews214 for win-future
none
Patch
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Archive of layout-test-results from ews214 for win-future
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Wilander 2019-08-30 14:03:10 PDT
When tracking protections are enabled, we should downgrade all third-party referrers to their origins. Note that this downgrade will be specific to Cocoa so other ports will have to adopt as they see fit.
Comment 1 Radar WebKit Bug Importer 2019-08-30 14:03:43 PDT
<rdar://problem/54895650>
Comment 2 John Wilander 2019-08-30 15:23:59 PDT
Created attachment 377755 [details]
Patch
Comment 3 Chris Dumez 2019-08-30 16:18:34 PDT
Comment on attachment 377755 [details]
Patch

Looks like this causes quite a few layout test failures, a lot of them in web-platform-tests. Aren't we concerned about compatibility here?
Comment 4 John Wilander 2019-08-30 16:19:50 PDT
I'm looking into it. I didn't think ITP was on for any of these tests. The downgrade only happens under that setting.
Comment 5 John Wilander 2019-08-30 16:22:08 PDT
Hmm. The layout tests all seem to turn ITP on. At least for Cocoa. Maybe that's desirable. How are WPT run by others? With our settings?
Comment 6 Chris Dumez 2019-08-30 16:22:56 PDT
(In reply to John Wilander from comment #5)
> Hmm. The layout tests all seem to turn ITP on. At least for Cocoa. Maybe
> that's desirable. How are WPT run by others? With our settings?

They run the tests in Safari or STP so yes, with ITP enabled.
Comment 7 youenn fablet 2019-09-03 23:34:21 PDT
Comment on attachment 377755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377755&action=review

> Source/WebKit/ChangeLog:11
> +        Cocoa so other ports will have to adopt as they see fit.

It might be good to ping other port maintainers on their plan related to ITP.
Ideally this code would be platform generic.

Some tests are failing, ITP should probably be disabled for all WPT tests, the easiest way might be to update testharnessreport.js.
Comment 8 John Wilander 2019-09-04 05:41:36 PDT
(In reply to youenn fablet from comment #7)
> Comment on attachment 377755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377755&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        Cocoa so other ports will have to adopt as they see fit.
> 
> It might be good to ping other port maintainers on their plan related to ITP.
> Ideally this code would be platform generic.
> 
> Some tests are failing, ITP should probably be disabled for all WPT tests,
> the easiest way might be to update testharnessreport.js.

Thanks!

I assume that’s upstreaming some change to testharnessreport.js? Do I first land a change locally and then do a PR somewhere?
Comment 9 John Wilander 2019-09-04 05:42:24 PDT
(In reply to John Wilander from comment #8)
> (In reply to youenn fablet from comment #7)
> > Comment on attachment 377755 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=377755&action=review
> > 
> > > Source/WebKit/ChangeLog:11
> > > +        Cocoa so other ports will have to adopt as they see fit.
> > 
> > It might be good to ping other port maintainers on their plan related to ITP.
> > Ideally this code would be platform generic.
> > 
> > Some tests are failing, ITP should probably be disabled for all WPT tests,
> > the easiest way might be to update testharnessreport.js.
> 
> Thanks!
> 
> I assume that’s upstreaming some change to testharnessreport.js? Do I first
> land a change locally and then do a PR somewhere?

Locally == our open source repo, not just my machine. :)
Comment 10 youenn fablet 2019-09-04 05:45:43 PDT
> > I assume that’s upstreaming some change to testharnessreport.js? Do I first
> > land a change locally and then do a PR somewhere?
> 
> Locally == our open source repo, not just my machine. :)

testharnessreport.js is specific to WebKit.
You just have to edit LayoutTests/resources/testharnessreport.js
Tthere are already some lines dedicated to tweaking the test configuration for WPT tests only (tests served by wpt server basically).
Comment 11 Michael Catanzaro 2019-09-04 05:57:51 PDT
(In reply to youenn fablet from comment #7)
> It might be good to ping other port maintainers on their plan related to ITP.
> Ideally this code would be platform generic.

I guess this is why I was CCed?

My opinion is that GTK/WPE should prioritize implementing ITP. I know it could be ready in time for 2.28 if planned for. Limiting information available in the referrer makes a lot of sense to me, so implementing this same check in NetworkDataTaskSoup.cpp would be good.

Regarding ITP and related behavior changes, I'd generally consider whatever Apple does to be the desired behavior, and any divergence from that should be treated as a port bug unless we have reason to do otherwise.
Comment 12 Michael Catanzaro 2019-09-04 06:00:39 PDT
(In reply to Michael Catanzaro from comment #11)
> Limiting information
> available in the referrer makes a lot of sense to me, so implementing this
> same check in NetworkDataTaskSoup.cpp would be good.

There's a lot of network-related code that *could* be platform-generic, but not without significant refactoring, and nobody expects John to do all that refactoring himself as he continues to tweak ITP. Adding layout tests is great because it lets port maintainers know where we need changes to match the Cocoa behavior.
Comment 13 John Wilander 2019-09-04 06:07:40 PDT
(In reply to Michael Catanzaro from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > Limiting information
> > available in the referrer makes a lot of sense to me, so implementing this
> > same check in NetworkDataTaskSoup.cpp would be good.
> 
> There's a lot of network-related code that *could* be platform-generic, but
> not without significant refactoring, and nobody expects John to do all that
> refactoring himself as he continues to tweak ITP. Adding layout tests is
> great because it lets port maintainers know where we need changes to match
> the Cocoa behavior.

Thanks for this update, Michael. That’s helpful. There are a couple of times where I’ve deliberately made an ITP change specific to Cocoa since I thought other ports wanted to decide when and if to adopt these iterative changes. I will try to reverse that thinking and share as much functionality as possible going forward.
Comment 14 John Wilander 2019-09-12 17:39:21 PDT
Created attachment 378694 [details]
Patch
Comment 15 John Wilander 2019-09-12 17:39:53 PDT
Comment on attachment 378694 [details]
Patch

Removing the review flag for now until I see how this thing is doing on the bots.
Comment 16 EWS Watchlist 2019-09-12 19:07:54 PDT
Comment on attachment 378694 [details]
Patch

Attachment 378694 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13027764

Number of test failures exceeded the failure limit.
Comment 17 EWS Watchlist 2019-09-12 19:07:57 PDT
Created attachment 378702 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 18 John Wilander 2019-09-16 18:26:45 PDT
Created attachment 378923 [details]
Patch
Comment 19 EWS Watchlist 2019-09-16 19:58:58 PDT
Comment on attachment 378923 [details]
Patch

Attachment 378923 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13038170

Number of test failures exceeded the failure limit.
Comment 20 EWS Watchlist 2019-09-16 19:59:00 PDT
Created attachment 378929 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 21 John Wilander 2019-09-17 00:06:33 PDT
Created attachment 378943 [details]
Patch
Comment 22 John Wilander 2019-09-17 00:35:08 PDT
Created attachment 378945 [details]
Patch
Comment 23 EWS Watchlist 2019-09-17 02:28:20 PDT
Comment on attachment 378945 [details]
Patch

Attachment 378945 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13039407

New failing tests:
http/tests/referrer-policy/unsafe-url/cross-origin-http-http.html
http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies.php
http/tests/referrer-policy/unsafe-url/cross-origin-http.https.html
http/tests/referrer-policy/no-referrer-when-downgrade/same-origin.html
http/tests/referrer-policy/no-referrer-when-downgrade/cross-origin-http-http.html
http/tests/referrer-policy/no-referrer-when-downgrade/cross-origin-http.https.html
Comment 24 EWS Watchlist 2019-09-17 02:28:23 PDT
Created attachment 378952 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 25 Kate Cheney 2019-09-25 14:53:34 PDT
Created attachment 379581 [details]
Patch
Comment 26 EWS Watchlist 2019-09-25 16:50:43 PDT
Comment on attachment 379581 [details]
Patch

Attachment 379581 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13068182

New failing tests:
http/tests/referrer-policy/unsafe-url/cross-origin-http-http.html
http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies.php
http/tests/referrer-policy/unsafe-url/cross-origin-http.https.html
http/tests/referrer-policy/no-referrer-when-downgrade/same-origin.html
http/tests/referrer-policy/no-referrer-when-downgrade/cross-origin-http-http.html
http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies-when-private-browsing-enabled.php
http/tests/referrer-policy/no-referrer-when-downgrade/cross-origin-http.https.html
Comment 27 EWS Watchlist 2019-09-25 16:50:47 PDT
Created attachment 379592 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 28 Kate Cheney 2019-09-25 16:52:55 PDT
Created attachment 379593 [details]
Patch
Comment 29 Kate Cheney 2019-09-25 16:53:42 PDT
trying to see if I can get this to pass mac-wk2 tests.
Comment 30 Kate Cheney 2019-09-25 19:46:39 PDT
Created attachment 379616 [details]
Patch
Comment 31 John Wilander 2019-09-26 08:55:58 PDT
Comment on attachment 379616 [details]
Patch

Looks good to me. Thanks for bringing this one home, Kate! I'll defer the formal r+ to someone else to not review my own code. :)
Comment 32 Kate Cheney 2019-09-26 09:21:14 PDT
Created attachment 379646 [details]
Patch
Comment 33 Kate Cheney 2019-09-26 09:21:49 PDT
(In reply to John Wilander from comment #31)
> Comment on attachment 379616 [details]
> Patch
> 
> Looks good to me. Thanks for bringing this one home, Kate! I'll defer the
> formal r+ to someone else to not review my own code. :)

Sounds good :)
Comment 34 Brent Fulgham 2019-09-26 09:38:44 PDT
Comment on attachment 379646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379646&action=review

Looks good. I think the tests could be a bit better with the creation of a common utility function, but that doesn't have to be done in this patch.

> Tools/ChangeLog:52
> +        * BuildSlaveSupport/ews-build/steps_unittest.py: Updated unit-tests accordingly.

Looks like we have accidentally included extra ChangeLog stuff here.

> LayoutTests/http/tests/referrer-policy-script/no-referrer-when-downgrade/cross-origin-http-http.html:25
> +                document.body.appendChild(scriptElement);

It seems like this block of code could be a function provided by one of our test.js files (if there is one used by all of these tests). Otherwise we may need to update 'referrerPolicy' or 'src' in a number of places if we change our test infrastructure.

Perhaps something like 

function referrerCompletionHandler(policy) { ... }

Then this becomes:

testRunner.setStatisticsShouldDowngradeReferrer(false, referrerCompletionHandler('no-referrer-when-downgrade'));

> LayoutTests/http/tests/referrer-policy-script/no-referrer/cross-origin-http-http.html:24
> +    });

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false, referrerCompletionHandler('no-referrer'));

> LayoutTests/http/tests/referrer-policy-script/origin-when-cross-origin/cross-origin-http-http.html:23
> +        document.body.appendChild(scriptElement);

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false, referrerCompletionHandler('origin-when-cross-origin'));

> LayoutTests/http/tests/referrer-policy-script/origin/cross-origin-http.https.html:27
> +    });

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false, referrerCompletionHandler('origin'));
Comment 35 Kate Cheney 2019-09-26 17:18:21 PDT
Created attachment 379701 [details]
Patch
Comment 36 Kate Cheney 2019-09-26 17:38:47 PDT
Created attachment 379704 [details]
Patch
Comment 37 Brent Fulgham 2019-09-26 21:28:55 PDT
Comment on attachment 379704 [details]
Patch

excellent! r=me.
Comment 38 WebKit Commit Bot 2019-09-26 22:13:17 PDT
Comment on attachment 379704 [details]
Patch

Clearing flags on attachment: 379704

Committed r250413: <https://trac.webkit.org/changeset/250413>
Comment 39 WebKit Commit Bot 2019-09-26 22:13:20 PDT
All reviewed patches have been landed.  Closing bug.