Until CSP fully supports paths, we should log a warning if we encounter a source...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Aug 2012 17:24:41 +0000 (17:24 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Aug 2012 17:24:41 +0000 (17:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=93468

Patch by Mike West <mkwst@chromium.org> on 2012-08-08
Reviewed by Adam Barth.

Source/WebCore:

CSP 1.0 ignores path components of sources in directives' source lists.
'script-src http://example.com/path/to/directory' is treated exactly the
same as 'script-src http://example.com'. It's likely that this behavior
will change in CSP 1.1, which could break with developers' expectations.
This patch adds a warning when a path is encountered, alerting
developers to the fact that their current source is interpreted
differently than they might expect.

See http://crbug.com/128493 for additional context and discussion.

Tests for this change are covered by updating the existing Content
Security Policy tests to include the new console warnings.

* page/ContentSecurityPolicy.cpp:
(CSPSourceList):
(WebCore::CSPSourceList::CSPSourceList):
    Passing the directive name down into CSPSourceList so that we can
    generate informative error messages.
(WebCore::CSPSourceList::parse):
    Create a 'path' string, pass it into 'parseSource', and use it after
    parsing each source to determine whether a warning should be sent.
(WebCore::CSPSourceList::parseSource):
    Adding a 'path' argument so that we can see whether or not a
    specific source should generate a warning.
(WebCore::CSPDirective::CSPDirective):
    Passing the directive name down into CSPSourceList so that we can
    generate informative error messages.
(WebCore::ContentSecurityPolicy::reportIgnoredPathComponent):
    Generate the new warning message.
(WebCore):
* page/ContentSecurityPolicy.h:

LayoutTests:

* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowconnectionto.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowfontfrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowframefrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowimagefrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowmediafrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowobjectfrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowscriptfrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowstylefrom.html:
* http/tests/security/contentSecurityPolicy/1.1/securitypolicy-reporturi.html:
    All of the SecurityPolicy API tests with source lists contained
    sources with a trailing `/`. I've fixed that oversight.
* http/tests/security/contentSecurityPolicy/source-list-parsing-05-expected.txt:
* http/tests/security/contentSecurityPolicy/source-list-parsing-05.html:
* http/tests/security/contentSecurityPolicy/source-list-parsing-06-expected.txt:
* http/tests/security/contentSecurityPolicy/source-list-parsing-06.html:
    Updating the text of the test, and updating them to include the new
    console warnings.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@125047 268f45cc-cd09-0410-ab3c-d52691b4dbfc

17 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowconnectionto.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowfontfrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowframefrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowimagefrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowmediafrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowobjectfrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowscriptfrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowstylefrom.html
LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-reporturi.html
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-05-expected.txt
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-05.html
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-06-expected.txt
LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-06.html
Source/WebCore/ChangeLog
Source/WebCore/page/ContentSecurityPolicy.cpp
Source/WebCore/page/ContentSecurityPolicy.h

index 194fd80..90038ca 100644 (file)
@@ -1,3 +1,28 @@
+2012-08-08  Mike West  <mkwst@chromium.org>
+
+        Until CSP fully supports paths, we should log a warning if we encounter a source with a path.
+        https://bugs.webkit.org/show_bug.cgi?id=93468
+
+        Reviewed by Adam Barth.
+
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowconnectionto.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowfontfrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowframefrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowimagefrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowmediafrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowobjectfrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowscriptfrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-allowstylefrom.html:
+        * http/tests/security/contentSecurityPolicy/1.1/securitypolicy-reporturi.html:
+            All of the SecurityPolicy API tests with source lists contained
+            sources with a trailing `/`. I've fixed that oversight.
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-05-expected.txt:
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-05.html:
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-06-expected.txt:
+        * http/tests/security/contentSecurityPolicy/source-list-parsing-06.html:
+            Updating the text of the test, and updating them to include the new
+            console warnings.
+
 2012-08-08  Pavel Feldman  <pfeldman@chromium.org>
 
         Web Inspector: generate preview for the objects dumped into the console upon logging.
index 8d77332..9e33751 100644 (file)
@@ -9,7 +9,7 @@
           log('FAIL connection is not allowed when no policy exists.');
 
 
-      injectPolicy("connect-src http://notexample.com/;");
+      injectPolicy("connect-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsConnectionTo('http://example.com/'))
           log('PASS connection is not allowed when policy exists.');
index e65eb0c..6594603 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL font is not allowed when no policy exists.');
 
-      injectPolicy("font-src http://notexample.com/;");
+      injectPolicy("font-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsFontFrom('http://example.com/'))
           log('PASS font is not allowed when policy exists.');
index 161aa59..fc31721 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL frame is not allowed when no policy exists.');
 
-      injectPolicy("frame-src http://notexample.com/;");
+      injectPolicy("frame-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsFrameFrom('http://example.com/'))
           log('PASS frame is not allowed when policy exists.');
index 36efb50..ad3b297 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL image is not allowed when no policy exists.');
 
-      injectPolicy("img-src http://notexample.com/;");
+      injectPolicy("img-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsImageFrom('http://example.com/'))
           log('PASS image is not allowed when policy exists.');
index 85d0e23..899b333 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL media is not allowed when no policy exists.');
 
-      injectPolicy("media-src http://notexample.com/;");
+      injectPolicy("media-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsMediaFrom('http://example.com/'))
           log('PASS media is not allowed when policy exists.');
index 6ca694c..cffd083 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL object is not allowed when no policy exists.');
 
-      injectPolicy("object-src http://notexample.com/;");
+      injectPolicy("object-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsObjectFrom('http://example.com/'))
           log('PASS object is not allowed when policy exists.');
index def1055..9577192 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL script is not allowed when no policy exists.');
 
-      injectPolicy("script-src http://notexample.com/;");
+      injectPolicy("script-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsScriptFrom('http://example.com/'))
           log('PASS script is not allowed when policy exists.');
index 79be02f..c311080 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL style is not allowed when no policy exists.');
 
-      injectPolicy("style-src http://notexample.com/;");
+      injectPolicy("style-src http://notexample.com;");
 
       if (!document.SecurityPolicy.allowsStyleFrom('http://example.com/'))
           log('PASS style is not allowed when policy exists.');
index 4236a01..064694a 100644 (file)
@@ -8,7 +8,7 @@
       else
           log('FAIL document.SecurityPolicy.reportURIs has length ' + document.SecurityPolicy.reportURIs.length + ' when no policy exists.');
 
-      injectPolicy('report-uri http://example.com/');
+      injectPolicy('report-uri http://example.com');
 
       if (document.SecurityPolicy.reportURIs.length === 1)
           log('PASS document.SecurityPolicy.reportURIs has length 1 when policy exists.');
index e6ca694..293bc83 100644 (file)
@@ -1,6 +1,16 @@
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:*/'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:*/path'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:*/path?query=string'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path?query=string' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:*/path#anchor'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path#anchor' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/path'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/path?query=string'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path?query=string' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/path#anchor'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path#anchor' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/thisisa'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/thisisa' is being ignored. Be careful.
 CONSOLE MESSAGE: Unrecognized Content-Security-Policy directive 'pathwithasemicolon'.
 
-Paths should be ignored when evaluating sources.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source '127.0.0.1:8000/this'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/this' is being ignored. Be careful.
+Paths should be ignored when evaluating sources. This test passes if FAIL does not appear in the output, and each of the tests generates a warning about the path component.
 
 
 
index 997d86e..1c6e392 100644 (file)
@@ -19,5 +19,7 @@ var tests = [
 </head>
 <body onload="test()">
   <p>
-    Paths should be ignored when evaluating sources.
+    Paths should be ignored when evaluating sources. This test passes if FAIL
+    does not appear in the output, and each of the tests generates a warning
+    about the path component.
   </p>
index e6ca694..38fd9be 100644 (file)
@@ -1,6 +1,16 @@
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:*/'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:*/path'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:*/path?query=string'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path?query=string' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:*/path#anchor'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path#anchor' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/path'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/path?query=string'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path?query=string' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/path#anchor'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/path#anchor' is being ignored. Be careful.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/thisisa'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/thisisa' is being ignored. Be careful.
 CONSOLE MESSAGE: Unrecognized Content-Security-Policy directive 'pathwithasemicolon'.
 
-Paths should be ignored when evaluating sources.
+CONSOLE MESSAGE: The source list for Content Security Policy directive 'script-src' contains the source 'http://127.0.0.1:8000/this'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '/this' is being ignored. Be careful.
+Paths should be ignored when evaluating sources. This test passes if FAIL does not appear in the output, and each of the tests generates a warning about the path component.
 
 
 
index 4314252..3e47cbb 100644 (file)
@@ -19,5 +19,7 @@ var tests = [
 </head>
 <body onload="test()">
   <p>
-    Paths should be ignored when evaluating sources.
+    Paths should be ignored when evaluating sources. This test passes if FAIL
+    does not appear in the output, and each of the tests generates a warning
+    about the path component.
   </p>
index ac98054..8a3ead1 100644 (file)
@@ -1,3 +1,42 @@
+2012-08-08  Mike West  <mkwst@chromium.org>
+
+        Until CSP fully supports paths, we should log a warning if we encounter a source with a path.
+        https://bugs.webkit.org/show_bug.cgi?id=93468
+
+        Reviewed by Adam Barth.
+
+        CSP 1.0 ignores path components of sources in directives' source lists.
+        'script-src http://example.com/path/to/directory' is treated exactly the
+        same as 'script-src http://example.com'. It's likely that this behavior
+        will change in CSP 1.1, which could break with developers' expectations.
+        This patch adds a warning when a path is encountered, alerting
+        developers to the fact that their current source is interpreted
+        differently than they might expect.
+
+        See http://crbug.com/128493 for additional context and discussion.
+
+        Tests for this change are covered by updating the existing Content
+        Security Policy tests to include the new console warnings.
+
+        * page/ContentSecurityPolicy.cpp:
+        (CSPSourceList):
+        (WebCore::CSPSourceList::CSPSourceList):
+            Passing the directive name down into CSPSourceList so that we can
+            generate informative error messages.
+        (WebCore::CSPSourceList::parse):
+            Create a 'path' string, pass it into 'parseSource', and use it after
+            parsing each source to determine whether a warning should be sent.
+        (WebCore::CSPSourceList::parseSource):
+            Adding a 'path' argument so that we can see whether or not a
+            specific source should generate a warning.
+        (WebCore::CSPDirective::CSPDirective):
+            Passing the directive name down into CSPSourceList so that we can
+            generate informative error messages.
+        (WebCore::ContentSecurityPolicy::reportIgnoredPathComponent):
+            Generate the new warning message.
+        (WebCore):
+        * page/ContentSecurityPolicy.h:
+
 2012-08-08  Pavel Feldman  <pfeldman@chromium.org>
 
         Web Inspector: generate preview for the objects dumped into the console upon logging.
index 80b7da1..a0a6e24 100644 (file)
@@ -185,7 +185,7 @@ private:
 
 class CSPSourceList {
 public:
-    explicit CSPSourceList(ContentSecurityPolicy*);
+    CSPSourceList(ContentSecurityPolicy*, const String& directiveName);
 
     void parse(const String&);
     bool matches(const KURL&);
@@ -195,7 +195,7 @@ public:
 private:
     void parse(const UChar* begin, const UChar* end);
 
-    bool parseSource(const UChar* begin, const UChar* end, String& scheme, String& host, int& port, bool& hostHasWildcard, bool& portHasWildcard);
+    bool parseSource(const UChar* begin, const UChar* end, String& scheme, String& host, int& port, String& path, bool& hostHasWildcard, bool& portHasWildcard);
     bool parseScheme(const UChar* begin, const UChar* end, String& scheme);
     bool parseHost(const UChar* begin, const UChar* end, String& host, bool& hostHasWildcard);
     bool parsePort(const UChar* begin, const UChar* end, int& port, bool& portHasWildcard);
@@ -208,13 +208,15 @@ private:
 
     ContentSecurityPolicy* m_policy;
     Vector<CSPSource> m_list;
+    String m_directiveName;
     bool m_allowStar;
     bool m_allowInline;
     bool m_allowEval;
 };
 
-CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy)
+CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String& directiveName)
     : m_policy(policy)
+    , m_directiveName(directiveName)
     , m_allowStar(false)
     , m_allowInline(false)
     , m_allowEval(false)
@@ -256,14 +258,16 @@ void CSPSourceList::parse(const UChar* begin, const UChar* end)
             return; // We represent 'none' as an empty m_list.
         isFirstSourceInList = false;
 
-        String scheme, host;
+        String scheme, host, path;
         int port = 0;
         bool hostHasWildcard = false;
         bool portHasWildcard = false;
 
-        if (parseSource(beginSource, position, scheme, host, port, hostHasWildcard, portHasWildcard)) {
+        if (parseSource(beginSource, position, scheme, host, port, path, hostHasWildcard, portHasWildcard)) {
             if (scheme.isEmpty())
                 scheme = m_policy->securityOrigin()->protocol();
+            if (!path.isEmpty())
+                m_policy->reportIgnoredPathComponent(m_directiveName, String(beginSource, position - beginSource), path);
             m_list.append(CSPSource(scheme, host, port, hostHasWildcard, portHasWildcard));
         }
 
@@ -276,11 +280,9 @@ void CSPSourceList::parse(const UChar* begin, const UChar* end)
 //                   / "'self'"
 //
 bool CSPSourceList::parseSource(const UChar* begin, const UChar* end,
-                                String& scheme, String& host, int& port,
+                                String& scheme, String& host, int& port, String& path,
                                 bool& hostHasWildcard, bool& portHasWildcard)
 {
-    String path; // FIXME: We're ignoring the path component for now.
-
     if (begin == end)
         return false;
 
@@ -517,7 +519,7 @@ void CSPSourceList::addSourceUnsafeEval()
 class CSPDirective {
 public:
     CSPDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
-        : m_sourceList(policy)
+        : m_sourceList(policy, name)
         , m_text(name + ' ' + value)
         , m_selfURL(policy->url())
     {
@@ -1295,6 +1297,12 @@ void ContentSecurityPolicy::reportInvalidNonce(const String& nonce) const
     logToConsole(message);
 }
 
+void ContentSecurityPolicy::reportIgnoredPathComponent(const String& directiveName, const String& completeSource, const String& path) const
+{
+    String message = makeString("The source list for Content Security Policy directive '", directiveName, "' contains the source '", completeSource, "'. Content Security Policy 1.0 supports only schemes, hosts, and ports. Paths might be supported in the future, but for now, '", path, "' is being ignored. Be careful.");
+    logToConsole(message);
+}
+
 void ContentSecurityPolicy::logToConsole(const String& message, const String& contextURL, const WTF::OrdinalNumber& contextLine, PassRefPtr<ScriptCallStack> callStack) const
 {
     m_scriptExecutionContext->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, contextURL, contextLine.oneBasedInt(), callStack);
index 312ec51..a6b6027 100644 (file)
@@ -98,6 +98,7 @@ public:
 
     void reportDuplicateDirective(const String&) const;
     void reportInvalidNonce(const String&) const;
+    void reportIgnoredPathComponent(const String& directiveName, const String& completeSource, const String& path) const;
     void reportUnrecognizedDirective(const String&) const;
     void reportViolation(const String& directiveText, const String& consoleMessage, const KURL& blockedURL, const Vector<KURL>& reportURIs, const String& header, const String& contextURL = String(), const WTF::OrdinalNumber& contextLine = WTF::OrdinalNumber::beforeFirst(), PassRefPtr<ScriptCallStack> = 0) const;