Bug 201353 – Resource Load Statistics: Downgrade all third-party referrer headers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

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.

[Radar WebKit Bug Importer]

<rdar://problem/54895650>

[John Wilander]

Created attachment 377755 [details]
Patch

[Chris Dumez]

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?

[John Wilander]

I'm looking into it. I didn't think ITP was on for any of these tests. The downgrade only happens under that setting.

[John Wilander]

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?

[Chris Dumez]

(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.

[youenn fablet]

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.

[John Wilander]

(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?

[John Wilander]

(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. :)

[youenn fablet]

> > 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).

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[John Wilander]

(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.

[John Wilander]

Created attachment 378694 [details]
Patch

[John Wilander]

Comment on attachment 378694 [details]
Patch

Removing the review flag for now until I see how this thing is doing on the bots.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[John Wilander]

Created attachment 378923 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[John Wilander]

Created attachment 378943 [details]
Patch

[John Wilander]

Created attachment 378945 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Kate Cheney]

Created attachment 379581 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Kate Cheney]

Created attachment 379593 [details]
Patch

[Kate Cheney]

trying to see if I can get this to pass mac-wk2 tests.

[Kate Cheney]

Created attachment 379616 [details]
Patch

[John Wilander]

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. :)

[Kate Cheney]

Created attachment 379646 [details]
Patch

[Kate Cheney]

(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 :)

[Brent Fulgham]

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'));

[Kate Cheney]

Created attachment 379701 [details]
Patch

[Kate Cheney]

Created attachment 379704 [details]
Patch

[Brent Fulgham]

Comment on attachment 379704 [details]
Patch

excellent! r=me.

[WebKit Commit Bot]

Comment on attachment 379704 [details]
Patch

Clearing flags on attachment: 379704

Committed r250413: <https://trac.webkit.org/changeset/250413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.