Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[IAST] Safeguard Method Replace aspects with try/catch #5841

Merged
merged 4 commits into from
Aug 6, 2024
Merged

[IAST] Safeguard Method Replace aspects with try/catch #5841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2317eac
CallSite replace aspects securization
daniel-romano-DD Aug 1, 2024
cb89e7c
Minor fixes
daniel-romano-DD Aug 2, 2024
543b947
Updated overload exclusions in tests
daniel-romano-DD Aug 2, 2024
c6b25ee
Fixed now unsupported tests for netcore 3
daniel-romano-DD Aug 3, 2024
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
49 changes: 28 additions & 21 deletions tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/ReplaceAspectAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="ReplaceAspectAnalyzer.cs" company="Datadog">
// <copyright file="ReplaceAspectAnalyzer.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>
Expand Down Expand Up @@ -28,7 +28,7 @@ public class ReplaceAspectAnalyzer : DiagnosticAnalyzer
/// <summary>
/// The severity of the diagnostic
/// </summary>
public const DiagnosticSeverity Severity = DiagnosticSeverity.Info;
public const DiagnosticSeverity Severity = DiagnosticSeverity.Error;

#pragma warning disable RS2008 // Enable analyzer release tracking for the analyzer project
private static readonly DiagnosticDescriptor MissingTryCatchRule = new(
Expand Down Expand Up @@ -92,6 +92,9 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
}

var bodyBlock = methodDeclaration.Body;
var isVoidMethod = methodDeclaration.ReturnType is PredefinedTypeSyntax { Keyword.Text: "void" };
int expectedStatements = isVoidMethod ? 2 : 3;

if (bodyBlock is null)
{
// If we don't have a bodyBlock, it's probably a lambda or expression bodied member
Expand All @@ -107,15 +110,15 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
return;
}

if (bodyBlock.Statements.Count != 3)
if (bodyBlock.Statements.Count != expectedStatements)
{
// We require exactly 3 statements, so this must be an error
// We require exactly a predefined amount of statements, so this must be an error
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

// check the first statement
if (bodyBlock.Statements[0] is not LocalDeclarationStatementSyntax localDeclaration)
if (!isVoidMethod && bodyBlock.Statements[0] is not LocalDeclarationStatementSyntax)
{
andrewlock marked this conversation as resolved.
Show resolved Hide resolved
// this is an error, and we can't go much further
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
Expand Down Expand Up @@ -180,25 +183,29 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
}

// final check, do we return the variable?
if (bodyBlock.Statements[2] is not ReturnStatementSyntax returnStatement)
if (!isVoidMethod)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}
if (bodyBlock.Statements[2] is not ReturnStatementSyntax returnStatement)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

// should be returning the variable
if (returnStatement.Expression is not IdentifierNameSyntax identifierName)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}
// should be returning the variable
if (returnStatement.Expression is not IdentifierNameSyntax identifierName)
{
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
return;
}

