shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Support for SARIF output format

Open arkq opened this issue 1 year ago • 7 comments

Resolves #2405

PS. @koalaman, you've stated in https://github.com/koalaman/shellcheck/issues/2405#issuecomment-991807084 that you don't want support for SARIF. I'm not sure whether you've changed your mind about that. I'd like to use ShellCheck with GitHub actions as a security analysis tool (see this for reference: https://github.com/ludeeus/action-shellcheck/pull/62). I'm not a Haskell programmer at all, so this PR might have some issues regarding Haskell good-practices. Anyway, I'm willing to invest some time to finish this work as long as you will merge the final outcome into master.

arkq avatar Sep 03 '22 13:09 arkq

I've created a simple check for GitHub code scanning based on test script with errors: https://github.com/Arkq/shellcheck/blob/security/.github/workflows/build.yml#L71

Outcome:

Listing

image

Details

image

arkq avatar Sep 03 '22 20:09 arkq

I've quickly read so I may be mistaken, but did you manage rules property ? It's part of SARIF standard:)

nvuillam avatar Sep 12 '22 17:09 nvuillam

I’m not sure are you talking to me directly or is it a mass email ?

Get Outlook for iOShttps://aka.ms/o0ukef


From: Nicolas Vuillamy @.> Sent: Monday, September 12, 2022 11:02:23 AM To: koalaman/shellcheck @.> Cc: Subscribed @.***> Subject: Re: [koalaman/shellcheck] Support for SARIF output format (PR #2569)

I've quickly read so I may be mistaken, but did you manage rules property ? It's part of SARIF standard:)

— Reply to this email directly, view it on GitHubhttps://github.com/koalaman/shellcheck/pull/2569#issuecomment-1244029970, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUW5SQO736DTUQCL6XSMQ2TV55OZ7ANCNFSM6AAAAAAQD4ZRN4. You are receiving this because you are subscribed to this thread.Message ID: @.***>

candacebutler8879 avatar Sep 12 '22 17:09 candacebutler8879

my comment is for @Arkq but please feel free to comment also :)

https://github.com/microsoft/sarif-tutorials/blob/main/samples/1-Introduction/simple-example.sarif

In tools.drivers, there is a rules property with entries corresponding to the ruleIds that wee can find in the results :)

nvuillam avatar Sep 12 '22 17:09 nvuillam

In tools.drivers, there is a rules property with entries corresponding to the ruleIds that wee can find in the results

Yes, you are right. I think that the rules property should (or maybe even shall) be included. However, as shown in the attached screenshots it works (at least on GitHub) without such property - it lacks rule help text though. I can try to add rules to this PR, but I'm not Haskell programmer, so this might not be easy task for me, and I do not want to invest more time in something which at the and might end up in a trash bin. So, firstly I'd like to know whether at the end this feature will get merged into master :)

arkq avatar Sep 12 '22 19:09 arkq

https://sarifweb.azurewebsites.net/Validation

You can try any generated SARIF here :)

nvuillam avatar Sep 12 '22 19:09 nvuillam

The validation passes, but there are some suggestions (one of them is adding rules):

  • runs[0].results[0].message: The 'message' property of this result contains a 'text' property. Consider replacing it with 'id' and 'arguments' properties. This potentially reduces the log file size, allows the message text to be improved without modifying the log file, and enables localization.
  • runs[0]: This run does not provide 'versionControlProvenance'. As a result, it is not possible to determine which version of code was analyzed, nor to map relative paths to their locations within the repository.
  • runs[0].results[0].locations[0].physicalLocation.region: The 'region' object in this result location does not provide a 'snippet' property. Providing a code snippet enables users to see the code that triggered the result, even if they are not enlisted in the code.
  • runs[0].results[0].locations[0].physicalLocation: This result location does not provide a 'contextRegion' property. Providing a context region enables users to see a portion of the code that surrounds the result, even if they are not enlisted in the code.
  • 'runs[0].tool.driver' does not provide a 'rules' property. 'rules' contain information that helps users understand why each rule fires and what the user can do to fix it.

arkq avatar Sep 13 '22 08:09 arkq

Nothing appears to have changed since 2019, so I'm still very skeptical about this. Are there any SARIF consumers outside of the Microsoft sphere? GitHub and VSC already have third party plugins, so the value add of another JSON format specific to those is minimal.

koalaman avatar Oct 20 '22 02:10 koalaman

in MegaLinter we use a rust package for the conversion

https://crates.io/crates/shellcheck-sarif

It would have better performances if it was directly embedded in shellcheck :)

nvuillam avatar Oct 20 '22 08:10 nvuillam

Nothing appears to have changed since 2019

I think that the next release of Clang will contain SARIF output formatter https://clang.llvm.org/doxygen/Sarif_8h_source.html so that's a change for start. SARIF adaptation among open source projects is not fast but it will happen eventually IMHO. The format is indeed bloated, but for a reason. It was designed to be flexible enough to support a large variety of tools. The spec is big, but a particular tool can support only a subset of that. Anyway, I think that the standardization for errors/warnings reporting is a good thing.

However, as a maintainer of few projects, I do agree that adding something (which maintainer does not need) to the project is a though decision, mostly from the maintenance point of view. So, if you feel that this addition might be too much, I will understand that. But personally I think that maintenance impact of this new formatter will not be big because of your plugin-like design for output formatters.

arkq avatar Oct 20 '22 17:10 arkq