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

fix: do not allow to upload files with invalid formats #558

Merged
merged 10 commits into from
Feb 22, 2021

Conversation

LauraBeatris
Copy link
Contributor

@LauraBeatris LauraBeatris commented Feb 18, 2021

What: Don't allow to upload files with invalid formats.

Why: Files with formats that aren't accepted aren't uploaded in an input file, in fact, it's not even allowed to select them, so the tests should also follow the same behavior.

How: Verify if the element has an accept attribute, if it does, verify if the file/files match the MIME types specified.

Checklist:

  • Documentation
  • Tests
  • Typings
  • Ready to be merged

@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #558 (1a2192e) into master (ad427a6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #558   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          699       710   +11     
  Branches       221       223    +2     
=========================================
+ Hits           699       710   +11     
Impacted Files Coverage Δ
src/upload.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad427a6...c7b60b9. Read the comment docs.

@LauraBeatris
Copy link
Contributor Author

LauraBeatris commented Feb 18, 2021

Still need to add a test case for the following MIME types: image/*,video/* , or in general, the ones that accept all types of extensions

@LauraBeatris
Copy link
Contributor Author

LauraBeatris commented Feb 18, 2021

Still need to add a test case for the following MIME types: image/*,video/* , or in general, the ones that accept all types of extensions

Handled at a7c715a

I've decided to use regex to validate cases where multiple images, videos, and audio formats are supplied, like "flags"

Given a accept attribute like the following: image/* and file type: image/jpg

  const fileMimeType = type.match(/\w+\/+/g) // image
  const isValidMimeType = acceptAttribute.includes(fileMimeType) // true - image/* includes image 

@LauraBeatris LauraBeatris marked this pull request as draft February 18, 2021 22:58
@LauraBeatris LauraBeatris marked this pull request as ready for review February 18, 2021 23:33
@LauraBeatris LauraBeatris marked this pull request as draft February 19, 2021 01:40
@LauraBeatris
Copy link
Contributor Author

LauraBeatris commented Feb 19, 2021

Still need to improve the validation logic for MIME types - After thinking a little bit more, it would be better to use another dependency to validate the file type. It would return code and make it easier to maintain.

@ph-fritsche
Copy link
Member

ph-fritsche commented Feb 19, 2021

First of all: Thanks for this pull request :)

As the user is not able to select files that don't match the accept field, I'm not sure if any changes are necessary - if it is just up to the author of the test to be sensible about it.
userEvent methods are "simulate this action by the user" and we don't need to simulate actions that a user can not perform because the browser already prevents them from doing so.
But maybe it makes the life of the test author easier when reusing some fixtures and the necessary changes are small.
@nickmccurdy What do you think?

The changes in upload are too spread out over the function.
The change should be just one line at line 16, applying the filter before the slice.

I don't think validating the types here is complex enough to command outsourcing it in another dependency.
Correct me if I'm wrong, but the only allowed variants with wildcard are:

  • type/* which can be easily translated to new RegExp(`^${type}\\/`)
  • * (which is equivalent to not providing an accept attribute)

@LauraBeatris
Copy link
Contributor Author

@ph-fritsche

userEvent methods are "simulate this action by the user" and we don't need to simulate actions that a user can not perform because the browser already prevents them from doing so.

I really agree with that but, there are some situations worth discussing. For instance, if a button is disabled, the user won't be able to click on it, so the browser is already preventing the action - but upload also checks for the disable attribute.

Another situation is, the user may have 4 files and just one of them is not allowed to be selected, so the action will be performed anyway, in that way, I think that it's better if upload follows the same behavior.

Let's also say that an engineer removes the accept attribute of an input component, or removes an specific file type - The tests would break.

@LauraBeatris
Copy link
Contributor Author

LauraBeatris commented Feb 19, 2021

I don't think validating the types here is complex enough to command outsourcing it in another dependency.
Correct me if I'm wrong, but the only allowed variants with wildcard are:

type/* which can be easily translated to new RegExp(^${type}\\/)

  • (which is equivalent to not providing an accept attribute)

I think that * is not a valid value 🤔 , MDN docs just mention audio/*, image/* and video/*

@ph-fritsche
Copy link
Member

Let's follow the (living standard) spec: https://html.spec.whatwg.org/multipage/input.html#file-upload-state-(type=file)

  • anything starting with a dot . - true if the File.name ends on it
  • audio/*, image/*, video/* - true if the File.type starts with it (excluding the *)
  • anything else - true if the File.type is equal

@jeysal
Copy link

jeysal commented Feb 19, 2021

Could throwing or console.erroring (not sure if there's precedent for that in testing lib) be a way to get that benefit Laura mentioned (tests breaking when removing an accept attribute) while also not causing potentially hard to understand behavior where the tester may not know why some files are just being lost?

@ph-fritsche
Copy link
Member

@LauraBeatris I opened a PR with suggested changes here: LauraBeatris#1

@ph-fritsche
Copy link
Member

Could throwing or console.erroring (not sure if there's precedent for that in testing lib) be a way to get that benefit Laura mentioned (tests breaking when removing an accept attribute) while also not causing potentially hard to understand behavior where the tester may not know why some files are just being lost?

Removing an accept attribute would not break the test. Adding one would, but would also be the expected behavior of the test if you exclude a file per accept that you tried to upload.

@jeysal
Copy link

jeysal commented Feb 19, 2021

Removing an accept attribute would not break the test. Adding one would, but would also be the expected behavior of the test if you exclude a file per accept that you tried to upload.

Sorry, I worded that incorrectly - removing a type from the list of accepted types.
And yes, you would want the test to break if that type is being uploaded in it, that's the point.

Wdyt about throw/console.error? Might save some people from getting stuck trying to figure out why their uploads are silently ignored

@ph-fritsche
Copy link
Member

ph-fritsche commented Feb 19, 2021

And yes, you would want the test to break if that type is being uploaded in it, that's the point.

The test would break if it tests for the uploaded files to be applied in that input/change event.

Wdyt about throw/console.error? Might save some people from getting stuck trying to figure out why their uploads are silently ignored

This would also mean that the user could not use userEvent.upload to test the accept attribute.
So there would be exactly the same situation as before this PR.

edit: Maybe put a bit too strict: Yes he could test it, but a change would meaning an expect().toThrow would be needed on test. Which might mask exceptions in the tested code. So no, an exception or console.error is no solution.

fix: apply file accept tokens
@LauraBeatris LauraBeatris marked this pull request as ready for review February 19, 2021 11:46
@LauraBeatris
Copy link
Contributor Author

I'll fix the coverage soon

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

Successfully merging this pull request may close these issues.

4 participants