From fc2f98bf7d6b207d67fc62561f1b5b2fece4f616 Mon Sep 17 00:00:00 2001
From: Carmine DiMascio <cdimascio@tripadvisor.com>
Date: Fri, 24 May 2019 13:43:43 -0400
Subject: [PATCH] enhance error handling

---
 src/middlewares/openapi.multipart.ts | 62 ++++++++++++++++++----------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/src/middlewares/openapi.multipart.ts b/src/middlewares/openapi.multipart.ts
index ad5ae4fa..b0e6a41a 100644
--- a/src/middlewares/openapi.multipart.ts
+++ b/src/middlewares/openapi.multipart.ts
@@ -11,27 +11,26 @@ export function multipart(openApiContext: OpenApiContext, multerOpts: {} = {}) {
       }
       mult.any()(req, res, err => {
         if (err) {
-          if (err instanceof multer.MulterError) {
-            // TODO is special handling for MulterErrors needed
-            console.error(err);
-            next(validationError(500, req.path, err.message));
-          } else {
-            // HACK
-            // TODO improve multer error handling
-            const missingField = /Multipart: Boundary not found/i.test(
-              err.message || '',
-            );
-            if (missingField) {
-              next(validationError(400, req.path, 'multipart file(s) required.'))
-            } else {
-              console.error(err);
-              next(validationError(500, req.path, err.message));
-            }
-          }
+          next(error(req, err));
         } else {
-          req.files.forEach(f => {
-            req.body[f.fieldname] = '';
-          });
+          // TODO:
+          // If a form parameter 'file' is defined to take file value, but the user provides a string value instead
+          // req.files will be empty and req.body.file will be populated with a string
+          // This will incorrectly PASS validation. 
+          // Instead, we should return a 400 with an invalid type e.g. file expects a file, but found string.
+          // 
+          // In order to support this, we likely need to inspect the schema directly to find the type. 
+          // For example, if param with type: 'string', format: 'binary' is defined, we expect to see it in
+          // req.files. If it's not present we should throw a 400
+          // 
+          // This is a bit complex because the schema may be defined inline (easy) or via a $ref (complex) in which
+          // case we must follow the $ref to check the type.
+          if (req.files) {
+            // add files to body
+            req.files.forEach(f => {
+              req.body[f.fieldname] = '';
+            });
+          }
           next();
         }
       });
@@ -42,7 +41,8 @@ export function multipart(openApiContext: OpenApiContext, multerOpts: {} = {}) {
 }
 
 function isValidContentType(req) {
-  return req.headers['content-type'].includes('multipart/form-data');
+  const contentType = req.headers['content-type'];
+  return !contentType || contentType.includes('multipart/form-data');
 }
 
 function isMultipart(req) {
@@ -54,3 +54,23 @@ function isMultipart(req) {
     req.openapi.schema.requestBody.content['multipart/form-data']
   );
 }
+
+function error(req, err) {
+  if (err instanceof multer.MulterError) {
+    // TODO is special handling for MulterErrors needed
+    console.error(err);
+    return validationError(500, req.path, err.message);
+  } else {
+    // HACK
+    // TODO improve multer error handling
+    const missingField = /Multipart: Boundary not found/i.test(
+      err.message || '',
+    );
+    if (missingField) {
+      return validationError(400, req.path, 'multipart file(s) required.');
+    } else {
+      console.error(err);
+      return validationError(500, req.path, err.message);
+    }
+  }
+}