fix: FORMS-1237 validate formSubmissionId parameters (#1457)

* fix: FORMS-1237 validate formSubmissionId parameters

* add tests

* added some philosophical docs

app/src/README.md

@@ -0,0 +1,67 @@
+# Backend API
+
+This is the source code for the Express API. It contains all routes (endpoints) needed to run the application.
+
+## Design
+
+The code is broken down into layers:
+
+- The `middleware` layer provides a collection of utility functions that validate requests, check permissions, etc.
+- The `route` layer uses `middleware` to validate requests and check permissions, and then performs work by calling a `controller`
+- The `controller` layer deals with HTTP requests and responses, and performs work by calling a `service`
+- The `service` layer contains business logic and manipulates data by calling the `model`
+- The `model` layer is the ORM that deals with the database
+
+> Note: Between the `controller` and `service` there is a logical division where nothing below the `controller` knows about requests and responses, and nothing above the `service` touches the `model`.
+
+### Middleware
+
+Express middleware always has to call `next`, otherwise the request hangs.
+
+- when called as `next()` it chains to the next piece of middleware
+- when called as `next(obj)` execution jumps to the error handler with `obj` as the error object
+
+We wrap middleware in a `try`/`catch` to make it obvious that `next` is always called:
+
+```javascript
+const middleware = async (req, _res, next) => {
+  try {
+    // If possible the helper functions should throw a Problem rather than
+    // return a response that needs handling logic.
+    const formId = _getValidFormId(req);
+    const foo = service.getForm(formId);
+
+    next();
+  } catch (error) {
+    next(error);
+  }
+};
+```
+
+#### Testing
+
+Testing the middleware should include but is not limited to:
+
+- TODO
+
+### Routes
+
+Sets up routes with Middleware and then calls a Controller.
+
+#### Testing
+
+Testing the routes should include but is not limited to:
+
+- TODO
+
+### Controllers
+
+Calls Services
+
+### Services
+
+Calls ORM
+
+### ORM
+
+Calls DB

app/src/forms/common/middleware/validateParameter.js

@@ -122,6 +122,24 @@ const validateFormId = async (_req, _res, next, formId) => {
   }
 };
 
