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

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

Closed
BA-JBI opened this issue Aug 20, 2024 · 5 comments · Fixed by #1695
Closed

Comments

@BA-JBI
Copy link
Contributor

BA-JBI commented Aug 20, 2024

Subject

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

return new RedirectResponse($this->urlGenerator->generate('sonata_user_admin_resetting_check_email', [
'username' => $username,
]));

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:

$username = $request->query->get('username');
if (null === $username) {

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
Copy link
Contributor

Hanmac commented Aug 20, 2024

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
Copy link
Contributor Author

BA-JBI commented Aug 21, 2024

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
Copy link
Contributor

Hanmac commented Aug 21, 2024

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

@VincentLanglet
Copy link
Member

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

@Hanmac
Copy link
Contributor

Hanmac commented Aug 21, 2024

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

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