diff --git a/server/src/app-config/README.md b/server/src/app-config/README.md index 3147d4eb0..4a3d6eea8 100644 --- a/server/src/app-config/README.md +++ b/server/src/app-config/README.md @@ -8,10 +8,10 @@ the pre-init Express middleware. ``` env.schema.ts Zod catalog of the whole env surface; fail-fast at boot derive.ts pure (raw env) → typed namespace functions, exact per-site coercions -env.ts readEnv() live accessor + validateEnvAtBoot() +env.ts readEnv() live accessor + normalizeEnvAtBoot() + validateEnvAtBoot() app-url.ts getAppUrl()/getMcpSafeUrl() instance base-URL resolution data-paths.ts resolveDataPaths(): data/, uploads/, backups, tmp, logs, key files, DB file -boot-validate.ts side-effect import used by index.ts (validates before other modules load) +boot-validate.ts side-effect import used by index.ts (trims, then validates before other modules load) parsers.ts shared coercion helpers (boolTrueLoose, numberOr, csvList, …) ``` @@ -200,3 +200,5 @@ The exemption list is enforced by the `no-restricted-syntax` ban on - NODE_ENV comparisons are case-sensitive at some sites (HSTS activation, platform statics, spa-fallback, authService dev_mode, oidcService frontendUrl) and case-insensitive at others. + Surrounding whitespace is stripped once at boot (`normalizeEnvAtBoot`, + together with LOG_LEVEL and DEFAULT_LANGUAGE), the casing is not. diff --git a/server/src/app-config/boot-validate.ts b/server/src/app-config/boot-validate.ts index a9edc1593..bd780aa51 100644 --- a/server/src/app-config/boot-validate.ts +++ b/server/src/app-config/boot-validate.ts @@ -1,12 +1,16 @@ /** - * Side-effect module: validates the environment the moment it is imported. + * Side-effect module: normalizes and validates the environment the moment it + * is imported. * * index.ts imports this immediately after `import 'dotenv/config'` — as a * side-effect import (not a function call) so validation runs BEFORE the rest * of the import graph executes its module-load side effects (config.ts key * resolution, ...). A plain call in index.ts's body - * would run after all of those, because imports hoist. + * would run after all of those, because imports hoist. The trim runs first, + * so everything after it, Express and the plugin children included, reads the + * cleaned value. */ -import { validateEnvAtBoot } from './env'; +import { normalizeEnvAtBoot, validateEnvAtBoot } from './env'; +normalizeEnvAtBoot(); validateEnvAtBoot(); diff --git a/server/src/app-config/env.schema.ts b/server/src/app-config/env.schema.ts index f49250feb..d69c82231 100644 --- a/server/src/app-config/env.schema.ts +++ b/server/src/app-config/env.schema.ts @@ -60,6 +60,7 @@ export const envSchema = z.object({ HOST: anyString, // NODE_ENV and TZ are deliberately unvalidated: non-standard values (e.g. // NODE_ENV=staging) behave as "not production" today and must keep booting. + // Surrounding whitespace on NODE_ENV is trimmed before this runs (env.ts). NODE_ENV: anyString, TZ: anyString, LOG_LEVEL: oneOf(['error', 'warn', 'info', 'debug']), diff --git a/server/src/app-config/env.ts b/server/src/app-config/env.ts index 34ee6fbed..8b2dbf433 100644 --- a/server/src/app-config/env.ts +++ b/server/src/app-config/env.ts @@ -39,6 +39,45 @@ function reportedValue(key: string, value: string | undefined): string { return SECRET_ENV_KEYS.has(key) ? '***' : JSON.stringify(value); } +/** + * Variables whose surrounding whitespace is stripped once at boot. The schema + * already validates them trimmed, but every reader compares the raw string: + * NODE_ENV="production " served no web UI at all (#2565), LOG_LEVEL="debug " + * fell back to info, DEFAULT_LANGUAGE="de " to English. Deliberately a short + * list, not every variable: a secret may carry its spaces on purpose. + */ +const TRIMMED_AT_BOOT = ['NODE_ENV', 'LOG_LEVEL', 'DEFAULT_LANGUAGE'] as const; + +/** + * Strips the whitespace a .env line or a compose file left around the values in + * TRIMMED_AT_BOOT and writes the result back, so Express (`app.get('env')`), + * the plugin child processes and any library reading NODE_ENV itself see the + * same value as readEnv(). Casing is left alone. Each change is reported, since + * the banner cannot show a trailing space. + * + * NODE_ENV="test " is the one exception: trimmed it would switch a running + * instance onto an in-memory database without crons, so it is only reported. + */ +export function normalizeEnvAtBoot(raw: RawEnv = process.env as RawEnv): void { + for (const key of TRIMMED_AT_BOOT) { + const value = raw[key]; + if (value === undefined) continue; + const trimmed = value.trim(); + if (trimmed === value) continue; + if (key === 'NODE_ENV' && trimmed === 'test') { + bootWarning(`${key}=${JSON.stringify(value)} has surrounding whitespace and is not treated as "test".`); + continue; + } + raw[key] = trimmed; + bootWarning(`${key}=${JSON.stringify(value)} has surrounding whitespace, using ${JSON.stringify(trimmed)}.`); + } +} + +/** Runs before the file logger exists, so it goes to stderr as a process warning. */ +function bootWarning(message: string): void { + process.emitWarning(message, { code: 'TREK_ENV_WHITESPACE' }); +} + /** * Fail-fast startup validation: a variable that is PRESENT but malformed aborts * boot with an aggregated report; unset/blank variables always pass (their diff --git a/server/src/index.ts b/server/src/index.ts index ebad020c5..930de1f7b 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -53,7 +53,7 @@ const onListen = () => { ...(HOST ? [` Host: ${HOST}`] : []), ` Container Port: ${PORT}`, ` App URL: ${appUrl}`, - ` Environment: ${env.app.nodeEnv?.toLowerCase() || 'development'}`, + ` Environment: ${env.app.nodeEnv || 'development'}`, ` Timezone: ${tz}`, ` Origins: ${origins}`, ` Log level: ${LOG_LVL}`, diff --git a/server/tests/unit/app-config/boot-normalize.test.ts b/server/tests/unit/app-config/boot-normalize.test.ts new file mode 100644 index 000000000..ff1b49b5b --- /dev/null +++ b/server/tests/unit/app-config/boot-normalize.test.ts @@ -0,0 +1,70 @@ +import { normalizeEnvAtBoot, readEnv } from '../../../src/app-config/env'; + +import { describe, it, expect, vi, afterEach } from 'vitest'; + +/** Everything one normalisation printed as a warning. */ +function warnings(raw: Record): string { + const warn = vi.spyOn(process, 'emitWarning').mockImplementation(() => {}); + normalizeEnvAtBoot(raw); + const printed = warn.mock.calls.map((c) => String(c[0])).join('\n'); + warn.mockRestore(); + return printed; +} + +describe('normalizeEnvAtBoot (#2565)', () => { + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + vi.resetModules(); + }); + + it('trims a padded NODE_ENV in place and says so', () => { + for (const padded of ['production ', ' production', 'production\r', '\tproduction\n']) { + const raw: Record = { NODE_ENV: padded }; + const printed = warnings(raw); + expect(raw.NODE_ENV, JSON.stringify(padded)).toBe('production'); + expect(printed).toContain(`NODE_ENV=${JSON.stringify(padded)}`); + expect(printed).toContain('"production"'); + } + }); + + it('trims LOG_LEVEL and DEFAULT_LANGUAGE the same way', () => { + const raw: Record = { LOG_LEVEL: 'debug ', DEFAULT_LANGUAGE: ' de' }; + const printed = warnings(raw); + expect(raw.LOG_LEVEL).toBe('debug'); + expect(raw.DEFAULT_LANGUAGE).toBe('de'); + expect(printed).toContain('LOG_LEVEL="debug "'); + expect(printed).toContain('DEFAULT_LANGUAGE=" de"'); + }); + + it('leaves clean, unset and differently cased values alone and stays quiet', () => { + const raw: Record = { NODE_ENV: 'Production', LOG_LEVEL: 'info' }; + expect(warnings(raw)).toBe(''); + expect(raw).toEqual({ NODE_ENV: 'Production', LOG_LEVEL: 'info' }); + }); + + it('does not turn a padded "test" into the test runtime', () => { + // NODE_ENV=test means an in-memory database and no crons. An instance that + // has been running on "test " with its file database must not wake up on + // an empty one after an update, so this one is only reported. + const raw: Record = { NODE_ENV: 'test ' }; + const printed = warnings(raw); + expect(raw.NODE_ENV).toBe('test '); + expect(printed).toContain('NODE_ENV="test "'); + }); + + it('never touches other variables, secrets keep their surrounding spaces', () => { + const raw: Record = { SMTP_PASS: ' pass ', TZ: 'UTC ' }; + expect(warnings(raw)).toBe(''); + expect(raw).toEqual({ SMTP_PASS: ' pass ', TZ: 'UTC ' }); + }); + + it('runs from the boot import, so everything reading process.env sees the trimmed value', async () => { + vi.stubEnv('NODE_ENV', 'production '); + vi.spyOn(process, 'emitWarning').mockImplementation(() => {}); + await import('../../../src/app-config/boot-validate'); + expect(process.env.NODE_ENV).toBe('production'); + expect(readEnv().app.nodeEnv).toBe('production'); + expect(readEnv().app.isProduction).toBe(true); + }); +}); diff --git a/wiki/Environment-Variables.md b/wiki/Environment-Variables.md index 88f07a870..2fbb89904 100644 --- a/wiki/Environment-Variables.md +++ b/wiki/Environment-Variables.md @@ -38,7 +38,8 @@ aborting, because `reset-admin.js` — the way back into a locked-out instance to keep working. The three plugin caps `TREK_PLUGIN_AI_PER_DAY`, `TREK_PLUGIN_NOTIFY_PER_DAY` and `TREK_PLUGIN_AUDIT_MAX_ROWS` are read by the plugin host rather than by the boot schema, so a malformed value there falls back to the default without a warning. `NODE_ENV` and `TZ` are not validated at all, so a non-standard value -like `NODE_ENV=staging` still boots. +like `NODE_ENV=staging` still boots. Spaces around `NODE_ENV`, `LOG_LEVEL` and `DEFAULT_LANGUAGE` are trimmed at startup with a +warning in the log; the web UI is only served with `NODE_ENV=production` in lowercase. ---