Skip to content

Commit

Permalink
Fix escaping of URLs in Perforce and Subversion drivers
Browse files Browse the repository at this point in the history
  • Loading branch information
@Seldaek
Seldaek committed Aug 25, 2018
1 parent 3d01ef2 commit bf12529
Show file tree
Hide file tree
Showing 10 changed files with 32 additions and 31 deletions.
2 changes: 1 addition & 1 deletion src/Composer/Repository/Vcs/FossilDriver.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public static function supports(IOInterface $io, Config $config, $url, $deep = f
return false; return false;
} }


$process = new ProcessExecutor(); $process = new ProcessExecutor($io);

This comment has been minimized.

Sign in to view

Copy link
@staabm

staabm Sep 4, 2018

Contributor

Should the ProcessExecutors' io arg be handled as mandatory instead of optional?

This comment has been minimized.

Sign in to view

Copy link
@Seldaek

Seldaek Sep 4, 2018

Author Member

It's kinda tricky.. I passed it whenever possible but in a few places it gets messy
// check whether there is a fossil repo in that path // check whether there is a fossil repo in that path
if ($process->execute('fossil info', $output, $url) === 0) { if ($process->execute('fossil info', $output, $url) === 0) {
return true; return true;
Expand Down
4 changes: 2 additions & 2 deletions src/Composer/Repository/Vcs/HgDriver.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public static function supports(IOInterface $io, Config $config, $url, $deep = f
return false; return false;
} }


$process = new ProcessExecutor(); $process = new ProcessExecutor($io);
// check whether there is a hg repo in that path // check whether there is a hg repo in that path
if ($process->execute('hg summary', $output, $url) === 0) { if ($process->execute('hg summary', $output, $url) === 0) {
return true; return true;
Expand All @@ -222,7 +222,7 @@ public static function supports(IOInterface $io, Config $config, $url, $deep = f
return false; return false;
} }


$processExecutor = new ProcessExecutor(); $processExecutor = new ProcessExecutor($io);
$exit = $processExecutor->execute(sprintf('hg identify %s', ProcessExecutor::escape($url)), $ignored); $exit = $processExecutor->execute(sprintf('hg identify %s', ProcessExecutor::escape($url)), $ignored);


return $exit === 0; return $exit === 0;
Expand Down
4 changes: 2 additions & 2 deletions src/Composer/Repository/Vcs/SvnDriver.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -307,10 +307,10 @@ public static function supports(IOInterface $io, Config $config, $url, $deep = f
return false; return false;
} }


$processExecutor = new ProcessExecutor(); $processExecutor = new ProcessExecutor($io);


$exit = $processExecutor->execute( $exit = $processExecutor->execute(
"svn info --non-interactive {$url}", "svn info --non-interactive ".ProcessExecutor::escape('{'.$url.'}'),
$ignoredOutput $ignoredOutput
); );


Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Util/Bitbucket.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public function __construct(IOInterface $io, Config $config, ProcessExecutor $pr
{ {
$this->io = $io; $this->io = $io;
$this->config = $config; $this->config = $config;
$this->process = $process ?: new ProcessExecutor; $this->process = $process ?: new ProcessExecutor($io);
$this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config); $this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config);
$this->time = $time; $this->time = $time;
} }
Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Util/Filesystem.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ protected function directorySize($directory)


protected function getProcess() protected function getProcess()
{ {
return new ProcessExecutor; return $this->processExecutor;
} }


/** /**
Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Util/GitHub.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(IOInterface $io, Config $config, ProcessExecutor $pr
{ {
$this->io = $io; $this->io = $io;
$this->config = $config; $this->config = $config;
$this->process = $process ?: new ProcessExecutor; $this->process = $process ?: new ProcessExecutor($io);
$this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config); $this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config);
} }


Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Util/GitLab.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public function __construct(IOInterface $io, Config $config, ProcessExecutor $pr
{ {
$this->io = $io; $this->io = $io;
$this->config = $config; $this->config = $config;
$this->process = $process ?: new ProcessExecutor(); $this->process = $process ?: new ProcessExecutor($io);
$this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config); $this->remoteFilesystem = $remoteFilesystem ?: Factory::createRemoteFilesystem($this->io, $config);
} }


Expand Down
18 changes: 9 additions & 9 deletions src/Composer/Util/Perforce.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public static function checkServerExists($url, ProcessExecutor $processExecutor)
{ {
$output = null; $output = null;


return 0 === $processExecutor->execute('p4 -p ' . $url . ' info -s', $output); return 0 === $processExecutor->execute('p4 -p ' . ProcessExecutor::escape($url) . ' info -s', $output);
} }


public function initialize($repoConfig) public function initialize($repoConfig)
Expand Down Expand Up @@ -105,7 +105,7 @@ public function generateUniquePerforceClientName()
public function cleanupClientSpec() public function cleanupClientSpec()
{ {
$client = $this->getClient(); $client = $this->getClient();
$task = 'client -d ' . $client; $task = 'client -d ' . ProcessExecutor::escape($client);
$useP4Client = false; $useP4Client = false;
$command = $this->generateP4Command($task, $useP4Client); $command = $this->generateP4Command($task, $useP4Client);
$this->executeCommand($command); $this->executeCommand($command);
Expand Down Expand Up @@ -383,7 +383,7 @@ public function p4Login()
if ($this->windowsFlag) { if ($this->windowsFlag) {
$this->windowsLogin($password); $this->windowsLogin($password);
} else { } else {
$command = 'echo ' . $password . ' | ' . $this->generateP4Command(' login -a', false); $command = 'echo ' . ProcessExecutor::escape($password) . ' | ' . $this->generateP4Command(' login -a', false);
$exitCode = $this->executeCommand($command); $exitCode = $this->executeCommand($command);
$result = trim($this->commandResult); $result = trim($this->commandResult);
if ($exitCode) { if ($exitCode) {
Expand All @@ -408,7 +408,7 @@ public function getFileContent($file, $identifier)
{ {
$path = $this->getFilePath($file, $identifier); $path = $this->getFilePath($file, $identifier);


$command = $this->generateP4Command(' print ' . $path); $command = $this->generateP4Command(' print ' . ProcessExecutor::escape($path));
$this->executeCommand($command); $this->executeCommand($command);
$result = $this->commandResult; $result = $this->commandResult;


Expand All @@ -429,7 +429,7 @@ public function getFilePath($file, $identifier)
} }


$path = substr($identifier, 0, $index) . '/' . $file . substr($identifier, $index); $path = substr($identifier, 0, $index) . '/' . $file . substr($identifier, $index);
$command = $this->generateP4Command(' files ' . $path, false); $command = $this->generateP4Command(' files ' . ProcessExecutor::escape($path), false);
$this->executeCommand($command); $this->executeCommand($command);
$result = $this->commandResult; $result = $this->commandResult;
$index2 = strpos($result, 'no such file(s).'); $index2 = strpos($result, 'no such file(s).');
Expand All @@ -452,7 +452,7 @@ public function getBranches()
if (!$this->isStream()) { if (!$this->isStream()) {
$possibleBranches[$this->p4Branch] = $this->getStream(); $possibleBranches[$this->p4Branch] = $this->getStream();
} else { } else {
$command = $this->generateP4Command('streams //' . $this->p4Depot . '/...'); $command = $this->generateP4Command('streams '.ProcessExecutor::escape('//' . $this->p4Depot . '/...'));
$this->executeCommand($command); $this->executeCommand($command);
$result = $this->commandResult; $result = $this->commandResult;
$resArray = explode(PHP_EOL, $result); $resArray = explode(PHP_EOL, $result);
Expand All @@ -464,7 +464,7 @@ public function getBranches()
} }
} }
} }
$command = $this->generateP4Command('changes '. $this->getStream() . '/...', false); $command = $this->generateP4Command('changes '. ProcessExecutor::escape($this->getStream() . '/...'), false);
$this->executeCommand($command); $this->executeCommand($command);
$result = $this->commandResult; $result = $this->commandResult;
$resArray = explode(PHP_EOL, $result); $resArray = explode(PHP_EOL, $result);
Expand Down Expand Up @@ -527,7 +527,7 @@ protected function getChangeList($reference)
return null; return null;
} }
$label = substr($reference, $index); $label = substr($reference, $index);
$command = $this->generateP4Command(' changes -m1 ' . $label); $command = $this->generateP4Command(' changes -m1 ' . ProcessExecutor::escape($label));
$this->executeCommand($command); $this->executeCommand($command);
$changes = $this->commandResult; $changes = $this->commandResult;
if (strpos($changes, 'Change') !== 0) { if (strpos($changes, 'Change') !== 0) {
Expand Down Expand Up @@ -555,7 +555,7 @@ public function getCommitLogs($fromReference, $toReference)
} }
$index = strpos($fromReference, '@'); $index = strpos($fromReference, '@');
$main = substr($fromReference, 0, $index) . '/...'; $main = substr($fromReference, 0, $index) . '/...';
$command = $this->generateP4Command('filelog ' . $main . '@' . $fromChangeList. ',' . $toChangeList); $command = $this->generateP4Command('filelog ' . ProcessExecutor::escape($main . '@' . $fromChangeList. ',' . $toChangeList));
$this->executeCommand($command); $this->executeCommand($command);


return $this->commandResult; return $this->commandResult;
Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Util/Svn.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public function __construct($url, IOInterface $io, Config $config, ProcessExecut
$this->url = $url; $this->url = $url;
$this->io = $io; $this->io = $io;
$this->config = $config; $this->config = $config;
$this->process = $process ?: new ProcessExecutor; $this->process = $process ?: new ProcessExecutor($io);
} }


public static function cleanEnv() public static function cleanEnv()
Expand Down
25 changes: 13 additions & 12 deletions tests/Composer/Test/Util/PerforceTest.php
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@


use Composer\Util\Perforce; use Composer\Util\Perforce;
use PHPUnit\Framework\TestCase; use PHPUnit\Framework\TestCase;
use Composer\Util\ProcessExecutor;


/** /**
* @author Matt Whittom <Matt.Whittom@veteransunited.com> * @author Matt Whittom <Matt.Whittom@veteransunited.com>
Expand Down Expand Up @@ -344,7 +345,7 @@ public function testGetBranchesWithStream()
{ {
$this->setPerforceToStream(); $this->setPerforceToStream();


$expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port streams //depot/...'; $expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port streams '.ProcessExecutor::escape('//depot/...');
$this->processExecutor->expects($this->at(0)) $this->processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand All @@ -357,7 +358,7 @@ function ($command, &$output) {
} }
) )
); );
$expectedCommand2 = 'p4 -u user -p port changes //depot/branch/...'; $expectedCommand2 = 'p4 -u user -p port changes '.ProcessExecutor::escape('//depot/branch/...');
$expectedCallback = function ($command, &$output) { $expectedCallback = function ($command, &$output) {
$output = 'Change 1234 on 2014/03/19 by Clark.Stuth@Clark.Stuth_test_client \'test changelist\''; $output = 'Change 1234 on 2014/03/19 by Clark.Stuth@Clark.Stuth_test_client \'test changelist\'';


Expand All @@ -374,7 +375,7 @@ function ($command, &$output) {


public function testGetBranchesWithoutStream() public function testGetBranchesWithoutStream()
{ {
$expectedCommand = 'p4 -u user -p port changes //depot/...'; $expectedCommand = 'p4 -u user -p port changes '.ProcessExecutor::escape('//depot/...');
$expectedCallback = function ($command, &$output) { $expectedCallback = function ($command, &$output) {
$output = 'Change 5678 on 2014/03/19 by Clark.Stuth@Clark.Stuth_test_client \'test changelist\''; $output = 'Change 5678 on 2014/03/19 by Clark.Stuth@Clark.Stuth_test_client \'test changelist\'';


Expand Down Expand Up @@ -458,7 +459,7 @@ function ($command, &$output) {


public function testGetComposerInformationWithoutLabelWithoutStream() public function testGetComposerInformationWithoutLabelWithoutStream()
{ {
$expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot -p port print //depot/composer.json'; $expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot -p port print '.ProcessExecutor::escape('//depot/composer.json');
$this->processExecutor->expects($this->at(0)) $this->processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand All @@ -484,7 +485,7 @@ function ($command, &$output) {


public function testGetComposerInformationWithLabelWithoutStream() public function testGetComposerInformationWithLabelWithoutStream()
{ {
$expectedCommand = 'p4 -u user -p port files //depot/composer.json@0.0.1'; $expectedCommand = 'p4 -u user -p port files '.ProcessExecutor::escape('//depot/composer.json@0.0.1');
$this->processExecutor->expects($this->at(0)) $this->processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand All @@ -498,7 +499,7 @@ function ($command, &$output) {
) )
); );


$expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot -p port print //depot/composer.json@10001'; $expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot -p port print '.ProcessExecutor::escape('//depot/composer.json@10001');
$this->processExecutor->expects($this->at(1)) $this->processExecutor->expects($this->at(1))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand Down Expand Up @@ -527,7 +528,7 @@ public function testGetComposerInformationWithoutLabelWithStream()
{ {
$this->setPerforceToStream(); $this->setPerforceToStream();


$expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port print //depot/branch/composer.json'; $expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port print '.ProcessExecutor::escape('//depot/branch/composer.json');
$this->processExecutor->expects($this->at(0)) $this->processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand Down Expand Up @@ -555,7 +556,7 @@ function ($command, &$output) {
public function testGetComposerInformationWithLabelWithStream() public function testGetComposerInformationWithLabelWithStream()
{ {
$this->setPerforceToStream(); $this->setPerforceToStream();
$expectedCommand = 'p4 -u user -p port files //depot/branch/composer.json@0.0.1'; $expectedCommand = 'p4 -u user -p port files '.ProcessExecutor::escape('//depot/branch/composer.json@0.0.1');
$this->processExecutor->expects($this->at(0)) $this->processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand All @@ -569,7 +570,7 @@ function ($command, &$output) {
) )
); );


$expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port print //depot/branch/composer.json@10001'; $expectedCommand = 'p4 -u user -c composer_perforce_TEST_depot_branch -p port print '.ProcessExecutor::escape('//depot/branch/composer.json@10001');
$this->processExecutor->expects($this->at(1)) $this->processExecutor->expects($this->at(1))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand)) ->with($this->equalTo($expectedCommand))
Expand Down Expand Up @@ -621,7 +622,7 @@ public function testCheckServerExists()
{ {
$processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock(); $processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock();


$expectedCommand = 'p4 -p perforce.does.exist:port info -s'; $expectedCommand = 'p4 -p '.ProcessExecutor::escape('perforce.does.exist:port').' info -s';
$processExecutor->expects($this->at(0)) $processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand), $this->equalTo(null)) ->with($this->equalTo($expectedCommand), $this->equalTo(null))
Expand All @@ -642,7 +643,7 @@ public function testCheckServerClientError()
{ {
$processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock(); $processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock();


$expectedCommand = 'p4 -p perforce.does.exist:port info -s'; $expectedCommand = 'p4 -p '.ProcessExecutor::escape('perforce.does.exist:port').' info -s';
$processExecutor->expects($this->at(0)) $processExecutor->expects($this->at(0))
->method('execute') ->method('execute')
->with($this->equalTo($expectedCommand), $this->equalTo(null)) ->with($this->equalTo($expectedCommand), $this->equalTo(null))
Expand Down Expand Up @@ -712,7 +713,7 @@ public function testCleanupClientSpecShouldDeleteClient()
$this->perforce->setFilesystem($fs); $this->perforce->setFilesystem($fs);


$testClient = $this->perforce->getClient(); $testClient = $this->perforce->getClient();
$expectedCommand = 'p4 -u ' . self::TEST_P4USER . ' -p ' . self::TEST_PORT . ' client -d ' . $testClient; $expectedCommand = 'p4 -u ' . self::TEST_P4USER . ' -p ' . self::TEST_PORT . ' client -d ' . ProcessExecutor::escape($testClient);
$this->processExecutor->expects($this->once())->method('execute')->with($this->equalTo($expectedCommand)); $this->processExecutor->expects($this->once())->method('execute')->with($this->equalTo($expectedCommand));


$fs->expects($this->once())->method('remove')->with($this->perforce->getP4ClientSpec()); $fs->expects($this->once())->method('remove')->with($this->perforce->getP4ClientSpec());
Expand Down

0 comments on commit bf12529

Please sign in to comment.