Bug 843311 - update CSP report-uri parsing to be spec compliant. r=grobinson
authorSid Stamm <sstamm@mozilla.com>
Fri, 24 Jan 2014 10:24:08 -0800
changeset 181099 84802a40e62d5db5f9ca3225170a48828cd83570
parent 181098 ccbec5ed3ce7cd7429c4e0210dc474938e9bb064
child 181100 fc6dc7e3ae7cc41cab396d91c4fc85664cf9f19a
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgrobinson
bugs843311
milestone29.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 843311 - update CSP report-uri parsing to be spec compliant. r=grobinson
content/base/src/CSPUtils.jsm
content/base/test/unit/test_csputils.js
dom/locales/en-US/chrome/security/csp.properties
--- a/content/base/src/CSPUtils.jsm
+++ b/content/base/src/CSPUtils.jsm
@@ -381,42 +381,27 @@ CSPRep.fromString = function(aStr, self,
       for (let i in uriStrings) {
         var uri = null;
         try {
           // Relative URIs are okay, but to ensure we send the reports to the
           // right spot, the relative URIs are expanded here during parsing.
           // The resulting CSPRep instance will have only absolute URIs.
           uri = gIoService.newURI(uriStrings[i],null,selfUri);
 
-          // if there's no host, don't do the ETLD+ check.  This will throw
-          // NS_ERROR_FAILURE if the URI doesn't have a host, causing a parse
-          // failure.
+          // if there's no host, this will throw NS_ERROR_FAILURE, causing a
+          // parse failure.
           uri.host;
 
-          // Verify that each report URI is in the same etld + 1 and that the
-          // scheme and port match "self" if "self" is defined, and just that
-          // it's valid otherwise.
-          if (self) {
-            if (gETLDService.getBaseDomain(uri) !==
-                gETLDService.getBaseDomain(selfUri)) {
-              cspWarn(aCSPR,
-                      CSPLocalizer.getFormatStr("reportURInotETLDPlus1",
-                                                [gETLDService.getBaseDomain(uri)]));
-              continue;
-            }
-            if (!uri.schemeIs(selfUri.scheme)) {
-              cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotSameSchemeAsSelf",
-                                                       [uri.asciiSpec]));
-              continue;
-            }
-            if (uri.port && uri.port !== selfUri.port) {
-              cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotSamePortAsSelf",
-                                                       [uri.asciiSpec]));
-              continue;
-            }
+          // warn about, but do not prohibit non-http and non-https schemes for
+          // reporting URIs.  The spec allows unrestricted URIs resolved
+          // relative to "self", but we should let devs know if the scheme is
+          // abnormal and may fail a POST.
+          if (!uri.schemeIs("http") && !uri.schemeIs("https")) {
+            cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotHttpsOrHttp",
+                                                     [uri.asciiSpec]));
           }
         } catch(e) {
           switch (e.result) {
             case Components.results.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS:
             case Components.results.NS_ERROR_HOST_IS_IP_ADDRESS:
               if (uri.host !== selfUri.host) {
                 cspWarn(aCSPR,
                         CSPLocalizer.getFormatStr("pageCannotSendReportsTo",
@@ -426,17 +411,17 @@ CSPRep.fromString = function(aStr, self,
               break;
 
             default:
               cspWarn(aCSPR, CSPLocalizer.getFormatStr("couldNotParseReportURI",
                                                        [uriStrings[i]]));
               continue;
           }
         }
-        // all verification passed: same ETLD+1, scheme, and port.
+        // all verification passed
         okUriStrings.push(uri.asciiSpec);
       }
       aCSPR._directives[UD.REPORT_URI] = okUriStrings.join(' ');
       continue directive;
     }
 
     // POLICY URI //////////////////////////////////////////////////////////
     if (dirname === UD.POLICY_URI) {
@@ -644,44 +629,27 @@ CSPRep.fromStringSpecCompliant = functio
       for (let i in uriStrings) {
         var uri = null;
         try {
           // Relative URIs are okay, but to ensure we send the reports to the
           // right spot, the relative URIs are expanded here during parsing.
           // The resulting CSPRep instance will have only absolute URIs.
           uri = gIoService.newURI(uriStrings[i],null,selfUri);
 
-          // if there's no host, don't do the ETLD+ check.  This will throw
-          // NS_ERROR_FAILURE if the URI doesn't have a host, causing a parse
-          // failure.
+          // if there's no host, this will throw NS_ERROR_FAILURE, causing a
+          // parse failure.
           uri.host;
 
-          // Verify that each report URI is in the same etld + 1 and that the
-          // scheme and port match "self" if "self" is defined, and just that
-          // it's valid otherwise.
-          if (self) {
-            if (gETLDService.getBaseDomain(uri) !==
-                gETLDService.getBaseDomain(selfUri)) {
-              cspWarn(aCSPR, 
-                      CSPLocalizer.getFormatStr("reportURInotETLDPlus1",
-                                                [gETLDService.getBaseDomain(uri)]));
-              continue;
-            }
-            if (!uri.schemeIs(selfUri.scheme)) {
-              cspWarn(aCSPR,
-                      CSPLocalizer.getFormatStr("reportURInotSameSchemeAsSelf",
-                                                [uri.asciiSpec]));
-              continue;
-            }
-            if (uri.port && uri.port !== selfUri.port) {
-              cspWarn(aCSPR,
-                      CSPLocalizer.getFormatStr("reportURInotSamePortAsSelf",
-                                                [uri.asciiSpec]));
-              continue;
-            }
+          // warn about, but do not prohibit non-http and non-https schemes for
+          // reporting URIs.  The spec allows unrestricted URIs resolved
+          // relative to "self", but we should let devs know if the scheme is
+          // abnormal and may fail a POST.
+          if (!uri.schemeIs("http") && !uri.schemeIs("https")) {
+            cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotHttpsOrHttp",
+                                                     [uri.asciiSpec]));
           }
         } catch(e) {
           switch (e.result) {
             case Components.results.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS:
             case Components.results.NS_ERROR_HOST_IS_IP_ADDRESS:
               if (uri.host !== selfUri.host) {
                 cspWarn(aCSPR, CSPLocalizer.getFormatStr("pageCannotSendReportsTo",
                                                          [selfUri.host, uri.host]));
@@ -690,17 +658,17 @@ CSPRep.fromStringSpecCompliant = functio
               break;
 
             default:
               cspWarn(aCSPR, CSPLocalizer.getFormatStr("couldNotParseReportURI", 
                                                        [uriStrings[i]]));
               continue;
           }
         }
-        // all verification passed: same ETLD+1, scheme, and port.
+        // all verification passed.
        okUriStrings.push(uri.asciiSpec);
       }
       aCSPR._directives[UD.REPORT_URI] = okUriStrings.join(' ');
       continue directive;
     }
 
     // POLICY URI //////////////////////////////////////////////////////////
     if (dirname === UD.POLICY_URI) {
--- a/content/base/test/unit/test_csputils.js
+++ b/content/base/test/unit/test_csputils.js
@@ -660,71 +660,88 @@ test(function test_FrameAncestor_ignores
       do_check_true(testPermits(URI("http://user1:pass1@self.com/foo"),
                                 URI("http://username:password@self.com/bar")));
       do_check_true(testPermits(URI("http://self.com/foo"),
                                 URI("http://username:password@self.com/bar")));
      });
 
 test(function test_CSP_ReportURI_parsing() {
       var cspr;
-      var SD = CSPRep.SRC_DIRECTIVES_OLD;
+      var SD = CSPRep.SRC_DIRECTIVES_NEW;
       var self = "http://self.com:34";
       var parsedURIs = [];
 
       var uri_valid_absolute = self + "/report.py";
-      var uri_invalid_host_absolute = "http://foo.org:34/report.py";
+      var uri_other_host_absolute = "http://foo.org:34/report.py";
       var uri_valid_relative = "/report.py";
       var uri_valid_relative_expanded = self + uri_valid_relative;
       var uri_valid_relative2 = "foo/bar/report.py";
       var uri_valid_relative2_expanded = self + "/" + uri_valid_relative2;
       var uri_invalid_relative = "javascript:alert(1)";
+      var uri_other_scheme_absolute = "https://self.com/report.py";
+      var uri_other_scheme_and_host_absolute = "https://foo.com/report.py";
 
-      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_absolute, URI(self));
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_valid_absolute, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       do_check_in_array(parsedURIs, uri_valid_absolute);
       do_check_eq(parsedURIs.length, 1);
 
-      cspr = CSPRep.fromString("allow *; report-uri " + uri_invalid_host_absolute, URI(self));
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_other_host_absolute, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
-      do_check_in_array(parsedURIs, "");
+      do_check_in_array(parsedURIs, uri_other_host_absolute);
       do_check_eq(parsedURIs.length, 1); // the empty string is in there.
 
-      cspr = CSPRep.fromString("allow *; report-uri " + uri_invalid_relative, URI(self));
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_invalid_relative, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       do_check_in_array(parsedURIs, "");
       do_check_eq(parsedURIs.length, 1);
 
-      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_relative, URI(self));
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_valid_relative, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       do_check_in_array(parsedURIs, uri_valid_relative_expanded);
       do_check_eq(parsedURIs.length, 1);
 
-      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_relative2, URI(self));
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_valid_relative2, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       dump(parsedURIs.length);
       do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
       do_check_eq(parsedURIs.length, 1);
 
+      // make sure cross-scheme reporting works
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_other_scheme_absolute, URI(self));
+      parsedURIs = cspr.getReportURIs().split(/\s+/);
+      dump(parsedURIs.length);
+      do_check_in_array(parsedURIs, uri_other_scheme_absolute);
+      do_check_eq(parsedURIs.length, 1);
+
+      // make sure cross-scheme, cross-host reporting works
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " + uri_other_scheme_and_host_absolute, URI(self));
+      parsedURIs = cspr.getReportURIs().split(/\s+/);
+      dump(parsedURIs.length);
+      do_check_in_array(parsedURIs, uri_other_scheme_and_host_absolute);
+      do_check_eq(parsedURIs.length, 1);
+
       // combination!
