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

This middleware no longer supports get requests? #134

Open
skeets23 opened this issue Jan 20, 2021 · 1 comment
Open

This middleware no longer supports get requests? #134

skeets23 opened this issue Jan 20, 2021 · 1 comment

Comments

@skeets23
Copy link

So I recently updated this package in my project, and I traced back a bug to this change in the source code that I can't understand for the life of me:

Response.php:

...
protected function makeStatusCode()
    {
        if ($this->getRequest()->isMethod('get') || ($this->checkOTP() === Constants::OTP_VALID)) {
            return SymfonyResponse::HTTP_OK;
        }

...

.... so.... any request that is a "get" is OK? How does that make sense?

Actually, if they're not authenticated, it's definitely not ok, and all this does is hide the error message, and display the page asking for the code again with no explanation as to what happened.

Unless I'm misunderstanding something, $this->getRequest()->isMethod('get') || needs to be removed from the if statement.

This change happened in commit 92b9b8c

Yes, I see that the documentation says to use <form method="POST" action="/some-action">, but this is an inferior approach -- you then have to set a static redirect for the post request, so that no matter what the original URL is, they're redirected to the same page.

Example: User goes to mysite.com/settings/some-setting, and they hit the 2FA middleware. What I would expect is that once you complete the 2FA challenge successfully, it takes you to mysite.com/settings/some-setting, but that won't be possible using this library currently because it requires a POST request, and mysite.com/settings/some-setting is a GET route.

And you can't just use back() in the post request either, that will blow up if they messed up the 2FA code 2 or 3 times from too many redirects.

It was much more simple before, just create a form exactly like this <form>, and then the original URL is retained, and a GET request is sent to the original URL, passes through the 2FA middleware, and puts the user exactly where they expect to end up.

I see no valid reason to require POST in the middleware. If there is an easy way to get the user back to their original GET request while requiring that the 2FA code is sent over a POST request, then it's not documented, so either way there's a problem.

@LorenzoSapora
Copy link

Hey @antonioribeiro any insight into this?

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

No branches or pull requests

2 participants