Clarify behavior of `properties.security-severity` in SARIF files

[jsoref]

trafficstars

Code of Conduct

• [X] I have read and agree to the GitHub Docs project's Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#reportingdescriptor-object

What part(s) of the article would you like to see updated?

Docs change: update the description for properties.security-severity to make it clear that it should only be used for Security rules, where its use is recommended. If you include a value for this field, the query is treated as a security query.

Additional information

https://github.com/community/community/discussions/34137#discussioncomment-3822930

[cmwilson21]

👋 @jsoref - Thanks so much for opening an issue! I'll triage this for the team to take a look :eyes:

[github-actions[bot]]

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert :eyes:

[felicitymay]

Hi @jsoref - I had a quick check in with the code scanning team. As you've noticed in the documentation, are three different properties that feed into how an alert is displayed in GitHub's code scanning interface:

• properties.precision which describes how accurate the detection is. This information is not displayed in the UI but is used to help sort alerts with the most urgent first.
• properties.problem.severity which is used to classify non-security queries into error, warning or recommendation
• properties.security-severity which describes the severity for security queries.

I notice that in the documentation we recommend that you set all three properties, but it seems as if you should only set a properties.security-severity for security queries to avoid the query being presented as a security alert. I think we should update the documentation to make it clear that the field is only recommended for security queries, and what happens if you use the field for other query types.

