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.

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

Fix #2704: ensure route registered via $routes->cli() not accessible via web browser even autoroute is true #2707

Merged
merged 12 commits into from
Mar 22, 2020

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Mar 15, 2020

Fix #2704: ensure route registered via $routes->cli() not accessible via web browser even autoroute is true.

Checklist:

  • Securely signed commits
  • Unit testing, with >80% coverage

@samsonasik
Copy link
Member Author

samsonasik commented Mar 15, 2020

Before :
Screen Shot 2020-03-15 at 5 58 15 PM

After:
Screen Shot 2020-03-19 at 7 09 21 PM

@samsonasik
Copy link
Member Author

samsonasik commented Mar 15, 2020

Not sure how to write unit test for it I've added test for it. I included screenshot before and after changed above.

@samsonasik
Copy link
Member Author

@MGatner I've added feature test case for demoing 404 page thrown when open cli routes from http

@samsonasik
Copy link
Member Author

it seems need some autoload tweak for testing "App" namespace for feature test demo, I will re-check again.

@samsonasik
Copy link
Member Author

The test tweak implemented.

@samsonasik
Copy link
Member Author

@@MGatner travis build green 🎉

{
if (is_string($route))
{
if (strpos($route, $controller . '::' . $methodName) === 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

using strpos is on purpose to handle parameter, eg: controller::method/$1:

$routes->cli('hello/(:any)', 'Commands\Hello::index/$1');

@samsonasik
Copy link
Member Author

I've added feature test case for demoing 404 page thrown when open parameterized cli routes from http

@samsonasik
Copy link
Member Author

I've added condition check for default method index when routing defined in cli as controller name only, which fallback to method index, eg:

$routes->cli('hello', 'Hello');

@samsonasik
Copy link
Member Author

I've added condition for multi case, eg: capitalized controller or method in URI with compare with strtolower of the controller and method.

@samsonasik samsonasik changed the title Fix #2704: ensure route registered via $routes->cli() not accessible ia web browser even autoroute is true Fix #2704: ensure route registered via $routes->cli() not accessible via web browser even autoroute is true Mar 19, 2020
@samsonasik
Copy link
Member Author

travis build green 🎉

@lonnieezell lonnieezell merged commit 5ae1695 into codeigniter4:develop Mar 22, 2020
@samsonasik samsonasik deleted the fix-2704 branch March 22, 2020 03:09
*/
public function testOpenCliRoutesFromHttpGot404($from, $to, $httpGet)
{
$this->expectException(PageNotFoundException::class);
Copy link
Member

@kenjis kenjis Apr 11, 2022

Choose a reason for hiding this comment

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

If I add the following test case,
the test passes because there is no App\Controllers\Hello class.

            'parameterized method cli' => [
                'hello/(:segment)',
                'Hello::$1',
                'Hello/index',
            ],

*/
public function testOpenCliRoutesFromHttpGot404($from, $to, $httpGet)
{
$this->expectException(PageNotFoundException::class);
Copy link
Member

Choose a reason for hiding this comment

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

These tests pass because there is no App\Controllers\Hello class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: $routes->cli() accessible via web browser if autoroute is true
3 participants