Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<Env> {
get(key: string): { argumentCount: number; key: string } {
return { argumentCount: arguments.length, key };
}
}

export default BindingEntrypoint;
Original file line number Diff line number Diff line change
Expand Up @@ -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<Env> {
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,10 @@
"service": "cloudflare-worker-workerentrypoint-rpc-sub",
"entrypoint": "NoPropagationEntrypoint",
},
{
"binding": "SUB_WORKER_UNINSTRUMENTED",
"service": "cloudflare-worker-workerentrypoint-rpc-sub",
"entrypoint": "UninstrumentedEntrypoint",
},
],
}
23 changes: 22 additions & 1 deletion packages/cloudflare/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string | RegExp>` - 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
Expand Down Expand Up @@ -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<string | RegExp>;

/**
* Table names that should stay instrumented even though they match the reserved `cf_` prefix used
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -44,6 +45,8 @@ export function instrumentEnv<Env extends Record<string, unknown>>(env: Env, opt
return env;
}

const shouldPropagateRpcTrace = createRpcPropagationResolver(options);

return new Proxy(env, {
get(target, prop, receiver) {
const item = Reflect.get(target, prop, receiver);
Expand Down Expand Up @@ -91,7 +94,7 @@ export function instrumentEnv<Env extends Record<string, unknown>>(env: Env, opt
return instrumented;
}

if (!options?.enableRpcTracePropagation) {
if (!shouldPropagateRpcTrace(String(prop))) {
return item;
}

Comment on lines 94 to 100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The module-level instrumentedBindings cache is checked before the shouldPropagateRpcTrace predicate, causing cached bindings to bypass RPC propagation rules on subsequent requests with different configurations.
Severity: MEDIUM

Suggested Fix

The order of operations should be changed. The shouldPropagateRpcTrace predicate should be checked before attempting to retrieve a binding from the instrumentedBindings cache. This ensures that the current invocation's configuration is always respected, even if a cached version of the binding exists from a previous invocation with a different configuration.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts#L94-L100

Potential issue: A module-level cache, `instrumentedBindings`, stores instrumented RPC
bindings. This cache is checked before the `shouldPropagateRpcTrace` predicate is
evaluated. If an RPC binding is accessed with a permissive configuration (e.g.,
`enableRpcTracePropagation: true`), it is instrumented and cached. A subsequent
invocation with a more restrictive configuration will hit the cache and return the
already-instrumented binding, bypassing the new configuration's allow-list check. This
results in unintended trace propagation for RPC calls.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down
25 changes: 25 additions & 0 deletions packages/cloudflare/src/utils/rpcPropagation.ts
Original file line number Diff line number Diff line change
@@ -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);
}
67 changes: 67 additions & 0 deletions packages/cloudflare/test/instrumentations/instrumentEnv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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<typeof vi.fn>) =>
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');
});
});
});
61 changes: 61 additions & 0 deletions packages/cloudflare/test/utils/rpcPropagation.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
Loading