Fixes to password reset flow

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);

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);
   }
 
 }

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];
   }
 
   /**

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;