1178337 - Valid referrer attribute values

Status: RESOLVED FIXED

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

Description

[Franziskus Kiefer [:franziskus]]

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.

Comment 1

[Franziskus Kiefer [:franziskus]]

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

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: change valid referrer attribute values

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 4

[Henry Chang [:hchang]]

Attachment: 0002-Bug-1178337-Part-1-Supports-all-referrer-policy-for-.patch

Comment 5

[Henry Chang [:hchang]]

Attachment: 0003-Bug-1187337-Part-2-Update-add-anchor-area-iframe-tes.patch

Comment 6

[Henry Chang [:hchang]]

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!

Comment 7

[Franziskus Kiefer [:franziskus]]

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

Comment 8

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

Comment 9

[Henry Chang [:hchang]]

Attachment: 0002-Bug-1178337-Part-2-Update-add-test-cases.-r-sicking.patch

Comment 10

[Henry Chang [:hchang]]

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!

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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"

Comment 12

[Henry Chang [:hchang]]

Thanks for your review! Jonas!

Comment 13

[Henry Chang [:hchang]]

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

Comment 14

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch (carry r+)

Comment 15

[Henry Chang [:hchang]]

Attachment: 0002-Bug-1178337-Part-2-Update-add-test-cases.-r-sicking.patch (carry r+)

Comment 16

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch (carry r+)

Comment 17

[Henry Chang [:hchang]]

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.

Comment 18

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1178337-Part-1-Supports-all-referrer-policies-fo.patch

Comment 19

[Henry Chang [:hchang]]

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

Comment 20

[Henry Chang [:hchang]]

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!

Comment 21

[:Gijs (he/him)]

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?

Comment 22

[Henry Chang [:hchang]]

(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

Comment 23

[Henry Chang [:hchang]]

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!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90ab64a386dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe0c1d00f16

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90ab64a386dd
https://hg.mozilla.org/mozilla-central/rev/4fe0c1d00f16

Comment 26

[Jean-Yves Perrier [:teoli]]

Page https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a updated with the values.

Also added a note in https://developer.mozilla.org/en-US/Firefox/Releases/49#HTML