diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 3d5abe0ddd9..c89f61bb241 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -639,12 +639,19 @@ struct GetterCallback; liftKj(info, [&]() { \ auto isolate = info.GetIsolate(); \ auto context = isolate->GetCurrentContext(); \ + // For interceptors on the prototype, HolderV2() returns the prototype, but This() \ + // returns the actual instance. We need to validate This() for security, but use \ + // HolderV2() wouldn't work here since it's the prototype object. \ + // Note: This still uses the deprecated This() API - see V8 bug 455600234. \ auto obj = info.This(); \ auto& js = Lock::from(isolate); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ + /* Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where */ \ + /* a native object is used as a prototype and accessed through a derived object. */ \ if (!isContext && \ - !wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj)) { \ + (!wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj) || \ + obj != info.This())) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -689,8 +696,11 @@ struct GetterCallback; auto obj = info.This(); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ + /* Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where */ \ + /* a native object is used as a prototype and accessed through a derived object. */ \ if (!isContext && \ - !wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj)) { \ + (!wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj) || \ + obj != info.This())) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -889,7 +899,11 @@ struct SetterCallback(nullptr))->HasInstance(obj)) { + // Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where + // a native object is used as a prototype and accessed through a derived object. + if (!isContext && + (!wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj) || + obj != info.This())) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -915,7 +929,11 @@ struct SetterCallback(nullptr))->HasInstance(obj)) { + // Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where + // a native object is used as a prototype and accessed through a derived object. + if (!isContext && + (!wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj) || + obj != info.This())) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -1199,7 +1217,9 @@ struct WildcardPropertyCallbacks v8::Local { auto isolate = info.GetIsolate(); auto context = isolate->GetCurrentContext(); - auto obj = info.This(); + // With the interceptor on the instance template, HolderV2() returns the instance + // (same as This()), so we can use the non-deprecated HolderV2() API. + auto obj = info.HolderV2(); auto& wrapper = TypeWrapper::from(isolate); if (!wrapper.getTemplate(isolate, static_cast(nullptr))->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); @@ -1249,7 +1269,10 @@ struct ResourceTypeBuilder { template inline void registerWildcardProperty() { - prototype->SetHandler( + // Install on instance template (not prototype) so that HolderV2() == This(). + // This matches Chromium's approach and avoids needing the deprecated + // PropertyCallbackInfo::This(). + instance->SetHandler( WildcardPropertyCallbacks{}); } @@ -1449,7 +1472,11 @@ struct ResourceTypeBuilder { template inline void registerInspectProperty() { - using Gcb = GetterCallback; + // Use PropertyGetterCallback (FunctionCallbackInfo) instead of GetterCallback + // (PropertyCallbackInfo) because this property is on the prototype but needs access + // to instance state. FunctionCallbackInfo::This() is not deprecated, while + // PropertyCallbackInfo::This() is. + using Gcb = PropertyGetterCallback; auto v8Name = v8StrIntern(isolate, name); @@ -1457,7 +1484,8 @@ struct ResourceTypeBuilder { auto symbol = v8::Symbol::New(isolate, v8Name); inspectProperties->Set(v8Name, symbol, v8::PropertyAttribute::ReadOnly); - prototype->SetNativeDataProperty(symbol, &Gcb::callback, nullptr, v8::Local(), + auto getterFn = v8::FunctionTemplate::New(isolate, &Gcb::callback); + prototype->SetAccessorProperty(symbol, getterFn, v8::Local(), static_cast( v8::PropertyAttribute::ReadOnly | v8::PropertyAttribute::DontEnum)); } diff --git a/v8-this-deprecation-case.md b/v8-this-deprecation-case.md new file mode 100644 index 00000000000..cd134a5a8a9 --- /dev/null +++ b/v8-this-deprecation-case.md @@ -0,0 +1,84 @@ +# Case for PropertyCallbackInfo::This() + +## Summary + +Unfortunately, Cloudflare Workers (workerd) genuinely needs `PropertyCallbackInfo::This()` and cannot follow Chromium's pattern. We tested moving interceptors from prototype to instance template (like Chromium does) and it breaks RPC functionality. + +## What We Tested + +### Attempt: Move Interceptors to Instance Template + +Chromium puts interceptors on `instance_template`, while workerd puts them on `prototype`: + +```python +# Chromium (interface.py line 5815) +interceptor_template = "${instance_template}" +``` + +```cpp +// Workerd (resource.h) +prototype->SetHandler(...) // On prototype, not instance +``` + +We tried changing workerd to match Chromium: + +```cpp +instance->SetHandler(...) // Changed to instance +``` + +Result: Our RPC tests fail: +``` +TypeError: stub.foo is not a function +TypeError: stub.increment is not a function +``` + +### Why It Fails + +Workerd uses prototype-level interceptors for RPC stubs. These are wrapper objects that intercept any property access and forward it over RPC: + +```javascript +const stub = env.MY_DURABLE_OBJECT.get(id); +stub.anyMethodName(args); // Intercepted and sent over RPC +``` + +With interceptor on prototype: +- Works for any object inheriting from `JsRpcStub.prototype` +- Enables flexible RPC proxying + +With interceptor on instance: +- Only works for objects created directly from the template +- Breaks RPC stub pattern + +This is fundamentally different from Chromium's use of interceptors (HTMLCollection, Storage, etc.) where the interceptor is on the actual object being accessed. + +## Backwards compatibility issue + +In addition to the RPC issue we also have a backwards compatibility issue. Workers take +backwards compatibility very seriously - we can't break already-deployed scripts when +we run them with a newer version of V8. + +In 2022 we moved to setting accessors on the prototype instead of on the instance. +This solved some compatibility issues, eg. people could not subclass and get the +expected behaviour. It also aligns better with IDL. However we still have a +compatibility flag for this since it is trivially observable from JS. + +## What Workerd Needs + +| Use Case | API Needed | Can Migrate? | +|----------|-----------|--------------| +| Prototype properties (new Workers) | `FunctionCallbackInfo::This()` | Already done (2022) | +| Named interceptors (RPC stubs) | `PropertyCallbackInfo::This()` | **No** - breaks RPC | +| Instance properties (legacy Workers) | `PropertyCallbackInfo::This()` | **No** - backwards compat | + +## Conclusion + +`PropertyCallbackInfo::This()` is needed for workerd's legitimate use case of prototype-level interceptors. This is not a case of something that can be fixed by following Chromium's pattern - we tested that and it breaks functionality. + +Options: +1. Keep `This()` available for prototype-level interceptors +2. Provide alternative API to get the receiver in `PropertyCallbackInfo` +3. Clarify the migration path for embedders who need prototype-level interceptors + +--- + +*Erik Corry, Cloudflare*