From fc30454ca9f653d6d365924cdee117e04f4b5785 Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:12:12 +0200 Subject: [PATCH] fix(currency): the browser was calling CoinGecko directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rateSource.server.ts opens with "Server-side only. Browsers read /api/rates instead, which keeps the rate on one origin (no third-party connect-src) and collapses one upstream call per visitor into one per minute for the whole platform." That was not true in production. Measured 2026-08-28 on orangecat.ch: api.coingecko.com present in a CLIENT chunk on disk /opt/orangecat/app/.next/static/chunks/0c7qzb-nbsq51.js browser fetched https://api.coingecko.com/api/v3/simple/price directly 534ms, in the critical path, on page load So every visitor's IP went to a third party, and the "one call per minute for the platform" became one per visitor. serverOnlyModules.test.ts exists to prevent exactly this and did not, because it only looked for a `.server` import inside a file that itself declares 'use client'. That is not how bundling works: anything a client component reaches, through however many hops, is compiled into the client bundle, and the files in between carry no directive. The real chain was three hops and every file after the first looked innocent: dashboard/bookings/page.tsx 'use client' → services/bookings/index.ts (no directive) → services/currency/rates.server.ts → services/currency/rateSource.server.ts → CoinGecko The walk is transitive now and the failure prints the whole chain, since the import that matters is rarely in the file you would open first. It finds 431 client entry points that can reach a .server module, so it lands as a ratchet at exactly that number rather than a demand for zero — red about unscheduled work is how a gate gets ignored. Nothing has been retired yet and the number is not a list to grind down one file at a time: the chains run through shared helpers (config/cat-plans → services/cat/credit-metering → rates.server accounts for a large share), so cutting one edge retires dozens. That refactor belongs in its own change, not smuggled into the commit that first makes the problem visible. The concrete harm is fixed now and defensively: fetchUpstream refuses to call out when it is not running on Node, so even bundled into a client chunk it cannot leak. Deliberately not `typeof window` — jsdom defines it, so that check would refuse during every component test and report a green suite for a module that never ran. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012dpTLxh5GJWeWTF1UEvcD5 --- __tests__/unit/serverOnlyModules.test.ts | 153 ++++++++++++++++-- .../rate-refresh-outside-render.test.ts | 23 +++ src/services/currency/rateSource.server.ts | 35 ++++ 3 files changed, 197 insertions(+), 14 deletions(-) diff --git a/__tests__/unit/serverOnlyModules.test.ts b/__tests__/unit/serverOnlyModules.test.ts index bf50f8120..b32151049 100644 --- a/__tests__/unit/serverOnlyModules.test.ts +++ b/__tests__/unit/serverOnlyModules.test.ts @@ -3,19 +3,39 @@ * * The suffix is a convention, and conventions don't fail builds. The stakes are * concrete: `services/currency/rateSource.server.ts` reaches a third party and - * kicks off a refresh the moment it loads. Bundled into a client chunk it would - * ship an outbound CoinGecko call to every visitor — a CSP violation, a privacy + * kicks off a refresh the moment it loads. Bundled into a client chunk it ships + * an outbound CoinGecko call to every visitor — a CSP violation, a privacy * leak, and one upstream request per person instead of one per minute for the * whole platform. Browsers get rates from our own /api/rates instead. * - * A `'use client'` file importing anything `.server` is that mistake, so this - * catches it in CI rather than in a network tab. + * This checked only whether a `'use client'` file imported a `.server` module + * DIRECTLY, and that is not how bundling works. Anything a client component + * reaches, through however many hops, is compiled into the client bundle — and + * the intermediate files carry no directive to give the game away. + * + * It shipped. Measured in production 2026-08-28: the browser fetched + * `https://api.coingecko.com/api/v3/simple/price` directly on page load, 534ms + * in the critical path, and `api.coingecko.com` was present in a client chunk + * on disk. The chain was three hops and every file after the first looked + * innocent: + * + * dashboard/bookings/page.tsx 'use client' + * → services/bookings/index.ts (no directive) + * → services/currency/rates.server.ts + * → services/currency/rateSource.server.ts → CoinGecko + * + * So the walk is transitive now, and the failure names the whole chain — a bare + * "this file is an offender" is close to useless when the import that matters + * is three files away from the one you have to edit. */ -import { readFileSync, readdirSync, statSync } from 'node:fs'; -import { join } from 'node:path'; +import { existsSync, readFileSync, readdirSync, statSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; -const IMPORT_SERVER = /(?:from|import\()\s*['"][^'"]*\.server['"]/; +const SRC = 'src'; + +/** `from '…'` and `import('…')`, capturing the specifier. */ +const IMPORT_SPECIFIER = /(?:from|import\()\s*['"]([^'"]+)['"]/g; function sourceFiles(dir: string, out: string[] = []): string[] { for (const name of readdirSync(dir)) { @@ -29,13 +49,118 @@ function sourceFiles(dir: string, out: string[] = []): string[] { return out; } -describe("'use client' modules never import server-only code", () => { - it('finds no client file importing a *.server module', () => { - const offenders = sourceFiles('src').filter(file => { - const source = readFileSync(file, 'utf8'); - return /^\s*['"]use client['"]/m.test(source) && IMPORT_SERVER.test(source); - }); +function isClientFile(path: string): boolean { + return /^\s*['"]use client['"]/m.test(readFileSync(path, 'utf8')); +} + +/** + * Turn a specifier into a file we can keep walking, or null for anything that + * cannot pull our own server code in (node_modules, css, assets). + */ +function resolveSpecifier(fromFile: string, specifier: string): string | null { + let base: string; + if (specifier.startsWith('@/')) { + base = join(SRC, specifier.slice(2)); + } else if (specifier.startsWith('.')) { + base = resolve(dirname(fromFile), specifier); + } else { + return null; // package import + } + + for (const candidate of [ + base, + `${base}.ts`, + `${base}.tsx`, + join(base, 'index.ts'), + join(base, 'index.tsx'), + ]) { + if (existsSync(candidate) && statSync(candidate).isFile()) { + return candidate; + } + } + return null; +} + +function importsOf(file: string): string[] { + const source = readFileSync(file, 'utf8'); + const out: string[] = []; + for (const match of source.matchAll(IMPORT_SPECIFIER)) { + const resolved = resolveSpecifier(file, match[1]); + if (resolved) { + out.push(resolved); + } + } + return out; +} + +/** + * @returns the import chain from `entry` to the first `.server` module it can + * reach, or null. Depth-first and memoized per entry: the graph is small, and + * naming ONE complete chain per offending page is what makes this fixable. + */ +function chainToServerModule(entry: string): string[] | null { + const seen = new Set(); + + function walk(file: string, trail: string[]): string[] | null { + if (seen.has(file)) { + return null; + } + seen.add(file); + + for (const next of importsOf(file)) { + const nextTrail = [...trail, next]; + if (/\.server\.tsx?$/.test(next)) { + return nextTrail; + } + const deeper = walk(next, nextTrail); + if (deeper) { + return deeper; + } + } + return null; + } + + return walk(entry, [entry]); +} + +/** + * How many client entry points can currently reach a `.server` module. + * + * 431 when the transitive walk was first switched on, and deliberately left at + * exactly that: a ratchet, not a target. Demanding zero today would make this + * red about work nobody has scheduled, which is how a gate teaches people to + * ignore it. It may fall or hold, never rise. + * + * Nothing has been retired yet, and the number is not a to-do list to grind + * down one file at a time. The chains run through shared helpers — + * `config/cat-plans` → `services/cat/credit-metering` → `rates.server` accounts + * for a large share on its own — so cutting one edge retires dozens at once. + * Splitting those modules is a real refactor and belongs in its own change, + * not smuggled into the commit that first makes the problem visible. + * + * The concrete harm this was hiding is fixed separately and defensively, in + * rateSource.server.ts: even bundled into a client chunk it now refuses to call + * out from a browser. + */ +const CLIENT_TO_SERVER_BASELINE = 431; + +describe("'use client' modules never reach server-only code", () => { + it('does not let more client entry points reach a *.server module', () => { + const offenders = sourceFiles(SRC) + .filter(isClientFile) + .map(file => chainToServerModule(file)) + .filter((chain): chain is string[] => chain !== null) + .map(chain => chain.join('\n → ')); + + if (offenders.length > CLIENT_TO_SERVER_BASELINE) { + // Print one full chain so the failure is actionable: the import that + // matters is usually several files from the one you would think to open. + throw new Error( + `${offenders.length} client entry points reach a .server module, up from ` + + `${CLIENT_TO_SERVER_BASELINE}. One chain:\n\n ${offenders[0]}\n` + ); + } - expect(offenders).toEqual([]); + expect(offenders.length).toBeLessThanOrEqual(CLIENT_TO_SERVER_BASELINE); }); }); diff --git a/__tests__/unit/services/rate-refresh-outside-render.test.ts b/__tests__/unit/services/rate-refresh-outside-render.test.ts index a02ee4ade..560914b3d 100644 --- a/__tests__/unit/services/rate-refresh-outside-render.test.ts +++ b/__tests__/unit/services/rate-refresh-outside-render.test.ts @@ -18,6 +18,7 @@ import { getCachedRateSnapshot, + getRateSnapshot, __setSnapshotForTests, } from '@/services/currency/rateSource.server'; @@ -82,4 +83,26 @@ describe('getCachedRateSnapshot refresh timing', () => { jest.runOnlyPendingTimers(); expect(fetchMock).not.toHaveBeenCalled(); }); + + // Production 2026-08-28: this module reached a client chunk through a + // transitive import and the browser called CoinGecko directly on page load — + // 534ms in the critical path and every visitor's IP handed to a third party. + // The import graph is being repaired separately; this refuses regardless. + it('never calls out when it finds itself running in a browser', async () => { + // Start from nothing cached, or a snapshot left by an earlier test would + // be served before the fetch is ever attempted and prove nothing. + __setSnapshotForTests(null); + + // A browser bundle has no Node version record, even where `process.env` + // has been shimmed. jsdom's `window` is NOT the discriminator — it is + // present in this very test. + const versions = process.versions; + Object.defineProperty(process, 'versions', { value: {}, configurable: true }); + try { + await expect(getRateSnapshot()).resolves.toBeNull(); + expect(fetchMock).not.toHaveBeenCalled(); + } finally { + Object.defineProperty(process, 'versions', { value: versions, configurable: true }); + } + }); }); diff --git a/src/services/currency/rateSource.server.ts b/src/services/currency/rateSource.server.ts index 29ef80be7..1a2d42252 100644 --- a/src/services/currency/rateSource.server.ts +++ b/src/services/currency/rateSource.server.ts @@ -62,6 +62,20 @@ function ageOf(s: RateSnapshot): number { return Date.now() - s.fetchedAt; } +/** + * Are we executing in a browser rather than on the server? + * + * Deliberately NOT `typeof window`. jsdom — every component test in this repo — + * defines `window`, so that check would refuse to fetch during tests and report + * a passing suite for a module that never ran. Node identifies itself with a + * version record that bundlers do not synthesise when they shim `process.env` + * for the browser, which distinguishes the two environments that actually + * matter here. + */ +function runningInBrowser(): boolean { + return typeof process === 'undefined' || !process.versions?.node; +} + /** * A rate we would be willing to price money with. * @@ -75,6 +89,27 @@ function isSaneRate(value: unknown): value is number { } async function fetchUpstream(): Promise { + // This module is server-only by contract, and the contract was being broken. + // Measured in production 2026-08-28: `api.coingecko.com` was present in a + // client chunk on disk, and the browser fetched it directly on page load — + // 534ms in the critical path, one upstream call per visitor instead of one + // per minute for the platform, and every visitor's IP handed to a third + // party. It arrives through a transitive import that carries no directive + // (dashboard page → services/bookings → rates.server → here), which is why + // the `'use client'` check never saw it. + // + // The import graph is being fixed separately and slowly; this is the part + // that must not wait. Browsers have /api/rates, which is same-origin and + // shared, so refusing here costs them nothing. + if (runningInBrowser()) { + logger.warn( + 'Refusing to fetch rates from a browser — use /api/rates', + { reason: 'rateSource.server reached the client bundle' }, + 'Currency' + ); + return null; + } + try { const response = await fetch(SOURCE_URL, { headers: { Accept: 'application/json' },