From c65af3e3a1832515c2e8acaeb15a19e4b6728dac Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 18 Jul 2012 17:20:01 +0200 Subject: [PATCH] Add ValidatingArrayLoader and more validation for the validate command --- src/Composer/Command/ValidateCommand.php | 18 +- src/Composer/Package/Loader/ArrayLoader.php | 8 +- src/Composer/Package/Loader/JsonLoader.php | 15 +- .../Package/Loader/LoaderInterface.php | 29 ++ .../Package/Loader/RootPackageLoader.php | 2 +- .../Package/Loader/ValidatingArrayLoader.php | 279 ++++++++++++++++++ .../Test/Installer/InstallerInstallerTest.php | 3 +- .../Loader/ValidatingArrayLoaderTest.php | 222 ++++++++++++++ 8 files changed, 567 insertions(+), 9 deletions(-) create mode 100644 src/Composer/Package/Loader/LoaderInterface.php create mode 100644 src/Composer/Package/Loader/ValidatingArrayLoader.php create mode 100644 tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php diff --git a/src/Composer/Command/ValidateCommand.php b/src/Composer/Command/ValidateCommand.php index 2747e2d0a..8a09fffc4 100644 --- a/src/Composer/Command/ValidateCommand.php +++ b/src/Composer/Command/ValidateCommand.php @@ -17,6 +17,8 @@ use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Output\OutputInterface; use Composer\Json\JsonFile; use Composer\Json\JsonValidationException; +use Composer\Package\Loader\ValidatingArrayLoader; +use Composer\Package\Loader\ArrayLoader; use Composer\Util\RemoteFilesystem; use Composer\Util\SpdxLicenseIdentifier; @@ -109,7 +111,7 @@ EOT $warnings[] = 'No license specified, it is recommended to do so'; } - if (preg_match('{[A-Z]}', $manifest['name'])) { + if (!empty($manifest['name']) && preg_match('{[A-Z]}', $manifest['name'])) { $suggestName = preg_replace('{(?:([a-z])([A-Z])|([A-Z])([A-Z][a-z]))}', '\\1\\3-\\2\\4', $manifest['name']); $suggestName = strtolower($suggestName); @@ -120,6 +122,20 @@ EOT ); } + // TODO validate package repositories' packages using the same technique as below + try { + $loader = new ValidatingArrayLoader(new ArrayLoader(), false); + if (!isset($manifest['version'])) { + $manifest['version'] = '1.0.0'; + } + if (!isset($manifest['name'])) { + $manifest['version'] = 'dummy/dummy'; + } + $loader->load($manifest); + } catch (\Exception $e) { + $errors = array_merge($errors, explode("\n", $e->getMessage())); + } + // output errors/warnings if (!$errors && !$publishErrors && !$warnings) { $output->writeln('' . $file . ' is valid'); diff --git a/src/Composer/Package/Loader/ArrayLoader.php b/src/Composer/Package/Loader/ArrayLoader.php index c760eb388..10ba2b3ed 100644 --- a/src/Composer/Package/Loader/ArrayLoader.php +++ b/src/Composer/Package/Loader/ArrayLoader.php @@ -19,7 +19,7 @@ use Composer\Package\Version\VersionParser; * @author Konstantin Kudryashiv * @author Jordi Boggiano */ -class ArrayLoader +class ArrayLoader implements LoaderInterface { protected $versionParser; @@ -31,7 +31,7 @@ class ArrayLoader $this->versionParser = $parser; } - public function load($config) + public function load(array $config) { if (!isset($config['name'])) { throw new \UnexpectedValueException('Unknown package has no name defined ('.json_encode($config).').'); @@ -82,8 +82,8 @@ class ArrayLoader $package->setHomepage($config['homepage']); } - if (!empty($config['keywords'])) { - $package->setKeywords(is_array($config['keywords']) ? $config['keywords'] : array($config['keywords'])); + if (!empty($config['keywords']) && is_array($config['keywords'])) { + $package->setKeywords($config['keywords']); } if (!empty($config['license'])) { diff --git a/src/Composer/Package/Loader/JsonLoader.php b/src/Composer/Package/Loader/JsonLoader.php index fcecc0014..19e047c9a 100644 --- a/src/Composer/Package/Loader/JsonLoader.php +++ b/src/Composer/Package/Loader/JsonLoader.php @@ -17,8 +17,19 @@ use Composer\Json\JsonFile; /** * @author Konstantin Kudryashiv */ -class JsonLoader extends ArrayLoader +class JsonLoader { + private $loader; + + public function __construct(LoaderInterface $loader) + { + $this->loader = $loader; + } + + /** + * @param string|JsonFile $json A filename, json string or JsonFile instance to load the package from + * @return \Composer\Package\PackageInterface + */ public function load($json) { if ($json instanceof JsonFile) { @@ -29,6 +40,6 @@ class JsonLoader extends ArrayLoader $config = JsonFile::parseJson($json); } - return parent::load($config); + return $this->loader->load($config); } } diff --git a/src/Composer/Package/Loader/LoaderInterface.php b/src/Composer/Package/Loader/LoaderInterface.php new file mode 100644 index 000000000..f0645805f --- /dev/null +++ b/src/Composer/Package/Loader/LoaderInterface.php @@ -0,0 +1,29 @@ + + * Jordi Boggiano + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Composer\Package\Loader; + +/** + * Defines a loader that takes an array to create package instances + * + * @author Jordi Boggiano + */ +interface LoaderInterface +{ + /** + * Converts a package from an array to a real instance + * + * @param array $package Package config + * @return \Composer\Package\PackageInterface + */ + public function load(array $package); +} diff --git a/src/Composer/Package/Loader/RootPackageLoader.php b/src/Composer/Package/Loader/RootPackageLoader.php index 51c676112..28b12a6b8 100644 --- a/src/Composer/Package/Loader/RootPackageLoader.php +++ b/src/Composer/Package/Loader/RootPackageLoader.php @@ -40,7 +40,7 @@ class RootPackageLoader extends ArrayLoader parent::__construct($parser); } - public function load($config) + public function load(array $config) { if (!isset($config['name'])) { $config['name'] = '__root__'; diff --git a/src/Composer/Package/Loader/ValidatingArrayLoader.php b/src/Composer/Package/Loader/ValidatingArrayLoader.php new file mode 100644 index 000000000..0deb5c364 --- /dev/null +++ b/src/Composer/Package/Loader/ValidatingArrayLoader.php @@ -0,0 +1,279 @@ + + * Jordi Boggiano + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Composer\Package\Loader; + +use Composer\Package; +use Composer\Package\Version\VersionParser; + +/** + * @author Jordi Boggiano + */ +class ValidatingArrayLoader implements LoaderInterface +{ + private $loader; + private $versionParser; + private $ignoreErrors; + private $errors = array(); + + public function __construct(LoaderInterface $loader, $ignoreErrors = true, VersionParser $parser = null) + { + $this->loader = $loader; + $this->ignoreErrors = $ignoreErrors; + if (!$parser) { + $parser = new VersionParser(); + } + $this->versionParser = $parser; + } + + public function load(array $config) + { + $this->config = $config; + + $this->validateRegex('name', '[A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*', true); + + if (!empty($config['version'])) { + try { + $this->versionParser->normalize($config['version']); + } catch (\Exception $e) { + unset($this->config['version']); + $this->errors[] = 'version : invalid value ('.$config['version'].'): '.$e->getMessage(); + } + } + + $this->validateRegex('type', '[a-z0-9-]+'); + $this->validateString('target-dir'); + $this->validateArray('extra'); + $this->validateFlatArray('bin'); + $this->validateArray('scripts'); // TODO validate event names & listener syntax + $this->validateString('description'); + $this->validateUrl('homepage'); + $this->validateFlatArray('keywords', '[A-Za-z0-9 -]+'); + + if (isset($config['license'])) { + if (is_string($config['license'])) { + $this->validateRegex('license', '[A-Za-z0-9+. ()-]+'); + } else { + $this->validateFlatArray('license', '[A-Za-z0-9+. ()-]+'); + } + } + + $this->validateString('time'); + if (!empty($this->config['time'])) { + try { + $date = new \DateTime($config['time']); + } catch (\Exception $e) { + $this->errors[] = 'time : invalid value ('.$this->config['time'].'): '.$e->getMessage(); + unset($this->config['time']); + } + } + + $this->validateArray('authors'); + if (!empty($this->config['authors'])) { + foreach ($this->config['authors'] as $key => $author) { + if (isset($author['homepage']) && !$this->filterUrl($author['homepage'])) { + $this->errors[] = 'authors.'.$key.'.homepage : invalid value, must be a valid http/https URL'; + unset($this->config['authors'][$key]['homepage']); + } + if (isset($author['email']) && !filter_var($author['email'], FILTER_VALIDATE_EMAIL)) { + $this->errors[] = 'authors.'.$key.'.email : invalid value, must be a valid email address'; + unset($this->config['authors'][$key]['email']); + } + if (isset($author['name']) && !is_string($author['name'])) { + $this->errors[] = 'authors.'.$key.'.name : invalid value, must be a string'; + unset($this->config['authors'][$key]['name']); + } + if (isset($author['role']) && !is_string($author['role'])) { + $this->errors[] = 'authors.'.$key.'.role : invalid value, must be a string'; + unset($this->config['authors'][$key]['role']); + } + } + if (empty($this->config['authors'])) { + unset($this->config['authors']); + } + } + + $this->validateArray('support'); + if (!empty($this->config['support'])) { + if (isset($this->config['support']['email']) && !filter_var($this->config['support']['email'], FILTER_VALIDATE_EMAIL)) { + $this->errors[] = 'support.email : invalid value, must be a valid email address'; + unset($this->config['support']['email']); + } + + if (isset($this->config['support']['irc']) + && (!filter_var($this->config['support']['irc'], FILTER_VALIDATE_URL) || !preg_match('{^irc://}iu', $this->config['support']['irc'])) + ) { + $this->errors[] = 'support.irc : invalid value, must be '; + unset($this->config['support']['irc']); + } + + foreach (array('issues', 'forum', 'wiki', 'source') as $key) { + if (isset($this->config['support'][$key]) && !$this->filterUrl($this->config['support'][$key])) { + $this->errors[] = 'support.'.$key.' : invalid value, must be a valid http/https URL'; + unset($this->config['support'][$key]); + } + } + if (empty($this->config['support'])) { + unset($this->config['support']); + } + } + + // TODO validate require/require-dev/replace/provide + // TODO validate suggest + // TODO validate autoload + // TODO validate minimum-stability + + // TODO validate dist + // TODO validate source + + // TODO validate repositories + + $this->validateFlatArray('include-path'); + + // branch alias validation + if (isset($this->config['extra']['branch-alias'])) { + if (!is_array($this->config['extra']['branch-alias'])) { + $this->errors[] = 'extra.branch-alias : must be an array of versions => aliases'; + } else { + foreach ($this->config['extra']['branch-alias'] as $sourceBranch => $targetBranch) { + // ensure it is an alias to a -dev package + if ('-dev' !== substr($targetBranch, -4)) { + $this->errors[] = 'extra.branch-alias.'.$sourceBranch.' : the target branch ('.$targetBranch.') must end in -dev'; + unset($this->config['extra']['branch-alias'][$sourceBranch]); + + continue; + } + + // normalize without -dev and ensure it's a numeric branch that is parseable + $validatedTargetBranch = $this->versionParser->normalizeBranch(substr($targetBranch, 0, -4)); + if ('-dev' !== substr($validatedTargetBranch, -4)) { + $this->errors[] = 'extra.branch-alias.'.$sourceBranch.' : the target branch ('.$targetBranch.') must be a parseable number like 2.0-dev'; + unset($this->config['extra']['branch-alias'][$sourceBranch]); + } + } + } + } + + if ($this->errors && !$this->ignoreErrors) { + throw new \Exception(implode("\n", $this->errors)); + } + + $package = $this->loader->load($this->config); + $this->errors = array(); + unset($this->config); + + return $package; + } + + private function validateRegex($property, $regex, $mandatory = false) + { + if (!$this->validateString($property, $mandatory)) { + return false; + } + + if (!preg_match('{^'.$regex.'$}u', $this->config[$property])) { + $this->errors[] = $property.' : invalid value, must match '.$regex; + unset($this->config[$property]); + + return false; + } + + return true; + } + + private function validateString($property, $mandatory = false) + { + if (isset($this->config[$property]) && !is_string($this->config[$property])) { + $this->errors[] = $property.' : should be a string, '.gettype($this->config[$property]).' given'; + unset($this->config[$property]); + + return false; + } + + if (!isset($this->config[$property]) || trim($this->config[$property]) === '') { + if ($mandatory) { + $this->errors[] = $property.' : must be present'; + } + unset($this->config[$property]); + + return false; + } + + return true; + } + + private function validateArray($property, $mandatory = false) + { + if (isset($this->config[$property]) && !is_array($this->config[$property])) { + $this->errors[] = $property.' : should be an array, '.gettype($this->config[$property]).' given'; + unset($this->config[$property]); + + return false; + } + + if (!isset($this->config[$property]) || !count($this->config[$property])) { + if ($mandatory) { + $this->errors[] = $property.' : must be present and contain at least one element'; + } + unset($this->config[$property]); + + return false; + } + + return true; + } + + private function validateFlatArray($property, $regex = null, $mandatory = false) + { + if (!$this->validateArray($property, $mandatory)) { + return false; + } + + $pass = true; + foreach ($this->config[$property] as $key => $value) { + if (!is_string($value) && !is_numeric($value)) { + $this->errors[] = $property.'.'.$key.' : must be a string or int, '.gettype($value).' given'; + unset($this->config[$property][$key]); + $pass = false; + + continue; + } + + if ($regex && !preg_match('{^'.$regex.'$}u', $value)) { + $this->errors[] = $property.'.'.$key.' : invalid value, must match '.$regex; + unset($this->config[$property][$key]); + $pass = false; + } + } + + return $pass; + } + + private function validateUrl($property, $mandatory = false) + { + if (!$this->validateString($property, $mandatory)) { + return false; + } + + if (!$this->filterUrl($this->config[$property])) { + $this->errors[] = $property.' : invalid value, must be a valid http/https URL'; + unset($this->config[$property]); + + return false; + } + } + + private function filterUrl($value) + { + return filter_var($value, FILTER_VALIDATE_URL) && preg_match('{^https?://}iu', $value); + } +} diff --git a/tests/Composer/Test/Installer/InstallerInstallerTest.php b/tests/Composer/Test/Installer/InstallerInstallerTest.php index db84105b2..bfc641029 100644 --- a/tests/Composer/Test/Installer/InstallerInstallerTest.php +++ b/tests/Composer/Test/Installer/InstallerInstallerTest.php @@ -16,6 +16,7 @@ use Composer\Composer; use Composer\Config; use Composer\Installer\InstallerInstaller; use Composer\Package\Loader\JsonLoader; +use Composer\Package\Loader\ArrayLoader; use Composer\Package\PackageInterface; class InstallerInstallerTest extends \PHPUnit_Framework_TestCase @@ -28,7 +29,7 @@ class InstallerInstallerTest extends \PHPUnit_Framework_TestCase protected function setUp() { - $loader = new JsonLoader(); + $loader = new JsonLoader(new ArrayLoader()); $this->packages = array(); for ($i = 1; $i <= 4; $i++) { $this->packages[] = $loader->load(__DIR__.'/Fixtures/installer-v'.$i.'/composer.json'); diff --git a/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php b/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php new file mode 100644 index 000000000..e9a00e34a --- /dev/null +++ b/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php @@ -0,0 +1,222 @@ + + * Jordi Boggiano + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Composer\Test\Package\Loader; + +use Composer\Package; +use Composer\Package\Loader\ValidatingArrayLoader; + +class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase +{ + /** + * @dataProvider successProvider + */ + public function testLoadSuccess($config) + { + $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); + $internalLoader + ->expects($this->once()) + ->method('load') + ->with($config); + + $loader = new ValidatingArrayLoader($internalLoader, false); + $loader->load($config); + } + + public function successProvider() + { + return array( + array( // minimal + array( + 'name' => 'foo/bar', + ), + ), + array( // complete + array( + 'name' => 'foo/bar', + 'description' => 'Foo bar', + 'version' => '1.0.0', + 'type' => 'library', + 'keywords' => array('a', 'b'), + 'homepage' => 'https://foo.com', + 'time' => '2010-10-10T10:10:10+00:00', + 'license' => 'MIT', + 'authors' => array( + array( + 'name' => 'Alice', + 'email' => 'alice@example.org', + 'role' => 'Lead', + 'homepage' => 'http://example.org', + ), + array( + 'name' => 'Bob', + 'homepage' => 'http://example.com', + ), + ), + 'support' => array( + 'email' => 'mail@example.org', + 'issues' => 'http://example.org/', + 'forum' => 'http://example.org/', + 'wiki' => 'http://example.org/', + 'source' => 'http://example.org/', + 'irc' => 'irc://example.org/example', + ), + 'require' => array( + 'a/b' => '1.*', + 'example' => '>2.0-dev,<2.4-dev', + ), + 'require-dev' => array( + 'a/b' => '1.*', + 'example' => '>2.0-dev,<2.4-dev', + ), + 'conflict' => array( + 'a/b' => '1.*', + 'example' => '>2.0-dev,<2.4-dev', + ), + 'replace' => array( + 'a/b' => '1.*', + 'example' => '>2.0-dev,<2.4-dev', + ), + 'provide' => array( + 'a/b' => '1.*', + 'example' => '>2.0-dev,<2.4-dev', + ), + 'suggest' => array( + 'foo/bar' => 'Foo bar is very useful', + ), + 'autoload' => array( + 'psr-0' => array( + 'Foo\\Bar' => 'src/', + '' => 'fallback/libs/', + ), + 'classmap' => array( + 'dir/', + 'dir2/file.php', + ), + 'files' => array( + 'functions.php', + ), + ), + 'include-path' => array( + 'lib/', + ), + 'target-dir' => 'Foo/Bar', + 'minimum-stability' => 'dev', + 'repositories' => array( + array( + 'type' => 'composer', + 'url' => 'http://packagist.org/', + ) + ), + 'config' => array( + 'bin-dir' => 'bin', + 'vendor-dir' => 'vendor', + 'process-timeout' => 10000, + ), + 'scripts' => array( + 'post-update-cmd' => 'Foo\\Bar\\Baz::doSomething', + 'post-install-cmd' => array( + 'Foo\\Bar\\Baz::doSomething', + ), + ), + 'extra' => array( + 'random' => array('stuff' => array('deeply' => 'nested')), + ), + 'bin' => array( + 'bin/foo', + 'bin/bar', + ), + ), + ), + array( // test as array + array( + 'name' => 'foo/bar', + 'license' => array('MIT', 'WTFPL'), + ), + ), + ); + } + + /** + * @dataProvider failureProvider + */ + public function testLoadFailureThrowsException($config, $expectedErrors) + { + $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); + $loader = new ValidatingArrayLoader($internalLoader, false); + try { + $loader->load($config); + $this->fail('Expected exception to be thrown'); + } catch (\Exception $e) { + $errors = explode("\n", $e->getMessage()); + sort($expectedErrors); + sort($errors); + $this->assertEquals($expectedErrors, $errors); + } + } + + /** + * @dataProvider failureProvider + */ + public function testLoadSkipsInvalidDataWhenIgnoringErrors($config) + { + $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); + $internalLoader + ->expects($this->once()) + ->method('load') + ->with(array('name' => 'a/b')); + + $loader = new ValidatingArrayLoader($internalLoader, true); + $config['name'] = 'a/b'; + $loader->load($config); + } + + public function failureProvider() + { + return array( + array( + array( + 'name' => 'foo', + ), + array( + 'name : invalid value, must match [A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*' + ) + ), + array( + array( + 'name' => 'foo/bar', + 'homepage' => 'foo:bar', + ), + array( + 'homepage : invalid value, must be a valid http/https URL' + ) + ), + array( + array( + 'name' => 'foo/bar', + 'support' => array( + 'source' => 'foo:bar', + 'forum' => 'foo:bar', + 'issues' => 'foo:bar', + 'wiki' => 'foo:bar', + ), + ), + array( + 'support.source : invalid value, must be a valid http/https URL', + 'support.forum : invalid value, must be a valid http/https URL', + 'support.issues : invalid value, must be a valid http/https URL', + 'support.wiki : invalid value, must be a valid http/https URL', + ) + ), + ); + } +}