Is there a task tracking the deployment of this extension to Wikimedia production?

@Legoktm yes, it's now tracked in T150184.

The extension is now in a state where security review could start already. Is there an estimate how long it would take approximately?

Security review of TwoColConflict ext version 4194077ff44a5 (Jan 6, 2016)

Overall the extension is good. I would prefer that html be constructed via the Html class instead of using string concatenation, but in most cases (one exception) this extension uses string concatenation safely for constructing Html.

Major issues

• In getUnifiedText() '<span mw-twocolconflict-diffchange-title-pseudo="' . $this->context->msg( 'twoColConflict-diffchange-own-title' ) The ->parse() format of Message class is not meant to be used in the context of html attributes. This could lead to XSS if someone registered the username User:"onmouseover="doEvil()"d=" (for the foreign-title case). Please either use ->escaped() format when using messages in a context like this, or better yet don't manually construct the HTML attributes but use Html::element()/Html::rawElement().

Minor issues

• line 101, 102 of TwoColConflictPage (and other locations) - When using a Message object, please explicitly set ->parse() instead of relying on implicit ->__toString(). This just makes stuff much clearer.

Non-security

[This is just random comments and feedback I had when I was reading the code. This is not really part of the review. Feel free to ignore whatever you disagree with]

• wrapWikiMsg in TwoColConflictPage::addExplainConflictHeader - Doesn't matter, but you don't need to call ->msg() here, most people just do [ 'twoColConflict-explainconflict', $buttonLabel ].
• TwoColConflictPage::getUnifiedDiffText() - Wouldn't it be better if mw-twocolconflict-diffchange-title-pseudo was data-mw-twocolconflict-diffchange-title-pseudo?
• On the edit conflict view, the html seems unbalanced causing the page footer to be messed up. Maybe there is an unclosed <div> somewhere.
• MW Javascript coding conventions usually require that you make the $ be an argument to the immediately invoked function, where it is passed as jQuery.
• The javascript seems to not work (?)
• in getCollapsedText - htmlspecialchars is preferred over htmlentities unless you've got some specific reason you need it.
• Comment on trimWhiteSpaces is kind of misleading (I wouldn't consider whitespace a non-printing character, and the function does not remove other non-printing characters)
• The cursor is not a text selector on the diff screen.
• I can't seem to select the text of the other user's version.
• I'm a little worried about this interfering with other extensions that override the EditForm (Not sure if any currently deployed extensions do that, but its possible future extensions might for special content models). Maybe the hook should only override the edit form in the event of a conflict, and otherwise allow default behaviour. (Not really sure about that)
• What about Custom Content models that override the diff behaviour (e.g. Wikidata)? Does it really make sense to override conflict handling for them?

@Bawolff thank you so much for doing the review!
We're going to tackle your points in upcoming days.

						case 'add':
							$output[] = '<div class="mw-twocolconflict-diffchange-own">' .
								'<div class="mw-twocolconflict-diffchange-title">' .
							    '<span mw-twocolconflict-diffchange-title-pseudo="' .
								$this->context->msg( 'twoColConflict-diffchange-own-title' ) .
							    '" unselectable="on">' . // used by IE9
							    '</span>' .
								'</div>';
							$output[] = $changeSet['new'] . '</div>';
							break;
						case 'delete':
							$output[] = '<div class="mw-twocolconflict-diffchange-foreign">' .
								'<div class="mw-twocolconflict-diffchange-title">' .
							    '<span mw-twocolconflict-diffchange-title-pseudo="' .
								$this->context->msg(
									'twoColConflict-diffchange-foreign-title',
									$lastUser
								) .
							    '" unselectable="on">' . // used by IE9
							    '</span>' .
								'</div>';
							$output[] = $changeSet['old'] . '</div>';
							break;

Mixed tabs and spaces

The review is done, right? (So this should be closed and any issues should be filed as separate tasks)

In T149808#2929331, @greg wrote:

The review is done, right? (So this should be closed and any issues should be filed as separate tasks)

I guess so. Sometimes we seem to close the review when its done, othertimes we seem to only after all issues are fixed. I'm not sure what the official way is.

I guess semantically/contextualized processes would be: Keep it open, file sub-tasks for the issues identified/that need to be resolved before "ready", and resolve it when those are closed.

That way this task, which blocks the deploy to Beta, is clearly the representative of security readiness.

Change 331414 had a related patch set uploaded (by Addshore):
Use Message::parse instead of Message::__toString

https://gerrit.wikimedia.org/r/331414

Main issue patched in https://gerrit.wikimedia.org/r/#/c/331404/
Other issue linked above.

Only pending things are non security related in https://phabricator.wikimedia.org/T149808#2927471 and we will slowly look at these.

Change 331414 merged by jenkins-bot:
Use Message::parse instead of Message::__toString

https://gerrit.wikimedia.org/r/331414

Change 332348 had a related patch set uploaded (by WMDE-Fisch):
Various minor fixes related to the security review

https://gerrit.wikimedia.org/r/332348

Change 332348 merged by jenkins-bot:
Various minor fixes related to the security review

https://gerrit.wikimedia.org/r/332348