Propagating csp attribute changes on frameOwnerPropertiesChanged()

content/browser/site_per_process_browsertest.cc

 // 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/site_per_process_browsertest.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 556 matching lines @@
   const double kDeviceScaleFactor = 2.0;
 
   SitePerProcessHighDPIBrowserTest() {}
 
  protected:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     SitePerProcessBrowserTest::SetUpCommandLine(command_line);
     command_line->AppendSwitchASCII(
         switches::kForceDeviceScaleFactor,
         base::StringPrintf("%f", kDeviceScaleFactor));
+    // TODO(amalika): Remove this switch when
+    // the EmbedderCSPEnforcement becomes stable

[alexmos, 2016/09/30 06:20:42]

nit: wrap comments at 80 chars (this is fewer than that).

+    command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,

[alexmos, 2016/09/30 06:20:42]

Instead of doing this here, let's define a new class which extends
SitePerProcessBrowserTest and adds this flag.  Then put the new test in that
class.  There are some existing examples of this here, such as 
SitePerProcessHighDPIBrowserTest or SitePerProcessIgnoreCertErrorsBrowserTest.

+                                    "EmbedderCSPEnforcement");
   }
 };
 
 // SitePerProcessIgnoreCertErrorsBrowserTest
 
 class SitePerProcessIgnoreCertErrorsBrowserTest
     : public SitePerProcessBrowserTest {
  public:
   SitePerProcessIgnoreCertErrorsBrowserTest() {}
 
@@ skipping 2437 matching lines @@
       EXPECT_EQ(expect_scrollbar, has_scrollbar(child->current_frame_host()));
     }
   }
 }
 
 // Verify that "marginwidth" and "marginheight" properties on frame elements
 // propagate to child frames correctly.
 IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
                        FrameOwnerPropertiesPropagationMargin) {
   GURL main_url(embedded_test_server()->GetURL(
-      "a.com", "/frame_owner_properties_margin.html"));
+      "a.com", "/frame_owner_properties_margin_and_csp.html"));
   NavigateToURL(shell(), main_url);
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
   ASSERT_EQ(1u, root->child_count());
 
   EXPECT_EQ(
       " Site A ------------ proxies for B\n"
       "   +--Site B ------- proxies for A\n"
       "Where A = http://a.com/\n"
@@ skipping 63 matching lines @@
         "window.domAutomationController.send("
         "document.body.getAttribute('marginheight'));",
         &actual_margin_height));
     EXPECT_EQ(base::IntToString(current_margin_height), actual_margin_height);
 
     current_margin_width += 5;
     current_margin_height += 10;
   }
 }
 
+// Verify that "csp" property on frame
+// elements ropagates to child frames correctly.

[alexmos, 2016/09/30 06:20:42]

nits: 
s/ropagates/propagates/
Please fix the wrapping in the comment (and similar ones below).

Also, how about including the link to a bug here?

+IN_PROC_BROWSER_TEST_F(SitePerProcessHighDPIBrowserTest,
+                       FrameOwnerPropertiesPropagationCSP) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/frame_owner_properties_margin_and_csp.html"));

[alexmos, 2016/09/30 06:20:42]

Instead of reusing the file, it might be cleaner to just create a new file,
frame_owner_properties_csp.html, with what you want.  Margin and csp seems like
a weird/unrelated combination. :)  (btw, currently, I don't think the
FrameOwnerPropertiesPropagationMargin test is updated to the new filename).

+  NavigateToURL(shell(), main_url);

[alexmos, 2016/09/30 06:20:42]

nit: EXPECT_TRUE

+
+  // It is safe to obtain the root frame tree node here, as it doesn't change.
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  ASSERT_EQ(1u, root->child_count());
+
+  EXPECT_EQ(
+      " Site A ------------ proxies for B\n"
+      "   +--Site B ------- proxies for A\n"
+      "Where A = http://a.com/\n"
+      "      B = http://b.com/",
+      DepictFrameTree(root));
+
+  FrameTreeNode* child = root->child_at(0);
+
+  std::string csp;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      root,
+      "window.domAutomationController.send("
+      "document.getElementById('child-1').getAttribute('csp'));",
+      &csp));
+  EXPECT_EQ("object-src \'none\'", csp);
+
+  // Run the test over variety of parent/child cases.
+  GURL urls[] = {// Remote to remote.
+                 embedded_test_server()->GetURL("c.com", "/title2.html"),
+                 // Remote to local.
+                 embedded_test_server()->GetURL("a.com", "/title1.html"),
+                 // Local to remote.
+                 embedded_test_server()->GetURL("b.com", "/title2.html")};
+
+  std::vector<base::string16> csp_values = {

[alexmos, 2016/09/30 06:20:42]

why not just std::string and avoid all conversions?

+      base::ASCIIToUTF16("default-src a.com"),
+      base::ASCIIToUTF16("default-src b.com"),
+      base::ASCIIToUTF16("img-src c.com")};
+  base::string16 current_csp;
+
+  // Before each navigation, we change the csp property of the frame.
+  // We then check whether that property is applied
+  // correctly after the navigation has completed.
+  for (size_t i = 0; i < arraysize(urls); ++i) {
+    // Change csp before navigating.
+    current_csp = csp_values[i];
+    EXPECT_TRUE(ExecuteScript(
+        root,
+        base::StringPrintf("document.getElementById('child-1').setAttribute("
+                           "    'csp', '%s');",
+                           base::UTF16ToUTF8(current_csp).c_str())));

[alexmos, 2016/09/30 06:20:42]

nit: could just use csp_values[i] directly (here and below) without the temp
variable.

+
+    NavigateFrameToURL(child, urls[i]);
+
+    EXPECT_EQ(current_csp, child->frame_owner_properties().csp);

[alexmos, 2016/09/30 06:20:42]

Is it possible to observe that the passed CSP attribute took effect?  The point
of all those navigations is to check all possible ways the out-of-process child
frame can be created and ensure that the CSP value made it to the
RemoteFrameOwner in all cases.  It'd be nice to verify, for example, that you
can't include disallowed scripts/images/etc, according to the test policy for
each URL.  Or, alternatively, and probably easier, you could check that
csp_values[i] made it into
child->current_replication_state()->accumulated_csp_headers (meaning that the
frame committed with the right CSP header from the response, which would only
happen if the request pulled in the correct Embedding-CSP header).  Though I
guess that also requires the test server to support your new header?  If you
aren't that far along in your implementation, maybe add a TODO to validate this
later?

[amalika, 2016/09/30 07:48:51]

The deserialization and the algorithms have not been implemented yet as right
now we are doing the project in small patches but the intention was to indeed
expand this test. I will add a todo.

+  }
+}
+
 // Verify origin replication with an A-embed-B-embed-C-embed-A hierarchy.
 IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, OriginReplication) {
   GURL main_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b(c(a),b), a)"));
   EXPECT_TRUE(NavigateToURL(shell(), main_url));
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
 
   EXPECT_EQ(
@@ skipping 5096 matching lines @@
       " Site A ------------ proxies for B C\n"
       "   +--Site B ------- proxies for A C\n"
       "        +--Site C -- proxies for A B\n"
       "Where A = http://a.com/\n"
       "      B = http://b.com/\n"
       "      C = http://c.com/",
       DepictFrameTree(root));
 }
 
 }  // namespace content