fix: honor and validate source fields set alongside a DSN (#334)#337
Merged
Conversation
When a TOML source provided a `dsn`, buildDSNFromSource returned it verbatim, so any field also set on the source (sslmode, sslrootcert, instanceName, authentication, domain, and the connection identity fields) was silently ignored at connection time while still appearing in API/metadata. This was the footgun reported in #334 for sslmode. - mergeSourceFieldsIntoDSN: merge dual-home fields (fields that can also be expressed as DSN query params) into the DSN when it lacks them, mirroring the connection-params query builder. - validateDSNFieldConflicts: reject any identity or dual-home field that contradicts the DSN (matching/absent is fine). The DSN wins at connection time, so a differing field is now a hard load-time error instead of being silently dropped. Password is compared without echoing values. Fixes #334 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Note that fields which can also live in the DSN query string are merged when the DSN omits them, and that a field contradicting the DSN is rejected at startup instead of being silently ignored. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration “footgun” in the TOML loader where supplying a dsn caused other source fields (notably “dual-home” DSN query params like sslmode) to be silently ignored at connection time, despite still appearing in config metadata. It now merges supported standalone fields into the DSN when missing and fails fast on contradictions.
Changes:
- Add DSN-vs-field conflict validation to reject contradictory identity and dual-home fields at load time.
- Merge dual-home fields (
sslmode,sslrootcert, and SQL Server params) into an existing DSN when the DSN omits them. - Update/add TOML loader tests to cover conflict rejection and merge behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/config/toml-loader.ts | Adds DSN conflict validation and merges dual-home fields into DSNs to ensure standalone TOML fields affect actual connections. |
| src/config/tests/toml-loader.test.ts | Updates tests for the new fail-fast conflict behavior and verifies DSN merging behavior. |
- Detect SQLite from the DSN's parsed type instead of the source.type field, and check the type conflict before short-circuiting. A `type = "sqlite"` paired with a non-SQLite DSN (or the reverse) is now rejected rather than silently skipped. - Distinguish "DSN has no password" from "DSN has a different password" in the password-conflict error, without echoing either value. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- Compare hostnames case-insensitively (hostnames are case-insensitive, so DB.EXAMPLE.COM and db.example.com are the same host). - Avoid a "?&" sequence when merging into a DSN that already ends with a bare "?" (empty query string). - Reword validateDSNFieldConflicts docstring to describe the general "user set both DSN and field" invariant instead of listing only the subset of fields copied by processSourceConfigs. - Add merge tests for sslrootcert (postgres + verify-ca, with encoding), SQL Server authentication/domain, the verify-mode guard, and the bare "?" separator case. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
SafeURL drops query params with empty values (e.g. ?sslmode=), so an empty-but-present param looked absent — a conflicting field went undetected and the merge step could append a duplicate, yielding an ambiguous ?sslmode=&sslmode=require. Add getRawDSNQueryParam() to read raw key presence (distinguishing absent vs present-but-empty) and use it for both the dual-home conflict checks and the merge presence checks. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.
Problem
Closes #334.
When a TOML source specified a
dsn,buildDSNFromSource()returned the DSN verbatim — so any field also set on the source was silently ignored at connection time, while still appearing in API/metadata. The reporter setsslmode = "require"next to adsn(the pattern the docs recommend) and it did nothing; their DB rejected the non-SSL connection.Reviewing
docs/config/toml.mdx, the same footgun applied to every field that can also be encoded in a DSN, not justsslmode:dsntype, host, port, database, user, passwordsslmode, sslrootcert, instanceName, authentication, domainconnection_timeout, query_timeout, lazy, search_path, timezone, ssh_*, …Changes
mergeSourceFieldsIntoDSN()— when adsnis present, merge the dual-home fields into the DSN query string when absent, mirroring exactly what the connection-params path ofbuildDSNFromSource()produces. Sodsn+sslmode = "require"(orinstanceName = "ENV1", etc.) now actually affects the connection.validateDSNFieldConflicts()— reject any identity or dual-home field that contradicts the DSN. The DSN wins at connection time, so a differing field is now a hard load-time error instead of a silent drop. Matching/absent values are fine. Password is compared against the DSN directly and the error never echoes the values.Behavior change
The only user-visible change: a config that set an identity/dual-home field to a value different from the DSN used to silently ignore the field — it now errors at load time (fail-fast). Matching or DSN-only configs are unaffected, and the docs already describe the now-correct behavior (no doc change needed).
Tests
host,user,database,password,instanceName,sslmode, plus merge tests forsslmode/sslrootcert/instanceName.toml-loader.test.ts; full non-integration suite green apart from a pre-existingjson-rpc-integrationserver-spawn flake that fails identically onmain.🤖 Generated with Claude Code