Implement <iframe> referrer attribute

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: franziskus, Assigned: franziskus)

Tracking

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

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

3.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.51 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
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 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.
Attachment #8624316 - Flags: review?(mozilla)
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.
Attachment #8624319 - Flags: review?(mozilla)
Attachment #8624316 - Attachment is obsolete: true
Attachment #8628015 - Flags: review?(mozilla)
Attachment #8624319 - Attachment is obsolete: true
Attachment #8628016 - Flags: review?(mozilla)
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
Attachment #8628015 - Flags: review?(mozilla) → review+
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.
Attachment #8628016 - Flags: review?(mozilla)
Attachment #8628016 - Attachment is obsolete: true
Attachment #8630236 - Flags: review?(mozilla)
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 :-)
Attachment #8630236 - Flags: review?(mozilla) → review+
Attachment #8630236 - Attachment is obsolete: true
Attachment #8631289 - Flags: review?(bzbarsky)
Attachment #8628015 - Attachment is obsolete: true
Attachment #8631290 - Flags: review?(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.
Attachment #8631289 - Flags: review?(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
Attachment #8631290 - Flags: review?(bzbarsky) → review+
(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
Attachment #8631289 - Flags: review?(gijskruitbosch+bugs)
> 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 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.
Attachment #8631289 - Flags: review?(gijskruitbosch+bugs)
you r+ this already, but I changed it according to bug 1174913, so you might want to have a quick look again.
Attachment #8631290 - Attachment is obsolete: true
Attachment #8635583 - Flags: review?(bzbarsky)
made these similar to tests in bug 1174913 (using same sjs and helper).
Attachment #8631289 - Attachment is obsolete: true
Comment on attachment 8635583 [details] [diff] [review]
referrer attribute for iframe tag, src

r=me
Attachment #8635583 - Flags: review?(bzbarsky) → review+
Attachment #8636193 - Flags: review?(amarchesini)
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.
Attachment #8636193 - Flags: review?(amarchesini) → review+
r+ carries over (rebase)
Attachment #8636193 - Attachment is obsolete: true
Attachment #8637264 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/edb2d4f93dd0
https://hg.mozilla.org/mozilla-central/rev/a70b435a2e82
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.