Skip to content

fix: quote SQL identifiers in migration generation - #9

Merged
rekurt merged 1 commit into
masterfrom
feat/propose-fix-for-sql-injection-vulnerability
Apr 7, 2026
Merged

rekurt merged 1 commit into
masterfrom
feat/propose-fix-for-sql-injection-vulnerability

Conversation

@rekurt

@rekurt rekurt commented Apr 7, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Prevent SQL injection in generated migration SQL by ensuring schema identifiers from the database are never interpolated verbatim into statements.
  • Provide backend-aware quoting/escaping so generated SQL is safe for PostgreSQL (double quotes) and MySQL (backticks).

Description

  • Added quote_ident(ident: &str, dialect: SqlDialect) and column_definition_sql(col: &Column, dialect: SqlDialect) helpers and used them everywhere migration SQL is constructed instead of raw identifier interpolation.
  • Modified create_table_sql and create_index_sql to accept dialect: SqlDialect and to produce quoted/escaped identifiers and quoted column definitions.
  • Updated all ALTER TABLE/DROP TABLE/CREATE TABLE/CREATE INDEX/DROP INDEX generation sites to call the quoting helpers and stopped using col.definition() directly in migration generation.
  • Updated unit tests to assert quoted identifiers and added a regression test (postgres_identifiers_are_quoted_and_escaped) that builds a schema with malicious quoted identifiers to ensure output is escaped and not injectable.

Testing

  • Ran cargo fmt --check which passed after the changes.
  • Attempted cargo test -q but it failed in this environment due to crates.io dependency download being blocked (HTTP 403), so tests could not be executed here.
  • Attempted cargo test -q --offline which also failed because some dependencies are not available in the local offline cache, so test execution was not completed in this environment.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8978872db8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/migration.rs
Comment on lines +157 to +160
.columns
.iter()
.map(|c| quote_ident(c, dialect))
.collect::<Vec<_>>()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid quoting full index clauses in CREATE INDEX

Quoting every entry in idx.columns breaks non-trivial index definitions because these strings are not always bare identifiers. The PostgreSQL loader extracts raw clauses from pg_indexes.indexdef (for example created_at DESC, lower(email), or already-quoted names), and wrapping each clause with quote_ident turns them into invalid or semantically different SQL ("created_at DESC", "lower(email)", etc.). This means migrations generated for expression/sorted indexes can no longer be applied.

Useful? React with 👍 / 👎.

rekurt added a commit that referenced this pull request Apr 7, 2026
Incorporate MySQL backtick quoting and SQL injection escape test from
feat/propose-fix-for-sql-injection-vulnerability. Resolve conflicts
keeping our approach: index column clauses are never re-quoted,
simple identifiers pass through unquoted for readability.
@rekurt
rekurt merged commit f00c8f1 into master Apr 7, 2026
@rekurt
rekurt deleted the feat/propose-fix-for-sql-injection-vulnerability branch April 7, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant