fix(sql): stop trusting identifier text in generated DDL

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);
 }
 
 /**

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) => ({

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

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;
 }

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.

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<TableSchema> & { 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);
+    });
+  });
+});

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};`;
   }