Skip to content

fix(security): validate required env vars (SMB_ADDRESS, CORS_ORIGIN) at startup - #346

Merged
birme merged 3 commits into
mainfrom
security/232-env-startup-validation
Sep 23, 2026
Merged

birme merged 3 commits into
mainfrom
security/232-env-startup-validation

Conversation

@birme

@birme birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The server previously started even when critical environment variables were unset, silently falling back to defaults. This adds fail-fast startup validation for SMB_ADDRESS and CORS_ORIGIN: if either is missing, the process logs which variable is missing and exits with code 1 before the server begins listening.

The check is placed at the top of the startServer() startup path in src/server.ts (before dbManager.connect() and server.listen()), so it only triggers on actual boot and does not affect module-import behavior. DB_CONNECTION_STRING is intentionally excluded (tracked separately in #225).

Test plan

  • npm run typecheck passes
  • npm test shows no regressions from this change (identical pass/fail counts vs. clean main; the 94 pre-existing failures are an unrelated @fastify/cookie dynamic-import issue from a recent dependency bump)
  • Manual review: with SMB_ADDRESS or CORS_ORIGIN unset, startup exits 1 with a clear message; with both set, boot proceeds normally

Closes #232

… at startup

Fail fast at boot if SMB_ADDRESS or CORS_ORIGIN is unset instead of
silently falling back to defaults. The check runs inside the startServer
startup path before the server begins listening.

Closes #232

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Code Review — Verdict: Needs Changes

Self-authored PR; verdict recorded as a marker (GitHub blocks state-bearing self-review by the author's own account).

Blocking

  • src/server.ts:61-69 — the startup-validation behavior change ships with zero tests. Add coverage that missing/empty SMB_ADDRESS => exit(1), missing/empty CORS_ORIGIN => exit(1), and both-set => startup proceeds.

Warnings

  • src/server.ts:10-14 vs 61-68 — contradiction: module-level code still sets an SMB_ADDRESS default and logs "using defaults", but startServer() now exit(1)s when it's unset, so that default is dead/misleading. Remove it or reconsider requiring SMB_ADDRESS.
  • src/server.ts:66 — uses raw console.error instead of the project Log() convention used everywhere else in the file.

Note: requiring CORS_ORIGIN is a safe hardening (src/api.ts already sets origin:false when unset) but deploys that relied on that silent default will now refuse to boot — call this out in release notes.

Moving issue #232 back to Ready for a follow-up commit on this same branch.

…ESS default

Extract the required-env check from the startServer IIFE into an exported
validateRequiredEnv() so it can be unit-tested, and only auto-start the server
when server.ts is run directly (require.main === module) rather than when
imported. Add Jest coverage asserting exit(1) on missing/empty SMB_ADDRESS and
CORS_ORIGIN, and no exit when both are set.

Also reconcile the contradictory SMB_ADDRESS handling: drop the misleading
localhost default and "using defaults" warning now that SMB_ADDRESS is required,
and guard the URL-parse warning against an unset value. Switch the validation
error from raw console.error to the project Log() logger.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@birme

birme commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Code Review — Verdict: LGTM (self-authored PR; recorded as marker since GitHub blocks state-bearing self-review by the author's own account)

The follow-up commit adds src/server.test.ts (5 tests) covering missing/empty SMB_ADDRESS and CORS_ORIGIN => exit(1) and the both-set happy path, via a throwing process.exit mock and mocked ./api/./db/*. The require.main === module guard is correct (server still starts under ts-node in prod, stays inert under Jest import). Dead SMB_ADDRESS default removed, plaintext-URL warning guarded, validation error switched to Log().error. New test suite is self-contained and independent of the ~94 pre-existing environmental Jest failures.

Non-blocking: making CORS_ORIGIN hard-required is by design per #232 but is a deploy-affecting change — update readme.md:70 and confirm OSC/Docker deploys set CORS_ORIGIN (call out in release notes). DB-protocol validation still throws at import before env validation (tracked separately in #225).

Awaiting a human with a different account to approve + merge (required review; automation account cannot self-approve).

@birme
birme merged commit 2a9f849 into main Sep 23, 2026
4 checks passed
@birme
birme deleted the security/232-env-startup-validation branch September 23, 2026 16:17
birme added a commit that referenced this pull request Sep 30, 2026
PR #346 made CORS_ORIGIN a required env var, so instances on Eyevinn
Open Source Cloud (where arbitrary per-instance env vars can't be set)
failed to boot. Resolve the allowed origin from CORS_ORIGIN first, then
fall back to the OSC-injected OSC_HOSTNAME, and only fail fast when
neither is set.

Closes #383

Co-authored-by: birme <birme@eyevinn.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Add startup validation for SMB_ADDRESS and CORS_ORIGIN environment variables

2 participants