nucleos / NucleosUserBundle

👤 Lightweight user management for symfony.
https://docs.nucleos.rocks/projects/user-bundle/
MIT License
59 stars 18 forks source link

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

Closed pohlm01 closed 9 months ago

pohlm01 commented 9 months ago

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 mentions[^1] 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?

[^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 commented 9 months ago

Can you provide a PR with a fix @pohlm01 ?

pohlm01 commented 9 months ago

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…