Move securityCheck out of V8WrapperInstantiationScope

third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp

 /*
  * Copyright (C) 2009 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 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "bindings/core/v8/BindingSecurity.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/V8Location.h"
+#include "bindings/core/v8/WrapperCreationSecurityCheck.h"
 #include "core/dom/Document.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Location.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLFrameElementBase.h"
 #include "core/workers/MainThreadWorkletGlobalScope.h"
 #include "platform/weborigin/SecurityOrigin.h"
 
 namespace blink {
@@ skipping 219 matching lines @@
   // TODO(dcheng): Add ContextType, interface name, and property name as
   // arguments, so the generated exception can be more descriptive.
   ExceptionState exceptionState(isolate, ExceptionState::UnknownContext,
                                 nullptr, nullptr);
   exceptionState.throwSecurityError(
       targetWindow->sanitizedCrossDomainAccessErrorMessage(
           currentDOMWindow(isolate)),
       targetWindow->crossDomainAccessErrorMessage(currentDOMWindow(isolate)));
 }
 
+bool BindingSecurity::canEnterCreationContext(

[Yuki, 2017/03/31 09:49:37]

nit: Could you place the implementation in the same order to the declaration in
the header?
The style guide recommends the same order.

[adithyas, 2017/03/31 17:49:28]

Fixed

+    v8::Isolate* isolate,
+    v8::Local<v8::Context> currentContext,
+    v8::Local<v8::Context> creationContext,
+    ExceptionState& exceptionState) {
+  if (currentContext.IsEmpty())

[Yuki, 2017/03/31 09:49:37]

This must never happen, I think.

+    return false;
+
+  // If the context is different, we need to make sure that the current
+  // context has access to the creation context.
+  LocalFrame* frame = toLocalFrameIfNotDetached(creationContext);
+  if (!frame) {
+    // Sandbox detached frames - they can't create cross origin objects.
+    LocalDOMWindow* callingWindow = currentDOMWindow(isolate);
+    LocalDOMWindow* targetWindow = toLocalDOMWindow(creationContext);
+    if (shouldAllowAccessToDetachedWindow(callingWindow, targetWindow,
+                                          exceptionState)) {
+      return true;
+    }
+
+    CHECK_EQ(SecurityError, exceptionState.code());

[Yuki, 2017/03/31 09:49:37]

This is guaranteed.  If you'd like to add a CHECK, then it's better to put in
canAccessFrame.

[adithyas, 2017/03/31 17:49:28]

Sorry, this CHECK existed before, and I wasn't quite sure why. I've removed it
now.

+    return false;
+  }
+  const DOMWrapperWorld& currentWorld = DOMWrapperWorld::world(currentContext);
+  RELEASE_ASSERT(currentWorld.worldId() ==

[Yuki, 2017/03/31 09:49:37]

s/RELEASE_ASSERT/CHECK_EQ/

+                 DOMWrapperWorld::world(creationContext).worldId());
+
+  if (currentWorld.isMainWorld() &&
+      !shouldAllowAccessToFrame(currentDOMWindow(isolate), frame,
+                                exceptionState)) {
+    CHECK_EQ(SecurityError, exceptionState.code());
+    return false;
+  }
+
+  return true;
+}
+
+void BindingSecurity::wrapperCreationSecurityCheck(
+    v8::Isolate* isolate,
+    v8::Local<v8::Context> currentContext,

[Yuki, 2017/03/31 09:49:37]

I'd suggest

s/currentContext/accessingContext/
s/creationContext/targetContext/

or, if you think that this is specific to the wrapper creation and you'd like to
say "creationContext", then I'd recommend not to take |currentContext|.  You can
use isolate->GetCurrentContext() which is exactly the current context while
|currentContext| is not guaranteed to be the current context.

[adithyas, 2017/03/31 17:49:28]

I do think this is specific to wrapper creation so I will use
isolate->GetCurrentContext().

+    v8::Local<v8::Context> creationContext,
+    const WrapperTypeInfo* type,
+    v8::Local<v8::Value> crossContextException) {
+  ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext,
+                                type->interfaceName);
+
+  // According to
+  // https://html.spec.whatwg.org/multipage/browsers.html#security-location,
+  // cross-origin script access to a few properties of Location is allowed.
+  // Location already implements the necessary security checks.
+  if (type->equals(&V8Location::wrapperTypeInfo)) {
+    if (!crossContextException.IsEmpty()) {

[Yuki, 2017/03/31 09:49:37]

I think this function is "rethrow an exception in a secure manner", and if there
is no exception, nothing to do here at all.  Why don't you do the early-exit?

[adithyas, 2017/03/31 17:49:28]

Makes sense, changed to an early-exit.

+      // Convert cross-context exception to security error
+      LocalDOMWindow* callingWindow = currentDOMWindow(isolate);

[Yuki, 2017/03/31 09:49:37]

Be consistent with the arguments.  Here you ignore |currentContext| and use
currentDOMWindow(isolate).  If you really want to take the accessing context,
this should be toLocalDOMWindow(accessingContext).

[adithyas, 2017/03/31 17:49:28]

Fixed to use isolate->GetCurrentContext() / currentDOMWindow() consistently.

+      LocalDOMWindow* targetWindow = toLocalDOMWindow(creationContext);
+      exceptionState.throwSecurityError(
+          targetWindow->sanitizedCrossDomainAccessErrorMessage(callingWindow),
+          targetWindow->crossDomainAccessErrorMessage(callingWindow));
+    };
+  } else {

[Yuki, 2017/03/31 09:49:37]

You can return just after exceptionState.throwSecurityError (and it's
recommended because you shouldn't do more things after you threw an exception),
and then we don't need |else| here.  Please reduce the nest level.

[adithyas, 2017/03/31 17:49:28]

Done

+    if (canEnterCreationContext(isolate, currentContext, creationContext,

[Yuki, 2017/03/31 09:49:37]

I'd personally feel that this helper function doesn't help me understand the
code well.  Probably it would be easier to read if the code is expanded here.

Or, probably because the name "canEnterCreationContext" sounds like it's just
checking, but we're actually expecting it to throw an exception. 
(shouldAllowAccessTo should have the same issue, but I've already accustomed to
it...)

[adithyas, 2017/03/31 17:49:28]

Coming up with a good name is difficult here :) I think you're right, the helper
function does not make it easier to understand, so I've just inlined the
function.

+                                exceptionState) &&
+        !crossContextException.IsEmpty()) {
+      exceptionState.rethrowV8Exception(crossContextException);
+    }
+  }
+}
+
 }  // namespace blink