r74098 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r74097‎ | r74098 | r74099 >
Date:20:12, 1 October 2010
Author:reedy
Status:ok (Comments)
Tags:
Comment:
* (bug 25248) API: paraminfo errors with certain modules

Added a needsToken() function, rather than calling getTokenSalt, which can throw silly errors due to dependencies on parameters
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/api/ApiBase.php (modified) (history)
  • /trunk/phase3/includes/api/ApiBlock.php (modified) (history)
  • /trunk/phase3/includes/api/ApiDelete.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEditPage.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEmailUser.php (modified) (history)
  • /trunk/phase3/includes/api/ApiImport.php (modified) (history)
  • /trunk/phase3/includes/api/ApiMove.php (modified) (history)
  • /trunk/phase3/includes/api/ApiPatrol.php (modified) (history)
  • /trunk/phase3/includes/api/ApiProtect.php (modified) (history)
  • /trunk/phase3/includes/api/ApiRollback.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUnblock.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUndelete.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUpload.php (modified) (history)
  • /trunk/phase3/includes/api/ApiUserrights.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiMove.php
——@@ -233,6 +233,10 @@
234234 ) );
235235 }
236236
 237+ public function needsToken() {
 238+ return true;
 239+ }
 240+
237241 public function getTokenSalt() {
238242 return '';
239243 }
Index: trunk/phase3/includes/api/ApiProtect.php
——@@ -207,8 +207,12 @@
208208 ) );
209209 }
210210
 211+ public function needsToken() {
 212+ return true;
 213+ }
 214+
211215 public function getTokenSalt() {
212 - return null;
 216+ return '';
213217 }
214218
215219 protected function getExamples() {
Index: trunk/phase3/includes/api/ApiRollback.php
——@@ -128,6 +128,10 @@
129129 ) );
130130 }
131131
 132+ public function needsToken() {
 133+ return true;
 134+ }
 135+
132136 public function getTokenSalt() {
133137 return array( $this->getTitle()->getPrefixedText(), $this->getUser() );
134138 }
Index: trunk/phase3/includes/api/ApiUserrights.php
——@@ -125,6 +125,10 @@
126126 return parent::getPossibleErrors();
127127 }
128128
 129+ public function needsToken() {
 130+ return true;
 131+ }
 132+
129133 public function getTokenSalt() {
130134 return $this->getUser()->getName();
131135 }
Index: trunk/phase3/includes/api/ApiDelete.php
——@@ -246,6 +246,10 @@
247247 ) );
248248 }
249249
 250+ public function needsToken() {
 251+ return true;
 252+ }
 253+
250254 public function getTokenSalt() {
251255 return '';
252256 }
Index: trunk/phase3/includes/api/ApiImport.php
——@@ -154,6 +154,10 @@
155155 ) );
156156 }
157157
 158+ public function needsToken() {
 159+ return true;
 160+ }
 161+
158162 public function getTokenSalt() {
159163 return '';
160164 }
Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -481,6 +481,10 @@
482482 );
483483 }
484484
 485+ public function needsToken() {
 486+ return true;
 487+ }
 488+
485489 public function getTokenSalt() {
486490 return '';
487491 }
Index: trunk/phase3/includes/api/ApiUnblock.php
——@@ -129,6 +129,10 @@
130130 ) );
131131 }
132132
 133+ public function needsToken() {
 134+ return true;
 135+ }
 136+
133137 public function getTokenSalt() {
134138 return '';
135139 }
Index: trunk/phase3/includes/api/ApiEmailUser.php
——@@ -119,6 +119,10 @@
120120 ) );
121121 }
122122
 123+ public function needsToken() {
 124+ return true;
 125+ }
 126+
123127 public function getTokenSalt() {
124128 return '';
125129 }
Index: trunk/phase3/includes/api/ApiBlock.php
——@@ -183,6 +183,10 @@
184184 ) );
185185 }
186186
 187+ public function needsToken() {
 188+ return true;
 189+ }
 190+
