Enable perElementReferrer by default

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: fkiefer, Assigned: tnguyen)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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
Depends on: 1178337
Created attachment 8694659 [details] [diff] [review]
enable perElementReferrer by default
Assignee: nobody → franziskuskiefer
This needs an intent to ship, right?
intent to ship: https://groups.google.com/d/topic/mozilla.dev.platform/TXFImc_r9Jo/discussion
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Blocks: 1168540
Blocks: 999754
No longer blocks: 1168540
Depends on: 1168540
Depends on: 1264165
(Assignee)

Comment 4

2 years ago
Tried to run some tests and I found if I enable this preference, Bug 1101288 will appear.
I will take closer look.
Depends on: 1101288
(Assignee)

Updated

2 years ago
No longer depends on: 1101288
(Assignee)

Comment 5

2 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).
(Assignee)

Comment 6

2 years ago
Created attachment 8751135 [details] [diff] [review]
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8751169 [details] [diff] [review]
Update meta file
(Assignee)

Updated

2 years ago
Assignee: franziskuskiefer → tnguyen
(Assignee)

Updated

2 years ago
Attachment #8751169 - Flags: review?(franziskuskiefer)
(Assignee)

Updated

2 years ago
Attachment #8751135 - Flags: review?(franziskuskiefer)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
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?
Attachment #8751135 - Flags: review?(franziskuskiefer)
Attachment #8751169 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Comment 9

2 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.
(Assignee)

Comment 10

2 years ago
Created attachment 8754675 [details] [diff] [review]
Fix wrong policy associated with empty string
Attachment #8754675 - Flags: review?(franziskuskiefer)
(Assignee)

Updated

2 years ago
Attachment #8751135 - Attachment is obsolete: true
Comment on attachment 8754675 [details] [diff] [review]
Fix wrong policy associated with empty string

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

much better, thanks :)
Attachment #8754675 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Updated

2 years ago
Attachment #8694659 - Flags: review?(hsivonen)
(Assignee)

Updated

2 years ago
Attachment #8754675 - Flags: review?(hsivonen)
(Assignee)

Comment 12

2 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
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.)
Attachment #8754675 - Flags: review?(hsivonen) → review-
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.
Attachment #8694659 - Flags: review?(hsivonen)
Iteration: --- → 49.3 - Jun 6
(Assignee)

Comment 15

2 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
(Assignee)

Comment 16

2 years ago
Created attachment 8757163 [details] [diff] [review]
Fix wrong policy associated with empty string
Attachment #8757163 - Flags: review?(hsivonen)
(Assignee)

Updated

2 years ago
Attachment #8754675 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8694659 - Flags: review?(hsivonen)
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.
Attachment #8757163 - Flags: review?(hsivonen) → review-
(Assignee)

Comment 18

2 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.
(Assignee)

Comment 19

2 years ago
Created attachment 8757245 [details] [diff] [review]
ReferrerPolicy Attribute from string is not compliant with specs

Updated, will add test cases about this
Attachment #8757163 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8757781 [details] [diff] [review]
ReferrerPolicy from string is not compliant with specs
Attachment #8757245 - Attachment is obsolete: true
Attachment #8757781 - Flags: review?(hsivonen)
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?
(Assignee)

Comment 22

2 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
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4f62c76e7d3
Attachment #8694659 - Flags: review?(hsivonen) → review+
Attachment #8757781 - Flags: review?(hsivonen) → review+
Comment hidden (obsolete)
(Assignee)

Comment 25

2 years ago
Nice! Thanks Henri for looking at this bug.
(Assignee)

Comment 26

2 years ago
Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df05b3be2b3c&selectedJob=22414059
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1275803
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1179817

Comment 29

2 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
Keywords: checkin-needed

Comment 30

2 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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.