Bug 1793560 - Remove navigate-to CSP directive r=tschuster

It has never shipped after being implemented years ago, and was removed from spec in September 2022: w3c/webappsec-csp#564 Now skipping navigate-to WPT tests. Filed issue upstream for their future removal: w3c/webappsec-csp#608 Consensus seems to agree to remove, will do in follow up bug once landed. Also removed our own tests. Added a hack in StartDocumentLoad as just removing the navigate-to check call breaks some inhertiance, see comment for more info. Differential Revision: https://phabricator.services.mozilla.com/D181630
mozilla · Jan 3, 2024 · 465206f · 465206f
1 parent a1ed3bd
commit 465206f
Show file tree

Hide file tree

Showing 57 changed files with 39 additions and 721 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -1371,8 +1371,6 @@ module.exports = {
         "dom/security/test/csp/file_bug941404.html",
         "dom/security/test/csp/file_frameancestors_main.js",
         "dom/security/test/csp/file_main.js",
-        "dom/security/test/csp/file_navigate_to.html",
-        "dom/security/test/csp/file_navigate_to_request.html",
         "dom/security/test/csp/file_null_baseuri.html",
         "dom/security/test/csp/file_path_matching_redirect_server.sjs",
         "dom/security/test/csp/file_punycode_host_src.sjs",
@@ -1395,7 +1393,6 @@ module.exports = {
         "dom/security/test/csp/test_blocked_uri_in_reports.html",
         "dom/security/test/csp/test_blocked_uri_in_violation_event_after_redirects.html",
         "dom/security/test/csp/test_blocked_uri_redirect_frame_src.html",
-        "dom/security/test/csp/test_navigate_to.html",
         "dom/security/test/csp/test_null_baseuri.html",
         "dom/security/test/csp/test_path_matching.html",
         "dom/security/test/csp/test_report_for_import.html",

diff --git a/.prettierignore b/.prettierignore
@@ -212,7 +212,6 @@ devtools/client/styleeditor/test/test_private.html
 devtools/client/webconsole/test/browser/test-csp-violation.html
 devtools/client/webconsole/test/browser/test-external-script-errors.html
 devtools/client/webconsole/test/browser/test-mixedcontent-securityerrors.html
-devtools/client/webconsole/test/browser/test-navigate-to-parse-error.html
 devtools/client/webconsole/test/browser/test-network.html
 devtools/client/webconsole/test/browser/test_jsterm_screenshot_command.html
 devtools/server/tests/browser/animation-data.html

diff --git a/devtools/client/webconsole/test/browser/_webconsole.toml b/devtools/client/webconsole/test/browser/_webconsole.toml
@@ -137,7 +137,6 @@ support-files = [
   "test-message-categories-workers.html",
   "test-message-categories-workers.js",
   "test-mixedcontent-securityerrors.html",
-  "test-navigate-to-parse-error.html",
   "test-network-exceptions.html",
   "test-network-request.html",
   "test-network.html",
@@ -518,8 +517,6 @@ skip-if = ["a11y_checks"] # Bugs 1849028 and 1858041 clicked BUTTON.arrow is inc
 ["browser_webconsole_multiple_windows_and_tabs.js"]
 skip-if = ["win11_2009"] # Bug 1798331
 
-["browser_webconsole_navigate_to_parse_error.js"]
-
 ["browser_webconsole_network_attach.js"]
 
 ["browser_webconsole_network_exceptions.js"]

diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_navigate_to_parse_error.js b/devtools/client/webconsole/test/browser/browser_webconsole_navigate_to_parse_error.js
diff --git a/devtools/client/webconsole/test/browser/test-navigate-to-parse-error.html b/devtools/client/webconsole/test/browser/test-navigate-to-parse-error.html
diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
@@ -3475,8 +3475,7 @@ nsDocShell::DisplayLoadError(nsresult aError, nsIURI* aURI,
     CopyUTF8toUTF16(host, *formatStrs.AppendElement());
     error = "netTimeout";
   } else if (NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION == aError ||
-             NS_ERROR_CSP_FORM_ACTION_VIOLATION == aError ||
-             NS_ERROR_CSP_NAVIGATE_TO_VIOLATION == aError) {
+             NS_ERROR_CSP_FORM_ACTION_VIOLATION == aError) {
     // CSP error
     cssClass.AssignLiteral("neterror");
     error = "cspBlocked";
@@ -10560,22 +10559,6 @@ nsresult nsDocShell::DoURILoad(nsDocShellLoadState* aLoadState,
     NS_ADDREF(*aRequest = channel);
   }
 
-  nsCOMPtr<nsIContentSecurityPolicy> csp = aLoadState->Csp();
-  if (csp) {
-    // Check CSP navigate-to
-    bool allowsNavigateTo = false;
-    rv = csp->GetAllowsNavigateTo(aLoadState->URI(),
-                                  aLoadState->IsFormSubmission(),
-                                  false, /* aWasRedirected */
-                                  false, /* aEnforceWhitelist */
-                                  &allowsNavigateTo);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    if (!allowsNavigateTo) {
-      return NS_ERROR_CSP_NAVIGATE_TO_VIOLATION;
-    }
-  }
-
   const nsACString& typeHint = aLoadState->TypeHint();
   if (!typeHint.IsVoid()) {
     mContentTypeHint = typeHint;

diff --git a/docshell/base/nsDocShellLoadState.h b/docshell/base/nsDocShellLoadState.h
@@ -491,8 +491,7 @@ class nsDocShellLoadState final {
   bool mOriginalFrameSrc;
 
   // If this attribute is true, then the load was initiated by a
-  // form submission. This is important to know for the CSP directive
-  // navigate-to.
+  // form submission.
   bool mIsFormSubmission;
 
   // Contains a load type as specified by the nsDocShellLoadTypes::load*

diff --git a/docshell/base/nsDocShellTelemetryUtils.cpp b/docshell/base/nsDocShellTelemetryUtils.cpp
@@ -58,10 +58,6 @@ static const LoadErrorTelemetryResult sResult[] = {
         NS_ERROR_CSP_FORM_ACTION_VIOLATION,
         ErrorLabel::CSP_FORM_ACTION,
     },
-    {
-        NS_ERROR_CSP_NAVIGATE_TO_VIOLATION,
-        ErrorLabel::CSP_NAVIGATE_TO,
-    },
     {
         NS_ERROR_XFO_VIOLATION,
         ErrorLabel::XFO_VIOLATION,

diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
@@ -3593,23 +3593,15 @@ nsresult Document::StartDocumentLoad(const char* aCommand, nsIChannel* aChannel,
   rv = InitCOEP(aChannel);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Check CSP navigate-to
-  // We need to enforce the CSP of the document that initiated the load,
-  // which is the CSP to inherit.
+  // HACK: Calling EnsureIPCPoliciesRead() here will parse the CSP using the
+  // context's current mSelfURI (which is still the previous mSelfURI),
+  // bypassing some internal bugs with 'self' and iframe inheritance.
+  // Not calling it here results in the mSelfURI being the current mSelfURI and
+  // not the previous which breaks said inheritance.
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1793560#ch-8
   nsCOMPtr<nsIContentSecurityPolicy> cspToInherit = loadInfo->GetCspToInherit();
   if (cspToInherit) {
-    bool allowsNavigateTo = false;
-    rv = cspToInherit->GetAllowsNavigateTo(
-        mDocumentURI, loadInfo->GetIsFormSubmission(),
-        !loadInfo->RedirectChain().IsEmpty(), /* aWasRedirected */
-        true,                                 /* aEnforceWhitelist */
-        &allowsNavigateTo);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    if (!allowsNavigateTo) {
-      aChannel->Cancel(NS_ERROR_CSP_NAVIGATE_TO_VIOLATION);
-      return NS_OK;
-    }
+    cspToInherit->EnsureIPCPoliciesRead();
   }
 
   rv = InitCSP(aChannel);

diff --git a/dom/interfaces/security/nsIContentSecurityPolicy.idl b/dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ -61,11 +61,10 @@ interface nsIContentSecurityPolicy : nsISerializable
     BLOCK_ALL_MIXED_CONTENT        = 18,
     SANDBOX_DIRECTIVE              = 19,
     WORKER_SRC_DIRECTIVE           = 20,
-    NAVIGATE_TO_DIRECTIVE          = 21,
-    SCRIPT_SRC_ELEM_DIRECTIVE      = 22,
-    SCRIPT_SRC_ATTR_DIRECTIVE      = 23,
-    STYLE_SRC_ELEM_DIRECTIVE       = 24,
-    STYLE_SRC_ATTR_DIRECTIVE       = 25,
+    SCRIPT_SRC_ELEM_DIRECTIVE      = 21,
+    SCRIPT_SRC_ATTR_DIRECTIVE      = 22,
+    STYLE_SRC_ELEM_DIRECTIVE       = 23,
+    STYLE_SRC_ATTR_DIRECTIVE       = 24,
   };
 
   /**
@@ -154,24 +153,6 @@ interface nsIContentSecurityPolicy : nsISerializable
                           in unsigned long aLineNumber,
                           in unsigned long aColumnNumber);
 
-  /*
-   * Whether this policy allows a navigation subject to the navigate-to
-   * policy.
-   * @param aURI The target URI
-   * @param aIsFormSubmission True if the navigation was initiated by a form submission. This
-   *        is important since the form-action directive overrides navigate-to in that case.
-   * @param aWasRedirect True if a redirect has happened. Important for path-sensitivity.
-   * @param aEnforceAllowlist True if the allowlist of allowed targets must be enforced. If
-   *        this is true, the allowlist must be enforced even if 'unsafe-allow-redirects' is
-   *        used. If 'unsafe-allow-redirects' is not used then the allowlist is always enforced
-   * @return
-   *     Whether or not the effects of the navigation is allowed
-   */
-  boolean getAllowsNavigateTo(in nsIURI aURI,
-                              in boolean aIsFormSubmission,
-                              in boolean aWasRedirected,
-                              in boolean aEnforceAllowlist);
-
   /**
    * Whether this policy allows eval and eval-like functions
    * such as setTimeout("code string", time).
@@ -359,6 +340,11 @@ interface nsIContentSecurityPolicy : nsISerializable
    */
   AString toJSON();
 
+  /**
+   * Ensure policies from IPC are read/parsed.
+   */
+  [noscript] void EnsureIPCPoliciesRead();
+
 };
 
 typedef nsIContentSecurityPolicy_CSPDirective CSPDirective;

diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp
@@ -326,7 +326,8 @@ nsresult nsCSPContext::InitFromOther(nsCSPContext* aOtherContext) {
   return NS_OK;
 }
 
-void nsCSPContext::EnsureIPCPoliciesRead() {
+NS_IMETHODIMP
+nsCSPContext::EnsureIPCPoliciesRead() {
   // Most likely the parser errors already happened before serializing
   // the policy for IPC.
   bool previous = mSuppressParserLogMessages;
@@ -343,6 +344,7 @@ void nsCSPContext::EnsureIPCPoliciesRead() {
   }
 
   mSuppressParserLogMessages = previous;
+  return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -673,93 +675,6 @@ nsCSPContext::GetAllowsInline(CSPDirective aDirective, bool aHasUnsafeHash,
   return NS_OK;
 }
 
-NS_IMETHODIMP
-nsCSPContext::GetAllowsNavigateTo(nsIURI* aURI, bool aIsFormSubmission,
-                                  bool aWasRedirected, bool aEnforceAllowlist,
-                                  bool* outAllowsNavigateTo) {
-  /*
-   * The matrix below shows the different values of (aWasRedirect,
-   * aEnforceAllowlist) for the three different checks we do.
-   *
-   *  Navigation    | Start Loading  | Initiate Redirect | Document
-   *                | (nsDocShell)   | (nsCSPService)    |
-   *  -----------------------------------------------------------------
-   *  A -> B          (false,false)    -                   (false,true)
-   *  A -> ... -> B   (false,false)    (true,false)        (true,true)
-   */
-  *outAllowsNavigateTo = false;
-
-  EnsureIPCPoliciesRead();
-  // The 'form-action' directive overrules 'navigate-to' for form submissions.
-  // So in case this is a form submission and the directive 'form-action' is
-  // present then there is nothing for us to do here, see: 6.3.3.1.2
-  // https://www.w3.org/TR/CSP3/#navigate-to-pre-navigate
-  if (aIsFormSubmission) {
-    for (unsigned long i = 0; i < mPolicies.Length(); i++) {
-      if (mPolicies[i]->hasDirective(
-              nsIContentSecurityPolicy::FORM_ACTION_DIRECTIVE)) {
-        *outAllowsNavigateTo = true;
-        return NS_OK;
-      }
-    }
-  }
-
-  bool atLeastOneBlock = false;
-  for (unsigned long i = 0; i < mPolicies.Length(); i++) {
-    if (!mPolicies[i]->allowsNavigateTo(aURI, aWasRedirected,
-                                        aEnforceAllowlist)) {
-      if (!mPolicies[i]->getReportOnlyFlag()) {
-        atLeastOneBlock = true;
-      }
-
-      // If the load encountered a server side redirect, the spec suggests to
-      // remove the path component from the URI, see:
-      // https://www.w3.org/TR/CSP3/#source-list-paths-and-redirects
-      nsCOMPtr<nsIURI> blockedURIForReporting = aURI;
-      if (aWasRedirected) {
-        nsAutoCString prePathStr;
-        nsCOMPtr<nsIURI> prePathURI;
-        nsresult rv = aURI->GetPrePath(prePathStr);
-        NS_ENSURE_SUCCESS(rv, rv);
-        rv = NS_NewURI(getter_AddRefs(blockedURIForReporting), prePathStr);
-        NS_ENSURE_SUCCESS(rv, rv);
-      }
-
-      // Lines numbers and source file for the violation report
-      uint32_t lineNumber = 0;
-      uint32_t columnNumber = 1;
-      nsAutoCString spec;
-      JSContext* cx = nsContentUtils::GetCurrentJSContext();
-      if (cx) {
-        nsJSUtils::GetCallingLocation(cx, spec, &lineNumber, &columnNumber);
-        // If GetCallingLocation fails linenumber & columnNumber are set to
-        // (0, 1) anyway so we can skip checking if that is the case.
-      }
-
-      // Report the violation
-      nsresult rv = AsyncReportViolation(
-          nullptr,                                    // aTriggeringElement
-          nullptr,                                    // aCSPEventListener
-          blockedURIForReporting,                     // aBlockedURI
-          nsCSPContext::BlockedContentSource::eSelf,  // aBlockedSource
-          nullptr,                                    // aOriginalURI
-          u"navigate-to"_ns,                          // aViolatedDirective
-          u"navigate-to"_ns,                          // aEffectiveDirective
-          i,                                          // aViolatedPolicyIndex
-          u""_ns,                                     // aObserverSubject
-          NS_ConvertUTF8toUTF16(spec),                // aSourceFile
-          false,                                      // aReportSample
-          u""_ns,                                     // aScriptSample
-          lineNumber,                                 // aLineNum
-          columnNumber);                              // aColumnNum
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
-  }
-
-  *outAllowsNavigateTo = !atLeastOneBlock;
-  return NS_OK;
-}
-
 /**
  * For each policy, log any violation on the Error Console and send a report
  * if a report-uri is present in the policy

diff --git a/dom/security/nsCSPContext.h b/dom/security/nsCSPContext.h
@@ -149,8 +149,6 @@ class nsCSPContext : public nsIContentSecurityPolicy {
       nsTArray<mozilla::ipc::ContentSecurityPolicy>& aPolicies);
 
  private:
-  void EnsureIPCPoliciesRead();
-
   bool ShouldThrottleReport(
       const mozilla::dom::SecurityPolicyViolationEventInit&
           aViolationEventInit);

diff --git a/dom/security/nsCSPParser.cpp b/dom/security/nsCSPParser.cpp
@@ -456,20 +456,6 @@ nsCSPBaseSrc* nsCSPParser::keywordSource() {
     return new nsCSPKeywordSrc(CSP_UTF16KeywordToEnum(mCurToken));
   }
 
-  if (CSP_IsKeyword(mCurToken, CSP_UNSAFE_ALLOW_REDIRECTS)) {
-    if (!CSP_IsDirective(mCurDir[0],
-                         nsIContentSecurityPolicy::NAVIGATE_TO_DIRECTIVE)) {
-      // Only allow 'unsafe-allow-redirects' within navigate-to.
-      AutoTArray<nsString, 2> params = {u"unsafe-allow-redirects"_ns,
-                                        u"navigate-to"_ns};
-      logWarningErrorToConsole(nsIScriptError::warningFlag,
-                               "IgnoringSourceWithinDirective", params);
-      return nullptr;
-    }
-
-    return new nsCSPKeywordSrc(CSP_UTF16KeywordToEnum(mCurToken));
-  }
-
   return nullptr;
 }
 
@@ -861,18 +847,6 @@ nsCSPDirective* nsCSPParser::directiveName() {
     return nullptr;
   }
 
-  // Bug 1529068: Implement navigate-to directive.
-  // Once all corner cases are resolved we can remove that special
-  // if-handling here and let the parser just fall through to
-  // return new nsCSPDirective.
-  if (directive == nsIContentSecurityPolicy::NAVIGATE_TO_DIRECTIVE &&
-      !StaticPrefs::security_csp_enableNavigateTo()) {
-    AutoTArray<nsString, 1> params = {mCurToken};
-    logWarningErrorToConsole(nsIScriptError::warningFlag,
-                             "couldNotProcessUnknownDirective", params);
-    return nullptr;
-  }
-
   // Make sure the directive does not already exist
   // (see http://www.w3.org/TR/CSP11/#parsing)
   if (mPolicy->hasDirective(directive)) {