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.