security: require secret env vars at startup, remove dev fallbacks - #41
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesThis change adds shared required-environment validation. Auth-service and pds-core now reject startup when required secrets are missing. The unused CSRF secret configuration is removed from code, setup scripts, examples, and documentation. Required secret environment variables
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the pr-0127fa-41 environment in ePDS
|
|
Coverage Report for CI Build 31015296732Coverage increased (+0.1%) to 58.051%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/index.ts`:
- Around line 124-133: requireEnv currently accepts whitespace-only values
(e.g., " ") as valid, so update the function requireEnv to reject values that
are empty after trimming: retrieve process.env[name], check if value is
undefined or value.trim().length === 0, and throw the existing Error if so; keep
the same error message but ensure values consisting only of whitespace are
treated as missing. Use the function name requireEnv to locate and modify the
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efd7433b-d33c-47c0-9248-b6fa2724a6f4
📒 Files selected for processing (11)
.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/__tests__/csrf.test.tspackages/auth-service/src/context.tspackages/auth-service/src/index.tspackages/auth-service/src/middleware/csrf.tspackages/pds-core/.env.examplepackages/pds-core/src/index.tspackages/shared/src/__tests__/types.test.tspackages/shared/src/types.ts
💤 Files with no reviewable changes (4)
- packages/auth-service/src/tests/consent.test.ts
- packages/auth-service/src/context.ts
- packages/shared/src/types.ts
- packages/shared/src/tests/types.test.ts
Replace hardcoded development secret fallbacks with a requireEnv() helper that throws immediately if AUTH_SESSION_SECRET or EPDS_CALLBACK_SECRET are missing. This prevents services from silently running with well-known secrets in production. Also removes the unused csrfSecret config field and parameter — the CSRF middleware already uses crypto.randomBytes and ignores it. Closes #37
67e41b4 to
d4256ea
Compare
🦋 Changeset detectedLatest commit: 406421e The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR hardens ePDS startup configuration by removing hardcoded development secret fallbacks and making required secrets fail-fast at process startup, while also cleaning up dead CSRF secret configuration.
Changes:
- Introduces a shared
requireEnv()helper and switches auth-service + pds-core to requireAUTH_SESSION_SECRET/EPDS_CALLBACK_SECRETat startup (no moredev-*-change-medefaults). - Removes the unused CSRF secret config surface (
AUTH_CSRF_SECRET, config field, and the unused middleware parameter). - Updates example env files, docs, setup script, and adds tests/changeset for the behavior change.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/setup.sh | Stops generating / wiring AUTH_CSRF_SECRET in setup output. |
| packages/shared/src/types.ts | Removes dead csrfSecret field from shared config type. |
| packages/shared/src/require-env.ts | Adds requireEnv() helper for required env vars. |
| packages/shared/src/index.ts | Exports requireEnv() from shared barrel. |
| packages/shared/src/tests/types.test.ts | Updates type-shape compile-time test after removing csrfSecret. |
| packages/shared/src/tests/require-env.test.ts | Adds unit tests for requireEnv(). |
| packages/pds-core/src/index.ts | Requires EPDS_CALLBACK_SECRET at startup via requireEnv(). |
| packages/pds-core/.env.example | Marks EPDS_CALLBACK_SECRET as required in example env. |
| packages/auth-service/src/middleware/csrf.ts | Removes unused _secret parameter from csrfProtection(). |
| packages/auth-service/src/index.ts | Requires secrets via requireEnv(); updates CSRF middleware callsite. |
| packages/auth-service/src/context.ts | Removes csrfSecret from auth-service config interface. |
| packages/auth-service/src/tests/login-page-prompt-login.test.ts | Updates test app wiring for new csrfProtection() signature. |
| packages/auth-service/src/tests/csrf.test.ts | Updates CSRF middleware tests for new signature. |
| packages/auth-service/.env.example | Removes AUTH_CSRF_SECRET, clarifies required secrets. |
| docs/configuration.md | Updates AUTH_SESSION_SECRET docs to reflect required startup behavior. |
| .env.example | Removes AUTH_CSRF_SECRET and annotates required secrets. |
| .changeset/require-secret-env-vars.md | Adds operator-facing release note for required secrets and CSRF-secret removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai please review |
|
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
requireEnv() treated whitespace-only values (e.g. ' ') as present, so a secret set to blanks would boot with an effectively empty value rather than failing loudly. Trim before the emptiness check, and mark EPDS_CALLBACK_SECRET as required in docs/configuration.md — both pds-core and auth-service call requireEnv() on it and refuse to start when unset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the EPDS_CALLBACK_SECRET required-note change: prettier re-pads the Shared variables table column after the cell length changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/shared/src/tests/require-env.test.ts:28
- The inline comment claims an empty secret would "produce unsigned cookies", but the service would still sign values — just with an empty/predictable secret. This is a misleading security description for why empty/whitespace values must fail fast.
// An empty value is as dangerous as an absent one — it would silently
// produce unsigned cookies rather than failing loudly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.changeset/require-secret-env-vars.md:3
- The changeset marks this as a
patchrelease, but perdocs/PUBLISHING.md's bump-type guide, removing/renaming env vars and changes that require operator action are breaking changes and should not be shipped as a patch. This PR makesAUTH_SESSION_SECRET/EPDS_CALLBACK_SECRETmandatory at startup and removesAUTH_CSRF_SECRET, so the bump should be at leastminor(pre-1.0 guidance) rather thanpatch.
---
'ePDS': patch
---
|
Closing and immediately reopening to retrigger the Railway preview deploy — no code change intended. WhyThe PDS, Auth, and Demo were all reachable on attempt 1. Only This looks like preview-environment drift rather than anything in this branch:
The branch has been open since March, so the preview environment has most likely been torn down or gone stale, and the deploy check never fired to recreate it. Reopening should provision it again and let the e2e suite actually run. Reopening momentarily. |
|
Summary
'dev-*-change-me'fallbacks forAUTH_SESSION_SECRETandEPDS_CALLBACK_SECRETwith arequireEnv()helper that throws at startup if the variable is missingpds-core/src/index.tsforEPDS_CALLBACK_SECRETcsrfSecretconfig field and unused_secretparameter incsrfProtection()— the middleware usescrypto.randomBytesand ignores it.env.examplefiles withREQUIREDannotations and removeAUTH_CSRF_SECRETMotivation
If these env vars are not set in production, services previously ran silently with well-known secrets, allowing an attacker to forge session cookies (
AUTH_SESSION_SECRET) or HMAC callback signatures (EPDS_CALLBACK_SECRET).Breaking Change
Services will now refuse to start without
AUTH_SESSION_SECRETandEPDS_CALLBACK_SECRETset. Existing deployments that rely on the dev defaults must set these env vars before upgrading.Generate values with:
openssl rand -hex 32Closes #37
Summary by CodeRabbit
Summary by CodeRabbit