187191 public function getTokenSalt() {
188192 return '';
189193 }
Index: trunk/phase3/includes/api/ApiPatrol.php
——@@ -90,6 +90,10 @@
9191 ) );
9292 }
9393
 94+ public function needsToken() {
 95+ return true;
 96+ }
 97+
9498 public function getTokenSalt() {
9599 return '';
96100 }
Index: trunk/phase3/includes/api/ApiUndelete.php
——@@ -143,6 +143,10 @@
144144 ) );
145145 }
146146
 147+ public function needsToken() {
 148+ return true;
 149+ }
 150+
147151 public function getTokenSalt() {
148152 return '';
149153 }
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -1094,6 +1094,14 @@
10951095 }
10961096
10971097 /**
 1098+ * Returns whether this module requires a Token to execute
 1099+ * @returns bool
 1100+ */
 1101+ public function needsToken() {
 1102+ return false;
 1103+ }
 1104+
 1105+ /**
10981106 * Returns the token salt if there is one, '' if the module doesn't require a salt, else false if the module doesn't need a token
10991107 * @returns bool
11001108 */
——@@ -1155,7 +1163,7 @@
11561164 $ret[] = array( 'writedisabled' );
11571165 }
11581166
1159 - if ( $this->getTokenSalt() !== false ) {
 1167+ if ( $this->needsToken() ) {
11601168 $ret[] = array( 'missingparam', 'token' );
11611169 $ret[] = array( 'sessionfailure' );
11621170 }
Index: trunk/phase3/includes/api/ApiUpload.php
——@@ -459,6 +459,10 @@
460460 ) );
461461 }
462462
 463+ public function needsToken() {
 464+ return true;
 465+ }
 466+
463467 public function getTokenSalt() {
464468 return '';
465469 }
Index: trunk/phase3/RELEASE-NOTES
——@@ -421,6 +421,7 @@
422422 $wgAllowAsyncCopyUploads to be true.
423423 * sinumberingroup correctly gives size of 'user' group, and omits size of
424424 implicit groups rather than showing 0.
 425+* (bug 25248) API: paraminfo errors with certain modules
425426
426427 === Languages updated in 1.17 ===
427428

Follow-up revisions

RevisionCommit summaryAuthorDate
r74217MFT r74098, r74099...reedy15:53, 3 October 2010
r75259Followup r74098, fixup extensions that are using getTokenSalt, by adding need...reedy16:14, 23 October 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r73596follow up r72387: using ApiQuery here breaks ApiParamInfo (bug 25248)gurch10:46, 23 September 2010

Comments

#Comment by Reedy (talk | contribs)   20:18, 1 October 2010

Needs merging to 1.16 when reviewed (I'll do it)

#Comment by Catrope (talk | contribs)   15:30, 3 October 2010 
+		public function needsToken() {
+		return true;
+	}
+
 	public function getTokenSalt() {
 		return '';

Indented wrong.

Good otherwise, go ahead and backport.

#Comment by Reedy (talk | contribs)   15:32, 3 October 2010

Already fixed in r74099

#Comment by Bryan (talk | contribs)   17:28, 16 October 2010

I am not entirely happy with how this breaks backwards compatibility (with old extensions that do not have needsToken() but do return a salt), but I seen to clean way to handle this.

#Comment by Catrope (talk | contribs)   17:30, 16 October 2010

That's a very good point: old extensions will become vulnerable to CSRF if not upgraded. We need to avoid that somehow.

#Comment by Reedy (talk | contribs)   15:10, 23 October 2010

Suggested fix/way of going about it?

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

Remind me again what the result of our discussion in DC on this was?

#Comment by Reedy (talk | contribs)   21:48, 9 December 2010

I think it was a case of fixing like this wasn't ideal, but one of the only ways around it. IIRC, I said I would/I'm sure I did, clean up any extensions to try and minimise the likelihood of it

#Comment by Reedy (talk | contribs)   16:17, 23 October 2010

Might need to backport r75259 to 1.16

I'll do that when I get home (cba checking out 1.16 here)

Status & tagging log