Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion src/EFCore.Analyzers/AnalyzerReleases.Shipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,11 @@ EF1000 | Security | Disabled | RawSqlStringInjectionDiagnosticAnalyzer, [Docume
### New Rules
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
EF1002 | Security | Warning | InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer, [Documentation](https://learn.microsoft.com/ef/core/querying/sql-queries#passing-parameters)
EF1002 | Security | Warning | InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer, [Documentation](https://learn.microsoft.com/ef/core/querying/sql-queries#passing-parameters)

## Release 10.0.0

### New Rules
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
Copy link
Member

Choose a reason for hiding this comment

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

It feels like these should be the same diagnostic, after all they indicate the same thing on the same methods (risk of SQL injection etc.).

(regardless, I'd also lean towards implementing both in the same actual analyzer - they again analyze the same method invocations, FromSqlRaw and so on. But that's an internal implementation detail that the user doesn't care about, unlike the diagnostic ID).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it would be same diagnostic ID then it would have to be same analyzer. Although I don't have strong feeling about it (do I ever?) I have a preference into having two separate. Although both work on same methods, the analysis has different focus. And more importantly the code fix is only for the existing one - if we put it under one diagnostic ID, we'd need repeat the logic to know whether we're reporting for string interpolation problem (and allow code fix) or string concatenation (no code fix) and that feels like waste of CPU cycles.

Copy link
Member

Choose a reason for hiding this comment

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

If it would be same diagnostic ID then it would have to be same analyzer.

Why, is there some restriction about two analyzers emitting the same diagnostic ID?

The code fix is possibly a good argument... From a user perspective, the two warnings basically feel like variants of exactly the same thing (potential SQL injection due to piecing together strings with FromSqlRaw), and so the diagnostic should be the same. In other words, regardless of internal/implementation concerns, if I suppress the warning for FromSqlRaw (for interpolation) and then I "get it again" for another FromSqlRaw (from concatenation), that seems a bit bad.

I don't really see a CPU cycle/performance problem in repeating the check for the code fix - as far as I know, this would only happen once a diagnostic is actually emitted, which is an extremely cold event and not something we need to optimize for. Am I wrong?

Copy link
Contributor Author

@cincuranet cincuranet Sep 3, 2025

Choose a reason for hiding this comment

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

Why, is there some restriction about two analyzers emitting the same diagnostic ID?

Yes, RS1019 Diagnostic Id 'EF1002' is already used by analyzer ....

The code fix is possibly a good argument... From a user perspective, the two warnings basically feel like variants of exactly the same thing (potential SQL injection due to piecing together strings with FromSqlRaw), and so the diagnostic should be the same. In other words, regardless of internal/implementation concerns, if I suppress the warning for FromSqlRaw (for interpolation) and then I "get it again" for another FromSqlRaw (from concatenation), that seems a bit bad.

I don't know. For example, would you say IDE0018 Inline Variable Declaration and IDE0019 Inline as Type Check should be the same, because both are about inlining? (Maybe not a great example, but work with me.)

What if you want to disable the string concatenation warnings (because you "know what you're doing"), but you want to keep the interpolation warnings, just in case you make a mistake? More flexibility? Better?

Maybe @AndriySvyryd has some good point of view.

I don't really see a CPU cycle/performance problem in repeating the check for the code fix - as far as I know, this would only happen once a diagnostic is actually emitted, which is an extremely cold event and not something we need to optimize for. Am I wrong?

For code fix, this happens when the list of available code fixes is being populated - i.e. when user clicks on a 💡 (or does similar action).

Copy link
Member

Choose a reason for hiding this comment

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

Why, is there some restriction about two analyzers emitting the same diagnostic ID?

Yes, RS1019 Diagnostic Id 'EF1002' is already used by analyzer ....

Thanks, was not aware...

I don't know. For example, would you say IDE0018 Inline Variable Declaration and IDE0019 Inline as Type Check should be the same, because both are about inlining? (Maybe not a great example, but work with me.)

I understand the analogy you're trying to draw, but the above two examples are purely stylistic, and people/organizations have arbitrary choices on style (i.e. someone may want to style their code with inlined variable declarations but not with inlined type checks). The SQL injections warnings we're discussing here are about the actual (potentially dangerous) outcome of the code, i.e. the fact that SQL injection may result - regardless of the specific syntactic way of piecing together the string (concatenation vs. interpolation).

What if you want to disable the string concatenation warnings (because you "know what you're doing"), but you want to keep the interpolation warnings, just in case you make a mistake?

Isn't it weird for someone to globally disable string concat warnings because they know what they're doing, but then not also not what they're doing with interpolation?

String concatenation and interpolation are two alternative C# syntaxes to do the same thing - piece together a string from multiple components. The piecing together is the thing we're warning about here (because of the danger of SQL injection) - the specific syntactic mechanism doesn't seem relevant... In other words, I'm just not seeing a case where it would make sense to disable one but not the other.

For code fix, this happens when the list of available code fixes is being populated - i.e. when user clicks on a 💡 (or does similar action).

Right; it doesn't seem that an extra identification on the method at that point (when the user clicks) should be a perf concern. Do you disagree?

Maybe @AndriySvyryd has some good point of view.

Yep, let us know @AndriySvyryd.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe as a sort of mental exercise, imagine if we were building the entire thing from scratch today (i.e. a "SQL injection" but for both interpolation and concatenation), rather than just adding the missing concatenation warning; would you still think we'd do two separate warnings? Am asking to avoid us adding a new diagnostic only because we're adding interpolation rather than implementing it form the get-go.

Copy link
Member

@roji roji Sep 4, 2025

Choose a reason for hiding this comment

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

And one last point for thought...

What if you want to disable the string concatenation warnings (because you "know what you're doing"), but you want to keep the interpolation warnings, just in case you make a mistake? More flexibility? Better?

If you think we should keep separate diagnostics IDs to give the user more flexibility, then it's worth thinking why not also split FromSqlRaw, ExecuteSqlRaw, SqlQueryRaw etc. to different diagnostics IDs as well; in theory someone may say "I know what I'm doing" for one, but want warnings for the other.

But of course I don't think we should; just like we have a single diagnostic regardless of which method causes the possible SQL injection (FromSqlRaw, SqlQueryRaw...), I believe we should have the same diagnostic regardless of which C# syntax causes it (concatenation, interpolation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String concatenation and interpolation are two alternative C# syntaxes to do the same thing - piece together a string from multiple components. The piecing together is the thing we're warning about here (because of the danger of SQL injection) - the specific syntactic mechanism doesn't seem relevant... In other words, I'm just not seeing a case where it would make sense to disable one but not the other.

Now I see what's the difference in our views. When I do FromSqlRaw($"...") I see that as a mistake in method selection (instead of FromSql[Interpolated]), not that I want to build string SQL and it's a problem. When I do FromSqlRaw("..." + "...") that's a possible mistake.

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting... I can see what you mean...

Though it's possible that when a user does FromSqlRaw($"..."), they're not mistaken about the method selection, but really do want string interpolation because they need to e.g. parameterize a table name (something which FromSql[Interpolated] doesn't work for).

I agree it's a bit of a tricky question and not totally conclusive either way... I'm still focusing on what the warning is
"about", and in both cases it's possible SQL injection that may be happening; and the solution in both cases is to rewrite using FromSql[Interpolated] with parameters (if possible - otherwise if you really need to parameterize the table name you need to just suppress the warning). So if the problem and solution are generally the same, it feels like the same diagnostic.

Curious to hear what @AndriySvyryd thinks!

Copy link
Member

Choose a reason for hiding this comment

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

If possible, I'd prefer to have a single analyzer, to avoid overhead during analysis. I don't have a strong opinion on whether it should use different diagnostics, as I don't think it's common to use both interpolation and concatenation.

EF1003 | Security | Warning | StringConcatenationUsageInRawQueriesDiagnosticAnalyzer, [Documentation](https://learn.microsoft.com/ef/core/querying/sql-queries#passing-parameters)
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ private static bool AnalyzeFromSqlRawInvocation(IInvocationOperation invocation)
Debug.Assert(targetMethod.Name == "FromSqlRaw");

var compilation = invocation.SemanticModel!.Compilation;
var correctFromSqlRaw = FromSqlRawMethod(compilation);
var correctFromSqlRaw = SqlRawMethodsCompilationHelper.FromSqlRawMethod(compilation);

Debug.Assert(correctFromSqlRaw is not null, "Unable to find original `FromSqlRaw` method");

Expand All @@ -124,7 +124,7 @@ private static bool AnalyzeExecuteSqlRawInvocation(IInvocationOperation invocati

if (targetMethod.Name == "ExecuteSqlRaw")
{
var correctMethods = ExecuteSqlRawMethods(compilation);
var correctMethods = SqlRawMethodsCompilationHelper.ExecuteSqlRawMethods(compilation);

Debug.Assert(correctMethods.Any(), "Unable to find any `ExecuteSqlRaw` methods");

Expand All @@ -135,7 +135,7 @@ private static bool AnalyzeExecuteSqlRawInvocation(IInvocationOperation invocati
}
else
{
var correctMethods = ExecuteSqlRawAsyncMethods(compilation);
var correctMethods = SqlRawMethodsCompilationHelper.ExecuteSqlRawAsyncMethods(compilation);

Debug.Assert(correctMethods.Any(), "Unable to find any `ExecuteSqlRawAsync` methods");

Expand Down Expand Up @@ -164,7 +164,7 @@ private static bool AnalyzeSqlQueryRawInvocation(IInvocationOperation invocation

var compilation = invocation.SemanticModel!.Compilation;

var correctSqlQueryRaw = SqlQueryRawMethod(compilation);
var correctSqlQueryRaw = SqlRawMethodsCompilationHelper.SqlQueryRawMethod(compilation);

Debug.Assert(correctSqlQueryRaw is not null, "Unable to find original `SqlQueryRaw` method");

Expand Down Expand Up @@ -203,28 +203,4 @@ private static bool AnalyzeInterpolatedString(IInterpolatedStringOperation inter

return false;
}

private static IMethodSymbol? FromSqlRawMethod(Compilation compilation)
{
var type = compilation.RelationalQueryableExtensionsType();
return (IMethodSymbol?)type?.GetMembers("FromSqlRaw").FirstOrDefault(s => s is IMethodSymbol);
}

private static IEnumerable<IMethodSymbol> ExecuteSqlRawMethods(Compilation compilation)
{
var type = compilation.RelationalDatabaseFacadeExtensionsType();
return type?.GetMembers("ExecuteSqlRaw").Where(s => s is IMethodSymbol).Cast<IMethodSymbol>() ?? [];
}

private static IEnumerable<IMethodSymbol> ExecuteSqlRawAsyncMethods(Compilation compilation)
{
var type = compilation.RelationalDatabaseFacadeExtensionsType();
return type?.GetMembers("ExecuteSqlRawAsync").Where(s => s is IMethodSymbol).Cast<IMethodSymbol>() ?? [];
}

private static IMethodSymbol? SqlQueryRawMethod(Compilation compilation)
{
var type = compilation.RelationalDatabaseFacadeExtensionsType();
return (IMethodSymbol?)type?.GetMembers("SqlQueryRaw").FirstOrDefault(s => s is IMethodSymbol);
}
}
201 changes: 201 additions & 0 deletions src/EFCore.Analyzers/NullableAttributes.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Diagnostics.CodeAnalysis
{
#if !NETSTANDARD2_1
/// <summary>Specifies that null is allowed as an input even if the corresponding type disallows it.</summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class AllowNullAttribute : Attribute
{ }

/// <summary>Specifies that null is disallowed as an input even if the corresponding type allows it.</summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class DisallowNullAttribute : Attribute
{ }

/// <summary>Specifies that an output may be null even if the corresponding type disallows it.</summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class MaybeNullAttribute : Attribute
{ }

/// <summary>Specifies that an output will not be null even if the corresponding type allows it. Specifies that an input argument was not null when the call returns.</summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class NotNullAttribute : Attribute
{ }

/// <summary>Specifies that when a method returns <see cref="ReturnValue"/>, the parameter may be null even if the corresponding type disallows it.</summary>
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class MaybeNullWhenAttribute : Attribute
{
/// <summary>Initializes the attribute with the specified return value condition.</summary>
/// <param name="returnValue">
/// The return value condition. If the method returns this value, the associated parameter may be null.
/// </param>
public MaybeNullWhenAttribute(bool returnValue) => ReturnValue = returnValue;

/// <summary>Gets the return value condition.</summary>
public bool ReturnValue { get; }
}

/// <summary>Specifies that when a method returns <see cref="ReturnValue"/>, the parameter will not be null even if the corresponding type allows it.</summary>
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class NotNullWhenAttribute : Attribute
{
/// <summary>Initializes the attribute with the specified return value condition.</summary>
/// <param name="returnValue">
/// The return value condition. If the method returns this value, the associated parameter will not be null.
/// </param>
public NotNullWhenAttribute(bool returnValue) => ReturnValue = returnValue;

/// <summary>Gets the return value condition.</summary>
public bool ReturnValue { get; }
}

/// <summary>Specifies that the output will be non-null if the named parameter is non-null.</summary>
[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, AllowMultiple = true, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class NotNullIfNotNullAttribute : Attribute
{
/// <summary>Initializes the attribute with the associated parameter name.</summary>
/// <param name="parameterName">
/// The associated parameter name. The output will be non-null if the argument to the parameter specified is non-null.
/// </param>
public NotNullIfNotNullAttribute(string parameterName) => ParameterName = parameterName;

/// <summary>Gets the associated parameter name.</summary>
public string ParameterName { get; }
}

/// <summary>Applied to a method that will never return under any circumstance.</summary>
[AttributeUsage(AttributeTargets.Method, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class DoesNotReturnAttribute : Attribute
{ }

/// <summary>Specifies that the method will not return if the associated Boolean parameter is passed the specified value.</summary>
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class DoesNotReturnIfAttribute : Attribute
{
/// <summary>Initializes the attribute with the specified parameter value.</summary>
/// <param name="parameterValue">
/// The condition parameter value. Code after the method will be considered unreachable by diagnostics if the argument to
/// the associated parameter matches this value.
/// </param>
public DoesNotReturnIfAttribute(bool parameterValue) => ParameterValue = parameterValue;

/// <summary>Gets the condition parameter value.</summary>
public bool ParameterValue { get; }
}
#endif

/// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class MemberNotNullAttribute : Attribute
{
/// <summary>Initializes the attribute with a field or property member.</summary>
/// <param name="member">
/// The field or property member that is promised to be not-null.
/// </param>
public MemberNotNullAttribute(string member) => Members = [member];

/// <summary>Initializes the attribute with the list of field and property members.</summary>
/// <param name="members">
/// The list of field and property members that are promised to be not-null.
/// </param>
public MemberNotNullAttribute(params string[] members) => Members = members;

/// <summary>Gets field or property member names.</summary>
public string[] Members { get; }
}

/// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values when returning with the specified return value condition.</summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class MemberNotNullWhenAttribute : Attribute
{
/// <summary>Initializes the attribute with the specified return value condition and a field or property member.</summary>
/// <param name="returnValue">
/// The return value condition. If the method returns this value, the associated field or property member will not be null.
/// </param>
/// <param name="member">
/// The field or property member that is promised to be not-null.
/// </param>
public MemberNotNullWhenAttribute(bool returnValue, string member)
{
ReturnValue = returnValue;
Members = [member];
}

/// <summary>Initializes the attribute with the specified return value condition and list of field and property members.</summary>
/// <param name="returnValue">
/// The return value condition. If the method returns this value, the associated field and property members will not be null.
/// </param>
/// <param name="members">
/// The list of field and property members that are promised to be not-null.
/// </param>
public MemberNotNullWhenAttribute(bool returnValue, params string[] members)
{
ReturnValue = returnValue;
Members = members;
}

/// <summary>Gets the return value condition.</summary>
public bool ReturnValue { get; }

/// <summary>Gets field or property member names.</summary>
public string[] Members { get; }
}
}
46 changes: 32 additions & 14 deletions src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading