-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Analyzer for string concatenation in raw SQL methods. #36698
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.
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
Draft
cincuranet
wants to merge
1
commit into
dotnet:release/10.0
Choose a base branch
from
cincuranet:concat-analyzer
base: release/10.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
46
src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
RS1019
Diagnostic Id 'EF1002' is already used by analyzer ....I don't know. For example, would you say
IDE0018
Inline Variable Declaration andIDE0019
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.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, was not aware...
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).
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.
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?
Yep, let us know @AndriySvyryd.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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...
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see what's the difference in our views. When I do
FromSqlRaw($"...")
I see that as a mistake in method selection (instead ofFromSql[Interpolated]
), not that I want to build string SQL and it's a problem. When I doFromSqlRaw("..." + "...")
that's a possible mistake.There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.