Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9533 closed defect (fixed)

Dojox.grid.DataGrid escapeHTMLInData does not seem to work

Reported by: jdev Owned by: Bryan Forbes
Priority: high Milestone: 1.4
Component: DojoX Grid Version: 1.3.0rc2
Keywords: 13 Cc:
Blocked By: Blocking:

Description

I tried an app that we were planning on rolling out next week with 1.3.2 RC2 and found that it was escaping the HTML returned by our get function. I added escapeHTMLInData="false" to the tag as per the documentation with no success. I have tried it with FF 3.5 & IE6 and neither works.

Change History (18)

comment:1 Changed 10 years ago by Thomas Gelf

Details from #9564 (marked as duplicate):

Section "Options" on http://docs.dojocampus.org/dojox/grid explains that there would be "escapeHTMLInData" new in 1.3.2 - however there is no such option to be found in source code.

dojox.grid.DataGrid? now escapes HTML per default - protecting users from beginner errors, but breaking compatibility with existing code. While I'm not against doing so (is it always good to protect coders from themselves - even if such changes are not really welcome between minor releases) at least the (documented) escapeHTMLInData switch should be available.

Line 6845 in dojox/grid/DataGrid.js.uncompressed.js does this escape-alike-job as follows:

d = (d && d.replace) ? d.replace(/</g, '&lt;') : d;

While I'm not sure if this suffices to protect against all kinds of attacks it is pretty obvious that it has been forgotten to implement escapeHTMLInData.

Best regards, Thomas Gelf

comment:2 Changed 10 years ago by Thomas Gelf

One additional note: this ticket is tagged for "Milestone: 1.4" - could this be corrected? This is a compatibility-breaking bug, not a new feature (even if escapeHTMLInData has not been there before). And I guess it could easily be fixed - just add a check for escapeHTMLInData to the line mentioned above.

Cheers, Thomas

comment:3 Changed 10 years ago by Phil DeJarnett

I want to second this as a HUGE mistake. This blindly escapes data, and provides no workaround!

For now I'm going to delete the section of code manually, but I'm very disappointed in this change.

comment:4 Changed 10 years ago by Karl Tiedt

There is a work around, if you are expecting HTML data, you can supply a formatter and formatter output is not escaped (by adding the formatter you are saying, "I want this data exactly the way this formatter returns it")

comment:5 in reply to:  4 Changed 10 years ago by Karl Tiedt

Replying to ktiedt:

There is a work around, if you are expecting HTML data, you can supply a formatter and formatter output is not escaped (by adding the formatter you are saying, "I want this data exactly the way this formatter returns it")

Infact, from the page linked above....:

This will escape HTML brackets from the data to prevent HTML from user-inputted data being rendered with may contain JavaScript? and result in XSS attacks. This is true by default, and it is recommended that it remain true. Setting this to false will allow data to be displayed in the grid without filtering, and should be only used if it is known that the data won't contain malicious scripts. If HTML is needed in grid cells, it is recommended that you use the formatter function to generate the HTML (the output of formatter functions is not filtered, even with escapeHTMLInData set to true).

comment:6 in reply to:  4 Changed 10 years ago by Phil DeJarnett

Replying to ktiedt:

There is a work around, if you are expecting HTML data, you can supply a formatter and formatter output is not escaped (by adding the formatter you are saying, "I want this data exactly the way this formatter returns it")

This is BS. I have a formatter on every single column (to dynamically highlight search results), and it still escapes the results.

Look at the source, because I just did. The HTML escaping code is BEFORE the call to the formatter function.

Making your response bold and underlined doesn't change the fact that this is a bad piece of code. If I want to escape HTML in the results, I should pass it into a formatter, not the other way around. Your response is completely illogical.

comment:7 Changed 10 years ago by Karl Tiedt

Well, w/o code to see, I call your BS and raise you with a working test case

http://archive.dojotoolkit.org/nightly/dojotoolkit/dojox/grid/tests/test_yahoo_search.html

Making your response emotionally charged and in an aggressive manner does nothing to prove a point, test cases, examples, and other things that actually prove a point do... Please try to be constructive when it comes to the tickets. The bold and underline was simply to draw attention to the most relevant part of the comment from the documentation page.

comment:8 Changed 10 years ago by Phil DeJarnett

Sorry about seeming rude. But this fails in 1.3.2, which is the version this is about. Not the trunk!

This is 1.3.2 (running off my server unmodified). http://share.overzealous.com/dojo/dojox/grid/tests/test_yahoo_search.html

Obviously it's been fixed in the trunk, but (in my opinion) it should never have been included in 1.3.2 - it's too drastic of a change for a 0.0.x release.

comment:9 Changed 10 years ago by Phil DeJarnett

I want to apologize, again, to ktiedt. My previous response was completely inappropriate. I'd respond directly, but I cannot figure out how, so I will post it here.

(It is still broken in 1.3.2 :-)

comment:10 Changed 10 years ago by Karl Tiedt

No worries :) -- there isnt much we can do about 1.3.2, its out, however that doesn't make this bug anymore valid if its been fixed ;)

While I would agree this change was drastic and IMO rushed, it was done for security purposes and like many things.. it had its issues, but they have (currently) been addressed as far as we know at this point :)

comment:11 Changed 10 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

(In [19797]) Adding escapeHTMLInData option to 1.3, fixes #9533, !strict

comment:12 Changed 10 years ago by Kris Zyp

I committed the fix to the 1.3 branch (just learned how to do that, I thought that was done by the whoever cuts the release). However, I am confused as to why the formatter is not working when HTML escaping is not disabled. As OverZealous? states:

HTML escaping code is BEFORE the call to the formatter function.

Isn't that the desired behavior, so that your formatter output is not altered?

comment:13 Changed 10 years ago by Adam Peller

Keywords: 13 added

comment:14 Changed 10 years ago by Adam Peller

Milestone: 1.41.3.3

now that we have a 1.3.3 milestone, tagging as 1.3.3, since apparently the fix is already on the 1.3 branch.

comment:15 Changed 10 years ago by robwaite

I would just like to add that switching from getters to formatters does not seem like a fix that works across the board.

Formatters appear to me to work on a single field in a cell (defined in the structure of the grid). Our datagrid needed multiple data elements to get put together in a single cell. Perhaps I am totally incorrect here... but I still don't see a workaround on this that helps me.

Until escapeHTMLInData becomes a usable option... we are blocked from upgrading.

Also... as far as security goes... we load the data ourselves... the user has no access to the JSON data and no one is able to post data to our database. In our case... security has not been increased by the original breaking change. It seems that this security fix is to help people from hurting themselves... not from some deep security flaw in the Grid.

comment:16 Changed 10 years ago by Kris Zyp

You can use getters and formatters in combination. The getter should be used to pull/aggregate the data from the item (it can even return an object, I believe) and then the formatter can use the result from the getter and create the appropriate HTML.

comment:17 Changed 10 years ago by (none)

Milestone: 1.3.3

Milestone 1.3.3 deleted

comment:18 Changed 10 years ago by bill

Milestone: 1.4

we won't have a 1.3.3

Note: See TracTickets for help on using tickets.