Skip to content

Escape spaces in PHP binary path #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Escape spaces in PHP binary path #149

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
079eb1a
Escape spaces in PHP binary path
azatakmyradov Oct 4, 2023
8a8ac6d
Add tests for escaping php binary
azatakmyradov Oct 4, 2023
9498c76
Escape php binary path spaces according to os
azatakmyradov Oct 4, 2023
d0060cc
Add more tests for escaping string path
azatakmyradov Oct 5, 2023
66fadde
Trim path before escaping the value
azatakmyradov Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/Settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public static function parseArguments(array $arguments)

// Use the currently invoked php as the default if possible
if (defined('PHP_BINARY')) {
$settings->phpExecutable = PHP_BINARY;
$settings->phpExecutable = self::escapeString(PHP_BINARY);
}

foreach ($arguments as $argument) {
Expand Down Expand Up @@ -235,6 +235,22 @@ public static function getPathsFromStdIn()
$lines = explode("\n", rtrim($content));
return array_map('rtrim', $lines);
}

/**
* @param string $path
* @param string $os
* @return string
*/
public static function escapeString(string $path, string $os = PHP_OS)
{
if (stripos(strtoupper($os), 'WIN') === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (stripos(strtoupper($os), 'WIN') === 0) {
if (stripos($os, 'WIN') === 0) {

stripos() is already a case-insensitive comparison, so no need for the strtoupper().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl Yes I agree with you that it might make sense to escape it when it's actually used. First place I encountered the error was when running php artisan insights which in turn runs php vendor/bin/parallel-lint FILE_NAME_HERE. I think it's caused by parseArguments call in Application.php line 36 at which point php executable is already set. I guess I could escape it after because I see phpExecutable property is public.

At this point I can't use the package out of the box unless I pass override the php path manually using -p flag. However, I have to change script inside vendor directory if I were to use phpinsights by Nuno Madaruno which depends on this package.

What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems the comment is misplaced. the discussion thread was started about not using strtoupper:

$path = str_replace('^\\', '\\', preg_replace('`(?<!^) `', '^ ', escapeshellcmd($path)));
} else {
$path = preg_replace('`(?<!\\\) `', '\ ', escapeshellcmd($path));
}

return $path;
}
}

class ArrayIterator extends \ArrayIterator
Expand Down
35 changes: 35 additions & 0 deletions tests/Settings.parseArguments.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,41 @@ class SettingsParseArgumentsTest extends Tester\TestCase

Assert::equal($expectedSettings->syntaxErrorCallbackFile, $settings->syntaxErrorCallbackFile);
}

public function testEscapingPathForUnixSystems()
{
Settings::escapeString('path with spaces');

$php_path = "path with/spaces between/php.exe";
Assert::equal(Settings::escapeString($php_path, 'Darwin'), "path\\ with/spaces\\ between/php.exe");
}

public function testEscapingPathForWindows()
{
Settings::escapeString('path with spaces');

$php_path = "path with\spaces between\php.exe";
Assert::equal(Settings::escapeString($php_path, 'Windows'), 'path^ with\\\spaces^ between\\\php.exe');
}

public function testEscapingEmptyPath()
{
Assert::equal(Settings::escapeString(''), '');
}

public function testEscapingPathWithSpecialChars()
{
$path = "path/with/special&characters";

Assert::equal(Settings::escapeString($path), "path/with/special\&characters");
}

public function testEscapingPathWithLeadingTrailingSpaces()
{
$path = " path with\spaces ";

Assert::equal(Settings::escapeString($path), "path\\ with\\\spaces");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't seem to be correct, you imho \s should be encoded as \\s or even \\\\s since you enclosed it in " not '.

}
}

$testCase = new SettingsParseArgumentsTest;
Expand Down