From 11a0c823cc88bd9a3029da7abe1099ee3f40825c Mon Sep 17 00:00:00 2001 From: varun-pahuja Date: Fri, 23 Oct 2026 10:57:42 +0530 Subject: [PATCH] feat(server): add startup security checks for secrets and origins Adds checkSecuritySettings() in packages/server/src/security/startup-checks.ts and tests. Reports errors/warnings for: - session secrets < 32 bytes (UTF-8) - secret matches NEXT_PUBLIC_, VITE_, PUBLIC_, NUXT_PUBLIC_, EXPO_PUBLIC_, REACT_APP_ env vars - allowed origin not http(s):// or * - origin with path/query/trailing slash - * combined with other origins - duplicate secrets (warn) - http:// origin not localhost/127.0.0.1 (warn) Not exported from @chatcops/server yet. Closes #231 --- .changeset/security-startup-checks.md | 7 + .../server/src/security/startup-checks.ts | 141 ++++++++++++++++ .../tests/security/startup-checks.test.ts | 151 ++++++++++++++++++ 3 files changed, 299 insertions(+) create mode 100644 .changeset/security-startup-checks.md create mode 100644 packages/server/src/security/startup-checks.ts create mode 100644 packages/server/tests/security/startup-checks.test.ts diff --git a/.changeset/security-startup-checks.md b/.changeset/security-startup-checks.md new file mode 100644 index 00000000..b2fded7f --- /dev/null +++ b/.changeset/security-startup-checks.md @@ -0,0 +1,7 @@ +--- +--- + +Add server startup security checks for session secrets and allowed origins. +Not exported from @chatcops/server yet; the checks will be called by the +handler in a follow-up PR. Empty changeset per issue requirement. +--- diff --git a/packages/server/src/security/startup-checks.ts b/packages/server/src/security/startup-checks.ts new file mode 100644 index 00000000..12023d0a --- /dev/null +++ b/packages/server/src/security/startup-checks.ts @@ -0,0 +1,141 @@ +/** + * Startup checks for the security settings the server is about to add: the + * allowed origins and the secrets that sign session tokens. + * + * These are the mistakes that fail silently. A secret that is ten characters + * long works, and so does a secret that is also a `NEXT_PUBLIC_` variable, so + * nothing breaks until the day it does. A function that finds them before the + * server starts is worth more than one that finds them during a request. + * + * Nothing here logs or throws. The caller decides what to do with a problem. + */ + +/** The security settings a server is configured with. */ +export interface SecuritySettings { + /** The origins the server accepts requests from. `*` allows every origin. */ + allowedOrigins?: string[]; + /** The secrets that sign session tokens. */ + sessionSecrets?: string[]; +} + +/** One thing that is wrong with the security settings. */ +export interface SecurityProblem { + /** An error stops the server from starting. A warning is worth a look. */ + level: 'error' | 'warning'; + /** What is wrong, and what to do about it. Never contains a secret. */ + message: string; +} + +/** How many bytes a session secret needs to be worth signing with. */ +const MIN_SECRET_BYTES = 32; + +/** + * The env var prefixes whose value is shipped to the browser, so a secret + * that shares a value with one of them is in the client bundle. + */ +const PUBLIC_ENV_PREFIXES = ['NEXT_PUBLIC_', 'VITE_', 'PUBLIC_', 'NUXT_PUBLIC_', 'EXPO_PUBLIC_', 'REACT_APP_']; + +/** + * The environment to read, which is `process.env` where there is one. Workers + * have no `process`, so the checks still work there with an empty environment. + */ +function defaultEnv(): Record { + return (globalThis as { process?: { env?: Record } }).process?.env ?? {}; +} + +/** How many bytes a string takes in UTF-8, so a 32 byte secret is 32 bytes. */ +function utf8Length(value: string): number { + return new TextEncoder().encode(value).length; +} + +/** + * Finds the mistakes in the security settings, most serious first. + * + * `env` is the environment to look in, and defaults to `process.env`. + */ +export function checkSecuritySettings( + settings: SecuritySettings, + env?: Record, +): SecurityProblem[] { + const problems: SecurityProblem[] = []; + const secrets = settings.sessionSecrets ?? []; + const origins = settings.allowedOrigins ?? []; + const environment = env ?? defaultEnv(); + + for (const [index, secret] of secrets.entries()) { + if (utf8Length(secret) < MIN_SECRET_BYTES) { + problems.push({ + level: 'error', + message: `Session secret ${index + 1} is shorter than ${MIN_SECRET_BYTES} bytes.`, + }); + } + } + + for (const [index, secret] of secrets.entries()) { + for (const name of Object.keys(environment)) { + if (!PUBLIC_ENV_PREFIXES.some((prefix) => name.startsWith(prefix))) continue; + if (environment[name] !== secret) continue; + problems.push({ + level: 'error', + message: + `Session secret ${index + 1} is also the value of the ${name} environment variable, ` + + 'which is sent to the browser. Use a different secret.', + }); + } + } + + for (const [index, secret] of secrets.entries()) { + const first = secrets.indexOf(secret); + if (first !== index) { + problems.push({ + level: 'warning', + message: `Session secrets ${first + 1} and ${index + 1} are the same.`, + }); + } + } + + for (const [index, origin] of origins.entries()) { + if (origin === '*') continue; + if (!origin.startsWith('http://') && !origin.startsWith('https://')) { + problems.push({ + level: 'error', + message: `Allowed origin ${index + 1} must start with http:// or https://.`, + }); + continue; + } + let parsed: URL; + try { + parsed = new URL(origin); + } catch { + problems.push({ + level: 'error', + message: `Allowed origin ${index + 1} is not a valid URL.`, + }); + continue; + } + if (parsed.pathname !== '/' || parsed.search !== '' || origin.endsWith('/')) { + problems.push({ + level: 'error', + message: + `Allowed origin ${index + 1} must not have a path, a query or a trailing slash, ` + + 'because browsers send the origin without one.', + }); + continue; + } + if (origin.startsWith('http://') && parsed.hostname !== 'localhost' && parsed.hostname !== '127.0.0.1') { + problems.push({ + level: 'warning', + message: `Allowed origin ${index + 1} uses http://. Use https://, unless it is a local development server.`, + }); + } + } + + if (origins.includes('*') && origins.length > 1) { + problems.push({ + level: 'warning', + message: `Allowed origins include "*", so the other ${origins.length - 1} entries do nothing.`, + }); + } + + return problems; +} diff --git a/packages/server/tests/security/startup-checks.test.ts b/packages/server/tests/security/startup-checks.test.ts new file mode 100644 index 00000000..4e94a71a --- /dev/null +++ b/packages/server/tests/security/startup-checks.test.ts @@ -0,0 +1,151 @@ +import { describe, it, expect } from 'vitest'; +import { checkSecuritySettings } from '../../src/security/startup-checks.js'; + +const LONG_SECRET = 'a-secret-that-is-at-least-32-bytes-long'; + +describe('checkSecuritySettings', () => { + it('says nothing about a clean configuration', () => { + const problems = checkSecuritySettings( + { allowedOrigins: ['https://acme.com'], sessionSecrets: [LONG_SECRET] }, + {}, + ); + expect(problems).toEqual([]); + }); + + it('says nothing when there is nothing to check', () => { + expect(checkSecuritySettings({}, {})).toEqual([]); + }); + + it('reports a session secret shorter than 32 bytes', () => { + const problems = checkSecuritySettings({ sessionSecrets: ['short'] }, {}); + expect(problems).toEqual([ + { level: 'error', message: 'Session secret 1 is shorter than 32 bytes.' }, + ]); + }); + + it('counts the bytes of a secret, not its characters', () => { + // 16 two-byte characters are 32 bytes, so this one is long enough. + const problems = checkSecuritySettings({ sessionSecrets: ['é'.repeat(16)] }, {}); + expect(problems).toEqual([]); + }); + + it('reports a secret that is also a public environment variable', () => { + const problems = checkSecuritySettings( + { sessionSecrets: [LONG_SECRET] }, + { NEXT_PUBLIC_SESSION_SECRET: LONG_SECRET }, + ); + expect(problems).toEqual([ + { + level: 'error', + message: + 'Session secret 1 is also the value of the NEXT_PUBLIC_SESSION_SECRET environment variable, ' + + 'which is sent to the browser. Use a different secret.', + }, + ]); + }); + + it('reports a secret for every public environment variable prefix', () => { + for (const prefix of ['NEXT_PUBLIC_', 'VITE_', 'PUBLIC_', 'NUXT_PUBLIC_', 'EXPO_PUBLIC_', 'REACT_APP_']) { + const problems = checkSecuritySettings( + { sessionSecrets: [LONG_SECRET] }, + { [`${prefix}KEY`]: LONG_SECRET }, + ); + expect(problems).toHaveLength(1); + expect(problems[0].message).toContain(`${prefix}KEY`); + } + }); + + it('ignores a private environment variable with the same value', () => { + const problems = checkSecuritySettings( + { sessionSecrets: [LONG_SECRET] }, + { SESSION_SECRET: LONG_SECRET }, + ); + expect(problems).toEqual([]); + }); + + it('never puts the secret in a message', () => { + const secret = 'thirty-two-bytes-exactly-long-ok'; + const problems = checkSecuritySettings( + { sessionSecrets: [secret] }, + { NEXT_PUBLIC_KEY: secret }, + ); + expect(problems.length).toBeGreaterThan(0); + for (const problem of problems) { + expect(problem.message).not.toContain(secret); + } + }); + + it('reports two session secrets that are the same', () => { + const problems = checkSecuritySettings({ sessionSecrets: [LONG_SECRET, LONG_SECRET] }, {}); + expect(problems).toEqual([ + { level: 'warning', message: 'Session secrets 1 and 2 are the same.' }, + ]); + }); + + it('reports an allowed origin that is not a URL', () => { + const problems = checkSecuritySettings({ allowedOrigins: ['acme.com'] }, {}); + expect(problems).toEqual([ + { level: 'error', message: 'Allowed origin 1 must start with http:// or https://.' }, + ]); + }); + + it('reports an allowed origin that is not a valid URL', () => { + const problems = checkSecuritySettings({ allowedOrigins: ['https://'] }, {}); + expect(problems).toEqual([ + { level: 'error', message: 'Allowed origin 1 is not a valid URL.' }, + ]); + }); + + it('reports an allowed origin with a path, a query or a trailing slash', () => { + const problems = checkSecuritySettings( + { allowedOrigins: ['https://acme.com/app', 'https://acme.com/?a=1', 'https://acme.com/'] }, + {}, + ); + expect(problems).toHaveLength(3); + for (const problem of problems) { + expect(problem.level).toBe('error'); + expect(problem.message).toContain('must not have a path, a query or a trailing slash'); + } + }); + + it('allows a plain http:// origin on localhost', () => { + const problems = checkSecuritySettings( + { allowedOrigins: ['http://localhost:5173', 'http://127.0.0.1:8080'] }, + {}, + ); + expect(problems).toEqual([]); + }); + + it('warns about an http:// origin that is not local', () => { + const problems = checkSecuritySettings({ allowedOrigins: ['http://acme.com'] }, {}); + expect(problems).toEqual([ + { + level: 'warning', + message: 'Allowed origin 1 uses http://. Use https://, unless it is a local development server.', + }, + ]); + }); + + it('warns when "*" is used together with other origins', () => { + const problems = checkSecuritySettings({ allowedOrigins: ['*', 'https://acme.com'] }, {}); + expect(problems).toEqual([ + { level: 'warning', message: 'Allowed origins include "*", so the other 1 entries do nothing.' }, + ]); + }); + + it('allows "*" on its own', () => { + expect(checkSecuritySettings({ allowedOrigins: ['*'] }, {})).toEqual([]); + }); + + it('reads the environment from process.env when none is given', () => { + const secret = 'a-secret-that-is-at-least-32-bytes-long'; + process.env.NEXT_PUBLIC_CHECK_SECRET = secret; + try { + const problems = checkSecuritySettings({ sessionSecrets: [secret] }); + expect(problems).toHaveLength(1); + expect(problems[0].message).toContain('NEXT_PUBLIC_CHECK_SECRET'); + } finally { + delete process.env.NEXT_PUBLIC_CHECK_SECRET; + } + }); +});