do not coerce request body property values (#387)

* body validation, do not coerce types * factor validators into Validator class * null number test * update test * fix test * fix test
cdimascio · Oct 4, 2020 · 07af36f · 07af36f
1 parent b57116c
commit 07af36f
Show file tree

Hide file tree

Showing 8 changed files with 4,411 additions and 424 deletions.
diff --git a/README.md b/README.md
@@ -696,7 +696,7 @@ Specifies the options to passthrough to multer. express-openapi-validator uses m
   }
   ```
 
-### ▪️ coerceTypes (optional)
+### ▪️ coerceTypes (optional) - _deprecated_
 
 Determines whether the validator should coerce value types to match the type defined in the OpenAPI spec.
 

diff --git a/examples/1-standard/package-lock.json b/examples/1-standard/package-lock.json
diff --git a/examples/1-standard/package.json b/examples/1-standard/package.json
@@ -13,7 +13,7 @@
   "license": "MIT",
   "dependencies": {
     "body-parser": "^1.19.0",
-    "express-openapi-validator": "^4.0.2"
+    "express-openapi-validator": "file:../.."
   },
   "devDependencies": {
     "nodemon": "^2.0.4"

diff --git a/src/middlewares/openapi.request.validator.ts b/src/middlewares/openapi.request.validator.ts
@@ -1,4 +1,4 @@
-import { Ajv } from 'ajv';
+import { Ajv, ValidateFunction } from 'ajv';
 import { createRequestAjv } from '../framework/ajv';
 import {
   ContentType,
@@ -7,7 +7,6 @@ import {
 } from './util';
 import { NextFunction, RequestHandler, Response } from 'express';
 import {
-  ValidationSchema,
   OpenAPIV3,
   OpenApiRequest,
   RequestValidatorOptions,
@@ -16,6 +15,9 @@ import {
   NotFound,
   MethodNotAllowed,
   BadRequest,
+  ParametersSchema,
+  BodySchema,
+  ValidationSchema,
 } from '../framework/types';
 import { BodySchemaParser } from './parsers/body.parse';
 import { ParametersSchemaParser } from './parsers/schema.parse';
@@ -32,6 +34,7 @@ export class RequestValidator {
   private middlewareCache: { [key: string]: RequestHandler } = {};
   private apiDoc: OpenAPIV3.Document;
   private ajv: Ajv;
+  private ajvBody: Ajv;
   private requestOpts: ValidateRequestOpts = {};
 
   constructor(
@@ -43,6 +46,7 @@ export class RequestValidator {
     this.requestOpts.allowUnknownQueryParameters =
       options.allowUnknownQueryParameters;
     this.ajv = createRequestAjv(apiDoc, options);
+    this.ajvBody = createRequestAjv(apiDoc, { ...options, coerceTypes: false });
   }
 
   public validate(
@@ -94,28 +98,14 @@ export class RequestValidator {
   ): RequestHandler {
     const apiDoc = this.apiDoc;
     const schemaParser = new ParametersSchemaParser(this.ajv, apiDoc);
-    const bodySchemaParser = new BodySchemaParser(this.ajv, apiDoc);
+    const bodySchemaParser = new BodySchemaParser(this.ajvBody, apiDoc);
     const parameters = schemaParser.parse(path, reqSchema.parameters);
     const securityQueryParam = Security.queryParam(apiDoc, reqSchema);
     const body = bodySchemaParser.parse(path, reqSchema, contentType);
-
-    const isBodyBinary = body?.['format'] === 'binary';
-    const properties: ValidationSchema = {
-      ...parameters,
-      body: isBodyBinary ? {} : body,
-    };
-    // TODO throw 400 if missing a required binary body
-    const required =
-      (<SchemaObject>body).required && !isBodyBinary ? ['body'] : [];
-    // $schema: "http://json-schema.org/draft-04/schema#",
-    const schema = {
-      paths: this.apiDoc.paths,
-      components: this.apiDoc.components,
-      required: ['query', 'headers', 'params'].concat(required),
-      properties,
-    };
-
-    const validator = this.ajv.compile(schema);
+    const validator = new Validator(this.apiDoc, parameters, body, {
+      general: this.ajv,
+      body: this.ajvBody,
+    });
 
     return (req: OpenApiRequest, res: Response, next: NextFunction): void => {
       const openapi = <OpenApiRequestMetadata>req.openapi;
@@ -125,19 +115,20 @@ export class RequestValidator {
         req.params = openapi.pathParams ?? req.params;
       }
 
+      const schemaPoperties = validator.allSchemaProperties;
       const mutator = new RequestParameterMutator(
         this.ajv,
         apiDoc,
         path,
-        properties,
+        schemaPoperties,
       );
 
       mutator.modifyRequest(req);
 
       if (!this.requestOpts.allowUnknownQueryParameters) {
         this.processQueryParam(
           req.query,
-          schema.properties.query,
+          schemaPoperties.query,
           securityQueryParam,
         );
       }
@@ -149,11 +140,18 @@ export class RequestValidator {
           }
         : undefined;
 
-      const valid = validator({ query: {}, ...req, cookies });
-      if (valid) {
+      const data = { query: {}, ...req, cookies };
+      const valid = validator.validatorGeneral(data);
+      const validBody = validator.validatorBody(data);
+
+      if (valid && validBody) {
         next();
       } else {
-        const errors = augmentAjvErrors([...(validator.errors ?? [])]);
+        const errors = augmentAjvErrors(
+          []
+            .concat(validator.validatorGeneral.errors ?? [])
+            .concat(validator.validatorBody.errors ?? []),
+        );
         const err = ajvErrorsToValidatorError(400, errors);
         const message = this.ajv.errorsText(errors, { dataVar: 'request' });
         const error: BadRequest = new BadRequest({
@@ -191,6 +189,67 @@ export class RequestValidator {
   }
 }
 
+class Validator {
+  private readonly apiDoc: OpenAPIV3.Document;
+  readonly schemaGeneral: object;
+  readonly schemaBody: object;
+  readonly validatorGeneral: ValidateFunction;
+  readonly validatorBody: ValidateFunction;
+  readonly allSchemaProperties: ValidationSchema;
+
+  constructor(
+    apiDoc: OpenAPIV3.Document,
+    parametersSchema: ParametersSchema,
+    bodySchema: BodySchema,
+    ajv: {
+      general: Ajv;
+      body: Ajv;
+    },
+  ) {
+    this.apiDoc = apiDoc;
+    this.schemaGeneral = this._schemaGeneral(parametersSchema);
+    this.schemaBody = this._schemaBody(bodySchema);
+    this.allSchemaProperties = {
+      ...(<any>this.schemaGeneral).properties, // query, header, params props
+      body: (<any>this.schemaBody).properties.body, // body props
+    };
+    this.validatorGeneral = ajv.general.compile(this.schemaGeneral);
+    this.validatorBody = ajv.body.compile(this.schemaBody);
+  }
+
+  private _schemaGeneral(parameters: ParametersSchema): object {
+    // $schema: "http://json-schema.org/draft-04/schema#",
+    return {
+      paths: this.apiDoc.paths,
+      components: this.apiDoc.components,
+      required: ['query', 'headers', 'params'],
+      properties: { ...parameters, body: {} },
+    };
+  }
+
+  private _schemaBody(body: BodySchema): object {
+    // $schema: "http://json-schema.org/draft-04/schema#"
+    const isBodyBinary = body?.['format'] === 'binary';
+    const bodyProps = isBodyBinary ? {} : body;
+    const bodySchema = {
+      paths: this.apiDoc.paths,
+      components: this.apiDoc.components,
+      properties: {
+        query: {},
+        headers: {},
+        params: {},
+        cookies: {},
+        body: bodyProps,
+      },
+    };
+    const requireBody = (<SchemaObject>body).required && !isBodyBinary;
+    if (requireBody) {
+      (<any>bodySchema).required = ['body'];
+    }
+    return bodySchema;
+  }
+}
+
 class Security {
   public static queryParam(
     apiDocs: OpenAPIV3.Document,

diff --git a/src/middlewares/parsers/req.parameter.mutator.ts b/src/middlewares/parsers/req.parameter.mutator.ts
@@ -33,7 +33,6 @@ const REQUEST_FIELDS = {
 };
 
 type Schema = ReferenceObject | SchemaObject;
-type Parameter = ReferenceObject | ParameterObject;
 
 /**
  * A class top parse and mutate the incoming request parameters according to the openapi spec.

diff --git a/src/openapi.validator.ts b/src/openapi.validator.ts
@@ -176,7 +176,10 @@ export class OpenApiValidator {
       middlewares.push((req, res, next) => {
         if (router) return router(req, res, next);
         pContext
-          .then((context) => (router = this.installOperationHandlers(req.baseUrl, context)))
+          .then(
+            (context) =>
+              (router = this.installOperationHandlers(req.baseUrl, context)),
+          )
           .then((router) => router(req, res, next))
           .catch(next);
       });
@@ -309,6 +312,9 @@ export class OpenApiValidator {
         'securityHandlers is not supported. Use validateSecurities.handlers instead.',
       );
     }
+    if (options.coerceTypes) {
+      console.warn('coerceTypes is deprecated.');
+    }
 
     const multerOpts = (<any>options).multerOpts;
     if (multerOpts != null) {

diff --git a/test/coercion.spec.ts b/test/coercion.spec.ts
@@ -26,17 +26,57 @@ describe(packageJson.name, () => {
     app.server.close();
   });
 
-  it('should coerce is_cat to boolean since it is defined as a boolean in the spec', async () =>
+  it('should return 400 since is_cat is passed as string not boolean', async () =>
     request(app)
       .post(`${app.basePath}/coercion/pets`)
       .send({
         name: 'test',
         is_cat: 'true',
       })
+      .expect(400)
+      .then((r) => {
+        console.log(r.body);
+        expect(r.body.message).to.contain('is_cat should be boolean');
+      }));
+
+  it('should return 400 when age is passed as string, but number is expected', async () =>
+    request(app)
+      .post(`${app.basePath}/coercion/pets`)
+      .send({
+        name: 'test',
+        is_cat: true,
+        age: '13.5',
+      })
+      .expect(400)
+      .then((r) => {
+        expect(r.body.message).to.contain('age should be number');
+      }));
+
+  it('should return 400 when age (number) is null', async () =>
+    request(app)
+      .post(`${app.basePath}/coercion/pets`)
+      .send({
+        name: 'test',
+        is_cat: true,
+        age: null,
+      })
+      .expect(400)
+      .then((r) => {
+        expect(r.body.message).to.contain('age should be number');
+      }));
+
+  it('should return 200 when all are typed correctly', async () =>
+    request(app)
+      .post(`${app.basePath}/coercion/pets`)
+      .send({
+        name: 'test',
+        is_cat: true,
+        age: 13.5,
+      })
       .expect(200)
       .then((r) => {
-        expect(r.body.is_cat).to.be.a('boolean');
-        expect(r.body.is_cat).to.be.equal(true);
+        expect(r.body.age).to.equal(13.5);
+        expect(r.body.is_cat).to.equal(true);
       }));
 
   it('should keep is_cat as boolean', async () =>
@@ -52,16 +92,15 @@ describe(packageJson.name, () => {
         expect(r.body.is_cat).to.be.equal(true);
       }));
 
-  it('should coerce a is_cat from boolean to string since it is defined as such in the spec', async () =>
+  it('should return 400 when is_cat requires string type "true", but boolean specified', async () =>
     request(app)
       .post(`${app.basePath}/coercion/pets_string_boolean`)
       .send({
         name: 'test',
         is_cat: true,
       })
-      .expect(200)
+      .expect(400)
       .then((r) => {
-        expect(r.body.is_cat).to.be.a('string');
-        expect(r.body.is_cat).to.be.equal('true');
+        expect(r.body.message).to.contain('is_cat should be string');
       }));
 });
diff --git a/test/resources/coercion.yaml b/test/resources/coercion.yaml
@@ -56,6 +56,8 @@ components:
           type: string
         is_cat:
           type: boolean
+        age:
+          type: number
 
     PetStringBoolean:
       required: