From 17a26dfcc1f4d02b54e6cbb500f41bbe25609f1e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 7 Feb 2018 16:03:21 +0100 Subject: [PATCH] Validate the info.xml against the appstore schema file Signed-off-by: Joas Schilling --- core/Command/App/CheckCode.php | 54 +- core/register_command.php | 3 +- lib/private/App/AppManager.php | 5 + lib/private/App/CodeChecker/InfoChecker.php | 149 ++-- resources/app-info-shipped.xsd | 671 ++++++++++++++++++ resources/app-info.xsd | 652 +++++++++++++++++ .../appinfo/info.xml | 9 - tests/apps/testapp-infoxml/appinfo/info.xml | 12 - .../testapp-name-missing/appinfo/info.xml | 11 - tests/apps/testapp-version/appinfo/info.xml | 11 - tests/apps/testapp-version/appinfo/version | 1 - .../appinfo/info.xml | 13 + tests/apps/testapp_infoxml/appinfo/info.xml | 16 + .../testapp_name_missing/appinfo/info.xml | 15 + tests/apps/testapp_version/appinfo/info.xml | 15 + tests/lib/App/CodeChecker/InfoCheckerTest.php | 19 +- 16 files changed, 1461 insertions(+), 195 deletions(-) create mode 100644 resources/app-info-shipped.xsd create mode 100644 resources/app-info.xsd delete mode 100644 tests/apps/testapp-dependency-missing/appinfo/info.xml delete mode 100644 tests/apps/testapp-infoxml/appinfo/info.xml delete mode 100644 tests/apps/testapp-name-missing/appinfo/info.xml delete mode 100644 tests/apps/testapp-version/appinfo/info.xml delete mode 100644 tests/apps/testapp-version/appinfo/version create mode 100644 tests/apps/testapp_dependency_missing/appinfo/info.xml create mode 100644 tests/apps/testapp_infoxml/appinfo/info.xml create mode 100644 tests/apps/testapp_name_missing/appinfo/info.xml create mode 100644 tests/apps/testapp_version/appinfo/info.xml diff --git a/core/Command/App/CheckCode.php b/core/Command/App/CheckCode.php index 82a137e58e1..a129bcf1e10 100644 --- a/core/Command/App/CheckCode.php +++ b/core/Command/App/CheckCode.php @@ -44,20 +44,12 @@ use OC\App\CodeChecker\PrivateCheck; class CheckCode extends Command implements CompletionAwareInterface { - /** @var InfoParser */ - private $infoParser; - protected $checkers = [ 'private' => PrivateCheck::class, 'deprecation' => DeprecationCheck::class, 'strong-comparison' => StrongComparisonCheck::class, ]; - public function __construct(InfoParser $infoParser) { - parent::__construct(); - $this->infoParser = $infoParser; - } - protected function configure() { $this ->setName('app:check-code') @@ -134,50 +126,12 @@ class CheckCode extends Command implements CompletionAwareInterface { } if(!$input->getOption('skip-validate-info')) { - $infoChecker = new InfoChecker($this->infoParser); - - $infoChecker->listen('InfoChecker', 'mandatoryFieldMissing', function($key) use ($output) { - $output->writeln("Mandatory field missing: $key"); - }); - - $infoChecker->listen('InfoChecker', 'deprecatedFieldFound', function($key, $value) use ($output) { - if($value === [] || is_null($value) || $value === '') { - $output->writeln("Deprecated field available: $key"); - } else { - $output->writeln("Deprecated field available: $key => $value"); - } - }); - - $infoChecker->listen('InfoChecker', 'missingRequirement', function($minMax) use ($output) { - $output->writeln("Nextcloud $minMax version requirement missing"); - }); - - $infoChecker->listen('InfoChecker', 'differentVersions', function($versionFile, $infoXML) use ($output) { - $output->writeln("Different versions provided (appinfo/version: $versionFile - appinfo/info.xml: $infoXML)"); + // Can not inject because of circular dependency + $infoChecker = new InfoChecker(\OC::$server->getAppManager()); + $infoChecker->listen('InfoChecker', 'parseError', function($error) use ($output) { + $output->writeln("Invalid appinfo.xml file found: $error"); }); - $infoChecker->listen('InfoChecker', 'sameVersions', function($path) use ($output) { - $output->writeln("Version file isn't needed anymore and can be safely removed ($path)"); - }); - - $infoChecker->listen('InfoChecker', 'migrateVersion', function($version) use ($output) { - $output->writeln("Migrate the app version to appinfo/info.xml (add $version to appinfo/info.xml and remove appinfo/version)"); - }); - - if(OutputInterface::VERBOSITY_VERBOSE <= $output->getVerbosity()) { - $infoChecker->listen('InfoChecker', 'mandatoryFieldFound', function($key, $value) use ($output) { - $output->writeln("Mandatory field available: $key => $value"); - }); - - $infoChecker->listen('InfoChecker', 'optionalFieldFound', function($key, $value) use ($output) { - $output->writeln("Optional field available: $key => $value"); - }); - - $infoChecker->listen('InfoChecker', 'unusedFieldFound', function($key, $value) use ($output) { - $output->writeln("Unused field available: $key => $value"); - }); - } - $infoErrors = $infoChecker->analyse($appId); $errors = array_merge($errors, $infoErrors); diff --git a/core/register_command.php b/core/register_command.php index 372d775dc14..578b4f799b6 100644 --- a/core/register_command.php +++ b/core/register_command.php @@ -40,8 +40,7 @@ $application->add(new \Stecman\Component\Symfony\Console\BashCompletion\CompletionCommand()); $application->add(new OC\Core\Command\Status); $application->add(new OC\Core\Command\Check(\OC::$server->getSystemConfig())); -$infoParser = new \OC\App\InfoParser(); -$application->add(new OC\Core\Command\App\CheckCode($infoParser)); +$application->add(new OC\Core\Command\App\CheckCode()); $application->add(new OC\Core\Command\L10n\CreateJs()); $application->add(new \OC\Core\Command\Integrity\SignApp( \OC::$server->getIntegrityCodeChecker(), diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index 0e921ba1b7f..81037b42613 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -411,6 +411,7 @@ class AppManager implements IAppManager { /** * @inheritdoc + * In case you change this method, also change \OC\App\CodeChecker\InfoChecker::isShipped() */ public function isShipped($appId) { $this->loadShippedJson(); @@ -422,6 +423,10 @@ class AppManager implements IAppManager { return in_array($appId, $alwaysEnabled, true); } + /** + * In case you change this method, also change \OC\App\CodeChecker\InfoChecker::loadShippedJson() + * @throws \Exception + */ private function loadShippedJson() { if ($this->shippedApps === null) { $shippedJson = \OC::$SERVERROOT . '/core/shipped.json'; diff --git a/lib/private/App/CodeChecker/InfoChecker.php b/lib/private/App/CodeChecker/InfoChecker.php index e8791bd7037..f5de6d376d8 100644 --- a/lib/private/App/CodeChecker/InfoChecker.php +++ b/lib/private/App/CodeChecker/InfoChecker.php @@ -23,121 +23,88 @@ namespace OC\App\CodeChecker; -use OC\App\InfoParser; use OC\Hooks\BasicEmitter; +use OCP\App\AppPathNotFoundException; +use OCP\App\IAppManager; class InfoChecker extends BasicEmitter { - /** @var InfoParser */ - private $infoParser; + /** @var string[] */ + private $shippedApps; - private $mandatoryFields = [ - 'author', - 'description', - 'dependencies', - 'id', - 'licence', - 'name', - 'version', - ]; - private $optionalFields = [ - 'bugs', - 'category', - 'default_enable', - 'documentation', - 'namespace', - 'ocsid', - 'public', - 'remote', - 'repository', - 'types', - 'website', - ]; - private $deprecatedFields = [ - 'info', - 'require', - 'requiremax', - 'requiremin', - 'shipped', - 'standalone', - ]; - - public function __construct(InfoParser $infoParser) { - $this->infoParser = $infoParser; - } + /** @var string[] */ + private $alwaysEnabled; /** * @param string $appId * @return array + * @throws \RuntimeException */ - public function analyse($appId) { + public function analyse($appId): array { $appPath = \OC_App::getAppPath($appId); if ($appPath === false) { throw new \RuntimeException("No app with given id <$appId> known."); } - $errors = []; - - $info = $this->infoParser->parse($appPath . '/appinfo/info.xml'); - - if (!isset($info['dependencies']['nextcloud']['@attributes']['min-version'])) { - $errors[] = [ - 'type' => 'missingRequirement', - 'field' => 'min', - ]; - $this->emit('InfoChecker', 'missingRequirement', ['min']); - } - - if (!isset($info['dependencies']['nextcloud']['@attributes']['max-version'])) { - $errors[] = [ - 'type' => 'missingRequirement', - 'field' => 'max', - ]; - $this->emit('InfoChecker', 'missingRequirement', ['max']); - } - - foreach ($info as $key => $value) { - if(is_array($value)) { - $value = json_encode($value); - } - if (in_array($key, $this->mandatoryFields)) { - $this->emit('InfoChecker', 'mandatoryFieldFound', [$key, $value]); - continue; - } + $xml = new \DOMDocument(); + $xml->load($appPath . '/appinfo/info.xml'); - if (in_array($key, $this->optionalFields)) { - $this->emit('InfoChecker', 'optionalFieldFound', [$key, $value]); - continue; + $schema = \OC::$SERVERROOT . '/resources/app-info.xsd'; + try { + if ($this->isShipped($appId)) { + // Shipped apps are allowed to have the public and default_enabled tags + $schema = \OC::$SERVERROOT . '/resources/app-info-shipped.xsd'; } - - if (in_array($key, $this->deprecatedFields)) { - // skip empty arrays - empty arrays for remote and public are always added - if($value === '[]' && in_array($key, ['public', 'remote', 'info'])) { - continue; - } - $this->emit('InfoChecker', 'deprecatedFieldFound', [$key, $value]); - continue; - } - - $this->emit('InfoChecker', 'unusedFieldFound', [$key, $value]); + } catch (\Exception $e) { + // Assume it is not shipped } - foreach ($this->mandatoryFields as $key) { - if(!isset($info[$key])) { - $this->emit('InfoChecker', 'mandatoryFieldMissing', [$key]); + $errors = []; + if (!$xml->schemaValidate($schema)) { + foreach (libxml_get_errors() as $error) { $errors[] = [ - 'type' => 'mandatoryFieldMissing', - 'field' => $key, + 'type' => 'parseError', + 'field' => $error->message, ]; + $this->emit('InfoChecker', 'parseError', [$error->message]); } } - $versionFile = $appPath . '/appinfo/version'; - if (is_file($versionFile)) { - $version = trim(file_get_contents($versionFile)); - $this->emit('InfoChecker', 'migrateVersion', [$version]); - } - return $errors; } + + /** + * This is a copy of \OC\App\AppManager::isShipped(), keep both in sync. + * This method is copied, so the code checker works even when Nextcloud is + * not installed yet. The AppManager requires a database connection, which + * fails in that case. + * + * @param string $appId + * @return bool + * @throws \Exception + */ + protected function isShipped(string $appId): bool { + $this->loadShippedJson(); + return \in_array($appId, $this->shippedApps, true); + } + + /** + * This is a copy of \OC\App\AppManager::loadShippedJson(), keep both in sync + * This method is copied, so the code checker works even when Nextcloud is + * not installed yet. The AppManager requires a database connection, which + * fails in that case. + * + * @throws \Exception + */ + protected function loadShippedJson() { + if ($this->shippedApps === null) { + $shippedJson = \OC::$SERVERROOT . '/core/shipped.json'; + if (!file_exists($shippedJson)) { + throw new \Exception("File not found: $shippedJson"); + } + $content = json_decode(file_get_contents($shippedJson), true); + $this->shippedApps = $content['shippedApps']; + $this->alwaysEnabled = $content['alwaysEnabled']; + } + } } diff --git a/resources/app-info-shipped.xsd b/resources/app-info-shipped.xsd new file mode 100644 index 00000000000..97221d1fb9c --- /dev/null +++ b/resources/app-info-shipped.xsd @@ -0,0 +1,671 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/resources/app-info.xsd b/resources/app-info.xsd new file mode 100644 index 00000000000..4460b0f63b9 --- /dev/null +++ b/resources/app-info.xsd @@ -0,0 +1,652 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/apps/testapp-dependency-missing/appinfo/info.xml b/tests/apps/testapp-dependency-missing/appinfo/info.xml deleted file mode 100644 index c765400a76f..00000000000 --- a/tests/apps/testapp-dependency-missing/appinfo/info.xml +++ /dev/null @@ -1,9 +0,0 @@ - - - testapp-infoxml-version - 1.2.3 - Jane - A b c - Abc - Test app - diff --git a/tests/apps/testapp-infoxml/appinfo/info.xml b/tests/apps/testapp-infoxml/appinfo/info.xml deleted file mode 100644 index d4df1c3cd3f..00000000000 --- a/tests/apps/testapp-infoxml/appinfo/info.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - testapp-infoxml - 1.2.3 - Jane - A b c - Abc - Test app - - - - diff --git a/tests/apps/testapp-name-missing/appinfo/info.xml b/tests/apps/testapp-name-missing/appinfo/info.xml deleted file mode 100644 index 591c4361899..00000000000 --- a/tests/apps/testapp-name-missing/appinfo/info.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - testapp-version - 1.1.1 - Jane - A b c - Abc - - - - diff --git a/tests/apps/testapp-version/appinfo/info.xml b/tests/apps/testapp-version/appinfo/info.xml deleted file mode 100644 index 28e2475800f..00000000000 --- a/tests/apps/testapp-version/appinfo/info.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - testapp-version - Jane - A b c - Abc - Test app - - - - diff --git a/tests/apps/testapp-version/appinfo/version b/tests/apps/testapp-version/appinfo/version deleted file mode 100644 index 0495c4a88ca..00000000000 --- a/tests/apps/testapp-version/appinfo/version +++ /dev/null @@ -1 +0,0 @@ -1.2.3 diff --git a/tests/apps/testapp_dependency_missing/appinfo/info.xml b/tests/apps/testapp_dependency_missing/appinfo/info.xml new file mode 100644 index 00000000000..b0ca188aefc --- /dev/null +++ b/tests/apps/testapp_dependency_missing/appinfo/info.xml @@ -0,0 +1,13 @@ + + + testapp_infoxml + Test app + A b c + A b c + 1.2.3 + agpl + Jane + games + https://example.org + diff --git a/tests/apps/testapp_infoxml/appinfo/info.xml b/tests/apps/testapp_infoxml/appinfo/info.xml new file mode 100644 index 00000000000..b7575687fe5 --- /dev/null +++ b/tests/apps/testapp_infoxml/appinfo/info.xml @@ -0,0 +1,16 @@ + + + testapp_infoxml + Test app + A b c + A b c + 1.2.3 + agpl + Jane + games + https://example.org + + + + diff --git a/tests/apps/testapp_name_missing/appinfo/info.xml b/tests/apps/testapp_name_missing/appinfo/info.xml new file mode 100644 index 00000000000..872c8752c75 --- /dev/null +++ b/tests/apps/testapp_name_missing/appinfo/info.xml @@ -0,0 +1,15 @@ + + + testapp_name_missing + A b c + A b c + 1.2.3 + agpl + Jane + games + https://example.org + + + + diff --git a/tests/apps/testapp_version/appinfo/info.xml b/tests/apps/testapp_version/appinfo/info.xml new file mode 100644 index 00000000000..3a48fccd45f --- /dev/null +++ b/tests/apps/testapp_version/appinfo/info.xml @@ -0,0 +1,15 @@ + + + testapp_version + Test app + A b c + A b c + agpl + Jane + games + https://example.org + + + + diff --git a/tests/lib/App/CodeChecker/InfoCheckerTest.php b/tests/lib/App/CodeChecker/InfoCheckerTest.php index 760d9880739..9f354a4611c 100644 --- a/tests/lib/App/CodeChecker/InfoCheckerTest.php +++ b/tests/lib/App/CodeChecker/InfoCheckerTest.php @@ -44,19 +44,21 @@ class InfoCheckerTest extends TestCase { protected function setUp() { parent::setUp(); - $this->infoChecker = new InfoChecker(new InfoParser()); + $this->infoChecker = new InfoChecker(\OC::$server->getAppManager()); } public function appInfoData() { return [ - ['testapp-infoxml', []], - ['testapp-version', [['type' => 'mandatoryFieldMissing', 'field' => 'version']]], - ['testapp-dependency-missing', [ - ['type' => 'missingRequirement', 'field' => 'min'], - ['type' => 'missingRequirement', 'field' => 'max'], - ['type' => 'mandatoryFieldMissing', 'field' => 'dependencies'], + ['testapp_infoxml', []], + ['testapp_version', [ + ['type' => 'parseError', 'field' => 'Element \'licence\': This element is not expected. Expected is one of ( description, version ).' . "\n"], + ]], + ['testapp_dependency_missing', [ + ['type' => 'parseError', 'field' => 'Element \'info\': Missing child element(s). Expected is one of ( repository, screenshot, dependencies ).' . "\n"], + ]], + ['testapp_name_missing', [ + ['type' => 'parseError', 'field' => 'Element \'summary\': This element is not expected. Expected is ( name ).' . "\n"], ]], - ['testapp-name-missing', [['type' => 'mandatoryFieldMissing', 'field' => 'name']]], ]; } @@ -68,6 +70,7 @@ class InfoCheckerTest extends TestCase { */ public function testApps($appId, $expectedErrors) { $errors = $this->infoChecker->analyse($appId); + libxml_clear_errors(); $this->assertEquals($expectedErrors, $errors); }