-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Introduce the security-experimental CodeQL suite and experimental tag #11702
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
Conversation
…ries with an experimental tag
…ew tagging system
I'd be surprised if there aren't one or two CPP experimental queries with performance issues on larger projects, but these issues aren't exactly 'known', we don't have a list of them. Is it worth me putting aside an hour or two to identify them now? Or just deal with things as and when problems are reported? |
That sounds reasonable, but if you find yourself spending more time on it, it's OK to just exclude them 🙂 |
After a bit of experimentation: CPP and I guess Swift LGTM. I will be away for Christmas so please don't wait for my final 👍. |
You haven't added a |
…o turbo/experimental/combined
@erik-krogh fixed @geoffw0 excluded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS 👍
All the new queries perform OK.
And we will get way more experimental queries once I finish creating some heuristic variants of our ordinary queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for Ruby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 CPP and Swift.
Will Java be an included language for these query pack? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go and Java 👍
@cgaudette Please see the diff - all languages are included. Thanks for all your stamps. Now waiting for a final review from Python, C#, core (@adityasharad) and a stamp from another PM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoff and I looked at the performance data for Python, and agree that it doesn't look like there are any huge performance issues lurking around.
C#: The changes look good, but I would like to run the new suite against our existing DCA projects. I will circle back on this. Also a question: Are there any plans on changing the existing selectors such that they are not explicitly excluding the experimental queries by folder but instead using tags? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# LGTM
The DCA experiment to run the new query suite did terminate in time. |
@michaelnebel Thanks for looking into this, I think since the evaluation succeeded, we can move ahead with this and address any lower-level performance issues as they arise later. On:
We can definitely do that in a separate batch of work. If I understand your suggestion correctly and we were to implement this right now, that would actually pull in the non-security experimental queries into the existing suite selectors. To prevent that a thorough cleanup of the directory structure and tags would be in order, which is definitely worth doing, but probably not required now. Or did I misunderstand and you actually see this current PR impacting the existing selectors (something I wanted to avoid)? |
I completely agree that it shouldn't be a part of this PR. |
This PR implements the experimental CodeQL suite, by introducing a new suite selector
security-experimental
and suite implementations for all supported (and beta) languages. For more details and caveats, please see our internal roadmap item.Goal of the experimental suite
The
security-experimental
suite captures all queries captured bysecurity-extended
, but adds all existing queries that:experimental
foldersecurity
The experimental suite is not going to be advertised for use by code scanning users; not as part of the workflow, docs, neither the setup experience. It is meant as an internal or field testing shortcut. It does not change our current support level for experimental queries (which is "none"), and it does not impose any restrictions on contributions or experimental query writing, with the exception of adding a tag introduced by this PR.
Structure of this change
I've structured this PR in three distinct commits:
4ec401a
introduces the tagging mechanism. Previously, experimental queries were just put into each language's experimental directory, each with a different structure. The standard suite selectors have excluded this directory by path, instead of metadata/tags. This commit adds a newexperimental
tag to all applicable queries (see Goal section above for criterion used) to allow for a change to select by tags insteadb35a1d4
is a docs change and updates the contribution guidelines to require theexperimental
tag, as well as the experimental doc stub to indicate the existence of the experimental suite5fd5ebc
finally creates thesecurity-experimental
suite selector, and implementations for all supported languages. Plus Swift, which doesn't have an experimental folder, but gets this change anyway in anticipation.It's worth expanding on how the logic in 3. works: We still exclude everything in all experimental directories, but then explicitly re-include all queries tagged both security and experimental. This ignores quality, debugging, and other queries that shouldn't be considered for this and allows some amount of freedom of use of the experimental folder without every single query immediately ending up in this suite.
Things to review in detail
While each single change is simple, the combination may cause complex consequences to arise, so please pay close attention to the following, in addition to the general review and comments 🙂
code-scanning
,security-extended
, orsecurity-and-quality
suites, how users use them, and what results they produce. I'm somewhat confident that's the case, but I want to make sure.Timeline
I'll be offline until Dec 8000 17 to Jan 9, but please do add your review and questions in the meantime if you are around. I'm aiming to address most of them within January so this can ship in the same timeframe.
cc @NickLiffen