ClickHouse / ClickHouse/clickhouse-cs

EncloseColumnName / QuoteSingle / QuoteDouble skip escaping when input starts and ends with the quote char

Open
#316 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.