From 1cef324d1e6ba59ae7ea0b76b21181ddac8bf41c Mon Sep 17 00:00:00 2001 From: Dillon Mulroy Date: Thu, 5 Mar 2026 15:44:10 -0500 Subject: [PATCH 1/2] fix(cloudflare): use correct Proxy receiver in instrumentDurableObjectStorage The storage Proxy's get trap passed `receiver` (the proxy itself) to `Reflect.get`, causing native workerd getters like `storage.sql` to throw 'Illegal invocation' due to failed internal slot / brand checks. Use `target` as the receiver so native getters execute with the correct `this` binding. --- .../instrumentDurableObjectStorage.ts | 9 +- .../instrumentDurableObjectStorage.test.ts | 102 ++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts b/packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts index 29d47eb481f3..984bcb22707e 100644 --- a/packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts +++ b/packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts @@ -16,8 +16,13 @@ type StorageMethod = (typeof STORAGE_METHODS_TO_INSTRUMENT)[number]; */ export function instrumentDurableObjectStorage(storage: DurableObjectStorage): DurableObjectStorage { return new Proxy(storage, { - get(target, prop, receiver) { - const original = Reflect.get(target, prop, receiver); + get(target, prop, _receiver) { + // Use `target` as the receiver instead of the proxy (`_receiver`). + // Native workerd getters (e.g., `storage.sql`) validate `this` via + // internal slots. Passing the proxy as receiver breaks that check, + // causing "Illegal invocation: function called with incorrect `this` + // reference" errors. + const original = Reflect.get(target, prop, target); if (typeof original !== 'function') { return original; diff --git a/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts b/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts index 11c3228f905b..860495424614 100644 --- a/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts +++ b/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts @@ -182,6 +182,61 @@ describe('instrumentDurableObjectStorage', () => { }); }); + describe('native getter preservation (sql)', () => { + it('preserves native getter `this` binding for sql accessor', () => { + // Simulates workerd's native DurableObjectStorage where `sql` is a + // getter that validates `this` via internal slots (brand check). + // Using a private field as the closest JS equivalent of a native + // internal slot check — accessing `#sqlInstance` on the wrong `this` + // throws a TypeError, just like workerd's "Illegal invocation". + const storage = createBrandCheckedStorage(); + const instrumented = instrumentDurableObjectStorage(storage as any); + + // Before fix: this threw "Cannot read private member #sqlInstance + // from an object whose class did not declare it" (equivalent of + // workerd's "Illegal invocation: function called with incorrect + // `this` reference") + expect(() => (instrumented as any).sql).not.toThrow(); + expect((instrumented as any).sql).toBeDefined(); + expect((instrumented as any).sql.exec).toBeDefined(); + }); + + it('sql.exec works through the instrumented proxy', () => { + const storage = createBrandCheckedStorage(); + const instrumented = instrumentDurableObjectStorage(storage as any); + + const result = (instrumented as any).sql.exec('SELECT 1'); + expect(result).toEqual({ query: 'SELECT 1' }); + }); + + it('non-instrumented methods preserve native `this` binding', () => { + const storage = createBrandCheckedStorage(); + const instrumented = instrumentDurableObjectStorage(storage as any); + + expect(() => (instrumented as any).getAlarm()).not.toThrow(); + }); + + it('instrumented methods preserve native `this` binding and create spans', async () => { + const storage = createBrandCheckedStorage(); + const instrumented = instrumentDurableObjectStorage(storage as any); + + // put/get are in STORAGE_METHODS_TO_INSTRUMENT — they go through + // the startSpan + .apply(target, args) path, not the .bind(target) path. + // BrandCheckedStorage uses #data private field, so wrong `this` would throw. + await instrumented.put('key', 'value'); + await expect(instrumented.get('key')).resolves.toBe('value'); + + expect(sentryCore.startSpan).toHaveBeenCalledWith( + expect.objectContaining({ name: 'durable_object_storage_put' }), + expect.any(Function), + ); + expect(sentryCore.startSpan).toHaveBeenCalledWith( + expect.objectContaining({ name: 'durable_object_storage_get' }), + expect.any(Function), + ); + }); + }); + describe('error handling', () => { it('propagates errors from storage operations', async () => { const mockStorage = createMockStorage(); @@ -210,3 +265,50 @@ function createMockStorage(): any { }, }; } + +/** + * Creates a storage mock that uses a class with private fields to simulate + * workerd's native brand-checked getters. Private field access throws a + * TypeError when `this` is not the original instance, mimicking the + * "Illegal invocation" error from workerd native accessors. + */ +class BrandCheckedStorage { + #sqlInstance = { exec: (query: string) => ({ query }) }; + #data = new Map(); + + get sql() { + // Accessing #sqlInstance implicitly checks that `this` is a real + // BrandCheckedStorage instance. If `this` is a Proxy with wrong + // receiver, this throws TypeError. + return this.#sqlInstance; + } + + async get(key: string) { + return this.#data.get(key); + } + async put(key: string, value: unknown) { + this.#data.set(key, value); + } + async delete(key: string) { + return this.#data.delete(key); + } + async list() { + return new Map(this.#data); + } + async getAlarm() { + return null; + } + async setAlarm(_scheduledTime: number) {} + async deleteAlarm() {} + async deleteAll() { + this.#data.clear(); + } + async sync() {} + async transaction(cb: (txn: unknown) => unknown) { + return cb(this); + } +} + +function createBrandCheckedStorage() { + return new BrandCheckedStorage(); +} From f78e40ab7a010b81db32e034b5559c84393bef94 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Fri, 6 Mar 2026 10:44:55 +0100 Subject: [PATCH 2/2] Clean up tests --- .../instrumentDurableObjectStorage.test.ts | 109 +++--------------- 1 file changed, 13 insertions(+), 96 deletions(-) diff --git a/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts b/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts index 860495424614..188b007a0b59 100644 --- a/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts +++ b/packages/cloudflare/test/instrumentDurableObjectStorage.test.ts @@ -182,58 +182,22 @@ describe('instrumentDurableObjectStorage', () => { }); }); - describe('native getter preservation (sql)', () => { - it('preserves native getter `this` binding for sql accessor', () => { - // Simulates workerd's native DurableObjectStorage where `sql` is a - // getter that validates `this` via internal slots (brand check). - // Using a private field as the closest JS equivalent of a native - // internal slot check — accessing `#sqlInstance` on the wrong `this` - // throws a TypeError, just like workerd's "Illegal invocation". - const storage = createBrandCheckedStorage(); + describe('native getter preservation', () => { + it('preserves native getter `this` binding through the proxy', () => { + // Private fields simulate workerd's native brand check — + // accessing #sqlInstance on wrong `this` throws TypeError, + // like workerd's "Illegal invocation". + class BrandCheckedStorage { + #sqlInstance = { exec: () => {} }; + get sql() { + return this.#sqlInstance; + } + } + + const storage = new BrandCheckedStorage(); const instrumented = instrumentDurableObjectStorage(storage as any); - // Before fix: this threw "Cannot read private member #sqlInstance - // from an object whose class did not declare it" (equivalent of - // workerd's "Illegal invocation: function called with incorrect - // `this` reference") expect(() => (instrumented as any).sql).not.toThrow(); - expect((instrumented as any).sql).toBeDefined(); - expect((instrumented as any).sql.exec).toBeDefined(); - }); - - it('sql.exec works through the instrumented proxy', () => { - const storage = createBrandCheckedStorage(); - const instrumented = instrumentDurableObjectStorage(storage as any); - - const result = (instrumented as any).sql.exec('SELECT 1'); - expect(result).toEqual({ query: 'SELECT 1' }); - }); - - it('non-instrumented methods preserve native `this` binding', () => { - const storage = createBrandCheckedStorage(); - const instrumented = instrumentDurableObjectStorage(storage as any); - - expect(() => (instrumented as any).getAlarm()).not.toThrow(); - }); - - it('instrumented methods preserve native `this` binding and create spans', async () => { - const storage = createBrandCheckedStorage(); - const instrumented = instrumentDurableObjectStorage(storage as any); - - // put/get are in STORAGE_METHODS_TO_INSTRUMENT — they go through - // the startSpan + .apply(target, args) path, not the .bind(target) path. - // BrandCheckedStorage uses #data private field, so wrong `this` would throw. - await instrumented.put('key', 'value'); - await expect(instrumented.get('key')).resolves.toBe('value'); - - expect(sentryCore.startSpan).toHaveBeenCalledWith( - expect.objectContaining({ name: 'durable_object_storage_put' }), - expect.any(Function), - ); - expect(sentryCore.startSpan).toHaveBeenCalledWith( - expect.objectContaining({ name: 'durable_object_storage_get' }), - expect.any(Function), - ); }); }); @@ -265,50 +229,3 @@ function createMockStorage(): any { }, }; } - -/** - * Creates a storage mock that uses a class with private fields to simulate - * workerd's native brand-checked getters. Private field access throws a - * TypeError when `this` is not the original instance, mimicking the - * "Illegal invocation" error from workerd native accessors. - */ -class BrandCheckedStorage { - #sqlInstance = { exec: (query: string) => ({ query }) }; - #data = new Map(); - - get sql() { - // Accessing #sqlInstance implicitly checks that `this` is a real - // BrandCheckedStorage instance. If `this` is a Proxy with wrong - // receiver, this throws TypeError. - return this.#sqlInstance; - } - - async get(key: string) { - return this.#data.get(key); - } - async put(key: string, value: unknown) { - this.#data.set(key, value); - } - async delete(key: string) { - return this.#data.delete(key); - } - async list() { - return new Map(this.#data); - } - async getAlarm() { - return null; - } - async setAlarm(_scheduledTime: number) {} - async deleteAlarm() {} - async deleteAll() { - this.#data.clear(); - } - async sync() {} - async transaction(cb: (txn: unknown) => unknown) { - return cb(this); - } -} - -function createBrandCheckedStorage() { - return new BrandCheckedStorage(); -}