Only do security checks on javascript: URLs for frames for loading

third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Simon Hausmann (hausmann@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2006, 2008, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 32 matching lines @@
 using namespace HTMLNames;
 
 HTMLFrameElementBase::HTMLFrameElementBase(const QualifiedName& tagName, Document& document)
     : HTMLFrameOwnerElement(tagName, document)
     , m_scrollingMode(ScrollbarAuto)
     , m_marginWidth(-1)
     , m_marginHeight(-1)
 {
 }
 
-bool HTMLFrameElementBase::isURLAllowed() const
+bool HTMLFrameElementBase::isURLAllowed(ReasonForCallingURLAllowed reason) const
 {
     if (m_URL.isEmpty())
         return true;
 
     const KURL& completeURL = document().completeURL(m_URL);
 
-    if (protocolIsJavaScript(completeURL)) {
-        if (contentFrame() && !ScriptController::canAccessFromCurrentOrigin(toIsolate(&document()), contentFrame()))
-            return false;
+    if (reason == WillLoadURL && protocolIsJavaScript(completeURL)) {

[dcheng, 2016/07/27 15:15:58]

I wonder if we should just cache the result of this and have the various checks
consult the saved bit? It's not super intuitive (IMO) to callers what the right
enum value to pass should be, and I worry that if we add new calls, it'd be easy
to get it wrong.

[jochen (gone - plz use gerrit), 2016/07/27 15:19:37]

well, if you want to do layout and get it wrong, you'll crash in the
SECURITY_CHECK below.

If you want to load and use the layout check instead, we might be toast anyways,
if there is no cached value yet :-/

[dcheng, 2016/07/27 15:39:06]

I'm kind of thinking we should just reset this to false in insertedInto and
removedFrom: that would solve that issue right?

+        if (contentFrame()) {
+            v8::Isolate* isolate = toIsolate(&document());
+            SECURITY_CHECK(isolate->InContext());
+            if (!ScriptController::canAccessFromCurrentOrigin(toIsolate(&document()), contentFrame()))

[dcheng, 2016/07/27 15:15:58]

I wonder if we should remove the isolate->InContext() branch in
canAccessFromCurrentOrigin()?

[jochen (gone - plz use gerrit), 2016/07/27 15:19:37]

done

+                return false;
+        }
     }
 
     LocalFrame* parentFrame = document().frame();
     if (parentFrame)
         return parentFrame->isURLAllowed(completeURL);
 
     return true;
 }
 
 void HTMLFrameElementBase::openURL(bool replaceCurrentItem)
 {
-    if (!isURLAllowed())
+    if (!isURLAllowed(WillLoadURL))
         return;
 
     if (m_URL.isEmpty())
         m_URL = AtomicString(blankURL().getString());
 
     LocalFrame* parentFrame = document().frame();
     if (!parentFrame)
         return;
 
     // Support for <frame src="javascript:string">
@@ skipping 163 matching lines @@
     frameOwnerPropertiesChanged();
 }
 
 void HTMLFrameElementBase::setMarginHeight(int marginHeight)
 {
     m_marginHeight = marginHeight;
     frameOwnerPropertiesChanged();
 }
 
 } // namespace blink