diff --git a/apps/web/src/frontend/features/sql-editor/lib/tableBlueprintSql.ts b/apps/web/src/frontend/features/sql-editor/lib/tableBlueprintSql.ts index c32f0279..f6f829ae 100644 --- a/apps/web/src/frontend/features/sql-editor/lib/tableBlueprintSql.ts +++ b/apps/web/src/frontend/features/sql-editor/lib/tableBlueprintSql.ts @@ -890,8 +890,15 @@ function createTableSuffixSql( foreignKeys: [], ...(pkColumns.length ? { primaryKey: { columns: pkColumns } } : {}), }; - const quoted = pkColumns.map((c) => (/[()]/.test(c) ? c : quoteIdent(c, dialectName))); - return dialect.createTableSuffix(table, quoted); + // A blueprint's column names are typed by the person building the table, so + // they are identifiers — never the engine-rendered sorting key an expression + // could legitimately come from (`table.orderByFromCatalog` is unset here). + // They are quoted, with no passthrough: the old rule let any name containing + // a paren through raw, which is enough to close the clause and append DDL. + // ORDER BY and PRIMARY KEY are the same list, since a blueprint has no + // separate sorting key. + const quoted = pkColumns.map((c) => quoteIdent(c, dialectName)); + return dialect.createTableSuffix(table, quoted, quoted); } /** diff --git a/packages/db/src/providers/clickHouse/clickhouse.provider.ts b/packages/db/src/providers/clickHouse/clickhouse.provider.ts index 9ea56994..b96dca0c 100644 --- a/packages/db/src/providers/clickHouse/clickhouse.provider.ts +++ b/packages/db/src/providers/clickHouse/clickhouse.provider.ts @@ -117,7 +117,10 @@ export class ClickHouseProvider implements SchemaProvider { uniqueConstraints: [], indexes: [], ...(t.engine ? { engine: t.engine } : {}), - ...(orderBy.length ? { orderBy } : {}), + // Marked as engine-rendered: sorting_key is ClickHouse's own output, so + // its entries are already quoted and may be expressions. The column-flag + // fallback below pushes bare names and deliberately leaves this unset. + ...(orderBy.length ? { orderBy, orderByFromCatalog: true } : {}), }; columns[t.name] = []; primaryKeys[t.name] = fromPk.map((column, i) => ({ diff --git a/packages/sql/src/cores/schema-to-tables.ts b/packages/sql/src/cores/schema-to-tables.ts index 810624e0..a501aa7b 100644 --- a/packages/sql/src/cores/schema-to-tables.ts +++ b/packages/sql/src/cores/schema-to-tables.ts @@ -206,6 +206,9 @@ export function dbSchemaToTableSchemas(dbSchema: DbSchema): TableSchema[] { tablespace: table.tablespace, ...(table.engine ? { engine: table.engine } : {}), ...(table.orderBy?.length ? { orderBy: table.orderBy } : {}), + // Carried, not inferred: it says whether those entries are engine-rendered + // SQL or bare column names, which decides whether they get quoted. + ...(table.orderBy?.length && table.orderByFromCatalog ? { orderByFromCatalog: true } : {}), ...(table.storeType ? { storeType: table.storeType } : {}), triggers: triggersByTable.get(table.name) ?? [], primaryKey: pkColumns.length > 0 diff --git a/packages/sql/src/interfaces/schema.interface.ts b/packages/sql/src/interfaces/schema.interface.ts index a01b88ac..542499d0 100644 --- a/packages/sql/src/interfaces/schema.interface.ts +++ b/packages/sql/src/interfaces/schema.interface.ts @@ -123,6 +123,18 @@ export interface TableSchema { * prefix-plus-extra of `primaryKey.columns`. Absent elsewhere. */ orderBy?: string[]; + /** + * `orderBy` came from the engine's own rendering of the sorting key + * (ClickHouse `system.tables.sorting_key`), so each entry is already valid, + * correctly quoted SQL and may be an expression such as `cityHash64(id)`. + * The generator emits those verbatim. + * + * Absent means the entries are plain column names — from + * `system.columns.is_in_sorting_key`, or borrowed from the primary key — and + * must be identifier-quoted before they reach SQL. A column is named whatever + * somebody created, so an unquoted one can carry DDL out of the clause. + */ + orderByFromCatalog?: boolean; /** * What a non-SQL store keeps behind this "table" — for Redis `hash`, `json`, * `string`, `mixed`, `list`, `set`, `zset` or `stream`. Shown as a badge; @@ -166,6 +178,8 @@ export interface DbTable { engine?: string; /** See `TableSchema.orderBy`. */ orderBy?: string[]; + /** See `TableSchema.orderByFromCatalog`. */ + orderByFromCatalog?: boolean; /** See `TableSchema.storeType`. */ storeType?: string; } diff --git a/packages/sql/src/modules/dialect/sql-dialect.interface.ts b/packages/sql/src/modules/dialect/sql-dialect.interface.ts index cb6d1b78..72029081 100644 --- a/packages/sql/src/modules/dialect/sql-dialect.interface.ts +++ b/packages/sql/src/modules/dialect/sql-dialect.interface.ts @@ -98,9 +98,20 @@ export interface SqlDialect { /** * Clauses after the CREATE TABLE column list. ClickHouse: ` ENGINE = MergeTree ORDER BY …`. - * `quotedOrderBy` is identifier-quoted, or left raw for a function expression. - */ - createTableSuffix?(table: TableSchema, quotedOrderBy: string[]): string; + * + * Both arrays arrive ready to emit, and an implementation must use them + * rather than reading names off `table`: identifiers there are raw catalog + * values, and a column is named whatever somebody created, so emitting one + * unquoted lets it carry DDL out of the clause. + * + * `quotedOrderBy` is identifier-quoted, except when + * `table.orderByFromCatalog` says the entries are the engine's own rendering + * of the sorting key — those are already valid SQL and may be expressions + * such as `cityHash64(id)`, so they pass through verbatim. + * `quotedPrimaryKey` is always identifier-quoted: a primary key is a column + * list, never an expression. + */ + createTableSuffix?(table: TableSchema, quotedOrderBy: string[], quotedPrimaryKey: string[]): string; /** * ENGINE / ORDER BY changed. Omit when the dialect has no table engine. diff --git a/packages/sql/src/modules/migrations/sql-generator.module.test.ts b/packages/sql/src/modules/migrations/sql-generator.module.test.ts index b60aef7b..dad1682a 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.test.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.test.ts @@ -4,6 +4,7 @@ import { db2SqlDialect } from '../../providers/db2/db2.sql-dialect.js'; import { DIALECT_MAP } from '../dialect/registry.js'; import { TableDiff } from '../../interfaces/index.js'; import { TableSchema } from '../../interfaces/index.js'; +import { splitSqlStatements } from '../sql-text/sql-splitter.js'; const gen = new SqlGeneratorModule(); @@ -1666,3 +1667,206 @@ END`; expect(sql).not.toMatch(/END;;/); }); }); + +describe('identifier quoting: a name that only looks quoted is escaped, not trusted', () => { + // Names come from introspecting a source database. A column may be called + // something that merely looks wrapped, and emitting it verbatim turned the + // migration into "a valid CREATE, then a valid DROP" — a source schema the + // user does not control could inject DDL into their own migration. + // + // The property under test is structural, not textual: the payload's text + // necessarily still appears inside the escaped identifier. What must not + // happen is it escaping that identifier and becoming code. + const payload = '"a" INTEGER); DROP TABLE victim; --"'; + + const tableWith = (colName: string) => + tableSchema({ + name: 'orders', + columns: [ + { name: colName, type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'ok_col', type: 'INTEGER', nullable: true, primaryKey: false }, + ], + }); + + it.each(['postgres', 'sqlite', 'mysql', 'db2', 'sqlserver'])( + 'keeps the payload inside one statement on %s', + (dialect) => { + const ddl = gen.generateObjectDdl(tableWith(payload), dialect); + const stmts = splitSqlStatements(ddl); + // Before the fix this split into CREATE TABLE … / DROP TABLE victim / junk. + expect(stmts).toHaveLength(1); + expect(stmts.some((s) => /^\s*DROP\s+TABLE\s+victim/i.test(s.text))).toBe(false); + } + ); + + it('escapes it so the name still round-trips exactly (ANSI)', () => { + const ddl = gen.generateObjectDdl(tableWith(payload), 'postgres'); + // `"` doubled inside the wrapper is the name verbatim, not a truncation. + expect(ddl).toContain('"""a"" INTEGER); DROP TABLE victim; --"""'); + }); + + it.each([ + ['a plain quoted name', '"my col"'], + ['an escaped inner quote', '"a""b"'], + ['a bracketed name', '[my col]'], + ['an escaped inner bracket', '[a]]b]'], + ['a backticked name', '`my col`'], + ['an escaped inner backtick', '`a``b`'], + ])('passes %s through without re-quoting it', (_label, name) => { + const ddl = gen.generateObjectDdl(tableWith(name), 'postgres'); + expect(ddl).toContain(`${name} INTEGER`); + }); + + it('leaves an ordinary bare name bare', () => { + const ddl = gen.generateObjectDdl(tableWith('customer_id'), 'postgres'); + expect(ddl).toContain('customer_id INTEGER'); + expect(ddl).not.toContain('"customer_id"'); + }); + + it('still quotes a name that genuinely needs it', () => { + const ddl = gen.generateObjectDdl(tableWith('order id'), 'postgres'); + expect(ddl).toContain('"order id" INTEGER'); + }); + + it('escapes a lone delimiter, which cannot be a wrapper', () => { + for (const name of ['"', '"abc', 'abc"', '[abc', '`abc']) { + const ddl = gen.generateObjectDdl(tableWith(name), 'postgres'); + expect(ddl).toContain(`"${name.replace(/"/g, '""')}" INTEGER`); + } + }); +}); + +describe('ClickHouse ENGINE clause: identifiers are quoted, catalog expressions are not', () => { + const chTable = (over: Partial & { name: string }) => + tableSchema({ objectType: 'TABLE', ...over }); + const col = (name: string) => ({ name, type: 'Int32', nullable: false, primaryKey: false }); + /** The ` ENGINE = … ORDER BY …` suffix, which is the clause under test. */ + // The `)` that closes the column list starts its own line; the suffix follows + // it. Anchoring on the last `)` would instead find the ORDER BY tuple's. + const suffixOf = (ddl: string) => ddl.slice(ddl.indexOf('\n)') + 1).trim(); + + describe('existing output is unchanged', () => { + it.each([ + [ + 'a logical primary key becomes ORDER BY', + chTable({ name: 'coupons', primaryKey: { name: 'PRIMARY_KEY', columns: ['id'] }, columns: [col('id')] }), + ') ENGINE = MergeTree ORDER BY id;', + ], + [ + 'a composite key is wrapped', + chTable({ + name: 'oc', + primaryKey: { name: 'PRIMARY_KEY', columns: ['order_id', 'coupon_id'] }, + columns: [col('order_id'), col('coupon_id')], + }), + ') ENGINE = MergeTree ORDER BY (order_id, coupon_id);', + ], + [ + 'an engine PRIMARY KEY is emitted when it differs from ORDER BY', + chTable({ + name: 'c', + engine: 'ReplacingMergeTree', + primaryKey: { name: 'PRIMARY_KEY', columns: ['id'] }, + orderBy: ['id', 'code'], + columns: [col('id'), col('code')], + }), + ') ENGINE = ReplacingMergeTree PRIMARY KEY id ORDER BY (id, code);', + ], + ['no key at all falls back to tuple()', chTable({ name: 'e', columns: [col('id')] }), ') ENGINE = MergeTree ORDER BY tuple();'], + ])('%s', (_label, table, expected) => { + expect(suffixOf(gen.generateObjectDdl(table, 'clickhouse'))).toBe(expected); + }); + }); + + describe('a catalog-rendered sorting key is kept verbatim', () => { + // `orderByFromCatalog` marks system.tables.sorting_key — already valid, + // correctly quoted SQL, and the only place an expression may come from. + it.each([ + ['a function expression', ['cityHash64(id)'], ') ENGINE = MergeTree ORDER BY cityHash64(id);'], + ['an empty tuple', ['tuple()'], ') ENGINE = MergeTree ORDER BY tuple();'], + ['a composite expression', ['cityHash64(id)', 'created_at'], ') ENGINE = MergeTree ORDER BY (cityHash64(id), created_at);'], + ['a name the engine quoted itself', ['`order id`'], ') ENGINE = MergeTree ORDER BY `order id`;'], + ])('%s', (_label, orderBy, expected) => { + const ddl = gen.generateObjectDdl( + chTable({ name: 'e', engine: 'MergeTree', orderBy, orderByFromCatalog: true, columns: [col('id')] }), + 'clickhouse' + ); + expect(suffixOf(ddl)).toBe(expected); + }); + }); + + describe('an unmarked key is a column name and gets quoted', () => { + // Same text, without the marker: a plain column name from + // system.columns.is_in_sorting_key, so it must not reach SQL raw. + it('quotes a name containing parentheses', () => { + const name = 'id) ENGINE = MergeTree ORDER BY (id'; + const ddl = gen.generateObjectDdl( + chTable({ name: 'e', engine: 'MergeTree', orderBy: [name], columns: [col(name)] }), + 'clickhouse' + ); + expect(suffixOf(ddl)).toBe(`) ENGINE = MergeTree ORDER BY "${name}";`); + expect(splitSqlStatements(ddl)).toHaveLength(1); + }); + }); + + describe('injection attempts stay inside one statement', () => { + /** The dialect has no quoteIdentifier override, so ClickHouse quotes ANSI. */ + const quoted = (name: string) => `"${name.replace(/"/g, '""')}"`; + + // Each payload carries a parenthesis, because the rule being replaced passed + // any entry containing one through raw — without it these would not tell a + // fixed generator from a broken one. The `;` then starts a second statement. + it.each([ + ['a parenthesised statement break', 'id); DROP TABLE victim; --'], + ['a malformed quoted name', '"a" Int32) ORDER BY (x); DROP TABLE victim; --"'], + ['a clause rewrite with no semicolon', 'id) ENGINE = MergeTree ORDER BY (id'], + ])('ORDER BY: %s', (_label, name) => { + const ddl = gen.generateObjectDdl( + chTable({ name: 'e', engine: 'MergeTree', orderBy: [name], columns: [col(name)] }), + 'clickhouse' + ); + expect(suffixOf(ddl)).toContain(quoted(name)); + expect(splitSqlStatements(ddl)).toHaveLength(1); + expect(ddl).not.toMatch(/^\s*DROP\s+TABLE/im); + }); + + it.each([ + ['a parenthesised statement break', 'id); DROP TABLE victim; --'], + ['a malformed quoted name', '"a") ORDER BY (x); DROP TABLE victim; --"'], + ['a plain statement break', 'id; DROP TABLE victim; --'], + ])('PRIMARY KEY: %s', (_label, name) => { + // PRIMARY KEY is only emitted when it differs from ORDER BY. + const ddl = gen.generateObjectDdl( + chTable({ + name: 'e', + engine: 'MergeTree', + primaryKey: { name: 'PRIMARY_KEY', columns: [name] }, + orderBy: ['id', 'ts'], + columns: [col(name), col('id'), col('ts')], + }), + 'clickhouse' + ); + expect(suffixOf(ddl)).toContain(`PRIMARY KEY ${quoted(name)}`); + expect(splitSqlStatements(ddl)).toHaveLength(1); + expect(ddl).not.toMatch(/^\s*DROP\s+TABLE/im); + }); + + it('never takes the primary key straight from the table, marked or not', () => { + const name = 'id); DROP TABLE victim; --'; + // The catalog marker licenses ORDER BY only; it must not reach PRIMARY KEY. + const ddl = gen.generateObjectDdl( + chTable({ + name: 'e', + engine: 'MergeTree', + primaryKey: { name: 'PRIMARY_KEY', columns: [name] }, + orderBy: ['id'], + orderByFromCatalog: true, + columns: [col(name), col('id')], + }), + 'clickhouse' + ); + expect(ddl).toContain(`PRIMARY KEY ${quoted(name)}`); + expect(splitSqlStatements(ddl)).toHaveLength(1); + }); + }); +}); diff --git a/packages/sql/src/modules/migrations/sql-generator.module.ts b/packages/sql/src/modules/migrations/sql-generator.module.ts index 6e3d058a..e0772737 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.ts @@ -98,20 +98,40 @@ function ansiQuoteIdentifier(name: string): string { } /** - * Already wrapped — by an earlier pass here, or by the catalog that handed it - * over. Quoting it again nests the quotes and breaks the statement, and - * `bareName` genuinely is applied twice on some paths (the primary-key name - * reaches it once on the way in and once on the way out). + * Already wrapped, **and wrapped correctly** — by an earlier pass here, or by + * the catalog that handed it over. Such a name is passed through, because + * quoting it again nests the quotes and breaks the statement, and `bareName` + * genuinely is applied twice on some paths (the primary-key name reaches it + * once on the way in and once on the way out). + * + * Matching the delimiters alone is not enough, and the difference is a DDL + * injection. Names come from introspecting a source database, so a column may + * be called whatever somebody created — including something that merely + * *looks* wrapped: + * + * "a" INTEGER); DROP TABLE victim; --" + * + * That starts and ends with `"`, so a shape check called it quoted and emitted + * it verbatim, and `CREATE TABLE … ("a" INTEGER); DROP TABLE victim; --" …)` + * is a valid CREATE followed by a valid DROP. A source schema the user does not + * control — a vendor database, a restored dump, a shared or multi-tenant one — + * could therefore put arbitrary DDL into the migration they run on their target. + * + * So the body is checked too. Every dialect here escapes the closing delimiter + * by doubling it (`""`, ```` `` ````, `]]`), so with the doubled pairs removed a + * surviving delimiter means the body closes the identifier early and whatever + * follows is not part of the name. That name is not well-formed, and `ident` + * sends it to the dialect's quoter to be escaped from scratch instead. */ -function isQuotedIdentifier(name: string): boolean { +function isWellFormedQuotedIdentifier(name: string): boolean { if (name.length < 2) return false; - const first = name[0]!; - const last = name[name.length - 1]!; - return ( - (first === '"' && last === '"') || - (first === '`' && last === '`') || - (first === '[' && last === ']') - ); + const open = name[0]!; + if (open !== '"' && open !== '`' && open !== '[') return false; + // `[` is closed by `]`; the others close with the character that opened them. + const close = open === '[' ? ']' : open; + if (name[name.length - 1] !== close) return false; + const body = name.slice(1, -1); + return !body.split(close + close).join('').includes(close); } export class SqlGeneratorModule { @@ -150,7 +170,7 @@ export class SqlGeneratorModule { * without rewriting the dialect test suites. */ private ident(name: string): string { - if (isBareIdentifier(name) || isQuotedIdentifier(name)) return name; + if (isBareIdentifier(name) || isWellFormedQuotedIdentifier(name)) return name; return this.quoteIdentifier(name); } @@ -349,13 +369,21 @@ export class SqlGeneratorModule { let suffix = ''; if (dialect.createTableSuffix) { - const orderSrc = table.orderBy?.length ? table.orderBy : pkCols; - // ClickHouse sorting_key can be `tuple()` or `cityHash64(id)` — those are - // expressions, not identifiers, and quoting them is a syntax error. - suffix = dialect.createTableSuffix( - table, - orderSrc.map((c) => (/[()]/.test(c) ? c : this.ident(c))) - ); + // A sorting key the engine rendered itself (ClickHouse `sorting_key`) is + // already valid SQL and may be an expression — `tuple()`, `cityHash64(id)` + // — so it is emitted verbatim. Anything else is a plain column name and is + // quoted. + // + // Trust comes from `orderByFromCatalog`, never from the text: the previous + // rule passed any entry containing a paren through unquoted, which a column + // named `id) ORDER BY (id) --` satisfied, so a source schema could carry + // DDL out of the ORDER BY clause. + const quotedOrderBy = + table.orderByFromCatalog && table.orderBy?.length + ? [...table.orderBy] + : (table.orderBy?.length ? table.orderBy : pkCols).map((c) => this.ident(c)); + // A primary key is a column list, so it is always quoted. + suffix = dialect.createTableSuffix(table, quotedOrderBy, pkCols.map((c) => this.ident(c))); } return `CREATE TABLE ${this.qualify(table.name, mapping)} (\n${lines.join(',\n')}\n)${suffix};`; } diff --git a/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.test.ts b/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.test.ts index f9a702f3..639a51a6 100644 --- a/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.test.ts +++ b/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.test.ts @@ -43,19 +43,19 @@ describe('clickHouseSqlDialect CREATE TABLE suffix', () => { }); it('defaults to MergeTree ORDER BY the quoted key', () => { - expect(clickHouseSqlDialect.createTableSuffix?.(table(), ['id'])).toBe( + expect(clickHouseSqlDialect.createTableSuffix?.(table(), ['id'], [])).toBe( ' ENGINE = MergeTree ORDER BY id' ); }); it('wraps a composite ORDER BY', () => { - expect(clickHouseSqlDialect.createTableSuffix?.(table({ engine: 'MergeTree' }), ['id', 'code'])).toBe( + expect(clickHouseSqlDialect.createTableSuffix?.(table({ engine: 'MergeTree' }), ['id', 'code'], [])).toBe( ' ENGINE = MergeTree ORDER BY (id, code)' ); }); it('uses tuple() when there is no key', () => { - expect(clickHouseSqlDialect.createTableSuffix?.(table(), [])).toBe( + expect(clickHouseSqlDialect.createTableSuffix?.(table(), [], [])).toBe( ' ENGINE = MergeTree ORDER BY tuple()' ); }); @@ -69,7 +69,7 @@ describe('clickHouseSqlDialect CREATE TABLE suffix', () => { primaryKey: { name: 'PRIMARY_KEY', columns: ['id'] }, orderBy: ['id', 'code'], }); - expect(clickHouseSqlDialect.createTableSuffix?.(t, ['id', 'code'])).toBe( + expect(clickHouseSqlDialect.createTableSuffix?.(t, ['id', 'code'], ['id'])).toBe( ' ENGINE = MergeTree PRIMARY KEY id ORDER BY (id, code)' ); }); diff --git a/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.ts b/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.ts index 009e50af..803e7fb4 100644 --- a/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.ts +++ b/packages/sql/src/providers/clickHouse/clickhouse.sql-dialect.ts @@ -200,13 +200,16 @@ export const clickHouseSqlDialect: SqlDialect = { // CHECK / ASSUME. The key is ENGINE … ORDER BY after the closing paren. inlinePrimaryKeyConstraint: false, - createTableSuffix(table: TableSchema, quotedOrderBy: string[]): string { - const pkCols = table.primaryKey?.columns ?? []; + createTableSuffix(table: TableSchema, quotedOrderBy: string[], quotedPrimaryKey: string[]): string { // ClickHouse PRIMARY KEY defaults to ORDER BY; only emit it when they differ. + // The comparison is on the raw catalog values, which is what both quoted + // arrays were built from — quoting is for emitting, not for matching. const pkClause = - pkCols.length > 0 && !sameKeyList(pkCols, clickHouseOrderBy(table)) - ? ` PRIMARY KEY ${clickHouseOrderBySql(pkCols)}` + quotedPrimaryKey.length > 0 && !sameKeyList(table.primaryKey?.columns ?? [], clickHouseOrderBy(table)) + ? ` PRIMARY KEY ${clickHouseOrderBySql(quotedPrimaryKey)}` : ''; + // Only the pre-quoted arrays reach SQL: never `table.primaryKey.columns`, + // whose raw names let a hostile column close the clause and append DDL. return ` ${clickHouseEngineSql(table.engine)}${pkClause} ORDER BY ${clickHouseOrderBySql(quotedOrderBy)}`; },