From 6eca9d2d767ba710c384ca664b518d9c53af2a2a Mon Sep 17 00:00:00 2001 From: Jiri Cincura Date: Tue, 2 Sep 2025 19:38:48 +0200 Subject: [PATCH] Analyzer for string concatenation in raw SQL methods. --- .../AnalyzerReleases.Shipped.md | 9 +- ...ringUsageInRawQueriesDiagnosticAnalyzer.cs | 32 +- src/EFCore.Analyzers/NullableAttributes.cs | 201 +++++++ .../Properties/AnalyzerStrings.Designer.cs | 46 +- .../Properties/AnalyzerStrings.resx | 115 +++- .../SqlRawMethodsCompilationHelper.cs | 33 ++ ...tionUsageInRawQueriesDiagnosticAnalyzer.cs | 213 ++++++++ src/Shared/EFDiagnostics.cs | 1 + ...ringConcatenationUsageInRawQueriesTests.cs | 494 ++++++++++++++++++ 9 files changed, 1096 insertions(+), 48 deletions(-) create mode 100644 src/EFCore.Analyzers/NullableAttributes.cs create mode 100644 src/EFCore.Analyzers/SqlRawMethodsCompilationHelper.cs create mode 100644 src/EFCore.Analyzers/StringConcatenationUsageInRawQueriesDiagnosticAnalyzer.cs create mode 100644 test/EFCore.Analyzers.Tests/StringConcatenationUsageInRawQueriesTests.cs diff --git a/src/EFCore.Analyzers/AnalyzerReleases.Shipped.md b/src/EFCore.Analyzers/AnalyzerReleases.Shipped.md index ed10f609a61..0ea4833e46c 100644 --- a/src/EFCore.Analyzers/AnalyzerReleases.Shipped.md +++ b/src/EFCore.Analyzers/AnalyzerReleases.Shipped.md @@ -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) \ No newline at end of file +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 +--------|----------|----------|------- +EF1003 | Security | Warning | StringConcatenationUsageInRawQueriesDiagnosticAnalyzer, [Documentation](https://learn.microsoft.com/ef/core/querying/sql-queries#passing-parameters) \ No newline at end of file diff --git a/src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs b/src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs index e4a0ea8f28d..31d084489de 100644 --- a/src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs +++ b/src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs @@ -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"); @@ -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"); @@ -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"); @@ -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"); @@ -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 ExecuteSqlRawMethods(Compilation compilation) - { - var type = compilation.RelationalDatabaseFacadeExtensionsType(); - return type?.GetMembers("ExecuteSqlRaw").Where(s => s is IMethodSymbol).Cast() ?? []; - } - - private static IEnumerable ExecuteSqlRawAsyncMethods(Compilation compilation) - { - var type = compilation.RelationalDatabaseFacadeExtensionsType(); - return type?.GetMembers("ExecuteSqlRawAsync").Where(s => s is IMethodSymbol).Cast() ?? []; - } - - private static IMethodSymbol? SqlQueryRawMethod(Compilation compilation) - { - var type = compilation.RelationalDatabaseFacadeExtensionsType(); - return (IMethodSymbol?)type?.GetMembers("SqlQueryRaw").FirstOrDefault(s => s is IMethodSymbol); - } } diff --git a/src/EFCore.Analyzers/NullableAttributes.cs b/src/EFCore.Analyzers/NullableAttributes.cs new file mode 100644 index 00000000000..65ae3ef18b8 --- /dev/null +++ b/src/EFCore.Analyzers/NullableAttributes.cs @@ -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 + /// Specifies that null is allowed as an input even if the corresponding type disallows it. + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class AllowNullAttribute : Attribute + { } + + /// Specifies that null is disallowed as an input even if the corresponding type allows it. + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class DisallowNullAttribute : Attribute + { } + + /// Specifies that an output may be null even if the corresponding type disallows it. + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class MaybeNullAttribute : Attribute + { } + + /// 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. + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class NotNullAttribute : Attribute + { } + + /// Specifies that when a method returns , the parameter may be null even if the corresponding type disallows it. + [AttributeUsage(AttributeTargets.Parameter, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class MaybeNullWhenAttribute : Attribute + { + /// Initializes the attribute with the specified return value condition. + /// + /// The return value condition. If the method returns this value, the associated parameter may be null. + /// + public MaybeNullWhenAttribute(bool returnValue) => ReturnValue = returnValue; + + /// Gets the return value condition. + public bool ReturnValue { get; } + } + + /// Specifies that when a method returns , the parameter will not be null even if the corresponding type allows it. + [AttributeUsage(AttributeTargets.Parameter, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class NotNullWhenAttribute : Attribute + { + /// Initializes the attribute with the specified return value condition. + /// + /// The return value condition. If the method returns this value, the associated parameter will not be null. + /// + public NotNullWhenAttribute(bool returnValue) => ReturnValue = returnValue; + + /// Gets the return value condition. + public bool ReturnValue { get; } + } + + /// Specifies that the output will be non-null if the named parameter is non-null. + [AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, AllowMultiple = true, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class NotNullIfNotNullAttribute : Attribute + { + /// Initializes the attribute with the associated parameter name. + /// + /// The associated parameter name. The output will be non-null if the argument to the parameter specified is non-null. + /// + public NotNullIfNotNullAttribute(string parameterName) => ParameterName = parameterName; + + /// Gets the associated parameter name. + public string ParameterName { get; } + } + + /// Applied to a method that will never return under any circumstance. + [AttributeUsage(AttributeTargets.Method, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class DoesNotReturnAttribute : Attribute + { } + + /// Specifies that the method will not return if the associated Boolean parameter is passed the specified value. + [AttributeUsage(AttributeTargets.Parameter, Inherited = false)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class DoesNotReturnIfAttribute : Attribute + { + /// Initializes the attribute with the specified parameter value. + /// + /// The condition parameter value. Code after the method will be considered unreachable by diagnostics if the argument to + /// the associated parameter matches this value. + /// + public DoesNotReturnIfAttribute(bool parameterValue) => ParameterValue = parameterValue; + + /// Gets the condition parameter value. + public bool ParameterValue { get; } + } +#endif + + /// Specifies that the method or property will ensure that the listed field and property members have not-null values. + [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class MemberNotNullAttribute : Attribute + { + /// Initializes the attribute with a field or property member. + /// + /// The field or property member that is promised to be not-null. + /// + public MemberNotNullAttribute(string member) => Members = [member]; + + /// Initializes the attribute with the list of field and property members. + /// + /// The list of field and property members that are promised to be not-null. + /// + public MemberNotNullAttribute(params string[] members) => Members = members; + + /// Gets field or property member names. + public string[] Members { get; } + } + + /// 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. + [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)] +#if SYSTEM_PRIVATE_CORELIB + public +#else + internal +#endif + sealed class MemberNotNullWhenAttribute : Attribute + { + /// Initializes the attribute with the specified return value condition and a field or property member. + /// + /// The return value condition. If the method returns this value, the associated field or property member will not be null. + /// + /// + /// The field or property member that is promised to be not-null. + /// + public MemberNotNullWhenAttribute(bool returnValue, string member) + { + ReturnValue = returnValue; + Members = [member]; + } + + /// Initializes the attribute with the specified return value condition and list of field and property members. + /// + /// The return value condition. If the method returns this value, the associated field and property members will not be null. + /// + /// + /// The list of field and property members that are promised to be not-null. + /// + public MemberNotNullWhenAttribute(bool returnValue, params string[] members) + { + ReturnValue = returnValue; + Members = members; + } + + /// Gets the return value condition. + public bool ReturnValue { get; } + + /// Gets field or property member names. + public string[] Members { get; } + } +} diff --git a/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs b/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs index bcd1664c2ab..2d91c8191e8 100644 --- a/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs +++ b/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs @@ -10,8 +10,8 @@ namespace Microsoft.EntityFrameworkCore { using System; - - + + /// /// A strongly-typed resource class, for looking up localized strings, etc. /// @@ -23,15 +23,15 @@ namespace Microsoft.EntityFrameworkCore { [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] public class AnalyzerStrings { - + private static global::System.Resources.ResourceManager resourceMan; - + private static global::System.Globalization.CultureInfo resourceCulture; - + [global::System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] internal AnalyzerStrings() { } - + /// /// Returns the cached ResourceManager instance used by this class. /// @@ -45,7 +45,7 @@ internal AnalyzerStrings() { return resourceMan; } } - + /// /// Overrides the current thread's CurrentUICulture property for all /// resource lookups using this strongly typed resource class. @@ -59,7 +59,7 @@ internal AnalyzerStrings() { resourceCulture = value; } } - + /// /// Looks up a localized string similar to {0} is an internal API that supports the Entity Framework Core infrastructure and not subject to the same compatibility standards as public APIs. It may be changed or removed without notice in any release.. /// @@ -68,7 +68,7 @@ public static string InternalUsageMessageFormat { return ResourceManager.GetString("InternalUsageMessageFormat", resourceCulture); } } - + /// /// Looks up a localized string similar to Internal EF Core API usage.. /// @@ -77,7 +77,7 @@ public static string InternalUsageTitle { return ResourceManager.GetString("InternalUsageTitle", resourceCulture); } } - + /// /// Looks up a localized string similar to Risk of vulnerability to SQL injection.. /// @@ -86,16 +86,16 @@ public static string InterpolatedStringUsageInRawQueriesAnalyzerTitle { return ResourceManager.GetString("InterpolatedStringUsageInRawQueriesAnalyzerTitle", resourceCulture); } } - + /// - /// Looks up a localized string similar to Use method which protects against SQL injection.. + /// Looks up a localized string similar to Use method which protects against SQL injection. /// public static string InterpolatedStringUsageInRawQueriesCodeActionTitle { get { return ResourceManager.GetString("InterpolatedStringUsageInRawQueriesCodeActionTitle", resourceCulture); } } - + /// /// Looks up a localized string similar to Method '{0}' inserts interpolated strings directly into the SQL, without any protection against SQL injection. Consider using '{1}' instead, which protects against SQL injection, or make sure that the value is sanitized and suppress the warning.. /// @@ -104,7 +104,25 @@ public static string InterpolatedStringUsageInRawQueriesMessageFormat { return ResourceManager.GetString("InterpolatedStringUsageInRawQueriesMessageFormat", resourceCulture); } } - + + /// + /// Looks up a localized string similar to Risk of vulnerability to SQL injection.. + /// + public static string StringConcatenationUsageInRawQueriesAnalyzerTitle { + get { + return ResourceManager.GetString("StringConcatenationUsageInRawQueriesAnalyzerTitle", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Method '{0}' inserts concatenated strings directly into the SQL, without any protection against SQL injection. Consider using '{1}' instead, which protects against SQL injection, or make sure that the value is sanitized and suppress the warning.. + /// + public static string StringConcatenationUsageInRawQueriesMessageFormat { + get { + return ResourceManager.GetString("StringConcatenationUsageInRawQueriesMessageFormat", resourceCulture); + } + } + /// /// Looks up a localized string similar to DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor.. /// diff --git a/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx b/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx index 2282189be54..c3c04c79bb3 100644 --- a/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx +++ b/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx @@ -1,22 +1,121 @@ - + + - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx - 1.3 + 2.0 - System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor. @@ -36,4 +135,10 @@ Method '{0}' inserts interpolated strings directly into the SQL, without any protection against SQL injection. Consider using '{1}' instead, which protects against SQL injection, or make sure that the value is sanitized and suppress the warning. + + Risk of vulnerability to SQL injection. + + + Method '{0}' inserts concatenated strings directly into the SQL, without any protection against SQL injection. Consider using '{1}' instead, which protects against SQL injection, or make sure that the value is sanitized and suppress the warning. + \ No newline at end of file diff --git a/src/EFCore.Analyzers/SqlRawMethodsCompilationHelper.cs b/src/EFCore.Analyzers/SqlRawMethodsCompilationHelper.cs new file mode 100644 index 00000000000..7e7e15b948e --- /dev/null +++ b/src/EFCore.Analyzers/SqlRawMethodsCompilationHelper.cs @@ -0,0 +1,33 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.CodeAnalysis; + +namespace Microsoft.EntityFrameworkCore; + +internal static class SqlRawMethodsCompilationHelper +{ + public static IMethodSymbol? FromSqlRawMethod(Compilation compilation) + { + var type = compilation.RelationalQueryableExtensionsType(); + return (IMethodSymbol?)type?.GetMembers("FromSqlRaw").FirstOrDefault(s => s is IMethodSymbol); + } + + public static IEnumerable ExecuteSqlRawMethods(Compilation compilation) + { + var type = compilation.RelationalDatabaseFacadeExtensionsType(); + return type?.GetMembers("ExecuteSqlRaw").Where(s => s is IMethodSymbol).Cast() ?? []; + } + + public static IEnumerable ExecuteSqlRawAsyncMethods(Compilation compilation) + { + var type = compilation.RelationalDatabaseFacadeExtensionsType(); + return type?.GetMembers("ExecuteSqlRawAsync").Where(s => s is IMethodSymbol).Cast() ?? []; + } + + public static IMethodSymbol? SqlQueryRawMethod(Compilation compilation) + { + var type = compilation.RelationalDatabaseFacadeExtensionsType(); + return (IMethodSymbol?)type?.GetMembers("SqlQueryRaw").FirstOrDefault(s => s is IMethodSymbol); + } +} diff --git a/src/EFCore.Analyzers/StringConcatenationUsageInRawQueriesDiagnosticAnalyzer.cs b/src/EFCore.Analyzers/StringConcatenationUsageInRawQueriesDiagnosticAnalyzer.cs new file mode 100644 index 00000000000..2a288ba545d --- /dev/null +++ b/src/EFCore.Analyzers/StringConcatenationUsageInRawQueriesDiagnosticAnalyzer.cs @@ -0,0 +1,213 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.EntityFrameworkCore; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class StringConcatenationUsageInRawQueriesDiagnosticAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor Descriptor + = new( + EFDiagnostics.StringConcatenationUsageInRawQueries, + title: AnalyzerStrings.StringConcatenationUsageInRawQueriesAnalyzerTitle, + messageFormat: AnalyzerStrings.StringConcatenationUsageInRawQueriesMessageFormat, + category: "Security", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + public override ImmutableArray SupportedDiagnostics + => [Descriptor]; + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterOperationAction(AnalyzerInvocation, OperationKind.Invocation); + } + + private void AnalyzerInvocation(OperationAnalysisContext context) + { + var invocation = (IInvocationOperation)context.Operation; + var targetMethod = invocation.TargetMethod; + + var report = targetMethod.Name switch + { + "FromSqlRaw" => AnalyzeFromSqlRawInvocation(invocation), + "ExecuteSqlRaw" or "ExecuteSqlRawAsync" => AnalyzeExecuteSqlRawInvocation(invocation), + "SqlQueryRaw" => AnalyzeSqlQueryRawInvocation(invocation), + _ => false + }; + + if (report) + { + context.ReportDiagnostic( + Diagnostic.Create( + Descriptor, + GetTargetLocation(invocation.Syntax), + targetMethod.Name, + GetReplacementMethodName(targetMethod.Name))); + } + + static Location GetTargetLocation(SyntaxNode syntax) + { + if (syntax is not InvocationExpressionSyntax invocationExpression) + { + Debug.Fail("In theory should never happen"); + return syntax.GetLocation(); + } + + var targetNode = invocationExpression.Expression; + + while (targetNode is MemberAccessExpressionSyntax memberAccess) + { + targetNode = memberAccess.Name; + } + + // Generic name case, e.g. `db.Database.SqlQueryRaw(...)`. + // At this point `targetNode` is `SqlQueryRaw`, but we need location of the actual identifier + if (targetNode is GenericNameSyntax genericName) + { + return genericName.Identifier.GetLocation(); + } + + // We should appear at name expression, representing method name token, e.g.: + // db.Users.[|FromSqlRaw|](...) or db.Database.[|ExecuteSqlRaw|](...) + return targetNode.GetLocation(); + } + } + + internal static string GetReplacementMethodName(string oldName) + => oldName switch + { + "FromSqlRaw" => "FromSql", + "ExecuteSqlRaw" => "ExecuteSql", + "ExecuteSqlRawAsync" => "ExecuteSqlAsync", + "SqlQueryRaw" => "SqlQuery", + _ => oldName + }; + + private static bool AnalyzeFromSqlRawInvocation(IInvocationOperation invocation) + { + var targetMethod = invocation.TargetMethod; + Debug.Assert(targetMethod.Name == "FromSqlRaw"); + + var compilation = invocation.SemanticModel!.Compilation; + var correctFromSqlRaw = SqlRawMethodsCompilationHelper.FromSqlRawMethod(compilation); + + Debug.Assert(correctFromSqlRaw is not null, "Unable to find original `FromSqlRaw` method"); + + // Verify that the method is the one we analyze and its second argument, which corresponds to `string sql`, is a string concatenation + if (correctFromSqlRaw is null + || !targetMethod.ConstructedFrom.Equals(correctFromSqlRaw, SymbolEqualityComparer.Default) + || !TryGetStringConcatenation(invocation.Arguments[1].Value, out var concatenation)) + { + return false; + } + + // Report warning if string concatenation is not from constants + return AnalyzeConcatenation(concatenation); + } + + private static bool AnalyzeExecuteSqlRawInvocation(IInvocationOperation invocation) + { + var targetMethod = invocation.TargetMethod; + Debug.Assert(targetMethod.Name is "ExecuteSqlRaw" or "ExecuteSqlRawAsync"); + + var compilation = invocation.SemanticModel!.Compilation; + + if (targetMethod.Name == "ExecuteSqlRaw") + { + var correctMethods = SqlRawMethodsCompilationHelper.ExecuteSqlRawMethods(compilation); + + Debug.Assert(correctMethods.Any(), "Unable to find any `ExecuteSqlRaw` methods"); + + if (!correctMethods.Contains(targetMethod.ConstructedFrom, SymbolEqualityComparer.Default)) + { + return false; + } + } + else + { + var correctMethods = SqlRawMethodsCompilationHelper.ExecuteSqlRawAsyncMethods(compilation); + + Debug.Assert(correctMethods.Any(), "Unable to find any `ExecuteSqlRawAsync` methods"); + + if (!correctMethods.Contains(targetMethod.ConstructedFrom, SymbolEqualityComparer.Default)) + { + return false; + } + } + + // At this point assume that the method is correct since both `ExecuteSqlRaw` and `ExecuteSqlRawAsync` have multiple overloads. + // Checking for every possible one is too much work for almost no gain. + // So check whether the second argument, that corresponds to `string sql` parameter, is an string concatenation... + if (!TryGetStringConcatenation(invocation.Arguments[1].Value, out var concatenation)) + { + return false; + } + + // ...and report warning if string concatenation is not from constants + return AnalyzeConcatenation(concatenation); + } + + private static bool AnalyzeSqlQueryRawInvocation(IInvocationOperation invocation) + { + var targetMethod = invocation.TargetMethod; + Debug.Assert(targetMethod.Name == "SqlQueryRaw"); + + var compilation = invocation.SemanticModel!.Compilation; + + var correctSqlQueryRaw = SqlRawMethodsCompilationHelper.SqlQueryRawMethod(compilation); + + Debug.Assert(correctSqlQueryRaw is not null, "Unable to find original `SqlQueryRaw` method"); + + // Verify that the method is the one we analyze and its second argument, which corresponds to `string sql`, is string concatenation + if (correctSqlQueryRaw is null + || !targetMethod.ConstructedFrom.Equals(correctSqlQueryRaw, SymbolEqualityComparer.Default) + || !TryGetStringConcatenation(invocation.Arguments[1].Value, out var concatenation)) + { + return false; + } + + // Report warning if string concatenation is not from constants + return AnalyzeConcatenation(concatenation); + } + + private static bool TryGetStringConcatenation(IOperation operation, [NotNullWhen(true)] out IBinaryOperation? concatenation) + { + if (operation is IBinaryOperation + { + OperatorKind: BinaryOperatorKind.Add, + Type.SpecialType: SpecialType.System_String, + } binaryOperation) + { + concatenation = binaryOperation; + return true; + } + + concatenation = default; + return false; + } + + private static bool AnalyzeConcatenation(IBinaryOperation operation) + { + var left = operation.LeftOperand; + var right = operation.RightOperand; + + if ((left is IBinaryOperation leftBinary && AnalyzeConcatenation(leftBinary)) + || (right is IBinaryOperation rightBinary && AnalyzeConcatenation(rightBinary))) + { + return true; + } + + return !left.ConstantValue.HasValue || !right.ConstantValue.HasValue; + } +} diff --git a/src/Shared/EFDiagnostics.cs b/src/Shared/EFDiagnostics.cs index 8cb61890d9e..b8437c7980e 100644 --- a/src/Shared/EFDiagnostics.cs +++ b/src/Shared/EFDiagnostics.cs @@ -10,6 +10,7 @@ internal static class EFDiagnostics { public const string InternalUsage = "EF1001"; public const string InterpolatedStringUsageInRawQueries = "EF1002"; + public const string StringConcatenationUsageInRawQueries = "EF1003"; public const string SuppressUninitializedDbSetRule = "EFSPR1001"; // Diagnostics for [Experimental] diff --git a/test/EFCore.Analyzers.Tests/StringConcatenationUsageInRawQueriesTests.cs b/test/EFCore.Analyzers.Tests/StringConcatenationUsageInRawQueriesTests.cs new file mode 100644 index 00000000000..3a01f6c4d97 --- /dev/null +++ b/test/EFCore.Analyzers.Tests/StringConcatenationUsageInRawQueriesTests.cs @@ -0,0 +1,494 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Testing; +using VerifyCS = + CSharpAnalyzerVerifier; + +public class StringConcatenationUsageInRawQueriesTests +{ + private const string MyDbContext = + """ +using System.Collections.Generic; +using Microsoft.EntityFrameworkCore; + +class User +{ + public int Id { get; set; } + public string FirstName { get; set; } + public string LastName { get; set; } +} + +class MyDbContext : DbContext +{ + public DbSet Users { get; init; } +} +"""; + + #region FromSqlRaw + + [Fact] + public Task FromSqlRaw_constant_string_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task FromSqlRaw_constant_strings_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Users.FromSqlRaw("SELECT * FROM [Users]" + + " WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task FromSqlRaw_constant_string_with_parameters_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = {0}", id); + } +} +"""); + + [Fact] + public Task FromSqlRaw_argument_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = " + id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 18)); + + [Fact] + public Task FromSqlRaw_method_call_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = " + GetId()); + } + + int GetId() => 1; +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 18)); + + [Fact] + public Task FromSqlRaw_constant_string_member_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + const string Id = "1"; + + void M(MyDbContext db) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = " + Id); + } +} +"""); + + [Fact] + public Task FromSqlRaw_readonly_static_string_variable_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + readonly static string Id = "1"; + + void M(MyDbContext db) + { + db.Users.FromSqlRaw("SELECT * FROM [Users] WHERE [Id] = " + Id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(22, 18)); + + #endregion + + #region ExecuteSqlRaw + + [Fact] + public Task ExecuteSqlRaw_constant_string_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task ExecuteSqlRaw_constant_strings_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users]" + + " WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task ExecuteSqlRaw_constant_string_with_parameters_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = {0}", id); + } +} +"""); + + [Fact] + public Task ExecuteSqlRaw_argument_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = " + id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task ExecuteSqlRaw_method_call_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = " + GetId()); + } + + int GetId() => 1; +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task ExecuteSqlRaw_constant_string_member_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + const string Id = "1"; + + void M(MyDbContext db) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = " + Id); + } +} +"""); + + [Fact] + public Task ExecuteSqlRaw_readonly_static_string_variable_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + readonly static string Id = "1"; + + void M(MyDbContext db) + { + db.Database.ExecuteSqlRaw("DELETE FROM [Users] WHERE [Id] = " + Id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(22, 21)); + + #endregion + + #region ExecuteSqlRawAsync + + [Fact] + public Task ExecuteSqlRawAsync_constant_string_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task ExecuteSqlRawAsync_constant_strings_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users]" + + " WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task ExecuteSqlRawAsync_constant_string_with_parameters_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = {0}", id); + } +} +"""); + + [Fact] + public Task ExecuteSqlRawAsync_argument_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = " + id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task ExecuteSqlRawAsync_method_call_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = " + GetId()); + } + + int GetId() => 1; +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task ExecuteSqlRawAsync_constant_string_member_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + const string Id = "1"; + + void M(MyDbContext db) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = " + Id); + } +} +"""); + + [Fact] + public Task ExecuteSqlRawAsync_readonly_static_string_variable_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + readonly static string Id = "1"; + + void M(MyDbContext db) + { + db.Database.ExecuteSqlRawAsync("DELETE FROM [Users] WHERE [Id] = " + Id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(22, 21)); + + #endregion + + #region SqlQueryRaw + + [Fact] + public Task SqlQueryRaw_constant_string_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task SqlQueryRaw_constant_strings_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users]" + + " WHERE [Id] = 1"); + } +} +"""); + + [Fact] + public Task SqlQueryRaw_constant_string_with_parameters_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = {0}", id); + } +} +"""); + + [Fact] + public Task SqlQueryRaw_argument_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db, int id) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = " + id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task SqlQueryRaw_method_call_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + void M(MyDbContext db) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = " + GetId()); + } + + int GetId() => 1; +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(20, 21)); + + [Fact] + public Task SqlQueryRaw_constant_string_member_do_not_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + const string Id = "1"; + + void M(MyDbContext db) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = " + Id); + } +} +"""); + + [Fact] + public Task SqlQueryRaw_readonly_static_string_variable_report() + => VerifyCS.VerifyAnalyzerAsync( + $$""" +{{MyDbContext}} + +class C +{ + readonly static string Id = "1"; + + void M(MyDbContext db) + { + db.Database.SqlQueryRaw("SELECT [Age] FROM [Users] WHERE [Id] = " + Id); + } +} +""", new DiagnosticResult(EFDiagnostics.StringConcatenationUsageInRawQueries, DiagnosticSeverity.Warning).WithLocation(22, 21)); + + #endregion +}