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: only validate data from POST request body #695

Merged
merged 3 commits into from
Apr 10, 2023

Conversation

miguel-rn
Copy link
Contributor

With the current data validation checks in the controllers, an empty POST request with valid GET parameters will cause the validation to pass. Since this data is always later fetched from the POST request body, it will be null and errors will be thrown.

With the current data validation check, an empty POST request with valid GET parameters will cause the validation to pass but a TypeError will be thrown because on line 110 it is fetching null data from POST.
With the current data validation check, an empty POST request with valid GET parameters will cause the validation to pass but credentials which are later fetched from POST request body will be null.
With the current data validation check, an empty POST request with valid GET parameters will cause the validation to pass later when data is fetched from POST request body, it will be null.
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Thank you. You're correct. Using $this->validate() is not a good practice.

@datamweb
Copy link
Collaborator

@miguel-rn Thank you!

@datamweb datamweb merged commit 799c647 into codeigniter4:develop Apr 10, 2023
@datamweb datamweb added the enhancement New feature or request label Apr 10, 2023
@miguel-rn
Copy link
Contributor Author

I noticed a related issue in the docs, I will make a pull request to update.

miguel-rn added a commit to miguel-rn/shield that referenced this pull request Apr 10, 2023
Fix validation check to only validate POST request body.

codeigniter4#695
@sammyskills
Copy link
Contributor

This is good, but I have to ask.

Is it possible for a $_GET request to pass through a route defined as $_POST, i.e., $route->post() ?

@lonnieezell
Copy link
Member

Not that I'm aware of. The only way I could see anything like that happening is if the route was defined with add() instead of specifying an HTTP method.

@kenjis
Copy link
Member

kenjis commented Apr 12, 2023

$_GET is not related to GET request. It just stores Query String (?foo=bar) in the URI.
So yes, you can POST a request and set $_GET values.

@lonnieezell
Copy link
Member

Sorry, I misread that. You're correct.

@sammyskills
Copy link
Contributor

Maybe I didn't explain properly, I was actually talking about the GET request, in respect to the route definitions:

$routes->get();
$routes->post();

For a route defined explicitly as $route->post(), can $this->request->getGet() pass through it?

@sammyskills
Copy link
Contributor

My main concern is, for example, there is a route definition for the loginAction() method like so:

$routes->post('login', 'LoginController::loginAction');

Is it possible that GET parameters will pass the validation using $this->validate() method?

@kenjis
Copy link
Member

kenjis commented Apr 12, 2023

For a route defined explicitly as $route->post(), can $this->request->getGet() pass through it?

Yes. Because $this->request->getGet() returns $_GET values.

Is it possible that GET parameters will pass the validation using $this->validate() method?

Yes. Because $this->validate() maybe validates $request->getVar() data.
See the Warning: https://codeigniter4.github.io/CodeIgniter4/incoming/controllers.html#this-validate

@sammyskills
Copy link
Contributor

Thank you @kenjis.

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

Successfully merging this pull request may close these issues.

5 participants