Move 'frame-src' CSP checks into FrameFetchContext.

third_party/WebKit/Source/core/loader/FrameFetchContext.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 508 matching lines @@
 
     // Don't send CSP messages for preloads, we might never actually display those items.
     ContentSecurityPolicy::ReportingStatus cspReporting = forPreload ?
         ContentSecurityPolicy::SuppressReport : ContentSecurityPolicy::SendReport;
 
     // m_document can be null, but not in any of the cases where csp is actually used below.
     // ImageResourceTest.MultipartImage crashes w/o the m_document null check.
     // I believe it's the Resource::Raw case.
     const ContentSecurityPolicy* csp = m_document ? m_document->contentSecurityPolicy() : nullptr;
 
+    // If we're loading a frame, grab it's parent's policy for 'frame-src' checks:
+    if (!csp && type == Resource::MainResource) {

[Yoav Weiss, 2016/06/02 09:15:14]

I understand that the assumption here is that csp is NULL (because m_document
is) when we're navigating to a frame.
Can you make that assumption explicit (comment, etc)?

+        if (Frame* parentFrame = frame()->tree().parent()) {
+            csp = parentFrame->securityContext()->contentSecurityPolicy();
+            if (!csp->allowChildFrameFromSource(url, redirectStatus, cspReporting)) {
+                // TODO(mkwst): If we cancel the request after a redirect, we never instantiate
+                // a document, and therefore don't inherit the loader's sandbox flags, or trigger
+                // a load event. This is strange.
+                if (redirectStatus == ResourceRequest::RedirectStatus::FollowedRedirect) {
+                    frame()->document()->enforceSandboxFlags(SandboxOrigin);
+                    frame()->owner()->dispatchLoad();
+                }
+                return ResourceRequestBlockedReasonCSP;
+            }

[Yoav Weiss, 2016/06/02 09:15:14]

should we return here? Or nullify csp? Otherwise, we've change csp to the
parent's, which may change code paths further down the function? (e.g. we'd call
csp->allowRequest when we previously didn't) I'm not sure that outcome would be
different, but still..

Alternatively, isolating the frame logic in a helper function would help making
sure it doesn't have such side effects on the rest of the function.

+        }
+    }
+
     if (csp) {
         if (!shouldBypassMainWorldCSP && !csp->allowRequest(resourceRequest.requestContext(), url, redirectStatus, cspReporting))
             return ResourceRequestBlockedReasonCSP;
     }
 
     if (type == Resource::Script || type == Resource::ImportResource) {
         ASSERT(frame());
         if (!frame()->loader().client()->allowScriptFromSource(!frame()->settings() || frame()->settings()->scriptEnabled(), url)) {
             frame()->loader().client()->didNotAllowScript();
             // TODO(estark): Use a different ResourceRequestBlockedReason
@@ skipping 274 matching lines @@
 }
 
 DEFINE_TRACE(FrameFetchContext)
 {
     visitor->trace(m_document);
     visitor->trace(m_documentLoader);
     FetchContext::trace(visitor);
 }
 
 } // namespace blink