From 9920993ca4e52e3dd497b5932569aa05c527722d Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:03:39 -0700 Subject: [PATCH 01/14] Add github action for tseting a PR branch on push --- .github/workflows/test.yml | 43 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 .github/workflows/test.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..49490ae --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,43 @@ +name: Test code + +on: + workflow_dispatch: + pull_request: + +permissions: + contents: read + +# This allows a subsequently queued workflow run to interrupt previous runs +concurrency: + group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' + cancel-in-progress: true + +jobs: + lint: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + + - name: Check out repo + uses: actions/checkout@v4 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + - name: Build and run linter + uses: docker/build-push-action@v5 + with: + context: . + file: .docker/Dockerfile + tags: test-chatbot + cache-from: type=gha + cache-to: type=gha,mode=max + outputs: type=docker + + - name: Run linter + run: | + docker run --rm --memory=1g \ + -v ${{ github.workspace }}:/chatbot_server \ + -w /chatbot_server \ + lint-chatbot-server \ + sh -c "pnpm install --frozen-lockfile && pnpm test" \ No newline at end of file From d77a66527f75c4d9671d399323d30802d61b4b05 Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:07:21 -0700 Subject: [PATCH 02/14] Add copy scripts/ line to dockerfile --- .docker/Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/.docker/Dockerfile b/.docker/Dockerfile index c04abec..a283ae4 100644 --- a/.docker/Dockerfile +++ b/.docker/Dockerfile @@ -6,6 +6,7 @@ RUN corepack use pnpm@* ARG WORKDIR=/chatbot_server WORKDIR ${WORKDIR} +COPY scripts/ ${WORKDIR}/scripts/ # Adding package.json first will cache our dependencies so # that they do not have to be re-installed when the image rebuilds From 6c853627d36c1cb27de35f093a4ab8281bbb05f6 Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:16:24 -0700 Subject: [PATCH 03/14] Replace add with copy for gitconfig --- .docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.docker/Dockerfile b/.docker/Dockerfile index a283ae4..16b625c 100644 --- a/.docker/Dockerfile +++ b/.docker/Dockerfile @@ -21,6 +21,6 @@ COPY . . RUN chmod +x ./scripts/*.sh || true # Set up git RUN apk add git -ADD git-config.txt ${WORKDIR} +COPY git-config.txt ${WORKDIR} EXPOSE 7000 \ No newline at end of file From 74850b6e530edbd937e912855baf0ac892f7e199 Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:20:49 -0700 Subject: [PATCH 04/14] Sync tax for test-chatbot, remove cache-to, cache-from --- .github/workflows/test.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 49490ae..1e45885 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -30,8 +30,6 @@ jobs: context: . file: .docker/Dockerfile tags: test-chatbot - cache-from: type=gha - cache-to: type=gha,mode=max outputs: type=docker - name: Run linter @@ -39,5 +37,5 @@ jobs: docker run --rm --memory=1g \ -v ${{ github.workspace }}:/chatbot_server \ -w /chatbot_server \ - lint-chatbot-server \ + test-chatbot \ sh -c "pnpm install --frozen-lockfile && pnpm test" \ No newline at end of file From 771f04d8e470d8d3fde1cc817352fab7f32fb668 Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:24:41 -0700 Subject: [PATCH 05/14] Set docker buildkit to 0 in gitconfig --- .github/workflows/test.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1e45885..dbaaaf3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,7 @@ concurrency: cancel-in-progress: true jobs: - lint: + test: runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -32,7 +32,9 @@ jobs: tags: test-chatbot outputs: type=docker - - name: Run linter + - name: Run tests + env: + DOCKER_BUILDKIT: 0 run: | docker run --rm --memory=1g \ -v ${{ github.workspace }}:/chatbot_server \ From 93b64d8212021d620230254d9dd380c09e022b97 Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Tue, 24 Jun 2025 13:29:14 -0700 Subject: [PATCH 06/14] Add conditional for copying git config if exists, otherwise --- .docker/Dockerfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.docker/Dockerfile b/.docker/Dockerfile index 16b625c..cce3dfb 100644 --- a/.docker/Dockerfile +++ b/.docker/Dockerfile @@ -21,6 +21,7 @@ COPY . . RUN chmod +x ./scripts/*.sh || true # Set up git RUN apk add git -COPY git-config.txt ${WORKDIR} + +RUN if [ -f git-config.txt ]; then cp git-config.txt ~/.gitconfig; fi EXPOSE 7000 \ No newline at end of file From 5907ac9968e23e9de9f758fc38fff13c8a93c8bb Mon Sep 17 00:00:00 2001 From: Mark de Dios Date: Sun, 6 Jul 2025 17:29:13 -0700 Subject: [PATCH 07/14] Add twilio incoming messages schema for validation --- src/middlewares/webhook.middleware.js | 0 src/validations/index.js | 1 + .../twilio/incoming.message.validation.js | 151 ++++++++++++++++++ 3 files changed, 152 insertions(+) create mode 100644 src/middlewares/webhook.middleware.js create mode 100644 src/validations/twilio/incoming.message.validation.js diff --git a/src/middlewares/webhook.middleware.js b/src/middlewares/webhook.middleware.js new file mode 100644 index 0000000..e69de29 diff --git a/src/validations/index.js b/src/validations/index.js index a708e33..fd4ee68 100644 --- a/src/validations/index.js +++ b/src/validations/index.js @@ -1 +1,2 @@ module.exports.envValidation = require('./env.validation'); +module.exports.incomingTwilioMessageValidation = require('./twilio/incoming.message.validation'); diff --git a/src/validations/twilio/incoming.message.validation.js b/src/validations/twilio/incoming.message.validation.js new file mode 100644 index 0000000..cc68625 --- /dev/null +++ b/src/validations/twilio/incoming.message.validation.js @@ -0,0 +1,151 @@ +const joi = require('joi '); + +/** + * + * @param {String} value + * @param {import('joi').CustomValidator} helpers + * @returns {import('joi').ErrorReport|String} + */ +function validatePhoneNumber(value, helpers) { + // Must start with + and have 10-15 digits after + const phoneRegex = /^\+[1-9]\d{1,14}$/; + + if (!phoneRegex.test(value)) { + return helpers.error('any.invalid'); + } + + return value; +}; + +/** + * + * @param {String} value + * @param {import('joi').CustomHelpers} helpers + * @returns {import('joi').ErrorReport|String } + */ +function validateSafeString(value, helpers) { + // Check for common SQL injection patterns + const dangerousPatterns = [ + /('|(\\?')|(;|\\?;))/i, // Single quotes and semicolons + /--(\\s|$)/i, // SQL comments + /\b(DROP|DELETE|INSERT|UPDATE|SELECT|UNION|CREATE|ALTER|EXEC|EXECUTE)\b/i, // SQL keywords + /', + '', + 'normal text ', + '', + '' + ]; + + dangerousStrings.forEach(str => { + const result = validateSafeString(str, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + }); + + it('should reject strings with javascript protocol', () => { + const dangerousStrings = [ + 'javascript:alert("xss")', + 'JAVASCRIPT:alert("xss")', + 'normal text javascript:alert("xss")', + 'javascript:document.cookie', + 'javascript:void(0)' + ]; + + dangerousStrings.forEach(str => { + const result = validateSafeString(str, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + }); + }); + + describe('Edge cases', () => { + it('should handle null and undefined values', () => { + const invalidValues = [null, undefined]; + + invalidValues.forEach(value => { + const result = validateSafeString(value, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + }); + + it('should handle non-string values', () => { + const invalidValues = [123, {}, [], true, false]; + + invalidValues.forEach(value => { + const result = validateSafeString(value, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + }); + + it('should handle strings that contain safe parts of dangerous patterns', () => { + const safeStrings = [ + 'script is a word', + 'javascript is a programming language', + 'SELECT is a verb', + 'DROP is a verb', + 'INSERT is a verb', + 'UPDATE is a verb', + 'CREATE is a verb', + 'ALTER is a verb', + 'EXEC is short for execute', + 'UNION is a word' + ]; + + safeStrings.forEach(str => { + const result = validateSafeString(str, mockHelpers); + + expect(result).toBe(str); + expect(mockHelpers.error).not.toHaveBeenCalled(); + }); + }); + }); + + describe('Function behavior', () => { + it('should return the original value for safe strings', () => { + const safeString = 'Hello, world!'; + const result = validateSafeString(safeString, mockHelpers); + + expect(result).toBe(safeString); + expect(typeof result).toBe('string'); + }); + + it('should call helpers.error with correct error code for dangerous strings', () => { + const dangerousString = 'SELECT * FROM users'; + + validateSafeString(dangerousString, mockHelpers); + + expect(mockHelpers.error).toHaveBeenCalledTimes(1); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + + it('should return error object from helpers.error for dangerous strings', () => { + const dangerousString = 'SELECT * FROM users'; + const result = validateSafeString(dangerousString, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + }); + + it('should detect the first dangerous pattern in a string', () => { + const dangerousString = 'normal text more text'; + const result = validateSafeString(dangerousString, mockHelpers); + + expect(result).toEqual({ code: 'string.unsafe', message: 'Validation error: string.unsafe' }); + expect(mockHelpers.error).toHaveBeenCalledWith('string.unsafe'); + }); + }); +}); \ No newline at end of file diff --git a/src/__tests__/utils/withRetryAndCatch.test.js b/src/__tests__/utils/withRetryAndCatch.test.js index eba3452..9c1c2f9 100644 --- a/src/__tests__/utils/withRetryAndCatch.test.js +++ b/src/__tests__/utils/withRetryAndCatch.test.js @@ -11,8 +11,7 @@ describe('withRetryAndCatch', () => { jest.resetModules(); jest.doMock('../../utils/withRetry', () => { return jest.fn((fn) => { - // Just return the handler as-is for test purposes - + // Return the handler as-is for test purposes return fn; }); }); @@ -24,17 +23,14 @@ describe('withRetryAndCatch', () => { const maybePromise = fn(...args); if (maybePromise && typeof maybePromise.catch === 'function') { - return maybePromise.catch(next); } return maybePromise; } catch (err) { if (typeof next === 'function') { - next(err); } else { - throw err; } } @@ -256,7 +252,9 @@ describe('withRetryAndCatch', () => { it('should work with async/await handlers', async () => { const asyncHandler = jest.fn(async (req, res) => { - await new Promise(resolve => setTimeout(resolve, 10)); + await new Promise(resolve => { + setTimeout(() => resolve(), 10); + }); return { processed: true }; }); diff --git a/src/routes/messaging.routes.js b/src/routes/messaging.routes.js index 36341a6..e51bcde 100644 --- a/src/routes/messaging.routes.js +++ b/src/routes/messaging.routes.js @@ -1,11 +1,11 @@ const express = require('express'); const { messagingController } = require('../controllers'); const validate = require('../middlewares/validate'); -const { incomingMessageValidation } = require('../validations'); +const { twilioIncomingMessageValidation } = require('../validations'); const router = express.Router(); -router.post('/webhook/incoming-message', validate(incomingMessageValidation), messagingController.handleIncomingMessage); +router.post('/webhook/incoming-message', validate(twilioIncomingMessageValidation), messagingController.handleIncomingMessage); router.get('/', (req, res) => { res.send('📨 Messaging API is working'); diff --git a/src/services/messaging/messaging.service.js b/src/services/messaging/messaging.service.js index 668e22d..f8fe3d4 100644 --- a/src/services/messaging/messaging.service.js +++ b/src/services/messaging/messaging.service.js @@ -1,7 +1,10 @@ +const { env } = require('../../config/config'); const logger = require('../../config/logger'); const twilioClient = require('../../config/twilio'); const SOURCES = require('../../utils/constants'); +const shouldValidate = env !== 'test'; + /** * * @param {Express.Request} req @@ -11,7 +14,7 @@ function processMessage(req, requestId) { const source = identifySource(req); if (source === SOURCES.TWILIO) { - twilioClient.webhook()(req); + twilioClient.webhook({ validate: shouldValidate })(req); logger.info('Received Twilio Message', { requestId, diff --git a/src/utils/validatePhoneNumber.js b/src/utils/validatePhoneNumber.js new file mode 100644 index 0000000..dbc4435 --- /dev/null +++ b/src/utils/validatePhoneNumber.js @@ -0,0 +1,22 @@ +/** + * @param {String} value + * @param {import('joi').CustomValidator} helpers + * @returns {import('joi').ErrorReport|String} + */ +function validatePhoneNumber(value, helpers) { + // Must be a string + if (typeof value !== 'string') { + return helpers.error('any.invalid'); + } + // Must start with +1 followed by exactly 10 digits (US/Canada NANP format) + const phoneRegex = /^\+1\d{10}$/; + + if (!phoneRegex.test(value)) { + return helpers.error('any.invalid'); + } + + return value; +}; + +module.exports = validatePhoneNumber; + diff --git a/src/utils/validateSafeString.js b/src/utils/validateSafeString.js new file mode 100644 index 0000000..499bfd7 --- /dev/null +++ b/src/utils/validateSafeString.js @@ -0,0 +1,30 @@ +/** + * @param {String} value + * @param {import('joi').CustomHelpers} helpers + * @returns {import('joi').ErrorReport|String} + */ +function validateSafeString(value, helpers) { + // Check if value is a string + if (typeof value !== 'string') { + return helpers.error('string.unsafe'); + } + + // Check for common SQL injection and XSS patterns + const dangerousPatterns = [ + /--/, // SQL comments: any occurrence of -- + // SQL keywords with other SQL syntax (e.g., SELECT ... FROM, DROP TABLE, etc.) + /\b(SELECT|DROP|DELETE|INSERT|UPDATE|UNION|CREATE|ALTER|EXEC|EXECUTE)\b\s+.*\b(FROM|TABLE|INTO|SET|PROCEDURE|VALUES|WHERE|JOIN|ON|BY)\b/i, + /