1223838 - Enable perElementReferrer by default

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Franziskus Kiefer [:franziskus]]

Per-element referrer-policies are currently disabled by default with preference perElementReferrer. When the renaming landed (bug 1187357) we could enable this considering that the spec is in a better shape now [1].

[1] https://github.com/w3c/webappsec-referrer-policy

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: enable perElementReferrer by default

Comment 2

[Boris Zbarsky [:bzbarsky]]

This needs an intent to ship, right?

Comment 3

[Franziskus Kiefer [:franziskus]]

intent to ship: https://groups.google.com/d/topic/mozilla.dev.platform/TXFImc_r9Jo/discussion

Comment 4

[Thomas Nguyen (:tnguyen)]

Tried to run some tests and I found if I enable this preference, Bug 1101288 will appear.
I will take closer look.

Comment 5

[Thomas Nguyen (:tnguyen)]

The below case failed:
https://dxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug704320_policyset.html#40
This line fails and full referrerLevel was sent (expect none).

Comment 6

[Thomas Nguyen (:tnguyen)]

Attachment: Fix wrong policy associated with empty string

Speculative loading image: In the case there's no referrer policy attribute, currently empty string will be added and considered as default referrer policy value.
RP_Unset should be used in this case.

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Update meta file

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8751135 [details] [diff] [review]
Fix wrong policy associated with empty string

Review of attachment 8751135 [details] [diff] [review]:
-----------------------------------------------------------------

The code itself looks good to me for what it's doing.

I'm not really sure why it's necessary though. You set RP_Unset everywhere instead of an empty string, right? Maybe I'm missing something but couldn't you just check for an empty string _and_ PR_Unset where necessary instead of adding all this code?

Comment 9

[Thomas Nguyen (:tnguyen)]

Thanks Franziskus for your suggestion.
You are right. I think I got confused between [1] and [2]
[1] https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
[2] https://w3c.github.io/webappsec-referrer-policy/#terms

Removed unnecessary code.

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Fix wrong policy associated with empty string

Comment 11

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754675 [details] [diff] [review]
Fix wrong policy associated with empty string

Review of attachment 8754675 [details] [diff] [review]:
-----------------------------------------------------------------

much better, thanks :)

Comment 12

[Thomas Nguyen (:tnguyen)]

Hi Henri,
Could you please review these patches?
These patches changes:
- Change network.http.enablePerElementReferrer preference default to true 
- Handle empty_string in referrerpolicy attribute of preloading image element correctly.(The empty string corresponds to no referrer policy, or RP_Unset). That fixed failures running on try.
Thanks

Comment 13

[Henri Sivonen (:hsivonen) (not reading bugmail until 2023-12-11)]

Comment on attachment 8754675 [details] [diff] [review]
Fix wrong policy associated with empty string

Review of attachment 8754675 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't change what AttributeReferrerPolicyFromString() does, and that method is incompliant.

The HTML Standard says "An additional state is given by the empty string (which is also a valid referrer policy). The attribute's invalid value default and missing value default are both the empty string state." Furthermore, the Referrer Policy spec specifies an empty-string policy but no policy named "default": https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-empty-string . Still, ReferrerPolicy.h a) distinguishes between default and unset and b) maps default to "default". (Additionally, ReferrerPolicy.h does a whole lot of repeated case folding. I suggest making as ASCII-lowercase copy of the specified string first and then doing case-sensitive comparisons.)

Comment 14

[Henri Sivonen (:hsivonen) (not reading bugmail until 2023-12-11)]

Comment on attachment 8694659 [details] [diff] [review]
enable perElementReferrer by default

Review of attachment 8694659 [details] [diff] [review]:
-----------------------------------------------------------------

Please re-request review on this patch after the mechanics of going from an attribute value to a referrer policy enum (and the set of values that the enum can take) have been update to the spec.

Comment 15

[Thomas Nguyen (:tnguyen)]

Thanks Henri for looking at this.
I filed Bug 1275803  for fixing [1] is not compliant with specs [2]. It would be better to check and fix all usages of function ReferrerPolicyFromString [1] and update/add more test cases if necessary

In this bug, I'd like to update as your suggestion, but only for AttributeReferrerPolicyFromString which is related to referrer policy attribute in [3] to make it a good shape compliant to specs

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/ReferrerPolicy.h#54
[2] https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
[3] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/ReferrerPolicy.h#107

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: Fix wrong policy associated with empty string

Comment 17

[Henri Sivonen (:hsivonen) (not reading bugmail until 2023-12-11)]

Comment on attachment 8757163 [details] [diff] [review]
Fix wrong policy associated with empty string

Review of attachment 8757163 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Thomas Nguyen[:tnguyen] from comment #15)
> Thanks Henri for looking at this.
> I filed Bug 1275803  for fixing [1] is not compliant with specs [2]. It
> would be better to check and fix all usages of function
> ReferrerPolicyFromString [1] and update/add more test cases if necessary

