Username leakage from password recovery when no email service setup or no useremail

[bourgeoa]

This is a follow on issue#1771
We need to have allways same response when user exists/notExists

• PR fix issue 1770: #1773 resolves the case when email service is setup
• when email service is not setup the server should return Email service is not set up also when username do not exist
• when email service is setup, and email do not exist the server returns Account recovery email has not been provided
  these are/should be rare cases when email is setup and only on really experimental server.
  When username do not exist I propose to return the actual no error

Adding (in username if (!exists)) a call to verifyEmailDependencies ()

node-solid-server/lib/models/account-manager.js

Lines 535 to 543 in 88d3a86

	verifyEmailDependencies (userAccount) {
	if (!this.emailService) {
	throw new Error('Email service is not set up')
	}
	if (!userAccount.email) {
	throw new Error('Account recovery email has not been provided')
	}
	}

Also replacing throw error with success

node-solid-server/lib/models/account-manager.js

Lines 540 to 542 in 88d3a86

	if (!userAccount.email) {
	throw new Error('Account recovery email has not been provided')
	}

with

if (userAccount && !userAccount.email) {
  return resetLinkMessage ()

fix branch created fix/issue#1774

[zg009]

Also replacing throw error with success

node-solid-server/lib/models/account-manager.js

Lines 540 to 542 in 88d3a86

	if (!userAccount.email) {
	throw new Error('Account recovery email has not been provided')
	}

with

if (userAccount && !userAccount.email) {
  return resetLinkMessage ()

fix branch created fix/issue#1774

The only ways to get the resetLinkMessage() call into the AccountManager class is to either make it static from the PasswordResetEmailRequest class, copy it into the AccountManager class, or to pass the instance of PasswordResetEmailRequest class into the AccountManager class. I currently have where if it throws the Error in verifyEmailDependencies with specific message, do renderSuccess() in PasswordResetEmailRequest.loadUser if !exists

[bourgeoa]

Thank you for looking at this

if (userAccount && !userAccount.email) {
 return resetLinkMessage ()

This was an error
You should keep the throw Error, with the check on userAccount the function will not return an Error when the username do not exist
So we should have

 if (userAccount && !userAccount.email) { 
   throw new Error('Account recovery email has not been provided')
 }

And you will have simply something like

  if(!exist) {
   this.accountManager.verifyEmailDependencies() // without userAccount, this will only throw Error on `email service not setup`
   return this.resetLinkMessage() // this is the expected email sent message
  }

[bourgeoa]

Well there is alsoa logic issue with the 2 PRs you will have a conflict with the last merged one.

So you have the choice to keep only one PR for both issues.

Or a bit more complex

• to merge in main on github PR Username leakage from password reset mechanism #1770
• to locally on main branch make a git pull
• change branch to fix/issue#1774
• make the changes commit and push to github

[zg009]

I took the changes I made from 1770 and integrated them with this fix/issue#1774 request, so fix/issue#1770 pull request can be closed without merge as all changes are incorporated into the PR which fixes 1774, as it fixes both 1770 and 1774.