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

CodeQL alert suppression #11723

Merged
merged 5 commits into from Dec 21, 2022
Merged

CodeQL alert suppression #11723

merged 5 commits into from Dec 21, 2022

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Dec 16, 2022

This pull request changes the alert suppression queries to accept:

  • the legacy lgtm and lgtm[query-id] comments on the line before an alert
  • the new comment codeql[query-id] on the line before an alert

Note that the legacy lgtm and lgtm[query-id] comments are still accepted on the same line as an alert, but the new codeql[query-id] comments are not.

javascript/ql/src/AlertSuppression.ql Fixed Show fixed Hide fixed
@aibaars aibaars force-pushed the alert-suppression branch 2 times, most recently from 81db2ad to e792222 Compare Dec 16, 2022
@github-actions github-actions bot added the Swift label Dec 16, 2022
@aibaars aibaars force-pushed the alert-suppression branch 8 times, most recently from ef74927 to bce2b47 Compare Dec 19, 2022
@aibaars aibaars marked this pull request as ready for review Dec 19, 2022
@aibaars aibaars requested review from a team as code owners Dec 19, 2022
@aibaars aibaars changed the title Alert suppression CodeQL alert suppression Dec 19, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

I still think it's natural to support "end-of-line-suppression-comments".
But I suppose product has weighed in on this?

The implementations seems fine.
The expected output is hard to read given that the old comments now support "line above" suppression.

I just got two comments for now.


CodeQlSuppressionComment() {
// match `codeql[...]` anywhere in the comment
annotation = this.(Comment).getText().regexpFind("(?i)\\bcodeql\\s*\\[[^\\]]*\\]", _, _) and
Copy link
Contributor

@erik-krogh erik-krogh Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
annotation = this.(Comment).getText().regexpFind("(?i)\\bcodeql\\s*\\[[^\\]]*\\]", _, _) and
annotation = this.getText().regexpFind("(?i)\\bcodeql\\s*\\[[^\\]]*\\]", _, _) and

Redundant cast?

Copy link
Contributor Author

@aibaars aibaars Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked redundant to me too, but the results are empty without it. @alexet do you know why that is?

@aibaars aibaars merged commit 98c5b81 into github:main Dec 21, 2022
35 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants