diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index-sub-worker.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index-sub-worker.ts index 6c8638be5ad1..a582c0e341aa 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index-sub-worker.ts +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index-sub-worker.ts @@ -67,4 +67,12 @@ export const NoPropagationEntrypoint = Sentry.withSentry( MySubWorkerEntrypointBase, ); +// Deliberately not wrapped with Sentry: nothing strips a trailing RPC metadata argument here, so +// this is what a caller corrupts if it propagates to a receiver it has no guarantees about. +export class UninstrumentedEntrypoint extends WorkerEntrypoint { + get(key: string): { argumentCount: number; key: string } { + return { argumentCount: arguments.length, key }; + } +} + export default BindingEntrypoint; diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index.ts index d366e7d71afc..c41fa7ce5e18 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index.ts +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/index.ts @@ -11,6 +11,9 @@ interface Env { SUB_WORKER_NO_PROPAGATION: Fetcher & { get(key: string): Promise<{ argumentCount: number; key: string }>; }; + SUB_WORKER_UNINSTRUMENTED: Fetcher & { + get(key: string): Promise<{ argumentCount: number; key: string }>; + }; } class LoopbackEntrypointBase extends WorkerEntrypoint { @@ -29,7 +32,9 @@ export default Sentry.withSentry( dsn: env.SENTRY_DSN, traceLifecycle: 'static', tracesSampleRate: 1.0, - enableRpcTracePropagation: true, + // Allowlisted by binding name: `SUB_WORKER_UNINSTRUMENTED` is deliberately left out, since its + // receiver has no Sentry to strip a trailing metadata argument. + enableRpcTracePropagation: ['SUB_WORKER', 'SUB_WORKER_NO_PROPAGATION'], }), { async fetch(request, env, ctx) { @@ -61,6 +66,10 @@ export default Sentry.withSentry( } } + if (url.pathname === '/call-uninstrumented-rpc') { + return Response.json(await env.SUB_WORKER_UNINSTRUMENTED.get('uninstrumented-key')); + } + if (url.pathname === '/call-entrypoint-rpc-no-propagation') { const result = await env.SUB_WORKER_NO_PROPAGATION.get('no-prop-key'); return Response.json(result); diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/test.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/test.ts index 9309d0f4f97b..3f9726b0ffd9 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/test.ts +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/test.ts @@ -244,6 +244,23 @@ it('captures errors thrown by custom WorkerEntrypoint RPC methods', async ({ sig await runner.completed(); }); +// Regression test for https://github.com/getsentry/sentry-javascript/issues/23233: a receiver that +// is not instrumented never strips Sentry's trailing metadata argument, so a caller must only +// propagate to bindings it was explicitly told about. +it('does not change RPC method arguments for a binding left off the allowlist', async ({ signal }) => { + const runner = createRunner(__dirname) + .expect(envelope => { + const transactionEvent = envelope[1]?.[0]?.[1] as Event; + expect(transactionEvent.transaction).toBe('GET /call-uninstrumented-rpc'); + }) + .start(signal); + + const response = await runner.makeRequest<{ argumentCount: number; key: string }>('get', '/call-uninstrumented-rpc'); + expect(response).toEqual({ argumentCount: 1, key: 'uninstrumented-key' }); + + await runner.completed(); +}); + it('does not inject RPC trace metadata into receiver calls when enableRpcTracePropagation is disabled', async ({ signal, }) => { diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/wrangler.jsonc b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/wrangler.jsonc index badfcd962843..6327e35d9fff 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/wrangler.jsonc +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-workerentrypoint-rpc/wrangler.jsonc @@ -14,5 +14,10 @@ "service": "cloudflare-worker-workerentrypoint-rpc-sub", "entrypoint": "NoPropagationEntrypoint", }, + { + "binding": "SUB_WORKER_UNINSTRUMENTED", + "service": "cloudflare-worker-workerentrypoint-rpc-sub", + "entrypoint": "UninstrumentedEntrypoint", + }, ], } diff --git a/packages/cloudflare/src/client.ts b/packages/cloudflare/src/client.ts index f1b74d5a4195..95a83d3868e0 100644 --- a/packages/cloudflare/src/client.ts +++ b/packages/cloudflare/src/client.ts @@ -200,6 +200,16 @@ interface BaseCloudflareOptions { * - Create spans for each RPC method invocation * - Capture errors thrown by RPC methods * + * Accepts: + * - `false` (default) - never propagate. + * - `true` - propagate on every Durable Object and Service Binding. + * - `Array` - propagate on only the bindings whose names match the given strings or regular expressions. + * + * + * Prefer the array form when you call bindings whose receiver may not run Sentry. RPC calls carry + * trace context as a trailing argument, and only a Sentry-instrumented receiver strips it again — + * anywhere else it arrives as a real argument and changes the method's signature. + * * **Important:** This option should be enabled on **both sides** for full trace propagation. * * @default false @@ -229,8 +239,19 @@ interface BaseCloudflareOptions { * MyEntrypointBase, * ); * ``` + * @example + * ```ts + * // Only propagate to `env.ORDERS` and every `env.SVC_*` binding + * export default Sentry.withSentry( + * (env) => ({ + * dsn: env.SENTRY_DSN, + * enableRpcTracePropagation: ['ORDERS', /^SVC_/], + * }), + * handler, + * ); + * ``` */ - enableRpcTracePropagation?: boolean; + enableRpcTracePropagation?: boolean | Array; /** * Table names that should stay instrumented even though they match the reserved `cf_` prefix used diff --git a/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts b/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts index 5a440503a4ee..eb660e4126e9 100644 --- a/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts +++ b/packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts @@ -12,6 +12,7 @@ import { } from '../../utils/isBinding'; import { instrumentD1 } from './instrumentD1'; import { appendRpcMeta } from '../../utils/rpcMeta'; +import { createRpcPropagationResolver } from '../../utils/rpcPropagation'; import { instrumentDurableObjectNamespace, STUB_NON_RPC_METHODS } from '../instrumentDurableObjectNamespace'; import { instrumentFetcher } from './instrumentFetcher'; import { instrumentQueueProducer } from './instrumentQueueProducer'; @@ -44,6 +45,8 @@ export function instrumentEnv>(env: Env, opt return env; } + const shouldPropagateRpcTrace = createRpcPropagationResolver(options); + return new Proxy(env, { get(target, prop, receiver) { const item = Reflect.get(target, prop, receiver); @@ -91,7 +94,7 @@ export function instrumentEnv>(env: Env, opt return instrumented; } - if (!options?.enableRpcTracePropagation) { + if (!shouldPropagateRpcTrace(String(prop))) { return item; } diff --git a/packages/cloudflare/src/utils/rpcPropagation.ts b/packages/cloudflare/src/utils/rpcPropagation.ts new file mode 100644 index 000000000000..3e76fb5af862 --- /dev/null +++ b/packages/cloudflare/src/utils/rpcPropagation.ts @@ -0,0 +1,25 @@ +import { stringMatchesSomePattern } from '@sentry/core'; +import type { CloudflareOptions } from '../client'; + +const PROPAGATE_TO_NONE = () => false; +const PROPAGATE_TO_ALL = () => true; + +/** + * Builds the per-binding predicate that decides whether a binding takes part in RPC trace + * propagation. + */ +export function createRpcPropagationResolver(options: CloudflareOptions | undefined): (bindingName: string) => boolean { + const value: CloudflareOptions['enableRpcTracePropagation'] | undefined = options?.enableRpcTracePropagation; + + if (value === true) { + return PROPAGATE_TO_ALL; + } + + if (!Array.isArray(value) || !value.length) { + return PROPAGATE_TO_NONE; + } + + // Strings must match a binding name exactly, without this, an entry of `DB` would also enable + // propagation for a binding named `MY_DB`. Regular expressions still give pattern matching. + return (bindingName: string) => stringMatchesSomePattern(bindingName, value, true); +} diff --git a/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts b/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts index 72f9d0774507..f1f18a5376ef 100644 --- a/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts +++ b/packages/cloudflare/test/instrumentations/instrumentEnv.test.ts @@ -89,6 +89,34 @@ describe('instrumentEnv', () => { expect(instrumentDurableObjectNamespace).not.toHaveBeenCalled(); }); + it('instruments only the DurableObjectNamespace bindings named in the allowlist', () => { + const allowed = { idFromName: vi.fn(), idFromString: vi.fn(), get: vi.fn(), newUniqueId: vi.fn() }; + const denied = { idFromName: vi.fn(), idFromString: vi.fn(), get: vi.fn(), newUniqueId: vi.fn() }; + const env = { COUNTER: allowed, SESSIONS: denied }; + const instrumented = instrumentEnv(env, { enableRpcTracePropagation: ['COUNTER'] }); + + expect((instrumented.COUNTER as any).__instrumented).toBe(true); + expect(instrumented.SESSIONS).toBe(denied); + expect(instrumentDurableObjectNamespace).toHaveBeenCalledTimes(1); + expect(instrumentDurableObjectNamespace).toHaveBeenCalledWith(allowed); + }); + + it('matches allowlisted binding names exactly rather than as substrings', () => { + const doNamespace = { idFromName: vi.fn(), idFromString: vi.fn(), get: vi.fn(), newUniqueId: vi.fn() }; + const env = { MY_COUNTER: doNamespace }; + const instrumented = instrumentEnv(env, { enableRpcTracePropagation: ['COUNTER'] }); + + expect(instrumented.MY_COUNTER).toBe(doNamespace); + }); + + it('supports regular expressions in the allowlist', () => { + const doNamespace = { idFromName: vi.fn(), idFromString: vi.fn(), get: vi.fn(), newUniqueId: vi.fn() }; + const env = { SVC_ORDERS: doNamespace }; + const instrumented = instrumentEnv(env, { enableRpcTracePropagation: [/^SVC_/] }); + + expect((instrumented.SVC_ORDERS as any).__instrumented).toBe(true); + }); + it('detects and instruments DurableObjectNamespace bindings when enableRpcTracePropagation is enabled', () => { const doNamespace = { idFromName: vi.fn(), @@ -486,5 +514,44 @@ describe('instrumentEnv', () => { expect(rpcMethod).toHaveBeenCalledWith('arg1'); }); + + // A receiver without Sentry never strips the trailing metadata argument, so a caller has to be + // able to limit propagation to the bindings it knows are instrumented. + // See https://github.com/getsentry/sentry-javascript/issues/23233. + it('injects meta only into JSRPC calls on allowlisted bindings', () => { + vi.spyOn(SentryCore, 'getTraceData').mockReturnValue({ + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', + baggage: 'sentry-environment=production', + }); + + const allowedMethod = vi.fn(); + const deniedMethod = vi.fn(); + const createJsrpcBinding = (rpcMethod: ReturnType) => + new Proxy( + { fetch: vi.fn(), myRpcMethod: rpcMethod }, + { + get(target, prop) { + if (prop in target) { + return Reflect.get(target, prop); + } + return () => {}; + }, + }, + ); + + const env = { ORDERS: createJsrpcBinding(allowedMethod), EXTERNAL: createJsrpcBinding(deniedMethod) }; + const instrumented = instrumentEnv(env, { enableRpcTracePropagation: ['ORDERS'] }); + + instrumented.ORDERS.myRpcMethod('first'); + instrumented.EXTERNAL.myRpcMethod('first'); + + expect(allowedMethod).toHaveBeenCalledWith('first', { + __sentry_rpc_meta__: { + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', + baggage: 'sentry-environment=production', + }, + }); + expect(deniedMethod).toHaveBeenCalledWith('first'); + }); }); }); diff --git a/packages/cloudflare/test/utils/rpcPropagation.test.ts b/packages/cloudflare/test/utils/rpcPropagation.test.ts new file mode 100644 index 000000000000..b34e9c33b109 --- /dev/null +++ b/packages/cloudflare/test/utils/rpcPropagation.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from 'vitest'; +import { createRpcPropagationResolver } from '../../src/utils/rpcPropagation'; + +describe('createRpcPropagationResolver', () => { + it('propagates to nothing when no options are available', () => { + const shouldPropagate = createRpcPropagationResolver(undefined); + + expect(shouldPropagate('MY_DO')).toBe(false); + }); + + it('propagates to nothing when the option is unset', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: undefined }); + + expect(shouldPropagate('MY_DO')).toBe(false); + expect(shouldPropagate('EXTERNAL')).toBe(false); + }); + + it('propagates to nothing when the option is `false`', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: false }); + + expect(shouldPropagate('MY_DO')).toBe(false); + }); + + it('propagates to every binding when the option is `true`', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: true }); + + expect(shouldPropagate('MY_DO')).toBe(true); + expect(shouldPropagate('EXTERNAL')).toBe(true); + }); + + it('propagates only to allowlisted binding names', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: ['MY_DO', 'EXTERNAL'] }); + + expect(shouldPropagate('MY_DO')).toBe(true); + expect(shouldPropagate('EXTERNAL')).toBe(true); + expect(shouldPropagate('OTHER')).toBe(false); + }); + + it('propagates to nothing for an empty allowlist', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: [] }); + + expect(shouldPropagate('MY_DO')).toBe(false); + }); + + it('matches binding names exactly, never as a substring', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: ['DB'] }); + + expect(shouldPropagate('DB')).toBe(true); + expect(shouldPropagate('MY_DB')).toBe(false); + expect(shouldPropagate('DB_REPLICA')).toBe(false); + }); + + it('supports regular expressions for pattern matching', () => { + const shouldPropagate = createRpcPropagationResolver({ enableRpcTracePropagation: [/^SVC_/] }); + + expect(shouldPropagate('SVC_ORDERS')).toBe(true); + expect(shouldPropagate('SVC_USERS')).toBe(true); + expect(shouldPropagate('ORDERS')).toBe(false); + expect(shouldPropagate('PREFIXED_SVC_ORDERS')).toBe(false); + }); +});