microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
193 stars 91 forks source link

SARIF1010.RuleIdMustBeConsistent falsely claims ruleId property is absent #2799

Open dvitek opened 7 months ago

dvitek commented 7 months ago

Here is a small sarif document with a whitespace rule id that would cause SARIF1010.RuleIdMustBeConsistent to erroneously claim that the ruleId property is absent:

1.sarif(17,17): error SARIF1010: runs[0].results[0]: This result contains neither of the properties 'ruleId' or 'rule.id'. The SARIF specification (§3.27.5) requires at least one of these properties to be present.

{
    "version": "2.1.0",
    "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
    "runs": [
        {
            "tool": {
                "driver": {
                    "name": "codeskanner",
                    "rules": [
                        {
                            "id": " "
                        }
                    ]
                }
            },
            "results": [
                {
                    "ruleId": " ",
                    "level": "warning",
                    "message" : {"text": "hello"}
                }]
        }
    ]
}

Specifically, according to the SARIF standard cited by the diagnostic (https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317643), empty string or whitespace only rule IDs are legal. Regardless of whether they are legal, the ruleId property is present.

While it usually would not be desirable to have such a silly ruleId (a space), we like to stress test our static analysis framework using a fuzzer. The fuzzer is good at exercising corner cases like this. We would like to be able to validate that the resulting sarif is standard-compliant, even if it is idiotic. This is the only false positive we are currently seeing from the validator.

SARIF1010.RuleIdMustBeConsistent.cs uses string.IsNullOrWhiteSpace to determine whether a ruleId property is absent.
https://github.com/microsoft/sarif-sdk/blob/d6d80621aa032c80bec392f9e925b7e7799ca671/src/Sarif.Multitool.Library/Rules/SARIF1010.RuleIdMustBeConsistent.cs#L41

If SARIF1010.RuleIdMustBeConsistent is intending to enforce the SARIF standard, then I do not think string.IsNullOrWhiteSpace is the correct predicate. I rather suspect == null would be correct.