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

log password reset email errors to log and do not reveal to user #17896

Closed
wants to merge 1 commit into from

Conversation

geekdenz
Copy link
Contributor

fixes #17091

Description:

The lines changed may have already achieved this previously. I could not reproduce seeing any revealing details in the errors I provoked. However, it is now ensured that no detail of the exception is logged to the user and they just get told to contact the admin. The exception is logged to the system logs with the injected logger instance. Currently it is only using the implied __toString() method of the exception, which should provide adequate detail to admins. However, there might be better detail that could be logged here. Maybe we want to log the message with $ex->getMessage() instead like was previously fed back to the user.

Also, I noticed that Piwik::translate was used instead of using an injected Translator instance. @tsteur mentioned that we prefer to use DI, but there are a few instances of Piwik::translate in this file, so I left it for now, also because there it uses the injected class anyway and it might be easier to understand for a significant amount of users/developers.

Review

@geekdenz geekdenz marked this pull request as draft August 17, 2021 03:42
@geekdenz geekdenz marked this pull request as ready for review August 18, 2021 05:57
@geekdenz geekdenz added the Needs Review PRs that need a code review label Aug 18, 2021
@geekdenz geekdenz added this to the 4.5.0 milestone Aug 18, 2021
@geekdenz geekdenz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 19, 2021
@@ -179,8 +191,8 @@ public function initiatePasswordResetProcess($loginOrEmail, $newPassword)
} catch (Exception $ex) {
// remove password reset info
$this->removePasswordResetInfo($login);

throw new Exception($ex->getMessage() . Piwik::translate('Login_ContactAdmin'));
$this->logger->error($ex); // log exception details to log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geekdenz this would be potentially logged to the user screen. We don't have a functionality to only log to logs and in this case would likely also not want to log potential information to logs.

Could above information also be triggered for different errors where we maybe still want to show the original error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could catch the specific Exception (find out which one - AuthenticationException or similar) and otherwise also log the message. But in case of an AuthenticationException, log:

An unspecified error occured, please contact an admin.

or similar. Would this be desirable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really need to log anything in particular here as the message will be shown to the user anyway.

@geekdenz
Copy link
Contributor Author

When this error happens, please check your SMTP settings.

Also tell user where to change settings.

Catch the specific exception and show above error message.

@geekdenz
Copy link
Contributor Author

No need to merge as solution is documentation. See issue.

@geekdenz geekdenz closed this Aug 25, 2021
@sgiehl sgiehl deleted the m-17091 branch April 5, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing SMTP connection might result in information disclosure in password recovery
2 participants