Valid referrer attribute values

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: franziskus, Assigned: hchang)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(URL)

Attachments

(2 attachments, 7 obsolete attachments)

27.04 KB, patch
hchang
: review+
Details | Diff | Splinter Review
8.50 KB, patch
Gijs
: review+
mcmanus
: review+
Details | Diff | Splinter Review
Delivery via referrer attribute allows only three values (no-referrer, origin, unsafe-url) while delivery via meta allows five values (no-referrer, origin, unsafe-url, no-referrer-when-downgrade, origin-when-cross-origin). Delivery via referrer attribute should allow all five values as well.
This requires spec change.
The spec considers now all five values for attributes as well [1], so we should do so as well.

[1] https://github.com/w3c/webappsec-referrer-policy/pull/2
Jonas can you review this? You did some referrer work recently.

I added valid referrer values for attribute referrer policies (see comment 1). Also moved kReferrerTable to ReferrerPolicy.h to have valid referrer policies in one place.
Assignee: nobody → franziskuskiefer
Attachment #8694232 - Flags: review?(jonas)
Comment on attachment 8694232 [details] [diff] [review]
change valid referrer attribute values

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

Can you make sure that we have tests for

<meta name="referrer" content="origin"><a referrerpolicy="none-when-downgrade" href="X">link</a>

Clicking the link should send a full URL when X uses https, and no URL when X uses http. I.e. we should in this case ignore the <meta> and use the policy defined on the <a>.

::: netwerk/base/ReferrerPolicy.h
@@ +51,5 @@
>  const char kRPS_Always[]                      = "always";
>  const char kRPS_Unsafe_URL[]                  = "unsafe-url";
>  
> +/* table of valid referrer policy values */
> +const nsAttrValue::EnumTable kReferrerTable[] = {

It doesn't seem like a good idea to stick an array like this in a header file. Won't it get duplicated into every cpp file it is included in? Are you relying on the linker optimizing away the duplicates?

Why not just put it in nsGenericHTMLElement.cpp since it's element-specific anyway?
Attachment #8694232 - Flags: review?(jonas) → review+
(Assignee)

Updated

3 years ago
Assignee: franziskuskiefer → hchang
(Assignee)

Comment 6

3 years ago
Comment on attachment 8731170 [details] [diff] [review]
0003-Bug-1187337-Part-2-Update-add-anchor-area-iframe-tes.patch

Hi Franziskus,

I separated the original patches to 2 patches and this one is for test cases. 

1) Ideally "test_iframe_referrer_invalid.html" should only has "default" referrer policy since all referrer policies for element attributes are all supported now. If you also so, I'll move them to "test_iframe_referrer.html" in the next patch.

2) What I added was trying to test "none-when-downgrade". To test that, I have to make the scheme of the document itself and the link different. So, I slightly modify "referrer_helper.js" and "referrer_testserver.sjs" to support linking across  different scheme. Not sure if this is the best place to do this kind of test. test_bug704320_https_http.html and the relevant tests are also suitable to do, too. . I'd feel "test_iframe_referrer.html" is way better. What do you think of this?

I haven't actually run the modified test cases but need your feedback to know if I am on the right track (I mean modifying referrer_testserver.sjs to support custom scheme and adding more tests in test_iframe_referrer.html) :)

Thanks!
Attachment #8731170 - Flags: feedback?(franziskuskiefer)
Comment on attachment 8731170 [details] [diff] [review]
0003-Bug-1187337-Part-2-Update-add-anchor-area-iframe-tes.patch

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

(In reply to Henry Chang [:henry] from comment #6)
> Comment on attachment 8731170 [details] [diff] [review]
> 0003-Bug-1187337-Part-2-Update-add-anchor-area-iframe-tes.patch
> 
> 1) Ideally "test_iframe_referrer_invalid.html" should only has "default"
> referrer policy since all referrer policies for element attributes are all
> supported now. If you also so, I'll move them to "test_iframe_referrer.html"
> in the next patch.

