Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security][SecurityBundle] Change exception thrown by#[IsCsrfTokenValid] attribute#57622
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
InvalidCsrfTokenException390fe9d to5224b66Comparedf7f6f2 to58f68f4Compareseems test fail because it take old test with depedencies ?? |
The To me, the issue is that the |
Side note, the |
eltharin commentedAug 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So I can keep in place InvalidCsrfTokenException and create another on in Security/Csrf extending BadRequestException and link it to the Exception throw in IsCsrfTokenValidAttributeListener ? |
change is OK, do I move IsCsrfTokenValidAttributeListener in Security/Csrf ? |
InvalidCsrfTokenException22c900f to9f653c4CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should move the attribute/listener
clear old getMessage function
bc01d90 to9717ef8CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for working on this@eltharin!
Before going further withmoving the listener/attribute to security-csrf, let's see what other @symfony/mergers think. Maybe we should just focus this PR on fixing the bug and the move in a follow-up PR (or not at all).
| @@ -11,6 +11,7 @@ CHANGELOG | |||
| * Add ability to fetch LDAP roles | |||
| * Add `OAuth2TokenHandlerFactory` for `AccessTokenFactory` | |||
| * Add discovery support to `OidcTokenHandler` and `OidcUserInfoTokenHandler` | |||
| * Change Exception thrown by IsCsrfTokenValid Attribute | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be insecurity-csrf &security-http
| @@ -9,17 +9,17 @@ | |||
| * file that was distributed with this source code. | |||
| */ | |||
| namespace Symfony\Component\Security\Http\EventListener; | |||
| namespace Symfony\Component\Security\Csrf\EventListener; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunately, it isn't this simple, we need to keep it in both places, deprecating the one in http. See#46094 for how we did this with theSecurity helper.
9717ef8 tobfe93b9CompareOk I revert the move and wait and see |
Istrongly disagree here. Regardless of whether the user is logged in, CSRF is100% an authentication problem.
The CSRF token system -and by extension the If the listener can’t validate the token, it can’t confirm that the user who sent the request is who they claim to be (again: regardless of its login status). And not being able to confirm the identity or intent behind a request is, by definition, an authentication failure. |
We can keep 403 code error, the really problem is the login redirection. |
#[IsCsrfTokenValid] attribute
Uh oh!
There was an error while loading.Please reload this page.
Change InvalidCsrfTokenException behavior :
Actually, when InvalidCsrfTokenException throw, a 403 Response is return or if there is a userAuthenticator, response is converted to 301-Redirect To Login,
For example, For a Basic Contact Form, if the CSRF is bad, I'm redirected to Login Page => Why ?
InvalidCsrfTokenException extends from AuthenticationException but a Csrf Error is not necessarily an authentication error.
And is 403 error code really associated to a CSRF error ?
So I propose to change parent from AuthenticationException to BadRequestHttpException for have a 400 Bad Request Error whitch is most adapted to CSRF error and to not have a redirection to login page;