The non-attribute cases are:
CSP, <meta>, HTTP header(?) and LoadImageXPCOM(). It seems bad not to sync them with the attribute right here. Why shouldn't they be synced with the attribute right here?

> In this bug, I'd like to update as your suggestion, but only for
> AttributeReferrerPolicyFromString which is related to referrer policy
> attribute in [3] to make it a good shape compliant to specs

Maybe CSP, <meta>, HTTP header(?) and LoadImageXPCOM() can be allowed to be out of sync with the attribute and be handled in the other bug (though it worries me), but to handle the attribute consistently, this patch should also use AttributeReferrerPolicyFromString instead of ReferrerPolicyFromString at
https://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#558

r- because the patch didn't change that, too.

Also, I'd be much happier about r+ing a patch that synced the non-attribute sources of policy with the spec at the same time.

Comment 18

[Thomas Nguyen (:tnguyen)]

(In reply to Henri Sivonen (:hsivonen) from comment #17)

> Maybe CSP, <meta>, HTTP header(?) and LoadImageXPCOM() can be allowed to be
> out of sync with the attribute and be handled in the other bug (though it
> worries me), but to handle the attribute consistently, this patch should
> also use AttributeReferrerPolicyFromString instead of
> ReferrerPolicyFromString at
> https://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLImageElement.
> cpp#558
> 

:) Thanks for promptly review this. 


> (In reply to Thomas Nguyen[:tnguyen] from comment #15)
> > Thanks Henri for looking at this.
> > I filed Bug 1275803  for fixing [1] is not compliant with specs [2]. It
> > would be better to check and fix all usages of function
> > ReferrerPolicyFromString [1] and update/add more test cases if necessary
> 
> The non-attribute cases are:
> CSP, <meta>, HTTP header(?) and LoadImageXPCOM(). It seems bad not to sync
> them with the attribute right here. Why shouldn't they be synced with the
> attribute right here?

Probably, it's necessary to add more test cases for non-attribute fixing (because our current implementation for non-attribute is quite different from specs, return No_referrer vs Return Unset).
I just would like to make clearer to move non-attribute cases to another bug. This bug only fixes referrer attribute of element - as title.

Comment 19

[Thomas Nguyen (:tnguyen)]

Attachment: ReferrerPolicy Attribute from string is not compliant with specs

Updated, will add test cases about this

Comment 20

[Thomas Nguyen (:tnguyen)]

Attachment: ReferrerPolicy from string is not compliant with specs

Comment 21

[Henri Sivonen (:hsivonen) (not reading bugmail until 2023-12-11)]

Comment on attachment 8757781 [details] [diff] [review]
ReferrerPolicy from string is not compliant with specs

Review of attachment 8757781 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLImageElement.cpp
@@ +568,5 @@
>        aNameSpaceID == kNameSpaceID_None &&
>        aNotify) {
> +    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue);
> +    if (!InResponsiveMode() &&
> +        referrerPolicy != RP_Unset &&

Why is RP_Unset special-cased here?

Comment 22

[Thomas Nguyen (:tnguyen)]

(In reply to Henri Sivonen (:hsivonen) from comment #21)
> Comment on attachment 8757781 [details] [diff] [review]
> ReferrerPolicy from string is not compliant with specs
> 
> Review of attachment 8757781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLImageElement.cpp
> @@ +568,5 @@
> >        aNameSpaceID == kNameSpaceID_None &&
> >        aNotify) {
> > +    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue);
> > +    if (!InResponsiveMode() &&
> > +        referrerPolicy != RP_Unset &&
> 
> Why is RP_Unset special-cased here?
Thanks for looking at this.
Logically, if we put an invalid value into referrerPolicy attribute (then is parsed as RP_Unset) --> the old policy is overruled unexpectedly as new one (RP_Unset).
I added a test case for that in the patch

Comment 23

[Thomas Nguyen (:tnguyen)]

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

Comment 24

[Thomas Nguyen (:tnguyen)]

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

Comment 25

[Thomas Nguyen (:tnguyen)]

Nice! Thanks Henri for looking at this bug.

Comment 26

[Thomas Nguyen (:tnguyen)]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df05b3be2b3c&selectedJob=22414059

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9e1c46cf0a
enable perElementReferrer by default. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02a42d1be7c
Update web platform test meta file. r=franziskuskiefer
https://hg.mozilla.org/integration/mozilla-inbound/rev/471079c11bee
Fix wrong policy associated with empty string. r=fkiefer,hsivonen

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e9e1c46cf0a
https://hg.mozilla.org/mozilla-central/rev/b02a42d1be7c
https://hg.mozilla.org/mozilla-central/rev/471079c11bee

Comment 31

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/50#HTML
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/area
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/referrerPolicy