-
Notifications
You must be signed in to change notification settings - Fork 683
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
Add CodeQL SAST #952
Add CodeQL SAST #952
Conversation
.github/workflows/codeql.yml
Outdated
actions: read | ||
contents: read |
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.
According to the documentation, this is only needed for private repos...
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.
Sorry, I don't follow. Permissions:
at all or some specific right
?
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.
According to the CodeQL Github Actions README file:
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.
I see. I'll remove it.
.github/workflows/codeql.yml
Outdated
# CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ] | ||
# Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support |
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.
We can probably remove this comment?
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.
It were just some hints for the future, but if you prefer clean code, sure.
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.
Sure, we can keep it 👍
matrix: | ||
language: [ 'cpp' ] |
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.
is matrix
needed here? there's only one language...
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.
Makes it easier to add additional languages if one day you would like to scan python too for example.
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.
I don't think we'll need to examine the python code because it's mostly for tests or configuration, but we can keep it as is
@sashashura the build is failing because CodeQL check fail: These errors should be ignored because PcapPlusPlus uses these to parse SSL/TLS packets. However I'm not sure there is a way to suppress/ignore errors on specific lines as this issue says: github/codeql#9298 (comment) Should we add |
Interesting behavior. From my experience all security issues found by a codeql.yml already merged in repository are placed in Security tab and are visible only to maintainers. It is easy to click a button and suppress specific warnings there if they are false. |
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.
@sashashura the next step would be to add a badge to README.md, and then let it run for some time. If CodeQL is stable and works as expected we can probably remove LGTM
CodeQL is the continuation LGTM. It integrates into GitHub Security tab and is more configurable. I have set it up to scan only for C++ issues.