From b004f3a0bd3788a763109658b31456f2e0cba5b8 Mon Sep 17 00:00:00 2001 From: Jan Tvrdik Date: Sun, 8 May 2016 17:29:40 +0200 Subject: [PATCH] WIP: solution based on CompilerExtension --- composer.json | 3 + src/SecuredLinksControlTrait.php | 75 ------------ src/SecuredLinksExtension.php | 135 +++++++++++++++++++++ src/SecuredLinksPresenterTrait.php | 127 ------------------- src/SecuredRouter.php | 79 ++++++------ src/SecuredRouterFactory.php | 17 +++ src/SecuredRouterFlagBased.php | 99 --------------- tests/cases/SecuredLinksExtensionTest.phpt | 98 +++++++++++++++ tests/fixtures/TestPresenter.php | 58 +++++++++ 9 files changed, 351 insertions(+), 340 deletions(-) delete mode 100644 src/SecuredLinksControlTrait.php create mode 100644 src/SecuredLinksExtension.php delete mode 100644 src/SecuredLinksPresenterTrait.php create mode 100644 src/SecuredRouterFactory.php delete mode 100644 src/SecuredRouterFlagBased.php create mode 100644 tests/cases/SecuredLinksExtensionTest.phpt create mode 100644 tests/fixtures/TestPresenter.php diff --git a/composer.json b/composer.json index 99d2fc4..3128a71 100644 --- a/composer.json +++ b/composer.json @@ -17,7 +17,10 @@ "nette/utils": "~2.2" }, "require-dev": { + "nette/di": "~2.2", + "nette/robot-loader": "~2.2", "nette/tester": "~1.3", + "tracy/tracy": "~2.2", "mockery/mockery": "~0.9" }, "extra": { diff --git a/src/SecuredLinksControlTrait.php b/src/SecuredLinksControlTrait.php deleted file mode 100644 index a2ad601..0000000 --- a/src/SecuredLinksControlTrait.php +++ /dev/null @@ -1,75 +0,0 @@ -getPresenter()->createSecuredLink($this, $link, $destination); - } - - - /** - * For @secured annotated signal handler methods checks if URL parameters has not been changed - * - * @param string $signal - * @throws Nette\Application\UI\BadSignalException if there is no handler method or the security token does not match - * @throws \LogicException if there is no redirect in a secured signal - */ - public function signalReceived($signal) - { - $method = $this->formatSignalMethod($signal); - $secured = FALSE; - - if (method_exists($this, $method)) { - $reflection = new Nette\Reflection\Method($this, $method); - $secured = $reflection->hasAnnotation('secured'); - if ($secured) { - $params = array($this->getUniqueId()); - if ($this->params) { - foreach ($reflection->getParameters() as $param) { - if ($param->isOptional()) { - continue; - } - if (isset($this->params[$param->name])) { - $params[$param->name] = $this->params[$param->name]; - } - } - } - - if (!isset($this->params['_sec']) || $this->params['_sec'] !== $this->getPresenter()->getCsrfToken(get_class($this), $method, $params)) { - throw new Nette\Application\UI\BadSignalException("Invalid security token for signal '$signal' in class {$this->reflection->name}."); - } - } - } - - parent::signalReceived($signal); - - if ($secured && !$this->getPresenter()->isAjax()) { - throw new \LogicException("Secured signal '$signal' did not redirect. Possible csrf-token reveal by http referer header."); - } - } - -} diff --git a/src/SecuredLinksExtension.php b/src/SecuredLinksExtension.php new file mode 100644 index 0000000..afe10b4 --- /dev/null +++ b/src/SecuredLinksExtension.php @@ -0,0 +1,135 @@ +getContainerBuilder(); + $builder->addDefinition($this->prefix('routerFactory')) + ->setImplement(SecuredRouterFactory::class) + ->setParameters(['Nette\Application\IRouter innerRouter']) + ->setArguments([ + $builder->literal('$innerRouter'), + '@Nette\Application\IPresenterFactory', + '@Nette\Http\Session', + $this->findSecuredPresentersMeta() + ]); + + $innerRouter = $builder->getByType(IRouter::class); + $builder->getDefinition($innerRouter) + ->setAutowired(FALSE); + + $builder->addDefinition($this->prefix('router')) + ->setClass(IRouter::class) + ->setFactory("@{$this->name}.routerFactory::create", ["@$innerRouter"]) + ->setAutowired(TRUE); + } + + + /** + * @return array + */ + private function findSecuredPresentersMeta() + { + $builder = $this->getContainerBuilder(); + $presenters = $builder->findByType(Presenter::class); + $secured = []; + + foreach ($presenters as $presenterDef) { + $presenterClass = $presenterDef->getClass(); + $presenterRef = new \ReflectionClass($presenterClass); + + foreach ($this->findSecuredDestinations($presenterRef) as $destination => $params) { + if (Strings::endsWith($destination, '!')) { + $key = 'do'; + $value = Strings::substring($destination, 0, -1); + + } else { + $key = 'action'; + $value = $destination; + } + + $secured[$presenterClass][$key][$value] = $params; + } + } + return $secured; + } + + + /** + * @param ReflectionClass $classRef + * @return Generator + */ + private function findSecuredDestinations(ReflectionClass $classRef) + { + foreach ($this->findTargetMethods($classRef) as $destination => $methodRef) { + if ($this->isSecured($methodRef, $ignoredParams)) { + yield $destination => $ignoredParams; + } + } + } + + + /** + * @param ReflectionClass $classRef + * @return Generator|ReflectionMethod[] + */ + private function findTargetMethods(ReflectionClass $classRef) + { + foreach ($classRef->getMethods() as $methodRef) { + $methodName = $methodRef->getName(); + + if (Strings::startsWith($methodName, 'action') && $classRef->isSubclassOf(Presenter::class)) { + $destination = Strings::firstLower(Strings::after($methodName, 'action')); + yield $destination => $methodRef; + + } elseif (Strings::startsWith($methodName, 'handle')) { + $destination = Strings::firstLower(Strings::after($methodName, 'handle')) . '!'; + yield $destination => $methodRef; + + } elseif (Strings::startsWith($methodName, 'createComponent')) { + $returnType = PhpReflection::getReturnType($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; + } + } + } + } + } + + + /** + * @param ReflectionMethod $ref + * @param array|bool $params + * @return bool + */ + private function isSecured(ReflectionMethod $ref, & $params) + { + if (preg_match('#^[ \t/*]*@secured(?:[ \t]+(\[.*?\])?|$)#m', $ref->getDocComment(), $matches)) { + $params = !empty($matches[1]) ? Neon::decode($matches[1]) : TRUE; + return TRUE; + + } else { + return FALSE; + } + } +} diff --git a/src/SecuredLinksPresenterTrait.php b/src/SecuredLinksPresenterTrait.php deleted file mode 100644 index 7e64450..0000000 --- a/src/SecuredLinksPresenterTrait.php +++ /dev/null @@ -1,127 +0,0 @@ -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; - } - - - /** - * Returns unique token for method and params - * @param string $control - * @param string $method - * @param array $params - * @return string - */ - 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/src/SecuredRouter.php b/src/SecuredRouter.php index cf4f4d9..d178dfa 100644 --- a/src/SecuredRouter.php +++ b/src/SecuredRouter.php @@ -7,13 +7,13 @@ use Nette\Application\IPresenterFactory; use Nette\Application\IRouter; use Nette\Application\Request; -use Nette\Application\UI\Presenter; use Nette\Http\Session; use ReflectionMethod; class SecuredRouter implements IRouter { + /** @var IRouter */ private $inner; @@ -23,17 +23,22 @@ class SecuredRouter implements IRouter /** @var Session */ private $session; + /** @var array */ + private $secured; + /** * @param IRouter $inner * @param IPresenterFactory $presenterFactory * @param Session $session + * @param array $secured */ - public function __construct(IRouter $inner, IPresenterFactory $presenterFactory, Session $session) + public function __construct(IRouter $inner, IPresenterFactory $presenterFactory, Session $session, array $secured) { $this->inner = $inner; $this->presenterFactory = $presenterFactory; $this->session = $session; + $this->secured = $secured; } @@ -57,9 +62,11 @@ public function match(Nette\Http\IRequest $httpRequest) */ public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) { - if ($this->isSignatureRequired($appRequest)) { - $signature = $this->getSignature($appRequest); - $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); + if ($this->isSignatureRequired($appRequest, $ignoredParams)) { + $signature = $this->getSignature($appRequest, $ignoredParams); + $params = $appRequest->getParameters(); + $params['_sec'] = $signature; + $appRequest->setParameters($params); } return $this->inner->constructUrl($appRequest, $refUrl); @@ -67,32 +74,23 @@ public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) /** - * @param Request $appRequest + * @param Request $appRequest + * @param array|bool $ignoredParams * @return bool */ - protected function isSignatureRequired(Request $appRequest) + protected function isSignatureRequired(Request $appRequest, & $ignoredParams) { $presenterName = $appRequest->getPresenterName(); $presenterClass = $this->presenterFactory->getPresenterClass($presenterName); - if (!is_a($presenterClass, Presenter::class)) { + if (!isset($this->secured[$presenterClass])) { return FALSE; } $params = $appRequest->getParameters(); - - if (isset($params['action'])) { - $methodName = $presenterClass::formatActionMethod($params['action']); - $methodRef = new ReflectionMethod($presenterClass, $methodName); - if ($this->isSecured($methodRef)) { - return TRUE; - } - } - - if (isset($params['do'])) { - $methodName = $presenterClass::formatSignalMethod($params['do']); - $methodRef = new ReflectionMethod($presenterClass, $methodName); - if ($this->isSecured($methodRef)) { + foreach ($this->secured[$presenterClass] as $key => $foobar) { + if (isset($params[$key], $this->secured[$presenterClass][$key][$params[$key]])) { + $ignoredParams = $this->secured[$presenterClass][$key][$params[$key]]; return TRUE; } } @@ -102,41 +100,44 @@ protected function isSignatureRequired(Request $appRequest) /** - * @param ReflectionMethod $ref + * @param Request $appRequest + * @param array|bool $ignoredParams * @return bool */ - public function isSecured(ReflectionMethod $ref) - { - return (bool) preg_match('#^[ \t*]*@secured(\s|$)#m', $ref->getDocComment()); - } - - - /** - * @param Request $appRequest - * @return bool - */ - private function isSignatureValid(Request $appRequest) + private function isSignatureValid(Request $appRequest, $ignoredParams) { $signature = $appRequest->getParameter('_sec'); - return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); + return ($signature !== NULL && hash_equals($this->getSignature($appRequest, $ignoredParams), $signature)); } /** - * @param Request $appRequest + * @param Request $appRequest + * @param array|bool $ignoredParams * @return string */ - private function getSignature(Request $appRequest) + private function getSignature(Request $appRequest, $ignoredParams) { - $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); + $sessionSection = $this->session->getSection('Nextras.SecuredLinks'); if (!isset($sessionSection->token)) { $sessionSection->token = function_exists('random_bytes') ? random_bytes(16) : Nette\Utils\Random::generate(16, "\x00-\xFF"); } - $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; - $hash = hash_hmac('sha1', serialize($data), $sessionSection->token); + if ($ignoredParams === TRUE) { + $params = $appRequest->getParameters(); + } elseif ($ignoredParams === FALSE) { + $params = []; + } else { + $params = $appRequest->getParameters(); + foreach ($ignoredParams as $key) { + unset($params[$key]); + } + } + + $data = [$this->session->getId(), $appRequest->getPresenterName(), $params]; + $hash = hash_hmac('sha1', json_encode($data), $sessionSection->token); return substr($hash, 0, 6); } } diff --git a/src/SecuredRouterFactory.php b/src/SecuredRouterFactory.php new file mode 100644 index 0000000..a8ffb38 --- /dev/null +++ b/src/SecuredRouterFactory.php @@ -0,0 +1,17 @@ +inner = $inner; - $this->presenterFactory = $presenterFactory; - $this->session = $session; - } - - - /** - * @inheritdoc - */ - public function match(Nette\Http\IRequest $httpRequest) - { - $appRequest = $this->inner->match($httpRequest); - - if ($appRequest !== NULL && $this->isSignatureValid($appRequest)) { - $appRequest->setFlag(self::SIGNED); - } - - return $appRequest; - } - - - /** - * @inheritdoc - */ - public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) - { - if ($appRequest->hasFlag(self::SIGNED)) { - $signature = $this->getSignature($appRequest); - $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); - } - - return $this->inner->constructUrl($appRequest, $refUrl); - } - - - /** - * @param Request $appRequest - * @return bool - */ - private function isSignatureValid(Request $appRequest) - { - $signature = $appRequest->getParameter('_sec'); - return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); - } - - - /** - * @param Request $appRequest - * @return string - */ - private function getSignature(Request $appRequest) - { - $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); - if (!isset($sessionSection->token)) { - $sessionSection->token = function_exists('random_bytes') - ? random_bytes(16) - : Nette\Utils\Random::generate(16, "\x00-\xFF"); - } - - $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; - $hash = hash_hmac('sha1', json_encode($data), $sessionSection->token); - return substr($hash, 0, 6); - } -} diff --git a/tests/cases/SecuredLinksExtensionTest.phpt b/tests/cases/SecuredLinksExtensionTest.phpt new file mode 100644 index 0000000..aa51dcd --- /dev/null +++ b/tests/cases/SecuredLinksExtensionTest.phpt @@ -0,0 +1,98 @@ +createContainer(); + $router = $dic->getByType(IRouter::class); + Assert::type(SecuredRouter::class, $router); + + $linkGenerator = $dic->getByType(LinkGenerator::class); + + Assert::same( + 'http://example.com/test?action=delete&_sec=a99c34', + $linkGenerator->link('Test:delete') + ); + + Assert::same( + 'http://example.com/test?do=pay&action=default&_sec=b5a0f0', + $linkGenerator->link('Test:default', ['do' => 'pay']) + ); + + Assert::same( + 'http://example.com/test?do=pay&amount=1&action=default&_sec=05cb93', + $linkGenerator->link('Test:default', ['do' => 'pay', 'amount' => 1]) + ); + + Assert::same( + 'http://example.com/test?do=pay&amount=2&action=default&_sec=ca909f', + $linkGenerator->link('Test:default', ['do' => 'pay', 'amount' => 2]) + ); + + Assert::same( + 'http://example.com/test?do=pay2&amount=1&action=default&_sec=410f1c', + $linkGenerator->link('Test:default', ['do' => 'pay2', 'amount' => 1]) + ); + + Assert::same( + 'http://example.com/test?do=pay2&amount=2&action=default&_sec=410f1c', // intentionally the same hash + $linkGenerator->link('Test:default', ['do' => 'pay2', 'amount' => 2]) + ); + } + + + /** + * @return \Nette\DI\Container + */ + private function createContainer() + { + $compiler = new Nette\DI\Compiler; + $compiler->addExtension('nette.http', new HttpExtension); + $compiler->addExtension('nette.http.sessions', new SessionExtension); + $compiler->addExtension('nette.application', new ApplicationExtension(TRUE, [__DIR__ . '/../fixtures'])); + $compiler->addExtension('nextras.securedLinks', new SecuredLinksExtension); + + eval($compiler->compile( + [ + 'services' => [new Nette\DI\Statement(Route::class, ['//example.com/'])] + ], + 'SecuredLinksExtensionContainer' + )); + + $sessionSection = Mockery::mock('alias:Nette\Http\SessionSection'); + $sessionSection->token = 'abcd'; + + $session = Mockery::mock('Nette\Http\Session'); + $session->shouldReceive('getSection')->with('Nextras.SecuredLinks')->andReturn($sessionSection); + $session->shouldReceive('getId')->times(8)->andReturn('session_id_1'); + + /** @var Nette\DI\Container $dic */ + $dic = new SecuredLinksExtensionContainer(); + $dic->removeService('nette.http.sessions.session'); + $dic->addService('nette.http.sessions.session', $session); + return $dic; + } +} + +(new SecuredLinksExtensionTest)->run(); diff --git a/tests/fixtures/TestPresenter.php b/tests/fixtures/TestPresenter.php new file mode 100644 index 0000000..f8399be --- /dev/null +++ b/tests/fixtures/TestPresenter.php @@ -0,0 +1,58 @@ +terminate(); + } + + + /** @secured */ + public function handlePay($amount) + { + } + + + /** @secured [amount] */ + public function handlePay2($amount) + { + } + + + /** @secured */ + public function handleList(array $sections) + { + } + + + /** + * @secured + */ + public function actionDelete() + { + + } + + + /** + * @return TestControl + */ + protected function createComponentMyControl() + { + return new TestControl(); + } +}