Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Authoring): Run virus scanner on asset upload #191

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

geoffreykwan
Copy link
Member

Changes

Run virus scanner on files that are uploaded by an author.

Test

Install and start up Clam AV (you can look at the changes to scripts/beforeInstall.sh and perform the equivalent for your system)

  1. Install Clam AV
  2. Run freshclam to update the virus definitions
  3. Update the clamav configuration file. For TCPAddr you may need to find your local IP and use that instead of 127.0.0.1 since we use Docker locally and Docker might intercept connections to 127.0.0.1.
  4. You will also need to change the CLAMAV_ADDRESS in ProjectAssetAPIController.java from 127.0.0.1 to your local IP.
private String CLAMAV_ADDRESS = "127.0.0.1";
  1. Start Clam AV

Download mock virus files from https://www.eicar.org/download-anti-malware-testfile/ (if your browser prevents downloading the files, you can try downloading using wget).

  • eicar.com.txt
  • eicarcom2.zip

Test uploading files on WISE

  • Try uploading the two mock virus files to WISE. You should not be able to upload them and should receive an error.
  • Try uploading files that are allowed. They should be allowed.
  • Try uploading a mock virus file and a file that is allowed at the same time using drag and drop. The allowed file should be uploaded but the mock virus file should return an error.
  • Try uploading files until the project asset limit for the project no longer has any more space. You should not be able to upload files past the project asset disk space limit.

Closes #190

Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

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

Functionality seems to work great.

I have two suggestions regarding code:

  1. Allow users to set CLAMAV_ADDRESS in application.properties. This will allow users to change the address without having to recompile the code.
  2. Move the if (clamavClient == null || isScanOk(clamavClient, file)) { check from addAsset() to the isUserAllowedToUpload() function. This would simplify the addAsset() function and put code that returns UPLOADING_THIS_FILE_NOT_ALLOWED_MESSAGE together.

@geoffreykwan
Copy link
Member Author

The clamav server address is now specified in the application.properties file.

Test these scenarios and make sure everything still works

  • clamav.server.address is not in the application.properties file at all
  • clamav.server.address is specified but the value is empty
  • clamav.server.address is specified

Copy link
Member

@hirokiterashima hirokiterashima left a comment

Choose a reason for hiding this comment

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

Looks good.

If you pass in a second param to getProperty() that gets used as a default value, can you get rid of the null check and it would behave the same?

String clamavServerAddress = appProperties.getProperty("clamav.server.address", "127.0.0.1");

@geoffreykwan
Copy link
Member Author

If there's a default value, then it will always try to make a connection/scan request even if the server admin never intended to install Clam AV. This would be unnecessary processing.

@geoffreykwan geoffreykwan merged commit 208558d into develop Nov 23, 2022
@geoffreykwan geoffreykwan deleted the issue-190-run-virus-scanner-on-asset-upload branch November 23, 2022 16:17
@geoffreykwan
Copy link
Member Author

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Run virus scanner on asset file uploaded by author
2 participants