Simplify calculating sandbox flags

Currently, DocumentLoader sets FrameLoader::frame_owner_sandbox_flags_
only to immediately read that state back. Give FrameLoader a
GetForcedSandboxFlags() helper to access its permanent flags, then
have DocumentLoader assemble the state it needs rather than using
FrameLoader::EffectiveSandboxFlags().

The only other use of FrameLoader::EffectiveSandboxFlags() is a
test-only case. Switch it to use PendingEffectiveSandboxFlags(), since
all the tests seem to be happy with it.

Change-Id: Id58eda10c18843b3e2ddc4b962513507e2e66aeb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333335
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794799}

content/renderer/render_view_browsertest.cc

@@ -215,7 +215,6 @@ FrameReplicationState ReconstructReplicationStateForTesting(
   // public blink API...
   result.name = frame->AssignedName().Utf8();
   result.unique_name = test_render_frame->unique_name();
-  result.frame_policy.sandbox_flags = frame->EffectiveSandboxFlagsForTesting();
   // result.should_enforce_strict_mixed_content_checking is calculated in the
   // browser...
   result.origin = frame->GetSecurityOrigin();

third_party/blink/public/web/web_local_frame.h

@@ -544,15 +544,6 @@ class WebLocalFrame : public WebFrame {
 
   // Iframe sandbox ---------------------------------------------------------
 
-  // TODO(ekaramad): This method is only exposed for testing for certain tests
-  // outside of blink/ that are interested in approximate value of the
-  // FrameReplicationState. This method should be replaced with one in content/
-  // where the notion of FrameReplicationState is relevant to.
-  // Returns the effective sandbox flags which are inherited from their parent
-  // frame.
-  virtual network::mojom::WebSandboxFlags EffectiveSandboxFlagsForTesting()
-      const = 0;
-
   // Returns false if this frame, or any parent frame is sandboxed and does not
   // have the flag "allow-downloads" set.
   virtual bool IsAllowedToDownload() const = 0;

third_party/blink/renderer/core/frame/web_local_frame_impl.cc

@@ -2324,31 +2324,6 @@ void WebLocalFrameImpl::CopyImageAtForTesting(
   GetFrame()->CopyImageAtViewportPoint(IntPoint(pos_in_viewport));
 }
 
-network::mojom::blink::WebSandboxFlags
-WebLocalFrameImpl::EffectiveSandboxFlagsForTesting() const {
-  if (!GetFrame())
-    return network::mojom::blink::WebSandboxFlags::kNone;
-  network::mojom::blink::WebSandboxFlags flags =
-      GetFrame()->Loader().EffectiveSandboxFlags();
-  if (RuntimeEnabledFeatures::FeaturePolicyForSandboxEnabled()) {
-    // When some of sandbox flags set in the 'sandbox' attribute are implemented
-    // as policies they are removed form the FrameOwner's sandbox flags to avoid
-    // being considered again as part of inherited or CSP sandbox.
-    // Note: if the FrameOwner is remote then the effective flags would miss the
-    // part of sandbox converted to FeaturePolicies. That said, with
-    // FeaturePolicyForSandbox all such flags should be part of the document's
-    // FeaturePolicy. For certain flags such as "downloads", dedicated API
-    // should be used (see IsAllowedToDownload()).
-    auto* local_owner = GetFrame()->DeprecatedLocalOwner();
-    if (local_owner && local_owner->OwnerType() ==
-                           mojom::blink::FrameOwnerElementType::kIframe) {
-      flags |= To<HTMLIFrameElement>(local_owner)
-                   ->sandbox_flags_converted_to_feature_policies();
-    }
-  }
-  return flags;
-}
-
 bool WebLocalFrameImpl::IsAllowedToDownload() const {
   if (!GetFrame())
     return true;

third_party/blink/renderer/core/frame/web_local_frame_impl.h

@@ -248,8 +248,6 @@ class CORE_EXPORT WebLocalFrameImpl final
       const WebVector<WebString>& words) override;
   void SetContentSettingsClient(WebContentSettingsClient*) override;
   void ReloadImage(const WebNode&) override;
-  network::mojom::blink::WebSandboxFlags EffectiveSandboxFlagsForTesting()
-      const override;
   bool IsAllowedToDownload() const override;
   bool FindForTesting(int identifier,
                       const WebString& search_text,

third_party/blink/renderer/core/loader/document_loader.cc

@@ -1501,6 +1501,30 @@ void DocumentLoader::DidCommitNavigation() {
   }
 }
 
+network::mojom::blink::WebSandboxFlags DocumentLoader::CalculateSandboxFlags() {
+  auto sandbox_flags = GetFrameLoader().GetForcedSandboxFlags() |
+                       content_security_policy_->GetSandboxMask() |
+                       frame_policy_.sandbox_flags;
+  if (archive_) {
+    // The URL of a Document loaded from a MHTML archive is controlled by
+    // the Content-Location header. This would allow UXSS, since
+    // Content-Location can be arbitrarily controlled to control the
+    // Document's URL and origin. Instead, force a Document loaded from a
+    // MHTML archive to be sandboxed, providing exceptions only for creating
+    // new windows.
+    DCHECK(commit_reason_ == CommitReason::kRegular ||
+           commit_reason_ == CommitReason::kInitialization);
+    sandbox_flags |= (network::mojom::blink::WebSandboxFlags::kAll &
+                      ~(network::mojom::blink::WebSandboxFlags::kPopups |
+                        network::mojom::blink::WebSandboxFlags::
+                            kPropagatesToAuxiliaryBrowsingContexts));
+  } else if (commit_reason_ == CommitReason::kXSLT) {
+    // An XSLT document inherits sandbox flags from the document that create it.
+    sandbox_flags |= frame_->DomWindow()->GetSandboxFlags();
+  }
+  return sandbox_flags;
+}
+
 scoped_refptr<SecurityOrigin> DocumentLoader::CalculateOrigin(
     Document* owner_document,
     network::mojom::blink::WebSandboxFlags sandbox_flags) {
@@ -1635,31 +1659,7 @@ void DocumentLoader::CommitNavigation() {
             .BoolValue());
   }
 
-  // Make the snapshot value of sandbox flags from the beginning of navigation
-  // available in frame loader, so that the value could be further used to
-  // initialize sandbox flags in security context. crbug.com/1026627
-  GetFrameLoader().SetFrameOwnerSandboxFlags(frame_policy_.sandbox_flags);
-
-  network::mojom::blink::WebSandboxFlags sandbox_flags =
-      GetFrameLoader().EffectiveSandboxFlags() |
-      content_security_policy_->GetSandboxMask();
-  if (archive_) {
-    // The URL of a Document loaded from a MHTML archive is controlled by
-    // the Content-Location header. This would allow UXSS, since
-    // Content-Location can be arbitrarily controlled to control the
-    // Document's URL and origin. Instead, force a Document loaded from a
-    // MHTML archive to be sandboxed, providing exceptions only for creating
-    // new windows.
-    DCHECK(commit_reason_ == CommitReason::kRegular ||
-           commit_reason_ == CommitReason::kInitialization);
-    sandbox_flags |= (network::mojom::blink::WebSandboxFlags::kAll &
-                      ~(network::mojom::blink::WebSandboxFlags::kPopups |
-                        network::mojom::blink::WebSandboxFlags::
-                            kPropagatesToAuxiliaryBrowsingContexts));
-  } else if (commit_reason_ == CommitReason::kXSLT) {
-    sandbox_flags |= frame_->DomWindow()->GetSandboxFlags();
-  }
-
+  auto sandbox_flags = CalculateSandboxFlags();
   auto security_origin = CalculateOrigin(owner_document, sandbox_flags);
 
   GlobalObjectReusePolicy global_object_reuse_policy =

third_party/blink/renderer/core/loader/document_loader.h

@@ -339,6 +339,7 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
       mojom::MHTMLLoadResult::kSuccess;
 
  private:
+  network::mojom::blink::WebSandboxFlags CalculateSandboxFlags();
   scoped_refptr<SecurityOrigin> CalculateOrigin(
       Document* owner_document,
       network::mojom::blink::WebSandboxFlags);

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -214,7 +214,14 @@ ResourceRequest FrameLoader::ResourceRequestForReload(
 FrameLoader::FrameLoader(LocalFrame* frame)
     : frame_(frame),
       progress_tracker_(MakeGarbageCollected<ProgressTracker>(frame)),
-      forced_sandbox_flags_(network::mojom::blink::WebSandboxFlags::kNone),
+      // Frames need to inherit the sandbox flags of their parent frame.
+      // These can be fixed at construction time, because the only actions that
+      // trigger a sandbox flags change in the parent will necessarily detach
+      // this frame.
+      forced_sandbox_flags_(
+          frame_->Tree().Parent()
+              ? frame_->Tree().Parent()->GetSecurityContext()->GetSandboxFlags()
+              : network::mojom::blink::WebSandboxFlags::kNone),
       dispatching_did_clear_window_object_in_main_world_(false),
       detached_(false),
       virtual_time_pauser_(
@@ -1494,11 +1501,6 @@ void FrameLoader::ForceSandboxFlags(
   forced_sandbox_flags_ |= flags;
 }
 
-void FrameLoader::SetFrameOwnerSandboxFlags(
-    network::mojom::blink::WebSandboxFlags flags) {
-  frame_owner_sandbox_flags_ = flags;
-}
-
 void FrameLoader::DispatchDidClearDocumentOfWindowObject() {
   if (state_machine_.CreatingInitialEmptyDocument())
     return;
@@ -1532,32 +1534,11 @@ void FrameLoader::DispatchDidClearWindowObjectInMainWorld() {
   Client()->DispatchDidClearWindowObjectInMainWorld();
 }
 
-network::mojom::blink::WebSandboxFlags FrameLoader::EffectiveSandboxFlags()
-    const {
-  network::mojom::blink::WebSandboxFlags flags = forced_sandbox_flags_;
-  if (frame_->Owner()) {
-    // Cannot use flags in frame_owner->GetFramePolicy().sandbox_flags, because
-    // frame_owner's frame policy is volatile and can be changed by javascript
-    // before navigation commits. Uses a snapshot
-    // value(frame_owner_sandbox_flags_) which is set in
-    // DocumentInit::WithFramePolicy instead. crbug.com/1026627
-    DCHECK(frame_owner_sandbox_flags_.has_value());
-    flags |= frame_owner_sandbox_flags_.value();
-  }
-  // Frames need to inherit the sandbox flags of their parent frame.
-  if (Frame* parent_frame = frame_->Tree().Parent())
-    flags |= parent_frame->GetSecurityContext()->GetSandboxFlags();
-  return flags;
-}
-
 network::mojom::blink::WebSandboxFlags
 FrameLoader::PendingEffectiveSandboxFlags() const {
   network::mojom::blink::WebSandboxFlags flags = forced_sandbox_flags_;
   if (FrameOwner* frame_owner = frame_->Owner())
     flags |= frame_owner->GetFramePolicy().sandbox_flags;
-  // Frames need to inherit the sandbox flags of their parent frame.
-  if (Frame* parent_frame = frame_->Tree().Parent())
-    flags |= parent_frame->GetSecurityContext()->GetSandboxFlags();
   return flags;
 }
 

third_party/blink/renderer/core/loader/frame_loader.h

@@ -148,16 +148,9 @@ class CORE_EXPORT FrameLoader final {
   // sandbox attribute of any parent frames.
   void ForceSandboxFlags(network::mojom::blink::WebSandboxFlags flags);
 
-  // Set frame_owner's effective sandbox flags, which are sandbox flags value
-  // at the beginning of navigation.
-  void SetFrameOwnerSandboxFlags(network::mojom::blink::WebSandboxFlags flags);
-
-  // Includes the collection of forced, inherited, and FrameOwner's sandbox
-  // flags, where the FrameOwner's flag is snapshotted from the last committed
-  // navigation. Note: with FeaturePolicyForSandbox the frame owner's sandbox
-  // flags only includes the flags which are *not* implemented as feature
-  // policies already present in the FrameOwner's ContainerPolicy.
-  network::mojom::blink::WebSandboxFlags EffectiveSandboxFlags() const;
+  network::mojom::blink::WebSandboxFlags GetForcedSandboxFlags() const {
+    return forced_sandbox_flags_;
+  }
 
   // Includes the collection of forced, inherited, and FrameOwner's sandbox
   // flags. Note: with FeaturePolicyForSandbox the frame owner's sandbox flags
@@ -310,14 +303,6 @@ class CORE_EXPORT FrameLoader final {
   std::unique_ptr<ClientNavigationState> client_navigation_;
 
   network::mojom::blink::WebSandboxFlags forced_sandbox_flags_;
-  // A snapshot value of frame_owner's sandbox flags states at the beginning of
-  // navigation. For main frame which does not have a frame owner, the value is
-  // base::nullopt.
-  // The snapshot value is needed because of potential racing conditions on
-  // sandbox attribute on iframe element.
-  // crbug.com/1026627
-  base::Optional<network::mojom::blink::WebSandboxFlags>
-      frame_owner_sandbox_flags_ = base::nullopt;
 
   bool dispatching_did_clear_window_object_in_main_world_;
   bool detached_;