Bug: Validation wrong behaviour - Session mixed with internal validator state

[jmingov]

Describe the bug
The Validation class is mixing current request data and session data from previous request.
It is using the state (session stored errors) from previous request in the return of the run() method to decide if the validation was a success or not.

CodeIgniter4/system/Validation/Validation.php

Line 193 in 8bd6644

return ! empty($this->getErrors()) ? false : true;

public function getErrors(): array
[....]
		if (isset($_SESSION, $_SESSION['_ci_validation_errors']))
		{
			$this->errors = unserialize($_SESSION['_ci_validation_errors']);
		}
	}

	return $this->errors ?? [];
}

This leads to false positives when running a validation after trying to validate any data with errors in the previous request.
Also a call to reset() will not clean this mixed state since is not clearing the session data as we can see:

public function reset(): ValidationInterface
{
	$this->data         = [];
	$this->rules        = [];
	$this->errors       = [];
	$this->customErrors = [];

	return $this;
}

CodeIgniter 4 version
4.0.3 (from composer install)

Affected module(s)

system/Validation/Validation.php

Expected behavior, and steps to reproduce if appropriate
The run() method should return only the errors from the current execution context, it should not mix previous errors from the session, so validate behaves as expected and the validator can be reused and executed as a standalone validator.

How to reproduce the bug:

I. e. We have a controller with 2 methods:

getForm --> GET request. Actions: Validate some data from the request $_GET parameters and show a form to /postForm
postForm --> POST request, Actions: Validate data from the request $_POST parameters (it should fail) and return redirect()->back()->withInput(); so the validation errors are stored in the session and available after the redirect.

The redirect() will go back to /getForm, with valid GET parameters, and fail the validation because the getErrors() call in the return of the run() method will check also the session stored data.

So if you have session errors, you cant trust the result.

Context

• OS: Windows
• Web server: Nginx
• PHP version 7.4.2

There is a PR from time ago, where it changed: 337f29a

IMHO, we should treat session validation errors independently from the validator own errors. I.e from a method like $validation->hasOld(); or something like that.

Please let me know if any further clarification is required and if we agree, ill try to send a PR.

Br, J

[michalsn]

@jmingov It would help a lot if you could provide an example controller and view. Thanks!

[jmingov]

Hi,

There are 2 sample controller methods, one using controllers validate() method and $_REQUEST (/getform1) and the other using a non shared validation instance and a hardcoded array (/getform2).

Routes to be added:

// Sample1
$routes->get('/getform1', 'Users::getform1');
// Sample2
$routes->get('/getform2', 'Users::getform2');
// HELPER POST
$routes->post('/postform', 'Users::postform');

How to:

Files: CI_ValBug.zip

1. • Drop the file Users.php in the controllers root folder and the file simpleform.php in the view 's root folder of any CI4 app.
2. • Navigate to /getform1 or /getform2
3. • Submit the form (with invalid data, so the /postform can redirect back with the error in the session.

Expected: To see the view again with the errors from the POST failed validation.
Reality: You see the validation error response from the GET page (both getform1 and 2).

Br,
J

[jmingov]

If you only want to see the code and have a look here:

https://gist.github.com/jmingov/7d68056f1867221107f3f28616021699

[michalsn]

Thank you and sorry for the delay.

Sadly this is the desired behavior. I mean... checking session for error messages is what we want. It's also the most common use case. The only thing I can suggest is using flash session variable to determine if the validation process for GET method should be processed.

[jmingov]

Thank you for the response, (and congrats for the framework).

No worries for the delay, but does not makes much sense (to me)...

A few questions if you don't mind:

• How should we clear the state from the validator? (Not possible at the moment, only with another redirect if it contains session errors).
• How can it be used to validate standalone data? And reused? (When you have errors already in the session?)
• Even if we have errors in the session from prev request, we may need to perform some validation before we reach the code that validates the session errors? Whatever the desired behavior is, the documentation does not say a word about any of this, and from a security POV, its wrong since you cant validate any data but session errors after the redirect()->back()->withInput();

Returning $this->getErrors() instead $this->errors from Validation.run() is the problem, and makes the validaton class use cases pretty limited no?

Br, j

[michalsn]

If validation errors in the session are really the only thing on your way, then just delete them:

session()->remove('_ci_validation_errors')

• We use session because we want the validation errors to be available after redirect - we can't do this without session.
• In general, I believe your use case is quite "specific" - but it's only my opinion.
• I agree that the user guide may be lacking in information about this. But I don't really see any security issues in current implementation.

Personally, I would think for a moment if the application flow is really correct. And if it is, then maybe you should extend the current Validation class, make changes that you want, and use this version in a validation service.

If you will have more questions, please feel free to post them in our Forum. Thanks!