microsoft / DevSkim

DevSkim is a set of IDE plugins, language analyzers, and rules that provide security "linting" capabilities.
MIT License
912 stars 116 forks source link

Add support as Roslyn Analyzer #619

Open rjmurillo opened 5 months ago

rjmurillo commented 5 months ago

Is your feature request related to a problem? Please describe. The experience is clunky when using DevSkim. I either use the command line and output a SARIF file, then use a SARIF viewer, or use a Visual Studio or VS Code extension to integrate. This does not help me because I use JetBrains Rider.

Describe the solution you'd like What would be great if the analyzers show up as Roslyn Code analyzer so it works when I just run dotnet build without all the extra steps.

Describe alternatives you've considered

Additional context https://github.com/rjmurillo/moq.analyzers/pull/83

gfs commented 5 months ago

Thanks for the feedback and suggestion. The GitHub Action is aimed to close the gap here for users who either don't prefer the IDE extension or who use an editor that we don't currently support.

That said, this is an interesting request and sounds like it could be a cool integration. However, I'm not sure about the feasibility of packaging DevSkim as a Roslyn analyzer. I don't have a lot of experience with analyzers, but from the documentation I found at first on authoring roslyn analyzers (https://devblogs.microsoft.com/dotnet/how-to-write-a-roslyn-analyzer/) it looks like analyzers are defined by registering call back methods when specific AST nodes are encountered but DevSkim is primarily regular expression based, so it doesn't look like a clean repackaging at first blush.

If you have some domain specific knowledge here that I'm missing that means that the repackage is a simpler task than it seems, I'm certainly open to the idea.

rjmurillo commented 5 months ago

Architecturally it could work (we have rules that are RegEx based in the Moq Analyzers project). The difference is understanding more about which symbols are applicable. So it's not a context-less scan of code. There are cases defined in your must-match that are narrowing enough, for example. It also can avoid false positives.

For example: we use NerdBank Git Version which stamps the git commit as an assembly property.

internal static partial class ThisAssembly {
    internal const string GitCommitId = "9429e11d88c89cc1ecd460f03f8003ba2cca1b53";
}

This gets flagged incorrectly as a DS173237 because it's hex based and 30 char or more

gfs commented 5 months ago

I'll think on this a bit more. It seems like it might be able to slot in like the structured data queries feature, narrowed down by a new field for the roslyn class to trigger on.