From 1c145aee61894924ba545332f13b4d39c19883a7 Mon Sep 17 00:00:00 2001 From: Matthew Rathbone Date: Fri, 6 Mar 2026 14:48:02 -0600 Subject: [PATCH] fix: escape user-supplied values in SQL queries (buildSchemaFilter + Trino) - buildSchemaFilter now uses escapeString() to escape single quotes in schema, only, and ignore filter values. - Trino driver now wraps this.db with wrapIdentifier() in all queries (listSchemas, listTables, listTableColumns, buildPaginatedQuery). - Trino listTableColumns escapes table and schema string literals. - Added unit tests for SQL injection in buildSchemaFilter and Trino. --- .../backend/lib/db/clients/trino.ts | 14 ++--- apps/studio/src/lib/db/clients/utils.ts | 6 +- .../tests/unit/lib/db/clients/trino.spec.ts | 58 +++++++++++++++++-- .../tests/unit/lib/db/clients/utils.spec.js | 27 ++++++++- 4 files changed, 88 insertions(+), 17 deletions(-) diff --git a/apps/studio/src-commercial/backend/lib/db/clients/trino.ts b/apps/studio/src-commercial/backend/lib/db/clients/trino.ts index bef34e2ba..46e4f111b 100644 --- a/apps/studio/src-commercial/backend/lib/db/clients/trino.ts +++ b/apps/studio/src-commercial/backend/lib/db/clients/trino.ts @@ -43,7 +43,7 @@ import { createCancelablePromise, joinFilters } from "@/common/utils" -import { buildSchemaFilter } from "@/lib/db/clients/utils" +import { buildSchemaFilter, escapeString } from "@/lib/db/clients/utils" import { AlterTableSpec, TableKey @@ -296,7 +296,7 @@ export class TrinoClient extends BasicDatabaseClient { async listSchemas(filter: SchemaFilterOptions): Promise { log.info('filters in listSchemas', filter) - const sql = `show schemas from ${this.db}` + const sql = `show schemas from ${this.wrapIdentifier(this.db)}` const result = await this.driverExecuteSingle(sql) return result?.rows ? result.rows.map((row) => row.Schema) : [] @@ -306,7 +306,7 @@ export class TrinoClient extends BasicDatabaseClient { log.info('filters in listTables', filter) const schemaFilter = buildSchemaFilter(filter, 'table_schema') const whereClause = schemaFilter ? `WHERE ${schemaFilter}` : '' - const sql = `select * from ${this.db}.information_schema.tables ${whereClause}` + const sql = `select * from ${this.wrapIdentifier(this.db)}.information_schema.tables ${whereClause}` const result = await this.driverExecuteSingle(sql) return result.rows.map((row) => ({ @@ -320,9 +320,9 @@ export class TrinoClient extends BasicDatabaseClient { const sql = ` SELECT * - FROM ${this.db}.information_schema.columns - WHERE table_schema = '${schema}' - AND table_name = '${table}' + FROM ${this.wrapIdentifier(this.db)}.information_schema.columns + WHERE table_schema = '${escapeString(schema)}' + AND table_name = '${escapeString(table)}' ORDER BY ordinal_position ` const result = await this.driverExecuteSingle(sql) @@ -714,7 +714,7 @@ export class TrinoClient extends BasicDatabaseClient { SELECT ${wrappedSelects}, ROW_NUMBER() OVER (${rowNumberOrderClause}) AS rownum - FROM ${this.db}.${tableRef} + FROM ${this.wrapIdentifier(this.db)}.${tableRef} ${filter} ) SELECT * diff --git a/apps/studio/src/lib/db/clients/utils.ts b/apps/studio/src/lib/db/clients/utils.ts index f0ea7b457..551b9c493 100644 --- a/apps/studio/src/lib/db/clients/utils.ts +++ b/apps/studio/src/lib/db/clients/utils.ts @@ -48,17 +48,17 @@ export function buildSchemaFilter(filter, schemaField = 'schema_name') { const { schema, only, ignore } = filter if (schema) { - return `${schemaField} = '${schema}'`; + return `${schemaField} = '${escapeString(schema)}'`; } const where = []; if (only && only.length) { - where.push(`${schemaField} IN (${only.map((name) => `'${name}'`).join(',')})`); + where.push(`${schemaField} IN (${only.map((name) => `'${escapeString(name)}'`).join(',')})`); } if (ignore && ignore.length) { - where.push(`${schemaField} NOT IN (${ignore.map((name) => `'${name}'`).join(',')})`); + where.push(`${schemaField} NOT IN (${ignore.map((name) => `'${escapeString(name)}'`).join(',')})`); } return where.join(' AND '); diff --git a/apps/studio/tests/unit/lib/db/clients/trino.spec.ts b/apps/studio/tests/unit/lib/db/clients/trino.spec.ts index 1d3d7ca0f..7ff84edfd 100644 --- a/apps/studio/tests/unit/lib/db/clients/trino.spec.ts +++ b/apps/studio/tests/unit/lib/db/clients/trino.spec.ts @@ -1,11 +1,16 @@ +const capturedQueries: string[] = [] + jest.mock('trino-client', () => { - const mockQuery = jest.fn().mockResolvedValue({ - [Symbol.asyncIterator]: async function* () { - yield { - data: [['1.0.0']], - columns: [{ name: '_col0', type: 'varchar' }] + const mockQuery = jest.fn().mockImplementation((sql: string) => { + capturedQueries.push(sql) + return Promise.resolve({ + [Symbol.asyncIterator]: async function* () { + yield { + data: [['1.0.0']], + columns: [{ name: '_col0', type: 'varchar' }] + } } - } + }) }) return { @@ -139,3 +144,44 @@ describe('TrinoClient SSL configuration (bug #3695)', () => { expect(createCall.ssl).toBeUndefined() }) }) + +describe('TrinoClient SQL escaping', () => { + let client: TrinoClient + + beforeEach(async () => { + jest.clearAllMocks() + capturedQueries.length = 0 + client = new TrinoClient(makeServer(), makeDatabase()) + await client.connect() + capturedQueries.length = 0 + }) + + it('should wrap catalog name with identifier quoting in listSchemas', async () => { + const maliciousDb = "cat; DROP TABLE users --" + ;(client as any).db = maliciousDb + await client.listSchemas(null) + + const sql = capturedQueries[0] + // Catalog name must be inside double-quote identifiers + expect(sql).toContain('"cat; DROP TABLE users --"') + }) + + it('should wrap catalog name with identifier quoting in listTables', async () => { + const maliciousDb = "cat; DROP TABLE users --" + ;(client as any).db = maliciousDb + await client.listTables(null) + + const sql = capturedQueries[0] + // Catalog name must be inside double-quote identifiers + expect(sql).toContain('"cat; DROP TABLE users --".information_schema') + }) + + it('should escape schema and table names in listTableColumns', async () => { + await client.listTableColumns("test'; DROP TABLE users --", "public'; DROP TABLE users --") + + const sql = capturedQueries[0] + // Single quotes in values must be doubled to stay inside SQL string literals + expect(sql).toContain("public''; DROP TABLE users --") + expect(sql).toContain("test''; DROP TABLE users --") + }) +}) diff --git a/apps/studio/tests/unit/lib/db/clients/utils.spec.js b/apps/studio/tests/unit/lib/db/clients/utils.spec.js index 74de6aeec..75186ff1c 100644 --- a/apps/studio/tests/unit/lib/db/clients/utils.spec.js +++ b/apps/studio/tests/unit/lib/db/clients/utils.spec.js @@ -1,4 +1,4 @@ -import { buildSelectTopQuery, escapeString, isAllowedReadOnlyQuery } from "../../../../../src/lib/db/clients/utils"; +import { buildSchemaFilter, buildSelectTopQuery, escapeString, isAllowedReadOnlyQuery } from "../../../../../src/lib/db/clients/utils"; describe('Escape String', () => { it("should escape single quotes", () => { @@ -82,6 +82,31 @@ describe("buildSelectTopQuery", () => { }) }) +describe('buildSchemaFilter SQL injection', () => { + it("should escape single quotes in schema name to prevent SQL injection", () => { + const malicious = "'; DROP TABLE users; --" + const result = buildSchemaFilter({ schema: malicious }) + // The single quote must be doubled so the value stays inside the SQL string literal + // Safe: schema_name = '''; DROP TABLE users; --' (the '' is an escaped quote inside the literal) + // Unsafe: schema_name = ''; DROP TABLE users; --' (the ' closes the literal, rest is executable) + // Result: schema_name = '''; DROP TABLE users; --' + // In SQL: opening ', then '' (escaped quote), then rest of string, closing ' + // The whole value is treated as one string literal — safe. + expect(result).toBe("schema_name = '''; DROP TABLE users; --'") + }) + + it("should escape single quotes in 'only' filter values", () => { + const result = buildSchemaFilter({ only: ["public", "'; DROP TABLE users; --"] }) + expect(result).toContain("'public'") + expect(result).toContain("'''; DROP TABLE users; --'") + }) + + it("should escape single quotes in 'ignore' filter values", () => { + const result = buildSchemaFilter({ ignore: ["'; DROP TABLE users; --"] }) + expect(result).toContain("'''; DROP TABLE users; --'") + }) +}) + describe('isAllowedReadOnly', () => { it('Should return as a read only query', () => { const queries = [