Success for sending password email is reported to user even when it actually failed due to server misconfiguration

Describe the Bug

When sending email fails, for example due to bad credentials, POST /api/admins/forgot-password does correctly return a 500 error, but the built-in UI still shows a success in that case, it claims the email was sent when it actually wasn't, and the user is none the wiser.

Note that I do not mean the case where the email does not exist in the database, I'm talking about a server misconfiguration, problem with the email server, etc.

Root Cause

The issue is here (no check for response.ok):

payload/packages/next/src/views/ForgotPassword/ForgotPasswordForm/index.tsx

Lines 25 to 39 in 1072171

    
           const handleResponse: FormProps['handleResponse'] = (res, successToast, errorToast) => { 
        
             res 
        
               .json() 
        
               .then(() => { 
        
                 setHasSubmitted(true) 
        
                 successToast(t('general:submissionSuccessful')) 
        
               }) 
        
               .catch(() => { 
        
                 errorToast( 
        
                   loginWithUsername 
        
                     ? t('authentication:usernameNotValid') 
        
                     : t('authentication:emailNotValid'), 
        
                 ) 
        
               }) 
        
           }

Link to the code that reproduces this issue

unnecessary, any app will do once you change the email config to a bad one

Reproduction Steps

Create a blank project, set some bad email configuration in it that isn't blank, for example some SMTP configuration with bad credentials. Then try to use the forgot password form.

Which area(s) are affected? (Select all that apply)

area: ui

Environment Info

Irrelevant

Please add a reproduction in order for us to be able to investigate.

Depending on the quality of reproduction steps, this issue may be closed if no reproduction is provided.

Why was this issue marked with the `invalid-reproduction` label?

To be able to investigate, we need access to a reproduction to identify what triggered the issue. We prefer a link to a public GitHub repository created with create-payload-app@latest -t blank or a forked/branched version of this repository with tests added (more info in the reproduction-guide).

To make sure the issue is resolved as quickly as possible, please make sure that the reproduction is as minimal as possible. This means that you should remove unnecessary code, files, and dependencies that do not contribute to the issue. Ensure your reproduction does not depend on secrets, 3rd party registries, private dependencies, or any other data that cannot be made public. Avoid a reproduction including a whole monorepo (unless relevant to the issue). The easier it is to reproduce the issue, the quicker we can help.

Please test your reproduction against the latest version of Payload to make sure your issue has not already been fixed.

I added a link, why was it still marked?

Ensure the link is pointing to a codebase that is accessible (e.g. not a private repository). "example.com", "n/a", "will add later", etc. are not acceptable links -- we need to see a public codebase. See the above section for accepted links.

Useful Resources

Hey @CherryDT

I think this is exactly how it should work. If you were to show failure status on this flow then a potential bad actor could enumerate accounts based on that status. Why expose more information than necessary? If the user entered their email correctly, then they will get an email. Worst case, they come back to this view and retry.

That would be the case of invalid user data but not in case of a unknown server error. It should at least indicate that something went wrong not related to the user input and they should try later.

I do see the point though, if no email was sent at all, the server wouldn't notice the error, so in case of a server issue it can open up a temporary "attack" window to guess users because a success would then indicate the user does not exist.

However, in that case - about the potential bad actor: The endpoint should by that logic not return 500! The information for the bad actor is right there in the status code.

Ahh I see what you're saying. This isn't caused by incorrect user email, but by incorrect email setup in configuration. Yeah, that's totally valid and agreeable to me.

What adapter are you using? If you are using @payloadcms/email-nodemailer, are you explicitly setting the skipVerify property?

This doesn't address the root issue, though. Sounds like something that should be fixed.

	const handleResponse: FormProps['handleResponse'] = (res, successToast, errorToast) => {
	res
	.json()
	.then(() => {
	setHasSubmitted(true)
	successToast(t('general:submissionSuccessful'))
	})
	.catch(() => {
	errorToast(
	loginWithUsername
	? t('authentication:usernameNotValid')
	: t('authentication:emailNotValid'),
	)
	})
	}