Merge pull request #1496 from mistydemeo/fix_circular_reference

Brain: avoid retaining dangerous robot/brain references

package.json

@@ -28,6 +28,7 @@
   "devDependencies": {
     "chai": "~2.1.0",
     "coveralls": "^3.0.2",
+    "is-circular": "^1.0.2",
     "mocha": "^5.2.0",
     "mockery": "^1.4.0",
     "nyc": "^13.0.0",

src/brain.js

@@ -22,8 +22,10 @@ let reconstructUserIfNecessary = function (user, robot) {
     // populating the new user with its values.
     // Also add the `robot` field so it gets a reference.
     user.robot = robot
+    let newUser = new User(id, user)
+    delete user.robot
 
-    return new User(id, user)
+    return newUser
   } else {
     return user
   }
@@ -39,7 +41,9 @@ class Brain extends EventEmitter {
       users: {},
       _private: {}
     }
-    this.robot = robot
+    this.getRobot = function () {
+      return robot
+    }
 
     this.autoSave = true
 
@@ -146,7 +150,7 @@ class Brain extends EventEmitter {
     if (data && data.users) {
       for (let k in data.users) {
         let user = this.data.users[k]
-        this.data.users[k] = reconstructUserIfNecessary(user, this.robot)
+        this.data.users[k] = reconstructUserIfNecessary(user, this.getRobot())
       }
     }
 
@@ -168,7 +172,7 @@ class Brain extends EventEmitter {
     if (!options) {
       options = {}
     }
-    options.robot = this.robot
+    options.robot = this.getRobot()
 
     if (!user) {
       user = new User(id, options)
@@ -179,6 +183,7 @@ class Brain extends EventEmitter {
       user = new User(id, options)
       this.data.users[id] = user
     }
+    delete options.robot
 
     return user
   }

test/brain_test.js

@@ -10,6 +10,8 @@ chai.use(require('sinon-chai'))
 
 const expect = chai.expect
 
+const isCircular = require('is-circular')
+
 // Hubot classes
 const Brain = require('../src/brain')
 const User = require('../src/user')
@@ -65,6 +67,7 @@ describe('Brain', function () {
         expect(user.constructor.name).to.equal('User')
         expect(user.id).to.equal('4')
         expect(user.name).to.equal('new')
+        expect(isCircular(this.brain)).to.be.false
       })
     })
 
@@ -326,6 +329,8 @@ describe('Brain', function () {
       for (let user of this.brain.usersForRawFuzzyName('Guy One')) {
         expect(user.constructor.name).to.equal('User')
       }
+
+      expect(isCircular(this.brain)).to.be.false
     })
   })
 })