Franziskus Kiefer [:franziskus] Reporter
	Description • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 9 years ago

This needs an intent to ship, right?

	Franziskus Kiefer [:franziskus] Reporter
	Comment 3 • 9 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 4 • 8 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 5 • 8 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 6 • 8 years ago

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.

	Franziskus Kiefer [:franziskus] Reporter
	Comment 8 • 8 years ago

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?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 9 • 8 years ago

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.

	Franziskus Kiefer [:franziskus] Reporter
	Comment 11 • 8 years ago

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

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

much better, thanks :)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 12 • 8 years ago

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

	Henri Sivonen (:hsivonen)
	Comment 13 • 8 years ago

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

	Henri Sivonen (:hsivonen)
	Comment 14 • 8 years ago

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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 15 • 8 years ago

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

	Henri Sivonen (:hsivonen)
	Comment 17 • 8 years ago

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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 18 • 8 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 19 • 8 years ago

Updated, will add test cases about this

	Henri Sivonen (:hsivonen)
	Comment 21 • 8 years ago

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?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 22 • 8 years ago

(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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 23 • 8 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 25 • 8 years ago

Nice! Thanks Henri for looking at this bug.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 26 • 8 years ago

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

	Pulsebot
	Comment 29 • 8 years ago

Pushed by cbook@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

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

	Florian Scholz (Open Web Docs)
	Comment 31 • 8 years ago

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