From 9e2887af3e0fdc952d7783ccf465b3bea2c72aa7 Mon Sep 17 00:00:00 2001
From: crazybg321 <ivan.milchev321@gmail.com>
Date: Tue, 28 Aug 2018 16:30:54 +0200
Subject: [PATCH] [Security] Fixed issue #214 (#216)

* fixed sending will when authorization fails

* updated variable names in test
---
 lib/handlers/connect.js | 17 ++++++++++++-----
 test/will.js            | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/lib/handlers/connect.js b/lib/handlers/connect.js
index d408d26e..955d9b74 100644
--- a/lib/handlers/connect.js
+++ b/lib/handlers/connect.js
@@ -18,6 +18,7 @@ var connectActions = [
   fetchSubs,
   restoreSubs,
   storeWill,
+  registerClient,
   doConnack,
   emptyQueue
 ]
@@ -41,7 +42,7 @@ function handleConnect (client, packet, done) {
   }
 
   client.id = packet.clientId || uuid.v4()
-  client.will = packet.will
+  client._will = packet.will
 
   clearTimeout(client._connectTimer)
   client._connectTimer = null
@@ -71,7 +72,6 @@ function authenticate (arg, done) {
   function negate (err, successful) {
     var errCode
     if (!err && successful) {
-      client.broker.registerClient(client)
       return done()
     } else if (err) {
       if (err.returnCode && (err.returnCode >= 1 && err.returnCode <= 3)) {
@@ -79,21 +79,21 @@ function authenticate (arg, done) {
         write(client, {
           cmd: 'connack',
           returnCode: err.returnCode
-        }, client.close.bind(client, done))
+        }, client.close.bind(client, done.bind(this, err)))
       } else {
         // If errorCode is 4 or not a number
         errCode = 4
         write(client, {
           cmd: 'connack',
           returnCode: 4
-        }, client.close.bind(client, done))
+        }, client.close.bind(client, done.bind(this, err)))
       }
     } else {
       errCode = 5
       write(client, {
         cmd: 'connack',
         returnCode: 5
-      }, client.close.bind(client, done))
+      }, client.close.bind(client, done.bind(this, new Error(errorMessages[errCode]))))
     }
     var error = new Error(errorMessages[errCode])
     error.errorCode = errCode
@@ -132,6 +132,7 @@ function restoreSubs (arg, done) {
 }
 
 function storeWill (arg, done) {
+  this.client.will = this.client._will
   if (this.client.will) {
     this.client.broker.persistence.putWill(
       this.client,
@@ -142,6 +143,12 @@ function storeWill (arg, done) {
   }
 }
 
+function registerClient (arg, done) {
+  var client = this.client
+  client.broker.registerClient(client)
+  done()
+}
+
 function Connack (arg) {
   this.cmd = 'connack'
   this.returnCode = 0
diff --git a/test/will.js b/test/will.js
index a9207293..7772c046 100644
--- a/test/will.js
+++ b/test/will.js
@@ -224,3 +224,43 @@ test('does not deliver a will without authorization', function (t) {
 
   s.conn.destroy()
 })
+
+test('does not deliver a will without authentication', function (t) {
+  let authenticated = false
+  var opts = {}
+  // willConnect populates opts with a will
+  var s = willConnect(setup(aedes({ authenticate: (_1, _2, _3, callback) => { authenticated = true; callback(new Error(), false) } })), opts)
+
+  s.broker.on('clientError', function () {
+    t.equal(authenticated, true, 'authentication called')
+    t.end()
+  })
+
+  s.broker.mq.on('mywill', function (packet, cb) {
+    t.fail('received will without authentication')
+    cb()
+  })
+})
+
+test('does not deliver will if keepalive is triggered during authentication', function (t) {
+  var opts = {}
+  opts.keepalive = 1
+  var broker = aedes({
+    authenticate: function (c, u, p, cb) {
+      setTimeout(function () {
+        cb(null, true)
+      }, 3000)
+    }
+  })
+
+  broker.on('keepaliveTimeout', function () {
+    t.end()
+  })
+
+  broker.mq.on('mywill', function (packet, cb) {
+    cb()
+    t.fail('Received will when it was not expected')
+  })
+
+  willConnect(setup(broker), opts)
+})