Bug 179053 – Consider supporting the `referrerpolicy` attribute.

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

[Mike West]

It doesn't look like Safari TP supports the `referrerpolicy` attribute, which defines a referrer policy for a given element's request: https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-referrer-attribute

See http://w3c-test.org/referrer-policy/no-referrer-when-downgrade/attr-referrer/cross-origin/http-http/img-tag/insecure-protocol.swap-origin-redirect.http.html for example,  which Chrome and Firefox both pass. There are _lots_ of these tests that would go green with support for the attribute. :)

[Radar WebKit Bug Importer]

<rdar://problem/35275307>

[Rob Buis]

Created attachment 361628 [details]
Patch

[EWS Watchlist]

Comment on attachment 361628 [details]
Patch

Attachment 361628 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11099758

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[EWS Watchlist]

Created attachment 361629 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361628 [details]
Patch

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

New failing tests:
http/tests/referrer-policy/strict-origin-when-cross-origin/cross-origin-http-http.html
http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin.html
http/tests/security/referrer-policy-nested-subframe.html
http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin.html
http/tests/referrer-policy/strict-origin/same-origin.html
storage/indexeddb/modern/gc-closes-database-private.html

[EWS Watchlist]

Created attachment 361630 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Rob Buis]

Created attachment 361631 [details]
Patch

[Brent Fulgham]

Before landing we need to make sure this doesn’t bypass any ITO protections.

[EWS Watchlist]

Comment on attachment 361631 [details]
Patch

Attachment 361631 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11100851

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[EWS Watchlist]

Created attachment 361636 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 361640 [details]
Patch

[Rob Buis]

