Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated Router to properly translate uri dashes that map to controller subdirs #4307

Merged
merged 7 commits into from
Mar 9, 2021
Merged

updated Router to properly translate uri dashes that map to controller subdirs #4307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
70ccd3e
updated Router to properly translate uri dashes that map to controlle…
sneakyimp Feb 19, 2021
3b3684a
changed Router::validateRequest to Router::scanControllers, added isV…
sneakyimp Feb 22, 2021
c1bf57d
PHPCBF cleanup
sneakyimp Feb 22, 2021
a06dc3d
added numerous dash tests, etc. to RouterTest
sneakyimp Feb 22, 2021
e47fa2a
Router fixes: fix errant return true in setDirectory, fix (bool) cast…
sneakyimp Mar 1, 2021
0570064
fixed broken assertions in RouterTest::testRouterPriorDirectory
sneakyimp Mar 1, 2021
a06f1f1
added space after boolean cast as requested by paulbalandan
sneakyimp Mar 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 75 additions & 17 deletions system/Router/Router.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -526,7 +532,6 @@ public function autoRoute(string $uri)
}

$defaultNamespace = $this->collection->getDefaultNamespace();
$controllerName = $this->controllerName();
if ($this->collection->getHTTPVerb() !== 'cli')
{
$controller = '\\' . $defaultNamespace;
Expand Down Expand Up @@ -573,32 +578,61 @@ 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
*
* @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
*
* @param array $segments URI segments
*
* @return array returns an array of remaining uri segments that don't map onto a directory
*/
protected function scanControllers(array $segments): array
sneakyimp marked this conversation as resolved.
Show resolved Hide resolved
{
$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;
}
Comment on lines +611 to +615
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? As this is untested, can you add a test case covering this possibility.

Copy link
Contributor Author

@sneakyimp sneakyimp Feb 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulbalandan thank you for your detailed review.

This particular bit of code is not something I've written. I don't know why (or even if) it's necessary. There might be a situation where someone wants to set Router::directory and just have all the segments represent controller/method/params?

I have reviewed the code and it looks like the only place that Router::directory ever gets set is in the Router::setDirectory method which is only called in two places. The first is right there in that same scanControllers/validateRequest function. The second is in system/Test/FeatureTestTrait.php. In that latter case, it gets set to null which would cause isset($this->directory) to return false:

$v = null;
var_dump(isset($v)); // outputs bool(false)

I do not know under what circumstances anyone might want to call setDirectory prior to calling scanControllers or autoRoute function. I can imagine that it might be possible to instantiate a Router object somewhere, set its directory, and want to feed it parameters, but I can't seem to figure out any situation where CodeIgniter does this or might do it? It might be possible for a single Router instance to call autoRoute or scanControllers twice, but I honestly don't know -- and I don't know why it would be called with a different $segments parameter the second time.

It was in there when I began working on this function, and it seemed safest to leave it in. On further reflection, being able to set an arbitrary directory location for a controller and then passing it arbitrary parameters seems unsafe from a security perspective.

If we want to create a test to make sure this code runs, this will do it:

	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');
		
		var_dump($router->directory());
		var_dump($router->controllerName());
		var_dump($router->methodName());
	}

Currently, the output of those last lines:

string(12) "foo/bar/baz/"
string(15) "Some_controller"
string(11) "some_method"

I could add this test and create assertions to expect those values, but I'm not certain why we would want to allow that behavior or if those are the desired values. Personally, I think it'd be safer to remove this check and force any autoRoute or scanControllers functionality to constrain code execution to the controllers folder.


// 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)
{
$test = $this->directory . ucfirst($this->translateURIDashes === true ? str_replace('-', '_', $segments[0]) : $segments[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(APPPATH . 'Controllers/' . $test . '.php') && $directoryOverride === false && is_dir(APPPATH . 'Controllers/' . $this->directory . ucfirst($segments[0])))
// 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(array_shift($segments), true);
$this->setDirectory($segmentConvert, true, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering. You added a third param to setDirectory optionally set to true but purposely set it to false here. As you have promoted a stricter check on the segments, no validation here seems a bit off. Or am I missing something?

array_shift($segments);
continue;
}

Expand All @@ -614,29 +648,53 @@ protected function validateRequest(array $segments): array
/**
* Sets the sub-directory that the controller is in.
*
* @param string|null $dir
* @param boolean|false $append
* @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)
public function setDirectory(string $dir = null, bool $append = false, bool $validate = true)
{
if (empty($dir))
{
$this->directory = null;
return;
}

$dir = ucfirst($dir);
if ($validate)
{
$segments = explode('/', trim($dir, '/'));
foreach ($segments as $segment)
{
if (! $this->isValidSegment($segment))
{
return;
sneakyimp marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Comment on lines +663 to +673
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition is not tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added RouterTest::testSetDirectoryValid and RouterTest::testSetDirectoryInvalid which test the setDirectory function to run this block of code for both valid and invalid directories.


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 boolean
*/
private function isValidSegment(string $segment): bool
{
return (bool) preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment);
}

//--------------------------------------------------------------------

/**
Expand Down
183 changes: 181 additions & 2 deletions tests/system/Router/RouterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\Test\CIUnitTestCase;
use Config\Modules;
use CodeIgniter\Exceptions\PageNotFoundException;

class RouterTest extends CIUnitTestCase
{
Expand Down Expand Up @@ -81,9 +82,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');
}

//--------------------------------------------------------------------
Expand Down Expand Up @@ -231,6 +232,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);
Expand Down Expand Up @@ -575,4 +723,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('foo/bar/baz/', $router->directory());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sneakyimp Why is this test necessary?
Why does setDirectory() take precedence over URI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found your answer.
https://github.com/codeigniter4/CodeIgniter4/pull/4307/files#r584375184

$this->assertEquals('Some_controller', $router->controllerName());
$this->assertEquals('some_method', $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());
}
}