@seanbudd, This has been tested in

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

@nvdaes - can you show an example of a failure?

@nvdaes - can you show an example of a failure?

Unfortunately, the CodeQL action seems to fail just when is triggered in a PR, not for other events for example not when the code is analized on workflow_call or schedule events.
Anyway, if it detects a failure, an alert can be found in the security page of the repo, so that NV Access may check these alerts for add-ons submitted to the store and request authors to fix them or remove the add-on.

We may use the .sarif files generated by the analyze action to see if the results property contains elements, and then post a comment in the corresponding issue requesting authors to contact NV Access. I'll convert this to a draft for now.

I think this can be reviewed again.
Since analysis was always successful,even when errors were found, since it seems to fail just for pull requests or whendifferences in code ara analized,not when analysis is performed for a set of files for example on workflow_call, I've created a securityAnalysis.js file to determine if erors are present in results (with default configuration, since a deeper analysis may produce more false positive results).
Iferrors are found, the jobwill fail now and an issue commentwill be post, and the issue will be closed. In this case, an artifact with a sarif file will be uploaded to the action, since I don't know if reading lots of alerts in the security scanning page of GitHub will be very comfortable or accessible. In this way authors may see the produced artifact or NV Access may provide it.
We may also provide the artifact URL in the issue.
I don't think that this is violating security reporting good practices, since codeQL is publicly available and authors should know about this, that maybedocumented in the readme or in the issue template.
Also, if authors don't include source code in their add-ons, security analysis will be successfull. We may check the sarif files to make a warning somewere if no files are analyzed for Python and Javascript.

I've tested this in the following workflow runs:

• Add-on containing Python source code without failures related to code analysis
• Add-on without errors, just with cpython files
• Add-on with error in Python file, contains good JavaScript
• Add-on without errors

Note that some workflows fail due to things not related to analysis.

cc: @CyrilleB79 (security reporter and analyst)

@nvdaes Given that you propose blocking submission on failure, can you please
explain this point in more detail?

I was meaning workflows on my fork of the store, shown here as examples of tests made by me. Some workflows may fail if the used add-on is already on my fork of the store, and I mentioned it to indicate that the important thing in those cases is to pay attention to the workflow until the analysis job.
Hope this clarifies what I wanted to mean. This was related to my tests, not to the pull request itself.

Can you please add documentation on how to handle these failures as a reviewer?
For example, I'd encourage using a Sarif Viewer, particularly the VS Code one: https://marketplace.visualstudio.com/items?itemName=MS-SarifVSCode.sarif-viewer

Even better, can these Sarif files become viewable in GitHub?
From what I can understand, these can be uploaded via these steps and viewed at a URL like https://github.com/nvaccess/nvda/security/code-scanning

Do you confirm that this check is mandatory to have an add-on included or updated in the store? If yes, this probably needs to be documented. Thanks.

I would say that any failures will need to be manually overruled by NV Access

Sean wrote:

I would say that any failures will need to be manually overruled by NV Access

I think this is a good idea. Anyway I'll try to add documentation in the submitters guide. I think this is the best place, and, after testing the VS Code extension, I'll add a note about it, as well as about how to see sarifs on GitHub, though I don't know if this would be fully accessible, deppending of the number of errors and so on.

Is there anything other than VS Code extension (which many/most add-on devs
don't use), or "possibly inaccessible" GitHub solution?

According to my tests, in the security code scanning page of GitHub, alerts are shown like issues and we can press enter in the alert title.
Example:

Title: Incomplete URL substring sanitization

High
#1 opened
Body of the alert:

Incomplete URL substring sanitization
High
#1 opened last month • Detected by CodeQL in addon/.../readFeeds/init.py:681
master

Also, since this maybe incomplete, when the analysis fails, in this PR we make that GitHub Actions download the add-on and the sarif files containing failures to the artifact sections of the GitHub workflow.
Testing VS Code, I feel that the Sarif Viewer extension can be used, but this is not friendly. I moved to the status bar, pressed a related button and so on.
I prefer to open the sarif file with Notepad++, or with VS Code, and search with control+f, for example, for startline, to see where failures are found. I'll attach here an artifact from readFeeds 26.1.0.

results-python.zip
This is a sample artifact showing an error
results-python.zip

I'll mention this VS Code extension.
Whe a sarif file is opened, we can press NVDA+space and then t, so that NVDA moves to the table of errors. There, pressing enter in a clickable result, more details have been shown.
Extension:

Name: SARIF Explorer
Id: trailofbits.sarif-explorer
Description: SARIF Explorer: Explore static analysis results effectively and enjoyably.
Version: 1.1.0
Publisher: Trail of Bits
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=trailofbits.sarif-explorer

Output after clicking:

py/incomplete-url-substring-sanitization
1

Result marked as TODO
init.py:681
The string https://www.google.com may be at an arbitrary position in the sanitized URL.

No result selected

I've made a commit to add the GitHub workflow link to the issue when the bot comments about code analysis failure, so that documenting and seing results and artifacts can be easier.
Otherwise, in the documentation we will need to say something about "the corresponding workflow", and this would be confusing.

See tests for this at

nvdaes#999

@seanbudd , I've added documentation. This is ready for review.

I think we should remove the info about Sarif Explorer extension for VS Code. I don't get consistent results across different computers, and now I cannot use the extension after installing it.
For me the best approach is to open the sarif file in Notepad++, or in VS Code, searching text like the word "region" to see where the errors are located.

I'll apply all suggested changes, including the quotation about security.
I've tested the sarif viewer with Firefox and I'll use it for myself, so I think it's a good idea.

@seanbudd, I think this is ready.

Thanks for merging this, @seanbud.
I think we should inform about this on the NVDA add-ons mailing list, so that people is aware of this change and know that their code can be analized when submitting add-ons.
I'll do it tomorrow, if you don't prefer to do it on behalf of NV Access.