r95680 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r95679‎ | r95680 | r95681 >
Date:17:31, 29 August 2011
Author:hashar
Status:reverted (Comments)
Tags:
Comment:
(bug 30617) inline comments forms cannot be closed

On clicking a line, a javascript insert a <tr> element containg the
form. That <tr> did not get an unique id which caused troubles when
opening multiples inline comments.

This patch forge an id based on the diff line it applies to which should
be unique since we only allow one comment form per line.
Modified paths:
  • /trunk/extensions/CodeReview/modules/ext.codereview.linecomment.js (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/modules/ext.codereview.linecomment.js
——@@ -17,9 +17,11 @@
1818 * @param lineCode jQuery object
1919 */
2020 lcShowForm: function( lineCode ) {
 21+ var htmlId = 'comment-for-' + lineCode.attr('id');
 22+
2123 lineCode.unbind( 'click' );
2224 lineCode.click( function () {
23 - $( '#lineComment' ).fadeOut( 200 ).remove();
 25+ $( '#'+htmlId ).fadeOut( 200 ).remove();
2426
2527 lineCode.unbind( 'click' );
2628 lineCode.click( function() {
——@@ -28,7 +30,7 @@
2931 });
3032
3133 var lineComment =
32 - $('<tr id="lineComment"><td colspan="3">'
 34+ $('<tr id="' + htmlId + '"><td colspan="3">'
3335 +'<textarea id="lineCommentContent" rows="5"></textarea><br/>'
3436 +'<input id="lineCommentSend" type="button" value="Send">'
3537 +'</td></tr>');

Follow-up revisions

RevisionCommit summaryAuthorDate
r107074make sure the line ID is an integer before injecting it...hashar16:14, 22 December 2011
r108350Reverting inline commenting from CodeReview...johnduhart06:55, 8 January 2012

Comments

#Comment by Tim Starling (talk | contribs)   05:53, 8 December 2011

htmlId is not escaped for CSS, so $( '#'+htmlId ) is not guaranteed to work. document.getElementById() will work reliably.

Also it's not escaped for HTML, so the HTML construction needs to be something like 

$(<td colspan="3">'
    +'<textarea id="lineCommentContent" rows="5"></textarea><br/>'
    +'<input id="lineCommentSend" type="button" value="Send">'
    +'</td></tr>').wrap($('<tr/>').id(htmlId));
#Comment by Hashar (talk | contribs)   16:12, 22 December 2011

I do not understand how an attacker can take advantage of this for cross site scripting. The line id is generated by the PHP script. One would have to change the id using javascript, which mean it could already inject whatever it wants anywhere.

Anyway, I got a follow up change to make sure the line id is an integer :-)

#Comment by Hashar (talk | contribs)   16:23, 22 December 2011

Follow up is r107074

#Comment by Tim Starling (talk | contribs)   02:30, 28 December 2011

I didn't say it was an XSS vulnerability, I just said it was wrong.

Status & tagging log