David Bruant Reporter
	Description • 10 years ago

      No description provided.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 1 • 10 years ago

Looks like most of what we'll need in the backend is part of bug 704320.  The scope of work for this bug is to add on top of that: simply setting the referrer policy based on the CSP header when the CSP header gets parsed.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 2 • 10 years ago

Here's a first whack at the CSP bits for a referrer policy.

Garrett: does this look sane (before I start writing tests for it)?

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 3 • 10 years ago

FYI, if you want to try it out, you'll have to apply the patch on bug 704320 first, since this sets mReferrerPolicy on nsDocument that doesn't exist without it.

	Garrett Robinson [:grobinson]
	Comment 4 • 10 years ago

Comment on attachment 8372673 [details] [diff] [review]
first whack

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

Other than comments, lgtm. Test away!

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +197,5 @@
> +  const unsigned short REFERRER_POLICY_DEFAULT   = 2;
> +  const unsigned short REFERRER_POLICY_ORIGIN    = 3;
> +  const unsigned short REFERRER_POLICY_ALWAYS    = 4;
> +
> +   

trailing whitespace

::: content/base/src/contentSecurityPolicy.js
@@ +182,5 @@
> +   * otherwise NEVER.
> +   */
> +  get referrerPolicy() {
> +    let CSP = Ci.nsIContentSecurityPolicy;
> +    return this._policies.reduce(

This matches the conflict resolution defined in the spec (3.2.5.13.1). So we got that going for us, which is nice.

@@ +189,5 @@
> +          if (prev._referrerPolicy == CSP.REFERRER_POLICY_UNSET)
> +            return cur._referrerPolicy;
> +
> +          // if current CSP is unset, we can use the previous one.
> +          if (cur == CSP.REFERRER_POLICY_UNSET)

Did you mean cur._referrerPolicy?

@@ +191,5 @@
> +
> +          // if current CSP is unset, we can use the previous one.
> +          if (cur == CSP.REFERRER_POLICY_UNSET)
> +            return prev._referrerPolicy;
> +          

trailing whitespace

@@ +196,5 @@
> +          // if the two are the same, use it.
> +          if (cur._referrerPolicy == prev._referrerPolicy)
> +            return cur._referrerPolicy;
> +
> +          // if we get here, the policies disagree and neither is unset.

s/unset/set?

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8372673 [details] [diff] [review]
first whack

This will no longer work due to the CSP rewrite.  Also, there's a speculative parser we have to seed with any referrer policy set on the document before the parser is triggered.

And tests.  This needs tests.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 6 • 10 years ago

Tests for the CSP referrer directive implementation

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 7 • 10 years ago

Chris: I'd like you to do a pass on this implementation before I flag someone for the changes in dom (I'll need someone from necko and henri or bz to look at the changes to the way the html5 parser is set up to use CSP's referrer policy).

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

Comment on attachment 8527994 [details] [diff] [review]
bug965727-referrer

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

The CSP parts look good. r=me on those bits. Please try to avoid c-style casts wherever possible and let a dom-peer look over the other changes in this patch.

::: dom/base/nsDocument.cpp
@@ +2990,5 @@
> +  uint32_t referrerPolicy = mozilla::net::RP_Default;
> +  rv = csp->GetReferrerPolicy(&referrerPolicy, &hasReferrerPolicy);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (hasReferrerPolicy) {
> +    // Referrer policy spec (section 6.1) says that once the referrer policy

Section 6.1 where? Probably worth adding the link to the section in the spec.

@@ +2993,5 @@
> +  if (hasReferrerPolicy) {
> +    // Referrer policy spec (section 6.1) says that once the referrer policy
> +    // is set, any future attempts to change it result in No-Referrer.
> +    if (!mReferrerPolicySet) {
> +      mReferrerPolicy = (ReferrerPolicy) referrerPolicy;

Even though a C-Style cast probably works correctly here, please rather use a static_cast here.

@@ +2996,5 @@
> +    if (!mReferrerPolicySet) {
> +      mReferrerPolicy = (ReferrerPolicy) referrerPolicy;
> +      mReferrerPolicySet = true;
> +    } else if (mReferrerPolicy != referrerPolicy) {
> +      mReferrerPolicy = mozilla::net::RP_No_Referrer;

Is it worth adding any kind of log here!

@@ +3000,5 @@
> +      mReferrerPolicy = mozilla::net::RP_No_Referrer;
> +    }
> +
> +    // Any Referrer Policy is set for the speculative parser in
> +    // nsHTMLDocument::StartDocumentLoad()

This comment is confusing, does that mean some code is missing here? Please clarify.

::: dom/html/nsHTMLDocument.cpp
@@ +679,5 @@
>      executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());
> +    if (mReferrerPolicySet) {
> +      // CSP may have set the referrer policy, so a speculative parser should
> +      // start with the new referrer policy.
> +      executor->SetSpeculationReferrerPolicy((mozilla::net::ReferrerPolicy) mReferrerPolicy);

Please try to avoid c-style casts.

@@ +1658,5 @@
> +    // start with the new referrer policy.
> +    nsHtml5TreeOpExecutor* executor = nullptr;
> +    executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());
> +    if (executor && mReferrerPolicySet) {
> +      executor->SetSpeculationReferrerPolicy((mozilla::net::ReferrerPolicy) mReferrerPolicy);

same here, please no c-style cast.

::: dom/security/nsCSPContext.cpp
@@ +258,5 @@
>  
>  NS_IMETHODIMP
> +nsCSPContext::GetReferrerPolicy(uint32_t* outPolicy, bool* outIsSet)
> +{
> +  *outIsSet = false;

Nit: unlikely but still possible that outPolicy not gets assigned - do you want to also assign a defaultvalue to outPolicy up here?

::: dom/security/nsCSPUtils.h
@@ +355,5 @@
>  
>      inline bool getReportOnlyFlag() const
>        { return mReportOnly; }
>  
> +    inline void setReferrerPolicy(nsAString& aValue)

Nit: To indicate that no one should ever change aValue you could add rewrite to:
setReferrerPolicy(const nsAString* aValue)

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

Comment on attachment 8527991 [details] [diff] [review]
bug965727-referrer-tests

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

Really nice tests - just some nits. r=me.

::: dom/base/test/csp/file_csp_referrerdirective.html
@@ +9,5 @@
> +var id = "unknown";
> +for (var i=0; i < args.length; i++) {
> +  var arg = unescape(args[i]);
> +  if (arg.indexOf('=') > 0 &&
> +      arg.indexOf('id') == 0) {

Nit: no need for a linebreak here.

@@ +21,5 @@
> +  'results': {
> +    'sameorigin': false,
> +    'crossorigin': false,
> +    'downgrade': false
> +  }};

Nit: I think the second } and the semicolon should go in the next line.

::: dom/base/test/csp/mochitest.ini
@@ +101,5 @@
>    file_multi_policy_injection_bypass_2.html
>    file_multi_policy_injection_bypass_2.html^headers^
>    file_form-action.html
> +  file_csp_referrerdirective.html
> +  referrerdirective.sjs

since every supporting file is prefixed with file_, probably it's worth changing the name to file_referrerdirective.sjs for consistency.

::: dom/base/test/csp/test_CSP_referrerdirective.html
@@ +5,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=965727
> +-->

Nit: Usually we add/set the bugnumber in the <title>. If you decide to add the bugnumber to the title you could remove that comment.

@@ +30,5 @@
> +   the scripts are blocked, the test fails (they should run).  When loaded,
> +   each script updates a results object in the test page, and then when the
> +   test page has finished loading all the scripts, it postMessages back to this
> +   page.  Once all tests are done, the results are checked.
> + */

Nice description, really like that. Probably worth adding a leading '*' to every line which makes it easier to pinpoint the comment start and end.

@@ +33,5 @@
> +   page.  Once all tests are done, the results are checked.
> + */
> +
> +var testData = {
> +  'default': { 'csp': "script-src * 'unsafe-inline'; referrer default",

You could factor out
> "script-src * 'unsafe-inline';
and only keep the bits that vary in the test-setup here and add the rest where you assign 'elt.src'.

@@ +60,5 @@
> +                          'downgrade': 'none' }},
> +  };
> +
> +
> +var bug965727 = {

can we rename that variable? 'bug965727' is not very descriptive.

@@ +98,5 @@
> +    SimpleTest.finish();
> +  }
> +};
> +
> +SimpleTest.waitForExplicitFinish();

Nit: Move that up to the top to make it more explicit that we have to wait for an explicit finish().

@@ +112,5 @@
> +      // one iframe created for each test case
> +      for (var id in testData) {
> +        var elt = document.createElement("iframe");
> +        elt.src = "https://example.com/tests/dom/base/test/csp/" +
> +                  "file_csp_testserver.sjs?" +

No need for a line break before "file_csp_testserver.sjs".

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 10 • 10 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Really nice tests - just some nits. r=me.

Thanks for the speedy review!  I disagree with some of your nits.

> > +  referrerdirective.sjs
> 
> since every supporting file is prefixed with file_, probably it's worth
> changing the name to file_referrerdirective.sjs for consistency.

I'd like to move away from using file_ in front of everything since it makes paths really long.  Can we start to reserve file_ for html files and make all the rest start with just whatever?  Kind of like using CSP in the file when it's already in dom/base/test/csp...

> You could factor out
> > "script-src * 'unsafe-inline';
> and only keep the bits that vary in the test-setup here and add the rest
> where you assign 'elt.src'.

This adds unnecessary complexity to the code and makes the policies harder to read at a quick glance.  I'd like to keep the CSP policies intact for easy reading.

> Nit: Move that up to the top to make it more explicit that we have to wait
> for an explicit finish().

I wanted to put the SimpleTest.waitForExplicitFinish() as close to the entry point as possible so when we're debugging a test failure, it's clear that we didn't forget it.  All the stuff above it is just function definitions (nothing executes before it).

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 11 • 10 years ago

Fixed some nits.  Carrying over r=ckerschb.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 12 • 10 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Section 6.1 where? Probably worth adding the link to the section in the spec.

Eugh... it's still a draft and I hate to link from the code to a draft that will move.  Should I drop the section reference or just give it a name?

Thanks for the review comments!  Carrying over r=ckerschb.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8531022 [details] [diff] [review]
bug965727-referrer


jst: I know you're super busy, but since you reviewed some of the meta referrer stuff, can you look at the dom/ and parser/ stuff here too?  There are not many changes.  I had to add a method to the nsHtml5TreeOpExecutor to set the speculation referrer policy not based on a string, but on a ReferrerPolicy (simplistic overload), and I had to modify nsHTMLDocument to tell the speculation parser what if any referrer policy was set on the document before parsing time (needed for CSP-set policies, but not <meta>-set policies).

	Christoph Kerschbaumer [:ckerschb]
	Comment 14 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> Created attachment 8531022 [details] [diff] [review]
> bug965727-referrer
> 
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> > Section 6.1 where? Probably worth adding the link to the section in the spec.
> 
> Eugh... it's still a draft and I hate to link from the code to a draft that
> will move.  Should I drop the section reference or just give it a name?

What you have right now works for me. Was just another nit anyway :-)

	Johnny Stenback (:jst)
	Comment 15 • 10 years ago

Comment on attachment 8531022 [details] [diff] [review]
bug965727-referrer

Looks great, one thing caught my eye when reading through this:

- In nsCSPContext::GetReferrerPolicy():

+  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
+    nsAutoString refpol;

Maybe hoist that nsAutoString outside of the loop to avoid calling its ctor/dtor every iteration.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 16 • 10 years ago

updated for bitrot.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 17 • 10 years ago

Thanks jst!  Updated with your suggestion (and for bitrot).  Carrying over r=jst and r=ckerschb.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 18 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2973cb4fa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/eabee47625c6

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 19 • 10 years ago

Had to disable the tests on b2g:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8277e5192972

Was getting this in the logcat (and then a timeout because the https content doesn't load):

12-17 20:15:13.424 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Ddefault%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520default%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.424 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Ddefault%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520default%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)
12-17 20:15:13.424 E/Mochitest(  796):     at setReady (about:neterror?e=proxyConnectFailure&u=https%3A//example.co
12-17 20:15:13.504 E/Mochitest(  796): Content JS ERROR: net-error
12-17 20:15:13.504 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.504 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)
12-17 20:15:13.504 E/Mochitest(  796):     at setReady (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/te
12-17 20:15:13.534 E/Mochitest(  796): Content JS ERROR: net-error
12-17 20:15:13.534 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin-when-crossorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin-when-crossorigin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.534 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin-when-crossorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin-when-crossorigin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 20 • 10 years ago

ckerschb pointed me here, same problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=826805#c17

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7c2973cb4fa1
https://hg.mozilla.org/mozilla-central/rev/eabee47625c6
https://hg.mozilla.org/mozilla-central/rev/8277e5192972

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

Updated https://developer.mozilla.org/en-US/Firefox/Releases/37
and
https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives