From 825e1b5e5c57802c2939e496b3ac7240826841af Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 25 Sep 2017 14:50:33 -0600 Subject: [PATCH 1/3] Fixes to password reset flow --- routes/api/account/index.js | 24 ++++------ services/domainlist.js | 53 +++++++++++----------- services/users.js | 42 ++++++----------- test/server/services/domainlist.js | 73 +++++++++++++++++++++++++++++- 4 files changed, 123 insertions(+), 69 deletions(-) diff --git a/routes/api/account/index.js b/routes/api/account/index.js index 99cb9a468..85558d33c 100644 --- a/routes/api/account/index.js +++ b/routes/api/account/index.js @@ -50,22 +50,18 @@ router.post('/password/reset', async (req, res, next) => { try { let token = await UsersService.createPasswordResetToken(email, loc); - if (!token) { - res.status(204).end(); - return; + if (token) { + await mailer.sendSimple({ + template: 'password-reset', + locals: { + token, + rootURL: ROOT_URL + }, + subject: 'Password Reset', + to: email + }); } - // Send the password reset email. - await mailer.sendSimple({ - template: 'password-reset', // needed to know which template to render! - locals: { // specifies the template locals. - token, - rootURL: ROOT_URL - }, - subject: 'Password Reset', - to: email - }); - res.status(204).end(); } catch (e) { return next(e); diff --git a/services/domainlist.js b/services/domainlist.js index 29673e2b6..a75d62dca 100644 --- a/services/domainlist.js +++ b/services/domainlist.js @@ -2,6 +2,8 @@ const debug = require('debug')('talk:services:domainlist'); const _ = require('lodash'); const SettingsService = require('./settings'); +const {ROOT_URL} = require('../config'); + /** * The root domainlist object. * @type {Object} @@ -17,31 +19,24 @@ class Domainlist { /** * Loads domains white list in from the database */ - load() { - return SettingsService - .retrieve() - .then((settings) => { - - // Insert the settings domains whitelist. - this.upsert(settings.domains); - }); + async load() { + const {domains} = await SettingsService.retrieve(); + this.upsert(domains); } /** * Inserts the domains whitelist data * @param {Array} list list of domains to be set to the whitelist */ - upsert(lists) { + async upsert(lists) { // Add the domains to this array and also be sure are all unique domains if (!('whitelist' in lists)) { return; } - this.lists['whitelist'] = Domainlist.parseList(lists['whitelist']); - debug(`Added ${lists['whitelist'].length} domains to the whitelist.`); - - return Promise.resolve(this); + this.lists.whitelist = Domainlist.parseList(lists.whitelist); + debug(`Added ${lists.whitelist.length} domains to the whitelist.`); } /** @@ -51,19 +46,22 @@ class Domainlist { */ match(list, url) { + // Parse the url that we're matching with. const domainToMatch = Domainlist.parseURL(url); // This will return true in the event that at least one blockword is found // in the phrase. - for (let i = 0; i < list.length; i++) { - if (list[i] === domainToMatch) { - return true; - } - } + return list.indexOf(domainToMatch) >= 0; + } - // We've walked over all the whitelisted domains, and haven't had a - // mismatch... It is not an allowed domain! - return false; + /** + * Checks to see if the passed url matches the domain of the root path. + * + * @param {String} url + * @returns {Boolean} true if the domains match + */ + static matchMount(url) { + return Domainlist.parseURL(url) === Domainlist.parseURL(ROOT_URL); } /** @@ -84,7 +82,7 @@ class Domainlist { let domain; // removes protocol and get domain - if (url.indexOf('://') > -1) { + if (url.indexOf('//') > -1) { domain = url.split('/')[2]; } else { domain = url.split('/')[0]; @@ -96,13 +94,14 @@ class Domainlist { return domain.toLowerCase(); } - static urlCheck(url) { + static async urlCheck(url) { const dl = new Domainlist(); - return dl.load() - .then(() => { - return dl.match(dl.lists['whitelist'], url); - }); + // Load the domain list. + await dl.load(); + + // Perform a match. + return dl.match(dl.lists.whitelist, url); } } diff --git a/services/users.js b/services/users.js index 35810ef7f..faea67d75 100644 --- a/services/users.js +++ b/services/users.js @@ -1,8 +1,6 @@ const assert = require('assert'); const uuid = require('uuid'); const bcrypt = require('bcryptjs'); -const url = require('url'); -const Wordlist = require('./wordlist'); const errors = require('../errors'); const { @@ -22,9 +20,10 @@ const USER_ROLES = require('../models/enum/user_roles'); const RECAPTCHA_WINDOW_SECONDS = 60 * 10; // 10 minutes. const RECAPTCHA_INCORRECT_TRIGGER = 5; // after 3 incorrect attempts, recaptcha will be required. -const SettingsService = require('./settings'); const ActionsService = require('./actions'); const MailerService = require('./mailer'); +const Wordlist = require('./wordlist'); +const Domainlist = require('./domainlist'); const EMAIL_CONFIRM_JWT_SUBJECT = 'email_confirm'; const PASSWORD_RESET_JWT_SUBJECT = 'password_reset'; @@ -557,11 +556,10 @@ module.exports = class UsersService { email = email.toLowerCase(); - const [user, settings] = await Promise.all([ + const [user, domainValidated] = await Promise.all([ UserModel.findOne({profiles: {$elemMatch: {id: email}}}), - SettingsService.retrieve(), + Domainlist.urlCheck(loc), ]); - if (!user) { // Since we don't want to reveal that the email does/doesn't exist @@ -569,19 +567,11 @@ module.exports = class UsersService { // endpoint. return; } - let redirectDomain; - try { - const {hostname, port} = url.parse(loc); - redirectDomain = hostname; - if (port) { - redirectDomain += `:${port}`; - } - } catch (e) { - throw new Error('redirect location is invalid'); - } - if (settings.domains.whitelist.indexOf(redirectDomain) === -1) { - throw new Error('redirect location is not on the list of acceptable domains'); + // If the domain didn't match any of the whitelisted domains and if it + // didn't match the mount domain, then throw an error. + if (!domainValidated && !Domainlist.matchMount(loc)) { + throw new Error('user supplied location exists on non-permitted domain'); } const payload = { @@ -619,16 +609,14 @@ module.exports = class UsersService { * Verifies a jwt and returns the associated user. * @param {String} token the JSON Web Token to verify */ - static verifyPasswordResetToken(token) { - return UsersService - .verifyToken(token, { - subject: PASSWORD_RESET_JWT_SUBJECT - }) + static async verifyPasswordResetToken(token) { + const {userId, loc} = await UsersService.verifyToken(token, { + subject: PASSWORD_RESET_JWT_SUBJECT + }); - // TODO: add search by __v as well - .then((decoded) => { - return Promise.all([UsersService.findById(decoded.userId), decoded.loc]); - }); + const user = await UsersService.findById(userId); + + return [user, loc]; } /** diff --git a/test/server/services/domainlist.js b/test/server/services/domainlist.js index 9dafa438a..33afdda87 100644 --- a/test/server/services/domainlist.js +++ b/test/server/services/domainlist.js @@ -26,13 +26,84 @@ describe('services.Domainlist', () => { }); + describe('#parseURL', () => { + it('parses the domain correctly', () => { + [ + ['http://google.ca/test', 'google.ca'], + ['http://google.ca:80/test', 'google.ca'], + ['https://google.ca/test', 'google.ca'], + ['https://google.ca:443/test', 'google.ca'], + ['//google.ca/test', 'google.ca'], + ['//google.ca:80/test', 'google.ca'], + ['//google.ca:443/test', 'google.ca'], + ['google.ca/test', 'google.ca'], + ['google.ca:80/test', 'google.ca'], + ['google.ca:443/test', 'google.ca'], + ['http://google.ca/', 'google.ca'], + ['http://google.ca:80/', 'google.ca'], + ['https://google.ca/', 'google.ca'], + ['https://google.ca:443/', 'google.ca'], + ['//google.ca/', 'google.ca'], + ['//google.ca:80/', 'google.ca'], + ['//google.ca:443/', 'google.ca'], + ['google.ca/', 'google.ca'], + ['google.ca:80/', 'google.ca'], + ['google.ca:443/', 'google.ca'], + ['google.ca', 'google.ca'], + ['http://google.ca', 'google.ca'], + ['http://google.ca:80', 'google.ca'], + ['https://google.ca', 'google.ca'], + ['https://google.ca:443', 'google.ca'], + ['//google.ca', 'google.ca'], + ['//google.ca:80', 'google.ca'], + ['//google.ca:443', 'google.ca'], + ['google.ca', 'google.ca'], + ['google.ca:80', 'google.ca'], + ['google.ca:443', 'google.ca'], + ['http://google.Ca/test', 'google.ca'], + ['http://google.ca:80/test', 'google.ca'], + ['https://google.Ca/test', 'google.ca'], + ['https://google.ca:443/test', 'google.ca'], + ['//google.Ca/test', 'google.ca'], + ['//google.Ca:80/test', 'google.ca'], + ['//google.Ca:443/test', 'google.ca'], + ['google.Ca/test', 'google.ca'], + ['google.ca:80/test', 'google.ca'], + ['google.ca:443/test', 'google.ca'], + ['http://Google.ca/', 'google.ca'], + ['http://google.Ca:80/', 'google.ca'], + ['https://Google.ca/', 'google.ca'], + ['https://google.Ca:443/', 'google.ca'], + ['//Google.ca/', 'google.ca'], + ['//google.Ca:80/', 'google.ca'], + ['//google.Ca:443/', 'google.ca'], + ['Google.ca/', 'google.ca'], + ['google.Ca:80/', 'google.ca'], + ['google.Ca:443/', 'google.ca'], + ['Google.ca', 'google.ca'], + ['http://Google.ca', 'google.ca'], + ['http://google.Ca:80', 'google.ca'], + ['https://Google.ca', 'google.ca'], + ['https://google.Ca:443', 'google.ca'], + ['//Google.ca', 'google.ca'], + ['//google.Ca:80', 'google.ca'], + ['//google.Ca:443', 'google.ca'], + ['Google.ca', 'google.ca'], + ['google.Ca:80', 'google.ca'], + ['google.Ca:443', 'google.ca'], + ].forEach(([domain, hostname]) => { + expect(Domainlist.parseURL(domain), `domain ${domain} should be parsed as ${hostname}`).to.equal(hostname); + }); + }); + }); + describe('#match', () => { const whiteList = Domainlist.parseList(domainlists['whitelist']); it('does match on an included domain', () => { [ - 'wapo.com', + 'http://wapo.com', 'nytimes.com' ].forEach((domain) => { expect(domainlist.match(whiteList, domain)).to.be.true; From 203d3c7ee961763df004e161e6b61cb2b87a19e7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 25 Sep 2017 16:12:40 -0600 Subject: [PATCH 2/3] bug fixes --- app.js | 18 +++-------- routes/api/account/index.js | 17 +++++------ services/email/email-confirm.html.ejs | 2 +- services/email/email-confirm.txt.ejs | 2 +- services/email/password-reset.html.ejs | 2 +- services/email/password-reset.txt.ejs | 2 +- services/locals.js | 20 ++++++++++++ services/mailer.js | 4 +++ services/users.js | 6 +++- views/admin/password-reset.ejs | 42 ++++++++++++++++++++------ 10 files changed, 77 insertions(+), 38 deletions(-) create mode 100644 services/locals.js diff --git a/app.js b/app.js index 9da2041fc..2989ae00a 100644 --- a/app.js +++ b/app.js @@ -6,13 +6,9 @@ const merge = require('lodash/merge'); const helmet = require('helmet'); const compression = require('compression'); const cookieParser = require('cookie-parser'); -const { - BASE_URL, - BASE_PATH, - MOUNT_PATH, - STATIC_URL, - HELMET_CONFIGURATION, -} = require('./url'); +const {HELMET_CONFIGURATION} = require('./config'); +const {MOUNT_PATH} = require('./url'); +const {applyLocals} = require('./services/locals'); const routes = require('./routes'); const debug = require('debug')('talk:app'); @@ -57,12 +53,8 @@ app.set('view engine', 'ejs'); // ROUTES //============================================================================== -// Apply the BASE_PATH, BASE_URL, and MOUNT_PATH on the app.locals, which will -// make them available on the templates and the routers. -app.locals.BASE_URL = BASE_URL; -app.locals.BASE_PATH = BASE_PATH; -app.locals.MOUNT_PATH = MOUNT_PATH; -app.locals.STATIC_URL = STATIC_URL; +// Add the locals to the app renderer. +applyLocals(app.locals); debug(`mounting routes on the ${MOUNT_PATH} path`); diff --git a/routes/api/account/index.js b/routes/api/account/index.js index 85558d33c..c35f709cb 100644 --- a/routes/api/account/index.js +++ b/routes/api/account/index.js @@ -4,9 +4,6 @@ const UsersService = require('../../../services/users'); const mailer = require('../../../services/mailer'); const authorization = require('../../../middleware/authorization'); const errors = require('../../../errors'); -const { - ROOT_URL -} = require('../../../config'); //============================================================================== // ROUTES @@ -55,7 +52,6 @@ router.post('/password/reset', async (req, res, next) => { template: 'password-reset', locals: { token, - rootURL: ROOT_URL }, subject: 'Password Reset', to: email @@ -74,22 +70,23 @@ router.post('/password/reset', async (req, res, next) => { * 2) the new password {String} */ router.put('/password/reset', async (req, res, next) => { - - const { - token, - password - } = req.body; + const {check} = req.query; + const {token, password} = req.body; if (!token) { return next(errors.ErrMissingToken); } - if (!password || password.length < 8) { + if (check !== 'true' && (!password || password.length < 8)) { return next(errors.ErrPasswordTooShort); } try { let [user, loc] = await UsersService.verifyPasswordResetToken(token); + if (check === 'true') { + res.status(204).end(); + return; + } // Change the users' password. await UsersService.changePassword(user.id, password); diff --git a/services/email/email-confirm.html.ejs b/services/email/email-confirm.html.ejs index 08a505521..8a1bd05f7 100644 --- a/services/email/email-confirm.html.ejs +++ b/services/email/email-confirm.html.ejs @@ -1,3 +1,3 @@

<%= t('email.confirm.has_been_requested') %> <%= email %>.

-

<%= t('email.confirm.to_confirm') %> Confirm Email

+

<%= t('email.confirm.to_confirm') %> Confirm Email

<%= t('email.confirm.if_you_did_not') %>

diff --git a/services/email/email-confirm.txt.ejs b/services/email/email-confirm.txt.ejs index 6d3cb219c..d327220a7 100644 --- a/services/email/email-confirm.txt.ejs +++ b/services/email/email-confirm.txt.ejs @@ -4,6 +4,6 @@ <%= t('email.confirm.to_confirm') %> - <%= rootURL %>/confirm/endpoint#<%= token %> + <%= BASE_URL %>confirm/endpoint#<%= token %> <%= t('email.confirm.if_you_did_not') %> diff --git a/services/email/password-reset.html.ejs b/services/email/password-reset.html.ejs index 258f0d079..c0ec4ea46 100644 --- a/services/email/password-reset.html.ejs +++ b/services/email/password-reset.html.ejs @@ -1,2 +1,2 @@

<%= t('email.password_reset.we_received_a_request') %>
-<%= t('email.password_reset.if_you_did') %> <%= t('email.password_reset.please_click') %>.

+<%= t('email.password_reset.if_you_did') %> <%= t('email.password_reset.please_click') %>.

diff --git a/services/email/password-reset.txt.ejs b/services/email/password-reset.txt.ejs index a8387925d..e8db4bab2 100644 --- a/services/email/password-reset.txt.ejs +++ b/services/email/password-reset.txt.ejs @@ -1,3 +1,3 @@ <%= t('email.password_reset.we_received_a_request') %>. <%= t('email.password_reset.if_you_did') %> <%= t('email.password_reset.please_click') %>: -<%= rootURL %>/admin/password-reset#<%= token %> +<%= BASE_URL %>admin/password-reset#<%= token %> diff --git a/services/locals.js b/services/locals.js new file mode 100644 index 000000000..9a6a6bec8 --- /dev/null +++ b/services/locals.js @@ -0,0 +1,20 @@ +const { + BASE_URL, + BASE_PATH, + MOUNT_PATH, + STATIC_URL, +} = require('../url'); + +const applyLocals = (locals) => { + + // Apply the BASE_PATH, BASE_URL, and MOUNT_PATH on the app.locals, which will + // make them available on the templates and the routers. + locals.BASE_URL = BASE_URL; + locals.BASE_PATH = BASE_PATH; + locals.MOUNT_PATH = MOUNT_PATH; + locals.STATIC_URL = STATIC_URL; +}; + +module.exports = { + applyLocals, +}; diff --git a/services/mailer.js b/services/mailer.js index 68252bea3..8d650726d 100644 --- a/services/mailer.js +++ b/services/mailer.js @@ -4,6 +4,7 @@ const kue = require('./kue'); const path = require('path'); const fs = require('fs'); const _ = require('lodash'); +const {applyLocals} = require('./locals'); const i18n = require('./i18n'); @@ -92,6 +93,9 @@ const mailer = module.exports = { // Prefix the subject with `[Talk]`. subject = `[Talk] ${subject}`; + applyLocals(locals); + + // Attach the templating function. locals['t'] = i18n.t; return Promise.all([ diff --git a/services/users.js b/services/users.js index faea67d75..1dc9bceb6 100644 --- a/services/users.js +++ b/services/users.js @@ -610,12 +610,16 @@ module.exports = class UsersService { * @param {String} token the JSON Web Token to verify */ static async verifyPasswordResetToken(token) { - const {userId, loc} = await UsersService.verifyToken(token, { + const {userId, loc, version} = await UsersService.verifyToken(token, { subject: PASSWORD_RESET_JWT_SUBJECT }); const user = await UsersService.findById(userId); + if (version !== user.__v) { + throw new Error('password reset token has expired'); + } + return [user, loc]; } diff --git a/views/admin/password-reset.ejs b/views/admin/password-reset.ejs index f02e5b6f8..6725409d1 100644 --- a/views/admin/password-reset.ejs +++ b/views/admin/password-reset.ejs @@ -15,11 +15,13 @@ background: #fff; } - #root form { + .container { max-width: 300px; - border: 1px solid lightgrey; - box-shadow: 0px 10px 24px 2px rgba(0,0,0,0.2); margin: 50px auto; + } + + #root form { + display: none; padding: 15px; } @@ -81,7 +83,8 @@
-
+
+ Set new password -
foo
From 9ae59ef9985886b5768e843e21c5cc5061013f3b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 26 Sep 2017 09:34:37 -0600 Subject: [PATCH 3/3] added parent url to the login popup --- client/coral-embed-stream/src/components/Embed.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/coral-embed-stream/src/components/Embed.js b/client/coral-embed-stream/src/components/Embed.js index 86d36d47e..7fc6d1474 100644 --- a/client/coral-embed-stream/src/components/Embed.js +++ b/client/coral-embed-stream/src/components/Embed.js @@ -29,7 +29,7 @@ export default class Embed extends React.Component { }; render() { - const {activeTab, commentId, root, data, auth: {showSignInDialog, signInDialogFocus}, blurSignInDialog, focusSignInDialog, hideSignInDialog} = this.props; + const {activeTab, commentId, root, data, auth: {showSignInDialog, signInDialogFocus}, blurSignInDialog, focusSignInDialog, hideSignInDialog, router: {location: {query: {parentUrl}}}} = this.props; const {user} = this.props.auth; const hasHighlightedComment = !!commentId; @@ -37,7 +37,7 @@ export default class Embed extends React.Component {