Code review comment for lp://staging/~doanac/ubuntu-ci-services-itself/webui-status-performance

Revision history for this message
William Grant (wgrant) wrote :

This web worker approach looks fine, assuming we're OK with the browser restrictions (most notably the lack of support in IE <10 and the Android stock browser before 4.4). The JSONP requests should be to a variety of domains, so we won't run into per-origin concurrent request limits.

Separately, I'm a little concerned about some of the DOM construction mechanisms in use here.

48 + if(e.data.error) {
49 + sts.innerHTML = 'ERROR: ' + e.data.error;
50 + return;
51 + }

This will render a textual error message as if it was HTML. Users can often control error content (eg. if a method includes the bad input in an error message about invalid arguments), and setting innerHTML will cause markup to be parsed -- a cross-site scripting vulnerability.

53 + var txt = "<pre>";
54 + for (i in resp) {
55 + if(resp[i]['status'] != 'okay') {
56 + txt += resp[i]['status'].toUpperCase() + ': ';
57 }
58 + txt += resp[i]['label'] + ": " + resp[i]['value'] + "\n";
59 }
60 - });
61 + txt += "</pre>";
62 + sts.innerHTML = txt;

Likewise here. If any of the status, label or value fields are user-controlled, this is an XSS vulnerability. Or if some data happens to contain <, & or other metacharacters you'll end up misrendering.

YUI3 provides reasonably nice APIs for safe DOM construction. We often use a pattern like this:

  var div = Y.Node.create("<tr><th></th><td><span></span><a></a></td></tr>");
  div.one("th").set("text", "Header");
  div.one("span").set("text", "Label:");
  div.one("a").set("text", "Value").set("href", "http://url.to/whatever");
  parent.append(div)

For more complex structures, it's often cleaner to use a templating language like mustache or handlebars. Handlebars is supported by YUI3 core, in http://yuilibrary.com/yui/docs/handlebars/.

review: Approve (browser ajax hell)

« Back to merge proposal