From 96afb248dcab217b8d79a0f4daadd0c6edaf3c88 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Fri, 1 Mar 2024 15:56:04 -0600 Subject: [PATCH 01/14] JSRPC: Don't allow serializing application class instances. The default behavior of V8 serialization (and structured clone, I guess) is to serialize a class instance as if it were a plain object, completely ignoring the prototype. This is probably not useful in real use cases since it is lossy. So, we'd prefer that this just fails, both because it's not useful and because we might want to introduce better behavior in the future. --- src/workerd/api/tests/js-rpc-test.js | 11 +++++++++ src/workerd/api/worker-rpc.c++ | 1 + src/workerd/jsg/ser.c++ | 35 ++++++++++++++++++++++---- src/workerd/jsg/ser.h | 37 ++++++++++++++++++++++++++-- 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index ac5918a188c..5bf16b463b1 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -138,6 +138,10 @@ export class MyService extends WorkerEntrypoint { async tryUseGlobalRpcPromisePipeline() { return await globalRpcPromise.increment(1); } + + async getNonRpcClass() { + return {obj: new NonRpcClass()}; + } } export class MyActor extends DurableObject { @@ -351,6 +355,13 @@ export let namedServiceBinding = { name: "TypeError", message: "The RPC receiver does not implement the method \"ctx\"." }); + + // Can't serialize instances of classes that aren't derived from RpcTarget. + await assert.rejects(() => Promise.resolve(env.MyService.getNonRpcClass()), { + // TODO(bug): Why isn't this a DOMException? + name: "Error", + message: "# could not be cloned." + }); }, } diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index 4c0a6ec0ce9..5360002716c 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -45,6 +45,7 @@ void serializeJsValue(jsg::Lock& js, jsg::JsValue value, Func makeBuilder) { jsg::Serializer serializer(js, jsg::Serializer::Options { .version = 15, .omitHeader = false, + .treatClassInstancesAsPlainObjects = false, .externalHandler = externalHandler, }); serializer.write(js, value); diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 12036f319be..7d481d49b4d 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -9,13 +9,16 @@ namespace workerd::jsg { Serializer::ExternalHandler::~ExternalHandler() noexcept(false) {} -Serializer::Serializer(Lock& js, kj::Maybe maybeOptions) - : ser(js.v8Isolate, this) { +Serializer::Serializer(Lock& js, Options options) + : externalHandler(options.externalHandler), + treatClassInstancesAsPlainObjects(options.treatClassInstancesAsPlainObjects), + ser(js.v8Isolate, this) { #ifdef KJ_DEBUG kj::requireOnStack(this, "jsg::Serializer must be allocated on the stack"); #endif - auto options = kj::mv(maybeOptions).orDefault({}); - externalHandler = options.externalHandler; + if (!treatClassInstancesAsPlainObjects) { + prototypeOfObject = js.obj().getPrototype(); + } KJ_IF_SOME(version, options.version) { KJ_ASSERT(version >= 13, "The minimum serialization version is 13."); KJ_ASSERT(jsg::check(ser.SetWriteVersion(version))); @@ -47,6 +50,25 @@ void Serializer::ThrowDataCloneError(v8::Local message) { isolate->ThrowException(makeDOMException(isolate, message, "DataCloneError")); } +bool Serializer::HasCustomHostObject(v8::Isolate* isolate) { + // V8 will always call WriteHostObject() for objects that have internal fields. We only need + // to override IsHostObject() if we want to treat pure-JS objects differently, which we do if + // treatClassInstancesAsPlainObjects is false. + return !treatClassInstancesAsPlainObjects; +} + +v8::Maybe Serializer::IsHostObject(v8::Isolate* isolate, v8::Local object) { + // This is only called if HasCustomHostObject() returned true. + KJ_ASSERT(!treatClassInstancesAsPlainObjects); + KJ_ASSERT(!prototypeOfObject.IsEmpty()); + + // If the object's prototype is Object.prototype, then it is a plain object, which we'll allow + // to be serialized normally. Otherwise, it is a class instance, which we should treat as a host + // object. Inside `WriteHostObject()` we will throw DataCloneError due to the object not having + // internal fields. + return v8::Just(object->GetPrototype() != prototypeOfObject); +} + v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::Local object) { try { if (object->InternalFieldCount() != Wrappable::INTERNAL_FIELD_COUNT || @@ -54,6 +76,9 @@ v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::Local> maybeTransfer) { - Serializer ser(js, kj::none); + Serializer ser(js); KJ_IF_SOME(transfers, maybeTransfer) { for (auto& item : transfers) { ser.transfer(js, item); diff --git a/src/workerd/jsg/ser.h b/src/workerd/jsg/ser.h index 901fa12f9fc..d7ae0282533 100644 --- a/src/workerd/jsg/ser.h +++ b/src/workerd/jsg/ser.h @@ -78,6 +78,26 @@ class Serializer final: v8::ValueSerializer::Delegate { // When set to true, the serialization header is not written to the output buffer. bool omitHeader = false; + // The structured clone spec states that instances of classes are serialized as if they were + // plain objects: their "own" properties are serialized, but the prototype is completely + // ignored. Upon deserialization, the value is no longer class instance, it's just a plain + // object. This is probably not useful behavior in any real use case, but that's what the spec + // says. + // + // If this flag is true, we'll follow the spec. If it's false, then instances of classes + // (i.e. objects which have a prototype which is not Object.prototype) will not be serializable + // (they throw DataCloneError). + // + // TODO(someday): Perhaps we could create a framework for application-defined classes to define + // their own serializers. However, we would need to be extremely careful about this when + // deserializing data from a possibly-malicious source. Such serialization frameworks have + // a history of creating security bugs as people declare various classes serializable without + // fully thinking through what an attacker could do by sending them an instance of that class + // when it isn't expected. Probably, we just shouldn't support this over RPC at all. For DO + // storage, it could be OK since the application would only be deserializing objects it wrote + // itself, but it may not be worth it to support for only that use case. + bool treatClassInstancesAsPlainObjects = true; + // ExternalHandler, if any. Typically this would be allocated on the stack just before the // Serializer. kj::Maybe externalHandler; @@ -95,7 +115,8 @@ class Serializer final: v8::ValueSerializer::Delegate { kj::Array> transferredArrayBuffers; }; - explicit Serializer(Lock& js, kj::Maybe maybeOptions = kj::none); + explicit Serializer(Lock& js): Serializer(js, {}) {} + explicit Serializer(Lock& js, Options options); inline ~Serializer() noexcept(true) {} // noexcept(true) because Delegate's is noexcept KJ_DISALLOW_COPY_AND_MOVE(Serializer); @@ -127,6 +148,8 @@ class Serializer final: v8::ValueSerializer::Delegate { private: // v8::ValueSerializer::Delegate implementation void ThrowDataCloneError(v8::Local message) override; + bool HasCustomHostObject(v8::Isolate* isolate) override; + v8::Maybe IsHostObject(v8::Isolate* isolate, v8::Local object) override; v8::Maybe WriteHostObject(v8::Isolate* isolate, v8::Local object) override; v8::Maybe GetSharedArrayBufferId( @@ -139,8 +162,18 @@ class Serializer final: v8::ValueSerializer::Delegate { kj::Vector arrayBuffers; kj::Vector> sharedBackingStores; kj::Vector> backingStores; - v8::ValueSerializer ser; bool released = false; + bool treatClassInstancesAsPlainObjects; + + // Initialized to point at the prototype of `Object` if and only if + // `treatClassInstancesAsPlainObjects` is false (in which case we will need to check against this + // prototype in IsHostObject()). + v8::Local prototypeOfObject; + + // The actual ValueSerializer. Note that it's important to define this last because its + // constructor will call back to the Delegate, which is this object, so we hope that this object + // is fully initialized before that point! + v8::ValueSerializer ser; }; // Wraps the v8::ValueDeserializer and v8::ValueDeserializer::Delegate implementation. From 40798593489697503fa59ea049cde8173bef14fb Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Fri, 1 Mar 2024 19:23:41 -0600 Subject: [PATCH 02/14] JSRPC: Improve the error message when failing to serialize host objects. V8's default implementation of `WriteHostObject()` is pretty bad. It does NOT call `ThrowDataCloneError()`; instead it throws a regular old `Error`. The error message is also very weird, it puts the characters `#<>` around the type name. This commit changes to throwing a DataCloneError with a nicer error message. --- src/workerd/api/tests/js-rpc-test.js | 6 ++--- src/workerd/jsg/ser.c++ | 33 +++++++++++++++++++++------- src/workerd/jsg/ser.h | 6 +++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index 5bf16b463b1..52afaf96fd1 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -358,9 +358,9 @@ export let namedServiceBinding = { // Can't serialize instances of classes that aren't derived from RpcTarget. await assert.rejects(() => Promise.resolve(env.MyService.getNonRpcClass()), { - // TODO(bug): Why isn't this a DOMException? - name: "Error", - message: "# could not be cloned." + name: "DataCloneError", + message: 'Could not serialize object of type "NonRpcClass". This type does not support ' + + 'serialization.' }); }, } diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index 7d481d49b4d..f2e02104a3f 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -44,10 +44,29 @@ v8::Maybe Serializer::GetSharedArrayBufferId( return v8::Just(n); } +void Serializer::throwDataCloneErrorForObject(jsg::Lock& js, v8::Local obj) { + // The default error that V8 would generate is "# could not be cloned." -- for some + // reason, it surrounds the type name in "#<>", which seems bizarre? Let's generate a better + // error. + v8::Local message = js.str(kj::str( + "Could not serialize object of type \"", obj->GetConstructorName(), "\". This type does " + "not support serialization.")); + js.throwException(jsg::JsValue(makeDOMException(js.v8Isolate, message, "DataCloneError"))); +} + void Serializer::ThrowDataCloneError(v8::Local message) { - // makeDOMException could throw an exception. If it does, we'll end up crashing but that's ok? auto isolate = v8::Isolate::GetCurrent(); - isolate->ThrowException(makeDOMException(isolate, message, "DataCloneError")); + try { + isolate->ThrowException(makeDOMException(isolate, message, "DataCloneError")); + } catch (JsExceptionThrown&) { + // Apparently an exception was thrown during the construction of the DOMException. Most likely + // we were terminated. In any case, we'll let that exception stay scheduled and propagate back + // to V8. + } catch (...) { + // A KJ exception was thrown, we'll have to convert it to JavaScript and propagate that + // exception instead. + throwInternalError(isolate, kj::getCaughtExceptionAsKj()); + } } bool Serializer::HasCustomHostObject(v8::Isolate* isolate) { @@ -71,6 +90,8 @@ v8::Maybe Serializer::IsHostObject(v8::Isolate* isolate, v8::Local Serializer::WriteHostObject(v8::Isolate* isolate, v8::Local object) { try { + jsg::Lock& js = jsg::Lock::from(isolate); + if (object->InternalFieldCount() != Wrappable::INTERNAL_FIELD_COUNT || object->GetAlignedPointerFromInternalField(Wrappable::WRAPPABLE_TAG_FIELD_INDEX) != &Wrappable::WRAPPABLE_TAG) { @@ -79,9 +100,7 @@ v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::Local( @@ -99,9 +118,7 @@ v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::Local obj); + // v8::ValueSerializer::Delegate implementation void ThrowDataCloneError(v8::Local message) override; bool HasCustomHostObject(v8::Isolate* isolate) override; From e5601d9e4533862722748ac1f5a5e83b489f1094 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 09:07:36 -0600 Subject: [PATCH 03/14] JSRPC: Serialize functions as RPC stubs. This required a V8 patch to allow functions to be treated as host objects. This commit constructs the RpcStubs but they are not actually callable yet -- there is more work to be done in subsequent commits. --- WORKSPACE | 1 + ...lizer-SetTreatFunctionsAsHostObjects.patch | 103 ++++++++++++++++++ src/workerd/api/worker-rpc.c++ | 12 ++ src/workerd/api/worker-rpc.h | 4 + src/workerd/jsg/ser.c++ | 16 ++- src/workerd/jsg/ser.h | 8 +- 6 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch diff --git a/WORKSPACE b/WORKSPACE index 48934388854..413247193d0 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -450,6 +450,7 @@ http_archive( "//:patches/v8/0012-Always-enable-continuation-preserved-data-in-the-bui.patch", "//:patches/v8/0013-Attach-continuation-context-to-Promise-thenable-task.patch", "//:patches/v8/0014-increase-visibility-of-virtual-method.patch", + "//:patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch", ], integrity = "sha256-jcBk1hBhzrMHRL0EDTgHKBVrJPsP1SLZL6A5/l6arrs=", strip_prefix = "v8-12.2.281.18", diff --git a/patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch b/patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch new file mode 100644 index 00000000000..e0d8e3f5bd9 --- /dev/null +++ b/patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch @@ -0,0 +1,103 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Kenton Varda +Date: Sat, 2 Mar 2024 09:00:18 -0600 +Subject: Add ValueSerializer::SetTreatFunctionsAsHostObjects(). + +Previously, ValueSerializer would always refuse to serialize functions. This commit gives the embedder the option to handle them as host objects. + +This is intended for use in an RPC system, where a function can be "serialized" by replacing it with a stub which, when invoked, performs an RPC back to the originating isolate in order to execute the original function there. + +diff --git a/include/v8-value-serializer.h b/include/v8-value-serializer.h +index 596be18adeb3a5a81794aaa44b1d347dec6c0c7d..141f138e08de849e3e02b3b2b346e643b9e40c70 100644 +--- a/include/v8-value-serializer.h ++++ b/include/v8-value-serializer.h +@@ -195,6 +195,15 @@ class V8_EXPORT ValueSerializer { + */ + void SetTreatArrayBufferViewsAsHostObjects(bool mode); + ++ /** ++ * Indicate whether to treat Functions as host objects, ++ * i.e. pass them to Delegate::WriteHostObject. This should not be ++ * called when no Delegate was passed. ++ * ++ * The default is not to treat Functions as host objects. ++ */ ++ void SetTreatFunctionsAsHostObjects(bool mode); ++ + /** + * Write raw data in various common formats to the buffer. + * Note that integer types are written in base-128 varint format, not with a +diff --git a/src/api/api.cc b/src/api/api.cc +index 336aaf5512f743d1202d503e388d2ac9ef5a9377..d736a92e87c758a82247af6623d5a706c646c978 100644 +--- a/src/api/api.cc ++++ b/src/api/api.cc +@@ -3577,6 +3577,10 @@ void ValueSerializer::SetTreatArrayBufferViewsAsHostObjects(bool mode) { + private_->serializer.SetTreatArrayBufferViewsAsHostObjects(mode); + } + ++void ValueSerializer::SetTreatFunctionsAsHostObjects(bool mode) { ++ private_->serializer.SetTreatFunctionsAsHostObjects(mode); ++} ++ + Maybe ValueSerializer::WriteValue(Local context, + Local value) { + auto i_isolate = reinterpret_cast(context->GetIsolate()); +diff --git a/src/objects/value-serializer.cc b/src/objects/value-serializer.cc +index f0b1956253fe603ad6da30a41fb923078ef6bd78..23eff3120d5b20ce4ac1ec8d3700e82a49150308 100644 +--- a/src/objects/value-serializer.cc ++++ b/src/objects/value-serializer.cc +@@ -304,6 +304,10 @@ void ValueSerializer::SetTreatArrayBufferViewsAsHostObjects(bool mode) { + treat_array_buffer_views_as_host_objects_ = mode; + } + ++void ValueSerializer::SetTreatFunctionsAsHostObjects(bool mode) { ++ treat_functions_as_host_objects_ = mode; ++} ++ + void ValueSerializer::WriteTag(SerializationTag tag) { + uint8_t raw_tag = static_cast(tag); + WriteRawBytes(&raw_tag, sizeof(raw_tag)); +@@ -572,8 +576,13 @@ Maybe ValueSerializer::WriteJSReceiver(Handle receiver) { + + // Eliminate callable and exotic objects, which should not be serialized. + InstanceType instance_type = receiver->map()->instance_type(); +- if (IsCallable(*receiver) || (IsSpecialReceiverInstanceType(instance_type) && +- instance_type != JS_SPECIAL_API_OBJECT_TYPE)) { ++ if (IsCallable(*receiver)) { ++ if (treat_functions_as_host_objects_) { ++ return WriteHostObject(Handle::cast(receiver)); ++ } ++ return ThrowDataCloneError(MessageTemplate::kDataCloneError, receiver); ++ } else if (IsSpecialReceiverInstanceType(instance_type) && ++ instance_type != JS_SPECIAL_API_OBJECT_TYPE) { + return ThrowDataCloneError(MessageTemplate::kDataCloneError, receiver); + } + +diff --git a/src/objects/value-serializer.h b/src/objects/value-serializer.h +index 4747119155007e1fa0075440fefa1dfee036c48a..68be2829f36633a386f54668ce9147267c377272 100644 +--- a/src/objects/value-serializer.h ++++ b/src/objects/value-serializer.h +@@ -102,6 +102,15 @@ class ValueSerializer { + */ + void SetTreatArrayBufferViewsAsHostObjects(bool mode); + ++ /* ++ * Indicate whether to treat Functions as host objects, ++ * i.e. pass them to Delegate::WriteHostObject. This should not be ++ * called when no Delegate was passed. ++ * ++ * The default is not to treat Functions as host objects. ++ */ ++ void SetTreatFunctionsAsHostObjects(bool mode); ++ + private: + // Managing allocations of the internal buffer. + Maybe ExpandBuffer(size_t required_capacity); +@@ -181,6 +190,7 @@ class ValueSerializer { + size_t buffer_capacity_ = 0; + bool has_custom_host_objects_ = false; + bool treat_array_buffer_views_as_host_objects_ = false; ++ bool treat_functions_as_host_objects_ = false; + bool out_of_memory_ = false; + Zone zone_; + uint32_t version_; diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index 5360002716c..b825ec45fd8 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -834,6 +834,18 @@ void JsRpcTarget::serialize(jsg::Lock& js, jsg::Serializer& serializer) { }); } +void RpcSerializerExternalHander::serializeFunction( + jsg::Lock& js, jsg::Serializer& serializer, v8::Local func) { + serializer.writeRawUint32(static_cast(rpc::SerializationTag::JS_RPC_STUB)); + + auto handle = jsg::JsRef(js, jsg::JsObject(func)); + rpc::JsRpcTarget::Client cap = kj::heap( + IoContext::current(), kj::mv(handle), true); + write([cap = kj::mv(cap)](rpc::JsValue::External::Builder builder) mutable { + builder.setRpcTarget(kj::mv(cap)); + }); +} + // JsRpcTarget implementation specific to entrypoints. This is used to deliver the first, top-level // call of an RPC session. class EntrypointJsRpcTarget final: public JsRpcTargetBase { diff --git a/src/workerd/api/worker-rpc.h b/src/workerd/api/worker-rpc.h index 8f952146174..b8f0a9978cf 100644 --- a/src/workerd/api/worker-rpc.h +++ b/src/workerd/api/worker-rpc.h @@ -46,6 +46,10 @@ class RpcSerializerExternalHander final: public jsg::Serializer::ExternalHandler size_t size() { return externals.size(); } + // We serialize functions by turning them into RPC stubs. + void serializeFunction( + jsg::Lock& js, jsg::Serializer& serializer, v8::Local func) override; + private: kj::Vector externals; }; diff --git a/src/workerd/jsg/ser.c++ b/src/workerd/jsg/ser.c++ index f2e02104a3f..9ca13f9ed36 100644 --- a/src/workerd/jsg/ser.c++ +++ b/src/workerd/jsg/ser.c++ @@ -7,7 +7,10 @@ namespace workerd::jsg { -Serializer::ExternalHandler::~ExternalHandler() noexcept(false) {} +void Serializer::ExternalHandler::serializeFunction( + jsg::Lock& js, jsg::Serializer& serializer, v8::Local func) { + JSG_FAIL_REQUIRE(DOMDataCloneError, func, " could not be cloned."); +} Serializer::Serializer(Lock& js, Options options) : externalHandler(options.externalHandler), @@ -19,6 +22,10 @@ Serializer::Serializer(Lock& js, Options options) if (!treatClassInstancesAsPlainObjects) { prototypeOfObject = js.obj().getPrototype(); } + if (externalHandler != kj::none) { + // If we have an ExternalHandler, we'll ask it to serialize host objects. + ser.SetTreatFunctionsAsHostObjects(true); + } KJ_IF_SOME(version, options.version) { KJ_ASSERT(version >= 13, "The minimum serialization version is 13."); KJ_ASSERT(jsg::check(ser.SetWriteVersion(version))); @@ -95,6 +102,13 @@ v8::Maybe Serializer::WriteHostObject(v8::Isolate* isolate, v8::LocalInternalFieldCount() != Wrappable::INTERNAL_FIELD_COUNT || object->GetAlignedPointerFromInternalField(Wrappable::WRAPPABLE_TAG_FIELD_INDEX) != &Wrappable::WRAPPABLE_TAG) { + KJ_IF_SOME(eh, externalHandler) { + if (object->IsFunction()) { + eh.serializeFunction(js, *this, object.As()); + return v8::Just(true); + } + } + // v8::ValueSerializer by default will send us anything that has internal fields, but this // object doesn't appear to match the internal fields expected on a JSG object. // diff --git a/src/workerd/jsg/ser.h b/src/workerd/jsg/ser.h index c8de245fa61..564342113df 100644 --- a/src/workerd/jsg/ser.h +++ b/src/workerd/jsg/ser.h @@ -66,10 +66,10 @@ class Serializer final: v8::ValueSerializer::Delegate { // appropriate exception should be thrown. class ExternalHandler { public: - // We declare the destructor just so that this class has a virtual method, which ensures it is - // polymorphic (has a vtable) so that dynamic_cast can be used on it. We mark this method pure - // (`= 0`) so that `ExternalHandler` itself cannot be instantiated. - virtual ~ExternalHandler() noexcept(false) = 0; + // Tries to serialize a function as an external. The default implementation throws + // DataCloneError. + virtual void serializeFunction( + jsg::Lock& js, jsg::Serializer& serializer, v8::Local func); }; struct Options { From 35d488609b912aca3e163a2dbac0062d902c9fa4 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 09:20:55 -0600 Subject: [PATCH 04/14] JSRPC: Refactor: Pull callImpl() out as standalone function. We need to make RpcStub itself callable, so having this as a private method of JsRpcProperty no longer works. --- src/workerd/api/worker-rpc.c++ | 27 +++++++++++++++++++++------ src/workerd/api/worker-rpc.h | 8 -------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index b825ec45fd8..ac3175ac536 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -162,8 +162,20 @@ rpc::JsRpcTarget::Client JsRpcProperty::getClientForOneCall( return result; } +namespace { + +struct JsRpcPromiseAndPipleine { + jsg::JsPromise promise; + rpc::JsRpcTarget::CallResults::Pipeline pipeline; +}; + +// Core implementation of making an RPC call, reusable for many cases below. template -JsRpcProperty::PromiseAndPipleine JsRpcProperty::callImpl(jsg::Lock& js, FillOpFunc&& fillOpFunc) { +JsRpcPromiseAndPipleine callImpl( + jsg::Lock& js, + JsRpcClientProvider& parent, + kj::StringPtr name, + FillOpFunc&& fillOpFunc) { // Note: We used to enforce that RPC methods had to be called with the correct `this`. That is, // we prevented people from doing: // @@ -185,7 +197,7 @@ JsRpcProperty::PromiseAndPipleine JsRpcProperty::callImpl(jsg::Lock& js, FillOpF // `path` will be filled in with the path of property names leading from the stub represented by // `client` to the specific property / method that we're trying to invoke. kj::Vector path; - auto client = parent->getClientForOneCall(js, path); + auto client = parent.getClientForOneCall(js, path); auto& ioContext = IoContext::current(); @@ -228,10 +240,13 @@ JsRpcProperty::PromiseAndPipleine JsRpcProperty::callImpl(jsg::Lock& js, FillOpF } } +} // namespace + jsg::Ref JsRpcProperty::call(const v8::FunctionCallbackInfo& args) { jsg::Lock& js = jsg::Lock::from(args.GetIsolate()); - auto [promise, pipeline] = callImpl(js, [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { + auto [promise, pipeline] = callImpl(js, *parent, name, + [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { kj::Vector argv(args.Length()); for (int n = 0; n < args.Length(); n++) { argv.add(jsg::JsValue(args[n])); @@ -286,7 +301,7 @@ jsg::JsValue finallyImpl(jsg::Lock& js, v8::Local promise, jsg::JsValue JsRpcProperty::then(jsg::Lock& js, v8::Local handler, jsg::Optional> errorHandler) { - auto promise = callImpl(js, + auto promise = callImpl(js, *parent, name, [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { op.setGetProperty(); }).promise; @@ -295,7 +310,7 @@ jsg::JsValue JsRpcProperty::then(jsg::Lock& js, v8::Local handler, } jsg::JsValue JsRpcProperty::catch_(jsg::Lock& js, v8::Local errorHandler) { - auto promise = callImpl(js, + auto promise = callImpl(js, *parent, name, [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { op.setGetProperty(); }).promise; @@ -304,7 +319,7 @@ jsg::JsValue JsRpcProperty::catch_(jsg::Lock& js, v8::Local errorH } jsg::JsValue JsRpcProperty::finally(jsg::Lock& js, v8::Local onFinally) { - auto promise = callImpl(js, + auto promise = callImpl(js, *parent, name, [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { op.setGetProperty(); }).promise; diff --git a/src/workerd/api/worker-rpc.h b/src/workerd/api/worker-rpc.h index b8f0a9978cf..94a9d53d33f 100644 --- a/src/workerd/api/worker-rpc.h +++ b/src/workerd/api/worker-rpc.h @@ -193,14 +193,6 @@ class JsRpcProperty: public JsRpcClientProvider { void visitForGc(jsg::GcVisitor& visitor) { visitor.visit(parent); } - - struct PromiseAndPipleine { - jsg::JsPromise promise; - rpc::JsRpcTarget::CallResults::Pipeline pipeline; - }; - - template - PromiseAndPipleine callImpl(jsg::Lock& js, FillOpFunc&& fillOpFunc); }; // A JsRpcStub object forwards JS method calls to the remote Worker/Durable Object over RPC. From 05a19ec40eb7eef39c556139529c2540fe66814b Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 09:24:51 -0600 Subject: [PATCH 05/14] JSRPC: Refactor: Don't template callImpl(). We can just use an old fashion if statement here. --- src/workerd/api/worker-rpc.c++ | 56 +++++++++++++++------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index ac3175ac536..11d8de0989a 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -170,12 +170,12 @@ struct JsRpcPromiseAndPipleine { }; // Core implementation of making an RPC call, reusable for many cases below. -template JsRpcPromiseAndPipleine callImpl( jsg::Lock& js, JsRpcClientProvider& parent, kj::StringPtr name, - FillOpFunc&& fillOpFunc) { + // If `maybeArgs` is provided, this is a call, otherwise it is a property access. + kj::Maybe&> maybeArgs) { // Note: We used to enforce that RPC methods had to be called with the correct `this`. That is, // we prevented people from doing: // @@ -213,7 +213,25 @@ JsRpcPromiseAndPipleine callImpl( pathBuilder.set(path.size(), name); } - fillOpFunc(builder.getOperation()); + KJ_IF_SOME(args, maybeArgs) { + // This is a function call with arguments. + kj::Vector argv(args.Length()); + for (int n = 0; n < args.Length(); n++) { + argv.add(jsg::JsValue(args[n])); + } + + // If we have arguments, serialize them. + // Note that we may fail to serialize some element, in which case this will throw back to JS. + if (argv.size() > 0) { + serializeJsValue(js, js.arr(argv.asPtr()), [&](capnp::MessageSize hint) { + // TODO(perf): Actually use the size hint. + return builder.getOperation().initCallWithArgs(); + }); + } + } else { + // This is a property access. + builder.getOperation().setGetProperty(); + } auto callResult = builder.send(); @@ -245,22 +263,7 @@ JsRpcPromiseAndPipleine callImpl( jsg::Ref JsRpcProperty::call(const v8::FunctionCallbackInfo& args) { jsg::Lock& js = jsg::Lock::from(args.GetIsolate()); - auto [promise, pipeline] = callImpl(js, *parent, name, - [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { - kj::Vector argv(args.Length()); - for (int n = 0; n < args.Length(); n++) { - argv.add(jsg::JsValue(args[n])); - } - - // If we have arguments, serialize them. - // Note that we may fail to serialize some element, in which case this will throw back to JS. - if (argv.size() > 0) { - serializeJsValue(js, js.arr(argv.asPtr()), [&](capnp::MessageSize hint) { - // TODO(perf): Actually use the size hint. - return op.initCallWithArgs(); - }); - } - }); + auto [promise, pipeline] = callImpl(js, *parent, name, args); return jsg::alloc( jsg::JsRef(js, promise), @@ -301,28 +304,19 @@ jsg::JsValue finallyImpl(jsg::Lock& js, v8::Local promise, jsg::JsValue JsRpcProperty::then(jsg::Lock& js, v8::Local handler, jsg::Optional> errorHandler) { - auto promise = callImpl(js, *parent, name, - [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { - op.setGetProperty(); - }).promise; + auto promise = callImpl(js, *parent, name, kj::none).promise; return thenImpl(js, promise, handler, errorHandler); } jsg::JsValue JsRpcProperty::catch_(jsg::Lock& js, v8::Local errorHandler) { - auto promise = callImpl(js, *parent, name, - [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { - op.setGetProperty(); - }).promise; + auto promise = callImpl(js, *parent, name, kj::none).promise; return catchImpl(js, promise, errorHandler); } jsg::JsValue JsRpcProperty::finally(jsg::Lock& js, v8::Local onFinally) { - auto promise = callImpl(js, *parent, name, - [&](rpc::JsRpcTarget::CallParams::Operation::Builder op) { - op.setGetProperty(); - }).promise; + auto promise = callImpl(js, *parent, name, kj::none).promise; return finallyImpl(js, promise, onFinally); } From 012ac446541ffea52fa2d1b475c720c9bb972762 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 09:28:55 -0600 Subject: [PATCH 06/14] JSRPC: Refactor: Factor out code to create JsRpcPromise. --- src/workerd/api/worker-rpc.c++ | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index 11d8de0989a..ce209f6a24a 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -167,6 +167,12 @@ namespace { struct JsRpcPromiseAndPipleine { jsg::JsPromise promise; rpc::JsRpcTarget::CallResults::Pipeline pipeline; + + jsg::Ref asJsRpcPromise(jsg::Lock& js) && { + return jsg::alloc( + jsg::JsRef(js, promise), + IoContext::current().addObject(kj::heap(kj::mv(pipeline)))); + } }; // Core implementation of making an RPC call, reusable for many cases below. @@ -263,11 +269,7 @@ JsRpcPromiseAndPipleine callImpl( jsg::Ref JsRpcProperty::call(const v8::FunctionCallbackInfo& args) { jsg::Lock& js = jsg::Lock::from(args.GetIsolate()); - auto [promise, pipeline] = callImpl(js, *parent, name, args); - - return jsg::alloc( - jsg::JsRef(js, promise), - IoContext::current().addObject(kj::heap(kj::mv(pipeline)))); + return callImpl(js, *parent, name, args).asJsRpcPromise(js); } namespace { From 6c050255a4dd3ea534641ac16734ef0abc9287ae Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 10:10:08 -0600 Subject: [PATCH 07/14] JSRPC: Monkeypatch assert.rejects to not be confused by callable thenables. We're about to make JsRpcPromise be callable, which makes it a function. But `isValidThenable()` in `internal_asserts.ts` thinks that thenables cannot be callable, which means `assert.rejects` doesn't our promises. This commit monkey-patches it to solve the problem, but maybe `isValidThenable()` itself should change... --- src/workerd/api/tests/js-rpc-test.js | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index 52afaf96fd1..d64911f6635 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -1,6 +1,17 @@ import assert from 'node:assert'; import {WorkerEntrypoint,DurableObject,RpcStub,RpcTarget} from 'cloudflare:workers'; +// Monkey-patch assert.rejects so that it automatically converts the return value of the function +// to a Promise. Otherwise it gets confused when the callback returns a thenable that happens to +// be callable; it thinks it has received a function rather than a promise. +// TODO(cleanup): This is needed because `isValidThenable()` in internal_asserts.ts explicitly +// refuses to accept functions, even if the function has `then` and `catch` methods. Can we +// change that? If so we could remove this hack here. +let originalRejects = assert.rejects.bind(assert); +assert.rejects = (func, err) => { + return originalRejects(() => Promise.resolve(func()), err); +} + class MyCounter extends RpcTarget { constructor(i = 0) { super(); @@ -263,21 +274,21 @@ export let namedServiceBinding = { message: "The RPC receiver does not implement the method \"instanceObject\"." }); - await assert.rejects(() => Promise.resolve(env.MyService.instanceObject), { + await assert.rejects(() => env.MyService.instanceObject, { name: "TypeError", message: "The RPC receiver does not implement the method \"instanceObject\"." }); - await assert.rejects(() => Promise.resolve(env.MyService.throwingProperty), { + await assert.rejects(() => env.MyService.throwingProperty, { name: "Error", message: "PROPERTY THREW" }); - await assert.rejects(() => Promise.resolve(env.MyService.throwingMethod()), { + await assert.rejects(() => env.MyService.throwingMethod(), { name: "Error", message: "METHOD THREW" }); - await assert.rejects(() => Promise.resolve(env.MyService.rejectingPromiseProperty), { + await assert.rejects(() => env.MyService.rejectingPromiseProperty, { name: "Error", message: "REJECTED" }); @@ -357,7 +368,7 @@ export let namedServiceBinding = { }); // Can't serialize instances of classes that aren't derived from RpcTarget. - await assert.rejects(() => Promise.resolve(env.MyService.getNonRpcClass()), { + await assert.rejects(() => env.MyService.getNonRpcClass(), { name: "DataCloneError", message: 'Could not serialize object of type "NonRpcClass". This type does not support ' + 'serialization.' From 8d0b4119731aa03a1a32f5ba1da1e2d534de6f6d Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 10:19:49 -0600 Subject: [PATCH 08/14] JSRPC: Change internal_thenable.js isValidThenable() to accept callable thenables. This lets us get rid of the monkey-patch in js-rpc-test.js, but I'm not completely sure if it's correct. It does seem that V8 itself accepts callable thenables, but the check here looks very explicit so I'm not sure if it has a reason. `isValidThenable()` was also requiring the presence of a `catch` method, but this is not actually a requirement of thenables, so I removed that check as well. --- src/node/internal/internal_assert.ts | 5 +---- src/workerd/api/tests/js-rpc-test.js | 11 ----------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/node/internal/internal_assert.ts b/src/node/internal/internal_assert.ts index c3606fc9ed2..eb0e36447b1 100644 --- a/src/node/internal/internal_assert.ts +++ b/src/node/internal/internal_assert.ts @@ -846,10 +846,7 @@ function isValidThenable(maybeThennable: any): boolean { return true; } - const isThenable = typeof maybeThennable.then === "function" && - typeof maybeThennable.catch === "function"; - - return isThenable && typeof maybeThennable !== "function"; + return typeof maybeThennable.then === "function"; } export { AssertionError }; diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index d64911f6635..79be5bad133 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -1,17 +1,6 @@ import assert from 'node:assert'; import {WorkerEntrypoint,DurableObject,RpcStub,RpcTarget} from 'cloudflare:workers'; -// Monkey-patch assert.rejects so that it automatically converts the return value of the function -// to a Promise. Otherwise it gets confused when the callback returns a thenable that happens to -// be callable; it thinks it has received a function rather than a promise. -// TODO(cleanup): This is needed because `isValidThenable()` in internal_asserts.ts explicitly -// refuses to accept functions, even if the function has `then` and `catch` methods. Can we -// change that? If so we could remove this hack here. -let originalRejects = assert.rejects.bind(assert); -assert.rejects = (func, err) => { - return originalRejects(() => Promise.resolve(func()), err); -} - class MyCounter extends RpcTarget { constructor(i = 0) { super(); From 16fa442ec265e3171e6299f2d5c3c13275fbee36 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 10:34:44 -0600 Subject: [PATCH 09/14] JSRPC: Make JsRpcStub and JsRpcPromise callable. This allows a stub to point to a bare function, and allows an RPC to directly return a bare function. --- src/workerd/api/tests/js-rpc-test.js | 18 +++++++++++++++ src/workerd/api/worker-rpc.c++ | 34 ++++++++++++++++++++++++---- src/workerd/api/worker-rpc.h | 8 +++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index 79be5bad133..f07ed7d9098 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -142,6 +142,10 @@ export class MyService extends WorkerEntrypoint { async getNonRpcClass() { return {obj: new NonRpcClass()}; } + + async getFunction() { + return (a, b) => a ^ b; + } } export class MyActor extends DurableObject { @@ -241,6 +245,20 @@ export let namedServiceBinding = { assert.strictEqual(await counter.increment(7), 15); } + { + let func = await env.MyService.objectProperty.func; + assert.strictEqual(await func(3, 7), 21); + } + { + let func = await env.MyService.getFunction(); + assert.strictEqual(await func(3, 6), 5); + } + { + // Pipeline the function call. + let func = env.MyService.getFunction(); + assert.strictEqual(await func(3, 6), 5); + } + // A property that returns a Promise will wait for the Promise. assert.strictEqual(await env.MyService.promiseProperty, 123); diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index ce209f6a24a..8483d7cccc5 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -179,7 +179,7 @@ struct JsRpcPromiseAndPipleine { JsRpcPromiseAndPipleine callImpl( jsg::Lock& js, JsRpcClientProvider& parent, - kj::StringPtr name, + kj::Maybe name, // If `maybeArgs` is provided, this is a call, otherwise it is a property access. kj::Maybe&> maybeArgs) { // Note: We used to enforce that RPC methods had to be called with the correct `this`. That is, @@ -209,14 +209,24 @@ JsRpcPromiseAndPipleine callImpl( auto builder = client.callRequest(); + // This code here is slightly overcomplicated in order to avoid pushing anything to the + // kj::Vector in the common case that the parent path is empty. I'm probably trying too hard + // but oh well. if (path.empty()) { - builder.setMethodName(name); + KJ_IF_SOME(n, name) { + builder.setMethodName(n); + } else { + // No name and no path, must be directly calling a stub. + builder.initMethodPath(0); + } } else { - auto pathBuilder = builder.initMethodPath(path.size() + 1); + auto pathBuilder = builder.initMethodPath(path.size() + (name != kj::none)); for (auto i: kj::indices(path)) { pathBuilder.set(i, path[i]); } - pathBuilder.set(path.size(), name); + KJ_IF_SOME(n, name) { + pathBuilder.set(path.size(), n); + } } KJ_IF_SOME(args, maybeArgs) { @@ -272,6 +282,18 @@ jsg::Ref JsRpcProperty::call(const v8::FunctionCallbackInfo JsRpcStub::call(const v8::FunctionCallbackInfo& args) { + jsg::Lock& js = jsg::Lock::from(args.GetIsolate()); + + return callImpl(js, *this, kj::none, args).asJsRpcPromise(js); +} + +jsg::Ref JsRpcPromise::call(const v8::FunctionCallbackInfo& args) { + jsg::Lock& js = jsg::Lock::from(args.GetIsolate()); + + return callImpl(js, *this, kj::none, args).asJsRpcPromise(js); +} + namespace { jsg::JsValue thenImpl(jsg::Lock& js, v8::Local promise, @@ -796,6 +818,10 @@ static kj::Maybe makeCallPipeline(jsg::Lock& js, jsg:: } else KJ_IF_SOME(stub, obj.tryUnwrapAs(js)) { // Just grab the stub directly! return stub->getClient(); + } else if (value.isFunction()) { + // It's a plain function. We'll allow its instance properties to be accessed, like with a + // plain object. It will also be callable. + allowInstanceProperties = true; } else { // Not an RPC object. This is going to fail to serialize? We'll just say it doesn't support // pipelining. diff --git a/src/workerd/api/worker-rpc.h b/src/workerd/api/worker-rpc.h index 94a9d53d33f..82a7b59e26e 100644 --- a/src/workerd/api/worker-rpc.h +++ b/src/workerd/api/worker-rpc.h @@ -113,6 +113,9 @@ class JsRpcPromise: public JsRpcClientProvider { rpc::JsRpcTarget::Client getClientForOneCall( jsg::Lock& js, kj::Vector& path) override; + // Expect that the call is itself going to return a function... and call that. + jsg::Ref call(const v8::FunctionCallbackInfo& args); + // Implement standard Promise interface, especially `then()` so that this works as a custom // thenable. // @@ -131,6 +134,7 @@ class JsRpcPromise: public JsRpcClientProvider { kj::Maybe> getProperty(jsg::Lock& js, kj::String name); JSG_RESOURCE_TYPE(JsRpcPromise) { + JSG_CALLABLE(call); JSG_WILDCARD_PROPERTY(getProperty); JSG_METHOD(then); JSG_METHOD_NAMED(catch, catch_); @@ -225,9 +229,13 @@ class JsRpcStub: public JsRpcClientProvider { // for testing to be able to construct a loopback stub. static jsg::Ref constructor(jsg::Lock& js, jsg::Ref object); + // Call the stub itself as a function. + jsg::Ref call(const v8::FunctionCallbackInfo& args); + kj::Maybe> getRpcMethod(jsg::Lock& js, kj::String name); JSG_RESOURCE_TYPE(JsRpcStub) { + JSG_CALLABLE(call); JSG_WILDCARD_PROPERTY(getRpcMethod); } From ca221dd020e5a11e37446444676468ecfd540da6 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 12:41:01 -0600 Subject: [PATCH 10/14] JSRPC: Allow accessing properties of functions. Sometimes people make a "callable object" by creating a function and then giving it properties. We should support this. I had originally wanted to disallow this if the function's prototype had been tampered with, like: ``` let func = () => {}; func.__proto__ = SomeClass.prototype; ``` This technique allows you to create a "callable class instance". In theory we ought to treat it like a user-defined class, i.e., don't support it, unless it extends JsRpcTarget. Unfortunately, it's sort of hard to distinguish "plain functions". There are multiple prototypes to check for: Function, AsyncFunction, and maybe others. I decided to give up and say that we treat all callable objects as functions. --- src/workerd/api/tests/js-rpc-test.js | 6 +++++- src/workerd/api/worker-rpc.c++ | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index f07ed7d9098..c5dd1f6b97a 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -144,7 +144,9 @@ export class MyService extends WorkerEntrypoint { } async getFunction() { - return (a, b) => a ^ b; + let func = (a, b) => a ^ b; + func.someProperty = 123; + return func; } } @@ -252,11 +254,13 @@ export let namedServiceBinding = { { let func = await env.MyService.getFunction(); assert.strictEqual(await func(3, 6), 5); + assert.strictEqual(await func.someProperty, 123); } { // Pipeline the function call. let func = env.MyService.getFunction(); assert.strictEqual(await func(3, 6), 5); + assert.strictEqual(await func.someProperty, 123); } // A property that returns a Promise will wait for the Promise. diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index 8483d7cccc5..6a7a60b6408 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -659,6 +659,9 @@ private: } else if (object.isInstanceOf(js)) { // Yes. It's a JsRpcTarget. allowInstanceProperties = false; + } else if (jsg::JsValue(object).isFunction()) { + // Yes. It's a function. + allowInstanceProperties = true; } else { failLookup(name); } From 3cdedb6f098bd6cde7d4280d6dced2d70348a6e2 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 13:45:22 -0600 Subject: [PATCH 11/14] JSRPC: Make sure JsRpcPromise and JsRpcProperty aren't treated as functions. Since these types are callable, `IsFunction()` returns true, but if we want to actually handle "serializing" these, we need it to work very differently. So, make sure we don't count them, and add some tests. In the future we might decide to actually support these. --- src/workerd/api/tests/js-rpc-test.js | 101 +++++++++++++++++++++++++++ src/workerd/api/worker-rpc.c++ | 29 +++++++- 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index c5dd1f6b97a..0cebaddab8c 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -148,6 +148,33 @@ export class MyService extends WorkerEntrypoint { func.someProperty = 123; return func; } + + getRpcPromise(callback) { + return callback(); + } + getNestedRpcPromise(callback) { + return {value: callback()}; + } + getRemoteNestedRpcPromise(callback) { + // Use a function as a cheap way to return a JsRpcStub that has a remote property `value` which + // itself is initialized as a JsRpcPromise. + let result = () => {}; + result.value = callback(); + return result; + } + getRpcProperty(callback) { + return callback.foo; + } + getNestedRpcProperty(callback) { + return {value: callback.foo}; + } + getRemoteNestedRpcProperty(callback) { + // Use a function as a cheap way to return a JsRpcStub that has a remote property `value` which + // itself is initialized as a JsRpcProperty. + let result = () => {}; + result.value = callback.foo; + return result; + } } export class MyActor extends DurableObject { @@ -517,6 +544,80 @@ export let crossContextSharingDoesntWork = { }, } +export let serializeRpcPromiseOrProprety = { + async test(controller, env, ctx) { + // What happens if we actually try to serialize a JsRpcPromise or JsRpcProperty? Let's make + // sure these aren't, for instance, treated as functions because they are callable. + + let func = () => {return {x: 123};}; + func.foo = {x: 456}; + + // If we directly return returning a JsRpcPromise, the system automatically awaits it on the + // server side because it's a thenable. + assert.deepEqual(await env.MyService.getRpcPromise(func), {x: 123}) + + // Pipelining also works. + assert.strictEqual(await env.MyService.getRpcPromise(func).x, 123) + + // If a JsRpcPromise appears somewhere in the serialization tree, it'll just fail serialization. + // NOTE: We could choose to make this work later. + await assert.rejects(() => env.MyService.getNestedRpcPromise(func), { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcPromise". This type does not support ' + + 'serialization.' + }); + await assert.rejects(() => env.MyService.getNestedRpcPromise(func).value, { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcPromise". This type does not support ' + + 'serialization.' + }); + await assert.rejects(() => env.MyService.getNestedRpcPromise(func).value.x, { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcPromise". This type does not support ' + + 'serialization.' + }); + + // Things get a little weird when we return a stub which itself has properties that reflect + // our RPC promise. If we await fetch the JsRpcPromise itself, this works, again because + // somewhere along the line V8 says "oh look a thenable" and awaits it, before it can be + // subject to serialization. That's fine. + assert.deepEqual(await env.MyService.getRemoteNestedRpcPromise(func).value, {x: 123}); + await assert.rejects(() => env.MyService.getRemoteNestedRpcPromise(func).value.x, { + name: "TypeError", + message: 'The RPC receiver does not implement the method "value".' + }); + + // The story is similar for a JsRpcProperty -- though the implementation details differ. + assert.deepEqual(await env.MyService.getRpcProperty(func), {x: 456}) + assert.strictEqual(await env.MyService.getRpcProperty(func).x, 456) + await assert.rejects(() => env.MyService.getNestedRpcProperty(func), { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcProperty". This type does not support ' + + 'serialization.' + }); + await assert.rejects(() => env.MyService.getNestedRpcProperty(func).value, { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcProperty". This type does not support ' + + 'serialization.' + }); + await assert.rejects(() => env.MyService.getNestedRpcProperty(func).value.x, { + name: "DataCloneError", + message: 'Could not serialize object of type "JsRpcProperty". This type does not support ' + + 'serialization.' + }); + + assert.deepEqual(await env.MyService.getRemoteNestedRpcProperty(func).value, {x: 456}); + await assert.rejects(() => env.MyService.getRemoteNestedRpcProperty(func).value.x, { + name: "TypeError", + message: 'The RPC receiver does not implement the method "value".' + }); + await assert.rejects(() => env.MyService.getRemoteNestedRpcProperty(func).value(), { + name: "TypeError", + message: '"value" is not a function.' + }); + }, +} + // Test that exceptions thrown from async native functions have a proper stack trace. (This is // not specific to RPC but RPC is a convenient place to test it since we can easily define the // callee to throw an exception.) diff --git a/src/workerd/api/worker-rpc.c++ b/src/workerd/api/worker-rpc.c++ index 6a7a60b6408..9b968fe9362 100644 --- a/src/workerd/api/worker-rpc.c++ +++ b/src/workerd/api/worker-rpc.c++ @@ -408,6 +408,29 @@ jsg::Ref JsRpcStub::deserialize( return jsg::alloc(ioctx.addObject(kj::heap(reader.getRpcTarget()))); } +static bool isFunctionForRpc(jsg::Lock& js, v8::Local func) { + jsg::JsObject obj(func); + if (obj.isInstanceOf(js) || obj.isInstanceOf(js)) { + // Don't allow JsRpcProperty or JsRpcPromise to be treated as plain functions, even though they + // are technically callable. These types need to be treated specially (if we decide to let + // them be passed over RPC at all). + return false; + } + return true; +} + +static bool isFunctionForRpc(jsg::Lock& js, jsg::JsValue value) { + if (!value.isFunction()) return false; + return isFunctionForRpc(js, v8::Local(value).As()); +} + +static inline bool isFunctionForRpc(jsg::Lock& js, v8::Local val) { + return isFunctionForRpc(js, jsg::JsValue(val)); +} +static inline bool isFunctionForRpc(jsg::Lock& js, jsg::JsObject val) { + return isFunctionForRpc(js, jsg::JsValue(val)); +} + // Create a CallPipeline wrapping the given value. // // Returns none if the value is not an object and so isn't pipelineable. @@ -499,7 +522,7 @@ public: switch (op.which()) { case rpc::JsRpcTarget::CallParams::Operation::CALL_WITH_ARGS: { - JSG_REQUIRE(propHandle->IsFunction(), TypeError, + JSG_REQUIRE(isFunctionForRpc(js, propHandle), TypeError, kj::str("\"", methodNameForErrors, "\" is not a function.")); auto fn = propHandle.As(); @@ -659,7 +682,7 @@ private: } else if (object.isInstanceOf(js)) { // Yes. It's a JsRpcTarget. allowInstanceProperties = false; - } else if (jsg::JsValue(object).isFunction()) { + } else if (isFunctionForRpc(js, object)) { // Yes. It's a function. allowInstanceProperties = true; } else { @@ -821,7 +844,7 @@ static kj::Maybe makeCallPipeline(jsg::Lock& js, jsg:: } else KJ_IF_SOME(stub, obj.tryUnwrapAs(js)) { // Just grab the stub directly! return stub->getClient(); - } else if (value.isFunction()) { + } else if (isFunctionForRpc(js, obj)) { // It's a plain function. We'll allow its instance properties to be accessed, like with a // plain object. It will also be callable. allowInstanceProperties = true; From 0bb6c64363eec9266a9e536f7f4b3d049df4e97a Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 16:43:37 -0600 Subject: [PATCH 12/14] JSRPC: Add missing visitForGc(). --- src/workerd/api/worker-rpc.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/workerd/api/worker-rpc.h b/src/workerd/api/worker-rpc.h index 82a7b59e26e..7a180567909 100644 --- a/src/workerd/api/worker-rpc.h +++ b/src/workerd/api/worker-rpc.h @@ -144,6 +144,10 @@ class JsRpcPromise: public JsRpcClientProvider { private: jsg::JsRef inner; IoOwn pipeline; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(inner); + } }; // Represents a property -- possibly, a method -- of a remote RPC object. From 9709be17b98959d3231c7290b59532aef1da37d7 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Sat, 2 Mar 2024 16:54:27 -0600 Subject: [PATCH 13/14] JSRPC: Implement visitForMemoryInfo() for RPC types. --- src/workerd/api/worker-rpc.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/workerd/api/worker-rpc.h b/src/workerd/api/worker-rpc.h index 7a180567909..ce6ee227eca 100644 --- a/src/workerd/api/worker-rpc.h +++ b/src/workerd/api/worker-rpc.h @@ -141,6 +141,10 @@ class JsRpcPromise: public JsRpcClientProvider { JSG_METHOD(finally); } + void visitForMemoryInfo(jsg::MemoryTracker& tracker) const { + tracker.trackField("inner", inner); + } + private: jsg::JsRef inner; IoOwn pipeline; @@ -191,6 +195,11 @@ class JsRpcProperty: public JsRpcClientProvider { JSG_METHOD(finally); } + void visitForMemoryInfo(jsg::MemoryTracker& tracker) const { + tracker.trackField("parent", parent); + tracker.trackField("name", name); + } + private: // The parent object from which this property was obtained. jsg::Ref parent; From cfd3d79b771458ef4b326d5b6af8a80df951b6a5 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Tue, 5 Mar 2024 13:29:06 -0600 Subject: [PATCH 14/14] JSRPC: Test that objects with null prototypes are not accessible. This should be the same as a class instance of application-defined type. --- src/workerd/api/tests/js-rpc-test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/workerd/api/tests/js-rpc-test.js b/src/workerd/api/tests/js-rpc-test.js index 0cebaddab8c..b56aea62706 100644 --- a/src/workerd/api/tests/js-rpc-test.js +++ b/src/workerd/api/tests/js-rpc-test.js @@ -105,6 +105,9 @@ export class MyService extends WorkerEntrypoint { get functionProperty() { return (a, b) => a - b; } get objectProperty() { + let nullPrototype = {foo: 123}; + nullPrototype.__proto__ = null; + return { func: (a, b) => a * b, deeper: { @@ -112,6 +115,7 @@ export class MyService extends WorkerEntrypoint { }, counter5: new MyCounter(5), nonRpc: new NonRpcClass(), + nullPrototype, someText: "hello", }; } @@ -143,6 +147,12 @@ export class MyService extends WorkerEntrypoint { return {obj: new NonRpcClass()}; } + async getNullPrototypeObject() { + let obj = {foo: 123}; + obj.__proto__ = null; + return obj; + } + async getFunction() { let func = (a, b) => a ^ b; func.someProperty = 123; @@ -394,6 +404,10 @@ export let namedServiceBinding = { name: "TypeError", message: "The RPC receiver does not implement the method \"nonRpc\"." }); + await assert.rejects(() => env.MyService.objectProperty.nullPrototype.foo, { + name: "TypeError", + message: "The RPC receiver does not implement the method \"nullPrototype\"." + }); // Extra-paranoid check that we can't access methods on env or ctx. await assert.rejects(() => env.MyService.objectProperty.env.MyService.noArgsMethod(), { @@ -411,6 +425,11 @@ export let namedServiceBinding = { message: 'Could not serialize object of type "NonRpcClass". This type does not support ' + 'serialization.' }); + await assert.rejects(() => env.MyService.getNullPrototypeObject(), { + name: "DataCloneError", + message: 'Could not serialize object of type "Object". This type does not support ' + + 'serialization.' + }); }, }