Review comments won't be added for manually merged add-ons #3279
Comments
|
I think there needs to be a mechanism to create exceptions for certain add-on versions (i.e. like approved submitters, but for addon SHAs). |
|
In fact, I think that allowing errors is risky. Perhaps a workflow maybe created for create a review comment, then merge, then transform, with a workflow_dispatch event to be triggered manually. |
|
What do you think about the mechanism of allowing add-on version SHAs to bypass codeQL and virus total checks |
I don't understand this mechanism well, since a version is supposed to be submitted just once, an two version of different add-ons may be completely different. |
|
The idea is that when the submission fails, a PR is opened to add the SHA to a list of human reviewed submissions. If NV Access overrules the code scanning, the PR is merged. Then the issue can be relabeled, and the submission action can succeed. |
|
@nvdaes Could the code which creates the review be broken out into its own workflow, that runs on merge? This seems like a task that could be made independent.
It could check whether any merged add-on has no review URL, and if not, create one.
As a side effect, the first run would probably open review issues for any add-ons which haven't been submitted since reviews started.
|
Ah, seems a good idea. |
|
Luke wrote: It could check whether any merged add-on has no review URL, and if not, create one. I think this may cause problems and unpredictable effects, since several calls to GitHub API to create discussions may be needed, and I remember that these multiple API calls didn't work for my tests. |
|
Another approach would be to create an "overruled" label to be applied to the PR opened when the issue was labelled. Then, the PR could be merged and further jobs maybe also called. In this way,relabelling the issue and closing the created PR with the add-on metadata wouldn't be needed. Also, pull request for overruled add-ons would hava a label to make easier to distinghish or search them on GitHub. |
I don't think this is a viable solution as it would allow users with labelling permissions to get around our security checks |
OK, I understand. |
|
I think we may use a custom composite action to check if the add-on is added to trustedAddons, since this may also be used for virusTotalAnalysys in the future, so creating an action to be used as a single step maybe much easier. |
|
I'mve started the creation of a composite action to be used as a single step when secure analysis (or, eventually, virus total in the future) fails. |
|
I comment here about my progress, in case this isnot the right direction. |
|
I'll try another approach after many tests. I think that when analysis has succeeded, the sha256 should also be added to a trustedAddons array in a file named trustedAddons.json. Then, merge master will deppend on a job named verifyTrustedAddons, which can check if the sha256 of the current submission is there. A trusted add-on can be added when analysis has been successful, and also if a created PR when analysis has failed is merged. |
…3397) 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.
If someone submits an add-on which fails codeQl analysis, and NV Access merges it manually, the review URL won't be shown in the store.
The text was updated successfully, but these errors were encountered: