nvaccess
Review comments won't be added for manually merged add-ons
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.
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). That way after manual approval the issue can be relabeled and handled automatically. Perhaps we could allow specific errors, but I think that's risky as context could change over time.
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
What do you think about the mechanism of allowing add-on version SHAs to bypass codeQL and virus total checksWhat 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.
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.
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. Also, a job to create a JSON file with data about overruled add-ons, like issue and PR number, sha256, id of publisher etc., maybe called too.
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
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 think I'll try to create it, like I did with the used build-discussion action.
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. For now it can read the sha256 of a file specified as an input, and add the sha256 with a value of true or false. Later, my plan is to add another step in the composite action to create a PR if the add-on sha256 hasn't previously added. The action, in its initial stage, can be found at
https://github.com/nvdaes/setTrustedNvdaAddons
I comment here about my progress, in case this isnot the right direction. For now, my composite action can add an array with sha256 of trusted add-ons, and create a PR. I've thought to add a step in the CodeQL analysis job, to see if the sha256 of the current add-on is included in the array of trusted add-ons. If it's included, other stepsof the would be skipped. If the analysis is performed and it fails, then aPR adding the sha256 to trusted add-ons will be created. In this way, if the PR is merged and the issue is relabelled, code analysis will check that the add-on is trusted and further steps will be skipped.
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.
@seanbudd , I think that this can be closed since there shouldn't be needed to submit add-ons manually, and when a PR for manual approval is created, the mentioned issue shouldn't happen anymore. Anyway I won't close this in case you want to wait to fix the VirusTotal issue or something.