Change AJV allErrors default and support user setting (#955)

* 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 #954 * (Revisions) Support setting allErrors for AJV validation - Do not set allErrors by default **breaking change** * (Revisions) Support setting allErrors for AJV validation - Allow allErrors to be set on requests and responses independently
cdimascio · Aug 31, 2024 · 392f1dd · 392f1dd
1 parent 826ba62
commit 392f1dd
Show file tree

Hide file tree

Showing 17 changed files with 336 additions and 84 deletions.
diff --git a/src/framework/ajv/index.ts b/src/framework/ajv/index.ts
@@ -32,7 +32,6 @@ function createAjv(
   const { ajvFormats, ...ajvOptions } = options;
   const ajv = new AjvDraft4({
     ...ajvOptions,
-    allErrors: true,
     formats: formats,
   });
 

diff --git a/src/framework/ajv/options.ts b/src/framework/ajv/options.ts
@@ -11,28 +11,31 @@ export class AjvOptions {
   constructor(options: NormalizedOpenApiValidatorOpts) {
     this.options = options;
   }
+
   get preprocessor(): Options {
     return this.baseOptions();
   }
 
   get response(): Options {
-    const { coerceTypes, removeAdditional } = <ValidateResponseOpts>(
+    const { allErrors, coerceTypes, removeAdditional } = <ValidateResponseOpts>(
       this.options.validateResponses
     );
     return {
       ...this.baseOptions(),
+      allErrors,
       useDefaults: false,
       coerceTypes,
       removeAdditional,
     };
   }
 
   get request(): RequestValidatorOptions {
-    const { allowUnknownQueryParameters, coerceTypes, removeAdditional } = <
+    const { allErrors, allowUnknownQueryParameters, coerceTypes, removeAdditional } = <
       ValidateRequestOpts
     >this.options.validateRequests;
     return {
       ...this.baseOptions(),
+      allErrors,
       allowUnknownQueryParameters,
       coerceTypes,
       removeAdditional,
@@ -44,8 +47,13 @@ export class AjvOptions {
   }
 
   private baseOptions(): Options {
-    const { coerceTypes, formats, validateFormats, serDes, ajvFormats } =
-      this.options;
+    const {
+      coerceTypes,
+      formats,
+      validateFormats,
+      serDes,
+      ajvFormats,
+    } = this.options;
     const serDesMap = {};
     for (const serDesObject of serDes) {
       if (!serDesMap[serDesObject.format]) {
@@ -69,7 +77,7 @@ export class AjvOptions {
       coerceTypes,
       useDefaults: true,
       removeAdditional: false,
-      validateFormats: validateFormats,
+      validateFormats,
       formats,
       serDesMap,
       ajvFormats,

diff --git a/src/framework/types.ts b/src/framework/types.ts
@@ -47,12 +47,26 @@ export interface Options extends ajv.Options {
 export interface RequestValidatorOptions extends Options, ValidateRequestOpts { }
 
 export type ValidateRequestOpts = {
+  /**
+   * Whether AJV should check all rules and collect all errors or return after the first error.
+   *
+   * This should not be set to `true` in production. See [AJV: Security risks of trusted
+   * schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas).
+   */
+  allErrors?: boolean;
   allowUnknownQueryParameters?: boolean;
   coerceTypes?: boolean | 'array';
   removeAdditional?: boolean | 'all' | 'failing';
 };
 
 export type ValidateResponseOpts = {
+  /**
+   * Whether AJV should check all rules and collect all errors or return after the first error.
+   *
+   * This should not be set to `true` in production. See [AJV: Security risks of trusted
+   * schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas).
+   */
+  allErrors?: boolean;
   removeAdditional?: boolean | 'all' | 'failing';
   coerceTypes?: boolean | 'array';
   onError?: (err: InternalServerError, json: any, req: Request) => void;

diff --git a/test/699.spec.ts b/test/699.spec.ts
@@ -182,7 +182,6 @@ describe('699 serialize response components only', () => {
       3005,
       (app) => {
         app.get([`${app.basePath}/users/:id?`], (req, res) => {
-          debugger;
           if (typeof req.params.id !== 'string') {
             throw new Error("Should be not be deserialized to ObjectId object");
           }

diff --git a/test/881.spec.ts b/test/881.spec.ts
@@ -50,6 +50,9 @@ function createServer() {
     app.use(
       OpenApiValidator.middleware({
         apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
         validateResponses: true,
       }),
     );

diff --git a/test/additional.props.spec.ts b/test/additional.props.spec.ts
@@ -15,14 +15,22 @@ describe(packageJson.name, () => {
       'resources',
       'additional.properties.yaml',
     );
-    app = await createApp({ apiSpec }, 3005, (app) =>
-      app.use(
-        `${app.basePath}/additional_props`,
-        express
-          .Router()
-          .post(`/false`, (req, res) => res.json(req.body))
-          .post(`/true`, (req, res) => res.json(req.body)),
-      ),
+    app = await createApp(
+      {
+        apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
+      },
+      3005,
+      (app) =>
+        app.use(
+          `${app.basePath}/additional_props`,
+          express
+            .Router()
+            .post(`/false`, (req, res) => res.json(req.body))
+            .post(`/true`, (req, res) => res.json(req.body)),
+        ),
     );
   });
 

diff --git a/test/ajv.options.spec.ts b/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,7 +116,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');

diff --git a/test/all-errors.spec.ts b/test/all-errors.spec.ts
@@ -0,0 +1,106 @@
+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 });
+    });
+    app.get(`${app.basePath}/persons`, (req, res) => {
+      res.send({ bname: req.query.bname });
+    });
+  };
+
+  before(async () => {
+    const apiSpec = path.join('test', 'resources', 'multiple-validations.yaml');
+
+    allErrorsApp = await createApp(
+      {
+        apiSpec,
+        formats: {
+          'starts-with-b': (v) => /^b/i.test(v),
+        },
+        validateRequests: {
+          allErrors: true,
+        },
+        validateResponses: {
+          allErrors: true,
+        },
+      },
+      3005,
+      defineRoutes,
+      true,
+    );
+
+    notAllErrorsApp = await createApp(
+      {
+        apiSpec,
+        formats: {
+          'starts-with-b': (v) => /^b/i.test(v),
+        },
+        validateResponses: true,
+      },
+      3006,
+      defineRoutes,
+      true,
+    );
+  });
+
+  after(() => {
+    allErrorsApp.server.close();
+    notAllErrorsApp.server.close();
+  });
+
+  it('should return 200 if short b-name is posted', async () =>
+    request(allErrorsApp)
+      .post(`${allErrorsApp.basePath}/persons`)
+      .set('content-type', 'application/json')
+      .send({ bname: 'Bob' })
+      .expect(200));
+
+  it('should return 200 if short b-name is fetched', async () =>
+    request(allErrorsApp)
+      .get(`${allErrorsApp.basePath}/persons?bname=Bob`)
+      .expect(200));
+
+  it('should include all request 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).to.have.lengthOf(2);
+      }));
+
+  it('should include only first request 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).to.have.lengthOf(1);
+      }));
+
+  it('should include all response validation errors when allErrors=true', async () =>
+    request(allErrorsApp)
+      .get(`${allErrorsApp.basePath}/persons?bname=Maximillian`)
+      .expect(500)
+      .then((res) => {
+        expect(res.body.errors).to.have.lengthOf(2);
+      }));
+
+  it('should include only first response validation error when allErrors=false', async () =>
+    request(notAllErrorsApp)
+      .get(`${notAllErrorsApp.basePath}/persons?bname=Maximillian`)
+      .expect(500)
+      .then((res) => {
+        expect(res.body.errors).to.have.lengthOf(1);
+      }));
+});
diff --git a/test/all.of.spec.ts b/test/all.of.spec.ts
@@ -11,11 +11,19 @@ describe(packageJson.name, () => {
   before(async () => {
     // Set up the express app
     const apiSpec = path.join('test', 'resources', 'all.of.yaml');
-    app = await createApp({ apiSpec }, 3005, (app) =>
-      app.use(
-        `${app.basePath}`,
-        express.Router().post(`/all_of`, (req, res) => res.json(req.body)),
-      ),
+    app = await createApp(
+      {
+        apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
+      },
+      3005,
+      (app) =>
+        app.use(
+          `${app.basePath}`,
+          express.Router().post(`/all_of`, (req, res) => res.json(req.body)),
+        ),
     );
   });
 

diff --git a/test/empty.servers.spec.ts b/test/empty.servers.spec.ts
@@ -10,13 +10,19 @@ describe('empty servers', () => {
   before(async () => {
     // Set up the express app
     const apiSpec = path.join('test', 'resources', 'empty.servers.yaml');
-    app = await createApp({ apiSpec }, 3007, app =>
-      app.use(
-        ``,
-        express
-          .Router()
-          .get(`/pets`, (req, res) => res.json(req.body)),
-      ),
+    app = await createApp(
+      {
+        apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
+      },
+      3007,
+      (app) =>
+        app.use(
+          ``,
+          express.Router().get(`/pets`, (req, res) => res.json(req.body)),
+        ),
     );
   });
 
@@ -28,7 +34,7 @@ describe('empty servers', () => {
     request(app)
       .get(`/pets`)
       .expect(400)
-      .then(r => {
+      .then((r) => {
         expect(r.body.errors).to.be.an('array');
         expect(r.body.errors).to.have.length(2);
       }));

diff --git a/test/multi.spec.spec.ts b/test/multi.spec.spec.ts
@@ -50,6 +50,9 @@ function createServer() {
     app.use(
       OpenApiValidator.middleware({
         apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
       }),
     );