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.

Sign up for GitHub

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

Chore: Suppress unqualified CodeQL admonitions #676

Merged
merged 1 commit into from
Nov 23, 2024
Merged

Chore: Suppress unqualified CodeQL admonitions #676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/codeql.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ jobs:
languages: ${{ matrix.language }}
config-file: ./.github/codeql.yml
queries: +security-and-quality
# run an 'alert-suppression' query
packs: "codeql/${{ matrix.language }}-queries:AlertSuppression.ql"

#- name: Autobuild
# uses: github/codeql-action/autobuild@v2
Expand All @@ -62,4 +64,21 @@ jobs:
uv pip install --system '.[test]'

- name: Perform CodeQL Analysis
id: analyze
uses: github/codeql-action/analyze@v3
with:
category: "/language:${{matrix.language}}"
# define the output folder for SARIF files
output: sarif-results

# Unlock inline mechanism to suppress CodeQL warnings.
# https://github.com/github/codeql/issues/11427#issuecomment-1721059096
- name: Dismiss alerts
# if: github.ref == 'refs/heads/main'
uses: advanced-security/dismiss-alerts@v1
with:
# specify a 'sarif-id' and 'sarif-file'
sarif-id: ${{ steps.analyze.outputs.sarif-id }}
sarif-file: sarif-results/${{ matrix.language }}.sarif
env:
GITHUB_TOKEN: ${{ github.token }}
Comment on lines 66 to +84
Copy link
Member Author

@amotl amotl Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @aibaars. Thank you very much for submitting a fix to add the missing id: analyze per GH-677. The workflow recipe works well now, running to completion.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, it looks like the admonitions that have been opened are not automatically being dismissed again. On the CodeQL workflow run, there is a warning:

1 configuration not found

Warning: Code scanning may not have found all the alerts introduced by this pull request, because 1 configuration present on refs/heads/main was not found:

Actions workflow (codeql.yml)

⌛  .github/workflows/codeql.yml:analyze/language:python

-- https://github.com/crate/crate-python/pull/676/checks?check_run_id=32972101671

Copy link

@aibaars aibaars Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You also need to run an alert-suppression query. That is a QL analysis that searches for the #codeql comments. According to the example in the readme this can be done as follows:

    - name: Initialize CodeQL
      uses: github/codeql-action/init@v2
      with:
        languages: ${{ matrix.language }}
        # run an 'alert-suppression' query
        packs: "codeql/${{ matrix.language }}-queries:AlertSuppression.ql"

Copy link

Choose a reason for hiding this comment

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

Another problem, you'll likely end up with a bunch of pairs of new and fixed alerts because you are adding the #codeql comments on the same line as the alert. CodeScanning uses a hash-based "fingerprint" to track an alert across commits. This hash is calculated from the 100 non-whitespace character counted from the start of the line on which the alert occurs. When you put a suppression comment on the same line, then CodeScanning considers the alert without the comment to be a different one. As a result the old one is closed as "fixed" and a new one is created in the "dismissed" state.

You can put the comment on the line before the alert to avoid this problem in most cases. This is why the documentation recommends the following style:

# codeql[py/unused-global-variable]
apilevel = "2.0"

However, if you have alerts very close to each other (fewer than 100 characters apart) you still see this behaviour.

Copy link
Member Author

@amotl amotl Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @aibaars,

thank you so much for your elaborate and valuable comments. 💯

I've adjusted the patch to use your suggestions, which resolved the admonitions by @Github-advanced-security at #676 (review). Excellent! 🌻

We guess that will also resolve the items at security/code-scanning after merging?

With kind regards,
Andreas.

Copy link
Member Author

@amotl amotl Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess this warning / stalling workflow task may resolve itself also after merging?

image

1 configuration not found

Warning: Code scanning cannot determine the alerts introduced by this pull request, because 1 configuration present on refs/heads/main was not found:

-- https://github.com/crate/crate-python/pull/676/checks?check_run_id=33421035019

Copy link
Member Author

@amotl amotl Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FYI: After merging, the admonitions in GitHub » Security » Code scanning have not been resolved automatically.

I guess this can be expected, because there is a corresponding note:

Rule py/unused-global-variable is not supported by Copilot Autofix for CodeQL.

I will just go ahead and dismiss those manually?

1 change: 1 addition & 0 deletions src/crate/client/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
# regex!
__version__ = "1.0.0"

# codeql[py/unused-global-variable]
apilevel = "2.0"
threadsafety = 1
paramstyle = "qmark"
Loading