Add new analyzer for ICorsHeaderAppender/CorsHeaderAppender usage (#326)

* Add new analyzer to prevent injection and instantiation of ICorsHeaderAppender/CorsHeaderAppender

* Switching IsClassWhiteListed to take in ISymbol; replacing INamedTypeSymbol.IsNullOrError checks with simple null checks

* Move CorsHeaderAppenderUsageAnalyzer to the new D2L.CodeStyle.Analyzers.Visibility namespace (for ease of future generalization)

src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj

@@ -31,6 +31,7 @@
     <WarningLevel>4</WarningLevel>
   </PropertyGroup>
   <ItemGroup>
+    <Compile Include="Visibility\CorsHeaderAppenderUsageAnalyzer.cs" />
     <Compile Include="Immutability\ImmutableGenericAttributeAnalyzer.cs" />
     <Compile Include="ApiUsage\JsonParamBinderAttribute\JsonParamBinderAnalyzer.cs" />
     <Compile Include="ApiUsage\JsonParamBinderAttribute\JsonParamBinderAnalyzerFixer.cs" />

src/D2L.CodeStyle.Analyzers/Diagnostics.cs

@@ -271,5 +271,15 @@ public static class Diagnostics {
 			isEnabledByDefault: true,
 			description: "Unnecessary mutability annotations should be removed to keep the code base clean"
 		);
+
+		public static readonly DiagnosticDescriptor DangerousUsageOfCorsHeaderAppender = new DiagnosticDescriptor(
+			id: "D2L0031",
+			title: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities.",
+			messageFormat: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities.",
+			category: "Safety",
+			defaultSeverity: DiagnosticSeverity.Error,
+			isEnabledByDefault: true,
+			description: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities."
+		);
 	}
 }

src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs

@@ -0,0 +1,146 @@
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.Linq;
+using D2L.CodeStyle.Analyzers.Extensions;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace D2L.CodeStyle.Analyzers.Visibility {
+
+	[DiagnosticAnalyzer( LanguageNames.CSharp )]
+	internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer {
+
+		private const string CorsInterface = "D2L.LP.Web.Cors.ICorsHeaderAppender";
+		private const string CorsClass = "D2L.LP.Web.Cors.CorsHeaderAppender";
+
+		public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
+			=> ImmutableArray.Create(
+				Diagnostics.DangerousUsageOfCorsHeaderAppender
+			);
+
+		public override void Initialize( AnalysisContext context ) {
+			context.EnableConcurrentExecution();
+			context.RegisterCompilationStartAction(
+				RegisterCorsHeadersUsageAnalyzer
+			);
+		}
+
+		public void RegisterCorsHeadersUsageAnalyzer(
+			CompilationStartAnalysisContext context
+		) {
+			INamedTypeSymbol interfaceType = context.Compilation
+				.GetTypeByMetadataName( CorsInterface );
+
+			if( !interfaceType.IsNullOrErrorType() ) {
+				context.RegisterSyntaxNodeAction(
+					ctx => PreventInjection(
+						ctx,
+						interfaceType
+					),
+					SyntaxKind.ConstructorDeclaration
+				);
+			}
+
+			INamedTypeSymbol classType = context.Compilation
+				.GetTypeByMetadataName( CorsClass );
+
+			if( !classType.IsNullOrErrorType() ) {
+				context.RegisterSyntaxNodeAction(
+					ctx => PreventManualInstantiation(
+						ctx,
+						classType
+					),
+					SyntaxKind.ObjectCreationExpression
+				);
+			}
+
+		}
+
+		/// <summary>
+		/// Prevent injection of the ICorsHttpHeaderHelper interface except in
+		/// a specific whitelist of classes.
+		/// </summary>
+		private static void PreventInjection(
+			SyntaxNodeAnalysisContext context,
+			INamedTypeSymbol interfaceType
+		) {
+			var constructor = context.Node as ConstructorDeclarationSyntax;
+			if( constructor == null ) {
+				return;
+			}
+
+			foreach( var parameter in constructor.ParameterList.Parameters ) {
+				INamedTypeSymbol paramType = context.SemanticModel
+					.GetTypeInfo( parameter.Type )
+					.Type as INamedTypeSymbol;
+
+				if( paramType == null || !paramType.Equals( interfaceType ) ) {
+					return;
+				}
+
+				var parentClasses = context.Node
+					.Ancestors()
+					.Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) );
+
+				var parentSymbols = parentClasses.Select(
+					c => context.SemanticModel.GetDeclaredSymbol( c )
+				);
+
+				if( parentSymbols.Any( IsClassWhitelisted ) ) {
+					return;
+				}
+
+				context.ReportDiagnostic(
+					Diagnostic.Create(
+						Diagnostics.DangerousUsageOfCorsHeaderAppender,
+						parameter.GetLocation()
+					)
+				);
+			}
+		}
+
+		/// <summary>
+		/// We never want to allow manual instantiation of this object, so no
+		/// whitelist here.
+		/// </summary>
+		private static void PreventManualInstantiation(
+			SyntaxNodeAnalysisContext context,
+			INamedTypeSymbol classType
+		) {
+			var instantiation = context.Node as ObjectCreationExpressionSyntax;
+			if( instantiation == null ) {
+				return;
+			}
+
+			INamedTypeSymbol baseType = context.SemanticModel
+				.GetTypeInfo( instantiation )
+				.Type as INamedTypeSymbol;
+
+			if( baseType == null || !baseType.Equals( classType ) ) {
+				return;
+			}
+
+			context.ReportDiagnostic(
+				Diagnostic.Create(
+					Diagnostics.DangerousUsageOfCorsHeaderAppender,
+					context.Node.GetLocation()
+				)
+			);
+		}
+
+		/// <summary>
+		/// A list of classes that have been cleared to implement these headers
+		/// </summary>
+		private static readonly ImmutableHashSet<string> WhitelistedClasses = new HashSet<string> {
+			"D2L.LP.Web.ContentHandling.Handlers.ContentHttpHandler",
+			"D2L.LP.Web.Files.FileViewing.StreamFileViewerResult",
+			"D2L.LP.Web.Files.FileViewing.Default.StreamFileViewerResultFactory"
+		}.ToImmutableHashSet();
+
+		private static bool IsClassWhitelisted( ISymbol classSymbol ) {
+			return WhitelistedClasses.Contains( classSymbol.ToString() );
+		}
+	}
+}

