Repository navigation
Add CodeQL security analysis to GitHub Actions workflows #36957
Description
Activity
What is CodeQL, and why is it worth doing?
CodeQL is a tool for finding vulnerabilities in the entire codebase. Since it was acquired by GitHub last year, it has been integrated natively, and now it can be easily integrated to workflows with GitHub Actions. It's free for open-source repositories, so there's no harm in using it. Of course, it doesn't find all the vulnerabilities, but it's much better than nothing.
https://github.com/proxy/securitylab.github.com/tools/codeqlCodeQL may be a good fit for lots of repositories, but I suspect this is not one of them. I could be wrong. Maybe there's a way to open a pull request adding it such that we can see what kind of results we get without having to land it in the repository first.
CodeQL was the tool created by/for LGTM.com. I therefore assume that enabling it here will result in similar results to LGTM.com. At the current time LGTM.com flags 160 things in the Node.js repository. Of those 160 alerts, 153 are in three dependencies that are vendored into the repository: V8, gyp, and inspector_protocol. The issues flagged appear to contain many false positives and/or test/tooling code that is not relevant to Node.js core.
Of the remaining 7, there are multiple false positives such as this incorrect flagging of a line in configure.py. I reported this as a bug in the CodeQL issue tracker in September. It's entirely possible a fix is imminent or already happened on the GitHub CodeQL side, but LGTM.com is still reporting this issue so maybe not.
Basically, my expectation is that a CodeQL analysis at the current time would produce too much noise to be useful. I could be wrong. Or I could be right but it will change as the tool evolves. As I said above, maybe there's a way to open a pull request adding it such that we can see what kind of results we get without having to land it in the repository first. If so, I'd welcome that.
Reacted by akhil marsonyaIf CodeQL is able to reliably detect use of unsafe array iteration or report where primordials should be used, I'd be up for that, personally. I guess it depends how configurable it is, I agree it's not helpful if it produces more false positives than useful warnings.
Reacted by Darshan Sen and akhil marsonyaI won't block but I'm generally not a fan due to the complexity and false positives. eslint seems to cover the majority of what we'd need.
Reacted by akhil marsonyaI'm going to close this, but feel free to comment or re-open if you think there's a reason to keep this open. Thanks!
It seems that a PR was opened by someone to add this before, but somehow it was closed. In many cases, I think it's worth doing this and I'm ready for PR, so if you are willing to add it, I will raise a PR.