codeql-implementation

[SAUMILDHANKAR]

Fixes #2400

What changes did you make?

• Added a default CodeQL workflow for scanning website repo.
• Included security and quality query to the default workflow.
• Set up this workflow to run once a week on Friday as discussed in the team meeting.

Why did you make the changes (we will use this info to test)?

• As per the requirement for this issue, we are setting up code scanning for website repo and implementing the default CodeQL workflow for security scanning of website repo.
• I have also added notes to create custom queries in the comments of the issue (summarized comments: comment 1, comment 2, and comment 3). If there is a need to create a custom query in future, they can be referenced.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

• No screenshots needed as there are no visual changes.

[github-actions]

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b SAUMILDHANKAR-Codeql-implementation gh-pages
git pull https://github.com/SAUMILDHANKAR/website.git Codeql-implementation

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[roslynwythe]

Hi @SAUMILDHANKAR I'm looking at the log of the scan at https://github.com/hackforla/website/actions/runs/5384746526/jobs/9773025851 and I had a few questions. It looks like it scanned all of our JS files (270 files) and found 6 errors and 6 warnings. How can we see the errors and warnings? If there is an error in a scan triggered by a push or PR, would you expect GitHub to display the errors in the same manner it displays the results of other checks, or do we have to go to the Security settings? Would merging be prevented until errors are resolved?

[SAUMILDHANKAR]

Hi @roslynwythe thanks for taking a look at this PR. Please use the following link to see all the notifications that would be generated once this PR is merged: https://github.com/hackforla/website/security/code-scanning?query=pr%3A4886+is%3Aopen. I see 57 notifications for the website repo. Once merged, best place to view the results would be under the security tab, though one can also look at the logs of a workflow run.

It is similar to other checks and will only fail if the severity level is critical or high. None of the 57 notifications that have been generated so far have this severity level, therefore the check for this PR passed. Similarly, merging will be blocked only for severity level critical or high.

Also, we can change the settings if we want merging to be not blocked at all. Please see the image below for the different options that would be available in website repo after merging this PR:

[roslynwythe]

Thank you @SAUMILDHANKAR that was very useful information. The Security tab doesn't show any scan information so it sounds like it only reflects the scheduled scans performed on the default branch, not scans triggered by PRs. Is that correct? And I didn't see any details about the errors/warnings in the logs; please show me where to find those.

[SAUMILDHANKAR]

@roslynwythe Thank you. You are correct, security tab won't display information about unmerged PR scans. It will display the results of weekly workflow run.

The workflow run that happened with this PR was successful, so no errors reported in the logs. However, if there is an error, there would be a cross on one of the steps in the workflow run (part of the link that you shared) and we can expand using the dropdowns to learn more.

[t-will-gillis]

Unbelievable, but I posted these questions on #2400 instead of here.

Hi @SAUMILDHANKAR thanks so much for your extensive work on this issue. There is a lot of info here to digest and I apologize if I am asking questions that you have already answered, but just to confirm- the current implementation will scan all of the js files in the website repo and generate in this case the 57 notifications that we could address one by one. Then it is also scanning future PRs, correct? You indicate that it will block merging for critical or high security issues- for the others I assume it would flag the issues so they could be addressed before merging?

Side discussion: As a group, do we need to decide which “Protection rules” to use, i.e. what “Security” level and “Errors, Warnings, or Any”- or for now do we go with the defaults?

Lastly, the 3 linked comments I believe refer to custom queries you wrote for scanning yaml and liquid code. Would these scans run at the same time as scans for the js files?

Thank you again- this is going to be very useful when it is up and running

[roslynwythe]

@SAUMILDHANKAR @t-will-gillis I agree that we should not block merging on Medium level alerts but when reviewing a PR, I wish I could see the details of all of the alerts pertaining to the PR in the GitHub checks area, because they would be useful for identifying code quality issues, and of course we want to resolve those prior to merge. But if I understand you correctly, we will not see those Medium or Warning or Note level alerts until after merge.

