1251378 - We don't propagate the document's referrer policy to a worker

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[(no longer active)]

This test hits this issue: <https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/policies/referrer-origin-worker.html>

What happens is the document is served with a CSP that sets the referrer policy to "origin", which is stored correctly in nsIDocument::mReferrerPolicy.  But in the worker fetch code when we try to set the referrer, we get to this code <https://hg.mozilla.org/mozilla-central/annotate/d848a5628d801a460a7244cbcdea22d328d8b310/dom/base/nsContentUtils.cpp#l8116> which discards the referrer completely.

Comment 1

[Andrew Overholt [:overholt]]

Christoph, is this something you can take?

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Franziskus, can you take a look please? This *seems* to be simple. Let me know if it's not.

Comment 3

[Franziskus Kiefer [:franziskus]]

Passing this on to Thomas.

Comment 4

[Thomas Nguyen (:tnguyen)]

Sorry for late reply. 
I am taking a look at the issue and trying to write a local patch/test, interestingly when running fetch in worker test <https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/policies/referrer-origin-worker.html> 
I see nsCSPContext does not add policy in header Content-Security-Policy: referrer origin) to mPolicies. (it does if I run referrer-origin.html, fetch not in worker). Maybe I have to look closer on that.

Comment 5

[Thomas Nguyen (:tnguyen)]

Referrer policy is not delivered from CSP which does not work properly in fetch() in dedicated worker. Depend on bug 1259648

Comment 6

[Henry Chang [:hchang]]

Hi Thomas,

Just for reference: https://bugzilla.mozilla.org/show_bug.cgi?id=1251872

The above bug slightly updated the referrer policy propagation. Thanks :)

Comment 7

[Henry Chang [:hchang]]

A quick guess:

If |fetch| is originated from main thread, [1] would set the document to the FetchDriver so that the fetch request could get some info from the document. However, if |fetch| is originated from worker thread, [2] wouldn't set a document to FetchDriver. Therefore, none of the information could be obtained.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#201
[2] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#138

Comment 8

[Thomas Nguyen (:tnguyen)]

Attachment: Part1 v1

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Part2 v1

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: part 3: update test case

Comment 11

[Thomas Nguyen (:tnguyen)]

Thanks Henry for looking at that.

Hi Frankziskus.
Could you please look at the patches and tell me if I am going wrong direction?
- Part 1 : ensure that CSP has initialized properly for fetching in worker, then get the referrer policy to propagate to worker.
- Part 2 : Fix the fetch(). We should use environment's referrer policy if there's no referrer policy in the request.
- Part 3 : Update web-platform test case to pass web-platform-test.
Thank you very much.

Comment 12

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8735388 [details] [diff] [review]
part 3: update test case

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

f+ if the tests are actually working now :)

Comment 13

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8735387 [details] [diff] [review]
Part2 v1

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

lgtm in general with the comments below.

You should probably get review from Ehsan for this.

::: dom/fetch/FetchDriver.cpp
@@ +263,5 @@
>      // Step 2. Set the referrer.
>      nsAutoString referrer;
>      mRequest->GetReferrer(referrer);
> +
> +    // Referrer Policy in Request can be used to override a referrer policy

The Referr Policy ...

@@ +265,5 @@
>      mRequest->GetReferrer(referrer);
> +
> +    // Referrer Policy in Request can be used to override a referrer policy
> +    // associated with an environment settings object.
> +    // If there's no Referrer Policy in the request, should inherite

... , it should be inherited

@@ +271,2 @@
>      ReferrerPolicy referrerPolicy = mRequest->ReferrerPolicy_();
> +    net::ReferrerPolicy net_referrerPolicy = mRequest->GetEnvironmentReferrerPolicy();

does this return RP_Default if referrerPolicy of mRequest is empty?

::: dom/fetch/InternalRequest.h
@@ +476,5 @@
>    // "about:client": client (default)
>    // URL: an URL
>    nsString mReferrer;
>    ReferrerPolicy mReferrerPolicy;
> +  mozilla::net::ReferrerPolicy mEnvironmentReferrerPolicy;

can't we use the other ReferrerPolicy that's used for mReferrerPolicy?

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8735386 [details] [diff] [review]
Part1 v1

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

lgtm to me with the comments below. That's the right direction I think.

You probably need review from Christoph for the CSP things, some worker peer (Jonas or Blake maybe).

::: dom/workers/ScriptLoader.cpp
@@ +1164,5 @@
>      }
>  
>      DataReceived();
> +
> +    // We should make sure csp initilized if there's no csp.

