Merge pull request #13750 from owncloud/enhanced-code-checker

Implement php code checker to detect usage of not allowed private ...

3rdparty

@@ -1 +1 @@
-Subproject commit a32d3924bd0012a5410fff4666131cbdfdec2001
+Subproject commit 5142d69c5c467c651a7ef72ea1f09dcfb7ba25b5

core/command/app/checkcode.php

@@ -0,0 +1,53 @@
+<?php
+/**
+ * Copyright (c) 2015 Thomas Müller <deepdiver@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace OC\Core\Command\App;
+
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputArgument;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+
+class CheckCode extends Command {
+	protected function configure() {
+		$this
+			->setName('app:check-code')
+			->setDescription('check code to be compliant')
+			->addArgument(
+				'app-id',
+				InputArgument::REQUIRED,
+				'enable the specified app'
+			);
+	}
+
+	protected function execute(InputInterface $input, OutputInterface $output) {
+		$appId = $input->getArgument('app-id');
+		$codeChecker = new \OC\App\CodeChecker();
+		$codeChecker->listen('CodeChecker', 'analyseFileBegin', function($params) use ($output) {
+			$output->writeln("<info>Analysing {$params}</info>");
+		});
+		$codeChecker->listen('CodeChecker', 'analyseFileFinished', function($params) use ($output) {
+			$count = count($params);
+			$output->writeln(" {$count} errors");
+			usort($params, function($a, $b) {
+				return $a['line'] >$b['line'];
+			});
+
+			foreach($params as $p) {
+				$line = sprintf("%' 4d", $p['line']);
+				$output->writeln("    <error>line $line: {$p['disallowedToken']} - {$p['reason']}</error>");
+			}
+		});
+		$errors = $codeChecker->analyse($appId);
+		if (empty($errors)) {
+			$output->writeln('<info>App is compliant - awesome job!</info>');
+		} else {
+			$output->writeln('<error>App is not compliant</error>');
+		}
+	}
+}

core/register_command.php

@@ -15,6 +15,7 @@ $application->add(new OC\Core\Command\Db\ConvertType(\OC::$server->getConfig(),
 $application->add(new OC\Core\Command\Upgrade(\OC::$server->getConfig()));
 $application->add(new OC\Core\Command\Maintenance\SingleUser());
 $application->add(new OC\Core\Command\Maintenance\Mode(\OC::$server->getConfig()));
+$application->add(new OC\Core\Command\App\CheckCode());
 $application->add(new OC\Core\Command\App\Disable());
 $application->add(new OC\Core\Command\App\Enable());
 $application->add(new OC\Core\Command\App\ListApps());

lib/private/app/codechecker.php

@@ -0,0 +1,130 @@
+<?php
+/**
+ * Copyright (c) 2015 Thomas Müller <deepdiver@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace OC\App;
+
+use OC\Hooks\BasicEmitter;
+use PhpParser\Lexer;
+use PhpParser\Node;
+use PhpParser\Node\Name;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitorAbstract;
+use PhpParser\Parser;
+use RecursiveCallbackFilterIterator;
+use RecursiveDirectoryIterator;
+use RecursiveIteratorIterator;
+use RegexIterator;
+use SplFileInfo;
+
+class CodeChecker extends BasicEmitter {
+
+	const CLASS_EXTENDS_NOT_ALLOWED = 1000;
+	const CLASS_IMPLEMENTS_NOT_ALLOWED = 1001;
+	const STATIC_CALL_NOT_ALLOWED = 1002;
+	const CLASS_CONST_FETCH_NOT_ALLOWED = 1003;
+	const CLASS_NEW_FETCH_NOT_ALLOWED =  1004;
+
+	/** @var Parser */
+	private $parser;
+
+	/** @var string[] */
+	private $blackListedClassNames;
+
+	public function __construct() {
+		$this->parser = new Parser(new Lexer);
+		$this->blackListedClassNames = [
+			// classes replaced by the public api
+			'OC_API',
+			'OC_App',
+			'OC_AppConfig',
+			'OC_Avatar',
+			'OC_BackgroundJob',
+			'OC_Config',
+			'OC_DB',
+			'OC_Files',
+			'OC_Helper',
+			'OC_Hook',
+			'OC_Image',
+			'OC_JSON',
+			'OC_L10N',
+			'OC_Log',
+			'OC_Mail',
+			'OC_Preferences',
+			'OC_Request',
+			'OC_Response',
+			'OC_Template',
+			'OC_User',
+			'OC_Util',
+		];
+	}
+
+	/**
+	 * @param string $appId
+	 * @return array
+	 */
+	public function analyse($appId) {
+		$appPath = \OC_App::getAppPath($appId);
+		if ($appPath === false) {
+			throw new \RuntimeException("No app with given id <$appId> known.");
+		}
+
+		return $this->analyseFolder($appPath);
+	}
+
+	/**
+	 * @param string $folder
+	 * @return array
+	 */
+	public function analyseFolder($folder) {
+		$errors = [];
+
+		$excludes = array_map(function($item) use ($folder) {
+			return $folder . '/' . $item;
+		}, ['vendor', '3rdparty', '.git', 'l10n']);
+
+		$iterator = new RecursiveDirectoryIterator($folder, RecursiveDirectoryIterator::SKIP_DOTS);
+		$iterator = new RecursiveCallbackFilterIterator($iterator, function($item) use ($folder, $excludes){
+			/** @var SplFileInfo $item */
+			foreach($excludes as $exclude) {
+				if (substr($item->getPath(), 0, strlen($exclude)) === $exclude) {
+					return false;
+				}
+			}
+			return true;
+		});
+		$iterator = new RecursiveIteratorIterator($iterator);
+		$iterator = new RegexIterator($iterator, '/^.+\.php$/i');
+
+		foreach ($iterator as $file) {
+			/** @var SplFileInfo $file */
+			$this->emit('CodeChecker', 'analyseFileBegin', [$file->getPathname()]);
+			$errors = array_merge($this->analyseFile($file), $errors);
+			$this->emit('CodeChecker', 'analyseFileFinished', [$errors]);
+		}
+
+		return $errors;
+	}
+
+
+	/**
+	 * @param string $file
+	 * @return array
+	 */
+	public function analyseFile($file) {
+		$code = file_get_contents($file);
+		$statements = $this->parser->parse($code);
+
+		$visitor = new CodeCheckVisitor($this->blackListedClassNames);
+		$traverser = new NodeTraverser;
+		$traverser->addVisitor($visitor);
+
+		$traverser->traverse($statements);
+
+		return $visitor->errors;
+	}
+}

lib/private/app/codecheckvisitor.php

@@ -0,0 +1,111 @@
+<?php
+/**
+ * Copyright (c) 2015 Thomas Müller <deepdiver@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace OC\App;
+
+use OC\Hooks\BasicEmitter;
+use PhpParser\Lexer;
+use PhpParser\Node;
+use PhpParser\Node\Name;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitorAbstract;
+use PhpParser\Parser;
+use RecursiveCallbackFilterIterator;
+use RecursiveDirectoryIterator;
+use RecursiveIteratorIterator;
+use RegexIterator;
+use SplFileInfo;
+
+class CodeCheckVisitor extends NodeVisitorAbstract {
+
+	public function __construct($blackListedClassNames) {
+		$this->blackListedClassNames = array_map('strtolower', $blackListedClassNames);
+	}
+
+	public $errors = [];
+
+	public function enterNode(Node $node) {
+		if ($node instanceof Node\Stmt\Class_) {
+			if (!is_null($node->extends)) {
+				$this->checkBlackList($node->extends->toString(), CodeChecker::CLASS_EXTENDS_NOT_ALLOWED, $node);
+			}
+			foreach ($node->implements as $implements) {
+				$this->checkBlackList($implements->toString(), CodeChecker::CLASS_IMPLEMENTS_NOT_ALLOWED, $node);
+			}
+		}
+		if ($node instanceof Node\Expr\StaticCall) {
+			if (!is_null($node->class)) {
+				if ($node->class instanceof Name) {
+					$this->checkBlackList($node->class->toString(), CodeChecker::STATIC_CALL_NOT_ALLOWED, $node);
+				}
+				if ($node->class instanceof Node\Expr\Variable) {
+					/**
+					 * TODO: find a way to detect something like this:
+					 *       $c = "OC_API";
+					 *       $n = $i::call();
+					 */
+				}
+			}
+		}
+		if ($node instanceof Node\Expr\ClassConstFetch) {
+			if (!is_null($node->class)) {
+				if ($node->class instanceof Name) {
+					$this->checkBlackList($node->class->toString(), CodeChecker::CLASS_CONST_FETCH_NOT_ALLOWED, $node);
+				}
+				if ($node->class instanceof Node\Expr\Variable) {
+					/**
+					 * TODO: find a way to detect something like this:
+					 *       $c = "OC_API";
+					 *       $n = $i::ADMIN_AUTH;
+					 */
+				}
+			}
+		}
+		if ($node instanceof Node\Expr\New_) {
+			if (!is_null($node->class)) {
+				if ($node->class instanceof Name) {
+					$this->checkBlackList($node->class->toString(), CodeChecker::CLASS_NEW_FETCH_NOT_ALLOWED, $node);
+				}
+				if ($node->class instanceof Node\Expr\Variable) {
+					/**
+					 * TODO: find a way to detect something like this:
+					 *       $c = "OC_API";
+					 *       $n = new $i;
+					 */
+				}
+			}
+		}
+	}
+
+	private function checkBlackList($name, $errorCode, Node $node) {
+		if (in_array(strtolower($name), $this->blackListedClassNames)) {
+			$this->errors[]= [
+				'disallowedToken' => $name,
+				'errorCode' => $errorCode,
+				'line' => $node->getLine(),
+				'reason' => $this->buildReason($name, $errorCode)
+			];
+		}
+	}
+
+	private function buildReason($name, $errorCode) {
+		static $errorMessages= [
+			CodeChecker::CLASS_EXTENDS_NOT_ALLOWED => "used as base class",
+			CodeChecker::CLASS_IMPLEMENTS_NOT_ALLOWED => "used as interface",
+			CodeChecker::STATIC_CALL_NOT_ALLOWED => "static method call on private class",
+			CodeChecker::CLASS_CONST_FETCH_NOT_ALLOWED => "used to fetch a const from",
+			CodeChecker::CLASS_NEW_FETCH_NOT_ALLOWED => "is instanciated",
+		];
+
+		if (isset($errorMessages[$errorCode])) {
+			return $errorMessages[$errorCode];
+		}
+
+		return "$name usage not allowed - error: $errorCode";
+	}
+}

