fix(db): enable TLS certificate validation by default for DB connections#979
fix(db): enable TLS certificate validation by default for DB connections#979
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Good security fix — enabling TLS certificate validation by default is the right call. Implementation is clean, tests are thorough, and CI is green. Two items worth addressing.
Code Issues
Should Fix
-
tools/migrate-hooks.ts:127 — This file still has
rejectUnauthorized: false. The PR fixessrc/db/client.tsbut leaves an identical insecure pattern in the migration tool. Even though it's a standalone script, it connects to the same database and should get the same security treatment (ideally by reusinggetSslConfig()or at minimum applying the same default). Not in this diff, but worth fixing as part of this effort. -
src/db/client.ts:38 —
fs.readFileSync(process.env.DATABASE_CA_CERT, ...)has no error handling. If the operator setsDATABASE_CA_CERTto a nonexistent or unreadable path, they'll get a rawENOENT/EACCESerror during pool initialization. The codebase commonly guardsreadFileSyncwithexistsSync(seesrc/backends/completion.ts,src/utils/cascadeEnv.ts,src/backends/codex/index.ts). Consider adding anexistsSynccheck with a descriptive error message likethrow new Error(\DATABASE_CA_CERT file not found: ${certPath}`)` to help operators diagnose misconfiguration at startup.
🕵️ claude-code · claude-opus-4-6 · run details
…oks TLS - Add existsSync check before readFileSync in getSslConfig() with a descriptive error message to help operators diagnose misconfiguration - Fix tools/migrate-hooks.ts to use rejectUnauthorized: true (same as src/db/client.ts) by extracting an identical getSslConfig() helper, also with the existsSync guard for DATABASE_CA_CERT - Add test coverage for the new error case when DATABASE_CA_CERT path does not exist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review @nhopeatall! Both issues are addressed in the latest commit.
🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean security fix that changes the default TLS behavior from rejectUnauthorized: false to true, properly enabling certificate validation for production database connections. The DATABASE_CA_CERT env var provides a clean escape hatch for managed databases with private CAs.
All pg.Pool instantiation sites (2 total: src/db/client.ts and tools/migrate-hooks.ts) are updated consistently. No remaining instances of rejectUnauthorized: false in the codebase. Test coverage is thorough — all five SSL scenarios are tested. CI passes cleanly.
Minor Observations
getSslConfigduplication (tools/migrate-hooks.ts:20): The function is a verbatim copy ofgetSslConfig()fromsrc/db/client.ts. Not blocking since this is a standalone migration script, but worth noting — if more tools need DB connections in the future, consider extracting to a shared module.- Breaking change for self-signed certs: Deployments using self-signed certificates without configuring
DATABASE_CA_CERTwill break. This is the correct trade-off for a security fix, and the migration path (setDATABASE_CA_CERT) is well-documented.
🕵️ claude-code · claude-opus-4-6 · run details
The dev database uses a self-signed certificate chain, which started failing after TLS rejectUnauthorized was enabled by default in #979. Add DATABASE_SSL=false to all migration steps in the dev deploy workflow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g services PR #979 tightened DB SSL defaults to rejectUnauthorized: true, but the dev database uses a self-signed certificate. The deploy workflow already passed DATABASE_SSL=false to one-off migration containers via -e flags, but the long-running router and dashboard containers read their env from /opt/services/cascade-dev.env — which never had this variable set. Result: every router startup since that PR crashed at seedAgentDefinitions with "self-signed certificate in certificate chain" before the process could serve any traffic. Add an idempotent step (sed removes any existing line, echo appends the correct value) that runs once per deploy, before docker compose restarts both services. Since both containers share the same env_file, a single write fixes both the router and the dashboard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g services (#986) PR #979 tightened DB SSL defaults to rejectUnauthorized: true, but the dev database uses a self-signed certificate. The deploy workflow already passed DATABASE_SSL=false to one-off migration containers via -e flags, but the long-running router and dashboard containers read their env from /opt/services/cascade-dev.env — which never had this variable set. Result: every router startup since that PR crashed at seedAgentDefinitions with "self-signed certificate in certificate chain" before the process could serve any traffic. Add an idempotent step (sed removes any existing line, echo appends the correct value) that runs once per deploy, before docker compose restarts both services. Since both containers share the same env_file, a single write fixes both the router and the dashboard. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Migration containers were missing DATABASE_SSL and DATABASE_CA_CERT, causing SELF_SIGNED_CERT_IN_CHAIN failures after TLS cert validation was enabled by default in #979. Add --env-file /opt/services/cascade.env to the three migration steps (db migrate, trigger config migration, hooks migration) so they pick up the same SSL configuration already used by the re-encrypt step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Migration containers were missing DATABASE_SSL and DATABASE_CA_CERT, causing SELF_SIGNED_CERT_IN_CHAIN failures after TLS cert validation was enabled by default in #979. Add --env-file /opt/services/cascade.env to the three migration steps (db migrate, trigger config migration, hooks migration) so they pick up the same SSL configuration already used by the re-encrypt step. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
rejectUnauthorized: falsetorejectUnauthorized: true, enabling proper certificate validation for production database connections (H2 HIGH security fix)DATABASE_CA_CERTenv var — Supports custom CA certificates for managed databases (AWS RDS, Azure, GCP Cloud SQL) by reading a PEM file and passing it as thecaoption topg.PoolDATABASE_SSL=falsestill disables SSL entirely; existing deployments using standard trusted CAs work without changesCLAUDE.mdwithDATABASE_CA_CERTenvironment variable documentationTest plan
tests/unit/db/client.test.tspassDATABASE_CA_CERTignored whenDATABASE_SSL=falseaddedTrello Card
https://trello.com/c/69c00fefd8fd575563861ce4
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details