From 697ad6a86d3560eeb3d38bea2472fe2ee0d008ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 20 Apr 2018 14:11:06 +0200 Subject: [PATCH 1/3] generated resource routes should allow / --- .../AppFramework/Routing/RouteConfig.php | 2 +- tests/lib/Route/RouterTest.php | 49 ++++++++++++++++++- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index 654cad8b0238..6f5a7aad36e8 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -221,7 +221,7 @@ private function processResources($routes) $this->router->create($routeName, $url)->method($verb)->action( new RouteActionHandler($this->container, $controllerName, $actionName) - ); + )->requirements(['id' => '.+']); // allow / in {id} parameter } } } diff --git a/tests/lib/Route/RouterTest.php b/tests/lib/Route/RouterTest.php index 4243b876f7ab..a7ec902be1ea 100644 --- a/tests/lib/Route/RouterTest.php +++ b/tests/lib/Route/RouterTest.php @@ -1,5 +1,6 @@ * @author Thomas Müller * * @copyright Copyright (c) 2018, ownCloud GmbH @@ -25,16 +26,41 @@ use OC\Route\Router; use OCP\ILogger; + +class LoadableRouter extends Router { + /** + * @param bool $loaded + */ + public function setLoaded($loaded) { + $this->loaded = $loaded; + } +} + class RouterTest extends \Test\TestCase { + + /** @var ILogger */ + private $l; + + /** + * RouterTest constructor. + * + * @param string $name + * @param array $data + * @param string $dataName + */ + public function __construct($name = null, array $data = [], $dataName = '') { + parent::__construct($name, $data, $dataName); + $this->l = $this->createMock(ILogger::class); + } + /** * @dataProvider providesWebRoot * @param $expectedBase * @param $webRoot */ public function testWebRootSetup($expectedBase, $webRoot) { - $l = $this->createMock(ILogger::class); - $router = new Router($l, $webRoot); + $router = new Router($this->l, $webRoot); $this->assertEquals($expectedBase, $router->getGenerator()->getContext()->getBaseUrl()); } @@ -47,4 +73,23 @@ public function providesWebRoot() { ['/oc/index.php', '/oc/'], ]; } + + public function testMatchURLParamContainingSlash() { + $router = new LoadableRouter($this->l, ''); + + $called = false; + + $router->useCollection('root'); + $router->create('test', '/resource/{id}') + ->action(function() use (&$called) { + $called = true; + })->requirements(['id' => '.+']); + + // don't load any apps + $router->setLoaded(true); + + $router->match('/resource/id%2Fwith%2Fslashes'); + + self::assertTrue($called); + } } From be0614a0cccfe552f01d935ecfca5389edb0d3dc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 19 Jun 2018 15:14:46 +0200 Subject: [PATCH 2/3] Add unit tests for slash routes --- tests/lib/Route/RouterTest.php | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/lib/Route/RouterTest.php b/tests/lib/Route/RouterTest.php index a7ec902be1ea..e147ecf13eb1 100644 --- a/tests/lib/Route/RouterTest.php +++ b/tests/lib/Route/RouterTest.php @@ -25,6 +25,7 @@ use OC\Route\Router; use OCP\ILogger; +use Symfony\Component\Routing\Exception\ResourceNotFoundException; class LoadableRouter extends Router { @@ -74,13 +75,16 @@ public function providesWebRoot() { ]; } - public function testMatchURLParamContainingSlash() { + /** + * @dataProvider urlParamSlashProvider + */ + public function testMatchURLParamContainingSlash($routeUrl, $matchUrl, $expectedCalled) { $router = new LoadableRouter($this->l, ''); $called = false; $router->useCollection('root'); - $router->create('test', '/resource/{id}') + $router->create('test', $routeUrl) ->action(function() use (&$called) { $called = true; })->requirements(['id' => '.+']); @@ -88,8 +92,22 @@ public function testMatchURLParamContainingSlash() { // don't load any apps $router->setLoaded(true); - $router->match('/resource/id%2Fwith%2Fslashes'); + try { + $router->match($matchUrl); + } catch (ResourceNotFoundException $e) { + $called = false; + } + + self::assertEquals($expectedCalled, $called); + } - self::assertTrue($called); + public function urlParamSlashProvider() { + return [ + ['/resource/{id}', '/resource/id%2Fwith%2Fslashes', true], + ['/resource/{id}/sub', '/resource/id%2Fwith%2Fslashes/sub', true], + ['/resource/{id}/sub', '/resource/id%2Fwith%2Fslashes/subx', false], + ['/resource/{id}', '/resource/id/with/slashes', true], + ['/resource/{id}/sub', '/resource/id/with/slashes/sub', true], + ]; } } From 69a3d22185ea2089da36fe735d10747a41bce6c5 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 26 Jun 2018 16:36:25 +0200 Subject: [PATCH 3/3] Add unit test for routes when no slashes allowed --- tests/lib/Route/RouterTest.php | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/tests/lib/Route/RouterTest.php b/tests/lib/Route/RouterTest.php index e147ecf13eb1..a3e4e8076328 100644 --- a/tests/lib/Route/RouterTest.php +++ b/tests/lib/Route/RouterTest.php @@ -27,7 +27,6 @@ use OCP\ILogger; use Symfony\Component\Routing\Exception\ResourceNotFoundException; - class LoadableRouter extends Router { /** * @param bool $loaded @@ -39,7 +38,6 @@ public function setLoaded($loaded) { class RouterTest extends \Test\TestCase { - /** @var ILogger */ private $l; @@ -78,16 +76,19 @@ public function providesWebRoot() { /** * @dataProvider urlParamSlashProvider */ - public function testMatchURLParamContainingSlash($routeUrl, $matchUrl, $expectedCalled) { + public function testMatchURLParamContainingSlash($routeUrl, $slashesAllowed, $matchUrl, $expectedCalled) { $router = new LoadableRouter($this->l, ''); $called = false; $router->useCollection('root'); - $router->create('test', $routeUrl) - ->action(function() use (&$called) { - $called = true; - })->requirements(['id' => '.+']); + $route = $router->create('test', $routeUrl) + ->action(function () use (&$called) { + $called = true; + }); + if ($slashesAllowed) { + $route->requirements(['id' => '.+']); + } // don't load any apps $router->setLoaded(true); @@ -103,11 +104,19 @@ public function testMatchURLParamContainingSlash($routeUrl, $matchUrl, $expected public function urlParamSlashProvider() { return [ - ['/resource/{id}', '/resource/id%2Fwith%2Fslashes', true], - ['/resource/{id}/sub', '/resource/id%2Fwith%2Fslashes/sub', true], - ['/resource/{id}/sub', '/resource/id%2Fwith%2Fslashes/subx', false], - ['/resource/{id}', '/resource/id/with/slashes', true], - ['/resource/{id}/sub', '/resource/id/with/slashes/sub', true], + // slashed disallowed + ['/resource/{id}', false, '/resource/id%2Fwith%2Fslashes', false], + ['/resource/{id}/sub', false, '/resource/id%2Fwith%2Fslashes/sub', false], + ['/resource/{id}/sub', false, '/resource/id%2Fwith%2Fslashes/subx', false], + ['/resource/{id}', false, '/resource/id/with/slashes', false], + ['/resource/{id}/sub', false, '/resource/id/with/slashes/sub', false], + + // slashed allowed + ['/resource/{id}', true, '/resource/id%2Fwith%2Fslashes', true], + ['/resource/{id}/sub', true, '/resource/id%2Fwith%2Fslashes/sub', true], + ['/resource/{id}/sub', true, '/resource/id%2Fwith%2Fslashes/subx', false], + ['/resource/{id}', true, '/resource/id/with/slashes', true], + ['/resource/{id}/sub', true, '/resource/id/with/slashes/sub', true], ]; } }