Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: improve SARIF presentation in GitHub dashboard #1415

Closed
laurentsimon opened this issue Dec 23, 2021 · 2 comments
Closed

Feature: improve SARIF presentation in GitHub dashboard #1415

laurentsimon opened this issue Dec 23, 2021 · 2 comments
Labels

Comments

@laurentsimon
Copy link
Contributor

laurentsimon commented Dec 23, 2021

For each SARIF "rule", we currently use the check name. There are some limitations to this approach:

  1. For example, for the pinned-dependencies check, each result has the title Pinned-Dependencies. There are various "rules"/sub-checks within this check: pinned workflow dependencies, pinned docker file images, pip install, go install, curl | bash, etc.

It may be beneficial to update the title of each results to a more specific name, e.g. Pinned-Dependencies: workflows, Pinned-Dependencies: pip, etc. Today, a user needs to click on each Pinned-Dependencies result to see if it relates to pip, go, workflows, dockerfile, etc

  1. Some checks like branch protection contain several settings. It may be beneficial to have a different result for each setting: this would allow users to decide which they want to fix or not. Currently, if a user clicks "Won't Fix" because they have fixed all settings they are about but don't want to fix, say, reviewCounts > 1 and dismissStaleReview, the results will never appear again in the dashboard even if some setting the users care about are later violated, e.g. force push is re-enabled.

Based on the two examples above, I think it would be beneficial to allow some checks to expose rules/sub-checks as first-class citizen in SARIF. The way we could implement it is to add a list of rules: to the checks.yaml, where a rule would contain a name, ID and one-line description:

CheckName:
 - rules:
   - ForcePush:
     - title: Force push settings
     - short: Enable users to push to ...

When we log using Warn3() function, we can add a RuleID to LogMessage in order to specify a rule. If left empty, it will be ignored. Otherwise, SARIF will use them to create rules and results.

Note1: I envisage the implementation described above as a temporary solution. Once we have separated checks and policy, I think each rule could be mapped to a policy "rule"; and we could use raw results + default policy to generate SARIF, instead of relying on the current non-raw results.

Note2: we currently output individual results for each log message if and only if there is a path that refers to an existing file. This is why the branch protection settings are are not displayed individually. Theoretically, we could change this behavior and display individual results for each detail. However, this approach leads to additional complications:

  1. GitHub's logics differs w.r.t to result fingerprinting (e.g. a bug marked as "Won't Fix" should not affect other results) when there is no existing file to point to. I found a way to mitigate this problem by generating a unique filename for each setting/warn detail
  2. Some checks, e.g. dependency-update-tool, logs warning for both dependabot and renovabot: do we actually want these to appear in different results? I'm tempted to say no, because we only want users to enable one or the other.

I've created a v2 action to test this out, see #1074 (comment) for steps to test. I've also created a PR for these changes #1421. This seems to work, but let's see what everyone thinks. This would address point 2 above. To address point1, we'd still need to add the rules in the checks.yaml and add them to each Warn3().

@laurentsimon
Copy link
Contributor Author

cc @azeemsgoogle

@laurentsimon
Copy link
Contributor Author

chatted offline with @azeemsgoogle We will keep the current results as-is for the next release. Once we have the raw results, we will use them to have more granular "rules".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant