Support setting allErrors for AJV validation

AJV recommends setting option `allErrors` to `false` in production.
pdate `createAjv()` to respect the user's setting. Avoid introducing a
breaking change by defaulting to `true` when not defined by the user.

Add tests:
1. Make sure `AjvOptions` sets the value appropriately based on whether
   the end user defined `allErrors` or not.
2. When validating requests, make sure the number of errors reported
   (when multiple occur) is 1 when `allErrors` is `false`.

The `allErrors` configuration for OpenAPISchemaValidator is not changed
by this commit since that validation is for trusted content.

Fixes cdimascio#954

src/framework/ajv/index.ts

@@ -32,7 +32,6 @@ function createAjv(
   const { ajvFormats, ...ajvOptions } = options;
   const ajv = new AjvDraft4({
     ...ajvOptions,
-    allErrors: true,
     formats: formats,
   });
 

src/framework/ajv/options.ts

@@ -11,6 +11,7 @@ export class AjvOptions {
   constructor(options: NormalizedOpenApiValidatorOpts) {
     this.options = options;
   }
+
   get preprocessor(): Options {
     return this.baseOptions();
   }
@@ -44,7 +45,7 @@ export class AjvOptions {
   }
 
   private baseOptions(): Options {
-    const { coerceTypes, formats, validateFormats, serDes, ajvFormats } =
+    const { coerceTypes, formats, validateFormats, serDes, allErrors, ajvFormats } =
       this.options;
     const serDesMap = {};
     for (const serDesObject of serDes) {
@@ -72,6 +73,7 @@ export class AjvOptions {
       validateFormats: validateFormats,
       formats,
       serDesMap,
+      allErrors: allErrors === undefined || allErrors,
       ajvFormats,
     };
 

src/framework/types.ts

@@ -150,6 +150,13 @@ export interface OpenApiValidatorOpts {
   serDes?: SerDes[];
   formats?: Format[] | Record<string, ajv.Format>;
   ajvFormats?: FormatsPluginOptions;
+  /**
+   * Whether AJV should check all rules and collect all errors or return after the first error.
+   *
+   * The default is `true`. This should be set to `false` in production. See [AJV: Security risks of
+   * trusted schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas).
+   */
+  allErrors?: boolean;
   fileUploader?: boolean | multer.Options;
   multerOpts?: multer.Options;
   $refParser?: {

test/ajv.options.spec.ts

@@ -10,8 +10,8 @@ describe('AjvOptions', () => {
     apiSpec: './spec',
     validateApiSpec: false,
     validateRequests: {
-        allowUnknownQueryParameters: false,
-        coerceTypes: false,
+      allowUnknownQueryParameters: false,
+      coerceTypes: false,
     },
     validateResponses: {
       coerceTypes: false,
@@ -44,7 +44,7 @@ describe('AjvOptions', () => {
     expect(options.validateSchema).to.be.false;
   });
 
-  it('should not validate schema for multipar since schema is validated on startup', async () => {
+  it('should not validate schema for multipart since schema is validated on startup', async () => {
     const ajv = new AjvOptions(baseOptions);
     const options = ajv.multipart;
     expect(options.validateSchema).to.be.false;
@@ -60,7 +60,7 @@ describe('AjvOptions', () => {
         },
       ],
     });
-    const options = ajv.multipart;
+    const options = ajv.preprocessor;
     expect(options.serDesMap['custom-1']).has.property('deserialize');
     expect(options.serDesMap['custom-1']).does.not.have.property('serialize');
   });
@@ -75,7 +75,7 @@ describe('AjvOptions', () => {
         },
       ],
     });
-    const options = ajv.multipart;
+    const options = ajv.preprocessor;
     expect(options.serDesMap).has.property('custom-1');
     expect(options.serDesMap['custom-1']).has.property('serialize');
     expect(options.serDesMap['custom-1']).does.not.have.property('deserialize');
@@ -92,7 +92,7 @@ describe('AjvOptions', () => {
         },
       ],
     });
-    const options = ajv.multipart;
+    const options = ajv.preprocessor;
     expect(options.serDesMap).has.property('custom-1');
     expect(options.serDesMap['custom-1']).has.property('serialize');
     expect(options.serDesMap['custom-1']).has.property('deserialize');
@@ -116,9 +116,26 @@ describe('AjvOptions', () => {
         },
       ],
     });
-    const options = ajv.multipart;
+    const options = ajv.preprocessor;
     expect(options.serDesMap).has.property('custom-1');
     expect(options.serDesMap['custom-1']).has.property('serialize');
     expect(options.serDesMap['custom-1']).has.property('deserialize');
   });
+
+  it('should default to allErrors', () => {
+    const ajv = new AjvOptions({
+      ...baseOptions,
+    });
+    const options = ajv.preprocessor;
+    expect(options.allErrors).to.be.true;
+  });
+
+  it('should respect user allErrors setting', () => {
+    const ajv = new AjvOptions({
+      ...baseOptions,
+      allErrors: false,
+    });
+    const options = ajv.preprocessor;
+    expect(options.allErrors).to.be.false;
+  });
 });

