Skip to content

Using IsCsrfTokenValid attribute with invalid token redirects user to login page #57343

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.

Sign up for GitHub

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

Jump to bottom
Open
cyve opened this issue Jun 7, 2024 · 11 comments · May be fixed by #57622
Open

Using IsCsrfTokenValid attribute with invalid token redirects user to login page #57343

cyve opened this issue Jun 7, 2024 · 11 comments · May be fixed by #57622
Labels
Bug HttpKernel Security Status: Needs Review

Comments

@cyve
Copy link
Contributor

cyve commented Jun 7, 2024

Symfony version(s) affected

7.1.1

Description

Hi,
I tried the new IsCsrfTokenValid attribute on a route to handle a form. When the token is invalid, the user is redirected to the login page instead of an error page. Probably because IsCsrfTokenValidAttributeListener throws a InvalidCsrfTokenException witch extends AuthenticationException.

How to reproduce

Add IsCsrfTokenValid attribute on a route handling a form

#[Route('/add-to-cart', name: 'add_to_cart', methods: ['POST'])]
#[IsCsrfTokenValid('add_to_cart')]
public function __invoke(Request $request): Response
{
}

Load the page and wait for the token to expire (or generate an invalid token)

<form method="post" action="{{ path('add_to_cart') }}">
  <input type="hidden" name="product" value="{{ product.id }}">
  <input type="hidden" name="_token" value="this_token_is_invalid">
</form>

Possible Solution

Maybe we could throw a BadRequestHttpException instead of an InvalidCsrfTokenException in IsCsrfTokenValidAttributeListener ? But I guess there is a good reason for InvalidCsrfTokenException to extend AuthenticationException, so I can't really see the implications.
If this solution looks good to you, I can create a PR.

Additional Context

No response
@cyve cyve added the Bug label Jun 7, 2024
@eltharin
Copy link
Contributor

I think InvalidCsrfTokenException must extends AccessDeniedException instaed of AuthenticationException to get a 403 response instead a login page redirection.

For the moment it's possible in a custom authenticator to set the start function as mentioned in #16026 (comment)

@xabbuh
Copy link
Member

xabbuh commented Jun 30, 2024

Can you create a small example application that allows to reproduce your issue?

@eltharin
Copy link
Contributor

sure,
https://github.com/eltharin/reproducer_symfony_57343.git
juste have to make a composer install and serve it,
If you go to https://127.0.0.1:8000/test you will be redirect to /login (I don't agree with that but it's another discussion)
if you log with user@fr.fr / password and go to /test you are still redirected to /login
if you change InvalidCsrfTokenException extends AccessDeniedException instaed of AuthenticationException you have an error page with Invalid CSRF Token

do you want I make the PR ? (Bug or New Feature ? )

This symptome is joining issues :
#16026
#54318
I'm working on a PR for add a security option to block the login redirect and show error if we are already logged for those who want to prevent these behavior.

@eltharin
Copy link
Contributor

With reflexion, BadRequestHttpException will be a better choice for extends, beacause a Csrf Error is not automaticly linked to an authentification

@eltharin eltharin linked a pull request Jul 2, 2024 that will close this issue
Open
@eltharin
Copy link
Contributor

What about the HTTP code ? 403 is for acces denied, Is really CSRF error an acces denied ? For a contact form can we speak about Access ?

@netonevess
Copy link

for a temporary solution, I did the following code:
https://github.com/netonevess/Symfony72-csrf-response-modifyer/blob/main/InvalidTokenListener.php

My software was redirecting to the login page once an invalid CSRF using the new attribute #[IsCsrfTokenValid] and now a custom JSON message.

@eltharin
Copy link
Contributor

eltharin commented Aug 3, 2024

Yeah I created a Exception Transformer near you made, But it can be better to be in core

@netonevess
Copy link

Yeah I created a Exception Transformer near you made, But it can be better to be in core

please help us to improve that one. You can create a PR to this repo

@eltharin
Copy link
Contributor

I already did the PR, juste waiting for someone valid it : #57622

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 11, 2024 •
edited
Loading

Maybe @yguedidi would like to have a look at this issue and the related PR proposal? (because #52961)

@eltharin
Copy link
Contributor

friendly ping @yguedidi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpKernel Security Status: Needs Review
Projects
None yet
Milestone
No milestone
6 participants
@nicolas-grekas @xabbuh @cyve @eltharin @carsonbot @netonevess