Create manual approval process for errors with code scanned add-ons #3397

nvdaes · 2024-05-06T19:52:42Z

Issue number

Fixes issue #3279

Summary of the issue

When security analysis fails for an add-on, if the created pull request is merged manually, review comment and transform Actions aren't performed.

Development strategy

Created a reviewed.json file, to store addon ids with an array of sha256 of reviewed add-on versions.
The branch created for the add-on in the submission issue is checked out in the getAddonId job, to upload the add-on metadata json file as an artifact. This will be used by the securityAnalysis.js file to calculate the addonId and sha256, to be writen in the reviewed.json file if analysis fails.
If sha256 of the current submission is included in reviewedAddons.json, the workflow won't fail, so that mergeToMaster is run when the branch for the created issue is removed and the issue is relabelled.
If analysis fails, a PR including the sha256 to the reviewedAddons.json will be created, and a comment will be added to the submission issue showing the direct link to analysis results and more details about the process.
If that PR is merged, the authorIssuenumber Branch should be deleted and the issue relabeled to trigger a new workflow, this time with the sha256 of the add-on added to the reviewedAddons.json file.
After that, the analysis workflow won't fail and the mergeMaster and further Jobs Will be run.
As extra features arised during the development of this PR, paths to be validated are closed in quotation marks so that add-on ids with spaces are accepted, and the security analysis workflow includes an additional job to show non critical security errors to authors, which won't prevent the submission merge. To determine if these low severity errors (considered warnings) will be excluded, we use different configuration files available in the .github/codeql folder. The github/codeql-action/init action is used to determine the languages to be analyzed, and the configuration file to be used for each job.
In the first job, we exclude critical security errors and the analysis may fail. In the second one, the default configuration is used and the analysys won't file and won't prevent to merge submissions. Instead, warnings will be reported on the submission issue to be considered by authors.

Testing performed

Include security errors with score 7-10. to test that critical errors will make analysis to fail. Note that we don't have an add-on with critical errors and we have performed an equivalent check for testing including a wider score range.
Show warnings
Add-on without errors

seanbudd · 2024-05-08T04:40:52Z

.github/workflows/codeql-analysis.yml

+      - name: Download add-on again
+        uses: actions/download-artifact@v4
+        with:
+          name: addon


why does this download need to happen again?

The download has to be performed again since the add-on has been moved to a zip file and then expanded, so the addon.nvda-addon file is not present..
I'll address this review ASAP, but perhaps it won't be finished today, in case you want this to be fixed quickly, feel free to do it yourself.

Instead can you make a copy rather than rename the zip file?

.github/workflows/codeql-analysis.yml

seanbudd · 2024-05-08T04:41:24Z

.github/workflows/codeql-analysis.yml

+        if: failure()
+        uses: peter-evans/create-pull-request@v6
+        with:
+          add-paths: trustedAddons.json


Suggested change

add-paths: trustedAddons.json

add-paths: reviewedAddons.json

So, given your other comment, this would be something like:

Suggested change

add-paths: trustedAddons.json

add-paths: overriddenFalsePositives.json

I think that reviewed is more precisse than false positive, since codeQL, imo, is not detecting false positive but just errors in code, that may or not be related to security issues in the context of the whole add-on. I think that this is not the same as flagging an add-on as containing malicious contents. It's analyzing just source code. Anyway I'm not a native english speaker, but for Spanish, imo reviewed is a more accurate word for CodeQL.

.github/workflows/codeql-analysis.yml

seanbudd · 2024-05-08T04:44:20Z