test/all-errors.spec.ts

@@ -0,0 +1,73 @@
+import * as path from 'path';
+import * as request from 'supertest';
+import { expect } from 'chai';
+import { createApp } from './common/app';
+
+describe('request body validation with and without allErrors', () => {
+  let allErrorsApp;
+  let notAllErrorsApp;
+
+  const defineRoutes = (app) => {
+    app.post(`${app.basePath}/persons`, (req, res) => {
+      res.send({ success: true });
+    });
+  };
+
+  before(async () => {
+    const apiSpec = path.join('test', 'resources', 'multiple-validations.yaml');
+
+    allErrorsApp = await createApp(
+      {
+        apiSpec,
+        formats: { 'starts-with-b': (v) => /^b/i.test(v) },
+        // allErrors is set to true when undefined
+      },
+      3005,
+      defineRoutes,
+      true,
+    );
+
+    notAllErrorsApp = await createApp(
+      {
+        apiSpec,
+        formats: { 'starts-with-b': (v) => /^b/i.test(v) },
+        allErrors: false,
+      },
+      3006,
+      defineRoutes,
+      true,
+    );
+  });
+
+  after(() => {
+    allErrorsApp.server.close();
+    notAllErrorsApp.server.close();
+  });
+
+  it('should return 200 if short b-name is provided', async () =>
+    request(allErrorsApp)
+      .post(`${allErrorsApp.basePath}/persons`)
+      .set('content-type', 'application/json')
+      .send({ bname: 'Bob' })
+      .expect(200));
+
+  it('should include all validation errors when allErrors=true', async () =>
+    request(allErrorsApp)
+      .post(`${allErrorsApp.basePath}/persons`)
+      .set('content-type', 'application/json')
+      .send({ bname: 'Maximillian' })
+      .expect(400)
+      .then((res) => {
+        expect(res.body.errors.length).to.equal(2);
+      }));
+
+  it('should include only first validation error when allErrors=false', async () =>
+    request(notAllErrorsApp)
+      .post(`${notAllErrorsApp.basePath}/persons`)
+      .set('content-type', 'application/json')
+      .send({ bname: 'Maximillian' })
+      .expect(400)
+      .then((res) => {
+        expect(res.body.errors.length).to.equal(1);
+      }));
+});

test/resources/multiple-validations.yaml

@@ -0,0 +1,29 @@
+openapi: 3.0.1
+info:
+  title: Multiple validations for allErrors check
+  version: 1.0.0
+servers:
+  - url: /v1
+paths:
+  /persons:
+    post:
+      requestBody:
+        content:
+          application/json:
+            schema:
+              $ref: "#/components/schemas/Person"
+      responses:
+        200:
+          description: Success
+
+components:
+  schemas:
+    Person:
+      required:
+        - bname
+      type: object
+      properties:
+        bname:
+          type: string
+          format: starts-with-b
+          maxLength: 10