1175736 - Implement <iframe> referrer attribute

Status: RESOLVED FIXED

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

Description

[Franziskus Kiefer [:franziskus]]

Implementing the referrer attribute for the iframe tag to specify a referrer policy for iframes.

http://w3c.github.io/webappsec/specs/referrer-policy/#referrer-policy-delivery-referrer-attribute

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, src

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

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

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8624316 [details] [diff] [review]
referrer attribute for iframe tag, src

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

This looks fine, please address my nits, flag me again and then we can hand this off to some peers for review.

::: dom/base/nsFrameLoader.cpp
@@ +377,5 @@
>        loadInfo->SetReferrer(referrer);
>      }
>    }
>  
> +  mozilla::net::ReferrerPolicy referrerPolicy = mOwnerContent->OwnerDoc()->GetReferrerPolicy();

please add a nice block comment on top explaining that per element referrers can overrule document wide referrers.

@@ +386,5 @@
> +    if (iframe) {
> +      mozilla::net::ReferrerPolicy iframeReferrerPolicy;
> +      nsresult rv = iframe->GetReferrerPolicy(iframeReferrerPolicy);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      // if the image does not provide a referrer attribute, ignore this

I would rephrase that to
// if the iframe does provide a referrer attribute, then overrule the doucment wide referrer policy.

::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
@@ +25,5 @@
>             attribute DOMString        longDesc;
>             attribute DOMString        marginHeight;
>             attribute DOMString        marginWidth;
>             attribute DOMString        name;
> +           attribute DOMString        referrer;

please update the uuid since you added an attribute.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8624319 [details] [diff] [review]
referrer attribute for iframe tag, tests

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

Looks good regarding the testcover, but please simplify the tests and remove duplicate code. Also, please remove the 'message' listener once you don't need it anymore.

::: dom/base/test/iframe_referrer_testserver.sjs
@@ +1,1 @@
> +var BASE_URL = 'example.com/tests/dom/base/test/iframe_referrer_testserver.sjs';

Please add a nice little description on top (inlcuding bug number and what handleRequest is doing).

@@ +131,5 @@
> +
> +function handleRequest(request, response) {
> +  var sharedKey = 'iframe_referrer_testserver.sjs';
> +  var params = request.queryString.split('&');
> +  var action = params[0].split('=')[1];

Same as in the other tests, please:
* set headers on top of that function and not within every if-else branch;
* unescape arguments on top (as much as possible)
* simplify (remove duplicate code) from createTest functions and would be great to use some more descriptive function names other than test2, test3, etc.

::: dom/base/test/test_iframe_referrer.html
@@ +118,5 @@
> +  yield checkIndividualResults("unsafe-url in iframe", ["full"], [name]);
> +
> +  // test referrer policy in regular load with multiple images
> +  var policies = ['unsafe-url', 'origin', 'no-referrer'];
> +  var expected = ["full", "origin", "none"];

you are not resuing the policies[] and expected[], I think you could just use the values directly when assiging to name. No need for the detour. Also in other places underneath.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, src

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

Comment 7

[Franziskus Kiefer [:franziskus]]

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

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628015 [details] [diff] [review]
referrer attribute for iframe tag, src

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

Looks good otherwise - this on is ready for a peer review!

::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
@@ +25,5 @@
>             attribute DOMString        longDesc;
>             attribute DOMString        marginHeight;
>             attribute DOMString        marginWidth;
>             attribute DOMString        name;
> +           attribute DOMString        referrer;

as agreed, remove those bits, only use referrer within *.webidl

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628016 [details] [diff] [review]
referrer attribute for iframe tag, tests

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

Ok, let me look it over one more time, but overall looks good.

::: dom/base/test/iframe_referrer_testserver.sjs
@@ +87,5 @@
> +  var headString = '<head>';
> +  headString += '<meta name="referrer" content="' + aPolicy + '"></head>';
> +
> +  return createTestPage2(headString, aPolicy, aName);
> +}

The only difference between speculative and regular load is the '<script></script>' in the head, right? do you really need two different functions to accomplish that?
Please also add a comment that <script></script> triggers the speculative load.

@@ +116,5 @@
> +         </html>';
> +}
> +
> +function handleRequest(request, response) {
> +  var sharedKey = 'iframe_referrer_testserver.sjs';

nit: I prefer defining constants at the top of the file, like what you do with BASE_URL. Please also capitalize and use 'const' instead of var for both cases.

@@ +174,5 @@
> +    response.setHeader('Cache-Control', 'no-cache', false);
> +    response.setHeader('Content-Type', 'text/plain', false);
> +    response.write(getSharedState(sharedKey));
> +    return;
> +  }

