Move securityCheck out of V8WrapperInstantiationScope

third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h

 /*
  * 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 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * 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.
  */
 
 #ifndef V8DOMWrapper_h
 #define V8DOMWrapper_h
 
-#include "bindings/core/v8/BindingSecurity.h"
 #include "bindings/core/v8/DOMDataStore.h"
 #include "bindings/core/v8/ScriptWrappable.h"
 #include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/WrapperCreationSecurityCheck.h"
 #include "core/CoreExport.h"
 #include "v8/include/v8.h"
 #include "wtf/Compiler.h"
 #include "wtf/text/AtomicString.h"
 
 namespace blink {
 
 struct WrapperTypeInfo;
 
 class V8DOMWrapper {
@@ skipping 70 matching lines @@
   SECURITY_CHECK(toScriptWrappable(wrapper) == impl);
   return wrapper;
 }
 
 class V8WrapperInstantiationScope {
   STACK_ALLOCATED();
 
  public:
   V8WrapperInstantiationScope(v8::Local<v8::Object> creationContext,
                               v8::Isolate* isolate,
-                              bool withSecurityCheck)
+                              const WrapperTypeInfo* type)
       : m_didEnterContext(false),
         m_context(isolate->GetCurrentContext()),
         m_tryCatch(isolate),
-        m_convertExceptions(false) {
+        m_type(type) {
     // creationContext should not be empty. Because if we have an
     // empty creationContext, we will end up creating
     // a new object in the context currently entered. This is wrong.
     RELEASE_ASSERT(!creationContext.IsEmpty());
     v8::Local<v8::Context> contextForWrapper =
         creationContext->CreationContext();
 
     // For performance, we enter the context only if the currently running
     // context is different from the context that we are about to enter.
     if (contextForWrapper == m_context)
       return;
-    if (withSecurityCheck) {
-      securityCheck(isolate, contextForWrapper);
-    } else {
-      m_convertExceptions = true;
-    }
+
     m_context = v8::Local<v8::Context>::New(isolate, contextForWrapper);
     m_didEnterContext = true;
     m_context->Enter();
   }
 
   ~V8WrapperInstantiationScope() {
     if (!m_didEnterContext) {
       m_tryCatch.ReThrow();
       return;
     }
     m_context->Exit();
-    // Rethrow any cross-context exceptions as security error.
-    if (m_tryCatch.HasCaught()) {
-      if (m_convertExceptions) {
-        m_tryCatch.Reset();
-        convertException();
-      }
+
+    v8::Isolate* isolate = m_context->GetIsolate();
+    v8::Local<v8::Value> caughtException = m_tryCatch.Exception();
+
+    m_tryCatch.Reset();
+    WrapperCreationSecurityCheck::securityCheck(

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

This code seems expected to (re)throw an exception in a secure manner, rather
than just performing a security check.  Maybe better to change the name?

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

OK, changed to a more descriptive name.

+        isolate, isolate->GetCurrentContext(), m_context, m_type,
+        caughtException);
+
+    if (m_tryCatch.HasCaught())

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

You've reset m_tryCatch on line 160.  This is meaningless, isn't it?

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

Hmm, does Reset() completely disable the TryCatch? securityCheck can throw an
exception, and if it does throw an exception, the tryCatch would catch it and
HasCaught() would return true right?

[Yuki, 2017/04/03 08:29:25]

Ah, now I see the point.  Then, I'd prefer an early-exit here, too.

  if (!m_didEnterContext) {
    m_tryCatch.ReThrow();
    return;
  }
  m_context->Exit();

  if (!m_tryCatch.HasCaught())
    return;

  // A cross-context exception exists, so rethrow it.
  verifyContextAccessAndHandleCrossContextException();
  m_tryCatch.ReThrow();

Given the above logic, I'd feel that
  rethrowCrossContextException();
  m_tryCatch.ReThrow();
would be more straightforward because "(re)throw an exception" is the expected
behavior.  The following m_tryCatch.ReThrow() makes sense at a glance.

[adithyas, 2017/04/03 15:20:54]

I think verifyContextAccessAndHandleCrossContextException needs to be called
regardless of whether a cross context exception is thrown or not. The security
checks (shouldAllowAccessToFrame/DetachedWindow) inside the method needs to be
run regardless of whether a cross context exception was thrown.

I could maybe split it up into two methods: verifyContextAccess and
rethrowCrossContextException

if (!m_didEnterContext) {
  m_tryCatch.ReThrow();
  return;
}
m_context->Exit();

crossContextException = m_tryCatch.Exception();
m_tryCatch.Reset();

if (!verifyContextAccess()) { // verifyContextAccess throws a security error
when it returns false
  m_tryCatch.ReThrow();
  return;
}

if (crossContextException.Empty())
  return;

rethrowCrossContextException(crossContextException);
m_tryCatch.ReThrow();

WDYT?

[Yuki, 2017/04/05 07:59:19]

I'm getting better understanding.  The original implementation was correct about
when to perform the security check.

It's too late to perform the security check in the destructor.  We need to
perform the security check *BEFORE* we're going to create a new wrapper object. 
Otherwise, there may be a security leak.  On this point, the original
implementation was correct.  However, when the security check failed, we
shouldn't even try to create a new wrapper object (the original implementation
ignores the failure of the security check, I don't like it.  We should abort the
wrapper creation in that case).

       m_tryCatch.ReThrow();
-    }
   }
 
   v8::Local<v8::Context> context() const { return m_context; }
 
  private:
-  void securityCheck(v8::Isolate*, v8::Local<v8::Context> contextForWrapper);
-  void convertException();
-
   bool m_didEnterContext;
   v8::Local<v8::Context> m_context;
   v8::TryCatch m_tryCatch;
-  bool m_convertExceptions;
+  const WrapperTypeInfo* m_type;
 };
 
 }  // namespace blink
 
 #endif  // V8DOMWrapper_h