HTML escape in internationalized message about play1 HOT 9 OPEN

@asolntsev Why isn't it a security hole? Basically, it allows unescaped HTML to sneak into the page content via using parameterised messages. It's true that the message text comes from messages.* file, but the parameter values can come directly from the user.

from play1.

Thanks to your explanation about the wrong pull request, the multiple make sense !
Please let me quote your points from the PR in this discussion so everybody can read them :

However, I tend to disagree with your reasoning that it's not a security issue, for 2 reasons:

• It's really easy to pass unescaped user input inadvertently, and I'd expect a framework to handle it for me;
• Play! framework does escape ${} strings in templates, so it's quite natural that you'd expect similar behaviour from &{} syntax. Passing html code in parameters can be handled with calling .raw() explicitly.

Your points make sense. They made me wonder :

• if &{...} parameters are escaped, how could you pass parameters that you don't want to be escaped ? Do you mean you would call .raw() on each parameter ?
• I'm concerned about the fact that it could be a breaking change

Finally, I performed some tests in one of my apps and, indeed, it's easy to to insert some user script that would not be escaped. So, while I'm still not sure about the best way to fix this, I definitely agree that this is an issue.

from play1.

@Rotkaf I agree. Play framework should escape parameters of messages.

@xael-fry What do you think?
Probably we could make it at least configurable (not to break existing functionality)?

from play1.

@Rotkaf @xael-fry I have changed my mind :)
Actually it's not a security hole because all messages come from messages.* files that are located inside application code and never come from user. So, hacker cannot put <script> there.

from play1.

@sergey-moshnikov I do not think too that it is a security hole, because no parameter value should come directly from the user. Instead, it would be the responsibility of the controller, for example, to escape the user input properly. Except this case, messages are indeed controlled by the application, so no arbitrary value should get in.

from play1.

Not to start a debate, but there's escaping in ${} by default, why &{} should be less safe in that regard?

from play1.

@sergey-moshnikov I think it IS a potential security hole.

Apparently, I just was drunk when I was writing #1038 (comment) :)

@tomparle Yes, for sure, no parameter value should come directly from the user. In ideal world. But in reality, it happens quite often. The &{msg, args} syntax makes it too easy to make such a bug.

@xael-fry @tomparle I believe we should merge #1318.

from play1.

there's escaping in ${} by default, why &{} should be less safe

There is one explanation that may sound reasonable. Arguments of ${} mostly come from unauthorized source (like user input), while arguments of &{} mostly come from messages.en or similar file from the project. In our projects, these messages often contain html like Please <a href="...">register</a> to get discount - so it's convenient to disable html escaping by default. But not for arguments!

from play1.

@asolntsev I agree with you that it may happen more than I first expected, that was the point of my test as I commented at the end of my previous post.

Two points :

• it makes sense that it would be disabled for messages, but html escaping should be enabled for arguments. This would be a change that should be documented with a (simple) migration guideline
• as @sergey-moshnikov said, #1318 is an incorrect PR since it embeds a lot more than intended. It should be first curated to only deal with the message escaping before it is reviewed then merged.

from play1.