Merge pull request juice-shop#1912 from juice-shop/fix/chatbot-crash

Fix Chatbot Crash
eneyeitech · Oct 26, 2022 · 5f0db2e · 5f0db2e
2 parents b163b41 + d89c3e8
commit 5f0db2e
Show file tree

Hide file tree

Showing 11 changed files with 122 additions and 64 deletions.
diff --git a/routes/chatbot.ts b/routes/chatbot.ts
@@ -46,7 +46,7 @@ async function initialize () {
 
 void initialize()
 
-async function processQuery (user: User, req: Request, res: Response) {
+async function processQuery (user: User, req: Request, res: Response, next: NextFunction) {
   const username = user.username
   if (!username) {
     res.status(200).json({
@@ -57,16 +57,26 @@ async function processQuery (user: User, req: Request, res: Response) {
   }
 
   if (!bot.factory.run(`currentUser('${user.id}')`)) {
-    bot.addUser(`${user.id}`, username)
-    res.status(200).json({
-      action: 'response',
-      body: bot.greet(`${user.id}`)
-    })
+    try {
+      bot.addUser(`${user.id}`, username)
+      res.status(200).json({
+        action: 'response',
+        body: bot.greet(`${user.id}`)
+      })
+    } catch (err) {
+      next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
+    }
     return
   }
 
   if (bot.factory.run(`currentUser('${user.id}')`) !== username) {
     bot.addUser(`${user.id}`, username)
+    try {
+      bot.addUser(`${user.id}`, username)
+    } catch (err) {
+      next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
+      return
+    }
   }
 
   if (!req.body.query) {
@@ -172,12 +182,15 @@ module.exports.status = function status () {
         return
       }
 
-      bot.addUser(`${user.id}`, username)
-
-      res.status(200).json({
-        status: bot.training.state,
-        body: bot.training.state ? bot.greet(`${user.id}`) : `${config.get('application.chatBot.name')} isn't ready at the moment, please wait while I set things up`
-      })
+      try {
+        bot.addUser(`${user.id}`, username)
+        res.status(200).json({
+          status: bot.training.state,
+          body: bot.training.state ? bot.greet(`${user.id}`) : `${config.get('application.chatBot.name')} isn't ready at the moment, please wait while I set things up`
+        })
+      } catch (err) {
+        next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
+      }
       return
     }
 
@@ -221,7 +234,7 @@ module.exports.process = function respond () {
     }
 
     if (req.body.action === 'query') {
-      await processQuery(user, req, res)
+      await processQuery(user, req, res, next)
     } else if (req.body.action === 'setname') {
       setUserName(user, req, res)
     }

diff --git a/routes/dataErasure.ts b/routes/dataErasure.ts
@@ -18,7 +18,7 @@ const router = express.Router()
 router.get('/', async (req: Request, res: Response, next: NextFunction): Promise<void> => {
   const loggedInUser = insecurity.authenticatedUsers.get(req.cookies.token)
   if (!loggedInUser) {
-    next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+    next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
     return
   }
   const email = loggedInUser.data.email
@@ -54,7 +54,7 @@ interface DataErasureRequestParams {
 router.post('/', async (req: Request<{}, {}, DataErasureRequestParams>, res: Response, next: NextFunction): Promise<void> => {
   const loggedInUser = insecurity.authenticatedUsers.get(req.cookies.token)
   if (!loggedInUser) {
-    next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+    next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
     return
   }
 

diff --git a/routes/dataExport.ts b/routes/dataExport.ts
@@ -109,7 +109,7 @@ module.exports = function dataExport () {
         next(new Error(`Error retrieving orders for ${updatedEmail}`))
       })
     } else {
-      next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+      next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
     }
   }
 }
diff --git a/routes/orderHistory.ts b/routes/orderHistory.ts
@@ -18,7 +18,7 @@ module.exports.orderHistory = function orderHistory () {
       const order = await orders.find({ email: updatedEmail })
       res.status(200).json({ status: 'success', data: order })
     } else {
-      next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+      next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
     }
   }
 }

diff --git a/routes/profileImageFileUpload.ts b/routes/profileImageFileUpload.ts
@@ -43,7 +43,7 @@ module.exports = function fileUpload () {
           res.location(process.env.BASE_PATH + '/profile')
           res.redirect(process.env.BASE_PATH + '/profile')
         } else {
-          next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+          next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
         }
       } else {
         res.status(415)

diff --git a/routes/profileImageUrlUpload.ts b/routes/profileImageUrlUpload.ts
@@ -33,7 +33,7 @@ module.exports = function profileImageUrlUpload () {
             } else UserModel.findByPk(loggedInUser.data.id).then(async (user: UserModel | null) => { return await user?.update({ profileImage: url }) }).catch((error: Error) => { next(error) })
           })
       } else {
-        next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+        next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
       }
     }
     res.location(process.env.BASE_PATH + '/profile')

diff --git a/routes/saveLoginIp.ts b/routes/saveLoginIp.ts
@@ -23,7 +23,7 @@ module.exports = function saveLoginIp () {
         lastLoginIp = security.sanitizeSecure(lastLoginIp)
       }
       if (lastLoginIp === undefined) {
-        lastLoginIp = utils.toSimpleIpAddress(req.connection.remoteAddress)
+        lastLoginIp = utils.toSimpleIpAddress(req.socket.remoteAddress)
       }
       UserModel.findByPk(loggedInUser.data.id).then((user: UserModel | null) => {
         user?.update({ lastLoginIp: lastLoginIp?.toString() }).then((user: UserModel) => {

diff --git a/routes/updateUserProfile.ts b/routes/updateUserProfile.ts
@@ -37,7 +37,7 @@ module.exports = function updateUserProfile () {
         next(error)
       })
     } else {
-      next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+      next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
     }
   }
 }
diff --git a/routes/userProfile.ts b/routes/userProfile.ts
@@ -66,7 +66,7 @@ module.exports = function getUserProfile () {
           next(error)
         })
       } else {
-        next(new Error('Blocked illegal activity by ' + req.connection.remoteAddress))
+        next(new Error('Blocked illegal activity by ' + req.socket.remoteAddress))
       }
     })
   }