if (!localDeclaration.Declaration.Variables.Any()
|| localDeclaration.Declaration.Variables[0] is not { } variable
|| variable.Identifier.ToString() != identifierName.Identifier.ToString())
{
// not returning the right thing
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
LocalDeclarationStatementSyntax localDeclaration = (LocalDeclarationStatementSyntax)bodyBlock.Statements[0];
if (!localDeclaration.Declaration.Variables.Any()
|| localDeclaration.Declaration.Variables[0] is not { } variable
|| variable.Identifier.ToString() != identifierName.Identifier.ToString())
{
// not returning the right thing
context.ReportDiagnostic(Diagnostic.Create(MissingTryCatchRule, bodyBlock.GetLocation()));
}
}
}
}
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/ReplaceAspectCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private async Task<Document> AddTryCatch(Document document, MethodDeclarationSyn

var catchDeclaration = SyntaxFactory.CatchDeclaration(SyntaxFactory.IdentifierName("Exception"), SyntaxFactory.Identifier("ex"));
var logExpression = SyntaxFactory.ExpressionStatement(
SyntaxFactory.ParseExpression($$"""Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));
SyntaxFactory.ParseExpression($$"""IastModule.Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));

var catchSyntax = SyntaxFactory.CatchClause()
.WithDeclaration(catchDeclaration)
Expand Down
6 changes: 2 additions & 4 deletions ...net461/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,17 +422,15 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.Object,System.Object,System.Object,System.Object)\",\"\",[0],[False],[None],Default,[])] Concat(System.Object,System.Object,System.Object,System.Object)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
" [AspectMethodReplace(\"System.String::ToCharArray(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down
9 changes: 2 additions & 7 deletions ...net6.0/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,7 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.String,System.String,System.String,System.String)\",\"\",[0],[False],[StringLiterals],Default,[])] Concat(System.String,System.String,System.String,System.String)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
Expand All @@ -470,11 +469,9 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down Expand Up @@ -562,8 +559,6 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.String[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object)",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object)",
"[AspectClass(\"mscorlib,netstandard,System.Runtime\",[None],Sink,[ReflectionInjection])] Datadog.Trace.Iast.Aspects.ActivatorAspect",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String)\",\"\",[1,0],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String,System.Object[])\",\"\",[2,1],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
Expand Down
9 changes: 2 additions & 7 deletions ...app3.1/Datadog.Trace.SourceGenerators/AspectsDefinitionsGenerator/AspectsDefinitions.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,7 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Concat(System.String,System.String,System.String,System.String)\",\"\",[0],[False],[StringLiterals],Default,[])] Concat(System.String,System.String,System.String,System.String)",
" [AspectMethodReplace(\"System.String::Concat(System.String[])\",\"\",[0],[False],[None],Default,[])] Concat(System.String[])",
" [AspectMethodReplace(\"System.String::Concat(System.Object[])\",\"\",[0],[False],[None],Default,[])] Concat(System.Object[])",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Concat2(System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Concat(System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] Concat(System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32)",
" [AspectMethodReplace(\"System.String::Substring(System.Int32,System.Int32)\",\"\",[0],[False],[StringLiteral_0],Default,[])] Substring(System.String,System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::ToCharArray()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToCharArray(System.String)",
Expand All @@ -457,11 +456,9 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.String[],System.Int32,System.Int32)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.String[],System.Int32,System.Int32)",
" [AspectMethodReplace(\"System.String::Join(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.Char,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Object[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.String[])\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.String[])",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] Join(System.String,System.Collections.IEnumerable)",
" [AspectMethodReplace(\"System.String::Join(System.String,System.Collections.Generic.IEnumerable`1<System.String>)\",\"\",[0],[False],[None],Default,[])] JoinString(System.String,System.Collections.Generic.IEnumerable)",
" [AspectMethodReplace(\"System.String::ToUpper()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String)",
" [AspectMethodReplace(\"System.String::ToUpper(System.Globalization.CultureInfo)\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpper(System.String,System.Globalization.CultureInfo)",
" [AspectMethodReplace(\"System.String::ToUpperInvariant()\",\"\",[0],[False],[StringLiteral_0],Default,[])] ToUpperInvariant(System.String)",
Expand Down Expand Up @@ -549,8 +546,6 @@ internal static partial class AspectDefinitions
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.String[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.String[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Object[])\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object[])",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.Char,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.Char,System.Object)",
" [AspectMethodReplace(\"System.Text.StringBuilder::AppendJoin(System.String,System.Collections.Generic.IEnumerable`1<!!0>)\",\"\",[0],[False],[None],Default,[])] AppendJoin(System.Text.StringBuilder,System.String,System.Object)",
"[AspectClass(\"mscorlib,netstandard,System.Runtime\",[None],Sink,[ReflectionInjection])] Datadog.Trace.Iast.Aspects.ActivatorAspect",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String)\",\"\",[1,0],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
" [AspectMethodInsertBefore(\"System.Activator::CreateInstance(System.String,System.String,System.Object[])\",\"\",[2,1],[False,False],[None],Default,[])] ReflectionInjectionParam(System.String)",
Expand Down
Loading
Loading