From 7c75ee39766c9a521e369356b9e69c835a421703 Mon Sep 17 00:00:00 2001
From: Yuri Astrakhan <yurik@wikimedia.org>
Date: Wed, 8 Oct 2014 21:47:35 -0400
Subject: [PATCH] Remove duplicate param escaping code

wfEscapeShellArg() can handle multiple params, escaping each.
This patch changes wfShellExec() to call wfEscapeShellArg() directly
instead of doing the gluing itself.

This patch also extends wfEscapeShellArg() to accept an array parameter
optionally instead of as separate args, which is often useful.

Added also unit test cases for single, multiple args, and single array args.

Change-Id: I7a0761cc2ba98c210a9eacadd12da407d933e42a
---
 includes/GlobalFunctions.php                  | 25 +++++------
 .../GlobalFunctions/wfEscapeShellArgTest.php  | 43 +++++++++++++++++++
 2 files changed, 54 insertions(+), 14 deletions(-)
 create mode 100644 tests/phpunit/includes/GlobalFunctions/wfEscapeShellArgTest.php

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index c0b8913f4a..dfced1c0bb 100644
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -2660,13 +2660,19 @@ function wfIniGetBool( $setting ) {
  * Also fixes the locale problems on Linux in PHP 5.2.6+ (bug backported to
  * earlier distro releases of PHP)
  *
- * @param string $args,...
+ * @param string ... strings to escape and glue together, or a single array of strings parameter
  * @return string
  */
 function wfEscapeShellArg( /*...*/ ) {
 	wfInitShellLocale();
 
 	$args = func_get_args();
+	if ( count( $args ) === 1 && is_array( reset( $args ) ) ) {
+		// If only one argument has been passed, and that argument is an array,
+		// treat it as a list of arguments
+		$args = reset( $args );
+	}
+
 	$first = true;
 	$retVal = '';
 	foreach ( $args as $arg ) {
@@ -2799,12 +2805,7 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(),
 		}
 	}
 	if ( is_array( $cmd ) ) {
-		// Command line may be given as an array, escape each value and glue them together with a space
-		$cmdVals = array();
-		foreach ( $cmd as $val ) {
-			$cmdVals[] = wfEscapeShellArg( $val );
-		}
-		$cmd = implode( ' ', $cmdVals );
+		$cmd = wfEscapeShellArg( $cmd );
 	}
 
 	$cmd = $envcmd . $cmd;
@@ -3047,7 +3048,7 @@ function wfShellWikiCmd( $script, array $parameters = array(), array $options =
 	}
 	$cmd[] = $script;
 	// Escape each parameter for shell
-	return implode( " ", array_map( 'wfEscapeShellArg', array_merge( $cmd, $parameters ) ) );
+	return wfEscapeShellArg( array_merge( $cmd, $parameters ) );
 }
 
 /**
@@ -3092,10 +3093,7 @@ function wfMerge( $old, $mine, $yours, &$result ) {
 	fclose( $yourtextFile );
 
 	# Check for a conflict
-	$cmd = wfEscapeShellArg( $wgDiff3 ) . ' -a --overlap-only ' .
-		wfEscapeShellArg( $mytextName ) . ' ' .
-		wfEscapeShellArg( $oldtextName ) . ' ' .
-		wfEscapeShellArg( $yourtextName );
+	$cmd = wfEscapeShellArg( $wgDiff3, '-a', '--overlap-only', $mytextName, $oldtextName, $yourtextName );
 	$handle = popen( $cmd, 'r' );
 
 	if ( fgets( $handle, 1024 ) ) {
@@ -3106,8 +3104,7 @@ function wfMerge( $old, $mine, $yours, &$result ) {
 	pclose( $handle );
 
 	# Merge differences
-	$cmd = wfEscapeShellArg( $wgDiff3 ) . ' -a -e --merge ' .
-		wfEscapeShellArg( $mytextName, $oldtextName, $yourtextName );
+	$cmd = wfEscapeShellArg( $wgDiff3, '-a', '-e', '--merge', $mytextName, $oldtextName, $yourtextName );
 	$handle = popen( $cmd, 'r' );
 	$result = '';
 	do {
diff --git a/tests/phpunit/includes/GlobalFunctions/wfEscapeShellArgTest.php b/tests/phpunit/includes/GlobalFunctions/wfEscapeShellArgTest.php
new file mode 100644
index 0000000000..cb334d2fd2
--- /dev/null
+++ b/tests/phpunit/includes/GlobalFunctions/wfEscapeShellArgTest.php
@@ -0,0 +1,43 @@
+<?php
+
+/**
+ * @group GlobalFunctions
+ * @covers ::wfEscapeShellArg
+ */
+class wfEscapeShellArgTest extends MediaWikiTestCase {
+	public function testSingleInput() {
+		if ( wfIsWindows() ) {
+			$expected = '"blah"';
+		} else {
+			$expected = "'blah'";
+		}
+
+		$actual = wfEscapeShellArg( 'blah' );
+
+		$this->assertEquals( $expected, $actual );
+	}
+
+	public function testMultipleArgs() {
+		if ( wfIsWindows() ) {
+			$expected = '"foo" "bar" "baz"';
+		} else {
+			$expected = "'foo' 'bar' 'baz'";
+		}
+
+		$actual = wfEscapeShellArg( 'foo', 'bar', 'baz' );
+
+		$this->assertEquals( $expected, $actual );
+	}
+
+	public function testMultipleArgsAsArray() {
+		if ( wfIsWindows() ) {
+			$expected = '"foo" "bar" "baz"';
+		} else {
+			$expected = "'foo' 'bar' 'baz'";
+		}
+
+		$actual = wfEscapeShellArg( array( 'foo', 'bar', 'baz' ) );
+
+		$this->assertEquals( $expected, $actual );
+	}
+}
-- 
2.20.1