Skip to content

fix(mysql): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2172

Open
HandSonic wants to merge 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-mysql
Open

fix(mysql): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2172
HandSonic wants to merge 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-mysql

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Part of the SQL-injection hardening tracked in #1914 (multi-plugin effort; this PR covers MySQL). 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:

  • New MysqlSqlEscapes helper:
    • escapeSqlLiteral — doubles backslashes first, then single quotes (mirrors the existing MysqlAccountSqlBuilder.stringLiteral behavior for MySQL string literals).
    • quoteIdentifier/escapeIdentifier — strips one surrounding backtick pair and doubles every embedded backtick for quoted-identifier positions.
  • Canonicalizes three divergent identifier-quoting copies across the plugin into this one helper; applied across 10 files (see diff).
  • Positions where escaping is impossible by design are guarded by strict validation instead:
    • ENGINE / CHARSET / COLLATE via a requireMysqlName whitelist ([A-Za-z0-9_]+-style names).
    • DEFAULT via numeric/expression pattern validation; bit literals validated as [01]+; hex literals in generated DML (0x... template and binary/varbinary processors) validated as well-formed hex, closing a 0x-prefixed injection passthrough.
    • ENUM/SET values are escaped as literals.
  • MysqlIdentifierProcessor.quoteIdentifier doubles embedded backticks instead of wrapping verbatim.
  • Also fixes a missing dot separator in the dropView qualified name.
  • Added MysqlSqlEscapesTest covering backslash+quote literal doubling, backtick identifier doubling, malicious-name neutralization, and whitelist rejections.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Command: mvn -B -pl chat2db-community-plugins/chat2db-community-mysql -f chat2db-community-server/pom.xml -Dmaven.test.skip=false -DskipTests=false -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false clean test
  • Result: Tests run: 627, Failures: 0, Errors: 0, Skipped: 0 (full module suite, including the new MysqlSqlEscapesTest and all pre-existing tests).
  • Manual verification: escaping is identity for names without special characters; a name containing a backtick is emitted doubled inside a backtick-quoted identifier; invalid ENGINE/CHARSET/COLLATE/DEFAULT values are rejected with IllegalArgumentException.

Risk and compatibility

  • Public API or stored data: N/A — only changes generated SQL for metadata/DDL paths.
  • Database or driver compatibility: quoted output matches MySQL backtick identifier and backslash-aware literal rules; behavior for ordinary names is unchanged.
  • Network, privacy, or security: closes second-order SQL-injection paths in generated metadata/DDL SQL.
  • Community / Local / Pro boundary: N/A.
  • Backward compatibility: names containing backticks that previously produced broken/injectable SQL now produce correctly escaped SQL; invalid ENGINE/CHARSET/COLLATE/DEFAULT values are now rejected instead of emitted verbatim.

Reviewer map

  • Start here: MysqlSqlEscapes.java (new helper) and MysqlSqlEscapesTest.java.
  • Then: the three deduplicated quoting call chains; the requireMysqlName whitelist sites (ENGINE/CHARSET/COLLATE); DEFAULT and bit-literal validation in MysqlColumnTypeEnum; the dropView dot-separator fix.
  • Failure condition: a metadata/DDL SQL string still interpolates an unescaped name or literal.
  • Rollback or disable path: revert the commit on this branch.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: The fix, verification, and PR description were produced with AI coding assistance.

@HandSonic
HandSonic requested a review from openai0229 as a code owner July 26, 2026 09:29
@openai0229 openai0229 moved this to In Review in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic marked this pull request as draft July 26, 2026 09:47
@HandSonic

Copy link
Copy Markdown
Contributor Author

Converting to draft: final adversarial review pass is still in progress for this branch. Will mark ready for review once it completes. (process note: PR was opened prematurely by automation)

@openai0229 openai0229 moved this from In Review to In Progress in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic marked this pull request as ready for review July 26, 2026 10:28
@openai0229 openai0229 moved this from In Progress to In Review in Chat2DB Community Jul 26, 2026
…processor (OtterMind#1914)

- MysqlDmlValueTemplate.wrapHex: require hex digits for the 0x template,
  matching wrapBit's requireBitLiteral
- MysqlBinaryProcessor/MysqlVarBinaryProcessor: only pass values through
  raw when they are well-formed 0x hex literals (isHexLiteral), closing the
  0x-prefixed injection passthrough in generated DML
- MysqlIdentifierProcessor.quoteIdentifier: double embedded backticks via
  MysqlSqlEscapes.quoteIdentifierRaw instead of wrapping verbatim
- add regression tests for all three paths
…tainer review (OtterMind#1914)

- MysqlIdentifierProcessor (SPI ISQLIdentifierProcessor): INSTANCE, always-quote
  backtick quoteIdentifier with one-pair strip + doubling, escapeString with
  backslash-first-then-quote doubling, static escapeIdentifier for quoted templates
- MysqlMetaData call sites use getSQLIdentifierProcessor(); builders/managers/enums/
  value processors use MysqlIdentifierProcessor.INSTANCE
- non-escapable validation moved to MysqlSqlGuards (engine/charset/collation names,
  numeric defaults, bit/hex literals, definers, asc/desc, enum constants, enum/set lists)
- MysqlSqlEscapes removed; tests migrated to MysqlIdentifierProcessorTest (630 green)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants