fix(db): one SSL decision shared by every pool (DB-11) - #17
Conversation
migrate.ts and seed.ts each carried their own copy of
`NODE_ENV === 'production' ? { rejectUnauthorized: false } : false`.
client.ts -- the pool the whole application runs on -- carried no `ssl`
key at all, and neither did scripts/orphan-diagnostic.ts.
An absent `ssl` key does not mean "let pg decide sensibly". pg resolves
`this.ssl = typeof config.ssl === 'undefined' ? readSSLConfigFromEnvironment()
: config.ssl`, and with PGSSLMODE unset that lands on `defaults.ssl === false`.
So the application pool connected in plaintext, unconditionally, including
in production.
It never surfaced on AWS because Aurora is in-VPC and the connection is
internal. It bites against any managed Postgres requiring TLS on a public
endpoint. The signature misdirects, too: Dockerfile:35 is
`migrate.js && index.js`, so migrate configures SSL, connects, exits 0, and
then the app fails -- logs read "migration fine, database unreachable",
which looks like a database fault rather than a client-config one.
The drift is the defect, so this adds api/src/db/ssl.ts as the single
decision instead of a fourth copy of the ternary. Behaviour outside
production is unchanged: previously `false` by pg default, now `false` by
explicit decision.
`rejectUnauthorized: false` is carried over verbatim, not endorsed -- it
encrypts but does not verify the chain. Tightening it needs a CA bundle
decided first, so it is a deliberate follow-up rather than a silent
posture change; the reasoning is in the ssl.ts header.
api/scripts/*.ts are left alone on purpose: they set ssl unconditionally
and always target a remote endpoint, so routing them through a
NODE_ENV-conditional helper would downgrade them to plaintext.
Refs TRO-240 (DB-11)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three groups, because the defect had three faces.
1. The decision itself, per NODE_ENV -- including that "production" is
matched exactly, so a deploy setting NODE_ENV=Production cannot silently
drop to plaintext.
2. That the application pool actually applies it. client.ts builds its pool
at module scope, so this stubs NODE_ENV, resets the module registry and
re-imports. Constructing a pg Pool opens no socket, so no database is
touched. Also asserts the option survives pg's config merge: pg does
`Object.assign({}, config, parse(connectionString))`, so an explicit
`ssl` only survives because parse() omits the key when the URL has no
sslmode. If a pg upgrade changes that, the fix becomes a silent no-op
and this test is the only thing that would notice.
3. That no file under api/src/db sets `ssl` to anything but
resolveDatabaseSsl(). This is the one that prevents recurrence: a future
`new Pool(...)` with its own policy fails the suite rather than quietly
adding a fifth.
Verified red first against the unfixed call sites: 7 failed / 8 passed, every
failure an AssertionError on the claimed behaviour, the headline being
`expected undefined to deeply equal { rejectUnauthorized: false }` for the
application pool under NODE_ENV=production.
What this CANNOT prove: that TLS actually negotiates. That needs a managed
Postgres endpoint requiring TLS on a public address, which this repo's test
environment does not have. These assertions cover the decision and its
propagation to every call site -- everything up to the socket.
Refs TRO-240 (DB-11)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records what was broken, why AWS never showed it, why the Docker `migrate && index` chain makes the logs read as a database fault, and the two scoping calls: rejectUnauthorized:false carried over rather than tightened, and api/scripts/*.ts left on their unconditional-TLS policy because a NODE_ENV-conditional helper would downgrade them. Marks the end-to-end claim unverified: nothing here proves TLS negotiates, because that needs a managed endpoint requiring TLS and the local docker Postgres speaks plaintext only. Refs TRO-240 (DB-11) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 seconds Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughDatabase SSL configuration is centralized in ChangesDatabase SSL configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PoolCaller
participant resolveDatabaseSsl
participant pgPool
PoolCaller->>resolveDatabaseSsl: Resolve SSL options from environment and DATABASE_URL
resolveDatabaseSsl-->>PoolCaller: Return TLS configuration or false
PoolCaller->>pgPool: Create PostgreSQL Pool with ssl option
pgPool-->>PoolCaller: Apply connection parameters and sslmode precedence
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Brings in gate G7b, review-patterns.mjs and merge-changes.mjs. CHANGES.md needed re-merging by hand: my merge-base still carried `CHANGES.md merge=union` in .gitattributes, and the union driver spliced the file exactly as merge-changes.mjs's header predicts -- it drops the shared context lines, which here are `**How to run it.**` and the ``` fences, leaving 13 fences (odd) and my TRO-240 entry with no run block and no rollback block. Redone with `merge-changes.mjs --ours --theirs`: 10 entries, 14 fences, all entries byte-identical to their source. `--check` passes. The merge driver is gone from .gitattributes as of this merge, so the next one conflicts loudly instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntext
CodeRabbit, MAJOR, on the ticket's own premise: nothing proved a connection
-string parameter could not override the resolved SSL object. It can, and
does.
Established by reading pg rather than inferring it from the earlier finding,
then confirmed empirically (pg 8.16.3 / pg-connection-string 2.9.1):
pg-connection-string:76 sslmode present -> config.ssl = {}
pg-connection-string:133-135 case 'disable' -> config.ssl = false
connection-parameters.js:56 Object.assign({}, config, parse(connString))
-- parse() is the LAST source, so its ssl key
overwrites the caller's. The comment on :54
says so outright.
connection-parameters.js:81 that value is then used as-is
Effective order, weakest to strongest: pg defaults -> PGSSLMODE -> the ssl
option this helper returns -> sslmode in the connection string.
Measured, passing an explicit { rejectUnauthorized: false } throughout:
absent -> {rejectUnauthorized:false}; disable -> false (PLAINTEXT); prefer,
require, verify-ca, verify-full -> {}; no-verify -> {rejectUnauthorized:false}.
So `disable` is the only value that discards our option, and it is exactly the
one a DATABASE_URL copied from a provider dashboard can carry.
The ssl option can never win that argument, so resolveDatabaseSsl now refuses
to start in production when the URL's sslmode resolves to plaintext, naming the
parameter and the remedy. It deliberately does NOT rewrite the URL: silently
editing an operator's explicit instruction is the same class of mistake as the
original bug -- code reporting one thing and doing another.
Outside production sslmode=disable is still allowed; local Postgres and the CI
container are plaintext-only and the guard would otherwise be unsatisfiable.
Two of the eight new tests were red first (`expected [Function] to throw an
error`). The other six assert behaviour that must NOT change, and two of those
characterise pg itself -- if a future pg makes the explicit option win, they
fail, which is the signal this guard can be relaxed.
DEPLOYMENT PRECONDITION: if the production DATABASE_URL in SSM already contains
sslmode=disable, this converts a working in-VPC deploy into a startup failure.
The value is in SSM and could not be inspected from here.
Refs TRO-240 (DB-11)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review triage — the finding was a LIVE DEFECT, not a coverage gapGate: Which layer wins, and how it was establishedThe connection string wins. Established by reading pg source — independently of the earlier
Effective precedence, weakest → strongest: pg defaults → Measured, passing an explicit
The fix is code, not a testBecause the It deliberately does not rewrite the URL. Silently editing an operator's explicit instruction is the same class of mistake as the original bug — code reporting one thing while doing another. Outside production,
Tests — with the red/green split stated honestly22 cases total. Of the 8 new ones, 2 were red against the unguarded helper (both Two of those 6 characterise pg itself: they pin that The compiled artifact was exercised too, since
|
# Conflicts: # CHANGES.md # api/src/db/migrate.ts
1 participant
TRO-240 — [DB-11] One SSL decision, shared by every pool
Closes TRO-240. Post-baseline finding
DB-11(not one of the audit's 68).What was broken
api/src/db/client.ts:17-26built the pool the entire application uses with nosslkey atall, while
migrate.ts:32andseed.ts:44each carried their own copy of the conditional.An absent
sslkey is not "let pg decide sensibly" — verified in pg 8.16.3 source rather thanassumed.
connection-parameters.js:100doesthis.ssl = typeof config.ssl === 'undefined' ? readSSLConfigFromEnvironment() : config.ssl, and withPGSSLMODEunset that lands ondefaults.ssl, which isfalse(defaults.js:43). The app poolconnected in plaintext, unconditionally, in production.
A fourth pool had the same defect and was not in the ticket:
api/src/db/scripts/orphan-diagnostic.ts:34. Fixed — same one-line change through the same helper.Leaving it would have left the trap.
Why the failure signature is confusing
Dockerfile:35isnode dist/db/migrate.js && node dist/index.js.migrate.tsdoes configure SSL,so it connects, runs, exits 0, and the
&&proceeds. Thenindex.jsstarts andclient.tsfails.Logs show a successful migration immediately followed by a connection error — reading like a database
problem rather than a client-config one.
connectionTimeoutMillis: 2000makes it fail fast andcrash-loop instead of surfacing a clear TLS error.
What changed
api/src/db/ssl.ts—resolveDatabaseSsl(nodeEnv = process.env.NODE_ENV). All four pools underapi/src/db/call it. The value is unchanged from what the scripts already did.The helper is not what prevents recurrence. The third test group is: it walks
api/src/db/fornew Pool(, pins the known site list, and asserts no file setssslto anything butresolveDatabaseSsl(). A future file adding its own policy fails the suite. The helper alone wouldhave left the next author free to hand-roll a fifth.
rejectUnauthorized: falsewas carried over verbatim, not tightened. It encrypts but does notverify the chain. A federal deployment likely wants
rejectUnauthorized: trueplus an explicitca—that needs the CA bundle decided first, so it is a deliberate follow-up rather than a silent posture
change. Reasoning is in the
ssl.tsheader and inCHANGES.md.Evidence
api/src/db/__tests__/ssl.test.ts— 15 cases, 7 failed / 8 passed beforeverdict: passscripts/factory/gate.sh, run by the orchestrator independentlyDATABASE_URL=…ship_wt_tro_240, docker:5433,NODE_ENVunset so vitest setstestweb/touchedreview-patterns!/as any/as unknown as/fixed sleepsObserved. The headline red failure was an
AssertionErroron the claimed behaviour, not an importerror:
Beyond source, the compiled artifact was checked:
api/dist/db/ssl.jsis emitted and all fourcompiled call sites import it with correct relative paths, so
Dockerfile:35resolves the module.Running compiled
dist/db/client.jswith a dummy URL gaveNODE_ENV=production → {"rejectUnauthorized":false}and
NODE_ENV=development → false. Everything up to the socket is observed.Also confirmed nothing mutates
NODE_ENVat runtime —ssm.tssetsDATABASE_URL/SESSION_SECRETbut only reads
NODE_ENV(:39) — so reading it at module scope inclient.tsis safe.Two bugs in the test itself were caught by the red/green cycle and are worth recording: a
negative-lookahead regex (
ssl:\s*(?!helper)) that matched correct code because\s*can consumenothing, and
Client.connectionParametersbeing absent from@types/pg— resolved withReflect.get(returns
any, so no assertion operator) rather thanas any/as unknown as, both of which aregate G7b failures.
Not verified — and this is the important limitation
That TLS actually negotiates. Proving it needs a managed Postgres requiring TLS on a public
endpoint; the local docker Postgres speaks plaintext only, so a green local suite is silent on the
real failure mode. "Render now starts" is untested — confirming it means deploying and reading the
startup logs.
Found, not fixed
orphan-diagnostic.ts:180queriesd.program_id IS NULL— a column dropped by029_drop_program_id_column.sql(confirmed present). That script's "projects without program"query throws on any database that has run 029. Pre-existing, unrelated to SSL, needs its own ticket.
api/scripts/migrate-shadow.ts:32,create-test-user.ts:35,check-db-user.ts:10,19setssl: { rejectUnauthorized: false }unconditionally. They are operator scripts outside
api/tsconfig.json'sinclude: ["src/**/*"],always aimed at a remote AWS endpoint, and routing them through a
NODE_ENV-conditional helperwould silently downgrade them to plaintext whenever
NODE_ENVis unset — which is how they arenormally invoked. Documented as out-of-scope in
CHANGES.md.Rollback
git revertthe three commits. That restores the missingsslkey on the app pool and the duplicatedconditionals in
migrate.ts/seed.ts. No schema change, no migration, no data touched.Summary by CodeRabbit
Bug Fixes
Tests