fix: FORMS-1336 validate fileId parameters (#1453)

* fix: FORMS-1336 validate fileId parameters

* fix variable names

* fix variable names - again!

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

@@ -25,10 +25,10 @@ const _validateUuid = (parameter, parameterName) => {
  * This validator requires that either the :formId or :formSubmissionId route
  * parameter also exists.
  *
- * @param {*} req the Express object representing the HTTP request
- * @param {*} _res the Express object representing the HTTP response - unused
- * @param {*} next the Express chaining function
- * @param {*} documentTemplateId the :documentTemplateId value from the route
+ * @param {*} req the Express object representing the HTTP request.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ * @param {*} documentTemplateId the :documentTemplateId value from the route.
  */
 const validateDocumentTemplateId = async (req, _res, next, documentTemplateId) => {
   try {
@@ -60,13 +60,31 @@ const validateDocumentTemplateId = async (req, _res, next, documentTemplateId) =
   }
 };
 
+/**
+ * Validates that the :fileId 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 {*} fileId the :fileId value from the route.
+ */
+const validateFileId = async (_req, _res, next, fileId) => {
+  try {
+    _validateUuid(fileId, 'fileId');
+
+    next();
+  } catch (error) {
+    next(error);
+  }
+};
+
 /**
  * Validates that the :formId 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 {*} formId the :formId value from the route
+ * @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 {*} formId the :formId value from the route.
  */
 const validateFormId = async (_req, _res, next, formId) => {
   try {
@@ -82,10 +100,10 @@ const validateFormId = async (_req, _res, next, formId) => {
  * Validates that the :formVersionDraftId route parameter exists and is a UUID.
  * This validator requires that the :formId route parameter also exists.
  *
- * @param {*} req the Express object representing the HTTP request
- * @param {*} _res the Express object representing the HTTP response - unused
- * @param {*} next the Express chaining function
- * @param {*} formVersionDraftId the :formVersionDraftId value from the route
+ * @param {*} req the Express object representing the HTTP request.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ * @param {*} formVersionDraftId the :formVersionDraftId value from the route.
  */
 const validateFormVersionDraftId = async (req, _res, next, formVersionDraftId) => {
   try {
@@ -108,10 +126,10 @@ const validateFormVersionDraftId = async (req, _res, next, formVersionDraftId) =
  * Validates that the :formVersionId route parameter exists and is a UUID. This
  * validator requires that the :formId route parameter also exists.
  *
- * @param {*} req the Express object representing the HTTP request
- * @param {*} _res the Express object representing the HTTP response - unused
- * @param {*} next the Express chaining function
- * @param {*} formVersionId the :formVersionId value from the route
+ * @param {*} req the Express object representing the HTTP request.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ * @param {*} formVersionId the :formVersionId value from the route.
  */
 const validateFormVersionId = async (req, _res, next, formVersionId) => {
   try {
@@ -134,10 +152,10 @@ const validateFormVersionId = async (req, _res, next, formVersionId) => {
  * Validates that the :externalApiId route parameter exists and is a UUID. This
  * validator requires that the :formId route parameter also exists.
  *
- * @param {*} req the Express object representing the HTTP request
- * @param {*} _res the Express object representing the HTTP response - unused
- * @param {*} next the Express chaining function
- * @param {*} externalAPIId the :externalAPIId value from the route
+ * @param {*} req the Express object representing the HTTP request.
+ * @param {*} _res the Express object representing the HTTP response - unused.
+ * @param {*} next the Express chaining function.
+ * @param {*} externalAPIId the :externalAPIId value from the route.
  */
 const validateExternalAPIId = async (req, _res, next, externalAPIId) => {
   try {
@@ -158,6 +176,7 @@ const validateExternalAPIId = async (req, _res, next, externalAPIId) => {
 
 module.exports = {
   validateDocumentTemplateId,
+  validateFileId,
   validateFormId,
   validateFormVersionId,
   validateFormVersionDraftId,

app/src/forms/file/routes.js

@@ -1,6 +1,7 @@
 const routes = require('express').Router();
 const controller = require('./controller');
 const apiAccess = require('../auth/middleware/apiAccess');
+const validateParameter = require('../common/middleware/validateParameter');
 
 const P = require('../common/constants').Permissions;
 const { currentFileRecord, hasFileCreate, hasFilePermissions } = require('./middleware/filePermissions');
@@ -10,6 +11,8 @@ const rateLimiter = require('../common/middleware').apiKeyRateLimiter;
 
 routes.use(currentUser);
 
+routes.param('fileId', validateParameter.validateFileId);
+
 routes.post('/', hasFileCreate, fileUpload.upload, async (req, res, next) => {
   await controller.create(req, res, next);
 });

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

@@ -5,6 +5,7 @@ const validateParameter = require('../../../../../src/forms/common/middleware/va
 const formService = require('../../../../../src/forms/form/service');
 const submissionService = require('../../../../../src/forms/submission/service');
 
+const fileId = uuidv4();
 const formId = uuidv4();
 const formSubmissionId = uuidv4();
 
@@ -197,6 +198,49 @@ describe('validateDocumentTemplateId', () => {
   });
 });
 
+describe('validateFileId', () => {
+  describe('400 response when', () => {
+    const expectedStatus = { status: 400 };
+
+    test('fileId is missing', async () => {
+      const req = getMockReq({
+        params: {},
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFileId(req, res, next);
+
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+
+    test.each(invalidUuids)('fileId is "%s"', async (eachFileId) => {
+      const req = getMockReq({
+        params: { fileId: eachFileId },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFileId(req, res, next, eachFileId);
+
+      expect(next).toBeCalledWith(expect.objectContaining(expectedStatus));
+    });
+  });
+
+  describe('allows', () => {
+    test('uuid for fileId', async () => {
+      const req = getMockReq({
+        params: {
+          fileId: fileId,
+        },
+      });
+      const { res, next } = getMockRes();
+
+      await validateParameter.validateFileId(req, res, next, fileId);
+
+      expect(next).toBeCalledWith();
+    });
+  });
+});
+
 describe('validateFormId', () => {
   describe('400 response when', () => {
     const expectedStatus = { status: 400 };

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

@@ -81,6 +81,7 @@ describe(`${basePath}`, () => {
   it('should have correct middleware for POST', async () => {
     await appRequest.post(path);
 
+    expect(validateParameter.validateFileId).toBeCalledTimes(0);
     expect(apiAccess).toBeCalledTimes(0);
     expect(filePermissions.currentFileRecord).toBeCalledTimes(0);
     expect(filePermissions.hasFileCreate).toBeCalledTimes(1);
@@ -99,7 +100,7 @@ describe(`${basePath}/:id`, () => {
   it('should have correct middleware for DELETE', async () => {
     await appRequest.delete(path);
 
-    // expect(validateParameter.validateFileId).toBeCalledTimes(1);
+    expect(validateParameter.validateFileId).toBeCalledTimes(1);
     expect(apiAccess).toBeCalledTimes(0);
     expect(filePermissions.currentFileRecord).toBeCalledTimes(1);
     expect(filePermissions.hasFileCreate).toBeCalledTimes(0);
@@ -113,7 +114,7 @@ describe(`${basePath}/:id`, () => {
   it('should have correct middleware for GET', async () => {
     await appRequest.get(path);
 
-    // expect(validateParameter.validateFileId).toBeCalledTimes(1);
+    expect(validateParameter.validateFileId).toBeCalledTimes(1);
     expect(apiAccess).toBeCalledTimes(1);
     expect(filePermissions.currentFileRecord).toBeCalledTimes(1);
     expect(filePermissions.hasFileCreate).toBeCalledTimes(0);