Add GitHub Advance Security and CodeQL

[GeekMasher]

Hi team, I've created this PR to add security results from GitHub Advance Security / CodeQL into your workflow and thought the team might appreciate having a look at the results of the security testing directly in GitHub. Currently, it will also scan on all new PR's that are created to check for security issues and it's scheduled to run every week on Tuesday to make sure you are running the latest and greatest rules from CodeQL.

I have ignored the test path as it was reporting a few False Positives in the Unit tests.

[Snuffleupagus]

We're already planning on adding the PR-integration of LGTM (see #11965), which as far as I understand uses CodeQL internally, so my question is how this is different and if we really need both?

[Snuffleupagus]

There's also the question of false positives, which all tools will suffer from[1], and specifically how to ignore those?

---

[1] For example, we're using ESLint extensively and there it's very easy to white-list (on an individual basis) false positives.

[GeekMasher]

@Snuffleupagus Both LGTM and GitHub use the same technology (CodeQL) but GitHub Advance security is built into GitHub and the next generation of the tool. You can mark False Positives in GitHub Security tab itself (see screenshot below).

Note: this screenshot is from another project as an example and not based on the results from this project.

From what I saw of my scan, you only have a few issues that were reported. I removed some false positives already using the configuration file.

FYI, you can also push ESLint results into GitHub Advance Security 👀

[timvandermeij]

Interesting. From what I'm reading GitHub Advanced Security/CodeQL is more specifically related to finding potential security issues whereas LGTM is more general about finding anyt defects in code using static analysis (not necessarily security-related). I agree that the advantage of GitHub Advanced Security/CodeQL is that GitHub itself provides it, so it's not really an external tool. If GitHub Advanced Security/CodeQL and LGTM indeed differ in this way (I'm not 100% sure, so please correct me if I'm wrong here), it might make sense to enable them both?

[GeekMasher]

@timvandermeij Not necessarily, you can enable all of the Query Suites (list of rules) LGTM does in GitHub Advance security (security and quality) but I didn't know if that was something we wanted to start off with.

This can be enabled by using the security-and-quality query suite, see this guide.

[Snuffleupagus]

Please note that we generally don't use separate fixup commits, to keep the history cleaner and to ensure that all patches are complete and fully working on their own; hence please squash the commits together such that we can test this!

[timvandermeij]

Aside from squashing the commits, would it perhaps make sense to enable the security-and-quality suite too so we have both security and quality checks managed by GitHub? This might make LGTM integration obsolete, provided that it provides more-or-less the same functionality of course (plus it's likely easier to integrate given that external services need additional steps from project admins to enable them).

[Snuffleupagus]

Given that LGTM has already been enabled, and in order to avoid too much scope-creep in this PR, maybe we could defer #13737 (comment) to another time?

[timvandermeij]

Yes, good idea. I didn't see that LGTM was already enabled. In that case the current PR should be fine (i.e., the security checks only).

@GeekMasher Could you squash the commits into one so we can give this a try? Moreover, the codeql-config.yml file seems empty now; can we remove that (and the corresponding config-file: ./.github/codeql/codeql-config.yml line) to keep the changes minimal?

[timvandermeij]

Closing since this is fixed up and merged in #13828 now. Thanks!

[GeekMasher]

@timvandermeij apologies for the delay in my replay, I was out for holiday and limited access.

Glad to see the code merged and I hope this is useful going forward

[timvandermeij]

No worries! Thank you for introducing CodeQL to us; we're now using it in our build process, configured it to ignore the false positives/irrelevant alerts and fixed the ones that it flagged correctly, so overall it's a very welcome addition.