yeah, the _invalid tests are not necessary anymore. Instead we should add some correct tests to _referrer.html test case for the now valid attribute values.

> 2) What I added was trying to test "none-when-downgrade". To test that, I
> have to make the scheme of the document itself and the link different. So, I
> slightly modify "referrer_helper.js" and "referrer_testserver.sjs" to
> support linking across  different scheme. Not sure if this is the best place
> to do this kind of test. test_bug704320_https_http.html and the relevant
> tests are also suitable to do, too. . I'd feel "test_iframe_referrer.html"
> is way better. What do you think of this?

adding the scheme in test_iframe_referrer.html makes sense. just make sure to structure it properly, it's getting a bit bloated (even more) and difficult to read.

::: dom/base/test/test_anchor_area_referrer_invalid.html
@@ +25,5 @@
>          {ATTRIBUTE_POLICY: 'origin-when-cross-origin',
>           NAME: 'origin-when-cross-origin-with-no-meta',
>           META_POLICY: '',
>           DESC: "origin-when-cross-origin (anchor) with no meta",
> +         RESULT: 'origin'},

is this a cross-origin test now? or why do we expect origin? (Same for the other changes)

::: dom/base/test/test_iframe_referrer_invalid.html
@@ +25,5 @@
>          {ATTRIBUTE_POLICY: 'origin-when-cross-origin',
>           NAME: 'origin-when-cross-origin-with-no-meta',
>           META_POLICY: '',
>           DESC: "origin-when-cross-origin (iframe) with no meta",
> +         RESULT: 'origin'},

and here as well
Attachment #8731170 - Flags: feedback?(franziskuskiefer) → feedback+
(Assignee)

Comment 8

3 years ago
Attachment #8694232 - Attachment is obsolete: true
Attachment #8731169 - Attachment is obsolete: true
Attachment #8731170 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8731648 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Attachment #8731649 - Flags: review?(jonas)
(Assignee)

Comment 10

3 years ago
Hi Jonas,

Could you please have a review on the new patches? 

Part 1: Almost the same as what you have reviewed except for the enum value change of RP_Unset. The reason being is, we rely on RP_Unset to identify if an element has a attribute to overrule the document's referrer policy. Unfortunately, RP_Unset and RP_No_Referrer_When_Downgrade internally has the same enum value so that RP_No_Referrer_When_Downgrade would never ever overrule the document's policy. (yes that's exact what you what us to test in the first place!)

Part 2: To have the test support cross scheme test, "referrer_testserver.sjs", "referrer_helpfer.js" and all test cases which require "referrer_helpfer.js" need to be updated. The tests I aded in the patch is in "test_anchor_area_referrer.html". In theory, most of the test cases in test_*_invalid should be moved to other test case since they are no longer invalid. I will do it in the next patch if this patch is passed the review :)

Thanks!

Updated

3 years ago
Blocks: 1168540
Comment on attachment 8731648 [details] [diff] [review]
0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

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

::: netwerk/base/ReferrerPolicy.h
@@ +27,5 @@
>    /* spec tokens: always unsafe-url */
>    RP_Unsafe_URL                  = nsIHttpChannel::REFERRER_POLICY_UNSAFE_URL,
>  
>    /* referrer policy is not set */
> +  // Using the same enum as RP_No_Referrer_When_Downgrade would let us

"would leave us"
Attachment #8731648 - Flags: review?(jonas) → review+
(Assignee)

Comment 12

3 years ago
Thanks for your review! Jonas!
(Assignee)

Comment 14

3 years ago
Attachment #8731648 - Attachment is obsolete: true
Attachment #8731649 - Attachment is obsolete: true
Attachment #8743708 - Flags: review+
(Assignee)

Comment 16

3 years ago
Attachment #8743708 - Attachment is obsolete: true
Attachment #8743798 - Flags: review+
(Assignee)