.github/workflows/securityAnalysis.js

  const contents = fs.readFileSync(path);
  const data = JSON.parse(contents);
  const runs = data.runs[0];
  const results = runs.results;
  if (results.length === 0) {
    core.info("Security analysis succeeded");
  } else {
+    trustedAddonsData.trustedAddons.push(hex);
+    const stringified = JSON.stringify(trustedAddonsData, null, 2);


for submitters.json, we add a comment noting the author name.
Can you do something similar for adding a comment referencing the add-on id? Perhaps these should be grouped by add-on id.

seanbudd · 2024-05-08T04:44:43Z

.github/workflows/securityAnalysis.js

@@ -1,12 +1,27 @@
-module.exports = ({core}, path) => {
+module.exports = ({github, core}, path) => {


Could we change the framing for this to "Reviewed" add-on. I think "trusted" has too may implications

seanbudd · 2024-05-08T04:48:06Z

.github/workflows/codeql-analysis.yml

            Security analysis has failed for this add-on.
            [See GitHub workflow](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})
            Please, contact NV Access for more details.


please update this message

add a reference to the submitters guide: https://github.com/nvaccess/addon-datastore/blob/master/docs/submitters/submissionGuide.md

copy in this information directly: "You can open this link and download artifacts containing the results of the analysis. Unzip artifacts and open the .sarif file in your preferred editor. For example, you can use Microsoft's Sarif web based reader. NV Access will determine whether or not the detection should prevent the add-on from being accepted. Please review the warnings and consider whether you want to fix this in the add-on. If you can provide more context on the failure in the submission, please do."

note that the issue should be kept open

note the PR reference created that needs to be merged, like the submitters.json message

Did we resolve the question of why we weren't linking directly to the .sarif file instead? Even if it expires in 90 days, that is far preferable IMO.

I linked to an action for closing submission issues with a check failed label after 90 days, and would be happy to do a PR for same if @nvdaes doesn't want to.

seanbudd · 2024-05-08T05:13:08Z

Is it possible to only block CodeQL if the error severity is critical or above?
It might require creating a custom ruleset.
For lower level issues a report still should be generated and given to authors, but at this stage we don't think it requires a human review or being blocked from merging.

XLTechie · 2024-05-08T05:33:58Z

@seanbudd You have made several suggestions here, replacing "trusted" with "reviewed". I think the word "review"/"reviewed", is too overloaded. We now use it for add-on reviews by the public, and there may be need to track things related to that in the future. Additionally, it had other meanings in the past, that the community still knows. I request some other word. Perhaps "overridden"? Or "manualSecCheck" or some such?

seanbudd · 2024-05-08T05:43:32Z

Maybe "overrideFalsePositive" to be the most specific

XLTechie · 2024-05-08T05:53:03Z

.github/workflows/codeql-analysis.yml

+        if: failure()
+        uses: peter-evans/create-pull-request@v6
+        with:
+          add-paths: trustedAddons.json


So, given your other comment, this would be something like:

Suggested change

add-paths: trustedAddons.json

add-paths: overriddenFalsePositives.json

.github/workflows/codeql-analysis.yml

…Ql job

nvdaes · 2024-05-08T19:25:24Z

I've created an artifact with the submitted json add-on metadata, checking out the created branch. This is uploaded as an artifact and it should be downloaded by the codeQL job, to get the addonId and sha256.
I'll create a link to the sarif file to be posted on the add-on submission issue.
Abut rulesets, if they are created, I imagine that this PR should be changed or skipped, but in this case I don't imagine how add-ons which critical alerts can be manually approved.
Another possibility would be to read the sarif file and check for critical errors included in results. For now I'll complete the pending work unless you have any other feedback.

XLTechie · 2024-05-08T21:51:38Z

Thanks for all your hard work, @nvdaes!

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

…xist in the workflow

nvdaes · 2024-05-09T19:24:00Z

Sean wrote:

Is it possible to only block CodeQL if the error severity is critical or above?

Using CodeQL action, we may configure filters to include and exclude.
We may perform even several kinds of analysis, I think.

nvdaes · 2024-05-09T20:06:07Z

I think this is almost working, but two comments are added by the bot on failures.
See nvdaes#1201.
Perhaps we shouldn't use parallels jobs with matrix to avoid this.

nvdaes · 2024-05-10T18:41:13Z

I've tested that add-ons can be submit after closing in quotes the step to validate submissions, to accept ids with spaces, in

https://github.com/nvdaes/addon-datastore/actions/runs/9036423367/job/24833330757

This add-on doesn't have spaces in ids, but this should work if some add-ons uses this.

nvdaes · 2024-05-11T01:38:44Z

Sean wrote:

Is it possible to only block CodeQL if the error severity is critical or above? For lower level issues a report still should be generated and given to authors, but at this stage we don't think it requires a human review or being blocked from merging.

I'll try to address this in this PR with another commit, blocking errors but not warnings. Seems that some add-ons were blocked due to warnings.
I'll create another workflow specifying another configuration for QL. In this way, warnings will be downloable from the issue, visible to users and authors, but just errors will block merges.
If this doesn't work as expected for you, I'll revert the last commit.

nvdaes · 2024-05-11T04:49:37Z

My idea was to create two jobs, the first using a configuration file to exclude warnings, which would create a PR commenting on the submission issue.
The second one wouldnt exclude warnings, and would be run if the first one failed, needing the first one. This second job would analyze warnings and would post a comment just for authors, without creating a PR with reviewedAddons. continue-on-error of the second job would be set to true so that the workflow could succeeded and mergeMaster could be run.
But the configuration file excluding warnings seems not to work. If I exclude default queries to make query filters to work, then a fatal error is raised since no queries for Python are found. Seems that queries need to be specified.
So I leave this PR without more commits. Anyway this is the code of the configuration file tested to exclude warnings:

name: CodeQL Configuration excluding warnings
disable-default-queries: true
paths:
    - addon
query-filters:
- exclude:
  problems.severity:
  - warning
  - recommendation

nvdaes · 2024-05-11T06:47:29Z

Finally I've made a last commit to accept add-ons with CodeQL warnings, just reporting this to authors.
For errors merges should be blocked and a PR to add the reviewed add-on should be created.
I've tested this at

https://github.com/nvdaes/addon-datastore/actions/runs/9041653622

nvdaes · 2024-05-11T06:49:43Z

Note that you may need to enable advanced setup for CodeQL. When you do it, a new workflow will be shown. Now it's used but it's hidden, and it needs to be edited to be shown. Later I have removed the edition, but I have advanced setup configured and my approach seems to be working.

.github/workflows/securityAnalysis.js

.github/workflows/codeql-analysis.yml

seanbudd · 2024-05-13T06:59:34Z

.github/workflows/codeql-analysis.yml

 jobs:
+  analyzeExcludingWarnings:
+    name: Analyze add-on excluding warnings


I can't seem to find where warnings are excluded, can you point to this?

Warnings are excluded via a configuration file including filters to exclude warnings and recommendation. I read that this would show just important errors, and I checked that it's the case for the readFeeds add-on. Also MathCAT was rejected just for warnings.
Warnings are excluded in the first job, which will fail just for important errors.
If this job fails, the second one won't be run and the PR will be created.
If the first job doesn't fail, the second job will search for warnings (less important errors), and these will be reported to authors without creating the PR to add this to reviewedAddons.

Where is this configuration file? can it be documented here please

It's in the .github/codeql subfolder. It contains two configuration files, one to exclude warnings ant the other one to include them. The init action determines which configuration file should be used for each job.

These files appear to have not been committed to this PR.
The file on master doesn't contain anything relevant:
https://github.com/nvaccess/addon-datastore/blob/master/.github/codeql/codeql-config.yml

Sorry, I tested all in my master branch and files were there.

nvdaes · 2024-05-13T07:41:41Z

I've also documented the config files in the development strategy.
Should I test again or you think it's enough?

nvdaes · 2024-05-13T21:23:08Z

I've perform test in:

[Submit add-on]: nvdaes/addon-datastore#1258, without warnings.
[Submit add-on]: Test show warning nvdaes/addon-datastore#1263, with warning.
[Submit add-on]: error nvdaes/addon-datastore#1265, where I expected an error but a warning has been shown

I've used this example:

https://codeql.github.com/codeql-query-help/python/py-clear-text-logging-sensitive-data/

I suppose that more suites can be added in the CodeQL settings file (advanced settings of the repo), to determine what will be considered an error or a warning.
I've created a testing repo to upload fake add-ons for testing.
If you provide me another example to tes, I'll do.

.github/codeql/codeql-config-excluding-warnings.yml

nvdaes · 2024-05-14T05:40:26Z

I think we're going throught the right way. I've excluded just low security severity for testing, and this PR has been created, with the addonId

nvdaes#1268

Also, I haven't need to use the configuration workflow of advanced settings. In fact I've removed it.
Now I'll test excluding all except critical.

nvdaes · 2024-05-14T07:00:49Z

For reference, I'm selecting 9-10 security severity levels based on this document:

https://colinsalmcorner.com/fine-tuning-codeql-scans/

nvdaes · 2024-05-14T07:12:40Z

Test for showing warnings merging the PR:

nvdaes#1265 (comment)

See the last comment on the issue.

nvdaes · 2024-05-14T15:15:16Z

@seanbudd , I think this is ready for review.

seanbudd

Thanks @nvdaes

Unlock merge master for trusted add-ons

bfc65da

seanbudd changed the title ~~Unlock merge master for trusted add-ons~~ Create manual approval process for errors with code scanned add-ons May 8, 2024

seanbudd reviewed May 8, 2024

View reviewed changes

seanbudd mentioned this pull request May 8, 2024

Add support for Virus Total #3294

Merged

seanbudd marked this pull request as draft May 8, 2024 05:24

XLTechie suggested changes May 8, 2024

View reviewed changes

nvdaes added 3 commits May 8, 2024 11:10

Address review: copy artifact in a zip file instead of moving it

fab63ae

Test

5ccede3

Use addonMetadata.json artifact to get add-on ID and sha256 from code…

2b0d5f2

…Ql job

nvdaes and others added 4 commits May 9, 2024 18:17

Apply suggestions from code review

02dae81

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Use overwrite in upload artifact to avoid failures if sarif results e…

0ae33f1

…xist in the workflow

Add reviewedAddons.json file

a8be318

Add direct link to analysis results and update issue comment

22e0ba7

Improvements to manual approval process

97b47de

nvdaes added 2 commits May 10, 2024 19:39

Avoid duplicate comments on issues

8ccffe0

Accept add-on ids with spacces

ea6b5cc

nvdaes marked this pull request as ready for review May 10, 2024 18:41

Report warnings to authors without locking merge

a3039f7

nvdaes requested a review from seanbudd May 11, 2024 06:49

nvdaes mentioned this pull request May 11, 2024

[Submit add-on]: Sound Splitter, stable translation update #3455

Closed

seanbudd reviewed May 13, 2024

View reviewed changes

Apply suggestions from code review

9bd941f

seanbudd reviewed May 13, 2024

View reviewed changes

nvdaes added 2 commits May 13, 2024 09:28

Apply suggestions

4182c2a

Add missing config files

a22937d

nvdaes added 2 commits May 13, 2024 14:14

Merge branch 'master' into fichMerge

736adc6

Fix syntax

08f9d43

seanbudd reviewed May 14, 2024

View reviewed changes

.github/codeql/codeql-config-excluding-warnings.yml Show resolved Hide resolved

Analyze critical security issues

e9913b2

nvdaes requested a review from XLTechie May 14, 2024 15:16

seanbudd approved these changes May 16, 2024

View reviewed changes

seanbudd merged commit 71a6483 into nvaccess:master May 16, 2024

nvdaes deleted the fichMerge branch May 16, 2024 03:19

seanbudd mentioned this pull request Jun 3, 2024

Review comments won't be added for manually merged add-ons #3279

Closed

	add-paths: trustedAddons.json
	add-paths: overriddenFalsePositives.json

		@@ -1,12 +1,27 @@
		module.exports = ({core}, path) => {
		module.exports = ({github, core}, path) => {

Create manual approval process for errors with code scanned add-ons #3397

Create manual approval process for errors with code scanned add-ons #3397

Conversation

nvdaes commented May 6, 2024 • edited Loading

Issue number

Summary of the issue

Development strategy

Testing performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XLTechie May 8, 2024 • edited Loading

Choose a reason for hiding this comment

seanbudd commented May 8, 2024

XLTechie commented May 8, 2024 via email • edited Loading

seanbudd commented May 8, 2024

Choose a reason for hiding this comment

nvdaes commented May 8, 2024

XLTechie commented May 8, 2024 via email

nvdaes commented May 9, 2024

nvdaes commented May 9, 2024

nvdaes commented May 10, 2024

nvdaes commented May 11, 2024

nvdaes commented May 11, 2024

nvdaes commented May 11, 2024

nvdaes commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvdaes commented May 13, 2024

nvdaes commented May 13, 2024

I suppose that more suites can be added in the CodeQL settings file (advanced settings of the repo), to determine what will be considered an error or a warning. I've created a testing repo to upload fake add-ons for testing. If you provide me another example to tes, I'll do.

nvdaes commented May 14, 2024

nvdaes commented May 14, 2024

nvdaes commented May 14, 2024

nvdaes commented May 14, 2024

seanbudd left a comment

Choose a reason for hiding this comment

nvdaes commented May 6, 2024 •

edited

Loading

XLTechie May 8, 2024 •

edited

Loading

XLTechie commented May 8, 2024 via email •

edited

Loading

I suppose that more suites can be added in the CodeQL settings file (advanced settings of the repo), to determine what will be considered an error or a warning.
I've created a testing repo to upload fake add-ons for testing.
If you provide me another example to tes, I'll do.