Skip to content

[EFCore] Fix MySQL/MariaDB literal sanitization #4986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martincostello wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[EFCore] Fix MySQL/MariaDB literal sanitization #4986

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
63262fd
[EFCore] Fix MySQL/MariaDB literal sanitization
martincostello Aug 17, 2026
5f77484
[EFCore] Fix typo
martincostello Aug 17, 2026
d1f4ff8
[EFCore] Address feedback
martincostello Aug 17, 2026
4d59d56
Merge branch 'main' into sanitise-mysql-literals
martincostello Aug 19, 2026
c2c37e2
[EFCore] Address feedback
martincostello Aug 19, 2026
7575ba8
Merge branch 'main' into sanitise-mysql-literals
martincostello Aug 22, 2026
09f48fd
[EFCore] Update CHANGELOG
martincostello Aug 22, 2026
8b0ded7
[EFCore] Sanitize double-quoted literals
martincostello Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Instrumentation.EntityFrameworkCore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
`db.query.summary` length limit is reached.
([#4929](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4929))

* Fixed query sanitization so that backslash-escaped quotes (`'a\'b'`) in
MySQL/MariaDB string literals and PostgreSQL dollar-quoted strings
(`$$...$$`) are correctly redacted.
([#4985](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4985))

@stevejgordon stevejgordon Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
([#4985](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4985))
([#4986](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4986))


## 1.17.0-beta.1

Released 2026-Jul-17
Expand Down
18 changes: 17 additions & 1 deletion ...y.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,18 @@ public override void OnEventWritten(string name, object? payload)
// able to sanitize arbitrary commands for other query dialects.
var sanitizeQuery = IsSqlLikeProvider(providerName);

// MySQL/MariaDB treat a backslash as a string-literal escape
// character by default, which the sanitizer must honor to avoid
// leaking literal content into the query text.
var useBackslashEscapes = IsBackslashEscapeProvider(providerName);

DatabaseSemanticConventionHelper.ApplyConventionsForQueryText(
activity,
commandText,
this.options.EmitOldAttributes,
this.options.EmitNewAttributes,
sanitizeQuery);
sanitizeQuery,
useBackslashEscapes);
break;

case CommandType.TableDirect:
Expand Down Expand Up @@ -369,6 +375,16 @@ DbSystemNames.Sqlite or
};
}

internal static bool IsBackslashEscapeProvider(string? providerOrCommandName)
{
// MySQL and MariaDB (which use the MySQL providers) treat a backslash as a
// string-literal escape character unless the NO_BACKSLASH_ESCAPES SQL mode
// is enabled. The other supported engines follow the SQL standard where
// only a doubled quote ('') escapes a quote.
(_, var dbSystemName) = GetDbSystemNames(providerOrCommandName);

@stevejgordon stevejgordon Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does anything actually detect if NO_BACKSLASH_ESCAPES is enabled or not and adjust the logic accordingly?

@martincostello martincostello Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No.

return dbSystemName == DbSystemNames.Mysql;
}

private void AddTag(Activity activity, (string Old, string New) attributes, string? value)
=> this.AddTag(activity, attributes, (value, value));

Expand Down
5 changes: 3 additions & 2 deletions src/Shared/DatabaseSemanticConventionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,15 @@ public static void ApplyConventionsForQueryText(
string? commandText,
bool emitOldAttributes,
bool emitNewAttributes,
bool sanitizeQuery = true)
bool sanitizeQuery = true,
bool useBackslashEscapes = false)
{
var queryText = commandText ?? string.Empty;
var querySummary = string.Empty;

if (sanitizeQuery)
{
var sqlStatementInfo = SqlProcessor.GetSanitizedSql(commandText);
var sqlStatementInfo = SqlProcessor.GetSanitizedSql(commandText, useBackslashEscapes);

queryText = sqlStatementInfo.SanitizedSql;
querySummary = sqlStatementInfo.DbQuerySummary;
Expand Down
138 changes: 121 additions & 17 deletions src/Shared/SqlProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
private const char DashChar = '-';
private const char ForwardSlashChar = '/';
private const char SingleQuoteChar = '\'';
private const char BackslashChar = '\\';
private const char DollarChar = '$';
private const char AsteriskChar = '*';
private const char UnderscoreChar = '_';
private const char DotChar = '.';
Expand All @@ -31,6 +33,7 @@
private const char UnicodePrefixChar = 'N';

private static readonly ConcurrentDictionary<string, SqlStatementInfo> Cache = new();
private static readonly ConcurrentDictionary<string, SqlStatementInfo> BackslashEscapeCache = new();

private static readonly char[] WhitespaceChars = [SpaceChar, TabChar, CarriageReturnChar, NewLineChar];
#if !NET
Expand Down Expand Up @@ -93,6 +96,7 @@
// We only increment on successful TryAdd. This may result in a slightly oversized cache
// under high concurrency but this is acceptable for this scenario.
private static int approxCacheCount;
private static int approxBackslashEscapeCacheCount;

private enum SqlKeyword
{
Expand Down Expand Up @@ -143,35 +147,52 @@
View,
}

public static SqlStatementInfo GetSanitizedSql(string? sql)
/// <summary>
/// Sanitizes a SQL statement by replacing its literal values with placeholders and computes the
/// corresponding <c>db.query.summary</c>. Results are cached per statement and dialect.
/// </summary>
/// <param name="sql">The SQL statement to sanitize.</param>
/// <param name="useBackslashEscapes">
/// <see langword="true"/> if the source database treats a backslash as a string-literal escape
/// character (MySQL and MariaDB with the default <c>NO_BACKSLASH_ESCAPES</c> mode disabled);
/// otherwise <see langword="false"/>.
/// </param>
/// <returns>The sanitized SQL and query summary.</returns>
public static SqlStatementInfo GetSanitizedSql(string? sql, bool useBackslashEscapes = false) =>
sql != null
? useBackslashEscapes

@stevejgordon stevejgordon Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code is a little hard to read now with nested ternaries. Would an if/else be clearer? This may depend on the decision on cache count sharing.

@martincostello martincostello Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It ended up like this because the code I started with had "use expression bodied member", then once that was applied it had "simplify if statement", then that had "use ternary". I just kept accepting until the IDE stopped suggesting refactorings. I figured that was easier than having to suppress it to keep the simple if.

@martincostello martincostello Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

public static SqlStatementInfo GetSanitizedSql(string? sql, bool useBackslashEscapes = false)
{
    if (sql == null)
    {
        return default;
    }

    return useBackslashEscapes
        ? GetSanitizedSql(sql, BackslashEscapeCache, ref approxBackslashEscapeCacheCount, useBackslashEscapes: true)
        : GetSanitizedSql(sql, Cache, ref approxCacheCount, useBackslashEscapes: false);
}
image

then:

public static SqlStatementInfo GetSanitizedSql(string? sql, bool useBackslashEscapes = false)
{
    return sql == null
        ? default
        : useBackslashEscapes
        ? GetSanitizedSql(sql, BackslashEscapeCache, ref approxBackslashEscapeCacheCount, useBackslashEscapes: true)
        : GetSanitizedSql(sql, Cache, ref approxCacheCount, useBackslashEscapes: false);
}
image

? GetSanitizedSql(sql, BackslashEscapeCache, ref approxBackslashEscapeCacheCount, useBackslashEscapes: true)

@stevejgordon stevejgordon Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If an application uses more tha one DB technology, one using backslash escapes and one not, I think we could end up with two caches up to 1000 entries each. Arguably, that may be okay, but we might need to document that, or maintain a shared count across both caches?

@martincostello martincostello Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure that's needed - if you have a multi-engine application and do enough SQL with it for there to be an internal cache with 2000 entries, that's probably neither here nor there in the grander scheme?

: GetSanitizedSql(sql, Cache, ref approxCacheCount, useBackslashEscapes: false)
: default;

private static SqlStatementInfo GetSanitizedSql(
string sql,
ConcurrentDictionary<string, SqlStatementInfo> cache,
ref int approxCount,
bool useBackslashEscapes)
{
if (sql == null)
{
return default;
}

if (Cache.TryGetValue(sql, out var sqlStatementInfo))
if (cache.TryGetValue(sql, out var sqlStatementInfo))
{
return sqlStatementInfo;
}

sqlStatementInfo = SanitizeSql(sql);
sqlStatementInfo = SanitizeSql(sql, useBackslashEscapes);

// Fast-path capacity check using our own approximate count to avoid ConcurrentDictionary.Count cost.
if (Volatile.Read(ref approxCacheCount) >= CacheCapacity)
if (Volatile.Read(ref approxCount) >= CacheCapacity)
{
return sqlStatementInfo;
}

// Attempt to add when under capacity. Increment our count only on successful add.
if (Cache.TryAdd(sql, sqlStatementInfo))
if (cache.TryAdd(sql, sqlStatementInfo))
{
Interlocked.Increment(ref approxCacheCount);
Interlocked.Increment(ref approxCount);
return sqlStatementInfo;
}

// If another thread added meanwhile, return the cached value if available.
return Cache.TryGetValue(sql, out var existing) ? existing : sqlStatementInfo;
return cache.TryGetValue(sql, out var existing) ? existing : sqlStatementInfo;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down Expand Up @@ -239,7 +260,7 @@
return false;
}

private static SqlStatementInfo SanitizeSql(string sql)
private static SqlStatementInfo SanitizeSql(string sql, bool useBackslashEscapes)
{
var sqlSpan = sql.AsSpan();

Expand All @@ -252,6 +273,7 @@
var buffer = rentedBuffer.AsSpan();

ParseState state = default;
state.UseBackslashEscapes = useBackslashEscapes;

// Precompute the summary buffer slice once and carry it via state to avoid repeated Span.Slice calls.
state.SummaryBuffer = buffer.Slice(rentedBuffer.Length / 2);
Expand All @@ -264,6 +286,7 @@
}

if (SanitizeStringLiteral(sqlSpan, buffer, ref state) ||
SanitizeDollarQuotedLiteral(sqlSpan, buffer, ref state) ||
SanitizeHexLiteral(sqlSpan, buffer, ref state) ||
SanitizeNumericLiteral(sqlSpan, buffer, ref state))
{
Expand Down Expand Up @@ -756,6 +779,7 @@
// If so, we want to skip the Unicode prefix when sanitizing.
var isUnicode = state.ParsePosition >= 1 && sql[state.ParsePosition - 1] is UnicodePrefixChar;

var literalStart = state.ParsePosition;
var searchPos = state.ParsePosition + 1;
while (searchPos < sql.Length)
{
Expand All @@ -766,6 +790,21 @@
}

searchPos += quoteIndex;

// Skip a backslash-escaped quote (\'). MySQL/MariaDB (with the default
// NO_BACKSLASH_ESCAPES disabled) treat a backslash as a string escape
// character, so a quote preceded by an odd number of backslashes does
// not terminate the literal. Without this a value such as 'a\'secret'
// would be mis-parsed and the trailing "secret" copied into the sanitized

Check warning on line 798 in src/Shared/SqlProcessor.cs

View workflow job for this annotation

GitHub Actions / lint-misspell-sanitycheck / Check for typos 

"mis" should be "miss" or "mist".
Comment thread
martincostello marked this conversation as resolved.
Outdated
// SQL verbatim. This is gated on the dialect because '\' is not an escape
// in the other engines, where treating it as one would instead cause
// a '' -escaped literal to be incorrectly parsed.
if (state.UseBackslashEscapes && IsBackslashEscaped(sql, searchPos, literalStart))
{
searchPos += 1;
continue;
}

if (searchPos + 1 < sql.Length && sql[searchPos + 1] == SingleQuoteChar)
{
// Skip escaped quote ('')
Expand Down Expand Up @@ -793,6 +832,57 @@
return false;
}

private static bool IsBackslashEscaped(ReadOnlySpan<char> sql, int quoteIndex, int literalStart)
{
var backslashes = 0;
for (var i = quoteIndex - 1; i > literalStart && sql[i] == BackslashChar; i--)
{
backslashes++;
}

return (backslashes & 1) == 1;
}

private static bool SanitizeDollarQuotedLiteral(ReadOnlySpan<char> sql, Span<char> buffer, ref ParseState state)
{
// PostgreSQL dollar-quoted string: $tag$...$tag$ (the tag is optional, so $$...$$ is valid).
// The body between the delimiters is a literal with no escaping, so it must be redacted.
// This syntax is unambiguous across the SQL dialects handled here, so it is safe to apply.
var start = state.ParsePosition;
if (sql[start] != DollarChar)
{
return false;
}

// Parse the opening delimiter: a dollar sign, an optional tag, then a closing dollar sign.
var tagEnd = start + 1;
while (tagEnd < sql.Length && IsDollarQuoteTagChar(sql[tagEnd]))
{
tagEnd++;
}
Comment thread
martincostello marked this conversation as resolved.

if (tagEnd >= sql.Length || sql[tagEnd] != DollarChar)
{
return false;
}

var delimiter = sql.Slice(start, tagEnd - start + 1);
var bodyStart = tagEnd + 1;

var closeOffset = sql.Slice(bodyStart).IndexOf(delimiter);
if (closeOffset < 0)
{
return false;

@stevejgordon stevejgordon Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Returning false here leaves state.ParsePosition unchanged, so the $ character falls through to ParseNextToken and is emitted verbatim. The body of the unterminated literal then flows through subsequent iterations as plain identifier/keyword tokens and is not redacted.

SanitizeStringLiteral handles its equivalent unterminated case by advancing state.ParsePosition to sql.Length and emitting ?, which keeps the security guarantee consistent. Worth doing the same here I think.

}

state.ParsePosition = bodyStart + closeOffset + delimiter.Length;
buffer[state.SanitizedPosition++] = SanitizationPlaceholder;
return true;

static bool IsDollarQuoteTagChar(char c)
=> char.IsAsciiLetterOrDigit(c) || c == UnderscoreChar;
}

private static bool SanitizeHexLiteral(ReadOnlySpan<char> sql, Span<char> buffer, ref ParseState state)
{
var i = state.ParsePosition;
Expand Down Expand Up @@ -922,7 +1012,7 @@
// "IN ('a)b', 'secret')"), which would leave the parser positioned in the middle of
// that literal. Every subsequent quote would then be mismatched and the remaining
// values would be copied into the sanitized SQL verbatim instead of being replaced.
if (TryFindEndOfInClause(sql, parsePosition, out var closeParenIndex))
if (TryFindEndOfInClause(sql, parsePosition, state.UseBackslashEscapes, out var closeParenIndex))
{
state.ParsePosition = closeParenIndex;
buffer[state.SanitizedPosition++] = SanitizationPlaceholder;
Expand All @@ -943,7 +1033,7 @@
/// <see langword="false"/> if the clause is not terminated, in which case the caller
/// falls back to sanitizing each value individually.
/// </returns>
private static bool TryFindEndOfInClause(ReadOnlySpan<char> sql, int start, out int closeParenIndex)
private static bool TryFindEndOfInClause(ReadOnlySpan<char> sql, int start, bool useBackslashEscapes, out int closeParenIndex)
{
var length = sql.Length;
var i = start;
Expand All @@ -970,7 +1060,7 @@
return true;

case SingleQuoteChar:
i = SkipStringLiteral(sql, i);
i = SkipStringLiteral(sql, i, useBackslashEscapes);
break;

case DashChar:
Expand All @@ -993,7 +1083,7 @@

// Returns the index after the closing quote, or the end of the input if the
// literal is not terminated.
static int SkipStringLiteral(ReadOnlySpan<char> sql, int quotePosition)
static int SkipStringLiteral(ReadOnlySpan<char> sql, int quotePosition, bool useBackslashEscapes)
{
var length = sql.Length;
var i = quotePosition + 1;
Expand All @@ -1008,6 +1098,14 @@

i += quoteIndex;

// A backslash-escaped quote (\') does not terminate the literal in dialects that use
// backslash escapes. See the note in SanitizeStringLiteral.
if (useBackslashEscapes && IsBackslashEscaped(sql, i, quotePosition))
{
i += 1;
continue;
}

// A doubled quote ('') is an escaped quote within the literal.
if (i + 1 < length && sql[i + 1] == SingleQuoteChar)
{
Expand Down Expand Up @@ -1100,6 +1198,12 @@

public bool SanitizeNextNonKeywordToken; // 1 byte

/// <summary>
/// Whether the source dialect treats a backslash as a string-literal escape character
/// (MySQL/MariaDB). Controls whether <c>\'</c> is recognized as an escaped quote.
/// </summary>
public bool UseBackslashEscapes; // 1 byte

/// <summary>
/// Used to track if we are in an escaped identifier (e.g., "[table]").
/// </summary>
Expand Down
Loading
Loading