Multiple XSS vulnerabilities
lukeasrodgers opened this issue · 2 comments
Input for a comment or user email (and possibly other data as well) with HTML in it will be rendered as HTML. I realize this is not a real application, but since people use it as a "best practices" demo, it might be wise to address this.
Some of the issues are easily fixed by just adding include Escaped to the cell code.
Arguably, though, we would want to prevent the user from getting HTML into the database via their input in the first place. This could be done by either escaping the input, stripping tags from the input, or failing with a validation error.
My somewhat selfish reason for opening this issue is that I'm currently struggling to come up with a good way of doing this that minimizes boilerplate. Failing with a validation error doesn't seem ideal (what if the user is just innocently copy-pasting HTML?), but the other options (e.g. custom type a la HtmlStripped = Dry::Types::Constructor.new(String, fn: -> v { strip_tags(v) })) don't seem right either.
I would be happy to open a PR to address this issue.
Hi Luke,
we've been thinking a lot about this and decided in no way will we blindly escape all HTML (several times) the way Rails does in a brute-force attempt to protect you. Guessing we both agree that this is first of all incredibly slow, second highly-inefficient since often you have to "un-protect" strings again because Rails escapes them, and third, a result of a fundamentally wrong approach to software architecture.
Escaping input via the form twins or the dry-types is a good idea. Automatically escaping properties in Cells another, and the best is to have a true view model, that really doesn't allow you to access properties from the model anymore (even nested) without having them escaped.
With the release of TRB2, we can work on a fix, but there's really nothing quick we can do (like escaping everything etc.).
Gotcha. But why not at least add include Escaped to the cell code? (EDIT- maybe this is what you meant by your desire to avoid "blindly escaping all HTML"? I don't think so, though.)
My concern is that people who are used to the Rails "escape all the things" philosophy may get tripped up by this difference. Obviously it doesn't cover all the bases, but I think it's better than nothing.
cheers