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

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 2 • 9 years ago

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

	Christoph Kerschbaumer [:ckerschb]
	Comment 3 • 9 years ago

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.

	Christoph Kerschbaumer [:ckerschb]
	Comment 4 • 9 years ago

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.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 7 • 9 years ago

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

	Christoph Kerschbaumer [:ckerschb]
	Comment 8 • 9 years ago

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

	Christoph Kerschbaumer [:ckerschb]
	Comment 9 • 9 years ago

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.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 11 • 9 years ago

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

	Christoph Kerschbaumer [:ckerschb]
	Comment 12 • 9 years ago

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

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

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.

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

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 17 • 9 years ago

(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

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

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

	:Gijs (he/him)
	Comment 19 • 9 years ago

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.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 20 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 21 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 22 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 23 • 9 years ago

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

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

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

r=me

	Andrea Marchesini [:baku]
	Comment 25 • 9 years ago

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.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 26 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 27 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 28 • 9 years ago

r+ carries over (rebase)

	Pulsebot
	Comment 29 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 30 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 31 • 9 years ago

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