r70052 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70051‎ | r70052 | r70053 >
Date:22:20, 27 July 2010
Author:platonides
Status:deferred
Tags:
Comment:
Remove register globals vulnerability.
Do not use $wgUser for the name of a function argument.
Use the namespace indexes instead of the namespace names in English.
Tabify
Modified paths:
  • /trunk/extensions/WhiteListEdit/WhiteListAuth.php (modified) (history)
  • /trunk/extensions/WhiteListEdit/WhiteListEdit.php (modified) (history)
  • /trunk/extensions/WhiteListEdit/WhiteListEdit_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/WhiteListEdit/WhiteListAuth.php
——@@ -42,20 +42,20 @@
4343 {
4444
4545 /* $result value:
46 - * true=Access Granted
47 - * false=Access Denied
48 - * null=Don't know/don't care (not 'allowed' or 'denied')
 46+ * true = Access Granted
 47+ * false = Access Denied
 48+ * null = Don't know/don't care (not 'allowed' or 'denied')
4949 * Return value:
50 - * true=Later functions can override.
51 - * false=Later functions not consulted.
 50+ * true = Later functions can override.
 51+ * false = Later functions not consulted.
5252 */
53 - static function CheckWhiteList(&$title, &$wgUser, $action, &$result) {
 53+ static function CheckWhiteList(&$title, &$user, $action, &$result) {
5454
5555 $override = WHITELIST_NOACTION;
5656
5757
5858 /* Bail if the user isn't restricted.... */
59 - if( !in_array('restricttowhitelist', $wgUser->getRights()) ) {
 59+ if( !in_array('restricttowhitelist', $user->getRights()) ) {
6060 $result = null; /* don't care */
6161 return true; /* Later functions can override */
6262 }
——@@ -71,17 +71,17 @@
7272 /* Check global allow/deny lists */
7373 $override = self::GetOverride($true_title, $action);
7474
75 - /* Check if page is on whitelist */
76 - if( WHITELIST_NOACTION == $override )
77 - $override = self::IsAllowedNamespace( $true_title, $wgUser, $action );
 75+ /* Check if page is on whitelist */
 76+ if( WHITELIST_NOACTION == $override )
 77+ $override = self::IsAllowedNamespace( $true_title, $user, $action );
7878
7979 /* Check if page is on whitelist */
8080 if( WHITELIST_NOACTION == $override )
81 - $override = self::IsAllowed( $true_title, $wgUser, $action );
 81+ $override = self::IsAllowed( $true_title, $user, $action );
8282
8383 /* Check if user page */
8484 if( WHITELIST_NOACTION == $override )
85 - $override = self::IsUserPage( $true_title->GetPrefixedText(), $wgUser );
 85+ $override = self::IsUserPage( $true_title->GetPrefixedText(), $user );
8686
8787 switch( $override )
8888 {
——@@ -103,41 +103,41 @@
104104 {
105105 global $wgWhiteListOverride;
106106
107 - $allowView = $allowEdit = $denyView = $denyEdit = false;
 107+ $allowView = $allowEdit = $denyView = $denyEdit = false;
108108
109 - foreach( $wgWhiteListOverride['always']['read'] as $value )
110 - {
111 - if( self::RegexCompare($title, $value) )
112 - {
113 - $allowView = true;
114 - }
115 - }
 109+ foreach( $wgWhiteListOverride['always']['read'] as $value )
 110+ {
 111+ if( self::RegexCompare($title, $value) )
 112+ {
 113+ $allowView = true;
 114+ }
 115+ }
116116
117 - foreach( $wgWhiteListOverride['always']['edit'] as $value )
118 - {
119 - if( self::RegexCompare($title, $value) )
120 - {
121 - $allowEdit = true;
122 - }
123 - }
 117+ foreach( $wgWhiteListOverride['always']['edit'] as $value )
 118+ {
 119+ if ( self::RegexCompare($title, $value) )
 120+ {
 121+ $allowEdit = true;
 122+ }
 123+ }
124124
125125 unset($override);
126126
127127 foreach( $wgWhiteListOverride['never']['read'] as $value )
128 - {
129 - if( self::RegexCompare($title, $value) )
130 - {
131 - $denyView = true;
132 - }
133 - }
 128+ {
 129+ if ( self::RegexCompare($title, $value) )
 130+ {
 131+ $denyView = true;
 132+ }
 133+ }
134134
135 - foreach( $wgWhiteListOverride['never']['edit'] as $value )
136 - {
137 - if( self::RegexCompare($title, $value) )
138 - {
139 - $denyEdit = true;
140 - }
141 - }
 135+ foreach( $wgWhiteListOverride['never']['edit'] as $value )
 136+ {
 137+ if ( self::RegexCompare($title, $value) )
 138+ {
 139+ $denyEdit = true;
 140+ }
 141+ }
142142
143143 if( $action == 'edit' )
144144 {
——@@ -163,12 +163,12 @@
164164
165165 /* Allow access to user pages (unless disabled)
166166 */
167 - static function IsUserPage( $title_text, &$wgUser )
 167+ static function IsUserPage( $title_text, &$user )
168168 {
169169 global $wgWhiteListAllowUserPages;
170170
171 - $userPage = $wgUser->getUserPage()->getPrefixedText();
172 - $userTalkPage = $wgUser->getTalkPage()->getPrefixedText();
 171+ $userPage = $user->getUserPage()->getPrefixedText();
 172+ $userTalkPage = $user->getTalkPage()->getPrefixedText();
173173
174174 if( ($wgWhiteListAllowUserPages == true) &&
175175 ($title_text == $userPage) || ($title_text == $userTalkPage) )
——@@ -177,25 +177,24 @@
178178 return WHITELIST_NOACTION;
179179 }
180180
181 - static function IsAllowedNamespace( &$title, &$wgUser, $action)
182 - {
 181+ static function IsAllowedNamespace( &$title, &$user, $action)
 182+ {
 183+ $page_ns = $title->getNamespace();
 184+ if ( ( $page_ns == NS_MEDIAWIKI ) ||
 185+ ( $page_ns == NS_FILE ) ||
 186+ ( $page_ns == NS_HELP ) )
 187+ {
 188+ return WHITELIST_GRANT;
 189+ }
183190
184 - $page_ns = $title->getNsText();
185 - if( ($page_ns == 'Mediawiki' ) ||
186 - ($page_ns == 'Image' ) ||
187 - ($page_ns == 'Help' ) )
188 - {
189 - return WHITELIST_GRANT;
190 - }
 191+ return WHITELIST_NOACTION;
 192+ }
191193
192 - return WHITELIST_NOACTION;
193 - }
194194
195 -
196195 /* Check whether the page is whitelisted.
197196 * returns true if page is on whitelist, false if it is not.
198197 */
199 - static function IsAllowed( &$title, &$wgUser, $action )
 198+ static function IsAllowed( &$title, &$user, $action )
200199 {
201200 $expired = false;
202201
——@@ -209,47 +208,47 @@
210209 $current_date = date("Y-m-d H:i:s");
211210 $sql = "SELECT wl_page_title
212211 FROM " . $wl_table_name . "
213 - WHERE wl_user_id = " . $dbr->addQuotes($wgUser->getId()) . "
 212+ WHERE wl_user_id = " . $dbr->addQuotes($user->getId()) . "
214213 AND ( (wl_expires_on >= " . $dbr->addQuotes($current_date) . ")
215214 OR ( wl_expires_on = " . $dbr->addQuotes('') . "))";
216215 if( $action == 'edit' ) {
217216 $sql .= "
218217 AND wl_allow_edit = " . $dbr->addQuotes('1');
219218 }
220 -//wfDebug($sql);
221219
222 - // We should also check that $title is not a redirect to a whitelisted page
223 - $redirecttitle = null;
224 - $article = new Article($title);
225 - if (is_object($article))
226 - {
227 - $pagetext = $article->getContent();
228 - $redirecttitle = Title::newFromRedirect($pagetext);
229 - }
 220+ // We should also check that $title is not a redirect to a whitelisted page
 221+ $redirecttitle = null;
 222+ $article = new Article($title);
 223+ if (is_object($article))
 224+ {
 225+ $pagetext = $article->getContent();
 226+ $redirecttitle = Title::newFromRedirect($pagetext);
 227+ }
230228
231229 /* Loop through each result returned and
232230 * check for matches.
233231 */
234 - $dbr->begin();
 232+ $dbr->begin();
235233 $db_results = $dbr->query( $sql , __METHOD__, true);
236 - $dbr->commit();
 234+ $dbr->commit();
237235 while( $db_result = $dbr->fetchObject($db_results) )
238236 {
239237 if( self::RegexCompare($title, $db_result->wl_page_title) )
240238 {
241239 $dbr->freeResult($db_results);
242 -//wfDebug("\n\nAccess granted based on PAGE [" . $db_result->wl_page_title . "]\n\n");
 240+ //wfDebug("\n\nAccess granted based on PAGE [" . $db_result->wl_page_title . "]\n\n");
243241 return WHITELIST_GRANT;
244242 }
245 - if ($redirecttitle)
246 - {
247 - if( self::RegexCompare($redirecttitle, $db_result->wl_page_title) )
248 - {
249 - $dbr->freeResult($db_results);
250 -//wfDebug("\n\nAccess granted based on REDIRECT to PAGE [" . $db_result->wl_page_title . "]\n\n");
251 - return WHITELIST_GRANT;
252 - }
253 - }
 243+
 244+ if ($redirecttitle)
 245+ {
 246+ if ( self::RegexCompare($redirecttitle, $db_result->wl_page_title) )
 247+ {
 248+ $dbr->freeResult($db_results);
 249+ //wfDebug("\n\nAccess granted based on REDIRECT to PAGE [" . $db_result->wl_page_title . "]\n\n");
 250+ return WHITELIST_GRANT;
 251+ }
 252+ }
254253 }
255254 $dbr->freeResult($db_results);
256255
——@@ -259,7 +258,7 @@
260259 /* Returns true if hit, false otherwise */
261260 static function RegexCompare(&$title, $sql_regex)
262261 {
263 - global $wgWhiteListWildCardInsensitive;
 262+ global $wgWhiteListWildCardInsensitive;
264263
265264 $ret_val = false;
266265
——@@ -348,3 +347,4 @@
349348 return true;
350349 }
351350 } /* End class */
 351+
Index: trunk/extensions/WhiteListEdit/WhiteListEdit_body.php
——@@ -297,7 +297,7 @@
298298
299299 function DisplayContractorEditDetails( $contractorId )
300300 {
301 - global $wgOut, $wgUser, $wgWhiteListUsePrettyCalendar;
 301+ global $wgOut, $wgWhiteListUsePrettyCalendar;
302302 $dbr = wfGetDB( DB_SLAVE );
303303
304304 $wgOut->addScript( <<<END
Index: trunk/extensions/WhiteListEdit/WhiteListEdit.php
——@@ -31,7 +31,7 @@
3232 $wgExtensionCredits['specialpage'][] = array(
3333 'path' => __FILE__,
3434 'name' => 'WhiteListEdit',
35 - 'version' => 'v0.11.2',
 35+ 'version' => 'v0.12',
3636 'author' => array('Paul Grinberg', 'Mike Sullivan'),
3737 'email' => 'gri6507 at yahoo dot com, ms-mediawiki AT umich DOT edu',
3838 'descriptionmsg' => 'whitelist-desc',
——@@ -39,10 +39,8 @@
4040 );
4141
4242 # these are the groups and the rights used within this extension
43 -if ( !isset( $wgWhiteListRestrictedGroup ) )
44 - $wgWhiteListRestrictedGroup = 'restricted';
45 -if ( !isset( $wgWhiteListManagerGroup ) )
46 - $wgWhiteListManagerGroup = 'manager';
 43+$wgWhiteListRestrictedGroup = 'restricted';
 44+$wgWhiteListManagerGroup = 'manager';
4745
4846 # Define groups and rights
4947 if ( !isset( $wgGroupPermissions['*']['usewhitelist'] ) )
——@@ -77,18 +75,15 @@
7876 $wgWhiteListOverride['never']['edit'] = array();
7977
8078 # Define default case insensitivity setting
81 -if ( !isset( $wgWhiteListWildCardInsensitive ) )
82 - $wgWhiteListWildCardInsensitive = true;
 79+$wgWhiteListWildCardInsensitive = true;
8380
8481 # Define default user page setting
85 -if ( !isset( $wgWhiteListAllowUserPages ) )
86 - $wgWhiteListAllowUserPages = true;
 82+$wgWhiteListAllowUserPages = true;
8783
8884 # If you want the pretty calendar feature, you must install the Extension:Usage_Statistics.
8985 # If you do not want that feature, then set the following variable to false.
9086 # NOTE: you do not actually need the gnuplot extension for the functionality needed by this extension
91 -if ( !isset( $wgWhiteListUsePrettyCalendar ) )
92 - $wgWhiteListUsePrettyCalendar = true;
 87+$wgWhiteListUsePrettyCalendar = true;
9388
9489 $dir = dirname( __FILE__ ) . '/';
9590

Status & tagging log