Most importantly, for notes note.who and note.note weren't escaped. The other changes are pretty much only for consistency since we know that most of those strings don't contain dangerous characters. Let me know if you think I overdid it.

Comment on attachment 540618 [details] [diff] [review] v1 Review of attachment 540618 [details] [diff] [review]: ----------------------------------------------------------------- Better too much than too less. Now that I see this code, it is really obvious how any GET parameter could have been used for XSS. What I’m not too sure about: could anyone hijack the Config object? In any case, better be save than not :-) Thanks! ::: js/UserInterface.js @@ +192,4 @@ > > + var resultText = result.length ? result[0].getTitle().getText() : fallbacks[role]; > + var resultHTML = (resultText.charAt(0) == '#') ? > + '<a href="irc://irc.mozilla.org/' + escape(resultText.slice(1)) + '">' + escape() does handle all the cases that .escapeContent does, right? @@ +1555,5 @@ > if (!self._machineResultHasNotes(result)) > return ''; > return '<div class="note">' + > result.notes.map(function htmlForNote(note) { > + return '[<strong>' + note.who.escapeContent() + '</strong>' + we probably want to add a mailto: if the string matches a simple mail-like regex.

(In reply to comment #1) > Better too much than too less. "too little" :) > Now that I see this code, it is really > obvious how any GET parameter could have been used for XSS. Hah, you're right. I hadn't even noticed the most apparent one until now. > What I’m not too sure about: could anyone hijack the Config object? I don't think so, as long as they haven't already hijacked something else (but then they wouldn't need to hijack Config). At least I can't think of any way at the moment. > In any case, better be save than not :-) "safe" :P > ::: js/UserInterface.js > @@ +192,4 @@ > > > > + var resultText = result.length ? result[0].getTitle().getText() : fallbacks[role]; > > + var resultHTML = (resultText.charAt(0) == '#') ? > > + '<a href="irc://irc.mozilla.org/' + escape(resultText.slice(1)) + '">' + > > escape() does handle all the cases that .escapeContent does, right? In this context, we should compare it to escapeAttribute rather than escapeContent, since " could harm here (but not <)... but yes: escape("<>&\"'") == "%3C%3E%26%22%27" > @@ +1555,5 @@ > > if (!self._machineResultHasNotes(result)) > > return ''; > > return '<div class="note">' + > > result.notes.map(function htmlForNote(note) { > > + return '[<strong>' + note.who.escapeContent() + '</strong>' + > > we probably want to add a mailto: if the string matches a simple mail-like > regex. I don't think it's worth it. Treating the star author as an email address is a relict from Tinderbox, and the thought of somebody writing an email in response to someone's TBPL star seems pretty absurd to me :)

(In reply to comment #2) > > Now that I see this code, it is really > > obvious how any GET parameter could have been used for XSS. > > Hah, you're right. I hadn't even noticed the most apparent one until now. ?tree= ? :-D > > What I’m not too sure about: could anyone hijack the Config object? > > I don't think so, as long as they haven't already hijacked something else > (but then they wouldn't need to hijack Config). At least I can't think of > any way at the moment. Hm I just saw that you have escaped some of the Config vars, but missed other cases. I don’t think those need to be escaped. And thanks for correcting my English :-D

(In reply to comment #3) > (In reply to comment #2) > > > Now that I see this code, it is really > > > obvious how any GET parameter could have been used for XSS. > > > > Hah, you're right. I hadn't even noticed the most apparent one until now. > > ?tree= ? :-D Shh! ;) > > > What I’m not too sure about: could anyone hijack the Config object? > > > > I don't think so, as long as they haven't already hijacked something else > > (but then they wouldn't need to hijack Config). At least I can't think of > > any way at the moment. > > Hm I just saw that you have escaped some of the Config vars, but missed > other cases. I've escaped those where it might be conceivable that they'd contain a funny character (like &) at some point. Though that really makes no sense for repo and result names, so I've removed escaping for them again in this patch.

Comment on attachment 540759 [details] [diff] [review] v2 Review of attachment 540759 [details] [diff] [review]: ----------------------------------------------------------------- Wrong patch :-D

Indeed.

Comment on attachment 541006 [details] [diff] [review] v2 Review of attachment 541006 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/UserInterface.js @@ +398,3 @@ > "Please choose a tree from the list on the upper left.<br/>" + > 'Maybe the tree you’re looking for is on the <a href="' + > Config.alternateTinderboxPushlogURL + this._treeName + This also needs to be escaped, what if this._treeName is "><script ...

http://hg.mozilla.org/webtools/tbpl/rev/9980c25399a1 This landed almost two years ago.