r75274 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r75273‎ | r75274 | r75275 >
Date:17:22, 23 October 2010
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
BREAKING CHANGE: Per r70640 CR, salt patrol tokens with rcid in the API too. This means patrol tokens are now different for every recentchanges entry.
Modified paths:
  • /trunk/phase3/includes/api/ApiPatrol.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryRecentChanges.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryRecentChanges.php
——@@ -79,14 +79,7 @@
8080 return false;
8181 }
8282
83 - // The patrol token is always the same, let's exploit that
84 - static $cachedPatrolToken = null;
85 - if ( !is_null( $cachedPatrolToken ) ) {
86 - return $cachedPatrolToken;
87 - }
88 -
89 - $cachedPatrolToken = $wgUser->editToken();
90 - return $cachedPatrolToken;
 83+ return $wgUser->editToken( $rc->getAttribute( 'rc_id' ) );
9184 }
9285
9386 /**
——@@ -192,7 +185,8 @@
193186 'rc_cur_id',
194187 'rc_type',
195188 'rc_moved_to_ns',
196 - 'rc_moved_to_title'
 189+ 'rc_moved_to_title',
 190+ 'rc_deleted'
197191 ) );
198192
199193 /* Determine what properties we need to display. */
Index: trunk/phase3/includes/api/ApiPatrol.php
——@@ -95,7 +95,8 @@
9696 }
9797
9898 public function getTokenSalt() {
99 - return '';
 99+ $params = $this->extractRequestParams();
 100+ return $params['rcid'];
100101 }
101102
102103 protected function getExamples() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r78141BREAKING CHANGE: Require POST for patrolling revisions and salt the patrol to...btongminh21:29, 9 December 2010
r78438RELEASE-NOTES for API action=patrol changes (r75274, r78141)catrope14:16, 15 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r70640Per comments, follow-up to r70278: make patrol tokens compatibles between api...ialex18:56, 7 August 2010

Comments

#Comment by Gurch (talk | contribs)   20:08, 25 October 2010

If the patrol token is different each time, the client must query recentchanges every time they want to patrol a page.

That equates to a big loss in efficiency versus just querying once per session and caching the token, assuming the client is getting the rcids themselves from some other place, e.g. the IRC feed, and so isn't querying recentchanges already.

Since patrolling via index.php doesn't have this restriction, this may result in clients using that in preference to the API, which is probably undesirable.

#Comment by Gurch (talk | contribs)   20:12, 25 October 2010

ignore that, I see it was changed in index.php too.

Might I ask why? It seems to hamper things for no good reason... sure you could trick someone into following a link and patrolling a revision, but you'd have to be inordinately clever/lucky to both get the right RCID in the URL and have them follow it before someone else marked it as patrolled anyway. And patrolling is pretty benign, it's not like we're talking about deleting or blocking here.

#Comment by Catrope (talk | contribs)   19:38, 26 October 2010

The larger problem is that it was exposing the unsalted edit token, which is also used for things like editing and moving, in GET URLs.

#Comment by Gurch (talk | contribs)   00:13, 27 October 2010

Doesn't the API require POST for patrolling?

#Comment by Catrope (talk | contribs)   00:21, 27 October 2010

Nope

#Comment by Gurch (talk | contribs)   05:31, 27 October 2010

Should it do?

#Comment by Bryan (talk | contribs)   20:01, 16 November 2010

You could also create a dedicated patrolling token, to minimize the impact of a stolen token. That would be my preference.

#Comment by Bryan (talk | contribs)   14:13, 27 November 2010

comments on this?

#Comment by Krinkle (talk | contribs)   21:35, 16 November 2010

Two weirdnesses:

1) When trying to patrol an unpatrolled (or patrolled) edit with a wrong or missing token the page shows: 

 == Session failure ==
 There seems to be a problem with your login session; this action has been canceled as a precaution against session hijacking. Go back to the previous page, reload that page and then try again.

Seems a weird error to show in that case. Rollback specifically nags about badtoken if I recall correctly (same in the API)

2 )When attempting to patrol an edit and the token is correct but the edit was already patrolled it doesn't indicate in any way that it's been patrolled already, it just says "The selected revision of ... has been marked as patrolled."

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

I'm going to make the following two breaking changes:

  • The API patrol edit token will now always be salted with "patrol"
  • The API patrol module will now always require POST

I think that would sufficiently limit the damage done by a token leak.

Status & tagging log