... csp is initialized ...

@@ +1170,5 @@
> +      NS_ConvertASCIItoUTF16 cspHeaderValue(tCspHeaderValue);
> +      NS_ConvertASCIItoUTF16 cspROHeaderValue(tCspROHeaderValue);
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +
> +      nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();

we get the principal earlier in this function already. We probably should use that one as it also makes sure that we actually have a principal.

@@ +1186,5 @@
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +
> +        // check if eval is allowed
> +        bool evalAllowed = false;

evalAllowed is usually initialised with true, we might want to do the same here.

@@ +1196,5 @@
> +        mWorkerPrivate->SetEvalAllowed(evalAllowed);
> +
> +        // Set ReferrerPolicy
> +        bool hasReferrerPolicy = false;
> +        uint32_t rp = mozilla::net::RP_Default;

rp is set to default in GetReferrerPolicy anyway. Is the mozilla:: necessary?

::: dom/workers/WorkerPrivate.cpp
@@ +1767,5 @@
>  
>  WorkerLoadInfo::WorkerLoadInfo()
>    : mWindowID(UINT64_MAX)
>    , mServiceWorkerID(0)
> +  , mReferrerPolicy(mozilla::net::RP_Default)

mozilla:: shouldn't be necessary

Comment 15

[Thomas Nguyen (:tnguyen)]

Attachment: Part 2 v2

Thanks Franziskus for your feedback
Hi Ehsan.
It seems you are busy now :(. Could you please review the patch when you have time?
Thanks

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: Part1 v2

Hi Christoph.
Could you please take a look into the patch?
Thanks

Comment 17

[(no longer active)]

Comment on attachment 8737057 [details] [diff] [review]
Part 2 v2

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

::: dom/fetch/InternalRequest.h
@@ +478,5 @@
>    nsString mReferrer;
>    ReferrerPolicy mReferrerPolicy;
> +  // The Environment Referrer Policy should be net::ReferrerPolicy so that it
> +  // could be associated with nsIHttpChannel
> +  // Default value: net::RP_Default

Can you please reword this comment to make it more useful?  The part about the default value is not needed, but it would be helpful to document what this will be for Request objects created from Window or Worker contexts.

::: dom/fetch/Request.cpp
@@ +410,5 @@
>    }
>  
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(global);
> +    nsCOMPtr<nsIDocument> doc;

This can be hoisted down to the body of the if condition below.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8737058 [details] [diff] [review]
Part1 v2

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

LGTM, but would be good if Jonas has a final look at the changes within workers/ScriptLoader.

