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

Won't Fix alerts are still shown #986

Open
laurentsimon opened this issue Mar 16, 2022 · 7 comments
Open

Won't Fix alerts are still shown #986

laurentsimon opened this issue Mar 16, 2022 · 7 comments

Comments

@laurentsimon
Copy link

I'm one of the maintainers of the scorecard's project and we integrated with the code scanning a few months ago.

One user ossf/scorecard-action#143 reported that the results keeps showing after the file is updated via dependabot, even though the results were marked as Won't Fix

TL;DR: scorecard creates alerts if the actions defined in a GitHub workflows are not pinned by hash ( https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions). Once in a while, dependabot creates PR to update the hash when a new version of an action becomes available. After accepting a dependabot PR, the code scanning shows previous alerts that were tagged with Won't Fix.

My feature request is that the code scanning dashboard should only show alerts for code that has been changed "meaningfully" by developers, rather than simply updated via dependabot and renovatebot for version updates.

@aibaars
Copy link
Collaborator

aibaars commented Mar 16, 2022

Are the alerts reported on a line that was updated by dependabot? If so, the following might explain what's going on.

CodeScanning computes a hash value of the line on which an alert was found and uses this hash as a "fingerprint" of the alert. If a subsequent scan finds an alert with the same fingerprint, then this alert is considered the same as the first. This works really well if the file containing the alert is modified as the fingerprint is stable if code is modified anywhere in the file, except if the line containing the alert is modified. If this happens then CodeScanning considers the previous alert to be "closed", while creating a fresh one in its place. The fresh alert won't be connected to the closed one so things like "won't fix" status are lost. This behaviour is quite reasonable for meaningful changes to the line, however, it is not great for meaningless changes such as adding a comment (or updating a version hash).

@laurentsimon
Copy link
Author

yes this is exactly the behavior. Version updates don't change the code semantic, so one way to improve would simply be to look at previous commit and see if the line was changed by a dependabot/renovatebot PR only. If that's the case, it'd be safe to assume the code has not changed.

Would this be practical? Would you anticipate problems with this approach?

@azeemshaikh38
Copy link

A suggestion - could github/codeql-action/upload-sarif action be expanded to have a exclude option, wherein users can specify ruleIDs they want to ignore? That enables users to completely disable rules/checks they are not interested in.

@azeemshaikh38
Copy link

Or better yet - the UI should have an option to allow users to ignore all failures related to a ruleID.

@laurentsimon
Copy link
Author

cc @josepalafox

@josepalafox
Copy link

Hi @laurentsimon I talked with a few folks and received the following overview and suggestion:

To provide some technical details here, Code Scanning uses the SARIF partialFingerprints property to determine whether an alert is the same as a previously seen alert. (Specifically it uses the fingerprint called primaryLocationLineHash.)

When the CodeQL Action is used to upload SARIF files we check if this property is present, and if it is not we calculate a value based on the hash of the surrounding lines in the file. If you make changes to the line containing the alert, the partial fingerprint will change.

If a tool wishes to override this definition of if an alert is "new" or not, they can do this by providing their own value for the partialFingerprints.primaryLocationLineHash which remains stable when certain properties of the line change.

@laurentsimon
Copy link
Author

Thank you @josepalafox !

I will take a look.

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

No branches or pull requests

4 participants