search

gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.52k stars 376 forks source link

xml Element.setAttribute() should replace unsafe characters in attribute value with appropriate entity #741

Closed dankurka closed 9 years ago

dankurka commented 9 years ago

Originally reported on Google Code with ID 733

Found in GWT Release: 1.3.3

Detailed description:

Example:
--
Document document = XMLParser.createDocument();
Element element = document.createElement("foo");
document.appendChild(element);
element.setAttribute("bar", "<");
Window.alert(document.toString());
--

Generated XML is '<foo bar="<"/>'; should be '<foo bar="&lt;"/>'

Workaround if you have one:

Replace occurances of unsafe characters with appropriate entity; but it'd
be good if GWT itself did this.

Links to the relevant GWT Developer Forum posts:

Reported by g.clinch on 2007-02-26 17:56:25

dankurka commented 9 years ago 
This is not a dupe of 254, but it could probably get fixed at the same time.
Assigning to knorton and matching 254's settings.

Reported by gwt.team.morrildl on 2007-03-01 14:32:45

dankurka commented 9 years ago 
Maybe it would be better to create a new method setSafeAttribute() and perform the
substitution only in it, to avoid paying the performance penalty in every usage of
the original setAttribute() method (when, for example, the attribute value is a well
known constant).

Reported by ankostis on 2007-04-19 23:31:35

dankurka commented 9 years ago 
Without more information about use cases, I'm recommending that we not take any 
action on this issue. All the browsers treat the text you pass into setAttribute as

pure text, so I don't currently know of anyway to exploit unsafe characters in 
setAttribute. These values cannot, for instance, be used to add additional DOM nodes

(like a script tag). It is the their return value that differs. This makes it 
difficult to depend upon setAttribute for storing data for later use, but that's a

pattern that should probably be avoided anyway.

More information on the particular use case would be very helpful for resolving this

issue.

Reported by gwt.team.knorton on 2007-04-26 12:23:36

dankurka commented 9 years ago 
The Document in this case is turned into a string representation and then POSTed back
to the origin server to 
be processed (it's RPC, essentially, but interfacing to an existing, custom interface).
 The issue here is that 
some of the elements added to the Document may come straight from the user.  If the
user is asked for some 
text and they enter '<', calling setAttribute in its current state 'corrupts' the XML,
and the whole thing breaks.  
It's my understanding that setAttribute should be entirely safe, accepting any text
and encoding it suitably 
into the Document.

I'm not averse to ankostis' suggestion, but it flies against my experience of the DOM
spec from other 
languages (here's an example in python):

>>> import xml.dom.minidom
>>> document = xml.dom.minidom.Document()
>>> element = document.createElement("foo")
>>> document.appendChild(element)
<DOM Element: foo at 0x7b080>
>>> element.setAttribute("bar", "<")
>>> print document.toxml()
<?xml version="1.0" ?>
<foo bar="&lt;"/>
>>>

Reported by g.clinch on 2007-04-30 23:38:46

dankurka commented 9 years ago

Reported by gwt.team.morrildl on 2007-05-10 23:00:02

dankurka commented 9 years ago 
This issue is quite critical since currently it allows to create invalid XML. Then 
this invalid XML is submitted to server and app breaks. As API user I expect any 
maniulation (well, at least such trivial manipulation) through DOM to produce 
correct XML.

Not only users of this API should be aware of this issued to use this API correctly,

but in 99% of cases they have to wrap it into home-made function like this:

  Utils.safeSetAttribute(e, "foo", "<&>");

In one of such wrappings is missed anyware in the code, it may break the app.
Not to mention that since each app creator has to duplicate such function, 
possiblity of the error is increased a lot.

Please raise priority to critical and fix in upcoming realease.

Reported by sergekh2 on 2007-08-06 05:27:01

dankurka commented 9 years ago 
Please fix this. 
What's the use of having XML DOM if I have to do the encoding myself???

Reported by altden on 2007-08-10 04:41:28

dankurka commented 9 years ago 
I totally misread this one the first time. I don't know if it's a possibility for 1.4,
but I'm going to bump it to high.

Reported by gwt.team.knorton on 2007-08-10 15:29:52

dankurka commented 9 years ago 
An update on this: it did not make it into 1.4, but I have a patch that I'm testing.
I will try to post a solution here 
soon that will allow you to get around this bug in the 1.4 release.

Reported by gwt.team.knorton on 2007-08-28 04:30:45

dankurka commented 9 years ago 
Kelly, can you dust off your patch?

Reported by bruce+personal@google.com on 2008-04-10 01:48:53

dankurka commented 9 years ago

Reported by bruce+personal@google.com on 2008-04-10 01:49:04

dankurka commented 9 years ago

Reported by ecc%google.com@gtempaccount.com on 2008-04-23 21:15:59

dankurka commented 9 years ago 
Found my old patch. I'm going to try to bring it up-to-date tonight.

Reported by knorton+personal@google.com on 2008-04-24 04:36:46

dankurka commented 9 years ago 
Another status update, because I need to put this on hold for a little while.

With some fixes of the unit tests, my patch works just great ... except for Safari
2
where the XMLSerializer is busted and basically has the same problem reported in this
bug. I do think there is a way to do some of this by hand in Safari2. This task would
be made orders of magnitude easier if we had a Safari3 deferred binding. (Perhaps I
could add a gwt-property directly in the XML module, that way you would only take on
another permutation if you included XML).

I'm actually considering getting this reviewed and committed without fixing it for
Safari2, filing another bug, and then following up with a second patch. Even without
Safari2 working it is better than what we have now.

Reported by gwt.team.knorton on 2008-04-24 21:38:48

dankurka commented 9 years ago

Reported by knorton+personal@google.com on 2008-04-25 18:55:32

dankurka commented 9 years ago 
r2650.

Reported by knorton+personal@google.com on 2008-05-03 03:43:02

dankurka commented 9 years ago 
1_5_RC has been released.

Reported by scottb+legacy@google.com on 2008-08-14 02:24:14

dankurka commented 9 years ago

Reported by rjrjr@google.com on 2011-02-09 00:17:37