sonata-project / SonataUserBundle

Symfony SonataUserBundle
https://docs.sonata-project.org/projects/SonataUserBundle
MIT License
339 stars 488 forks source link

Username/Email is passed as clear GET Parameter to CheckEmailAction on password reset process #1692

Closed BA-JBI closed 2 weeks ago

BA-JBI commented 3 weeks ago

Subject

When filling username or email address in Password reset action this value is passed as plain GET parameter to Redirect.

https://github.com/sonata-project/SonataUserBundle/blob/35fe4f6fc49391cc0dff21241acfd49038ee4f5e/src/Action/RequestAction.php#L69-L71

This may cause privacy problems because most server logs are not configured by default to filter or anonymize GET parameters from logfiles.

Expected results

As there is no need to pass this value as plain value, a minimal invasive solution would be hashing it, before generating the redirect url.

This should NOT be a breaking change, because the only use of this value is the following code: https://github.com/sonata-project/SonataUserBundle/blob/35fe4f6fc49391cc0dff21241acfd49038ee4f5e/src/Action/CheckEmailAction.php#L37-L39

If you nevertheless think this may be breaking because anyone could have overridden the CheckEmailAction and uses the username value there, i suppose at least to make it configurable if the value is hashed or not.

Please let me know what you think, so i can suppose a pull requst

Hanmac commented 3 weeks ago

I mean, is the username really needed? 🤷‍♂️

The CheckEmailAction only says "You! Check your e-mails". it doesn't use the username at all? Only checks if the parameter exists?

BA-JBI commented 3 weeks ago

I mean, is the username really needed?

I do NOT need it. And if the condition in CheckEmailAction should stay in future, a boolean flag would be enough.

Hanmac commented 3 weeks ago

@VincentLanglet what is your opinion? Does the CheckEmailAction need to have username param?

VincentLanglet commented 3 weeks ago

You can drop it and if tests passed, it'll be merged

Hanmac commented 3 weeks ago

@BA-JBI do you want to make the PR or should i do it?