tests/D2L.CodeStyle.Analyzers.Test/D2L.CodeStyle.Analyzers.Tests.csproj

@@ -47,6 +47,7 @@
     <EmbeddedResource Include="Specs\ImmutabilityExceptionInheritanceAnalyzer.cs" />
     <EmbeddedResource Include="Specs\ImmutableCollectionsAnalyzer.cs" />
     <EmbeddedResource Include="Specs\ImmutableGenericAttributeAnalyzer.cs" />
+    <EmbeddedResource Include="Specs\CorsHeaderAppenderUsageAnalyzer.cs" />
     <Compile Include="TestBase.cs" />
     <Compile Include="Extensions\TypeSymbolExtensionsTests.cs" />
     <Compile Include="ApiUsage\NotNullAnalyzerTests.cs" />
@@ -167,4 +168,4 @@
   <Target Name="AfterBuild">
   </Target>
   -->
-</Project>
+</Project>

tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs

@@ -0,0 +1,97 @@
+// analyzer: D2L.CodeStyle.Analyzers.Visibility.CorsHeaderAppenderUsageAnalyzer
+
+using D2L.LP.Web.Cors;
+
+namespace D2L.LP.Web.Cors {
+
+	public interface ICorsHeaderAppender { }
+
+	public class CorsHeaderAppender : ICorsHeaderAppender {
+
+		public CorsHeaderAppender() { }
+
+		public CorsHeaderAppender( int i ) { }
+
+	}
+
+	public interface ISomeOtherSafeInterface { }
+
+	public class SomeOtherSafeClass : ISomeOtherSafeInterface { }
+
+}
+
+namespace D2L.LP.Web.ContentHandling.Handlers {
+
+	public class ContentHttpHandler { // Whitelisted by necessity
+
+		public ContentHttpHandler(
+			ICorsHeaderAppender corsHelper
+		) { }
+
+	}
+
+}
+
+namespace D2L.LP.Web.Files.FileViewing { // Whitelisted by necessity
+
+	public class StreamFileViewerResult {
+
+		public StreamFileViewerResult(
+			ICorsHeaderAppender corsHelper
+		) { }
+
+	}
+
+}
+
+namespace D2L.LP.Web.Files.FileViewing.Default {
+
+	public class StreamFileViewerResultFactory { // Whitelisted by necessity
+
+		public StreamFileViewerResultFactory(
+			ICorsHeaderAppender corsHelper
+		) { }
+
+	}
+
+}
+
+namespace D2L.CodeStyle.Analyzers.CorsHeaderAppenderUsageAnalyzer.Examples {
+
+	public sealed class BadClass {
+
+		public BadClass(
+			/* DangerousUsageOfCorsHeaderAppender */ ICorsHeaderAppender corsHelper /**/
+		) { } 
+
+		public void UsesCorsHelper_ManualInstantiation_NoParams() {
+			ICorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender() /**/;
+		}
+
+		public void UsesCorsHelper_ManualInstantiation_WithParams() {
+			const int i = 10;
+			ICorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender( i ) /**/;
+		}
+
+		public void UsesCorsHelper_ManualInstantiation_OnlyInstantiationTriggersDiagnostic() {
+			CorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender() /**/;
+		}
+	}
+
+	public sealed class GoodClass {
+
+		ISomeOtherSafeInterface m_safeInterface;
+
+		public GoodClass(
+			ISomeOtherSafeInterface safeInterface
+		) {
+			m_safeInterface = safeInterface;
+		}
+
+		public void SafeMethod() {
+			ISomeOtherSafeInterface safeInterface = new SomeOtherSafeClass();
+		}
+
+	}
+
+}