lib/private/installer.php

@@ -308,7 +308,7 @@ class OC_Installer{
 		}
 		$info=OC_App::getAppInfo($extractDir.'/appinfo/info.xml', true);
 		// check the code for not allowed calls
-		if(!$isShipped && !OC_Installer::checkCode($info['id'], $extractDir)) {
+		if(!$isShipped && !OC_Installer::checkCode($extractDir)) {
 			OC_Helper::rmdirr($extractDir);
 			throw new \Exception($l->t("App can't be installed because of not allowed code in the App"));
 		}
@@ -511,7 +511,7 @@ class OC_Installer{
 			OC_Appconfig::setValue($app, 'ocsid', $info['ocsid']);
 		}
 
-		//set remote/public handelers
+		//set remote/public handlers
 		foreach($info['remote'] as $name=>$path) {
 			OCP\CONFIG::setAppValue('core', 'remote_'.$name, $app.'/'.$path);
 		}
@@ -529,58 +529,16 @@ class OC_Installer{
 	 * @param string $folder the folder of the app to check
 	 * @return boolean true for app is o.k. and false for app is not o.k.
 	 */
-	public static function checkCode($appname, $folder) {
-		$blacklist=array(
-			// classes replaced by the public api
-			'OC_API::',
-			'OC_App::',
-			'OC_AppConfig::',
-			'OC_Avatar',
-			'OC_BackgroundJob::',
-			'OC_Config::',
-			'OC_DB::',
-			'OC_Files::',
-			'OC_Helper::',
-			'OC_Hook::',
-			'OC_Image::',
-			'OC_JSON::',
-			'OC_L10N::',
-			'OC_Log::',
-			'OC_Mail::',
-			'OC_Request::',
-			'OC_Response::',
-			'OC_Template::',
-			'OC_User::',
-			'OC_Util::',
-		);
 
+	public static function checkCode($folder) {
 		// is the code checker enabled?
-		if(OC_Config::getValue('appcodechecker', false)) {
-			// check if grep is installed
-			$grep = \OC_Helper::findBinaryPath('grep');
-			if (!$grep) {
-				OC_Log::write('core',
-					'grep not installed. So checking the code of the app "'.$appname.'" was not possible',
-					OC_Log::ERROR);
-				return true;
-			}
-
-			// iterate the bad patterns
-			foreach($blacklist as $bl) {
-				$cmd = 'grep --include \\*.php -ri '.escapeshellarg($bl).' '.$folder.'';
-				$result = exec($cmd);
-				// bad pattern found
-				if($result<>'') {
-					OC_Log::write('core',
-						'App "'.$appname.'" is using a not allowed call "'.$bl.'". Installation refused.',
-						OC_Log::ERROR);
-					return false;
-				}
-			}
-			return true;
-
-		}else{
+		if(!OC_Config::getValue('appcodechecker', false)) {
 			return true;
 		}
+
+		$codeChecker = new \OC\App\CodeChecker();
+		$errors = $codeChecker->analyseFolder($folder);
+
+		return empty($errors);
 	}
 }

tests/data/app/code-checker/test-const.php

@@ -0,0 +1,10 @@
+<?php
+
+/**
+ * Class BadClass - accessing consts on blacklisted classes is not allowed
+ */
+class BadClass {
+	public function foo() {
+		$bar = OC_API::ADMIN_AUTH;
+	}
+}

tests/data/app/code-checker/test-extends.php

@@ -0,0 +1,8 @@
+<?php
+
+/**
+ * Class BadClass - sub class a forbidden class is not allowed
+ */
+class BadClass extends OC_Hook {
+
+}

tests/data/app/code-checker/test-implements.php

@@ -0,0 +1,9 @@
+<?php
+
+/**
+ * Class BadClass - sub class a forbidden class is not allowed
+ *     NOTE: lowercase typo is intended
+ */
+class BadClass implements oC_Avatar {
+
+}

tests/data/app/code-checker/test-new.php

@@ -0,0 +1,10 @@
+<?php
+
+/**
+ * Class BadClass - creating an instance of a blacklisted class is not allowed
+ */
+class BadClass {
+	public function foo() {
+		$bar = new OC_AppConfig();
+	}
+}

tests/data/app/code-checker/test-static-call.php

@@ -0,0 +1,10 @@
+<?php
+
+/**
+ * Class BadClass - calling static methods on blacklisted classes is not allowed
+ */
+class BadClass {
+	public function foo() {
+		OC_App::isEnabled('bar');
+	}
+}

tests/lib/app/codechecker.php

@@ -0,0 +1,38 @@
+<?php
+/**
+ * Copyright (c) 2015 Thomas Müller <deepdiver@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace Test\App;
+
+use OC;
+
+class CodeChecker extends \Test\TestCase {
+
+	/**
+	 * @dataProvider providesFilesToCheck
+	 * @param $expectedErrors
+	 * @param $fileToVerify
+	 */
+	public function testFindInvalidUsage($expectedErrorToken, $expectedErrorCode, $fileToVerify) {
+		$checker = new OC\App\CodeChecker();
+		$errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify");
+
+		$this->assertEquals(1, count($errors));
+		$this->assertEquals($expectedErrorCode, $errors[0]['errorCode']);
+		$this->assertEquals($expectedErrorToken, $errors[0]['disallowedToken']);
+	}
+
+	public function providesFilesToCheck() {
+		return [
+			['OC_Hook', 1000, 'test-extends.php'],
+			['oC_Avatar', 1001, 'test-implements.php'],
+			['OC_App', 1002, 'test-static-call.php'],
+			['OC_API', 1003, 'test-const.php'],
+			['OC_AppConfig', 1004, 'test-new.php'],
+		];
+	}
+}