From 07f690062b6a9f6816da141654837a62f113b107 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 16 Sep 2026 23:43:05 +0200 Subject: [PATCH 01/19] Validate environment variables at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a validated schema in start/env.ts using @adonisjs/env, so a misconfigured environment is reported once, in full, before the server accepts traffic. `required` means the application genuinely cannot start without the variable. Anything the code already supplies a default for is optional, with the default noted beside it, so deployments relying on a built-in default keep booting. HOST and LOG_LEVEL are dropped entirely, as nothing reads them. lib/env extends Adonis' Env rather than mutating Env.schema, which cannot be typed: the schema is a type alias over a const, so it resists declaration merging, and assigning onto the imported object would make validator availability depend on import order. Overriding the inherited static widens it instead, which stays assignable. Two validators live there: - `integer`, with `.positive()` and `.nonNegative()`. Env.schema.number casts with Number() and rejects only NaN, so it accepts 1.5 and -5 — weaker than the safeGetEnvInt helpers it replaces. Zero is allowed where zero is meaningful, such as a disabled timeout. - `hostList`, for SCYLLA_HOSTS. Validates each host and optional port and returns the parsed list, replacing a hand-rolled check that ran after validation had already finished and so could not be reported alongside other failures. Passwords and API keys use Env.schema.secret, which redacts itself in logs, JSON and string coercion; call .release() to read one. Two fixes fell out of running the schema against each env file rather than reading it: - UI_URL rejected http://localhost:3000, because the URL format requires a TLD by default. server/.env.example could not boot the server. - .env.githubci set NODE_ENV=CI, which is outside the enum. Changed to test. All four NODE_ENV comparisons in server/ are negative, so the two values behave identically; CI detection uses the separate CI variable. Co-Authored-By: Claude Opus 5 (1M context) --- .env.githubci | 2 +- CHANGELOG.md | 2 + server/bin/www.ts | 10 +- server/jest.config.cjs | 5 +- server/lib/env/index.ts | 44 +++++++++ server/lib/env/validators.test.ts | 139 ++++++++++++++++++++++++++ server/lib/env/validators.ts | 159 ++++++++++++++++++++++++++++++ server/package-lock.json | 125 +++++++++++++++++++++++ server/package.json | 10 ++ server/start/env.ts | 143 +++++++++++++++++++++++++++ 10 files changed, 629 insertions(+), 10 deletions(-) create mode 100644 server/lib/env/index.ts create mode 100644 server/lib/env/validators.test.ts create mode 100644 server/lib/env/validators.ts create mode 100644 server/start/env.ts diff --git a/.env.githubci b/.env.githubci index 8630ae61c..eab8fe804 100644 --- a/.env.githubci +++ b/.env.githubci @@ -66,7 +66,7 @@ REDIS_PORT=6379 OTEL_SERVICE_NAME=COOP_TEST_SERVICE -NODE_ENV=CI +NODE_ENV=test ITEM_QUEUE_TRAFFIC_PERCENTAGE='0' UI_URL=http://localhost:3000 diff --git a/CHANGELOG.md b/CHANGELOG.md index bc13d7c8b..66ad71bf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ For more information about each release including git tags and artifacts, see [R ### Changed +- Invalid environment variable values now prevent startup instead of falling back to defaults ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) +- Boolean environment variables accept only `1`, `0`, `true` and `false` ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Scylla is now optional via `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` ([#918](https://github.com/roostorg/coop/pull/918) by [@sunilatlas](https://github.com/sunilatlas)) - Settings "Other" tab renamed to "Partial Items" and its settings relocated ([#965](https://github.com/roostorg/coop/pull/965) by [@golden-fox07](https://github.com/golden-fox07)) - Queue deletion is refused while routing rules still reference the queue ([#808](https://github.com/roostorg/coop/pull/808) by [@reitblatt](https://github.com/reitblatt)) diff --git a/server/bin/www.ts b/server/bin/www.ts index 9612a4727..ba2d667f4 100755 --- a/server/bin/www.ts +++ b/server/bin/www.ts @@ -1,6 +1,7 @@ #!/usr/bin/env node import http from 'http'; import { promisify } from 'util'; +import env from '#start/env'; import _ from 'lodash'; import getBottle from '../iocContainer/index.js'; @@ -35,7 +36,7 @@ try { process.exit(1); } -const port = parsePort(process.env.PORT) ?? 8080; +const port = env.get('PORT', 8080); app.set('port', port); const server = http @@ -145,10 +146,3 @@ process.on('unhandledRejection', (reason) => { process.once('SIGTERM', shutdownOnce); process.once('SIGINT', shutdownOnce); - -function parsePort(val: string | undefined): number | undefined { - if (!val) return undefined; - - const parsed = parseInt(val, 10); - return isNaN(parsed) ? undefined : parsed; -} diff --git a/server/jest.config.cjs b/server/jest.config.cjs index 89b766db9..70ad01275 100644 --- a/server/jest.config.cjs +++ b/server/jest.config.cjs @@ -59,7 +59,10 @@ module.exports = { moduleNameMapper: {}, // An array of regexp pattern strings, matched against all module paths before considered 'visible' to the module loader - // modulePathIgnorePatterns: [], + // `build/` holds the compiled output, including a copy of `package.json` so + // the `#`-prefixed subpath imports resolve when running it. Without this, + // jest's haste map sees two packages named `server` and warns on every run. + modulePathIgnorePatterns: ['/build/'], // Activates notifications for test results // notify: false, diff --git a/server/lib/env/index.ts b/server/lib/env/index.ts new file mode 100644 index 000000000..84b88b650 --- /dev/null +++ b/server/lib/env/index.ts @@ -0,0 +1,44 @@ +import { Env as BaseEnv } from '@adonisjs/env'; + +import { hostList, integer } from './validators.js'; + +/** + * `@adonisjs/env`'s `Env` with our own validators folded into `schema`, so a + * schema literal reads uniformly as `Env.schema.*` regardless of whether a + * given validator ships with Adonis. + * + * Subclassing is what makes that possible. `Env.schema` is declared as a type + * alias over a const rather than an interface, so it cannot be extended by + * declaration merging, and assigning onto the imported object would leave the + * types behind while making validator availability depend on import order. + * Overriding the inherited static sidesteps both. + * + * Note `BaseEnv.create` constructs a `BaseEnv` rather than `new this(...)`, so + * it returns the base class. That is fine here: the subclass exists for the + * schema namespace, not for instance behaviour. + */ +export class Env< + EnvValues extends Record, +> extends BaseEnv { + static override schema = { + ...BaseEnv.schema, + integer, + hostList, + }; +} + +export { hostList, integer } from './validators.js'; + +/** + * Re-exported so `start/env.ts` imports everything it needs from here, keeping + * `@adonisjs/env` behind this module. + */ +export { errors } from '@adonisjs/env'; + +/** + * Re-exported so `config/*` modules can name secret-typed values without + * importing from `@adonisjs/env`'s own dependency tree. A `Secret` redacts + * itself in logs, `JSON.stringify` and string coercion; call `.release()` to + * read the underlying value. + */ +export type { Secret } from '@poppinss/utils'; diff --git a/server/lib/env/validators.test.ts b/server/lib/env/validators.test.ts new file mode 100644 index 000000000..072274823 --- /dev/null +++ b/server/lib/env/validators.test.ts @@ -0,0 +1,139 @@ +import { hostList, integer } from './validators.js'; + +describe('integer env validator', () => { + describe('integer', () => { + const validate = integer(); + + test('accepts any integer, including zero and negatives', () => { + expect(validate('SOME_VAR', '10')).toBe(10); + expect(validate('SOME_VAR', '0')).toBe(0); + expect(validate('SOME_VAR', '-3')).toBe(-3); + }); + + // `Env.schema.number` accepts this, because `Number('1.5')` is not NaN. + test('rejects floats', () => { + expect(() => validate('SOME_VAR', '1.5')).toThrow(); + }); + + // The retired helpers used `parseInt`, which read this as 12. + test('rejects a numeric prefix followed by junk', () => { + expect(() => validate('SOME_VAR', '12abc')).toThrow(); + }); + + test('rejects a missing or empty value', () => { + expect(() => validate('SOME_VAR', undefined)).toThrow(); + expect(() => validate('SOME_VAR', '')).toThrow(); + }); + + test('names the variable and the offending value', () => { + expect(() => validate('SOME_VAR', '1.5')).toThrow(/SOME_VAR/); + expect(() => validate('SOME_VAR', '1.5')).toThrow(/1\.5/); + }); + }); + + describe('integer.positive', () => { + const validate = integer.positive(); + + test('accepts an integer greater than zero', () => { + expect(validate('SOME_VAR', '7')).toBe(7); + }); + + test('rejects zero and negatives', () => { + expect(() => validate('SOME_VAR', '0')).toThrow(); + expect(() => validate('SOME_VAR', '-5')).toThrow(); + }); + }); + + describe('integer.nonNegative', () => { + const validate = integer.nonNegative(); + + test('accepts zero, since zero is meaningful for retries and timeouts', () => { + expect(validate('SOME_VAR', '0')).toBe(0); + }); + + test('rejects negatives and floats', () => { + expect(() => validate('SOME_VAR', '-1')).toThrow(); + expect(() => validate('SOME_VAR', '0.5')).toThrow(); + }); + }); + + describe('.optional()', () => { + test('returns undefined when unset or empty', () => { + expect( + integer.positive.optional()('SOME_VAR', undefined), + ).toBeUndefined(); + expect(integer.positive.optional()('SOME_VAR', '')).toBeUndefined(); + }); + + test('still validates a value that is present', () => { + expect(integer.positive.optional()('SOME_VAR', '3')).toBe(3); + expect(() => integer.positive.optional()('SOME_VAR', '0')).toThrow(); + }); + + // An empty value is absent, so the consumer's default applies rather than + // the feature being silently disabled by a zero. + test('treats an empty value as absent rather than zero', () => { + expect(integer.nonNegative.optional()('SOME_VAR', '')).toBeUndefined(); + expect(integer.nonNegative.optional()('SOME_VAR', '0')).toBe(0); + }); + }); +}); + +describe('hostList env validator', () => { + const validate = hostList(); + + // The three shipped env files between them use all of these forms. + test('accepts a bare hostname, an IP, and a host with a port', () => { + expect(validate('SCYLLA_HOSTS', 'scylla')).toEqual(['scylla']); + expect(validate('SCYLLA_HOSTS', '127.0.0.1')).toEqual(['127.0.0.1']); + expect(validate('SCYLLA_HOSTS', '127.0.0.1:9042')).toEqual([ + '127.0.0.1:9042', + ]); + }); + + test('splits a comma-separated list and trims each entry', () => { + expect(validate('SCYLLA_HOSTS', 'db1, db2 ,db3:9043')).toEqual([ + 'db1', + 'db2', + 'db3:9043', + ]); + }); + + test('ignores empty entries from stray commas', () => { + expect(validate('SCYLLA_HOSTS', 'db1,,db2,')).toEqual(['db1', 'db2']); + }); + + // This is what the old post-validation block existed to catch. + test('rejects a value that contains no hosts at all', () => { + expect(() => validate('SCYLLA_HOSTS', ',')).toThrow(/SCYLLA_HOSTS/); + expect(() => validate('SCYLLA_HOSTS', ' , , ')).toThrow(); + }); + + test('rejects an unset or empty value', () => { + expect(() => validate('SCYLLA_HOSTS', undefined)).toThrow(); + expect(() => validate('SCYLLA_HOSTS', '')).toThrow(); + }); + + test('rejects an invalid host', () => { + expect(() => validate('SCYLLA_HOSTS', 'not a host')).toThrow(); + expect(() => validate('SCYLLA_HOSTS', 'db1,not a host')).toThrow(); + }); + + test('rejects an out-of-range or non-numeric port', () => { + expect(() => validate('SCYLLA_HOSTS', 'db1:0')).toThrow(); + expect(() => validate('SCYLLA_HOSTS', 'db1:70000')).toThrow(); + expect(() => validate('SCYLLA_HOSTS', 'db1:abc')).toThrow(); + }); + + describe('.optional()', () => { + test('returns undefined when unset, since Scylla itself is optional', () => { + expect(hostList.optional()('SCYLLA_HOSTS', undefined)).toBeUndefined(); + expect(hostList.optional()('SCYLLA_HOSTS', '')).toBeUndefined(); + }); + + test('still validates a value that is present', () => { + expect(hostList.optional()('SCYLLA_HOSTS', 'db1')).toEqual(['db1']); + expect(() => hostList.optional()('SCYLLA_HOSTS', ',')).toThrow(); + }); + }); +}); diff --git a/server/lib/env/validators.ts b/server/lib/env/validators.ts new file mode 100644 index 000000000..fcc92789c --- /dev/null +++ b/server/lib/env/validators.ts @@ -0,0 +1,159 @@ +/** + * Integer validators, folded into `Env.schema` by `./index.js`. + * + * An Adonis schema entry is just a `(key, value) => T` function, so these sit + * alongside the built-in `Env.schema.*` validators without any special support. + * + * They exist because `Env.schema.number` casts with `Number()` and rejects only + * `NaN` — it accepts `1.5`, `-5` and `0` quite happily. Pool sizes, timeouts and + * counts need an integer within range, and a value outside it stops the process + * starting rather than being substituted with a default. + */ + +import { Env as BaseEnv } from '@adonisjs/env'; + +type Validator = (key: string, value?: string) => T; + +type IntegerValidator = (() => Validator) & { + optional: () => Validator; +}; + +/** `integer()` plus its range-constrained variants. */ +type IntegerValidators = IntegerValidator & { + positive: IntegerValidator; + nonNegative: IntegerValidator; +}; + +/** + * Schema functions signal failure by throwing a plain `Error`. `EnvValidator` + * catches each one, collects its `message`, and reports every invalid variable + * together in a single `E_INVALID_ENV_VARIABLES` whose `help` lists them all. + * + * Constructing that error here instead would collapse the detail, since its own + * `message` is the generic "Validation failed for one or more environment + * variables" — that, rather than the specifics, is what would be listed. + * + * The wording mirrors `@poppinss/validator-lite` so built-in and custom + * failures read identically in that list. + */ +function invalid(key: string, value: string, expectation: string): never { + throw new Error( + `"${key}" env variable must be ${expectation} (Current value: "${value}")`, + ); +} + +function castToInteger( + key: string, + value: string, + minimum: number, + expectation: string, +): number { + const casted = Number(value); + if (!Number.isInteger(casted) || casted < minimum) { + invalid(key, value, expectation); + } + return casted; +} + +/** + * An empty string is treated as absent rather than as a value, matching how + * `@poppinss/validator-lite` handles every other schema type: `FOO=` means + * unset, so a consumer's default applies instead of `FOO` being read as `0`. + */ +function makeIntegerValidator( + minimum: number, + expectation: string, +): IntegerValidator { + const required = (): Validator => (key, value) => { + if (!value) { + invalid(key, '', expectation); + } + return castToInteger(key, value, minimum, expectation); + }; + + const optional = (): Validator => (key, value) => + value ? castToInteger(key, value, minimum, expectation) : undefined; + + return Object.assign(required, { optional }); +} + +/** + * `integer()` accepts any integer; the sub-validators constrain the range. + * + * - `integer.positive()` — 1 or greater. Ports, pool sizes, limits. + * - `integer.nonNegative()` — 0 or greater, where zero is meaningful: a + * disabled timeout, no retries. + * + * Each also has an `.optional()` form, so a variable the application supplies a + * default for reads as `Env.schema.integer.positive.optional()`. + */ +export const integer: IntegerValidators = Object.assign( + makeIntegerValidator(Number.NEGATIVE_INFINITY, 'an integer'), + { + positive: makeIntegerValidator(1, 'an integer greater than zero'), + nonNegative: makeIntegerValidator(0, 'an integer of zero or greater'), + }, +); + +type HostListValidator = (() => Validator) & { + optional: () => Validator; +}; + +const HOST_LIST_EXPECTATION = + 'a comma-separated list of hosts, each optionally suffixed with ":port"'; + +/** Reuses Adonis' own host check rather than reimplementing FQDN/IP matching. */ +const validateHost = BaseEnv.schema.string({ format: 'host' }); + +function parseHostList(key: string, value: string): readonly string[] { + const entries = value + .split(',') + .map((entry) => entry.trim()) + .filter((entry) => entry.length > 0); + + if (entries.length === 0) { + invalid(key, value, HOST_LIST_EXPECTATION); + } + + for (const entry of entries) { + // Only treat a single colon as a port separator: a bare IPv6 address + // contains several, and has no port to split off. + const separators = entry.split(':').length - 1; + const [host, port] = + separators === 1 ? entry.split(':') : [entry, undefined]; + + validateHost(key, host); + + if (port !== undefined) { + const casted = Number(port); + if (!Number.isInteger(casted) || casted < 1 || casted > 65535) { + invalid(key, entry, 'a host with a port between 1 and 65535'); + } + } + } + + return entries; +} + +/** + * A comma-separated list of hosts, such as Scylla's contact points. Entries may + * carry an explicit `:port`; those that don't fall back to the driver's own + * port setting. + * + * Returning the parsed list means consumers receive `string[]` rather than + * re-splitting the raw value, and an empty or malformed list is reported + * alongside every other invalid variable instead of throwing separately after + * validation has already finished. + */ +export const hostList: HostListValidator = Object.assign( + (): Validator => (key, value) => { + if (!value) { + invalid(key, '', HOST_LIST_EXPECTATION); + } + return parseHostList(key, value); + }, + { + optional: (): Validator => (key, value) => + value ? parseHostList(key, value) : undefined, + }, +); diff --git a/server/package-lock.json b/server/package-lock.json index 2ec9fb01a..7a8173db7 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -9,6 +9,7 @@ "version": "1.0.0", "license": "Apache-2.0", "dependencies": { + "@adonisjs/env": "^7.1.0", "@apollo/server": "^5.5.0", "@as-integrations/express5": "^1.1.2", "@aws-sdk/client-s3": "^3.1017.0", @@ -23,6 +24,7 @@ "@node-saml/passport-saml": "^5.1.0", "@opentelemetry/api": "^1.8.0", "@opentelemetry/semantic-conventions": "^1.22.0", + "@poppinss/utils": "^7.1.0", "@roostorg/coop-integration-example": "^2.0.0", "@roostorg/coop-types": "^2.4.0", "@sendgrid/mail": "^8.1.6", @@ -121,6 +123,20 @@ "typescript-eslint": "^8.57.2" } }, + "node_modules/@adonisjs/env": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/@adonisjs/env/-/env-7.1.0.tgz", + "integrity": "sha512-DXKCDIDoHzjxx9O7HvFYYBGQENmF5ANTUXBGE65ygOcJTtXyrLBISUkjrIQGVAKUmcLPF9WInkXQfxMnfO2VJw==", + "license": "MIT", + "dependencies": { + "@poppinss/utils": "^7.0.0", + "@poppinss/validator-lite": "^2.1.2", + "split-lines": "^3.0.0" + }, + "engines": { + "node": ">=24.0.0" + } + }, "node_modules/@apollo/cache-control-types": { "version": "1.0.3", "resolved": "https://registry.npmjs.org/@apollo/cache-control-types/-/cache-control-types-1.0.3.tgz", @@ -3539,6 +3555,58 @@ "node": ">=20" } }, + "node_modules/@poppinss/exception": { + "version": "1.2.3", + "resolved": "https://registry.npmjs.org/@poppinss/exception/-/exception-1.2.3.tgz", + "integrity": "sha512-dCED+QRChTVatE9ibtoaxc+WkdzOSjYTKi/+uacHWIsfodVfpsueo3+DKpgU5Px8qXjgmXkSvhXvSCz3fnP9lw==", + "license": "MIT" + }, + "node_modules/@poppinss/object-builder": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/@poppinss/object-builder/-/object-builder-1.1.0.tgz", + "integrity": "sha512-FOrOq52l7u8goR5yncX14+k+Ewi5djnrt1JwXeS/FvnwAPOiveFhiczCDuvXdssAwamtrV2hp5Rw9v+n2T7hQg==", + "license": "MIT", + "engines": { + "node": ">=20.6.0" + } + }, + "node_modules/@poppinss/string": { + "version": "1.7.2", + "resolved": "https://registry.npmjs.org/@poppinss/string/-/string-1.7.2.tgz", + "integrity": "sha512-A182GLDfi36iDCbhDrHB0xzrPM1fO3GHnhCDIdadf8C6eycgct4m7zusbLwEh6GPaj2Pz5BVos7XK16w7tZ7wQ==", + "license": "MIT", + "dependencies": { + "@types/pluralize": "^0.0.33", + "case-anything": "^3.1.2", + "pluralize": "^8.0.0", + "slugify": "^1.6.9" + } + }, + "node_modules/@poppinss/types": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/@poppinss/types/-/types-1.2.1.tgz", + "integrity": "sha512-qUYnzl0m9HJTWsXtr8Xo7CwDx6wcjrvo14bOVbIMIlKJCzKrm3LX55dRTDr1/x4PpSvKVgmxvC6Ly2YiqXKOvQ==", + "license": "MIT" + }, + "node_modules/@poppinss/utils": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/@poppinss/utils/-/utils-7.1.0.tgz", + "integrity": "sha512-/0+NFusRdWFACF75gm/PZSw11HSfPbyg7FT9p0RPsDHSqwJq2ZMCcFA60HwOjSWx2WBaGhdvwf78EI1hEOiXhQ==", + "license": "MIT", + "dependencies": { + "@poppinss/exception": "^1.2.3", + "@poppinss/object-builder": "^1.1.0", + "@poppinss/string": "^1.7.2", + "@poppinss/types": "^1.2.1", + "flattie": "^1.1.1" + } + }, + "node_modules/@poppinss/validator-lite": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/@poppinss/validator-lite/-/validator-lite-2.1.2.tgz", + "integrity": "sha512-UhSG1ouT6r67VbEFHK/8ax3EMZYHioew9PqGmEZjV41G15aPZi6cyhXtBVvF9xqkHMflA5V680k7bQzV0kfD5w==", + "license": "MIT" + }, "node_modules/@protobufjs/aspromise": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/@protobufjs/aspromise/-/aspromise-1.1.2.tgz", @@ -11515,6 +11583,12 @@ "@types/pg": "*" } }, + "node_modules/@types/pluralize": { + "version": "0.0.33", + "resolved": "https://registry.npmjs.org/@types/pluralize/-/pluralize-0.0.33.tgz", + "integrity": "sha512-JOqsl+ZoCpP4e8TDke9W79FDcSgPAR0l6pixx2JHkhnRjvShyYiAYw2LVsnA7K08Y6DeOnaU6ujmENO4os/cYg==", + "license": "MIT" + }, "node_modules/@types/qs": { "version": "6.14.0", "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.14.0.tgz", @@ -13512,6 +13586,18 @@ ], "license": "CC-BY-4.0" }, + "node_modules/case-anything": { + "version": "3.1.2", + "resolved": "https://registry.npmjs.org/case-anything/-/case-anything-3.1.2.tgz", + "integrity": "sha512-wljhAjDDIv/hM2FzgJnYQg90AWmZMNtESCjTeLH680qTzdo0nErlCxOmgzgX4ZsZAtIvqHyD87ES8QyriXB+BQ==", + "license": "MIT", + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/mesqueeb" + } + }, "node_modules/cassandra-driver": { "version": "4.9.0", "resolved": "https://registry.npmjs.org/cassandra-driver/-/cassandra-driver-4.9.0.tgz", @@ -15400,6 +15486,15 @@ "dev": true, "license": "ISC" }, + "node_modules/flattie": { + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/flattie/-/flattie-1.1.1.tgz", + "integrity": "sha512-9UbaD6XdAL97+k/n+N7JwX46K/M6Zc6KcFYskrYL8wbBV/Uyk0CTAMY0VT+qiK5PM7AIc9aTWYtq65U7T+aCNQ==", + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/follow-redirects": { "version": "1.16.0", "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.16.0.tgz", @@ -18875,6 +18970,15 @@ "node": ">=20" } }, + "node_modules/pluralize": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/pluralize/-/pluralize-8.0.0.tgz", + "integrity": "sha512-Nc3IT5yHzflTfbjgqWcCPpo7DaKy4FnpB0l/zCAW0Tc7jxAiuqSxHasntB3D7887LSrA93kDJ9IXovxJYxyLCA==", + "license": "MIT", + "engines": { + "node": ">=4" + } + }, "node_modules/possible-typed-array-names": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/possible-typed-array-names/-/possible-typed-array-names-1.1.0.tgz", @@ -19780,6 +19884,15 @@ "node": ">=8" } }, + "node_modules/slugify": { + "version": "1.6.9", + "resolved": "https://registry.npmjs.org/slugify/-/slugify-1.6.9.tgz", + "integrity": "sha512-vZ7rfeehZui7wQs438JXBckYLkIIdfHOXsaVEUMyS5fHo1483l1bMdo0EDSWYclY0yZKFOipDy4KHuKs6ssvdg==", + "license": "MIT", + "engines": { + "node": ">=8.0.0" + } + }, "node_modules/smol-toml": { "version": "1.8.0", "resolved": "https://registry.npmjs.org/smol-toml/-/smol-toml-1.8.0.tgz", @@ -19851,6 +19964,18 @@ "node": "*" } }, + "node_modules/split-lines": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/split-lines/-/split-lines-3.0.0.tgz", + "integrity": "sha512-d0TpRBL/VfKDXsk8JxPF7zgF5pCUDdBMSlEL36xBgVeaX448t+yGXcJaikUyzkoKOJ0l6KpMfygzJU9naIuivw==", + "license": "MIT", + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/split-on-first": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/split-on-first/-/split-on-first-1.1.0.tgz", diff --git a/server/package.json b/server/package.json index c8aae2025..7b10656e9 100644 --- a/server/package.json +++ b/server/package.json @@ -5,7 +5,10 @@ "type": "module", "scripts": { "build": "tsc", + "postbuild": "mkdir -p build && cp package.json build/package.json", + "prestart": "npm run postbuild", "start": "tsc-watch --onSuccess \"node --trace-warnings --env-file-if-exists=.env ./build/bin/www.js\"", + "prestart:trace": "npm run postbuild", "start:trace": "tsc-watch --onSuccess \"node --trace-warnings --env-file-if-exists=.env --require ../nodejs-instrumentation/build/autoinstrumentation.js ./build/bin/www.js\"", "test": "npm run test:local", "test:local": "NODE_OPTIONS=\"--no-warnings --loader ts-node/esm\" node --env-file-if-exists=.env node_modules/.bin/jest --watch --detectOpenHandles", @@ -26,7 +29,13 @@ }, "author": "Roostorg", "license": "Apache-2.0", + "imports": { + "#config/*": "./config/*.js", + "#lib/env": "./lib/env/index.js", + "#start/*": "./start/*.js" + }, "dependencies": { + "@adonisjs/env": "^7.1.0", "@apollo/server": "^5.5.0", "@as-integrations/express5": "^1.1.2", "@aws-sdk/client-s3": "^3.1017.0", @@ -41,6 +50,7 @@ "@node-saml/passport-saml": "^5.1.0", "@opentelemetry/api": "^1.8.0", "@opentelemetry/semantic-conventions": "^1.22.0", + "@poppinss/utils": "^7.1.0", "@roostorg/coop-integration-example": "^2.0.0", "@roostorg/coop-types": "^2.4.0", "@sendgrid/mail": "^8.1.6", diff --git a/server/start/env.ts b/server/start/env.ts new file mode 100644 index 000000000..316fece6a --- /dev/null +++ b/server/start/env.ts @@ -0,0 +1,143 @@ +import { Env } from '#lib/env'; + +// `optional()` here means the application supplies a default, not that the +// value is unimportant. See the corresponding `config/*.ts` module for what +// that default is. Only variables the app genuinely cannot start without are +// required, so that existing deployments relying on a built-in default keep +// booting. An optional enum still rejects an invalid value when one is set. +const env = await Env.create(new URL('./', import.meta.url), { + NODE_ENV: Env.schema.enum.optional([ + 'development', + 'production', + 'test', + ] as const), + // For a future logger: + // LOG_LEVEL: Env.schema.string(), + PORT: Env.schema.integer.positive.optional(), + // `tld: false` so `http://localhost:3000` is accepted — the URL format + // requires a TLD by default, which rejects every local and container-internal + // hostname. + UI_URL: Env.schema.string({ format: 'url', tld: false }), + + OTEL_SERVICE_NAME: Env.schema.string.optional(), + + // Emails: + NOREPLY_EMAIL: Env.schema.string.optional({ format: 'email' }), + SUPPORT_EMAIL: Env.schema.string.optional({ format: 'email' }), + TEAM_EMAIL: Env.schema.string.optional({ format: 'email' }), + EMAIL_TRANSPORT: Env.schema.enum.optional(['console']), + + // Postgresql configuration: + DATABASE_HOST: Env.schema.string({ format: 'host' }), + DATABASE_READ_ONLY_HOST: Env.schema.string.optional({ format: 'host' }), + DATABASE_PORT: Env.schema.integer.positive.optional(), + DATABASE_NAME: Env.schema.string.optional(), + DATABASE_USER: Env.schema.string.optional(), + DATABASE_PASSWORD: Env.schema.secret(), + DATABASE_SSL: Env.schema.boolean.optional(), + DATABASE_POOL_MAX: Env.schema.integer.positive.optional(), + DATABASE_READ_POOL_MAX: Env.schema.integer.positive.optional(), + // Zero disables these, so they allow it. + DATABASE_POOL_MAX_LIFETIME_SECONDS: Env.schema.integer.nonNegative.optional(), + DATABASE_POOL_IDLE_TIMEOUT_MS: Env.schema.integer.nonNegative.optional(), + DATABASE_POOL_CONNECTION_TIMEOUT_MS: + Env.schema.integer.nonNegative.optional(), + DATABASE_QUERY_TIMEOUT_MS: Env.schema.integer.nonNegative.optional(), + DATABASE_IDLE_IN_TRANSACTION_TIMEOUT_MS: + Env.schema.integer.nonNegative.optional(), + DATABASE_STATEMENT_TIMEOUT_MS: Env.schema.integer.nonNegative.optional(), + DATABASE_KEEPALIVE: Env.schema.boolean.optional(), + DATABASE_KEEPALIVE_INITIAL_DELAY_MS: + Env.schema.integer.nonNegative.optional(), + DATABASE_PRINT_LOGS: Env.schema.boolean.optional(), + + // Redis: + REDIS_USE_CLUSTER: Env.schema.boolean(), + REDIS_HOST: Env.schema.string({ format: 'host' }), + REDIS_PORT: Env.schema.integer.positive.optional(), + REDIS_USER: Env.schema.string.optional(), + REDIS_PASSWORD: Env.schema.secret.optional(), + + // Secrets: + SESSION_SECRET: Env.schema.string(), + GRAPHQL_MAX_DEPTH: Env.schema.integer.positive.optional(), + + // Default to `clickhouse`; ANALYTICS_ADAPTER falls back to WAREHOUSE_ADAPTER. + WAREHOUSE_ADAPTER: Env.schema.enum.optional([ + 'noop', + 'clickhouse', + 'postgresql', + ]), + ANALYTICS_ADAPTER: Env.schema.enum.optional([ + 'noop', + 'clickhouse', + 'postgresql', + ]), + // Legacy: use WAREHOUSE_ADAPTER and ANALYTICS_ADAPTER instead: + DATA_WAREHOUSE_PROVIDER: Env.schema.enum.optional([ + 'noop', + 'clickhouse', + 'postgresql', + ]), + // Clickhouse settings, only used when WAREHOUSE_ADAPTER or ANALYTICS_ADAPTER is clickhouse: + CLICKHOUSE_HOST: Env.schema.string.optional({ format: 'host' }), + CLICKHOUSE_PORT: Env.schema.integer.positive.optional(), + CLICKHOUSE_USERNAME: Env.schema.string.optional(), + CLICKHOUSE_PASSWORD: Env.schema.secret.optional(), + CLICKHOUSE_DATABASE: Env.schema.string.optional(), + CLICKHOUSE_PROTOCOL: Env.schema.enum.optional(['https', 'http']), + CLICKHOUSE_POOL_SIZE: Env.schema.integer.positive.optional(), + // Zero means no retries and no delay respectively, so these allow it. + CLICKHOUSE_INSERT_MAX_RETRIES: Env.schema.integer.nonNegative.optional(), + CLICKHOUSE_INSERT_RETRY_INITIAL_MS: Env.schema.integer.nonNegative.optional(), + CLICKHOUSE_INSERT_RETRY_MAX_MS: Env.schema.integer.nonNegative.optional(), + // Clickhouse Memory settings. Zero disables the external-memory thresholds. + CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_GROUP_BY: + Env.schema.integer.nonNegative.optional(), + CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_SORT: + Env.schema.integer.nonNegative.optional(), + CLICKHOUSE_MAX_THREADS: Env.schema.integer.positive.optional(), + CLICKHOUSE_MAX_BLOCK_SIZE: Env.schema.integer.positive.optional(), + // Other Clickhouse: + CLICKHOUSE_RULE_INSIGHTS_LOOKBACK_DAYS: + Env.schema.integer.positive.optional(), + + // Scylla: + // Contact points, e.g. "db1,db2:9043". Parsed into a list here so consumers + // receive `string[]` rather than re-splitting the raw value. + SCYLLA_HOSTS: Env.schema.hostList.optional(), + SCYLLA_USERNAME: Env.schema.string.optional(), + SCYLLA_PASSWORD: Env.schema.secret.optional(), + SCYLLA_LOCAL_DATACENTER: Env.schema.string.optional(), + + // Selects the NCMEC CyberTipline endpoint that "Submit to NCMEC" decisions are + // routed to. Anything other than the literal string `production` (including + // being unset) sends submissions to https://exttest.cybertip.org (the NCMEC + // sandbox, reports are discarded). Set to `production` only when the + // CyberTipline credentials configured in Settings → NCMEC are production + // credentials issued by NCMEC and your integration has been approved for live + // reporting. + NCMEC_ENV: Env.schema.enum.optional(['production', 'test']), + NCMEC_DEBUG: Env.schema.boolean.optional(), + + // Debugging: + EXPOSE_SENSITIVE_IMPLEMENTATION_DETAILS_IN_ERRORS: + Env.schema.boolean.optional(), + ALLOW_USER_INPUT_LOCALHOST_URIS: Env.schema.boolean.optional(), + LOG_REQUEST_BODY: Env.schema.boolean.optional(), + + // Integrations: + GROQ_SECRET_KEY: Env.schema.secret.optional(), + SENDGRID_API_KEY: Env.schema.secret.optional(), + GOOGLE_PLACES_API_KEY: Env.schema.secret.optional(), + OPEN_AI_API_KEY: Env.schema.secret.optional(), + SLACK_APP_BEARER_TOKEN: Env.schema.secret.optional(), + // Defaults to http://localhost:9876/ in `hmaService`. + HMA_SERVICE_URL: Env.schema.string.optional({ format: 'url', tld: false }), + + // Others: + ITEM_INVESTIGATION_AND_STRIKES_ENABLED: Env.schema.boolean.optional(), + ITEM_QUEUE_TRAFFIC_PERCENTAGE: Env.schema.number(), +}); + +export default env; From 8d98cc5875f45512022c8c1d8df4cb3b25ecd5f8 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 16 Sep 2026 23:43:22 +0200 Subject: [PATCH 02/19] Read configuration through config modules Introduces config/app.ts and config/security.ts, reading the validated environment rather than process.env, and turns ConfigService from a one-field value object into a class that owns the URLs the application hands out. Every outbound URL was previously interpolated at its call site from a bare uiUrl: the SAML callback and issuer, the post-login redirect, the password-reset link in two places, and the invite link. Each of those would independently produce a doubled slash if UI_URL carried a trailing one, and a mismatched SAML ACS URL is a login outage for a whole org. Normalising happens once in the constructor instead. uiUrl stays public for notificationsService, which passes the origin into formatNotification rather than asking for a specific path. ConfigService is registered with bottle.factory rather than bottle.value, so it is built on first use. It is still a singleton per container, as factories are memoised. The private #uiUrl field makes the class nominally typed, which caught a { uiUrl: string } stand-in in userManagementService.test.ts that had drifted from the real thing. api.ts now takes its helmet options, session secret and cookie settings from config. The helmet ternary is deliberately unchanged in meaning: production keeps helmet's own defaults, and only development relaxes the Content-Security-Policy for Vite's dev server, which needs inline and eval'd scripts plus a websocket. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + server/api.ts | 46 ++++---------- server/config/app.ts | 22 +++++++ server/config/security.ts | 24 +++++++ server/graphql/datasources/OrgApi.ts | 5 +- server/iocContainer/index.ts | 7 ++- .../services/configService/configService.ts | 63 +++++++++++++++++++ server/services/configService/index.ts | 1 + .../userManagementService.test.ts | 5 +- .../userManagementService.ts | 8 +-- 10 files changed, 136 insertions(+), 46 deletions(-) create mode 100644 server/config/app.ts create mode 100644 server/config/security.ts create mode 100644 server/services/configService/configService.ts create mode 100644 server/services/configService/index.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 66ad71bf2..55436068e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ For more information about each release including git tags and artifacts, see [R ### Fixed +- Doubled slashes in generated links when `UI_URL` ends with a slash ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Rule history dropping other rules' versions when filtered by start date ([#1056](https://github.com/roostorg/coop/pull/1056) by [@juanmrad](https://github.com/juanmrad)) - `RetryFailedNcmecDecisionsJob` ignoring `NCMEC_ENV` and retrying test decisions ([#928](https://github.com/roostorg/coop/pull/928) by [@taobojlen](https://github.com/taobojlen)) - Queue creation failing with "name already exists" on the default reviewer selection ([#1069](https://github.com/roostorg/coop/pull/1069) by [@jess-upscrolled](https://github.com/jess-upscrolled), closes [#1074](https://github.com/roostorg/coop/issues/1074)) diff --git a/server/api.ts b/server/api.ts index a832ab875..ad9dc8d7b 100644 --- a/server/api.ts +++ b/server/api.ts @@ -17,6 +17,8 @@ import { ATTR_EXCEPTION_STACKTRACE, ATTR_EXCEPTION_TYPE, } from '@opentelemetry/semantic-conventions'; +import appConfig from '#config/app'; +import securityConfig from '#config/security'; import connectPgSimple from 'connect-pg-simple'; import cors from 'cors'; import express, { type ErrorRequestHandler, type Request } from 'express'; @@ -86,8 +88,6 @@ async function getCPUUsage() { return 1 - (endIdle - startIdle) / (endTotal - startTotal); } -// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -const env = process.env.NODE_ENV || 'development'; const sessionStore = connectPgSimple(session); export default async function makeApiServer(deps: Dependencies) { @@ -97,25 +97,7 @@ export default async function makeApiServer(deps: Dependencies) { app.use(cors()); - app.use( - helmet( - env === 'production' - ? {} - : { - contentSecurityPolicy: { - directives: { - defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'unsafe-inline'", "'unsafe-eval'"], - styleSrc: ["'self'", "'unsafe-inline'"], - imgSrc: ["'self'", 'data:', 'blob:', 'https:', 'http:'], - connectSrc: ["'self'", 'ws:', 'wss:', 'https:', 'http:'], - fontSrc: ["'self'", 'data:', 'https:'], - frameSrc: ["'self'"], - }, - }, - }, - ), - ); + app.use(helmet(securityConfig.helmet)); app.use(express.json({ limit: '50mb' })); app.get('/ready', async (_req, res) => { @@ -132,14 +114,13 @@ export default async function makeApiServer(deps: Dependencies) { const sessionStoreInstance = new sessionStore({ pool: KyselyPgPool }); app.use( session({ - secret: process.env.SESSION_SECRET!, + secret: appConfig.session.secret, store: sessionStoreInstance, cookie: { - secure: process.env.NODE_ENV === 'production', - httpOnly: true, + secure: appConfig.session.cookie.secure, + httpOnly: appConfig.session.cookie.httpOnly, sameSite: 'lax', - // 30 Days in milliseconds - maxAge: 30 * 24 * 60 * 60 * 1000, + maxAge: appConfig.session.cookie.maxAge, }, resave: false, saveUninitialized: false, @@ -194,11 +175,8 @@ export default async function makeApiServer(deps: Dependencies) { done(null, { entryPoint: samlSettings.sso_url as string, idpCert: samlSettings.cert as string, - // I could use UI_URL here but technically the API could be hosted - // on a different domain in the future so hopefully this is more - // robust, not that it will likely matter. - callbackUrl: `${deps.ConfigService.uiUrl}/api/v1/saml/login/${orgId}/callback`, - issuer: deps.ConfigService.uiUrl, + callbackUrl: deps.ConfigService.samlCallbackUrl(orgId), + issuer: deps.ConfigService.samlIssuer, }); }, }, @@ -220,7 +198,7 @@ export default async function makeApiServer(deps: Dependencies) { failureFlash: true, }), (_req, res) => { - res.redirect(`${deps.ConfigService.uiUrl}/dashboard`); + res.redirect(deps.ConfigService.dashboardUrl); }, ); @@ -267,12 +245,12 @@ export default async function makeApiServer(deps: Dependencies) { }, }), plugins: [ - ...(process.env.NODE_ENV === 'production' + ...(appConfig.env === 'production' ? [ApolloServerPluginLandingPageDisabled()] : []), ], validationRules: [safeDepthLimit(safeGetEnvInt('GRAPHQL_MAX_DEPTH', 10))], - introspection: process.env.NODE_ENV !== 'production', + introspection: appConfig.env !== 'production', formatError(formattedError, error) { // unwrapResolverError removes the GraphQLError wrapper added by graphql-js // when a non-GraphQL error is thrown from a resolver. diff --git a/server/config/app.ts b/server/config/app.ts new file mode 100644 index 000000000..cec19bab6 --- /dev/null +++ b/server/config/app.ts @@ -0,0 +1,22 @@ +import env from '#start/env'; + +const NODE_ENV = env.get('NODE_ENV', 'development'); + +export default { + env: NODE_ENV, + + // Public origin of the frontend. Used to build the links and redirects the + // application hands out, so it must be the origin a browser reaches, not an + // internal one. + uiUrl: env.get('UI_URL'), + + session: { + secret: env.get('SESSION_SECRET'), + cookie: { + secure: NODE_ENV === 'production', + httpOnly: true, + // 30 Days in milliseconds + maxAge: 30 * 24 * 60 * 60 * 1000, + }, + }, +}; diff --git a/server/config/security.ts b/server/config/security.ts new file mode 100644 index 000000000..615926447 --- /dev/null +++ b/server/config/security.ts @@ -0,0 +1,24 @@ +import app from '#config/app'; + +export default { + // Production keeps helmet's own defaults, which include a strict + // Content-Security-Policy. Development relaxes it for Vite's dev server, + // which needs inline and eval'd scripts plus a websocket for HMR — so these + // directives must never be the production branch. + helmet: + app.env === 'production' + ? {} + : { + contentSecurityPolicy: { + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'", "'unsafe-inline'", "'unsafe-eval'"], + styleSrc: ["'self'", "'unsafe-inline'"], + imgSrc: ["'self'", 'data:', 'blob:', 'https:', 'http:'], + connectSrc: ["'self'", 'ws:', 'wss:', 'https:', 'http:'], + fontSrc: ["'self'", 'data:', 'https:'], + frameSrc: ["'self'"], + }, + }, + }, +}; diff --git a/server/graphql/datasources/OrgApi.ts b/server/graphql/datasources/OrgApi.ts index 255318a7e..3988978fe 100644 --- a/server/graphql/datasources/OrgApi.ts +++ b/server/graphql/datasources/OrgApi.ts @@ -1,5 +1,4 @@ import crypto from 'node:crypto'; -import { URL } from 'node:url'; import { inject, type Dependencies } from '../../iocContainer/index.js'; import { CoopEmailAddress } from '../../services/sendEmailService/index.js'; @@ -56,14 +55,14 @@ class OrgAPI { orgId, }); - const url = new URL(`${this.config.uiUrl}/signup/${token}`); + const url = this.config.signupUrl(token); const msg = { to: email, from: CoopEmailAddress.NoReply, subject: "You've been invited to join your team on Coop!", html: `Hi, and welcome to Coop! Your admin has invited you to join the ${org.name} Coop team.