nit: I usually prefer to have the easy 'return' cases further up in the function and the more complicated stuff gets, the further down it is in the function. In that sense you could move the above with the early return further up in the function to get it out of the way :-)

@@ +177,5 @@
> +    return;
> +  }
> +
> +  response.setHeader('Cache-Control', 'no-cache', false);
> +  response.setHeader('Content-Type', 'text/html; charset=utf-8', false);

why are these headers any different from the ones in 'get-test-results'? I think they should apply to all of the requests handled within that function, no? If so, maybe move those bits further up.

Comment 10

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

Comment 11

[Franziskus Kiefer [:franziskus]]

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8630236 [details] [diff] [review]
referrer attribute for iframe tag, tests

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

Ok, r=me, looks way better now :-)

Comment 13

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

Comment 14

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, src

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631289 [details] [diff] [review]
referrer attribute for iframe tag, tests

Again, I won't be able to review this in a sane timeframe.  Please find another reviewer.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631290 [details] [diff] [review]
referrer attribute for iframe tag, src

Does this allow the iframe to specify a looser referrer policy than its document does?  If so, is that desirable?

>+      mozilla::net::ReferrerPolicy iframeReferrerPolicy = iframe->GetReferrerPolicy();

Where is this GetReferrerPolicy implemented?

r=me modulo those

Comment 17

[Franziskus Kiefer [:franziskus]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 8631290 [details] [diff] [review]
> referrer attribute for iframe tag, src
> 
> Does this allow the iframe to specify a looser referrer policy than its
> document does?  If so, is that desirable?
> 
well, at the moment this is possible, yes. It might not be desirable, but for now we always override. However, we might want to push for this allowing only to tighten (this is not defined yet [1]).

> >+      mozilla::net::ReferrerPolicy iframeReferrerPolicy = iframe->GetReferrerPolicy();
> 
> Where is this GetReferrerPolicy implemented?
this is implemented here [2]

[1] https://w3c.github.io/webappsec/specs/referrer-policy/#determine-requests-referrer
[2] https://dxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLElement.h#237

Comment 18

[Boris Zbarsky [:bzbarsky]]

> this is implemented here [2]

I see.  The same question as in bug 1174913 then: why are we parsing the attribute, then reserializing it, then reparsing it?

What I suggest is that we just push GetReferrerPolicy up to Element or nsIContent, implement it in terms of GetParsedAttr in the HTML case instead of reserializing an enum and reparsing it, and use it in bug 1174913.

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8631289 [details] [diff] [review]
referrer attribute for iframe tag, tests

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

These tests have pretty similar issues to the ones I reviewed in the bug for <a> and <area>. It also looks like the sjs is largely very similar, and it would be nice if some of that could be abstracted/reused instead of copied.

Comment 20

[Franziskus Kiefer [:franziskus]]

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

Comment 21

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, src

you r+ this already, but I changed it according to bug 1174913, so you might want to have a quick look again.

Comment 22

[Franziskus Kiefer [:franziskus]]

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

Comment 23

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

made these similar to tests in bug 1174913 (using same sjs and helper).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635583 [details] [diff] [review]
referrer attribute for iframe tag, src

r=me

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8636193 [details] [diff] [review]
referrer attribute for iframe tag, tests

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

::: dom/base/test/test_iframe_referrer.html
@@ +17,5 @@
> +
> +  const sjs = "/tests/dom/base/test/referrer_testserver.sjs?";
> +  const ATTRIBUTE_POLICY = 'attributePolicy';
> +  const NEW_ATTRIBUTE_POLICY = 'newAttributePolicy';
> +  const NAME = 'name';

any particular reason why you cannot just do: "name:" in the testCases array of objects?
Same for the other consts.

::: dom/base/test/test_iframe_referrer_changing.html
@@ +15,5 @@
> +  <script type="application/javascript;version=1.7">
> +
> +  const sjs = "/tests/dom/base/test/referrer_testserver.sjs?";
> +  const ATTRIBUTE_POLICY = 'attributePolicy';
> +  const NEW_ATTRIBUTE_POLICY = 'newAttributePolicy';

same here.

Comment 26

[Franziskus Kiefer [:franziskus]]

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

Comment 27

[Franziskus Kiefer [:franziskus]]

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

Comment 28

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for iframe tag, tests

r+ carries over (rebase)

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edb2d4f93dd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70b435a2e82

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edb2d4f93dd0
https://hg.mozilla.org/mozilla-central/rev/a70b435a2e82

Comment 31

[Jean-Yves Perrier [:teoli]]

Doc created
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/referrer

Docs updated
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/referrer
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe
https://developer.mozilla.org/en-US/Firefox/Releases/42#HTML