fix(h2): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2177
fix(h2): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2177HandSonic wants to merge 4 commits into
Conversation
…tterMind#1914) Override quoteIdentifier/quoteQualifiedIdentifier/buildTableName/buildColumns and reimplement buildCreateTable/buildAlterTable/buildUpdate/buildTemplate/ buildCreateDatabase in H2SqlBuilder so every identifier is double-quote escaped and every comment literal is single-quote escaped. Validate metadata TYPE_NAME against an allow-list and neutralize hostile COLUMN_DEF defaults in generated DDL. Apply the export SCRIPT NODATA sentinel before substituting the escaped schema name so schema names containing NODATA are not corrupted. Document the raw-name contract on H2SqlEscapes and add attack-string tests for the builder, manager and metadata paths.
…tterMind#1914) Route column.getColumnType() in buildCreateTable and generateAlterColumnSql (ADD/MODIFY) through H2SqlEscapes.requireSafeTypeName so a hostile type string cannot break out of generated DDL. Adds attack-string rejection tests.
openai0229
left a comment
There was a problem hiding this comment.
Thanks for hardening these second-order SQL injection paths. The escaping behavior added here is needed, but H2SqlEscapes creates a second dialect-escaping abstraction. The Community SPI already defines ISQLIdentifierProcessor as the owner of both identifier quoting (quoteIdentifier) and SQL string-literal escaping (escapeString). H2 currently inherits DefaultSQLIdentifierProcessor through DefaultMetaService.
Please implement an H2-specific identifier processor (or strengthen the appropriate shared processor), return it from H2Meta#getSQLIdentifierProcessor(), and reuse that processor from H2Meta, H2DBManager, and H2SqlBuilder instead of routing these call sites through H2SqlEscapes.
Simply switching to the current DefaultSQLIdentifierProcessor is not sufficient: its quoteIdentifier only wraps invalid names and does not double embedded double quotes, while its escapeString preserves adjacent quote pairs instead of encoding every quote in a raw value. The processor implementation therefore needs focused coverage for embedded identifier quotes, already quoted identifiers, consecutive literal quotes, and case-sensitive H2 metadata names. Identifier quoting and string-literal escaping should remain separate processor methods.
This keeps one reusable dialect contract and prevents future H2 SQL-building call sites from bypassing the hardening.
|
Would you be willing to take this opportunity to improve this class of architectural issue more broadly, rather than limiting the change to the seven H2 call sites in this diff? It looks likely that identifier quoting and SQL string-literal escaping have drifted away from This is a broader improvement request, so please let us know whether you would be willing to take it on as part of this work, or as focused follow-up work if the scope is too large for this PR. |
… review (OtterMind#1914) - new H2IdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier with double-quote doubling, escapeString with single-quote doubling - H2Meta overrides getSQLIdentifierProcessor(); metadata call sites use it - builders/managers use H2IdentifierProcessor.INSTANCE - non-escapable validation moved to H2SqlGuards (type names, column defaults) - H2SqlEscapes removed; tests migrated (27 green)
|
Agreed — thank you for the direction. I've reworked this branch onto the SPI contract:
I will roll the same pattern out to the sibling PRs (#2172-#2176 and #2194-#2206): each plugin gets its escaping consolidated into its own |
Related issue
Part of the SQL-injection hardening tracked in #1914 (multi-plugin effort; this PR covers H2). Prior art: #2052 (Oracle
escapeSqlLiteral), #2053 (SQL Server identifier quoting).Summary
These are second-order injection paths: values such as table/schema/view/index names originate from the connected database's own metadata, so exploitation requires a maliciously named object in a target database. The fix is still worthwhile hardening and matches the pattern already merged for Oracle and SQL Server.
What changed:
H2SqlEscapeshelper:escapeSqlLiteral— neutralizes values interpolated into single-quoted SQL string literals (single-quote doubling).quoteIdentifier/escapeIdentifier— strips one surrounding quote pair and doubles every embedded double quote (") for quoted-identifier positions.tableDDLnow double-quotes identifiers; quoted output is more faithful to actual object names (previously identifiers were emitted unquoted). The assertion inH2MetaResultSetLifecycleTestwas updated accordingly — it now expects the quoted form rather than a weakened check.H2SqlEscapesTestcovering literal doubling, identifier doubling, and malicious-name neutralization.Affected surfaces
Verification
mvn -B -pl chat2db-community-plugins/chat2db-community-h2 -f chat2db-community-server/pom.xml -Dmaven.test.skip=false -DskipTests=false -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false clean test"is emitted with doubled quotes inside a delimited identifier.Risk and compatibility
Reviewer map
H2SqlEscapes.java(new helper) andH2SqlEscapesTest.java.H2MetaData/H2DBManager/H2SqlBuilder; the updated assertion inH2MetaResultSetLifecycleTest.Contributor declaration
AI assistance: The fix, verification, and PR description were produced with AI coding assistance.