Redact constants that get inlined from variables into query in log #35724

cincuranet · 2025-03-04T11:16:08Z

This adds OriginallyParameter into SqlConstantExpression to detect a case when something that was originally a parameter in query was turned into inline value. This value should be then handled the same way as we do for parameters (regarding EnableSensitiveDataLogging).

The SQL generator is then generating two versions of query. The first is the standard one that will be sent to database and other is with inlined values redacted. Later, if sensitive logging is off, diagnostic logger will use it instead of regular DbCommand.CommandText.

PR Overview

This PR introduces a new mechanism to mark constant SQL expressions that were originally parameters so that, when inlined into a query, they are handled like parameters with respect to sensitive data logging. Key changes include:

Adding an extra flag (“originallyParameter”) to the SqlConstantExpression and propagating it through various factory methods and visitors.
Updating multiple components (SQL generators, expression visitors, and diagnostic loggers) to use the new flag and to pass along a new “logCommandText” parameter for redacted logging.
Revising XML documentation and method signatures across the EF Core Relational components for consistency.

Reviewed Changes

File	Description
src/EFCore.Relational/Query/SqlExpressions/SqlConstantExpression.cs	Adds the “originallyParameter” flag to the constructors and updates ApplyTypeMapping/Quote.
src/EFCore.Relational/Query/QuerySqlGenerator.cs	Updates SQL generation logic to pass the flag when composing the command text.
src/EFCore.Relational/Query/ISqlExpressionFactory.cs	Modifies Constant method overloads to include the new parameter with a default value.
src/EFCore.Relational/Diagnostics/*.cs	Introduces a new “logCommandText” parameter and revises log formatting in diagnostic events.
src/EFCore.Relational/Query/SqlNullabilityProcessor.cs	Revises calls to Constant to include the originallyParameter flag (often set to true).
(Other files across the query translation and diagnostics areas)	Propagates the new flag through SQL expression visitors and translators.

Copilot reviewed 57 out of 57 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs:131

[nitpick] Consider adding unit tests to verify that inline constant expressions flagged with 'originallyParameter: true' are correctly redacted when sensitive data logging is disabled.

processedValues.Add(_sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), originallyParameter: true, typeMapping: typeMapping));

src/EFCore.Relational/Diagnostics/Internal/RelationalCommandDiagnosticsLogger.cs:320

[nitpick] Consider adding tests to verify that the new 'logCommandText' parameter is used appropriately in scenarios where sensitive data logging is disabled, ensuring the redacted command text is correctly applied.

command.CommandText.TrimEnd());

roji

Thanks, looks pretty good - see comments below.

Also, don't we need test coverage for this? Or did I miss it?

roji · 2025-03-07T12:38:53Z

src/EFCore.Relational/Diagnostics/CommandEventData.cs

    /// <param name="startTime">The start time of this event.</param>
    /// <param name="commandSource">Source of the command.</param>
    public CommandEventData(
        EventDefinitionBase eventDefinition,
        Func<EventDefinitionBase, EventData, string> messageGenerator,
        DbConnection connection,
        DbCommand command,
+        string? logCommandText,


A bit nitty, but another option instead of passing in a nullable text, is to simply always pass in a redactedCommandText parameter - if there are no redacted parameters, it's the same as DbCommand.CommandText. This would both makes the meaning of this parameter clear, and also remove the need for the assumption that if logSensitiveValues is true, this is non-null (i.e. no more bang (!) when accessing it)

roji · 2025-03-07T12:42:11Z

src/EFCore.Relational/Diagnostics/Internal/RelationalCommandDiagnosticsLogger.cs

@@ -219,6 +219,7 @@ private CommandEndEventData BroadcastCommandCreated(
            CommandCreated,
            connection,
            command,
+            command.CommandText,


If we keep this parameter as nullable, shouldn't it be null here (because below we pass false)? Though as above, I do believe we should just always pass it in and make the parameter non-nullable.

roji · 2025-03-07T12:43:15Z

src/EFCore.Relational/Diagnostics/Internal/RelationalCommandDiagnosticsLogger.cs

@@ -431,11 +438,13 @@ public virtual InterceptionResult<object> CommandScalarExecuting(

            definition.Log(
                this,
-                command.Parameters.FormatParameters(ShouldLogParameterValues(command)),
+                command.Parameters.FormatParameters(ShouldLogSensitiveData()),


BTW I'm not sure a rename of ShouldLogParameterValues to ShouldLogSensitiveData is actually necessary - this still controls whether parameter data is logged or not (it's just that now it also covers inlined parameters). But no strong feelings if we want to make this rename.

roji · 2025-03-07T12:46:04Z

src/EFCore.Relational/Query/ISqlExpressionFactory.cs

    /// <param name="typeMapping">The <see cref="RelationalTypeMapping" /> associated with the expression.</param>
    /// <returns>An expression representing a constant in a SQL tree.</returns>
-    SqlExpression Constant(object value, RelationalTypeMapping? typeMapping = null);
+    SqlExpression Constant(object value, bool originallyParameter = false, RelationalTypeMapping? typeMapping = null);


Suggest renaming to isSensitive to convey that the constant contains sensitive information (to be redacted) rather than where it came from (this also aligns with the naming of LogSensitiveValues)... In theory we may have other things that become sensitive at some point without having been a parameter originally?

Suggested change

SqlExpression Constant(object value, bool originallyParameter = false, RelationalTypeMapping? typeMapping = null);

SqlExpression Constant(object value, bool isSensitive = false, RelationalTypeMapping? typeMapping = null);

roji · 2025-03-07T12:49:52Z

src/EFCore.Relational/Query/ISqlExpressionFactory.cs

    /// <param name="typeMapping">The <see cref="RelationalTypeMapping" /> associated with the expression.</param>
    /// <returns>An expression representing a constant in a SQL tree.</returns>
-    SqlExpression Constant(object value, RelationalTypeMapping? typeMapping = null);
+    SqlExpression Constant(object value, bool originallyParameter = false, RelationalTypeMapping? typeMapping = null);


Also, split to a different method, i.e. keep the current overload and add another one that accepts the new required flag. Adding the parameter to the existing signature breaks existing code, and also forces everyone to explicitly specify typeMapping: when they don't want to pass in originallyParameter - which is the 99% case. Once you do that, you can revert all the additions of typeMapping: in this PR.

roji · 2025-03-07T12:55:34Z

src/EFCore.Relational/Query/SqlExpressions/SqlConstantExpression.cs

+    /// <summary>
+    ///    Whether the expression was originally a parameter.
+    /// </summary>
+    public virtual bool OriginallyParameter { get; }


IsSensitive?

roji · 2025-03-07T12:58:59Z

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

@@ -765,7 +765,7 @@ InExpression ProcessInExpressionValues(
                        continue;
                    }

-                    processedValues.Add(_sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), typeMapping));
+                    processedValues.Add(_sqlExpressionFactory.Constant(value, value?.GetType() ?? typeof(object), originallyParameter: true, typeMapping: typeMapping));


BTW I'm still slightly undecided about what to do when the user requests inlining via EF.Constant() (in VisitSqlParameter in this visitor)... I think we talked about this briefly, not sure what we said. Part of me does feel like a parameter is a parameter, and the fact that the user requests inlining it doesn't make it less sensitive (and should therefore be redacted) - but I'm really unsure.

Maybe bring to a quick design discussion?

roji · 2025-03-07T13:00:09Z

src/EFCore.Relational/Storage/IRelationalCommandTemplate.cs

+    /// <summary>
+    ///     Gets the command text to be logged.
+    /// </summary>
+    string? LogCommandText { get; }


RedactedCommandText (also elsewhere)?

roji · 2025-03-07T13:02:55Z

src/EFCore.Relational/Storage/RelationalCommandBuilder.cs

+
+        if (!Dependencies.LoggingOptions.IsSensitiveDataLoggingEnabled)
+        {
+            _logCommandTextBuilder = new();


This means that we always generate the two strings once IsSensitiveDataLoggingEnabled is enabled, even when there are no sensitive values. Can we instead lazily initialize this the first time that Append is called with an actual value that needs to be redacted?

roji · 2025-03-07T13:04:16Z

src/EFCore.Relational/Storage/RelationalCommandBuilder.cs

@@ -66,17 +74,19 @@ public virtual IRelationalCommandBuilder RemoveParameterAt(int index)
    }

    /// <inheritdoc />
-    public virtual IRelationalCommandBuilder Append(string value)
+    public virtual IRelationalCommandBuilder Append(string value, string? logValue = null)


Maybe just pass in a boolean isSensitive, or redact? This would centralize the redacted representation (question mark) here, rather than force all callers to duplicate it - unless you can see a case where we'd need to pass something other than a question mark?

cincuranet added 11 commits February 20, 2025 11:38

Initial pass

ddbc022

Progress.

b4fc112

Progress.

e5ee577

Constants

0e4b840

Fix constant expression quote.

9e7984c

Properly quote RelationalCommand.

5cec734

Fix tests.

e034af5

More tests.

816aa80

Fix new lines.

a318a89

Allocated builder only when needed.

4aadf6f

Fix constant.

6eff338

cincuranet marked this pull request as ready for review March 7, 2025 11:37

cincuranet requested a review from a team as a code owner March 7, 2025 11:37

cincuranet requested a review from Copilot March 7, 2025 11:37

Copilot AI reviewed Mar 7, 2025

View reviewed changes

roji requested changes Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redact constants that get inlined from variables into query in log #35724

Redact constants that get inlined from variables into query in log #35724

cincuranet commented Mar 4, 2025 •

edited

Loading

roji left a comment

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

roji Mar 7, 2025

	SqlExpression Constant(object value, bool originallyParameter = false, RelationalTypeMapping? typeMapping = null);
	SqlExpression Constant(object value, bool isSensitive = false, RelationalTypeMapping? typeMapping = null);

Redact constants that get inlined from variables into query in log #35724

Are you sure you want to change the base?

Redact constants that get inlined from variables into query in log #35724

Conversation

cincuranet commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cincuranet commented Mar 4, 2025 •

edited

Loading