Skip to content

Commit

Permalink
[ci] Add DevDiv required Roslyn analyzers, fix errors (#704)
Browse files Browse the repository at this point in the history
As part of building secure software, Microsoft DevDiv has a set of
[Roslyn anaylzers][0] dealing with security that should be run on every
managed assembly.

Adds these analyzers and fix any errors they introduce.


~~ Running Analyzers ~~

In order to run the Roslyn analyzers, the NuGet package
[`Microsoft.CodeAnalysis.FxCopAnalyzers`][1] must be added to each
project.  Rather than do this manually now, and for each new project
in the future, we instead add this to the `Directory.Build.props` file,
which automatically adds it to all projects.

By default, adding the NuGet package runs all included analyzers at
each analyzer's default severity level.  At this time, we are only
concerned with the prescribed security set, so we use
`.editorconfig` to set those analyzers as `error`, and all other
analyzers as `none`.

Projects that wish to opt out of running the analyzers can set 
`<DisableRoslynAnalyzers>True</DisableRoslynAnalyzers>`.

~~ Fixing Errors ~~

The only errors surfaced by these analyzers is
[CA3075: Insecure DTD Processing][2].  These were fixed by using
`new XmlReaderSettings { XmlResolver = null }`, which will not attempt
to resolve and download any DTD files.


~~ Move `NullableAttributes.cs` ~~

`NullableAttributes.cs` is moved to the `src\utils` directory.  

This file was added to `Java.Interop.Tools.JavaCallableWrappers.csproj`
via `..\Java.Interop\`.  However, because the file resided in the
directory containing the strict `.editorconfig` for `Java.Interop.dll`,
it was applying those `.editorconfig` rules to
`Java.Interop.Tools.JavaCallableWrappers.dll`.


Moving it to a neutral directory fixed this.

~~ Other Notes ~~

Updating the `Java.Interop.dll` to the latest analyzer NuGet version
triggered some errors we had handled for that specific assembly, which
likely did not exist in the old analyzers and thus were not being
surfaced as errors.  They do not appear to be rules that we are
actually concerned with, so they were disabled:

  * CA1021 - Don't use out parameters
  * CA1045 - Don't use reference parameters
  * CA1822 - Mark methods static if they don't reference instance members
  * CA1002 - Don't expose generic Lists

[0]: https://github.com/dotnet/roslyn-analyzers
[1]: https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers/
[2]: https://docs.microsoft.com/en-us/visualstudio/code-quality/ca3075?view=vs-2019
  • Loading branch information
jpobst committed Sep 8, 2020
1 parent a98c1ae commit ac914ce
Show file tree
Hide file tree
Showing 9 changed files with 315 additions and 13 deletions.
301 changes: 301 additions & 0 deletions .editorconfig

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,13 @@
<_RunJNIEnvGen Condition=" '$(JIBuildingForNetCoreApp)' == 'True' ">dotnet "$(_JNIEnvGenPath)"</_RunJNIEnvGen>
<_RunJNIEnvGen Condition=" '$(JIBuildingForNetCoreApp)' != 'True' ">$(Runtime) "$(_JNIEnvGenPath)"</_RunJNIEnvGen>
</PropertyGroup>

<!-- Add Roslyn analyzers NuGet to all projects -->
<ItemGroup Condition=" '$(DisableRoslynAnalyzers)' != 'True' ">
<PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="3.3.0">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
<Compile Include="..\Java.Interop.Tools.TypeNameMappings\Java.Interop.Tools.TypeNameMappings\JavaNativeTypeManager.cs">
<Link>JavaNativeTypeManager.cs</Link>
</Compile>
<Compile Include="..\Java.Interop\NullableAttributes.cs">
<Compile Include="..\utils\NullableAttributes.cs">
<Link>NullableAttributes.cs</Link>
</Compile>
</ItemGroup>
Expand Down
4 changes: 0 additions & 4 deletions src/Java.Interop/.editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,12 @@ dotnet_diagnostic.CA2213.severity = error
dotnet_diagnostic.CA2242.severity = error
dotnet_diagnostic.CA2000.severity = error
dotnet_diagnostic.CA2220.severity = error
dotnet_diagnostic.CA1822.severity = error
dotnet_diagnostic.CA2241.severity = error
dotnet_diagnostic.CA1012.severity = error
dotnet_diagnostic.CA1019.severity = error
dotnet_diagnostic.CA1040.severity = error
dotnet_diagnostic.CA1023.severity = error
dotnet_diagnostic.CA1044.severity = error
dotnet_diagnostic.CA1021.severity = error
dotnet_diagnostic.CA1045.severity = error
dotnet_diagnostic.CA1020.severity = error
dotnet_diagnostic.CA1051.severity = error
dotnet_diagnostic.CA1034.severity = error
Expand Down Expand Up @@ -67,7 +64,6 @@ dotnet_diagnostic.CA1027.severity = error
dotnet_diagnostic.CA1005.severity = error
dotnet_diagnostic.CA1004.severity = error
dotnet_diagnostic.CA1000.severity = error
dotnet_diagnostic.CA1002.severity = error
dotnet_diagnostic.CA1006.severity = error
dotnet_diagnostic.CA1010.severity = error
dotnet_diagnostic.CA1007.severity = error
Expand Down
6 changes: 1 addition & 5 deletions src/Java.Interop/Java.Interop.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<DefineConstants>DEBUG;$(DefineConstants)</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Compile Condition=" '$(TargetFramework)' != 'netstandard2.0' " Remove="NullableAttributes.cs" />
<Compile Condition=" '$(TargetFramework)' == 'netstandard2.0' " Include="..\utils\NullableAttributes.cs" />
<Compile Remove="Java.Interop\JniLocationException.cs" />
</ItemGroup>
<PropertyGroup>
Expand Down Expand Up @@ -49,10 +49,6 @@
<None Include="Documentation\Java.Interop\IJavaPeerable.xml" />
<None Include="Documentation\Java.Interop\JniManagedPeerStates.xml" />
<None Include="Documentation\Java.Interop\JniEnvironment.References.xml" />
<PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.9.7">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<ProjectReference Include="..\..\build-tools\jnienv-gen\jnienv-gen.csproj"
ReferenceOutputAssembly="false"
/>
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public class JavaApiTestHelper
public static JavaApi GetLoadedApi ()
{
var api = new JavaApi ();
using (var xr = XmlReader.Create (ApiPath))
using (var xr = XmlReader.Create (ApiPath, new XmlReaderSettings { XmlResolver = null }))
api.Load (xr, false);
return api;
}
Expand Down
2 changes: 1 addition & 1 deletion tools/generator/ApiVersionsProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public class ApiVersionsProvider
{
public void Parse (string apiVersionsFilePath)
{
using (var reader = XmlReader.Create (apiVersionsFilePath))
using (var reader = XmlReader.Create (apiVersionsFilePath, new XmlReaderSettings { XmlResolver = null }))
Parse (reader);
}

Expand Down
2 changes: 1 addition & 1 deletion tools/generator/CodeGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolve
string apiXmlFile = filename;
string apiSourceAttr = null;

using (var xr = XmlReader.Create (filename)) {
using (var xr = XmlReader.Create (filename, new XmlReaderSettings { XmlResolver = null })) {
xr.MoveToContent ();
apiSourceAttr = xr.GetAttribute ("api-source");
}
Expand Down

0 comments on commit ac914ce

Please sign in to comment.