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

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.

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.

Kate, thanks for taking on the work.

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.

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

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.

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

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).

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

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 :-(

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.

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

Carrying over r+ from bholley

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

Carrying over r+ from ckerschb

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).

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.

Carrying over r+

Carried over r+

Carry over r+

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)

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!

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.

Updated uuid

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

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

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

(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

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

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.

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.

Carry over r+

carry over r+ from previous reviews

Web platform tests passing

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

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

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.

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!

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]

Updated patch to apply cleanly to mozilla-inbound. Carrying over r+ from previous.

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] ~

Rebased onto -inbound

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

Fix bad export

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

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

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/

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