Page MenuHomePhabricator

Security review for Extension:JADE
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project ..

This extension creates two namespaces, and adds a content handler for one of them. That's about all.
https://www.mediawiki.org/wiki/JADE

Description of how the tool will be used at WMF ..

Mostly, third-party patrolling tools like Huggle will be making API edits into the Jade: namespace, in order to coordinate between patrollers.

Dependencies

No dependencies.

Has this project been reviewed before?

No, only intra-team code review in GitHub and Gerrit.

Working test environment

A mw-vagrant role is available: vagrant roles enable jade

Post-deployment

https://www.mediawiki.org/wiki/Wikimedia_Scoring_Platform_team
IRC: awight

Event Timeline

awight created this task.Feb 26 2018, 8:25 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 26 2018, 8:25 PM
awight updated the task description. (Show Details)Mar 2 2018, 5:52 PM
awight updated the task description. (Show Details)
Bawolff claimed this task.Mar 10 2018, 7:00 AM
Bawolff moved this task from Backlog to In Progress on the Security-Team-Reviews board.
Bawolff closed this task as Resolved.Mar 10 2018, 7:21 AM
Bawolff moved this task from In Progress to Awaiting remediation on the Security-Team-Reviews board.

Review passed.

Some minor comments

  • "JudgmentValidator.php" line 172 - should probably be $wgContLang->ucfirst() unless you are very sure it will only be english text fed to this as I believe php's ucfirst doesn't work well with non-ascii characters.
  • includes/ContentHandlers/JudgmentEditAction.php - This seems unused... If its not used it should probably be deleted. If there are plans to alter the edit action process in significant ways, I would like to review that just to make sure its all safe.

Review passed.
Some minor comments

Thanks, good points!

  • "JudgmentValidator.php" line 172 - should probably be $wgContLang->ucfirst() unless you are very sure it will only be english text fed to this as I believe php's ucfirst doesn't work well with non-ascii characters.

In theory, the only correct workflow validates the data, so we know the string is a member of an ASCII enum—however, the validation functions are public, so it's possible for a really determined programmer such as future me to run the workflow out-of-order :-)

  • includes/ContentHandlers/JudgmentEditAction.php - This seems unused... If its not used it should probably be deleted. If there are plans to alter the edit action process in significant ways, I would like to review that just to make sure its all safe.

Oops!

Change 419211 merged by jenkins-bot:
[mediawiki/extensions/JADE@master] Security review followups

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