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

Use request->method for HTTP verb #2012

Merged
merged 5 commits into from
May 17, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 16, 2019

Description
By default the loaded route collection uses REQUEST_METHOD directly from $_SERVER:
$this->HTTPVerb = strtolower($_SERVER['REQUEST_METHOD'] ?? 'cli');
This means when Router->checkRoutes() matches its collection it uses the actual request method instead of the spoofed method from the Request object. This PR injects the proper HTTP verb prior using $request->getMethod()

To recreate the bug, create a resource route to a non-autorouted controller:
$routes->resource('foo', ['controller' =>'MyNamespace\Controllers\Foo']); And then try to access one of the non-POST supported methods by spoofing: Routing ignores the PUT routes and attempts to autoroute:Controller or its method is not found: App\Controllers\Foo`

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member Author

MGatner commented May 17, 2019

Calling backup! @lonnieezell I think this is probably the correct way to handle this - however all the tests presume that routes will be matched off $_SERVER['REQUEST_METHOD'] instead of $request->method() which accounts for spoofing. Before I (or anyone) goes through and updates the tests I want to make sure this is the desired approach.

@lonnieezell
Copy link
Member

@MGatner Yes, I think that should work.

Although we should be able to do away with the is_cli() check if we handle that during the CodeIgniter::spoofRequestMethod() call. Looks like it doesn't set the request method to 'cli' there, and it probably should.

@MGatner
Copy link
Member Author

MGatner commented May 17, 2019

@lonnieezell These changes are in place. It's failing on testResourcesWithWebsafe, which when I look at it makes me wonder how that ever succeeded: resource() should definitely be returning more than just the 3 routes listed in the test.

@MGatner
Copy link
Member Author

MGatner commented May 17, 2019

Nevermind, it was supposed to be testing POST! Quick fix coming up.

@MGatner
Copy link
Member Author

MGatner commented May 17, 2019

Looks good now!

@lonnieezell lonnieezell merged commit 439bcdf into codeigniter4:develop May 17, 2019
@MGatner MGatner deleted the routes-respect-spoofs branch May 17, 2019 19:46
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.

2 participants