Re-enable Object.observe and add enforcement for security invariants.

src/runtime.cc

Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index e28783d4e06a5a71267830a6c5f4832c33d2599e..1e1da2f940dc107265be926385e0e7aa1f96f575 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -14817,7 +14817,7 @@ RUNTIME_FUNCTION(Runtime_IsAccessCheckNeeded) {
   SealHandleScope shs(isolate);
   ASSERT(args.length() == 1);
   CONVERT_ARG_CHECKED(HeapObject, obj, 0);
-  return isolate->heap()->ToBoolean(obj->IsAccessCheckNeeded());
+  return isolate->heap()->ToBoolean(obj->map()->is_access_check_needed());
 }
 
 
@@ -14902,32 +14902,48 @@ RUNTIME_FUNCTION(Runtime_ObservationWeakMapCreate) {
 }
 
 
+static bool ContextsHaveSameOrigin(Handle<Context> context1,
+                                   Handle<Context> context2) {
+  return *context1 == *context2 ||

[rossberg, 2014/04/30 11:28:32]

Drop this micro opt (i.e., just check the security token).

[rafaelw, 2014/05/02 03:22:32]

Done.

+      context1->security_token() == context2->security_token();
+}
+
+
 RUNTIME_FUNCTION(Runtime_IsAccessAllowedForObserver) {

[rossberg, 2014/04/30 11:28:32]

Rename this to talk about SameOrigin, not Access.

[rafaelw, 2014/05/02 03:22:32]

Done.

   HandleScope scope(isolate);
   ASSERT(args.length() == 3);
   CONVERT_ARG_HANDLE_CHECKED(JSFunction, observer, 0);
   CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 1);

[rossberg, 2014/04/30 11:28:32]

Side note: shouldn't this be JSReceiver, to include proxies?

Likewise, it seems that we currently do not handle observers that are function
proxies (i.e., the previous line also would have to be JSReceiver, not
JSFunction).

[rafaelw, 2014/05/02 03:22:32]

Per offline discussion with Adam, we're going to assume that proxies aren't
presently supported by O.o.

On 2014/04/30 11:28:32, rossberg wrote:

   RUNTIME_ASSERT(object->map()->is_access_check_needed());
-  CONVERT_ARG_HANDLE_CHECKED(Object, key, 2);
-  SaveContext save(isolate);
-  isolate->set_context(observer->context());
-  if (!isolate->MayNamedAccess(
-          object, isolate->factory()->undefined_value(), v8::ACCESS_KEYS)) {
-    return isolate->heap()->false_value();
-  }
-  bool access_allowed = false;
-  uint32_t index = 0;
-  if (key->ToArrayIndex(&index) ||
-      (key->IsString() && String::cast(*key)->AsArrayIndex(&index))) {
-    access_allowed =
-        isolate->MayIndexedAccess(object, index, v8::ACCESS_GET) &&
-        isolate->MayIndexedAccess(object, index, v8::ACCESS_HAS);
-  } else {
-    access_allowed =
-        isolate->MayNamedAccess(object, key, v8::ACCESS_GET) &&
-        isolate->MayNamedAccess(object, key, v8::ACCESS_HAS);
-  }
-  return isolate->heap()->ToBoolean(access_allowed);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, record, 2);
+
+  Handle<Context> observer_context(observer->context()->native_context(),
+      isolate);
+  Handle<Context> object_context(object->GetCreationContext());
+  Handle<Context> record_context(record->GetCreationContext());
+
+  return isolate->heap()->ToBoolean(
+      ContextsHaveSameOrigin(object_context, observer_context) &&
+      ContextsHaveSameOrigin(object_context, record_context));
+}
+
+
+RUNTIME_FUNCTION(Runtime_ObjectWasCreatedInCurrentOrigin) {
+  HandleScope scope(isolate);
+  ASSERT(args.length() == 1);
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, receiver, 0);
+
+  if (!receiver->map()->is_access_check_needed())
+    return isolate->heap()->true_value();

[rossberg, 2014/04/30 11:28:32]

Is this safe? It seems this would allow you to apply the getNotifier method from
one context to get the notifier for an object (even a locked-down one) from a
different context, and from there you can go to Function and boom.

[rafaelw, 2014/05/02 03:22:32]

This isn't really a concern since if we had access to this method from another
origin, the game would already be up -- and if we used our "own" then the only
gain would be passing an object from a remote origin -- which, again, should
have this bit set.

However, per the discussion about being conservative (and also because we're
already in a runtime call, so simply doing the origin check isn't much more
work), we'll always do that check.

FWIW, Adam prepaired this part of the patch and this check wasn't a perf
consideration it was to make the proxy tests pass -- which we are now punting.

On 2014/04/30 11:28:32, rossberg wrote:

+
+  // Given that proxies aren't currently exposed through the API, it's
+  // hard to imagine how they could end up with the access check needed bit set.
+  ASSERT(!receiver->IsJSProxy());
+
+  Handle<JSObject> object = Handle<JSObject>::cast(receiver);
+  Handle<Context> creation_context(object->GetCreationContext(), isolate);
+  return isolate->heap()->ToBoolean(
+      ContextsHaveSameOrigin(creation_context, isolate->native_context()));
 }