1261003 - W3C referrerpolicy content attribute doesn't work in FF

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Thomas Nguyen (:tnguyen)]

Running web platform test [1] on FF, it fails in many cases because referrerpolicy content attribute doesn't work.

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/referrer-policy-test-case.js#79
(for example one case : /referrer-policy/no-referrer/attr-referrer/cross-origin/http-http/iframe-tag/generic.keep-origin-redirect.http.html)

Tried to change referrerpolicy in this line to IDL attribute referrerPolicy and the case passed

Running on Canary Chrome I had an opposite result : referrerpolicy works and referrePolicy does not work. 
[1] https://github.com/w3c/web-platform-tests/tree/master/referrer-policy

Comment 1

[Thomas Nguyen (:tnguyen)]

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/common.js#28
I think content attribute should be set in more official way like setAttribute(attr, attributes[attr]).

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So this is specifically about the *IDL* attribute rather than the *content* attribute?

Comment 3

[Thomas Nguyen (:tnguyen)]

More clearly, 
In current web-platform-test, line [2] set attribute which is defined in [1] (referrerpolicy - content attribute in this case)
AFAIK, content attribute is the attribute (set from the content HTML) and could be set in javascript using element.setAttribute() (I have no idea but it seems FF does not support set using element[attr] = xxx, we should use setAttribute())
On the other hand, IDL attribute is also known as a JavaScript property could be set using JavaScript like element.idlAttr = xxx (or element[idlAttr] = xxx).

That's why if we replace referrerpolicy (content attr) to referrerPolicy (idl attr) in line [1], we could pass the test.
Or, changing the line [2] to use setAttribute() also passes. It would make more sense. 
[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/referrer-policy-test-case.js#79 
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/common.js#28

(In reply to :Ms2ger from comment #2)
> So this is specifically about the *IDL* attribute rather than the *content*
> attribute?

I am not sure the object of these tests is IDL or content attribute, personally I think it should be idl attribute. Anyway, purpose of the tests is triggering referrer policy, so both of them are fine.
Thanks

Comment 4

[Franziskus Kiefer [:franziskus]]

ok, looks like this an error in the web platform tests. filed https://github.com/w3c/web-platform-tests/issues/2822.

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: IDL attribute

Bracket used in [3] should be used to access property of element (or IDL attribute in this case) I think we should change content attribute (referrerpolicy) to IDL attribute (referrerPolicy) in [1]
Referrer policy IDL and content attribute are described in [3]
[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/referrer-policy-test-case.js#79 
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/common.js#28
[3] https://w3c.github.io/webappsec-referrer-policy/#html-image-element-attributes
Hi Franziskus, Josh. Could you please review that?

Comment 6

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8744773 [details] [diff] [review]
IDL attribute

I think web-platform tests should not be enabled ( which is disabled in meta folder [1]) in this bug.
It should be enabled in Bug 1168540 after all dependencies are fixed, because one failed case might be caused by several reasons.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/referrer-policy

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We should enable them now, no question about it. The only reason they were disabled is <https://github.com/w3c/web-platform-tests/issues/1874>, which is long fixed.

Comment 8

[Thomas Nguyen (:tnguyen)]

(In reply to :Ms2ger from comment #7)
> We should enable them now, no question about it. The only reason they were
> disabled is <https://github.com/w3c/web-platform-tests/issues/1874>, which
> is long fixed.
At least, due to Bug 1223838, perElementRefererer policy preference is set to false. Then setting referrerPolicy attribute on image element has no effect. If we enable them here, there will be a lot failures on try.

Comment 9

[(no longer active)]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #8)
> (In reply to :Ms2ger from comment #7)
> > We should enable them now, no question about it. The only reason they were
> > disabled is <https://github.com/w3c/web-platform-tests/issues/1874>, which
> > is long fixed.
> At least, due to Bug 1223838, perElementRefererer policy preference is set
> to false. Then setting referrerPolicy attribute on image element has no
> effect. If we enable them here, there will be a lot failures on try.

You can enable the pref for all of the tests in a directory by creating a __dir__.ini file like this: <https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/service-workers/service-worker/__dir__.ini>.  With that you should be able to enable all of these tests.  The test harness would set the pref before running the tests so everything should work correctly.

Does this make sense?

Comment 10

[Thomas Nguyen (:tnguyen)]

Bug 1223838, comment 6, there're 76 cases failed with this bug (and more than a half of them also contains bug 1178337, and bug 1261298).

I will try with you suggestion, but only enable the bugs does not contain bug 1178337, and bug 1261298.
Thanks Ehsan

Comment 11

[Thomas Nguyen (:tnguyen)]

Bug 1223838, comment 6, there're 76 cases failed with this bug (and more than a half of them also contain bug 1178337, and bug 1261298).

I will try with you suggestion, but only enable the case does not contain bug 1178337, and bug 1261298.
Thanks Ehsan

Comment 12

[(no longer active)]

Cool.  FWIW we can also have tests that are expected to fail.  Once you have set the pref, you can use this tool <http://wptrunner.readthedocs.io/en/latest/expectation.html#generating-expectation-files> to update the expectation data.  I'll clear the review request for now.

Comment 13

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf2ef858027a

Comment 14

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7801eb7d3703

Comment 15

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2665ace157e

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: Update meta file

Comment 17

[Thomas Nguyen (:tnguyen)]

Try looks good.
We still have tests that are expected to fail in img-tag due to bug 1257249

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aff4dae71ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25f97b1cf4d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3aff4dae71ac
https://hg.mozilla.org/mozilla-central/rev/a25f97b1cf4d

Comment 20

[Ethan Tseng [:ethan]]

Thomas, thanks for fixing this!