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.

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

Fix CodeQL issues #2228

Closed
LaszloGombos opened this issue Feb 22, 2023 · 5 comments
Closed

Fix CodeQL issues #2228

LaszloGombos opened this issue Feb 22, 2023 · 5 comments
Assignees
Labels
enhancement Issue adding new functionality

Comments

@LaszloGombos
Copy link
Collaborator

LaszloGombos commented Feb 22, 2023

https://github.com/dracutdevs/dracut/runs/8493550424
https://github.com/dracutdevs/dracut/security/code-scanning

Security Alerts:

10 high
Other Alerts:

3 warnings
17 notes

Would be good to resolve at least the 10 high alerts

@LaszloGombos LaszloGombos added the bug Our bugs label Feb 22, 2023
@LaszloGombos
Copy link
Collaborator Author

@nabijaczleweli Do you have any interest to look into these ?

@nabijaczleweli
Copy link
Contributor

the page linked doesn't work (the popups are broken and the direct code links all link to nothing), and the summaries mostly range from "so what?" to "idiotic", so not really, no

@LaszloGombos
Copy link
Collaborator Author

@nabijaczleweli Thanks. Understandable. I think that scan is out of date which is why lot of links do not work, and if you upload a PR now that would not be out of date on the PR run.

We only have a few of us and and if we not able to make this bot pass, then it is better to disable it (and continue drive blind).

I suspect once we make it green, it might be more useful on a per PR bases

@johannbg
Copy link
Collaborator

@LaszloGombos given that you decided to merge this prior to fixing the issues the scanner had already detected as I had mentioned we should do prior to merging #1987 so we would have a clean state to start from this is now your responsibility to fix...

@LaszloGombos
Copy link
Collaborator Author

LaszloGombos commented Feb 23, 2023

@johannbg I might not fully understand how this work, but what do you mean by "clean slate" ?

It seems to me that this works differently than shellcheck. There is no mechanism for inline suppression. Newly uploaded code passes the check with green (see #2229) and now we have a tool to check new PRs and GitHub's infrastructure to manage the reported issues.

See https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/managing-code-scanning-alerts-for-your-repository

I agree with @nabijaczleweli in that it should not be a goal to resolve all issues as a lot of them is just noise. It seem to "dismiss noise", the expectation is NOT to annotate the source code, but simply manage it in the https://github.com/dracutdevs/dracut/security/code-scanning dashboard. Once you mark an issue "Dismiss" in the dashboard, it is gone without any code changes. I already marked quite a few of them not applicable and intent to resolve some of them but not all.

Link to the Dismiss'd issues - https://github.com/dracutdevs/dracut/security/code-scanning?query=is%3Aclosed+branch%3Amaster

I could certainly mark all of the issues "closed" for "clean slate", which is more or less the state we would be in if we just staring at that open PR without landing it and hoping that somebody would reopen a stale PR.

We need to triage and decide which one we care to fix - ideally before writing a lot of code. The best way to triage is to land the PR.

This is a good bug (and good time) to discuss which one is safe to close and which one we expect to resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue adding new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants