PDA

View Full Version : Security Issue with input in Grids and how to avoid it?



Arno.Nyhm
16 Nov 2009, 7:11 AM
user moldoe point in his thread http://www.extjs.com/forum/showthread.php?t=85345 about html in headers also to one other interesting issue:

a user input is rendered as html:

try it yourself here:
http://www.extjs.com/examples/explorer.html#editablegrid

enter in column "company name":

Company Name <a href="#" onclick="alert('hello');"> click </A>i think in this case it is a security problem and it should switched off by default.

and give a addtional switch to allow it like:
setHtmlDecodeUserInput(false)

sven
17 Nov 2009, 5:29 AM
You should never ever trust data from users. There are valid points when you want to allow this. You can override postprocessvalue of the editor to encode it, if you need it encoded.

As the framework can never know, if you need it encoded or not, there is no way we can do this. It is up to the developer to decide, if you want to allow this or not.

Arno.Nyhm
17 Nov 2009, 8:28 AM
but for the security reason the grideditor should by default encode it. and not the other way.

sven
17 Nov 2009, 8:30 AM
No it should not. You, the developer, are the only one that knows if it should do that or not. There are valid reasons to allow markup. This change will even brake the examples.

The editor has a postprocessmethod that you need to override if you are not trusting the user input.


The underlaying framework can never know what you want to do with the data. So it should NEVER EVER change it.

Martin.Trummer
18 Nov 2009, 2:01 AM
I understand both of your arguments.

Why can't GXT just have one global switch like GXT.setXSSSafe(boolean)?
This would be perfectly backwards compatible and the users of the lib could decide what they want to do?

I would prefer that all components in the framework are XSS Safe by default and when I am really sure, that there's no problem, I'd switch this off for those (few) cases (or call another function)

to make this point more clear

e.g. TabItem.setText() uses setInnerHtml under the covers: which is really nasty: then the function should at least be called TabItem.setHtml()!!
so we, as your users, must be aware of all the places in your lib where you use setInnerHtml() - you must admit that this really unsexy..



myGrid.setXSSSafe(true/false)
myTabItem.setXSSSafe(false) or maybe

myTabItem.setText()
myTabItem.setHtml()