[osbre]

Escape quote characters in DatabaseDialect::quoteIdentifier() to prevent potential SQL injection when identifiers come from untrusted input.

The method wraps an identifier in quote characters, but previously did not escape quote characters already inside it. This meant an identifier containing a quote could end its own quoting, causing the rest of the value to be treated as SQL instead of part of the identifier:

$dialect->quoteIdentifier('ti"tle');

// before: "ti"tle"
// after:  "ti""tle"

Doubling the quote character is the escaping format expected by MySQL/SQLite and PostgreSQL. This also matches how other frameworks handle identifier quoting, including Laravel's Illuminate\Database\Grammar::wrapValue and Doctrine DBAL's AbstractPlatform::quoteSingleIdentifier.

Handling this at the dialect level means all call sites - relations, table aliases, migration statements, etc. - get the same protection without having to sanitise identifiers individually.

Testing

Added unit test at packages/database/tests/Config/DatabaseDialectTest.php.

[github-actions]

Benchmark Results

Comparison of fix/escape-sql-identifiers against 3.x (e16e5a4844f0371d61c077008155dd1cc222baf2).

Open to see the benchmark results

No benchmark changes above ±5%.

_{Generated by phpbench against commit 1e68f29}

[xHeaven]

Hey, I think this needs a more thorough refactor, as some paths still quote things without calling the fixed method, such as HasTableAlias.php:45 and FieldStatement.php:40.

[osbre]

@xHeaven wouldn't that be a separate follow-up PR? to keep the changes small and focused

[xHeaven]

@xHeaven wouldn't that be a separate follow-up PR? to keep the changes small and focused

Good question, I'm not sure what'd be the best here. Those changes are closely related (essentially the same thing) to this one, I would personally keep them in a single PR. @brendt what's your call here?

[brendt]

Keep them in the same PR :)

[github-actions]

This pull request is stale because it has been open for 30 days with no activity.

[github-actions]

This pull request was closed because it has been inactive for 1 day since being marked as stale.

[brendt]

Is this ready to merge?

[xHeaven]

Note

This message is generated by an AI agent on behalf of @xHeaven.

I think the earlier concern still stands. The helper change looks correct, but HasTableAlias::quoteIdentifier() still wraps identifiers without escaping them, and FieldStatement::compile() still returns fields unquoted for SQLite. We agreed to include those fixes here.

There’s also an issue in Query::compile(): for PostgreSQL it replaces every backtick with a double quote after identifiers have been escaped. For example, an order-by identifier containing a backtick compiles like this:

-- Before Query::compile():
"name` DESC --" ASC

-- After:
"name" DESC --" ASC

These issues predate this PR, but they leave the escaping fix incomplete. Could we cover those paths too, with tests checking the final compiled SQL?

[osbre]

Yeah unfortunately it goes much deeper than this simple change. We can check how Eloquent and Doctrine solve this to get better ideas.

[xHeaven]

Yeah unfortunately it goes much deeper than this simple change. We can check how Eloquent and Doctrine solve this to get better ideas.

Let's do that, I recommend broadening this PR if needed, rather than solving just part of the issue. Do you want to work on this (I presume it'll be a bit more substantial change) or should we take over here?

[osbre]

@xHeaven Feel free to take over 👍 I no longer have the capacity to work on it

[osbre]

Last time I checked it involved refactoring more stuff to pass DatabaseDialect parameter to properly escape identifiers, or inventing a new value object that can be passed around