Bug 1301529 - Remove X-Frame-Options allow-from. r=ckerschb

Differential Revision: https://phabricator.services.mozilla.com/D38672

--HG--
extra : moz-landing-system : lando

devtools/server/actors/errordocs.js

@@ -112,7 +112,10 @@ const SOURCE_MAP_LEARN_MORE =
   "https://developer.mozilla.org/en-US/docs/Tools/Debugger/Source_map_errors";
 const TLS_LEARN_MORE =
   "https://blog.mozilla.org/security/2018/10/15/removing-old-versions-of-tls/";
+const X_FRAME_OPTIONS_LEARN_MORE =
+  "https://developer.mozilla.org/docs/Web/HTTP/Headers/X-Frame-Options";
 const ErrorCategories = {
+  "X-Frame-Options": X_FRAME_OPTIONS_LEARN_MORE,
   "Insecure Password Field": INSECURE_PASSWORDS_LEARN_MORE,
   "Mixed Content Message": MIXED_CONTENT_LEARN_MORE,
   "Mixed Content Blocker": MIXED_CONTENT_LEARN_MORE,

dom/base/test/test_x-frame-options.html

@@ -75,10 +75,10 @@ var testFramesLoaded = function() {
   var test11 = frame.contentDocument.getElementById("test").textContent;
   is(test11, "allow-from-allow", "test allow-from-allow");
 
-  // iframe from different origin, with allow-from: other - should not load
+  // iframe from different origin, with allow-from: other - should load as we no longer support allow-from (Bug 1301529)
   frame = harness.contentDocument.getElementById("allow-from-deny");
   var test12 = frame.contentDocument.getElementById("test");
-  is(test12, null, "test allow-from-deny");
+  isnot(test12, null, "test allow-from-deny");
 
   // iframe from different origin, X-F-O: SAMEORIGIN, multipart - should not load
   frame = harness.contentDocument.getElementById("sameorigin-multipart");
@@ -91,7 +91,7 @@ var testFramesLoaded = function() {
   is(test14, "sameorigin-multipart2", "test sameorigin-multipart2");
 
 
-  // frames from bug 836132 tests
+  // frames from bug 836132 tests, no longer supported allow-from
   {
     frame = harness.contentDocument.getElementById("allow-from-allow-1");
     var theTestResult = frame.contentDocument.getElementById("test");
@@ -100,10 +100,11 @@ var testFramesLoaded = function() {
       is(theTestResult.textContent, "allow-from-allow-1", "test allow-from-allow-1");
     }
   }
+  // Verify allow-from no longer works
   for (var i = 1; i<=14; i++) {
     frame = harness.contentDocument.getElementById("allow-from-deny-" + i);
     var theTestResult = frame.contentDocument.getElementById("test");
-    is(theTestResult, null, "test allow-from-deny-" + i);
+    isnot(theTestResult, null, "test allow-from-deny-" + i);
   }
 
   // call tests to check principal comparison, e.g. a document can open a window

dom/browser-element/mochitest/browserElement_XFrameOptionsAllowFrom.js

@@ -1,64 +0,0 @@
-/* Any copyright is dedicated to the public domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-// Bug 690168 - Support Allow-From notation for X-Frame-Options header.
-"use strict";
-
-/* global browserElementTestHelpers */
-
-SimpleTest.waitForExplicitFinish();
-browserElementTestHelpers.setEnabledPref(true);
-browserElementTestHelpers.addPermission();
-
-var initialScreenshotArrayBuffer = null;
-
-function arrayBuffersEqual(a, b) {
-  var x = new Int8Array(a);
-  var y = new Int8Array(b);
-  if (x.length != y.length) {
-    return false;
-  }
-
-  for (var i = 0; i < x.length; i++) {
-    if (x[i] != y[i]) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
-function runTest() {
-  var iframe = document.createElement("iframe");
-  iframe.setAttribute("mozbrowser", "true");
-  iframe.height = "1000px";
-
-  var step1, stepfinish;
-  // The innermost page we load will fire an alert when it successfully loads.
-  iframe.addEventListener("mozbrowsershowmodalprompt", function(e) {
-    switch (e.detail.message) {
-      case "step 1":
-        step1 = SpecialPowers.snapshotWindow(iframe.contentWindow);
-        break;
-      case "step 2":
-        ok(false, "cross origin page loaded");
-        break;
-      case "finish":
-        // The page has now attempted to load the X-Frame-Options page; take
-        // another screenshot.
-        stepfinish = SpecialPowers.snapshotWindow(iframe.contentWindow);
-        ok(
-          step1.toDataURL() == stepfinish.toDataURL(),
-          "Screenshots should be identical"
-        );
-        SimpleTest.finish();
-    }
-  });
-
-  document.body.appendChild(iframe);
-
-  iframe.src =
-    "http://example.com/tests/dom/browser-element/mochitest/file_browserElement_XFrameOptionsAllowFrom.html";
-}
-
-addEventListener("testready", runTest);

dom/browser-element/mochitest/mochitest-oop.ini

@@ -59,9 +59,6 @@ disabled = Disabling some OOP tests for WebIDL scope changes (bug 1310706 for re
 [test_browserElement_oop_Titlechange.html]
 [test_browserElement_oop_TopBarrier.html]
 [test_browserElement_oop_XFrameOptions.html]
-[test_browserElement_oop_XFrameOptionsAllowFrom.html]
-#skip-if = asan # bug 1189592 - should be OK when ASAN mochitests are on Ubuntu 16.04
-disabled = Disabling some OOP tests for WebIDL scope changes (bug 1310706 for re-enabling)
 [test_browserElement_oop_XFrameOptionsDeny.html]
 disabled = Disabling some OOP tests for WebIDL scope changes (bug 1310706 for re-enabling)
 [test_browserElement_oop_XFrameOptionsSameOrigin.html]

dom/browser-element/mochitest/mochitest.ini

@@ -48,7 +48,6 @@ support-files =
   browserElement_Titlechange.js
   browserElement_TopBarrier.js
   browserElement_XFrameOptions.js
-  browserElement_XFrameOptionsAllowFrom.js
   browserElement_XFrameOptionsDeny.js
   browserElement_XFrameOptionsSameOrigin.js
   file_browserElement_AlertInFrame.html
@@ -75,8 +74,6 @@ support-files =
   file_browserElement_TargetBlank.html
   file_browserElement_TargetTop.html
   file_browserElement_XFrameOptions.sjs
-  file_browserElement_XFrameOptionsAllowFrom.html
-  file_browserElement_XFrameOptionsAllowFrom.sjs
   file_browserElement_XFrameOptionsDeny.html
   file_browserElement_XFrameOptionsSameOrigin.html
   file_bug741717.sjs
@@ -135,7 +132,6 @@ skip-if = android_version == '22' # bug 1358876, bug 1322607
 [test_browserElement_inproc_Titlechange.html]
 [test_browserElement_inproc_TopBarrier.html]
 [test_browserElement_inproc_XFrameOptions.html]
-[test_browserElement_inproc_XFrameOptionsAllowFrom.html]
 [test_browserElement_inproc_XFrameOptionsDeny.html]
 [test_browserElement_inproc_XFrameOptionsSameOrigin.html]
 [test_browserElement_inproc_Reload.html]

dom/browser-element/mochitest/test_browserElement_inproc_XFrameOptionsAllowFrom.html

@@ -1,13 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<head>
-  <title>Test for Bug 690168</title>
-  <script src="/tests/SimpleTest/SimpleTest.js"></script>
-  <script type="application/javascript" src="browserElementTestHelpers.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
-</head>
-<body>
-<script type="application/javascript" src="browserElement_XFrameOptionsAllowFrom.js">
-</script>
-</body>
-</html>

dom/browser-element/mochitest/test_browserElement_oop_XFrameOptionsAllowFrom.html

@@ -1,13 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<head>
-  <title>Test for Bug 690168</title>
-  <script src="/tests/SimpleTest/SimpleTest.js"></script>
-  <script type="application/javascript" src="browserElementTestHelpers.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
-</head>
-<body>
-<script type="application/javascript" src="browserElement_XFrameOptionsAllowFrom.js">
-</script>
-</body>
-</html>

dom/locales/en-US/chrome/security/security.properties

@@ -126,3 +126,11 @@ FeaturePolicyInvalidAllowValue=Feature Policy: Skipping unsupported allow value
 ReferrerLengthOverLimitation=HTTP Referrer header: Length is over “%1$S” bytes limit - stripping referrer header down to origin: “%2$S”
 # LOCALIZATION NOTE: "%1$S" is the limitation length (bytes) of referrer URI, "%2$S" is the origin of the referrer URI.
 ReferrerOriginLengthOverLimitation=HTTP Referrer header: Length of origin within referrer is over “%1$S” bytes limit - removing referrer with origin “%2$S”.
+
+# X-Frame-Options
+# LOCALIZATION NOTE: %1$S is the header value, %2$S is frame URI and %3$S is the parent document URI.
+XFOInvalid = Invalid X-Frame-Options: “%1$S” header from “%2$S” loaded into “%3$S”.
+# LOCALIZATION NOTE: %1$S is the header value, %2$S is frame URI and %3$S is the parent document URI.
+XFODeny = Load denied by X-Frame-Options: “%1$S” from “%2$S”, site does not permit any framing. Attempted to load into “%3$S”.
+# LOCALIZATION NOTE: %1$S is the header value, %2$S is frame URI and %3$S is the parent document URI.
+XFOSameOrigin = Load denied by X-Frame-Options: “%1$S” from “%2$S”, site does not permit cross-origin framing from “%3$S”.

dom/security/FramingChecker.cpp

@@ -17,27 +17,112 @@
 #include "mozilla/dom/nsCSPUtils.h"
 #include "mozilla/dom/LoadURIOptionsBinding.h"
 #include "mozilla/NullPrincipal.h"
+#include "nsIStringBundle.h"
 
 using namespace mozilla;
 
+void FramingChecker::ReportError(const char* aMessageTag,
+                                 nsIDocShellTreeItem* aParentDocShellItem,
+                                 nsIURI* aChildURI, const nsAString& aPolicy) {
+  MOZ_ASSERT(aParentDocShellItem, "Need a parent docshell");
+  if (!aChildURI || !aParentDocShellItem) {
+    return;
+  }
+
+  Document* parentDocument = aParentDocShellItem->GetDocument();
+  nsCOMPtr<nsIURI> parentURI;
+  parentDocument->NodePrincipal()->GetURI(getter_AddRefs(parentURI));
+  MOZ_ASSERT(!parentDocument->NodePrincipal()->IsSystemPrincipal(),
+             "Should not get system principal here.");
+
+  // Get the parent URL spec
+  nsAutoCString parentSpec;
+  nsresult rv;
+  rv = parentURI->GetAsciiSpec(parentSpec);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+
+  // Get the child URL spec
+  nsAutoCString childSpec;
+  rv = aChildURI->GetAsciiSpec(childSpec);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+
+  nsCOMPtr<nsIStringBundleService> bundleService =
+      mozilla::services::GetStringBundleService();
+  nsCOMPtr<nsIStringBundle> bundle;
+  rv = bundleService->CreateBundle(
+      "chrome://global/locale/security/security.properties",
+      getter_AddRefs(bundle));
+  if (NS_FAILED(rv)) {
+    return;
+  }
+
+  if (NS_WARN_IF(!bundle)) {
+    return;
+  }
+
+  nsCOMPtr<nsIConsoleService> console(
+      do_GetService(NS_CONSOLESERVICE_CONTRACTID));
+  nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
+  if (!console || !error) {
+    return;
+  }
+
+  // Localize the error message
+  nsAutoString message;
+  AutoTArray<nsString, 3> formatStrings;
+  formatStrings.AppendElement(aPolicy);
+  CopyASCIItoUTF16(childSpec, *formatStrings.AppendElement());
+  CopyASCIItoUTF16(parentSpec, *formatStrings.AppendElement());
+  rv = bundle->FormatStringFromName(aMessageTag, formatStrings, message);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+
+  rv = error->InitWithWindowID(message, EmptyString(), EmptyString(), 0, 0,
+                               nsIScriptError::errorFlag, "X-Frame-Options",
+                               parentDocument->InnerWindowID());
+  if (NS_FAILED(rv)) {
+    return;
+  }
+  console->LogMessage(error);
+}
+
 /* static */
 bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
                                                 const nsAString& aPolicy,
                                                 nsIDocShell* aDocShell) {
-  static const char allowFrom[] = "allow-from";
-  const uint32_t allowFromLen = ArrayLength(allowFrom) - 1;
-  bool isAllowFrom =
-      StringHead(aPolicy, allowFromLen).LowerCaseEqualsLiteral(allowFrom);
+  nsresult rv;
+  // Find the top docshell in our parent chain that doesn't have the system
+  // principal and use it for the principal comparison.  Finding the top
+  // content-type docshell doesn't work because some chrome documents are
+  // loaded in content docshells (see bug 593387).
+  nsCOMPtr<nsIDocShellTreeItem> thisDocShellItem(aDocShell);
+  nsCOMPtr<nsIDocShellTreeItem> parentDocShellItem;
+  nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = thisDocShellItem;
+  nsCOMPtr<Document> topDoc;
+  nsCOMPtr<nsIScriptSecurityManager> ssm =
+      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
 
-  // return early if header does not have one of the values with meaning
-  if (!aPolicy.LowerCaseEqualsLiteral("deny") &&
-      !aPolicy.LowerCaseEqualsLiteral("sameorigin") && !isAllowFrom) {
-    return true;
+  if (!ssm) {
+    MOZ_CRASH();
   }
 
   nsCOMPtr<nsIURI> uri;
   aHttpChannel->GetURI(getter_AddRefs(uri));
 
+  // return early if header does not have one of the values with meaning
+  if (!aPolicy.LowerCaseEqualsLiteral("deny") &&
+      !aPolicy.LowerCaseEqualsLiteral("sameorigin")) {
+    nsCOMPtr<nsIDocShellTreeItem> root;
+    curDocShellItem->GetSameTypeRootTreeItem(getter_AddRefs(root));
+    ReportError("XFOInvalid", root, uri, aPolicy);
+    return true;
+  }
+
   // XXXkhuey when does this happen?  Is returning true safe here?
   if (!aDocShell) {
     return true;
@@ -62,21 +147,6 @@ bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
     return true;
   }
 
-  // Find the top docshell in our parent chain that doesn't have the system
-  // principal and use it for the principal comparison.  Finding the top
-  // content-type docshell doesn't work because some chrome documents are
-  // loaded in content docshells (see bug 593387).
-  nsCOMPtr<nsIDocShellTreeItem> thisDocShellItem(aDocShell);
-  nsCOMPtr<nsIDocShellTreeItem> parentDocShellItem;
-  nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = thisDocShellItem;
-  nsCOMPtr<Document> topDoc;
-  nsresult rv;
-  nsCOMPtr<nsIScriptSecurityManager> ssm =
-      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
-  if (!ssm) {
-    MOZ_CRASH();
-  }
-
   // If the X-Frame-Options value is SAMEORIGIN, then the top frame in the
   // parent chain must be from the same origin as this document.
   bool checkSameOrigin = aPolicy.LowerCaseEqualsLiteral("sameorigin");
@@ -109,7 +179,7 @@ bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
 
         // one of the ancestors is not same origin as this document
         if (NS_FAILED(rv)) {
-          ReportXFOViolation(curDocShellItem, uri, eSAMEORIGIN);
+          ReportError("XFOSameOrigin", curDocShellItem, uri, aPolicy);
           return false;
         }
       }
@@ -129,34 +199,10 @@ bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
   // not met (current docshell is not the top docshell), prohibit the
   // load.
   if (aPolicy.LowerCaseEqualsLiteral("deny")) {
-    ReportXFOViolation(curDocShellItem, uri, eDENY);
+    ReportError("XFODeny", curDocShellItem, uri, aPolicy);
     return false;
   }
 
-  topDoc = curDocShellItem->GetDocument();
-  topDoc->NodePrincipal()->GetURI(getter_AddRefs(topUri));
-
-  // If the X-Frame-Options value is "allow-from [uri]", then the top
-  // frame in the parent chain must be from that origin
-  if (isAllowFrom) {
-    if (aPolicy.Length() == allowFromLen ||
-        (aPolicy[allowFromLen] != ' ' && aPolicy[allowFromLen] != '\t')) {
-      ReportXFOViolation(curDocShellItem, uri, eALLOWFROM);
-      return false;
-    }
-    rv = NS_NewURI(getter_AddRefs(uri), Substring(aPolicy, allowFromLen));
-    if (NS_FAILED(rv)) {
-      return false;
-    }
-    bool isPrivateWin =
-        topDoc->NodePrincipal()->OriginAttributesRef().mPrivateBrowsingId > 0;
-    rv = ssm->CheckSameOriginURI(uri, topUri, true, isPrivateWin);
-    if (NS_FAILED(rv)) {
-      ReportXFOViolation(curDocShellItem, uri, eALLOWFROM);
-      return false;
-    }
-  }
-
   return true;
 }
 
@@ -259,81 +305,3 @@ bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel,
 
   return true;
 }
-
-/* static */
-void FramingChecker::ReportXFOViolation(nsIDocShellTreeItem* aTopDocShellItem,
-                                        nsIURI* aThisURI, XFOHeader aHeader) {
-  MOZ_ASSERT(aTopDocShellItem, "Need a top docshell");
-
-  nsCOMPtr<nsPIDOMWindowOuter> topOuterWindow = aTopDocShellItem->GetWindow();
-  if (!topOuterWindow) {
-    return;
-  }
-
-  nsPIDOMWindowInner* topInnerWindow = topOuterWindow->GetCurrentInnerWindow();
-  if (!topInnerWindow) {
-    return;
-  }
-
-  nsCOMPtr<nsIURI> topURI;
-
-  nsCOMPtr<Document> document = aTopDocShellItem->GetDocument();
-  nsresult rv = document->NodePrincipal()->GetURI(getter_AddRefs(topURI));
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  if (!topURI) {
-    return;
-  }
-
-  nsCString topURIString;
-  nsCString thisURIString;
-
-  rv = topURI->GetSpec(topURIString);
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  rv = aThisURI->GetSpec(thisURIString);
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  nsCOMPtr<nsIConsoleService> consoleService =
-      do_GetService(NS_CONSOLESERVICE_CONTRACTID);
-  nsCOMPtr<nsIScriptError> errorObject =
-      do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
-
-  if (!consoleService || !errorObject) {
-    return;
-  }
-
-  nsString msg = NS_LITERAL_STRING("Load denied by X-Frame-Options: ");
-  msg.Append(NS_ConvertUTF8toUTF16(thisURIString));
-
-  switch (aHeader) {
-    case eDENY:
-      msg.AppendLiteral(" does not permit framing.");
-      break;
-    case eSAMEORIGIN:
-      msg.AppendLiteral(" does not permit cross-origin framing.");
-      break;
-    case eALLOWFROM:
-      msg.AppendLiteral(" does not permit framing by ");
-      msg.Append(NS_ConvertUTF8toUTF16(topURIString));
-      msg.Append('.');
-      break;
-  }
-
-  // It is ok to use InitWithSanitizedSource, because the source string is
-  // empty.
-  rv = errorObject->InitWithSanitizedSource(
-      msg, EmptyString(), EmptyString(), 0, 0, nsIScriptError::errorFlag,
-      "X-Frame-Options", topInnerWindow->WindowID());
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  consoleService->LogMessage(errorObject);
-}

dom/security/FramingChecker.h

@@ -22,14 +22,24 @@ class FramingChecker {
                                 nsIContentSecurityPolicy* aCSP);
 
  protected:
-  enum XFOHeader { eDENY, eSAMEORIGIN, eALLOWFROM };
+  enum XFOHeader { eDENY, eSAMEORIGIN };
+
+  /**
+   * Logs to the window about a X-Frame-Options error.
+   *
+   * @param aMessageTag the error message identifier to log
+   * @param aParentDocShellItem the containing docshell that the frame is
+   * loading into
+   * @param aChildURI the URI of the frame attempting to load
+   * @param aPolicy the header value string from the frame
+   */
+  static void ReportError(const char* aMessageTag,
+                          nsIDocShellTreeItem* aParentDocShellItem,
+                          nsIURI* aChildURI, const nsAString& aPolicy);
 
   static bool CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
                                          const nsAString& aPolicy,
                                          nsIDocShell* aDocShell);
-
-  static void ReportXFOViolation(nsIDocShellTreeItem* aTopDocShellItem,
-                                 nsIURI* aThisURI, XFOHeader aHeader);
 };
 
 #endif /* mozilla_dom_FramingChecker_h */