Implement cross-origin attributes using access check interceptors.

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 12 matching lines @@
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
  * 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.
  */
 
 #include "bindings/core/v8/BindingSecurity.h"
 
+#include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/V8Binding.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"
 
@@ skipping 172 matching lines @@
   CHECK(target && !target->frame())
       << "This version of shouldAllowAccessToFrame() must be used only for "
       << "detached windows.";
   if (!target->document())
     return false;
   return canAccessFrame(accessingWindow,
                         target->document()->getSecurityOrigin(), target,
                         exceptionState);
 }
 
+void BindingSecurity::failedAccessCheckFor(v8::Isolate* isolate,
+                                           const Frame* target) {
+  // TODO(dcheng): See if this null check can be removed or hoisted to a
+  // different location.
+  if (!target)
+    return;
+
+  DOMWindow* targetWindow = target->domWindow();
+
+  // FIXME: We should modify V8 to pass in more contextual information (context,
+  // property, and object).
+  ExceptionState exceptionState(ExceptionState::UnknownContext, 0, 0,

[Yuki, 2016/12/07 12:03:40]

ExceptionState should be created in the call sites, where interface name, etc.
are clear.  It's better to be close as much as possible to the point that the
execution enters into Blink from V8.

It's also not good that we actually throw a v8::Exception here because V8 APIs
no longer work correctly having an exception.  It's better to *schedule* an
exception in the ExceptionState (with throwSecurityError) but not actually throw
it yet (the dtor of ExceptionState actually throws).

e.g.
    void foo() {
      BindingSecurity::failedAccessCheckFor(...);
    }
    void bar() { foo(); }
    void baz() {
      bar();
      do_something();  // do something with V8 APIs, but V8 APIs do nothing
because an exception is thrown.
    }
It's very hard to understand why do_something() doesn't work in a specific case
just seeing baz().

We'd like the following coding pattern.
    void foo(ExceptionState& exceptionState) {
      BindingSecurity::failedAccessCheckFor(...);
    }
    void bar(ExceptionState& exceptionState) {
      foo(exceptionState);
    }
    void baz(ExceptionState& exceptionState) {
      bar(exceptionState);
      if (exceptionState.hadException()) {
        return handle_error();
      }
      do_something();
    }
Then, it's a caller's duty to check exceptionState.hadException().

nit: Please use non-deprecated version of ExceptionState's ctors.  We no longer
need isolate->GetCurrentContext()->Global().

[haraken, 2016/12/08 08:21:02]

What do you mean? Note that BindingSecurity::failedAccessCheckFor is called by
V8 (via failedAccessCheckCallbackInMainThread or window's interceptor), not by
some method in Blink. I think it's just fine to throw an exception here.

[dcheng, 2016/12/08 09:04:10]

I moved this around to make it so the interceptors can invoke this helper as
well. Originally, I didn't expose this at all, but this leads to a problem:

A named getter interceptor can signal an access failure just by not setting a
return value. v8 will notice that there was no value returned, and call the
access check failed callback

However, a named setter interceptor can't easily signal failure. So it needs to
throw an exception. To make the implementation of the interceptors in Blink
consistent, I have them consistently throw exceptions on access check failures.

In a followup patch, we can actually improve the error messages further: since
we know the context in which it's being invoked, we can instantiate
ExceptionState with the interface name, property name, and the access type: the
net result should be that our SecurityErrors will be more useful afterwards.

[Yuki, 2016/12/09 07:02:49]

Both of failedAccessCheckCallbackInMainThread and window's interceptor are
Blink's functions, I think?  And it's not easy to guarantee that the call sites
return to V8 right after this function call.

In general, I think it's hard to track a *hidden* exception thrown in Blink
because most of code in Blink doesn't care about a possible exception unlike V8
(we don't have many returnIfAbruptCompletion).  If it does care about a possible
exception, then it should use an ExceptionState, and the ExceptionState should
be instantiated right after the entry from V8 to Blink.

As same as BindingSecurity::shouldAllowAccessTo, the call sites should
instantiate an ExceptionState with appropriate interface name, method/attribute
name, context, etc.
dcheng@, could you add a TODO comment at least for this point if you cannot make
it in this CL?  I'd recommend to do it in this CL since it shouldn't be so hard,
though.

[dcheng, 2016/12/09 07:35:20]

That's correct. However, if an interceptor is installed, v8 will not call the
failedAccessCheckCallbackInMainThread unless:
- That type (getter, setter, enumerator, etc) of interceptor is not installed.
- If that interceptor is installed *and* appeared to do nothing (which is
impossible to determine for setters) *and* threw no exceptions.
I agree with all this. But the reason this is in a shared helper is so that the
SecurityError exception can be generated consistently for the cross-origin
interceptor path: this is only used to generate errors for that specific case. I
think it is better than duplicating it into the generated bindings. Otherwise,
things like
https://codereview.chromium.org/2439013002/diff/480001/third_party/WebKit/Sou...
would have to build the exception manually. I'm happy to discuss this more
offline, if that would help clarify things.
I will add a TODO: it's not hard, but I'd like to minimize the number of
behavior changes in this CL, since it aims to reimplement our bindings for
cross-origin things.

+                                isolate->GetCurrentContext()->Global(),
+                                isolate);
+  exceptionState.throwSecurityError(
+      targetWindow->sanitizedCrossDomainAccessErrorMessage(
+          currentDOMWindow(isolate)),
+      targetWindow->crossDomainAccessErrorMessage(currentDOMWindow(isolate)));
+}
+
 }  // namespace blink