From c0611feab7ee8cd1b808943fb9701b8deb5e7a14 Mon Sep 17 00:00:00 2001 From: JPeer264 Date: Tue, 22 Sep 2026 16:19:16 +0200 Subject: [PATCH] fix(cloudflare): Call connect() on the binding for service bindings and DO stubs The service binding and Durable Object stub proxies returned connect() unbound, so calling it ran the native method with the proxy as `this` and workerd threw "Illegal invocation". The proxies cannot bind every function, because reading `.bind` on an RPC method returns another RPC property, so only connect() is forwarded to the binding. Co-Authored-By: Claude Opus 5 --- .../suites/binding-connect/index.ts | 44 +++++++++++++++++++ .../suites/binding-connect/test.ts | 25 +++++++++++ .../suites/binding-connect/wrangler.jsonc | 26 +++++++++++ .../instrumentDurableObjectNamespace.ts | 4 ++ .../instrumentations/worker/instrumentEnv.ts | 4 ++ .../instrumentDurableObjectNamespace.test.ts | 24 +++++++++- .../instrumentations/instrumentEnv.test.ts | 24 ++++++++++ 7 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 dev-packages/cloudflare-integration-tests/suites/binding-connect/index.ts create mode 100644 dev-packages/cloudflare-integration-tests/suites/binding-connect/test.ts create mode 100644 dev-packages/cloudflare-integration-tests/suites/binding-connect/wrangler.jsonc diff --git a/dev-packages/cloudflare-integration-tests/suites/binding-connect/index.ts b/dev-packages/cloudflare-integration-tests/suites/binding-connect/index.ts new file mode 100644 index 000000000000..cf28459187c5 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/binding-connect/index.ts @@ -0,0 +1,44 @@ +import * as Sentry from '@sentry/cloudflare'; +import { DurableObject } from 'cloudflare:workers'; + +interface Env { + SENTRY_DSN: string; + SELF: Fetcher; + CONNECT_DO: DurableObjectNamespace; +} + +export class ConnectDurableObject extends DurableObject {} + +function tryConnect(connect: () => Socket): string { + try { + const socket = connect(); + socket.opened.catch(() => {}); + socket.closed.catch(() => {}); + return 'ok'; + } catch (error) { + return (error as Error).message; + } +} + +export default Sentry.withSentry( + (env: Env) => ({ + dsn: env.SENTRY_DSN, + tracesSampleRate: 1.0, + }), + { + async fetch(request, env) { + const url = new URL(request.url); + + if (url.pathname === '/connect') { + const stub = env.CONNECT_DO.get(env.CONNECT_DO.idFromName('connect')); + + return Response.json({ + durableObject: tryConnect(() => stub.connect('127.0.0.1:9')), + service: tryConnect(() => env.SELF.connect('127.0.0.1:9')), + }); + } + + return new Response('not found', { status: 404 }); + }, + } satisfies ExportedHandler, +); diff --git a/dev-packages/cloudflare-integration-tests/suites/binding-connect/test.ts b/dev-packages/cloudflare-integration-tests/suites/binding-connect/test.ts new file mode 100644 index 000000000000..defed12ded34 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/binding-connect/test.ts @@ -0,0 +1,25 @@ +import type { Envelope, SerializedStreamedSpanContainer } from '@sentry/core'; +import { expect, it } from 'vitest'; +import { createRunner } from '../../runner'; + +it('calls connect() on Durable Object stubs and service bindings with the binding as `this`', async ({ signal }) => { + const runner = createRunner(__dirname) + .expect((envelope: Envelope) => { + const spanItem = envelope[1].find(item => item[0].type === 'span'); + expect(spanItem).toBeDefined(); + const segmentSpan = (spanItem![1] as SerializedStreamedSpanContainer).items.find(span => !!span.is_segment); + expect(segmentSpan).toMatchObject({ + name: 'GET', + status: 'ok', + attributes: expect.objectContaining({ 'url.path': { type: 'string', value: '/connect' } }), + }); + }) + .start(signal); + + const result = await runner.makeRequest<{ durableObject: string; service: string }>('get', '/connect'); + + expect(result?.durableObject).toBe('ok'); + // Local workerd rejects CONNECT on a Worker after the `this` check, so only that check is asserted here. + expect(result?.service).not.toMatch(/Illegal invocation/); + await runner.completed(); +}); diff --git a/dev-packages/cloudflare-integration-tests/suites/binding-connect/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/binding-connect/wrangler.jsonc new file mode 100644 index 000000000000..0ad9e3a17f67 --- /dev/null +++ b/dev-packages/cloudflare-integration-tests/suites/binding-connect/wrangler.jsonc @@ -0,0 +1,26 @@ +{ + "name": "binding-connect-worker", + "main": "index.ts", + "compatibility_date": "2025-06-17", + "compatibility_flags": ["nodejs_compat"], + "migrations": [ + { + "new_sqlite_classes": ["ConnectDurableObject"], + "tag": "v1", + }, + ], + "durable_objects": { + "bindings": [ + { + "class_name": "ConnectDurableObject", + "name": "CONNECT_DO", + }, + ], + }, + "services": [ + { + "binding": "SELF", + "service": "binding-connect-worker", + }, + ], +} diff --git a/packages/cloudflare/src/instrumentations/instrumentDurableObjectNamespace.ts b/packages/cloudflare/src/instrumentations/instrumentDurableObjectNamespace.ts index cb3db118125a..3c61d730fb59 100644 --- a/packages/cloudflare/src/instrumentations/instrumentDurableObjectNamespace.ts +++ b/packages/cloudflare/src/instrumentations/instrumentDurableObjectNamespace.ts @@ -65,6 +65,10 @@ function instrumentDurableObjectStub(stub: DurableObjectStub, propagateRpcTrace: return instrumentFetcher((...args) => Reflect.apply(value, target, args)); } + if (prop === 'connect' && typeof value === 'function') { + return (...args: unknown[]) => Reflect.apply(value, target, args); + } + if ( propagateRpcTrace && typeof value === 'function' && diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts b/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts index 5f95320f657a..33b02b9bd74f 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts @@ -101,6 +101,10 @@ export function instrumentEnv>(env: Env, opt return instrumentFetcher((...args) => Reflect.apply(value, target, args)); } + if (p === 'connect' && typeof value === 'function') { + return (...args: unknown[]) => Reflect.apply(value, target, args); + } + if ( propagateRpcTrace && typeof value === 'function' && diff --git a/packages/cloudflare/test/instrumentations/instrumentDurableObjectNamespace.test.ts b/packages/cloudflare/test/instrumentations/instrumentDurableObjectNamespace.test.ts index d86c8081d0c2..a92d2a582d10 100644 --- a/packages/cloudflare/test/instrumentations/instrumentDurableObjectNamespace.test.ts +++ b/packages/cloudflare/test/instrumentations/instrumentDurableObjectNamespace.test.ts @@ -323,11 +323,31 @@ describe('instrumentDurableObjectNamespace', () => { const instrumented = instrumentDurableObjectNamespace(namespace, true); const stub = instrumented.get({ toString: () => 'id', equals: () => false } as any); + (stub as any).connect('127.0.0.1:9'); - // connect and dup should be the original functions, not wrapped - expect((stub as any).connect).toBe(connectFn); + expect(connectFn).toHaveBeenCalledWith('127.0.0.1:9'); expect((stub as any).dup).toBe(dupFn); }); + + it('calls connect with the underlying stub as `this`', () => { + const { namespace: originalNamespace } = createMockNamespace(); + const rawStub = { + id: { toString: () => 'mock-id', equals: () => false, name: 'test' }, + fetch: vi.fn(), + connect(this: unknown) { + if (this !== rawStub) { + throw new TypeError('Illegal invocation: function called with incorrect `this` reference.'); + } + return 'socket'; + }, + }; + const namespace = { ...originalNamespace, get: vi.fn().mockReturnValue(rawStub) }; + const instrumented = instrumentDurableObjectNamespace(namespace); + + const stub = instrumented.get({ toString: () => 'id', equals: () => false } as any); + + expect((stub as any).connect('127.0.0.1:9')).toBe('socket'); + }); }); describe('non-function properties', () => { diff --git a/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts b/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts index c23f9a2d607e..9219091d53bd 100644 --- a/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts +++ b/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts @@ -410,6 +410,30 @@ describe('instrumentEnv', () => { }); }); + it('calls JSRPC connect with the underlying binding as `this`', () => { + const jsrpcTarget = { + fetch: vi.fn(), + connect(this: unknown, _address: string) { + if (this !== jsrpcProxy) { + throw new TypeError('Illegal invocation: function called with incorrect `this` reference.'); + } + return 'socket'; + }, + }; + const jsrpcProxy = new Proxy(jsrpcTarget, { + get(target, prop) { + if (prop in target) { + return Reflect.get(target, prop); + } + return () => {}; + }, + }); + const env = { SERVICE: jsrpcProxy }; + const instrumented = instrumentEnv(env, { rpcTracePropagationBindings: [/.*/] }); + + expect(instrumented.SERVICE.connect('127.0.0.1:9')).toBe('socket'); + }); + it('does not inject meta into JSRPC fetch calls', () => { vi.spyOn(SentryCore, 'getTraceData').mockReturnValue({ 'sentry-trace': 'abc-def-1',