Propagating csp attribute changes on frameOwnerPropertiesChanged()

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 80c95ff99ac5d34d5842621b0052d0d123cde54d..9fb0be8d73d6b87248d4ce2e89a4ca29fd0025e2 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -574,6 +574,10 @@ class SitePerProcessHighDPIBrowserTest : public SitePerProcessBrowserTest {
     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");
   }
 };
 
@@ -3031,7 +3035,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
 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.
@@ -3115,6 +3119,67 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   }
 }
 
+// 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(