PlzNavigate: Enforce 'frame-src' CSP on the browser.

content/browser/frame_host/csp_context_impl.h

+// Copyright 2017 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.
+
+#ifndef CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_IMPL_H_
+#define CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_IMPL_H_
+
+#include "content/common/content_security_policy/csp_context.h"
+
+namespace content {
+
+class FrameTreeNode;
+
+class CSPContextImpl : public CSPContext {

[nasko, 2017/02/15 21:28:44]

Why do we need to subclass? Also, missing a class level comment.

[arthursonzogni, 2017/02/16 17:32:40]

Because I have to implement the virtual methods

The CSPContext represents the system on which the CSP are checked on. A system
can report violation to the user, it can display messages on the console and it
tells you when the CSP should by bypassed.

By doing this, the CSP classes are not tied to anything like the CSP classes
inside blink were.

If one day, we want to check the CSP in another place, the network stack or the
renderer for instance, you can. You will just have to implement these 3
functions.

It is also useful in my test. I can make a custom implementation of CSPContext
to check the console message and test what happens when a scheme is bypassed by
the CSP for instance.
Done.

[alexmos, 2017/02/24 06:40:27]

As an alternative to this, would it be easier if RenderFrameHostImpl just
extended CSPContext and implemented the three functions directly?  Just curious
how this compares to having CSPContextImpl as an accessor on RFHI, since
CSPContextImpl is tied to its RFHI anyway.

[arthursonzogni, 2017/02/24 16:13:29]

Yes, it might be a good idea.
I am just a little bit afraid of the method names of CSPContext are too simple.
I will rename some of them. For instance
Allow()           => AllowContentSecurityPolicy(),
ReportViolation() => ReportContentSecurityPolicyViolation();

[alexmos, 2017/03/01 02:22:28]

Thanks for trying this out!  On one hand, I like the simplification and that we
don't need to pass the rfh->content_security_policies() as an arg anymore.  On
the other hand, it was nice to have all CSP methods grouped into one object,
i.e. it'd be still nice to write the Allow check as something like
rfh->content_security_policy()->Allow(...).  I'm not too thrilled by the name
AllowContentSecurityPolicy, as it sounds like it's "allowing a CSP", rather than
checking the frame's CSP to see if a directive is allowed; perhaps we can come
up with a better name.  I'm curious what Nasko thinks here.  Nasko?

[nasko, 2017/03/03 23:16:53]

I like the methods being folded into RFH and avoiding the Impl class. What if we
rename the allow method to be a bit more intuitive when read - IsAllowedByCsp?
Or something similar?

[alexmos, 2017/03/03 23:40:35]

I like IsAllowedByCsp() - I think we can go with that.

[arthursonzogni, 2017/03/06 15:09:02]

Done.

+ public:
+  CSPContextImpl(FrameTreeNode* frame_tree_node);
+  void LogToConsole(const std::string& message) override;
+
+  // Inform the renderer process that a navigation has been blocked by a content
+  // security policy.
+  void ReportViolation(const CSPViolationParams& violation_params) override;
+
+ private:
+  bool SchemeShouldBypassCSP(const base::StringPiece& scheme) override;
+
+  // Never nullptr;
+  FrameTreeNode* frame_tree_node_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_IMPL_H_ */