[EFCore] Fix MySQL/MariaDB literal sanitization - #4986
Conversation
Sanitize SQL for MySQL and MariaDB when dollar-quoted literals and backslash-escaped single quotes are present.
github-actions
Bot
added
the
comp:instrumentation.entityframeworkcore
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4986 +/- ##
==========================================
+ Coverage 77.97% 78.11% +0.13%
==========================================
Files 477 484 +7
Lines 20395 20601 +206
==========================================
+ Hits 15904 16092 +188
- Misses 4491 4509 +18 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fix typo in comment.
There was a problem hiding this comment.
Pull request overview
Fixes SQL sanitization for EFCore instrumentation (and shared SQL processing) to prevent literal leakage for MySQL/MariaDB backslash-escaped quotes and PostgreSQL dollar-quoted string literals, while keeping the existing query-summary behavior and adding tests to lock in the new parsing behavior.
Changes:
- Added dialect-aware sanitization to handle MySQL/MariaDB backslash-escaped single quotes.
- Added sanitization support for PostgreSQL dollar-quoted string literals (
$tag$...$tag$/$$...$$). - Updated EFCore instrumentation to select the backslash-escape dialect for MySQL providers and added/expanded unit tests + changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/OpenTelemetry.Instrumentation.EntityFrameworkCore.Tests/EntityFrameworkDiagnosticListenerTests.cs | Adds coverage for detecting providers requiring backslash-escape parsing. |
| test/OpenTelemetry.Contrib.Shared.Tests/SqlProcessorTests.cs | Adds regression tests for backslash-escaped quotes and dollar-quoted string handling. |
| src/Shared/SqlProcessor.cs | Implements dialect-aware sanitization, including backslash-escaped quote handling and dollar-quoted literals. |
| src/Shared/DatabaseSemanticConventionHelper.cs | Plumbs dialect flag into sanitization call for query text attributes. |
| src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs | Detects MySQL providers and enables backslash-escape sanitization in EFCore query text tagging. |
| src/OpenTelemetry.Instrumentation.EntityFrameworkCore/CHANGELOG.md | Documents the sanitization fixes in the component changelog. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-25 20:52 UTC Review the latest changes. Status above doesn't look right?
|
Avoid false positive for dollar-quoted values.
| * 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)) |
There was a problem hiding this comment.
| ([#4985](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4985)) | |
| ([#4986](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/4986)) |
| var closeOffset = sql.Slice(bodyStart).IndexOf(delimiter); | ||
| if (closeOffset < 0) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
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.
| public static SqlStatementInfo GetSanitizedSql(string? sql, bool useBackslashEscapes = false) => | ||
| sql != null | ||
| ? useBackslashEscapes | ||
| ? GetSanitizedSql(sql, BackslashEscapeCache, ref approxBackslashEscapeCacheCount, useBackslashEscapes: true) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| /// <returns>The sanitized SQL and query summary.</returns> | ||
| public static SqlStatementInfo GetSanitizedSql(string? sql, bool useBackslashEscapes = false) => | ||
| sql != null | ||
| ? useBackslashEscapes |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
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);
}
| // 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); |
There was a problem hiding this comment.
Does anything actually detect if NO_BACKSLASH_ESCAPES is enabled or not and adjust the logic accordingly?
- Fix CHANGELOG PR number. - Sanitize unterminated literals. - Make ternary slightly more readable. - Add comment about `NO_BACKSLASH_ESCAPES`.
martincostello
added
the
keep-open
Fix entry location.
Fix MySQL/MariaDB double-quoted literal (valid syntax when `ANSI_QUOTES` is disabled) not being sanitized.
Changes
Sanitize SQL for MySQL and MariaDB when dollar-quoted literals, double-quoted literals and backslash-escaped single quotes are present.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)