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

Nested keys should not be escaped by default #854

Closed
mathieumg opened this issue Dec 30, 2016 · 7 comments
Closed

Nested keys should not be escaped by default #854

mathieumg opened this issue Dec 30, 2016 · 7 comments

Comments

@mathieumg
Copy link

@mathieumg mathieumg commented Dec 30, 2016

This commit d367309 made escapeValue default to true which is technically breaking, but I get the security concern behind it. However, a side-effect of that was that it started escaping nested keys by default. Nested keys should be trusted by default in my opinion, since you control what content your other keys contain. Obviously, if the keys you nest themselves have interpolation they should still escape that interpolation.

Basically, this is ok:

value = this.escapeValue ? regexSafe(this.escape(value)) : regexSafe(value);

This should not escape by default:

value = this.escapeValue ? regexSafe(utils.escape(value)) : regexSafe(value);

I did not submit a PR because I'm not sure of the approach you want to take here: never escaping when nesting, add a separate option to control nesting escaping or something else. It will be my pleasure to submit one once an approach is chosen.

Thank you!

@jamuhl
Copy link
Member

@jamuhl jamuhl commented Dec 31, 2016

Will be back from my xmas holidays on 9th january...will have a deeper look then. Thanks for reaching out with that side effect.

@mathieumg
Copy link
Author

@mathieumg mathieumg commented Dec 31, 2016

No problem, I set escapeValue to true for the time being since I trust/already sanitize everything I send to interpolate, but it might not be the case for everyone so that would be a dangerous "workaround" for them. Enjoy your holidays! 🍻

@jamuhl
Copy link
Member

@jamuhl jamuhl commented Jan 9, 2017

i think it would be safe to remove the escaping there...can't figure out a way someone could exploit that for an xss attack.

so basically i think removing that line and publishing a major version should be ok. Still if someone more clever sees a risk we could introduce an additional options (which i prefer to avoid - enough options right now ;) )

@mathieumg
Copy link
Author

@mathieumg mathieumg commented Jan 11, 2017

Did you want me to submit the PR that does that or were you taking care of it? Thanks!

@jamuhl
Copy link
Member

@jamuhl jamuhl commented Jan 11, 2017

I can do that...it's just removing a line of code ;)

You need that fast? Just to plan - can do a release the next days - or wait a little to include some other features.

@mathieumg
Copy link
Author

@mathieumg mathieumg commented Jan 11, 2017

No hurry, I use the escapeValue as explained in #854 (comment) for the time being. Just wanted to follow-up! Alright, I'll let you do it when there is the next major release then. Thanks!

@jamuhl
Copy link
Member

@jamuhl jamuhl commented Jan 16, 2017

published in i18next@5.0.0

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

Successfully merging a pull request may close this issue.

None yet
2 participants