Sid Stamm [:geekboy or :sstamm] Reporter
	Description • 10 years ago

We should implement the child-src directive for CSP 1.1.  This may just be an alias to frame-src, but we should check.

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

So the intention is that this is frame-src + worker restrictions, but it's unclear in the spec if the enforcement goes all the way down into the frame tree, or if it's just one level of nesting.  dveditz is bringing this up in the working group, and we'll figure out what to do once it's clearer in the spec.

	Daniel Veditz [:dveditz]
	Comment 2 • 9 years ago

The WG members were clear that the directive only applies to the directly nested context, not all of them -- that is, the explicit <iframe src=> not any frames in frames in that frame. If we have suggestions on clarifying the spec wording we can submit github PRs on it.

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

Kate, thanks for taking on the work.

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 6 • 9 years ago

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

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

Comment on attachment 8654335 [details] [diff] [review]
Implement CSP 1.1 child-src directive

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

Looks great! Only nitpicky stuff! Please do remove all the *.orig files and have 3 patches total for review.
1) all the csp stuff
2) the webidl bits (because we need a special dom-reviewer for those)
3) testcases

I haven't looked at the testcases yet, I will do so in our next iteration!

::: dom/base/nsContentPolicyUtils.h.orig
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* 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/. */
> +

I suppose you wanna delete that file from your patch :-)

::: dom/base/nsContentUtils.cpp
@@ +7965,3 @@
>    case nsIContentPolicy::TYPE_INTERNAL_WORKER:
>    case nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER:
> +    return nsIContentPolicy::TYPE_WORKER;

Even though I would love to have that separation, we have to check with Ehsan if we can do that that easily. Potnetially we have to user the internal_types and pass them through to for CSP policies. I will check with him.

::: dom/base/nsContentUtils.cpp.orig
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* 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/. */
> +

I suppose you wanna remove that file as well :-)

::: dom/base/nsIContentPolicyBase.idl
@@ +273,5 @@
>  
> +  /**
> +   * Indicates a Worker or Shared Worker.
> +   */
> +  const nsContentPolicyType TYPE_WORKER = 35;

You have to ref the uuid of nsIContentPolicy.idl as well as nsIContentPolicyBase.idl. Just do> ./mach uuid

::: dom/base/nsIContentPolicyBase.idl.orig
@@ +2,5 @@
> +/* vim: set ft=cpp tw=78 sw=2 et ts=8 : */
> +/* 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/. */
> +

remove that file.

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +49,5 @@
>    const unsigned short FORM_ACTION_DIRECTIVE          = 14;
>    const unsigned short REFERRER_DIRECTIVE             = 15;
>    const unsigned short WEB_MANIFEST_SRC_DIRECTIVE     = 16;
>    const unsigned short UPGRADE_IF_INSECURE_DIRECTIVE  = 17;
> +  const unsigned short CHILD_SRC_DIRECTIVE            = 18;

please ref the uuid here as well.

::: dom/interfaces/security/nsIContentSecurityPolicy.idl.orig
@@ +1,4 @@
> +/* 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/. */
> +

remove that file

::: dom/security/nsCSPParser.cpp
@@ +997,5 @@
>    }
>  
> +  // child-src has it's own class to handle frame-src if necessary
> +  if (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::CHILD_SRC_DIRECTIVE)) {
> +    mChildSrc = new nsCSPChildSrcDirective(CSP_StringToCSPDirective(mCurToken), false);

you can drop the second argument from the constructor of sCSPChildSrcDirective because it's always going to be false initially, right? just initalize it to false internally within the constructor.

@@ +1003,5 @@
> +  }
> +
> +  // if we have a frame-src, cache it so we can decide whether to use child-src
> +  if (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::FRAME_SRC_DIRECTIVE)) {
> +    mFrameSrc = new nsCSPDirective(CSP_StringToCSPDirective(mCurToken));

please log to the console here that the frame-src directive is deprecated and one should use child-src from now on.

@@ +1102,5 @@
>    }
> +
> +  if (mChildSrc) {
> +    // if we have a child-src, it handles frame-src too, unless frame-src is set
> +    mChildSrc->setHandleFrameSrc(!mFrameSrc);

pretty nifty - i like that.

::: dom/security/nsCSPParser.h
@@ +234,5 @@
>      nsCSPKeywordSrc*   mUnsafeInlineKeywordSrc; // null, otherwise invlidate()
>  
> +    // cache variables for child-src and frame-src directive handling
> +    nsCSPChildSrcDirective* mChildSrc;
> +    nsCSPDirective*         mFrameSrc;

please extend your comment explaining why we need those two cache members.

::: dom/security/nsCSPUtils.cpp
@@ +948,5 @@
> +}
> +
> +/* =============== nsCSPChildSrcDirective ============= */
> +
> +nsCSPChildSrcDirective::nsCSPChildSrcDirective(CSPDirective aDirective, bool aHandleFrameSrc) : nsCSPDirective(aDirective), mHandleFrameSrc(aHandleFrameSrc)

nit: please bring the initialization list to the front;
nsCSPChildSrcDirective::nsCSPChildSrcDirective(CSPDirective aDirective, bool aHandleFrameSrc)
  : nsCSPDirective(aDirective)
  , mHandleFrameSrc(aHandleFrameSrc)

@@ +956,5 @@
> +nsCSPChildSrcDirective::~nsCSPChildSrcDirective()
> +{
> +}
> +
> +void nsCSPChildSrcDirective::setHandleFrameSrc( bool aHandleFrameSrc )

nit: remove spaces

@@ +970,5 @@
> +    case nsIContentPolicy::TYPE_WORKER:
> +      return true;
> +    default:
> +      return false;
> +  }

I would prefer
if (aContentType == nsIContentPolicy::TYPE_SUBDOCUMENT) {
  return mHandleFrameSrc;
}
if (aContentType == nsIContentPolicy::TYPE_WORKER) {
  return true;
}
return false;

but that's really nitpicky, I agree

@@ +982,5 @@
> +    case nsIContentSecurityPolicy::CHILD_SRC_DIRECTIVE:
> +      return true;
> +    default:
> +      return false;
> +  }

same here if you don't mind.

::: dom/security/nsCSPUtils.cpp.orig
@@ +3,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/. */
> +
> +#include "nsCSPUtils.h"

delete

::: dom/security/nsCSPUtils.h
@@ +333,5 @@
> +    explicit nsCSPChildSrcDirective(CSPDirective aDirective, bool aHandleFrameSrc);
> +    virtual ~nsCSPChildSrcDirective();
> +
> +    void setHandleFrameSrc(bool aHandleFrameSrc);
> +    

remove trailing whitespace please.

::: dom/webidl/CSPDictionaries.webidl
@@ +30,1 @@
>  };

before landing you have to move all the dictionaryTOJson related parts into one patch and have bholly r+ it, otherwise you can't land it without proper dom-reviewer in the commit message.

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 10 • 9 years ago

Comment on attachment 8654992 [details] [diff] [review]
Bug 1045891 - Implement CSP 1.1 child-src directive

Updated with feedback.

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

Comment on attachment 8654992 [details] [diff] [review]
Bug 1045891 - Implement CSP 1.1 child-src directive

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

All in all, that already looks pretty good. Please separate code changes from test changes and split into 2 patches. As mentioned in the actual review we should wait till the complete separation between workers and scripts has happenend in Bug 1198078. We then can send intenral content policy types for workers to CSP and perform a mapping of internal content policy types to directives.

::: dom/base/nsContentPolicyUtils.h
@@ +125,5 @@
>      CASE_RETURN( TYPE_INTERNAL_VIDEO          );
>      CASE_RETURN( TYPE_INTERNAL_TRACK          );
>      CASE_RETURN( TYPE_INTERNAL_XMLHTTPREQUEST );
>      CASE_RETURN( TYPE_INTERNAL_EVENTSOURCE    );
> +    CASE_RETURN( TYPE_WORKER                  );

I think we should wait till Bug 1198078 lands, which introduces TYPE_INTERNAL_SERVICE_WORKER, then we would be able to differentiate between:
* TYPE_INTERNAL_SERVICE_WORKER
* TYPE_INTERNAL_WORKER
* TYPE_INTERNAL_SHARED_WORKER
and map to child src accordingly without introducing a new 'eternal' content type.

::: dom/base/nsContentUtils.cpp
@@ +7980,3 @@
>    case nsIContentPolicy::TYPE_INTERNAL_WORKER:
>    case nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER:
> +    return nsIContentPolicy::TYPE_WORKER;

then all of those would still map to script, which I think is the right thing to do.

::: dom/security/nsCSPParser.cpp
@@ +1003,5 @@
> +  }
> +
> +  // if we have a frame-src, cache it so we can decide whether to use child-src
> +  if (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::FRAME_SRC_DIRECTIVE)) {
> +    const char16_t* params[] = { mCurToken.get(), u"child-src" };

NS_LITERAL_STRING("child-src").get()

::: dom/security/nsCSPParser.h
@@ +233,5 @@
>      bool               mHasHashOrNonce; // false, if no hash or nonce is defined
>      nsCSPKeywordSrc*   mUnsafeInlineKeywordSrc; // null, otherwise invlidate()
>  
> +    // cache variables for child-src and frame-src directive handling.
> +    // frame-src is deprecated in favor of child-src, however if we 

nit: delete trailing whitespace

::: dom/security/nsCSPUtils.cpp
@@ +951,5 @@
> +
> +nsCSPChildSrcDirective::nsCSPChildSrcDirective(CSPDirective aDirective)
> +  : nsCSPDirective(aDirective)
> +{
> +  mHandleFrameSrc = false;

move that up in the intialization list:
, mHandleFrameSrc(false)

@@ +960,5 @@
> +}
> +
> +void nsCSPChildSrcDirective::setHandleFrameSrc(bool aHandleFrameSrc)
> +{
> +  mHandleFrameSrc = aHandleFrameSrc;

actually, this should never be allowed to be toggled back to false, right?
maybe make this:
void nsCSPChildSrcDirective::setHandleFrameSrc()
{
  mHandleFrameSrc = true;
}

::: dom/security/test/csp/file_child-src_iframe.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +  <head>
> +    <title>Bug XXXX</title>

substitute XXX with actaul bug number

@@ +17,5 @@
> +      src += "&page_id=" + escape(page_id);
> +      testframe = document.getElementById('testframe');
> +      testframe.src = src;
> +    }
> +    catch (e) { 

nit: trailing whitespace

@@ +23,5 @@
> +        window.parent.postMessage({id:page_id, message:"blocked"}, 'http://mochi.test:8888');
> +      } else {
> +        window.parent.postMessage({id:page_id, message:"exception"}, 'http://mochi.test:8888');
> +      }
> +      console.log(e);

remove console.log

::: dom/security/test/csp/file_child-src_worker.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +  <head>
> +    <title>Bug XXXX</title>

please replace XXX

@@ +13,5 @@
> +      // whether CSP allows or blocks the load.
> +      /*
> +      window.parent.postMessage({id:page_id, message:"allowed"}, 'http://mochi.test:8888');
> +      console.log('posted message for '+page_id);
> +      */

if the above code is not used, please remove.

@@ +17,5 @@
> +      */
> +
> +      worker = new Worker('file_testserver.sjs?file='+escape("tests/dom/security/test/csp/file_child-src_worker.js"));
> +      worker.onmessage = function(ev) {
> +        console.log("got message from worker: "+page_id);

please remove console.log

@@ +22,5 @@
> +        window.parent.postMessage({id:page_id, message:"allowed"}, 'http://mochi.test:8888');
> +      };
> +      worker.postMessage('foo');
> +    }
> +    catch (e) { 

nit: trailing whitespace

::: dom/security/test/csp/file_child-src_worker.js
@@ +1,5 @@
> +onmessage = function(e) {
> +  console.log("worker working");
> +  postMessage('worker');
> +  console.log("worker done!");
> +};

remove console.log and add newline add the end of file

::: dom/security/test/csp/test_child-src_iframe.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug XXXX</title>

replace XXX with bugnumber

@@ +23,5 @@
> +
> +var tests = {
> +  'same-src': {
> +    id: "same-src",
> +    file: "file_child-src_iframe.html",

the file does never change, right? please add a const on top

@@ +71,5 @@
> +function checkFinished() {
> +    if (Object.keys(finished).length == Object.keys(tests).length) {
> +      window.ConnectSrcExaminer.remove();
> +      SimpleTest.finish();
> +    }

Nit: use same indentation throughout the file

@@ +76,5 @@
> +}
> +
> +function recvMessage(ev) {
> +  console.log('got message:');
> +  console.log(ev);

please remove console.log

@@ +90,5 @@
> +// that are blocked by CSP and bubble up the result to the including iframe
> +// document (parent).
> +function examiner() {
> +  SpecialPowers.addObserver(this, "csp-on-violate-policy", false);
> +  SpecialPowers.addObserver(this, "specialpowers-http-notify-request", false);

those observers are not that crisp unforuntately and always cause issues on b2g :-( can you please rewrite and rely on something else to verify that a test succeeds or not?

::: dom/security/test/csp/test_child-src_worker.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug XXXX</title>

fill XXX with number

@@ +23,5 @@
> +
> +var tests = {
> +  'same-src-worker': {
> +    id: "same-src-worker",
> +    file: "file_child-src_worker.html",

the file does not change, right? Please define above:
const TEST_FILE = "file_child-src_worker.html";

@@ +54,5 @@
> +    SimpleTest.finish();
> +  }
> +}
> +
> +window.addEventListener('message', recvMessage, false);

please also remove the message listener right before you call SimpleTest.finish();

@@ +59,5 @@
> +
> +function loadNextTest() {
> +  for (item in tests) {
> +    test = tests[item];
> +    console.log(test);

nit: please remove console.log everywhere.

@@ +82,5 @@
> +loadNextTest();
> +
> +</script>
> +</body>
> +</html>

nit: newline at the end of file

::: dom/security/test/csp/test_connect-src.html
@@ +76,5 @@
>  examiner.prototype  = {
>    observe: function(subject, topic, data) {
> +    console.log(subject);
> +    console.log(topic);
> +    console.log(data);

please remove console.log here as well.

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

Dan, do you think it's worth adding a TYPE_WORKER here [1] that is then also
(1) passed to addons implementing content policies, or should we rather
(2) use our *INTERNAL* types for workers [also 1] and pass them to our CSP implementation.

I am rather for (2), but I would like to hear your opinion too.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl#200

	Daniel Veditz [:dveditz]
	Comment 13 • 9 years ago

In an impromptu discussion we agreed on (2). Changing existing types so that workers were no longer TYPE_SCRIPT might catch other nsIContentPolicy implementations by surprise (e.g., NoScript would need to know about the change, and any add-on like it).

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 14 • 9 years ago

Adding 1048048 as a dependency so that we can use internal types and not add TYPE_WORKER.

	Bobby Holley (:bholley)
	Comment 15 • 9 years ago

Comment on attachment 8654991 [details] [diff] [review]
Bug 1045891 - CSPDictionary change for child-src directive

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

Sorry for the long delay, I was on PTO. Would have been better to get another DOM peer to do this one :-(

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

Comment on attachment 8666937 [details] [diff] [review]
Bug 1045891 - CSP 2 child-src implementation

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

Please address my comments and r=me. As far as the redirect with preloads: You got my green lights to land, but we should find out within a separate bug what we are going to do about redirected preloads.

::: dom/base/nsContentPolicy.cpp
@@ +132,5 @@
>  
>      nsCOMPtr<nsIContentPolicy> cspService =
>        do_GetService(CSPSERVICE_CONTRACTID);
>  
> +    /*

nit: maybe you can resolve that whitespace problem, there shouldn't be any change at this line, right?

::: dom/base/nsContentUtils.cpp
@@ +8046,5 @@
> +  case nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER:
> +  case nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD:
> +  case nsIContentPolicy::TYPE_INTERNAL_IMAGE_PRELOAD:
> +  case nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD:
> +    return aType;

mhm, this is duplicating logic, right? What if we do:

if (aType == InternalContentPolicyTypeToExternalOrWorker(aType) ||
    aType == InternalContentPolicyTypeToExternalOrPreload(aType)) {
  return aType
}
return InternalContentPolicyTypeToExternal(aType);

::: dom/locales/en-US/chrome/security/csp.properties
@@ +83,5 @@
>  # %1$S is the name of the duplicate directive
>  duplicateDirective = Duplicate %1$S directives detected.  All but the first instance will be ignored.
> +# LOCALIZATION NOTE (deprecatedDirective):
> +# %1$S is the name of the deprecated directive, %2$S is the name of the replacement.
> +deprecatedDirective = The %1$S directive has been deprecated. Please use the %2$S directive instead.

I know we aren't super consistent within that file either, but I would like you to use:

deprecatedDirective = Directive '%1$S' has been deprecated, please use directive '%2$S' instead.

::: dom/security/nsCSPContext.cpp
@@ +123,5 @@
>    // Since we know whether we are dealing with a preload, we have to convert
>    // the internal policytype ot the external policy type before moving on.
> +  // We still need to know if this is a worker so child-src can handle that
> +  // case correctly.
> +  aContentType = nsContentUtils::InternalContentPolicyTypeToExternalOrWorker(aContentType);

We potentially need to call InternalContentPolicyTypeToExternalOrCSPInternal - we need to find out.

::: dom/security/nsCSPParser.h
@@ +233,5 @@
>      bool               mHasHashOrNonce; // false, if no hash or nonce is defined
>      nsCSPKeywordSrc*   mUnsafeInlineKeywordSrc; // null, otherwise invlidate()
>  
> +    // cache variables for child-src and frame-src directive handling.
> +    // frame-src is deprecated in favor of child-src, however if we 

nit: trailing whitespace

::: dom/security/nsCSPUtils.cpp
@@ +976,5 @@
> +
> +  if (aContentType == nsIContentPolicy::TYPE_INTERNAL_WORKER
> +      || aContentType == nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER
> +      || aContentType == nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER
> +      ) {

please simplify to

return (aContentType == nsIContentPolicy::TYPE_INTERNAL_WORKER ||
        aContentType == nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER || aContentType == nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER);

@@ +991,5 @@
> +  }
> +
> +  if (aDirective == nsIContentSecurityPolicy::CHILD_SRC_DIRECTIVE){
> +    return true;
> +  }

same here, please simplify:
return (aDirective == nsIContentSecurityPolicy::CHILD_SRC_DIRECTIVE);

::: dom/security/nsCSPUtils.h
@@ +320,5 @@
>  
> +/* =============== nsCSPChildSrcDirective ============= */
> +
> +/*
> + * In CSP 1.1 and 2, the child-src directive covers both workers and

It's only 'CSP 2' - please remove CSP 1.1.

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

Comment on attachment 8666935 [details] [diff] [review]
Bug 1045891 - Tests for child-src

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

Kate, I am seeing 'new Worker()' as well as 'navigator.serviceWorker.register', shouldn't there also be a test for 'new SharedWorker()'?

::: dom/security/test/csp/test_child-src_iframe.html
@@ +97,5 @@
> +    // append the CSP that should be used to serve the file
> +    src += "&csp=" + escape(test.policy);
> +    // add our identifier
> +    src += "#" + escape(test.id);
> +    console.log(src);

please remove console.log here and elsewhere

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 20 • 9 years ago

Carrying over r+ from bholley

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 21 • 9 years ago

Fleshed out tests for:
* Shared & Service workers
* data: urls for Workers and Shared Workers
* behavior under redirects

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 22 • 9 years ago

Carrying over r+ from ckerschb

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

Comment on attachment 8668157 [details] [diff] [review]
CSP 2 child-src implementation

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

::: dom/security/nsCSPService.cpp
@@ +314,5 @@
>    nsCOMPtr<nsIURI> originalUri;
>    rv = oldChannel->GetOriginalURI(getter_AddRefs(originalUri));
>    NS_ENSURE_SUCCESS(rv, rv);
>    nsContentPolicyType policyType =
> +    nsContentUtils::InternalContentPolicyTypeToExternalOrWorker(loadInfo->InternalContentPolicyType());

I think this needs to be:
nsContentUtils::InternalContentPolicyTypeToExternalOrCSPInternal

Can you run that through TRY please (it should work).

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

Comment on attachment 8668156 [details] [diff] [review]
child-src_test.patch

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

Those tests look great, good to see that we have coverage for shared workers, service workers and regular workers. Please note that we also landed csp/test_service_worker.html in the meantime and we have to update script-src to match child-src for that test. r=me with the nits fixed!

::: dom/security/test/csp/file_child-src_shared_worker_data.html
@@ +7,5 @@
> +  <body>
> +  <script type="text/javascript">
> +    var page_id = window.location.hash.substring(1);
> +    var shared_worker = "onconnect = function(e) { var port = e.ports[0]; port.addEventListener('message', function(e) { port.postMessage('success'); }); port.start(); }"
> +    

nit: trailing whitespace

@@ +15,5 @@
> +
> +      worker.port.onmessage = function(ev) {
> +        window.parent.postMessage({id:page_id, message:"allowed"}, 'http://mochi.test:8888');
> +      };
> +      

nit: trailing whitespace

::: dom/security/test/csp/file_redirect_worker.sjs
@@ +1,2 @@
> +// SJS file to serve resources for CSP redirect tests
> +// This file redirects to a specified resource.

nit: please add the bugnumber and mention that this *.sjs file is specifically for that test.

@@ +8,5 @@
> +    query[name] = unescape(value);
> +  });
> +
> +  var thisSite = "http://mochi.test:8888";
> +  var otherSite = "http://example.com";

nit: define thisSite and otherSite as 'const' outside the function's scope.

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 25 • 9 years ago

Carrying over r+

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 26 • 9 years ago

Carried over r+

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 27 • 9 years ago

Carry over r+

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 28 • 9 years ago

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

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 29 • 9 years ago

Treeherder failures are in unrelated code. Windows mochitests don't appear to run, even after a second attempt. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5cd4732b2a)

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

Hi, when applying to mozilla-inbound i got:

remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIContentSecurityPolicy in changeset f3652b110a52
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIContentSecurityPolicy

could you take a look, thanks!

	(no longer active)
	Comment 31 • 9 years ago

Looks like you're getting bitten by bug 1171721.  You'd need to update the uuids of all interfaces in the IDL file just to satisfy this hook.

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 32 • 9 years ago

Updated uuid

	Pulsebot
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/069d6c54f461
https://hg.mozilla.org/integration/mozilla-inbound/rev/26e162e72ae1
https://hg.mozilla.org/integration/mozilla-inbound/rev/895c42544609

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

Hi, sorry had to back this out seems this caused problems like  TEST-UNEXPECTED-PASS | /content-security-policy/blink-contrib/self-doesnt-match-blob.sub.html | Violation report status OK. - expected FAIL  

https://treeherder.mozilla.org/logviewer.html#?job_id=16656765&repo=mozilla-inbound

	Pulsebot
	Comment 35 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb7c98c4814
https://hg.mozilla.org/integration/mozilla-inbound/rev/1953211fa29b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5ceae251b4

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

(In reply to Carsten Book [:Tomcat] from comment #34)
> Hi, sorry had to back this out seems this caused problems like 
> TEST-UNEXPECTED-PASS |
> /content-security-policy/blink-contrib/self-doesnt-match-blob.sub.html |
> Violation report status OK. - expected FAIL  
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=16656765&repo=mozilla-
> inbound

I thought web-platform-tests for CSP are disabled on inbound?
Anyway, Kate, that test is enforcing child-src for workers and was failing because we did not support child-src. You can just delete the whole file [1]. Please make sure there are no other web platform tests we need to update. When I do a grep for "child-src" within testing/web-platform/tests/content-security-policy it appears several times, please make sure all these tests are passing before pushing to inbound again.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/content-security-policy/blink-contrib/self-doesnt-match-blob.sub.html.ini#1

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 37 • 9 years ago

Updating the web platform tests to work with child-src.

	James Graham [:jgraham]
	Comment 39 • 9 years ago

Comment on attachment 8682179 [details] [diff] [review]
Web platform test fixes for child-src

This is fine, but in general you don't actually need files or sections containing only expected: PASS; that's the default. So there isn't a problem here but the files will be trimed a bit on the next update.

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

Comment on attachment 8682179 [details] [diff] [review]
Web platform test fixes for child-src

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

Yeah, no need to have *.ini fails that only include 'PASS' as that's the default :-) But since James offered to fix that up with the next upstream, that's fine with me. Thanks James.

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 41 • 9 years ago

Carry over r+

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 42 • 9 years ago

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

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 43 • 9 years ago

carry over r+ from previous reviews

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 44 • 9 years ago

Web platform tests passing

	Pulsebot
	Comment 45 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/781a76befe01
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44d41985fed
https://hg.mozilla.org/integration/mozilla-inbound/rev/14818a2329a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c590b18c5885

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 46 • 9 years ago

I had to back these out for b2g opt mochitest 7 failures: https://treeherder.mozilla.org/logviewer.html#?job_id=16927684&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2e821fa20d

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 47 • 9 years ago

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

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 48 • 9 years ago

Per email conversation with Christoph and Steve, disabling the failing b2g test. It is highly likely that the offending test requires SSL since it deals with service workers.

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

Hi, 

the CSP child-src tests patch failed to apply:

A patch file named '%s' is already applied.
Enter the new patch name (old one was 'file_1045891.txt'): patch-1b.patch
renamed 1045891 -> patch-1b.patch
applying patch-1b.patch
patching file dom/security/test/csp/mochitest.ini
Hunk #2 FAILED at 208
1 out of 2 hunks FAILED -- saving rejects to file dom/security/test/csp/mochitest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh patch-1b.patch

could you take a look, thanks!

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

hm the rej file is :

--- mochitest.ini
+++ mochitest.ini
@@ -196,8 +209,13 @@ skip-if = buildapp == 'b2g' || buildapp
 skip-if = buildapp == 'b2g' || buildapp == 'mulet' || toolkit == 'gonk' || toolkit == 'android'
 [test_upgrade_insecure_cors.html]
 skip-if = buildapp == 'b2g' || buildapp == 'mulet' || toolkit == 'gonk' || toolkit == 'android'
 [test_report_for_import.html]
 [test_blocked_uri_in_reports.html]
 skip-if = e10s || buildapp == 'b2g' # http-on-opening-request observer not supported in child process (bug 1009632)
 [test_service_worker.html]
 skip-if = buildapp == 'b2g' #no ssl support
+[test_child-src_worker.html]
+skip-if = buildapp == 'b2g' #investigate in bug 1222904
+[test_child-src_worker_data.html]
+[test_child-src_worker-redirect.html]
+[test_child-src_iframe.html]

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 51 • 9 years ago

Updated patch to apply cleanly to mozilla-inbound. 

Carrying over r+ from previous.

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

still fails to apply :(

rej file:

--- mochitest.ini
+++ mochitest.ini
@@ -195,8 +208,13 @@ skip-if = buildapp == 'b2g' || buildapp
 [test_upgrade_insecure_referrer.html]
 skip-if = buildapp == 'b2g' || buildapp == 'mulet' || toolkit == 'gonk' || toolkit == 'android'
 [test_upgrade_insecure_cors.html]
 skip-if = buildapp == 'b2g' || buildapp == 'mulet' || toolkit == 'gonk' || toolkit == 'android'
 [test_report_for_import.html]
 [test_blocked_uri_in_reports.html]
 [test_service_worker.html]
 skip-if = buildapp == 'b2g' #no ssl support
+[test_child-src_worker.html]
+skip-if = buildapp == 'b2g' #investigate in bug 1222904
+[test_child-src_worker_data.html]
+[test_child-src_worker-redirect.html]
+[test_child-src_iframe.html]
~

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 53 • 9 years ago

Rebased onto -inbound

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

Kate, it seems you accidentally deleted the actual test files when rebasing the tests:

 0:04.54     The error occurred when validating the result of the execution. The reported error is:
 0:04.54 
 0:04.54         Test manifest (/Users/ckerschb/Documents/moz/inbound/dom/security/test/csp/mochitest.ini) lists test that does not exist: test_child-src_worker.html, test_child-src_worker_data.html, test_child-src_worker-redirect.html, test_child-src_iframe.html

	Kate McKinley [:kmckinley, :Kate] Assignee
	Comment 55 • 9 years ago

Fix bad export

	Pulsebot
	Comment 56 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/58d4c3876d0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca475a0254a
https://hg.mozilla.org/integration/mozilla-inbound/rev/459d282e48ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/a735ff65e239

	Carsten Book [:Tomcat]
	Comment 57 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/58d4c3876d0c
https://hg.mozilla.org/mozilla-central/rev/4ca475a0254a
https://hg.mozilla.org/mozilla-central/rev/459d282e48ab
https://hg.mozilla.org/mozilla-central/rev/a735ff65e239

	Kohei Yoshino [:kohei]
	Comment 58 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/csp-directive-frame-src-has-been-deprecated-in-favor-of-child-src/

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

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives#child-src
and
https://developer.mozilla.org/en-US/Firefox/Releases/45#Security