Looking over the alerts, it appears that the scanner was interpreting liquid elements as js. This happened with the triple dashed lines ending the front matter (see alert#31 https://github.com/hackforla/website/security/code-scanning/31) and also with the characters {% at the start of a liquid tag (see alert#32 https://github.com/hackforla/website/security/code-scanning/32). In both cases, syntax errors were reported. Is there a way to prevent those instances? Also I wonder why sometimes it alerts on missing semicolons (alert#28 https://github.com/hackforla/website/security/code-scanning/28) but other times it ignores missing semicolons (lines 7-20 of wins.js). Maybe it is looking for inconsistency. Personally I would prefer that it always alerted on missing semicolons.

Also could you explain the "Extraction Errors" and "Could not process some files due to syntax errors" (circled)

[SAUMILDHANKAR]

Hi @t-will-gillis, thanks for reviewing this PR.

• Correct! The current implementation will scan all of the js files in the website repo and generate in this case the 57 notifications that could be addressed one by one. It will also scan future PRs. If the severity level isn't critical or high, the scan that runs with the PR will pass and merging won't be blocked. Rest of the alerts can be viewed in the code scanning alerts under the security tab. We can use a filter like is:open pr:4886 to view all the alerts related to a particular PR. This way they can be addressed before merging if required.
• For "protection rules", based on my understanding we might have to look at the severity levels assigned to all the queries in the CodeQL repo and then based on hackforla website repo's codebase decide if any of the queries might need immediate attention and then update accordingly. As this would need more analysis, my recommendation would be to address this in a subsequent issue.
• The custom queries mentioned in my comments are not part of this PR. They won't be running with the scan and are for informational purpose. They can be referenced in case we decide to write a custom query specific to our website repo in future.

@roslynwythe

• I just figured out that using a filter like is:open pr:4886, we can view the details of the security alerts prior to the merge as well. Sorry about my earlier comment.
• The syntax errors are generated by js/syntax-error Rule ID. This seems to be part of queries: security and quality which I added in the workflow. On removing this, the syntax errors went away when I tested in my repo. Another way could be just excluding that specific query as mentioned in the documentation here. Downside of removing this query might be that we miss true syntax errors. We can also dismiss this alert as a False Positive, will look into if it would remember our selection for future scans as well. For semicolon insertion alert, which is part of Rule-ID js/automatic-semicolon-insertion, one of the options can be reporting this issue in CodeQL repo for further improvements.
• CodeQL creates a database for all the code in a repo using an extractor program. For our repo, it was not able to process 6 files due to some reason and hence it reported 6 extraction errors. These errors are separate from the alerts that are generated after each workflow run. Here is a link for more information: https://docs.github.com/en/code-security/codeql-cli/using-the-codeql-cli/extractor-options#setting-extractor-options-from-files

[SAUMILDHANKAR]

Update: In my own repo, I dismissed one of the syntax errors as a false positive and in subsequent workflow runs, the alert is permanently marked as closed. So, this can be one of the ways to address alerts which we do not want to resolve or make an issue of.

[roslynwythe]

Hi @SAUMILDHANKAR it sounds like once this PR is merged, we will need to create some new issues.

• an issue to examine each Security alert and either dismiss it as a false positive alert or create issue(s) to resolve it
• an issue to modify the current github actions comment that gives instructions on how to checkout the PR branch, so that it instructs reviewers to follow the link

https://github.com/hackforla/website/security/code-scanning?query=pr%3A[REPLACE WITH PR#]+is%3Aopen

in order to view the alerts that will be added as a result of merging the PR

• an issue to review the 'protection levels' for each alert. Above you state

we might have to look at the severity levels assigned to all the queries in the CodeQL repo and then based on hackforla website repo's codebase decide if any of the queries might need immediate attention and then update accordingly.

Could you explain how we look at all the queries in the CodeQL repo? Are 'queries' equivalent to 'alerts' in this context? Are you saying that we could increase the severity level assigned to a particular alert? Or just that we could decide to change the level at which merge is blocked?

Please advise if this sounds correct and if you would be willing to write any of these issues.

[SAUMILDHANKAR]

Hi @roslynwythe, thanks for creating an outline for the issues. The first two issues that you mentioned look ready. For the third issue, I would like to add the following:

• For JavaScript, this folder has most of the queries that I have encountered so far. Also, CodeQL team might add new ones.
• Each query is corresponding a .ql file which can generate multiple alerts.
• If we want to change the severity level of an alert, one way could be to exclude the corresponding .ql file from the workflow and then create a similar custom query with a modified severity level by updating the metadata property. I am assuming a lot of analysis would have been done to tag a query with a particular severity so if we would like to update it, it might require similar effort. I was mainly referring to changing the levels at which the merge is blocked. So, the analysis before changing it would be something like how many .ql files in CodeQL repo would be impacted and how does that affect the code in the website repo.

I am happy to write these issues. Please suggest if I should wait till this review is complete.

[t-will-gillis]

Hi @SAUMILDHANKAR Fantastic work on this issue. I know that you and @roslynwythe are still discussing details, and I am happy to give my thoughts on any of the questions about implementation settings and future functionalities. In the meantime, I will submit my approval.
Thanks again!

[roslynwythe]

Thank you @SAUMILDHANKAR for a great job on this PR and demo, and for your patience and clarity answering our questions.
I recommend that you write 3 ER (Emergent Requirement) issues which state the requirements we have identified, and briefly propose an issue(s) to address each requirement. Then, PM can advise how to structure the issues so that we don't waste any time or effort. Bonnie might want to split up the first one into multiple issues, and before writing an issue to dive into the CodeQL queries, she might want a DR (Decision Record) about whether we need to make any changes in the severity or blocking levels. When each ER is ready please add the 'ready for dev lead' label and comment mention me so I can help get PM approval. Thank you again for your valuable contributions.

[SAUMILDHANKAR]

@t-will-gillis @roslynwythe Thanks a lot for all your help with the issue and the PR. Your feedback helped me understand CodeQL workflow in more detail. Really appreciate your approval as well.

@roslynwythe Thanks for sharing information about ER issues. Planning to work on this over the weekend. Will add a comment mention when the ER is ready for review as suggested.

[roslynwythe]

@SAUMILDHANKAR I would like to discuss the ER for reviewing the CodeQL security alerts with you, but I cannot find your Slack contact information or email. We need to expedite the creation of this particular issue, so please advise re: your progress.

[SAUMILDHANKAR]

@roslynwythe Sorry for the delay. I was planning to work on this yesterday but got delayed. I just read through the issue that you created, and it looks good. Please let me know if you would still like to discuss anything. I will leave a message on slack as well.
Also, I am starting to write the other 2 ERs now, as discussed earlier. Thank you.

[roslynwythe]

• ER ER: Analyze CodeQL security/code quality alerts #4963