Skip to content

C++: Combine results for cpp/weak-cryptographic-algorithm #6007

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

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 4, 2021

The issue here is that cpp/weak-cryptographic-algorithm often reports many pieces of evidence of the same issue, that a particular cryptographic algorithm is in use (see https://lgtm.com/rules/2162200683/alerts/, though that is probably still showing an older version of the query). The noise of all these extra results is particularly unpleasant when viewing them alongside other query results.

The solution in this PR is to combine all alerts in a file into a single alert at the first location, with $@ references to the other results. The results look much cleaner on LGTM.

@geoffw0 geoffw0 added the C++ label Jun 4, 2021
@geoffw0 geoffw0 requested a review from a team as a code owner June 4, 2021 09:27
MathiasVP
MathiasVP previously approved these changes Jun 4, 2021
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! (assuming the tests pass.)

Do we need to write another change-note given that we already have one for this query from #5896?

@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Jun 4, 2021
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 4, 2021

Do we need to write another change-note given that we already have one for this query from #5896?

I don't think so. There may (hopefully!) at some point be a third PR increasing the @precision of this query to high, at which point we'll want a second change note I think.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 4, 2021

LGTM! (assuming the tests pass.)

Looks like I forgot to make a PR for the internal test as well. I'll do that later today.

@geoffw0 geoffw0 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jun 4, 2021
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 4, 2021

Internal changes made. This should be ready to merge (together with the other PR) when the tests pass on that one.

@MathiasVP MathiasVP merged commit f21e949 into github:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants