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

Added CodeQL code-scan workflow #67

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

lgndluke
Copy link

Hey Gabriel,

I've added a GitHub Actions workflow to the project, which utilizes CodeQL to provide a comprehensive code analysis.
This workflow may be useful for detecting vulnerabilities / programming errors inside the project that might otherwise go unnoticed.

The analysis data of the main branch (and only the data of the main branch, as far as I am aware of) is stored inside the Security section under "Code scanning" inside the GitHub project.

Example: Results listed inside the projects Security section

Code-Scanning
Link to check out the results of my latest r3 code-scan run: Click here to be redirected

When inspecting an open result it provides detailed information, as well as a recommendation on how to fix/resolve the potential problem.

Example: Code-Scan alert

Alert1


Alert2


Alert3


Link to check out the result that is displayed here: Click here to be redirected

It's also worth noting, that the workflow is capable of automatically closing previous result once they have been fixed.
(Will be updated on the next workflow run!)

Example: Closed Alert

Closed_Alert1
Link to check out the closed alert displayed here: Click here to be redirected

Currently it is set to active whenever a commit to the main branch is made.

on:
  push:
    branches:
      - main

However, this behavior could also be changed so that the workflow for example is regularly run as a scheduled cron-job, or for example whenever a pull-request is being merged.
The triggers can also be combined, you can find out more about them here: CodeQL-Documentation

Sidenote: Furthermore, I've also added the ".idea" directory created by my IDE of choice to the .gitignore file.

Best regards,
Lukas Jeckle

@TechInterMezzo
Copy link

@lgndluke I don't know if you only gave permissions to the authors of r3 but I can't view any code scan reports. The Security tab says that nothing is published and the links show 404.

@lgndluke
Copy link
Author

lgndluke commented Nov 27, 2024

Hey @TechInterMezzo,

I did not change any permissions within my fork of r3.
After taking a quick look into the settings tab of the repository I also wasn't able to find a suitable looking option to change those.

I currently assume the fact that you are unable to access the code scanning results / alerts could be due to some sort of GitHub standard (allowing only collaborators of the project to access the results / alerts), or maybe there is an organization level rule implied by the r3-team organization, which manages those kind of access permissions. However, I can't confirm any of this.

To verify could you please try to open this code-scanning alert from another project of mine?

If it opens, then the r3-team most likely has put some access permissions up. Anyhow, if it doesn't open then I'd assume it's a GitHub standard only allowing collaborators to access a projects code scanning results.

@PR-Assignee / Gabriel
Please let me know, if you should also face problems reviewing the code-scanning alerts. While I highly doubt it, I'm more than happy to investigate further if needed.

Best regards,
Lukas Jeckle

@r3-gabriel
Copy link
Member

r3-gabriel commented Nov 27, 2024

I´ve had a look and I have no access either. The links point to your forked repo, so I assume that our repo / organization rules do not apply there. You might need to look at the security settings of your repo:
image

The link from your alternate project also does not work for me. I get a page 404. Maybe its set to private?

Regarding CodeQL: I have never worked with it, but I looked at the things it found in the provided screenshots. All examples I´ve looked at where so far unhelpful as the high & critical findings are fine in the corresponding contexts. It might require some tweaking to avoid lots of unuseful findings.

As an example for a critical finding (scheduler_update.go:27):
httpReq, err := http.NewRequest(http.MethodGet, url, nil)

This is "bad" because the input that goes into the url variable for the HTTP request is dynamic - and must stay dynamic to work correctly in this context. And I would agree that this is bad if we took any user input to build the URL. CodeQL does not know that the url variable is built from the system configuration and does not take untrusted user input. I am not certain this can be addressed in a way that would satisfy this finding.

There are probably a number of flaws that a tool like this can show, which are useful to learn. I will try it out locally to see if it can be configured to help us. But I want to avoid flooding the public repo with unuseful findings.

@r3-gabriel r3-gabriel self-assigned this Nov 27, 2024
@lgndluke
Copy link
Author

Hey Gabriel,

Thanks for the feedback. I've checked again, and I've noticed I am unable to change the r3 forks visibility.

grafik

For my alternative project. The visibility is set to public.

grafik

Hence, I assume that the scan results are only visible to Collaborators of a project.

I've currently invited you to be a collaborator in my fork of r3. This way I hope you'll be able to access the scan-results.
grafik

Please let me know if this works.

Let me know if there is anything else I can do to help.

Best regards,
Luke

@r3-gabriel
Copy link
Member

I´ve accepted your invitation. I am now able to view the security findings in your forked repo. I will have a look at them now.

Since I have no experience with how CodeQL works and how it interacts with repos, I will have to spend some time on it before I can judge if it makes sense for us.

Regardless, thank you for the iniative :)

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

Successfully merging this pull request may close these issues.

3 participants