From 1fecb1cce45eeb9045b1a703f8b8062d64357615 Mon Sep 17 00:00:00 2001 From: smartass Date: Fri, 12 Jun 2026 01:37:15 +0500 Subject: [PATCH] fix: sql guard gaps, date tz, timeouts, payload --- README.md | 9 ++++++++ src/config/registry.ts | 8 +++++++ src/db/mysql.ts | 7 +++++- src/db/postgres.ts | 19 ++++++++++++++++ src/db/sqlGuard.ts | 25 +++++++++++++++++---- src/net/tunnel.ts | 7 +++++- src/tools/respond.ts | 2 +- test/integration/mysql.test.ts | 15 +++++++++++++ test/integration/postgres.test.ts | 28 +++++++++++++++++++++++ test/unit/config/registry.test.ts | 29 ++++++++++++++++++++++++ test/unit/db/mysql.test.ts | 12 ++++++++++ test/unit/db/postgres.test.ts | 32 +++++++++++++++++++++++--- test/unit/db/sqlGuard.test.ts | 37 +++++++++++++++++++++++++++++++ test/unit/net/tunnel.test.ts | 17 ++++++++++++++ 14 files changed, 237 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 0074c77..11c590e 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,10 @@ rejected (the schemas are strict). | `readonly` | boolean | defaults to `false`; see Readonly mode below | | `ssh` | object | optional SSH tunnel (see below) | +For PostgreSQL, a connection without a `database` falls back to the conventional +`postgres` maintenance database for `list_databases` and `test_connection`. On +servers that lack it, set `database` explicitly. + The `ssh` object accepts: | Field | Type | Notes | @@ -135,6 +139,11 @@ blocked by the SQL guard. For a hard guarantee in untrusted contexts, still connect with a read-only database user — readonly mode is defence in depth, not a substitute for database permissions. +The PostgreSQL mechanism is a libpq startup option, which PgBouncer in +transaction or statement pooling mode does **not** forward to the server. Behind +such a pooler the readonly flag is silently ineffective; use a read-only +database user there instead. + ## Security notes - The store is a plaintext JSON file written with `0600` permissions. The trust diff --git a/src/config/registry.ts b/src/config/registry.ts index c398587..607cf3c 100644 --- a/src/config/registry.ts +++ b/src/config/registry.ts @@ -124,6 +124,14 @@ export const createRegistry = (options: RegistryOptions): Registry => { throw new ConnectionNotFoundError(name, store.map((c) => c.name).sort()) } const base: Record = { ...store[index] } + // Switching engine without an explicit port must drop the old engine's + // port so the new engine's default applies downstream; an explicit port + // (value or null) in the patch is honored by the loop below. + const switchesEngine = + patch.type !== undefined && patch.type !== base.type && !('port' in patch) + if (switchesEngine) { + delete base.port + } for (const [key, value] of Object.entries(patch)) { if (value === null) { delete base[key] diff --git a/src/db/mysql.ts b/src/db/mysql.ts index 4bfaddc..41dcfcc 100644 --- a/src/db/mysql.ts +++ b/src/db/mysql.ts @@ -35,9 +35,14 @@ export const createMysqlDriver = (target: DriverTarget): Driver => { connectionLimit: 4, maxIdle: 1, idleTimeout: 60_000, + connectTimeout: 10_000, multipleStatements: false, supportBigNumbers: true, - bigNumberStrings: true + bigNumberStrings: true, + // DATE and DATETIME are wall-clock types; returning them as strings + // avoids a tz day-shift when normalizeCell calls toISOString(). + // TIMESTAMP stays a Date — it is absolute in mysql. + dateStrings: ['DATE', 'DATETIME'] }) pools.set(key, pool) return pool diff --git a/src/db/postgres.ts b/src/db/postgres.ts index a4bb6cc..72a8746 100644 --- a/src/db/postgres.ts +++ b/src/db/postgres.ts @@ -14,8 +14,25 @@ import { assertSafeStatement } from './sqlGuard.js' const MAINTENANCE_DB = 'postgres' +const DATE_OID = 1082 +const TIMESTAMP_OID = 1114 + +const identityParser = (value: string): string => value + +// pg-types parses DATE and TIMESTAMP-without-tz to LOCAL JS Dates, so a later +// toISOString() shifts the calendar day in non-UTC zones. Keep these wall-clock +// types as raw strings; TIMESTAMPTZ (1184) is absolute and keeps its default +// parser so a Date is still produced. +const buildTypeOverrides = (): pg.TypeOverrides => { + const types = new pg.TypeOverrides() + types.setTypeParser(DATE_OID, identityParser) + types.setTypeParser(TIMESTAMP_OID, identityParser) + return types +} + export const createPostgresDriver = (target: DriverTarget): Driver => { const pools = new Map() + const types = buildTypeOverrides() const getPool = (database: string): pg.Pool => { const existing = pools.get(database) @@ -29,6 +46,8 @@ export const createPostgresDriver = (target: DriverTarget): Driver => { password: target.config.password, database, max: 4, + connectionTimeoutMillis: 10_000, + types, ...(target.config.readonly ? { options: '-c default_transaction_read_only=on' } : {}) }) pool.on('error', (error) => { diff --git a/src/db/sqlGuard.ts b/src/db/sqlGuard.ts index 84cb574..d204fe4 100644 --- a/src/db/sqlGuard.ts +++ b/src/db/sqlGuard.ts @@ -28,8 +28,17 @@ const isWordChar = (ch: string): boolean => { // Reads a dollar-quote tag starting at the first '$'. Returns the full opening // token (e.g. '$$' or '$fn$') when sql[i] begins a valid dollar quote, or null. +// A tag's first character must be a letter or underscore (postgres rule), so +// '$1$' is a positional parameter '$1' plus a bare '$', not a dollar quote; +// '$$' (empty tag) stays valid. Subsequent tag chars may include digits. const readDollarTag = (sql: string, i: number): string | null => { - let j = i + 1 + if (sql[i + 1] === '$') { + return sql.slice(i, i + 2) + } + if (!/[A-Za-z_]/.test(sql[i + 1] ?? '')) { + return null + } + let j = i + 2 while (j < sql.length && /[A-Za-z0-9_]/.test(sql[j])) { j += 1 } @@ -98,7 +107,7 @@ const hasContentAfterTopLevelSemicolon = (sql: string, engine: Engine): boolean continue } if (ch === '"') { - i = skipDoubleQuoted(sql, i) + i = skipDoubleQuoted(sql, i, engine) continue } if (engine === 'mysql' && ch === '`') { @@ -179,10 +188,18 @@ const skipSingleQuoted = (sql: string, start: number, engine: Engine): number => return i } -const skipDoubleQuoted = (sql: string, start: number): number => { +const skipDoubleQuoted = (sql: string, start: number, engine: Engine): number => { + // mysql treats "..." as a string literal honoring backslash escapes; + // postgres "..." is an identifier where backslash is a literal char. + const backslashEscapes = engine === 'mysql' let i = start + 1 while (i < sql.length) { - if (sql[i] === '"') { + const ch = sql[i] + if (backslashEscapes && ch === '\\') { + i += 2 + continue + } + if (ch === '"') { if (sql[i + 1] === '"') { i += 2 continue diff --git a/src/net/tunnel.ts b/src/net/tunnel.ts index aec0d39..0ca1978 100644 --- a/src/net/tunnel.ts +++ b/src/net/tunnel.ts @@ -31,7 +31,12 @@ export const buildSshAuth = ( passphrase: ssh.passphrase } } - if (ssh.agent && env.SSH_AUTH_SOCK) { + if (ssh.agent) { + // Enabling agent is explicit intent: never silently fall through to + // password when the socket var is missing. + if (!env.SSH_AUTH_SOCK) { + throw new Error('ssh agent auth requested but SSH_AUTH_SOCK is not set') + } return { agent: env.SSH_AUTH_SOCK } } if (ssh.password) { diff --git a/src/tools/respond.ts b/src/tools/respond.ts index 0fb79cc..90bbc4b 100644 --- a/src/tools/respond.ts +++ b/src/tools/respond.ts @@ -1,7 +1,7 @@ import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js' export const ok = (payload: unknown): CallToolResult => ({ - content: [{ type: 'text', text: JSON.stringify(payload, null, 2) }] + content: [{ type: 'text', text: JSON.stringify(payload) }] }) export const fail = (message: string): CallToolResult => ({ diff --git a/test/integration/mysql.test.ts b/test/integration/mysql.test.ts index 7bf340d..567e787 100644 --- a/test/integration/mysql.test.ts +++ b/test/integration/mysql.test.ts @@ -101,6 +101,21 @@ describe('mysql integration', () => { expect(result.rows[0][0]).toBe('9007199254740993') }) + it('returns DATE and DATETIME as strings, no tz day-shift', async () => { + const { driver } = await db().get('my') + await driver.query({ + sql: 'CREATE TABLE events (id int AUTO_INCREMENT PRIMARY KEY, day date, at datetime)', + rowLimit: 10 + }) + await driver.query({ + sql: "INSERT INTO events (day, at) VALUES ('2026-06-11', '2026-06-11 12:34:56')", + rowLimit: 10 + }) + const result = await driver.query({ sql: 'SELECT day, at FROM events', rowLimit: 10 }) + expect(result.rows[0][0]).toBe('2026-06-11') + expect(result.rows[0][1]).toBe('2026-06-11 12:34:56') + }) + it('rejects multi-statement and session-level sql', async () => { const { driver } = await db().get('my') await expect(driver.query({ sql: 'SELECT 1; SELECT 2', rowLimit: 10 })).rejects.toThrow( diff --git a/test/integration/postgres.test.ts b/test/integration/postgres.test.ts index e582280..348afad 100644 --- a/test/integration/postgres.test.ts +++ b/test/integration/postgres.test.ts @@ -86,6 +86,34 @@ describe('postgres integration', () => { expect(select.rows).toEqual([['ada']]) }) + it('returns DATE columns as the stored calendar day, no tz shift', async () => { + const { driver } = await db().get('pg') + await driver.query({ + sql: 'CREATE TABLE events (id serial PRIMARY KEY, day date NOT NULL)', + rowLimit: 10 + }) + await driver.query({ + sql: "INSERT INTO events (day) VALUES ('2026-06-11')", + rowLimit: 10 + }) + const result = await driver.query({ sql: 'SELECT day FROM events', rowLimit: 10 }) + expect(result.rows).toEqual([['2026-06-11']]) + }) + + it('returns TIMESTAMP without tz as a wall-clock string', async () => { + const { driver } = await db().get('pg') + await driver.query({ + sql: 'CREATE TABLE moments (id serial PRIMARY KEY, at timestamp NOT NULL)', + rowLimit: 10 + }) + await driver.query({ + sql: "INSERT INTO moments (at) VALUES ('2026-06-11 12:34:56')", + rowLimit: 10 + }) + const result = await driver.query({ sql: 'SELECT at FROM moments', rowLimit: 10 }) + expect(result.rows[0][0]).toBe('2026-06-11 12:34:56') + }) + it('truncates SELECT results at rowLimit', async () => { const { driver } = await db().get('pg') const result = await driver.query({ diff --git a/test/unit/config/registry.test.ts b/test/unit/config/registry.test.ts index 0cf5dbf..c02486a 100644 --- a/test/unit/config/registry.test.ts +++ b/test/unit/config/registry.test.ts @@ -89,6 +89,35 @@ describe('registry', () => { expect(() => registry.update('u', { host: '' })).toThrow() }) + it('drops the inherited port when switching engine without a new port', () => { + writeStore(storePath, [conn('switch', { type: 'postgres', port: 5432 })]) + const registry = createRegistry({ storePath, env: {} }) + const updated = registry.update('switch', { type: 'mysql' }) + expect(updated.config.type).toBe('mysql') + expect(updated.config.port).toBeUndefined() + }) + + it('keeps an explicit port when switching engine', () => { + writeStore(storePath, [conn('switch', { type: 'postgres', port: 5432 })]) + const registry = createRegistry({ storePath, env: {} }) + const updated = registry.update('switch', { type: 'mysql', port: 3307 }) + expect(updated.config.port).toBe(3307) + }) + + it('clears the port when switching engine with an explicit null', () => { + writeStore(storePath, [conn('switch', { type: 'postgres', port: 5432 })]) + const registry = createRegistry({ storePath, env: {} }) + const updated = registry.update('switch', { type: 'mysql', port: null }) + expect(updated.config.port).toBeUndefined() + }) + + it('keeps the port when type is unchanged', () => { + writeStore(storePath, [conn('same', { type: 'postgres', port: 5432 })]) + const registry = createRegistry({ storePath, env: {} }) + const updated = registry.update('same', { readonly: true }) + expect(updated.config.port).toBe(5432) + }) + it('update can rename unless the new name is taken', () => { writeStore(storePath, [conn('old-name'), conn('taken')]) const registry = createRegistry({ storePath, env: {} }) diff --git a/test/unit/db/mysql.test.ts b/test/unit/db/mysql.test.ts index af5501f..9eaa861 100644 --- a/test/unit/db/mysql.test.ts +++ b/test/unit/db/mysql.test.ts @@ -126,6 +126,18 @@ describe('createMysqlDriver', () => { }) }) + it('sets a connect timeout on every pool', async () => { + const driver = createMysqlDriver(target()) + await driver.query({ sql: 'select 1', rowLimit: 10 }) + expect(mysqlState.state.pools[0].options.connectTimeout).toBe(10_000) + }) + + it('returns DATE and DATETIME as strings to avoid tz day-shift', async () => { + const driver = createMysqlDriver(target()) + await driver.query({ sql: 'select 1', rowLimit: 10 }) + expect(mysqlState.state.pools[0].options.dateStrings).toEqual(['DATE', 'DATETIME']) + }) + it('reuses pools per database', async () => { const driver = createMysqlDriver(target()) await driver.query({ sql: 'select 1', rowLimit: 10 }) diff --git a/test/unit/db/postgres.test.ts b/test/unit/db/postgres.test.ts index 2329768..6d3363a 100644 --- a/test/unit/db/postgres.test.ts +++ b/test/unit/db/postgres.test.ts @@ -41,9 +41,14 @@ const pgState = vi.hoisted(() => { return { state, FakePool } }) -vi.mock('pg', () => ({ - default: { Pool: pgState.FakePool } -})) +vi.mock('pg', async () => { + // Use the real TypeOverrides so the driver's date/timestamp parser overrides + // are exercised against pg's genuine default parsers (e.g. timestamptz). + const actual = (await vi.importActual('pg')) as { default: { TypeOverrides: unknown } } + return { + default: { Pool: pgState.FakePool, TypeOverrides: actual.default.TypeOverrides } + } +}) import type { ConnectionConfig } from '../../../src/config/types.js' import { createPostgresDriver } from '../../../src/db/postgres.js' @@ -87,6 +92,27 @@ describe('createPostgresDriver', () => { expect(pgState.state.pools[1].options).toMatchObject({ database: 'other' }) }) + it('sets a connection timeout on every pool', async () => { + const driver = createPostgresDriver(target()) + await driver.query({ sql: 'select 1', rowLimit: 10 }) + expect(pgState.state.pools[0].options.connectionTimeoutMillis).toBe(10_000) + }) + + it('returns wall-clock date/timestamp types as raw strings, keeps tz absolute', async () => { + const driver = createPostgresDriver(target()) + await driver.query({ sql: 'select 1', rowLimit: 10 }) + const types = pgState.state.pools[0].options.types as { + getTypeParser: (oid: number, format?: string) => (value: string) => unknown + } + // DATE (1082) and TIMESTAMP without tz (1114) stay raw strings. + expect(types.getTypeParser(1082)('2026-06-11')).toBe('2026-06-11') + expect(types.getTypeParser(1114)('2026-06-11 12:34:56')).toBe('2026-06-11 12:34:56') + // TIMESTAMPTZ (1184) keeps the default parser, which is not identity. + expect(types.getTypeParser(1184)('2026-06-11 12:34:56+00')).not.toBe( + '2026-06-11 12:34:56+00' + ) + }) + it('passes readonly as a startup option', async () => { const driver = createPostgresDriver(target({ readonly: true })) await driver.query({ sql: 'select 1', rowLimit: 10 }) diff --git a/test/unit/db/sqlGuard.test.ts b/test/unit/db/sqlGuard.test.ts index 54dfab2..437c1c4 100644 --- a/test/unit/db/sqlGuard.test.ts +++ b/test/unit/db/sqlGuard.test.ts @@ -66,6 +66,24 @@ describe('assertSafeStatement', () => { ).not.toThrow() }) + it('does not treat a digit-leading dollar tag as a dollar quote', () => { + // $1$ is a positional param $1 followed by a bare $, NOT a dollar + // quote. The ; after it is top-level, so this is two statements. + expect(() => assertSafeStatement('SELECT $1$ ; DROP TABLE x', 'postgres')).toThrow( + /one SQL statement/ + ) + }) + + it('allows digits later inside a dollar tag', () => { + expect(() => + assertSafeStatement('select $tag1$ a ; b $tag1$', 'postgres') + ).not.toThrow() + }) + + it('keeps plain positional params with values unaffected', () => { + expect(() => assertSafeStatement('SELECT $1', 'postgres')).not.toThrow() + }) + it('ignores semicolons inside line comments', () => { expect(() => assertSafeStatement('select 1 -- ; select 2', 'postgres')).not.toThrow() expect(() => assertSafeStatement('select 1 # ; select 2', 'mysql')).not.toThrow() @@ -94,6 +112,25 @@ describe('assertSafeStatement', () => { expect(() => assertSafeStatement("select 'a\\'; select 2'", 'mysql')).not.toThrow() }) + it('handles mysql backslash escape inside double-quoted string', () => { + // mysql treats "..." as a string; \" does not close it, so the ; + // stays inside the literal and this is a single statement. + expect(() => assertSafeStatement('SELECT "x\\";y" AS c', 'mysql')).not.toThrow() + }) + + it('handles double-quote doubling in both engines', () => { + expect(() => assertSafeStatement('select "a""b;c"', 'mysql')).not.toThrow() + expect(() => assertSafeStatement('select "a""b;c"', 'postgres')).not.toThrow() + }) + + it('does not treat backslash as escape in postgres double-quoted identifiers', () => { + // For postgres "..." is an identifier with no backslash escapes, so + // "a\" closes at the second quote and '; SELECT 1' is a second statement. + expect(() => assertSafeStatement('SELECT "a\\";SELECT 1', 'postgres')).toThrow( + /one SQL statement/ + ) + }) + it('treats postgres E-strings as backslash-escaped (single statement)', () => { // E'it\'s; fine' is one literal: \' is an escaped quote, so the ; // stays inside the string and this is a single statement. diff --git a/test/unit/net/tunnel.test.ts b/test/unit/net/tunnel.test.ts index ff14765..c4ef20b 100644 --- a/test/unit/net/tunnel.test.ts +++ b/test/unit/net/tunnel.test.ts @@ -109,6 +109,23 @@ describe('buildSshAuth', () => { expect(auth).toEqual({ agent: '/tmp/agent.sock' }) }) + it('throws when agent is requested but SSH_AUTH_SOCK is unset', () => { + expect(() => buildSshAuth(sshConfig({ agent: true, password: 'pw' }), {})).toThrow( + /SSH_AUTH_SOCK is not set/ + ) + }) + + it('throws when agent is requested but SSH_AUTH_SOCK is empty', () => { + expect(() => buildSshAuth(sshConfig({ agent: true }), { SSH_AUTH_SOCK: '' })).toThrow( + /SSH_AUTH_SOCK is not set/ + ) + }) + + it('still prefers privateKey over a requested agent', () => { + const auth = buildSshAuth(sshConfig({ agent: true, privateKey: 'PEM' }), {}) + expect(auth).toEqual({ privateKey: 'PEM', passphrase: undefined }) + }) + it('falls back to password', () => { expect(buildSshAuth(sshConfig({ password: 'pw' }))).toEqual({ password: 'pw' }) })