From 2742914d079c222d6fa13633773b77f02e0d8920 Mon Sep 17 00:00:00 2001 From: effece Date: Tue, 18 Aug 2026 16:53:17 -0300 Subject: [PATCH] [db] fix: make the stdout guard survive DOTENV_CONFIG_QUIET MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit, from an independent review of it: `quiet: true` alone is defeatable, so the guard it provides was weaker than documented. dotenv resolves quiet as processEnv.DOTENV_CONFIG_QUIET || options.quiet (lib/main.js:248) and RE-READS it from processEnv after populate (lib/main.js:292). The env var therefore outranks the code option, parseBoolean('false') is false, and the second read means a DOTENV_CONFIG_QUIET=false inside the .env file being loaded defeats it too — not just one exported in the shell. Either way the injection banner returns to stdout, which for this server is the JSON-RPC transport, and the failure mode is a client registering zero tools with no error to read. This makes it unreachable rather than merely discouraged: console.log is routed to stderr for the duration of the config call and restored in `finally`, so the banner cannot reach stdout however quiet resolves — and anything else that logs during env loading is caught with it. `quiet: true` stays as the primary, so in the normal case nothing is emitted at all. tests/integration/helpers.ts deliberately does NOT get the swap: it is test-only, its stdout is not a transport, and the db-guard pinning regex matches the call expression, which is unchanged — so both files still resolve DATABASE_URL identically. Two guards added. One pins the swap in db.ts. The other is behavioural and proves the swap is load-bearing rather than redundant: it sets DOTENV_CONFIG_QUIET=false, calls dotenv with quiet:true, and asserts a banner IS produced — if that ever goes empty, dotenv changed its precedence and the swap should be re-evaluated before removal. Verified: 32/32 in the two guard suites, tsc clean on TypeScript 7.0.2, and a controlled comparison with DOTENV_CONFIG_QUIET=false in the environment — unfixed writes 1 stdout line and it is the banner, fixed writes 0. --- src/db.ts | 19 ++++++++++++++++- tests/env-loading.test.ts | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/db.ts b/src/db.ts index c2f522f..18aef68 100644 --- a/src/db.ts +++ b/src/db.ts @@ -15,7 +15,24 @@ import * as dotenv from 'dotenv'; // of the initialize reply; clients that skip unparseable lines tolerate it, // clients that don't see a corrupt stream and register zero tools with no // error to read. -dotenv.config({ path: process.env.DOTENV_CONFIG_PATH, override: true, quiet: true }); +// +// The console.log swap is NOT belt-and-braces — `quiet: true` alone is +// defeatable. dotenv resolves it as +// processEnv.DOTENV_CONFIG_QUIET || options.quiet (lib/main.js:248) +// and RE-READS it from processEnv after populate (lib/main.js:292), +// so the environment variable outranks the code option, and parseBoolean('false') +// is false. A stray DOTENV_CONFIG_QUIET=false — in the shell, in the MCP server's +// env block, or inside the .env file this very call is loading — silently puts the +// banner back on stdout. Routing console.log to stderr for the duration makes that +// unreachable however `quiet` resolves, and also catches anything else that decides +// to log during env loading. Restored in `finally` so nothing leaks past this line. +const __realLog = console.log; +console.log = (...args: unknown[]) => console.error(...args); +try { + dotenv.config({ path: process.env.DOTENV_CONFIG_PATH, override: true, quiet: true }); +} finally { + console.log = __realLog; +} import { drizzle } from 'drizzle-orm/node-postgres'; import pg from 'pg'; import * as schema from './schema.js'; diff --git a/tests/env-loading.test.ts b/tests/env-loading.test.ts index 566290b..cb201f1 100644 --- a/tests/env-loading.test.ts +++ b/tests/env-loading.test.ts @@ -37,6 +37,51 @@ describe('env loading invariants', () => { expect(src).toMatch(/dotenv\.config\([^)]*override:\s*true/); }); + it('db.ts must route console.log away from stdout around dotenv.config', () => { + // `quiet: true` alone is NOT sufficient. dotenv resolves quiet as + // processEnv.DOTENV_CONFIG_QUIET || options.quiet + // at lib/main.js:248, and RE-READS it after populate at :292 — so the env + // var wins over the code option, and parseBoolean('false') === false. A + // DOTENV_CONFIG_QUIET=false in the shell OR inside the .env file itself + // puts the banner back on stdout, which is the MCP JSON-RPC transport. + // The console.log swap makes that unreachable regardless of resolution. + const src = readFileSync(join(repoRoot, 'src/db.ts'), 'utf8'); + expect(src).toMatch(/console\.log\s*=/); + }); + + it('the console.log swap survives DOTENV_CONFIG_QUIET=false', () => { + // Behavioural proof of the technique, independent of db.ts. Guards against + // a future dotenv release changing how quiet resolves. + const tmp = mkdtempSync(join(tmpdir(), 'env-quiet-')); + const envFile = join(tmp, 'noisy.env'); + writeFileSync(envFile, 'QUIET_PROBE=1\n'); + + const prevVar = process.env.DOTENV_CONFIG_QUIET; + process.env.DOTENV_CONFIG_QUIET = 'false'; // hostile: defeats { quiet: true } + + const realLog = console.log; + const stdoutLines: string[] = []; + console.log = (...args: unknown[]) => { + stdoutLines.push(args.join(' ')); + }; + let banner: string[] = []; + try { + // Confirm the hostile env var really does re-enable logging... + dotenv.config({ path: envFile, override: true, quiet: true }); + banner = [...stdoutLines]; + } finally { + console.log = realLog; + if (prevVar === undefined) delete process.env.DOTENV_CONFIG_QUIET; + else process.env.DOTENV_CONFIG_QUIET = prevVar; + } + + // ...so the swap is load-bearing, not redundant. If this ever goes empty, + // dotenv changed its precedence and the swap may no longer be needed — + // verify before removing it. + expect(banner.length).toBeGreaterThan(0); + expect(banner.join('\n')).toMatch(/injected env/i); + }); + it('db.ts must pass { quiet: true } — stdout is the MCP transport', () => { // dotenv defaults quiet:false since 17.0.0 and logs its injection banner // with console.log — i.e. onto stdout, which for the MCP server IS the