From 750f689a97f39f82eab923187cc2a0620bd9dfc0 Mon Sep 17 00:00:00 2001 From: DANze11 <55740827+DANze11@users.noreply.github.com> Date: Thu, 6 Aug 2026 00:48:09 +0300 Subject: [PATCH] fix: split DSN credentials on the last @ of the authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SafeURL took the first '@' as the userinfo separator. '@' is a legal password character, so a password carrying one was cut short there and its tail folded into the hostname. For postgres://user:pa@ss@dbhost:5432/mydb the password became "pa" and the host "ss@dbhost". This is not connector-specific: SafeURL parses the DSN for all five connectors, so any of them rejects a password containing '@'. The symptom is a DNS failure (getaddrinfo EAI_FAIL on the mangled host) or, when the truncated prefix happens to be a valid password elsewhere, a plain login failure — neither points at the password. It also weakens obfuscateDSNPassword, which masks the DSN before it is logged and relies on the same split. Given the DSN above it produced postgres://user:**@ss@dbhost:5432/mydb leaving the part of the password after the '@' in the string. DBHub prints that line per source at startup, so the tail reached the server's log — and, on the stdio transport, the client's. The exposure is limited to an operator's own password on a host that already holds the config, and only arises in a deployment that cannot connect at all, so this is filed as a bug rather than through the advisory process; say the word if you would rather handle it the other way. Standard URL parsing ends userinfo at the last '@' of the authority, so match that. The search is bounded by the start of the path, because the path is still attached at this point and may legitimately contain '@' (a database named "a@b"): an unbounded lastIndexOf would then pick that one as the separator. Percent-encoding already worked, since the parser decodes username and password. This makes the unencoded form work too, which is what a DSN assembled from an environment variable ends up carrying. Tests cover the multi-'@' password, '@' in the path (with and without credentials), the percent-encoded form, the case with a port, and the masking above, which had no test for this at all. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/utils/__tests__/dsn-obfuscate.test.ts | 11 +++++ src/utils/__tests__/safe-url.test.ts | 57 +++++++++++++++++++++++ src/utils/safe-url.ts | 16 ++++++- 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/utils/__tests__/dsn-obfuscate.test.ts b/src/utils/__tests__/dsn-obfuscate.test.ts index 11956ce9..bbf8ec04 100644 --- a/src/utils/__tests__/dsn-obfuscate.test.ts +++ b/src/utils/__tests__/dsn-obfuscate.test.ts @@ -67,6 +67,17 @@ describe('DSN Obfuscation Utilities', () => { const result = obfuscateDSNPassword(dsn); expect(result).toBe('postgres://user:****@localhost:5432?sslmode=require'); }); + + it('should obfuscate the whole password when it contains an @', () => { + // Splitting the authority on the first '@' masks only the part before it + // and leaves the rest in the string, so the tail of the password reaches + // whatever reads this line — DBHub prints it per source at startup. + const dsn = 'postgres://user:pa@ss@localhost:5432/db'; + const result = obfuscateDSNPassword(dsn); + + expect(result).toBe('postgres://user:*****@localhost:5432/db'); + expect(result).not.toContain('ss@'); + }); }); describe('obfuscateSSHConfig', () => { diff --git a/src/utils/__tests__/safe-url.test.ts b/src/utils/__tests__/safe-url.test.ts index 05e805c2..403341e7 100644 --- a/src/utils/__tests__/safe-url.test.ts +++ b/src/utils/__tests__/safe-url.test.ts @@ -104,4 +104,61 @@ describe('SafeURL', () => { it('should throw an error for URLs without a protocol', () => { expect(() => new SafeURL('localhost:5432/dbname')).toThrow('Invalid URL format: missing protocol'); }); + + describe("'@' inside the password", () => { + it('splits on the last @ of the authority, not the first', () => { + // Splitting on the first '@' yields password 'pa' and hostname + // 'ss@localhost', so the failure surfaces as a DNS lookup error rather + // than as a bad password — a confusing way to learn it was truncated. + const url = new SafeURL('sqlserver://user:pa@ss@localhost/dbname'); + + expect(url.username).toBe('user'); + expect(url.password).toBe('pa@ss'); + expect(url.hostname).toBe('localhost'); + expect(url.pathname).toBe('/dbname'); + }); + + it('handles a password containing several @ characters', () => { + const url = new SafeURL('sqlserver://user:a@b@c@host/db'); + + expect(url.username).toBe('user'); + expect(url.password).toBe('a@b@c'); + expect(url.hostname).toBe('host'); + }); + + it('does not treat an @ in the path as the separator', () => { + // The path is still attached when the authority is split, so an unbounded + // lastIndexOf would pick the '@' in the database name instead. + const url = new SafeURL('sqlserver://user:pass@localhost/we@ird'); + + expect(url.username).toBe('user'); + expect(url.password).toBe('pass'); + expect(url.hostname).toBe('localhost'); + expect(url.pathname).toBe('/we@ird'); + }); + + it('keeps working when only the path contains an @', () => { + const url = new SafeURL('sqlserver://localhost/we@ird'); + + expect(url.username).toBe(''); + expect(url.password).toBe(''); + expect(url.hostname).toBe('localhost'); + expect(url.pathname).toBe('/we@ird'); + }); + + it('still accepts a percent-encoded @', () => { + const url = new SafeURL('sqlserver://user:pa%40ss@localhost/dbname'); + + expect(url.password).toBe('pa@ss'); + expect(url.hostname).toBe('localhost'); + }); + + it('keeps the port when the password carries an @', () => { + const url = new SafeURL('sqlserver://user:pa@ss@localhost:1433/dbname'); + + expect(url.password).toBe('pa@ss'); + expect(url.hostname).toBe('localhost'); + expect(url.port).toBe('1433'); + }); + }); }); \ No newline at end of file diff --git a/src/utils/safe-url.ts b/src/utils/safe-url.ts index 5b3b491f..ba84ebb2 100644 --- a/src/utils/safe-url.ts +++ b/src/utils/safe-url.ts @@ -82,8 +82,20 @@ export class SafeURL implements ISafeURL { }); } - // Extract authentication - const atIndex: number = urlString.indexOf('@'); + // Extract authentication. + // + // The separator is the LAST '@' of the authority, not the first: '@' is a + // legal password character, and passwords carrying one are common. Taking + // the first '@' would cut the password short and fold its tail into the + // hostname, which surfaces as a DNS failure rather than as a bad password. + // + // The search is bounded by the start of the path, because the path is + // still attached at this point and may legitimately contain '@' (a + // database named "a@b", say) — an unbounded lastIndexOf would then treat + // that one as the separator. + const pathStart: number = urlString.indexOf('/'); + const authorityEnd: number = pathStart === -1 ? urlString.length : pathStart; + const atIndex: number = urlString.lastIndexOf('@', authorityEnd - 1); if (atIndex !== -1) { const auth: string = urlString.substring(0, atIndex); urlString = urlString.substring(atIndex + 1);