PlzNavigate: Enforce 'form-action' CSP on the browser-side.

content/browser/frame_host/form_submission_throttle_browsertest.cc

Index: content/browser/frame_host/form_submission_throttle_browsertest.cc
diff --git a/content/browser/frame_host/form_submission_throttle_browsertest.cc b/content/browser/frame_host/form_submission_throttle_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f1ef5d5684fe02e59aed05d7507b3fc9a3bbc9e4
--- /dev/null
+++ b/content/browser/frame_host/form_submission_throttle_browsertest.cc
@@ -0,0 +1,142 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#include "content/browser/frame_host/form_submission_throttle.h"
+
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/common/browser_side_navigation_policy.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "url/url_constants.h"
+#include "url/url_util.h"
+
+namespace content {
+
+class FormSubmissionBrowserTest : public ContentBrowserTest {
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(FormSubmissionBrowserTest,
+                       CheckContentSecurityPolicyFormAction) {
+  // The FormSubmittionThrottle aren't used without PlzNavigate.

[alexmos, 2017/02/28 02:48:46]

nit: FormSubmittionThrottle -> FormSubmissionThrottle, and aren't -> isn't (also
in second test)

[arthursonzogni, 2017/03/07 16:25:51]

Done.

+  if (!IsBrowserSideNavigationEnabled())
+    return;
+
+  const struct {
+    GURL main_page_url;
+    GURL form_page_url;
+    NavigationThrottle::ThrottleCheckResult start_expectation;
+    NavigationThrottle::ThrottleCheckResult redirect_expectation;
+  } kTestCases[] = {
+      // Form submissions are allowed by default when there is not CSP.
+      {
+          embedded_test_server()->GetURL(
+              "/form_submission_throttle/no_csp.html"),
+          embedded_test_server()->GetURL("/simple_page.html"),
+          NavigationThrottle::PROCEED,  // start expectation.
+          NavigationThrottle::PROCEED   // redirect expectation.
+      },
+
+      // No form submission is allowed when the calling RenderFrameHost's CSP
+      // is "form-action 'none'".
+      {
+          embedded_test_server()->GetURL(
+              "/form_submission_throttle/form_action_none.html"),
+          embedded_test_server()->GetURL("/simple_page.html"),
+          NavigationThrottle::CANCEL,  // start expectation.
+          NavigationThrottle::CANCEL   // redirect expectation.
+      },
+
+      // The path of the source-expression is only enforced when there is no
+      // redirection. By using this behavior, this test can checks a case where

[alexmos, 2017/02/28 02:48:46]

Just curious, is this because of what the spec says?  It strikes me as somewhat
non-intuitive behavior.

[Mike West, 2017/03/02 10:45:34]

Totally non-intuitive. Totally what the spec says. See
https://www.w3.org/TR/CSP2/#source-list-paths-and-redirects for detail.

[arthursonzogni, 2017/03/07 16:25:51]

Acknowledged.

+      // the request is canceled in WillStartRequest() but not in
+      // WillRedirectRequest().
+      {
+          embedded_test_server()->GetURL(
+              "/form_submission_throttle/form_action_with_path.html"),
+          embedded_test_server()->GetURL("/not_the_file.html"),
+          NavigationThrottle::CANCEL,  // start expectation.
+          NavigationThrottle::PROCEED  // redirect expectation.
+      },
+  };
+
+  for (const auto& test : kTestCases) {
+    SCOPED_TRACE(testing::Message()
+                 << std::endl
+                 << "main_page_url = " << test.main_page_url << std::endl
+                 << "form_page_url = " << test.form_page_url << std::endl);
+
+    // Load the main page.
+    EXPECT_TRUE(NavigateToURL(shell(), test.main_page_url));
+
+    // Build a new form submission navigation.
+    FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                              ->GetFrameTree()
+                              ->root();
+    std::unique_ptr<NavigationHandle> handle = NavigationHandleImpl::Create(
+        test.form_page_url,      // url
+        std::vector<GURL>(),     // redirect chain
+        root,                    // frame_tree_node
+        true,                    // is_renderer_initiated
+        false,                   // is_same_page
+        base::TimeTicks::Now(),  // navigation_start
+        0,                       // pending_nav_entry_id
+        false,                   // started_from_context_menu
+        false,                   // should_bypass_main_world_csp
+        true);                   // is_form_submission
+
+    // Test the expectations with a FormSubmissionThrottle.
+    std::unique_ptr<NavigationThrottle> throttle =
+        FormSubmissionThrottle::MaybeCreateThrottleFor(handle.get());
+    ASSERT_TRUE(throttle);
+    EXPECT_EQ(test.start_expectation, throttle->WillStartRequest());
+    EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest());

[alexmos, 2017/02/28 02:48:46]

Maybe not in this test, but it'd be nice to add an end-to-end test that performs
an actual form navigation (1) has some coverage that is_form_submission() is
initialized properly, and (2) verifies that you land on the error page when
blocked by the throttle.  Or perhaps this is covered elsewhere?

[Mike West, 2017/03/02 10:45:34]

Are we landing on an error page? That sounds great, but I thought it was blocked
on changing the error page behavior in such a way as to commit a reasonable URL
(we talked about this in the context of XFO, as I recall).

[arthursonzogni, 2017/03/07 16:25:51]

There is some layout tests that does what you want to do. One problem could be
that the form-action CSP is checked on both side, it is mainly the renderer-side
one that is blocking the request. Maybe I would have to disable it on the
renderer-side too in order to test it too.
I didn't use an error page when the form-action directive is infringed. I could
by returning BLOCK_REQUEST instead of CANCEL in the NavigationThrottle.

[Mike West, 2017/03/09 08:20:04]

If alexmos@ and creis@ are ok with landing on an error page (I recall some
issues with the Chrome Web Store's in particular being committed), then I'd
prefer it to a blank page.

[arthursonzogni, 2017/03/10 09:35:37]

FYI: We currently have some issue with the error pages. This patch should fix
them
https://codereview.chromium.org/2736863003

A new patch will ensure that there will be no problems with CWS and error pages:
https://codereview.chromium.org/2736863003/

Using an error page with form-action is not required for this patch to launch.
When there will be no more problems with the error pages, I will enable them.

+  }
+}
+
+IN_PROC_BROWSER_TEST_F(FormSubmissionBrowserTest,
+                       CheckContentSecurityPolicyFormActionBypassCSP) {
+  // The FormSubmittionThrottle aren't used without PlzNavigate.
+  if (!IsBrowserSideNavigationEnabled())
+    return;
+
+  GURL main_url = embedded_test_server()->GetURL(
+      "/form_submission_throttle/form_action_none.html");
+  GURL form_url = embedded_test_server()->GetURL("/simple_page.html");
+
+  // Load the main page.
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  // Build a new form submission navigation.
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  std::unique_ptr<NavigationHandle> handle =
+      NavigationHandleImpl::Create(form_url,             // url
+                                   std::vector<GURL>(),  // redirect chain
+                                   root,                 // frame_tree_node
+                                   true,   // is_renderer_initiated
+                                   false,  // is_same_page
+                                   base::TimeTicks::Now(),  // navigation_start
+                                   0,      // pending_nav_entry_id
+                                   false,  // started_from_context_menu
+                                   true,   // should_bypass_main_world_csp
+                                   true);  // is_form_submission
+
+  // Test the expectations with a FormSubmissionThrottle.

[alexmos, 2017/02/28 02:48:46]

Can you add a short comment to make it more obvious why these should PROCEED? 
I.e., because should_bypass_main_world_csp is true?

[arthursonzogni, 2017/03/07 16:25:51]

Done.

+  std::unique_ptr<NavigationThrottle> throttle =
+      FormSubmissionThrottle::MaybeCreateThrottleFor(handle.get());
+  ASSERT_TRUE(throttle);
+  EXPECT_EQ(NavigationThrottle::PROCEED, throttle->WillStartRequest());
+  EXPECT_EQ(NavigationThrottle::PROCEED, throttle->WillRedirectRequest());
+}
+
+}  // namespace content