- Click on this link to get started! The link expires in 24 hours, so please make sure to sign up soon. + Click on this link to get started! The link expires in 24 hours, so please make sure to sign up soon.

Best,
Coop Support Team`, diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index 4ce8a390a..6a5f4e0a2 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -101,6 +101,7 @@ import { type ApiKeyService, } from '../services/apiKeyService/index.js'; import { type CombinedPg } from '../services/combinedDbTypes.js'; +import { ConfigService } from '../services/configService/index.js'; import { makeDerivedFieldsService, type DerivedFieldsService, @@ -441,7 +442,7 @@ export interface Dependencies { Tracer: SafeTracer; Meter: CoopMeter; KeyValueStore: StringNumberKeyValueStore; - ConfigService: { uiUrl: string }; + ConfigService: ConfigService; } // Takes a class and returns a type that just contains its public methods and @@ -1666,7 +1667,9 @@ export default async function getBottle( 'SigningKeyPairStorageService', (container) => new PostgresSigningKeyPairStorage(container.KyselyPg), ); - bottle.value('ConfigService', { uiUrl: safeGetEnvVar('UI_URL') }); + // A factory rather than a value so the instance is built on first use. It is + // still a singleton per container, as `bottle.factory` memoises. + bottle.factory('ConfigService', () => new ConfigService()); bottle.value('S3StoreObjectFactory', s3StoreObjectFactory); bottle.factory('sendEmail', makeSendEmail); register(bottle, 'KeyValueStore', makeKeyValueStore); diff --git a/server/services/configService/configService.ts b/server/services/configService/configService.ts new file mode 100644 index 000000000..b8ef32054 --- /dev/null +++ b/server/services/configService/configService.ts @@ -0,0 +1,63 @@ +import appConfig from '#config/app'; + +/** + * Owns the externally-facing URLs the application hands out — SAML callbacks, + * post-login redirects, password resets — so their paths are defined once + * rather than interpolated at each call site. + * + * The origin defaults to `config/app`; the constructor parameter exists so a + * test can pin it without going through the environment. + */ +export default class ConfigService { + readonly #uiUrl: string; + + constructor(uiUrl: string = appConfig.uiUrl) { + // `UI_URL` may or may not carry a trailing slash; normalising once here + // avoids every consumer producing `//dashboard` when it does. + this.#uiUrl = uiUrl.replace(/\/+$/, ''); + } + + /** Public origin of the frontend, without a trailing slash. */ + get uiUrl(): string { + return this.#uiUrl; + } + + /** + * SAML issuer. Deliberately the UI origin rather than the API's own, since + * that is the identifier registered with each org's identity provider. + */ + get samlIssuer(): string { + return this.#uiUrl; + } + + /** Where a user lands after a successful login. */ + get dashboardUrl(): string { + return `${this.#uiUrl}/dashboard`; + } + + /** + * Where an org's identity provider posts its SAML assertion back to. Note + * this is on the UI origin: the API could be hosted on a different domain, + * and the frontend proxies the callback through to it. + */ + samlCallbackUrl(orgId: string): string { + return `${this.#uiUrl}/api/v1/saml/login/${orgId}/callback`; + } + + /** + * Link emailed to a user so they can set a new password. The token is + * hex-encoded (`randomBytes(32).toString('hex')`) and therefore already + * path-safe; a token format using other characters would need encoding here. + */ + resetPasswordUrl(token: string): string { + return `${this.#uiUrl}/reset_password/${token}`; + } + + /** + * Link emailed to invite someone into an org. Like the reset token, the + * invite token is hex-encoded and therefore already path-safe. + */ + signupUrl(token: string): string { + return `${this.#uiUrl}/signup/${token}`; + } +} diff --git a/server/services/configService/index.ts b/server/services/configService/index.ts new file mode 100644 index 000000000..cc870e866 --- /dev/null +++ b/server/services/configService/index.ts @@ -0,0 +1 @@ +export { default as ConfigService } from './configService.js'; diff --git a/server/services/userManagementService/userManagementService.test.ts b/server/services/userManagementService/userManagementService.test.ts index b51c1ea09..8739fa926 100644 --- a/server/services/userManagementService/userManagementService.test.ts +++ b/server/services/userManagementService/userManagementService.test.ts @@ -1,6 +1,7 @@ import { type Kysely } from 'kysely'; import { makeTestWithFixture } from '../../test/utils.js'; +import { ConfigService } from '../configService/index.js'; import { MIN_PASSWORD_LENGTH } from './constants.js'; import type { UserManagementPg } from './index.js'; import UserManagementService from './userManagementService.js'; @@ -16,9 +17,7 @@ const mockDb = { const mockSendEmail = jest.fn(); -const mockConfigService = { - uiUrl: 'http://localhost:3000', -}; +const mockConfigService = new ConfigService('http://localhost:3000'); describe('UserManagementService', () => { const testWithFixtures = makeTestWithFixture(() => { diff --git a/server/services/userManagementService/userManagementService.ts b/server/services/userManagementService/userManagementService.ts index bf86bdbd0..c4618b97d 100644 --- a/server/services/userManagementService/userManagementService.ts +++ b/server/services/userManagementService/userManagementService.ts @@ -373,12 +373,12 @@ class UserManagementService { const { userId, orgId } = existingUser; const token = await this.#createPasswordResetToken({ userId, orgId }); - const url = new URL(`${this.configService.uiUrl}/reset_password/` + token); + const url = this.configService.resetPasswordUrl(token); const msg = { to: email, from: CoopEmailAddress.NoReply, subject: '[Coop] Reset your password', - html: `You recently indicated that you forgot your Coop password. Click on this link to create a new password. The link expires in 1 hour, so please make sure to sign up soon. + html: `You recently indicated that you forgot your Coop password. Click on this link to create a new password. The link expires in 1 hour, so please make sure to sign up soon.

