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

Data uri validation #1510

Merged
merged 5 commits into from
Jun 29, 2018
Merged

Data uri validation #1510

merged 5 commits into from
Jun 29, 2018

Conversation

Shudrum
Copy link
Contributor

@Shudrum Shudrum commented May 24, 2018

Hi there!

Maybe you'll find this completely useless, or, I hope, useful, but in many of my projects, I use the data URI format to send pictures between my clients and my APIs.

This format is not so simple to validate, and just a regex is not sufficient. This is why I propose to add this validation: string().dataUri().

I do not now what order may be the best, so I put it just near the base64 as the use may be for some people quite the same.

Do not hesitate if you have any questions!

Thank you!

@Marsup
Copy link
Collaborator

Marsup commented Jun 7, 2018

Hey, just a word to tell you I'm not ignoring you.

I see value in this one but the 2nd regex opens up for a ReDoS vulnerability, so I'm scratching my head with a few others to find a way out, but maybe there's none, so we might need to go for a half validation there.

Just let us think about this one for a while, but if you have a solution I'd be glad to read it.

@Shudrum
Copy link
Contributor Author

Shudrum commented Jun 8, 2018

HI!

First of all, thank you for your comment, never heard of ReDoS before ^^'

And I think I see why you said the second regex can be an evil one. I must admit somthing: I haven't write this one by myself. I have just copied the base64 one just before you can find here: 14d4319#diff-d91d658f77d8b9da32acdedc3b5950d0R451

So, now did the two are evils regexp, or the two are good? The difference is if the paddingRequired option is set to false on the base64 test. So I can pick the first one regex: base64 with the padding required: ^(?:[A-Za-z0-9+\/]{4})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$.

I cannot say if this is a really good idea or not. I looked at the pull request #1156 adding the evil regex and see not comment about it.

I add a commit avoiding the evil request and requiring the padding (I have no idea what it is) allowing us to maybe merge this request.

Maybe the pull request #1156 need to be challenged?

@Shudrum
Copy link
Contributor Author

Shudrum commented Jun 8, 2018

(I can remove this commit on demand if you think the regex is not evil).

@Marsup Marsup self-assigned this Jun 9, 2018
@Marsup Marsup added the feature New functionality or improvement label Jun 9, 2018
@Marsup
Copy link
Collaborator

Marsup commented Jun 9, 2018

Oh... That regex was already there then, ahah... 😅
After all, we ruled out that this regex is ReDoS, a flaw in our tools/analysis.
I think it would then be fair to support the same option as the base64 validation, mind doing the modifications ?

@Shudrum
Copy link
Contributor Author

Shudrum commented Jun 9, 2018

Option added, exactly the same way the base64 option.

@Shudrum
Copy link
Contributor Author

Shudrum commented Jun 25, 2018

I know this is not easy to be a maintainer (I used to be a 3000+ stars project main maintainer). But no news about this PR.

Is it forgotten (so there is ping)? Or should I modify something?

Thank you!

edit: Or simply not an important PR, understandable.

@Marsup Marsup added this to the 13.5.0 milestone Jun 29, 2018
@Marsup Marsup merged commit 8b39221 into hapijs:master Jun 29, 2018
Marsup added a commit that referenced this pull request Jun 29, 2018
@Shudrum Shudrum deleted the dataUri branch June 29, 2018 15:55
@Shudrum
Copy link
Contributor Author

Shudrum commented Jun 29, 2018

Thanks. And sorry for the forgotten documentation update.

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants