r49756 - Code Review

From MediaWiki.org

Jump to: navigation, search
Repository:MediaWiki
Revision:r49755 | r49756 (on ViewVC) | r49757 >
Date:04:23, 23 April 2009
Author:werdna
Status:resolved (Comments)
Tags:
Comment:Add import/export interface for filters so that filters can be copied across wikis
Modified paths:

Diff [purge]

Index: trunk/extensions/AbuseFilter/AbuseFilter.php
===================================================================
--- trunk/extensions/AbuseFilter/AbuseFilter.php	(revision 49755)
+++ trunk/extensions/AbuseFilter/AbuseFilter.php	(revision 49756)
@@ -43,6 +43,7 @@
 $wgAutoloadClasses['AbuseFilterViewExamine'] = "$dir/Views/AbuseFilterViewExamine.php";
 $wgAutoloadClasses['AbuseFilterChangesList'] = "$dir/Views/AbuseFilterViewExamine.php";
 $wgAutoloadClasses['AbuseFilterViewDiff'] = "$dir/Views/AbuseFilterViewDiff.php";
+$wgAutoloadClasses['AbuseFilterViewImport'] = "$dir/Views/AbuseFilterViewImport.php";
 
 $wgAutoloadClasses['AbuseFilterVariableHolder'] = "$dir/AbuseFilterVariableHolder.php";
 $wgAutoloadClasses['AFComputedVariable'] = "$dir/AbuseFilterVariableHolder.php";
@@ -102,7 +103,7 @@
 $wgAjaxExportList[] = 'AbuseFilter::ajaxCheckFilterWithVars';
 
 // Bump the version number every time you change any of the .css/.js files
-$wgAbuseFilterStyleVersion = 8;
+$wgAbuseFilterStyleVersion = 9;
 
 $wgAbuseFilterRestrictedActions = array( 'block', 'degroup' );
 
Index: trunk/extensions/AbuseFilter/SpecialAbuseFilter.php
===================================================================
--- trunk/extensions/AbuseFilter/SpecialAbuseFilter.php	(revision 49755)
+++ trunk/extensions/AbuseFilter/SpecialAbuseFilter.php	(revision 49756)
@@ -90,6 +90,11 @@
 			$pageType = 'edit';
 		}
 		
+		if ( $subpage == 'import' ) {
+			$view = 'AbuseFilterViewImport';
+			$pageType = 'import';
+		}
+		
 		// Links at the top
 		AbuseFilter::addNavigationLinks( $wgOut, $this->mSkin, $pageType );
 
Index: trunk/extensions/AbuseFilter/Views/AbuseFilterViewImport.php
===================================================================
--- trunk/extensions/AbuseFilter/Views/AbuseFilterViewImport.php	(revision 0)
+++ trunk/extensions/AbuseFilter/Views/AbuseFilterViewImport.php	(revision 49756)
@@ -0,0 +1,18 @@
+<?php
+
+class AbuseFilterViewImport extends AbuseFilterView {
+	
+	function show( ) {
+		global $wgOut;
+		
+		$wgOut->addWikiMsg( 'abusefilter-import-intro' );
+		
+		$html = Xml::textarea( 'wpImportText', '', 40, 20 );
+		$html .= Xml::submitButton( wfMsg( 'abusefilter-import-submit' ) );
+		$url = SpecialPage::getTitleFor( 'AbuseFilter', 'new' )->getFullURL();
+		
+		$html = Xml::tags( 'form', array( 'method' => 'post', 'action' => $url ), $html );
+		
+		$wgOut->addHTML( $html );
+	}
+}
Index: trunk/extensions/AbuseFilter/Views/AbuseFilterViewEdit.php
===================================================================
--- trunk/extensions/AbuseFilter/Views/AbuseFilterViewEdit.php	(revision 49755)
+++ trunk/extensions/AbuseFilter/Views/AbuseFilterViewEdit.php	(revision 49756)
@@ -360,10 +360,16 @@
 			$fields['abusefilter-edit-history'] = 
 				$sk->makeKnownLinkObj( $this->getTitle( 'history/'.$filter ), $history_display );
 		}
+		
+		// Add export
+		$exportText = serialize( array( 'row' => $row, 'actions' => $actions ) );
+		$tools .= Xml::tags( 'a', array( 'href' => 'javascript:afShowExport();' ),
+								wfMsgExt( 'abusefilter-edit-export', 'parseinline' ) );
+		$tools .= Xml::element( 'textarea',
+					array( 'readonly' => 'readonly', 'id' => 'mw-abusefilter-export' ),
+					$exportText );
 
-		if ($tools) {
-			$fields['abusefilter-edit-tools'] = $tools;
-		}
+		$fields['abusefilter-edit-tools'] = $tools;
 
 		$form = Xml::buildForm( $fields );
 		$form = Xml::fieldset( wfMsg( 'abusefilter-edit-main' ), $form );
@@ -660,55 +666,77 @@
 
 		$row->mOriginalRow = clone $row;
 		$row->mOriginalActions = $origActions;
-
-		$textLoads = array( 
-			'af_public_comments' => 'wpFilterDescription', 
-			'af_pattern' => 'wpFilterRules', 
-			'af_comments' => 'wpFilterNotes' );
-
-		foreach( $textLoads as $col => $field ) {
-			$row->$col = trim($wgRequest->getVal( $field ));
-		}
-
-		$row->af_deleted = $wgRequest->getBool( 'wpFilterDeleted' );
-		$row->af_enabled = $wgRequest->getBool( 'wpFilterEnabled' ) && !$row->af_deleted;
-		$row->af_hidden = $wgRequest->getBool( 'wpFilterHidden' );
-		global $wgAbuseFilterIsCentral;
-		$row->af_global = $wgRequest->getBool( 'wpFilterGlobal' ) && $wgAbuseFilterIsCentral;
-
-		// Actions
-		global $wgAbuseFilterAvailableActions;
-		$actions = array();
-		foreach( $wgAbuseFilterAvailableActions as $action ) {
-			// Check if it's set
-			$enabled = $wgRequest->getBool( 'wpFilterAction'.ucfirst($action) );
-
-			if ($enabled) {
-				$parameters = array();
-
-				if ($action == 'throttle') {
-					// We need to load the parameters
-					$throttleCount = $wgRequest->getIntOrNull( 'wpFilterThrottleCount' );
-					$throttlePeriod = $wgRequest->getIntOrNull( 'wpFilterThrottlePeriod' );
-					$throttleGroups = explode( "\n", 
-						trim( $wgRequest->getText( 'wpFilterThrottleGroups' ) ) );
-
-					$parameters[0] = $this->mFilter; // For now, anyway
-					$parameters[1] = "$throttleCount,$throttlePeriod";
-					$parameters = array_merge( $parameters, $throttleGroups );
-				} elseif ($action == 'warn') {
-					$specMsg = $wgRequest->getVal( 'wpFilterWarnMessage' );
-
-					if ($specMsg == 'other')
-						$specMsg = $wgRequest->getVal( 'wpFilterWarnMessageOther' );
-					
-					$parameters[0] = $specMsg;
-				} elseif ($action == 'tag') {
-					$parameters = explode("\n", $wgRequest->getText( 'wpFilterTags' ) );
+		
+		// Check for importing
+		$import = $wgRequest->getVal( 'wpImportText' );
+		if ($import) {
+			$data = unserialize($import);
+			
+			$importRow = $data['row'];
+			$actions = $data['actions'];
+			
+			$copy = array(
+						'af_public_comments',
+						'af_pattern',
+						'af_comments',
+						'af_deleted',
+						'af_enabled',
+						'af_hidden',
+					);
+			
+			foreach( $copy as $name ) {
+				$row->$name = $importRow->$name;
+			}
+		} else {
+			$textLoads = array( 
+				'af_public_comments' => 'wpFilterDescription', 
+				'af_pattern' => 'wpFilterRules', 
+				'af_comments' => 'wpFilterNotes' );
+	
+			foreach( $textLoads as $col => $field ) {
+				$row->$col = trim($wgRequest->getVal( $field ));
+			}
+	
+			$row->af_deleted = $wgRequest->getBool( 'wpFilterDeleted' );
+			$row->af_enabled = $wgRequest->getBool( 'wpFilterEnabled' ) && !$row->af_deleted;
+			$row->af_hidden = $wgRequest->getBool( 'wpFilterHidden' );
+			global $wgAbuseFilterIsCentral;
+			$row->af_global = $wgRequest->getBool( 'wpFilterGlobal' ) && $wgAbuseFilterIsCentral;
+	
+			// Actions
+			global $wgAbuseFilterAvailableActions;
+			$actions = array();
+			foreach( $wgAbuseFilterAvailableActions as $action ) {
+				// Check if it's set
+				$enabled = $wgRequest->getBool( 'wpFilterAction'.ucfirst($action) );
+	
+				if ($enabled) {
+					$parameters = array();
+	
+					if ($action == 'throttle') {
+						// We need to load the parameters
+						$throttleCount = $wgRequest->getIntOrNull( 'wpFilterThrottleCount' );
+						$throttlePeriod = $wgRequest->getIntOrNull( 'wpFilterThrottlePeriod' );
+						$throttleGroups = explode( "\n", 
+							trim( $wgRequest->getText( 'wpFilterThrottleGroups' ) ) );
+	
+						$parameters[0] = $this->mFilter; // For now, anyway
+						$parameters[1] = "$throttleCount,$throttlePeriod";
+						$parameters = array_merge( $parameters, $throttleGroups );
+					} elseif ($action == 'warn') {
+						$specMsg = $wgRequest->getVal( 'wpFilterWarnMessage' );
+	
+						if ($specMsg == 'other')
+							$specMsg = $wgRequest->getVal( 'wpFilterWarnMessageOther' );
+						
+						$parameters[0] = $specMsg;
+					} elseif ($action == 'tag') {
+						$parameters = explode("\n", $wgRequest->getText( 'wpFilterTags' ) );
+					}
+	
+					$thisAction = array( 'action' => $action, 'parameters' => $parameters );
+					$actions[$action] = $thisAction;
 				}
-
-				$thisAction = array( 'action' => $action, 'parameters' => $parameters );
-				$actions[$action] = $thisAction;
 			}
 		}
 
Index: trunk/extensions/AbuseFilter/AbuseFilter.class.php
===================================================================
--- trunk/extensions/AbuseFilter/AbuseFilter.class.php	(revision 49755)
+++ trunk/extensions/AbuseFilter/AbuseFilter.class.php	(revision 49756)
@@ -126,6 +126,7 @@
 					'examine' => 'Special:AbuseFilter/examine',
 					'log' => 'Special:AbuseLog',
 					'tools' => 'Special:AbuseFilter/tools',
+					'import' => 'Special:AbuseFilter/import',
 				);
 				
 		// Save some translator work
Index: trunk/extensions/AbuseFilter/edit.js
===================================================================
--- trunk/extensions/AbuseFilter/edit.js	(revision 49755)
+++ trunk/extensions/AbuseFilter/edit.js	(revision 49756)
@@ -144,6 +144,10 @@
 	window.location = wgScript + '?title=MediaWiki:'+encodeURIComponent( message )+'&action=edit';
 }
 
+function afShowExport() {
+	document.getElementById( 'mw-abusefilter-export' ).style.display = 'block';
+}
+
 addOnloadHook( function() {
 	addHandler( document.getElementById( wgFilterBoxName ), 'keyup', function() {
 		el = document.getElementById( 'mw-abusefilter-syntaxresult' );
@@ -168,4 +172,10 @@
 	}
 
 	setupActions();
-} );
\ No newline at end of file
+	
+	var exporter = document.getElementById( 'mw-abusefilter-export' );
+	
+	if (exporter ) {
+		exporter.style.display = 'none';
+	}
+} );
Index: trunk/extensions/AbuseFilter/AbuseFilter.i18n.php
===================================================================
--- trunk/extensions/AbuseFilter/AbuseFilter.i18n.php	(revision 49755)
+++ trunk/extensions/AbuseFilter/AbuseFilter.i18n.php	(revision 49756)
@@ -209,6 +209,7 @@
 	'abusefilter-edit-tools' => 'Tools:',
 	'abusefilter-edit-test-link' => 'Test this filter against recent edits',
 	'abusefilter-edit-global' => 'Apply this filter globally',
