Skip to content
This repository was archived by the owner on Apr 17, 2026. It is now read-only.

Add support for VirusTotal#61

Merged
seanbudd merged 6 commits into
nvaccess:masterfrom
nvdaaddons:virusTotal
Jun 11, 2024
Merged

Add support for VirusTotal#61
seanbudd merged 6 commits into
nvaccess:masterfrom
nvdaaddons:virusTotal

Conversation

@nvdaes

@nvdaes nvdaes commented May 27, 2024

Copy link
Copy Markdown

Issue number

Fixes issue nvaccess#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

  • Virus Total CLI is installed when needed.
  • Add-ons are scanned when the submission issue is created.
  • Info about the add-on file is requested to Virus Total later, when the pull request is created, to give time to Virus Total to show results, trying to avoid getting empty analysis.
  • NV Access needs to create an API key in Virus Total.
  • The addonMetadata.json artifact is used to get the add-on id and sha256.
  • A falsePositiveAddons.json file has been added. If VirusTotal analysis fails, a pull request will be created adding the sha256 of the addon to a list associated with the add-on ID, in the falsePositiveAddons.json file.
  • If VirusTotal should be skipped for this add-on, NV Access will merge the created pull request, delete the branch created for the submission (in the form submitterIssueNumber), and relabel the issue to trigger a new workflow.

Testing performed

  • Use the sha256 of a malicious add-on, simulating that this sha correspond to a non malicious add-on.
  • Use the real sha256 calculated for the same add-on.

nvdaes#1299

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Tested with a valid add-on:

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

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Test simulating that customNotifications is a malicious add-on, hard-coding the sha256 of a flagged add-on (here we see the VirusTotal failure):

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

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Last test: after merging the manualApproval PR, simulating that NV Access accepted this add-on as a false positive:

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

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

@seanbudd , this is ready for review.

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Sorry, I remembered that codeQl analysis workflow needs to be updated so that reviewedAddons.json can be used for codeQl and virusTotal, and the manualApproval pull request considers both analysis. I'll submit tests now.

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Test: virusTotal success, codeQl excluding warnings fails:

https://github.com/nvdaes/addon-datastore/actions/runs/9259669099/job/25472099153

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Test: maualApproval PR merged after analysis failure. The submission issue is closed as completed via its PR:

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

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Test: making VirusTotal and codeQL analysis fail, to see how a unique pull request for manual approval is created, and just an issue comment requesting to keep the submission issue opened. readFeeeds 24.0.0 and the same issue testing just codeQl failure is ised:

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

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

I think that all changes are tested. The submission issue for readFeeds 24.0.0 can be found at

nvdaes#1387

@nvdaes

nvdaes commented May 27, 2024

Copy link
Copy Markdown
Author

Si I think this is ready for review, @seanbudd

Comment thread README.md Outdated
Comment thread .github/workflows/sendJsonFile.yml Outdated
Comment on lines +84 to +87
git checkout -b ${{ github.event.sender.login }}${{ steps.get-data.outputs.issueNumber }}
git add .
git commit -m "Submit add-on"
git push origin ${{ github.event.issue.user.login }}${{ steps.get-data.outputs.issueNumber }}
git push origin ${{ github.event.sender.login }}${{ steps.get-data.outputs.issueNumber }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should remain as is. Please don't use the event sender

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment thread .github/workflows/sendJsonFile.yml Outdated
Comment thread .github/workflows/checkAndSubmitAddonMetadata.yml
nvdaes and others added 2 commits June 11, 2024 05:20
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@nvdaes

nvdaes commented Jun 11, 2024

Copy link
Copy Markdown
Author

@seanbudd , I think all your suggestions are applied.

Comment thread .github/workflows/sendJsonFile.yml Outdated
@seanbudd

Copy link
Copy Markdown
Member

note it seems I am unable to commit/push to your fork

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

nvdaes commented Jun 11, 2024

Copy link
Copy Markdown
Author

I've applied your last suggestion about sender. I'll try to grant you access to my fork.

@nvdaes

nvdaes commented Jun 11, 2024

Copy link
Copy Markdown
Author

@seanbudd , I have send you an invitation as an admin of my fork. Please accept it.

@seanbudd

Copy link
Copy Markdown
Member

@coderabbitai review

@seanbudd seanbudd merged commit 8a76eb0 into nvaccess:master Jun 11, 2024
@seanbudd seanbudd deleted the virusTotal branch June 11, 2024 05:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants