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

sneakyimp · 2021-02-19T06:02:50Z

Description
I have made a minor tweak to CodeIgniter\Router::validateRequest so that dashes in a subdirectory are mapped to underscores if translateURIDashes= true. This appears to correctly auto-route requests when you have a dash in the URI that corresponds to a controller directory.

Checklist:

sneakyimp · 2021-02-19T17:35:20Z

@paulbalandan @kenjis @michalsn Could someone review this PR to fix #4294?

paulbalandan

I've looked at the changes and it looks good to me. I agree with the changes you've made. I have a minor cosmetic request. Change $segmentConvert to simply $segment. :)

I'll check first if this method is covered by tests or not before approving this for merge.

paulbalandan · 2021-02-19T18:31:08Z

Can you add a test case for this one similar to here https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Router/RouterTest.php#L215-L227 but using the underscored subfolder as in your case. It should assert that it will still get the correct controller and method.

sneakyimp · 2021-02-19T19:12:56Z

Thank you, @paulbalandan!

I've looked at the changes and it looks good to me. I agree with the changes you've made. I have a minor cosmetic request. Change $segmentConvert to simply $segment. :)

Please note that I chose a different variable name to distinguish this value from the raw segments referred to elsewhere. It was my feeling that this more clearly indicated that the segment had been changed with ucfirst and with the dash-to-underscore replacement. I'd like to keep it because I think it makes the intent of the code much clearer. It was pretty difficult to understand the intent of this code because there are so few comments and the variables and functions are poorly named. In particular, the function validateRequest is very poorly named. I'd very much like to give that function a different name but have not done so in order to expedite matters. Please let me know if I may keep this other var name for greater clarity?

I have another question: may I remove the ucfirst call in setDirectory? This function is only called in the validateRequest function, where I have already applied ucfirst to the supplied parameter, and in system/Test/FeatureTestTrait.php where the supplied parameter is null.

I'll check first if this method is covered by tests or not before approving this for merge.

I have run the phpunit test and it passed all the previous tests, but please do run any additional tests:

vendor/phpunit/phpunit/phpunit tests/system/Router

I'll work on adding another test as you directed.

paulbalandan · 2021-02-19T19:44:00Z

The tests run pretty smoothly and in fact the coverage increased for Router so that's good. But since we're fixing a bug here we'll want to document it that the fix has been well tested.

sneakyimp · 2021-02-23T00:22:14Z

@paulbalandan I have committed another set of changes. NOTE that it implements more stringent enforcement of PSR-4 compliance in that it will throw PageNotFoundExceptions for controller names which are not acceptable PHP class names. This has the benefit of rejecting any .. or . attempts which may sneak into the Router but it will also reject controller names with dashes or periods or puncutation, etc. This should make the Router a bit more efficient because it eliminates a fair amount of additional logic that was getting executed.

Note that I did change RouterTest::testZeroAsURIPath() to expect an Exception instead of passing. A zero is not a valid Controller name -- you can't declare a class 0 {}; in PHP nor can you use a zero in any namespace declaration.

system/Router/Router.php

paulbalandan · 2021-02-28T14:46:18Z

system/Router/Router.php

+		// if a prior directory value has been set, just return segments and get out of here
+		if (isset($this->directory))
+		{
+			return $segments;
+		}


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

@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.

paulbalandan · 2021-02-28T14:49:07Z

system/Router/Router.php

+		if ($validate)
+		{
+			$segments = explode('/', trim($dir, '/'));
+			foreach ($segments as $segment)
+			{
+				if (! $this->isValidSegment($segment))
+				{
+					return;
+				}
+			}
+		}


This addition is not tested.

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.

system/Router/Router.php

sneakyimp · 2021-03-01T06:48:22Z

@paulbalandan I've pushed another commit and I think it resolves the issues you mentioned in your review -- for some reason github only offers me a "resolve conversation" button rather than continuing our discussion. I hope you will please take a look and let me know if you have further concerns.

paulbalandan · 2021-03-01T07:09:51Z

system/Router/Router.php

+	 */
+	private function isValidSegment(string $segment): bool
+	{
+		return (bool)preg_match('/^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$/', $segment);


CS: Add space after the cast. (bool) preg_match

paulbalandan · 2021-03-01T07:14:34Z

system/Router/Router.php

 			{
-				$this->setDirectory(array_shift($segments), true);
+				$this->setDirectory($segmentConvert, true, false);


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?

sneakyimp · 2021-03-06T02:06:44Z

@paulbalandan sorry I'm slow to respond. For some reason github isn't letting me respond directly under your review comment so I'm commenting here.

CS: Add space after the cast. (bool) preg_match

I have added the space, committed to my local repo, rebased on the latest develop branch of CI4, and pushed. Please let me know if you have additional requests, questions, or issues.

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?

By default, setDirectory has $validate=true. The only reason I set it to $validate=false when I call it in scanController is because I have already validated $segmentConvert a few lines before. My thinking was that it would be inefficient to validate the segment twice. There might be some better way to do this -- I considered having setDirectory return a boolean depending on whether the validation succeded or not, but setDirectory is a public method, and this would probably have introduced a Breaking Change.

sneakyimp · 2021-03-06T03:26:56Z

I have no idea why these checks failed. They seem totally unrelated to my PR.

kenjis · 2021-03-06T04:13:05Z

It is probably because ubuntu is now updated to 20.04.
But I don't know what's wrong.

paulbalandan · 2021-03-06T04:36:07Z

Ubuntu latest is now 20.04 so only 7.4+ is built in in the image. We need to specify the extensions we need for lower php versions. I'll send a PR.

paulbalandan · 2021-03-08T13:00:07Z

Please rebase,

…r subdirectories. fix issue codeigniter4#4294

…alidSegment, tweaks to enforce PSR4 compliance in directory/namespace segments

… in isValidSegment, add back deprecated validateRequest function, add RouterTest methods for better code coverage

sneakyimp

Please rebase,

I have rebased and force-pushed. Please let me know if you have any additional thoughts.

sneakyimp · 2021-02-28T22:58:08Z

system/Router/Router.php

+		if ($validate)
+		{
+			$segments = explode('/', trim($dir, '/'));
+			foreach ($segments as $segment)
+			{
+				if (! $this->isValidSegment($segment))
+				{
+					return;
+				}
+			}
+		}


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.

kenjis · 2022-04-08T03:22:44Z

tests/system/Router/RouterTest.php

+		$router->setDirectory('foo/bar/baz', false, true);
+		$router->handle('Some_controller/some_method/param1/param2/param3');
+
+		$this->assertEquals('foo/bar/baz/', $router->directory());


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

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

sneakyimp mentioned this pull request Feb 19, 2021

Bug 4069 - query returns Result even when query result is FALSE #4176

Merged

5 tasks

paulbalandan reviewed Feb 19, 2021

View reviewed changes

sneakyimp force-pushed the bugs/issue-4294 branch from 406f845 to 8176e9c Compare February 22, 2021 22:40

kenjis reviewed Feb 28, 2021

View reviewed changes

system/Router/Router.php Outdated Show resolved Hide resolved

kenjis reviewed Feb 28, 2021

View reviewed changes

system/Router/Router.php Outdated Show resolved Hide resolved

paulbalandan reviewed Feb 28, 2021

View reviewed changes

sneakyimp force-pushed the bugs/issue-4294 branch from 9ee8ecb to f1dc863 Compare March 1, 2021 00:48

paulbalandan reviewed Mar 1, 2021

View reviewed changes

sneakyimp force-pushed the bugs/issue-4294 branch from 185619b to 9e0c7b3 Compare March 6, 2021 02:05

sneakyimp added 7 commits March 8, 2021 12:17

updated Router to properly translate uri dashes that map to controlle…

70ccd3e

…r subdirectories. fix issue codeigniter4#4294

changed Router::validateRequest to Router::scanControllers, added isV…

3b3684a

…alidSegment, tweaks to enforce PSR4 compliance in directory/namespace segments

PHPCBF cleanup

c1bf57d

added numerous dash tests, etc. to RouterTest

a06dc3d

Router fixes: fix errant return true in setDirectory, fix (bool) cast…

e47fa2a

… in isValidSegment, add back deprecated validateRequest function, add RouterTest methods for better code coverage

fixed broken assertions in RouterTest::testRouterPriorDirectory

0570064

added space after boolean cast as requested by paulbalandan

a06f1f1

sneakyimp force-pushed the bugs/issue-4294 branch from 9e0c7b3 to a06f1f1 Compare March 8, 2021 20:18

sneakyimp commented Mar 8, 2021

View reviewed changes

paulbalandan approved these changes Mar 9, 2021

View reviewed changes

paulbalandan merged commit 3fd3863 into codeigniter4:develop Mar 9, 2021

kenjis reviewed Apr 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sneakyimp commented Feb 19, 2021 •

edited by paulbalandan

Loading

sneakyimp commented Feb 19, 2021

paulbalandan left a comment

paulbalandan commented Feb 19, 2021

sneakyimp commented Feb 19, 2021

paulbalandan commented Feb 19, 2021

sneakyimp commented Feb 23, 2021

paulbalandan Feb 28, 2021

sneakyimp Feb 28, 2021 •

edited

Loading

paulbalandan Feb 28, 2021

sneakyimp Feb 28, 2021

sneakyimp commented Mar 1, 2021

paulbalandan Mar 1, 2021

paulbalandan Mar 1, 2021

sneakyimp commented Mar 6, 2021

sneakyimp commented Mar 6, 2021

kenjis commented Mar 6, 2021

paulbalandan commented Mar 6, 2021

paulbalandan commented Mar 8, 2021

sneakyimp left a comment

sneakyimp Feb 28, 2021

kenjis Apr 8, 2022

kenjis Apr 8, 2022

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

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

Conversation

sneakyimp commented Feb 19, 2021 • edited by paulbalandan Loading

sneakyimp commented Feb 19, 2021

paulbalandan left a comment

Choose a reason for hiding this comment

paulbalandan commented Feb 19, 2021

sneakyimp commented Feb 19, 2021

paulbalandan commented Feb 19, 2021

sneakyimp commented Feb 23, 2021

Choose a reason for hiding this comment

sneakyimp Feb 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneakyimp commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneakyimp commented Mar 6, 2021

sneakyimp commented Mar 6, 2021

kenjis commented Mar 6, 2021

paulbalandan commented Mar 6, 2021

paulbalandan commented Mar 8, 2021

sneakyimp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneakyimp commented Feb 19, 2021 •

edited by paulbalandan

Loading

sneakyimp Feb 28, 2021 •

edited

Loading