when updating a code build recipe json returned isn't escaped

On Wed, Mar 23, 2011 at 5:11 AM, Ian Booth <email address hidden> wrote:
> Hi
>
> See bug: https://bugs.launchpad.net/launchpad/+bug/740096
>
> A recipe page will render ok when first loaded even with "bad" data for
> the text fields. As described in the bug, if a text field contains
> something like "<input//onfocus=alert(1)>", this is bad data. If some
> inline editing is done, the page is updated and an xhr response with
> data for all the object's fields is received, then for each field,
>
> _unmarshallField(self, field_name, field, detail=NORMAL_DETAIL)
>
> is called. This in turn invokes the unmarshall(self, entry, value)
> method on the IFieldMarshaller implementation for each field. For text
> fields, a TextFieldMarshaller is used. This just returns the value
> though. It doesn't escape the text data. I think it should? I hacked in
> a call to cgi.escape() and it would have worked but it barfed when it
> tried to escape the lp cache data. I hacked around this and it worked.
>
> Soooo, should TextFieldMarshaller escape stuff when it unmarshalls?

Nope. Escaping should be done on the other end (when reading/displaying
the data) not when storing it.

A thought experiment: what if there were some other non-HTML-rendering
client, it might be impossible to escape all strings such that both it
and web browsers would be happy with the strings. Therefore the raw
strings should be stored and the client should escape the strings before
using them in potentially dangerous ways.

> so, we need a way to push stuff through without escaping eg lp cache
> data. If not, what other approach should be used to ensure stuff is
> properly escaped when it is rendered?

If we escape on the read side the above won't be a problem.

YUI includes HTML escaping functionality:
http://developer.yahoo.com/yui/3/api/Escape.html

On Wed, Mar 23, 2011 at 8:47 AM, Ian Booth <email address hidden> wrote:
> Hi Benji
>
> >
> > Nope. Escaping should be done on the other end (when reading/displaying
> > the data) not when storing it.
> >
>
> Yes, of course you are correct. Not sure what I was smoking there. Sad
> thing is, I had just finished another similar bug where I used the YUI
> escape function in js to escape some data. This was actually the first
> time anyone used Y.Escape.html(xxxx). I think we must have hand coded an
> escape function and had been using that, but I couldn't find it so I
> decided to use the YUI one. Why didn't I just reuse this same approach
> here? I think I had gone stupid by the end of the day :-(
>
> I'll start looking at client.js and find the right place to add the
> functionality.

Cool. Consider that client.js may not be the right place though.

For example, if I had a page that when I clicked a button added the
letter "A" to the end of an entity's attributes. If the original value
were "<p>" we wouldn't want the LP client to return "&lt;p&gt;" from the
entity and therefore result in us writing back "&lt;p&gt;A".

Instead, the escaping needs to be done when the strings are used in a
context that would misinterpret them, JS itself is fine with HTML in
strings so they need to be escaped when inserted into the page itself.
I'm not sure where the right place to do that is, or even if there is a
single right place.

I haven't looked at the code that's causing this issue, but I suspect
it's because HTML has been constructed by concatenating strings.

Always using the DOM, by way of Y.Node, would avoid this problem
altogether.

It's widely known that mythical creatures die when code like the
following is written:

    var link = 'See <a href="' + url + '">' + desc + '</a>';

But escaping can be forgotten and quickly produces unreadable code:

    var link =
        'See <a href="' + Y.Escape.html(url) +
        '">' + Y.Escape.html(desc) + '</a>';

I propose that the following be the way we build content in
Javascript, and we should add it to the reviewer notes:

    var link = Y.Node.create("<span>See <a /></span>");
    link.one("a")
        .set("href", url)
        .set("text", desc);

Found it!

I was on the wrong track entirely. What was being served up by lazr
restful was correctly escaped after all. It turned out to be caused by
Tim's new Javascript object modified event stuff.

The offending code is in client.js:

function update_cached_object(cache_name, cache, entry)
{
  var fields_changed = new Array();
  for (var name in cache) {
    var old_value = cache[name];
    var new_value = entry.get(name); <------ this line
    if (name != 'lp_html') {
      if (old_value != new_value) {
        fields_changed.push(name);
        cache[name] = new_value;

See how it is getting the new_value as the raw data value from the Entry
object. It should be getting the escaped html value from the lp_html
dict (which is accessible via the Entry object). I've fixed the code and
it works.

I also found something else though. The attribute html values contained
in the xhr response packet after a patch operation are wrapped inside
<p></p> and this causes a false positive in the old_value != new_value
check.

ie
Page first rendered: recipe name = "firefox-daily"
Inline edit something and get a xhr repsonse: recipe name =
"<p>firefox-daily</p>"

This causes recipe name value to be re-rendered. The cache is updated
and so this effect only happens the very first time an xhr repsonse is
processed after the page initially renders. It wouldn't be a problem
except that the base_branch and deb_version_template fields actually
animate (flash green) as if something had really changed.

I've not yet investigated why stuff is wrapped in <p></p> when the web
service is used. Do you know why? I think we should aim to make the
initial page render and web service patch updates render the same data
don't you think?

On Thu, Mar 24, 2011 at 2:47 PM, Ian Booth <email address hidden> wrote:
> I've not yet investigated why stuff is wrapped in <p></p> when the web
> service is used. Do you know why? I think we should aim to make the
> initial page render and web service patch updates render the same data
> don't you think?

for sure.

-Rob

Fixed in stable r12671 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/12671>.

Fixed by r12683.