(In reply to Brent Fulgham from comment #8)
> Before landing we need to make sure this doesn’t bypass any ITO protections.

Did you mean ITP? And if so were you thinking about NetworkDataTaskCocoa::restrictRequestReferrerToOriginIfNeeded? AFAIK that is unrelated, but I have not looked at that code in detail.

[Ryosuke Niwa]

Please hide this behind a runtime flag.

[Rob Buis]

Created attachment 361915 [details]
Patch

[Rob Buis]

Created attachment 361918 [details]
Patch

[Rob Buis]

Created attachment 361926 [details]
Patch

[Rob Buis]

(In reply to Ryosuke Niwa from comment #13)
> Please hide this behind a runtime flag.

Done.

[Rob Buis]

Created attachment 362107 [details]
Patch

[EWS Watchlist]

Comment on attachment 362107 [details]
Patch

Attachment 362107 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11158150

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[EWS Watchlist]

Created attachment 362109 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 362123 [details]
Patch

[EWS Watchlist]

Comment on attachment 362123 [details]
Patch

Attachment 362123 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11161692

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[EWS Watchlist]

Created attachment 362137 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 362265 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 362265 [details]
Patch

This lgtm but probably someone who is more familiar with that should review. Maybe you want to send an email to webkit-dev to announce it and get more visibility.

[Rob Buis]

Created attachment 362386 [details]
Patch

[Darin Adler]

Comment on attachment 362386 [details]
Patch

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

> Source/WebCore/html/HTMLIFrameElement.cpp:103
> +        m_referrerPolicy = ReferrerPolicy::EmptyString;
> +        if (auto policy = parseReferrerPolicy(value, ReferrerPolicySource::HTTPHeader))
> +            m_referrerPolicy = *policy;

I think value_or would be a more natural way to write this than an if statement.

Also seems a little unintuitive that it is correct to pass HTTPHeader when this is coming from an attribute, not an HTTP header field.

> Source/WebCore/html/HTMLIFrameElement.cpp:125
> +    if (m_referrerPolicy != ReferrerPolicy::EmptyString)

I would have reversed this if statement.

> Source/WebCore/html/HTMLIFrameElement.cpp:126
> +        return attributeWithoutSynchronization(referrerpolicyAttr).convertToASCIILowercase();

This is a really peculiar getter style; I don’t think I’ve seen it before in DOM. More typically either the getter is entirely a reflection of the attribute (in which case we use [Reflect]) or the getter is one of a set of hard-coded specific strings, so this function would be a switch statement and not call attributeWithoutSynchronization at all. Maybe this is just an alternate implementation of the switch statement?

No stripping of leading or trailing spaces needed? Maybe if there are spaces then the entire referrer policy is ignored and we just have EmptyString. Do we have test cases covering that part of the behavior?

> Source/WebCore/html/HTMLIFrameElement.cpp:127
> +    return String();

I kinda like "{ }" better than "String()"; not sure if others agree.

> Source/WebCore/html/HTMLIFrameElement.h:45
> +    ReferrerPolicy referrerPolicy() const override { return m_referrerPolicy; }

I think our style would be to use final here rather than override (or both).

[youenn fablet]

Comment on attachment 362386 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        if valid it is used for the subframe load.

Is it supposed to have an impact on only the load of the iframe main resource or also to all sub resources of the iframe?

> Source/WebCore/ChangeLog:13
> +        Test: web-platform-tests/html/dom/reflection-embedded.html

It seems the reflection-embedded.html test is mostly checking setter and getter of the attribute.
Do we have tests to ensure the right referrer policy is used for the iframe main resource load?

> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-embedded.html:1
> +<!DOCTYPE html>

Unnecessary change?

[Rob Buis]

Created attachment 363320 [details]
Patch

[Rob Buis]

Created attachment 363331 [details]
Patch

[Rob Buis]

Created attachment 363338 [details]
Patch

[EWS Watchlist]

Comment on attachment 363338 [details]
Patch

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

New failing tests:
http/tests/referrer-policy-iframe/strict-origin/same-origin.html
http/tests/referrer-policy-iframe/strict-origin-when-cross-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http.https.html
http/tests/referrer-policy-iframe/strict-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/same-origin/cross-origin-http-http.html

[EWS Watchlist]

Created attachment 363353 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[youenn fablet]

Comment on attachment 363338 [details]
Patch

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

> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
> +    finishJSTest();

These seems like great tests. I wish we could contribute them to WPT.

To testharnessify these tests, you might be able to do the following:
- Move them to http/wpt.
- Replace port 8000 by port 8800
- Include testharness.js/testharnessreport.js instead of js-test resources.
- Replace shouldBeEqualToString by assert_equals
- Replace finishJSTest by done.
- Maybe add a title tag to have a good -expected.txt

To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
You can use /common/get-host-info.sub.js for that purpose.

[Rob Buis]

Comment on attachment 362386 [details]
Patch

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

>> Source/WebCore/html/HTMLIFrameElement.cpp:103
>> +            m_referrerPolicy = *policy;
> 
> I think value_or would be a more natural way to write this than an if statement.
> 
> Also seems a little unintuitive that it is correct to pass HTTPHeader when this is coming from an attribute, not an HTTP header field.

Using value_or now. And besides unintuitive, it was wrong of me to use HTTPHeader mode! Unlike the HTTP header, the attribute does not use the list notation but a single referrer policy value.

>> Source/WebCore/html/HTMLIFrameElement.cpp:125
>> +    if (m_referrerPolicy != ReferrerPolicy::EmptyString)
> 
> I would have reversed this if statement.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:126
>> +        return attributeWithoutSynchronization(referrerpolicyAttr).convertToASCIILowercase();
> 
> This is a really peculiar getter style; I don’t think I’ve seen it before in DOM. More typically either the getter is entirely a reflection of the attribute (in which case we use [Reflect]) or the getter is one of a set of hard-coded specific strings, so this function would be a switch statement and not call attributeWithoutSynchronization at all. Maybe this is just an alternate implementation of the switch statement?
> 
> No stripping of leading or trailing spaces needed? Maybe if there are spaces then the entire referrer policy is ignored and we just have EmptyString. Do we have test cases covering that part of the behavior?

I think I have seen that style, but I reverted to the switch now.
Surprisingly I see no coverage for the leading or trailing spaces. reflection-embedded.htm does not test it. I verified it works by using the Inspector, but maybe I need to add such tests to reflection-embedded.htm...

>> Source/WebCore/html/HTMLIFrameElement.cpp:127
>> +    return String();
> 
> I kinda like "{ }" better than "String()"; not sure if others agree.

Done.

>> Source/WebCore/html/HTMLIFrameElement.h:45
>> +    ReferrerPolicy referrerPolicy() const override { return m_referrerPolicy; }
> 
> I think our style would be to use final here rather than override (or both).

Done.

[Rob Buis]

Comment on attachment 363338 [details]
Patch

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

>> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
>> +    finishJSTest();
> 
> These seems like great tests. I wish we could contribute them to WPT.
> 
> To testharnessify these tests, you might be able to do the following:
> - Move them to http/wpt.
> - Replace port 8000 by port 8800
> - Include testharness.js/testharnessreport.js instead of js-test resources.
> - Replace shouldBeEqualToString by assert_equals
> - Replace finishJSTest by done.
> - Maybe add a title tag to have a good -expected.txt
> 
> To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
> You can use /common/get-host-info.sub.js for that purpose.

These are based on http/tests/referrer-policy. I wonder if both hierarchies need testing for more possible values (no-referrer, unsafe etc.) though?

About upstreaming these, WPT has similar/more extensive tests like:
http://w3c-test.org/referrer-policy/no-referrer/attr-referrer/same-origin/http-http/iframe-tag/swap-origin-redirect/generic.http.html
When loading those tests directly we PASS, however when importing them lots will FAIL/timeout, with a lot of Blocking access warnings, I am guessing that www1.localhost:8000 is not accepted. I think I have seen similar warnings in other WPT tests, not sure if it is a setup issue or the tests can do some more to prevent it.
So, not sure what it the best approach here. Also the patch is getting big, time to split up tests and code?

[Rob Buis]

Created attachment 363369 [details]
Patch

[Rob Buis]

I think more needs to be done for this patch, especially regarding tests, but reviews are welcome. And thanks for the reviews so far!

[Darin Adler]

Comment on attachment 363369 [details]
Patch

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

> Source/WebCore/html/HTMLIFrameElement.cpp:123
> +        return "no-referrer";

Should use the _s suffix so these get the ASCIILiteral optimization.

> Source/WebCore/html/HTMLIFrameElement.cpp:139
> +    default:
> +        return { };

We want a warning if we forget to include a case. The way to get that with some compilers is to omit "default" and put the return outside the switch statement. Please do that.

> Source/WebCore/loader/SubframeLoader.cpp:323
> +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> +        policy = ownerElement.referrerPolicy();

When Ryosuke said this should be a runtime-enabled feature, did he mean that respecting the policy should be runtime-enabled, but the DOM manifestation of the policy attribute could be included unconditionally? I’m not sure I know the state of the art for runtime-enabled DOM features.

[Rob Buis]

Created attachment 363456 [details]
Patch

[Rob Buis]

Comment on attachment 363369 [details]
Patch

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

>> Source/WebCore/html/HTMLIFrameElement.cpp:123
>> +        return "no-referrer";
> 
> Should use the _s suffix so these get the ASCIILiteral optimization.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:139
>> +        return { };
> 
> We want a warning if we forget to include a case. The way to get that with some compilers is to omit "default" and put the return outside the switch statement. Please do that.

I knew I had forgotten something about the switch :) Done.

>> Source/WebCore/loader/SubframeLoader.cpp:323
>> +        policy = ownerElement.referrerPolicy();
> 
> When Ryosuke said this should be a runtime-enabled feature, did he mean that respecting the policy should be runtime-enabled, but the DOM manifestation of the policy attribute could be included unconditionally? I’m not sure I know the state of the art for runtime-enabled DOM features.

I am not sure either, I saw at least EncryptedMedia flag works like this. Happy to change it if needed.

[Darin Adler]

Comment on attachment 363456 [details]
Patch

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

Looks fine to me; worried about that question about exposing the attribute when the feature is turned off.

> Source/WebCore/html/HTMLIFrameElement.cpp:141
> +    return { };

Could put an ASSERT_NOT_REACHED() here too. Very unimportant since it’s not really a practical possibility at all.

[Rob Buis]

Comment on attachment 362386 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        if valid it is used for the subframe load.
> 
> Is it supposed to have an impact on only the load of the iframe main resource or also to all sub resources of the iframe?

As far as I know only the load of the iframe main resource.

>> Source/WebCore/ChangeLog:13
>> +        Test: web-platform-tests/html/dom/reflection-embedded.html
> 
> It seems the reflection-embedded.html test is mostly checking setter and getter of the attribute.
> Do we have tests to ensure the right referrer policy is used for the iframe main resource load?

I copied existing http/tests/referrer-policy tests for iframe referrerpolicy attribute, but I think we also may want to test the remaining values:
https://bugs.webkit.org/show_bug.cgi?id=195269.
As mentioned there are WPT tests as well but due to localhost/127.0.0.1 issues we can't run them.

>> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-embedded.html:1
>> +<!DOCTYPE html>
> 
> Unnecessary change?

Fixed.

[Rob Buis]

Comment on attachment 363338 [details]
Patch

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

>>> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
>>> +    finishJSTest();
>> 
>> These seems like great tests. I wish we could contribute them to WPT.
>> 
>> To testharnessify these tests, you might be able to do the following:
>> - Move them to http/wpt.
>> - Replace port 8000 by port 8800
>> - Include testharness.js/testharnessreport.js instead of js-test resources.
>> - Replace shouldBeEqualToString by assert_equals
>> - Replace finishJSTest by done.
>> - Maybe add a title tag to have a good -expected.txt
>> 
>> To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
>> You can use /common/get-host-info.sub.js for that purpose.
> 
> These are based on http/tests/referrer-policy. I wonder if both hierarchies need testing for more possible values (no-referrer, unsafe etc.) though?
> 
> About upstreaming these, WPT has similar/more extensive tests like:
> http://w3c-test.org/referrer-policy/no-referrer/attr-referrer/same-origin/http-http/iframe-tag/swap-origin-redirect/generic.http.html
> When loading those tests directly we PASS, however when importing them lots will FAIL/timeout, with a lot of Blocking access warnings, I am guessing that www1.localhost:8000 is not accepted. I think I have seen similar warnings in other WPT tests, not sure if it is a setup issue or the tests can do some more to prevent it.
> So, not sure what it the best approach here. Also the patch is getting big, time to split up tests and code?

For the record, I think http/tests/referrer-policy are very easy to read, much easier than wpt/referrer-policy ones, but the latter are more exhaustive. I think we can go with the (expanded) http/tests/referrer-policy tests for now.

When I looked again at importing wpt/referrer-policy tests, I noticed some more could be made to pass by using get-host-info.sub.js, but I think not all of them could be made to pass. I assume there are more wpt tests that have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?

[youenn fablet]

> When I looked again at importing wpt/referrer-policy tests, I noticed some
> more could be made to pass by using get-host-info.sub.js, but I think not
> all of them could be made to pass. I assume there are more wpt tests that
> have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?

WPT upstream would like WebKit to support things like www1.localhost but so far we have not been able to find a simple and proper solution that would be easy for both developers and bots.

I am surprised we could not use everywhere get-host-info.sub.js for these tests, though it is true it might be less convenient since you will need to implement in JS the templating done by the WPT server.

> Source/WebCore/loader/SubframeLoader.cpp:322
> +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())

