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

Multiple XSS vulnerabilities #25

Open
lukeasrodgers opened this issue Dec 21, 2016 · 2 comments
Open

Multiple XSS vulnerabilities #25

lukeasrodgers opened this issue Dec 21, 2016 · 2 comments

Comments

@lukeasrodgers
Copy link

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.

@apotonick
Copy link
Owner

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.).

@lukeasrodgers
Copy link
Author

lukeasrodgers commented Dec 21, 2016

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

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

No branches or pull requests

2 participants