Update command doesn't escape php_binary path, update fails when path has spaces

[adamperrry]

Bug Report

--- ✅ If you are in the correct location now... --->

• Yes, I reviewed the contribution guidelines.
• Yes, more specifically, I reviewed the guidelines on how to write clear bug reports.

Describe the current, buggy behavior

On macOS, at least, the wp cli update command fails when the path to the PHP binary running wp-cli (i.e. the value of PHP_BINARY when running the phar - see Utils\get_php_binary()) has a space in it. For our app, we give the option to download different versions of PHP, and we store those binaries in the Application Support folder. When a user is running wp-cli using any of those downloaded versions, the update will fail. However, it doesn't fail when they run the update command from our bundled version of PHP, which lives at a location without spaces in the path.

Describe how other contributors can replicate this bug

• Move the PHP binary to a location with a space in the path.
• Add it to $PATH and run an older version of wp-cli with it
• Run wp cli update and watch the command fail, saying the location doesn't exist, with the path given ending at the space.

Describe what you expect as the correct outcome

I expect the wp cli update command to succeed regardless of where the PHP binaries being used live.

Let us know what environment you are running this on

OS:	Darwin 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:22 PDT 2023; root:xnu-8796.121.3~7/RELEASE_X86_64 x86_64
Shell:	/bin/zsh
PHP binary:	/Users/adam.perry/Library/Application Support/Local/lightning-services/php-8.0.22+6/bin/darwin/bin/php
PHP version:	8.0.22
php.ini used:	/Users/adam.perry/Library/Application Support/Local/run/CiJoccyP_/conf/php/php.ini
MySQL binary:	/Users/adam.perry/dev/local/flywheel-local/extraResources/lightning-services/mysql-8.0.16+6/bin/darwin/bin/mysql
MySQL version:	mysql  Ver 8.0.16 for macos10.14 on x86_64 (MySQL Community Server - GPL)
SQL modes:	
WP-CLI root dir:	phar://wp-cli.phar/vendor/wp-cli/wp-cli
WP-CLI vendor dir:	phar://wp-cli.phar/vendor
WP_CLI phar path:	/Users/adam.perry/Local Sites/vernacom/app/public
WP-CLI packages dir:	
WP-CLI cache dir:	/Users/adam.perry/.wp-cli/cache
WP-CLI global config:	/Users/adam.perry/dev/local/flywheel-local/extraResources/bin/wp-cli/config.yaml
WP-CLI project config:	
WP-CLI version:	2.7.1

Provide a possible solution

In CLI_Command.php > CLI_Command > update, change:

$php_binary = Utils\get_php_binary();

to

$php_binary = escapeshellarg(Utils\get_php_binary());

DIFF:

diff --git a/php/commands/src/CLI_Command.php b/php/commands/src/CLI_Command.php
index 9ed7e080..09b3e1ec 100644
--- a/php/commands/src/CLI_Command.php
+++ b/php/commands/src/CLI_Command.php
@@ -356,7 +356,7 @@ class CLI_Command extends WP_CLI_Command {
                }
 
                $allow_root = WP_CLI::get_runner()->config['allow-root'] ? '--allow-root' : '';