diff --git a/test/api/chatBotSpec.ts b/test/api/chatBotSpec.ts
@@ -9,8 +9,9 @@ const { initialize, bot } = require('../../routes/chatbot')
 const fs = require('fs')
 const utils = require('../../lib/utils')
 
-const REST_URL = 'http://localhost:3000/rest/'
-const API_URL = 'http://localhost:3000/api/'
+const URL = 'http://localhost:3000'
+const REST_URL = `${URL}/rest/`
+const API_URL = `${URL}/api/`
 let trainingData: { data: any[] }
 
 async function login ({ email, password }: { email: string, password: string }) {
@@ -54,7 +55,7 @@ describe('/chatbot', () => {
         password: '0Y8rMnww$*9VFYE§59-!Fg1L6t&6lB'
       })
 
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -64,6 +65,7 @@ describe('/chatbot', () => {
       }, true).get(REST_URL + 'chatbot/status')
         .expect('status', 200)
         .expect('json', 'body', /What shall I call you?/)
+        .promise()
     })
   })
 
@@ -76,7 +78,7 @@ describe('/chatbot', () => {
 
       const testCommand = trainingData.data[0].utterances[0]
 
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -93,6 +95,7 @@ describe('/chatbot', () => {
         .expect('status', 200)
         .expect('json', 'action', 'namequery')
         .expect('json', 'body', 'I\'m sorry I didn\'t get your name. What shall I call you?')
+        .promise()
     })
 
     it('Returns greeting if username is defined', async () => {
@@ -104,7 +107,7 @@ describe('/chatbot', () => {
       bot.addUser('1337', 'bkimminich')
       const testCommand = trainingData.data[0].utterances[0]
 
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -121,6 +124,7 @@ describe('/chatbot', () => {
         .expect('status', 200)
         .expect('json', 'action', 'response')
         .expect('json', 'body', bot.greet('1337'))
+        .promise()
     })
 
     it('Returns proper response for registered user', async () => {
@@ -130,7 +134,7 @@ describe('/chatbot', () => {
       })
       bot.addUser('12345', 'bkimminich')
       const testCommand = trainingData.data[0].utterances[0]
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -151,6 +155,7 @@ describe('/chatbot', () => {
           }
         })
         .expect('status', 200)
+        .promise()
         .then(({ json }) => {
           // @ts-expect-error
           expect(trainingData.data[0].answers).toContainEqual(json)
@@ -162,28 +167,29 @@ describe('/chatbot', () => {
         email: '[email protected]',
         password: 'bW9jLmxpYW1nQGhjaW5pbW1pay5ucmVvamI='
       })
-      await void frisby.get(API_URL + '/Products/1')
+      const { json } = await frisby.get(API_URL + '/Products/1')
         .expect('status', 200)
-        .then(({ json }) => {
-          return frisby.setup({
-            request: {
-              headers: {
-                Authorization: `Bearer ${token}`,
-                'Content-Type': 'application/json'
-              }
-            }
-          }, true)
-            .post(REST_URL + 'chatbot/respond', {
-              body: {
-                action: 'query',
-                query: 'How much is ' + json.data.name + '?'
-              }
-            })
-            .expect('status', 200)
-            .expect('json', 'action', 'response')
-            .then(({ body = json.body }) => {
-              expect(body).toContain(`${json.data.name} costs ${json.data.price}¤`)
-            })
+        .promise()
+
+      await frisby.setup({
+        request: {
+          headers: {
+            Authorization: `Bearer ${token}`,
+            'Content-Type': 'application/json'
+          }
+        }
+      }, true)
+        .post(REST_URL + 'chatbot/respond', {
+          body: {
+            action: 'query',
+            query: 'How much is ' + json.data.name + '?'
+          }
+        })
+        .expect('status', 200)
+        .expect('json', 'action', 'response')
+        .promise()
+        .then(({ body = json.body }) => {
+          expect(body).toContain(`${json.data.name} costs ${json.data.price}¤`)
         })
     })
 
@@ -192,7 +198,7 @@ describe('/chatbot', () => {
         email: `stan@${config.get('application.domain')}`,
         password: 'ship coffin krypt cross estate supply insurance asbestos souvenir'
       })
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -209,6 +215,7 @@ describe('/chatbot', () => {
         .expect('status', 200)
         .expect('json', 'action', 'response')
         .expect('json', 'body', /NotGuybrushThreepwood/)
+        .promise()
     })
 
     it('POST returns error for unauthenticated user', () => {
@@ -238,7 +245,7 @@ describe('/chatbot', () => {
       })
       const testCommand = functionTest[0].utterances[0]
       const testResponse = '3be2e438b7f3d04c89d7749f727bb3bd'
-      await void frisby.setup({
+      await frisby.setup({
         request: {
           headers: {
             Authorization: `Bearer ${token}`,
@@ -261,6 +268,42 @@ describe('/chatbot', () => {
         .expect('status', 200)
         .expect('json', 'action', 'response')
         .expect('json', 'body', testResponse)
+        .promise()
+    })
+
+    it('Returns a 500 when the user name is set to crash request', async () => {
+      await frisby.post(`${API_URL}/Users`, {
+        headers: {
+          'Content-Type': 'application/json'
+        },
+        body: {
+          email: `chatbot-testuser@${config.get('application.domain')}`,
+          password: 'testtesttest',
+          username: '"',
+          role: 'admin'
+        }
+      }).promise()
+
+      const { token } = await login({
+        email: `chatbot-testuser@${config.get('application.domain')}`,
+        password: 'testtesttest'
+      })
+
+      const functionTest = trainingData.data.filter(data => data.intent === 'queries.functionTest')
+      const testCommand = functionTest[0].utterances[0]
+      await frisby.post(REST_URL + 'chatbot/respond', {
+        headers: {
+          Authorization: `Bearer ${token}`,
+          'Content-Type': 'application/json'
+        },
+        body: {
+          action: 'query',
+          query: testCommand
+        }
+      })
+        .inspectResponse()
+        .expect('status', 500)
+        .promise()
     })
   })
 })