From 4ea9a5c38ee69da25eb43ffcb1fa465dd4b6010e Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Mon, 11 May 2026 12:38:17 +0200 Subject: [PATCH] fix(otel): consolidate to single OTel SDK init at instrumentation.register hook All three apps' instrumentation.ts files now call initOtelServerNode directly instead of initSentryServer/initSentryServerNode, closing the startup window where @sentry/nextjs auto-instrumentation could send unscrubbed errors before bindAll() fires. bindOtelInstrumentation no longer calls initOtelServerNode (SDK init belongs at app boot, binding at request scope). Orphaned sentry/ init-server*.ts files deleted; their package.json subpath exports removed. Co-Authored-By: Claude Sonnet 4.6 --- apps/cms/instrumentation.ts | 15 +-- apps/web-next/instrumentation.ts | 19 ++-- apps/web-tanstack/src/instrumentation.ts | 15 +-- packages/core-shared/package.json | 2 - .../di/bind-otel-instrumentation.test.ts | 30 +++--- .../di/bind-otel-instrumentation.ts | 25 ++--- .../core-shared/src/instrumentation/index.ts | 2 - .../sentry/init-server-node.test.ts | 53 --------- .../sentry/init-server-node.ts | 40 ------- .../sentry/init-server.test.ts | 101 ------------------ .../src/instrumentation/sentry/init-server.ts | 40 ------- 11 files changed, 54 insertions(+), 288 deletions(-) delete mode 100644 packages/core-shared/src/instrumentation/sentry/init-server-node.test.ts delete mode 100644 packages/core-shared/src/instrumentation/sentry/init-server-node.ts delete mode 100644 packages/core-shared/src/instrumentation/sentry/init-server.test.ts delete mode 100644 packages/core-shared/src/instrumentation/sentry/init-server.ts diff --git a/apps/cms/instrumentation.ts b/apps/cms/instrumentation.ts index 9702a61..a030856 100644 --- a/apps/cms/instrumentation.ts +++ b/apps/cms/instrumentation.ts @@ -2,19 +2,22 @@ // CMS is server-only (Payload admin UI). No instrumentation-client.ts here — // Payload admin UI bundling is opinionated and the public DSN flow is // out-of-scope per spec §8. +// +// Initializes the OTel SDK here so PII scrub processors are active from the +// very first request — before bindAll() fires (C1 fix). export async function register() { if ( process.env["NEXT_RUNTIME"] === "nodejs" || process.env["NEXT_RUNTIME"] === "edge" ) { - const { initSentryServer } = await import( - "@repo/core-shared/instrumentation/sentry/init-server" + const { initOtelServerNode } = await import( + "@repo/core-shared/instrumentation/otel/init-server-node" ); - initSentryServer({ - dsn: process.env["CMS_SENTRY_DSN"], - app: "cms", - release: process.env["VERCEL_GIT_COMMIT_SHA"], + initOtelServerNode({ + dsn: process.env["CMS_SENTRY_DSN"] ?? "", + serviceName: "cms", + environment: process.env["NODE_ENV"] ?? "development", }); } } diff --git a/apps/web-next/instrumentation.ts b/apps/web-next/instrumentation.ts index 96d3ff1..fb70122 100644 --- a/apps/web-next/instrumentation.ts +++ b/apps/web-next/instrumentation.ts @@ -1,19 +1,22 @@ // apps/web-next/instrumentation.ts -// Next.js convention: this module runs once on server boot. -// Delegates to the centralized init helper in core-shared. +// Next.js convention: this module runs once on server boot (before any request handler). +// Initializes the OTel SDK here so PII scrub processors are active from the very first +// request — before bindAll() fires. Calling initOtelServerNode here (not inside bindAll) +// closes the startup window where @sentry/nextjs auto-instrumentation could send +// unscrubbed errors (C1 fix). export async function register() { if ( process.env["NEXT_RUNTIME"] === "nodejs" || process.env["NEXT_RUNTIME"] === "edge" ) { - const { initSentryServer } = await import( - "@repo/core-shared/instrumentation/sentry/init-server" + const { initOtelServerNode } = await import( + "@repo/core-shared/instrumentation/otel/init-server-node" ); - initSentryServer({ - dsn: process.env["WEB_NEXT_SENTRY_DSN"], - app: "web-next", - release: process.env["VERCEL_GIT_COMMIT_SHA"], + initOtelServerNode({ + dsn: process.env["WEB_NEXT_SENTRY_DSN"] ?? "", + serviceName: "web-next", + environment: process.env["NODE_ENV"] ?? "development", }); } } diff --git a/apps/web-tanstack/src/instrumentation.ts b/apps/web-tanstack/src/instrumentation.ts index 5b13c9a..86a651e 100644 --- a/apps/web-tanstack/src/instrumentation.ts +++ b/apps/web-tanstack/src/instrumentation.ts @@ -1,9 +1,12 @@ // apps/web-tanstack/src/instrumentation.ts -// Server-entry hook. Imported at the top of the server entry file. -import { initSentryServerNode } from "@repo/core-shared/instrumentation/sentry/init-server-node"; +// Server-entry hook. Imported at the top of the server entry file before any +// request handler runs. Initializes the OTel SDK here so PII scrub processors +// are active from the very first request (C1 fix — closes the startup window +// where Sentry auto-instrumentation could send unscrubbed errors). +import { initOtelServerNode } from "@repo/core-shared/instrumentation/otel/init-server-node"; -initSentryServerNode({ - dsn: process.env["WEB_TANSTACK_SENTRY_DSN"], - app: "web-tanstack", - release: process.env["VITE_GIT_COMMIT_SHA"], +initOtelServerNode({ + dsn: process.env["WEB_TANSTACK_SENTRY_DSN"] ?? "", + serviceName: "web-tanstack", + environment: process.env["NODE_ENV"] ?? "development", }); diff --git a/packages/core-shared/package.json b/packages/core-shared/package.json index d9c2608..0075a5f 100644 --- a/packages/core-shared/package.json +++ b/packages/core-shared/package.json @@ -16,9 +16,7 @@ "./instrumentation": "./src/instrumentation/index.ts", "./instrumentation/otel": "./src/instrumentation/otel/index.ts", "./instrumentation/otel/init-server-node": "./src/instrumentation/otel/init-server-node.ts", - "./instrumentation/sentry/init-server": "./src/instrumentation/sentry/init-server.ts", "./instrumentation/sentry/init-client": "./src/instrumentation/sentry/init-client.ts", - "./instrumentation/sentry/init-server-node": "./src/instrumentation/sentry/init-server-node.ts", "./instrumentation/sentry/init-client-react": "./src/instrumentation/sentry/init-client-react.ts" }, "scripts": { diff --git a/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.test.ts b/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.test.ts index 126c192..37adead 100644 --- a/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.test.ts +++ b/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.test.ts @@ -10,32 +10,22 @@ vi.mock("@sentry/nextjs", () => ({ setUser: vi.fn(), })); -vi.mock("@/instrumentation/otel/init-server-node", () => ({ - initOtelServerNode: vi.fn(), -})); - import { Container } from "inversify"; import { bindOtelInstrumentation } from "@/instrumentation/di/bind-otel-instrumentation"; -import { initOtelServerNode } from "@/instrumentation/otel/init-server-node"; import { INSTRUMENTATION_SYMBOLS } from "@/instrumentation/symbols"; import { OtelTracer } from "@/instrumentation/otel/otel-tracer"; import { OtelLogger } from "@/instrumentation/otel/otel-logger"; +// NOTE: initOtelServerNode is intentionally NOT called by bindOtelInstrumentation. +// The SDK is initialized by each app's instrumentation.ts register() hook so PII +// scrub processors are active before the first request (C1 fix). There is therefore +// no initOtelServerNode mock or call-count assertion here. + describe("bindOtelInstrumentation", () => { beforeEach(() => { vi.clearAllMocks(); }); - it("calls initOtelServerNode with the provided DSN and app as serviceName", () => { - const c = new Container(); - bindOtelInstrumentation(c, { dsn: "https://x@y/1", app: "web-next" }); - expect(initOtelServerNode).toHaveBeenCalledTimes(1); - expect((initOtelServerNode as ReturnType).mock.calls[0]![0]).toMatchObject({ - dsn: "https://x@y/1", - serviceName: "web-next", - }); - }); - it("binds OtelTracer + OtelLogger to the container", () => { const c = new Container(); bindOtelInstrumentation(c, { dsn: "https://x@y/1", app: "web-next" }); @@ -52,4 +42,14 @@ describe("bindOtelInstrumentation", () => { expect(tracer).toBeInstanceOf(OtelTracer); expect(logger).toBeInstanceOf(OtelLogger); }); + + it("rebinds when called a second time (idempotent container state)", () => { + const c = new Container(); + bindOtelInstrumentation(c, { dsn: "https://x@y/1", app: "web-next" }); + // Second call should not throw even if TRACER is already bound. + expect(() => + bindOtelInstrumentation(c, { dsn: "https://x@y/2", app: "cms" }), + ).not.toThrow(); + expect(c.get(INSTRUMENTATION_SYMBOLS.TRACER)).toBeInstanceOf(OtelTracer); + }); }); diff --git a/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.ts b/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.ts index 66c7010..e5e7bd7 100644 --- a/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.ts +++ b/packages/core-shared/src/instrumentation/di/bind-otel-instrumentation.ts @@ -3,7 +3,6 @@ import type { Container } from "inversify"; import { OtelTracer } from "../otel/otel-tracer"; import { OtelLogger } from "../otel/otel-logger"; import { OtelMetrics } from "../otel/otel-metrics"; -import { initOtelServerNode } from "../otel/init-server-node"; import { INSTRUMENTATION_SYMBOLS } from "../symbols"; import type { ITracer, ILogger, IMetrics } from "../index"; @@ -13,24 +12,20 @@ export type BindOtelOpts = { release?: string; }; +/** + * Binds OtelTracer, OtelLogger, and OtelMetrics to the DI container. + * + * NOTE: The OTel NodeSDK is NOT initialized here. It is initialized by each + * app's instrumentation.ts `register()` hook (Next.js convention / server-entry + * hook for TanStack) so that PII scrub processors are active before the very + * first request handler runs — before bindAll() fires. Calling initOtelServerNode + * here as well would create a second SDK init path and reintroduce the startup + * window vulnerability (C1 fix). + */ export function bindOtelInstrumentation( container: Container, opts: BindOtelOpts, ): { tracer: ITracer; logger: ILogger; metrics: IMetrics } { - const environment = - process.env["SENTRY_ENVIRONMENT"] ?? - process.env["VERCEL_ENV"] ?? - process.env["NODE_ENV"] ?? - "development"; - const release = opts.release ?? process.env["VERCEL_GIT_COMMIT_SHA"] ?? "unknown"; - - initOtelServerNode({ - dsn: opts.dsn, - serviceName: opts.app, - environment, - release, - }); - const tracer = new OtelTracer(); const logger = new OtelLogger(); const metrics = new OtelMetrics(); diff --git a/packages/core-shared/src/instrumentation/index.ts b/packages/core-shared/src/instrumentation/index.ts index 933c2d1..3e01e58 100644 --- a/packages/core-shared/src/instrumentation/index.ts +++ b/packages/core-shared/src/instrumentation/index.ts @@ -27,6 +27,4 @@ export { export { bindOtelInstrumentation as bindSentryInstrumentation } from "./di/bind-otel-instrumentation"; export type { BindOtelOpts as BindSentryOpts } from "./di/bind-otel-instrumentation"; -// initSentryServerNode removed from barrel — callers use the subpath export directly: -// @repo/core-shared/instrumentation/sentry/init-server-node export { initSentryClientReact } from "./sentry/init-client-react"; diff --git a/packages/core-shared/src/instrumentation/sentry/init-server-node.test.ts b/packages/core-shared/src/instrumentation/sentry/init-server-node.test.ts deleted file mode 100644 index 3cadb04..0000000 --- a/packages/core-shared/src/instrumentation/sentry/init-server-node.test.ts +++ /dev/null @@ -1,53 +0,0 @@ -// packages/core-shared/src/instrumentation/sentry/init-server-node.test.ts -import { describe, it, expect, vi, beforeEach } from "vitest"; - -vi.mock("@sentry/node", () => ({ - init: vi.fn(), -})); - -import * as SentryNode from "@sentry/node"; -import { initSentryServerNode } from "@/instrumentation/sentry/init-server-node"; - -describe("initSentryServerNode", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it("calls SentryNode.init with sendDefaultPii: false (R31)", () => { - initSentryServerNode({ dsn: "https://x@y/1", app: "web-tanstack" }); - const call = (SentryNode.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - expect(call["sendDefaultPii"]).toBe(false); - }); - - it("does NOT attach beforeSend/beforeSendTransaction (scrubbing is at OTel layer)", () => { - initSentryServerNode({ dsn: "https://x@y/1", app: "web-tanstack" }); - const call = (SentryNode.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - // PII scrubbing happens in PiiScrubSpanProcessor / PiiScrubLogRecordProcessor - // before data reaches the Sentry exporter — no need for beforeSend hooks here. - expect(call["beforeSend"]).toBeUndefined(); - expect(call["beforeSendTransaction"]).toBeUndefined(); - }); - - it("tags events with the app name", () => { - initSentryServerNode({ dsn: "https://x@y/1", app: "web-tanstack" }); - const call = (SentryNode.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - const initialScope = call["initialScope"] as { tags?: Record }; - expect(initialScope?.tags?.["app"]).toBe("web-tanstack"); - }); - - it("is a no-op when dsn is missing", () => { - initSentryServerNode({ dsn: "", app: "web-tanstack" }); - expect(SentryNode.init).not.toHaveBeenCalled(); - initSentryServerNode({ dsn: undefined as unknown as string, app: "web-tanstack" }); - expect(SentryNode.init).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/core-shared/src/instrumentation/sentry/init-server-node.ts b/packages/core-shared/src/instrumentation/sentry/init-server-node.ts deleted file mode 100644 index 992f0a1..0000000 --- a/packages/core-shared/src/instrumentation/sentry/init-server-node.ts +++ /dev/null @@ -1,40 +0,0 @@ -// packages/core-shared/src/instrumentation/sentry/init-server-node.ts -// NOTE: PII scrubbing is now handled at the OTel pipeline layer via PiiScrubSpanProcessor -// and PiiScrubLogRecordProcessor (see otel/pii-scrub-processor.ts). The beforeSend / -// beforeSendTransaction hooks are no longer needed here (R32, R33 still enforced at OTel layer). -import * as SentryNode from "@sentry/node"; -import type { InitServerOpts } from "./init-server"; - -/** - * Server-side init for non-Next.js runtimes (TanStack Start). Mirrors - * init-server.ts but uses @sentry/node directly. R31, R32, R33 still apply - * (scrubbing enforced at the OTel processor layer before data reaches Sentry). - */ -export function initSentryServerNode(opts: InitServerOpts): void { - if (!opts.dsn) return; - - const isProd = process.env["NODE_ENV"] === "production"; - const tracesSampleRate = - process.env["SENTRY_TRACES_SAMPLE_RATE"] !== undefined - ? Number(process.env["SENTRY_TRACES_SAMPLE_RATE"]) - : isProd - ? 0.1 - : 1.0; - - const environment = - process.env["SENTRY_ENVIRONMENT"] ?? - process.env["VERCEL_ENV"] ?? - process.env["NODE_ENV"] ?? - "development"; - const release = opts.release ?? process.env["VITE_GIT_COMMIT_SHA"] ?? "unknown"; - - SentryNode.init({ - dsn: opts.dsn, - environment, - release, - tracesSampleRate, - sendDefaultPii: false, // R31 - // R32, R33: PII scrubbing happens at the OTel processor layer before data reaches Sentry. - initialScope: { tags: { app: opts.app } }, - }); -} diff --git a/packages/core-shared/src/instrumentation/sentry/init-server.test.ts b/packages/core-shared/src/instrumentation/sentry/init-server.test.ts deleted file mode 100644 index 6b20b9c..0000000 --- a/packages/core-shared/src/instrumentation/sentry/init-server.test.ts +++ /dev/null @@ -1,101 +0,0 @@ -// packages/core-shared/src/instrumentation/sentry/init-server.test.ts -import { describe, it, expect, vi, beforeEach } from "vitest"; - -vi.mock("@sentry/nextjs", () => ({ - init: vi.fn(), - replayIntegration: vi.fn(() => ({ name: "Replay" })), -})); - -import * as Sentry from "@sentry/nextjs"; -import { initSentryServer } from "@/instrumentation/sentry/init-server"; - -describe("initSentryServer", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it("calls Sentry.init with sendDefaultPii: false (R31)", () => { - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - expect(Sentry.init).toHaveBeenCalledTimes(1); - const call = (Sentry.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - expect(call["sendDefaultPii"]).toBe(false); - }); - - it("passes the configured DSN", () => { - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - const call = (Sentry.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - expect(call["dsn"]).toBe("https://x@y/1"); - }); - - it("does NOT attach beforeSend/beforeSendTransaction (scrubbing is at OTel layer)", () => { - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - const call = (Sentry.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - // PII scrubbing happens in PiiScrubSpanProcessor / PiiScrubLogRecordProcessor - // before data reaches the Sentry exporter — no need for beforeSend hooks here. - expect(call["beforeSend"]).toBeUndefined(); - expect(call["beforeSendTransaction"]).toBeUndefined(); - }); - - it("uses SENTRY_TRACES_SAMPLE_RATE env when set", () => { - vi.stubEnv("SENTRY_TRACES_SAMPLE_RATE", "0.25"); - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - const call = (Sentry.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - expect(call["tracesSampleRate"]).toBe(0.25); - vi.unstubAllEnvs(); - }); - - it("defaults tracesSampleRate to 1.0 in dev, 0.1 in production", () => { - // Save and clear rate env to test default logic - const prevRate = process.env["SENTRY_TRACES_SAMPLE_RATE"]; - delete process.env["SENTRY_TRACES_SAMPLE_RATE"]; - - vi.stubEnv("NODE_ENV", "development"); - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - expect( - ((Sentry.init as ReturnType).mock.calls[0]![0] as Record)[ - "tracesSampleRate" - ], - ).toBe(1.0); - - (Sentry.init as ReturnType).mockClear(); - vi.stubEnv("NODE_ENV", "production"); - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - expect( - ((Sentry.init as ReturnType).mock.calls[0]![0] as Record)[ - "tracesSampleRate" - ], - ).toBe(0.1); - - vi.unstubAllEnvs(); - if (prevRate !== undefined) process.env["SENTRY_TRACES_SAMPLE_RATE"] = prevRate; - }); - - it("tags events with the app name", () => { - initSentryServer({ dsn: "https://x@y/1", app: "web-next" }); - const call = (Sentry.init as ReturnType).mock.calls[0]![0] as Record< - string, - unknown - >; - const initialScope = call["initialScope"] as { tags?: Record }; - expect(initialScope?.tags?.["app"]).toBe("web-next"); - }); - - it("is a no-op when dsn is empty/undefined", () => { - initSentryServer({ dsn: "", app: "web-next" }); - expect(Sentry.init).not.toHaveBeenCalled(); - initSentryServer({ dsn: undefined as unknown as string, app: "web-next" }); - expect(Sentry.init).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/core-shared/src/instrumentation/sentry/init-server.ts b/packages/core-shared/src/instrumentation/sentry/init-server.ts deleted file mode 100644 index 46125da..0000000 --- a/packages/core-shared/src/instrumentation/sentry/init-server.ts +++ /dev/null @@ -1,40 +0,0 @@ -// packages/core-shared/src/instrumentation/sentry/init-server.ts -// NOTE: PII scrubbing is now handled at the OTel pipeline layer via PiiScrubSpanProcessor -// and PiiScrubLogRecordProcessor (see otel/pii-scrub-processor.ts). The beforeSend / -// beforeSendTransaction hooks are no longer needed here (R32, R33 still enforced at OTel layer). -import * as Sentry from "@sentry/nextjs"; - -export type InitServerOpts = { - dsn: string | undefined; - app: "web-next" | "cms" | "web-tanstack"; - release?: string; -}; - -export function initSentryServer(opts: InitServerOpts): void { - if (!opts.dsn) return; - - const isProd = process.env["NODE_ENV"] === "production"; - const tracesSampleRate = - process.env["SENTRY_TRACES_SAMPLE_RATE"] !== undefined - ? Number(process.env["SENTRY_TRACES_SAMPLE_RATE"]) - : isProd - ? 0.1 - : 1.0; - - const environment = - process.env["SENTRY_ENVIRONMENT"] ?? - process.env["VERCEL_ENV"] ?? - process.env["NODE_ENV"] ?? - "development"; - const release = opts.release ?? process.env["VERCEL_GIT_COMMIT_SHA"] ?? "unknown"; - - Sentry.init({ - dsn: opts.dsn, - environment, - release, - tracesSampleRate, - sendDefaultPii: false, // R31 — non-negotiable - // R32, R33: PII scrubbing happens at the OTel processor layer before data reaches Sentry. - initialScope: { tags: { app: opts.app } }, - }); -}