Skip to content

Require JWT secret configuration - #2868

Open
junjun107 wants to merge 2 commits into
developfrom
security-require-jwt-secret
Open

Require JWT secret configuration#2868
junjun107 wants to merge 2 commits into
developfrom
security-require-jwt-secret

Conversation

@junjun107

@junjun107 junjun107 commented Aug 25, 2026

Copy link
Copy Markdown
Member

7 on the list

Fail at startup instead of signing tokens with an insecure fallback secret, and cover the configuration behavior with regression tests.

Co-authored-by: Cursor <cursoragent@cursor.com>

@hanapotski hanapotski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve, with a couple of operational follow-ups to confirm before/at merge.

This is a clean, correct security fix. Removing the hardcoded JWT signing-secret fallback and failing fast at startup when JWT_SECRET is unset is the right posture — a committed fallback secret in a public repo means anyone can forge valid JWTs for any role, so fail-closed beats silently signing with a weak key.

What's good

  • Minimal and focused — one meaningful line changed, no scope creep.
  • Fail-fast at startup rather than lazily at first sign/verify. Since the secret is a hard requirement, crashing on boot is the correct failure mode.
  • Regression tests included, covering both the missing and configured cases, using jest.isolateModules so the tests don't pollute each other's module state. Location/naming matches the existing suite.
  • No collateral test breakage: only routers import jwt-session, and the existing test files import controllers directly (not routers), so the new throw won't break the existing suite at import time.

Things to confirm before merging (not code defects — consequences of fail-closed)

The server now refuses to boot without JWT_SECRET, so every environment that runs it needs the var set. Two gaps:

  1. docker-compose.yml will crash-loop. The api service sets DATABASE_URL, POSTGRES_*, and WAIT_FOR but no JWT_SECRET. After this change, docker-compose up (a documented local workflow) throws at startup. Consider adding JWT_SECRET to that service's environment: block in this PR so the local Docker path keeps working.

  2. Heroku config vars. Each auto-deploy app (foodoasis, foodoasisdev, foodoasisvite) must already have a JWT_SECRET config var set, or the next deploy crash-loops. Worth verifying in Heroku config before merging.

  3. Local dev onboarding (minor). Contributors running the server locally now need JWT_SECRET in server/.env. There's no .env.example to signal that — a one-line doc note or sample env entry would save newcomers a confusing "why won't the server start." Fine as a follow-up.

Nitpick (optional)

Good that the weak fallback string is gone. Keep it out of the PR/commit text verbatim as this lands in the public repo — the current commit message is appropriately generic, so no change needed.

Net: the code change itself is correct and I'd approve it. The one gap inside the diff is that docker-compose.yml should also get a JWT_SECRET so the fail-fast doesn't break the documented local container workflow; the Heroku check is an out-of-band confirmation.


Written by Claude on behalf of @hanapotski.

Fail-fast startup now requires JWT_SECRET, which docker-compose up did not set, so the documented local container workflow crash-looped.
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.

2 participants