feat(unblock): add unblock rate limits (#131); r=rfk

.travis.yml

@@ -7,8 +7,8 @@ env:
     - COVERALLS_REPO_TOKEN=zSbPkOBuZrK6NktxXW1ZzVEPOUDr9ePpN
 
 node_js:
- - '0.10'
  - '4'
+ - '6'
 
 sudo: false
 

lib/actions.js

@@ -29,7 +29,8 @@ var ACCOUNT_STATUS_ACTION = {
   accountDestroy: true,
   passwordChange: true,
   passwordForgotSendCode: true,
-  accountStatusCheck: true
+  accountStatusCheck: true,
+  sendUnblockCode: true
 }
 
 // Actions that send an email, and hence might make
@@ -38,7 +39,8 @@ var EMAIL_SENDING_ACTION = {
   accountCreate: true,
   recoveryEmailResendCode: true,
   passwordForgotSendCode: true,
-  passwordForgotResendCode: true
+  passwordForgotResendCode: true,
+  sendUnblockCode: true
 }
 
 module.exports = {

lib/config/config.js

@@ -73,6 +73,12 @@ module.exports = function (fs, path, url, convict) {
         format: 'nat',
         env: 'MAX_BAD_LOGINS_PER_IP'
       },
+      maxUnblockAttempts: {
+        doc: 'Number of login attempts that can be unblocked',
+        default: 5,
+        env: 'MAX_UNBLOCK_ATTEMPTS',
+        format: 'nat'
+      },
       maxVerifyCodes: {
         doc: 'Number code verifictions within rateLimitIntervalSeconds before throttling',
         default: 10,
@@ -85,7 +91,8 @@ module.exports = function (fs, path, url, convict) {
         env: 'BAD_LOGIN_ERRNO_WEIGHTS',
         default: {
           '102': 2,
-          '125': 4
+          '125': 4,
+          '126': 2
         }
       },
       ipRateLimitIntervalSeconds: {

lib/email_record.js

@@ -12,16 +12,18 @@ module.exports = function (limits, now) {
   function EmailRecord() {
     this.vc = []
     this.xs = []
+    this.ub = []
   }
 
   EmailRecord.parse = function (object) {
     var rec = new EmailRecord()
     object = object || {}
     rec.bk = object.bk       // timestamp when the account was banned
     rec.rl = object.rl       // timestamp when the account was rate-limited
-    rec.vc = object.vc || [] // timestamps when code verifications happened
-    rec.xs = object.xs || [] // timestamps when emails were sent
+    rec.vc = object.vc || rec.vc // timestamps when code verifications happened
+    rec.xs = object.xs || rec.xs // timestamps when emails were sent
     rec.pr = object.pr       // timestamp of the last password reset
+    rec.ub = object.ub || rec.ub
     return rec
   }
 
@@ -80,6 +82,31 @@ module.exports = function (limits, now) {
     this.vc.push(now())
   }
 
+  EmailRecord.prototype.addUnblock = function () {
+    this.ub.push(now())
+  }
+
+  EmailRecord.prototype.canUnblock = function () {
+    this.trimUnblocks(now())
+
+    return this.ub.length <= limits.maxUnblockAttempts
+  }
+
+  EmailRecord.prototype.trimUnblocks = function (now) {
+    if (this.ub.length === 0) { return }
+    // ub is naturally ordered from oldest to newest
+    // and we only need to keep up to limits.maxUnblockAttempts + 1
+
+    var i = this.ub.length - 1
+    var n = 0
+    var ub = this.ub[i]
+    while (ub > (now - limits.rateLimitIntervalMs) && n <= limits.maxUnblockAttempts) {
+      ub = this.ub[--i]
+      n++
+    }
+    this.ub = this.ub.slice(i + 1)
+  }
+
   EmailRecord.prototype.shouldBlock = function () {
     return this.isRateLimited() || this.isBlocked()
   }
@@ -111,12 +138,16 @@ module.exports = function (limits, now) {
     return Math.max(0, rateLimitAfter, banAfter)
   }
 
-  EmailRecord.prototype.update = function (action) {
+  EmailRecord.prototype.update = function (action, unblock) {
     // Reject immediately if they've been explicitly blocked.
     if (this.isBlocked()) {
       return this.retryAfter()
     }
 
+    if (unblock) {
+      this.addUnblock()
+    }
+
     // For code-checking actions, we may need to rate-limit.
     if (actions.isCodeVerifyingAction(action)) {
       // If they're already being blocked then don't count any more hits,

lib/limits.js

@@ -20,6 +20,7 @@ module.exports = function (config, mc, log) {
     this.maxEmails = settings.maxEmails
     this.maxBadLogins = settings.maxBadLogins
     this.maxBadLoginsPerIp = settings.maxBadLoginsPerIp
+    this.maxUnblockAttempts = settings.maxUnblockAttempts
     this.maxVerifyCodes = settings.maxVerifyCodes
     this.ipRateLimitIntervalSeconds = settings.ipRateLimitIntervalSeconds
     this.ipRateLimitIntervalMs = settings.ipRateLimitIntervalSeconds * 1000

lib/server.js

@@ -127,15 +127,16 @@ module.exports = function createServer(config, log) {
             if (ipRecord.isBlocked()) {
               // a blocked ip should just be ignored completely
               // it's malicious, it shouldn't penalize emails or allow
-              // any escape hatches. just abort!
+              // (most) escape hatches. just abort!
               return {
                 block: true,
                 retryAfter: ipRecord.retryAfter()
               }
             }
 
 
-            var blockEmail = emailRecord.update(action)
+            var wantsUnblock = req.body.payload && req.body.payload.unblockCode
+            var blockEmail = emailRecord.update(action, !!wantsUnblock)
             var blockIpEmail = ipEmailRecord.update(action)
             var blockIp = ipRecord.update(action, email)
 
@@ -156,6 +157,7 @@ module.exports = function createServer(config, log) {
                 retryAfter = 0
               }
             }
+            var canUnblock = emailRecord.canUnblock()
 
             // IP's that are in blocklist should be blocked
             // and not return a retryAfter because it is not known
@@ -177,6 +179,7 @@ module.exports = function createServer(config, log) {
                 function () {
                   return {
                     block: retryAfter > 0,
+                    unblock: canUnblock,
                     retryAfter: retryAfter
                   }
                 }

test/local/email_record_tests.js

@@ -13,7 +13,8 @@ function simpleEmailRecord() {
   var limits = {
     rateLimitIntervalMs: 500,
     blockIntervalMs: 800,
-    maxEmails: 2
+    maxEmails: 2,
+    maxUnblockAttempts: 2
   }
   return new (emailRecord(limits, now))()
 }
@@ -163,6 +164,20 @@ test(
   }
 )
 
+test(
+  'unblock',
+  function (t) {
+    var er = simpleEmailRecord()
+    er.addUnblock()
+    t.ok(er.canUnblock(), 'unblock limit is not reached')
+    er.addUnblock()
+    t.ok(er.canUnblock(), 'unblock limit is still not reached')
+    er.addUnblock()
+    t.equal(er.canUnblock(), false, 'maxUnblockAttempts exceeded')
+    t.end()
+  }
+)
+
 test(
   'parse works',
   function (t) {
@@ -205,6 +220,10 @@ test(
     t.equal(er.shouldBlock(), true, 'account is now blocked')
     t.equal(er.update('accountCreate'), 0, 'email action in a blocked account')
 
+    t.equal(er.ub.length, 0, 'no unblock attempts')
+    er.update('bogus', true)
+    t.equal(er.ub.length, 1, '1 unblock attempt')
+
     er.rl = 2000
     t.equal(er.shouldBlock(), true, 'account is blocked due to rate limiting')
     t.equal(er.isBlocked(), false, 'account is not outright banned')

test/memcache-helper.js

@@ -89,13 +89,19 @@ module.exports.blockedIpCheck = blockedIpCheck
 function badLoginCheck() {
   return P.all([
     mc.getAsync(TEST_IP + TEST_EMAIL),
-    mc.getAsync(TEST_IP)
+    mc.getAsync(TEST_IP),
+    mc.getAsync(TEST_EMAIL)
   ])
-  .spread(function (d1, d2) {
+  .spread(function (d1, d2, d3) {
     var ipEmailRecord = IpEmailRecord.parse(d1)
     var ipRecord = IpRecord.parse(d2)
+    var emailRecord = EmailRecord.parse(d3)
     mc.end()
-    return {ipEmailRecord: ipEmailRecord, ipRecord: ipRecord}
+    return {
+      ipEmailRecord: ipEmailRecord,
+      ipRecord: ipRecord,
+      emailRecord: emailRecord
+    }
   })
 }
 

test/remote/unblock_tests.js

@@ -0,0 +1,85 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+'use strict'
+
+const test = require('tap').test
+const restify = require('restify')
+const TestServer = require('../test_server')
+const Promise = require('bluebird')
+const mcHelper = require('../memcache-helper')
+
+const TEST_EMAIL = 'test@example.com'
+const TEST_IP = '192.0.2.1'
+
+const config = {
+  listen: {
+    port: 7000
+  }
+}
+
+const testServer = new TestServer(config)
+
+const client = restify.createJsonClient({
+  url: 'http://127.0.0.1:' + config.listen.port
+})
+
+Promise.promisifyAll(client, { multiArgs: true })
+
+test(
+  'startup',
+  t => {
+    testServer.start(err => {
+      t.type(testServer.server, 'object', 'test server was started')
+      t.notOk(err, 'no errors were returned')
+      t.end()
+    })
+  }
+)
+
+test(
+  'clear everything',
+  t => {
+    mcHelper.clearEverything(
+      err => {
+        t.notOk(err, 'no errors were returned')
+        t.end()
+      }
+    )
+  }
+)
+
+test(
+  'check with unblockCode in paylaod gets counted',
+  t => {
+    return client.postAsync('/check', {
+      email: TEST_EMAIL,
+      ip: TEST_IP,
+      action: 'accountLogin',
+      payload: {
+        unblockCode: 'abcd1234'
+      }
+    })
+    .spread((req, res, obj) => {
+      t.equal(res.statusCode, 200, 'first login attempt noted')
+      return mcHelper.badLoginCheck()
+    })
+    .then(records => {
+      t.equal(records.emailRecord.ub.length, 1, 'should have record 1 unblock attempt')
+      t.end()
+    })
+    .catch(err => {
+      t.fail(err)
+      t.end()
+    })
+  }
+)
+
+test(
+  'teardown',
+  t => {
+    testServer.stop()
+    t.equal(testServer.server.killed, true, 'test server has been killed')
+    t.end()
+  }
+)