Standardise environment variable handling

.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

CHANGELOG.md

@@ -18,20 +18,26 @@ For more information about each release including git tags and artifacts, see [R
 
 ### Changed
 
-- Scylla is now optional via `ITEM_INVESTIGATION_AND_STRIKES_ENABLED` ([#918](https://github.com/roostorg/coop/pull/918) by [@sunilatlas](https://github.com/sunilatlas))
+- 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 `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))
 - Production Node images moved from Debian 11 (bullseye) to Debian 12 (bookworm) ([#1138](https://github.com/roostorg/coop/pull/1138) by [@juanmrad](https://github.com/juanmrad))
 
 ### 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))
 
 ### 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))

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`
 

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
@@ -99,6 +105,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=

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',

server/api.ts

@@ -17,6 +17,10 @@ import {
   ATTR_EXCEPTION_STACKTRACE,
   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';
@@ -34,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';
@@ -86,8 +89,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 +98,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,15 +115,9 @@ export default async function makeApiServer(deps: Dependencies) {
   const sessionStoreInstance = new sessionStore({ pool: KyselyPgPool });
   app.use(
     session({
-      secret: process.env.SESSION_SECRET!,
+      secret: sessionConfig.secret,
       store: sessionStoreInstance,
-      cookie: {
-        secure: process.env.NODE_ENV === 'production',
-        httpOnly: true,
-        sameSite: 'lax',
-        // 30 Days in milliseconds
-        maxAge: 30 * 24 * 60 * 60 * 1000,
-      },
+      cookie: sessionConfig.cookie,
       resave: false,
       saveUninitialized: false,
       proxy: true,
@@ -194,11 +171,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 +194,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 +241,12 @@ export default async function makeApiServer(deps: Dependencies) {
       },
     }),
     plugins: [
-      ...(process.env.NODE_ENV === 'production'
+      ...(appConfig.inProduction
         ? [ApolloServerPluginLandingPageDisabled()]
         : []),
     ],
-    validationRules: [safeDepthLimit(safeGetEnvInt('GRAPHQL_MAX_DEPTH', 10))],
-    introspection: process.env.NODE_ENV !== 'production',
+    validationRules: [safeDepthLimit(graphqlConfig.maxDepth)],
+    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.

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));

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;
-}

server/config/app.ts

@@ -0,0 +1,40 @@
+import env from '#start/env';
+
+/**
+ * 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 {
+  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.
+  get uiUrl() {
+    return env.get('UI_URL');
+  },
+
+  /** Identifies this process in traces and as the Postgres `application_name`. */
+  get serviceName() {
+    return env.get('OTEL_SERVICE_NAME', 'coop-service');
+  },
+};