From d74662df7df72ad9ec238b78223acc0e7f65311f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Wed, 28 Jan 2015 22:08:50 +0100 Subject: [PATCH] implement php code checker to detect usage of not allowed private APIs - including console command to check local code to be used by developers --- 3rdparty | 2 +- core/command/app/checkcode.php | 53 ++++++++ core/register_command.php | 1 + lib/private/app/codechecker.php | 115 ++++++++++++++++++ lib/private/app/codecheckervisitor.php | 111 +++++++++++++++++ lib/private/installer.php | 2 +- tests/data/app/code-checker/test-const.php | 10 ++ tests/data/app/code-checker/test-extends.php | 8 ++ .../data/app/code-checker/test-implements.php | 9 ++ tests/data/app/code-checker/test-new.php | 10 ++ .../app/code-checker/test-static-call.php | 10 ++ tests/lib/app/codechecker.php | 38 ++++++ 12 files changed, 367 insertions(+), 2 deletions(-) create mode 100644 core/command/app/checkcode.php create mode 100644 lib/private/app/codechecker.php create mode 100644 lib/private/app/codecheckervisitor.php create mode 100644 tests/data/app/code-checker/test-const.php create mode 100644 tests/data/app/code-checker/test-extends.php create mode 100644 tests/data/app/code-checker/test-implements.php create mode 100644 tests/data/app/code-checker/test-new.php create mode 100644 tests/data/app/code-checker/test-static-call.php create mode 100644 tests/lib/app/codechecker.php diff --git a/3rdparty b/3rdparty index a32d3924bd..cac665dce1 160000 --- a/3rdparty +++ b/3rdparty @@ -1 +1 @@ -Subproject commit a32d3924bd0012a5410fff4666131cbdfdec2001 +Subproject commit cac665dce1393c067fb3ffe4347f6e0b01dda8bf diff --git a/core/command/app/checkcode.php b/core/command/app/checkcode.php new file mode 100644 index 0000000000..55c30b900b --- /dev/null +++ b/core/command/app/checkcode.php @@ -0,0 +1,53 @@ + + * 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("Analysing {$params}"); + }); + $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(" line $line: {$p['disallowedToken']} - {$p['reason']}"); + } + }); + $errors = $codeChecker->analyse($appId); + if (empty($errors)) { + $output->writeln('App is compliant - awesome job!'); + } else { + $output->writeln('App is not compliant'); + } + } +} diff --git a/core/register_command.php b/core/register_command.php index 5aa55be3e2..d7aaf9a41b 100644 --- a/core/register_command.php +++ b/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()); diff --git a/lib/private/app/codechecker.php b/lib/private/app/codechecker.php new file mode 100644 index 0000000000..28816a8fdc --- /dev/null +++ b/lib/private/app/codechecker.php @@ -0,0 +1,115 @@ + + * 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; + + 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."); + } + + $errors = []; + + $excludes = array_map(function($item) use ($appPath) { + return $appPath . '/' . $item; + }, ['vendor', '3rdparty', '.git', 'l10n']); + + $iterator = new RecursiveDirectoryIterator($appPath, RecursiveDirectoryIterator::SKIP_DOTS); + $iterator = new RecursiveCallbackFilterIterator($iterator, function($item) use ($appPath, $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; + } +} diff --git a/lib/private/app/codecheckervisitor.php b/lib/private/app/codecheckervisitor.php new file mode 100644 index 0000000000..939c905bcf --- /dev/null +++ b/lib/private/app/codecheckervisitor.php @@ -0,0 +1,111 @@ + + * 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"; + } +} diff --git a/lib/private/installer.php b/lib/private/installer.php index b4fbe527b4..e77504f4c1 100644 --- a/lib/private/installer.php +++ b/lib/private/installer.php @@ -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); } diff --git a/tests/data/app/code-checker/test-const.php b/tests/data/app/code-checker/test-const.php new file mode 100644 index 0000000000..2af6baf2f3 --- /dev/null +++ b/tests/data/app/code-checker/test-const.php @@ -0,0 +1,10 @@ + + * 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'], + ]; + } +}