::: dom/workers/ScriptLoader.cpp
@@ +1171,5 @@
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +
> +      rv = principal->EnsureCSP(nullptr, getter_AddRefs(csp));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (csp) {

EnsureCSP always returns a CSP object, so you can skip the subsequent |if(csp)| check. I think it makes sense to pass parentDoc as the first arg to EnsureCSP so CSP gets initalized with the doc (if not null), otherwise we fall back to using the principal when setting the requestcontext for CSP within EnsureCCSP(). Jonas probably needs to review the worker bits anyway.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8737058 [details] [diff] [review]
Part1 v2

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

If I'm right below, then this clearly needs more tests since we apparently don't test that read-only policies are only read-only.

Generally though, I don't know what specs are saying should happen. So I can't verify that the patch does the right thing in that regard. Please work with Christoph to make sure that the correct behavior is implemented here.

Our tests for CSP have been pretty spotty in general. So please make sure that you write lots of tests that both tests that things that should get blocked are blocked, and that things that shouldn't get blocked aren't blocked.

r=me with the below fixed.

::: dom/workers/ScriptLoader.cpp
@@ +1174,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (csp) {
> +        // If there's a CSP header, apply it.
> +        if (!cspHeaderValue.IsEmpty()) {
> +          rv = CSP_AppendCSPFromHeader(csp, cspHeaderValue, finalURI);

Shouldn't the last argument be |true|?

@@ +1179,5 @@
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +        // If there's a report-only CSP header, apply it.
> +        if (!cspROHeaderValue.IsEmpty()) {
> +          rv = CSP_AppendCSPFromHeader(csp, cspROHeaderValue, finalURI);

And |false| here?

Comment 20

[Thomas Nguyen (:tnguyen)]

Thanks you for your review.

> ::: dom/workers/ScriptLoader.cpp
> @@ +1174,5 @@
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +      if (csp) {
> > +        // If there's a CSP header, apply it.
> > +        if (!cspHeaderValue.IsEmpty()) {
> > +          rv = CSP_AppendCSPFromHeader(csp, cspHeaderValue, finalURI);
> 
> Shouldn't the last argument be |true|?
I see the last argurment is bool aReportOnly.
I guess you meant rv = CSP_AppendCSPFromHeader(csp, cspHeaderValue, false); :)

> @@ +1179,5 @@
> > +          NS_ENSURE_SUCCESS(rv, rv);
> > +        }
> > +        // If there's a report-only CSP header, apply it.
> > +        if (!cspROHeaderValue.IsEmpty()) {
> > +          rv = CSP_AppendCSPFromHeader(csp, cspROHeaderValue, finalURI);
> 
> And |false| here?

Similarly, did you mean rv = CSP_AppendCSPFromHeader(csp, cspROHeaderValue, true)? 
Or am I missing something?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, yes, I got the true/false mixed up. That means that the patch as-is doesn't enforce *any* CSP policies in workers. How did that get past tests?

Comment 22

[Thomas Nguyen (:tnguyen)]

Thanks Jonas
Setting true or false both make past tests passed. Policies both are parsed successfully and appended to CSP.
But setting true would have log warning in [1]
[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#1203.

Anyway, it should be false.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If the tests pass both with true or false, then either something else is wrong in the code, or the tests aren't testing the right thing.

If you pass "false" then that should block the network requests, and if you pass "true" it should not. That's a big difference. Are the tests checking whether the network requests reach the server?

Comment 24

[Thomas Nguyen (:tnguyen)]

 (In reply to Jonas Sicking (:sicking) from comment #23)
> If the tests pass both with true or false, then either something else is
> wrong in the code, or the tests aren't testing the right thing.
> 
> If you pass "false" then that should block the network requests, and if you
> pass "true" it should not. That's a big difference. Are the tests checking
> whether the network requests reach the server?

Thanks Jonas. You are right. Probably there's something wrong in the code or tests. But I think it might not relate to these patches, because the patches mostly fix referrer policy related things ( except the csp parsing and csp policy adding). I am not sure, let me write some tests and see.

Comment 25

[Thomas Nguyen (:tnguyen)]

Hi Christoph, Jonas.
I am not quite sure if the CSP specs in [1] is what our current CSP implementation follows (because I see referrer directive is not mentioned anymore in new published CSP specs [2]). Could you please advise?

Well, if we are following [1], I see : 
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#335
This should check report-only before adding referrerPolicy (only add in case non-report-only policy), as [3]. Probably should file a bug to fix and test that.

[1] https://www.w3.org/TR/2014/WD-CSP11-20140211
[2] https://www.w3.org/TR/CSP2/
[3] https://www.w3.org/TR/2014/WD-CSP11-20140211/#usage-3

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know what CSP syntax and features that we implement. Hopefully Christoph does.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #25)
> Hi Christoph, Jonas.
> I am not quite sure if the CSP specs in [1] is what our current CSP
> implementation follows (because I see referrer directive is not mentioned
> anymore in new published CSP specs [2]). Could you please advise?

There was a lot of discussion about removing the referrer directive from the CSP spec. At some point it moved to the referrer spec [1] but now it even got removed from there [2]. Anyway, our standpoint is, since we already have it and people on the web rely on it, why remove it?

[1] https://www.w3.org/TR/referrer-policy/#referrer-policy-delivery
[2] https://w3c.github.io/webappsec-referrer-policy/

> Well, if we are following [1], I see : 
> https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.
> cpp#335
> This should check report-only before adding referrerPolicy (only add in case
> non-report-only policy), as [3]. Probably should file a bug to fix and test
> that.

That is indeed a bug and we should fix that. The non-normative part indicates that the referrer policy should be deliverd through Content-Security-Policy and only through Content-Security-Policy. (And obviously through meta CSP).

Can you please file a bug to ignore the referrer policy if delivered through a CSPRO? Thanks!


[3] https://www.w3.org/TR/2014/WD-CSP11-20140211/#usage-3

Comment 28

[Thomas Nguyen (:tnguyen)]

ohh, I did not mean removing referrer directive :), obviously it makes sense to keep it. I just would like to know version of CSP specs(and refererer directive is an example of difference). 
Filed a bug to ignore referrer policy delivered through a CSPRO and thanks Christoph for clarifying that.
That's exactly the reason why aReportOnly does not work in comment 23

Comment 29

[Thomas Nguyen (:tnguyen)]

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

Comment 30

[Thomas Nguyen (:tnguyen)]

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

Comment 31

[Thomas Nguyen (:tnguyen)]

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

Comment 32

[Thomas Nguyen (:tnguyen)]

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

Comment 33

[Thomas Nguyen (:tnguyen)]

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

Comment 34

[Thomas Nguyen (:tnguyen)]

Filed a bug to add test cases related to CSP bug 1268817. This should test for many syntaxes and directives, not only for referrer policy.

Comment 35

[Thomas Nguyen (:tnguyen)]

Attachment: part 2

Comment 36

[Thomas Nguyen (:tnguyen)]

Attachment: part 1

Updated as comment except removing |if (csp)|. If principal is SystemPrincipal, csp would be null.

Comment 37

[Thomas Nguyen (:tnguyen)]

There're cases failed on try.
- Mochitest dom/workers/test/test_csp.html
- Web platform test
 TEST-UNEXPECTED-PASS /content-security-policy/blink-contrib/worker-eval-blocked.sub.html expect failed
 TEST-UNEXPECTED-PASS /content-security-policy/blink-contrib/worker-function-function-blocked.sub.html expected fail
Will fix after my vacation. Thanks

Comment 38

[Thomas Nguyen (:tnguyen)]

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

Comment 39

[Thomas Nguyen (:tnguyen)]

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

Comment 40

[Thomas Nguyen (:tnguyen)]

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

Comment 41

[Thomas Nguyen (:tnguyen)]

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

Comment 42

[Thomas Nguyen (:tnguyen)]

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

Comment 43

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1

Updated to fix try failures:
- Web-platform-test failures: just enable these tests ( they were disabled, expected to fail)
- Mochitest: use worker's principal correctly (mWorkerPrivate->GetPrincipal() should be used).
These tests were passed for me locally.
Could you please take a look Cristoph? 
Thanks

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8748088 [details] [diff] [review]
Part 1

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

Everything within that patch but the *.ini files have already been reviewed, right?

r=me on the updates of the web-platform-tests; next time please create a separate patch for such things; this way we know what already got r+ed and what really needs to be reviewed. Thanks for fixing!

::: dom/security/nsCSPUtils.cpp
@@ +327,5 @@
> +                        const nsAString& aHeaderValue,
> +                        bool aReportOnly)
> +{
> +  NS_ASSERTION(aCsp, "aCsp should be a valid pointer (not null)!");
> +

please change the assertion to NS_ENSURE_ARG() so we don't deref a null-pointer accidentaly.

Comment 45

[Thomas Nguyen (:tnguyen)]

Attachment: part 1 v3

Comment 46

[Thomas Nguyen (:tnguyen)]

Attachment: Part 4

Comment 47

[Thomas Nguyen (:tnguyen)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #44)
> Comment on attachment 8748088 [details] [diff] [review]
> Part 1
> 
> Review of attachment 8748088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Everything within that patch but the *.ini files have already been reviewed,
> right?

There's still a change in ScriptLoader.cpp, that is
+      nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
I use this instead of principal we get earlier ( it may go to parent's principal and one test fails on try due to that)
> r=me on the updates of the web-platform-tests; next time please create a
> separate patch for such things; this way we know what already got r+ed and
> what really needs to be reviewed. Thanks for fixing!

Thanks for your review and suggestion. I updated separated the web platform tests to a new part.
Try looks good now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2c42d240c1

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Thomas Nguyen[:tnguyen] from comment #47)
> There's still a change in ScriptLoader.cpp, that is
> +      nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
> I use this instead of principal we get earlier ( it may go to parent's
> principal and one test fails on try due to that)

You should check with someone from the DOM team or someone who is familiar with the worker code. As far as I understand, if mWorkerPrivate->GetPrincipal() returns a nullptr then the code used to query the principal from the parent. Now you are skipping CSP completely if mWorkerPrivate->GetPrincipal() returns null. That doesn't sound quite right to me. When is that the case that mWorkerPrivate->GetPrincipal() returns null? Is it fine to skip CSP if that is the case? What test was failing?

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8748530 [details] [diff] [review]
part 1 v3

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

::: dom/workers/ScriptLoader.cpp
@@ +1168,5 @@
> +      NS_ConvertASCIItoUTF16 cspHeaderValue(tCspHeaderValue);
> +      NS_ConvertASCIItoUTF16 cspROHeaderValue(tCspROHeaderValue);
> +
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +      nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();

Please answer my questions from the previous comment!

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8748531 [details] [diff] [review]
Part 4

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

Looking at the code changes within this bug and also at the wpt tests we have to update (see underneath), I would assume this bug is essentially a dup of Bug 959388 as well, right? Using the code in this bug allows workers to specify their own CSP. We should definitely add a mochitest at dom/security/csp for this purpose.

>    <p>This test loads a worker, delivered with its own policy.
>    The eval() call in the worker should be forbidden by that
>    policy.  No report should be generated because the worker
>    policy does not set a report-uri (although this parent
>    resource does).</p>

[1] http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/blink-contrib/worker-eval-blocked.sub.html?force=1#17
[2] http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/blink-contrib/worker-function-function-blocked.sub.html?force=1#18

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8748531 [details] [diff] [review]
Part 4

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

Clearing my r? until we have answered comment 48 and 50.

Comment 52

[Thomas Nguyen (:tnguyen)]

Okay, thanks for your review. I have to take a closer look at that tomorrow when I go to office.
It fails at https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/test_csp.js#17

Comment 53

[Thomas Nguyen (:tnguyen)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #50)
> Comment on attachment 8748531 [details] [diff] [review]
> Part 4
> 
> Review of attachment 8748531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking at the code changes within this bug and also at the wpt tests we
> have to update (see underneath), I would assume this bug is essentially a
> dup of Bug 959388 as well, right? Using the code in this bug allows workers
> to specify their own CSP. We should definitely add a mochitest at
> dom/security/csp for this purpose.
Exactly, I used some CSP codes from the Bug 959388 to init CSP if there's no CSP is initialized. The way we init CSP is nearly the same (and the same as nsDocument.cpp). I have no idea if they could be consider as duplicated, and if we should follow inherited CSP as well :(
I think it's better to file a new bug for testing purpose because there would be many tests. Filed bug 1270152, it would be clearer between testing referrer-policy and CSP

Comment 54

[Thomas Nguyen (:tnguyen)]

Well, in attempting to explain why if I use principal in [1] the case will fail at [3], I found that mWorkerPrivate->GetPrincipal() at [1]. Parent principal will be used then csp will be inherited to worker (this is not expected and the case failed). 

Obviously, mWorkerPrivate->GetPrincipal() is null at [1] but will be set at [2] to channelPrincipal. 
So, If I use nsIPrincipal* principal = mWorkerPrivate->GetPrincipal() instead of;
That actually used channelPrincipal, csp was not inherited so the case passed

However, you are right, CSP should not be skipped completely if mWorkerPrivate->GetPrincipal() returns null. Parent principal should be queried in this case. I updated the new patch to fix that.
I am still concerned about Bug 959388, because I don't know deeply worker CSP inheritance from document in current implementation (I assume it is still inherited for blob, bug 1223647). Personally I don't think this bug and Bug 959388 would be a duplication. This bug we are fixing here is to add a fallback in case there's no CSP somehow (we passed eval() test case before because we are using default true in [4], but actually no CSP).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1018
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1148
[3] https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/test_csp.js#17
[4] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3533

Comment 55

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1 v4

Comment 56

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

FWIW, if I remember correctly, the case where the principal is null should be for subworkers.  e.g.

test.html

<script>
  var worker = new Worker("worker.js");
</script>

worker.js

var subworker = new Worker("subworker.js");

subworker.js

// This WorkerPrivate may have GetPrincipal() == nullptr.

Subworkers often don't get some of these main thread only objects, so you have to walk the worker's parents to the top to get them.

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

After thinking about this bug again in detail, I think we are missing one part here. Let me try to clarify: A referrer policy can be delivered through the meta element <meta referrer""> [1] or also through CSP:

(1) If delivered on the main document through <meta referrer""> then we should definitely propagate the referrer policy to the worker.

(2) But, workers can have their own CSP (See Bug 959388). The code within the ScriptLoader patch within this bug would also enable workers to ship their own CSP. According to the spec, the toplevel CSP should *not* apply to workers [2]. So, here are the questions:

(a) What if the document ships a CSP with a referrer Policy and the worker ships a CSP with a different referrer policy?

(b) Same as (a) but the main document uses <meta referrer=""> but the worker ships with a different referrer policy delivered through CSP. What should be the expected behavior?

(c) As per comment 56, what about sub workers? If a main document ships a referrer policy, the workers ships it's own referrer policy through CSP, which one applies to the sub worker? The main document's? The worker's?

We should definitely document the expected behavior in our code and potentially even fix 959388 before fixing this one.

Thomas, what do you think?

[1] https://www.w3.org/TR/referrer-policy/#referrer-policy-delivery
[2] https://www.w3.org/TR/CSP/#processing-model-workers

Comment 58

[Thomas Nguyen (:tnguyen)]

Hmm, that's interesting question. Actually specs did not mention exactly these cases. I see in new Specs `If a policy has previously been set, then we overwrite it with the new value if the new value is not the empty string`  in [1]
And [2] `when multiple sources specify a referrer policy, the value of the latest one will be used`
An similar inheritance situation of nested browser context in [3]

[1] https://w3c.github.io/webappsec-referrer-policy/#set-referrer-policy
[2] https://w3c.github.io/webappsec-referrer-policy/#unknown-policy-values
[3] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-implicit-nested

I think the priority of referrer policy delivered should be worker -> parent worker -> document.
Then a) should be worker
b) worker
c) still worker
I still need to file a question/bug to ask editor and confirm about that.
Thanks for looking into this.

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

I feel the same way and it seems reasonable to implement it that way. I suggest we fix Bug 959388 and then we fix this bug to get a semantic separation between the two bugs. Also desirable to have that separation for web platform tests which we would have otherwise updated within this bug but actually belong to Bug 959388. Thanks Thomas!

Comment 60

[Thomas Nguyen (:tnguyen)]

Just a quick clarification from editor for comment 58. That's exactly what we thought.
[1] https://github.com/w3c/webappsec-referrer-policy/issues/48#issuecomment-217444838.

Comment 61

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Just for posterity, comment 56 is true for some of the other things (GetWindow() comes to mind) but not GetPrincipal.

Workers have two principals during their lifetimes.  The first principal comes from the parent script context (either the Window, in the case of a top-level worker, or the parent worker, in the case of a subworker).  This principal is used to make the decision "is it ok to load this script?"  Once we have loaded the worker script, before executing it, we switch to the principal of the loaded script for all future security decisions (such as importScript() calls, XHR, IndexedDB, etc).

This dance is necessary to enable things like creating a cross-origin worker via CORS.

Comment 62

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8748531 [details] [diff] [review]
Part 4

Thanks Kyle for looking at this :)
Moving CSP related tests to Bug 959388

Comment 63

[Thomas Nguyen (:tnguyen)]

Attachment: part 2: use referrer policy of worker

Rebase, carry r+

Comment 64

[Thomas Nguyen (:tnguyen)]

Attachment: Propagate referrer policy to worker

Comment 65

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8767544 [details] [diff] [review]
Propagate referrer policy to worker

Hi Christoph,
Could you please review the new patch?
Thanks

Comment 66

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8767544 [details] [diff] [review]
Propagate referrer policy to worker

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

lgtm, r=me

::: dom/workers/ScriptLoader.cpp
@@ +1204,5 @@
>            mWorkerPrivate->SetReportCSPViolations(reportEvalViolations);
> +
> +          // Set ReferrerPolicy, default value is set in GetReferrerPolicy
> +          bool hasReferrerPolicy;
> +          uint32_t rp;

nit: please init those values;
hasReferrerPolicy = false;
rp = mozilla::net::RP_Default;

::: dom/workers/WorkerPrivate.cpp
@@ +3569,5 @@
>      mLoadInfo.mCSP->GetAllowsEval(&mLoadInfo.mReportCSPViolations,
>                                    &mLoadInfo.mEvalAllowed);
> +    // Set ReferrerPolicy
> +    bool hasReferrerPolicy;
> +    uint32_t rp;

same here, please init those 2 variables

Comment 67

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1 : Propagate referrer policy to worker

Thanks Chrishtoph.
Updated carry r+

Comment 68

[Thomas Nguyen (:tnguyen)]

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

Comment 69

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1251378 -> 1279494.patch
applying 1279494.patch
patching file netwerk/protocol/http/HttpBaseChannel.cpp
Hunk #1 FAILED at 1412
1 out of 1 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpBaseChannel.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1279494.patch

Comment 70

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1 : Propagate referrer policy to worker

Updated patch. Thanks

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4baef7f68f5
Propagate referrer policy to worker. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbe8d80256c
Use referrer policy of worker instead of default. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/441bc4305490
Update fetch referrer policy test case. r=ehsan

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4baef7f68f5
https://hg.mozilla.org/mozilla-central/rev/7cbe8d80256c
https://hg.mozilla.org/mozilla-central/rev/441bc4305490