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

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index c0b8913..dfced1c 100644 (file)
--- 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 (file)
index 0000000..cb334d2
--- /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 );
+       }
+}