feat: disable mail dispatch for invalid recovery email #2585
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2585 +/- ##
=======================================
Coverage 75.81% 75.82%
=======================================
Files 302 302
Lines 17784 17789 +5
=======================================
+ Hits 13483 13488 +5
Misses 3270 3270
Partials 1031 1031
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Also related to #1835, this is absolutely needed for anything serious in production. |
mszekiel
changed the title
|
We use Kratos in a privacy-focused organization and we would also really like to disable the "invalid" emails. What is needed to move this PR forward? |
There was a problem hiding this comment.
Thank you! To get this shipped we will need a couple of changes:
- Make this work in the new
codestrategy. - Configure this within the
codeandlinkstrategy - or alternatively inrecoveryandverification. - If this enabled, the user is mislead thinking that an email was sent but they had a typo and never receive one. Shouldn't we show an error that the email is not known?
| @@ -66,6 +66,7 @@ const ( | |||
| ViperKeyCourierSMTPClientKeyPath = "courier.smtp.client_key_path" | |||
| ViperKeyCourierTemplatesPath = "courier.template_override_path" | |||
| ViperKeyCourierTemplatesRecoveryInvalidEmail = "courier.templates.recovery.invalid.email" | |||
| ViperKeyCourierTemplatesRecoveryInvalidSend = "courier.templates.recovery.invalid.send" | |||
There was a problem hiding this comment.
I think this key should go in another object because here we are configuring the templates. Given that this affects recovery and the link and code strategy, it should probably be configured there?
For completeness it would also be good to be able to disable this for both recovery and verification.
| @@ -67,6 +67,12 @@ func (s *Sender) SendRecoveryLink(ctx context.Context, r *http.Request, f *recov | |||
|
|
|||
| address, err := s.r.IdentityPool().FindRecoveryAddressByValue(ctx, identity.RecoveryAddressTypeEmail, to) | |||
| if err != nil { | |||
| if !s.r.Config().CourierTemplatesRecoveryInvalidSend(ctx) { | |||
There was a problem hiding this comment.
Since this PR was opened, we also added a code strategy which is now the default and also needs this feature.
|
Hi, interested party here as we use kratos.
It should be possible to configure kratos in such a way that a user cannot see the difference between successful and unsuccessful sending. Having a difference, for example in the form of an error message, reveals information about the set of existing users. We'll add a message to the reset-link form to warn the user that if they do not receive an email, they should check if they made an error when typing their address, or if they're maybe registered using a different email address. |
|
@mszekiel Do you intend in continuing the PR, still baffles me that this behaviour is the default. |
|
Moved to #3075 |
Added the ability to configure whether the system should notify unknown recipients, if some tries to recover their account or verify their address ("anti-account-enumeration measures").
BREAKING CHANGES: By default, Kratos no longer sends out these Emails. If you want to keep notifying unknown addresses (keep the current behavior), set `selfservice.flows.recovery.notify_unknown_recipients` to `true` for recovery, or `selfservice.flows.verification.notify_unknown_recipients` for verification flows.
Closes #2345
Closes #2585
Added
sendproperty forcourier.templates.recovery.invalidconfiguration field, suppressing email sending when recovery email does not exist.Related issue(s)
#2345
Checklist
If this pull request addresses a security. vulnerability,
I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
Further Comments
Included additional configuration field for invalid recovery. I've also considered extending directly email field by
sendproperty but it would affect all other flows using the same struct. Extended schema definition. Here I wasn't sure if$reftemplate can be extended, so just overwrote it. ForgetEmailcypress command now it's possible to omit check if email is existing.