+/**
+ * Validates that the :formSubmissionId route parameter exists and is a UUID.
+ *
+ * @param {*} _req the Express object representing the HTTP request - unused.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ * @param {*} formSubmissionId the :formSubmissionId value from the route.
+ */
+const validateFormSubmissionId = async (_req, _res, next, formSubmissionId) => {
+  try {
+    _validateUuid(formSubmissionId, 'formSubmissionId');
+
+    next();
+  } catch (error) {
+    next(error);
+  }
+};
+
 /**
  * Validates that the :formVersionDraftId route parameter exists and is a UUID.
  * This validator requires that the :formId route parameter also exists.
@@ -179,6 +197,7 @@ module.exports = {
   validateExternalAPIId,
   validateFileId,
   validateFormId,
+  validateFormSubmissionId,
   validateFormVersionDraftId,
   validateFormVersionId,
 };

app/src/forms/submission/routes.js

@@ -1,15 +1,18 @@
-const apiAccess = require('../auth/middleware/apiAccess');
 const routes = require('express').Router();
 
-const controller = require('./controller');
-const P = require('../common/constants').Permissions;
+const apiAccess = require('../auth/middleware/apiAccess');
 const { currentUser, hasSubmissionPermissions, filterMultipleSubmissions } = require('../auth/middleware/userAccess');
+const P = require('../common/constants').Permissions;
 const rateLimiter = require('../common/middleware').apiKeyRateLimiter;
 const validateParameter = require('../common/middleware/validateParameter');
 
+const controller = require('./controller');
+
 routes.use(currentUser);
 
 routes.param('documentTemplateId', validateParameter.validateDocumentTemplateId);
+routes.param('formId', validateParameter.validateFormId);
+routes.param('formSubmissionId', validateParameter.validateFormSubmissionId);
 
 routes.get('/:formSubmissionId', rateLimiter, apiAccess, hasSubmissionPermissions([P.SUBMISSION_READ]), async (req, res, next) => {
   await controller.read(req, res, next);

app/tests/unit/forms/auth/middleware/apiAccess.spec.js

@@ -1,5 +1,5 @@
 const { getMockReq, getMockRes } = require('@jest-mock/express');
-const { v4: uuidv4 } = require('uuid');
+const uuid = require('uuid');
 
 const apiAccess = require('../../../../../src/forms/auth/middleware/apiAccess');
 const formService = require('../../../../../src/forms/form/service');
@@ -8,10 +8,10 @@ const fileService = require('../../../../../src/forms/file/service');
 const { NotFoundError } = require('objection');
 
 describe('apiAccess', () => {
-  const fileId = uuidv4();
-  const formId = uuidv4();
-  const formSubmissionId = uuidv4();
-  const secret = uuidv4();
+  const fileId = uuid.v4();
+  const formId = uuid.v4();
+  const formSubmissionId = uuid.v4();
+  const secret = uuid.v4();
 
   const token = Buffer.from(`${formId}:${secret}`).toString('base64');
   const authHeader = `Basic ${token}`;

app/tests/unit/forms/common/middleware/validateParameter.spec.js

@@ -1,24 +1,24 @@
 const { getMockReq, getMockRes } = require('@jest-mock/express');
-const { v4: uuidv4 } = require('uuid');
+const uuid = require('uuid');
 
 const validateParameter = require('../../../../../src/forms/common/middleware/validateParameter');
 const externalApiService = require('../../../../../src/forms/form/externalApi/service');
 const formService = require('../../../../../src/forms/form/service');
 const submissionService = require('../../../../../src/forms/submission/service');
 
-const fileId = uuidv4();
-const formId = uuidv4();
-const formSubmissionId = uuidv4();
+const fileId = uuid.v4();
+const formId = uuid.v4();
+const formSubmissionId = uuid.v4();
 
 // Various types of invalid UUIDs that we see in API calls.
-const invalidUuids = [[''], ['undefined'], ['{{id}}'], ['${id}'], [uuidv4() + '.'], [' ' + uuidv4() + ' ']];
+const invalidUuids = [[''], ['undefined'], ['{{id}}'], ['${id}'], [uuid.v4() + '.'], [' ' + uuid.v4() + ' ']];
 
 afterEach(() => {
   jest.clearAllMocks();
 });
 
 describe('validateDocumentTemplateId', () => {
-  const documentTemplateId = uuidv4();
+  const documentTemplateId = uuid.v4();
 
   const mockReadDocumentTemplateResponse = {
     formId: formId,
@@ -86,7 +86,7 @@ describe('validateDocumentTemplateId', () => {
 
     test('formId does not match', async () => {
       formService.documentTemplateRead.mockReturnValueOnce({
-        formId: uuidv4(),
+        formId: uuid.v4(),
         id: documentTemplateId,
       });
       const req = getMockReq({
@@ -107,7 +107,7 @@ describe('validateDocumentTemplateId', () => {
     test('submission formId does not match', async () => {
       submissionService.read.mockReturnValueOnce({
         form: {
-          id: uuidv4(),
+          id: uuid.v4(),
         },
       });
       const req = getMockReq({
@@ -200,7 +200,7 @@ describe('validateDocumentTemplateId', () => {
 });
 
 describe('validateExternalApiId', () => {
-  const externalApiId = uuidv4();
+  const externalApiId = uuid.v4();
 
   const mockReadExternalApiResponse = {
     formId: formId,
@@ -261,7 +261,7 @@ describe('validateExternalApiId', () => {
 
     test('formId does not match', async () => {
       externalApiService.readExternalAPI.mockReturnValueOnce({
-        formId: uuidv4(),
+        formId: uuid.v4(),
         id: externalApiId,
       });
       const req = getMockReq({
@@ -402,8 +402,51 @@ describe('validateFormId', () => {
   });
 });
 
+describe('validateFormSubmissionId', () => {
+  describe('400 response when', () => {
+    const expectedStatus = { status: 400 };
+
+    test('formSubmissionId is missing', async () => {
+      const req = getMockReq({
+        params: {},
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFormSubmissionId(req, res, next);
+
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test.each(invalidUuids)('formSubmissionId is "%s"', async (eachFormSubmissionId) => {
+      const req = getMockReq({
+        params: { formSubmissionId: eachFormSubmissionId },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFormSubmissionId(req, res, next, eachFormSubmissionId);
+
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+  });
+
+  describe('allows', () => {
+    test('uuid for formSubmissionId', async () => {
+      const req = getMockReq({
+        params: {
+          formSubmissionId: formSubmissionId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFormSubmissionId(req, res, next, formSubmissionId);
+
+      expect(next).toBeCalledWith();
+    });
+  });
+});
+
 describe('validateFormVersionDraftId', () => {
-  const formVersionDraftId = uuidv4();
+  const formVersionDraftId = uuid.v4();
 
   const mockReadDraftResponse = {
     formId: formId,
@@ -461,7 +504,7 @@ describe('validateFormVersionDraftId', () => {
 
     test('formId does not match', async () => {
       formService.readDraft.mockReturnValueOnce({
-        formId: uuidv4(),
+        formId: uuid.v4(),
         id: formVersionDraftId,
       });
       const req = getMockReq({
@@ -517,7 +560,7 @@ describe('validateFormVersionDraftId', () => {
 });
 
 describe('validateFormVersionId', () => {
-  const formVersionId = uuidv4();
+  const formVersionId = uuid.v4();
 
   const mockReadVersionResponse = {
     formId: formId,
@@ -575,7 +618,7 @@ describe('validateFormVersionId', () => {
 
     test('formId does not match', async () => {
       formService.readVersion.mockReturnValueOnce({
-        formId: uuidv4(),
+        formId: uuid.v4(),
         id: formVersionId,
       });
       const req = getMockReq({

app/tests/unit/forms/file/controller.spec.js

@@ -1,5 +1,5 @@
 const { getMockReq, getMockRes } = require('@jest-mock/express');
-const { v4: uuidv4 } = require('uuid');
+const uuid = require('uuid');
 
 const controller = require('../../../../src/forms/file/controller');
 const service = require('../../../../src/forms/file/service');
@@ -12,7 +12,7 @@ const currentUser = {
 const fileStorage = {
   createdAt: '2024-06-25T13:53:01-0700',
   createdBy: currentUser.usernameIdp,
-  id: uuidv4(),
+  id: uuid.v4(),
   mimeType: 'text/plain',
   originalName: 'testfile.txt',
   path: '/some/path',

app/tests/unit/forms/file/routes.spec.js

@@ -1,5 +1,5 @@
 const request = require('supertest');
-const { v4: uuidv4 } = require('uuid');
+const uuid = require('uuid');
 
 const { expressHelper } = require('../../../common/helper');
 
@@ -94,7 +94,7 @@ describe(`${basePath}`, () => {
 });
 
 describe(`${basePath}/:id`, () => {
-  const fileId = uuidv4();
+  const fileId = uuid.v4();
   const path = `${basePath}/${fileId}`;
 
   it('should have correct middleware for DELETE', async () => {