-      cspr = CSPRep.fromString("allow *; report-uri " +
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " +
                                uri_valid_relative2 + " " +
                                uri_valid_absolute, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
       do_check_in_array(parsedURIs, uri_valid_absolute);
       do_check_eq(parsedURIs.length, 2);
 
-      cspr = CSPRep.fromString("allow *; report-uri " +
+      cspr = CSPRep.fromStringSpecCompliant("default-src *; report-uri " +
                                uri_valid_relative2 + " " +
-                               uri_invalid_host_absolute + " " +
+                               uri_other_host_absolute + " " +
                                uri_valid_absolute, URI(self));
       parsedURIs = cspr.getReportURIs().split(/\s+/);
       do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
+      do_check_in_array(parsedURIs, uri_other_host_absolute);
       do_check_in_array(parsedURIs, uri_valid_absolute);
-      do_check_eq(parsedURIs.length, 2);
+      do_check_eq(parsedURIs.length, 3);
     });
 
 test(
      function test_bug634778_duplicateDirective_Detection() {
       var cspr;
       var SD = CSPRep.SRC_DIRECTIVES_OLD;
       var self = "http://self.com:34";
       var firstDomain = "http://first.com";
--- a/dom/locales/en-US/chrome/security/csp.properties
+++ b/dom/locales/en-US/chrome/security/csp.properties
@@ -20,23 +20,19 @@ errorWas = error was: "%1$S"
 # %1$S is the report URI that could not be parsed
 couldNotParseReportURI = couldn't parse report URI: %1$S
 # LOCALIZATION NOTE (couldNotProcessUnknownDirective):
 # %1$S is the unknown directive
 couldNotProcessUnknownDirective = Couldn't process unknown directive '%1$S'
 # LOCALIZATION NOTE (ignoringUnknownOption):
 # %1$S is the option that could not be understood
 ignoringUnknownOption = Ignoring unknown option %1$S
-# LOCALIZATION NOTE (reportURInotETLDPlus1):
-# %1$S is the ETLD of the report URI that could not be used
-reportURInotETLDPlus1 = The report URI (%1$S) must be from the same eTLD+1.
-# LOCALIZATION NOTE (reportURInotSameSchemeAsSelf, reportURInotSamePortAsSelf):
-# %1$S is the report URI that could not be used
-reportURInotSameSchemeAsSelf = The report URI (%1$S) must use the same scheme as the originating document.
-reportURInotSamePortAsSelf = The report URI (%1$S) must use the same port as the originating document.
+# LOCALIZATION NOTE (reportURInotHttpsOrHttp):
+# %1$S is the ETLD of the report URI that is not HTTP or HTTPS
+reportURInotHttpsOrHttp = The report URI (%1$) should be an HTTP or HTTPS URI.
 # LOCALIZATION NOTE (pageCannotSendReportsTo):
 # %1$S is the URI of the page with the policy
 # %2$S is the report URI that could not be used
 pageCannotSendReportsTo = page on %1$S cannot send reports to %2$S
 allowOrDefaultSrcRequired = 'allow' or 'default-src' directive required but not present.  Reverting to "default-src 'none'"
 # LOCALIZATION NOTE (failedToParseUnrecognizedSource):
 # %1$S is the CSP Source that could not be parsed
 failedToParseUnrecognizedSource = Failed to parse unrecognized source %1$S