Implement <meta name="referrer">

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: Adam Barth, Assigned: geekboy)

Tracking

(Depends on: 7 bugs, Blocks: 5 bugs, {addon-compat, dev-doc-needed, perf})

unspecified
mozilla36
addon-compat, dev-doc-needed, perf
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 36+)

Details

(URL)

Attachments

(12 attachments, 68 obsolete attachments)

7.67 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
8.11 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
75.57 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
47.11 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
52.03 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
7.98 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
2.43 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
33.45 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
28.87 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
14.99 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
11.15 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
23.40 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
http://wiki.whatwg.org/wiki/Meta_referrer

== Overview ==

This document describes a proposal the "referrer" metadata name. Using the referrer metadata attribute, a document can control the behavior if the Referer HTTP header attached to requests that originate from the document.

== Examples ==

This meta element instructs the user agent to omit the Referer header in all HTTP requests that originate from the document containing the element:

<meta name="referrer" content="never">

This meta element instructs the user agent to include the document's origin in the Referer header rather than the full URL of the document:

<meta name="referrer" content="origin">
(Reporter)

Comment 1

6 years ago
I also wrote up a brief blog post about this feature:

http://www.schemehostport.com/2011/11/referer-sic.html
(Reporter)

Updated

6 years ago
Blocks: 61660
Hi Adam,

I have a few questions on the spec.

1) What do you do if there are multiple meta referrer tags.  Example:
<head>
<meta name="referrer" content="never">
<script src="http://foobar.com/script1.js">
...
<meta name="referrer" content="origin">
<script src="http://foobar.com/script2.js">
...
</head>

Which one should take precedence?  The first one, the one most recently encountered, the most restrictive?  Would a referrer be sent when fetching script1.js?  How about script2.js?

2) Can you elaborate on this statement from the Issues section of the spec:
"What should happen if the origin is unique? Presumably the referrer should be omitted in that case."

Thanks!
(Assignee)

Comment 3

6 years ago
Adding URL since this would satisfy part of the "Shortened HTTP Referer Header" feature.
Assignee: nobody → tanvi

Comment 4

6 years ago
(In reply to Tanvi Vyas from comment #2)
> Hi Adam,
> 
> I have a few questions on the spec.
> 
> 1) What do you do if there are multiple meta referrer tags.  Example:
> <head>
> <meta name="referrer" content="never">
> <script src="http://foobar.com/script1.js">
> ...
> <meta name="referrer" content="origin">
> <script src="http://foobar.com/script2.js">
> ...
> </head>
> 
> Which one should take precedence?  The first one, the one most recently
> encountered, the most restrictive?  Would a referrer be sent when fetching
> script1.js?  How about script2.js?

The last meta tag processed wins, i.e. for script1 no referrer is send, for script2 the origin is send.

If at a later point a new meta tag is created in inserted into the document, that policy will be used.

> 
> 2) Can you elaborate on this statement from the Issues section of the spec:
> "What should happen if the origin is unique? Presumably the referrer should
> be omitted in that case."

In WebKit, we treat e.g. documents on file: as coming from a unique origin, i.e. when you reload the document, it'll have a "different" origin.

> 
> Thanks!
(Reporter)

Comment 5

6 years ago
These things are all up for discussion if you think this feature ought to work differently.

Having later meta elements override earlier ones lets the web page adjust its policy over time.  For example, consider a web application that executes as a single document over a long period of time but that displays conceptually different pages (e.g., the way Facebook works).  It might want to change the policy depending on what it's currently displaying.
The Referer header is inherently bad for privacy.  While the proposed meta
tag helps, by implementing it I think we also send the message to web
developers that Referer will be around forever.  I think we should instead
consider not sending it at all by default, or send a dummy value that is
useless for tracking purposes.

Fwiw, I have not sent a Referer for several years and I have yet to find
a site that doesn't work because of it (network.http.sendRefererHeader
set to zero in about:config).
(In reply to Mats Palmgren [:mats] from comment #6)
> Fwiw, I have not sent a Referer for several years and I have yet to find
> a site that doesn't work because of it (network.http.sendRefererHeader
> set to zero in about:config).

I did so, too, for years with success, but had to stop when I started using Launchpad for reporting Ubuntu bugs.

Comment 8

5 years ago
(In reply to Mats Palmgren [:mats] from comment #6)
> I think we should instead
> consider not sending it at all by default, or send a dummy value that is
> useless for tracking purposes.
Agreed. Is there another bug where this could be discussed? If a bug is created, can its creator add it to the "see also" field of this bug? Thanks.

> Fwiw, I have not sent a Referer for several years and I have yet to find
> a site that doesn't work because of it (network.http.sendRefererHeader
> set to zero in about:config).
The ECMAScript wiki [1] has some navigation ("trace") which is based on a combinaison of cookies and referer, I think (which makes a very poor feature when using several tabs at the same time)

[1] http://wiki.ecmascript.org/doku.php?do=recent&id=

Comment 9

5 years ago
I provided feedback on the meta referer a while back. Rough summury of discussions:

An intervention by Simon Pieters [1] mentionned speculative parsing and the fact that resources may be fetched *before* the meta is read, hence leaking the referer while the intention of the author might have been that there should be no leak. Since there seems to be no satisfying HTML-based solution for this, I
suggested [2] that it's when the document is delivered by the server that the server should express how the document should behave regarding
sending referer headers. The discussion ended by Adam Barth agreeing [3] and planning to propose this for CSP 1.1

Adam, has there been a move on CSP for that?
Regardless, has a solution been found for the <meta> element?

[1] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-January/034283.html
[2] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-January/034522.html
[3] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-January/034523.html
(Assignee)

Comment 10

5 years ago
I think there's no reason we can't support both the meta tag and a CSP directive.  In fact, I think we should support both.  We should also support user-specified tweaking of referrer-sending behavior.

Any way we go about it, we need to put in a bit more plumbing in Gecko to support referrer customization properly, and the mechanisms that use the plumbing can vary.

This bug is to implement the meta-referrer proposal, so unless there are reason's we shouldn't, lets focus on "how".  Are there any arguments *against* supporting it, admitting that it's imperfect and the major use cases may very well be limited? My thoughts are that the most popular meta-referrer use cases are:

(1) the "always" value to send HTTPS referrers to HTTP sites
(2) stripping referrer detail from third-party widgets

Comment 11

5 years ago
(In reply to Sid Stamm [:geekboy] from comment #10)
> I think there's no reason we can't support both the meta tag and a CSP
> directive.  In fact, I think we should support both.
What's your solution for speculative parsing and prefetching?

> My thoughts are that the most popular meta-referrer use cases are:
> 
> (1) the "always" value to send HTTPS referrers to HTTP sites
> (2) stripping referrer detail from third-party widgets
We've had such a discussion. For reference: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-December/034278.html
I'd be happy to have additional input on this feedback.
(Assignee)

Comment 12

5 years ago
the meta tag approach can't address speculative parsing and prefetching, clearly, but perhaps a CSP directive (via HTTP header) can address it, or perhaps a cached policy via .well-known... I'm just brainstorming here, but I don't think this is the right venue for picking alternate technologies.

Is there value to implementing the meta-referrer feature as-is, or should we close this and consider a different policy delivery mechanism?  (Assume we'll re-use the same referrer-controlling plumbing through other mechanisms in the future.)
(Reporter)

Comment 13

5 years ago
> Adam, has there been a move on CSP for that?

We're about to wrap up CSP 1.0 in the WebAppSec working group.  This proposal is on the table for CSP 1.1, which we'll start working on once CSP 1.0 is wrapped up.

w.r.t. the speculative parser, we just need to teach the speculative parser how to understand this <meta> tag the similar to how it understand the <base> tag.

Comment 14

5 years ago
> My thoughts are that the most popular meta-referrer use cases are:

For WebKit users, Facebook plans to implement an "origin" policy in the near future. This policy effectively represents how our site behaves today, but without relying on the abuse of existing browser behavior. Background: https://www.facebook.com/notes/facebook-engineering/protecting-privacy-with-referrers/392382738919

The meta-referrer proposal provides two immediate benefits with respect to Facebook users:

- We currently utilize document.location.replace() through an interstitial endpoint to perform external redirects for Mozilla users. Implementing a native redirect instead of relying upon JavaScript offers a slight performance improvement and enables the redirect to function with JavaScript disabled.

- We intentionally downgrade the interstitial from HTTPS to HTTP in order to send a referer header. This is obviously undesirable but currently necessary in our context. Support for meta-referrer enables us to maintain an a secure connection and resolves one of the last issues blocking implementation of Strict-Transport-Security 

Just throwing a bit of support behind the proposal. We'd love to see support in Firefox.

Comment 15

5 years ago
(In reply to Adam Barth from comment #13)
> w.r.t. the speculative parser, we just need to teach the speculative parser
> how to understand this <meta> tag the similar to how it understand the
> <base> tag.
Sounds good.

Just thinking of this case now:
<html>
  <head>
    <script>
      document.write('<meta name="referer" content="never">');
    </script>
  </head>
</html>

Does it mean the speculative parser has to wait for all inline scripts to finish?
If so, is it an acceptable downside?

How is <base> handled in this case?
(In reply to David Bruant from comment #15)
> (In reply to Adam Barth from comment #13)
> > w.r.t. the speculative parser, we just need to teach the speculative parser
> > how to understand this <meta> tag the similar to how it understand the
> > <base> tag.
> Sounds good.
> 
> Just thinking of this case now:
> <html>
>   <head>
>     <script>
>       document.write('<meta name="referer" content="never">');
>     </script>
>   </head>
> </html>
> 
> Does it mean the speculative parser has to wait for all inline scripts to
> finish?

No, it means that authors who are silly enough to document.write <meta name=referer> are on their own.

> If so, is it an acceptable downside?

No.

> How is <base> handled in this case?

Speculative GETs are made to bogus URLs.

Comment 17

5 years ago
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-January/034520.html sums up the differences between <base> and <meta referrer> here, imo.

Comment 18

5 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> (In reply to David Bruant from comment #15)
> > Does (...) the speculative parser has to wait for all inline scripts to
> > finish?
> 
> No, it means that authors who are silly enough to document.write <meta
> name=referer> are on their own.
Fair enough. Meaning that dynamically inserted <meta name=referer> (by document.write or appendChild) create a non-determinism when it comes to sending referers or not. To be documented.
Keywords: dev-doc-needed
Whiteboard: For documentation, see comment 18
Does this really need to be something embedded in the content of the page, vs. something in a HTTP header? Allowing this information only to be expressed only via a HTTP header would seem to prevent the negative issues above.

I remember having a discussion about using CSP to prevent meta-refresh, on the grounds that meta tags can be injected by scripts; it seems like the same concern would apply here.

Do we need to distinguish between navigations (the user clicked a link) vs. subresources (in particular, cross-site scripts and images) of the page? AFAICT, the use cases motivating this are for the former, and none are for the latter. I could see where a page would want to send referring information for nagivations, but not for, e.g. scripts served off of a third-party CDN service.
(Reporter)

Comment 20

5 years ago
Here's a use case for the latter.  Consider a social networking site that includes the user's uid in the URL.  When displaying advertisements (e.g., as subresources), the social network site might wish to block or truncate the Referer header when loading those resources so as to not leak the identity of the user who's page the advertisement is being displayed.

Comment 21

5 years ago
Why not just block all sending of Referer across origins? That would address most of the privacy issue without breaking compatibility with the (very few) sites that use it internally.

Comment 22

5 years ago
It would also break sites that use referer to allow deep-linking from a whitelist of other sites; that's not an uncommon use of referer.

Comment 23

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #22)
> It would also break sites that use referer to allow deep-linking from a
> whitelist of other sites; that's not an uncommon use of referer.

Such sites are already broken, because RFC 2616 doesn't require Referer to be sent:

15.1.3 Encoding Sensitive Information in URI's

   Because the source of a link might be private information or might
   reveal an otherwise private information source, it is strongly
   recommended that the user be able to select whether or not the
   Referer field is sent. For example, a browser client could have a
   toggle switch for browsing openly/anonymously, which would
   respectively enable/disable the sending of Referer and From
   information.

Comment 24

5 years ago
Yes, yes.  It's a matter of priorities.  They're just defaulting to a default-deny policy with exceptions, and the failure is that someone who should be able to get the content can't... but they're more interested in making absolutely sure that someone who _shouldn't_ be able to get the content can't.

That doesn't mean it's ok to just go break them even more, because the sites don't care: it's the users who suffer.

Comment 25

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #24)
> Yes, yes.  It's a matter of priorities.  They're just defaulting to a
> default-deny policy with exceptions, and the failure is that someone who
> should be able to get the content can't... but they're more interested in
> making absolutely sure that someone who _shouldn't_ be able to get the
> content can't.

Well, the behaviour I proposed meets the site's requirement, then ;-)

> That doesn't mean it's ok to just go break them even more, because the sites
> don't care: it's the users who suffer.

The users suffer anyway. They suffer either from an invisible privacy bug, or from visible site breakage. And no-one ever asks them which they prefer :-(

How about at least not sending the Referer cross-origin if it is https?
(Note that RFC 2616 allows you to send an https Referer cross-origin to any other https site, which is and always was completely stupid.)

Comment 26

5 years ago
> How about at least not sending the Referer cross-origin if it is https?

That might be more doable, yes.

Comment 27

5 years ago
(In reply to Sid Stamm [:geekboy] from comment #10)
> I think there's no reason we can't support both the meta tag and a CSP
> directive.  In fact, I think we should support both.

Since the current CSP spec specifies how to set a CSP policy in a meta tag, it shouldn't be supported in both ways by *different* syntax. (Note: I think meta tags are strictly less secure for specifying such policies than headers, but that's a separate issue and any directive affecting Referer shouldn't be an special case.)

I wrote up a detailed proposal for a 'restrict-referrer-leakage' CSP directive: https://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/127/restrict-referrer-leakage.txt
It's designed to be as consistent as possible with other CSP directives, which is why it's somewhat more general than strictly necessary.

Should I file a new bug for it here, or propose it to public-webappsec@ first?
(Assignee)

Comment 28

5 years ago
(In reply to David-Sarah Hopwood from comment #27)
> (In reply to Sid Stamm [:geekboy] from comment #10)
> > I think there's no reason we can't support both the meta tag and a CSP
> > directive.  In fact, I think we should support both.
> 
> Since the current CSP spec specifies how to set a CSP policy in a meta tag,
> it shouldn't be supported in both ways by *different* syntax. 

I think these are different features; the feature on this bug is about changing how the referrer on this page is sent in general; we could add something to CSP that would play nicely with this (I think).  This feature is probably valuable as an easy way for a web dev to say "strip referrers please", without having to learn CSP.

> (Note: I think
> meta tags are strictly less secure for specifying such policies than
> headers, but that's a separate issue and any directive affecting Referer
> shouldn't be an special case.)

:)  Yes.  I'm with you here, but there are some advantages to providing it.

> 
> I wrote up a detailed proposal for a 'restrict-referrer-leakage' CSP
> directive:
> https://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/127/restrict-
> referrer-leakage.txt
> It's designed to be as consistent as possible with other CSP directives,
> which is why it's somewhat more general than strictly necessary.
> 
> Should I file a new bug for it here, or propose it to public-webappsec@
> first?

Thanks for writing up the proposal.  I think you should file a bug, make it block 493857, and then propose it to public-webappsec at the same time.  There's no reason we can't work on both at the same time (especially if you're interested in helping write the patch for CSP in gecko... hint-hint).
Assignee: tanvi → administration
Assignee: administration → nobody

Comment 29

5 years ago
Created attachment 716863 [details] [diff] [review]
User preference

User preference implementation as described phase 1 of the following page:
https://wiki.mozilla.org/Privacy/Features/Shortened_HTTP_Referer_header

The last option ("just host") is not implemented yet because that seems to involve change to the nsIURI/nsIURL interface (i.e. needs a ClearPort() function).
Attachment #716863 - Flags: feedback?(sstamm)
(Assignee)

Comment 30

5 years ago
Comment on attachment 716863 [details] [diff] [review]
User preference

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

This patch is a good start at a global pref set via about:config!  Thanks for the patch!

I think it might make it clearer if we change SelectReferrerDetail to ReferrerPolicy everywhere.  This makes it a little clearer that it represents a policy choice.

Please use code formatting styles consistent with the rest of the files you're editing.  For example, see if () statements in HttpBaseChannel.cpp; you use a different format than the file currently uses.  Doing this for future edits will save time in future review.  :)

To make changes to the referrer, you probably don't want to modify existing nsIURI instances (they're not supposed to be mutable).  In HttpBaseChannel::SetReferrer(), you can populate mReferrer any way you want (it is a string).  For example, to use the policy "just host", you can grab the host out of the original URI and store that directly into the mReferrer instead of letting clone.GetAsciiSpec() do it for you.

We'll also need to write some tests for this (probably Mochitests are easiest) to make sure setting the pref has the desired effect.

After that, a next step is to write another patch (for application on top of this one) that reads the meta tag data and stores it in the docShell so that future loads on the page can inherit that policy.  But maybe revise this one (test it manually to make sure it works) then make some tests.

::: netwerk/protocol/http/nsHttpHandler.h
@@ +67,5 @@
>      nsHttpVersion  HttpVersion()             { return mHttpVersion; }
>      nsHttpVersion  ProxyHttpVersion()        { return mProxyHttpVersion; }
>      uint8_t        ReferrerLevel()           { return mReferrerLevel; }
>      bool           SendSecureXSiteReferrer() { return mSendSecureXSiteReferrer; }
> +    uint32_t       SelectReferrerDetail()    { return mSelectReferrerDetail; }

This should be called "GetSelectReferrerDetail".
Attachment #716863 - Flags: feedback?(sstamm)
From http://googlewebmastercentral.blogspot.co.uk/2012/03/upcoming-changes-in-googles-http.html

"Starting in April, for browsers with the appropriate support, we will be using the referrer meta tag to automatically simplify the referring URL that is sent by the browser when visiting a page linked from an organic search result. This results in a faster time to result and more streamlined experience for the user."

I tested this in Firefox and in Chrome by searching Google for "what is my referer" and clicking the first result:

Firefox:
http://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&ved=0CDIQFjAA&url=http%3A%2F%2Fwww.whatismyreferer.com%2F&ei=42QtUfO-M6Xr2QXZ7oD4Dg&usg=AFQjCNFt-KMqneTZfEb6OxjPZlD4ogiJcQ&bvm=bv.42965579,d.b2I 

This shows that Google's SERP links are http://www.google.com/url?... links that redirect to the given site.

Chrome:
https://www.google.com/

This shows that Google's links are links directly to the target site.

I think this is likely to be a significant performance issue that should be resolved ASAP. It also looks like something that should be easy to implement.
Keywords: perf
Priority: -- → P1
Sorry, I shouldn't have set the priority field in this DOM bug (I thought it was networking based on the patch).

Owen Chu: Thanks for your patch. But, your patch is fixing a different issue, which is bug 822869. Could you please attach your patch to that bug? I am going to write a message to dev-privacy (https://lists.mozilla.org/listinfo/dev-privacy) to see if we can finish the work you did and ship it in the product soon, and I think bug 822869 (or a new bug) will track that.
Priority: P1 → --

Comment 33

5 years ago
Just implementing the feature on its own (even if we can figure out how, given the lack of a sane spec) is a bit of a pain (have to find all the places where we start loads and add it), but how is Google defining "browsers with the appropriate support"?  If they're just hardcoding things, implementing this won't help.

Note also that I just looked at the search results on google.com in Chrome, and while they have the link behavior you describe they do NOT have any <meta name="referrer"> that I can see.  So are you sure that difference has to do with <meta name="referrer">?
Flags: needinfo?(bsmith)

Comment 34

5 years ago
> how is Google defining "browsers with the appropriate support"?

I can't speak for Google, but here at Facebook the definition is hard-coded internally. It is currently set to: Chrome >= 17 || Safari >= 6. We'd be more than happy to update this hard-coded definition to include a future FF release :-)

> and while they have the link behavior you describe they do NOT have any <meta name="referrer"> that I can see.

Odd, it's visible in my Chrome client: <meta id="mref" name="referrer" content="origin">

Comment 35

5 years ago
> Odd, it's visible in my Chrome client: <meta id="mref" name="referrer" content="origin">

Definitely not over here, in Chrome.  Loaded google.com, searched for "google", opened developer tools, did document.querySelectorAll("meta") and got back a single <meta itemprop="image"> in the list.  View source also shows nothing of the sort.

I should also note that implementing values other than "never" has the issues comment 6 describes...
(In reply to Boris Zbarsky (:bz) from comment #35)
> > Odd, it's visible in my Chrome client: <meta id="mref" name="referrer" content="origin">
> 
> Definitely not over here, in Chrome.  Loaded google.com, searched for
> "google", opened developer tools, did document.querySelectorAll("meta") and
> got back a single <meta itemprop="image"> in the list.  View source also
> shows nothing of the sort.

Make sure you are logged into google.com. I didn't see it either until I logged in. But, I definitely see it in both the "view source" and developer tools in Chrome.

I have posted a proposal for how to move forward with this feature, and the general shortening of the default value of the Referer header here:
https://groups.google.com/d/topic/mozilla.dev.privacy/wmPzPCdzIU8/discussion

I admit I would also like us see how to get to "never" by default but I think that my proposal provides a very good risk/reward ratio.
Flags: needinfo?(bsmith)

Comment 37

5 years ago
Ah, being logged in is key.  I was just confused because the hover tooltip in Chrome was a lie...

I personally can live with your proposal.
(In reply to Boris Zbarsky (:bz) from comment #33)
> Just implementing the feature on its own (even if we can figure out how,
> given the lack of a sane spec) is a bit of a pain (have to find all the
> places where we start loads and add it), but how is Google defining
> "browsers with the appropriate support"?  If they're just hardcoding things,
> implementing this won't help.

I know exactly how to implement this, at least for sub-resources, because of my work on getting the mixed content blocker to work with redirects and HSTS. I am less sure about how to implement it for navigations, but again the mixed content blocker work seemed to shed some insight into it works and I think it is a reasonable amount of effort considering the performance benefits.

As far as the insanity of the spec goes: It would be easier to specify, AFAICT, if it were specified as an HTTP header like Content-Security-Policy that cannot (currently) be set in a <meta> tag. I don't understand the need to support it in <meta>. If it would help move things along, I would be willing to write a draft of a spec for a CSP directive for this. That would have the added advantage that, at least for the DOM team, that we could move this bug to the security component. :)

Comment 39

5 years ago
Oh, I can tell you how to implement it for navigations.

Subresources are actually much harder given speculative loads (hey, the spec explicitly punts on those).  Having an HTTP header instead of a <meta> would address this.

I don't care that much about what component this is in.  I'm slightly more interested in who'd doing the work.  Do you want to, or should I throw together a WebKit-style "don't worry about the edge cases" implementation?

Comment 40

5 years ago
(In reply to Brian Smith (:bsmith) from comment #38)
> It would be easier to specify,
> AFAICT, if it were specified as an HTTP header like Content-Security-Policy
> that cannot (currently) be set in a <meta> tag. I don't understand the need
> to support it in <meta>. If it would help move things along, I would be
> willing to write a draft of a spec for a CSP directive for this. That would
> have the added advantage that, at least for the DOM team, that we could move
> this bug to the security component. :)
Adam Barth said he'd include that to CSP 1.1 [1]. I don't see it now [2] but maybe he already has a draft for that.

[1] http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Jan/0243.html
[2] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html

Comment 41

5 years ago
Created attachment 719688 [details] [diff] [review]
User preference

Improved the patch with Sid's recommendation.
Attachment #716863 - Attachment is obsolete: true
Attachment #719688 - Flags: feedback?(sstamm)

Comment 42

4 years ago
Created attachment 722382 [details] [diff] [review]
User preference

Minor modification to comments.
Attachment #719688 - Attachment is obsolete: true
Attachment #719688 - Flags: feedback?(sstamm)
Attachment #722382 - Flags: feedback?(sstamm)
(Assignee)

Comment 43

4 years ago
Comment on attachment 722382 [details] [diff] [review]
User preference

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

Owen: please see comment 32 (move this work over to bug 822869).  I think we should do that first, then implement the meta tag (or http header) on top of it.
Attachment #722382 - Flags: feedback?(sstamm)

Updated

4 years ago
Attachment #722382 - Attachment is obsolete: true

Comment 44

4 years ago
I just moved the patch over and obsoleted this one.  Thanks!

Comment 45

4 years ago
Created attachment 739700 [details] [diff] [review]
Implement the four meta referrer values (never, always, origin, default) as described in http://wiki.whatwg.org/wiki/Meta_referrer.
Attachment #739700 - Flags: feedback?(bzbarsky)

Comment 46

4 years ago
Comment on attachment 739700 [details] [diff] [review]
Implement the four meta referrer values (never, always, origin, default) as described in http://wiki.whatwg.org/wiki/Meta_referrer.

Some general comments:

1)  Please just make mReferrerPolicy a member on nsIDocument and create an
    inline nsIDocument::ReferrerPolicy() getter.
2)  Use an enumerated type for the member and various arguments.  Ideally one
    that all the code involved can see (e.g. declared in nsNetUtil.h).
3)  The docshell change doesn't make sense.  What you probably want is to
    change all the stuff passing through referrers there to pass through both a
    referrer and a policy or something.  Specifically, consider this testcase:

    <iframe name="foo"></iframe>
    <a target="foo" href="something">Click me</a>

    By the time you land in DoURILoad the docshell is that of the iframe, but I
    assume the relevant referrer policy is that of the parent document...
4)  You probably want an imagelib peer to review the imagelib, and especially
    imagelib API changes.
5)  GetPrePath is wrong if you need an ASCII string, which you do.  What you
    really want is more like nsContentUtils::GetASCIIOrigin.  I wonder whether
    we can move that someplace that necko can access it... or just use
    nsContentUtils here.
6)  Having both a "referrer" attribute and "setReferrer" method on the same
    interface will cause really bad things to happen because of
    <http://support.microsoft.com/kb/131104>.  You need to pick a different
    name.  I filed bug 863960 on the fact that this even compiled.
7)  Please propagate through the referrer policy in the view-source channel.
Attachment #739700 - Flags: feedback?(bzbarsky) → feedback+

Comment 47

4 years ago
Thanks, Boris.  Your feedback clarified several of the doubts I have about my implementation!

I've improved the patch based on your feedback.  The new changes can pass basic tests: http/https, four different settings (never, always, origin, and default), and the iframe case you mentioned in 3).  However, I'm not sure if my changes to docshell and imagelib are appropriate and following the design people in Mozilla expect.  Would you like to review the latest changes? Or who can I ask for help?  Thanks!

Comment 48

4 years ago
I can review the docshell parts.

For imagelib, I guess try :joedrew?
Flags: needinfo?(joe)
Comment on attachment 739700 [details] [diff] [review]
Implement the four meta referrer values (never, always, origin, default) as described in http://wiki.whatwg.org/wiki/Meta_referrer.

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

In general this looks fine from imagelib. I'm not super-enthusiastic about it being an int32_t rather than an enum, but that might just be an xpidl problem.

One comment: you'll want to generate a diff -U 8 so we have more context to review. :)
Attachment #739700 - Flags: feedback+
Flags: needinfo?(joe)

Comment 50

4 years ago
There is zero reason it can't be an IDL enum.  That would still become int32_t in the function signature, but would at least have sane names for the values.

Comment 51

4 years ago
Oh, and on the diff options, you want -p too.  ;)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F :)

Comment 53

4 years ago
Thanks for the advice and feedback!  I'll upload a new patch later today. :)

Comment 54

4 years ago
Created attachment 740567 [details] [diff] [review]
Improved the implementation based on the review feedback.

This new patch addressed all 7 points mentioned in Boris's comment.

Specifically, a new enum type "nsReferrerPolicy" is defined in nsIChannel.idl and is made visible to all involved code.  The referrer policy is now passed to docshell via nsDocShell::LoadURI().  There's one overloaded version of that function, which I'll change later if the current design is approved.
Attachment #739700 - Attachment is obsolete: true
Attachment #740567 - Flags: feedback?

Updated

4 years ago
Attachment #740567 - Flags: feedback? → feedback?(bzbarsky)

Comment 55

4 years ago
You can't use a C++ enum in a scriptable function signature: it has different ABI on different compilers.

Just use idl constants for the IDL bits and an enum in nsIDocument for the member whose values come from the idl.

Updated

4 years ago
Attachment #740567 - Flags: feedback?(bzbarsky) → feedback+

Comment 56

4 years ago
Created attachment 741168 [details] [diff] [review]
Update all callers of nsDocShell::LoadURI().

All callers of nsDocShell::LoadURI are updated to pass in the referrer policy through nsIDocShellLoadInfo objects.  The overloaded version of the function doesn't have to be changed because all callers pass in NULL as the referrer URI.

Callers of HttpBaseChannel::SetReferrer are reviewed and updated to call SetReferrerWithPolicy instead, except for the invocation of the function in the following files:

txMozillaStylesheetCompiler.cpp
nsFontFaceLoader.cpp
nsNetUtil.h
nsDownloadManager.cpp
nsOfflineCacheUpdate.cpp

I'm not sure if I can/should obtain a referrer in these call sites.  It would be great if you can give me some advice. :-)
Attachment #740567 - Attachment is obsolete: true
Attachment #741168 - Flags: feedback?(bzbarsky)
(In reply to Owen Chu from comment #45)
> Created attachment 739700 [details] [diff] [review]
> Implement the four meta referrer values (never, always, origin, default) as
> described in http://wiki.whatwg.org/wiki/Meta_referrer.

Did you see the discussion on dev.privacy a few weeks?

I still object to implementing "always" as specified in the wiki. Instead, I think that we should implement "always" to be like "origin" for HTTP pages. I suggest we try doing things that way and work on getting consensus for that (changing the proposed spec text above). If we implement "always" as specified in the wiki then we will risk backward-compatibility problems if we change this behavior later, whereas if we start out doing what I suggest, we can always change to what the wiki page says to do later without any concerns.
Facebook uses this mechanism for cross-origin links on facebook.com. They generate HTML with <meta referer=default> in the HTML. Then, in the click handler for links, they use the DOM to modify the element to be <meta referer=origin> if the link was a cross-origin link; then, after the click has been processed, they use the DOM to set it back to <meta referer=default>.

I believe many sites (Twitter, Google, etc.) would prefer to have similar behavior: cross-origin links are "origin" and same-origin links are "always".

I think the trick that Facebook is doing is ugly and also possibly broken if/when we consider likely future pre-fetching/pre-loading behavior. So, we should support some kind of "referer" attribute on links. IMO, such an attribute would be much better than <meta referer>. I would actually consider <a referer> to be a must-implement feature whereas I don't care whether or not we implement <meta referer> at all once we have <a referer>.

(Sorry for the late feedback here.)
(Assignee)

Comment 59

4 years ago
(In reply to Brian Smith (:bsmith) from comment #58)
> Facebook uses this mechanism for cross-origin links on facebook.com. 

If I recall correctly, they also want to use this mechanism for cross-origin frames, not just links.  They want to control data exfiltration without needing to use a redirect mechanism to scrub URLs.  This is where the real value of the feature is IMO; not in link-referrer changing, but in subordinate load referrers.
(In reply to Brian Smith (:bsmith) from comment #57)
> I still object to implementing "always" as specified in the wiki. Instead, I
> think that we should implement "always" to be like "origin" for HTTP pages.

I meant: ... for HTTPS pages.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #59)
> If I recall correctly, they also want to use this mechanism for cross-origin
> frames, not just links.

<iframe referer='...'> would make more sense for that, IMO.

Anyway, I am not against adding the <meta referer> if others support doing so. But, I think it is definitely important to have the other mechanisms (<a referer>, <iframe referer>, etc.).

Comment 61

4 years ago
Comment on attachment 741168 [details] [diff] [review]
Update all callers of nsDocShell::LoadURI().

This looks generally fine on the non-necko bits, but it would be good to say in the loadinfo and shentry idl where the values of that argument are coming from (by pointing to the constants on nsIHttpChannel).

Why is the referrerPolicy attribute on nIHttpChannel writable?

Why does EventSource::GetReferrerPolicy not just return an nsReferrerPolicy?  It's always returning NS_OK right now...

Please do sort out the actual http channel behavior with bsmith.
Attachment #741168 - Flags: feedback?(bzbarsky) → feedback+

Comment 62

4 years ago
Thanks for your feedback!  I'll update the patch again today.

Brian: Would you like to review my changes related to the http channel and the call sites of SetReferrer()?

Comment 63

4 years ago
Created attachment 742736 [details] [diff] [review]
Tidy up the patch
Attachment #741168 - Attachment is obsolete: true

Comment 64

4 years ago
Owen, are you blocked on something at this point, other than lack of time?

Comment 65

4 years ago
I've implemented a mochitest for testing and realized that speculative parsing sends out referrers before the meta referrer tag takes effect (as mentioned in comment 9).  Should I deal with it (by making the referrer take effect for subresources)?  Or should I leave it for future CSP enhancement?

Comment 66

4 years ago
Sid?
Flags: needinfo?(sstamm)
(Assignee)

Comment 67

4 years ago
Owen: this sounds a little problematic.  Do you know what Chrome does here?  I think suppressing the speculative loads is a suboptimal solution and I'm not familiar enough with the underpinnings to figure out how to pre-parse the meta tag.

bz: do you know if we have this kind of "can't prefetch until parsed" problem with any other meta or meta/http-equiv tags?
Flags: needinfo?(sstamm)

Comment 68

4 years ago
This already happens for <base>: see comment 13 and comment 16.

Comment 69

4 years ago
Created attachment 769638 [details] [diff] [review]
Support speculative loads and add test cases

I implemented support for speculative loads and added test cases to verify that the patch works as expected for http/https, four different referrer settings (default/never/always/origin), and seven html/css use cases.

I'm sure that the test cases are far from complete to catch all use cases.  For example, code using URLValue may need further review and modification.  It would be great to have some feedback on how to make the test cases (more) complete.

Thanks! =]
Attachment #742736 - Attachment is obsolete: true
Attachment #769638 - Flags: feedback?(bzbarsky)

Comment 70

4 years ago
Sorry for the lag here; still catching up from leave... I should hopefully get to this by Monday.

Comment 71

4 years ago
Comment on attachment 769638 [details] [diff] [review]
Support speculative loads and add test cases

Actually changing the referrer policy on the document based on speculation seems  wrong to me.  What if the speculation fails?  What we should probably do instead is pass a referrer policy through to the speculative load callsites.

As far as the test goes, it's worth testing the referrer sent for a subframe load via <iframe src>, a location.href set, an <object data="whatever"> load, a load via window.open, a form submission, an SVG resource document, maybe XSLT.  Maybe EventSource and WebSocket and XMLHttpREquest as well? Also document.load and FileReader?  And audio/video atuff...
Attachment #769638 - Flags: feedback?(bzbarsky) → feedback+

Updated

4 years ago
Duplicate of this bug: 933405
See Also: → bug 530396
(Assignee)

Updated

4 years ago
Assignee: nobody → sstamm

Comment 73

4 years ago
It's apparently possible to achieve the same thing using CSP 1.1 meta element and CSP 1.1 referrer directive [1].
Should this bug be closed in favor of bugs 663570 and bug 965727 ? (the work of this bug can certainly be split across these two bugs)

[1] http://lists.w3.org/Archives/Public/public-webappsec/2014Jan/0200.html
(Assignee)

Comment 74

4 years ago
I think we should still implement this bug, but we should be able to use the same back-end for bug 663570 and bug 965727.  Not all web developers will want to use CSP.
See Also: → bug 965727, bug 663570
(Assignee)

Comment 75

4 years ago
Hey Owen, the patch on this bug doesn't apply cleanly to mozilla-central.  Do you have a newer version of the patch I can try?
Flags: needinfo?(owenchu)

Comment 76

4 years ago
I don't understand. When bug 663570 and bug 965727 are implemented, 
<meta name="referrer" content="XXX">
will be equivalent to:
<meta http-equiv="Content-Security-Policy" content="referrer: XXX">

I don't understand the benefit (beyond a few chars) to implementing this bug in addition to bug 663570 and bug 965727
(Assignee)

Comment 77

4 years ago
I should have read the w3c thread you linked first, I kind of understand where you're coming from so I'm not dead set on this.

Seems to me that neither the CSP 1.1 meta directive or meta referrer are stable specs yet, so either is subject to change.  Since it's not a lot of extra work to support both (as you suggested they're different syntax for the same thing), I figure why not at least start with both since I'm currently hearing specific requests for the meta referrer implementation?

Comment 78

4 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #77)
> Seems to me that neither the CSP 1.1 meta directive or meta referrer are stable specs yet, so either is subject to change.
The referrer directive is as stable as <meta name="referrer"> (both are Adam Barth's work AFAICT. see comment #9). Blink is implementing https://code.google.com/p/chromium/issues/detail?id=309551
I just sent feedback about the meta element and based on the responses, it feels stable pretty stable.
What gives you this impression?

> I figure why not at least start with both since I'm
> currently hearing specific requests for the meta referrer implementation?
Implementing both will add spec/implementation complexity (decide what to do when both are present in the same page). But you're implementing, so if that's fine by you and the reviewers...
Are the specific requests that pressing they can't wait CSP 1.1 to stabilize? Facebook has been asking for this since 2010, I'm not sure I understand the sudden rush.
Implementing CSP meta and referrer would be a way to send feedback too and help the spec stabilize.

Comment 79

4 years ago
> Facebook has been asking for this since 2010, I'm not sure I understand the sudden rush.

Small update to comment #14, This is now the last remaining issue blocking Strict-Transport-Security on facebook.com for Firefox users (it's been enabled for Chrome/Safari users for some time). I wouldn't say that we're rushed, we're happy to wait for your preferred solution, but I would love to see HSTS enabled sooner rather than later.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #77)
> Seems to me that neither the CSP 1.1 meta directive or meta referrer are
> stable specs yet, so either is subject to change.  Since it's not a lot of
> extra work to support both (as you suggested they're different syntax for
> the same thing), I figure why not at least start with both since I'm
> currently hearing specific requests for the meta referrer implementation?

I think that we should choose either CSP referrer or <meta referrer> but not both. It seems like the CSP referrer mechanism could handle the t.co redirect case, if t.co put the CSP header on its 30x responses. So, that's +1 for CSP referrer.

AFAICT, Facebook needs to be able to control the referer on a link-by-link basis. IIUC, they are currently using onClick handlers to change the <meta referer> element in Chrome to do this. Supporting something similar using the CSP-based mechanism seems like a lot of work. So, going with CSP referrer may mean that we need to support <a referrer> too.

My vote is for doing <a referrer> first, CSP referrer next, and WONTFIX this.
(Assignee)

Comment 81

4 years ago
Owen: you've done a bit of work on this, how hard do you think it will be to repurpose the work you've done to fit into CSP/meta and CSP referrer?

Comment 82

4 years ago
Hello Sid,

As you pointed out, the patch doesn't apply cleanly to mozilla-central.  I'm in the process of migrating the changes to the newer mozilla-central codebase.  But before further implementing this feature, I'd like to make the test suite as complete as possible so that we know we won't leak referrer information in any imaginable way when the implementation is done.

I'm in Tahoe now and don't have the code at hand. I can upload the tests later when I get home.

As for CSP/meta and CSP referrer, I haven't read much CSP-related code.  It won't be much work if it's just a matter of parsing the markup in a different way and other code changes can still apply.  But I'm not sure if this is the case.
Flags: needinfo?(owenchu)
(Assignee)

Comment 83

4 years ago
do you have more tests for the suite that aren't posted on the bug here, or is that a todo?

I can help with the CSP related code.  I'll read through your patch and see what it'll take to implement CSP referrer.  I think we should probably opt for that over meta referrer so long as we can quickly implement meta tag support for CSP.
Flags: needinfo?(owenchu)

Comment 84

4 years ago
I have some more tests for the suite locally, and I'm still working on it based on the feedback Boris gave me in comment 71.
Flags: needinfo?(owenchu)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #83)
> I think we should probably opt for that over meta referrer so
> long as we can quickly implement meta tag support for CSP.

I don't want to speak for Facebook, Twitter, Google, etc. (it would be good for them to say one way or another) but it seems like they could likely make use of CSP referrer in the CSP header without needing meta tag support for CSP. Thus, I think we can/should ship CSP referrer without blocking on meta tag support for CSP.

Jeff Pinner ‏(@jpinner) from Twitter said he's having a meeting at Twitter today to discuss if/how CSP referrer could be used by t.co. I sent him a link to this bug. I hope somebody from Twitter will comment either here or in the CSP referrer bug (bug 965727).

Similarly, it would be nice if Alex Rice could comment on CSP referrer vs. <meta referer> and whether the CSP header (as opposed to <meta> support fro CSP) would meet Facebook's needs.
Flags: needinfo?(arice)

Comment 86

4 years ago
> Similarly, it would be nice if Alex Rice could comment on CSP referrer vs. <meta referer> and whether the CSP header (as opposed to <meta> support fro CSP) would meet Facebook's needs.

Facebook does not have any short term plans to change the global referrer policy from "default". Multiple internal measurement and abuse systems rely upon their ability to observe the referrer of traffic within our origin. I'd love to see these systems break this dependency, but it's unlikely to happen on any reasonable time frame. As a result, supplying a CSP referrer policy in the header is not functionality we'd use at this time. (not to say it isn't useful for many other sites)

The specific use case we're interested in is enforcing an "origin" referrer policy for all cross origin (external) traffic, while maintaining a "default" referrer policy for same origin traffic. In Chrome/Safari, this is currently accomplished through an onclick event applied to all anchor tags that inspects the anchor's destination and dynamically modifies the <meta referrer> policy. In Firefox, it's accomplished through an interstitial endpoint.

The CSP specification appears to prohibit modifications to the policy after one has been set. Assuming my reading of the spec is accurate, the technique Facebook currently uses to dynamically modify the referrer-policy would not appear to be compatible with CSP referrer set from a meta tag.

As I understand it, there are three possible solutions that would satisfy this use case. We have no strong preference between the three.

- <a referrer="origin" ... >
- <meta name="referrer" content="default">, modified to "origin" in an onclick handler on external URLs
- <meta http-equiv="content-security-policy" name="referrer" content="default">, modified to "origin" in an onclick handler on external URLs
Flags: needinfo?(arice)

Comment 87

4 years ago
(In reply to Alex Rice from comment #86)
> The CSP specification appears to prohibit modifications to the policy after
> one has been set.
That's my reading too.

> Assuming my reading of the spec is accurate, the technique
> Facebook currently uses to dynamically modify the referrer-policy would not
> appear to be compatible with CSP referrer set from a meta tag.
Yes. It reminds me a discussion on the topic of meta referrer expressiveness. "always"/"never" should be "complete"/"empty". We need 2 axis of expressiveness. One about what is being sent, one about in which context (when the referer is sent to same or different origin). I'll raise this issue on public-webappsec.

> As I understand it, there are three possible solutions that would satisfy
> this use case. We have no strong preference between the three.
> 
> - <a referrer="origin" ... >
> - <meta name="referrer" content="default">, modified to "origin" in an
> onclick handler on external URLs
> - <meta http-equiv="content-security-policy" name="referrer"
> content="default">, modified to "origin" in an onclick handler on external
> URLs
The last one won't be possible. The two first could work (though I'd wish the second disappeared)

Comment 88

4 years ago
Created attachment 8368614 [details] [diff] [review]
Test cases (WIP)

Comment 89

4 years ago
Created attachment 8368645 [details] [diff] [review]
Test cases (WIP)
Attachment #8368614 - Attachment is obsolete: true
(Assignee)

Comment 90

4 years ago
Created attachment 8372641 [details] [diff] [review]
implementation and a few tests

I rebased attachment 769638 [details] [diff] [review], integrating the policy enforcement logic into the stuff that got added when we landed bug 822869.  SetReferrer() could be cleaner.  I'm going to update bug 965727 to depend on this since it seems like this is pretty close and the logic for applying the referrer policy is mostly done.
Attachment #769638 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 965727

Comment 91

4 years ago
Created attachment 8379818 [details] [diff] [review]
Implementation and test cases

Added test cases suggested in comment 71.

Boris: FileReader seems to deal with local files only.  I'm not sure how to make it send a referrer.  WebSocket doesn't send any referrer;  At least I didn't see one in a _wsh.py file that handles WebSocket requests.  And, I have no clue how to test XSLT.  It would be great if you can give me some suggestions.  :-)

The current test result shows that <object>, document.load, <audio>, and <video> are not following the meta referrer tag yet.

I obsoleted both attachment 8368645 [details] [diff] [review] and attachment 8372641 [details] [diff] [review] as this one improves the first one and includes the second.
Attachment #8368645 - Attachment is obsolete: true
Attachment #8372641 - Attachment is obsolete: true
Attachment #8379818 - Flags: feedback?(sstamm)
Attachment #8379818 - Flags: feedback?(bzbarsky)

Comment 92

4 years ago
> Boris: FileReader seems to deal with local files only.

If it's not doing http, then no need to worry about referrers.  I guess it only reads blobs, so we're ok there.

> WebSocket doesn't send any referrer

Great.

> And, I have no clue how to test XSLT.

http://mxr.mozilla.org/mozilla-central/search?string=text%2Fxsl&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central shows lots of tests in the tree that you could try copying?

What feedback on the patch are you looking for?

Updated

4 years ago
Flags: needinfo?(owenchu)

Updated

4 years ago
Attachment #8379818 - Flags: feedback?(sstamm)
Attachment #8379818 - Flags: feedback?(bzbarsky)
Flags: needinfo?(owenchu)

Comment 93

4 years ago
Sorry for the late response.  I'd been away for a couple of days.

Boris: Thanks for the feedback.  That's all I need for the patch.  :-)

Sid: I'll finish the tests this week and see if I have time to continue on the implementation.

Comment 94

3 years ago
Created attachment 8384443 [details] [diff] [review]
Implementation and test cases
Attachment #8379818 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 999754
(Assignee)

Comment 95

3 years ago
Created attachment 8411401 [details] [diff] [review]
meta-ref

Fixed a bit of bitrot, tweaked tests so they run, and implemented referrer policy for <object> loads.

Still needs test for XSLT.

Owen, I'd like to get this done pretty quickly so we can move on the bugs it blocks.  Any chance the two of us can try and close this out this week, or should I pick it up?
Attachment #8384443 - Attachment is obsolete: true
Flags: needinfo?(owenchu)
(Assignee)

Comment 96

3 years ago
bz: since you've seen this and since most of the big changes are in docshell/content, would you be willing to review the patch once you get back from PTO?  I'll probably rope in someone from necko for those bits too.
Flags: needinfo?(bzbarsky)

Comment 97

3 years ago
I can do that, but I have 30+ patches ahead of it in my review queue as of right now.  So chances are review will be a bit laggy, especially for a patch this big.  :(
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 98

3 years ago
Ok, thanks bz.  I'll break this patch up into pieces, I think, and point you at just one or two.  Most of the heft comes from context (lots of one/two-line changes).

Comment 99

3 years ago
Sid: Yes, I can pick up the remaining work in the implementation this evening.  I'll let you know if there is still any code that I don't know how to patch tomorrow morning.

Last time I tried to add a test for XSLT, but it turned out that the SJS script's handleRequest function couldn't intercept the HTTP request for xml-stylesheet.  I wonder if this is a known limitation of Mochitest?
Flags: needinfo?(owenchu)
(Assignee)

Comment 100

3 years ago
Owen: AFAICT, the only thing left to do is test XSLT and split the patch (I'm happy to split the patch, will only take a few minutes).  I added the object loading stuff and made the tests work.

It couldn't intercept the HTTP request for xml-stylesheet?  That seems really odd.  Was the XSL request happening before the http handler was registered and started?

Comment 101

3 years ago
There's some work left in these files:

/content/base/src/nsSyncLoadService.cpp
/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
/netwerk/protocol/http/HttpChannelParent.cpp
/toolkit/components/downloads/nsDownloadManager.cpp
/uriloader/prefetch/nsOfflineCacheUpdate.cpp

I think that ideally/eventually we should be able to replace all calls to SetReferrer with SetReferrerWithPolicy and remove the former from HttpBaseChannel's interface.

I'll make a patch for the non-working XSLT test on top the one we have now and upload it in a couple of minutes.

Comment 102

3 years ago
Created attachment 8411927 [details] [diff] [review]
Non-working XSLT test
Attachment #8411927 - Flags: feedback?(sstamm)
(Assignee)

Comment 103

3 years ago
Comment on attachment 8411927 [details] [diff] [review]
Non-working XSLT test

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

::: content/base/test/bug704320.sjs
@@ +149,5 @@
> +  // TODO: Not sure where to put the meta tag.
> +  return '<?xml version="1.0" encoding="UTF-8"?>\n\
> +         <?xml-stylesheet type="text/xsl" href="' + url + '" ?>\n\
> +         <content>\n\
> +           <meta name="referrer" content="' + policy + '">\n\

Hmm... I'm starting to wonder if meta referrer even makes sense for XSLT.
Attachment #8411927 - Flags: feedback?(sstamm)
(Assignee)

Comment 104

3 years ago
Peter: You know about XSLT stuff.  This meta referrer thing is supposed to dictate how the document sends referrers for requests it generates.  Does this make sense for XML, especialy as XSLT requests seem to carry referrers?
Flags: needinfo?(peterv)
(Assignee)

Comment 105

3 years ago
Created attachment 8411993 [details] [diff] [review]
0_meta-ref-base
Attachment #8411401 - Attachment is obsolete: true
(Assignee)

Comment 106

3 years ago
Created attachment 8411995 [details] [diff] [review]
1_useSetReferrerWithPolicy
(Assignee)

Comment 107

3 years ago
Created attachment 8411996 [details] [diff] [review]
2_metaref_tests
(Assignee)

Comment 108

3 years ago
I split the patch into three parts (in order of application):

0 - the base implementation in parser/dochsell/nsIHttpChannels
1 - installation of referrer policy for the various things that generate requests and set referrers
2 - tests

The xslt tests apply on top of all of this (not folded into patch 2 in case we don't want them).  I also added some todos into patch 1 for items left to do in comment 101.
(Assignee)

Comment 109

3 years ago
Comment on attachment 8411993 [details] [diff] [review]
0_meta-ref-base

bz, mcmanus, I'm going to flag you two for review on this first patch for the content, docshell and necko bits.  The policy for loads doesn't necessarily always get set with only this first patch, but the bulk of the plubming is here.  I know you're both pretty slammed, so don't feel bad if you can't get to it right away.

If you want to test this locally, I recommend applying all three patches (0-2), but this one patch _should_ compile and not break other stuff without patches 1 and 2.
Attachment #8411993 - Flags: review?(mcmanus)
Attachment #8411993 - Flags: review?(bzbarsky)

Comment 110

3 years ago
Created attachment 8412640 [details] [diff] [review]
3_nsDownloadManager

Will add a test under toolkit/components/downloads/test/schema_migration/ if these changes make sense.

Comment 111

3 years ago
nsDownloadManager: Created a patch above.

nsWebBrowserPersist.cpp: The only way that the execution flow would hit the SetReferrer line with a non-null referrer is through nsIWebBrowserPersist::saveURI, so we can either change the interface to include an extra referrer policy parameter or just rely on the client to pass an appropriate referrer on the spot.  The latter makes more sense to me because the former would not be backward compatible.

Sid: Feel free to pick up the remaining stuff. I could only get this far today. :)
Comment on attachment 8411993 [details] [diff] [review]
0_meta-ref-base

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

::: netwerk/base/public/nsReferrerPolicy.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

please just name the file ReferrerPolicy.h

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsReferrerPolicy_h__

drop the ns

@@ +6,5 @@
> +#define nsReferrerPolicy_h__
> +
> +#include "nsIHttpChannel.h"
> +
> +enum nsReferrerPolicy {

place this in namespace mozilla::net and drop the ns prefixes

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1061,5 @@
>  
>    nsAutoCString spec;
>  
> +  // site-specified referrer trimming may affect the trim level
> +  // "Always" behaves like "origin", so in these two cases the trim level

sid and I talked this over on irc.. origin should be 2.. always should be unchanged. or we should at least double check it.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +203,5 @@
>    if (docUri)
>      mChannel->SetDocumentURI(docUri);
>    if (referrerUri)
>      mChannel->SetReferrerInternal(referrerUri);
> +    //TODO set referrer policy too?

you're definitely going to need this - otherwise e10s will break

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +55,5 @@
>       */
>      attribute nsIURI referrer;
>  
> +    /* Referrer policy enums */
> +    const long REFERRER_POLICY_DEFAULT = 0;

I like my enums to be unsigned.. can we make all the consts and member vars that carry this stuff around unsigned?
Attachment #8411993 - Flags: review?(mcmanus)
Comment on attachment 8411993 [details] [diff] [review]
0_meta-ref-base

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

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +50,1 @@
>       * never be sent as the referrer for a plaintext HTTP request).  The

idl needs a uuid bump
(Assignee)

Comment 114

3 years ago
Created attachment 8415685 [details] [diff] [review]
0_meta-ref-base

New meta-referrer base patch, addressing comments from mcmanus.
Attachment #8411993 - Attachment is obsolete: true
Attachment #8411993 - Flags: review?(bzbarsky)
Attachment #8415685 - Flags: review?(mcmanus)
Attachment #8415685 - Flags: review?(bzbarsky)
(Assignee)

Comment 115

3 years ago
Created attachment 8415687 [details] [diff] [review]
1_useSetReferrerWithPolicy

had to update patch 1_useSetReferrerWithPolicy due to changes in patch 0.
Attachment #8411995 - Attachment is obsolete: true
(Assignee)

Comment 116

3 years ago
Created attachment 8415689 [details] [diff] [review]
2_metaref_tests

Had to update tests from review comments.
Attachment #8411996 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8415687 - Attachment filename: useSetReferrerWithPolicy → 1_useSetReferrerWithPolicy
(Assignee)

Updated

3 years ago
Attachment #8415687 - Attachment description: useSetReferrerWithPolicy → 1_useSetReferrerWithPolicy
Comment on attachment 8415685 [details] [diff] [review]
0_meta-ref-base

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

you need to do the e10s bit.. otherwise the necko bits lgtm

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +203,5 @@
>    if (docUri)
>      mChannel->SetDocumentURI(docUri);
>    if (referrerUri)
>      mChannel->SetReferrerInternal(referrerUri);
> +    //TODO set referrer policy too?

you really do need this..
Attachment #8415685 - Flags: review?(mcmanus)
(Assignee)

Comment 118

3 years ago
(In reply to Patrick McManus [:mcmanus] from comment #117)
> you really do need this..

Hah, yeah, of course, sorry.  I got eager after working on this yesterday.  Owen is on it, and  we'll post an update when he's got it ready.

Comment 119

3 years ago
Created attachment 8416483 [details] [diff] [review]
metaref_tests

Fix typos and syntax errors in test_bug704320.html.
Attachment #8415689 - Attachment is obsolete: true

Comment 120

3 years ago
Created attachment 8416519 [details] [diff] [review]
3_nsDownloadManager

Update patch 3_nsDownloadManager due to changes in patch 0.
Attachment #8412640 - Attachment is obsolete: true

Comment 121

3 years ago
Created attachment 8416521 [details] [diff] [review]
4_nsSyncLoadService

Comment 122

3 years ago
Created attachment 8419480 [details] [diff] [review]
5_HttpChannelParent

I wonder if nsHttpChannel::SetReferrerWithPolicyInternal (previously called SetReferrerInternal) needs more work, as the referrer here doesn't go through the whole policy enforcing process implemented in HttpBaseChannel::SetReferrerWithPolicy.
Attachment #8419480 - Flags: review?(mcmanus)

Updated

3 years ago
Attachment #8419480 - Attachment description: :mcmanus → 5_HttpChannelParent
Attachment #8419480 - Attachment is patch: true
Comment on attachment 8419480 [details] [diff] [review]
5_HttpChannelParent

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

lgtm

::: netwerk/protocol/http/HttpChannelParent.h
@@ +74,5 @@
>    bool DoAsyncOpen(const URIParams&           uri,
>                     const OptionalURIParams&   originalUri,
>                     const OptionalURIParams&   docUri,
>                     const OptionalURIParams&   referrerUri,
> +                   const uint32_t&            referrerPolicy, 

whitespace at end of line
Attachment #8419480 - Flags: review?(mcmanus) → review+
Comment on attachment 8415685 [details] [diff] [review]
0_meta-ref-base

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

r+ on netwerk bits assuming it goes along with patch 5 for e10s issue

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +203,5 @@
>    if (docUri)
>      mChannel->SetDocumentURI(docUri);
>    if (referrerUri)
>      mChannel->SetReferrerInternal(referrerUri);
> +    //TODO set referrer policy too?

you really do need this..
Attachment #8415685 - Flags: review+

Comment 125

3 years ago
Created attachment 8420761 [details] [diff] [review]
5_HttpChannelParent

Remove a trailing whitespace.
Attachment #8419480 - Attachment is obsolete: true
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #104)
> Peter: You know about XSLT stuff.  This meta referrer thing is supposed to
> dictate how the document sends referrers for requests it generates.  Does
> this make sense for XML, especialy as XSLT requests seem to carry referrers?

Isn't this an HTML tag? If it is then it doesn't make sense in the XML source document for XSLT. It might make sense to support it in the result document, depending on how it's implemented it should jsut work as is or you need to add support in the output handler: http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaXMLOutput.cpp#742
Flags: needinfo?(peterv)
Comment on attachment 8415685 [details] [diff] [review]
0_meta-ref-base

I'm really sorry for the lag here.  :(

>+++ b/content/base/public/nsIDocument.h	Wed Apr 30 15:18:58 2014 -0700
>+using mozilla::net::ReferrerPolicy;

No using declarations in headers, please.

>+++ b/content/base/src/nsContentSink.cpp	Wed Apr 30 15:18:58 2014 -0700
>+    if (!result.IsEmpty()) {

So if I have:

  <meta name="referrer" content="origin">
  <meta name="referrer" content="">

I'll get the origin policy.  Is this specced somewhere?  It doesn't seem to be what http://wiki.whatwg.org/wiki/Meta_referrer says.

Furthermore, that wiki draft says this happend whenever a <meta name="referrer"> is inserted into the DOM, whether it's done by the parser or not.  That doesn't seem to be what this patch is implementing.  Why not?

>+      mDocument->SetHeaderData(nsGkAtoms::referrer, result);

Does that end up trimming leading/trailing whitespace, like the wikidraft says?  If not, why are we not doing that?

>+++ b/content/base/src/nsFrameLoader.cpp	Wed Apr 30 15:18:58 2014 -0700
>+  nsIDocument* doc = mOwnerContent->OwnerDoc();
>+  if (doc) {

OwnerDoc() never returns null; hence the name-without-Get.

>+++ b/docshell/base/nsDocShell.cpp	Wed Apr 30 15:18:58 2014 -0700

I wonder whether it would have made sense to make up a "referrer and policy" struct of some sort...  Either way, I guess.

>+++ b/netwerk/protocol/http/HttpChannelParent.cpp	Wed Apr 30 15:18:58 2014 -0700
>+    //TODO set referrer policy too?

What are the consequences of not doing this?

>+++ b/parser/html/nsHtml5SpeculativeLoad.cpp	Wed Apr 30 15:18:58 2014 -0700
>+      aExecutor->SetReferrerPolicy(mMetaReferrerPolicy);

This is changing actual document state based on a speculative tokenization.  That seems pretty wrong to me...  what if the speculation turns out to be incorrect?

What this should do instead is set a speculative referrer policy of some sort and that should be passed along to the various speculative loads we do.  Then whether the real load can reuse the speculative load will depend on whether the referrer policy matches.

>+              mSpeculativeLoadQueue.AppendElement()->InitMetaReferrerPolicy(
>+                                                              *referrerPolicy)

Again, this doesn't seem to be doing the whitespace trimming bits from the wiki.
Attachment #8415685 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 128

3 years ago
Thanks for the review, bz.  I'm going to try to pick up some of this bug since Owen has become pretty busy.

Looks like the W3C is considering picking up referrer policy spec stuff here:
https://w3c.github.io/webappsec/specs/referrer-policy/

(In reply to Boris Zbarsky [:bz] from comment #127)
> So if I have:
> 
>   <meta name="referrer" content="origin">
>   <meta name="referrer" content="">
> 
> I'll get the origin policy.  Is this specced somewhere?  

The above draft linked says to abort parsing/using any policy that is the empty string.  That makes the second meta tag a no-op.

> Furthermore, that wiki draft says this happend whenever a <meta
> name="referrer"> is inserted into the DOM, whether it's done by the parser
> or not.  That doesn't seem to be what this patch is implementing.  Why not?

That's a good question.  Is there a better place to process Meta tags (especially those inserted by script)?

> >+++ b/netwerk/protocol/http/HttpChannelParent.cpp	Wed Apr 30 15:18:58 2014 -0700
> >+    //TODO set referrer policy too?
> 
> What are the consequences of not doing this?

(FYI, probably means it doesn't work in e10s.  As mcmanus said in comment 124, we need to do this.)
Flags: needinfo?(bzbarsky)
> The above draft linked says to abort parsing/using any policy that is the empty string.  

Great, thanks.

> Is there a better place to process Meta tags (especially those inserted by script)?

HTMLMetaElement.cpp, depending on whether you actually want that behavior.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8416483 - Attachment is patch: true
(Assignee)

Updated

3 years ago
Attachment #8416519 - Attachment is patch: true
(Assignee)

Updated

3 years ago
Attachment #8416521 - Attachment is patch: true
(Assignee)

Comment 130

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #127)
> >+++ b/parser/html/nsHtml5SpeculativeLoad.cpp	Wed Apr 30 15:18:58 2014 -0700
> >+      aExecutor->SetReferrerPolicy(mMetaReferrerPolicy);
> 
> This is changing actual document state based on a speculative tokenization. 
> That seems pretty wrong to me...  what if the speculation turns out to be
> incorrect?
> 
> What this should do instead is set a speculative referrer policy of some
> sort and that should be passed along to the various speculative loads we do.
> Then whether the real load can reuse the speculative load will depend on
> whether the referrer policy matches.

I did some digging and don't quite fully grasp the best way to do this.  (And I'll admit I haven't fully figured out how the speculative parsing/preloading works and relates to the rest of the DOM construction, so please excuse my possibly incomplete understanding).  

My current thinking is:
1. Keep a "mSpeculativeReferrerPolicy" in the nsHtml5TreeOpExecutor, 
1.1. Use that for preloads triggered by the speculative parsing
2. Set the document's referrer policy (officially) in HTMLMetaElement.cpp when it gets added to the DOM
2.1. Use the referrer policy in nsDocument for any non-speculative loads/navigations.

I'm not exactly sure what to do when the *actual* policy and the speculative one don't match.  If I implement what I've just written, some loads could potentially carry a different referer than the others.  It seemes to me this only happens when script in the loading page creates a meta tag and adds it to the DOM.  

Am I correct in thinking this?  If that's the case, I'm not too worried about perfecting this since a properly non-script-generated <meta> will result in the same policy during and after speculative parsing.

What should I do to determine if we can reuse speculative loads?  Should we just accept the inconsistency?  I'm, frankly, happy with that.
Flags: needinfo?(bzbarsky)
> It seemes to me this only happens when script in the loading page creates a meta tag and
> adds it to the DOM

Or script writes out comments that comment out the <meta> the parser saw.

I _think_ those are the only two ways you can get mismatches.

> I'm not too worried about perfecting this since a properly non-script-generated <meta>
> will result in the same policy during and after speculative parsing.

Yes, agreed.

> What should I do to determine if we can reuse speculative loads?

We could conceivably store the referrer policy used as part of the check, right?  Seems like we'd want that as part of the hashkey anyway for stylesheets and images (just like the CORS mode is already), no matter what happens with preloads.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 132

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #131)
> We could conceivably store the referrer policy used as part of the check,
> right?  Seems like we'd want that as part of the hashkey anyway for
> stylesheets and images (just like the CORS mode is already), no matter what
> happens with preloads.

Yeah, this makes sense.  I'll need to figure out how to update the hashkey for preloaded stylesheets and images, so while I previously thought I could have an update for Monday, it may be a bit longer.  I'll probably update the setReferrerWithPolicy patch at the same time.  Thanks for the help, bz.
(Assignee)

Comment 133

3 years ago
Created attachment 8477730 [details] [diff] [review]
0_meta-ref-base - WIP

Just updating this bug on status.  I think I figured out how to tag the script preloads with the referrer policy (invalidating them if the regular load uses a different referrer policy), but the image and style cases seem more complex.

With images, the preloading uses most of the same logic as "real" loading.  I either need to add some load flags to imgILoader corresponding to the referrer policy, or thread an additional argument throughout all the various LoadImage function calls (nsContentUtils, imgILoader, etc).  Load flags seem easier to implement, but they are possibly fragile since this creates a second place where the referrer policies are enumerated (other is in ReferrerPolicy.h).  The argument-threading will bloat a bunch of call-sites that are not preloads.

There's a similar, but less-complicated issue with PreloadStyle... it uses the CSSLoader()->LoadSheet() just like the real loads do.  It does thread CORS mode through the calls, and I think I can do the same thing with the referrer policy.

bz: I'd like your thoughts on the approach with PreloadScript() and the first part of nsHtml5TreeOpExecutorPreloadStyle().  Am I on the right track?
Attachment #8415685 - Attachment is obsolete: true
Attachment #8477730 - Flags: feedback?(bzbarsky)
Comment on attachment 8477730 [details] [diff] [review]
0_meta-ref-base - WIP

Sorry for the lag; still catching up on things.

The scriptloader changes look pretty reasonable.

So do the css changes that are in the patch so far.  The CSS loader will need to consider the referrer policy as part of the hashtable key, presumably.
Attachment #8477730 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 135

3 years ago
Thanks, bz.  For stylesheets, I saw what you did in bug 732209, and I'll probably do something similar.  I haven't figured out the image loader stuff yet though.

This is getting complex and big so I'm going to pull out the image/style/script referrer stuff (preload and non preload) into their own patches.
Yep, bug 732209 is the right thing to crib.  Similarly, for images the patch that added CORS mode to them should be cribbable.
(Assignee)

Comment 137

3 years ago
I made good progress on the image loader, but the rabbit hole went deeper than I expected and I didn't get the patches into a shape I'm happy with.  I hope to post new (reorganized) patches that work sometime early next week.
(Assignee)

Updated

3 years ago
Depends on: 855443
(Assignee)

Comment 138

3 years ago
I refactored the patches a bit to segment based on the parts of gecko that got touched (functionality changes, probably maps pretty well to reviewers).  Here's a quick try run to catch major issues before I upload patches and start asking for feedback:

https://tbpl.mozilla.org/?tree=Try&rev=75d730ac21d9

There are a few more cases that need addressing with a first implementation of this feature.  If I catch any more I'll mention them when I upload the patches.

For example, on the try run there's an assertion caused by the tests for this bug (probably due to the number of iframes in use).  I confirmed the assertion happens even without the patches, and it's reported in bug 855443).  The fix is probably to split up the tests.

There's also a build failure on OSX that I'll work out before posting new patches.
(Assignee)

Comment 139

3 years ago
Created attachment 8487467 [details] [diff] [review]
0_meta-ref-base

This patch includes the foundational bits for <meta>-based referrer policies. It takes care of a few content loads (pings, link clicks, subdocuments, favicons), but not all (more patches for those).   There's one TODO in this document that is taken care of by the httpchannelparent patch.

mcmanus: I know you already looked at this, but I think I made a few minor changes to the necko bits and would like you to do a quick once-more-over (especially since it's been a while).
Attachment #8477730 - Attachment is obsolete: true
Attachment #8487467 - Flags: review?(mcmanus)
Attachment #8487467 - Flags: review?(bzbarsky)
(Assignee)

Comment 140

3 years ago
Comment on attachment 8416483 [details] [diff] [review]
metaref_tests

this test patch can be applied anywhere in the order of patches (or alone if you want to reproduce the assertions in comment 138).
Attachment #8416483 - Attachment description: 2_metaref_tests → metaref_tests
Attachment #8416483 - Attachment filename: 2_metaref_tests → metaref_tests
(Assignee)

Comment 141

3 years ago
Created attachment 8487470 [details] [diff] [review]
1_refpol_for_httpchannelparent

This is the e10s-ification patch for referrer policy stuff in necko.
Attachment #8420761 - Attachment is obsolete: true
Attachment #8487470 - Flags: review?(mcmanus)
I don't know when I'll be able to get to this.  If you want review here before the end of next week, you probably want a different reviewer....
(Assignee)

Comment 143

3 years ago
Created attachment 8487471 [details] [diff] [review]
2_refpol_for_scriptloads

Script loading (regular and speculative) referrer policy support.
Attachment #8415687 - Attachment is obsolete: true
(Assignee)

Comment 144

3 years ago
Created attachment 8487473 [details] [diff] [review]
3_refpol_for_styles

Stylesheet loader (speculative and final) referrer policy support.  This includes font loading.
(Assignee)

Comment 145

3 years ago
Created attachment 8487477 [details] [diff] [review]
4_refpol_for_images

Support for (speculative and final) referrer policies on image loads (HTML and CSS) and XUL and such.  Had to touch all the chrome loads too since all images seem to go through the same loader.
(Assignee)

Comment 146

3 years ago
Created attachment 8487481 [details] [diff] [review]
5_refpol_for_contentdom

Referrer policy support for XHR, EventSource, media elements, window.location, and window.open.  jst: I know you're probably chomping at the bit for this part of this feature, so do you want to review it?  Feel free to take on more reviews if you have time too!
Attachment #8487481 - Flags: review?(jst)
(Assignee)

Comment 147

3 years ago
Created attachment 8487484 [details] [diff] [review]
6_refpol_for_objects

This tiny little patch is for object loads.
Attachment #8487484 - Flags: review?(jst)
(Assignee)

Comment 148

3 years ago
Created attachment 8487486 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist

Support for nsIWebBrowserPersist storage and the implementation/tests it depends on.
(Assignee)

Comment 149

3 years ago
Created attachment 8487488 [details] [diff] [review]
8_nsDownloadManager
Attachment #8416519 - Attachment is obsolete: true
(Assignee)

Comment 150

3 years ago
Created attachment 8487490 [details] [diff] [review]
9_nsSyncLoadService and xslt

XSLT-created document referrer policies and the sync loader (it looks for referrer policies that may be available and loads with Default policy if none are present ... this plumbing will come in handy when/if we want client-specified referrer policies).  Peter, since you gave me the awesome advice before, would you please review some of this?
Attachment #8416521 - Attachment is obsolete: true
Attachment #8487490 - Flags: review?(peterv)
(Assignee)

Comment 151

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #142)
> I don't know when I'll be able to get to this.  If you want review here
> before the end of next week, you probably want a different reviewer....

I figured since you'd already seen it, you're the best for this patch.  Who would you recommend for help reviewing so I can avoid slamming your queue?

(Also, no rush, I'm out of the office for most of the next two weeks).
If this can wait order of a week or two, I can just do it myself.  ;)  You're right that it makes sense for me to, since I've already half-looked before.

Updated

3 years ago
Attachment #8487481 - Flags: review?(jst) → review+

Updated

3 years ago
Attachment #8487484 - Flags: review?(jst) → review+
(Assignee)

Updated

3 years ago
Attachment #8487471 - Flags: review?(jst)
Comment on attachment 8487471 [details] [diff] [review]
2_refpol_for_scriptloads

- In nsScriptLoader::ProcessScriptElement():

+
+    // Double-check that the referrer policy the preload used is the same as
+    // the referrer policy we have now.

We check more than the referrer policy here, so make this comment a bit more broad, i.e. something like "Check that what we preloaded matches what we're asked to load now".

...
       } else {
         // Drop the preload
         request = nullptr;
       }
+
     }

Remove the stray new empty line.

r=jst
Attachment #8487471 - Flags: review?(jst) → review+
Comment on attachment 8487467 [details] [diff] [review]
0_meta-ref-base

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

note to self dependent on 1_refpol_for_httpchannelparen for e10s parts
lgtm
Attachment #8487467 - Flags: review?(mcmanus) → review+
Attachment #8487470 - Flags: review?(mcmanus) → review+
Comment on attachment 8487490 [details] [diff] [review]
9_nsSyncLoadService and xslt

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

Is there a reason to use uint32_t instead of the enum? I'd prefer to use the latter if possible.

::: content/base/src/nsSyncLoadService.cpp
@@ +41,5 @@
>      NS_DECL_ISUPPORTS
>  
>      nsresult LoadDocument(nsIChannel* aChannel, nsIPrincipal *aLoaderPrincipal,
>                            bool aChannelIsSync, bool aForceToXML,
> +                          uint32_t referrerPolicy,

aReferrerPolicy

@@ +130,5 @@
>  nsSyncLoader::LoadDocument(nsIChannel* aChannel,
>                             nsIPrincipal *aLoaderPrincipal,
>                             bool aChannelIsSync,
>                             bool aForceToXML,
> +                           uint32_t referrerPolicy,

aReferrerPolicy

@@ +308,5 @@
>  /* static */
>  nsresult
>  nsSyncLoadService::LoadDocument(nsIURI *aURI, nsIPrincipal *aLoaderPrincipal,
>                                  nsILoadGroup *aLoadGroup, bool aForceToXML,
> +                                uint32_t referrerPolicy,

aReferrerPolicy

::: content/base/src/nsSyncLoadService.h
@@ +37,5 @@
>       * @param aResult [out] The document loaded from the URI.
>       */
>      static nsresult LoadDocument(nsIURI *aURI, nsIPrincipal *aLoaderPrincipal,
>                                   nsILoadGroup *aLoadGroup, bool aForceToXML,
> +                                 uint32_t referrerPolicy,

aReferrerPolicy

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ +373,5 @@
>      TX_DECL_ACOMPILEOBSERVER
>      NS_INLINE_DECL_REFCOUNTING(txCompileObserver)
>  
>      nsresult startLoad(nsIURI* aUri, txStylesheetCompiler* aCompiler,
> +                       nsIPrincipal* aSourcePrincipal, uint32_t referrerPolicy);

aReferrerPolicy

@@ +399,5 @@
>  
>  nsresult
>  txCompileObserver::loadURI(const nsAString& aUri,
>                             const nsAString& aReferrerUri,
> +                           uint32_t referrerPolicy,

aReferrerPolicy

@@ +454,5 @@
>  
>  nsresult
>  txCompileObserver::startLoad(nsIURI* aUri, txStylesheetCompiler* aCompiler,
> +                             nsIPrincipal* aReferrerPrincipal,
> +                             uint32_t referrerPolicy)

aReferrerPolicy

@@ +501,5 @@
>  
>  nsresult
>  TX_LoadSheet(nsIURI* aUri, txMozillaXSLTProcessor* aProcessor,
> +             nsILoadGroup* aLoadGroup, nsIPrincipal* aCallerPrincipal,
> +             uint32_t referrerPolicy)

aReferrerPolicy

@@ +630,5 @@
>  
>  nsresult
>  txSyncCompileObserver::loadURI(const nsAString& aUri,
>                                 const nsAString& aReferrerUri,
> +                               uint32_t referrerPolicy,

aReferrerPolicy

::: dom/xslt/xslt/txMozillaXMLOutput.cpp
@@ +778,5 @@
> +            if (!content.IsEmpty() && mDocument) {
> +                nsContentUtils::ASCIIToLower(content);
> +                mDocument->SetHeaderData(nsGkAtoms::referrer, content);
> +            }
> +        }

Given that HTMLMetaElement::BindToTree does this already you shouldn't actually need to do it here, HTMLMetaElement::BindToTree will be called as soon as the element is inserted in the result tree. Note that this doesn't remove whitespace and HTMLMetaElement::BindToTree doesn't seem to convert to lowercase.

::: dom/xslt/xslt/txStylesheetCompiler.cpp
@@ +26,5 @@
>  
>  using namespace mozilla;
>  
>  txStylesheetCompiler::txStylesheetCompiler(const nsAString& aStylesheetURI,
> +                                           uint32_t referrerPolicy,

aReferrerPolicy

@@ +36,5 @@
>  
>  txStylesheetCompiler::txStylesheetCompiler(const nsAString& aStylesheetURI,
>                                             txStylesheet* aStylesheet,
>                                             txListIterator* aInsertPosition,
> +                                           uint32_t referrerPolicy,

aReferrerPolicy

@@ +418,5 @@
>  
>  nsresult
>  txStylesheetCompiler::loadURI(const nsAString& aUri,
>                                const nsAString& aReferrerUri,
> +                              uint32_t referrerPolicy,

aReferrerPolicy

@@ +541,5 @@
>  }
>  
>  nsresult
>  txStylesheetCompilerState::init(const nsAString& aStylesheetURI,
> +                                uint32_t referrerPolicy,

aReferrerPolicy

::: dom/xslt/xslt/txStylesheetCompiler.h
@@ +49,5 @@
>      NS_IMETHOD_(MozExternalRefCountType) Release() = 0;
>  
>      virtual nsresult loadURI(const nsAString& aUri,
>                               const nsAString& aReferrerUri,
> +                             uint32_t referrerPolicy,

aReferrerPolicy

@@ +59,5 @@
>  };
>  
>  #define TX_DECL_ACOMPILEOBSERVER \
>    nsresult loadURI(const nsAString& aUri, const nsAString& aReferrerUri, \
> +                   uint32_t referrerPolicy, txStylesheetCompiler* aCompiler); \

aReferrerPolicy

@@ +70,5 @@
>  public:
>      explicit txStylesheetCompilerState(txACompileObserver* aObserver);
>      ~txStylesheetCompilerState();
>      
> +    nsresult init(const nsAString& aStylesheetURI, uint32_t referrerPolicy,

aReferrerPolicy

@@ +190,5 @@
>      friend class txStylesheetCompilerState;
>      friend bool TX_XSLTFunctionAvailable(nsIAtom* aName,
>                                             int32_t aNameSpaceID);
>      txStylesheetCompiler(const nsAString& aStylesheetURI,
> +                         uint32_t referrerPolicy,

aReferrerPolicy

@@ +195,5 @@
>                           txACompileObserver* aObserver);
>      txStylesheetCompiler(const nsAString& aStylesheetURI,
>                           txStylesheet* aStylesheet,
>                           txListIterator* aInsertPosition,
> +                         uint32_t referrerPolicy,

aReferrerPolicy
Attachment #8487490 - Flags: review?(peterv) → review+
(Assignee)

Comment 156

3 years ago
Comment on attachment 8487477 [details] [diff] [review]
4_refpol_for_images

Seth: would you be willing to take a look at these changes in the image loader to support referrer policies?
Attachment #8487477 - Flags: review?(seth)
(Assignee)

Comment 157

3 years ago
Comment on attachment 8487486 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist

jst: since you've looked at some of the other stuff, would you be ok taking this too?
Attachment #8487486 - Flags: review?(jst)
(Assignee)

Comment 158

3 years ago
Comment on attachment 8487473 [details] [diff] [review]
3_refpol_for_styles

heycam: I'm looking for a style system peer to help review these changes that enable referrer policies for style loads.  bz could probably review, but he's got a pretty healthy review queue going on ... any chance you could help out?  (Most of this patch is just threading the referrer policy through calls, the bulk is diff context and one-line changes).
Attachment #8487473 - Flags: review?(cam)
Comment on attachment 8487473 [details] [diff] [review]
3_refpol_for_styles

I think I'd rather defer to bz for that review; he'll have a better idea of whether this covers the entire set of style loading stuff, and whether the URIPrincipalAndCORSModeHashKey changes make sense.
Attachment #8487473 - Flags: review?(cam) → review?(bzbarsky)
Comment on attachment 8487486 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist

- In nsWebBrowser::SavePrivacyAwareURI():

+    mozilla::net::ReferrerPolicy refPol = mozilla::net::ReferrerPolicy_Default;
+    if (mDocShell)
+    {
+        nsCOMPtr<nsIDocument> doc = mDocShell->GetDocument();
+        if (doc)
+        {
+            refPol = doc->GetReferrerPolicy();
+        }
+    }
+    rv = mPersist->SavePrivacyAwareURI(uri, aCacheKey, aReferrer, refPol,
+                                       aPostData, aExtraHeaders, aFile, aIsPrivate);

The method where this code is was changed in this patch to take a referrer policy, so use it instead of digging one out of the document.

- In nsWebBrowserPersist::EnumPersistURIs():

-    rv = pthis->SaveURIInternal(uri, nullptr, nullptr, nullptr, nullptr, fileAsURI, true,
-                                pthis->mIsPrivate);
+    rv = pthis->SaveURIInternal(uri, nullptr, nullptr, mozilla::net::ReferrerPolicy_Default,
+                                nullptr, nullptr, fileAsURI, true, pthis->mIsPrivate);

This isn't right, but neither is the old code. If we were to change this we should do that in a followup unrelated to this bug. Maybe add a comment about the policy not mattering here since there is no referrer?

r- due to the first comment here.
Attachment #8487486 - Flags: review?(jst) → review-
Comment on attachment 8487467 [details] [diff] [review]
0_meta-ref-base

The commit comment here needs some grammar improvements.

> +++ b/content/base/src/nsContentSink.cpp	Wed Sep 10 13:56:20 2014 -0700

This part shouldn't be here at all, right?

>+++ b/content/html/content/src/HTMLMetaElement.cpp	Wed Sep 10 13:56:20 2014 -0700

CompressWhitespace is a slightly odd choice here, not least because it doesn't follow the spec.

What CompressWhitespace does is replace runs of ' ', '\r', '\n', and '\t' with spaces, dropping leading and trailing runs.  This is not the normal set of characters considered to be "whitespace" in HTML (which includes '\f' as well).

If you're implementing the draft at https://w3c.github.io/webappsec/specs/referrer-policy/ it says to do http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading-and-trailing-whitespace which explicitly refers to http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#space-character which is not what you're doing.

I suggest using nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace> here.  And adding a test that would detect the difference.

Of course if you're trying to implement https://w3c.github.io/webappsec/specs/referrer-policy/ there are all sorts of other issues here: you're not checking that your <meta> is a descendant of <head>, you're checking for emptiness _after_ stripping whitespace, not before, you're treating nonempty invalid values as "default" instead of "No Referrer", not implementing the set of values that draft actually defines...  I didn't check other details for now, since it's not clear to me what you're actually implementing here.

>+++ b/content/base/src/nsDocument.h	Wed Sep 10 13:56:20 2014 -0700
>+#include "ReferrerPolicy.h"

Already included via nsIDocument.h, right?

>+++ b/docshell/base/nsDocShellLoadInfo.cpp	Wed Sep 10 13:56:20 2014 -0700
>+   NS_ENSURE_ARG_POINTER(aReferrerPolicy);

No need.  It shouldn't be null.

>+++ b/docshell/shistory/public/nsISHEntry.idl	Wed Sep 10 13:56:20 2014 -0700

You need to fix the session restore code to save/restore this new attribute, right?  Otherwise restoring a closed tab, say, won't do the right thing.

>+++ b/docshell/shistory/src/nsSHEntry.cpp	Wed Sep 10 13:56:20 2014 -0700
>+  NS_ENSURE_ARG_POINTER(aReferrerPolicy);

Again, no need.

>+++ b/netwerk/base/public/ReferrerPolicy.h	Wed Sep 10 13:56:20 2014 -0700
>+  ReferrerPolicy_Never   = nsIHttpChannel::REFERRER_POLICY_NEVER,

Please file a followup bug on using that to replace the aSendReferrer flag of nsDocShell::DoURILoad, assuming it would be equivalent?

>+GetReferrerPolicyFromString(const nsAString& content, ReferrerPolicy& outPolicy)

I think using a pointer for the outparam instead would be clearer.

>+++ b/netwerk/protocol/http/HttpBaseChannel.cpp	Wed Sep 10 13:56:20 2014 -0700

>+#include "nsContentUtils.h"

Why is this needed?

>@@ -976,20 +999,25 @@ HttpBaseChannel::SetReferrer(nsIURI *ref

The changes here are a bit odd.  After these changes, in the ALWAYS/ORIGIN cases we are willing to send the referrer from https:// to http://, but subject to the gHttpHandler->SendSecureXSiteReferrer() condition?  That condition's name doesn't make it sound like it should affect the https:// to http:// case, but maybe that's ok and it just needs renaming?

>+++ b/netwerk/protocol/viewsource/nsViewSourceChannel.cpp	Wed Sep 10 13:56:20 2014 -0700
>+nsViewSourceChannel::SetReferrerWithPolicy(nsIURI * aReferrer,
>+                                           uint32_t referrerPolicy)

Please be consistent with the argument naming.

>+++ b/parser/html/nsHtml5SpeculativeLoad.h	Wed Sep 10 13:56:20 2014 -0700

Please reuse one of the existing string members instead of adding a new one.

>+++ b/parser/html/nsHtml5TreeBuilderCppSupplement.h	Wed Sep 10 13:56:20 2014 -0700

>+            referrerPolicy->CompressWhitespace();

This seems pretty clearly wrong, since it modifies the attribute value in-place.  So if you have <meta name="referrer" content="  something  "> the DOM will have .getAttribute("content") return "something" instead of "  something  ".

Please add a test for this and fix it (probably by doing the compressing inside InitMetaReferrerPolicy).

Also, see my comments about CompressWhitespace above.

>+            if (referrerPolicy) {

Um... you already called methods on it!  In any case, if the lowerCaseLiteralEqualsIgnoreAsciiCaseString call returned true, referrerPolicy is certainly not null.

>+++ b/parser/html/nsHtml5TreeOpExecutor.cpp	Wed Sep 10 13:56:20 2014 -0700
>+  if (NS_FAILED(rv)) {
>+    mSpeculationReferrerPolicy = mozilla::net::ReferrerPolicy_Default;

See comments on HTMLMetaElement above.

>+++ b/parser/html/nsHtml5TreeOpExecutor.h	Wed Sep 10 13:56:20 2014 -0700
>+    ReferrerPolicy   mSpeculationReferrerPolicy;

Don't you need to set this in the constructor?  Right now it seems like you'd get garbage values here if there is no <meta referrer>...  Please add a test?
Attachment #8487467 - Flags: review?(bzbarsky) → review-
Comment on attachment 8487467 [details] [diff] [review]
0_meta-ref-base

Oh, and shouldn't this new header file be exported into mozilla/net not toplevel, since it only defines things in the mozilla/net namespace?
Comment on attachment 8487473 [details] [diff] [review]
3_refpol_for_styles

>+++ b/content/base/public/nsIDocument.h	Tue Sep 09 13:21:48 2014 -0700
>+                            const ReferrerPolicy aReferrerPolicy) = 0;

The const doesn't really make sense, given it's passed by value.  Please fix the signature.

>+++ b/content/base/src/nsTreeSanitizer.cpp	Tue Sep 09 13:21:48 2014 -0700

Why would you not use aDocument's referrer policy here?

>+++ b/layout/style/CSSStyleSheet.h	Tue Sep 09 13:21:48 2014 -0700
>+  explicit CSSStyleSheet(CORSMode aCORSMode, ReferrerPolicy aReferrerPolicy);

Drop the explicit?

r=me, though it might be nice to document that the referrer policy of a sheet is used for its child sheets (which is the only place in this patch where you really use it).
Attachment #8487473 - Flags: review?(bzbarsky) → review+
Oh, and need tests, of course.
And by "need tests" I mean tests that test both the case when speculation succeeds and when it fails.
(Assignee)

Comment 166

3 years ago
Created attachment 8493359 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist
Attachment #8487486 - Attachment is obsolete: true
Attachment #8493359 - Flags: review?(jst)

Updated

3 years ago
Attachment #8493359 - Flags: review?(jst) → review+
(Assignee)

Comment 167

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #161)
> Comment on attachment 8487467 [details] [diff] [review]
> 0_meta-ref-base

Thanks for the review, and sorry it was kind of messy, bz.

> >+++ b/content/html/content/src/HTMLMetaElement.cpp	Wed Sep 10 13:56:20 2014 -0700
> 
> CompressWhitespace is a slightly odd choice here, not least because it
> doesn't follow the spec.

Indeed, it's completely the wrong thing to use here.

> If you're implementing the draft at
> https://w3c.github.io/webappsec/specs/referrer-policy/ it says to do
> http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading-
> and-trailing-whitespace which explicitly refers to
> http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#space-character
> which is not what you're doing.
> 
> I suggest using
> nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace> here.  And
> adding a test that would detect the difference.
> 
> Of course if you're trying to implement
> https://w3c.github.io/webappsec/specs/referrer-policy/ there are all sorts
> of other issues here: you're not checking that your <meta> is a descendant
> of <head>, you're checking for emptiness _after_ stripping whitespace, not
> before, you're treating nonempty invalid values as "default" instead of "No
> Referrer", not implementing the set of values that draft actually defines...
> I didn't check other details for now, since it's not clear to me what you're
> actually implementing here.

Apologies; it appears the draft has changed significantly since most of the stuff in patch 0_meta-ref-base was written.  I'll take a close look and bring the implementation into compliance with the one at w3c.github.io/webappsec/specs/referrer-policy before re-requesting review.  Sorry about that.
OK.  Note that the bit about being a descendant of <head> might be a pain to implement in the prescan.  If it is, it's worth pushing back against that part of the spec.
(In reply to Boris Zbarsky [:bz] from comment #168)
> OK.  Note that the bit about being a descendant of <head> might be a pain to
> implement in the prescan.  If it is, it's worth pushing back against that
> part of the spec.

FWIW, we need to implement a parent check for <img> that has a <picture> parent anyway. Good thing the prescan is actually the real speculation that runs the full tree construction algorithm...
> FWIW, we need to implement a parent check for <img> that has a <picture> parent anyway. 

Fair.  I'm just not sure I want to force Sid do that.  If he wants to, of course....

(Note that we'd need an ancestor check, not a parent check.)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #149)
> Created attachment 8487488 [details] [diff] [review]
> 8_nsDownloadManager

This download manager code is not used by Firefox any more, and is in the process of being decommissioned (see bug 851471). I don't think you need to patch this code.
(Assignee)

Comment 172

3 years ago
Thanks, Gavin.  

Paolo: I need a bit of advice about referrers in download manager(s).
* First, Gavin suggests the mimimum patch to nsDownloadManager to make it work (I will just hard-code a default referrer policy).  Would this be an ok approach since nsDownloadManager is going away?
* Second, I think I should do the work for jsdownloads in a follow-up bug.  I don't think referrers on downloads are a main use case for this meta referrer feature, but do want to fix them.  Does this sound reasonable to you?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 173

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #170)
> Fair.  I'm just not sure I want to force Sid do that.  If he wants to, of
> course....

Yeaaah.  I'm probably not the right person for that.  Let me look into what it takes to do the <head> check.

Comment 174

3 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #172)
> Paolo: I need a bit of advice about referrers in download manager(s).
> * First, Gavin suggests the mimimum patch to nsDownloadManager to make it
> work (I will just hard-code a default referrer policy).  Would this be an ok
> approach since nsDownloadManager is going away?

We don't run automated tests for nsDownloadManager.cpp on Desktop, and now that bug 901360 is well underway, we will not run them on Android either. The good news are that we'll probably complete the migration for Android in this release cycle, making any change unnecessary. You should file a separate bug for doing the proper nsDownloadManager.cpp changes, that can then be addressed by users of the legacy API as needed (Thunderbird and SeaMonkey come to my mind).

If you need to change something to avoid breaking the build, of course you can make the minimum required change in this bug.

> * Second, I think I should do the work for jsdownloads in a follow-up bug. 
> I don't think referrers on downloads are a main use case for this meta
> referrer feature, but do want to fix them.  Does this sound reasonable to
> you?

I guess that what we need to ensure here is that restarted downloads don't send a referrer that is different from the originally sent one. At the same time, the "open download page" command should continue to work. Both need to be addressed before we can call this feature complete.

We can do this by either storing the two pieces of information separately, or storing the referrer policy. The former seems more robust for backwards compatibility or add-ons that access the information directly. I'm fine with dealing with this in a separate bug.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 175

3 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #173)
> Let me look into what it takes to do the <head> check.

My quick attempt to do the <head> check did not meet success... I'm struggling a bit iwth nsIDOMHTMLHeadElement and massaging it into nsINode or something else useful.  This is basically what I was hoping to do inside HTMLMetaElement::BindToTree() when we bind a referrer meta tag:

    // Referrer Policy spec requires a <meta name="referrer" tag to be in the        
    // <head> element.
    nsCOMPtr<nsIDOMHTMLDocument> htmlDoc(do_QueryInterface(aDocument));              
    if (htmlDoc) {
      nsCOMPtr<nsIDOMHTMLHeadElement> headElt;
      htmlDoc->GetHead(getter_AddRefs(headElt));                                     
      
      nsCOMPtr<nsINode> headNode(do_QueryInterface(headElt));                        
      
      if (headElt && nsContentUtils::ContentIsDescendantOf(this, headNode)) {
        nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace>(content);   
        aDocument->SetHeaderData(nsGkAtoms::referrer, content);                      
      }                                                                              
    } 

But of course the conversion to nsCOMPtr<nsINode> doesn't work.  I'm gonna set this aside for a little while and work on simplifying the nsDownloadManager patch (thanks paolo), updating the patch for the new spec language, and making some tests for the speculative loads.
Status: NEW → ASSIGNED
> htmlDoc->GetHead(getter_AddRefs(headElt));                                     

So... the spec actually needs to define what it means here, since there might be multiple <head> elements and the document might not be an HTML document.

But I assume the sane thing would be to match Document.head, which you can do via:

  aDocument->GetHeadElement()

That said, is headElt returning null for you in this case?
(To be clear, there's a spec issue to be raised here.)
(Assignee)

Comment 178

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #176)
> > htmlDoc->GetHead(getter_AddRefs(headElt));                                     
>   aDocument->GetHeadElement()
> 
> That said, is headElt returning null for you in this case?

I'll try aDocument->GetHeadElement() again, but could not quickly figure out how to get Element into an nsIContent or nsINode for passing to ContentIsDescendantOf().  (I'm still trying to understand the various types here and how they relate).   Related errors from compiling this:

> error: no matching function for call to 'do_QueryInterface'
>       nsCOMPtr<nsIContent> headNode(do_QueryInterface(headElt));
>                                     ^~~~~~~~~~~~~~~~~
> error: static_cast from 'nsIDOMHTMLHeadElement *' to 'nsISupports *' is not allowed

(In reply to Boris Zbarsky [:bz] from comment #177)
> (To be clear, there's a spec issue to be raised here.)

Maybe I should join the working group to clear this up; looks like it may be a reasonable use of my time between this spec and CSP.
(Assignee)

Comment 179

3 years ago
Guess I was overthinking it.  This compiles and seems to work (tests attached to this bug pass with this change):

    Element* headElt = aDocument->GetHeadElement();
    if (headElt && nsContentUtils::ContentIsDescendantOf(this, headElt)) {
      nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace>(content);     
      aDocument->SetHeaderData(nsGkAtoms::referrer, content);                        
    }
(Assignee)

Updated

3 years ago
Blocks: 1073184
(Assignee)

Updated

3 years ago
Depends on: 1073187
(Assignee)

Comment 180

3 years ago
Comment on attachment 8487488 [details] [diff] [review]
8_nsDownloadManager

(In reply to :Paolo Amadini from comment #174)
> The good news are that we'll probably complete the migration for Android in
> this release cycle, making any change unnecessary. You should file a
> separate bug for doing the proper nsDownloadManager.cpp changes, that can
> then be addressed by users of the legacy API as needed (Thunderbird and
> SeaMonkey come to my mind).

Ok.  I'm just going to abandon the nsDownloadManager.cpp changes in this bug since if we don't make changes, the default referrer policy will be applied (build will not break).  I've filed bug 1073184 to address proper nsDownloadManager.cpp changes and I posted the patch there to keep this bug from being endless.

> I guess that what we need to ensure here is that restarted downloads don't
> send a referrer that is different from the originally sent one. At the same
> time, the "open download page" command should continue to work. Both need to
> be addressed before we can call this feature complete.
> I'm fine with dealing with this in a separate bug.

Great.  I filed bug 1073187 for this and will tackle it after this bug lands.
Attachment #8487488 - Attachment is obsolete: true
> but could not quickly figure out how to get Element into an nsIContent or nsINode

Element inherits from nsIContent, as you discovered.
(In reply to Boris Zbarsky [:bz] from comment #170)
> > FWIW, we need to implement a parent check for <img> that has a <picture> parent anyway. 
> 
> Fair.  I'm just not sure I want to force Sid do that.  If he wants to, of
> course....

I suppose I should write code that looks at the parser stack, but other than that, it seems appropriate to take care of the prescan from day 1.

> (Note that we'd need an ancestor check, not a parent check.)

Are you referring to scripting being off and there being <head><noscript><meta name='referrer'>? Is there another way for <meta name='referrer'> to be supposed to take effect and a parent check *in the parser* not being enough?

I assume that we'd want to ignore <meta name='referrer'> inside <template> inside <head>.
(In reply to Henri Sivonen (:hsivonen) from comment #182)
> I suppose I should write code that looks at the parser stack, but other than
> that, it seems appropriate to take care of the prescan from day 1.

Actually, it's pretty simple to copy
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilder.cpp#3727
to nsHtml5TreeBuilderCppSupplement.h and adapt as necessary to do the right check.

You could use 
if (findLastOrRoot(nsHtml5Atoms::head) {
  // has head ancestor
} else {
  // does not have head ancestor
}
but that doesn't deal with a <template> in between.

The preload code where you'd call this from lives in http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h#101
> Is there another way for <meta name='referrer'> to be supposed to take effect and a
> parent check *in the parser* not being enough?

Ah, good question.  I guess there are no containers that can really be kids of head and contain meta?  In that case just doing a parent check would be good enough, yes.
(Assignee)

Comment 185

3 years ago
Created attachment 8497881 [details] [diff] [review]
10_test_speculation

Here are some tests that check if preloads are reused when a referrer policy changes (or doesn't).  Is this kind of what you had in mind, bz?

I'm cleaning up the base implementation patch after addressing your comments, but want to write a few more tests (to check whitespace trimming and invalid policy behavior) before reflagging.  I also need to figure out why this implementation is failing in e10s (origin policy doesn't always work, but the others do).

While I'm updating the patch, I'm not sure I understand your request to reuse a string member in nsHTML5SpeculativeLoad.h... are you suggesting I concatenate the referrer policy to something like mCrossOrigin (and call it mCrossOriginAndReferrerPolicy)?  Given the way the various string members are used (mURL is the only one sent to all the preload functions [0]), this doesn't seem any simpler than a new string member.

[0] http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5SpeculativeLoad.cpp#24
Attachment #8497881 - Flags: review?(bzbarsky)
Flags: needinfo?(bzbarsky)
Comment on attachment 8497881 [details] [diff] [review]
10_test_speculation

>+        href="/tests/content/base/test/bug704320_counter.sjs?reset">

I don't think anything guarantees the server will get this request before the later ones in this document (e.g. if necko decides to prioritize the <script> over this <link>).

If you want to do this, you probably want to have an outer document that does this and onload load another document in a subframe that does the rest of the test.

>+    is(results[x].count, expected[x].count,
>+       "Expected " + expected[x].count + " loads for " + x + " requests.");

I'm not sure you can rely on the "full" requests happening.  That's sort of timing-dependent, I expect.

All you can really assume is that "full" should be somewhere in results[x].referrers.

>+      ok(results[x].referrers.contains(exRef),

Array.prototype.contains is nightly-only and about to be turned off again.  You want to use indexOf here for now.

Same in the other test.

r=me with those nits

> are you suggesting I concatenate the referrer policy to something like
> mCrossOrigin (and call it mCrossOriginAndReferrerPolicy)?

I was, yes.

The idea being that this struct would ideally be a union of several different sorts of structs, depending on what sort of preload we're actually doing, but in C++ you can't easily do unions of things that have constructors (yet).

> this doesn't seem any simpler than a new string member

It's not simpler, sure.  It just uses less memory...  I assume that's why Henri is repurposing some of the existing string members depending on the sort of preload we're doing.

I'm probably fine with a new member here as long as you run it by Henri.
Attachment #8497881 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 187

3 years ago
Created attachment 8498412 [details] [diff] [review]
10_test_speculation

Updated tests for speculation, incorporating bz's comments.

I condensed the test into one test_ file that loads two documents (with different referrer policy behaviors) in sequence.  The sequencing is done with a JS generator and I pulled out the reset into this sequence so it's guaranteed to happen at the right times.

Without doing a long setTimeout and crossing my fingers, I'm not sure how to test that "eventually" the preloads go unused and new loads happen.  As per bz's comments, I'm opportunistically checking the referrer of these non-preloads if they come in, but don't fail the tests if they take longer than the test.  At least running it locally I can ensure they're working (and if they do show up on tinderbox runs, the referrer is checked for the right policy).
Attachment #8497881 - Attachment is obsolete: true
Attachment #8498412 - Flags: review+
> I'm not sure how to test that "eventually" the preloads go unused and new loads happen.

The simplest way is to make the returned content depend on the referrer and then check that the page sees the right content returned.

For <img> that's a bit of a pain, but for <script> and <link rel="stylesheet"> it should be pretty easy.
(Assignee)

Comment 189

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #186)
> It's not simpler, sure.  It just uses less memory...  I assume that's why
> Henri is repurposing some of the existing string members depending on the
> sort of preload we're doing.
> 
> I'm probably fine with a new member here as long as you run it by Henri.

You know what, I just looked at the code again and I was wrong about my understanding of how it worked.  I thought mCrossOrigin was not sent to all the various preloaders (script/image/style).  Since it actually is sent to them all, there's no reason for me to not extend mCrossOrigin to include the referrer policy. Sorry for the mixup.
Um, no.  You want to put the referrer policy in some string that's not already otherwise occupied...  I see we misunderstood each other above.  I wasn't suggesting you stick both the crossorigin value and the referrer policy into a single string at the same time!

If every single thing needs the referrer policy, then just adding a new string for it is what we presumably need, since all the other strings will be used by something else.
(Assignee)

Comment 191

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #190)
> Um, no.  You want to put the referrer policy in some string that's not
> already otherwise occupied...  I see we misunderstood each other above.  I
> wasn't suggesting you stick both the crossorigin value and the referrer
> policy into a single string at the same time!

(this is probably why I was a bit confused, it sounds uncomfortable to do that)

> If every single thing needs the referrer policy, then just adding a new
> string for it is what we presumably need, since all the other strings will
> be used by something else.

Yeah, script preloads (for example) already use every existing string member:

34       aExecutor->PreloadScript(mUrl, mCharset, mTypeOrCharsetSource,
35                                mCrossOrigin, false);

So I think I have to add a new string.  

(In reply to Boris Zbarsky [:bz] from comment #188)
> The simplest way is to make the returned content depend on the referrer and
> then check that the page sees the right content returned.

How do I know _when_ to check the returned content?  Does querying the elements cause the script to block until they're fully loaded?  I had thought about putting an onload handler on each of the three elements that can be used to gate the "okay, what requests happened" check, but couldn't figure out how to get one working for the link tag (style check).
> How do I know _when_ to check the returned content?

Page's onload or load events on the elements.

> but couldn't figure out how to get one working for the link tag

Should work.  Either as onload on the element or addEventListener("load").  Simple testcase:

  data:text/html,<link rel="stylesheet" onload="alert(5)" href="data:text/css,xyz">
(Assignee)

Comment 193

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #192)
> > How do I know _when_ to check the returned content?
> 
> Page's onload or load events on the elements.

This is what I thought.  I tried a load handler on the page, but sometimes on slower machines the load event seems to fire before the final content is requested (but seems the preloads all happen first).  See for example: 
orange - https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2127105&repo=try
relevant patch - https://hg.mozilla.org/try/rev/85aaf37397ae

This test worked fine for me locally in linux64 debug.

This race is also how I interpreted your note in comment 186.

(In reply to Boris Zbarsky [:bz] from comment #186)
> >+    is(results[x].count, expected[x].count,
> >+       "Expected " + expected[x].count + " loads for " + x + " requests.");
> 
> I'm not sure you can rely on the "full" requests happening.  That's sort of
> timing-dependent, I expect.

I'll give the onload handlers another try, but the orange on try kind of worries me.
(Assignee)

Comment 194

3 years ago
Created attachment 8498472 [details] [diff] [review]
10_test_speculation

Ack, forgot a qrefresh.  Keeping r=bz.
Attachment #8498412 - Attachment is obsolete: true
Attachment #8498472 - Flags: review+
> This race is also how I interpreted your note in comment 186.

Erm, no.  You're seeing the opposite: you see a load for img with the full referrer, but not the origin referrer.

My guess is that this has more to do with the fact that the above patches don't make the referrer policy part of the image cache key, unless I'm missing something.
(Assignee)

Comment 196

3 years ago
Ok, as discussed in email with bz, I updated the refpol_for_images patch to include the referrer policy in the cache key.  

I also updated the tests to *require* non-prefetch loads in the case where the preloads can't be reused (to address my misunderstanding from comment 186).  I got onload to work for the three tags, but I had to complicate the test since the img tag was firing onerror instead of onload with an empty response body.  The test seems to work locally, and I'm checking on try.  If that's all good I'll post an update to 10_test_speculation.

I still need to address a couple of review comments and add tests for whitespace and invalid policies, but then I'll post a new set of patches.  Most haven't changed substantially except for some bitrot, and I'll keep the r+es on those.  For others, I'll try to list things that changed and ask for review again.  Mostly it's the base patch and the images patch that will be updated.
(Assignee)

Comment 197

3 years ago
Created attachment 8504982 [details] [diff] [review]
10_test_speculation

This updated patch that I've been sitting on for over a week addresses all of bz's comments on the speculative tests, carrying over r=bz.
Attachment #8498472 - Attachment is obsolete: true
Attachment #8504982 - Flags: review+
(Assignee)

Comment 198

3 years ago
Created attachment 8504984 [details] [diff] [review]
11_test_policyparser

As bz requested, here are some additional tests (patch applied on top of 10_test_speculation) to check for whitespace trimming and invalid policy behavior.
Attachment #8504984 - Flags: review?(bzbarsky)
(Assignee)

Comment 199

3 years ago
I spent a bit of time updating the patches in this bug for bitrot (some churn in layout), but am still debugging the e10s failure (only the "origin" policy fails on e10s, everything else seems to work fine).  Hopefully I can get that figured out soon and then will upload a new set of patches that apply cleanly and work properly.
Comment on attachment 8504984 [details] [diff] [review]
11_test_policyparser

>+          ok(false, "Need to be able to reset the request counter");
...
>+          ok(false, "Can't get results from the counter server.");

Don't both of those need SimpleTest.finish() to avoid hanging the test?

Past that, this is not testing most of the edge cases I was talking about in comment 161.  Specifically at least:

1) Cases like <meta name="referrer" content="   ">
2) Cases in which the content= value contains leading/trailing \f.

r=me with that fixed
Attachment #8504984 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 201

3 years ago
Created attachment 8505672 [details] [diff] [review]
11_test_policyparser

Added a bunch of test cases for whitespace and leading/trailing whitespace chars as requested.  Also, good catch on needing SimpleTest.finish() in the fail cases.  Added those too.  Carrying over r=bz.
Attachment #8504984 - Attachment is obsolete: true
Attachment #8505672 - Flags: review+
(Assignee)

Comment 202

3 years ago
Created attachment 8505697 [details] [diff] [review]
0_meta-ref-base

Okay, I believe I addressed all of bz's comments.

mcmanus: I'm flagging you again because I changed some stuff in HttpBaseChannel: 
* fixed the e10s issue by using SetRequestHeader instead of mRequestHead.SetHeader -- thanks jduell!
*  renamed the constants and policies to line up with the new specification language
* changed some logic in SetReferrerWithPolicy.

bz, these are some of the big changes from the last patch:
* Updated session restore to save the referrer policy
* Verified spec compliance, changed policy tokens and such and added relevant commenting
* Fixed the logic so the unsafe cross-protocol referrer policies and SendSecureXSiteReferrer work properly together
* Enforce header-as-a-parent for meta tags
* fixed whitespace trimming

I'll follow up with updates for all the other patches (bitrot, yay) and a link to a try run.
Attachment #8487467 - Attachment is obsolete: true
Attachment #8505697 - Flags: review?(mcmanus)
Attachment #8505697 - Flags: review?(bzbarsky)
(Assignee)

Comment 203

3 years ago
Created attachment 8505699 [details] [diff] [review]
1_refpol_for_httpchannelparent

update for bitrot, carrying over r=mcmanus.
Attachment #8487470 - Attachment is obsolete: true
Attachment #8505699 - Flags: review+
(Assignee)

Comment 204

3 years ago
Created attachment 8505700 [details] [diff] [review]
2_refpol_for_scriptloads

bitrot.  Carrying over r=jst.
Attachment #8505700 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8487471 - Attachment is obsolete: true
(Assignee)

Comment 205

3 years ago
Created attachment 8505714 [details] [diff] [review]
3_refpol_for_styles

Addressed bz's comments and bitrot.  Carrying over r=bz.
Attachment #8487473 - Attachment is obsolete: true
Attachment #8505714 - Flags: review+
(Assignee)

Comment 206

3 years ago
Created attachment 8505715 [details] [diff] [review]
4_refpol_for_images

Updated patch for bitrot and changes in the other patches.
Attachment #8487477 - Attachment is obsolete: true
Attachment #8487477 - Flags: review?(seth)
Attachment #8505715 - Flags: review?(seth)
(Assignee)

Comment 207

3 years ago
Created attachment 8505716 [details] [diff] [review]
5_refpol_for_contentdom

updated for bitrot, carrying over r=jst.
Attachment #8487481 - Attachment is obsolete: true
Attachment #8505716 - Flags: review+
(Assignee)

Comment 208

3 years ago
Created attachment 8505717 [details] [diff] [review]
6_refpol_for_objects
Attachment #8487484 - Attachment is obsolete: true
Attachment #8505717 - Flags: review+
(Assignee)

Comment 209

3 years ago
Created attachment 8505720 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist

update for bitrot, carrying over r=jst
Attachment #8493359 - Attachment is obsolete: true
Attachment #8505720 - Flags: review+
(Assignee)

Comment 210

3 years ago
Created attachment 8505721 [details] [diff] [review]
9_nsSyncLoadService

updated for bitrot, carrying over r=peterv.
Attachment #8487490 - Attachment is obsolete: true
Attachment #8505721 - Flags: review+
Sid, any chance of an interdiff?
Flags: needinfo?(sstamm)
(Assignee)

Comment 212

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #211)
> Sid, any chance of an interdiff?

I'll see what I can do.  I also need to update the metaref_tests to use the new spec syntax (semantically the same, just new tokens to support).

Try:
https://tbpl.mozilla.org/?tree=Try&rev=4e6069ca40bd
Flags: needinfo?(sstamm)
(Assignee)

Comment 213

3 years ago
Created attachment 8505744 [details] [diff] [review]
0_meta-ref-base interdiff

I think the interdiff worked here (there was a bunch of bitrot in the old patch, not sure if I got it all perfectly).
Comment on attachment 8505697 [details] [diff] [review]
0_meta-ref-base

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

I only looked at basechannel for the interdiffs.. r+
Attachment #8505697 - Flags: review?(mcmanus) → review+
Comment on attachment 8505697 [details] [diff] [review]
0_meta-ref-base

Thanks for the interdiff.

So per spec, <meta name="referrer"> whose content is empty string is ignored.  But what this patch does is trim whitespace and then pass to SetHeaderData, which will ignore empty string.

That won't match the spec for <meta name="referrer" content="  ">.  Per that spec, this case should be treated as "No Referrer" afaict.  If we're not planning to do that and have good reasons not to, please raise a spec bug?  But your code in GetReferrerPolicyFromString makes me think you were in fact trying to implement this....

>+  if (content.LowerCaseEqualsLiteral("never") ||

Per spec, this and the other comparisons in GetReferrerPolicyFromString should be case-sensitive comparisons, afaict.  That's presumably a bug in the spec that you should raise.

>+  rv = ssm->CheckSameOriginURI(referrer, mURI, false);

That doesn't look right.  The referrer may not match the request origin (e.g. in document.domain cases).

Please talk to Tanvi about what the right check here is.

>+  rv = SetRequestHeader(NS_LITERAL_CSTRING("Referer"), spec, false);

You're setting mReferrer before this line, but mReferrerPolicy after.  Since you can early-return in between, that seems worrisome.  Please either set both or neither in the case when SetRequestHeader fails.

>+            nsString* referrerPolicy = aAttributes->getValue(nsHtml5AttributeName::ATTR_CONTENT);

I was wrong: this does need to be null-checked.  Because the lowerCaseLiteralEqualsIgnoreAsciiCaseString was on "content", not "name".

This doesn't seem to implement the "two <meta> elements with different values means no referrer" behavior in the preloader...

GetReferrerPolicyFromString never returns anything other than NS_OK, right?  Can we just change it to returning the policy?

r=me with the above fixed.
Attachment #8505697 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 216

3 years ago
Thanks, bz.  Sent mail about the spec issues and will follow up accordingly.  I expect we'll end up with case-insensitive policy tokens and treating a content attribute value of "" and "    " the same way.

http://lists.w3.org/Archives/Public/public-webappsec/2014Oct/0047.html

seth: I flagged you for a review on image loading about a month ago (comment 156).  Are you willing to review this patch, or should I find someone else?
Flags: needinfo?(seth)
Comment on attachment 8505715 [details] [diff] [review]
4_refpol_for_images

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

Apologies for the slow review!

r+ from me with the below addressed.

::: image/src/imgLoader.cpp
@@ +631,5 @@
>                                  bool *aForcePrincipalCheckForCacheEntry,
>                                  nsIURI *aURI,
>                                  nsIURI *aInitialDocumentURI,
>                                  nsIURI *aReferringURI,
> +                                mozilla::net::ReferrerPolicy aReferrerPolicy,

Please add |using mozilla::net;| at the top of the file, with the other using statements, and get rid of |mozilla::net::| in the rest of the patch.

@@ +1616,5 @@
>    if (!request)
>      return false;
>  
> +  // make sure the referrer policy is the same as the one in cache
> +  // TODO(sstamm): Move this into ValidateCORSAndPrincipal

Looks like you still need to do this, right?

It's probably just because I'm not away of all the details, but it's not clear to me why the referrer policy needs to be the same as the one in the cache. Why do we care if we never send a network request?

@@ +1869,5 @@
>                                     imgIRequest **_retval)
>  {
>      imgRequestProxy *proxy;
> +    ReferrerPolicy refpol = mozilla::net::RP_No_Referrer_When_Downgrade;
> +    nsresult result = mozilla::net::GetReferrerPolicyFromString(aReferrerPolicy,

Our style guide requires that this variable be called |rv| and not |result|.

::: image/src/imgLoader.h
@@ +475,5 @@
>                            public nsIInterfaceRequestor,
>                            public nsIAsyncVerifyRedirectCallback
>  {
>  public:
> +  typedef mozilla::net::ReferrerPolicy ReferrerPolicy;

It looks like you don't need to add this typedef since you never refer to the |ReferrerPolicy| type in the declaration of |imgCacheValidator|.

::: image/src/imgRequest.h
@@ +255,5 @@
>    // default, imgIRequest::CORS_NONE.
>    int32_t mCORSMode;
>  
> +  // The Referrer Policy (defined in ReferrerPolicy.h) this image was loaded
> +  // with.  By default, mozilla::net::RP_No_Referrer_When_Downgrade.

Nit: should we refer to this as the |Referrer Policy| or |Referrer policy| in comments? We should make the different comments in this file consistent.

Another nit: I don't think it's worth documenting the default here, because you must pass a |ReferrerPolicy| value to |imgRequest::Init| - in other words, nobody ever relies on the default value.

::: image/test/unit/test_private_channel.js
@@ +76,5 @@
>    var uri = gIoService.newURI(gImgPath, null, null);
>    var loadGroup = Cc["@mozilla.org/network/load-group;1"].createInstance(Ci.nsILoadGroup);
>    loadGroup.notificationCallbacks = new NotificationCallbacks(isPrivate);
>    var loader = isPrivate ? gPrivateLoader : gPublicLoader;
> +  requests.push(loader.loadImageXPCOM(uri, null, null, "default", null, loadGroup, outer, null, 0, null, null));

I assume one of the other patches contains new tests that test the referrer policy functionality for images?

::: widget/cocoa/nsMenuItemIconX.mm
@@ +306,5 @@
>    }
>  
>    // Passing in null for channelPolicy here since nsMenuItemIconX::LoadIcon is
>    // not exposed to web content
> +  nsresult rv = loader->LoadImage(aIconURI, nullptr, nullptr, 

There's extra whitespace at the end of this line.

@@ +307,5 @@
>  
>    // Passing in null for channelPolicy here since nsMenuItemIconX::LoadIcon is
>    // not exposed to web content
> +  nsresult rv = loader->LoadImage(aIconURI, nullptr, nullptr, 
> +                                  mozilla::net::RP_No_Referrer_When_Downgrade,

I notice that we're just specifying |RP_No_Referrer_When_Downgrade| everywhere. A suggestion: you could add a default value (|RP_Default|?) that mapped to |RP_No_Referrer_When_Downgrade|. That would both be less verbose and would express more clearly that the person using that value is accepting the default rather than going out of their way to choose a specific policy.
Attachment #8505715 - Flags: review?(seth) → review+
Flags: needinfo?(seth)
(In reply to Seth Fowler [:seth] from comment #217)
> It's probably just because I'm not away of all the details, but it's not

I meant "not aware" here, sorry.
(Assignee)

Comment 219

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #215)
> >+  rv = ssm->CheckSameOriginURI(referrer, mURI, false);
> 
> That doesn't look right.  The referrer may not match the request origin
> (e.g. in document.domain cases).
> 
> Please talk to Tanvi about what the right check here is.

Tanvi and I just spoke, and we are both uncertain what is not right.  "referrer" is passed to HttpBaseChannel::SetReferrerWithPolicy(), where this call to CheckSameOriginURI is added.  mURI is the URI of the http channel being created (as a subresource, navigation, etc).  So I'm not sure what could be changed by use of document.domain... wouldn't that just affect the arguments passed into this SetReferrer() function?
Changing document.domain does not change referrer, but can allow some interesting scenarios. 

Concretely, say you have two pages: page A at http://a.foo.com and page B at http://b.foo.com.  Now if A triggers a location set on a subframe on B, say to http://b.foo.com/subframe, the referrer will be http://a.foo.com.  The question is whether this is a cross-origin request or not.  What is the request origin in this situation per the spec?

But more generally, per the curent spec text what matters here is whether the _request_ is cross-origin, which is a property of the request itself, not of the referrer being set.  So doing a same-origin check on the referrer URI is at best accidentally observably equivalent but really confusing and at worst just wrong.
(Assignee)

Comment 221

3 years ago
I spent a bunch of time last week rebasing the patches and incorporating Seth's feedback (thanks, Seth!).  I hope to have a new set of patches up sometime this week for review once I finish up with bz's feedback and this cross origin check.  Sorry for dragging my feet on this -- some other stuff has been consuming most of my time.

(In reply to Boris Zbarsky [:bz] from comment #220)
> But more generally, per the curent spec text what matters here is whether
> the _request_ is cross-origin, which is a property of the request itself,
> not of the referrer being set.  

Ah, it's clear to me now.  The "Referer" string may not equal the origin of the requesting context.  

Tanvi: does this help?  Is there some better source for the requesting context's origin that is available in this SetReferrerWithPolicy() function?
Flags: needinfo?(tanvi)
(Assignee)

Comment 222

3 years ago
Bug 946065 is gonna slow me down on this a bit.
(Assignee)

Comment 223

3 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #221)
> Tanvi: does this help?  Is there some better source for the requesting
> context's origin that is available in this SetReferrerWithPolicy() function?

I spoke with Chris about this, and I think the right idea is to use the channel's mLoadInfo->loadingPrincipal.
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadInfo.idl#66

It's very possible not every HttpBaseChannel will have mLoadInfo, in which case all I have to fall back on is the referrer URI.  My thinking is that we should try to use mLoadInfo->loadingPrincipal for the same origin check and fall back to the referrer URI if mLoadInfo is not available (I can't find any other loading context that's useful here).    If I understand correctly, we're moving all the channels to have an nsILoadInfo so when the same-origin-checking code falls back I'll post a warning in debug builds that we can turn into an assertion once bug 1087720 is fixed (and thus all channels should have mLoadInfo set prior to calls to SetReferrerWithPolicy).

Chris, does this match up with your suggestion?  (Tanvi, I'd like to hear what you think here too.)
Flags: needinfo?(mozilla)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #223)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #221)
> > Tanvi: does this help?  Is there some better source for the requesting
> > context's origin that is available in this SetReferrerWithPolicy() function?
> 
> I spoke with Chris about this, and I think the right idea is to use the
> channel's mLoadInfo->loadingPrincipal.
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadInfo.
> idl#66
> 
> It's very possible not every HttpBaseChannel will have mLoadInfo, in which
> case all I have to fall back on is the referrer URI.

At the moment, every channel created through NS_NewChannel (and variations)
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#201
will have a loadInfo attached.

> My thinking is that we
> should try to use mLoadInfo->loadingPrincipal for the same origin check and
> fall back to the referrer URI if mLoadInfo is not available (I can't find
> any other loading context that's useful here).    If I understand correctly,
> we're moving all the channels to have an nsILoadInfo so when the
> same-origin-checking code falls back I'll post a warning in debug builds
> that we can turn into an assertion once bug 1087720 is fixed (and thus all
> channels should have mLoadInfo set prior to calls to SetReferrerWithPolicy).

Agreed, once Bug 1087720 (which sets loadInfo on all JS created channels) lands all the channels in our codebase should have a loadInfo attached. Keep in mind that addon created channels most likely do not have a loadInfo attached to their channels. Not sure if that is relevant for this patch though.

> Chris, does this match up with your suggestion?  (Tanvi, I'd like to hear
> what you think here too.)

That aligns with my thinking. Another thing you should be aware of is that the loadInfo is going to be extended by a triggeringPrincipal (mLoadInfo->triggeringPrincipal), see Bug 1083422. Long story short, imagine a page that includes a css which includes an image. In that case the loadingPrincipal of the image would be the page (document) and the triggeringPrincipal would be the css.
Flags: needinfo?(mozilla)
(Assignee)

Updated

3 years ago
Attachment #8411927 - Attachment is obsolete: true
(Assignee)

Comment 225

3 years ago
Created attachment 8514434 [details] [diff] [review]
metaref_tests

These test the basic behavior of meta referrer policies.  I updated them to use the new spec tokens ("no-referrer" instead of "never", etc).  They still time out on e10s and b2g desktop, and I'm trying to sort through the failures to figure out what's going on.

Holding off on a review request until I figure out the failures.
Attachment #8416483 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #224)
> That aligns with my thinking. Another thing you should be aware of is that
> the loadInfo is going to be extended by a triggeringPrincipal
> (mLoadInfo->triggeringPrincipal), see Bug 1083422. Long story short, imagine
> a page that includes a css which includes an image. In that case the
> loadingPrincipal of the image would be the page (document) and the
> triggeringPrincipal would be the css.

It's unclear whether you need to check against the loadingPrincipal or the triggeringPrincipal.  The loadingPrincipal will be the NodePrincipal of the document in which the subresource is loading.  But a triggeringPrincipal could be the stylesheet that prompted the font (for example) subresource load.  Which one of these should be your referrer?  The document's origin or the stylesheets origin?  We can talk more in our upcoming meeting.

If the triggeringPrincipal, the triggeringPrincipal will not always be set.  So your fallback case won't look pretty.  
if (loadInfo->triggeringPrincipal) {
  loadingURI = loadInfo->triggeringPrincipal;
} else if (loadInfo->loadingPrincipal) {
  loadingURI = loadInfo->loadingPrincipal; 
} else {
  loadingURI = referrer;
Flags: needinfo?(tanvi)
(Assignee)

Comment 227

3 years ago
Okay, since nsILoadInfo::triggeringPrincipal does not yet exist, I'm going to use the loadingPrincipal and I filed bug 1091883 to investigate whether we should be using triggeringPrincipal here (given bz's scenario in comment 220) and if so, change from loadingPrincipal to triggeringPrincipal once it's available.
(Assignee)

Updated

3 years ago
Blocks: 1091883
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #227)
> Okay, since nsILoadInfo::triggeringPrincipal does not yet exist, I'm going
> to use the loadingPrincipal and I filed bug 1091883 to investigate whether
> we should be using triggeringPrincipal here (given bz's scenario in comment
> 220) and if so, change from loadingPrincipal to triggeringPrincipal once
> it's available.

This is a bit confusing, but using loadingPrincipal already encompasses using triggeringPrincipal in mozilla-central code.  If a triggeringPrincipal exists, loadingPrincipal is set to triggeringPrincipal.  If it doesn't exist loadingPrincipal is set to the document's principal.

With bug 1083422, we split this up.  With the code in that bug, if a triggeringPrincipal exists, triggeringPrincipal is set otherwise it is null.  And loadingPrincipal will always be the document's principal.

For this bug, use loadingPrincipal.  Then when bug 1083422 lands, we can quickly add a patch to bug 1091883 with a few lines like this:
if (loadInfo->triggeringPrincipal) {
  loadingURI = loadInfo->triggeringPrincipal;
} else if (loadInfo->loadingPrincipal) {
  loadingURI = loadInfo->loadingPrincipal; 
}

In bug 1091883, we can further followup to ensure we are getting the referrer we think we should be getting for the example Boris gave in comment 220.
> and fall back to the referrer URI if mLoadInfo is not available 

Please, just don't go there.

At this point, anything you actually care about should have a loadinfo.  If there isn't any for some reason, assume the most restrictive thing if that won't break the web.  In this case, assume cross-origin.  I'm pretty sure doing that here won't break the web.
(Assignee)

Comment 230

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #229)
> > and fall back to the referrer URI if mLoadInfo is not available 
> 
> Please, just don't go there.

You got it.  I'll just assume the load is cross-origin if there's no mLoadInfo or if mLoadInfo->loadingPrincipal is not set.
(Assignee)

Comment 231

3 years ago
Created attachment 8515258 [details] [diff] [review]
0_meta-ref-base (r=bz,mcmanus)
Attachment #8505697 - Attachment is obsolete: true
Attachment #8515258 - Flags: review+
(Assignee)

Comment 232

3 years ago
Created attachment 8515259 [details] [diff] [review]
1_refpol_for_httpchannelparent (r=mcmanus)
Attachment #8505699 - Attachment is obsolete: true
Attachment #8515259 - Flags: review+
(Assignee)

Comment 233

3 years ago
Created attachment 8515261 [details] [diff] [review]
2_refpol_for_scriptloads (r=jst)
Attachment #8505700 - Attachment is obsolete: true
Attachment #8515261 - Flags: review+
(Assignee)

Comment 234

3 years ago
Created attachment 8515262 [details] [diff] [review]
3_refpol_for_styles (r=bz)
Attachment #8505714 - Attachment is obsolete: true
Attachment #8515262 - Flags: review+
(Assignee)

Comment 235

3 years ago
Created attachment 8515264 [details] [diff] [review]
4_refpol_for_images (r=seth)
Attachment #8505715 - Attachment is obsolete: true
Attachment #8515264 - Flags: review+
(Assignee)

Comment 236

3 years ago
Created attachment 8515265 [details] [diff] [review]
5_refpol_for_contentdom (r=jst)
Attachment #8505716 - Attachment is obsolete: true
Attachment #8515265 - Flags: review+
(Assignee)

Comment 237

3 years ago
Created attachment 8515266 [details] [diff] [review]
6_refpol_for_objects (r=jst)
Attachment #8505717 - Attachment is obsolete: true
Attachment #8515266 - Flags: review+
(Assignee)

Comment 238

3 years ago
Created attachment 8515267 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist (r=jst)
Attachment #8505720 - Attachment is obsolete: true
Attachment #8515267 - Flags: review+
(Assignee)

Comment 239

3 years ago
Created attachment 8515268 [details] [diff] [review]
9_nsSyncLoadService (r=peterv)
Attachment #8505721 - Attachment is obsolete: true
Attachment #8515268 - Flags: review+
(Assignee)

Comment 240

3 years ago
Created attachment 8515270 [details] [diff] [review]
10_test_speculation (r=bz)
Attachment #8504982 - Attachment is obsolete: true
Attachment #8515270 - Flags: review+
(Assignee)

Comment 241

3 years ago
Created attachment 8515271 [details] [diff] [review]
11_test_policyparser (r=bz)
Attachment #8505672 - Attachment is obsolete: true
Attachment #8515271 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8505744 - Attachment is obsolete: true
(Assignee)

Comment 242

3 years ago
Alright.  Finally, here's an updated set of patches (updated for bitrot due to various csets and the big move from content/base to dom/base).

Things left to do:
* Figure out the e10s and other platform timeouts of the metaref_tests (before flagging for review)
* Add a default value to the policy types enum to make it clearer (as per seth's suggestions)

I think everything else is addressed in this set of patches.
(Assignee)

Comment 243

3 years ago
billm: in IRC you said you'd help me try and figure out why the tests are timing out.  Here's a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ceb428729349

... including a few relevant e10s logs for the timeout.  When I run the tests locally, they timeout regardless of whether I have other patches applied or not.  You can grab the tests via attachment 8514434 [details] [diff] [review] if you want to run them locally.
Flags: needinfo?(wmccloskey)
Talked to Sid about this. It sounds like there might be an issue with HTTPS-to-* redirects.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 245

3 years ago
And I spoke with jduell about this, who helped me verify that necko is doing all the right stuff in e10s with the https request/response.  Turns out that we're hitting bug 820466; the assertions come out of the parts of the metaref_tests that use https iframes.  

If we want to run these tests in e10s, we need to move the nsSecureBrowserUIImpl stuff into the parent.  My suscpicion is that many of the other try-run-oranges are due to this same issue.
Depends on: 820466
(Assignee)

Updated

3 years ago
Attachment #8514434 - Flags: review?(bzbarsky)
(Assignee)

Comment 246

3 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #245)
> Turns out that
> we're hitting bug 820466; the assertions come out of the parts of the
> metaref_tests that use https iframes.  

But this isn't causing the test failures.  The tests are failing on e10s because of some mixed-content-redirect issue.  Some of the tests are https iframes that 302 redirect to an http url, and even though the tests set prefs to disable mixed content blocking, on e10s the target http page is not displayed (and thus the https tests don't execute and finish).

Talking with Tanvi about this in a little bit, but locally I saw the e10s tests to pass and complete by changing the target of the redirect from an http to https URL.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #246)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #245)
> > Turns out that
> > we're hitting bug 820466; the assertions come out of the parts of the
> > metaref_tests that use https iframes.  
> 
> But this isn't causing the test failures.  The tests are failing on e10s
> because of some mixed-content-redirect issue.  Some of the tests are https
> iframes that 302 redirect to an http url, and even though the tests set
> prefs to disable mixed content blocking, on e10s the target http page is not
> displayed (and thus the https tests don't execute and finish).

Just talked to Sid about this, this is bug 1084504.  Changing to https (and hence removing the mixed content) should fix the mochitest for e10s.
See Also: → bug 1084504
(Assignee)

Comment 248

3 years ago
And apparently window.open doesn't work in b2g, so that explains why the tests are failing on b2g (they heavily use window.open).  For now I'm planning to disable the tests that use window.open on b2g (one-line fix in metaref_tests patch, dom/base/test/mochitest.ini).

So changing http->https should fix the timeout on e10s, disabling on b2g will fix the timeout on b2g-desktop, and I'll add SimpleTest.expectAssertions(0,15) in the test to catch the various number of assertions (bug 820466 and this bug comment 245).  When we fix bug 820466 we can remove the expectAssertions from this test.

bz: since I've got you flagged for review on metaref_tests, I won't upload a new version of that patch with these changes in case you've started review (unless you'd prefer to see the new version, just let me know).  I'll update all the rest of the patches that changed when I added the default value to the enum as per seth's suggestion.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b7694943219
(Assignee)

Comment 249

3 years ago
Created attachment 8517084 [details] [diff] [review]
2_refpol_for_scriptloads (r=jst)
Attachment #8515261 - Attachment is obsolete: true
Attachment #8517084 - Flags: review+
(Assignee)

Comment 250

3 years ago
Created attachment 8517086 [details] [diff] [review]
0_meta-ref-base (r=bz,mcmanus)
Attachment #8515258 - Attachment is obsolete: true
Attachment #8517086 - Flags: review+
(Assignee)

Comment 251

3 years ago
Created attachment 8517088 [details] [diff] [review]
3_refpol_for_styles (r=bz)
Attachment #8515262 - Attachment is obsolete: true
Attachment #8517088 - Flags: review+
(Assignee)

Comment 252

3 years ago
Created attachment 8517089 [details] [diff] [review]
4_refpol_for_images (r=seth)
Attachment #8515264 - Attachment is obsolete: true
Attachment #8517089 - Flags: review+
(Assignee)

Comment 253

3 years ago
Created attachment 8517091 [details] [diff] [review]
5_refpol_for_contentdom (r=jst)
Attachment #8515265 - Attachment is obsolete: true
Attachment #8517091 - Flags: review+
(Assignee)

Comment 254

3 years ago
Created attachment 8517092 [details] [diff] [review]
6_refpol_for_objects (r=jst)
Attachment #8515266 - Attachment is obsolete: true
Attachment #8517092 - Flags: review+
(Assignee)

Comment 255

3 years ago
Created attachment 8517093 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist (r=jst)
Attachment #8515267 - Attachment is obsolete: true
Attachment #8517093 - Flags: review+
(Assignee)

Comment 256

3 years ago
Created attachment 8517094 [details] [diff] [review]
9_nsSyncLoadService (r=peterv)
Attachment #8515268 - Attachment is obsolete: true
Attachment #8517094 - Flags: review+
(Assignee)

Comment 257

3 years ago
Created attachment 8517095 [details] [diff] [review]
10_test_speculation (r=bz)
Attachment #8515270 - Attachment is obsolete: true
Attachment #8517095 - Flags: review+
(Assignee)

Comment 258

3 years ago
Created attachment 8517096 [details] [diff] [review]
11_test_policyparser (r=bz)
Attachment #8515271 - Attachment is obsolete: true
Attachment #8517096 - Flags: review+
I haven't started reviewing the tests, so feel free to upload an updated version.
Comment on attachment 8514434 [details] [diff] [review]
metaref_tests

> contains a
> +// script that clicks a link after 5 seconds when all resources are (hopefully)
> +// loaded.

That seems like a recipy for randomorange.  Why is the load event not good enough?

>+  return '<!DOCTYPE HTML>\n\

This would have been a lot simpler with backticks, but ok.

>+               window.setTimeout(function() {\n\

Again, a likely source of random orange.  Why not have the window postmessage back when it's ready?

>+      document.getElementById('content').innerHTML +=

Why?  In any case, presumably this should be textContent, not innerHTML if we do want to dump this in the DOM.

r- for those timeouts.
Attachment #8514434 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 261

3 years ago
Created attachment 8520915 [details] [diff] [review]
metaref_tests

(In reply to Boris Zbarsky [:bz] from comment #260)
> That seems like a recipy for randomorange.  Why is the load event not good
> enough?

It is _absolutely_ good enough; I changed the test to use it.

> Again, a likely source of random orange.  Why not have the window
> postmessage back when it's ready?

Done.

> >+      document.getElementById('content').innerHTML +=
> 
> Why?  In any case, presumably this should be textContent, not innerHTML if
> we do want to dump this in the DOM.

Good catch.  Fixed.

And the backticks... I didn't know about those!  I updated the multiline strings to use those: much nicer on the eyes.
Attachment #8514434 - Attachment is obsolete: true
Attachment #8520915 - Flags: review?(bzbarsky)
Comment on attachment 8520915 [details] [diff] [review]
metaref_tests

>+             @import "` + _createTestUrl('import-css') + `";

Backticks are cooler than you think.  ;)  This could be:

               @import "${_createTestUrl('import-css')}";

and similar in other cases.  Basically, ${} inside a backtick string is treated as "treat the expression inside the curlies as JS, evaluate it, and insert it into the string at this point".

>+             window.addEventListener("message", function(message) {

I think it would be clearer to do that before the window.open() call, so you're not depending on ordering of the two windows.

r=me
Attachment #8520915 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 263

3 years ago
> (In reply to Boris Zbarsky [:bz] from comment #260)
> > That seems like a recipy for randomorange.  Why is the load event not good
> > enough?
...
> > Again, a likely source of random orange.  Why not have the window
> > postmessage back when it's ready?

I was a bit hasty in my revision of the tests; it appears after converting the timeouts to postMessage and load event, they time out on e10s.  Still investigating, and I plan to fix that before landing.
(Assignee)

Updated

3 years ago
Blocks: 1100362
(Assignee)

Comment 264

3 years ago
Created attachment 8523861 [details] [diff] [review]
metaref_tests (r=bz)

New patch that addresses bz's review comments.

Discussed this with bz via email, and I'm going to get this landed before fixing the e10s failure; since the failure is only in the new tests, I'm going to disable the tests in e10s and then figure out and fix the problem after landing this feature (bug 1100362).

bz: thanks for the pointers on the string templating... backticks are awesome.
Attachment #8520915 - Attachment is obsolete: true
Attachment #8523861 - Flags: review+
(Assignee)

Comment 265

3 years ago
Comment on attachment 8523861 [details] [diff] [review]
metaref_tests (r=bz)

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

::: dom/base/test/test_bug704320.html
@@ +193,5 @@
> +}
> +
> +// BEGIN
> +// Currently triggers assertions on e10s due to bug 820466.
> +SimpleTest.expectAssertions(0,15);

I just noticed I didn't remove this line -- since the tests are disabled on e10s, there should not be any assertions.  I'll remove this before landing.
(Assignee)

Comment 266

3 years ago
And it appears the tests are now timing out on android:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbefe1e172ec

I tried changing the use of backticks/templates back to line continuations, and it helped a little bit, but the tests still regularly time out on 2.3:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e0db5b22c7f1

There's no helpful output from the try runs (as far as I can tell), so to figure this out I'll probably have to set up a fennec build environment, which will take some time.
(Assignee)

Updated

3 years ago
Blocks: 1100609
(Assignee)

Comment 267

3 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #266)
> There's no helpful output from the try runs (as far as I can tell), so to
> figure this out I'll probably have to set up a fennec build environment,
> which will take some time.

jchen helpfully directed me to the logcat for the try runs.  It looks like the tests are timing out because they are long.  Filed bug 1100609 to make the tests work on Android, but for now I'll disable them.  Unfortunately I also have to revert the tests to use line continuations instead of backticks/templates because test_bug704320_policyset.html times out on b2g (it shares an sjs file with test_bug704320.html where all the backticks live).
(Assignee)

Comment 268

3 years ago
Created attachment 8524608 [details] [diff] [review]
metaref_tests (r=bz)

Reverted backticks/templates to line continuations and disabled the test on android for now.  Carrying over r=bz.

Try looks good:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=be3dcfdab08b
Attachment #8523861 - Attachment is obsolete: true
Attachment #8524608 - Flags: review+
(Assignee)

Comment 269

3 years ago
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f268d422e9fb
Flags: in-testsuite+
Target Milestone: --- → mozilla36

Updated

3 years ago
Blocks: 1101288
https://hg.mozilla.org/mozilla-central/rev/bf73bf22dfba
https://hg.mozilla.org/mozilla-central/rev/4c2d59fd05fb
https://hg.mozilla.org/mozilla-central/rev/a35d54820841
https://hg.mozilla.org/mozilla-central/rev/7d7cd5da5a7a
https://hg.mozilla.org/mozilla-central/rev/dd257f17530c
https://hg.mozilla.org/mozilla-central/rev/3511a31f2cd3
https://hg.mozilla.org/mozilla-central/rev/5f0f56a36128
https://hg.mozilla.org/mozilla-central/rev/d76316a3c1dd
https://hg.mozilla.org/mozilla-central/rev/4a7db36c3c75
https://hg.mozilla.org/mozilla-central/rev/52dfc1a43464
https://hg.mozilla.org/mozilla-central/rev/a0c582c95275
https://hg.mozilla.org/mozilla-central/rev/f268d422e9fb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(In reply to Jochen Eisinger from comment #4)
> The last meta tag processed wins, i.e. for script1 no referrer is send, for script2 the origin is send.

This is a problem. For example, if the second script is Google Analytics or an ad network, they are likely to set the meta tag to the most permissive one, overriding the wish of the page author. Often, the site admin and the page author aren't the same, and even if they are, they can't win against Google and Doubleclick.

The most restrictive should win, no matter where in the document it appears.
Comment on attachment 8517093 [details] [diff] [review]
7_refpol_for_nswebbrowserpersist (r=jst)

>+  persist.saveURI(sourceURI, null,
>+                  null, Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT,
>+                  null, null, targetURI, null);
Where's REFERRER_POLICY_DEFAULT defined?
Flags: needinfo?(sstamm)
(Assignee)

Comment 273

3 years ago
It's not (was an unfortunate mistake).  See bug 1103280.
Blocks: 1103280
Flags: needinfo?(sstamm)
I've documented there https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta (including comment 18)
and
https://developer.mozilla.org/en-US/Firefox/Releases/36#HTML

(Review needed, I hope to have got it right, but not 100% sure)
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #31)
> From
> http://googlewebmastercentral.blogspot.co.uk/2012/03/upcoming-changes-in-
> googles-http.html
> 
> "Starting in April, for browsers with the appropriate support, we will be
> using the referrer meta tag to automatically simplify the referring URL that
> is sent by the browser when visiting a page linked from an organic search
> result. This results in a faster time to result and more streamlined
> experience for the user."
> 
> I tested this in Firefox and in Chrome by searching Google for "what is my
> referer" and clicking the first result:
> 
> Firefox:
> http://www.google.com/
> url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&ved=0CDIQFjAA&url=http%3A%2F%2Fwww.
> whatismyreferer.com%2F&ei=42QtUfO-M6Xr2QXZ7oD4Dg&usg=AFQjCNFt-
> KMqneTZfEb6OxjPZlD4ogiJcQ&bvm=bv.42965579,d.b2I 
> 
> This shows that Google's SERP links are http://www.google.com/url?... links
> that redirect to the given site.
> 
> Chrome:
> https://www.google.com/
> 
> This shows that Google's links are links directly to the target site.
> 
> I think this is likely to be a significant performance issue that should be
> resolved ASAP. It also looks like something that should be easy to implement.

Whatismyreferer.com still shows SERP links. Do we need to evangelize Google?
Flags: needinfo?(brian)

Updated

3 years ago
Depends on: 1110036
(In reply to Masatoshi Kimura [:emk] from comment #275)
> Whatismyreferer.com still shows SERP links. Do we need to evangelize Google?

Google is unlikely to do anything until referrer control makes it to a Firefox release. In the most recent webappsec thread, it seems unclear whether <meta referrer> will even stay alive. See  https://groups.google.com/forum/#!topic/mozilla.dev.privacy/wmPzPCdzIU8 and http://lists.w3.org/Archives/Public/public-webappsec/2014Dec/0024.html and http://lists.w3.org/Archives/Public/public-webappsec/2014Dec/0027.html.
Flags: needinfo?(brian)

Comment 277

3 years ago
Filed Bug 1113431 and Bug 1113438 with edge cases.
(Assignee)

Updated

3 years ago
Depends on: 1113438
Blocks: 1107694
No longer blocks: 1107694
Depends on: 1107694

Updated

3 years ago
Depends on: 1116880

Comment 278

3 years ago
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #276)
> (In reply to Masatoshi Kimura [:emk] from comment #275)
> > Whatismyreferer.com still shows SERP links. Do we need to evangelize Google?
> 
> Google is unlikely to do anything until referrer control makes it to a
> Firefox release.

Brian, any way to get a notification when/if this makes it to a Firefox release?

Comment 279

3 years ago
It's in 36, which https://wiki.mozilla.org/Releases says should be released on 2/23.  But see dejavu in bug 1113431 (though it works fine with target=_blank, it's just middle click and context menu that's broken).

Comment 280

3 years ago
This missed the documentation on nsIWebBrowserPersist: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebBrowserPersist and any other interfaces that were changed (I've not checked).
Keywords: dev-doc-complete → dev-doc-needed
Whiteboard: For documentation, see comment 18

Updated

3 years ago
Depends on: 1124665

Comment 281

3 years ago
I've attempted to update https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebBrowserPersist - it'd be good if someone with the requisite background knowledge can check it and/or supplement it (in particular, the descriptions of the referrer policies in the .idl file were a little confusing at quick glance: REFERRER_POLICY_ORIGIN_WHEN_XORIGIN is like the default, but REFERRER_POLICY_ORIGIN had no such indication - I'm assuming that is, too, but I didn't want to put anything there that wasn't born out by the IDL docs.
Added to the 36 release notes with "<meta name="referrer"> implemented for more privacy" as wording. Don't hesitate to propose something else. Added a link to https://blog.mozilla.org/security/2015/01/21/meta-referrer/

By the way, it has been mentioned here (French):
http://www.zdnet.fr/actualites/firefox-36-annonce-un-coup-de-pouce-pour-la-vie-privee-39813509.htm
relnote-firefox: --- → 36+
Depends on: 1113431
(Assignee)

Updated

3 years ago
There is now an intent to ship for this: https://groups.google.com/forum/#!topic/mozilla.dev.platform/0elnyR7jZe8
In the future, when you make breaking changes to interfaces like this, please add the addon-compat keyword. The `saveURI` method is used in hundreds of add-ons[1], but this change was never documented properly, or added to the compatibility validator, meaning that many of them are now broken on release.

[1] https://mxr.mozilla.org/addons/ident?i=saveURI&filter=
Keywords: addon-compat
(Assignee)

Updated

2 years ago
Blocks: 1140638
Depends on: 1141142
(Assignee)

Updated

2 years ago
Depends on: 1156107
Depends on: 1161221
(Assignee)

Updated

2 years ago
Depends on: 1163743
Blocks: 1165501

Updated

2 years ago
Depends on: 1166891
See Also: → bug 1166910
Depends on: 1168538
Depends on: 1168540
Depends on: 1174915
Depends on: 1174921
Blocks: 1175736

Updated

2 years ago
Blocks: 1102599

Updated

2 years ago
Blocks: 1178101
Blocks: 1184781
Depends on: 1185019
Blocks: 1185719
Blocks: 1186308
Depends on: 1189364

Updated

a year ago
Depends on: 1279494
Depends on: 1276836
You need to log in before you can comment on or make changes to this bug.