Comment 17

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&tochange=2db01d5ccaab&selectedJob=19799780&fromchange=3ad92d133275204a8c3401c9ef9914e90e7fb4c3

browser/base/content/test/referrer/browser_referrer_middle_click.js test failures on try. Will try to fix this tmr.
(Assignee)

Comment 20

3 years ago
Comment on attachment 8744794 [details] [diff] [review]
0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

Hi :Gijs,

Could you please review the changes in browser.js and content.js? (since you are the reviewer of the last change of those parts.)

The reason of using a "unset" state of referrer policy is we need to differentiate the "default RP" and "unset RP" when it comes to "per element policy". If we didn't do that, 

1) The "unset per element RP" would incorrectly overrule the document-wide RP in browser.js and content.js. 

2) The "origin-when-downgrade" RP wouldn't overrule the document-wide RP in many places since it will be treated like "unset" RP.

Thanks!
Attachment #8744794 - Flags: review?(gijskruitbosch+bugs)

Comment 21

3 years ago
Comment on attachment 8744794 [details] [diff] [review]
0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

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

(In reply to Henry Chang [:henry] from comment #20)
> 2) The "origin-when-downgrade" RP wouldn't overrule the document-wide RP in
> many places since it will be treated like "unset" RP.

There doesn't seem to be such a thing as "origin-when-downgrade" - do you mean "no-referrer-when-downgrade" or "origin-when-cross-origin" ? I'm guessing the former...

The browser and content.js changes look OK to me, assuming someone else reviewed the rest.

One thing that's a bit odd is that DEFAULT still equals no-referrer-when-downgrade, but is unused, at least as far as the JS side is concerned (none of the APIs it accesses will now ever produce DEFAULT, as far as I can tell), which is a bit confusing. Could we just get rid of it, or are there network-side things that use it?
Attachment #8744794 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 22

3 years ago
(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8744794 [details] [diff] [review]
> 0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch
> 
> Review of attachment 8744794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Henry Chang [:henry] from comment #20)
> > 2) The "origin-when-downgrade" RP wouldn't overrule the document-wide RP in
> > many places since it will be treated like "unset" RP.
> 
> There doesn't seem to be such a thing as "origin-when-downgrade" - do you
> mean "no-referrer-when-downgrade" or "origin-when-cross-origin" ? I'm
> guessing the former...
> 

Oops! You are right. It should be "no-referrer-when-downgrade".

> The browser and content.js changes look OK to me, assuming someone else
> reviewed the rest.
> 
> One thing that's a bit odd is that DEFAULT still equals
> no-referrer-when-downgrade, but is unused, at least as far as the JS side is
> concerned (none of the APIs it accesses will now ever produce DEFAULT, as
> far as I can tell), which is a bit confusing. Could we just get rid of it,
> or are there network-side things that use it?

nsIHttpChannel::REFERRER_POLICY_DEFAULT is added by Bug 1113431 [1] for other purpose as far as I know. The "unset" state is in particular important to deal with element refererr policy overruling.

BTW, I think I should ask necko peer for reviewing the change in nsIHttpChannel. Thanks :)

[1] https://hg.mozilla.org/mozilla-central/rev/6a170ee249b2
(Assignee)

Comment 23

3 years ago
Comment on attachment 8744794 [details] [diff] [review]
0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

Hi Patrick,

Could you have a review for the change in nsIHttpChannel.idl? A new referrer policy state ("unset") is added to distinguish the "unset" and "default" state when deciding if the element referrer policy should overrule the document-wide policy. (changes in browser.js and content.js). Thanks!
Attachment #8744794 - Flags: review?(mcmanus)
Attachment #8744794 - Flags: review?(mcmanus) → review+

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90ab64a386dd
https://hg.mozilla.org/mozilla-central/rev/4fe0c1d00f16
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.