Best,
Coop Support Team`, @@ -424,12 +424,12 @@ class UserManagementService { }); // Send email using the standard flow (will be no-op if SendGrid not configured) - const url = new URL(`${this.configService.uiUrl}/reset_password/` + token); + const url = this.configService.resetPasswordUrl(token); const msg = { to: email, from: CoopEmailAddress.NoReply, subject: '[Coop] Reset your password', - html: `Your organization administrator has initiated a password reset for your account. Click on this link to create a new password. The link expires in 1 hour, so please make sure to reset your password soon. + html: `Your organization administrator has initiated a password reset for your account. Click on this link to create a new password. The link expires in 1 hour, so please make sure to reset your password soon.

Best,
Coop Support Team`, From 874630d22d0b0078925d2e13bb4c6d60376bff6e Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Wed, 16 Sep 2026 23:46:47 +0200 Subject: [PATCH 03/19] Expose environment as booleans on the app config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds inProduction, inDev and inTest to config/app.ts, derived once from NODE_ENV, and uses them in config/security.ts and api.ts. The comparison previously appeared at four call sites. Spelt as a string it fails open: `NODE_ENV === 'prod'` is silently non-production wherever it is written, and the mistake reads as correct. Deriving it once leaves one place to get it wrong. It also makes negation legible — `!appConfig.inProduction` rather than `appConfig.env !== 'production'` — which matters here because every NODE_ENV comparison in the server is a negative one. `env` is retained on the config even though nothing reads it now. This is the ergonomics of Adonis' `app.inProduction` without the dependency: @adonisjs/application peer-depends on @adonisjs/fold, so adopting it would mean a second IoC container alongside BottleJS. Co-Authored-By: Claude Opus 5 (1M context) --- server/api.ts | 4 ++-- server/config/app.ts | 11 ++++++++++- server/config/security.ts | 27 +++++++++++++-------------- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/server/api.ts b/server/api.ts index ad9dc8d7b..245fef6e8 100644 --- a/server/api.ts +++ b/server/api.ts @@ -245,12 +245,12 @@ export default async function makeApiServer(deps: Dependencies) { }, }), plugins: [ - ...(appConfig.env === 'production' + ...(appConfig.inProduction ? [ApolloServerPluginLandingPageDisabled()] : []), ], validationRules: [safeDepthLimit(safeGetEnvInt('GRAPHQL_MAX_DEPTH', 10))], - introspection: appConfig.env !== 'production', + introspection: !appConfig.inProduction, formatError(formattedError, error) { // unwrapResolverError removes the GraphQLError wrapper added by graphql-js // when a non-GraphQL error is thrown from a resolver. diff --git a/server/config/app.ts b/server/config/app.ts index cec19bab6..609cb2e3b 100644 --- a/server/config/app.ts +++ b/server/config/app.ts @@ -2,8 +2,17 @@ import env from '#start/env'; const NODE_ENV = env.get('NODE_ENV', 'development'); +// Derived once so the comparison lives in a single place. `NODE_ENV === 'prod'` +// is silently non-production everywhere it appears; `inProduction` is not. +const inProduction = NODE_ENV === 'production'; +const inDev = NODE_ENV === 'development'; +const inTest = NODE_ENV === 'test'; + export default { env: NODE_ENV, + inProduction, + inDev, + inTest, // Public origin of the frontend. Used to build the links and redirects the // application hands out, so it must be the origin a browser reaches, not an @@ -13,7 +22,7 @@ export default { session: { secret: env.get('SESSION_SECRET'), cookie: { - secure: NODE_ENV === 'production', + secure: inProduction, httpOnly: true, // 30 Days in milliseconds maxAge: 30 * 24 * 60 * 60 * 1000, diff --git a/server/config/security.ts b/server/config/security.ts index 615926447..ec4aec1c8 100644 --- a/server/config/security.ts +++ b/server/config/security.ts @@ -5,20 +5,19 @@ export default { // Content-Security-Policy. Development relaxes it for Vite's dev server, // which needs inline and eval'd scripts plus a websocket for HMR — so these // directives must never be the production branch. - helmet: - app.env === 'production' - ? {} - : { - contentSecurityPolicy: { - directives: { - defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'unsafe-inline'", "'unsafe-eval'"], - styleSrc: ["'self'", "'unsafe-inline'"], - imgSrc: ["'self'", 'data:', 'blob:', 'https:', 'http:'], - connectSrc: ["'self'", 'ws:', 'wss:', 'https:', 'http:'], - fontSrc: ["'self'", 'data:', 'https:'], - frameSrc: ["'self'"], - }, + helmet: app.inProduction + ? {} + : { + contentSecurityPolicy: { + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'", "'unsafe-inline'", "'unsafe-eval'"], + styleSrc: ["'self'", "'unsafe-inline'"], + imgSrc: ["'self'", 'data:', 'blob:', 'https:', 'http:'], + connectSrc: ["'self'", 'ws:', 'wss:', 'https:', 'http:'], + fontSrc: ["'self'", 'data:', 'https:'], + frameSrc: ["'self'"], }, }, + }, }; From 835c2d36aefafaae5974689421ca78a2fe73b3f1 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 00:20:34 +0200 Subject: [PATCH 04/19] Move Postgres configuration into config/database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Takes the connection parameters, pool tuning and log levels out of the IoC container, which had no business assembling them. iocContainer loses 145 lines and every DATABASE_* read; it now asks for `databaseConfig.connections.primary`, `.readReplica` and `.logLevels`. Defaults move next to the variables they belong to, so what a variable takes when unset is visible where it is declared rather than at the call site. They reproduce the previous values exactly, with two exceptions below. `statementTimeoutMs` and `keepAliveInitialDelayMs` stay `number | undefined` deliberately: the pg options they map to must be absent rather than zero when unconfigured, since zero means "no limit" for one and "no delay" for the other. DATABASE_READ_ONLY_HOST is no longer required, and falls back to DATABASE_HOST. A single-database deployment previously had to repeat the primary host to satisfy safeGetEnvVar, which is a needless chance to get subtly wrong. The replica keeps its own pool size either way, since read and write traffic are sized differently. Kysely logs nothing under NODE_ENV=test. It logs a query error even when the caller catches and handles it, so suites that deliberately exercise failure paths — a unique violation surfacing as a friendly "name already exists", say — fill the output with errors that are not failures and are easily mistaken for them. DATABASE_PRINT_LOGS still overrides this. Also deletes getEnvVarOrWarn, whose only caller was the Postgres application_name, now config/app's serviceName. That removes one of the dynamic `process.env[varName]` lookups. The pg `ssl` option keeps `rejectUnauthorized: false` verbatim. That is its own problem and is being tracked separately; moving it here at least reduces it to a single site. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + server/config/app.ts | 3 + server/config/database.ts | 154 ++++++++++++++++++ server/iocContainer/index.ts | 144 +--------------- server/start/env.ts | 2 + .../harness/transactionalPgPool.integ.test.ts | 4 +- server/test/setupMockedServer.ts | 8 +- 7 files changed, 171 insertions(+), 145 deletions(-) create mode 100644 server/config/database.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 55436068e..8e813351c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ For more information about each release including git tags and artifacts, see [R - Invalid environment variable values now prevent startup instead of falling back to defaults ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Boolean environment variables accept only `1`, `0`, `true` and `false` ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) +- `DATABASE_READ_ONLY_HOST` is now optional, falling back to `DATABASE_HOST` ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Scylla is now optional via `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` ([#918](https://github.com/roostorg/coop/pull/918) by [@sunilatlas](https://github.com/sunilatlas)) - Settings "Other" tab renamed to "Partial Items" and its settings relocated ([#965](https://github.com/roostorg/coop/pull/965) by [@golden-fox07](https://github.com/golden-fox07)) - Queue deletion is refused while routing rules still reference the queue ([#808](https://github.com/roostorg/coop/pull/808) by [@reitblatt](https://github.com/reitblatt)) diff --git a/server/config/app.ts b/server/config/app.ts index 609cb2e3b..a88e2fede 100644 --- a/server/config/app.ts +++ b/server/config/app.ts @@ -19,6 +19,9 @@ export default { // internal one. uiUrl: env.get('UI_URL'), + /** Identifies this process in traces and as the Postgres `application_name`. */ + serviceName: env.get('OTEL_SERVICE_NAME', 'coop-service'), + session: { secret: env.get('SESSION_SECRET'), cookie: { diff --git a/server/config/database.ts b/server/config/database.ts new file mode 100644 index 000000000..619b42ce5 --- /dev/null +++ b/server/config/database.ts @@ -0,0 +1,154 @@ +import appConfig from '#config/app'; +import env from '#start/env'; +import { LOG_LEVELS, type LogLevel } from 'kysely'; +import type { ClientConfig, PoolConfig } from 'pg'; + +/** + * Postgres connection and pool settings. + * + * Defaults live here rather than at the call site, so the value a variable + * takes when unset is visible next to the variable itself. + * + * `statementTimeoutMs` and `keepAliveInitialDelayMs` are deliberately left as + * `number | undefined`: the pg options they map to must be *absent* rather than + * zero when unconfigured, so Postgres and pg apply their own defaults. + */ +const config = { + host: env.get('DATABASE_HOST'), + readOnlyHost: env.get('DATABASE_READ_ONLY_HOST'), + port: env.get('DATABASE_PORT', 5432), + name: env.get('DATABASE_NAME', 'development'), + user: env.get('DATABASE_USER', 'postgres'), + password: env.get('DATABASE_PASSWORD'), + ssl: env.get('DATABASE_SSL', false), + + /** Logs every executed query, with SQL, bound params and duration. */ + printLogs: env.get('DATABASE_PRINT_LOGS', false), + + pool: { + max: env.get('DATABASE_POOL_MAX', 30), + readMax: env.get('DATABASE_READ_POOL_MAX', 150), + + /** pg's own default is 10s, which churns connections during quiet periods. */ + idleTimeoutMs: env.get('DATABASE_POOL_IDLE_TIMEOUT_MS', 300_000), + + /** pg's own default is 0 (wait forever). Fail fast if the db is unreachable. */ + connectionTimeoutMs: env.get('DATABASE_POOL_CONNECTION_TIMEOUT_MS', 15_000), + + /** Client-side bound on long-running queries. */ + queryTimeoutMs: env.get('DATABASE_QUERY_TIMEOUT_MS', 1_000_000), + + /** + * Server-side bound, defence in depth alongside `queryTimeoutMs`. Unset + * leaves Postgres' own default, which is no limit. + */ + statementTimeoutMs: env.get('DATABASE_STATEMENT_TIMEOUT_MS'), + + /** Kills sessions sitting idle inside an open transaction, holding locks. */ + idleInTransactionTimeoutMs: env.get( + 'DATABASE_IDLE_IN_TRANSACTION_TIMEOUT_MS', + 300_000, + ), + + /** Recycles each client after N seconds to dodge stale connections. */ + maxLifetimeSeconds: env.get('DATABASE_POOL_MAX_LIFETIME_SECONDS', 0), + + /** + * TCP keepalive surfaces NAT and load-balancer connection drops as pool + * errors rather than hung queries. On unless explicitly disabled. + */ + keepAlive: env.get('DATABASE_KEEPALIVE', true), + + /** Unset leaves pg's own initial delay. */ + keepAliveInitialDelayMs: env.get('DATABASE_KEEPALIVE_INITIAL_DELAY_MS'), + }, +}; + +/** + * Where to connect and as whom, with no pool tuning: what a single `pg.Client` + * should be built from. + * + * `connections.primary` is not a substitute. `PoolConfig extends ClientConfig`, + * so it type-checks, but pg forwards `statement_timeout`, `query_timeout` and + * `idle_in_transaction_session_timeout` to a plain client as well — and the + * last of those would terminate a connection that is deliberately held open + * inside a transaction, which is exactly what the test harness does. + */ +const connectionParams: ClientConfig = { + user: config.user, + database: config.name, + password: config.password.release(), + port: config.port, + host: config.host, + ssl: config.ssl ? { rejectUnauthorized: false } : undefined, +}; + +/** + * Pool tuning shared by both pools, in pg's own option names. + * + * `statement_timeout` and `keepAliveInitialDelayMillis` are omitted rather than + * zeroed when unconfigured, so Postgres and pg apply their own defaults. + */ +const poolTuning: PoolConfig = { + idleTimeoutMillis: config.pool.idleTimeoutMs, + connectionTimeoutMillis: config.pool.connectionTimeoutMs, + query_timeout: config.pool.queryTimeoutMs, + ...(config.pool.statementTimeoutMs !== undefined && { + statement_timeout: config.pool.statementTimeoutMs, + }), + idle_in_transaction_session_timeout: config.pool.idleInTransactionTimeoutMs, + maxLifetimeSeconds: config.pool.maxLifetimeSeconds, + // Connection drops surface as pool errors, handled by `createPgPool`. + keepAlive: config.pool.keepAlive, + ...(config.pool.keepAliveInitialDelayMs !== undefined && { + keepAliveInitialDelayMillis: config.pool.keepAliveInitialDelayMs, + }), +}; + +/** Everything needed to build the primary pool. */ +const primary: PoolConfig = { + ...connectionParams, + max: config.pool.max, + application_name: appConfig.serviceName, + ...poolTuning, +}; + +/** + * The primary pool's settings, redirected at the read replica. + * + * Falls back to the primary host when no replica is configured, so a + * single-database deployment works without repeating the host. It keeps its + * own pool size either way, since read and write traffic are sized + * differently. + */ +const readReplica: PoolConfig = { + ...primary, + max: config.pool.readMax, + host: config.readOnlyHost ?? config.host, +}; + +/** + * Kysely's own default is `['error']`; `DATABASE_PRINT_LOGS` opts in to + * `LOG_LEVELS`, every level Kysely defines, which also logs each executed + * query with its SQL, bound params and duration. + * + * Tests log nothing by default. Kysely logs a query error even when the caller + * catches and handles it, so a suite that deliberately exercises a failure + * path - a unique violation surfacing as a friendly "name already exists", for + * instance - fills the output with errors that are not failures, and are easily + * mistaken for them. `DATABASE_PRINT_LOGS` still overrides this when debugging. + */ +function resolveLogLevels(): ReadonlyArray { + if (config.printLogs) { + return LOG_LEVELS; + } + + return appConfig.inTest ? [] : ['error']; +} + +export default { + ...config, + connectionParams, + connections: { primary, readReplica }, + logLevels: resolveLogLevels(), +}; diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index 6a5f4e0a2..28b741a1f 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -3,6 +3,7 @@ import { createRequire } from 'module'; import Bottle from '@ethanresnick/bottlejs'; import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; +import databaseConfig from '#config/database'; import { types as scyllaTypes, type Host as ScyllaHost, @@ -252,12 +253,7 @@ import { import { createPgPool } from './createPgPool.js'; import { registerGqlDataSources } from './services/gqlDataSources.js'; import { registerWorkersAndJobs } from './services/workersAndJobs.js'; -import { - isEnvTrue, - register, - safeGetEnvNonNegativeInt, - safeGetEnvVar, -} from './utils.js'; +import { isEnvTrue, register, safeGetEnvVar } from './utils.js'; // the otel instrumentation currently intercepts require statements. support for // esm support is experimental so we should wait until it is stable @@ -451,17 +447,6 @@ export interface Dependencies { // treatment that TS gives to classes with private fields; see https://stackoverflow.com/questions/55281162/can-i-force-the-typescript-compiler-to-use-nominal-typing) export type PublicInterface = { [K in keyof T]: T[K] }; -export function getPgConnectionParams(): pg.ClientConfig { - return { - user: process.env.DATABASE_USER ?? 'postgres', - database: process.env.DATABASE_NAME ?? 'development', - password: safeGetEnvVar('DATABASE_PASSWORD'), - port: parseInt(process.env.DATABASE_PORT ?? '5432'), - host: safeGetEnvVar('DATABASE_HOST'), - ssl: isEnvTrue('DATABASE_SSL') ? { rejectUnauthorized: false } : undefined, - }; -} - /** * A function for creating our service container, configured for production. * Services can be rebound in other contexts (namely, tests) as needed. @@ -473,87 +458,6 @@ export default async function getBottle( manualReviewContentResolver?: ManualReviewContentResolver; } = {}, ) { - // Pool / client tuning shared by both Kysely pools. Defaults preserve our - // pre-Kysely behavior; env var names are generic. - const getPgPoolTuning = () => { - const statementTimeoutMs = - process.env.DATABASE_STATEMENT_TIMEOUT_MS?.trim(); - const keepAliveInitialDelayMs = - process.env.DATABASE_KEEPALIVE_INITIAL_DELAY_MS?.trim(); - return { - // pg's default is 10s, which churns connections during quiet periods. - idleTimeoutMillis: safeGetEnvNonNegativeInt( - 'DATABASE_POOL_IDLE_TIMEOUT_MS', - 300000, - ), - // pg's default is 0 (wait forever); fail fast if the db is unreachable. - connectionTimeoutMillis: safeGetEnvNonNegativeInt( - 'DATABASE_POOL_CONNECTION_TIMEOUT_MS', - 15000, - ), - // Client-side bound on long-running queries. - query_timeout: safeGetEnvNonNegativeInt( - 'DATABASE_QUERY_TIMEOUT_MS', - 1000000, - ), - // Optional server-side bound; defense in depth alongside `query_timeout`. - // Unset => Postgres' own default (no limit). - ...(statementTimeoutMs && { - statement_timeout: safeGetEnvNonNegativeInt( - 'DATABASE_STATEMENT_TIMEOUT_MS', - 0, - ), - }), - // Kill sessions sitting idle inside an open transaction (holding locks). - idle_in_transaction_session_timeout: safeGetEnvNonNegativeInt( - 'DATABASE_IDLE_IN_TRANSACTION_TIMEOUT_MS', - 300000, - ), - // Recycle each client after N seconds to dodge stale connections. - // 0 = never expire (default). - maxLifetimeSeconds: safeGetEnvNonNegativeInt( - 'DATABASE_POOL_MAX_LIFETIME_SECONDS', - 0, - ), - // TCP keepalive surfaces NAT/LB connection drops as pool errors - // (handled by `createPgPool`) rather than as hung queries. Defaults on; - // set DATABASE_KEEPALIVE=false to disable. - keepAlive: - process.env.DATABASE_KEEPALIVE?.trim().toLowerCase() !== 'false', - ...(keepAliveInitialDelayMs && { - keepAliveInitialDelayMillis: safeGetEnvNonNegativeInt( - 'DATABASE_KEEPALIVE_INITIAL_DELAY_MS', - 0, - ), - }), - }; - }; - - // NB: this is a function because safeGetEnvVar can throw, so we only want to - // try to look up the env vars (and throw if they're missing) _if someone - // actually tries to fetch a service from bottle that needs these env vars_. - // Not every worker/job needs every service or is given every var in its env. - // - // NB: while we can reasonably provide default values for some of the env vars - // below, we wouldn't want to provide default values for all of them, as then - // that would defeat the ability of safeGetEnvVar to alert us in prod if a - // worker that needs these vars is run without them. - const getPgMasterConnectionInfo = () => ({ - ...getPgConnectionParams(), - max: parseInt(process.env.DATABASE_POOL_MAX ?? '30'), - application_name: - getEnvVarOrWarn('OTEL_SERVICE_NAME') ?? 'unknown-coop-service', - ...getPgPoolTuning(), - }); - - // Kysely's default is `['error']`; opt-in to also logging every executed - // query (SQL, bound params, duration). - const kyselyLogLevels: ReadonlyArray<'query' | 'error'> = isEnvTrue( - 'DATABASE_PRINT_LOGS', - ) - ? ['query', 'error'] - : ['error']; - const bottle = new Bottle(); // Pg services. @@ -568,7 +472,7 @@ export default async function getBottle( // - KyselyPgReadReplica gives us the same type safety, but sends queries to our // replicas, for when we only need reads and we're ok w/ eventual consistency. bottle.factory('KyselyPgPool', () => - createPgPool(getPgMasterConnectionInfo()), + createPgPool(databaseConfig.connections.primary), ); bottle.factory( @@ -580,7 +484,7 @@ export default async function getBottle( pool: container.KyselyPgPool, cursor: Cursor, }), - log: kyselyLogLevels, + log: databaseConfig.logLevels, }), ); @@ -590,14 +494,10 @@ export default async function getBottle( new Kysely({ dialect: new PostgresDialect({ controlClient: pg.Client, - pool: createPgPool({ - ...getPgMasterConnectionInfo(), - max: parseInt(process.env.DATABASE_READ_POOL_MAX ?? '150'), - host: safeGetEnvVar('DATABASE_READ_ONLY_HOST'), - }), + pool: createPgPool(databaseConfig.connections.readReplica), cursor: Cursor, }), - log: kyselyLogLevels, + log: databaseConfig.logLevels, }), ); @@ -1870,35 +1770,3 @@ function serviceHasBeenAccessed( const propDesc = Object.getOwnPropertyDescriptor(container, serviceName); return typeof propDesc?.get !== 'function'; } - -/** - * Gets an env var, or logs a warning if the variable is not defined. This is - * useful for cases where an env var should be provided, but the app can recover - * on the off-chance that the variable was improperly omitted, and we'd rather - * have the fallback behavior than create an outage. However, we still want to - * log a warning so that we can see in DD that we need to set this variable. - * - * TODO: create a DD metric that counts these warnings, and set up a monitor to - * alert if there are any. - */ -function getEnvVarOrWarn(varName: string) { - const value = process.env[varName]; - - if (value == null) { - // NB: using this format for the logged JSON is taking on some tech debt - // (esp if/once we create a DD monitor/metric that uses `title` to find - // these errors), because we probably want to reformat these logged errors - // later in a way that makes them more consistent amongst each other and - // possibly also more consistent with CoopError errors. For now, though, - // figuring out that end state isn't worth the brainpower. - // eslint-disable-next-line no-console - console.warn( - jsonStringify({ - title: 'MissingEnvVar', - message: `Missing env var ${varName}`, - }), - ); - } - - return value; -} diff --git a/server/start/env.ts b/server/start/env.ts index 316fece6a..9fc0718fc 100644 --- a/server/start/env.ts +++ b/server/start/env.ts @@ -29,6 +29,8 @@ const env = await Env.create(new URL('./', import.meta.url), { // Postgresql configuration: DATABASE_HOST: Env.schema.string({ format: 'host' }), + // Falls back to DATABASE_HOST, so a deployment without a separate replica + // need not repeat the primary host. DATABASE_READ_ONLY_HOST: Env.schema.string.optional({ format: 'host' }), DATABASE_PORT: Env.schema.integer.positive.optional(), DATABASE_NAME: Env.schema.string.optional(), diff --git a/server/test/harness/transactionalPgPool.integ.test.ts b/server/test/harness/transactionalPgPool.integ.test.ts index b7be617ff..4815d4d54 100644 --- a/server/test/harness/transactionalPgPool.integ.test.ts +++ b/server/test/harness/transactionalPgPool.integ.test.ts @@ -4,14 +4,14 @@ * Proves that `createTransactionalTestDb` lets us wrap a whole test in a single * Postgres transaction that is rolled back at the end. */ +import databaseConfig from '#config/database'; import { Kysely, PostgresDialect, sql, type PostgresPool } from 'kysely'; import pg from 'pg'; -import { getPgConnectionParams } from '../../iocContainer/index.js'; import { makeKyselyTransactionWithRetry } from '../../utils/kyselyTransactionWithRetry.js'; import { createTransactionalTestDb } from './transactionalPgPool.js'; -const pgConfig = getPgConnectionParams(); +const pgConfig = databaseConfig.connectionParams; describe('createTransactionalTestDb', () => { it('exposes the pool metadata Kysely needs for query cancellation', async () => { diff --git a/server/test/setupMockedServer.ts b/server/test/setupMockedServer.ts index b75f75a82..acae3a405 100644 --- a/server/test/setupMockedServer.ts +++ b/server/test/setupMockedServer.ts @@ -3,13 +3,11 @@ // relying on here). import otel from '@opentelemetry/api'; +import databaseConfig from '#config/database'; import type pg from 'pg'; import * as superTest from 'supertest'; -import getBottle, { - getPgConnectionParams, - type Dependencies, -} from '../iocContainer/index.js'; +import getBottle, { type Dependencies } from '../iocContainer/index.js'; import makeServer from '../server.js'; import { type IDataWarehouse } from '../storage/dataWarehouse/IDataWarehouse.js'; import type { IDataWarehouseAnalytics } from '../storage/dataWarehouse/IDataWarehouseAnalytics.js'; @@ -40,7 +38,7 @@ export function disableConsoleLogging() { * `makeTransactionalTestWithFixture`. */ export async function makeMockedServer() { - const tdb = createTransactionalTestDb(getPgConnectionParams()); + const tdb = createTransactionalTestDb(databaseConfig.connectionParams); await tdb.begin(); const deps = await getBottleContainerWithIOMocks({ kyselyPool: tdb.pool }); From 64dfe870f1fa3c9d19978f4e3a55de0c0ae06faf Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 01:22:05 +0200 Subject: [PATCH 05/19] Move Redis configuration into config/redis Shaped after `@adonisjs/redis`' `defineConfig`: a map of named connections, each either plain ioredis options or a cluster config, told apart by the presence of `clusters`. The two clients the container builds differ only by `enableOfflineQueue`, so they become the `main` and `enqueueNoBuffer` connections rather than one factory taking an overrides argument. `iocContainer` no longer reads any Redis environment variable, and no longer decides between cluster and single-node: it branches on the shape it is handed. `REDIS_TLS` was already read by the container but was declared nowhere, so it is added to the schema and documented in `.env.example` for the first time. Cluster connections remain unconditionally TLS, as before, which the comments now say out loud. Co-Authored-By: Claude Opus 5 (1M context) --- server/.env.example | 2 + server/config/redis.ts | 84 ++++++++++++++++++++++++++++++++++++ server/iocContainer/index.ts | 65 +++------------------------- server/start/env.ts | 2 + 4 files changed, 95 insertions(+), 58 deletions(-) create mode 100644 server/config/redis.ts diff --git a/server/.env.example b/server/.env.example index 15ab39bfd..cd49cc3cb 100644 --- a/server/.env.example +++ b/server/.env.example @@ -99,6 +99,8 @@ REDIS_HOST=localhost REDIS_PORT=6379 REDIS_USER= REDIS_PASSWORD= +# Single-node connections only; cluster connections are always TLS. +REDIS_TLS=false # Signal API Keys/URLs OPEN_AI_API_KEY= diff --git a/server/config/redis.ts b/server/config/redis.ts new file mode 100644 index 000000000..19eba383c --- /dev/null +++ b/server/config/redis.ts @@ -0,0 +1,84 @@ +import env from '#start/env'; +import type { + ClusterNode, + ClusterOptions, + DNSLookupFunction, + RedisOptions, +} from 'ioredis'; + +/** + * Redis connections, shaped after `@adonisjs/redis`' `defineConfig`: a map of + * named connections, each either plain ioredis options or a cluster config. + * The two are told apart by the presence of `clusters`, so consumers branch on + * the shape they are handed rather than re-reading `REDIS_USE_CLUSTER`. + */ +type ClusterConnection = { + clusters: ClusterNode[]; + clusterOptions: ClusterOptions; +}; + +export type RedisConnection = RedisOptions | ClusterConnection; + +const host = env.get('REDIS_HOST'); +const port = env.get('REDIS_PORT', 6379); +const password = env.get('REDIS_PASSWORD'); +const user = env.get('REDIS_USER'); + +/** + * AUTH-enabled Redis (e.g. ElastiCache with an auth token) rejects every + * command with NOAUTH unless credentials are sent, which leaves ioredis stuck + * before "ready" and parks commands in the offline queue forever. Local dev + * Redis has no password, so only pass credentials when REDIS_PASSWORD is set; + * REDIS_USER may be set-but-empty, which means the default user. + */ +const auth: RedisOptions = password + ? { + ...(user ? { username: user } : {}), + password: password.release(), + } + : {}; + +/** + * See + * https://github.com/luin/ioredis/blob/c275e9a337a4aee1565e96fe631d28a29ecb4efa/README.md#special-note-aws-elasticache-clusters-with-tls + */ +const dnsLookup: DNSLookupFunction = (address, callback) => + callback(null, address); + +function connection(extra: RedisOptions = {}): RedisConnection { + const options: RedisOptions = { + // Required by BullMQ: its workers use blocking Redis commands that would + // otherwise be misinterpreted as timed-out requests. + maxRetriesPerRequest: null, + ...auth, + ...extra, + }; + + return env.get('REDIS_USE_CLUSTER') + ? { + clusters: [{ host, port }], + clusterOptions: { + dnsLookup, + // Cluster connections are always TLS, regardless of REDIS_TLS. + redisOptions: { tls: {}, ...options }, + }, + } + : { + host, + port, + ...options, + ...(env.get('REDIS_TLS', false) && { tls: { servername: host } }), + }; +} + +export default { + connections: { + main: connection(), + /** + * With `enableOfflineQueue: false`, a `queue.addBulk` while Redis is + * unreachable rejects immediately instead of resolving against the + * in-process buffer, failing the enqueue early with "couldn't enqueue". + */ + enqueueNoBuffer: connection({ enableOfflineQueue: false }), + }, +}; diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index 28b741a1f..855ba9c96 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -4,6 +4,7 @@ import Bottle from '@ethanresnick/bottlejs'; import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; import databaseConfig from '#config/database'; +import redisConfig, { type RedisConnection } from '#config/redis'; import { types as scyllaTypes, type Host as ScyllaHost, @@ -501,66 +502,14 @@ export default async function getBottle( }), ); - // AUTH-enabled Redis (e.g. ElastiCache with an auth token) rejects every - // command with NOAUTH unless credentials are sent, which leaves ioredis stuck - // before "ready" and parks commands in the offline queue forever. Local dev - // Redis has no password, so only pass credentials when REDIS_PASSWORD is set; - // REDIS_USER may be set-but-empty, which means the default user. Shared by the - // cluster and single-node paths so both authenticate identically. - const redisAuthOptions = (): { username?: string; password?: string } => - process.env.REDIS_PASSWORD - ? { - ...(process.env.REDIS_USER - ? { username: process.env.REDIS_USER } - : {}), - password: process.env.REDIS_PASSWORD, - } - : {}; - - const makeRedis = ( - extraOptions: { enableOfflineQueue?: boolean } = {}, - ): IORedis.Redis | Cluster => - safeGetEnvVar('REDIS_USE_CLUSTER') === 'true' - ? new IORedis.Cluster( - [ - { - host: safeGetEnvVar('REDIS_HOST'), - port: parseInt(process.env.REDIS_PORT ?? '6379'), - }, - ], - { - // See - // https://github.com/luin/ioredis/blob/c275e9a337a4aee1565e96fe631d28a29ecb4efa/README.md#special-note-aws-elasticache-clusters-with-tls - dnsLookup: (address, callback) => callback(null, address), - redisOptions: { - tls: {}, - // Required by BullMQ: its workers use blocking Redis commands - // that would otherwise be misinterpreted as timed-out requests. - maxRetriesPerRequest: null, - ...redisAuthOptions(), - ...extraOptions, - }, - }, - ) - : new IORedis.default({ - // Required by BullMQ: its workers use blocking Redis commands - // that would otherwise be misinterpreted as timed-out requests. - maxRetriesPerRequest: null, - port: parseInt(process.env.REDIS_PORT ?? '6379'), - host: safeGetEnvVar('REDIS_HOST'), - ...redisAuthOptions(), - ...(isEnvTrue('REDIS_TLS') - ? { tls: { servername: safeGetEnvVar('REDIS_HOST') } } - : {}), - ...extraOptions, - }); + const makeRedis = (connection: RedisConnection): IORedis.Redis | Cluster => + 'clusters' in connection + ? new IORedis.Cluster(connection.clusters, connection.clusterOptions) + : new IORedis.default(connection); - bottle.factory('IORedis', () => makeRedis()); - // With `enableOfflineQueue: false`, a `queue.addBulk` while Redis is - // unreachable rejects immediately instead of resolving against the - // in-process buffer. fails enqueue early with "couldn't enqueue". + bottle.factory('IORedis', () => makeRedis(redisConfig.connections.main)); bottle.factory('IORedisEnqueueNoBuffer', () => - makeRedis({ enableOfflineQueue: false }), + makeRedis(redisConfig.connections.enqueueNoBuffer), ); // Data Warehouse abstraction layer diff --git a/server/start/env.ts b/server/start/env.ts index 9fc0718fc..cf415be2e 100644 --- a/server/start/env.ts +++ b/server/start/env.ts @@ -59,6 +59,8 @@ const env = await Env.create(new URL('./', import.meta.url), { REDIS_PORT: Env.schema.integer.positive.optional(), REDIS_USER: Env.schema.string.optional(), REDIS_PASSWORD: Env.schema.secret.optional(), + // Single-node connections only: the cluster path is always TLS. + REDIS_TLS: Env.schema.boolean.optional(), // Secrets: SESSION_SECRET: Env.schema.string(), From 3be4ac072fe664802c236c0742c5e4c8c1aefd12 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 01:48:44 +0200 Subject: [PATCH 06/19] Move Scylla configuration into config/scylla MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renames `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` to `SCYLLA_ENABLED`, which is free because it has not shipped: it is still under `## [Unreleased]`. The env schema parses it, so `itemInvestigationAndStrikesEnabled` and its bespoke `false`/`0`/`no` parsing go away. `connection` is `null` when Scylla is disabled rather than an options object full of absent values, so `iocContainer` branches on the shape it is handed, the way it now does for Redis. When enabled, `optionalWhen` makes the four connection variables required, moving the failure from first use of the Scylla service to startup. `hostList` gains a matching `optionalWhen` so the custom validator composes like the built-in ones. `SCYLLA_PORT`, `SCYLLA_SSL` and `SCYLLA_SSL_SERVERNAME` were read by the container but declared nowhere; they are added to the schema and documented. This is the last user of `safeGetEnvVar` and `isEnvTrue`, which are now unused by `iocContainer`. Fixes the Scylla TLS options passing the certificate hostname as `host`. The driver hands those straight to `tls.connect(port, address, sslOptions)`, where an options `host` overrides the positional address — so enabling TLS retargeted every node connection at that one name, and failed outright when it was inferred from a contact point carrying an explicit `:port`. `servername` sets the SNI value and the identity check without moving the connection. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 +- server/.env.example | 20 ++++--- server/config/scylla.ts | 92 +++++++++++++++++++++++++++++++ server/iocContainer/index.ts | 76 ++++--------------------- server/lib/env/validators.test.ts | 40 ++++++++++++++ server/lib/env/validators.ts | 34 ++++++++++++ server/scylla/noOpScylla.test.ts | 37 ++----------- server/scylla/noOpScylla.ts | 23 +------- server/start/env.ts | 31 +++++++++-- 9 files changed, 225 insertions(+), 131 deletions(-) create mode 100644 server/config/scylla.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e813351c..22fa14708 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ For more information about each release including git tags and artifacts, see [R - Invalid environment variable values now prevent startup instead of falling back to defaults ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Boolean environment variables accept only `1`, `0`, `true` and `false` ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - `DATABASE_READ_ONLY_HOST` is now optional, falling back to `DATABASE_HOST` ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) -- Scylla is now optional via `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` ([#918](https://github.com/roostorg/coop/pull/918) by [@sunilatlas](https://github.com/sunilatlas)) +- Scylla is now optional via `SCYLLA_ENABLED` ([#918](https://github.com/roostorg/coop/pull/918) by [@sunilatlas](https://github.com/sunilatlas)) - Settings "Other" tab renamed to "Partial Items" and its settings relocated ([#965](https://github.com/roostorg/coop/pull/965) by [@golden-fox07](https://github.com/golden-fox07)) - Queue deletion is refused while routing rules still reference the queue ([#808](https://github.com/roostorg/coop/pull/808) by [@reitblatt](https://github.com/reitblatt)) - Long text fields in the review console collapse behind a "Read more" control ([#903](https://github.com/roostorg/coop/pull/903) by [@taobojlen](https://github.com/taobojlen)) @@ -36,6 +36,7 @@ For more information about each release including git tags and artifacts, see [R ### Fixed - Doubled slashes in generated links when `UI_URL` ends with a slash ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) +- Scylla TLS retargeting every connection to the certificate hostname instead of the cluster node ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Rule history dropping other rules' versions when filtered by start date ([#1056](https://github.com/roostorg/coop/pull/1056) by [@juanmrad](https://github.com/juanmrad)) - `RetryFailedNcmecDecisionsJob` ignoring `NCMEC_ENV` and retrying test decisions ([#928](https://github.com/roostorg/coop/pull/928) by [@taobojlen](https://github.com/taobojlen)) - Queue creation failing with "name already exists" on the default reviewer selection ([#1069](https://github.com/roostorg/coop/pull/1069) by [@jess-upscrolled](https://github.com/jess-upscrolled), closes [#1074](https://github.com/roostorg/coop/issues/1074)) diff --git a/server/.env.example b/server/.env.example index cd49cc3cb..8f3c5d932 100644 --- a/server/.env.example +++ b/server/.env.example @@ -76,17 +76,23 @@ CLICKHOUSE_PROTOCOL=http HMA_SERVICE_URL=http://localhost:9876 # Scylla Cluster Details -# Set to "false" to run Coop without a Scylla cluster. This disables the two -# Scylla-backed features — Item Investigation (item/user history views) and -# User Strikes (repeat-offender strike counts) — which then no-op: reads return -# empty (strike counts read as 0) and writes are dropped. When "false" (or -# "0"/"no"), the SCYLLA_* connection settings below are not required. -# Defaults to enabled. -ITEM_INVESTIGATION_AND_STRIKES_ENABLED=true +# Set to "false" (or "0") to run Coop without a Scylla cluster. This disables +# the two Scylla-backed features — Item Investigation (item/user history views) +# and User Strikes (repeat-offender strike counts) — which then no-op: reads +# return empty (strike counts read as 0) and writes are dropped. The SCYLLA_* +# connection settings below are then not required; otherwise they are, and the +# process refuses to start without them. Defaults to enabled. +SCYLLA_ENABLED=true SCYLLA_USERNAME=cassandra SCYLLA_PASSWORD=cassandra SCYLLA_HOSTS='127.0.0.1:9042' SCYLLA_LOCAL_DATACENTER='datacenter1' +SCYLLA_PORT=9042 +# TLS. SCYLLA_SSL_SERVERNAME sets the SNI value for certificate verification, +# for when the contact points don't match the server certificate (e.g. an AWS +# Keyspaces regional endpoint); it defaults to the first contact point. +SCYLLA_SSL=false +SCYLLA_SSL_SERVERNAME= # Local dev NODE_ENV=development diff --git a/server/config/scylla.ts b/server/config/scylla.ts new file mode 100644 index 000000000..3411bd2eb --- /dev/null +++ b/server/config/scylla.ts @@ -0,0 +1,92 @@ +import env from '#start/env'; +import { types as scyllaTypes, type ClientOptions } from 'cassandra-driver'; + +const enabled = env.get('SCYLLA_ENABLED', true); + +const contactPoints = env.get('SCYLLA_HOSTS'); +const username = env.get('SCYLLA_USERNAME'); +const password = env.get('SCYLLA_PASSWORD'); + +const firstContactPoint = contactPoints?.[0]; + +/** + * A contact point may carry an explicit `:port`, which is not part of the + * hostname a certificate is issued for. Only a single colon is a port + * separator: a bare IPv6 address has several, and no port to strip. + */ +const firstHostname = + firstContactPoint !== undefined && firstContactPoint.split(':').length === 2 + ? firstContactPoint.split(':')[0] + : firstContactPoint; + +// For TLS hostname verification we need an SNI value that matches the server +// cert. Prefer an explicit `SCYLLA_SSL_SERVERNAME` (e.g. the Keyspaces regional +// endpoint) over inferring one from `SCYLLA_HOSTS`, which may contain multiple +// contact points with different cert names. +const sslServerName = env.get('SCYLLA_SSL_SERVERNAME') ?? firstHostname; + +/** + * Scylla settings. + * + * `SCYLLA_ENABLED=false` swaps in a no-op client that drops writes and returns + * empty reads, disabling Item Investigation and User Strikes. The connection + * settings are then not needed, which is why they are optional here; when it is + * on, `start/env.ts` requires them. + * + * N.B. Currently all our services that use Scylla as a backing datastore use the + * same keyspace. If we ever need to add one (e.g. because we have tables that + * need a new replication strategy, or we support multiple datacenters) we will + * want one client per keyspace, since the driver is keyspace aware and switching + * with `USE KEYSPACE` all the time is annoying and likely error prone. + */ +export default { + enabled, + + /** + * `null` when disabled, so a consumer cannot accidentally build a client + * against settings that were never required. + */ + connection: !enabled + ? null + : ({ + contactPoints: contactPoints ? [...contactPoints] : undefined, + localDataCenter: env.get('SCYLLA_LOCAL_DATACENTER'), + keyspace: 'item_investigation_service', + // Omitted rather than half-filled when unset, so the driver falls back to + // connecting without authentication. + ...(username !== undefined && + password !== undefined && { + credentials: { username, password: password.release() }, + }), + protocolOptions: { + port: env.get('SCYLLA_PORT', 9042), + }, + // `servername` rather than `host`: the driver passes these straight to + // `tls.connect(port, address, sslOptions)`, where an options `host` + // overrides the positional address and so retargets the connection + // itself. `servername` sets the SNI value and the certificate identity + // check without moving where the driver connects. + sslOptions: env.get('SCYLLA_SSL', false) + ? { + servername: sslServerName, + rejectUnauthorized: true, + } + : undefined, + pooling: { + coreConnectionsPerHost: { + [scyllaTypes.distance.local]: 3, + [scyllaTypes.distance.remote]: 1, + }, + }, + queryOptions: { + // Quorum consistency requires a simple majority of nodes in a replica + // group to respond to read/write requests. Local Quorum is the same + // except it only expects nodes in the local datacenter to respond. For + // our current Scylla infrastructure quorum and local quorum will have + // identical behavior, but if we ever add another datacenter to the + // cluster using Quorum and requiring responses from multiple DCs would + // degrade performance significantly. + consistency: scyllaTypes.consistencies.localQuorum, + }, + } satisfies ClientOptions), +}; diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index 855ba9c96..a8a7114fc 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -5,10 +5,8 @@ import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; import databaseConfig from '#config/database'; import redisConfig, { type RedisConnection } from '#config/redis'; -import { - types as scyllaTypes, - type Host as ScyllaHost, -} from 'cassandra-driver'; +import scyllaConfig from '#config/scylla'; +import { type Host as ScyllaHost } from 'cassandra-driver'; import IORedis, { type Cluster } from 'ioredis'; import { Kysely, PostgresDialect } from 'kysely'; import _ from 'lodash'; @@ -61,9 +59,7 @@ import makeRuleEvaluator, { type RuleEvaluator, } from '../rule_engine/RuleEvaluator.js'; import { Scylla } from '../scylla/index.js'; -import NoOpScylla, { - itemInvestigationAndStrikesEnabled, -} from '../scylla/noOpScylla.js'; +import NoOpScylla from '../scylla/noOpScylla.js'; import { makeActionStatisticsService, type ActionStatisticsService, @@ -254,7 +250,7 @@ import { import { createPgPool } from './createPgPool.js'; import { registerGqlDataSources } from './services/gqlDataSources.js'; import { registerWorkersAndJobs } from './services/workersAndJobs.js'; -import { isEnvTrue, register, safeGetEnvVar } from './utils.js'; +import { register } from './utils.js'; // the otel instrumentation currently intercepts require statements. support for // esm support is experimental so we should wait until it is stable @@ -636,9 +632,7 @@ export default async function getBottle( executionContext, ); }, - itemInvestigationAndStrikesEnabled( - process.env.ITEM_INVESTIGATION_AND_STRIKES_ENABLED, - ), + scyllaConfig.enabled, ), ); @@ -652,64 +646,16 @@ export default async function getBottle( bottle.factory('Scylla', () => { // Scylla backs the item-investigation and user-strike features. Operators // who don't need those (and don't want to run a Scylla cluster) can set - // `ITEM_INVESTIGATION_AND_STRIKES_ENABLED=false` to swap in a no-op that - // drops writes and returns empty reads, so no `SCYLLA_*` connection env - // vars are required. Defaults to enabled to preserve existing behaviour. - if ( - !itemInvestigationAndStrikesEnabled( - process.env.ITEM_INVESTIGATION_AND_STRIKES_ENABLED, - ) - ) { + // `SCYLLA_ENABLED=false` to swap in a no-op that drops writes and returns + // empty reads, so no `SCYLLA_*` connection env vars are required. Defaults + // to enabled to preserve existing behaviour. + if (scyllaConfig.connection === null) { // eslint-disable-next-line no-restricted-syntax - logJson( - 'scylla.disabled ITEM_INVESTIGATION_AND_STRIKES_ENABLED=false; using no-op Scylla', - ); + logJson('scylla.disabled SCYLLA_ENABLED=false; using no-op Scylla'); return new NoOpScylla(); } - const contactPoints = safeGetEnvVar('SCYLLA_HOSTS') - .split(',') - .map((it) => it.trim()) - .filter((it) => it.length > 0); - // For TLS hostname verification we need an SNI value that matches the - // server cert. Prefer an explicit `SCYLLA_SSL_SERVERNAME` (e.g., the - // Keyspaces regional endpoint) over inferring one from `SCYLLA_HOSTS`, - // which may contain multiple contact points with different cert names. - const sslServerName = process.env.SCYLLA_SSL_SERVERNAME ?? contactPoints[0]; - const scyllaDriver = new ScyllaClient({ - contactPoints, - credentials: { - username: safeGetEnvVar('SCYLLA_USERNAME'), - password: safeGetEnvVar('SCYLLA_PASSWORD'), - }, - localDataCenter: safeGetEnvVar('SCYLLA_LOCAL_DATACENTER'), - keyspace: 'item_investigation_service', - protocolOptions: { - port: parseInt(process.env.SCYLLA_PORT ?? '9042'), - }, - sslOptions: isEnvTrue('SCYLLA_SSL') - ? { - host: sslServerName, - rejectUnauthorized: true, - } - : undefined, - pooling: { - coreConnectionsPerHost: { - [scyllaTypes.distance.local]: 3, - [scyllaTypes.distance.remote]: 1, - }, - }, - queryOptions: { - // Quorum consistency requires a simple majority of nodes in a - // replica group to respond to read/write requests. Local Quorum is - // the same except it only expects nodes in the local datacenter to - // respond. For our current Scylla infrastructure quorum and local - // quorum will have identical behavior, but if we ever add another - // datacenter to the cluster using Quorum and requiring responses - // from multiple DCs would degrade performance significantly - consistency: scyllaTypes.consistencies.localQuorum, - }, - }); + const scyllaDriver = new ScyllaClient(scyllaConfig.connection); // Surface cluster state changes so reconnect storms are visible in logs. scyllaDriver.on('hostUp', (host: ScyllaHost) => { diff --git a/server/lib/env/validators.test.ts b/server/lib/env/validators.test.ts index 072274823..a631aeead 100644 --- a/server/lib/env/validators.test.ts +++ b/server/lib/env/validators.test.ts @@ -136,4 +136,44 @@ describe('hostList env validator', () => { expect(() => hostList.optional()('SCYLLA_HOSTS', ',')).toThrow(); }); }); + + describe('.optionalWhen()', () => { + test('is required when the condition is false', () => { + expect(() => + hostList.optionalWhen(false)('SCYLLA_HOSTS', undefined), + ).toThrow(); + expect(hostList.optionalWhen(false)('SCYLLA_HOSTS', 'db1')).toEqual([ + 'db1', + ]); + }); + + test('is optional when the condition is true', () => { + expect( + hostList.optionalWhen(true)('SCYLLA_HOSTS', undefined), + ).toBeUndefined(); + }); + + test('still validates a value that is present, even when optional', () => { + expect(() => hostList.optionalWhen(true)('SCYLLA_HOSTS', ',')).toThrow(); + }); + + test('evaluates a function condition at validation time, not schema build time', () => { + let disabled = false; + const validate = hostList.optionalWhen(() => disabled); + + expect(() => validate('SCYLLA_HOSTS', undefined)).toThrow(); + + // The condition is re-read on each call, which is what lets it depend on + // a sibling variable that `Env.create` only puts in `process.env` after + // the schema object has been built. + disabled = true; + expect(validate('SCYLLA_HOSTS', undefined)).toBeUndefined(); + }); + + test('passes the key and value through to a function condition', () => { + const condition = jest.fn(() => true); + hostList.optionalWhen(condition)('SCYLLA_HOSTS', 'db1'); + expect(condition).toHaveBeenCalledWith('SCYLLA_HOSTS', 'db1'); + }); + }); }); diff --git a/server/lib/env/validators.ts b/server/lib/env/validators.ts index fcc92789c..71f1ee168 100644 --- a/server/lib/env/validators.ts +++ b/server/lib/env/validators.ts @@ -14,6 +14,25 @@ import { Env as BaseEnv } from '@adonisjs/env'; type Validator = (key: string, value?: string) => T; +/** + * Matches `@poppinss/validator-lite`'s own `optionalWhen` condition: truthy + * means the variable is optional. + * + * Prefer the function form. A plain boolean is evaluated when the schema object + * literal is built, which happens before `Env.create` copies `.env` file values + * into `process.env` — so a condition reading a sibling variable would see it + * only when it came from the real environment, not from an env file. + */ +type Condition = boolean | ((key: string, value?: string) => boolean); + +function isOptional( + condition: Condition, + key: string, + value?: string, +): boolean { + return typeof condition === 'function' ? condition(key, value) : condition; +} + type IntegerValidator = (() => Validator) & { optional: () => Validator; }; @@ -97,6 +116,9 @@ export const integer: IntegerValidators = Object.assign( type HostListValidator = (() => Validator) & { optional: () => Validator; + optionalWhen: ( + condition: Condition, + ) => Validator; }; const HOST_LIST_EXPECTATION = @@ -155,5 +177,17 @@ export const hostList: HostListValidator = Object.assign( { optional: (): Validator => (key, value) => value ? parseHostList(key, value) : undefined, + + /** + * Required unless `condition` says otherwise, mirroring the built-in + * `Env.schema.string.optionalWhen`. Used for a variable that only matters + * when the feature it configures is switched on. + */ + optionalWhen: + (condition: Condition): Validator => + (key, value) => + isOptional(condition, key, value) + ? hostList.optional()(key, value) + : hostList()(key, value), }, ); diff --git a/server/scylla/noOpScylla.test.ts b/server/scylla/noOpScylla.test.ts index bbf1c4049..f423b7f48 100644 --- a/server/scylla/noOpScylla.test.ts +++ b/server/scylla/noOpScylla.test.ts @@ -1,40 +1,15 @@ -import NoOpScylla, { - itemInvestigationAndStrikesEnabled, -} from './noOpScylla.js'; +import NoOpScylla from './noOpScylla.js'; import Scylla from './scylla.js'; /** - * Tests for the Scylla-disabled path used when - * `ITEM_INVESTIGATION_AND_STRIKES_ENABLED=false`. + * Tests for the Scylla-disabled path used when `SCYLLA_ENABLED=false`: the + * behavioural contract of {@link NoOpScylla} (drops writes, empty reads, + * connect/close resolve). * - * Two things are covered: - * 1. The behavioural contract of {@link NoOpScylla} (drops writes, empty reads, - * connect/close resolve). - * 2. The exact flag-parsing predicate (`itemInvestigationAndStrikesEnabled`) - * used by the `Scylla` DI factory in `iocContainer` to decide - * enabled-vs-disabled. Imported directly (not mirrored) so the - * default-enabled (upstream-preserving) behaviour is guarded by a test. + * The flag itself is now parsed by the env schema (`Env.schema.boolean`), so + * there is no bespoke predicate left to test here. */ -describe('ITEM_INVESTIGATION_AND_STRIKES_ENABLED gate predicate', () => { - test('defaults to enabled when unset (preserves upstream behaviour)', () => { - expect(itemInvestigationAndStrikesEnabled(undefined)).toBe(true); - expect(itemInvestigationAndStrikesEnabled('')).toBe(true); - }); - - test('is disabled only for explicit falsey values', () => { - for (const v of ['false', 'FALSE', ' false ', '0', 'no', 'No']) { - expect(itemInvestigationAndStrikesEnabled(v)).toBe(false); - } - }); - - test('stays enabled for truthy / unrelated values', () => { - for (const v of ['true', 'TRUE', '1', 'yes', 'anything']) { - expect(itemInvestigationAndStrikesEnabled(v)).toBe(true); - } - }); -}); - describe('NoOpScylla', () => { // A minimal DB shape for the generic parameter. type TestDB = { widgets: { id: number; name: string } }; diff --git a/server/scylla/noOpScylla.ts b/server/scylla/noOpScylla.ts index 99ab71fa7..1bdb548e3 100644 --- a/server/scylla/noOpScylla.ts +++ b/server/scylla/noOpScylla.ts @@ -1,30 +1,9 @@ import { type CqlSelectOptions, type DBDefinition } from './cqlUtils.js'; import Scylla from './scylla.js'; -/** - * Parses the `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` feature flag from its raw - * string value (i.e. `process.env.ITEM_INVESTIGATION_AND_STRIKES_ENABLED`). - * - * Shared by the `Scylla` DI factory in `iocContainer` (to decide whether to - * return a real Scylla or a {@link NoOpScylla}) and by the unit tests. Defaults - * to enabled when unset/empty so existing deployments are unaffected; only the - * explicit falsey values `false`/`0`/`no` (case/whitespace-insensitive) disable - * the Scylla-backed features. - * - * Kept as a pure function of its argument (it does not read `process.env` - * itself) so callers own where the value comes from and tests stay independent - * of the ambient environment. - */ -export function itemInvestigationAndStrikesEnabled( - raw: string | undefined, -): boolean { - return !['false', '0', 'no'].includes((raw ?? 'true').trim().toLowerCase()); -} - /** * A no-op implementation of {@link Scylla} used when the Scylla-backed features - * (item investigation and user strikes) are disabled via - * `ITEM_INVESTIGATION_AND_STRIKES_ENABLED=false`. + * (item investigation and user strikes) are disabled via `SCYLLA_ENABLED=false`. * * Scylla has no managed offering on some deployment platforms, and some * operators do not need the features that depend on it. Rather than gate the diff --git a/server/start/env.ts b/server/start/env.ts index cf415be2e..6a23aacb9 100644 --- a/server/start/env.ts +++ b/server/start/env.ts @@ -1,5 +1,18 @@ import { Env } from '#lib/env'; +/** + * Whether the Scylla-backed features are switched off, which makes the + * connection settings unnecessary. + * + * A function rather than a boolean because a boolean would be evaluated while + * the schema object below is being built — before `Env.create` copies `.env` + * file values into `process.env`. It would then be correct only for a + * deployment that sets `SCYLLA_ENABLED` as a real environment variable, and + * silently wrong for one that sets it in an env file. + */ +const scyllaDisabled = () => + process.env.SCYLLA_ENABLED === 'false' || process.env.SCYLLA_ENABLED === '0'; + // `optional()` here means the application supplies a default, not that the // value is unimportant. See the corresponding `config/*.ts` module for what // that default is. Only variables the app genuinely cannot start without are @@ -107,12 +120,21 @@ const env = await Env.create(new URL('./', import.meta.url), { Env.schema.integer.positive.optional(), // Scylla: + // Turning this off swaps in a no-op that drops writes and returns empty + // reads, disabling Item Investigation and User Strikes. The connection + // settings below are then not needed, which is what `optionalWhen` expresses. + SCYLLA_ENABLED: Env.schema.boolean.optional(), // Contact points, e.g. "db1,db2:9043". Parsed into a list here so consumers // receive `string[]` rather than re-splitting the raw value. - SCYLLA_HOSTS: Env.schema.hostList.optional(), - SCYLLA_USERNAME: Env.schema.string.optional(), - SCYLLA_PASSWORD: Env.schema.secret.optional(), - SCYLLA_LOCAL_DATACENTER: Env.schema.string.optional(), + SCYLLA_HOSTS: Env.schema.hostList.optionalWhen(scyllaDisabled), + SCYLLA_USERNAME: Env.schema.string.optionalWhen(scyllaDisabled), + SCYLLA_PASSWORD: Env.schema.secret.optionalWhen(scyllaDisabled), + SCYLLA_LOCAL_DATACENTER: Env.schema.string.optionalWhen(scyllaDisabled), + SCYLLA_PORT: Env.schema.integer.positive.optional(), + SCYLLA_SSL: Env.schema.boolean.optional(), + // An explicit SNI value for TLS hostname verification, for when the contact + // points don't match the server certificate. + SCYLLA_SSL_SERVERNAME: Env.schema.string.optional({ format: 'host' }), // Selects the NCMEC CyberTipline endpoint that "Submit to NCMEC" decisions are // routed to. Anything other than the literal string `production` (including @@ -140,7 +162,6 @@ const env = await Env.create(new URL('./', import.meta.url), { HMA_SERVICE_URL: Env.schema.string.optional({ format: 'url', tld: false }), // Others: - ITEM_INVESTIGATION_AND_STRIKES_ENABLED: Env.schema.boolean.optional(), ITEM_QUEUE_TRAFFIC_PERCENTAGE: Env.schema.number(), }); From 5da421489fe0b50b9139cda04426244145f315c3 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 02:12:15 +0200 Subject: [PATCH 07/19] Extract ScyllaDatabase from the iocContainer factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Scylla` factory was ~80 lines that constructed the driver, wired cluster and log forwarding onto it, worked around a listener leak, and then declared a subclass inline to add `connect`/`close`. That subclass existed only because `Scylla` held its client as `private`, so it could not be written at module scope; it captured the driver by closure instead. Widening to `protected` lets the class live in its own file and reach `this.client` normally. `connect` and `close` become abstract on `Scylla`. Both implementations already had them, and declaring them on the base removes the `& { connect, close }` patch the container had to intersect onto the binding's type — a third implementation can no longer omit them and fail at shutdown instead of compile time. The driver's `require` moves along with the client it constructs. It is not incidental: the otel instrumentation intercepts require statements, so an ordinary import here would compile, pass, and silently stop tracing Scylla. Two incidental simplifications, both verified equivalent: - The listener-cap workaround reached `controlConnection.hosts` through a double cast because `controlConnection` is internal and absent from the driver's types. `Client.hosts` is the same `HostMap`, is public, is typed as an `EventEmitter`, and is assigned in the constructor, so no cast is needed. - `ScyllaDatabase` is not generic. Nothing could bind the parameter at the only construction site, so it already resolved to its constraint. Co-Authored-By: Claude Opus 5 (1M context) --- server/iocContainer/index.ts | 78 +++------------------------------ server/scylla/scylla.ts | 15 +++++-- server/scylla/scyllaDatabase.ts | 78 +++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 77 deletions(-) create mode 100644 server/scylla/scyllaDatabase.ts diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index a8a7114fc..fbb414391 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -1,12 +1,10 @@ /* eslint-disable max-lines */ -import { createRequire } from 'module'; import Bottle from '@ethanresnick/bottlejs'; import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; import databaseConfig from '#config/database'; import redisConfig, { type RedisConnection } from '#config/redis'; import scyllaConfig from '#config/scylla'; -import { type Host as ScyllaHost } from 'cassandra-driver'; import IORedis, { type Cluster } from 'ioredis'; import { Kysely, PostgresDialect } from 'kysely'; import _ from 'lodash'; @@ -58,8 +56,9 @@ import { import makeRuleEvaluator, { type RuleEvaluator, } from '../rule_engine/RuleEvaluator.js'; -import { Scylla } from '../scylla/index.js'; +import type { Scylla } from '../scylla/index.js'; import NoOpScylla from '../scylla/noOpScylla.js'; +import ScyllaDatabase from '../scylla/scyllaDatabase.js'; import { makeActionStatisticsService, type ActionStatisticsService, @@ -238,7 +237,7 @@ import { } from '../utils/correlationIds.js'; import { getUsableCoreCount } from '../utils/cpu-helpers.js'; import { jsonStringify, type JsonOf } from '../utils/encoding.js'; -import { logErrorJson, logJson } from '../utils/logging.js'; +import { logJson } from '../utils/logging.js'; import { __throw, assertUnreachable } from '../utils/misc.js'; import SafeTracer from '../utils/SafeTracer.js'; import { @@ -252,10 +251,6 @@ import { registerGqlDataSources } from './services/gqlDataSources.js'; import { registerWorkersAndJobs } from './services/workersAndJobs.js'; import { register } from './utils.js'; -// the otel instrumentation currently intercepts require statements. support for -// esm support is experimental so we should wait until it is stable -const require = createRequire(import.meta.url); -const { Client: ScyllaClient } = require('cassandra-driver'); export type { DataSources } from './services/gqlDataSources.js'; export type ItemSubmissionMessageKey = { @@ -321,10 +316,7 @@ export interface Dependencies { // that each dependent service can type its arg more specifically with the set // of tables it is responsible for / allowed to query. // eslint-disable-next-line @typescript-eslint/no-explicit-any - Scylla: Scylla & { - connect: () => Promise; - close: () => Promise; - }; + Scylla: Scylla; // Data Warehouse abstraction DataWarehouse: IDataWarehouse; @@ -655,67 +647,7 @@ export default async function getBottle( return new NoOpScylla(); } - const scyllaDriver = new ScyllaClient(scyllaConfig.connection); - - // Surface cluster state changes so reconnect storms are visible in logs. - scyllaDriver.on('hostUp', (host: ScyllaHost) => { - // eslint-disable-next-line no-restricted-syntax - logJson(`scylla.hostUp address=${host.address}`); - }); - scyllaDriver.on('hostDown', (host: ScyllaHost) => { - // eslint-disable-next-line no-restricted-syntax - logJson(`scylla.hostDown address=${host.address}`); - }); - // Forward driver-internal warnings/errors (auth, TLS, connection drops, - // etc.); skip the very chatty `info`/`verbose` levels. - scyllaDriver.on( - 'log', - ( - level: 'verbose' | 'info' | 'warning' | 'error', - source: string, - message: string, - furtherInfo?: unknown, - ) => { - if (level !== 'warning' && level !== 'error') { - return; - } - const wrapped = new Error(`scylla.${level}: [${source}] ${message}`); - if (furtherInfo instanceof Error) { - wrapped.stack = furtherInfo.stack ?? wrapped.stack; - } - // eslint-disable-next-line no-restricted-syntax - logErrorJson({ - message: `scylla.driver.${level}`, - error: wrapped, - }); - }, - ); - - // cassandra-driver leaks ~4 HostMap listeners per failed `Client._connect()` - // retry and never recreates the HostMap, so the default cap of 10 trips - // after ~3 failures. Raise it so transient blips don't spam the warning, - // but keep it bounded so a true runaway is still noticeable. - const controlConnection = ( - scyllaDriver as unknown as { - controlConnection?: { - hosts?: { setMaxListeners?: (n: number) => void }; - }; - } - ).controlConnection; - controlConnection?.hosts?.setMaxListeners?.(15); - - class ClosableScylla< - DB extends Record>, - > extends Scylla { - /** Eagerly connect; idempotent once `connected` is true. */ - async connect() { - return scyllaDriver.connect(); - } - async close() { - return scyllaDriver.shutdown(); - } - } - return new ClosableScylla(scyllaDriver); + return new ScyllaDatabase(scyllaConfig.connection); }); bottle.factory('ItemInvestigationService', (container) => { diff --git a/server/scylla/scylla.ts b/server/scylla/scylla.ts index d0ddc0780..a549564ea 100644 --- a/server/scylla/scylla.ts +++ b/server/scylla/scylla.ts @@ -10,10 +10,17 @@ import { const StreamToAsyncIterator = _S2A.default; -export default class Scylla { - constructor(private client: ScyllaClient) { - this.client = client; - } +export default abstract class Scylla { + constructor(protected client: ScyllaClient) {} + + /** + * Open the connection. Implementations make this idempotent, since callers + * (e.g. the item-processing worker) connect eagerly to fail fast. + */ + abstract connect(): Promise; + + /** Close the connection and release whatever it holds. */ + abstract close(): Promise; async insert( opts: { diff --git a/server/scylla/scyllaDatabase.ts b/server/scylla/scyllaDatabase.ts new file mode 100644 index 000000000..16723a574 --- /dev/null +++ b/server/scylla/scyllaDatabase.ts @@ -0,0 +1,78 @@ +import { createRequire } from 'node:module'; +import type * as CassandraDriver from 'cassandra-driver'; +import { type ClientOptions, type Host as ScyllaHost } from 'cassandra-driver'; + +import { logErrorJson, logJson } from '../utils/logging.js'; +import { type DBDefinition } from './cqlUtils.js'; +import Scylla from './scylla.js'; + +// The otel instrumentation currently intercepts require statements. Support for +// esm is experimental, so we should wait until it is stable; until then the +// driver has to be loaded this way or Scylla calls go untraced. +const require = createRequire(import.meta.url); +const { Client } = require('cassandra-driver') as typeof CassandraDriver; + +/** + * Forwards driver-internal warnings and errors (auth, TLS, connection drops, + * etc.); skips the very chatty `info`/`verbose` levels. + */ +const scyllaLogger = ( + level: 'verbose' | 'info' | 'warning' | 'error', + source: string, + message: string, + furtherInfo?: unknown, +) => { + if (level !== 'warning' && level !== 'error') { + return; + } + const wrapped = new Error(`scylla.${level}: [${source}] ${message}`); + if (furtherInfo instanceof Error) { + wrapped.stack = furtherInfo.stack ?? wrapped.stack; + } + // eslint-disable-next-line no-restricted-syntax + logErrorJson({ + message: `scylla.driver.${level}`, + error: wrapped, + }); +}; + +/** + * A {@link Scylla} backed by a real cluster: owns the driver, the lifecycle + * methods the IoC container needs to open and close it, and the operational + * wiring (log forwarding, cluster-state visibility) that every connection + * wants but that the query layer has no opinion about. + * + * `NoOpScylla` is the counterpart used when `SCYLLA_ENABLED=false`. + */ +export default class ScyllaDatabase extends Scylla { + constructor(options: ClientOptions) { + super(new Client(options)); + + // Surface cluster state changes so reconnect storms are visible in logs. + this.client.on('hostUp', (host: ScyllaHost) => { + // eslint-disable-next-line no-restricted-syntax + logJson(`scylla.hostUp address=${host.address}`); + }); + this.client.on('hostDown', (host: ScyllaHost) => { + // eslint-disable-next-line no-restricted-syntax + logJson(`scylla.hostDown address=${host.address}`); + }); + + this.client.on('log', scyllaLogger); + + // cassandra-driver leaks ~4 HostMap listeners per failed `Client._connect()` + // retry and never recreates the HostMap, so the default cap of 10 trips + // after ~3 failures. Raise it so transient blips don't spam the warning, but + // keep it bounded so a true runaway is still noticeable. + this.client.hosts.setMaxListeners(15); + } + + /** Eagerly connect; idempotent once `connected` is true. */ + async connect() { + return this.client.connect(); + } + + async close() { + return this.client.shutdown(); + } +} From 0eca05e2b588a594fe7bbe6e2169c21f84ba18bc Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 03:12:04 +0200 Subject: [PATCH 08/19] Move data warehouse configuration into config/dataWarehouse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DataWarehouseFactory.createConfigFromEnv()` moves here wholesale, so the factory no longer reads the environment and the config is built once rather than on each of the three warehouse services the container registers. `POSTGRES_HOST`, `POSTGRES_PORT`, `POSTGRES_USERNAME`, `POSTGRES_PASSWORD` and `POSTGRES_DATABASE` were read by the `postgresql` adapter but declared nowhere; they are added to the schema and documented. They configure the warehouse, not the application database, which the comments now say — the names do not. Adapters under `plugins/` stop reading the environment. `clickhouseSettings.ts` keeps its interface and loses its getter, `clickhouseRetry.ts` takes a settings argument, and both adapters receive what they need through the options they already accepted. Neither imports `iocContainer` any more, which `plugins/` being an extension point for community adapters is the point of. Adds `as const` to the six enum declarations that lacked it. Without it `Env.schema.enum` widens to `string`, which is why the old code needed `as DataWarehouseProvider` and `as 'http' | 'https'` — the schema was discarding exactly what those casts asserted back. The casts go, and the provider switch becomes exhaustive enough for a real `assertUnreachable`. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + server/config/dataWarehouse.ts | 112 ++++++++++++++++++ server/graphql/datasources/RuleApi.ts | 8 +- server/iocContainer/index.ts | 33 ++++-- .../adapters/ClickhouseAnalyticsAdapter.ts | 10 +- .../analytics/adapters/clickhouseRetry.ts | 29 ++--- .../warehouse/utils/clickhouseSettings.ts | 31 ++--- server/start/env.ts | 21 +--- .../dataWarehouse/ClickhouseAdapter.ts | 5 +- .../dataWarehouse/DataWarehouseFactory.ts | 94 +++------------ 10 files changed, 193 insertions(+), 151 deletions(-) create mode 100644 server/config/dataWarehouse.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 22fa14708..3966e937e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ For more information about each release including git tags and artifacts, see [R ### Removed +- `postgresql` as a `WAREHOUSE_ADAPTER` / `ANALYTICS_ADAPTER` value, until it is implemented ([#1235](https://github.com/roostorg/coop/pull/1235) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Unused `GRAPHQL_OPAQUE_SCALAR_SECRET` and `LAUNCHDARKLY_SECRET` environment variables ([#1246](https://github.com/roostorg/coop/pull/1246) by [@ThisIsMissEm](https://github.com/ThisIsMissEm)) - Google Cloud Translation API, the `ENGLISH_TRANSLATION` derived field, and `GOOGLE_TRANSLATE_API_KEY` ([#1045](https://github.com/roostorg/coop/pull/1045) by [@julietshen](https://github.com/julietshen)) - `IMAGE_SIMILARITY_SCORE` and `IMAGE_EXACT_MATCH` signal types ([#1043](https://github.com/roostorg/coop/pull/1043) by [@julietshen](https://github.com/julietshen), closes [#686](https://github.com/roostorg/coop/issues/686)) diff --git a/server/config/dataWarehouse.ts b/server/config/dataWarehouse.ts new file mode 100644 index 000000000..a578562cd --- /dev/null +++ b/server/config/dataWarehouse.ts @@ -0,0 +1,112 @@ +import env from '#start/env'; + +import { type ClickhouseInsertRetrySettings } from '../plugins/analytics/adapters/clickhouseRetry.js'; +import { type ClickhouseMemorySettings } from '../plugins/warehouse/utils/clickhouseSettings.js'; +import { type DataWarehouseConfig } from '../storage/dataWarehouse/DataWarehouseFactory.js'; + +/** + * `max_bytes_before_external_*` above this spills to disk instead of failing + * the query. Zero disables the threshold entirely. + */ +const DEFAULT_MAX_BYTES_BEFORE_EXTERNAL = 1_500_000_000; + +const memory: ClickhouseMemorySettings = { + max_bytes_before_external_group_by: String( + env.get( + 'CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_GROUP_BY', + DEFAULT_MAX_BYTES_BEFORE_EXTERNAL, + ), + ), + max_bytes_before_external_sort: String( + env.get( + 'CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_SORT', + DEFAULT_MAX_BYTES_BEFORE_EXTERNAL, + ), + ), + max_threads: env.get('CLICKHOUSE_MAX_THREADS', 2), + max_block_size: String(env.get('CLICKHOUSE_MAX_BLOCK_SIZE', 32768)), +}; + +/** + * ClickHouse over HTTP can reset in-flight connections (remote restart, an + * idle-socket reaper in between), which is transient and worth a retry. + */ +const insertRetry: ClickhouseInsertRetrySettings = { + maxRetries: env.get('CLICKHOUSE_INSERT_MAX_RETRIES', 2), + initialTimeMsBetweenRetries: env.get( + 'CLICKHOUSE_INSERT_RETRY_INITIAL_MS', + 100, + ), + maxTimeMsBetweenRetries: env.get('CLICKHOUSE_INSERT_RETRY_MAX_MS', 1000), +}; + +/** + * Which warehouse backs analytics and reporting. + * + * `DATA_WAREHOUSE_PROVIDER` is the superseded spelling of `WAREHOUSE_ADAPTER` + * and is still honoured. `ANALYTICS_ADAPTER` defaults to whatever the warehouse + * is, so a deployment only sets it to split the two. + */ +const provider = env.get( + 'WAREHOUSE_ADAPTER', + env.get('DATA_WAREHOUSE_PROVIDER', 'clickhouse'), +); + +const analyticsProvider = env.get('ANALYTICS_ADAPTER', provider); + +/** + * Every warehouse Coop can talk to, keyed by the value `WAREHOUSE_ADAPTER` + * takes, with `connection` naming the one in use — the shape `@adonisjs/lucid` + * and `@adonisjs/redis` both use for "several possible backends, one selected". + * + * `satisfies Record` is what makes this exhaustive: the map + * must cover exactly the values the schema permits, so widening the enum is a + * type error here until the connection exists. A `switch` could only have caught + * that at runtime. + * + * `postgresql` is absent deliberately. The factory still carries its branches, + * but they are unimplemented — a no-op warehouse and a dialect that throws — so + * it is not offered as an adapter value until that work lands. + */ +const connections = { + noop: { provider: 'noop' }, + + clickhouse: { + provider: 'clickhouse', + connection: { + host: env.get('CLICKHOUSE_HOST', 'localhost'), + port: env.get('CLICKHOUSE_PORT', 8123), + username: env.get('CLICKHOUSE_USERNAME', 'default'), + password: env.get('CLICKHOUSE_PASSWORD')?.release() ?? '', + database: env.get('CLICKHOUSE_DATABASE', 'default'), + protocol: env.get('CLICKHOUSE_PROTOCOL', 'http'), + }, + pool: { max: env.get('CLICKHOUSE_POOL_SIZE', 10) }, + memory, + insertRetry, + }, +} satisfies Record; + +export default { + connections, + + /** Queries and transactions: the `DataWarehouse` and `DataWarehouseDialect` services. */ + warehouse: { + connection: provider, + + /** + * Bounds the window Rule Insights scans, so a memory-constrained instance + * reads far less than a full year. The client filters further and defaults + * to a one-week view. + */ + ruleInsightsLookbackDays: env.get( + 'CLICKHOUSE_RULE_INSIGHTS_LOOKBACK_DAYS', + 90, + ), + }, + + /** Bulk writes, CDC and logging: the `DataWarehouseAnalytics` service. */ + analytics: { + connection: analyticsProvider, + }, +}; diff --git a/server/graphql/datasources/RuleApi.ts b/server/graphql/datasources/RuleApi.ts index a80e88b56..6f85ef631 100644 --- a/server/graphql/datasources/RuleApi.ts +++ b/server/graphql/datasources/RuleApi.ts @@ -2,12 +2,12 @@ import { type Exception } from '@opentelemetry/api'; import { makeEnumLike } from '@roostorg/coop-types'; +import warehouseConfig from '#config/dataWarehouse'; import { type Kysely } from 'kysely'; import { type JsonObject } from 'type-fest'; import { uid } from 'uid'; import { inject, type Dependencies } from '../../iocContainer/index.js'; -import { safeGetEnvInt } from '../../iocContainer/utils.js'; import { type ActionCountsInput } from '../../services/actionStatisticsService/index.js'; import { type AggregationClause } from '../../services/aggregationsService/index.js'; import { type ConditionSetWithResultAsLogged } from '../../services/analyticsLoggers/index.js'; @@ -676,11 +676,9 @@ class RuleAPI { // all five at once, and bound the window (env-tunable, default 90 days) so // a memory-constrained instance scans far less than a full year. The client // filters further client-side and defaults to a one-week view. - const lookbackDays = safeGetEnvInt( - 'CLICKHOUSE_RULE_INSIGHTS_LOOKBACK_DAYS', - 90, + const startAt = new Date( + Date.now() - warehouseConfig.warehouse.ruleInsightsLookbackDays * DAY_MS, ); - const startAt = new Date(Date.now() - lookbackDays * DAY_MS); const runSafely = async ( fn: () => Promise, diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index fbb414391..14e9793ea 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -3,6 +3,7 @@ import Bottle from '@ethanresnick/bottlejs'; import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; import databaseConfig from '#config/database'; +import warehouseConfig from '#config/dataWarehouse'; import redisConfig, { type RedisConnection } from '#config/redis'; import scyllaConfig from '#config/scylla'; import IORedis, { type Cluster } from 'ioredis'; @@ -508,25 +509,33 @@ export default async function getBottle( // - 'DataWarehouse' - Core queries and transactions // - 'DataWarehouseDialect' - Type-safe Kysely queries // - 'DataWarehouseAnalytics' - Bulk writes, CDC, logging + // The config names which connection each of these uses; turning a name into a + // connection is wiring, so it belongs here rather than in the config. + function getWarehouseConfig() { + return warehouseConfig.connections[warehouseConfig.warehouse.connection]; + } + + function getAnalyticsConfig() { + return warehouseConfig.connections[warehouseConfig.analytics.connection]; + } + bottle.factory('DataWarehouse', () => { - const config = DataWarehouseFactory.createConfigFromEnv(); - const dataWarehouse = DataWarehouseFactory.createDataWarehouse(config); + const dataWarehouse = + DataWarehouseFactory.createDataWarehouse(getWarehouseConfig()); dataWarehouse.start(); return dataWarehouse; }); - bottle.factory('DataWarehouseDialect', () => { - const config = DataWarehouseFactory.createConfigFromEnv(); - return DataWarehouseFactory.createKyselyDialect(config); - }); + bottle.factory('DataWarehouseDialect', () => + DataWarehouseFactory.createKyselyDialect(getWarehouseConfig()), + ); - bottle.factory('DataWarehouseAnalytics', (container) => { - const config = DataWarehouseFactory.createConfigFromEnv(); - return DataWarehouseFactory.createAnalyticsAdapter( - config, + bottle.factory('DataWarehouseAnalytics', (container) => + DataWarehouseFactory.createAnalyticsAdapter( + getAnalyticsConfig(), container.DataWarehouseDialect, - ); - }); + ), + ); bottle.factory('ActionStatisticsAdapter', (container) => { return new ClickhouseActionStatisticsAdapter( diff --git a/server/plugins/analytics/adapters/ClickhouseAnalyticsAdapter.ts b/server/plugins/analytics/adapters/ClickhouseAnalyticsAdapter.ts index 7cbd8f2f2..b68ca4e0c 100644 --- a/server/plugins/analytics/adapters/ClickhouseAnalyticsAdapter.ts +++ b/server/plugins/analytics/adapters/ClickhouseAnalyticsAdapter.ts @@ -3,7 +3,7 @@ import { createClient, type ClickHouseClient } from '@clickhouse/client'; import { jsonStringify, tryJsonParse } from '../../../utils/encoding.js'; import { logErrorJson } from '../../../utils/logging.js'; import type SafeTracer from '../../../utils/SafeTracer.js'; -import { getClickhouseMemorySettings } from '../../warehouse/utils/clickhouseSettings.js'; +import { type ClickhouseMemorySettings } from '../../warehouse/utils/clickhouseSettings.js'; import { formatClickhouseQuery } from '../../warehouse/utils/clickhouseSql.js'; import type { IAnalyticsAdapter } from '../IAnalyticsAdapter.js'; import { @@ -14,6 +14,7 @@ import { import { isTransientNetworkError, withClickhouseInsertRetries, + type ClickhouseInsertRetrySettings, } from './clickhouseRetry.js'; export interface ClickhouseAnalyticsConnection { @@ -27,6 +28,10 @@ export interface ClickhouseAnalyticsConnection { export interface ClickhouseAnalyticsAdapterOptions { connection: ClickhouseAnalyticsConnection; + /** Per-query memory limits sent with every statement. */ + memory: ClickhouseMemorySettings; + /** How hard to retry a failed insert. */ + retry: ClickhouseInsertRetrySettings; tracer?: SafeTracer; defaultBatchSize?: number; } @@ -104,7 +109,7 @@ export class ClickhouseAnalyticsAdapter implements IAnalyticsAdapter { ...(password ? { password } : {}), database: options.connection.database, clickhouse_settings: { - ...getClickhouseMemorySettings(), + ...options.memory, }, }); @@ -118,6 +123,7 @@ export class ClickhouseAnalyticsAdapter implements IAnalyticsAdapter { }) => { await this.client.insert({ table, values, format: 'JSONEachRow' }); }, + options.retry, ); } diff --git a/server/plugins/analytics/adapters/clickhouseRetry.ts b/server/plugins/analytics/adapters/clickhouseRetry.ts index 0eba7a823..6f3d64159 100644 --- a/server/plugins/analytics/adapters/clickhouseRetry.ts +++ b/server/plugins/analytics/adapters/clickhouseRetry.ts @@ -1,9 +1,16 @@ -import { - safeGetEnvInt, - safeGetEnvNonNegativeInt, -} from '../../../iocContainer/utils.js'; import { withRetries } from '../../../utils/misc.js'; +/** + * How hard to retry a failed insert. Supplied by the caller rather than read + * from the environment, so adapters under `plugins/` stay independent of how a + * given deployment sources its configuration. + */ +export interface ClickhouseInsertRetrySettings { + maxRetries: number; + initialTimeMsBetweenRetries: number; + maxTimeMsBetweenRetries: number; +} + // Network errors we'll retry on. ClickHouse over HTTP can RST in-flight // connections (remote restart, idle-socket reaper between us and CH, etc.); // these are transient and worth one or two retries before giving up. @@ -27,20 +34,10 @@ export function isTransientNetworkError(err: unknown): boolean { export function withClickhouseInsertRetries( fn: (this: void, ...args: Args) => Promise, + retry: ClickhouseInsertRetrySettings, ): (...args: Args) => Promise { return withRetries( - { - maxRetries: safeGetEnvNonNegativeInt('CLICKHOUSE_INSERT_MAX_RETRIES', 2), - initialTimeMsBetweenRetries: safeGetEnvInt( - 'CLICKHOUSE_INSERT_RETRY_INITIAL_MS', - 100, - ), - maxTimeMsBetweenRetries: safeGetEnvInt( - 'CLICKHOUSE_INSERT_RETRY_MAX_MS', - 1000, - ), - isRetryableError: isTransientNetworkError, - }, + { ...retry, isRetryableError: isTransientNetworkError }, fn, ); } diff --git a/server/plugins/warehouse/utils/clickhouseSettings.ts b/server/plugins/warehouse/utils/clickhouseSettings.ts index e27792a71..68adb27aa 100644 --- a/server/plugins/warehouse/utils/clickhouseSettings.ts +++ b/server/plugins/warehouse/utils/clickhouseSettings.ts @@ -1,29 +1,14 @@ -import { safeGetEnvInt } from '../../../iocContainer/utils.js'; - -const DEFAULT_MAX_BYTES_BEFORE_EXTERNAL = 1_500_000_000; - +/** + * Per-query memory limits sent to ClickHouse with each statement. + * + * Values come from `config/dataWarehouse.ts` and are passed in by whoever + * constructs the adapter. Adapters under `plugins/` deliberately read no + * environment themselves, so a community-published adapter never has to know + * how this deployment sources its configuration. + */ export interface ClickhouseMemorySettings { max_bytes_before_external_group_by: string; max_bytes_before_external_sort: string; max_threads: number; max_block_size: string; } - -export function getClickhouseMemorySettings(): ClickhouseMemorySettings { - return { - max_bytes_before_external_group_by: String( - safeGetEnvInt( - 'CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_GROUP_BY', - DEFAULT_MAX_BYTES_BEFORE_EXTERNAL, - ), - ), - max_bytes_before_external_sort: String( - safeGetEnvInt( - 'CLICKHOUSE_MAX_BYTES_BEFORE_EXTERNAL_SORT', - DEFAULT_MAX_BYTES_BEFORE_EXTERNAL, - ), - ), - max_threads: safeGetEnvInt('CLICKHOUSE_MAX_THREADS', 2), - max_block_size: String(safeGetEnvInt('CLICKHOUSE_MAX_BLOCK_SIZE', 32768)), - }; -} diff --git a/server/start/env.ts b/server/start/env.ts index 6a23aacb9..a0d9d4f19 100644 --- a/server/start/env.ts +++ b/server/start/env.ts @@ -38,7 +38,7 @@ const env = await Env.create(new URL('./', import.meta.url), { NOREPLY_EMAIL: Env.schema.string.optional({ format: 'email' }), SUPPORT_EMAIL: Env.schema.string.optional({ format: 'email' }), TEAM_EMAIL: Env.schema.string.optional({ format: 'email' }), - EMAIL_TRANSPORT: Env.schema.enum.optional(['console']), + EMAIL_TRANSPORT: Env.schema.enum.optional(['console'] as const), // Postgresql configuration: DATABASE_HOST: Env.schema.string({ format: 'host' }), @@ -80,29 +80,20 @@ const env = await Env.create(new URL('./', import.meta.url), { GRAPHQL_MAX_DEPTH: Env.schema.integer.positive.optional(), // Default to `clickhouse`; ANALYTICS_ADAPTER falls back to WAREHOUSE_ADAPTER. - WAREHOUSE_ADAPTER: Env.schema.enum.optional([ - 'noop', - 'clickhouse', - 'postgresql', - ]), - ANALYTICS_ADAPTER: Env.schema.enum.optional([ - 'noop', - 'clickhouse', - 'postgresql', - ]), + WAREHOUSE_ADAPTER: Env.schema.enum.optional(['noop', 'clickhouse'] as const), + ANALYTICS_ADAPTER: Env.schema.enum.optional(['noop', 'clickhouse'] as const), // Legacy: use WAREHOUSE_ADAPTER and ANALYTICS_ADAPTER instead: DATA_WAREHOUSE_PROVIDER: Env.schema.enum.optional([ 'noop', 'clickhouse', - 'postgresql', - ]), + ] as const), // Clickhouse settings, only used when WAREHOUSE_ADAPTER or ANALYTICS_ADAPTER is clickhouse: CLICKHOUSE_HOST: Env.schema.string.optional({ format: 'host' }), CLICKHOUSE_PORT: Env.schema.integer.positive.optional(), CLICKHOUSE_USERNAME: Env.schema.string.optional(), CLICKHOUSE_PASSWORD: Env.schema.secret.optional(), CLICKHOUSE_DATABASE: Env.schema.string.optional(), - CLICKHOUSE_PROTOCOL: Env.schema.enum.optional(['https', 'http']), + CLICKHOUSE_PROTOCOL: Env.schema.enum.optional(['https', 'http'] as const), CLICKHOUSE_POOL_SIZE: Env.schema.integer.positive.optional(), // Zero means no retries and no delay respectively, so these allow it. CLICKHOUSE_INSERT_MAX_RETRIES: Env.schema.integer.nonNegative.optional(), @@ -143,7 +134,7 @@ const env = await Env.create(new URL('./', import.meta.url), { // CyberTipline credentials configured in Settings → NCMEC are production // credentials issued by NCMEC and your integration has been approved for live // reporting. - NCMEC_ENV: Env.schema.enum.optional(['production', 'test']), + NCMEC_ENV: Env.schema.enum.optional(['production', 'test'] as const), NCMEC_DEBUG: Env.schema.boolean.optional(), // Debugging: diff --git a/server/storage/dataWarehouse/ClickhouseAdapter.ts b/server/storage/dataWarehouse/ClickhouseAdapter.ts index ee28bf526..1073cab7e 100644 --- a/server/storage/dataWarehouse/ClickhouseAdapter.ts +++ b/server/storage/dataWarehouse/ClickhouseAdapter.ts @@ -14,7 +14,7 @@ import { type TransactionSettings, } from 'kysely'; -import { getClickhouseMemorySettings } from '../../plugins/warehouse/utils/clickhouseSettings.js'; +import { type ClickhouseMemorySettings } from '../../plugins/warehouse/utils/clickhouseSettings.js'; import { formatClickhouseQuery } from '../../plugins/warehouse/utils/clickhouseSql.js'; import type { DataWarehousePoolSettings, @@ -118,6 +118,7 @@ export class ClickhouseKyselyAdapter implements IDataWarehouseDialect { constructor( connectionSettings: ClickhouseConnectionSettings, + memorySettings: ClickhouseMemorySettings, _poolSettings?: DataWarehousePoolSettings, ) { const protocol = connectionSettings.protocol ?? 'http'; @@ -134,7 +135,7 @@ export class ClickhouseKyselyAdapter implements IDataWarehouseDialect { database: connectionSettings.database, clickhouse_settings: { allow_experimental_object_type: 1, - ...getClickhouseMemorySettings(), + ...memorySettings, }, }); diff --git a/server/storage/dataWarehouse/DataWarehouseFactory.ts b/server/storage/dataWarehouse/DataWarehouseFactory.ts index 5f85fd1ba..ff99bba17 100644 --- a/server/storage/dataWarehouse/DataWarehouseFactory.ts +++ b/server/storage/dataWarehouse/DataWarehouseFactory.ts @@ -5,6 +5,7 @@ /* eslint-disable max-classes-per-file */ import { type Kysely } from 'kysely'; +import { type ClickhouseInsertRetrySettings } from '../../plugins/analytics/adapters/clickhouseRetry.js'; import { ClickhouseAnalyticsAdapter as ClickhouseAnalyticsPlugin, NoOpAnalyticsAdapter, @@ -15,6 +16,7 @@ import { NoOpWarehouseAdapter, type IWarehouseAdapter, } from '../../plugins/warehouse/index.js'; +import { type ClickhouseMemorySettings } from '../../plugins/warehouse/utils/clickhouseSettings.js'; import { assertUnreachable } from '../../utils/misc.js'; import type SafeTracer from '../../utils/SafeTracer.js'; import { @@ -43,8 +45,6 @@ import { PostgresAnalyticsAdapter } from './PostgresAnalyticsAdapter.js'; */ export type DataWarehouseProvider = 'clickhouse' | 'postgresql' | 'noop'; -export type AnalyticsProvider = 'clickhouse' | 'postgresql' | 'noop'; - // Re-export the interface provider type for external use export type { IDataWarehouseProvider }; @@ -53,7 +53,10 @@ export type DataWarehouseConfig = provider: 'clickhouse'; connection: ClickhouseConnectionSettings; pool?: DataWarehousePoolSettings; - analyticsProvider?: AnalyticsProvider; + /** Per-query memory limits, passed to every adapter this config builds. */ + memory: ClickhouseMemorySettings; + /** Retry policy for inserts, used by the analytics adapter. */ + insertRetry: ClickhouseInsertRetrySettings; } | { provider: 'postgresql'; @@ -65,11 +68,9 @@ export type DataWarehouseConfig = database: string; }; pool?: DataWarehousePoolSettings; - analyticsProvider?: AnalyticsProvider; } | { provider: 'noop'; - analyticsProvider?: AnalyticsProvider; }; class NoOpKyselyDialect implements IDataWarehouseDialect { @@ -240,7 +241,11 @@ export class DataWarehouseFactory { ): IDataWarehouseDialect { switch (config.provider) { case 'clickhouse': - return new ClickhouseKyselyAdapter(config.connection, config.pool); + return new ClickhouseKyselyAdapter( + config.connection, + config.memory, + config.pool, + ); case 'postgresql': throw new Error('PostgreSQL Kysely dialect not yet implemented'); case 'noop': @@ -263,21 +268,16 @@ export class DataWarehouseFactory { config: DataWarehouseConfig, dialect?: IDataWarehouseDialect, ): IDataWarehouseAnalytics { - const analyticsProvider = config.analyticsProvider ?? config.provider; - - switch (analyticsProvider) { + switch (config.provider) { case 'noop': return new AnalyticsAdapterBridge('noop', new NoOpAnalyticsAdapter()); case 'clickhouse': - if (config.provider !== 'clickhouse') { - throw new Error( - 'Clickhouse analytics provider requires the clickhouse warehouse configuration.', - ); - } return new AnalyticsAdapterBridge( 'clickhouse', new ClickhouseAnalyticsPlugin({ connection: config.connection, + memory: config.memory, + retry: config.insertRetry, }), ); case 'postgresql': { @@ -289,68 +289,10 @@ export class DataWarehouseFactory { } default: return assertUnreachable( - analyticsProvider, - `Unknown analytics provider: ${analyticsProvider as string}`, - ); - } - } - - /** - * Create configuration from environment variables - */ - - static createConfigFromEnv(): DataWarehouseConfig { - const provider = (process.env.WAREHOUSE_ADAPTER ?? - process.env.DATA_WAREHOUSE_PROVIDER ?? - 'clickhouse') as DataWarehouseProvider; - const analyticsProvider = (process.env.ANALYTICS_ADAPTER ?? - provider) as AnalyticsProvider; - - switch (provider) { - case 'noop': - return { - provider: 'noop', - analyticsProvider, - }; - case 'clickhouse': - return { - provider: 'clickhouse', - analyticsProvider, - connection: { - host: process.env.CLICKHOUSE_HOST ?? 'localhost', - port: process.env.CLICKHOUSE_PORT - ? parseInt(process.env.CLICKHOUSE_PORT) - : 8123, - username: process.env.CLICKHOUSE_USERNAME ?? 'default', - password: process.env.CLICKHOUSE_PASSWORD ?? '', - database: process.env.CLICKHOUSE_DATABASE ?? 'default', - protocol: (process.env.CLICKHOUSE_PROTOCOL ?? 'http') as - 'http' | 'https', - }, - pool: { - max: process.env.CLICKHOUSE_POOL_SIZE - ? parseInt(process.env.CLICKHOUSE_POOL_SIZE) - : 10, - }, - }; - case 'postgresql': - return { - provider: 'postgresql', - analyticsProvider, - connection: { - host: process.env.POSTGRES_HOST ?? 'localhost', - port: process.env.POSTGRES_PORT - ? parseInt(process.env.POSTGRES_PORT) - : undefined, - username: process.env.POSTGRES_USERNAME ?? 'postgres', - password: process.env.POSTGRES_PASSWORD ?? '', - database: process.env.POSTGRES_DATABASE ?? 'postgres', - }, - }; - default: - return assertUnreachable( - provider, - `Unknown data warehouse provider: ${provider as string}`, + config, + `Unknown analytics provider: ${ + (config as DataWarehouseConfig).provider + }`, ); } } From e6098ced415ed5556cf9122e0181e8eb0697f7fd Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 03:51:24 +0200 Subject: [PATCH 09/19] Move email configuration into config/email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three sender addresses, the transport name and the SendGrid key move behind `config/email.ts`, and `notificationFormatter` stops repeating the `support@example.com` default that `sendEmailService` already had. The `NODE_ENV !== 'development'` guard on the console transport becomes `appConfig.inDev`. `SENDGRID_API_KEY` is a `Secret`, so it is released where it is handed to the SendGrid client rather than held in plaintext by the config. Deliberately not changed: `makeSendEmail` still chooses its transport by falling through — injected SES client, then `EMAIL_TRANSPORT=console`, then SendGrid if an API key happens to be set, then SES. Making that an explicit selection, and replacing the console transport with SMTP against a local mail catcher, is tracked separately. Co-Authored-By: Claude Opus 5 (1M context) --- server/config/email.ts | 24 +++++++++++++++++++ .../notificationFormatter.ts | 3 ++- .../sendEmailService/sendEmailService.ts | 16 +++++++------ 3 files changed, 35 insertions(+), 8 deletions(-) create mode 100644 server/config/email.ts diff --git a/server/config/email.ts b/server/config/email.ts new file mode 100644 index 000000000..5478bc4f0 --- /dev/null +++ b/server/config/email.ts @@ -0,0 +1,24 @@ +import env from '#start/env'; + +/** + * Outbound email. + * + * `transport` is only half a selector today: `makeSendEmail` falls through from + * an explicitly-injected SES client, to `console`, to SendGrid if an API key + * happens to be set, to SES. Making the choice explicit — and replacing the + * `console` transport with SMTP against a local mail catcher — is left to the + * change that adds an SMTP transport, so this module only moves the reads. + */ +export default { + transport: env.get('EMAIL_TRANSPORT'), + + /** Only consulted when no SES client is injected and `transport` is unset. */ + sendgridApiKey: env.get('SENDGRID_API_KEY'), + + /** The addresses Coop sends as. */ + addresses: { + noReply: env.get('NOREPLY_EMAIL', 'noreply@example.com'), + support: env.get('SUPPORT_EMAIL', 'support@example.com'), + team: env.get('TEAM_EMAIL', 'team@example.com'), + }, +}; diff --git a/server/services/notificationsService/notificationFormatter.ts b/server/services/notificationsService/notificationFormatter.ts index c70fb01f3..d4905d44e 100644 --- a/server/services/notificationsService/notificationFormatter.ts +++ b/server/services/notificationsService/notificationFormatter.ts @@ -1,3 +1,4 @@ +import emailConfig from '#config/email'; import { type ReadonlyDeep } from 'type-fest'; import { @@ -33,7 +34,7 @@ export function formatNotification( secondToLastPeriodPassRate, } = data; - const supportEmail = process.env.SUPPORT_EMAIL ?? 'support@example.com'; + const supportEmail = emailConfig.addresses.support; const rateDetails = lastPeriodPassRate && secondToLastPeriodPassRate ? `In the past hour, the rule's pass rate went up${ diff --git a/server/services/sendEmailService/sendEmailService.ts b/server/services/sendEmailService/sendEmailService.ts index 0384904c9..9a08872da 100644 --- a/server/services/sendEmailService/sendEmailService.ts +++ b/server/services/sendEmailService/sendEmailService.ts @@ -1,10 +1,12 @@ import { SendEmailCommand, SESClient } from '@aws-sdk/client-ses'; import sgMail from '@sendgrid/mail'; +import appConfig from '#config/app'; +import emailConfig from '#config/email'; export const CoopEmailAddress = { - NoReply: process.env.NOREPLY_EMAIL ?? 'noreply@example.com', - Support: process.env.SUPPORT_EMAIL ?? 'support@example.com', - Team: process.env.TEAM_EMAIL ?? 'team@example.com', + NoReply: emailConfig.addresses.noReply, + Support: emailConfig.addresses.support, + Team: emailConfig.addresses.team, } as const; export type CoopEmailAddress = @@ -80,7 +82,7 @@ export function makeSendEmailViaSendGrid(apiKey: string) { } export function makeSendEmailViaConsole() { - if (process.env.NODE_ENV !== 'development') { + if (!appConfig.inDev) { throw new Error( 'EMAIL_TRANSPORT=console is only available when NODE_ENV=development', ); @@ -100,13 +102,13 @@ const makeSendEmail = (clientOrContainer?: SESClient | unknown) => { return makeSendEmailViaSES(clientOrContainer); } - if (process.env.EMAIL_TRANSPORT === 'console') { + if (emailConfig.transport === 'console') { return makeSendEmailViaConsole(); } - const sendGridApiKey = process.env.SENDGRID_API_KEY; + const sendGridApiKey = emailConfig.sendgridApiKey; if (sendGridApiKey) { - return makeSendEmailViaSendGrid(sendGridApiKey); + return makeSendEmailViaSendGrid(sendGridApiKey.release()); } return makeSendEmailViaSES(new SESClient({})); From 9df59551c957b4f77bd60b437f1964c1bbdf273e Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 04:02:38 +0200 Subject: [PATCH 10/19] Fix a literal NUL byte making ncmecReporting.ts unsearchable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dedup key for reported media used a raw NUL character as its separator, written as an actual 0x00 byte in the source rather than as an escape. One such byte is enough for `file` to classify the source as `data` rather than text, and tools that skip binary files then skip it *silently* — `grep -r` across the repo returned no matches from this 81KB file at all, with no indication that it had been excluded. That is how an import of `ncmecDebugEnabled` here went unnoticed while its definition was being removed. The escape produces the same character, so the dedup key is unchanged; the file is simply text again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/services/ncmecService/ncmecReporting.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/services/ncmecService/ncmecReporting.ts b/server/services/ncmecService/ncmecReporting.ts index c5e388b72..648f96a18 100644 --- a/server/services/ncmecService/ncmecReporting.ts +++ b/server/services/ncmecService/ncmecReporting.ts @@ -621,7 +621,7 @@ export function toOriginalFileHashes(opts: { const trimmedAlgorithm = algorithm.trim(); if (trimmedHash === '' || trimmedAlgorithm === '') return; const hashType = trimmedAlgorithm.toUpperCase(); - const key = `${hashType}${trimmedHash}`; + const key = `${hashType}\u0000${trimmedHash}`; if (seen.has(key)) return; seen.add(key); result.push({ _text: trimmedHash, _attributes: { hashType } }); From 0a308c9cb6d02aa410aed2555504b46c6311f683 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 04:03:02 +0200 Subject: [PATCH 11/19] Move NCMEC configuration into config/ncmec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four call sites each derived `NCMEC_ENV !== 'production'` independently — the IoC container, the retry helper, the retry job and the reporting service. That value decides whether submissions go to the CyberTipline or to the sandbox at exttest.cybertip.org, so having it computed in four places was a poor property for it to have. It is now derived once, in `config/ncmec.ts`, with the reasoning next to it. `ncmecDebugEnabled()` goes away: it combined `NCMEC_DEBUG` with a not-in-production check, which is what `ncmecConfig.debug` now is. The `isTest` parameter threaded through the reporting service is deliberately left alone. Eight of its methods take it and it is forwarded twenty-one times, which does look like configuration wearing a parameter's clothes — but an explicit argument keeps "is this a real CyberTipline report?" visible in every signature that participates, and testable without touching the environment. Collapsing it is a decision about that service's API, not about where env vars are read. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/config/ncmec.ts | 29 +++++++++++++++++++ server/iocContainer/index.ts | 12 ++------ server/services/ncmecService/ncmecDebug.ts | 16 ++++------ .../services/ncmecService/ncmecReporting.ts | 9 ++---- .../ncmecService/retryNcmecSubmission.ts | 8 +++-- .../RetryFailedNcmecDecisionsJob.ts | 6 ++-- 6 files changed, 50 insertions(+), 30 deletions(-) create mode 100644 server/config/ncmec.ts diff --git a/server/config/ncmec.ts b/server/config/ncmec.ts new file mode 100644 index 000000000..7d4484e33 --- /dev/null +++ b/server/config/ncmec.ts @@ -0,0 +1,29 @@ +import appConfig from '#config/app'; +import env from '#start/env'; + +/** + * NCMEC CyberTipline reporting. + * + * `isTest` selects the endpoint submissions are routed to. Anything other than + * `NCMEC_ENV=production` — including being unset — sends to + * https://exttest.cybertip.org, the NCMEC sandbox, where reports are discarded. + * Operators are responsible for matching this to whether the credentials + * configured in Settings → NCMEC are production or test credentials issued by + * NCMEC. + * + * Derived once here because getting it wrong in either direction is serious: + * live reports sent to the sandbox are silently discarded, and test reports sent + * to production are filed as real CyberTipline reports. + */ +const isTest = env.get('NCMEC_ENV') !== 'production'; + +export default { + isTest, + + /** + * Opt-in debug logs and XML/JSON dumps for submissions. Also gated on not + * being in production, since the dumps contain reportable content and must + * not be written in a shared environment. Never includes credentials. + */ + debug: env.get('NCMEC_DEBUG', false) && !appConfig.inProduction, +}; diff --git a/server/iocContainer/index.ts b/server/iocContainer/index.ts index 14e9793ea..bacef3fc2 100644 --- a/server/iocContainer/index.ts +++ b/server/iocContainer/index.ts @@ -4,6 +4,7 @@ import opentelemetry from '@opentelemetry/api'; import { type ItemIdentifier } from '@roostorg/coop-types'; import databaseConfig from '#config/database'; import warehouseConfig from '#config/dataWarehouse'; +import ncmecConfig from '#config/ncmec'; import redisConfig, { type RedisConnection } from '#config/redis'; import scyllaConfig from '#config/scylla'; import IORedis, { type Cluster } from 'ioredis'; @@ -1038,16 +1039,9 @@ export default async function getBottle( getItemTypeEventuallyConsistent: container.getItemTypeEventuallyConsistent, }); - // Submissions go to the NCMEC test endpoint - // (exttest.cybertip.org) unless the deployment is explicitly - // configured for production via NCMEC_ENV=production. Operators - // are responsible for matching this to whether the credentials - // configured in Settings → NCMEC are production or test - // credentials issued by NCMEC. - const isTest = process.env.NCMEC_ENV !== 'production'; await container.NcmecService.submitReport( reportParams, - isTest, + ncmecConfig.isTest, ); const actionAndPolicy = await container.NcmecService.getNCMECActionsToRunAndPolicies( @@ -1062,7 +1056,7 @@ export default async function getBottle( actionAndPolicy != null && actionAndPolicy.actionsToRunIds != null && isNonEmptyArray(decisionActions) && - !isTest + !ncmecConfig.isTest ) { await publishActions({ decisionActions, diff --git a/server/services/ncmecService/ncmecDebug.ts b/server/services/ncmecService/ncmecDebug.ts index 70a26fcb5..11ae01cca 100644 --- a/server/services/ncmecService/ncmecDebug.ts +++ b/server/services/ncmecService/ncmecDebug.ts @@ -1,20 +1,16 @@ +import ncmecConfig from '#config/ncmec'; + import { jsonStringify } from '../../utils/encoding.js'; -// Opt-in debug logs + XML/JSON dumps for NCMEC submissions. Gated on -// `NCMEC_DEBUG=1` and `NODE_ENV !== 'production'` so we cannot leak +// Opt-in debug logs + XML/JSON dumps for NCMEC submissions. `ncmecConfig.debug` +// requires `NCMEC_DEBUG` *and* a non-production environment, so we cannot leak // reportable content in shared environments. Never log credentials. -export function ncmecDebugEnabled(): boolean { - return ( - process.env.NCMEC_DEBUG === '1' && process.env.NODE_ENV !== 'production' - ); -} - export function ncmecDebugLog( event: string, fields: Record, ): void { - if (!ncmecDebugEnabled()) { + if (!ncmecConfig.debug) { return; } // eslint-disable-next-line no-console @@ -25,7 +21,7 @@ export async function ncmecDebugDump( filename: string, contents: string, ): Promise { - if (!ncmecDebugEnabled()) { + if (!ncmecConfig.debug) { return; } try { diff --git a/server/services/ncmecService/ncmecReporting.ts b/server/services/ncmecService/ncmecReporting.ts index 648f96a18..6a2b51186 100644 --- a/server/services/ncmecService/ncmecReporting.ts +++ b/server/services/ncmecService/ncmecReporting.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines */ import type { Exception } from '@opentelemetry/api'; import { makeEnumLike, type ItemIdentifier } from '@roostorg/coop-types'; +import ncmecEnvConfig from '#config/ncmec'; import _Ajv from 'ajv'; import { sql, type Kysely } from 'kysely'; import _ from 'lodash'; @@ -29,11 +30,7 @@ import { type FormDataLikeWithStreams, } from '../networkingService/index.js'; import { type NcmecReportingServicePg } from './dbTypes.js'; -import { - ncmecDebugDump, - ncmecDebugEnabled, - ncmecDebugLog, -} from './ncmecDebug.js'; +import { ncmecDebugDump, ncmecDebugLog } from './ncmecDebug.js'; import { summarizeNcmecErrorForReviewer } from './ncmecReviewerErrors.js'; export const NCMECEvent = makeEnumLike([ @@ -485,7 +482,7 @@ export function summarizeCyberTipFailure( if (responseDescription != null) { parts.push(`responseDescription=${responseDescription}`); } - if (ncmecDebugEnabled()) { + if (ncmecEnvConfig.debug) { let snippet: string; try { const serialized = jsonStringify(body ?? null); diff --git a/server/services/ncmecService/retryNcmecSubmission.ts b/server/services/ncmecService/retryNcmecSubmission.ts index 83e6ef127..0e68ae795 100644 --- a/server/services/ncmecService/retryNcmecSubmission.ts +++ b/server/services/ncmecService/retryNcmecSubmission.ts @@ -1,3 +1,5 @@ +import ncmecConfig from '#config/ncmec'; + import { type Dependencies } from '../../iocContainer/index.js'; import { type ManualReviewToolService } from '../manualReviewToolService/manualReviewToolService.js'; import { @@ -104,11 +106,13 @@ export async function retryNcmecSubmission( return { kind: 'permanent_error', error }; } - const isTest = process.env.NCMEC_ENV !== 'production'; // submitReport owns `ncmec_reports_errors` writes via `jobId` so retries // always update retry_count/last_error. Don't double-write here. try { - const result = await deps.ncmecReporting.submitReport(reportParams, isTest); + const result = await deps.ncmecReporting.submitReport( + reportParams, + ncmecConfig.isTest, + ); if (result === 'SUCCESS') { return { kind: 'success' }; } diff --git a/server/workers_jobs/RetryFailedNcmecDecisionsJob.ts b/server/workers_jobs/RetryFailedNcmecDecisionsJob.ts index 0cdb7d232..bd9f2ae62 100644 --- a/server/workers_jobs/RetryFailedNcmecDecisionsJob.ts +++ b/server/workers_jobs/RetryFailedNcmecDecisionsJob.ts @@ -1,3 +1,4 @@ +import ncmecConfig from '#config/ncmec'; import _ from 'lodash'; import { v1 as uuidv1 } from 'uuid'; @@ -118,10 +119,9 @@ export default inject( getItemTypeEventuallyConsistent, }); submitReportInvoked = true; - const isTest = process.env.NCMEC_ENV !== 'production'; const reportResult = await ncmecService.submitReport( reportParams, - isTest, + ncmecConfig.isTest, ); if ( reportResult === 'UNSUPPORTED_ORG' || @@ -135,7 +135,7 @@ export default inject( if ( actionAndPolicy != null && actionAndPolicy.actionsToRunIds != null && - !isTest + !ncmecConfig.isTest ) { const actions = await moderationConfigService.getActions({ orgId, From b233b857d02171cb8fd2c0323d0e523e37591b61 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:49:15 +0200 Subject: [PATCH 12/19] Move integrations configuration into config/integrations `INTEGRATIONS_CONFIG_PATH`, `GOOGLE_PLACES_API_KEY` and `HMA_SERVICE_URL` were each read where they were used, with their defaults spelled at the call site. `GOOGLE_PLACES_API_KEY` is a `Secret`, so it is released where it is handed to the Google client. Note this preserves an existing oddity rather than fixing it: when the key is unset, `String(undefined)` sends the literal string "undefined" as the API key, exactly as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/config/integrations.ts | 20 +++++++++++++++++++ server/services/hmaService/index.ts | 4 ++-- .../loadIntegrationsConfig.ts | 3 ++- .../placesApiService/placesApiService.ts | 3 ++- 4 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 server/config/integrations.ts diff --git a/server/config/integrations.ts b/server/config/integrations.ts new file mode 100644 index 000000000..362b65d8b --- /dev/null +++ b/server/config/integrations.ts @@ -0,0 +1,20 @@ +import env from '#start/env'; + +/** + * Third-party services Coop talks out to, and where the adopter's integrations + * manifest lives. + */ +export default { + /** + * Where to find the adopter's integrations config file. Relative paths are + * resolved against the working directory; when unset, the loader falls back to + * `integrations.config.json` in the working directory and then in `server/`, + * since `npm run start` runs from the repo root. + */ + configPath: env.get('INTEGRATIONS_CONFIG_PATH'), + + googlePlacesApiKey: env.get('GOOGLE_PLACES_API_KEY'), + + /** Hasher-Matcher-Actioner, for perceptual hash matching. */ + hmaServiceUrl: env.get('HMA_SERVICE_URL', 'http://localhost:9876/'), +}; diff --git a/server/services/hmaService/index.ts b/server/services/hmaService/index.ts index be81a152d..1b53d2e13 100644 --- a/server/services/hmaService/index.ts +++ b/server/services/hmaService/index.ts @@ -1,4 +1,5 @@ /* eslint-disable max-lines */ +import integrationsConfig from '#config/integrations'; import { type JsonValue } from 'type-fest'; import { FormData } from 'undici'; @@ -185,8 +186,7 @@ export class HmaService { private readonly fetchHTTP: Dependencies['fetchHTTP'], kyselyPg: Dependencies['KyselyPg'], ) { - this.hmaServiceUrl = - process.env.HMA_SERVICE_URL ?? 'http://localhost:9876/'; + this.hmaServiceUrl = integrationsConfig.hmaServiceUrl; this.hashBankService = new HashBankService(kyselyPg); } diff --git a/server/services/integrationRegistry/loadIntegrationsConfig.ts b/server/services/integrationRegistry/loadIntegrationsConfig.ts index c7661bfd1..f27174af6 100644 --- a/server/services/integrationRegistry/loadIntegrationsConfig.ts +++ b/server/services/integrationRegistry/loadIntegrationsConfig.ts @@ -6,11 +6,12 @@ import { existsSync, readFileSync } from 'fs'; import path from 'path'; import type { CoopIntegrationsConfig } from '@roostorg/coop-types'; +import integrationsConfig from '#config/integrations'; import { jsonParse, type JsonOf } from '../../utils/encoding.js'; function getConfigPath(): string { - const envPath = process.env.INTEGRATIONS_CONFIG_PATH; + const envPath = integrationsConfig.configPath; if (envPath != null && envPath !== '') { return path.isAbsolute(envPath) ? envPath diff --git a/server/services/placesApiService/placesApiService.ts b/server/services/placesApiService/placesApiService.ts index 97194e9c4..407a711d5 100644 --- a/server/services/placesApiService/placesApiService.ts +++ b/server/services/placesApiService/placesApiService.ts @@ -1,4 +1,5 @@ import { Client } from '@googlemaps/google-maps-services-js'; +import integrationsConfig from '#config/integrations'; import { inject } from '../../iocContainer/index.js'; @@ -9,7 +10,7 @@ class PlacesApiService { const requestParams = { params: { place_id: placeId, - key: String(process.env.GOOGLE_PLACES_API_KEY), + key: String(integrationsConfig.googlePlacesApiKey?.release()), }, }; From 61e1f384fc3b0b7e90c9633b77f368128dfac2d9 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:49:31 +0200 Subject: [PATCH 13/19] Move the remaining configuration into config modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `config/debug.ts` holds the two switches that trade safety for local diagnosability — `EXPOSE_SENSITIVE_IMPLEMENTATION_DETAILS_IN_ERRORS` and `ALLOW_USER_INPUT_LOCALHOST_URIS` — with the reason each is off by default recorded next to it. `config/graphql.ts` and `config/featureFlags.ts` each hold one value, but give `GRAPHQL_MAX_DEPTH` and `ITEM_QUEUE_TRAFFIC_PERCENTAGE` somewhere to be explained rather than being a bare number at a call site. `session` moves out of `config/app.ts` into `config/session.ts`. It was the only part of the app config describing one specific piece of middleware. The whole cookie goes with it — `api.ts` no longer reassembles the attributes one by one, it hands `sessionConfig.cookie` to the middleware as it is. `satisfies CookieOptions` keeps that object honest now that nothing between here and express-session is checking it. `config/app.ts` also stops capturing its values at import and derives them on access, so that `env.set` in a test is respected. The following commit does the same to the remaining config modules and explains why the suite needs it. This is the last reader of `safeGetEnvVar` and `safeGetEnvInt` outside `iocContainer/utils.ts` itself. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/api.ts | 14 +++----- server/config/app.ts | 52 +++++++++++++++++------------- server/config/debug.ts | 25 ++++++++++++++ server/config/featureFlags.ts | 10 ++++++ server/config/graphql.ts | 9 ++++++ server/config/session.ts | 19 +++++++++++ server/routes/items/submitItems.ts | 7 ++-- server/utils/errors.ts | 5 +-- 8 files changed, 102 insertions(+), 39 deletions(-) create mode 100644 server/config/debug.ts create mode 100644 server/config/featureFlags.ts create mode 100644 server/config/graphql.ts create mode 100644 server/config/session.ts diff --git a/server/api.ts b/server/api.ts index 245fef6e8..e67d801fe 100644 --- a/server/api.ts +++ b/server/api.ts @@ -18,7 +18,9 @@ import { ATTR_EXCEPTION_TYPE, } from '@opentelemetry/semantic-conventions'; import appConfig from '#config/app'; +import graphqlConfig from '#config/graphql'; import securityConfig from '#config/security'; +import sessionConfig from '#config/session'; import connectPgSimple from 'connect-pg-simple'; import cors from 'cors'; import express, { type ErrorRequestHandler, type Request } from 'express'; @@ -36,7 +38,6 @@ import { buildPassportContext } from './graphql/utils/passportContext.js'; import { resolveSamlUser } from './graphql/utils/resolveSamlUser.js'; import { safeDepthLimit } from './graphql/utils/safeDepthLimit.js'; import { type Dependencies } from './iocContainer/index.js'; -import { safeGetEnvInt } from './iocContainer/utils.js'; import controllers from './routes/index.js'; import { createBodySchemaValidator } from './utils/bodySchemaValidation.js'; import { jsonStringify } from './utils/encoding.js'; @@ -114,14 +115,9 @@ export default async function makeApiServer(deps: Dependencies) { const sessionStoreInstance = new sessionStore({ pool: KyselyPgPool }); app.use( session({ - secret: appConfig.session.secret, + secret: sessionConfig.secret, store: sessionStoreInstance, - cookie: { - secure: appConfig.session.cookie.secure, - httpOnly: appConfig.session.cookie.httpOnly, - sameSite: 'lax', - maxAge: appConfig.session.cookie.maxAge, - }, + cookie: sessionConfig.cookie, resave: false, saveUninitialized: false, proxy: true, @@ -249,7 +245,7 @@ export default async function makeApiServer(deps: Dependencies) { ? [ApolloServerPluginLandingPageDisabled()] : []), ], - validationRules: [safeDepthLimit(safeGetEnvInt('GRAPHQL_MAX_DEPTH', 10))], + validationRules: [safeDepthLimit(graphqlConfig.maxDepth)], introspection: !appConfig.inProduction, formatError(formattedError, error) { // unwrapResolverError removes the GraphQLError wrapper added by graphql-js diff --git a/server/config/app.ts b/server/config/app.ts index a88e2fede..ac6b4c8a7 100644 --- a/server/config/app.ts +++ b/server/config/app.ts @@ -1,34 +1,40 @@ import env from '#start/env'; -const NODE_ENV = env.get('NODE_ENV', 'development'); - -// Derived once so the comparison lives in a single place. `NODE_ENV === 'prod'` -// is silently non-production everywhere it appears; `inProduction` is not. -const inProduction = NODE_ENV === 'production'; -const inDev = NODE_ENV === 'development'; -const inTest = NODE_ENV === 'test'; - +/** + * Derived on access rather than at import. + * + * `env.get` reads the validated values `Env.create` produced, and `env.set` + * updates them — so a getter lets a test say `env.set('NODE_ENV', 'production')` + * and have the rest of the application agree. A constant would instead hold + * whatever the environment was when this module was first imported. + * + * `NODE_ENV === 'prod'` is silently non-production everywhere it appears; + * `inProduction` is not, which is why the comparison lives here and not at each + * call site. + */ export default { - env: NODE_ENV, - inProduction, - inDev, - inTest, + get env() { + return env.get('NODE_ENV', 'development'); + }, + get inProduction() { + return this.env === 'production'; + }, + get inDev() { + return this.env === 'development'; + }, + get inTest() { + return this.env === 'test'; + }, // Public origin of the frontend. Used to build the links and redirects the // application hands out, so it must be the origin a browser reaches, not an // internal one. - uiUrl: env.get('UI_URL'), + get uiUrl() { + return env.get('UI_URL'); + }, /** Identifies this process in traces and as the Postgres `application_name`. */ - serviceName: env.get('OTEL_SERVICE_NAME', 'coop-service'), - - session: { - secret: env.get('SESSION_SECRET'), - cookie: { - secure: inProduction, - httpOnly: true, - // 30 Days in milliseconds - maxAge: 30 * 24 * 60 * 60 * 1000, - }, + get serviceName() { + return env.get('OTEL_SERVICE_NAME', 'coop-service'); }, }; diff --git a/server/config/debug.ts b/server/config/debug.ts new file mode 100644 index 000000000..be5178d7d --- /dev/null +++ b/server/config/debug.ts @@ -0,0 +1,25 @@ +import env from '#start/env'; + +/** + * Switches that trade safety for local diagnosability. Each one is off by + * default and should stay off anywhere shared. + */ +export default { + /** + * Include implementation detail — stack traces, internal messages — in error + * responses, rather than the sanitised message. + */ + get exposeUnsafeErrorDetails() { + return env.get('EXPOSE_SENSITIVE_IMPLEMENTATION_DETAILS_IN_ERRORS', false); + }, + + /** + * Permit loopback addresses in user-supplied URLs. Off by default because + * user-supplied URLs reach outbound requests (e.g. webhook callbacks), so + * allowing them widens SSRF reach to anything the server can see on + * localhost. + */ + get allowUserInputLocalhostUris() { + return env.get('ALLOW_USER_INPUT_LOCALHOST_URIS', false); + }, +}; diff --git a/server/config/featureFlags.ts b/server/config/featureFlags.ts new file mode 100644 index 000000000..c47d6476f --- /dev/null +++ b/server/config/featureFlags.ts @@ -0,0 +1,10 @@ +import env from '#start/env'; + +export default { + /** + * The fraction of item submissions routed through the async processing queue + * (BullMQ) rather than handled inline after returning 202. `1` sends all + * traffic through the queue, `0` none of it. + */ + itemQueueTrafficPercentage: env.get('ITEM_QUEUE_TRAFFIC_PERCENTAGE'), +}; diff --git a/server/config/graphql.ts b/server/config/graphql.ts new file mode 100644 index 000000000..7fdb467b0 --- /dev/null +++ b/server/config/graphql.ts @@ -0,0 +1,9 @@ +import env from '#start/env'; + +export default { + /** + * Rejects queries nested deeper than this, so a hostile or accidental deeply + * recursive query can't be turned into a denial of service. + */ + maxDepth: env.get('GRAPHQL_MAX_DEPTH', 10), +}; diff --git a/server/config/session.ts b/server/config/session.ts new file mode 100644 index 000000000..64d4f70c4 --- /dev/null +++ b/server/config/session.ts @@ -0,0 +1,19 @@ +import appConfig from '#config/app'; +import env from '#start/env'; +import type { CookieOptions } from 'express-session'; + +/** + * The express-session cookie. + */ +export default { + secret: env.get('SESSION_SECRET'), + + cookie: { + /** HTTPS-only outside development, where there is no TLS terminator. */ + secure: appConfig.inProduction, + httpOnly: true, + sameSite: 'lax', + /** 30 days, in milliseconds. */ + maxAge: 30 * 24 * 60 * 60 * 1000, + } satisfies CookieOptions, +}; diff --git a/server/routes/items/submitItems.ts b/server/routes/items/submitItems.ts index 33ac96dd2..ac2a7c6c3 100644 --- a/server/routes/items/submitItems.ts +++ b/server/routes/items/submitItems.ts @@ -1,11 +1,11 @@ import { type ItemIdentifier } from '@roostorg/coop-types'; +import featureFlags from '#config/featureFlags'; import { v1 as uuidv1 } from 'uuid'; import { type Dependencies, type ItemSubmissionMessageValue, } from '../../iocContainer/index.js'; -import { safeGetEnvVar } from '../../iocContainer/utils.js'; import { getFieldValueForRole, itemSubmissionToItemSubmissionWithTypeIdentifier, @@ -259,10 +259,7 @@ Dependencies): RequestHandlerWithBodies { // Send a configurable percentage of traffic to the async processing queue // (BullMQ), otherwise handle inline (in this process, immediately after // returning 202 to the user). Set to 1 to route all traffic through the queue. - const trafficPercentage = Number( - safeGetEnvVar('ITEM_QUEUE_TRAFFIC_PERCENTAGE'), - ); - if (Math.random() < trafficPercentage) { + if (Math.random() < featureFlags.itemQueueTrafficPercentage) { // toItemSubmission should always set a `submissionTime` property with a // valid Date, but due to legacy data the type returned, ItemSubmission, an // optional `submissionTime` property. this variable is used to convince diff --git a/server/utils/errors.ts b/server/utils/errors.ts index 9185fcd02..13f1c6073 100644 --- a/server/utils/errors.ts +++ b/server/utils/errors.ts @@ -5,6 +5,8 @@ // folder (even though the logic in them really ought to be in a // transport-agnostic service in the services folder), so, for now, this file // has to import just those files from the graphql folder. +import debugConfig from '#config/debug'; + import { type IntegrationErrorType } from '../graphql/datasources/IntegrationApi.js'; import { type OrgErrorType } from '../graphql/datasources/OrgApi.js'; import { @@ -365,8 +367,7 @@ export const makeBadRequestError = (title: string, data: ErrorInstanceData) => name: 'BadRequestError', }); -const exposeUnsafeErrorDetails = - process.env.EXPOSE_SENSITIVE_IMPLEMENTATION_DETAILS_IN_ERRORS === 'true'; +const exposeUnsafeErrorDetails = debugConfig.exposeUnsafeErrorDetails; export const sanitizeError = exposeUnsafeErrorDetails ? // In local dev, when exposeUnsafeErrorDetails is true, sanitizeError From 203fc8f39daadb9f2a4adcbfe604b3e1380ab11c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:56:41 +0200 Subject: [PATCH 14/19] Derive NCMEC and email config on access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving these reads into config modules changed *when* they happen. A `const isTest = env.get('NCMEC_ENV') !== 'production'` at module scope is evaluated once, the first time anything imports the module — which in a test run is whenever the first suite happens to pull it in, not when a test sets the value it wants. Four suites depended on setting the variable per-test, and broke: the NCMEC reporting and retry-job suites, the email transport suite, and the NCMEC submission integration test. Getters restore the old behaviour, because `env.get` reads the snapshot `Env.create` produced and `env.set` writes to it. The tests move from `process.env` to `env.set` to match. They are not interchangeable: `EnvProcessor` copies `.env` into `process.env` before validation and never looks at it again, so a test mutating `process.env` after boot is writing somewhere nothing reads. Going through `env` also types the values — `NCMEC_ENV` is `'production' | 'test' | undefined` rather than `string | undefined`, and restoring a captured value no longer needs the `delete process.env.X` branch for undefined. This is a workaround for these modules being read as ambient state rather than injected. The services that consume them — `makeSendEmail`, the NCMEC reporting helpers — take no configuration argument, so a test cannot hand them a configuration and has to reach for the environment instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/config/email.ts | 21 ++++++-- server/config/ncmec.ts | 17 ++++--- .../ncmecService/ncmecReporting.test.ts | 26 +++++----- .../sendEmailService/sendEmailService.test.ts | 51 ++++++------------- .../ncmec-report-submission.integ.test.ts | 3 +- .../RetryFailedNcmecDecisionsJob.test.ts | 21 ++++---- 6 files changed, 67 insertions(+), 72 deletions(-) diff --git a/server/config/email.ts b/server/config/email.ts index 5478bc4f0..51a32c41c 100644 --- a/server/config/email.ts +++ b/server/config/email.ts @@ -10,15 +10,26 @@ import env from '#start/env'; * change that adds an SMTP transport, so this module only moves the reads. */ export default { - transport: env.get('EMAIL_TRANSPORT'), + /** Derived on access so `env.set('EMAIL_TRANSPORT', …)` is respected. */ + get transport() { + return env.get('EMAIL_TRANSPORT'); + }, /** Only consulted when no SES client is injected and `transport` is unset. */ - sendgridApiKey: env.get('SENDGRID_API_KEY'), + get sendgridApiKey() { + return env.get('SENDGRID_API_KEY'); + }, /** The addresses Coop sends as. */ addresses: { - noReply: env.get('NOREPLY_EMAIL', 'noreply@example.com'), - support: env.get('SUPPORT_EMAIL', 'support@example.com'), - team: env.get('TEAM_EMAIL', 'team@example.com'), + get noReply() { + return env.get('NOREPLY_EMAIL', 'noreply@example.com'); + }, + get support() { + return env.get('SUPPORT_EMAIL', 'support@example.com'); + }, + get team() { + return env.get('TEAM_EMAIL', 'team@example.com'); + }, }, }; diff --git a/server/config/ncmec.ts b/server/config/ncmec.ts index 7d4484e33..de131c49b 100644 --- a/server/config/ncmec.ts +++ b/server/config/ncmec.ts @@ -11,19 +11,22 @@ import env from '#start/env'; * configured in Settings → NCMEC are production or test credentials issued by * NCMEC. * - * Derived once here because getting it wrong in either direction is serious: - * live reports sent to the sandbox are silently discarded, and test reports sent - * to production are filed as real CyberTipline reports. + * Derived in one place because getting it wrong in either direction is + * serious: live reports sent to the sandbox are silently discarded, and test + * reports sent to production are filed as real CyberTipline reports. */ -const isTest = env.get('NCMEC_ENV') !== 'production'; - export default { - isTest, + /** Derived on access so `env.set('NCMEC_ENV', …)` is respected. */ + get isTest() { + return env.get('NCMEC_ENV') !== 'production'; + }, /** * Opt-in debug logs and XML/JSON dumps for submissions. Also gated on not * being in production, since the dumps contain reportable content and must * not be written in a shared environment. Never includes credentials. */ - debug: env.get('NCMEC_DEBUG', false) && !appConfig.inProduction, + get debug() { + return env.get('NCMEC_DEBUG', false) && !appConfig.inProduction; + }, }; diff --git a/server/services/ncmecService/ncmecReporting.test.ts b/server/services/ncmecService/ncmecReporting.test.ts index 5f19a320f..fc4b12b60 100644 --- a/server/services/ncmecService/ncmecReporting.test.ts +++ b/server/services/ncmecService/ncmecReporting.test.ts @@ -1,3 +1,5 @@ +import env from '#start/env'; + import { buildInternetDetailsFromOrgSetting, clampIncidentDateTimeToPast, @@ -456,17 +458,17 @@ describe('NCMEC reporting', () => { // (this file was over the 500-line max-lines limit after expansion). describe('summarizeCyberTipFailure', () => { - const previousDebug = process.env.NCMEC_DEBUG; - const previousNodeEnv = process.env.NODE_ENV; + const previousDebug = env.get('NCMEC_DEBUG'); + const previousNodeEnv = env.get('NODE_ENV'); afterEach(() => { - process.env.NCMEC_DEBUG = previousDebug; - process.env.NODE_ENV = previousNodeEnv; + env.set('NCMEC_DEBUG', previousDebug); + env.set('NODE_ENV', previousNodeEnv); }); it('includes NCMEC responseCode and description in production', () => { - process.env.NCMEC_DEBUG = undefined; - process.env.NODE_ENV = 'production'; + env.set('NCMEC_DEBUG', undefined); + env.set('NODE_ENV', 'production'); const body = { reportResponse: { responseCode: { _text: '4100' }, @@ -481,8 +483,8 @@ describe('NCMEC reporting', () => { }); it('does not leak unknown body fields in production', () => { - process.env.NCMEC_DEBUG = undefined; - process.env.NODE_ENV = 'production'; + env.set('NCMEC_DEBUG', undefined); + env.set('NODE_ENV', 'production'); const body = { secret: 'reportable-content-or-pii', unrelated: { nested: 'data' }, @@ -494,8 +496,8 @@ describe('NCMEC reporting', () => { }); it('appends the truncated body when NCMEC_DEBUG is enabled in dev', () => { - process.env.NCMEC_DEBUG = '1'; - process.env.NODE_ENV = 'test'; + env.set('NCMEC_DEBUG', true); + env.set('NODE_ENV', 'test'); const body = { reportResponse: { responseCode: { _text: '4000' }, @@ -510,8 +512,8 @@ describe('NCMEC reporting', () => { }); it('still keeps body off in production even with NCMEC_DEBUG=1', () => { - process.env.NCMEC_DEBUG = '1'; - process.env.NODE_ENV = 'production'; + env.set('NCMEC_DEBUG', true); + env.set('NODE_ENV', 'production'); const body = { reportResponse: { responseCode: { _text: '0' } } }; const message = summarizeCyberTipFailure('/submit', 502, body); expect(message).not.toContain('body='); diff --git a/server/services/sendEmailService/sendEmailService.test.ts b/server/services/sendEmailService/sendEmailService.test.ts index 9dedd173e..906352adb 100644 --- a/server/services/sendEmailService/sendEmailService.test.ts +++ b/server/services/sendEmailService/sendEmailService.test.ts @@ -1,5 +1,6 @@ import { SendEmailCommand, type SESClient } from '@aws-sdk/client-ses'; import sgMail from '@sendgrid/mail'; +import env from '#start/env'; import makeSendEmail, { CoopEmailAddress, @@ -179,8 +180,8 @@ describe('sendEmailService', () => { describe('console backend', () => { it('prints the email and reports successful delivery', async () => { - const previousNodeEnv = process.env.NODE_ENV; - process.env.NODE_ENV = 'development'; + const previousNodeEnv = env.get('NODE_ENV'); + env.set('NODE_ENV', 'development'); const consoleSpy = jest .spyOn(console, 'log') .mockImplementation(() => {}); @@ -200,20 +201,16 @@ describe('sendEmailService', () => { msg, ); } finally { - if (previousNodeEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = previousNodeEnv; - } + env.set('NODE_ENV', previousNodeEnv); consoleSpy.mockRestore(); } }); it('is selected explicitly through EMAIL_TRANSPORT', async () => { - const previousTransport = process.env.EMAIL_TRANSPORT; - const previousNodeEnv = process.env.NODE_ENV; - process.env.EMAIL_TRANSPORT = 'console'; - process.env.NODE_ENV = 'development'; + const previousTransport = env.get('EMAIL_TRANSPORT'); + const previousNodeEnv = env.get('NODE_ENV'); + env.set('EMAIL_TRANSPORT', 'console'); + env.set('NODE_ENV', 'development'); const consoleSpy = jest .spyOn(console, 'log') .mockImplementation(() => {}); @@ -230,25 +227,17 @@ describe('sendEmailService', () => { ).resolves.toBe(true); expect(consoleSpy).toHaveBeenCalledTimes(1); } finally { - if (previousTransport === undefined) { - delete process.env.EMAIL_TRANSPORT; - } else { - process.env.EMAIL_TRANSPORT = previousTransport; - } - if (previousNodeEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = previousNodeEnv; - } + env.set('EMAIL_TRANSPORT', previousTransport); + env.set('NODE_ENV', previousNodeEnv); consoleSpy.mockRestore(); } }); it('rejects console transport outside development', () => { - const previousTransport = process.env.EMAIL_TRANSPORT; - const previousNodeEnv = process.env.NODE_ENV; - process.env.EMAIL_TRANSPORT = 'console'; - process.env.NODE_ENV = 'production'; + const previousTransport = env.get('EMAIL_TRANSPORT'); + const previousNodeEnv = env.get('NODE_ENV'); + env.set('EMAIL_TRANSPORT', 'console'); + env.set('NODE_ENV', 'production'); try { for (const makeTransport of [ @@ -260,16 +249,8 @@ describe('sendEmailService', () => { ); } } finally { - if (previousTransport === undefined) { - delete process.env.EMAIL_TRANSPORT; - } else { - process.env.EMAIL_TRANSPORT = previousTransport; - } - if (previousNodeEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = previousNodeEnv; - } + env.set('EMAIL_TRANSPORT', previousTransport); + env.set('NODE_ENV', previousNodeEnv); } }); }); diff --git a/server/test/integ/ncmec-report-submission.integ.test.ts b/server/test/integ/ncmec-report-submission.integ.test.ts index 02bbc358b..b800f376d 100644 --- a/server/test/integ/ncmec-report-submission.integ.test.ts +++ b/server/test/integ/ncmec-report-submission.integ.test.ts @@ -10,6 +10,7 @@ * Requires: `npm run up && npm run db:update` */ import { ScalarTypes } from '@roostorg/coop-types'; +import ncmecConfig from '#config/ncmec'; import { uid } from 'uid'; import { jsonStringify } from '../../utils/encoding.js'; @@ -412,7 +413,7 @@ describe('NCMEC report and submission (integration)', () => { expect(reportRow.report_id).toBe(ncmecReportId); expect(reportRow.reviewer_id).toBe(reviewerId); - expect(reportRow.is_test).toBe(process.env.NCMEC_ENV !== 'production'); + expect(reportRow.is_test).toBe(ncmecConfig.isTest); const reportedMediaIds = ( reportRow.reported_media as Array<{ id: string; typeId: string }> diff --git a/server/workers_jobs/RetryFailedNcmecDecisionsJob.test.ts b/server/workers_jobs/RetryFailedNcmecDecisionsJob.test.ts index 48a3de45a..57f513087 100644 --- a/server/workers_jobs/RetryFailedNcmecDecisionsJob.test.ts +++ b/server/workers_jobs/RetryFailedNcmecDecisionsJob.test.ts @@ -1,3 +1,4 @@ +import env from '#start/env'; import { v1 as uuidv1 } from 'uuid'; import { @@ -143,21 +144,17 @@ describe('RetryFailedNcmecDecisionsJob', () => { /** Snapshot of NCMEC_ENV across the suite so each test can mutate it * freely and we restore the original value in afterEach. */ - let originalNcmecEnv: string | undefined; + let originalNcmecEnv: 'production' | 'test' | undefined; beforeEach(() => { - originalNcmecEnv = process.env.NCMEC_ENV; + originalNcmecEnv = env.get('NCMEC_ENV'); }); afterEach(() => { - if (originalNcmecEnv === undefined) { - delete process.env.NCMEC_ENV; - } else { - process.env.NCMEC_ENV = originalNcmecEnv; - } + env.set('NCMEC_ENV', originalNcmecEnv); }); it('passes isTest=true to submitReport when NCMEC_ENV is unset', async () => { - delete process.env.NCMEC_ENV; + env.set('NCMEC_ENV', undefined); const deps = makeDeps({ decisions: [makeNcmecDecisionRow(ORG_ID)], }); @@ -179,7 +176,7 @@ describe('RetryFailedNcmecDecisionsJob', () => { }); it('passes isTest=true to submitReport when NCMEC_ENV is "test"', async () => { - process.env.NCMEC_ENV = 'test'; + env.set('NCMEC_ENV', 'test'); const deps = makeDeps({ decisions: [makeNcmecDecisionRow(ORG_ID)], }); @@ -201,7 +198,7 @@ describe('RetryFailedNcmecDecisionsJob', () => { }); it('passes isTest=false to submitReport when NCMEC_ENV=production', async () => { - process.env.NCMEC_ENV = 'production'; + env.set('NCMEC_ENV', 'production'); const deps = makeDeps({ decisions: [makeNcmecDecisionRow(ORG_ID)], }); @@ -223,7 +220,7 @@ describe('RetryFailedNcmecDecisionsJob', () => { }); it('does not publish actions when NCMEC_ENV is unset (isTest=true)', async () => { - delete process.env.NCMEC_ENV; + env.set('NCMEC_ENV', undefined); const deps = makeDeps({ decisions: [makeNcmecDecisionRow(ORG_ID)], // Simulate an org that has actions configured to run on NCMEC report @@ -251,7 +248,7 @@ describe('RetryFailedNcmecDecisionsJob', () => { }); it('publishes actions when NCMEC_ENV=production (isTest=false)', async () => { - process.env.NCMEC_ENV = 'production'; + env.set('NCMEC_ENV', 'production'); const deps = makeDeps({ decisions: [makeNcmecDecisionRow(ORG_ID)], getNCMECActionsToRunAndPolicies: jest.fn(async () => ({ From 2989539571b58199b338efc1d2302f4e296f1c7c Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:56:56 +0200 Subject: [PATCH 15/19] Separate URL checking from the rules this deployment applies `utils/url.ts` read `ALLOW_USER_INPUT_LOCALHOST_URIS` from `process.env` inside a default argument, so "is this a valid URL" and "what does this deployment consider valid" were the same function. That made the environment an input to every caller, including tests, which is why its own suite could not test the blocking behaviour without setting a variable first. The split is along that seam. `utils/url.ts` now takes the rules as an argument and knows nothing about configuration; `utils/urlValidation.ts` is the configured entry point, holds the single read of `debugConfig`, and is what every caller imports. `LOOPBACK_HOSTNAMES` is exported so a caller opting out of the configured defaults can still say "the usual loopback set". Two fixes fall out of it: `validateUrl` wrapped its whole body in `try`/`catch` and rethrew everything as `Invalid URL`, so a blocked hostname and an unparseable string were indistinguishable, and the scheme and hostname errors it raised were dead text. Only the parse is guarded now, via `URL.canParse`. `validateUrlOrNull` is removed. It carried a copy of the default options with a comment asking the reader to keep the two in sync by hand, and had no callers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/graphql/datasources/orgValidation.ts | 2 +- server/graphql/modules/org.ts | 2 +- .../fieldTypeHandlers.ts | 2 +- server/utils/url.test.ts | 58 ++++++------ server/utils/url.ts | 89 +++++++------------ server/utils/urlValidation.ts | 48 ++++++++++ 6 files changed, 109 insertions(+), 92 deletions(-) create mode 100644 server/utils/urlValidation.ts diff --git a/server/graphql/datasources/orgValidation.ts b/server/graphql/datasources/orgValidation.ts index 2b3d6140a..0911b15c8 100644 --- a/server/graphql/datasources/orgValidation.ts +++ b/server/graphql/datasources/orgValidation.ts @@ -1,7 +1,7 @@ import { createRequire } from 'node:module'; import type { IsEmailOptions } from 'validator/lib/isEmail.js'; -import { validateUrl } from '../../utils/url.js'; +import { validateUrl } from '../../utils/urlValidation.js'; // `validator` is CJS with UMD-style types whose `default` doesn't resolve to // a callable under `module: NodeNext`; `createRequire` gives us `module.exports` diff --git a/server/graphql/modules/org.ts b/server/graphql/modules/org.ts index 73af75047..3382251c5 100644 --- a/server/graphql/modules/org.ts +++ b/server/graphql/modules/org.ts @@ -10,7 +10,7 @@ import { import { filterDecisionsToFailedSubmissions } from '../../services/ncmecService/index.js'; import { UserPermission } from '../../services/userManagementService/index.js'; import { __throw } from '../../utils/misc.js'; -import { isValidUrl } from '../../utils/url.js'; +import { isValidUrl } from '../../utils/urlValidation.js'; import { type GQLIntegrationConfig, type GQLMatchingBanksResolvers, diff --git a/server/services/itemProcessingService/fieldTypeHandlers.ts b/server/services/itemProcessingService/fieldTypeHandlers.ts index f919d6db9..f63340346 100644 --- a/server/services/itemProcessingService/fieldTypeHandlers.ts +++ b/server/services/itemProcessingService/fieldTypeHandlers.ts @@ -17,7 +17,7 @@ import _ from 'lodash'; import { match } from 'ts-pattern'; import { doesThrow } from '../../utils/misc.js'; -import { isValidUrl, makeUrlString } from '../../utils/url.js'; +import { isValidUrl, makeUrlString } from '../../utils/urlValidation.js'; const { isPlainObject } = _; diff --git a/server/utils/url.test.ts b/server/utils/url.test.ts index 81e09d5ee..2be666bc4 100644 --- a/server/utils/url.test.ts +++ b/server/utils/url.test.ts @@ -1,46 +1,44 @@ -import { validateUrl } from './url.js'; +import { LOOPBACK_HOSTNAMES, validateUrl } from './url.js'; -describe('URL Tests', () => { - describe('Loopback gating (default)', () => { - beforeEach(() => { - // This absolutely is unsafe mutation of a global that'll be visible - // across test suites. However, this env var should only be relied upon - // by this module, so it should be ok. - process.env.ALLOW_USER_INPUT_LOCALHOST_URIS = 'false'; - }); +const blockingLoopback = { + allowedSchemes: ['http', 'https'], + blockedHostnames: [...LOOPBACK_HOSTNAMES], +}; - afterEach(() => { - delete process.env.ALLOW_USER_INPUT_LOCALHOST_URIS; - }); +const allowingLoopback = { + allowedSchemes: ['http', 'https'], + blockedHostnames: [], +}; +describe('URL Tests', () => { + describe('Loopback blocked', () => { test('Deny localhost domains', () => { - expect(() => validateUrl('https://localhost:3000')).toThrow(); - expect(() => validateUrl('https://127.0.0.1')).toThrow(); + expect(() => + validateUrl('https://localhost:3000', blockingLoopback), + ).toThrow(); + expect(() => + validateUrl('https://127.0.0.1', blockingLoopback), + ).toThrow(); }); test('Allow arbitrary external URLs', () => { - expect(() => validateUrl('https://example.com')).not.toThrow(); expect(() => - validateUrl('https://api.example.org/webhook'), + validateUrl('https://example.com', blockingLoopback), + ).not.toThrow(); + expect(() => + validateUrl('https://api.example.org/webhook', blockingLoopback), ).not.toThrow(); }); }); - describe('Loopback gating (development override)', () => { - beforeEach(() => { - // This absolutely is unsafe mutation of a global that'll be visible - // across test suites. However, this env var should only be relied upon - // by this module, so it should be ok. - process.env.ALLOW_USER_INPUT_LOCALHOST_URIS = 'true'; - }); - - afterEach(() => { - delete process.env.ALLOW_USER_INPUT_LOCALHOST_URIS; - }); - + describe('Loopback allowed', () => { test('Allow localhost domains', () => { - expect(() => validateUrl('https://localhost:3000')).not.toThrow(); - expect(() => validateUrl('https://127.0.0.1')).not.toThrow(); + expect(() => + validateUrl('https://localhost:3000', allowingLoopback), + ).not.toThrow(); + expect(() => + validateUrl('https://127.0.0.1', allowingLoopback), + ).not.toThrow(); }); }); diff --git a/server/utils/url.ts b/server/utils/url.ts index 555aa65c2..ec13e8efb 100644 --- a/server/utils/url.ts +++ b/server/utils/url.ts @@ -1,51 +1,45 @@ -import { type UrlString } from '@roostorg/coop-types'; - -import { instantiateOpaqueType } from './typescript-types.js'; +/** + * URL checks with no notion of how this deployment is configured — callers + * supply the rules. `./urlValidation.js` is the configured entry point that + * applies Coop's own rules; use that unless you specifically want to say what + * counts as valid. + */ -type UrlValidationOptions = { +export type UrlValidationOptions = { allowedSchemes: string[]; blockedHostnames: string[]; }; -function defaultBlockedHostnames(): string[] { - const { ALLOW_USER_INPUT_LOCALHOST_URIS } = process.env; +/** + * Loopback addresses, blocked by default to reduce SSRF risk from user-supplied + * URLs (e.g. webhook callbacks). Deployments wanting stricter blocking against + * their own public hostname pass `blockedHostnames` themselves. + */ +export const LOOPBACK_HOSTNAMES: readonly string[] = Object.freeze([ + 'localhost', + '127.0.0.1', +]); - // Block loopback addresses to reduce SSRF risk from user-supplied URLs (e.g. - // webhook callbacks). Operators who want stricter blocking against their own - // public hostname should pass `blockedHostnames` via `UrlValidationOptions`. - return ALLOW_USER_INPUT_LOCALHOST_URIS === 'true' - ? [] - : ['localhost', '127.0.0.1']; -} +export function validateUrl(value: string, opts: UrlValidationOptions) { + if (!URL.canParse(value)) { + throw new Error('Invalid URL'); + } -export function validateUrl( - value: string, - // If you update these opts make sure to update validateUrlOrNull's opts as - // well - opts: UrlValidationOptions = { - allowedSchemes: ['http', 'https'], - blockedHostnames: defaultBlockedHostnames(), - }, -) { - try { - const { allowedSchemes, blockedHostnames } = opts; - const { hostname, protocol } = new URL(value); // might throw. - const containsValidScheme = allowedSchemes.includes(protocol.slice(0, -1)); - if (!containsValidScheme) { - throw new Error('URL contains invalid scheme'); - } + const { allowedSchemes, blockedHostnames } = opts; + const { hostname, protocol } = new URL(value); + const containsValidScheme = allowedSchemes.includes(protocol.slice(0, -1)); + if (!containsValidScheme) { + throw new Error('URL contains invalid scheme'); + } - const containsBlockedHostname = blockedHostnames.includes(hostname); + const containsBlockedHostname = blockedHostnames.includes(hostname); - if (containsBlockedHostname) { - throw new Error('URL contains blocked hostname'); - } - } catch (_) { - throw new Error('Invalid URL'); + if (containsBlockedHostname) { + throw new Error('URL contains blocked hostname'); } } -export function isValidUrl(url: string, opts?: UrlValidationOptions) { +export function isValidUrl(url: string, opts: UrlValidationOptions) { try { validateUrl(url, opts); return true; @@ -53,26 +47,3 @@ export function isValidUrl(url: string, opts?: UrlValidationOptions) { return false; } } - -/** - * Returns a {@link UrlString} if the input string is a valid URL; else - * undefined. Does not accept urls that are invalid according to the default - * {@link UrlValidationOptions} used by {@link validateUrl}. - */ -export function makeUrlString(it: string) { - return isValidUrl(it) ? instantiateOpaqueType(it) : undefined; -} - -export function validateUrlOrNull( - value?: string, - // Keep this default in sync with `validateUrl`'s default. - opts: UrlValidationOptions = { - allowedSchemes: ['http', 'https'], - blockedHostnames: defaultBlockedHostnames(), - }, -) { - if (value == null) { - return; - } - validateUrl(value, opts); -} diff --git a/server/utils/urlValidation.ts b/server/utils/urlValidation.ts new file mode 100644 index 000000000..b9a65516b --- /dev/null +++ b/server/utils/urlValidation.ts @@ -0,0 +1,48 @@ +import { type UrlString } from '@roostorg/coop-types'; +import debugConfig from '#config/debug'; + +import { instantiateOpaqueType } from './typescript-types.js'; +import { + isValidUrl as isValidUrlAgainst, + LOOPBACK_HOSTNAMES, + validateUrl as validateUrlAgainst, + type UrlValidationOptions, +} from './url.js'; + +/** + * The rules this deployment applies to user-supplied URLs. + * + * Read on each call rather than captured, so `env.set` in a test is respected — + * see the note in `config/app.ts`. + */ +function deploymentOptions(): UrlValidationOptions { + return { + allowedSchemes: ['http', 'https'], + blockedHostnames: debugConfig.allowUserInputLocalhostUris + ? [] + : [...LOOPBACK_HOSTNAMES], + }; +} + +/** Throws `Invalid URL` unless `value` passes this deployment's rules. */ +export function validateUrl( + value: string, + opts: UrlValidationOptions = deploymentOptions(), +) { + validateUrlAgainst(value, opts); +} + +export function isValidUrl( + value: string, + opts: UrlValidationOptions = deploymentOptions(), +) { + return isValidUrlAgainst(value, opts); +} + +/** + * Returns a {@link UrlString} if the input passes this deployment's rules, else + * undefined. + */ +export function makeUrlString(it: string) { + return isValidUrl(it) ? instantiateOpaqueType(it) : undefined; +} From f5ed1d8c5771547584fe087f40c43725c54bb172 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:58:49 +0200 Subject: [PATCH 16/19] Read NODE_ENV through config in the last three call sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `org.ts` gets `appConfig.inProduction`, and reads in the order the comment above it describes: throw in production, return empty outside it. `apiKey.ts` returned `process.env.NODE_ENV !== 'production' ? '' : ''` — both branches the same, so the environment was never an input. It returns `''`. `bin/get-invite-token.ts` built its signup link from `process.env.UI_URL` with a localhost fallback. The fallback could not be reached: the script imports the IoC container, which loads `#start/env`, and `UI_URL` is required there — so an unset `UI_URL` fails the script before this line runs. `READ_ME_JWT_SECRET` in `user.ts` is deliberately left alone; that resolver is being removed separately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/bin/get-invite-token.ts | 4 ++-- server/graphql/modules/apiKey.ts | 2 +- server/graphql/modules/org.ts | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/server/bin/get-invite-token.ts b/server/bin/get-invite-token.ts index 95f1bb391..e4f0fe4ae 100644 --- a/server/bin/get-invite-token.ts +++ b/server/bin/get-invite-token.ts @@ -6,6 +6,7 @@ * Usage: * npm run get-invite -- --email "user@example.com" */ +import appConfig from '#config/app'; import yargs from 'yargs'; import { hideBin } from 'yargs/helpers'; @@ -42,8 +43,7 @@ async function getInviteToken() { } const invite = result[0]; - const uiUrl = process.env.UI_URL ?? 'http://localhost:3000'; - const signupUrl = `${uiUrl}/signup/${invite.token}`; + const signupUrl = `${appConfig.uiUrl}/signup/${invite.token}`; console.log('\n✅ Invite Token Found!\n'); console.log('═'.repeat(60)); diff --git a/server/graphql/modules/apiKey.ts b/server/graphql/modules/apiKey.ts index 23d1759b8..286302d7b 100644 --- a/server/graphql/modules/apiKey.ts +++ b/server/graphql/modules/apiKey.ts @@ -82,7 +82,7 @@ const Query: GQLQueryResolvers = { const apiKeyRecord = await context.services.ApiKeyService.getActiveApiKeyForOrg(user.orgId); if (!apiKeyRecord) { - return process.env.NODE_ENV !== 'production' ? '' : ''; + return ''; } // Return a message indicating the key exists but is hidden for security return 'API key exists (hidden for security)'; diff --git a/server/graphql/modules/org.ts b/server/graphql/modules/org.ts index 3382251c5..038bcf38d 100644 --- a/server/graphql/modules/org.ts +++ b/server/graphql/modules/org.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines */ +import appConfig from '#config/app'; import { GraphQLError } from 'graphql'; import { type JsonObject, type JsonValue } from 'type-fest'; @@ -423,9 +424,9 @@ const Org: GQLOrgResolvers = { // API Keys are required in prod, but no reason to throw outside prod (like // on engineers' local machines) if (!apiKey) { - return process.env.NODE_ENV !== 'production' - ? '' - : __throw(new GraphQLError('API Key not found')); + return appConfig.inProduction + ? __throw(new GraphQLError('API Key not found')) + : ''; } return apiKey.key; From 62c430977d7c852b41e60fb90961434dc092aa81 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 05:59:40 +0200 Subject: [PATCH 17/19] Remove the ad-hoc environment helpers they replaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `safeGetEnvVar`, `isEnvTrue`, `safeGetEnvInt` and `safeGetEnvNonNegativeInt` have no callers left: every variable they read is declared in `start/env.ts` and reached through a module in `config/`. Two behaviours go with them, both deliberately: `isEnvTrue` accepted `true`, `1` and `yes`, case-insensitively and trimmed. `Env.schema.boolean` accepts only `true`/`1`/`false`/`0`, exactly. An operator setting `SOMETHING=yes` used to get a silent false where they meant true, and now gets a startup error naming the variable. `safeGetEnvInt` logged to `console.error` and carried on with its default when a value was out of range, so a typo in a pool size or timeout survived as a log line nobody read. The validators in `lib/env` refuse to let the process start. The `NodeJS.ProcessEnv` block in `decs.d.ts` goes too. It typed those variables as `string | undefined` at the `process.env` reads that no longer exist — `env.get` returns the validated type instead. The rest of `decs.d.ts` stays; it is ambient declarations for untyped packages and has nothing to do with the environment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VPiJ3vsqgpbnwZWq1LRQLM --- server/decs.d.ts | 37 --------------------- server/iocContainer/utils.ts | 63 ------------------------------------ 2 files changed, 100 deletions(-) diff --git a/server/decs.d.ts b/server/decs.d.ts index 0e1e402ec..3c329a900 100644 --- a/server/decs.d.ts +++ b/server/decs.d.ts @@ -227,40 +227,3 @@ declare module '@stdlib/stats-binomial-test' { export = binomialTest; } - -namespace NodeJS { - interface ProcessEnv { - DATABASE_HOST?: string; - DATABASE_READ_ONLY_HOST?: string; - DATABASE_PORT?: string; - DATABASE_NAME?: string; - DATABASE_USER?: string; - DATABASE_PASSWORD?: string; - DATABASE_SSL?: string; - DATABASE_POOL_MAX?: string; - DATABASE_READ_POOL_MAX?: string; - DATABASE_POOL_IDLE_TIMEOUT_MS?: string; - DATABASE_POOL_CONNECTION_TIMEOUT_MS?: string; - DATABASE_QUERY_TIMEOUT_MS?: string; - DATABASE_IDLE_IN_TRANSACTION_TIMEOUT_MS?: string; - DATABASE_PRINT_LOGS?: string; - SESSION_SECRET?: string; - WAREHOUSE_ADAPTER?: string; - ANALYTICS_ADAPTER?: string; - DATA_WAREHOUSE_PROVIDER?: string; - NCMEC_ENV?: string; - NODE_ENV?: string; - EXPOSE_SENSITIVE_IMPLEMENTATION_DETAILS_IN_ERRORS?: string; - ALLOW_USER_INPUT_LOCALHOST_URIS?: string; - REDIS_USE_CLUSTER?: string; - REDIS_HOST?: string; - REDIS_PORT?: string; - REDIS_USER?: string; - REDIS_PASSWORD?: string; - GROQ_SECRET_KEY?: string; - SENDGRID_API_KEY?: string; - GOOGLE_PLACES_API_KEY?: string; - OPEN_AI_API_KEY?: string; - SLACK_APP_BEARER_TOKEN?: string; - } -} diff --git a/server/iocContainer/utils.ts b/server/iocContainer/utils.ts index db5baf205..f1776e452 100644 --- a/server/iocContainer/utils.ts +++ b/server/iocContainer/utils.ts @@ -2,8 +2,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import type Bottle from '@ethanresnick/bottlejs'; -import { jsonStringify } from '../utils/encoding.js'; -import { __throw } from '../utils/misc.js'; import { type Dependencies as Deps } from './index.js'; const DEPENDENCIES = Symbol(); @@ -650,64 +648,3 @@ function isConstructable(fn: any): fn is new (...args: any[]) => any { return false; } } - -/** - * Gets an env var, or throws if the variable is undefined. This is critical to - * make the app hard crash early (so we'll get alerts) if some expected config - * var is missing. - */ -export function safeGetEnvVar(varName: string): string { - return ( - process.env[varName] ?? __throw(new Error(`Missing env var ${varName}`)) - ); -} - -/** - * Returns true when the env var is set to a truthy value. Accepts `true`, `1`, - * and `yes` (case-insensitive) so callers don't have to worry about casing or - * common aliases. Any other value (including unset) returns false. - */ -export function isEnvTrue(varName: string): boolean { - const raw = process.env[varName]; - if (raw == null) return false; - return ['true', '1', 'yes'].includes(raw.trim().toLowerCase()); -} - -/** - * Gets an env var and parses it as a positive integer. Returns `defaultValue` - * if the variable is unset or invalid, logging an error on misconfiguration. - */ -export function safeGetEnvInt(varName: string, defaultValue: number): number { - const raw = process.env[varName]; - if (raw === undefined) return defaultValue; - const parsed = parseInt(raw, 10); - if (!Number.isInteger(parsed) || parsed <= 0) { - // eslint-disable-next-line no-console - console.error( - `Invalid env var ${varName}: expected a positive integer, got ${jsonStringify(raw)}. Using default value ${defaultValue}.`, - ); - return defaultValue; - } - return parsed; -} - -/** - * Like `safeGetEnvInt` but allows `0`. Use when zero is a meaningful value - * (e.g. disabling retries, no timeout). - */ -export function safeGetEnvNonNegativeInt( - varName: string, - defaultValue: number, -): number { - const raw = process.env[varName]; - if (raw === undefined) return defaultValue; - const parsed = parseInt(raw, 10); - if (!Number.isInteger(parsed) || parsed < 0) { - // eslint-disable-next-line no-console - console.error( - `Invalid env var ${varName}: expected a non-negative integer, got ${jsonStringify(raw)}. Using default value ${defaultValue}.`, - ); - return defaultValue; - } - return parsed; -} From 3fd7a66c6ce1d7b237ec7549ad3421df0c6b44c3 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 18:37:23 +0200 Subject: [PATCH 18/19] Remove exception for accessing process.env.* --- server/.eslintrc.cjs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/server/.eslintrc.cjs b/server/.eslintrc.cjs index 15ec558b3..1bd28d1ba 100644 --- a/server/.eslintrc.cjs +++ b/server/.eslintrc.cjs @@ -677,20 +677,14 @@ module.exports = { { files: ['test/**/*.ts', 'e2e/**/*.ts', './**/*.{spec,test}.ts'], rules: { - // Match prior test-only mutation policy: allow `this`, class internals, - // and `process.env.*`; production code is not in this override. + // Match prior test-only mutation policy: allow `this` and class + // internals; production code is not in this override. 'functional/immutable-data': [ 'error', { ignoreImmediateMutation: true, ignoreClasses: true, - ignoreAccessorPattern: [ - 'this', - 'this.*', - 'this.*.*', - // Tests toggle env vars and clean up with `delete process.env.*`. - 'process.env.*', - ], + ignoreAccessorPattern: ['this', 'this.*', 'this.*.*'], }, ], 'no-console': 'off', From 60fe41916244d36f296a7e1b5dd7c673e89782e6 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Thu, 17 Sep 2026 18:37:53 +0200 Subject: [PATCH 19/19] Improve documentation for environment variable for sessions --- docs/development/architecture.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/development/architecture.md b/docs/development/architecture.md index 7eaa3d987..d8cc6dc63 100644 --- a/docs/development/architecture.md +++ b/docs/development/architecture.md @@ -364,7 +364,7 @@ Session configuration: - Store: PostgreSQL-backed - Cookie: Secure flag in production, 30-day expiry -- Session secret: process.env.SESSION_SECRET +- Session secret: SESSION_SECRET environment variable Files: `/server/api.ts`