Reject sslrootcert and unknown sslmode values in PostgreSQL DSNs - #442
Merged
Merged
Conversation
On the DSN-only path (--dsn / DSN env), which bypasses TOML validation, the PostgreSQL parser silently dropped sslrootcert unless sslmode was verify-ca or verify-full. libpq treats require + a root CA as verify-ca, so a connection string copied from psql connected with no server verification and no warning. The same DSN inside a TOML source was already rejected. - sslrootcert with sslmode=require, disable or unset now fails with a message pointing at sslmode=verify-ca, matching TOML validation. - Unrecognised sslmode values (libpq's allow/prefer, typos) no longer fall through to `ssl: true`, which verified against the system CA store and never fell back to plaintext. They are rejected with the list of valid modes, as TOML already does. Replaces the two tests that asserted the silent-ignore behaviour and documents the difference from libpq in docs/config/command-line.mdx. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012UzToxWXzC7oQ9xMnhM192
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Explicitly empty sslmode and sslrootcert values bypass validation and need coverage.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR makes PostgreSQL DSN SSL validation consistent with TOML validation and documents the supported behavior.
Changes:
- Rejects unsupported
sslmodevalues. - Rejects incompatible
sslrootcertcombinations. - Updates tests and command-line documentation.
| File | Summary |
|---|---|
src/connectors/postgres/index.ts |
Adds strict SSL mode and root certificate validation. |
src/connectors/__tests__/dsn-parser.test.ts |
Tests rejection behavior and defaults. |
docs/config/command-line.mdx |
Documents PostgreSQL SSL restrictions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to bytebase/dbhub#441.
Problem
A DSN passed with
--dsnor theDSNenv var skips TOML validation, so the PostgreSQL DSN parser is the only check. The parser silently droppedsslrootcertunlesssslmodewasverify-caorverify-full:sslmode=require&sslrootcert=ca.pemsslmode=disable&sslrootcert=ca.pemsslrootcert=ca.pem(nosslmode)The first case is the dangerous one. libpq treats
requireplus a root CA asverify-ca, so a connection string copied frompsqlexpects the server to be verified. DBHub connected unverified and gave no warning. The same DSN inside a TOML[[sources]]was already rejected.The parser had a second silent fallback. An unrecognised
sslmode(libpq'sallow/prefer, or a typo) becamessl: true, which verifies against the system CA store and never falls back to plaintext. That is the opposite of whatallow/prefermean. TOML already rejects unknown modes.Change
In
src/connectors/postgres/index.ts:sslrootcertwithsslmode=require,disableor nosslmodenow throws:sslrootcert requires sslmode 'verify-ca' or 'verify-full' (got '…'). Use sslmode=verify-ca …sslmodeother thandisable/require/verify-ca/verify-fullnow throwsUnsupported sslmode '…'. Valid values: …. Thessl: truefallback is removed.Both rules match what TOML validation already enforces, so the DSN and TOML paths now accept exactly the same inputs. This is the same fail-fast choice #441 made for
sslcert/sslkey.Behaviour change: a
--dsnthat uses either combination now fails at startup with a clear message instead of connecting. That's intended, but worth a line in the release notes.Tests / docs
dsn-parser.test.ts: replaced the two tests that asserted the silent-ignore behaviour. The new tests cover rejection withrequire,disableand unset, rejection ofprefer/allow/verify_full/true, and that nosslmodestill leavessslunset.docs/config/command-line.mdx: thesslrootcertrow, plus a note on how DBHub differs from libpq here (no automaticrequire→verify-caupgrade, noallow/prefer).pnpm test:unit: 1156 passed;pnpm run buildOK.🤖 Generated with Claude Code
https://claude.ai/code/session_012UzToxWXzC7oQ9xMnhM192
Generated by Claude Code