Checking MIME types and file extension #7083
Conversation
Pull Request Test Coverage Report for Build 9443854722Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| expected_mime_type = Marcel::MimeType.for extension: file_extension | ||
|
|
||
| if content_type != expected_mime_type && content_type != 'application/octet-stream' | ||
| flash_message(:warning, "The uploaded file doesn't match its file extension (#{file_extension}).") |
There was a problem hiding this comment.
Internationalize this string
| expect(@student).to have_accepted_grouping_for(@assignment.id) | ||
|
|
||
| # Check MIME type for a correct file |
There was a problem hiding this comment.
You should create new tests rather than add to existing tests. Define different unit tests for each test input.
|
@AinaMerch looks like the tests failed, please take a look at that |
| params: { course_id: course.id, assignment_id: @assignment.id, new_files: [valid_file] } | ||
| expect(response).to have_http_status :ok | ||
|
|
||
| # update_files action assert assign to various instance variables. |
There was a problem hiding this comment.
You don't need to include the checks for "assigns" here or in the test below, they aren't relevant to this particular test.
| expect(@student).to have_accepted_grouping_for(@assignment.id) | ||
|
|
||
| invalid_file = fixture_file_upload('docx_file.docx', 'application/pdf') | ||
| allow(Marcel::MimeType) |
There was a problem hiding this comment.
Instead of using mocking, create a second test file: take a word document and rename it so that its extension is .pdf. Make sure the file name itself communicates that this is what has occurred, so that there's no confusion later. This creates a more authentic test than just mocking alone.
| params: { course_id: course.id, assignment_id: @assignment.id, new_files: [invalid_file] } | ||
|
|
||
| expect(response).to have_http_status :ok | ||
| sample_warning_message = "The uploaded file doesn't match its file extension (#{file_extension})." |
There was a problem hiding this comment.
Don't use string literals, use the internationalized string you defined
spec/fixtures/files/text_file.txt
Outdated
| @@ -0,0 +1 @@ | |||
| abcd | |||
There was a problem hiding this comment.
If this file is not being used in testing that's fine, but in this case you should remove it
config/locales/en.yml
Outdated
| @@ -35,6 +35,7 @@ en: | |||
| empty_file_warning: "%{file_name} is empty" | |||
| external_submit_only: MarkUs is only accepting external submits | |||
| file_conflicts: 'Your changes have not been made. The following file conflicts were detected:' | |||
| file_extension_mismatch: The uploaded file doesn't match its file extension %{extension}. | |||
There was a problem hiding this comment.
Reword to "The contents of the uploaded file do not match the file extension %{extension}".
Proposed Changes
When a student submits a file, use the Marcel gem (https://github.com/rails/marcel) in the back-end to guess the mimetype of the uploaded file, and compare against file extension (note that Marcel can also tell the expected mimetype based on file extension). If there's a mismatch, flash a warning message to the student, but allow the file to be submitted as normal.
In doing this, the main use case, checking .docx to .pdf works, but .txt to .pdf doesn't flash a warning. A test case that fails is added to show that issue.
...
Screenshots of your changes (if applicable)
Associated [documentation repository](https://github.com/MarkUsProject/Wiki) pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)