From 89c523f7cbfeac2744bc3ac6792a9baeb071bc95 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 23 Jun 2026 20:51:56 +0800 Subject: [PATCH 1/5] fix: honor and validate source fields set alongside a DSN (#334) 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) --- src/config/__tests__/toml-loader.test.ts | 187 +++++++++++++++++++++-- src/config/toml-loader.ts | 169 +++++++++++++++++++- 2 files changed, 344 insertions(+), 12 deletions(-) diff --git a/src/config/__tests__/toml-loader.test.ts b/src/config/__tests__/toml-loader.test.ts index 84863e69..baa5758d 100644 --- a/src/config/__tests__/toml-loader.test.ts +++ b/src/config/__tests__/toml-loader.test.ts @@ -116,29 +116,43 @@ dsn = "sqlite:///path/to/database.db" expect(result?.sources[0].user).toBeUndefined(); }); - it('should not override explicit connection params with DSN values', () => { + it('should reject identity fields that conflict with the DSN', () => { + // A DSN already encodes the connection identity; setting a field to a + // different value is silently ignored at connection time, so it must error. const tomlContent = ` [[sources]] id = "explicit_override" dsn = "postgres://dsn_user:pass@dsn_host:5432/dsn_db" type = "postgres" host = "explicit_host" -port = 9999 -database = "explicit_db" -user = "explicit_user" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting host"); + }); + + it('should accept identity fields that match the DSN', () => { + const tomlContent = ` +[[sources]] +id = "redundant" +dsn = "postgres://dsn_user:pass@dsn_host:5432/dsn_db" +type = "postgres" +host = "dsn_host" +port = 5432 +database = "dsn_db" +user = "dsn_user" `; fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); const result = loadTomlConfig(); - // Explicit values should be preserved, not overwritten by DSN expect(result?.sources[0]).toMatchObject({ - id: 'explicit_override', + id: 'redundant', type: 'postgres', - host: 'explicit_host', - port: 9999, - database: 'explicit_db', - user: 'explicit_user', + host: 'dsn_host', + port: 5432, + database: 'dsn_db', + user: 'dsn_user', }); }); @@ -483,6 +497,95 @@ sslmode = "invalid" expect(() => loadTomlConfig()).toThrow("invalid sslmode 'invalid'"); }); + it('should throw error when DSN sslmode conflicts with sslmode field', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:pass@localhost:5432/db?sslmode=disable" +sslmode = "require" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting sslmode"); + }); + + it('should accept matching DSN sslmode and sslmode field', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:pass@localhost:5432/db?sslmode=require" +sslmode = "require" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + const result = loadTomlConfig(); + + expect(result).toBeTruthy(); + expect(result?.sources[0].sslmode).toBe('require'); + }); + + it('should populate sslmode field from DSN query parameter', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:pass@localhost:5432/db?sslmode=require" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + const result = loadTomlConfig(); + + expect(result?.sources[0].sslmode).toBe('require'); + }); + + it('should throw error when DSN user conflicts with user field', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://dsn_user:pass@localhost:5432/db" +user = "other_user" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting user"); + }); + + it('should throw error when DSN database conflicts with database field', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:pass@localhost:5432/dsn_db" +database = "other_db" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting database"); + }); + + it('should throw error when password field conflicts with DSN password', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:dsn_pass@localhost:5432/db" +password = "other_pass" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + // The error must not echo the password values + expect(() => loadTomlConfig()).toThrow("password' field that conflicts"); + }); + + it('should throw error when DSN instanceName conflicts with instanceName field', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "sqlserver://sa:pass@localhost:1433/db?instanceName=ENV1" +instanceName = "ENV2" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting instanceName"); + }); + it('should throw error when sslmode is specified for SQLite', () => { const tomlContent = ` [[sources]] @@ -1134,6 +1237,70 @@ dsn = "mysql://user:pass@localhost:3306/testdb" expect(dsn).toBe('postgres://user:pass@localhost:5432/db'); }); + it('should merge sslmode field into a DSN that lacks it', () => { + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db', + sslmode: 'require', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode=require'); + }); + + it('should append sslmode with & when DSN already has query params', () => { + const source: SourceConfig = { + id: 'test', + type: 'sqlserver', + dsn: 'sqlserver://user:pass@localhost:1433/db?instanceName=ENV1', + sslmode: 'require', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('sqlserver://user:pass@localhost:1433/db?instanceName=ENV1&sslmode=require'); + }); + + it('should not duplicate sslmode when DSN already specifies it', () => { + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db?sslmode=require', + sslmode: 'require', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode=require'); + }); + + it('should merge instanceName field into a SQL Server DSN that lacks it', () => { + const source: SourceConfig = { + id: 'test', + type: 'sqlserver', + dsn: 'sqlserver://sa:pass@localhost:1433/db', + instanceName: 'ENV1', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('sqlserver://sa:pass@localhost:1433/db?instanceName=ENV1'); + }); + + it('should not add sslmode to a SQLite DSN', () => { + const source: SourceConfig = { + id: 'test', + type: 'sqlite', + dsn: 'sqlite:///path/to/db.sqlite', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('sqlite:///path/to/db.sqlite'); + }); + it('should build PostgreSQL DSN from individual params', () => { const source: SourceConfig = { id: 'test', diff --git a/src/config/toml-loader.ts b/src/config/toml-loader.ts index 07ccb8b3..321392b6 100644 --- a/src/config/toml-loader.ts +++ b/src/config/toml-loader.ts @@ -223,6 +223,98 @@ function validateToolsConfig( } } +/** + * Reject standalone fields that contradict a DSN. + * + * processSourceConfigs() copies identity fields (type/host/port/database/user) + * and the sslmode/sslrootcert query params from the DSN into the source only + * when the field is unset, so a value that differs here means the user + * explicitly set both the DSN and the field to incompatible values. The DSN + * always wins at connection time (buildDSNFromSource returns the DSN), so the + * field would otherwise be silently ignored. + */ +function validateDSNFieldConflicts(source: SourceConfig, configPath: string): void { + // SQLite DSNs only carry a path; nothing to cross-check + if (source.type === "sqlite") { + return; + } + + let url: SafeURL; + try { + url = new SafeURL(source.dsn!); + } catch { + // DSN parse failures are surfaced by the connector; skip the conflict check + return; + } + + const conflict = (field: string, fieldValue: string, dsnValue: string): never => { + throw new Error( + `Configuration file ${configPath}: source '${source.id}' has conflicting ${field}: ` + + `the DSN specifies '${dsnValue}' but the ${field} field is '${fieldValue}'. ` + + `Set ${field} in only one place, or make the two values match.` + ); + }; + + // Identity fields embedded in the DSN + const info = parseConnectionInfoFromDSN(source.dsn!); + if (info) { + if (source.type && info.type && source.type !== info.type) { + conflict("type", source.type, info.type); + } + if (source.host && info.host && source.host !== info.host) { + conflict("host", source.host, info.host); + } + if (source.port !== undefined && info.port !== undefined && source.port !== info.port) { + conflict("port", String(source.port), String(info.port)); + } + if (source.database && info.database && source.database !== info.database) { + conflict("database", source.database, info.database); + } + if (source.user && info.user && source.user !== info.user) { + conflict("user", source.user, info.user); + } + } + + // Password is never introspected into a field (omitted from API responses), + // so compare against the DSN directly. Never echo the values. + if (source.password && source.password !== url.password) { + throw new Error( + `Configuration file ${configPath}: source '${source.id}' has a 'password' field that conflicts ` + + `with the password embedded in the DSN. Set the password in only one place.` + ); + } + + // Dual-home query-string fields + const dsnSslmode = url.getSearchParam("sslmode"); + if (source.sslmode && dsnSslmode && dsnSslmode !== source.sslmode) { + conflict("sslmode", source.sslmode, dsnSslmode); + } + + const dsnSslrootcert = url.getSearchParam("sslrootcert"); + if ( + source.sslrootcert && + dsnSslrootcert && + expandHomeDir(source.sslrootcert) !== expandHomeDir(dsnSslrootcert) + ) { + conflict("sslrootcert", expandHomeDir(source.sslrootcert), expandHomeDir(dsnSslrootcert)); + } + + const dsnInstanceName = url.getSearchParam("instanceName"); + if (source.instanceName && dsnInstanceName && dsnInstanceName !== source.instanceName) { + conflict("instanceName", source.instanceName, dsnInstanceName); + } + + const dsnAuthentication = url.getSearchParam("authentication"); + if (source.authentication && dsnAuthentication && dsnAuthentication !== source.authentication) { + conflict("authentication", source.authentication, dsnAuthentication); + } + + const dsnDomain = url.getSearchParam("domain"); + if (source.domain && dsnDomain && dsnDomain !== source.domain) { + conflict("domain", source.domain, dsnDomain); + } +} + /** * Validate a single source configuration */ @@ -352,6 +444,15 @@ function validateSourceConfig(source: SourceConfig, configPath: string): void { } } + // Reject fields that contradict the DSN. A DSN already encodes the connection + // identity (type/host/port/database/user/password) and may carry query params + // (sslmode/sslrootcert/instanceName/authentication/domain). When the same value + // is also set as a standalone field with a different value, the field is + // silently ignored at connection time, so we fail fast instead. + if (source.dsn) { + validateDSNFieldConflicts(source, configPath); + } + // Validate sslrootcert if provided if (source.sslrootcert !== undefined) { if (source.sslmode !== "verify-ca" && source.sslmode !== "verify-full") { @@ -595,14 +696,78 @@ function expandHomeDir(filePath: string): string { return filePath; } +/** + * Merge "dual-home" fields (those that can be expressed either as a TOML field + * or as a DSN query parameter) into an existing DSN. Only appends a param when + * the DSN does not already specify it; conflicting values are rejected earlier + * by validateSourceConfig, so any param already present is guaranteed to match. + * + * This mirrors the query parameters built by the connection-params path of + * buildDSNFromSource so both paths produce equivalent DSNs. + */ +function mergeSourceFieldsIntoDSN(dsn: string, source: SourceConfig): string { + // SQLite DSNs never carry these parameters + if (source.type === "sqlite") { + return dsn; + } + + let url: SafeURL; + try { + url = new SafeURL(dsn); + } catch { + // If the DSN can't be parsed, leave it untouched; the connector surfaces errors + return dsn; + } + + const additions: string[] = []; + + // SQL Server query parameters + if (source.type === "sqlserver") { + if (source.instanceName && !url.getSearchParam("instanceName")) { + additions.push(`instanceName=${encodeURIComponent(source.instanceName)}`); + } + if (source.authentication && !url.getSearchParam("authentication")) { + additions.push(`authentication=${encodeURIComponent(source.authentication)}`); + } + if (source.domain && !url.getSearchParam("domain")) { + additions.push(`domain=${encodeURIComponent(source.domain)}`); + } + } + + if (source.sslmode && !url.getSearchParam("sslmode")) { + additions.push(`sslmode=${source.sslmode}`); + } + + if ( + source.sslrootcert && + source.type === "postgres" && + (source.sslmode === "verify-ca" || source.sslmode === "verify-full") && + !url.getSearchParam("sslrootcert") + ) { + const expandedCertPath = expandHomeDir(source.sslrootcert); + additions.push(`sslrootcert=${encodeURIComponent(expandedCertPath)}`); + } + + if (additions.length === 0) { + return dsn; + } + + const separator = dsn.includes("?") ? "&" : "?"; + return `${dsn}${separator}${additions.join("&")}`; +} + /** * Build DSN from source connection parameters * Similar to buildDSNFromEnvParams in env.ts but for TOML sources */ export function buildDSNFromSource(source: SourceConfig): string { - // If DSN is already provided, use it + // If DSN is already provided, use it — but merge in dual-home fields + // (sslmode/sslrootcert/instanceName/authentication/domain) so they actually + // affect the connection. Conflicts between the DSN query string and these + // fields are rejected at validation time, so any param already present in the + // DSN is guaranteed to match. if (source.dsn) { - return source.dsn; + return mergeSourceFieldsIntoDSN(source.dsn, source); } // Validate required fields From 1a1eb373931b86b4828aeeeba0f6e70612d92c81 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 23 Jun 2026 20:56:32 +0800 Subject: [PATCH 2/5] docs: document dsn + field conflict behavior (#334) 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) --- docs/config/toml.mdx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/config/toml.mdx b/docs/config/toml.mdx index 24c77153..00901de7 100644 --- a/docs/config/toml.mdx +++ b/docs/config/toml.mdx @@ -212,6 +212,12 @@ Sources define database connections. Each source represents a database that DBHu ``` + + + **Mixing `dsn` with individual fields.** A `dsn` already encodes the connection identity (type, host, port, database, user, password). Fields that can also live in the DSN query string — `sslmode`, `sslrootcert`, `instanceName`, `authentication`, `domain` — may be set alongside a `dsn` and are applied when the DSN omits them. + + If such a field is set to a value that **contradicts** the DSN (e.g. `dsn = "...?sslmode=disable"` together with `sslmode = "require"`, or a `host`/`user`/`database` that differs from the DSN), DBHub rejects the configuration at startup rather than silently ignoring the field. Set each value in only one place, or make the two values match. + ### connection_timeout From df73b284ae3c1bec828691328bfc8977ebcc5328 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 23 Jun 2026 21:05:19 +0800 Subject: [PATCH 3/5] fix: address review on DSN conflict validation (#337) - 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) --- src/config/__tests__/toml-loader.test.ts | 40 +++++++++++++++++++++- src/config/toml-loader.ts | 43 +++++++++++++++--------- 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/config/__tests__/toml-loader.test.ts b/src/config/__tests__/toml-loader.test.ts index baa5758d..8f40d880 100644 --- a/src/config/__tests__/toml-loader.test.ts +++ b/src/config/__tests__/toml-loader.test.ts @@ -570,8 +570,46 @@ password = "other_pass" `; fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); - // The error must not echo the password values expect(() => loadTomlConfig()).toThrow("password' field that conflicts"); + // The error must not echo either password value + expect(() => loadTomlConfig()).not.toThrow(/dsn_pass|other_pass/); + }); + + it('should report a clear error when password field is set but DSN has no password', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user@localhost:5432/db" +password = "field_pass" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("the DSN has no password"); + expect(() => loadTomlConfig()).not.toThrow(/field_pass/); + }); + + it('should throw error when type = "sqlite" conflicts with a non-SQLite DSN', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +type = "sqlite" +dsn = "postgres://user:pass@localhost:5432/db" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting type"); + }); + + it('should throw error when type = "postgres" conflicts with a SQLite DSN', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +type = "postgres" +dsn = "sqlite:///path/to/db.sqlite" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting type"); }); it('should throw error when DSN instanceName conflicts with instanceName field', () => { diff --git a/src/config/toml-loader.ts b/src/config/toml-loader.ts index 321392b6..806a4747 100644 --- a/src/config/toml-loader.ts +++ b/src/config/toml-loader.ts @@ -234,8 +234,27 @@ function validateToolsConfig( * field would otherwise be silently ignored. */ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): void { - // SQLite DSNs only carry a path; nothing to cross-check - if (source.type === "sqlite") { + const conflict = (field: string, fieldValue: string, dsnValue: string): never => { + throw new Error( + `Configuration file ${configPath}: source '${source.id}' has conflicting ${field}: ` + + `the DSN specifies '${dsnValue}' but the ${field} field is '${fieldValue}'. ` + + `Set ${field} in only one place, or make the two values match.` + ); + }; + + const info = parseConnectionInfoFromDSN(source.dsn!); + + // Reject a type field that disagrees with the DSN protocol. Checked before the + // SQLite short-circuit below, and keyed off the DSN's parsed type rather than + // source.type, so a `type = "sqlite"` paired with a non-SQLite DSN (or the + // reverse) is still caught instead of silently skipped. + if (source.type && info?.type && source.type !== info.type) { + conflict("type", source.type, info.type); + } + + // A SQLite DSN only carries a database path — no host/credentials or + // query-string fields to cross-check. + if (info?.type === "sqlite") { return; } @@ -247,20 +266,8 @@ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): vo return; } - const conflict = (field: string, fieldValue: string, dsnValue: string): never => { - throw new Error( - `Configuration file ${configPath}: source '${source.id}' has conflicting ${field}: ` + - `the DSN specifies '${dsnValue}' but the ${field} field is '${fieldValue}'. ` + - `Set ${field} in only one place, or make the two values match.` - ); - }; - // Identity fields embedded in the DSN - const info = parseConnectionInfoFromDSN(source.dsn!); if (info) { - if (source.type && info.type && source.type !== info.type) { - conflict("type", source.type, info.type); - } if (source.host && info.host && source.host !== info.host) { conflict("host", source.host, info.host); } @@ -278,9 +285,15 @@ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): vo // Password is never introspected into a field (omitted from API responses), // so compare against the DSN directly. Never echo the values. if (source.password && source.password !== url.password) { + if (!url.password) { + throw new Error( + `Configuration file ${configPath}: source '${source.id}' has a 'password' field but the DSN has no password. ` + + `The field is ignored at connection time — add the password to the DSN, or use individual connection parameters instead of a DSN.` + ); + } throw new Error( `Configuration file ${configPath}: source '${source.id}' has a 'password' field that conflicts ` + - `with the password embedded in the DSN. Set the password in only one place.` + `with the password in the DSN. Set the password in only one place.` ); } From ab403e540bcae4e34707b7b11ff29f71232f27cf Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 23 Jun 2026 21:53:35 +0800 Subject: [PATCH 4/5] fix: address second review round on DSN conflict validation (#337) - 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) --- src/config/__tests__/toml-loader.test.ts | 72 ++++++++++++++++++++++++ src/config/toml-loader.ts | 31 +++++++--- 2 files changed, 95 insertions(+), 8 deletions(-) diff --git a/src/config/__tests__/toml-loader.test.ts b/src/config/__tests__/toml-loader.test.ts index 8f40d880..4205d990 100644 --- a/src/config/__tests__/toml-loader.test.ts +++ b/src/config/__tests__/toml-loader.test.ts @@ -131,6 +131,20 @@ host = "explicit_host" expect(() => loadTomlConfig()).toThrow("conflicting host"); }); + it('should accept a host field that differs only in case from the DSN', () => { + const tomlContent = ` +[[sources]] +id = "case_host" +dsn = "postgres://user:pass@DB.EXAMPLE.COM:5432/db" +host = "db.example.com" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + const result = loadTomlConfig(); + + expect(result?.sources[0].id).toBe('case_host'); + }); + it('should accept identity fields that match the DSN', () => { const tomlContent = ` [[sources]] @@ -1327,6 +1341,64 @@ dsn = "mysql://user:pass@localhost:3306/testdb" expect(dsn).toBe('sqlserver://sa:pass@localhost:1433/db?instanceName=ENV1'); }); + it('should merge authentication and domain fields into a SQL Server DSN', () => { + const source: SourceConfig = { + id: 'test', + type: 'sqlserver', + dsn: 'sqlserver://user:pass@localhost:1433/db', + authentication: 'ntlm', + domain: 'CORP', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('sqlserver://user:pass@localhost:1433/db?authentication=ntlm&domain=CORP'); + }); + + it('should merge sslrootcert field into a postgres DSN for verify-ca', () => { + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db', + sslmode: 'verify-ca', + sslrootcert: '/etc/ssl/ca bundle.pem', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe( + 'postgres://user:pass@localhost:5432/db?sslmode=verify-ca&sslrootcert=' + + encodeURIComponent('/etc/ssl/ca bundle.pem') + ); + }); + + it('should not merge sslrootcert when sslmode is not a verify mode', () => { + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db', + sslmode: 'require', + sslrootcert: '/etc/ssl/ca.pem', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode=require'); + }); + + it('should not produce "?&" when the DSN ends with a bare "?"', () => { + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db?', + sslmode: 'require', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode=require'); + }); + it('should not add sslmode to a SQLite DSN', () => { const source: SourceConfig = { id: 'test', diff --git a/src/config/toml-loader.ts b/src/config/toml-loader.ts index 806a4747..a32f65af 100644 --- a/src/config/toml-loader.ts +++ b/src/config/toml-loader.ts @@ -226,12 +226,16 @@ function validateToolsConfig( /** * Reject standalone fields that contradict a DSN. * - * processSourceConfigs() copies identity fields (type/host/port/database/user) - * and the sslmode/sslrootcert query params from the DSN into the source only - * when the field is unset, so a value that differs here means the user - * explicitly set both the DSN and the field to incompatible values. The DSN - * always wins at connection time (buildDSNFromSource returns the DSN), so the - * field would otherwise be silently ignored. + * A DSN already encodes the connection identity (type/host/port/database/user/ + * password) and may carry query parameters (sslmode/sslrootcert plus the SQL + * Server instanceName/authentication/domain). When the user also sets one of + * those as a standalone field with a different value, the DSN wins at connection + * time (buildDSNFromSource returns the DSN), so the field would be silently + * ignored — we fail fast here instead. + * + * A field left unset never trips this check: processSourceConfigs() copies a + * subset of these (type/host/port/database/user + sslmode/sslrootcert) from the + * DSN into the source when the field is unset, so they end up matching. */ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): void { const conflict = (field: string, fieldValue: string, dsnValue: string): never => { @@ -268,7 +272,8 @@ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): vo // Identity fields embedded in the DSN if (info) { - if (source.host && info.host && source.host !== info.host) { + // Hostnames are case-insensitive, so compare them normalized + if (source.host && info.host && source.host.toLowerCase() !== info.host.toLowerCase()) { conflict("host", source.host, info.host); } if (source.port !== undefined && info.port !== undefined && source.port !== info.port) { @@ -765,7 +770,17 @@ function mergeSourceFieldsIntoDSN(dsn: string, source: SourceConfig): string { return dsn; } - const separator = dsn.includes("?") ? "&" : "?"; + // Pick the right join character: "?" when no query string exists yet, nothing + // when the DSN already ends with "?" or "&" (an empty/trailing query string), + // otherwise "&". + let separator: string; + if (!dsn.includes("?")) { + separator = "?"; + } else if (dsn.endsWith("?") || dsn.endsWith("&")) { + separator = ""; + } else { + separator = "&"; + } return `${dsn}${separator}${additions.join("&")}`; } From 7b51d857560c55f74320b3b8f2eb5670e315999d Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 23 Jun 2026 22:10:45 +0800 Subject: [PATCH 5/5] fix: handle empty-valued DSN query params in conflict/merge (#337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/config/__tests__/toml-loader.test.ts | 27 ++++++++ src/config/toml-loader.ts | 78 ++++++++++++++++++------ 2 files changed, 86 insertions(+), 19 deletions(-) diff --git a/src/config/__tests__/toml-loader.test.ts b/src/config/__tests__/toml-loader.test.ts index 4205d990..8f57664f 100644 --- a/src/config/__tests__/toml-loader.test.ts +++ b/src/config/__tests__/toml-loader.test.ts @@ -551,6 +551,18 @@ dsn = "postgres://user:pass@localhost:5432/db?sslmode=require" expect(result?.sources[0].sslmode).toBe('require'); }); + it('should treat an empty DSN sslmode (?sslmode=) as present and conflicting', () => { + const tomlContent = ` +[[sources]] +id = "test_db" +dsn = "postgres://user:pass@localhost:5432/db?sslmode=" +sslmode = "require" +`; + fs.writeFileSync(path.join(tempDir, 'dbhub.toml'), tomlContent); + + expect(() => loadTomlConfig()).toThrow("conflicting sslmode"); + }); + it('should throw error when DSN user conflicts with user field', () => { const tomlContent = ` [[sources]] @@ -1386,6 +1398,21 @@ dsn = "mysql://user:pass@localhost:3306/testdb" expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode=require'); }); + it('should not append a duplicate when the DSN has an empty-valued param', () => { + // SafeURL drops `?sslmode=`, but the raw presence check must still see it + // so we never produce an ambiguous `?sslmode=&sslmode=require`. + const source: SourceConfig = { + id: 'test', + type: 'postgres', + dsn: 'postgres://user:pass@localhost:5432/db?sslmode=', + sslmode: 'require', + }; + + const dsn = buildDSNFromSource(source); + + expect(dsn).toBe('postgres://user:pass@localhost:5432/db?sslmode='); + }); + it('should not produce "?&" when the DSN ends with a bare "?"', () => { const source: SourceConfig = { id: 'test', diff --git a/src/config/toml-loader.ts b/src/config/toml-loader.ts index a32f65af..a5c4ff13 100644 --- a/src/config/toml-loader.ts +++ b/src/config/toml-loader.ts @@ -223,6 +223,38 @@ function validateToolsConfig( } } +/** + * Read a query parameter's value from a DSN's raw query string, distinguishing + * "absent" (returns null) from "present but empty" (returns ""). + * + * SafeURL drops params whose value is empty (e.g. `?sslmode=`), which would make + * an empty-but-present param look absent — causing conflict checks to be skipped + * and the merge step to append a duplicate param. Scanning the raw query string + * here avoids that. + */ +function getRawDSNQueryParam(dsn: string, key: string): string | null { + const queryStart = dsn.indexOf("?"); + if (queryStart === -1) { + return null; + } + for (const pair of dsn.substring(queryStart + 1).split("&")) { + if (pair === "") { + continue; + } + const eq = pair.indexOf("="); + const rawKey = eq === -1 ? pair : pair.substring(0, eq); + if (rawKey === key) { + const rawValue = eq === -1 ? "" : pair.substring(eq + 1); + try { + return decodeURIComponent(rawValue); + } catch { + return rawValue; + } + } + } + return null; +} + /** * Reject standalone fields that contradict a DSN. * @@ -302,33 +334,35 @@ function validateDSNFieldConflicts(source: SourceConfig, configPath: string): vo ); } - // Dual-home query-string fields - const dsnSslmode = url.getSearchParam("sslmode"); - if (source.sslmode && dsnSslmode && dsnSslmode !== source.sslmode) { + // Dual-home query-string fields. Use the raw query string (not SafeURL, which + // drops empty-valued params) so a present-but-empty param like `?sslmode=` is + // still treated as present and a conflicting field is rejected. + const dsnSslmode = getRawDSNQueryParam(source.dsn!, "sslmode"); + if (source.sslmode && dsnSslmode !== null && dsnSslmode !== source.sslmode) { conflict("sslmode", source.sslmode, dsnSslmode); } - const dsnSslrootcert = url.getSearchParam("sslrootcert"); + const dsnSslrootcert = getRawDSNQueryParam(source.dsn!, "sslrootcert"); if ( source.sslrootcert && - dsnSslrootcert && + dsnSslrootcert !== null && expandHomeDir(source.sslrootcert) !== expandHomeDir(dsnSslrootcert) ) { conflict("sslrootcert", expandHomeDir(source.sslrootcert), expandHomeDir(dsnSslrootcert)); } - const dsnInstanceName = url.getSearchParam("instanceName"); - if (source.instanceName && dsnInstanceName && dsnInstanceName !== source.instanceName) { + const dsnInstanceName = getRawDSNQueryParam(source.dsn!, "instanceName"); + if (source.instanceName && dsnInstanceName !== null && dsnInstanceName !== source.instanceName) { conflict("instanceName", source.instanceName, dsnInstanceName); } - const dsnAuthentication = url.getSearchParam("authentication"); - if (source.authentication && dsnAuthentication && dsnAuthentication !== source.authentication) { + const dsnAuthentication = getRawDSNQueryParam(source.dsn!, "authentication"); + if (source.authentication && dsnAuthentication !== null && dsnAuthentication !== source.authentication) { conflict("authentication", source.authentication, dsnAuthentication); } - const dsnDomain = url.getSearchParam("domain"); - if (source.domain && dsnDomain && dsnDomain !== source.domain) { + const dsnDomain = getRawDSNQueryParam(source.dsn!, "domain"); + if (source.domain && dsnDomain !== null && dsnDomain !== source.domain) { conflict("domain", source.domain, dsnDomain); } } @@ -729,30 +763,36 @@ function mergeSourceFieldsIntoDSN(dsn: string, source: SourceConfig): string { return dsn; } - let url: SafeURL; try { - url = new SafeURL(dsn); + // Parse only to validate the DSN; if it can't be parsed, leave it untouched + // and let the connector surface the error. + new SafeURL(dsn); } catch { - // If the DSN can't be parsed, leave it untouched; the connector surfaces errors return dsn; } + // Use raw key presence (not SafeURL, which drops empty-valued params) so we + // never append a duplicate of a param the DSN already specifies, even as + // `?sslmode=`. Such empty-but-present params are rejected by validation when a + // conflicting field is set. + const hasParam = (key: string): boolean => getRawDSNQueryParam(dsn, key) !== null; + const additions: string[] = []; // SQL Server query parameters if (source.type === "sqlserver") { - if (source.instanceName && !url.getSearchParam("instanceName")) { + if (source.instanceName && !hasParam("instanceName")) { additions.push(`instanceName=${encodeURIComponent(source.instanceName)}`); } - if (source.authentication && !url.getSearchParam("authentication")) { + if (source.authentication && !hasParam("authentication")) { additions.push(`authentication=${encodeURIComponent(source.authentication)}`); } - if (source.domain && !url.getSearchParam("domain")) { + if (source.domain && !hasParam("domain")) { additions.push(`domain=${encodeURIComponent(source.domain)}`); } } - if (source.sslmode && !url.getSearchParam("sslmode")) { + if (source.sslmode && !hasParam("sslmode")) { additions.push(`sslmode=${source.sslmode}`); } @@ -760,7 +800,7 @@ function mergeSourceFieldsIntoDSN(dsn: string, source: SourceConfig): string { source.sslrootcert && source.type === "postgres" && (source.sslmode === "verify-ca" || source.sslmode === "verify-full") && - !url.getSearchParam("sslrootcert") + !hasParam("sslrootcert") ) { const expandedCertPath = expandHomeDir(source.sslrootcert); additions.push(`sslrootcert=${encodeURIComponent(expandedCertPath)}`);