From 70ccd3e8ebb707394a14e026b20c2432fac15ab1 Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Thu, 18 Feb 2021 21:55:22 -0800 Subject: [PATCH 1/7] updated Router to properly translate uri dashes that map to controller subdirectories. fix issue #4294 --- system/Router/Router.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index 1c61d121940d..cdbf46cb38a6 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -594,11 +594,13 @@ protected function validateRequest(array $segments): array // is found or when such a directory doesn't exist while ($c-- > 0) { - $test = $this->directory . ucfirst($this->translateURIDashes === true ? str_replace('-', '_', $segments[0]) : $segments[0]); + $segmentConvert = ucfirst($this->translateURIDashes === true ? str_replace('-', '_', $segments[0]) : $segments[0]); + $test = APPPATH . 'Controllers/' . $this->directory . $segmentConvert; - if (! is_file(APPPATH . 'Controllers/' . $test . '.php') && $directoryOverride === false && is_dir(APPPATH . 'Controllers/' . $this->directory . ucfirst($segments[0]))) + if (! is_file($test . '.php') && $directoryOverride === false && is_dir($test)) { - $this->setDirectory(array_shift($segments), true); + $this->setDirectory($segmentConvert, true); + array_shift($segments); continue; } From 3b3684ade8709e2f59d03e02c32cbe38ae27d116 Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Mon, 22 Feb 2021 14:38:11 -0800 Subject: [PATCH 2/7] changed Router::validateRequest to Router::scanControllers, added isValidSegment, tweaks to enforce PSR4 compliance in directory/namespace segments --- system/Router/Router.php | 75 +++++++++++++++++++++++------- tests/system/Router/RouterTest.php | 6 ++- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index cdbf46cb38a6..01a4b7c63538 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -498,7 +498,7 @@ public function autoRoute(string $uri) { $segments = explode('/', $uri); - $segments = $this->validateRequest($segments); + $segments = $this->scanControllers($segments); // If we don't have any segments left - try the default controller; // WARNING: Directories get shifted out of the segments array. @@ -512,6 +512,12 @@ public function autoRoute(string $uri) $this->controller = ucfirst(array_shift($segments)); } + $controllerName = $this->controllerName(); + if (!$this->isValidSegment($controllerName)) + { + throw new PageNotFoundException($this->controller . " is not a valid controller name"); + } + // Use the method name if it exists. // If it doesn't, no biggie - the default method name // has already been set. @@ -526,7 +532,6 @@ public function autoRoute(string $uri) } $defaultNamespace = $this->collection->getDefaultNamespace(); - $controllerName = $this->controllerName(); if ($this->collection->getHTTPVerb() !== 'cli') { $controller = '\\' . $defaultNamespace; @@ -573,33 +578,45 @@ public function autoRoute(string $uri) //-------------------------------------------------------------------- /** - * Attempts to validate the URI request and determine the controller path. + * Scans the controller directory, attempting to locate a controller matching the supplied uri $segments * * @param array $segments URI segments * - * @return array URI segments + * @return array returns an array of remaining uri segments that don't map onto a directory */ - protected function validateRequest(array $segments): array + protected function scanControllers(array $segments): array { $segments = array_filter($segments, function ($segment) { - // @phpstan-ignore-next-line - return ! empty($segment) || ($segment !== '0' || $segment !== 0); + return $segment !== ''; }); + // numerically reindex the array, removing gaps $segments = array_values($segments); - $c = count($segments); - $directoryOverride = isset($this->directory); + + // if a prior directory value has been set, just return segments and get out of here + if (isset($this->directory)) + { + return $segments; + } + // Loop through our segments and return as soon as a controller // is found or when such a directory doesn't exist + $c = count($segments); while ($c-- > 0) { $segmentConvert = ucfirst($this->translateURIDashes === true ? str_replace('-', '_', $segments[0]) : $segments[0]); + // as soon as we encounter any segment that is not PSR-4 compliant, stop searching + if (!$this->isValidSegment($segmentConvert)) { + return $segments; + } + $test = APPPATH . 'Controllers/' . $this->directory . $segmentConvert; - if (! is_file($test . '.php') && $directoryOverride === false && is_dir($test)) + // as long as each segment is *not* a controller file but does match a directory, add it to $this->directory + if (! is_file($test . '.php') && is_dir($test)) { - $this->setDirectory($segmentConvert, true); + $this->setDirectory($segmentConvert, true, false); array_shift($segments); continue; } @@ -617,26 +634,50 @@ protected function validateRequest(array $segments): array * Sets the sub-directory that the controller is in. * * @param string|null $dir - * @param boolean|false $append + * @param boolean $append + * @param boolean $validate if true, checks to make sure $dir consists of only PSR4 compliant segments */ - public function setDirectory(string $dir = null, bool $append = false) + public function setDirectory(string $dir = null, bool $append = false, bool $validate = true) { if (empty($dir)) { $this->directory = null; - return; + return true; } - $dir = ucfirst($dir); + if ($validate) + { + $segments = explode("/", trim($dir, "/")); + foreach($segments as $segment) + { + if (!$this->isValidSegment($segment)) + { + return; + } + } + } if ($append !== true || empty($this->directory)) { - $this->directory = str_replace('.', '', trim($dir, '/')) . '/'; + $this->directory = trim($dir, "/") . "/"; } else { - $this->directory .= str_replace('.', '', trim($dir, '/')) . '/'; + $this->directory .= trim($dir, "/") . "/"; } + + } + + /** + * Returns true if the supplied $segment string represents a valid PSR-4 compliant namespace/directory segment + * + * regex comes from https://www.php.net/manual/en/language.variables.basics.php + * + * @param string $segment + * @return bool + */ + private function isValidSegment(string $segment): bool { + return (boolean)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); } //-------------------------------------------------------------------- diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index 221bca8488e4..f7471bcb2a57 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -5,6 +5,8 @@ use CodeIgniter\HTTP\IncomingRequest; use CodeIgniter\Test\CIUnitTestCase; use Config\Modules; +use CodeIgniter\Exceptions\PageNotFoundException; + class RouterTest extends CIUnitTestCase { @@ -81,9 +83,9 @@ public function testZeroAsURIPath() { $router = new Router($this->collection, $this->request); - $router->handle('0'); + $this->expectException(PageNotFoundException::class); - $this->assertEquals('0', $router->controllerName()); + $router->handle('0'); } //-------------------------------------------------------------------- From c1bf57dfbe75315654cbc130b3dc1c107692614d Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Mon, 22 Feb 2021 14:39:00 -0800 Subject: [PATCH 3/7] PHPCBF cleanup --- system/Router/Router.php | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index 01a4b7c63538..e429738b18bf 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -512,10 +512,10 @@ public function autoRoute(string $uri) $this->controller = ucfirst(array_shift($segments)); } - $controllerName = $this->controllerName(); - if (!$this->isValidSegment($controllerName)) + $controllerName = $this->controllerName(); + if (! $this->isValidSegment($controllerName)) { - throw new PageNotFoundException($this->controller . " is not a valid controller name"); + throw new PageNotFoundException($this->controller . ' is not a valid controller name'); } // Use the method name if it exists. @@ -592,26 +592,25 @@ protected function scanControllers(array $segments): array // numerically reindex the array, removing gaps $segments = array_values($segments); - // if a prior directory value has been set, just return segments and get out of here if (isset($this->directory)) { return $segments; } - // Loop through our segments and return as soon as a controller // is found or when such a directory doesn't exist - $c = count($segments); + $c = count($segments); while ($c-- > 0) { $segmentConvert = ucfirst($this->translateURIDashes === true ? str_replace('-', '_', $segments[0]) : $segments[0]); // as soon as we encounter any segment that is not PSR-4 compliant, stop searching - if (!$this->isValidSegment($segmentConvert)) { + if (! $this->isValidSegment($segmentConvert)) + { return $segments; } - $test = APPPATH . 'Controllers/' . $this->directory . $segmentConvert; + $test = APPPATH . 'Controllers/' . $this->directory . $segmentConvert; // as long as each segment is *not* a controller file but does match a directory, add it to $this->directory if (! is_file($test . '.php') && is_dir($test)) @@ -633,9 +632,9 @@ protected function scanControllers(array $segments): array /** * Sets the sub-directory that the controller is in. * - * @param string|null $dir - * @param boolean $append - * @param boolean $validate if true, checks to make sure $dir consists of only PSR4 compliant segments + * @param string|null $dir + * @param boolean $append + * @param boolean $validate if true, checks to make sure $dir consists of only PSR4 compliant segments */ public function setDirectory(string $dir = null, bool $append = false, bool $validate = true) { @@ -647,10 +646,10 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val if ($validate) { - $segments = explode("/", trim($dir, "/")); - foreach($segments as $segment) + $segments = explode('/', trim($dir, '/')); + foreach ($segments as $segment) { - if (!$this->isValidSegment($segment)) + if (! $this->isValidSegment($segment)) { return; } @@ -659,13 +658,12 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val if ($append !== true || empty($this->directory)) { - $this->directory = trim($dir, "/") . "/"; + $this->directory = trim($dir, '/') . '/'; } else { - $this->directory .= trim($dir, "/") . "/"; + $this->directory .= trim($dir, '/') . '/'; } - } /** @@ -673,10 +671,11 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val * * regex comes from https://www.php.net/manual/en/language.variables.basics.php * - * @param string $segment - * @return bool + * @param string $segment + * @return boolean */ - private function isValidSegment(string $segment): bool { + private function isValidSegment(string $segment): bool + { return (boolean)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); } From a06dc3d14af161353da3fbbed1f4a2ed2ad83e70 Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Mon, 22 Feb 2021 15:36:58 -0800 Subject: [PATCH 4/7] added numerous dash tests, etc. to RouterTest --- tests/system/Router/RouterTest.php | 147 +++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index f7471bcb2a57..3a322bf04edb 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -233,6 +233,153 @@ public function testAutoRouteFindsControllerWithSubfolder() //-------------------------------------------------------------------- + public function testAutoRouteFindsDashedSubfolder() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + mkdir(APPPATH . 'Controllers/Dash_folder'); + + $router->autoRoute('dash-folder/mycontroller/somemethod'); + + rmdir(APPPATH . 'Controllers/Dash_folder'); + + $this->assertEquals('Dash_folder/', $router->directory()); + $this->assertEquals('Mycontroller', $router->controllerName()); + $this->assertEquals('somemethod', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteFindsDashedController() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + mkdir(APPPATH . 'Controllers/Dash_folder'); + file_put_contents(APPPATH . 'Controllers/Dash_folder/Dash_controller.php', ''); + + $router->autoRoute('dash-folder/dash-controller/somemethod'); + + unlink(APPPATH . 'Controllers/Dash_folder/Dash_controller.php'); + rmdir(APPPATH . 'Controllers/Dash_folder'); + + $this->assertEquals('Dash_folder/', $router->directory()); + $this->assertEquals('Dash_controller', $router->controllerName()); + $this->assertEquals('somemethod', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteFindsDashedMethod() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + mkdir(APPPATH . 'Controllers/Dash_folder'); + file_put_contents(APPPATH . 'Controllers/Dash_folder/Dash_controller.php', ''); + + $router->autoRoute('dash-folder/dash-controller/dash-method'); + + unlink(APPPATH . 'Controllers/Dash_folder/Dash_controller.php'); + rmdir(APPPATH . 'Controllers/Dash_folder'); + + $this->assertEquals('Dash_folder/', $router->directory()); + $this->assertEquals('Dash_controller', $router->controllerName()); + $this->assertEquals('dash_method', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteFindsDefaultDashFolder() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + mkdir(APPPATH . 'Controllers/Dash_folder'); + + $router->autoRoute('dash-folder'); + + rmdir(APPPATH . 'Controllers/Dash_folder'); + + $this->assertEquals('Dash_folder/', $router->directory()); + $this->assertEquals('Home', $router->controllerName()); + $this->assertEquals('index', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteFindsMByteDir() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + mkdir(APPPATH . 'Controllers/Φ'); + + $router->autoRoute('Φ'); + + rmdir(APPPATH . 'Controllers/Φ'); + + $this->assertEquals('Φ/', $router->directory()); + $this->assertEquals('Home', $router->controllerName()); + $this->assertEquals('index', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteFindsMByteController() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + file_put_contents(APPPATH . 'Controllers/Φ', ''); + + $router->autoRoute('Φ'); + + unlink(APPPATH . 'Controllers/Φ'); + + $this->assertEquals('Φ', $router->controllerName()); + $this->assertEquals('index', $router->methodName()); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteRejectsSingleDot() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + $this->expectException(PageNotFoundException::class); + + $router->autoRoute('.'); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteRejectsDoubleDot() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + $this->expectException(PageNotFoundException::class); + + $router->autoRoute('..'); + } + + //-------------------------------------------------------------------- + + public function testAutoRouteRejectsMidDot() + { + $router = new Router($this->collection, $this->request); + $router->setTranslateURIDashes(true); + + $this->expectException(PageNotFoundException::class); + + $router->autoRoute('Foo.bar'); + } + + //-------------------------------------------------------------------- + public function testDetectsLocales() { $router = new Router($this->collection, $this->request); From e47fa2aced9b4f70a0e20ae1a531e1531d59b597 Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Sun, 28 Feb 2021 16:36:53 -0800 Subject: [PATCH 5/7] Router fixes: fix errant return true in setDirectory, fix (bool) cast in isValidSegment, add back deprecated validateRequest function, add RouterTest methods for better code coverage --- system/Router/Router.php | 20 +++++++++++++++++-- tests/system/Router/RouterTest.php | 31 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index e429738b18bf..e5eb31e61a2e 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -577,6 +577,22 @@ public function autoRoute(string $uri) //-------------------------------------------------------------------- + /** + * Scans the controller directory, attempting to locate a controller matching the supplied uri $segments + * + * @param array $segments URI segments + * + * @return array returns an array of remaining uri segments that don't map onto a directory + * + * @deprecated this function name does not properly describe its behavior so it has been deprecated + */ + protected function validateRequest(array $segments): array + { + return $this->scanControllers($segments); + } + + //-------------------------------------------------------------------- + /** * Scans the controller directory, attempting to locate a controller matching the supplied uri $segments * @@ -641,7 +657,7 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val if (empty($dir)) { $this->directory = null; - return true; + return; } if ($validate) @@ -676,7 +692,7 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val */ private function isValidSegment(string $segment): bool { - return (boolean)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); + return (bool)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); } //-------------------------------------------------------------------- diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index 3a322bf04edb..ac15851b818f 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -724,4 +724,35 @@ public function testRegularExpressionPlaceholderWithUnicode() ]; $this->assertEquals($expected, $router->params()); } + + public function testRouterPriorDirectory() + { + $router = new Router($this->collection, $this->request); + + $router->setDirectory('foo/bar/baz', false, true); + $router->handle('Some_controller/some_method/param1/param2/param3'); + + $this->assertEquals('', $router->directory()); + $this->assertEquals('', $router->controllerName()); + $this->assertEquals('', $router->methodName()); + } + + public function testSetDirectoryValid() + { + $router = new Router($this->collection, $this->request); + $router->setDirectory('foo/bar/baz', false, true); + + $this->assertEquals('foo/bar/baz/', $router->directory()); + } + + public function testSetDirectoryInvalid() + { + $router = new Router($this->collection, $this->request); + $router->setDirectory('foo/bad-segment/bar', false, true); + + $internal = $this->getPrivateProperty($router, 'directory'); + + $this->assertNull($internal); + $this->assertEquals('', $router->directory()); + } } From 057006453ddaff780b5fad529ffbfb9995e74e73 Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Sun, 28 Feb 2021 16:53:54 -0800 Subject: [PATCH 6/7] fixed broken assertions in RouterTest::testRouterPriorDirectory --- tests/system/Router/RouterTest.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index ac15851b818f..e8b73e3adcf1 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -7,7 +7,6 @@ use Config\Modules; use CodeIgniter\Exceptions\PageNotFoundException; - class RouterTest extends CIUnitTestCase { @@ -732,9 +731,9 @@ public function testRouterPriorDirectory() $router->setDirectory('foo/bar/baz', false, true); $router->handle('Some_controller/some_method/param1/param2/param3'); - $this->assertEquals('', $router->directory()); - $this->assertEquals('', $router->controllerName()); - $this->assertEquals('', $router->methodName()); + $this->assertEquals('foo/bar/baz/', $router->directory()); + $this->assertEquals('Some_controller', $router->controllerName()); + $this->assertEquals('some_method', $router->methodName()); } public function testSetDirectoryValid() From a06f1f174a18d48139b38a42d81eb23cbb6e069b Mon Sep 17 00:00:00 2001 From: "J. Adams" Date: Fri, 5 Mar 2021 18:04:37 -0800 Subject: [PATCH 7/7] added space after boolean cast as requested by paulbalandan --- system/Router/Router.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index e5eb31e61a2e..e1560b0d4bd6 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -692,7 +692,7 @@ public function setDirectory(string $dir = null, bool $append = false, bool $val */ private function isValidSegment(string $segment): bool { - return (bool)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); + return (bool) preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment); } //--------------------------------------------------------------------