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

Bug: Validation returns incorrect errors after Redirect with Input #5839

Closed
MGatner opened this issue Mar 30, 2022 · 11 comments · Fixed by #5844
Closed

Bug: Validation returns incorrect errors after Redirect with Input #5839

MGatner opened this issue Mar 30, 2022 · 11 comments · Fixed by #5844
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@MGatner
Copy link
Member

MGatner commented Mar 30, 2022

PHP Version

7.4

CodeIgniter4 Version

4.1.9

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

MySQL

What happened?

Validation fails incorrectly following a redirect that uses _ci_validation_errors.

Steps to Reproduce

  1. Submit a form to a Controller endpoint that runs validation
  2. Have the validation fail and return redirect()->...->withInput()
  3. On the destination page run a passing validation and see it fail

This is due to getErrors() filling in the flashdata errors from _ci_validation_errors because $this->errors is empty:

// If we already have errors, we'll use those.
// If we don't, check the session to see if any were
// passed along from a redirect_with_input request.
if (empty($this->errors) && ! is_cli() && isset($_SESSION, $_SESSION['_ci_validation_errors'])) {
$this->errors = unserialize($_SESSION['_ci_validation_errors']);
}

Probably need to differentiate null but I can't dig into the implications of this right now.

Expected Output

The new Validation should pass regardless of the redirected errors.

Anything else?

No response

@MGatner MGatner added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 30, 2022
@kenjis
Copy link
Member

kenjis commented Mar 30, 2022

@MGatner I don't understand your use case.

  1. Submit a form. This is okay.
  2. Validation fails and redirects to where and why?
  3. Why does the destination page need to run validation again?

@iRedds
Copy link
Collaborator

iRedds commented Mar 30, 2022

The bug reproduces if a session is started in the request handler method, but not in the form display method.

$routes->get('/', 'Home::index');
$routes->post('/', 'Home::post');
public function index()
{
    return '<form method="post" action="/">
            <input type="text" name="user">
            <input type="submit">
            </form>';
    //return view('welcome_message');
}

public function post()
{
    session();
    $rules = [
        'user' => ['label' => 'User', 'rules' => 'required']
    ];

    if (!$this->validate($rules)) {
        return redirect()->to('/')->withInput();
    }
    return 'success';
}

If there was a validation error in the first attempt, then the second validation attempt, even if there were no errors, will cause a false positive.

@kenjis
Copy link
Member

kenjis commented Mar 30, 2022

@iRedds Thanks! I confirmed the behavior.

  1. The flash data _ci_validation_errors is set when the first validation.
  2. But redirected page does not use Session, so the flash data is not cleared.
  3. And the second validation does not pass even if there were no errors.

@kenjis
Copy link
Member

kenjis commented Mar 30, 2022

The following code fixes the issue.

    public function index()
    {
        session();
        return '<form method="post" action="/">
            <input type="text" name="user">
            <input type="submit">
            </form>';
    }

@MGatner
Copy link
Member Author

MGatner commented Mar 30, 2022

For a workaround I went with unset($_SESSION['_ci_validation_errors']) prior to running the validation.

@iRedds
Copy link
Collaborator

iRedds commented Mar 30, 2022

Yes, this is a special case.
But I think the following options should be considered:

  1. Starting a session for an HTTP request by default.
  2. ErrorBag class as a service? Which will collect errors and work with the session, but which will reset the state before starting validation.
    Something like this:
class ErrorBag extends MessageBag
{
    protected $messages = [];

    public function __construct()
    {
        $this->messages = session('key_for_errors') ?? [];
    }
    public function get(string $key) {}
    public function set(string $key, $value) {}
    public function has(string $key): bool {}
    public function hasErrors(): bool {}
    public function clear(): self {}
    public function all(): array {}
}

function errorBag()
{
    return new ErrorBag();
}

Validation::run()

$this->errors = errorBag()->clear();
//.... validation
return $this->errors->hasErrors();

To receive errors, the developer does not need to manually start the session and initialize the validation class.

@iRedds
Copy link
Collaborator

iRedds commented Mar 30, 2022

While i have the opportunity, i want to express my opinion. It seems to me that saving errors in the session in the withInput() method does not match the method name.

@kenjis kenjis changed the title Bug: Validation Bleeding Bug: Validation returns incorrect errors when Redirect with Input Mar 31, 2022
@kenjis
Copy link
Member

kenjis commented Mar 31, 2022

@MGatner Yes.
It is the previous validation errors for redirect request.
It should be removed before running validation.

I sent a PR #5844

@kenjis
Copy link
Member

kenjis commented Mar 31, 2022

@iRedds Agree with these. It seems worth considering.
I want to remove $_SESSION in the class.

@kenjis kenjis changed the title Bug: Validation returns incorrect errors when Redirect with Input Bug: Validation returns incorrect errors after Redirect with Input Mar 31, 2022
@kenjis
Copy link
Member

kenjis commented Apr 1, 2022

It seems to me that saving errors in the session in the withInput() method does not match the method name.

It should be withErrors()?

@MGatner
Copy link
Member Author

MGatner commented Apr 2, 2022

It is a strange conflation and part of the reason it took me so long to track down the underlying issue. withInput() doesn't make it apparent that validation errors will be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants