mirror of
https://github.com/beekeeper-studio/beekeeper-studio.git
synced 2026-03-13 10:12:54 +08:00
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.
This commit is contained in:
committed by
Day Matchullis
parent
ab89dcaa67
commit
1c145aee61
@@ -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<TrinoResult> {
|
||||
|
||||
async listSchemas(filter: SchemaFilterOptions): Promise<string[]> {
|
||||
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<TrinoResult> {
|
||||
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<TrinoResult> {
|
||||
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<TrinoResult> {
|
||||
SELECT
|
||||
${wrappedSelects},
|
||||
ROW_NUMBER() OVER (${rowNumberOrderClause}) AS rownum
|
||||
FROM ${this.db}.${tableRef}
|
||||
FROM ${this.wrapIdentifier(this.db)}.${tableRef}
|
||||
${filter}
|
||||
)
|
||||
SELECT *
|
||||
|
||||
@@ -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 ');
|
||||
|
||||
@@ -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 --")
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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 = [
|
||||
|
||||
Reference in New Issue
Block a user