PDA

View Full Version : Grid editing security



Hani
15 Jun 2007, 6:42 AM
Right now I can enter arbitrary html/js into an editable grid, so in the editable grid demo for instance, on firefox, I can enter this for a plant name:

<script>alert('vulnerable');</script>

Any plans on having values correctly escaped in editable grids? After all, <foo> is a valid name, and is input by users, so there's no reason to treat it as markup.

tryanDLS
15 Jun 2007, 7:52 AM
I would think you could handle this with a regex validator on the field or a vtype.-

Hani
15 Jun 2007, 9:27 AM
This isn't really a good fix, as it's something a bit more fundamental than that. The question is, is markup allowed in editable grids by intent, or accident? Very few (well, none, outside of techie admin type functionality) input elements allow markup, let alone random javascript that's executed on submit.

tryanDLS
15 Jun 2007, 11:46 AM
It's up to you to validate your input. There's nothing to prevent you from typing script or markup into a generic HTML input or textarea and submitting it. You have to code for it, or for example .Net throws an exception when seeing script in input elements.

Hani
15 Jun 2007, 1:59 PM
I dont mean to be rude, but I think you're sort of missing the point ;)

The issue isnt just about what is and isnt allowed, it's that by default, ext is XSS vulnerable. See http://en.wikipedia.org/wiki/Cross-site_scripting

The current behaviour forces users to explicitly work around this issue, because ext will execute any malicious scripts entered by the user, and there's no obvious way to disable that.

The correct solution (and I hope others will chime in to this thread, agreeing or not!) is to have some kind of html escaping flag, so when I enter <, it's converted to &lt; when displayed, and so on.

Failing that, I'd happily accept a proposed solution that allows me to plug in such behaviour.

I'd be surprised if any editable grid users expect or want to let their users enter <script> tags in grid cells.

jack.slocum
17 Jun 2007, 2:47 AM
Hani,

There is a built in renderer Ext.util.Format.htmlEncode which will do what you described (&lt;).

Hani
17 Jun 2007, 5:14 AM
Perfect, thanks!

Hani
19 Jun 2007, 6:20 AM
The only way I can fix this issue cleanly across all my editors is by modifying the ext source itself, and adding this line in GridView's doRender function, right after the renderer has been called.



if(p.value != undefined && typeof r.data[c.name] == 'string') p.value = Ext.util.Format.htmlEncode(p.value);


This obviously isnt ideal as it means that I have to modify the ext source itself. That particular code block is not very subclass friendly either, so subclassing isnt really a good option.

Is there any hope of having this added as a configuration option, or some refactoring being done so I can at least easily hook in this 'post renderer' behaviour in a subclass?

I can't add it to the default renderer alone since many tables use their own rendering (adding qtips and whatnot). I can't add it to every grid since its not a scalable solution and other developers are likely to forget to do it sometimes.

jack.slocum
19 Jun 2007, 8:01 AM
You could try wrapping your renderers on the fly via a ColumnModel subclass.


SecureColumnModel = Ext.extend(Ext.grid.ColumnModel, {
getRenderer : function(col){
var c = this.config[col];
if(!c.secureRenderer){
var fn = c.renderer;
if(!fn){
fn = Ext.grid.ColumnModel.defaultRenderer;
}
c.secureRenderer = function(){
return Ext.util.Format.htmlEncode(fn.apply(window, arguments));
}
}
return c.secureRenderer;
}
});

Note, this was typed in here so may need some tweaking.

72
30 Sep 2007, 6:56 AM
How hard it is to add config option to do that? Even if it will be false by default, who knows and who wants can easily switch (enable) it. I understand of performance, but i think that security is on first place.

jsakalos
30 Sep 2007, 12:04 PM
Wouldn't be better to post this in Feature Requests forum (or General Discussion)?

caustic
20 Aug 2008, 5:41 AM
How hard it is to add config option to do that? Even if it will be false by default, who knows and who wants can easily switch (enable) it. I understand of performance, but i think that security is on first place.

+1

Could not find such option in ExtJS 2.2, missing it greatly.

jsakalos
20 Aug 2008, 12:55 PM
Wrong forum. This is Ext 1.x: Bugs.