Page MenuHomePhabricator

Security review for TwoColConflict extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

The TwoColConflict extension provides an alternative two-column view for the edit conflict resolution page. It highlights differences between the editor's and the conflicting change directly in the textfield for an easy possibility to copy and paste desired pieces of the text and resolving the conflict.

Description of how the tool will be used at WMF

If installed, whenever an edit conflict is happening while using the standard wikitext editor, the extension will show a new two-column conflict resolution view. You will see your version of the text on the one side and the conflicting version on the other. Differences between those versions are highlighted and you can copy and paste desired text passages to adjust the version that should be saved.
The extension should be available as a beta-feature at the beginning.

Dependencies

Has this project been reviewed before?

Only review inside the TCB-Team. Implementation of the main requirements is still ongoing and is tracked in T143823, T149720 and T149721.

Working test environment

wfLoadExtension( 'TwoColConflict' );
  • After enabling the extension, try to produce an edit conflict and then see the new conflict-resolution view.

Post-deployment

TCB-Team

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 2 2016, 3:02 PM
Tobi_WMDE_SW updated the task description. (Show Details)Nov 3 2016, 9:48 AM
Legoktm added a subscriber: Legoktm.Nov 3 2016, 7:23 PM

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?

Bawolff added a subscriber: Bawolff.Jan 9 2017, 7:48 AM

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.

Reedy added a subscriber: Reedy.Jan 9 2017, 10:36 PM
						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

greg added a subscriber: greg.Jan 9 2017, 11:08 PM

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

Addshore closed this task as Resolved.Jan 9 2017, 11:09 PM
Addshore claimed this task.

We will file tasks for the other issues outlined in the next days!

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.

greg added a comment.Jan 9 2017, 11:26 PM

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