Skip to content

Conversation

@smelc
Copy link
Contributor

@smelc smelc commented May 9, 2025

Context

I am experimenting with GitHub's CodeQL static analyzer and this project is a good target, because it manipulates user input (in URLs).

How to trust this PR

Look at the vulnerabilities found on my fork: https://github.com/smelc/nix-security-tracker/security/code-scanning IMHO most of the reports are good:

image

By default this check is non intrusive, because this new pipeline only blocks merging when a new error, critical, or high severity is found in code changed by the PR (see corresponding doc). As visible in the image above, all existing alerts are below this treshold.

If we want the check to forbid merging PRs on lower severities it is also possible: https://docs.github.com/en/[email protected]/code-security/code-scanning/managing-your-code-scanning-configuration/editing-your-configuration-of-default-setup#defining-the-alert-severities-that-cause-a-check-failure-for-a-pull-request, here (not in the branch protection rule, which is why I'm highlighting it here):

image

Note that I disabled analyzing src/website/shared/migrations as it seemed to me those files had been generated.

We'll also want to check this in the branch protection settings of the repo:

image

so that malicious users cannot turn CodeQL off/things don't get green if CodeQL is silent

Maintenance

I will be available to maintain this pipeline in the foreseeable future, so this shouldn't be an additional burden on the team. Maintaining this will be part of my learning path on CodeQL.

@smelc smelc marked this pull request as ready for review May 9, 2025 12:42
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@fricklerhandwerk fricklerhandwerk merged commit 3ff9b5f into NixOS:main May 13, 2025
5 checks passed
@smelc
Copy link
Contributor Author

smelc commented May 13, 2025

@fricklerhandwerk> Thanks for merging! ❤️

Please note that you need to adjust the repository's settings (as explained in the PR's body) for the changes to:

  1. show in the security tab (as of now they don't, see below (or maybe I don't have enough rights to see them 🤔 ?)). I think this is a must have.
  2. block merging a PR if a new vulnerability is found. To me this is a matter of taste, so I'd say it's optional.

image

Let me know if I can help.

@fricklerhandwerk
Copy link
Collaborator

Yep, should be set up now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants