[huyplb]

Two commits closing the identifier-injection class in generated DDL. The first is generic; the second is the ClickHouse-specific remainder it originally left open.

1. 933bce4 — a quoted identifier was trusted by its shape

ident passed a name through untouched when it merely started and ended with a quoting delimiter:

return (first === '"' && last === '"')
    || (first === '`' && last === '`')
    || (first === '[' && last === ']');

Identifiers come from introspecting a source database, so a column is called whatever somebody created — including something that only looks wrapped. A column named "a" INTEGER); DROP TABLE victim; --" satisfied that check and was emitted verbatim:

CREATE TABLE orders (
  "a" INTEGER); DROP TABLE victim; --" INTEGER,
  ok_col INTEGER
);

A valid CREATE, then a valid DROP, then a comment. Verified end to end: a hostile source database, introspected normally, produced a migration that Fox Schema's own splitter cut into three statements, and running it dropped a table on the target. Dialect-independent.

The check now validates the wrapper. Every dialect escapes its closing delimiter by doubling it ("", ``, ]]), so with the doubled pairs removed, a surviving delimiter means the body closes the identifier early and the rest isn't part of the name. Such a name goes to the dialect's quoter to be escaped from scratch:

>>> INJECTION BLOCKED      name preserved exactly: true

Escaping rather than rejecting keeps the name faithful — the payload column is still created on the target under its exact original name.

2. d191571 — ClickHouse's ENGINE clause bypassed ident

PRIMARY KEY. The dialect read names off table itself, so they never met a quoter:

) ENGINE = MergeTree PRIMARY KEY id) ORDER BY (id) -- ORDER BY (id, ts);

createTableSuffix now receives quotedPrimaryKey from the caller — the only place holding ident — and is documented to use the arrays rather than reading names off table. A primary key is a column list, so it is always quoted, with no expression passthrough.

ORDER BY. The caller decided what was an expression by looking at the text:

orderSrc.map((c) => (/[()]/.test(c) ? c : this.ident(c)))

That exists because a sorting key may legitimately be tuple() or cityHash64(id). But a column named id) ENGINE = MergeTree ORDER BY (id also contains a paren, so it went through raw and rewrote the clause.

Trust now comes from provenance, not text. TableSchema.orderByFromCatalog marks entries that are ClickHouse's own rendering of system.tables.sorting_key — already valid, correctly quoted SQL, and the only place an expression can come from. The provider sets it there and deliberately leaves it unset for the system.columns.is_in_sorting_key fallback, which pushes bare column names. Marked entries pass through verbatim; everything else is quoted.

Both call sites were checked

The SQL editor's table blueprint (tableBlueprintSql.ts) had the same paren rule and handed the dialect raw primaryKey.columns. Its names are typed by the person building the table, never a catalog expression, so they are now quoted with no passthrough. Its own quoting primitive (quoteIdentifierIfNeeded) was already sound — strict plain-name regex, otherwise always escaping — so nothing changed there.

Compatibility

Existing valid output is byte-identical, checked against the cases #594 added:

) ENGINE = MergeTree ORDER BY id;
) ENGINE = MergeTree ORDER BY (order_id, coupon_id);
) ENGINE = ReplacingMergeTree PRIMARY KEY id ORDER BY (id, code);
) ENGINE = MergeTree ORDER BY tuple();

Catalog expressions still survive (cityHash64(id), tuple(), `order id`). Bare, properly quoted and schema-qualified identifiers are unaffected: "my col", [my col], `my col`, "a""b", [a]]b], `ab` `` all emit unchanged, and a bare name stays bare.

Test plan

• npx vitest run — 6914 passed, 1 expected fail, 145 skipped (was 6883 at branch point; +31 tests)
• npx eslint . — exit 0
• npm run lint:security — exit 0
• npm run typecheck — exit 0 (the createTableSuffix signature changed)
• Full exploit chain re-run against a real SQLite source and target, before and after
• Live ClickHouse — not run; Docker has no daemon in this environment

Tests assert the structural property — the DDL stays one statement — plus the exact quoted form, rather than the absence of the payload's text, which necessarily still appears inside the escaped identifier. Both sets were run against the unfixed code: 6 of 96 fail without commit 1, and 8 of the ClickHouse cases fail without commit 2. The compatibility and catalog-expression cases pass either way by design, guarding against over-quoting.

Known trade-off

A catalog primary_key that is itself an expression is now emitted quoted, so such a table needs manual review rather than silently producing a key over a column that doesn't exist. PRIMARY KEY taking no expressions is deliberate.

Remaining injection risks — verified, not fixed here

Both are outside this change and need their own decision:

1. table.engine is interpolated raw into the suffix. A hostile engine string yields two statements:

   ) ENGINE = MergeTree ORDER BY (id); DROP TABLE victim; -- ORDER BY id;

   It comes from system.tables.engine, which ClickHouse renders itself, so the engine name is from a fixed set — but engine parameters (ReplicatedMergeTree('/path', '{replica}'), Distributed(cluster, db, table)) are chosen at CREATE TABLE time, so this deserves a look. Validating it would need care not to break parameterised engines.

2. A newline in a name escapes a -- review: comment — and this one is dialect-independent and broader than ClickHouse. ident quotes but does not escape newlines, and a -- comment ends at the line break, so a table named x\nDROP TABLE victim; -- produces a migration containing a standalone, executable:

   DROP TABLE victim;

   Confirmed with the splitter. Every -- review: / -- comment path that interpolates a name is affected, which is many of them, so the fix belongs at the comment-emitting layer rather than here.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: 1260af63-4e24-4342-ad4e-7fcff85c459a)

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: 12cbc2d6-6065-42f4-a8d1-a65f93f8d8d0)