-               $php_binary = Utils\get_php_binary();
+               $php_binary =escapeshellarg( Utils\get_php_binary());
                $process    = Process::create( "{$php_binary} $temp --info {$allow_root}" );
                $result     = $process->run();
                if ( 0 !== $result->return_code || false === stripos( $result->stdout, 'WP-CLI version' ) ) {

Provide additional context/screenshots

[danielbachhuber]

Thanks for the report, @adamperrry !

It'd probably be better to switch the entire thing to WP_CLI\Utils\esc_cmd():

wp-cli/php/utils.php

Lines 269 to 283 in 90be24d

	/**
	* Given a template string and an arbitrary number of arguments,
	* returns the final command, with the parameters escaped.
	*/
	function esc_cmd( $cmd ) {
	if ( func_num_args() < 2 ) {
	trigger_error( 'esc_cmd() requires at least two arguments.', E_USER_WARNING );
	}
	$args = func_get_args();
	$cmd = array_shift( $args );
	return vsprintf( $cmd, array_map( 'escapeshellarg', $args ) );
	}

Feel free to submit a pull request, if you'd like. Here is some guidance on our pull request best practices.

We have some existing tests in the bundle, so I don't think you need to add new tests for this scenario:

https://github.com/wp-cli/wp-cli-bundle/blob/22e60f0568d54d91fca9c4c993c16df12252e54d/features/cli.feature#L31-L71

[danielbachhuber]

@adamperrry Looks like there's some problem with esc_cmd(), unfortunately: https://github.com/wp-cli/wp-cli-bundle/actions/runs/5744386636/job/15570571355?pr=563

I'm going to revert your PR for now. We could probably add a feature test to this repo so we have easier access to the failure.

[bgturner]

It looks like the logs for the Github action have expired and are no longer accessible:

https://github.com/wp-cli/wp-cli-bundle/actions/runs/5744386636/job/15570571355?pr=563

@danielbachhuber do you know what the issue is?

I'm still wrapping my head around how all these repos fit, but it looks like the wp-cli-bundle is mainly in charge of combining the various commands into the actual shippable phar -- is that a correct understanding?

Trying to get that error, I:

1. cloned the wp-cli-bundle repo
2. ran the behat tests to verify thing worked (a bunch of deprecation notices and some errors)
3. applied the change in Escape shell command in wp-cli update #5818 and re-ran the tests

It looks like the one new error is:

001 Scenario: Install WP-CLI nightly                      # features/cli.feature:120
      When I run `{PHAR_PATH} cli update --nightly --yes` # features/cli.feature:124
        $ /var/folders/52/nmp696211xn_dqh2pk4qhwmc0000gq/T/wp-cli-test-run--65428bfe77c590.99137120/wp-cli-build-65428bfe77dd61.15060864.phar cli update --nightly --yes
        Downloading from https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli-nightly.phar...
        md5 hash verified: 3416b03bac4826a30ab41eec8481215d
        
        
        	                                                                                                                       
        	 Warning: No WordPress installation found. If the command '' is in a plugin or theme, pass --path=`path/to/wordpress`. 
        	 Error: '' is not a registered wp command. See 'wp help' for available commands.                                       
        	 Did you mean 'db'?                                                                                                    
        	                                                                                                                       
        	                                                                                                                       
        
        Error: The downloaded PHAR is broken, try running wp cli update again.
        cwd: /var/folders/52/nmp696211xn_dqh2pk4qhwmc0000gq/T/wp-cli-test-run--65428bfe77c590.99137120/
        run time: 1.1823959350586
        exit status: 1 (RuntimeException)

Does any of that look like it has an obvious quick-fix?

When you say:

We could probably add a feature test to this repo so we have easier access to the failure.

Can you elaborate on that a bit? That would be another test here in wp-cli/wp-cli right? But wouldn't there still be the above failing error within the wp-cli-bundle repo?

[danielbachhuber]

do you know what the issue is?

@bgturner I don't recall, no 😔

If you want to re-create the original PR, we can spin up a wp-cli/wp-cli-bundle PR that uses the branch and see how the tests fail again. Or you can add the same feature test to this repo and we can see the error directly on the PR.

Given this is such a critical code path though, we should probably even have unit tests around this particular logic.

I'm still wrapping my head around how all these repos fit, but it looks like the wp-cli-bundle is mainly in charge of combining the various commands into the actual shippable phar -- is that a correct understanding?

Correct (and performing a final set of tests).

[bgturner]

@danielbachhuber -- was able to put in a little bit of time today and learned a bit more about Behat.

I think I'm creating the bones of tests for wp cli update in this draft PR: #5855

but I'm curious to see what errors show because I was definitely getting errors locally.