You could remove that if check since it should be EmptyString when not exposing the attribute.
You could add an ASSERT I guess.

> Looks fine to me; worried about that question about exposing the attribute
> when the feature is turned off.

This patch makes it so the attribute is not exposed when the feature is turned off. This allows feature detection and ensures that there is no change of functionality in case it is off.

[Rob Buis]

Created attachment 363620 [details]
Patch

[EWS Watchlist]

Comment on attachment 363620 [details]
Patch

Attachment 363620 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11375818

New failing tests:
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html

[EWS Watchlist]

Created attachment 363634 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 363640 [details]
Patch

[EWS Watchlist]

Comment on attachment 363640 [details]
Patch

Attachment 363640 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11378941

New failing tests:
fast/events/beforeunload-alert-user-interaction2.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html

[EWS Watchlist]

Created attachment 363644 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 363647 [details]
Patch

[EWS Watchlist]

Comment on attachment 363647 [details]
Patch

Attachment 363647 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11380629

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

[EWS Watchlist]

Created attachment 363657 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363647 [details]
Patch

Attachment 363647 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11380816

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 363665 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 363667 [details]
Patch

[Rob Buis]

(In reply to youenn fablet from comment #45)
> 
> 
> > When I looked again at importing wpt/referrer-policy tests, I noticed some
> > more could be made to pass by using get-host-info.sub.js, but I think not
> > all of them could be made to pass. I assume there are more wpt tests that
> > have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?
> 
> WPT upstream would like WebKit to support things like www1.localhost but so
> far we have not been able to find a simple and proper solution that would be
> easy for both developers and bots.
> 
> I am surprised we could not use everywhere get-host-info.sub.js for these
> tests, though it is true it might be less convenient since you will need to
> implement in JS the templating done by the WPT server.

I tried using get-host-info.sub.js before and the no-redirect tests were fine, but not the redirect ones.
I did not investigate further but was suspecting get_swapped_origin_netloc:
https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/generic/subresource/subresource.py

> > Source/WebCore/loader/SubframeLoader.cpp:322
> > +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> 
> You could remove that if check since it should be EmptyString when not
> exposing the attribute.
> You could add an ASSERT I guess.

I am not sure if that was your suggestion, but I moved the flag check check to IFrameElement::referrerPolicy, which gives a bit cleaner code.

[youenn fablet]

> I tried using get-host-info.sub.js before and the no-redirect tests were
> fine, but not the redirect ones.
> I did not investigate further but was suspecting get_swapped_origin_netloc:
> https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/
> generic/subresource/subresource.py

Indeed, this would need to be updated.
Maybe any request triggering subresource.py should have as query parameter the actual cross-origin domain to use.

This makes subresource.py more generic and the JS test is solely in control of which cross-origin domain to use, through get-host-info.sub.js.

> > > Source/WebCore/loader/SubframeLoader.cpp:322
> > > +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> > 
> > You could remove that if check since it should be EmptyString when not
> > exposing the attribute.
> > You could add an ASSERT I guess.
> 
> I am not sure if that was your suggestion, but I moved the flag check check
> to IFrameElement::referrerPolicy, which gives a bit cleaner code.

Sounds good.

[Rob Buis]

(In reply to youenn fablet from comment #59)
> > I tried using get-host-info.sub.js before and the no-redirect tests were
> > fine, but not the redirect ones.
> > I did not investigate further but was suspecting get_swapped_origin_netloc:
> > https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/
> > generic/subresource/subresource.py
> 
> Indeed, this would need to be updated.
> Maybe any request triggering subresource.py should have as query parameter
> the actual cross-origin domain to use.
> 
> This makes subresource.py more generic and the JS test is solely in control
> of which cross-origin domain to use, through get-host-info.sub.js.

Ok, should I try that as part of this bug or should I just land what we have?

[youenn fablet]

(In reply to Rob Buis from comment #60)
> Ok, should I try that as part of this bug or should I just land what we have?

I think we should land as is.
I will file a GitHub WPT issue on www.localhost to see how we should fix it upstream.
Based on that, we should work, probably upstream, to update these referrer policy tests so that they can be run in WebKit CI.

[Rob Buis]

Created attachment 363735 [details]
Patch

[Rob Buis]

(In reply to youenn fablet from comment #61)
> (In reply to Rob Buis from comment #60)
> > Ok, should I try that as part of this bug or should I just land what we have?
> 
> I think we should land as is.
> I will file a GitHub WPT issue on www.localhost to see how we should fix it
> upstream.
> Based on that, we should work, probably upstream, to update these referrer
> policy tests so that they can be run in WebKit CI.

Thanks, sounds good. I will land now that I added the remaining referrer-policy-iframe tests.

[WebKit Commit Bot]

Comment on attachment 363735 [details]
Patch

Clearing flags on attachment: 363735

Committed r242534: <https://trac.webkit.org/changeset/242534>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to Rob Buis from comment #63)
> (In reply to youenn fablet from comment #61)
> > (In reply to Rob Buis from comment #60)
> > > Ok, should I try that as part of this bug or should I just land what we have?
> > 
> > I think we should land as is.
> > I will file a GitHub WPT issue on www.localhost to see how we should fix it
> > upstream.
> > Based on that, we should work, probably upstream, to update these referrer
> > policy tests so that they can be run in WebKit CI.
> 
> Thanks, sounds good. I will land now that I added the remaining
> referrer-policy-iframe tests.

Filed https://github.com/web-platform-tests/wpt/issues/15692.
The idea would be to use hosts[alt] wherever simple cross origin loads are required and use domains[www] for the case where hierarchical URLS do matter.