CSP: Move 'worker-src' onto 'script-src'

third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp

Index: third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
index d501f9bbe8cf94dc2ef1ebf0f6b6d96467255f76..1b173c3f8daeaea5f7ad99e10ae9bb34c9fc0219 100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
@@ -769,19 +769,36 @@ bool CSPDirectiveList::allowWorkerFromSource(
     const KURL& url,
     ResourceRequest::RedirectStatus redirectStatus,
     ContentSecurityPolicy::ReportingStatus reportingStatus) const {
-  // 'worker-src' overrides 'child-src', which overrides the default
+  // 'worker-src' overrides 'script-src', which overrides the default
   // sources. So, we do this nested set of calls to 'operativeDirective()' to
-  // grab 'worker-src' if it exists, 'child-src' if it doesn't, and 'defaut-src'
+  // grab 'worker-src' if it exists, 'script-src' if it doesn't, and
+  // 'defaut-src'

[estark, 2016/11/29 22:01:16]

nit: wrapping is weird

[Mike West, 2016/11/30 12:34:30]

I blame clang format. :)

   // if neither are available.
-  SourceListDirective* whichDirective = operativeDirective(
-      m_workerSrc.get(), operativeDirective(m_childSrc.get()));
+  SourceListDirective* workerSrc = operativeDirective(
+      m_workerSrc.get(), operativeDirective(m_scriptSrc.get()));
+
+  // Workers used to be controlled via 'child-src'; for the moment, we'll check
+  // 'child-src' if 'worker-src' is not present, and a check against
+  // 'script-src'
+  // fails (e.g. we'll block 'https://example.com/worker' given

[estark, 2016/11/29 22:01:16]

nit: unclosed parenthesis *twitch*

But more importantly, I can't parse this comment and can't seem to figure out
exactly what's going on and why. Is the goal to prevent breakage by allowing
workers that are prohibited by script-src if they're allowed by child-src?

[Mike West, 2016/11/30 12:34:30]

I've rewritten it in the hopes of being a little clearer about what I'm
planning.

+  // "worker-src 'none'" or "worker-src 'none'; child-src https://example.com",
+  // but we'll allow it given
+  // "script-src https://not-example.com; child-src https://example.com".
+  //
+  // TODO(mkwst): Remove this once other vendors follow suit.
+  // http://crbug.com/662930

[estark, 2016/11/29 22:01:16]

nit: https

[Mike West, 2016/11/30 12:34:30]

Arg!

+  if (!checkSource(workerSrc, url, redirectStatus) && !m_workerSrc) {

[Mike West, 2016/11/29 17:05:14]

Bah. This should include `&& m_childSrc`. :(

[estark, 2016/11/29 22:01:16]

And shouldn't call operativeDirective on line 791?

[Mike West, 2016/11/30 12:34:30]

Hrm. Yeah, I think you're right.

+    SourceListDirective* childSrc = operativeDirective(m_childSrc.get());
+    if (checkSource(childSrc, url, redirectStatus))
+      return true;
+  }
 
   return reportingStatus == ContentSecurityPolicy::SendReport
              ? checkSourceAndReportViolation(
-                   whichDirective, url,
+                   workerSrc, url,
                    ContentSecurityPolicy::DirectiveType::WorkerSrc,
                    redirectStatus)
-             : checkSource(whichDirective, url, redirectStatus);
+             : checkSource(workerSrc, url, redirectStatus);
 }
 
 bool CSPDirectiveList::allowAncestors(
@@ -1138,8 +1155,7 @@ void CSPDirectiveList::addDirective(const String& name, const String& value) {
     setCSPDirective<SourceListDirective>(name, value, m_baseURI);
   } else if (type == ContentSecurityPolicy::DirectiveType::ChildSrc) {
     setCSPDirective<SourceListDirective>(name, value, m_childSrc);
-  } else if (type == ContentSecurityPolicy::DirectiveType::WorkerSrc &&
-             m_policy->experimentalFeaturesEnabled()) {

[estark, 2016/11/29 22:01:16]

Was this an intentional change? Not clear why we don't need the experimental
flag here anymore

[Mike West, 2016/11/30 12:34:30]

Yes. This is something I want to ship (hence going back to blink-dev with an
update to the intent to ship).

[estark, 2016/12/01 05:16:56]

Oh, sorry, missed that! Makes sense now

+  } else if (type == ContentSecurityPolicy::DirectiveType::WorkerSrc) {
     setCSPDirective<SourceListDirective>(name, value, m_workerSrc);
   } else if (type == ContentSecurityPolicy::DirectiveType::FormAction) {
     setCSPDirective<SourceListDirective>(name, value, m_formAction);
@@ -1195,14 +1211,14 @@ SourceListDirective* CSPDirectiveList::operativeDirective(
       return operativeDirective(m_scriptSrc.get());
     case ContentSecurityPolicy::DirectiveType::StyleSrc:
       return operativeDirective(m_styleSrc.get());
-    // Directives that default to child-src, which defaults to default-src.
+    // Directives that default to 'child-src' (which defaults to 'default-src')
     case ContentSecurityPolicy::DirectiveType::FrameSrc:
       return operativeDirective(m_frameSrc,
                                 operativeDirective(m_childSrc.get()));
-    // TODO(mkwst): Reevaluate this
+    // Directives that default to 'script-src' (which defaults to 'default-src')
     case ContentSecurityPolicy::DirectiveType::WorkerSrc:
       return operativeDirective(m_workerSrc.get(),
-                                operativeDirective(m_childSrc.get()));
+                                operativeDirective(m_scriptSrc.get()));
     default:
       return nullptr;
   }