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

Login form uses getMessage() instead of getMessageKey() #725

Closed
pohlm01 opened this issue Nov 20, 2023 · 2 comments · Fixed by #728
Closed

Login form uses getMessage() instead of getMessageKey() #725

pohlm01 opened this issue Nov 20, 2023 · 2 comments · Fixed by #728

Comments

@pohlm01
Copy link
Contributor

pohlm01 commented Nov 20, 2023

Description

I have trouble displaying a meaningful message to the user on the login screen if a TooManyLoginAttemptsAuthenticationException is thrown.
This is because, this exception does return an empty string on a getMessage() call, which I cannot translate to a meaningful message.

During troubleshooting, it turned out that Symphony mentions1 to never display the message of an AuthenticationException, as it may contain sensitive data.
A good example for sensitive data can be found in Symphony itself. When checking credentials, the message contains the information that the user does exist, but the password is wrong. Generally, the user should not know this information, but it gets currently leaked here.

I propose to replace the questionable getMessage() call in the login screen with the save getMessageKey() counterpart. Any opinions on that?

Footnotes

  1. Symphony mentions that: "Caution: Never use $exception->getMessage() for AuthenticationException instances. This message might contain sensitive information that you don't want to be publicly exposed. Instead, use $exception->getMessageKey() and $exception->getMessageData() like shown in the full example above. Use CustomUserMessageAuthenticationException if you want to set custom error messages."

@core23
Copy link
Member

core23 commented Nov 20, 2023

Can you provide a PR with a fix @pohlm01 ?

@pohlm01
Copy link
Contributor Author

pohlm01 commented Nov 22, 2023

Sure. I opened two PRs, one on the default branch, and one on the 1.13.x branch. I wasn't too sure about the maintenance status of the 1.13.x branch, but unfortunately this is where I would need the fix…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants