Move securityCheck out of V8WrapperInstantiationScope

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

Index: third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
index 516ef4dff5498d95b703de67ff7f23e7350fcaca..63158363223ba37924c480e445a76b3da1497fe8 100644
--- a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
@@ -32,6 +32,8 @@
 
 #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"
@@ -271,4 +273,72 @@ void BindingSecurity::failedAccessCheckFor(v8::Isolate* 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