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

content/browser/frame_host/form_submission_throttle_browsertest.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[nasko, 2017/03/16 21:49:47]

No "(c)" and 2017

[alexmos, 2017/03/16 23:05:35]

nit: update year

[arthursonzogni, 2017/03/17 14:58:25]

Done.

[arthursonzogni, 2017/03/17 14:58:25]

Done.

+// 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 FormSubmissionThrottle aren't used without PlzNavigate.

[alexmos, 2017/03/16 23:05:35]

nit: s/aren't/isn't/ (here and below)

[arthursonzogni, 2017/03/17 14:58:25]

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.

[alexmos, 2017/03/16 23:05:35]

nit: s/not/no/

[arthursonzogni, 2017/03/17 14:58:25]

Done.

+      {
+          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/03/16 23:05:36]

nit: s/checks/check/

[arthursonzogni, 2017/03/17 14:58:25]

Done.

+      // the request is canceled in WillStartRequest() but not in
+      // WillRedirectRequest().
+      // See https://www.w3.org/TR/CSP2/#source-list-paths-and-redirects for
+      // details.
+      {
+          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(

[nasko, 2017/03/16 21:49:47]

This approach feels more like an unit test than a browser test. I'll defer to
alexmos@, but I would've expected to see actual form submission coming from the
renderer, using an observer to capture the results, and verify against the test
expectations.

[alexmos, 2017/03/16 23:05:35]

I had the same comment, and Arthur pointed out that we've got layout tests that
cover this:
https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_...

This should give us end-to-end coverage, given that we now skip the
renderer-side enforcement with PlzNavigate.

[arthursonzogni, 2017/03/17 14:58:25]

Yes, I wanted to do an unit test initially, but since I needed the setup a frame
tree and the render frame host, I had to make a browsertest.

So yes, there is a set of layout test that provides an end-to-end coverage.
I have added mine in:
https://codereview.chromium.org/2749863002/
https://codereview.chromium.org/2737343004/
https://codereview.chromium.org/2749233002/
And also some layout test in this CL.

Please note that blink is still partially enforcing the 'form-action' CSP with
PlzNavigate. It is not the ideal solution to prove that it works. That's why I
have duplicated the set of layout test such that the navigations are blocked on
redirect, so that it tests the browser-side implementation.

I found that when the navigation happens with several windows/frames, we don't
use the CSP of the right frame, I filled the bug: https://crbug.com/700964

What is the actual behavior with PlzNavigate?
Let say that there is two frame/window A and B, A initiates a form submission
and B is navigating.
renderer-side: check the url against A's CSP
browser-side: check the url and its redirects against B's CSP

And without PlzNavigate?
renderer-side(1): check the url against A's CSP.
renderer-side(2): check the url and its redirects against B's CSP.

According to me the right behavior is to only use A's CSP. We will be in a
better position on the browser-side to fix the problem(i.e. select A instead of
B in the FormSubmissionThrottle)

+        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());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(FormSubmissionBrowserTest,
+                       CheckContentSecurityPolicyFormActionBypassCSP) {
+  // The FormSubmissionThrottle 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 that the navigation is allowed because "should_by_pass_main_world_csp"
+  // is true, even if it is a form submission and the policy is
+  // "form-action 'none'".
+  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