Add support for Virus Total#3294
Conversation
|
@seanbudd,I think this is readyfor review. |
|
I've created an API key now by the way |
|
@nvdaes how long is the retention period?
What is the workflow going to show that will be helpful if the artifact is missing?
… Note that we are posting a link to the workflow, not the artifact URL: artifacts maybe deleted after the retention period, so linking to the workflows
ensures that we have a permanent link, though this maybe controversial and I'm open to receive further comments.
|
|
Luke wrote:
By default, 90 days: Configuring the retention period for GitHub Actions artifacts and logs in your organization .
Just that it failed. |
|
Personally I think the artifact is what is important, and 90 days is more than enough time for NV Access to decide whether to override the decision of the automated system.
If 90 days is the limit, then think we should have a cron-timed cleanup workflow that posts a comment and closes any open PRs/issues that failed their initial check.
Github already has a sample workflow for this, all we need to do is use it with a label for failed security checks. It will even post an appropriate comment.
That is nearly a whole NVDA release cycle's worth of time, and I think the author should be able to submit a replacement issue by then if NV Access hasn't responded.
I don't believe there is value in keeping these open indefinitely if the artifact expires, nor are authors likely to be patient that long in any case.
|
|
Luke Luke wrote:
If you can, please share the mentioned GitHub workflow. |
|
Also, note that, in this case, the artifact is generated even if VirusTotal doesn't flag an add-on as malicious, to see other results like Non detected, suspicious, etc. |
|
It is the stale issue/PR closer workflow.
In this case we would need to modify it so it only looks for issues that have
the "failed-virusTotal-check" or similar label, instead of just generic stale
issues/PRs.
Tutorial (basic, not how we would use it exactly, but it gets there):
https://docs.github.com/en/actions/managing-issues-and-pull-requests/closing-inactive-issues
Full docs: https://github.com/marketplace/actions/close-stale-issues
|
|
See these input keywords:
only-labels
days-before-stale
days-before-close
stale-issue-message
stale-pr-message
close-issue-message
close-pr-message
stale-issue-label
close-issue-label
delete-branch
Oh, also debug-only (for testing).
|
|
@nvdaes - unrelated, but would you consider adding this for NVDA releases? right now we manually upload release builds (beta, rc, stable) to virus total. I think it would be worth also scanning alpha builds if we can do it automatically. Right now it will require some work to create an NVDA installer artifact to scan in GitHub Actions. |
|
@seanbudd Are you thinking to have AppVeyor notify a webhook when the merge compile is complete, including the same artifact uploaded to NV Access server, or the link to same?
|
|
@XLTechie - I think ideally compiling NVDA on GitHub actions, it would be a big step to help us migrate from appVeyor. But yes, we can also use a webhook to trigger the action |
|
About NVDA and VirusTotal, I used Appveyor years ago to manage add-ons, but I prefer GitHub Actions since it was simple for me, and I felt that the process to add projects to Appveyor was not very accessible and required to emulate a mouse click. So I haven't investigated about Appveyor from that time. |
@seanbudd I wasn't aware that was a goal of NV Access. @LeonarddeR seems to think it's impractical (nvaccess/nvda#10516 (comment)), and the discussion is still closed on that subject. |
|
Interesting subject, I will update nvaccess/nvda#10516 with most recent findings. |
|
I've triaged nvaccess/nvda#10516 and added more information there . This PR should also be unblocked now. |
|
Note: The sha of the malicious add-on has been commented without removing it from the js file used for the VirusTotal analysis. Should we remove it? |
|
@seanbudd I've addressed your review and checked that:
|
Can you please make this one file? There's no need to maintain two separate whitelists for add-ons |
|
|
@seanbudd, now we use a file named reviewedAddons.json for CodeQL analysis and VirusTotal. After testing that the PR includes a unique file, I've set the overwrite for the manualApproval artifact in VirusTotal and in the CodeQL analysis. The sha256 will be the same, so in the real life this can be done. |
|
This PR now contains a large number of files in addons/readFeeds |
|
Sorry. I test this creating or relabelling issues and making changes on the master branch of my fork, and sometimes changes are committed accidentally to this branch. |
| uses: ./.github/workflows/codeql-analysis.yml | ||
| createManualApproval: | ||
| needs: [getAddonId, virusTotal-analysis, codeQL-analysis] | ||
| if: ${{ always() && contains(join(needs.*.result, ','), 'failure') }} |
There was a problem hiding this comment.
will this will happen if getAddonId fails?
There was a problem hiding this comment.
No, since this needs codeQL-analysis and virusTotal-analysis, which need createPullRequest, which needs verifySubmitter, which won't be run if getAddonId fails.
|
thanks @nvdaes - this almost ready just one more question |
|
Thanks @nvdaes for all your efforts here! |
|
Thank you @seanbudd for your reviews. Testing has been hard, so thanks for your patience 😀 |
Summary of the issue In #3294 , a workflow contains a non indentent body. This bug has been reported by @josephsl and @mltony. See issue #3633. @josephsl has detected the exact bug. Yo u may want to review this @josephsl. Test nvdaes/addon-datastore/actions/runs/9237486054 Note: I've exceeded my VirusTotal API key limit today, trying to analyze all add-ons of the store, but the workflow syntax should be fixed.
Issue number
Fixes issue #3246
Summary of the issue
VirusTotal may catch malware bundled with add-ons.
Also, knowing the sha256 of scanned add-ons, the URL to see results at different datetimes maybe built, allowing users to see this information even before installing an add-on if this was included in the NVDA store in the future.
Development strategy
Testing performed
nvdaes#1299