In one of the action links that you shared, I can see an example which sets a security severity for a non-security query. I suspect that the UI will behave as expected if you remove this line from the SARIF file:

        "rules": [
          {
            "id": "unrecognized-spelling",
            "name": "UnrecognizedSpelling",
            "helpUri": "https://github.com/check-spelling/check-spelling/wiki/Event-descriptions#unrecognized-spelling",
            "shortDescription": {
              "text": "Unrecognized Spelling"
            },
            "fullDescription": {
              "text": "Token is neither in the dictionary nor expected"
            },
            "help": {
              "text": "?",
              "markdown": "**Remediation (click \"Show more\" below)**:\n\n- Correct spelling or add to expect.txt\n\n"
            },
            "defaultConfiguration": {
              "level": "error"
            },
            "properties": {
              "precision": "high",
              "problem.severity": "error",
              "security-severity": "7.0",
              "tags": [
                "source-code",
                "code-reviews"
              ]
            }
          },

---

I intend to amend the issue title and docs change as follows:

• Title: Clarify behavior of properties.security-severity in SARIF files
• Docs change: update the description for properties.security-severity to make it clear that it should only be used for Security queries, where its use is recommended. If you include a value for this field, the query is treated as a security query.

[felicitymay]

Hi @jsoref - thanks for making the changes to the issue summary - much appreciated ✨

I'll drop help-wanted and good-first-issue labels on the issue so people know that it's ready to work on.

[saadbitcoin]

This is good idea

[jsoref]

Name	Description
defaultConfiguration.level	Optional. Default severity level of the rule. Code scanning uses severity levels to help you understand how critical the result is for a given rule. This value can be overridden by the level attribute in the result object. For more information, see the result object. Default: warning.
properties.problem.severity	Recommended. A string that indicates the level of severity of any alerts generated by a non-security query. This, with the properties.precision property, determines whether the results are displayed by default on GitHub so that the results with the highest problem.severity, and highest precision are shown first. Can be one of: error, warning, or recommendation.

Note that defaultConfiguration.level doesn't list possible values...

So,

I have errors: https://github.com/check-spelling/duckdb/security/code-scanning?query=is%3Aopen+branch%3Aspell-check-with-spelling+severity%3Aerror https://github.com/check-spelling/check-spelling/blob/6d79bdfc8911544667104e5d2af5014d370360e6/sarif.json#L33

I have warnings: https://github.com/check-spelling/duckdb/security/code-scanning?query=is%3Aopen+branch%3Aspell-check-with-spelling+severity%3Awarning https://github.com/check-spelling/check-spelling/blob/6d79bdfc8911544667104e5d2af5014d370360e6/sarif.json#L111

The filter offers Note:

I've added recommendation per the docs, I'll have to see https://github.com/check-spelling/check-spelling/blob/6d79bdfc8911544667104e5d2af5014d370360e6/sarif.json#L85

https://github.com/check-spelling/duckdb/actions/runs/3235608726/jobs/5300277013#step:2:939

            {
              "id": "candidate-pattern",
              "name": "CandidatePattern",
              "helpUri": "https://github.com/check-spelling/check-spelling/wiki/Event-descriptions#candidate-pattern",
              "shortDescription": {
                "text": "Candidate Pattern"
              },
              "fullDescription": {
                "text": "A line with unrecognized tokens matches a pattern. Patterns enable the tool to ignore content."
              },
              "help": {
                "text": "?",
                "markdown": "**Remediation (click \"Show more\" below)**:\n\n- Add the pattern to [patterns](https://github.com/check-spelling/check-spelling/wiki/Configuration#patterns)\n- Remove the pattern from candidates.patterns\n\n\n"
              },
              "defaultConfiguration": {
                "level": "warning"
              },
              "properties": {
                "precision": "medium",
                "problem.severity": "recommendation",
                "tags": [
                  "source-code",
                  "code-reviews"
                ]
              }

...

        {
          "ruleId": "candidate-pattern",
          "ruleIndex": 0,
          "message": {
            "text": "Matches candidate pattern [\\(data:[^)]*?(?:[A-Z]{3,}|[A-Z][a-z]{2,}|[a-z]{3,})[^)]*\\)](#security-tab) (candidate-pattern)"
          },
          "locations": [
            {
              "physicalLocation": {
                "artifactLocation": {
                  "uri": "tools/juliapkg/src/table_function.jl",
                  "uriBaseId": "%SRCROOT%"
                },
                "region": {
                  "startLine": 64,
                  "startColumn": 29,
                  "endColumn": 47
                }
              }
            }
          ]
        },

https://github.com/check-spelling/duckdb/security/code-scanning?query=is%3Aopen+branch%3Atest+rule%3Acandidate-pattern

But, as you can see, it's a warning (not a recommendation and not a note):

[felicitymay]

@jsoref - I'm not an expert on this, but I would imagine that a recommendation in the SARIF file would map to a note in the UI. Have you tried omitting the defaultConfiguration.level property?

[jsoref]

I haven't tried but the docs clearly say that warning is the default for that property, so I don't expect it to change anything, although it might, because, it isn't like the docs are reliable :)

[felicitymay]

@jsoref - apologies for the delay in getting back to you on this.

Looking back through this issue, I'm summarizing the results of our various discussions.

Recommendations:

• Update the description for properties.security-severity to include:

  (Recommended only for security queries) If you include a value for this field, the query is treated as a security query.

• Update the description for defaultConfiguration.level to include the valid values of: note, warning, error and be clear that you can set an alert to override the default level by setting the level attribute in the result object.
• Expand the Understanding rules and results section to explain how severity, precision, level, and security-severity all impact how a result is shown in GitHub.
• Review use of the term "severity" within the descriptions to ensure that it's only used where the term is accurate.

[felicitymay]

Since Hacktoberfest is fast approaching, I'm going to take the following actions:

1. Create a new help-wanted issue to address the first two bullets in the comment above. This will be easier for someone to pick up than this issue.
2. Open an internal issue for the last two bullets since this is more complex work that requires collaboration.
3. Close this issue.

[felicitymay]

• https://github.com/github/docs/issues/28305 created
• Internal tracking issue also created.

[Roy-Hillel-Mend]

Hi, when working with the "security-severity" field, encountered a problem where when the value is "0" or "0.0", github security shows it as "Error" and not "Low" as expected. Is this a known problem? Is there a way to show "Low" when the rules properties > security-severity is "0.0" or "0"?

Thanks, Roy.

[nguyenalex836]

Hi @Roy-Hillel-Mend 👋 !

That question would be best handled by our exceptional support team. Please reach out to them, and they'll be happy to assist 💛

If there's anything in the documents that you have suggestions for updating, please feel free to open an issue ✨