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

Update hapijs/joi to 6.0.5 from 5.0.2 #2453

Closed
hueniverse opened this issue Mar 10, 2015 · 9 comments
Closed

Update hapijs/joi to 6.0.5 from 5.0.2 #2453

hueniverse opened this issue Mar 10, 2015 · 9 comments
Assignees
Milestone

Comments

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 10, 2015

No description provided.

@hueniverse hueniverse self-assigned this Mar 10, 2015
@hueniverse hueniverse added this to the 8.3.0 milestone Mar 10, 2015
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Mar 10, 2015

The introduction of "" around key names in error messages is causing all validation errors to include escaped HTML characters due to security. I am not sure what the right solution here is:

  • stop encoding "
  • encode " and just have ugly error messages
  • something else?

@evilpacket @geek @Marsup - thoughts?

This is the last item left to get 8.3 done.

hueniverse pushed a commit that referenced this issue Mar 10, 2015
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Mar 10, 2015

You can see the issue via the two failing tests on master.

@ghost
Copy link

@ghost ghost commented Mar 10, 2015

How about using a different kind of quotes (Backticks?) in the Joi messages?

@evilpacket
Copy link
Contributor

@evilpacket evilpacket commented Mar 10, 2015

I would recommend against stop encoding " as you know somebody is going to cram an error message someplace stupid and end up with content injection of some type.

Encoding quote would only give you ugly error messages when you are not in a browser context right? The general rule is that output should be encoded properly for the context in which it's headed. I hate suggesting it but a flag / option to disable the escaping for use in other context that do not require that type of output encoding (such as a cli?)

@kanongil
Copy link
Contributor

@kanongil kanongil commented Mar 10, 2015

It is impossible to guard against every conceivable usage of a json encoded server response. For example, the message can be templated into an HTML body, but could also be templated to a javascript var. The current implementation guards against the first but not the later which seems arbitrary.

Given this, I think that it should be up to the consumer to escape it properly depending on the scenario.

@geek
Copy link
Member

@geek geek commented Mar 10, 2015

@hueniverse can you point to where the encoding takes place?

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Mar 10, 2015

Encoding used to take place in joi but not anymore. It used to encode the inputs in any error message (e.g. keys). Now that it doesn't, hapi has to do it, but at the point hapi gets it, the " is already added to the error message by joi. These quotes are safe because they are added by joi around the user provided value. If joi went back to escaping those, we can have quotes and no security issues.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Mar 10, 2015

I don't know where that came from, and didn't detect it since it's not tested in Joi. Why should Joi bother with html, I don't see such a strong relationship, hapi though... I'll try to get back that old behavior anyway since it was an undesired side effect, but if someone could clarify.

@hueniverse hueniverse changed the title Update hapijs/joi to 6.0.4 from 5.0.2 Update hapijs/joi to 6.0.5 from 5.0.2 Mar 10, 2015
@Marsup
Copy link
Contributor

@Marsup Marsup commented Mar 11, 2015

Btw was wrong, it was tested before I merged the PR on message, my bad.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants