-
-
Notifications
You must be signed in to change notification settings - Fork 595
Update ParseFile to handle a data URI containing a number #483
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
=======================================
Coverage 84.76% 84.76%
=======================================
Files 48 48
Lines 4003 4003
Branches 903 903
=======================================
Hits 3393 3393
Misses 610 610
Continue to review full report at Codecov.
|
src/ParseFile.js
Outdated
@@ -25,7 +25,7 @@ export type FileSource = { | |||
}; | |||
|
|||
var dataUriRegexp = | |||
/^data:([a-zA-Z]*\/[a-zA-Z+.-]*);(charset=[a-zA-Z0-9\-\/\s]*,)?base64,/; | |||
/^data:([\w\/\+]+);(charset=[a-zA-Z0-9\-\/\s]*,)?base64,/; |
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.
@AppOrchestra Nice catch regarding the inability to match digits. Although the way you've set up this regex would also match things with an arbitrary # of slashes, like too//many//slashes/here
. In addition \w
(if I'm correct regarding the regex flavor) will probably match _
as well, which is not specifically desired.
To avoid the included _
character with \w
, and to factor in digits, you could probably do something like:
[-a-zA-Z0-9]+\/[-a-zA-Z0-9+.]+
This basically uses the existing regex, but makes a couple changes. It adds a -
to the first part of the regex, allowing for valid mime types with a -
at the start, like x-conference/x-cooltalk
(which is a thing, although rare). It also changes the quantifier *
for a +
, specifying that we expect at least one or more characters before and after the slash, something like /mimetype
or audio/
shouldn't be considered valid; the existing regex would allow this.
You could also just use \d
instead of 0-9
, totally up to you, just wanted to give you a heads up on that.
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 would recommend you modify the regex to avoid the _
character and to better detect a properly formatted mime type (as detailed in my earlier comment) .
I had a proper look at a few pages: (instead of copying what a popular data uri regex had...) and updated the regex to handle all valid mime-types, with some basic validation, and also fixed it to handle the charset param too. |
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.
Was about to say we can take this when I ran into some trouble with the CI. It may be unrelated to the changes introduced here, but I'm waiting to see what travis says once we've caught up with the master again.
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.
we're all good here. If anyone has any final thoughts before I bring this in let me know, I'll probably merge it later today.
The label |
The regex to extract the components from a base64 encoded string doesn't handle mime types which contain a number, e.g. audio/m4a.
It was failing with the error:
TypeError: Cannot read property '1' of null
at ParseFile (/app/node_modules/parse/lib/node/ParseFile.js:121)
The regex has been updated to allow numbers, and a test added which failed before the change.