Skip to content

Commit

Permalink
Handle proxy object prototypes more correctly
Browse files Browse the repository at this point in the history
  • Loading branch information
jasnell committed Jul 19, 2024
1 parent d46550a commit b2041c2
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 9 deletions.
6 changes: 3 additions & 3 deletions src/workerd/api/worker-rpc.c++
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,7 @@ private:
jsg::JsObject object,
rpc::JsRpcTarget::CallParams::Reader callParams,
bool allowInstanceProperties) {
auto prototypeOfObject = KJ_ASSERT_NONNULL(js.obj().getPrototype().tryCast<jsg::JsObject>());
auto prototypeOfObject = KJ_ASSERT_NONNULL(js.obj().getPrototype(js).tryCast<jsg::JsObject>());

// Get the named property of `object`.
auto getProperty = [&](kj::StringPtr kjName) {
Expand Down Expand Up @@ -1231,7 +1231,7 @@ private:
}

// Decide whether the new object is a suitable RPC target.
if (object.getPrototype() == prototypeOfObject) {
if (object.getPrototype(js) == prototypeOfObject) {
// Yes. It's a simple object.
allowInstanceProperties = true;
} else if (object.isInstanceOf<JsRpcTarget>(js)) {
Expand Down Expand Up @@ -1506,7 +1506,7 @@ static MakeCallPipeline::Result makeCallPipeline(jsg::Lock& js, jsg::JsValue val
return MakeCallPipeline::NonPipelinable();
});

if (obj.getPrototype() == js.obj().getPrototype()) {
if (obj.getPrototype(js) == js.obj().getPrototype(js)) {
// It's a plain object.
kj::Maybe<v8::Local<v8::Function>> maybeDispose;
jsg::JsValue disposeProperty = obj.get(js, js.symbolDispose());
Expand Down
6 changes: 3 additions & 3 deletions src/workerd/io/worker.c++
Original file line number Diff line number Diff line change
Expand Up @@ -1548,7 +1548,7 @@ Worker::Worker(kj::Own<const Script> scriptParam,
return;
}

handle = KJ_UNWRAP_OR(handle.getPrototype().tryCast<jsg::JsObject>(), {
handle = KJ_UNWRAP_OR(handle.getPrototype(js).tryCast<jsg::JsObject>(), {
// Reached end of prototype chain.

// For historical reasons, we assume a class is a Durable Object
Expand Down Expand Up @@ -2047,7 +2047,7 @@ void Worker::Lock::validateHandlers(ValidationErrorReporter& errorReporter) {
js.withinHandleScope([&]() {
// Find the prototype for `Object` by creating one.
auto obj = js.obj();
jsg::JsValue prototypeOfObject = obj.getPrototype();
jsg::JsValue prototypeOfObject = obj.getPrototype(js);

// Walk the prototype chain.
jsg::JsObject ctor(KJ_ASSERT_NONNULL(entry.value.tryGetHandle(js.v8Isolate)));
Expand Down Expand Up @@ -2084,7 +2084,7 @@ void Worker::Lock::validateHandlers(ValidationErrorReporter& errorReporter) {
}
}

proto = protoObj.getPrototype();
proto = protoObj.getPrototype(js);
}
});
}
Expand Down
20 changes: 19 additions & 1 deletion src/workerd/jsg/jsvalue-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ struct JsValueContext: public ContextGlobalObject {
return js.date(0);
}

struct Foo: public Object {
JSG_RESOURCE_TYPE(Foo) {}
};

JsValue checkProxyPrototype(Lock& js, JsValue value) {
JSG_REQUIRE(value.isProxy(), TypeError, "not a proxy");
auto obj = KJ_ASSERT_NONNULL(value.tryCast<JsObject>());
return obj.getPrototype(js);
}

JSG_RESOURCE_TYPE(JsValueContext) {
JSG_METHOD(takeJsValue);
JSG_METHOD(takeJsString);
Expand All @@ -88,9 +98,11 @@ struct JsValueContext: public ContextGlobalObject {
JSG_METHOD(setRef);
JSG_METHOD(getRef);
JSG_METHOD(getDate);
JSG_METHOD(checkProxyPrototype);
JSG_NESTED_TYPE(Foo);
}
};
JSG_DECLARE_ISOLATE_TYPE(JsValueIsolate, JsValueContext);
JSG_DECLARE_ISOLATE_TYPE(JsValueIsolate, JsValueContext, JsValueContext::Foo);

KJ_TEST("simple") {
Evaluator<JsValueContext, JsValueIsolate> e(v8System);
Expand All @@ -113,6 +125,12 @@ KJ_TEST("simple") {
"TypeError: Failed to execute 'takeJsObject' on 'JsValueContext': parameter 1 "
"is not of type 'JsObject'.");
e.expectEval("getDate() instanceof Date", "boolean", "true");
e.expectEval("checkProxyPrototype(new Proxy(class extends Foo{}, {})) === Foo",
"boolean", "true");
e.expectEval("checkProxyPrototype(new Proxy({}, { getPrototypeOf() { return Foo; } } )) === Foo",
"boolean", "true");
e.expectEval("checkProxyPrototype(new Proxy({}, { getPrototypeOf() { return String; } } )) "
"=== Foo", "boolean", "false");
}

} // namespace
Expand Down
46 changes: 46 additions & 0 deletions src/workerd/jsg/jsvalue.c++
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,52 @@ JsObject JsObject::jsonClone(Lock& js) {
return JsObject(obj);
}

JsValue JsObject::getPrototype(Lock& js) {
if (inner->IsProxy()) {
// Here we emulate the behavior of v8's GetPrototypeV2() function for proxies.
// If the proxy has a getPrototypeOf trap, we call it and return the result.
// Otherwise we return the prototype of the target object.
// Note that we do not check if the target object is extensible or not, or
// if the returned prototype is consistent with the target's prototype if
// the target is not extensible. See the comment below for more details.
auto proxy = inner.As<v8::Proxy>();
JSG_REQUIRE(!proxy->IsRevoked(), TypeError, "Proxy is revoked");
auto handler = proxy->GetHandler();
JSG_REQUIRE(handler->IsObject(), TypeError, "Proxy handler is not an object");
auto jsHandler = JsObject(handler.As<v8::Object>());
auto trap = jsHandler.get(js, "getPrototypeOf"_kj);
auto target = proxy->GetTarget();
if (trap.isUndefined()) {
JSG_REQUIRE(target->IsObject(), TypeError, "Proxy target is not an object");
// Run this through getPrototype to handle the case where the target is also a proxy.
return JsObject(target.As<v8::Object>()).getPrototype(js);
}
JSG_REQUIRE(trap.isFunction(), TypeError, "Proxy getPrototypeOf trap is not a function");
v8::Local<v8::Function> fn = ((v8::Local<v8::Value>)trap).As<v8::Function>();
v8::Local<v8::Value> args[] = { target };
auto ret = JsValue(check(fn->Call(js.v8Context(), jsHandler.inner, 1, args)));
JSG_REQUIRE(ret.isObject() || ret.isNull(), TypeError,
"Proxy getPrototypeOf trap did not return an object or null");
// TODO(maybe): V8 performs additional checks on the returned value to
// see if the proxy and the target are extensible or not, and if the
// returned prototype is consistent with the target's prototype if they
// are not extensible. To strictly match v8's behavior we should do the
// same but (a) v8 does not expose the necessary APIs to do so, and (b)
// it is not clear if we actually need to perform the additional check
// given how we are currently using this function.
if (target->IsObject() && jsg::JsObject(target.As<v8::Object>()) != ret) {
// This warning is only here to help us determine if the additional checks that
// v8 performs here might be necessary for us to match in the future. If we do
// have to match it, then we will likely need to do so with a compat flag and
// a v8 patch.
LOG_WARNING_PERIODICALLY("Proxy getPrototypeOf trap returned a different protototype "
"than the target's prototype");
}
return ret;
}
return JsValue(inner->GetPrototypeV2());
}

bool JsValue::isTruthy(Lock& js) const {
KJ_ASSERT(!inner.IsEmpty());
return inner->BooleanValue(js.v8Isolate);
Expand Down
2 changes: 1 addition & 1 deletion src/workerd/jsg/jsvalue.h
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ class JsObject final : public JsBase<v8::Object, JsObject> {
// Note that when called on a class constructor, this does NOT return `.prototype`, it still
// returns `.__proto__`. Usefully, though, a class constructor's `__proto__` is always the
// parent class's constructor.
inline JsValue getPrototype() { return JsValue(inner->GetPrototypeV2()); }
JsValue getPrototype(Lock& js) KJ_WARN_UNUSED_RESULT;

using JsBase<v8::Object, JsObject>::JsBase;

Expand Down
2 changes: 1 addition & 1 deletion src/workerd/jsg/ser.c++
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Serializer::Serializer(Lock& js, Options options)
kj::requireOnStack(this, "jsg::Serializer must be allocated on the stack");
#endif
if (!treatClassInstancesAsPlainObjects) {
prototypeOfObject = js.obj().getPrototype();
prototypeOfObject = js.obj().getPrototype(js);
}
if (externalHandler != kj::none) {
// If we have an ExternalHandler, we'll ask it to serialize host objects.
Expand Down

0 comments on commit b2041c2

Please sign in to comment.