+	'abusefilter-edit-export' => 'Export this filter to another wiki',
 
 	// Filter editing helpers
 	'abusefilter-edit-builder-select' => 'Select an option to add it at the cursor',
@@ -411,6 +412,7 @@
 	'abusefilter-topnav-examine' => 'Examine past edits',
 	'abusefilter-topnav-log' => 'Abuse Log',
 	'abusefilter-topnav-tools' => 'Debugging tools',
+	'abusefilter-topnav-import' => 'Import filter',
 	
 	// Logging
 	'abusefilter-log-name' => 'Abuse Filter log',
@@ -426,6 +428,12 @@
 	'abusefilter-diff-pattern' => 'Filter conditions',
 	'abusefilter-diff-invalid' => 'Unable to fetch the requested versions',
 	'abusefilter-diff-backhistory' => 'Back to filter history',
+	
+	// Import interface
+	'abusefilter-import-intro' => 'You can use this interface to import filters from other wikis.
+On the source wiki, click "export this filter to another wiki" under "tools" on the editing interface.
+Copy from the textbox that appears, and paste it into this textbox, then click "Import",',
+	'abusefilter-import-submit' => 'Import data',
 );
 
 /** Message documentation (Message documentation)

Comments

#Comment by VasilievVV (Talk | contribs)   15:00, 10 May 2009

unserialze() calls __wakeup() method of unserialized objects, which may cause remote execution vulnerability.

#Comment by Werdna (Talk | contribs)   03:48, 13 May 2009

You can't define a class in serialized output, so it isn't trivially exploitable.

Certainly something worth changing in some way, but can you actually show a proof-of-concept exploit? I don't think we use __wakeup anywhere in MediaWiki except for one class in AbuseFilter.

#Comment by Catrope (Talk | contribs)   15:28, 13 May 2009

This is correct: it'd only be exploitable given a class with a __wakeup() method that doesn't do sufficient sanity checking.

Even though, I vaguely recall Brion using json_decode() over unserialize() somewhere (CodeReview maybe) because of security concerns; will check with him.

#Comment by ^demon (Talk | contribs)   16:18, 15 May 2009

It was the other way around, used serialization instead of json. See r44643

#Comment by Tim Starling (Talk | contribs)   07:19, 22 May 2009

r44643 is concerned with trusted input. It's good practice to avoid using PHP serialize as an interchange format, it's easy to expose security vulnerabilities without realising it and there is a well known case of a class in PEAR doing exactly that. And it's ugly and hard for humans to read or edit, problems which XML and JSON don't have.

#Comment by Werdna (Talk | contribs)   13:03, 2 June 2009

Fixed up in r51341.

Status & tagging log

Views
Toolbox