Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
Allow "accept" filetype validation to contain a blank among the accepted MIME types#1441
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The backstory on this change is that on the Windows platform, if you install WinRAR (or WinZip or 7Zip), those tools will very often screw up the extension-to-mimetype mapping of the OS, so the file object's .type has an EMPTY STRING value when it gets here for validation.The intuitive thing to do is support EMPTY STRING by just putting a comma prefix on the accept value, such as "accept=',application/zip,application/x-zip-compressed'". But unfortunately that turns the regex into a mess that will accept ANY type.This patch handles the case of a blank "accept" type before RegExp has to.(Also, the RegExp has a period where I think a "\\." should be, but I made that change in a separate pull req since it's unlikely to be controversial, whereas this one may raise some eyebrows and require further discussion).
Thank you for the contribution. I've merged the other two PRs. This one is certainly more controversial. I don't like that its difficult to even reproduce the issue, so I can't really test this change. Since this is in the "additional" category, I'm okay with landing it anyway. @staabm any thoughts on this? |
| @@ -14,11 +15,23 @@ $.validator.addMethod("accept", function(value, element, param) { | |||
| // If we are using a wildcard, make it regex friendly | |||
| typeParam = typeParam.replace(/\*/g, ".*"); | |||
| // Allow checking for BLANK mimetype. | |||
| if (typeParam.match(/^\|/) || typeParam.match(/\|\|/)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess this wont work when the EMPTY STRING is the last element in the param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we also need to handle "space only" comma delimited values like a empty qualifier?
Update: scratch this note: spaces are allready strip when this line is reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oooo, good catch! I overlooked the possibility of ending the parm with a comma separator, and that should definitely have the same effect as a double-comma or a comma prefix.
Let me see if I can figure out how to revise a pull req from this web interface...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just do another commit at the same branch. We can squash it later when everything looks fine.
Could you also add a few unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done!
As for unit tests, I poked around in the tests directory and couldn't find any existing tests of the "accept" method. There are lots of tests for the "extension" method in /test/methods.js, but... I'm not super familiar with this codebase but I thought "accept" was intended to supercede "extension"?
An oversight in original patch didn't accommodate a comma at the END of the type param, only the beginning or middle. This corrects that problem.
Here's an illustration of why those blank choices need to be handled separately by this code and NOT by RegExp. Basically, the RegExp library that javascript uses has a known bug in it. Javascript console excerpt:
As you can see, RegExp "finds" a match where there really ISN'T a match, just because of the double-| (or blank prefix | or blank suffix |) in there. |
I am fine with the change, only thing missing is a test. |
Oh, very nice!#1373 looks good to me, but I'm not sure how to merge someone else's branch into my own using github's web interface. If you go ahead and merge#1373 into master, this PR will have merge conflicts that I'm pretty sure I can figure out how to clean up, and add more "accept" test cases to. |
Well, you can't run the tests from the GitHub web interface either (right?). Making a local clone and editing there shouldn't be a big barrier. Let me know if you need help with that. |
@rskm1 are you still interested in this? Your help is definitely very welcome. |
Closing, as its very old. if you still need this, please shout out. |
The backstory on this change is that on the Windows platform, if you install WinRAR (or WinZip or 7Zip), those tools will very often screw up the extension-to-mimetype mapping of the OS, so the file object's .type has an EMPTY STRING value when it gets here for validation.
The intuitive thing to do is support EMPTY STRING by just putting a comma prefix on the accept value, such as "accept=',application/zip,application/x-zip-compressed'". But unfortunately that turns the regex into a mess that will accept ANY type.
This patch handles the case of a blank "accept" type before RegExp has to.
(Also, the RegExp has a period where I think a "." should be, but I made that change in a separate pull req since it's unlikely to be controversial, whereas this one may raise some eyebrows and require further discussion).