Why does dismissing a CodeQL alert trigger a postback?

[chris-jenkinson]

Select Topic Area

Product Feedback

Body

It's incredibly common in a pull request for CodeQL to find false positives in unit tests. To get to a "clean" review, I'm flagging these alerts as false positives. Unfortunately, the Dismiss Alert button triggers a postback, taking me to the top of the pull request, requiring me to scroll back down to wherever I was at in the PR. This is a time consuming activity for even medium PRs, making me more likely to just ignore CodeQL alerts rather than address them.

[AlonaHlobina]

Hey @chris-jenkinson,
Thanks for taking the time to provide us with your feedback. We appreciate it, and it helps us to improve our services. The code scanning them is looking into improving the behaviour now.
Additionally, I suggest that you consider specifying directories to exclude test files from the code scanning analysis. This can help minimize the number of false positives you encounter in non-production code.

[chris-jenkinson]

If my language has more than one language in the array, how do I tokenize everything in the filter-sarif and upload-sarif steps?

[jketema]

I'm not sure what you mean by tokenize. Note that if you have more than one language and use the CodeQL action, this will spawn a separate job for each language.

[chris-jenkinson]

I'm currently using the template action found here: https://github.com/github/codeql-action and then specifying

strategy:
      fail-fast: false
      matrix:
        language: [ 'csharp', 'javascript' ]

In the filter-sarif example, there's this step that I've converted for csharp:

- name: filter-sarif
      uses: advanced-security/filter-sarif@v1
      with:
        patterns: |
          +**/*.cs
          -**/*Test*.cs
        input: sarif-results/csharp.sarif
        output: sarif-results/csharp.sarif

It's not clear if there's a way to say "filter like this for csharp, filter like this for javascript" but rather it appears that if I want to use this step I would need to split out my current .yml into two files, one for csharp and one for javascript?

[jketema]

There are two options here. One if is to make the step conditional and have one per language:

- name: filter-sarif
      uses: advanced-security/filter-sarif@v1
      if: matrix.language == 'csharp'
      with:
        patterns: |
          +**/*.cs
          -**/*Test*.cs
        input: sarif-results/csharp.sarif
        output: sarif-results/csharp.sarif

If you don't mind that the list of patterns contains patterns from multiple languages, something like the following should also be possible:

- name: filter-sarif
      uses: advanced-security/filter-sarif@v1
      with:
        patterns: |
          +**/*.cs
          -**/*Test*.cs
          +**/*.js
          -**/*Test*.js
        input: sarif-results/${{ matrix.language }}.sarif
        output: sarif-results/${{ matrix.language }}.sarif

[chris-jenkinson]

Thank you so much!

[hharen]

Could you please let us know if the alerts you were dismissing and were causing the whole page reload were in the unchanged files?

[chris-jenkinson]

They were in changed files.

[hharen]

And are you able to reproduce this behaviour?

[chris-jenkinson]

It seems to no longer be happening, which is great!

[chris-jenkinson]

This problem seems to be happening again. It is happening in added files. It's in a private repo so I can't give you a link.

[gg-sr]

+1. In C++, CodeQL has some false positives and purposely adds a lot of "Check notices" which, AFAIK, are intended to be dismissed, yet dismissing a single alert takes far more time that it should due to the page refreshing.

[jketema]

In C++, CodeQL has some false positives

Please report those false positives here: https://github.com/github/codeql

and purposely adds a lot of "Check notices" which, AFAIK, are intended to be dismissed

I you believe those should be dismissed, and not fixed, then you might want to exclude those queries in you CodeQL configuration.

[gg-sr]

Please report those false positives here

This is planned, but some false positives are hard to reproduce in small examples, or fixed upstream but not yet released, or already reported.

then you might want to exclude those queries in you CodeQL configuration

Some of these findings are useful, so I'd rather not disable them completely. For instance, Autosar rule A3-1-5 "A function definition shall only be placed in a class definition if (1) the function is intended to be inlined [...]" leads to CodeQL notes, and not errors (since CodeQL is not aware of developers' intentions).