-
Notifications
You must be signed in to change notification settings - Fork 186
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
Print warning when AddJsonFile is called with "local.settings.json" #2777
Open
BogdanYarotsky
wants to merge
27
commits into
Azure:main
Choose a base branch
from
BogdanYarotsky:local-settings-json-analyzer
base: main
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.
Open
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
12813bf
init commit
BogdanYarotsky e3de772
working on the unit test
BogdanYarotsky a00b2a6
fixed test code, WIP analyzer logic
BogdanYarotsky 32e1702
first green test
BogdanYarotsky 980398e
working analyzer
BogdanYarotsky 1de7a05
handling the case when custom code has the same signature for AddJson…
BogdanYarotsky 2577231
added a new failing test
BogdanYarotsky d86fe06
added one more test for variable check
BogdanYarotsky c37e1b6
added data flow check
BogdanYarotsky fc286fb
small refactoring
BogdanYarotsky d99202f
moved local settings json to const
BogdanYarotsky a9da19b
added tests for overloads
BogdanYarotsky 13608e6
simplifying the analyzer
BogdanYarotsky f47ba55
refactoring
BogdanYarotsky 4e5c3da
simplfied analyzer, added failing test
BogdanYarotsky 6c78878
added syntax tree walker to catch assignments
BogdanYarotsky 8ed0c25
WIP - handling assignments
BogdanYarotsky 3eab109
refactored analyzer to look for all declarations and assignments
BogdanYarotsky f950285
added test for static readonly, limited scope of assignments scanning
BogdanYarotsky 64c261b
added some comments, refactoring of LocalSettingsJsonNotAllowedAsConf…
BogdanYarotsky f156413
changed the id number of the new analyzer
BogdanYarotsky 5ba7ad6
added new .md file which describes the new warning
BogdanYarotsky 0ef4ad9
small changes to warning description
BogdanYarotsky ef661fc
changed the number of the new compiler warning to 16 to avoid conflic…
BogdanYarotsky bf542b8
Changed the Rule ID in the .md file
BogdanYarotsky 6465559
Improvements to azfw0016 description, fixed warning number in .md file
BogdanYarotsky 31f3485
updated azfw0016 warning ID to azfw0017 to avoid merge conflicts
BogdanYarotsky 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 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,27 @@ | ||
# AZFW0016: Redundant usage of local.settings.json in the worker configuration | ||
|
||
| | Value | | ||
|-|-| | ||
| **Rule ID** |AZFW00016| | ||
| **Category** |[Usage]| | ||
| **Severity** |Warning| | ||
|
||
## Cause | ||
|
||
This rule is triggered when `local.settings.json` is passed to `AddJsonFile()` method of the `IConfigurationBuilder`. | ||
|
||
## Rule description | ||
|
||
`local.settings.json` is a file that is automatically loaded to the configuration by Azure Functions Core Tools. | ||
There is no need to explicitly add it to the configuration of the worker during development. And in production scenarios `local.settings.json` should never be used because it means checking it into the source control. | ||
And this a securiy risk because this file usually contains sensitive information like connection strings. | ||
|
||
|
||
## How to fix violations | ||
|
||
Don't pass `local.settings.json` to `AddJsonFile()` when configuring the Azure Functions Worker. | ||
|
||
|
||
## When to suppress | ||
|
||
It is okay to suppress the warning if it happens in a code block that is guaranteed to execute only during the development (Debug configuration) and you want to be explicit about the source of configuration. It should never be suppressed for the code that will run in production. |
This file contains 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
143 changes: 143 additions & 0 deletions
143
sdk/Sdk.Analyzers/LocalSettingsJsonNotAllowedAsConfiguration.cs
This file contains 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,143 @@ | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.IO; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
|
||
namespace Microsoft.Azure.Functions.Worker.Sdk.Analyzers | ||
{ | ||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
public class LocalSettingsJsonNotAllowedAsConfiguration : DiagnosticAnalyzer | ||
{ | ||
private const string ConfigurationBuilderFullName = "Microsoft.Extensions.Configuration.IConfigurationBuilder"; | ||
private const string LocalSettingsJsonFileName = "local.settings.json"; | ||
private const string AddJsonFileMethodName = "AddJsonFile"; | ||
|
||
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = | ||
ImmutableArray.Create(DiagnosticDescriptors.LocalSettingsJsonNotAllowedAsConfiguration); | ||
|
||
public override void Initialize(AnalysisContext context) | ||
{ | ||
context.EnableConcurrentExecution(); | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze); | ||
context.RegisterSyntaxNodeAction(AnalyzeInvocation, SyntaxKind.InvocationExpression); | ||
} | ||
|
||
// This analyzer handles only the overloads of AddJsonFile() with a string as first argument | ||
private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) | ||
{ | ||
var invocation = (InvocationExpressionSyntax)context.Node; | ||
if (!IsAddJsonFileMethodWithAtLeastOneArgument(invocation, context)) | ||
{ | ||
return; | ||
} | ||
|
||
var firstArgument = invocation.ArgumentList.Arguments.First().Expression; | ||
if (!IsOfStringType(firstArgument, context)) | ||
{ | ||
return; | ||
} | ||
|
||
if (FindLiterals(firstArgument, context).Any(IsPathToLocalSettingsJson)) | ||
{ | ||
var diagnostic = CreateDiagnostic(firstArgument); | ||
context.ReportDiagnostic(diagnostic); | ||
} | ||
} | ||
|
||
// This method ensures that the analyzer doesn't check more than it should. | ||
// It looks for the literal values of the argument, from the easiest to hardest to find. | ||
// Current order: literal -> constant -> declaration -> assignment in containing method. | ||
private static IEnumerable<string> FindLiterals(ExpressionSyntax argument, SyntaxNodeAnalysisContext context) | ||
{ | ||
if (argument is LiteralExpressionSyntax literal) | ||
{ | ||
yield return literal.Token.ValueText; | ||
yield break; // no need to check further | ||
} | ||
|
||
var constant = context.SemanticModel.GetConstantValue(argument); | ||
if (constant.HasValue) | ||
{ | ||
yield return constant.Value.ToString(); | ||
yield break; // no need to check further | ||
} | ||
|
||
if (argument is not IdentifierNameSyntax identifier) | ||
{ | ||
yield break; | ||
} | ||
|
||
var identifierSymbol = context.SemanticModel.GetSymbolInfo(identifier).Symbol; | ||
if (identifierSymbol is null) | ||
{ | ||
yield break; | ||
} | ||
|
||
var declarationLiterals = identifierSymbol.DeclaringSyntaxReferences | ||
.Select(reference => reference.GetSyntax(context.CancellationToken)) | ||
.OfType<VariableDeclaratorSyntax>() | ||
.Select(variable => variable.Initializer?.Value) | ||
.OfType<LiteralExpressionSyntax>(); | ||
|
||
// usually there should be only 1 declaration but better safe than sorry | ||
foreach (var declarationLiteral in declarationLiterals) | ||
{ | ||
yield return declarationLiteral.Token.ValueText; | ||
} | ||
|
||
// let's check assignments in the containing method | ||
var containingMethod = argument.Ancestors() | ||
.OfType<MethodDeclarationSyntax>() | ||
.FirstOrDefault(); | ||
|
||
if (containingMethod is null) | ||
{ | ||
yield break; | ||
} | ||
|
||
var assignmentsInContainingMethod = containingMethod.DescendantNodes() | ||
.OfType<AssignmentExpressionSyntax>() | ||
.Where(assignment => assignment.SpanStart < argument.SpanStart); | ||
|
||
foreach (var assignment in assignmentsInContainingMethod) | ||
{ | ||
var leftSymbol = context.SemanticModel.GetSymbolInfo(assignment.Left).Symbol; | ||
if (SymbolEqualityComparer.Default.Equals(leftSymbol, identifierSymbol) | ||
&& assignment.Right is LiteralExpressionSyntax assignmentLiteral) | ||
{ | ||
yield return assignmentLiteral.Token.ValueText; | ||
} | ||
} | ||
} | ||
|
||
private static bool IsAddJsonFileMethodWithAtLeastOneArgument( | ||
InvocationExpressionSyntax invocation, SyntaxNodeAnalysisContext context) | ||
{ | ||
return invocation.ArgumentList.Arguments.Count > 0 | ||
&& invocation.Expression is MemberAccessExpressionSyntax { Name.Identifier.Text: AddJsonFileMethodName } memberAccess | ||
&& context.SemanticModel.GetSymbolInfo(memberAccess).Symbol is IMethodSymbol methodSymbol | ||
&& methodSymbol.ReceiverType?.ToDisplayString() == ConfigurationBuilderFullName; | ||
} | ||
|
||
private static bool IsOfStringType(ExpressionSyntax expression, SyntaxNodeAnalysisContext context) | ||
{ | ||
return context.SemanticModel.GetTypeInfo(expression).Type?.SpecialType == SpecialType.System_String; | ||
} | ||
|
||
private static bool IsPathToLocalSettingsJson(string literal) | ||
{ | ||
return Path.GetFileName(literal) == LocalSettingsJsonFileName; | ||
} | ||
|
||
private static Diagnostic CreateDiagnostic(ExpressionSyntax expression) | ||
{ | ||
return Diagnostic.Create( | ||
DiagnosticDescriptors.LocalSettingsJsonNotAllowedAsConfiguration, | ||
expression.GetLocation()); | ||
} | ||
} | ||
} |
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.
AZFW0016 already exists (recently merged) - can you make this AZFW0017?