From 35fa92f9ef14f796af7d1247961dde0f77170ae0 Mon Sep 17 00:00:00 2001 From: Jan Tvrdik Date: Tue, 10 May 2016 00:13:11 +0200 Subject: [PATCH] better --- composer.json | 8 +- .../NetteDI}/SecuredLinksExtension.php | 53 ++++- src/Bridges/PhpParser/ReturnTypeResolver.php | 188 ++++++++++++++++++ src/RedirectChecker.php | 33 +++ src/SecuredRouter.php | 7 +- src/deprecated/SecuredLinksControlTrait.php | 42 ++++ src/deprecated/SecuredLinksPresenterTrait.php | 119 +++++++++++ tests/cases/SecuredLinksExtensionTest.phpt | 3 +- tests/fixtures/TestPresenter.php | 44 +++- 9 files changed, 483 insertions(+), 14 deletions(-) rename src/{ => Bridges/NetteDI}/SecuredLinksExtension.php (74%) create mode 100644 src/Bridges/PhpParser/ReturnTypeResolver.php create mode 100644 src/RedirectChecker.php create mode 100644 src/deprecated/SecuredLinksControlTrait.php create mode 100644 src/deprecated/SecuredLinksPresenterTrait.php diff --git a/composer.json b/composer.json index 333dcab..f25864a 100644 --- a/composer.json +++ b/composer.json @@ -18,10 +18,14 @@ }, "require-dev": { "nette/di": "~2.2", - "nette/robot-loader": "~2.2", "nette/tester": "~1.3", "tracy/tracy": "~2.2", - "mockery/mockery": "~0.9" + "mockery/mockery": "~0.9", + "nikic/php-parser": "~2.0" + }, + "suggest": { + "nette/di": "to use SecuredLinksExtension", + "nikic/php-parser": "to detect return types without @return annotation" }, "extra": { "branch-alias": { diff --git a/src/SecuredLinksExtension.php b/src/Bridges/NetteDI/SecuredLinksExtension.php similarity index 74% rename from src/SecuredLinksExtension.php rename to src/Bridges/NetteDI/SecuredLinksExtension.php index 20313f1..186e9c2 100644 --- a/src/SecuredLinksExtension.php +++ b/src/Bridges/NetteDI/SecuredLinksExtension.php @@ -2,11 +2,12 @@ /** * This file is part of the Nextras Secured Links library. + * * @license MIT * @link https://github.com/nextras/secured-links */ -namespace Nextras\SecuredLinks; +namespace Nextras\SecuredLinks\Bridges\NetteDI; use Generator; use Nette; @@ -15,16 +16,22 @@ use Nette\DI\PhpReflection; use Nette\Neon\Neon; use Nette\Utils\Strings; +use Nextras\SecuredLinks\Bridges\PhpParser\ReturnTypeResolver; +use Nextras\SecuredLinks\RedirectChecker; +use Nextras\SecuredLinks\SecuredRouterFactory; +use PhpParser\Node; use ReflectionClass; use ReflectionMethod; class SecuredLinksExtension extends Nette\DI\CompilerExtension { + /** @var array */ public $defaults = [ 'annotation' => 'secured', // can be NULL to disable 'destinations' => [], + 'strictMode' => TRUE, ]; @@ -52,6 +59,12 @@ public function beforeCompile() ->setClass(IRouter::class) ->setFactory("@{$this->name}.routerFactory::create", ["@$innerRouter"]) ->setAutowired(TRUE); + + $builder->addDefinition($this->prefix('redirectChecker')) + ->setClass(RedirectChecker::class); + + $builder->getDefinition($builder->getByType(Nette\Application\Application::class)) + ->addSetup('?::$onResponse[] = ?', ['@self', '@Nextras\SecuredLinks\RedirectChecker']); } @@ -88,18 +101,20 @@ private function findSecuredDestinations() { $config = $this->validateConfig($this->defaults); + foreach ($config['destinations'] as $presenterClass => $destinations) { + yield $presenterClass => $destinations; + } + if ($config['annotation']) { $presenters = $this->getContainerBuilder()->findByType(Presenter::class); foreach ($presenters as $presenterDef) { $presenterClass = $presenterDef->getClass(); - $presenterRef = new \ReflectionClass($presenterClass); - yield $presenterClass => $this->findSecuredMethods($presenterRef); + if (!isset($config['destinations'][$presenterClass])) { + $presenterRef = new \ReflectionClass($presenterClass); + yield $presenterClass => $this->findSecuredMethods($presenterRef); + } } } - - foreach ($config['destinations'] as $presenterClass => $destinations) { - yield $presenterClass => $destinations; - } } @@ -135,13 +150,20 @@ private function findTargetMethods(ReflectionClass $classRef) yield $destination => $methodRef; } elseif (Strings::startsWith($methodName, 'createComponent')) { - $returnType = PhpReflection::getReturnType($methodRef); + $returnType = $this->getMethodReturnType($methodRef); if ($returnType !== NULL) { $returnTypeRef = new ReflectionClass($returnType); $componentName = Strings::firstLower(Strings::after($methodName, 'createComponent')); foreach ($this->findTargetMethods($returnTypeRef) as $innerDestination => $innerRef) { yield "$componentName-$innerDestination" => $innerRef; } + + } elseif ($this->config['strictMode']) { + $className = $methodRef->getDeclaringClass()->getName(); + throw new \LogicException( + "Unable to deduce return type for method $className::$methodName(); " . + "add @return annotation, install nikic/php-parser or disable strictMode in config" + ); } } } @@ -169,4 +191,19 @@ private function isSecured(ReflectionMethod $ref, & $params) return FALSE; } } + + + /** + * @param ReflectionMethod $methodRef + * @return NULL|string + */ + private function getMethodReturnType(ReflectionMethod $methodRef) + { + $returnType = PhpReflection::getReturnType($methodRef); + if ($returnType !== NULL || !interface_exists(\PhpParser\Node::class)) { + return $returnType; + } else { + return ReturnTypeResolver::getReturnType($methodRef); + } + } } diff --git a/src/Bridges/PhpParser/ReturnTypeResolver.php b/src/Bridges/PhpParser/ReturnTypeResolver.php new file mode 100644 index 0000000..ae95125 --- /dev/null +++ b/src/Bridges/PhpParser/ReturnTypeResolver.php @@ -0,0 +1,188 @@ +className = $className; + $this->methodName = $methodName; + $this->varTypes['this'][] = $className; + } + + + /** + * @param ReflectionMethod $methodRef + * @return NULL|string + */ + public static function getReturnType(ReflectionMethod $methodRef) + { + $fileContent = file_get_contents($methodRef->getDeclaringClass()->getFileName()); + + $traverser = new NodeTraverser(); + $traverser->addVisitor(new NameResolver); + $traverser->addVisitor($resolver = new self($methodRef->getDeclaringClass()->getName(), $methodRef->getName())); + $traverser->traverse((new ParserFactory)->create(ParserFactory::PREFER_PHP7)->parse($fileContent)); + + return count($resolver->returnTypes) === 1 ? $resolver->returnTypes[0] : NULL; + } + + + /** + * @inheritdoc + */ + public function enterNode(Node $node) + { + if ($node instanceof Node\Stmt\Class_ && $node->name === $this->className) { + $this->inClass = TRUE; + + } elseif ($this->inClass && $node instanceof Node\Stmt\ClassMethod && $node->name === $this->methodName) { + $this->inMethod = TRUE; + + } elseif ($this->inMethod) { + if ($node instanceof Node\Stmt\Return_ && $node->expr !== NULL) { + foreach ($this->getExpressionTypes($node->expr) as $type) { + $this->addReturnType($type); + } + + } elseif ($node instanceof Node\Expr\Assign) { + foreach ($this->getExpressionTypes($node->expr) as $type) { + $this->addVarType($node, $type); + } + } + } + } + + + /** + * @inheritdoc + */ + public function leaveNode(Node $node) + { + if ($this->inMethod && $node instanceof Node\Stmt\ClassMethod) { + $this->inMethod = FALSE; + + } elseif ($this->inClass && $node instanceof Node\Stmt\Class_) { + $this->inClass = FALSE; + } + } + + + /** + * @param Node\Expr $expr + * @return string[] + */ + private function getExpressionTypes(Node\Expr $expr) + { + $result = []; + + if ($expr instanceof Node\Expr\New_) { + if ($expr->class instanceof Node\Name) { + $result[] = (string) $expr->class; + } + + } elseif ($expr instanceof Node\Expr\Variable) { + if (is_string($expr->name) && isset($this->varTypes[$expr->name])) { + $result = $this->varTypes[$expr->name]; + } + + } elseif ($expr instanceof Node\Expr\PropertyFetch) { + if (is_string($expr->name)) { + foreach ($this->getExpressionTypes($expr->var) as $objType) { + $propertyRef = new \ReflectionProperty($objType, $expr->name); + $type = PhpReflection::parseAnnotation($propertyRef, 'var'); + $type = $type ? PhpReflection::expandClassName($type, PhpReflection::getDeclaringClass($propertyRef)) : NULL; + $result[] = $type; + } + } + + } elseif ($expr instanceof Node\Expr\MethodCall) { + if (is_string($expr->name)) { + foreach ($this->getExpressionTypes($expr->var) as $objType) { + $methodRef = new \ReflectionMethod($objType, $expr->name); + $result[] = PhpReflection::getReturnType($methodRef); + } + } + + } elseif ($expr instanceof Node\Expr\Assign) { + foreach ($this->getExpressionTypes($expr->expr) as $type) { + $this->addVarType($expr, $type); + $result[] = $type; + } + + } elseif ($expr instanceof Node\Expr\Clone_) { + $result = $this->getExpressionTypes($expr->expr); + } + + return $result; + } + + + /** + * @param string $exprType + * @return void + */ + private function addReturnType($exprType) + { + if ($exprType !== NULL && class_exists($exprType) && !in_array($exprType, $this->returnTypes)) { + $this->returnTypes[] = $exprType; + } + } + + + /** + * @param Node\Expr\Assign $node + * @param string $exprType + * @return void + */ + private function addVarType($node, $exprType) + { + if ($node->var instanceof Node\Expr\Variable && is_string($node->var->name) + && (empty($this->varTypes[$node->var->name]) || !in_array($exprType, $this->varTypes[$node->var->name])) + && $exprType !== NULL && class_exists($exprType) + ) { + $this->varTypes[$node->var->name][] = $exprType; + } + } +} diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php new file mode 100644 index 0000000..b470181 --- /dev/null +++ b/src/RedirectChecker.php @@ -0,0 +1,33 @@ +getRequests(); + $request = $requests[count($requests) - 1]; + + if ($request->hasFlag(SecuredRouter::SIGNED) && !$response instanceof RedirectResponse) { + throw new \LogicException('Secured request did not redirect. Possible CSRF-token reveal by HTTP referer header.'); + } + } +} diff --git a/src/SecuredRouter.php b/src/SecuredRouter.php index 5d520d0..c99e765 100644 --- a/src/SecuredRouter.php +++ b/src/SecuredRouter.php @@ -17,6 +17,9 @@ class SecuredRouter implements IRouter { + /** signed flag, marks requests which has been signed */ + const SIGNED = 'signed'; + /** length of secret token stored in session */ const SECURITY_TOKEN_LENGTH = 16; @@ -57,8 +60,8 @@ public function __construct(IRouter $inner, IPresenterFactory $presenterFactory, public function match(Nette\Http\IRequest $httpRequest) { $appRequest = $this->inner->match($httpRequest); - if ($appRequest !== NULL && !$this->isSignatureOk($appRequest)) { - return NULL; + if ($appRequest !== NULL && $this->isSignatureOk($appRequest)) { + $appRequest->setFlag(self::SIGNED); } return $appRequest; diff --git a/src/deprecated/SecuredLinksControlTrait.php b/src/deprecated/SecuredLinksControlTrait.php new file mode 100644 index 0000000..b20c415 --- /dev/null +++ b/src/deprecated/SecuredLinksControlTrait.php @@ -0,0 +1,42 @@ +formatSignalMethod($signal); + if (method_exists($this, $methodName)) { + $methodRef = new Nette\Reflection\Method($this, $methodName); + if ($methodRef->hasAnnotation('secured') && !$this->request->hasFlag(SecuredRouter::SIGNED)) { + $who = $this instanceof Presenter ? 'Presenter' : 'Control'; + throw new \LogicException( + "$who received request to secured signal which was not properly signed." . + "This indicate a bug in your installation of Nextras Secured Links." . + "Please consult documentation on how to properly migrate to Nextras Secured Links 2.0" + ); + } + } + + parent::signalReceived($signal); + } +} diff --git a/src/deprecated/SecuredLinksPresenterTrait.php b/src/deprecated/SecuredLinksPresenterTrait.php new file mode 100644 index 0000000..080ea21 --- /dev/null +++ b/src/deprecated/SecuredLinksPresenterTrait.php @@ -0,0 +1,119 @@ +getLastCreatedRequest(); + + do { + if ($lastRequest === NULL) { + break; + } + + $params = $lastRequest->getParameters(); + if (!isset($params[Nette\Application\UI\Presenter::SIGNAL_KEY])) { + break; + } + + if (($pos = strpos($destination, '#')) !== FALSE) { + $destination = substr($destination, 0, $pos); + } + + $a = strpos($destination, '//'); + if ($a !== FALSE) { + $destination = substr($destination, $a + 2); + } + + $signal = strtr(rtrim($destination, '!'), ':', '-'); + $a = strrpos($signal, '-'); + if ($a !== FALSE) { + if ($component instanceof Nette\Application\UI\Presenter && substr($destination, -1) !== '!') { + break; + } + + $component = $component->getComponent(substr($signal, 0, $a)); + $signal = (string) substr($signal, $a + 1); + } + + if ($signal == NULL) { // intentionally == + throw new Nette\Application\UI\InvalidLinkException('Signal must be non-empty string.'); + } + + // only PresenterComponent + if (!$component instanceof PresenterComponent) { + break; + } + + $reflection = $component->getReflection(); + $method = $component->formatSignalMethod($signal); + $signalReflection = $reflection->getMethod($method); + + if (!$signalReflection->hasAnnotation('secured')) { + break; + } + + $origParams = $lastRequest->getParameters(); + $protectedParams = array($component->getUniqueId()); + foreach ($signalReflection->getParameters() as $param) { + if ($param->isOptional()) { + continue; + } + if (isset($origParams[$component->getParameterId($param->name)])) { + $protectedParams[$param->name] = $origParams[$component->getParameterId($param->name)]; + } + } + + $protectedParam = $this->getCsrfToken(get_class($component), $method, $protectedParams); + + if (($pos = strpos($link, '#')) === FALSE) { + $fragment = ''; + } else { + $fragment = substr($link, $pos); + $link = substr($link, 0, $pos); + } + + $link .= (strpos($link, '?') !== FALSE ? '&' : '?') . $component->getParameterId('_sec') . '=' . $protectedParam . $fragment; + } while (FALSE); + + return $link; + } + + + /** + * @deprecated + */ + public function getCsrfToken($control, $method, $params) + { + $session = $this->getSession('Nextras.Application.UI.SecuredLinksPresenterTrait'); + if (!isset($session->token)) { + $session->token = Nette\Utils\Random::generate(); + } + + $params = Nette\Utils\Arrays::flatten($params); + $params = implode('|', array_keys($params)) . '|' . implode('|', array_values($params)); + return substr(md5($control . $method . $params . $session->token . $this->getSession()->getId()), 0, 8); + } +} diff --git a/tests/cases/SecuredLinksExtensionTest.phpt b/tests/cases/SecuredLinksExtensionTest.phpt index ffd6da3..8a59082 100644 --- a/tests/cases/SecuredLinksExtensionTest.phpt +++ b/tests/cases/SecuredLinksExtensionTest.phpt @@ -16,7 +16,7 @@ use Nette\Application\Routers\Route; use Nette\Bridges\ApplicationDI\ApplicationExtension; use Nette\Bridges\HttpDI\HttpExtension; use Nette\Bridges\HttpDI\SessionExtension; -use Nextras\SecuredLinks\SecuredLinksExtension; +use Nextras\SecuredLinks\Bridges\NetteDI\SecuredLinksExtension; use Nextras\SecuredLinks\SecuredRouter; use Tester; use Tester\Assert; @@ -96,6 +96,7 @@ class SecuredLinksExtensionTest extends Tester\TestCase $dic = new \SecuredLinksExtensionContainer(); $dic->removeService('nette.http.sessions.session'); $dic->addService('nette.http.sessions.session', $session); + return $dic; } } diff --git a/tests/fixtures/TestPresenter.php b/tests/fixtures/TestPresenter.php index f8399be..d03bb6c 100644 --- a/tests/fixtures/TestPresenter.php +++ b/tests/fixtures/TestPresenter.php @@ -12,9 +12,25 @@ public function handlePay($amount = 0) } } +interface TestControlFactory +{ + + /** + * @return TestControl + */ + public function create(); +} + class TestPresenter extends Presenter { + /** @var TestControl */ + public $testControl; + + /** @var TestControlFactory */ + public $testControlFactory; + + public function renderDefault() { $this->terminate(); @@ -51,8 +67,34 @@ public function actionDelete() /** * @return TestControl */ - protected function createComponentMyControl() + protected function createComponentMyControlA() + { + + } + + + protected function createComponentMyControlB() { return new TestControl(); } + + + protected function createComponentMyControlC() + { + $tmp = new TestControl(); + $control = $tmp; + return $control; + } + + + protected function createComponentMyControlD() + { + return clone $this->testControl; + } + + + protected function createComponentMyControlE() + { + return $this->testControlFactory->create(); + } }