STR 1- Decompress the attached zip into a folder 2- Run a HTTP server for that folder 3- Observe the console. (See attachment) Expected: As there is no active CSP policy in the active page, the should be no active CSP policy in the worker. Actual: CSP is being applied on the worker.

Look at the bottom of the console to see the CSP warning. The same eval() is in the index.html but there it's not beend prevented.

You can use python2 -m SimpleHTTPServer or python3 -m http.server to start a simple HTTP server.

I'll debug here a bit and let you know guys about my findings.

Bent, after some debugging it seems `eval()` function is not allowed by default on workers (even without any CSP policy at all) (see [1]]. Could you confirm that please? Thanks. [1] https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#2237

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4) > [1] > https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate. > cpp#2237 Sorry for the confusion, the code certainly looks like we set that to false by default, but we change its value here in the absence of CSP: https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5035

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #5) Thanks for the answer Ben! > Sorry for the confusion, the code certainly looks like we set that to false > by default, but we change its value here in the absence of CSP: > https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate. > cpp#5035 This is weird. That piece of code is never run as I have seen and moreover there is such report (CSP violation one). I'll keep trying to figure out what's going here. Thanks.

It seems to be specifically related to eval() function. Setting dependency with v2 for tracking purposes, please change it if needed.

These bits fix the issue. I'm not totally sure that's the proper fix. Let's see.

Nikhil, the test in this file always passes and it shouldn't unless the part 1 patch is applied. If the part 1 patch is not applied the test passes too but I see this: [65410] WARNING: '!aInstallEventSuccess', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 966 [65410] WARNING: 'NS_FAILED(aStatus)', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 144 [65410] WARNING: ServiceWorkerJob failed with error: NS_ERROR_DOM_ABORT_ERR I guess the navigator.serviceWorker.register() returned promise shouldn't resolve. If the part 1 patch is applied the error log above is gone. The test still passes. So I see here a couple of issue. First one is the eval thing. When creating the service worker at ServiceWorkerManager::CreateServiceWorker() we use our own WorkerLoadInfo so -by default- eval is not allowed. Following the code we never end up calling WorkerPrivate::GetLoadInfo() -as we use our own load info- when the code sets the member mEvalAllowed true in case no CSP policy present. I fixed this issue by setting mEvalAllowed true in the load info we create but I'm not sure if this is a proper fix. Let me know please. The second issue is the test always passing. I guess that shouldn't happen. Let me know how to deal with this. Should we file a bug for it?

Comment on attachment 8614183 [details] [diff] [review] Part 2: mochitest usefull for debugging the issue Review of attachment 8614183 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I forgot about this feedback request. register() is not affected by the install event result since 'install' event is sent to the SW independently of the register() call. If you want to test that eval is allowed in this test you should change the worker script to run eval at the top level instead of in an event handler.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #10) > Comment on attachment 8614183 [details] [diff] [review] > Part 2: mochitest usefull for debugging the issue > > Review of attachment 8614183 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for the feedback! > register() is not affected by the install event result since 'install' event > is sent to the SW independently of the register() call. Oh, I see. Understood. > If you want to test > that eval is allowed in this test you should change the worker script to run > eval at the top level instead of in an event handler. That worked, thanks.

Bug 1160458 - CSP activated by default in Service Workers. r=nsm,bent

Bug 1160458 - Test

Nikhil, Ben, I asked review at both of you guys because this is related to service workers and the worker load info for the service worker. The fix is quite trivial and might be wrong and this could end r-'ed so if so let me know how to continue please. Thanks!

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm Bug 1160458 - Test

Comment on attachment 8617892 [details] MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm https://reviewboard.mozilla.org/r/10727/#review9439 ::: dom/workers/ServiceWorkerManager.cpp:3418 (Diff revision 1) > + info.mEvalAllowed = true; > + info.mReportCSPViolations = false; > + Just use the CSP of the principal passed to CreateServiceWorker.

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm https://reviewboard.mozilla.org/r/10729/#review9441 Please also add another test where the registering page enforces a CSP that prevents eval and ensure that registration fails in that case due to EvalError. ::: dom/workers/test/serviceworkers/csp_worker.js:3 (Diff revision 1) > +self.addEventListener('install', evt => { remove this.

Comment on attachment 8617892 [details] MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm Bug 1160458 - CSP activated by default in Service Workers. r=nsm,bent

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm Bug 1160458 - Test

Comment on attachment 8617892 [details] MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm Bug 1160458 - Part 2: Test. r=nsm

Comment on attachment 8617892 [details] MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm https://reviewboard.mozilla.org/r/10727/#review9767 ::: dom/workers/ServiceWorkerManager.cpp:3593 (Diff revision 2) > + NS_ENSURE_SUCCESS(rv, rv); NS_WARN_IF form.

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm https://reviewboard.mozilla.org/r/10729/#review9769 ::: dom/workers/test/serviceworkers/test_eval_allowed.html:17 (Diff revision 2) > + var registration; This global can be removed. ::: dom/workers/test/serviceworkers/test_eval_allowed.html:21 (Diff revision 2) > + registration = swr; Instead of setting the registration, just return the Promise, so return navigator.serviceWorker.register("eval_worker.js"); ::: dom/workers/test/serviceworkers/test_eval_allowed.html:28 (Diff revision 2) > + ok(true, "eval in service worker script didn't cause an issue"); And then you can directly access the registration here since this is chained to register() ::: dom/workers/test/serviceworkers/test_eval_allowed.html:40 (Diff revision 2) > + ["dom.messageChannel.enabled", true], This pref isn't required. ::: dom/workers/test/serviceworkers/test_eval_not_allowed.html:23 (Diff revision 2) > + .catch(function() { There should also be a then() handler that fails the test. Without that this will just timeout. ::: dom/workers/test/serviceworkers/test_eval_not_allowed.html:35 (Diff revision 2) > + ["dom.messageChannel.enabled", true], This can be removed.

Comment on attachment 8617892 [details] MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Comment on attachment 8617893 [details] MozReview Request: Bug 1160458 - Part 2: Test. r=nsm Bug 1160458 - Part 2: Test. r=nsm

hi, part 1 failed to apply: (eg '1-3,5', or 's' to toggle the sort order between id & patch description) 1 adding 1160458 to series file renamed 1160458 -> 1160458.patch applying 1160458.patch patching file dom/workers/ServiceWorkerManager.cpp Hunk #1 FAILED at 2960 1 out of 1 hunks FAILED -- saving rejects to file dom/workers/ServiceWorkerManager.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 1160458.patch

Talked to Carsten on IRC. The patch applies cleanly, he will try again. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8614013c4ee4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd17d4e677a

https://hg.mozilla.org/mozilla-central/rev/8614013c4ee4 https://hg.mozilla.org/mozilla-central/rev/1fd17d4e677a

As NGA Program Manager suggested, let's replace the NGA-X milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed