ClickHouse / ClickHouse/clickhouse-cs
EncloseColumnName / QuoteSingle / QuoteDouble skip escaping when input starts and ends with the quote char
- Dominant language
- C#
- Stars
- 94
- Forks
- 22
- Avg merge
- 11h 26m
- Merged PRs (30d)
- 22
Description
## Description
`StringExtensions.EncloseColumnName`, `StringExtensions.QuoteSingle`, and `StringExtensions.QuoteDouble` in `ClickHouse.Driver/Utility/StringExtensions.cs` short-circuit and return the input unchanged whenever the first and last characters are the surrounding quote/backtick. They never inspect the interior of the string. Any embedded `` ` `` / `'` / `"` characters between those outer ones are therefore left unescaped, even though they will break (or, with malicious input, inject into) the resulting SQL.
```csharp
public static string QuoteSingle(this string str) =>
str.StartsWith("'", ...) && str.EndsWith("'", ...) ? str : $"'{str}'";
public static string QuoteDouble(this string str) =>
str.StartsWith("\"", ...) && str.EndsWith("\"", ...) ? str : $"\"{str}\"";
public static string EncloseColumnName(this string str)
{
...
if (str[0] == '`' && str[str.Length - 1] == '`')
return str; // Early return if already enclosed
...
}
```
`EncloseColumnName` is used by `SchemaResolver` to enclose user-supplied column names that are then interpolated into `SELECT … FROM table WHERE 1=0` probe queries (`SchemaResolver.cs:91`) and into the column list of `INSERT INTO …` queries via `BuildFromColumnTypes` (`SchemaResolver.cs:123`). It is also used for the database/table cache key (`SchemaResolver.cs:139-140`).
A column name such as `` `weird`name` `` (starts and ends with a backtick, contains a backtick in the middle) is returned unchanged. The resulting SQL has an unbalanced/unescaped identifier, which is at best an obscure parse error and at worst a SQL-injection vector if a caller forwards a column name from an untrusted source.
This mirrors clickhouse-connect issue [#737](https://github.com/ClickHouse/clickhouse-connect/issues/737), where the same shortcut exists in `quote_identifier`.
## ClickHouse server version
Code analysis only; not verified against a running server.
## Reproduction
```csharp
using ClickHouse.Driver.Utility;
using NUnit.Framework;
[TestFixture]
public class EncloseColumnNameTests
{
[Test]
public void EncloseColumnName_StartsAndEndsWithBacktickButHasInteriorBacktick_EscapesInterior()
{
var input = "`weird`name`";
var result = input.EncloseColumnName();
// Expected: interior backticks are escaped, e.g. "`\\`weird\\`name\\``"
// Actual: the input is returned unchanged because str[0] == '`' && str[^1] == '`'.
Assert.That(result, Is.Not.EqualTo(input));
}
[Test]
public void QuoteSingle_StartsAndEndsWithSingleQuoteButHasInteriorQuote_EscapesInterior()
{
var input = "'O'Reilly'";
var result = input.QuoteSingle();
// Actual: returned unchanged.
Assert.That(result, Is.Not.EqualTo(input));
}
}
```
Both assertions fail in the current code: the helpers return the input verbatim because the outer characters happen to match.
## Suggested fix
`StringExtensions.cs` should always escape interior occurrences of the quoting character before wrapping. The "already enclosed, leave alone" shortcut is unsafe — there is no way to distinguish "already-quoted, trusted" input from "untrusted input that happens to start and end with this character", so the helpers should treat the input as raw and consistently escape it. For `EncloseColumnName`, that means dropping the early-return at lines 29-30 and always running the `Replace` + wrap path. For `QuoteSingle`/`QuoteDouble`, drop the `StartsWith`/`EndsWith` guard and always escape the relevant character before wrapping.
## Link
Relayed from https://github.com/ClickHouse/clickhouse-connect/issues/737
Contributor guide
Assessment
This issue has not been assessed yet.