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

fix issue 1774 #1775

Merged
merged 2 commits into from
Mar 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion default-views/auth/reset-link-sent.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
</div>

<div class="alert alert-success">
<p>A Reset Password link has been sent to your email.</p>
<p>A Reset Password link has been sent from the associated email account.</p>
</div>
</div>
</body>
Expand Down
2 changes: 1 addition & 1 deletion lib/models/account-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ class AccountManager {
throw new Error('Email service is not set up')
}

if (!userAccount.email) {
if (userAccount && !userAccount.email) {
throw new Error('Account recovery email has not been provided')
}
}
Expand Down
9 changes: 6 additions & 3 deletions lib/requests/password-reset-email-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class PasswordResetEmailRequest extends AuthRequest {
.then(() => request.validate())
.then(() => request.loadUser())
.then(userAccount => request.sendResetLink(userAccount))
.then(() => request.renderSuccess())
.then(() => request.resetLinkMessage())
.catch(error => request.error(error))
}

Expand Down Expand Up @@ -123,7 +123,10 @@ class PasswordResetEmailRequest extends AuthRequest {
return this.accountManager.accountExists(username)
.then(exists => {
if (!exists) {
throw new Error('Account not found for that username')
// For security reason avoid leaking error information
Copy link
Member

Choose a reason for hiding this comment

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

Merged too quickly!

Suggested change
// For security reason avoid leaking error information
// For security reasons, avoid leaking error information

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

// See: https://github.com/nodeSolidServer/node-solid-server/issues/1770
this.accountManager.verifyEmailDependencies()
return this.resetLinkMessage()
}

const userData = { username }
Expand Down Expand Up @@ -191,7 +194,7 @@ class PasswordResetEmailRequest extends AuthRequest {
/**
* Displays the 'your reset link has been sent' success message view
*/
renderSuccess () {
resetLinkMessage () {
this.response.render('auth/reset-link-sent')
}
}
Expand Down
52 changes: 48 additions & 4 deletions test/unit/password-reset-email-request-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const HttpMocks = require('node-mocks-http')
const PasswordResetEmailRequest = require('../../lib/requests/password-reset-email-request')
const AccountManager = require('../../lib/models/account-manager')
const SolidHost = require('../../lib/models/solid-host')
const EmailService = require('../../lib/services/email-service')

describe('PasswordResetEmailRequest', () => {
describe('constructor()', () => {
Expand Down Expand Up @@ -153,6 +154,39 @@ describe('PasswordResetEmailRequest', () => {
expect(request.error).to.not.have.been.called()
})
})

it('should hande a reset request with no username without privacy leakage', () => {
const host = SolidHost.from({ serverUri: 'https://example.com' })
const store = { suffixAcl: '.acl' }
const accountManager = AccountManager.from({ host, multiuser: true, store })
accountManager.loadAccountRecoveryEmail = sinon.stub().resolves('[email protected]')
accountManager.sendPasswordResetEmail = sinon.stub().resolves()
accountManager.accountExists = sinon.stub().resolves(false)

const returnToUrl = 'https://example.com/resource'
const username = 'alice'
const response = HttpMocks.createResponse()
response.render = sinon.stub()

const options = { accountManager, username, returnToUrl, response }
const request = new PasswordResetEmailRequest(options)

sinon.spy(request, 'error')
sinon.spy(request, 'validate')
sinon.spy(request, 'loadUser')

return PasswordResetEmailRequest.handlePost(request)
.then(() => {
expect(request.validate).to.have.been.called()
expect(request.loadUser).to.have.been.called()
expect(request.loadUser).to.throw()
}).catch(() => {
expect(request.error).to.have.been.called()
expect(response.render).to.have.been.calledWith('auth/reset-link-sent')
expect(accountManager.loadAccountRecoveryEmail).to.not.have.been.called()
expect(accountManager.sendPasswordResetEmail).to.not.have.been.called()
})
})
})

describe('loadUser()', () => {
Expand All @@ -175,16 +209,26 @@ describe('PasswordResetEmailRequest', () => {
it('should throw an error if the user does not exist', done => {
const host = SolidHost.from({ serverUri: 'https://example.com' })
const store = { suffixAcl: '.acl' }
const accountManager = AccountManager.from({ host, multiuser: true, store })
const emailService = sinon.stub().returns(EmailService)
const accountManager = AccountManager.from({ host, multiuser: true, store, emailService })
accountManager.accountExists = sinon.stub().resolves(false)
const username = 'alice'

const options = { accountManager, username }
const request = new PasswordResetEmailRequest(options)

sinon.spy(request, 'resetLinkMessage')
sinon.spy(accountManager, 'userAccountFrom')
sinon.spy(accountManager, 'verifyEmailDependencies')

request.loadUser()
.catch(error => {
expect(error.message).to.equal('Account not found for that username')
.then(() => {
expect(accountManager.userAccountFrom).to.have.been.called()
expect(accountManager.verifyEmailDependencies).to.have.been.called()
expect(accountManager.verifyEmailDependencies).to.throw()
done()
})
.catch(() => {
expect(request.resetLinkMessage).to.have.been.called()
done()
})
})
Expand Down
Loading