arturadib / agility

Javascript MVC for the "write less, do more" programmer
http://agilityjs.com
MIT License
542 stars 70 forks source link

Bind raw HTML #73

Open colinmacc opened 12 years ago

colinmacc commented 12 years ago

Where agile.js:596 has $node.text(self.model.get(bindData.key).toString()); it might as easily use .html, permitting bindings to inject html instead of text.

I don't see why this isn't the default, as it's useful behaviour. If it weren't to be the default, it could be conditioned by another data- attribute, say data-bindhtml or somesuch.

colinmacc commented 12 years ago

I'd also like to suggest that some $node elements would sensibly default to .html() -

s for example, where others might more sensibly default to .text() -

for example (although even they might have e.g. )

shesek commented 12 years ago

Using .html() by default is problematic from a security point of view, it'll make the application very vulnerable to XSS attacks.

colinmacc commented 12 years ago

I do not see that the use of .html() provides a vector for XSS attack under jQuery, or therefore under agile.js. Users can't inject HTML into the DOM unless the agile.js code binds to an input and also simultaneously binds the same model data to a div or similar - I'm talking about providing .html() only to the single-direction value binding. So, unless the author does something egregiously foolish, all the HTML has to have been supplied by a server which was trusted to provide .js to the client in the first place.

shesek commented 12 years ago

Even when those values eventually come from the server, in many cases they would be originally user-generated content. For example, a user could comment on a website, get it stored to the database, and than the comment would get fetched from the server into a client-side model and bound to a DOM element. The content of his comment would get inserted as raw HTML into the DOM, rather than as text.

shesek commented 12 years ago

Also, probably more important than the XSS concern - in most cases you'll have regular text and not HTML, and it should be treated as such. Treating plain text as HTML would cause it to render incorrectly if it contains characters that have special meaning in HTML.

imho the default should be text, but it might be a good idea to allow HTML using some option.

colinmacc commented 12 years ago

Yes, I think so. It's sometimes necessary.

jdbeutel commented 12 years ago

I needed this to display lists of validation errors in a Grails app.

JamesHagerman commented 12 years ago

I actually made a fork of Agility to provide this ability. I haven't tested it much and I haven't done a pull request because I want to really use it before I start down that road.

Fundamentally, wouldn't this action break "pure MVC"? Data is no longer purely data but can affect the view. This is another reason I haven't done a pull request.

As far as XSS goes, shouldn't you always be sanitizing anything coming from the user anyways on both the client side and the server side? Yes, this functionality could cause accidental XSS vectors if developers are not careful but no more then they would using any other form of retrieving data from a server.

The original reason I added this functionality was to allow me to store a bunch of different templates and spit them back out again. There are easier ways to do this by far (such as putting html in a script tag) but again, I haven't used this stuff in anything other then some test code.

The fork I made is here: https://github.com/JamesHagerman/agility

It's my first fork and this is my first comment on github. I apologize if I've broken netiquette somewhere here...

tlack commented 12 years ago

+1 for using html() by default and text() as some kind of exception (perhaps alternative syntax in data-bind?). Very surprised by   came out as text as a result of my persist module calls.

mahadevan-k commented 11 years ago

I'm doing the same, because I realize that something as simple as a href= attribute modification allows a hacker to execute funcitons. Just being very careful about how I go about implementing the REST APIs.