r78141 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78140‎ | r78141 | r78142 >
Date:21:29, 9 December 2010
Author:btongminh
Status:ok (Comments)
Tags:
Comment:
BREAKING CHANGE: Require POST for patrolling revisions and salt the patrol token with 'patrol' instead of rc_id.

See my comments on r75274, for which this is a follow-up. Using a dedicated, but constant patrol token is in my opinion the optimal compromise between performance (only require fetching the token once) and security (leaking the token will only compromise the patrolling feature).
Modified paths:
  • /trunk/phase3/includes/api/ApiPatrol.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryInfo.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryRecentChanges.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryRecentChanges.php
@@ -79,7 +79,13 @@
8080 return false;
8181 }
8282
83 - return $wgUser->editToken( $rc->getAttribute( 'rc_id' ) );
 83+ // The patrol token is always the same, let's exploit that
 84+ static $cachedPatrolToken = null;
 85+ if ( is_null( $cachedPatrolToken ) ) {
 86+ $cachedPatrolToken = $wgUser->editToken( 'patrol' );
 87+ }
 88+
 89+ return $cachedPatrolToken;
8490 }
8591
8692 /**
Index: trunk/phase3/includes/api/ApiPatrol.php
@@ -59,6 +59,10 @@
6060 $this->getResult()->addValue( null, $this->getModuleName(), $result );
6161 }
6262
 63+ public function mustBePosted() {
 64+ return true;
 65+ }
 66+
6367 public function isWriteMode() {
6468 return true;
6569 }
@@ -95,8 +99,7 @@
96100 }
97101
98102 public function getTokenSalt() {
99 - $params = $this->extractRequestParams();
100 - return $params['rcid'];
 103+ return 'patrol';
101104 }
102105
103106 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiQueryInfo.php
@@ -87,6 +87,7 @@
8888 'unblock' => array( 'ApiQueryInfo', 'getUnblockToken' ),
8989 'email' => array( 'ApiQueryInfo', 'getEmailToken' ),
9090 'import' => array( 'ApiQueryInfo', 'getImportToken' ),
 91+ 'patrol' => array( 'ApiQueryRecentChanges', 'getPatrolToken' ),
9192 );
9293 wfRunHooks( 'APIQueryInfoTokens', array( &$this->tokenFunctions ) );
9394 return $this->tokenFunctions;

Follow-up revisions

RevisionCommit summaryAuthorDate
r78144Revert the addition of the patrol token from ApiQueryInfo from r78141; does n...btongminh21:42, 9 December 2010
r784371.17: Merge tagged revisions from trunk: r77878, r77981, r77982, r77994, r780...catrope14:14, 15 December 2010
r78438RELEASE-NOTES for API action=patrol changes (r75274, r78141)catrope14:16, 15 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r75274BREAKING CHANGE: Per r70640 CR, salt patrol tokens with rcid in the API too. ...catrope17:22, 23 October 2010

Comments

#Comment by Bryan (talk | contribs)   21:30, 9 December 2010

For the backporter: the RELEASE-NOTES need to be updated.

Perhaps this is overdoing security, by requiring both POST and creating a different token, but it doesn't hurt IMHO.

#Comment by Catrope (talk | contribs)   10:14, 10 December 2010

Did you update RELEASE-NOTES in trunk? In which rev?

#Comment by Bryan (talk | contribs)   10:17, 10 December 2010

No, because if this is backported to 1.17, it does not need release notes in 1.18.

Status & tagging log