Support ShellCheck SARIF rule metadata

[lzaoral]

This is a proof-of-concept at the moment.

Fixes #54.

[lzaoral]

@jamacku, Can you please check that the version of csgrep from this branch creates correct help links for ShellCheck reports on GitHub? You can get the RPM from Packit. Thank you!

[jamacku]

@lzaoral sure, I'll have a look. Thanks

[jamacku]

Hmm, from what I saw help paragraphs are still missing. I'm not sure where could be a problem.

[lzaoral]

@jamacku, Could please try it again with the latest changes? Note that this PR is just a proof-of-concept. I'm not really happy with the current implementation.

[jamacku]

@lzaoral It's unfortunate, but it still doesn't show a hint message in GitHub UI.

[jamacku]

@lzaoral I came across this repo with SARIF examples: https://github.com/microsoft/sarif-tutorials

It seems like a good source of information if it's not outdated.

https://github.com/microsoft/sarif-tutorials/blob/main/docs/2-Basics.md#rule-metadata

[lzaoral]

@jamacku, could you try it once again, please?

I've added the fullDescription key to the rule objects. This key is mandatory accroding to the SARIF 2.1 standard. [1] Note, that I've just copied an event message to this key for testing purposes, it will contain description that's not really general, e.g. "COMMIT_TO_IM appears unused. Verify it or export it.".

[1] https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317838

[jamacku]

@lzaoral It WORKS 🥳

But I'm not sure if it displays a correct message.

[lzaoral]

@jamacku, Could you try it once more, please? This time with much cleaner and simpler code. Thanks in advance!

[jamacku]

@lzaoral Please see:

• test csdiff jamacku/differential-shellcheck#29

It works great! 🥳 I'm thinking if it would make sense to say something like:

Defect reference: https://github.com/koalaman/shellcheck/wiki/SC2086

Or maybe even try to take advantage of supported markdown templating and generate something like:

Defect reference: [SC2086](https://github.com/koalaman/shellcheck/wiki/SC2086)

[jamacku]

@lzaoral Works great 💯

Markdown templating:

Defect searching by rule:

[lzaoral]

This PR should be finally ready for a proper review. 🥳

[jamacku]

It works like a charm! LGTM 👍

• [DEBUG]: test csdiff #68 redhat-plumbers-in-action/differential-shellcheck#117

[kdudka]

Thank you for moving it forward! Could we please extend the SARIF writer such that CWE IDs are preserved when available even for ShellCheck? We have our own mapping of SCNNNN to CWE-NNN in csmock: https://github.com/csutils/csmock/blob/0d96eeabbc064f828c35fbea95ebd3616295b8b7/cwe-map.csv#L425 I would like to avoid losing this data when we convert csmock's output to SARIF.

[lzaoral]

Unfortunately, SARIF allows at most one rule per defect. Will be something like this enough?

"properties": {
    "cwe": [
        "CWE-248"
    ]
    "tags": [
        "ShellCheck"
    ]
}

[kdudka]

SARIF's rule is def.checker + ": " + keyEvt.event in the current output of csgrep --mode=sarif. So assigning at most one rule to each defect is fine and we do not need to change it. On the other hand, we can assign many properties to each rule. I am not sure about the help text. If there is just one help text field for each rule, we can probably concatenate the text and provide both the links?

[lzaoral]

Yes, concatenating the help texts is possible.

[lzaoral]

I've updated the PR with changes addressing concerns raised in #68 (review).

[kdudka]

Thanks for the update! I think it works as expected now.

The implementation could be optimized for future extensions. I would create a structure RuleProps and a single map from ruleId to RuleProps, in order to make adding new properties in the future more straightforward. But this can be refactored later on when we actually need it.

[jamacku]

Hmm, it doesn't seem to work 😿

Tested in:

• test: csdiff PR #68 redhat-plumbers-in-action/differential-shellcheck#204

Reported defect is missing metadata:

[jamacku]

When the defect was already detected before it won't have new metadata assigned to it, but when it is a fresh defect, it works as expected. It seems like a bug or implementation detail on the GitHub side.

LGTM 💪

[lzaoral]

Thanks for the reviews!