[tbb-commits] [tor-browser/tor-browser-78.8.0esr-10.0-1] Bug 1687342: Blocked-URI in CSP reports should be URI before redirects with ref removed r=freddyb, dveditz a=pascalc

[sysrqb at torproject.org]

commit 834ae039ea8957f794624ca6d125628e3979569f
Author: Christoph Kerschbaumer <ckerschb at christophkerschbaumer.com>
Date:   Tue Feb 9 18:42:03 2021 +0000

    Bug 1687342: Blocked-URI in CSP reports should be URI before redirects with ref removed r=freddyb,dveditz a=pascalc
    
    Differential Revision: https://phabricator.services.mozilla.com/D103450
---
 dom/security/nsCSPContext.cpp                      | 38 ++++++++--------------
 .../test/csp/test_blocked_uri_in_reports.html      |  6 ++--
 .../frame-src/frame-src-redirect.html.ini          |  5 +++
 3 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp
index 3601f5a4f2fc..413044afcc88 100644
--- a/dom/security/nsCSPContext.cpp
+++ b/dom/security/nsCSPContext.cpp
@@ -950,20 +950,18 @@ void nsCSPContext::logToConsole(const char* aName,
 
 /**
  * Strip URI for reporting according to:
- * http://www.w3.org/TR/CSP/#violation-reports
+ * https://w3c.github.io/webappsec-csp/#security-violation-reports
  *
  * @param aURI
- *        The uri to be stripped for reporting
- * @param aSelfURI
- *        The uri of the protected resource
- *        which is needed to enforce the SOP.
- * @return ASCII serialization of the uri to be reported.
+ *        The URI of the blocked resource. In case of a redirect, this it the
+ *        initial URI the request started out with, not the redirected URI.
+ * @return The ASCII serialization of the uri to be reported ignoring
+ *         the ref part of the URI.
  */
-void StripURIForReporting(nsIURI* aURI, nsIURI* aSelfURI,
-                          nsACString& outStrippedURI) {
-  // 1) If the origin of uri is a globally unique identifier (for example,
-  // aURI has a scheme of data, blob, or filesystem), then return the
-  // ASCII serialization of uri’s scheme.
+void StripURIForReporting(nsIURI* aURI, nsACString& outStrippedURI) {
+  // If the origin of aURI is a globally unique identifier (for example,
+  // aURI has a scheme of data, blob, or filesystem), then
+  // return the ASCII serialization of uri’s scheme.
   bool isHttpFtpOrWs =
       (aURI->SchemeIs("http") || aURI->SchemeIs("https") ||
        aURI->SchemeIs("ftp") || aURI->SchemeIs("ws") || aURI->SchemeIs("wss"));
@@ -976,7 +974,7 @@ void StripURIForReporting(nsIURI* aURI, nsIURI* aSelfURI,
     return;
   }
 
-  // Return uri, with any fragment component removed.
+  // Return aURI, with any fragment component removed.
   aURI->GetSpecIgnoringRef(outStrippedURI);
 }
 
@@ -996,7 +994,7 @@ nsresult nsCSPContext::GatherSecurityPolicyViolationEventData(
 
   // document-uri
   nsAutoCString reportDocumentURI;
-  StripURIForReporting(mSelfURI, mSelfURI, reportDocumentURI);
+  StripURIForReporting(mSelfURI, reportDocumentURI);
   aViolationEventInit.mDocumentURI = NS_ConvertUTF8toUTF16(reportDocumentURI);
 
   // referrer
@@ -1004,17 +1002,9 @@ nsresult nsCSPContext::GatherSecurityPolicyViolationEventData(
 
   // blocked-uri
   if (aBlockedURI) {
-    // in case of blocking a browsing context (frame) we have to report
-    // the final URI in case of a redirect. For subresources we report
-    // the URI before redirects.
-    nsCOMPtr<nsIURI> uriToReport;
-    if (aViolatedDirective.EqualsLiteral("frame-src")) {
-      uriToReport = aBlockedURI;
-    } else {
-      uriToReport = aOriginalURI ? aOriginalURI : aBlockedURI;
-    }
     nsAutoCString reportBlockedURI;
-    StripURIForReporting(uriToReport, mSelfURI, reportBlockedURI);
+    StripURIForReporting(aOriginalURI ? aOriginalURI : aBlockedURI,
+                         reportBlockedURI);
     aViolationEventInit.mBlockedURI = NS_ConvertUTF8toUTF16(reportBlockedURI);
   } else {
     aViolationEventInit.mBlockedURI = NS_ConvertUTF8toUTF16(aBlockedString);
@@ -1914,4 +1904,4 @@ void nsCSPContext::SerializePolicies(
   }
 
   aPolicies.AppendElements(mIPCPolicies);
-}
\ No newline at end of file
+}
diff --git a/dom/security/test/csp/test_blocked_uri_in_reports.html b/dom/security/test/csp/test_blocked_uri_in_reports.html
index 0bb986ec3d04..f40d98efc5d3 100644
--- a/dom/security/test/csp/test_blocked_uri_in_reports.html
+++ b/dom/security/test/csp/test_blocked_uri_in_reports.html
@@ -19,9 +19,9 @@ SimpleTest.waitForExplicitFinish();
  * which gets redirected to:
  *  http://test1.example.com/tests/dom/security//test/csp/file_path_matching.js
  *
- * The blocked-uri in the csp-report should be:
- *   test1.example.com
- * instead of:
+ * The blocked-uri in the csp-report should be the original URI:
+ *   http://example.com/tests/dom/security/test/csp/file_path_matching_redirect_server.sjs
+ * instead of the redirected URI:
  *  http://test1.example.com/tests/com/security/test/csp/file_path_matching.js
  *
  * see also: http://www.w3.org/TR/CSP/#violation-reports
diff --git a/testing/web-platform/meta/content-security-policy/frame-src/frame-src-redirect.html.ini b/testing/web-platform/meta/content-security-policy/frame-src/frame-src-redirect.html.ini
new file mode 100644
index 000000000000..afc5b552aae4
--- /dev/null
+++ b/testing/web-platform/meta/content-security-policy/frame-src/frame-src-redirect.html.ini
@@ -0,0 +1,5 @@
+[frame-src-redirect.html]
+  expected: TIMEOUT
+  [Redirected iframe src should evaluate both enforced and report-only policies on both original request and when following redirect]
+    expected: TIMEOUT
+