CodeQL is missing an inline mechanism to suppress warnings

[bryevdv]

This was asked about in #9298 and the issue log stated "closed as not planned". The "answer" was to dismiss via the UI. But this alone is really not a sufficient mechanism:

• Having to interact with the UI is supremely annoying
• Spurious new warnings if the code moves cannot be avoided this way
• It is good to be forced to comment these exception inline where they happen
• What if you are running codeql locally? Then here is no mechanisim?

Please consider supporting this basic, table-stakes feature that many, many linting/scanning tools afford.

[sj]

Hi @bryevdv! Thanks for taking the time to give us feedback on CodeQL. We're constantly considering how we can improve GitHub code scanning and CodeQL.

Having to interact with the UI is supremely annoying

I completely understand where you're coming from on this one, and it's a topic that is often debated in the team — many different opinions! We've also asked users whether they'd prefer to dismiss alerts in the UX, or whether they'd prefer to do that in-line in the code. The former turned out to be more popular. But you're certainly not the only one who would have preferred the latter!

Two other reasons that led us to choose for UX-based dismissal:

• It means that in large organisations, security teams (who often don't have commit access to a codebase) can still help triage alerts and thereby reduce the number of alerts that developers need to deal with.
• It makes it a lot easier for the CodeQL team to improve the analysis based on the FP reports from users (and the messages they include in that). We keep an eye on how different CodeQL analyses (for different types of vulnerabilities) behave; if a particular category of results is ignored more than others, we take a closer look.

Maintaining two approaches for dismissing alerts (in-code + UX) can lead to significant confusion. However, this is certainly something we will continue thinking about.

Spurious new warnings if the code moves cannot be avoided this way

When code moves, the context of an alert changes too. For example, what was previously a non-exploitable SQL injection might suddenly be exploitable. Due the high precision of the CodeQL analysis, we don't hear very often that users are frustrating that we flag up the same alert again. If/when an alert re-appears, it's therefore often worth a quick look and double-check!

(Note that code scanning does a pretty good job at not flagging up the same alert again if there are small, unrelated changes to the code!)

What if you are running codeql locally? Then here is no mechanisim?

Many people use CodeQL in so many different ways! I'd love to hear more about how you use CodeQL locally, so we can see whether we can improve how the tool works for your particular workflow.

Please consider supporting this basic, table-stakes feature that many, many linting/scanning tools afford.

I hope I've been able to convince you that a lot of thought and consideration has gone into code scanning and CodeQL. The CodeQL engine and GitHub code scanning are carefully fine-tuned, and we aim to give our users much more precise alerts and a better user experience than most linters do.

Cheeky side-note 😛 — if you have ideas on how to improve the quality of our analysis further, please consider making a contribution to our open source codebase, or consider joining the team! Examples of current openings 1, 2, 3; full list here.

Thanks again for taking the time to share your thoughts — much appreciated. We'll definitely keep your suggestion in mind for future consideration!

[bryevdv]

HI @sj thanks for the reply

reasons that led us to choose for UX-based dismissal

Well, I used to work for Microsoft. I know that the primary reason is the desire to collect telemetry from the web UI. 🙂

security teams (who often don't have commit access to a codebase)

I appreciate a web UX might be useful for that situation, but I also submit that many/most OSS projects are not in that situation, and that overworked, oversubscribed OSS maintainers would really just prefer to streamline their process. Being able to leave one comment, once, inline, in one place, is the most economical option under that lens.

I'd love to hear more about how you use CodeQL locally

I'm not sure what there is to add. GH publishes instructions for using the CodeQL CLI. I (and I am sure others) do run CodeQL that way sometime. Perhaps I can turn the question around: if someone is using the CodeQL CLI, what is the accepted mechanism for them to ignore specific warnings? If there is not one, is that not an important feature omission?

Maintaining two approaches for dismissing alerts (in-code + UX) can lead to significant confusion.

Is there evidence for this claim? It's not personally confusing to me, at all. LGTM offered a line exclusion comment, can you speak to specific data collected at Semmle that demonstrated significant confusion when it was available? When I worked in MS DevDiv there were often controlled usability studies conducted on APIs, are you saying this specific question has been examined in proper usability studies?

And even if so: if having both options at once is the problem, the choice for one or the other method could a mutually exclusive global configuration for the repo.

[ckreibich]

Lack of support for in-code alert management is just bizarre to me. The code is the context — not your UI, not config files, not workflows. Combine this with the currently poor control over which files/folders to include/exclude in compiled code and it's just unnecessarily hard to configure CodeQL.

[amotl]

Same here, we are observing #11407 and #11408 on our code base, which are effectively false positives, and would love to use an inline mechanism to suppress the corresponding warnings. Just to list a few examples:

• Support computed columns in SQLAlchemy ORM crate/crate-python#474 (review)
• SQLAlchemy/DDL: Allow to turn off column store crate/crate-python#555 (review)

As far as we understand, CodeQL does not feature inline suppression comments/instructions, like what LGTM did with lgtm[py/import-and-import-from], right? (crate/crate-python@4397cc2e7)

The code is the context — not your UI.

👍

[sj]

Thanks for adding your voice to the discussion, everyone 🙇 . We'll definitely take this feedback into account!

[DarrenKipu]

I am honestly shocked that there is no inlining mechanism to suppress false positives. Or at least a way to identify them in a configuration file of some sort...

I just re-re-read through all the documentation again thinking I must have somehow missed it...or that since Ruby is a new kid on the block it hadn't been implemented yet...but nope.

Glad that I (finally) thought to come and look here...

[scotthain]

Is there a solution planned for this or has anyone found a workaround? Almost every other scanning and linting tool I have used in the past 5+ years has inline 'exception' capabilities. I understand the arguments that @sj is making here but this feels like an prescriptive solution that makes this untenable in the long run for some folks like myself.

[hannes-ucsc]

The key benefit of inline suppressions is that the reason and intent for the suppression is recorded in the most prominent place: at the site of the warning. Once a PR is merged, no one is going to look up the PR that introduced a CodeQL warning and dig out the dismissal comment. People read source code, not merged PRs.

As others have said, every other tool for static code analysis has this feature. I'd say the burden of proof lies with those that claim this feature is not needed.

[hoonto]

Not going to use it until it supports this.

[randombit]

I think my favorite part about this is that LGTM supported suppressing issues using a comment in the code - so someone made the deliberate decision to remove that feature after Semmle was acquired by Github.

[aibaars]

The advanced-security/dismiss-alerts Action implements support for inline suppression comments and automatically dismisses alerts in GitHub Code Scanning.

[amotl]

Dear Arthur,

thank you very much for telling us about advanced-security/dismiss-alerts. Based on what we reported at #11427 (comment), how we used lgtm-based suppression comments in the past, the documentation at getting started with dismiss-alerts,

A user can provide their own custom alert-suppression query, or use the ones that we provide (//lgtm or //codeql style comments).

and, referring to crate/crate-python@4397cc2e7, would that be a correct way to annotate code correspondingly today, in this case for Python?

class DummyTable(self.Base):  # codeql[py/unused-local-variable]
    pass

With kind regards,
Andreas.

[aibaars]

@amotl That would indeed be the right way to annotate code if you run the default AlertSuppression.ql query in addition to the normal CodeQL rules. The legacy #lgtm comments should also work, however, I see you removed them already from your code.

[mhuijgen]

@aibaars Although advanced-security/dismiss-alert indeed does the trick, the action only recommends to run it on push to the default branch. This makes any new PR, even though annotated with suppression on new defects, still fail for us, since the codeql check on the PR blatantly ignores the suppression in the uploaded sarif file.

We checked the sarif file before it gets uploaded by our workflow using the github/codeql-action/upload-sarif@v2 action, and it does contain the suppressions that AlertSuppression.ql sets (although we use the java one)

      "suppressions" : [ {
        "kind" : "inSource"
      } ]

However the codeql check on the PR seems to just ignore the suppression and makes the check fail and thus the PR cannot be merged and thus the dismiss-alerts action also does not get run.

Is there any way to make the codeql check on a PR pass if the defects it finds have these suppressions?

[aibaars]

You're right. Pull requests are a bit special, on the one hand the alert should be visible to allow a code reviewer to assess the whether they agree with suppressing the alert. On the other hand the alert should ideally not be counted for the check pass/fail judgement.

You can actually run the advanced-security/dismiss on pull requests, and this would work fine for alerts that are introduced in the pull request. If you add a suppression comment to the pull request then the behaviour is what you'd expect. However, for a pull request that suppresses an alert that is already in the default branch the behaviour is not great. At first the alert will be dismissed automatically both on the default branch and on the pull request even before the pull request is merged. If there is a subsequent analysis run on the default branch then the alert will be re-opened on the pull request and the default branch. You get this annoying flip-flopping behaviour until the pull request is finally merged. This is pretty confusing and that's why we recommend running on the default branch only.

I think the best solution would be to apply the advanced-security/dismiss only on the main branch, and configure CodeQL analysis as a non-required check. This way a pull request can still be merged even if the CodeQL check "failed". I would leave if up to a human reviewer to decide whether the failed CodeQL run is acceptable (because the new alerts are either suppressed or otherwise fine). The CodeQL check shouldn't fail very often, and when it does it hopefully makes the reviewer aware and double check the alerts before approving/merging the pull request.

[mhuijgen]

This still sounds like a workaround to me though. For PR cases I would expect the defects to either not show up on the PR because they are suppressed, or that they show up as suppressed and the codeql check still passes.

And on merge to the default branch the dismiss action can then properly mark them as suppressed on the advanced security codeql defect tab.

But this is probably not a trivial change to how CodeQL integrates in GitHub and requires more thought perhaps. But compared to other tools this is really a missing feature in GH/CodeQL integration.

[willyt150]

Similar to how we utilize linting, we'd like to be able to run the CodeQL CLI analysis locally if we want to try and catch/deal with alerts before creating our PRs.

However, all the alerts we have chosen to dismiss in the UI for the various reasons (unit test, false positive, intentional, etc...) show up again when running CodeQL CLI locally. We use both GitHub and Azure DevOps GitHub Advanced Security (older projects are still on Azure DevOps Git).

Is there a way to honor the dismissed alerts from Azure DevOps and GitHub? Otherwise it makes utilizing CodeQL CLI analysis locally difficult with having to know which alerts showing locally have already been addressed or not in the cloud.

[ruslanss]

Hi, @aibaars , is there a built-in Azure Pipelines yaml task for the advanced-security/dismiss action? I'm unable to find it in the docs.

[aibaars]

Hi, @aibaars , is there a built-in Azure Pipelines yaml task for the advanced-security/dismiss action? I'm unable to find it in the docs.

@perelman-g I'm afraid there isn't. Though it shouldn't be hard to port the advanced-security/dismiss action to an Azure Pipelines task. It's a fairly small TypeScript program https://github.com/advanced-security/dismiss-alerts/blob/main/src/main.ts

[cstich-otto]

I can also just support this issue. It is really weird that CodeQL has no suppor for inline code suppressions...
Almost every other code scanning tool has it, and for good reasons...

[ThrawnCA]

When running CodeQL via GitHub Actions, there is no UI option for dismissing false positives. There really ought to be a way to mark code to say, "The detector is wrong; this isn't a vulnerability."

For example, we log usernames on failed logins, which is a perfectly normal thing to do, but CodeQL mistakenly thinks we're logging a password (the field name is 'login'). CodeQL is wrong in this case, but we have no way to tell the workflow that.

[jsoref]

Unless I'm missing something, this means that each fork will have to perform the same analysis and dismiss the same code.

We're seeing people run codeql against a fork (I can't tell if it's disconnected, my guess is it is). They then file low grade issues based on their codeql reporting, and then file even lower grade PRs based on their filed issues.

Dealing with this is fairly expensive for high visibility projects with limited dev/triage-power.

I understand the risk of having stale or poorly reasoned flags. But I'm not sure that this balance is right.

In general, any time someone adds a suppression in code, it's an invitation for everyone else to second guess those suppressions, and my bet is that in general people are quite eager to do so. The difference is that the people likely to do so will tend to be the ones with a better understanding of the tooling, and thus more likely to file good bugs.