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
John Wilander
2019-08-30 14:03:10 PDT
Created attachment 377755 [details]
Patch
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?
I'm looking into it. I didn't think ITP was on for any of these tests. The downgrade only happens under that setting. 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? (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 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. (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? (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. :) > > 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).
(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. (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. (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. Created attachment 378694 [details]
Patch
Comment on attachment 378694 [details]
Patch
Removing the review flag for now until I see how this thing is doing on the bots.
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. 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
Created attachment 378923 [details]
Patch
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. 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
Created attachment 378943 [details]
Patch
Created attachment 378945 [details]
Patch
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 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
Created attachment 379581 [details]
Patch
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 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
Created attachment 379593 [details]
Patch
trying to see if I can get this to pass mac-wk2 tests. Created attachment 379616 [details]
Patch
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. :)
Created attachment 379646 [details]
Patch
(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 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')); Created attachment 379701 [details]
Patch
Created attachment 379704 [details]
Patch
Comment on attachment 379704 [details]
Patch
excellent! r=me.
Comment on attachment 379704 [details] Patch Clearing flags on attachment: 379704 Committed r250413: <https://trac.webkit.org/changeset/250413> All reviewed patches have been landed. Closing bug. |