Repository: cdimascio/express-openapi-validator (ts)

Issue:
Support configuring AJV `allErrors` to follow their recommended security practices
**Is your feature request related to a problem? Please describe.**

In [AJV's security considerations documentation](https://ajv.js.org/security.html#security-risks-of-trusted-schemas), they write "Do NOT use allErrors in production":

> Some keywords in JSON Schemas can lead to very slow validation for certain data. These keywords include (but may be not limited to):
> 
> - `pattern` and `format` for large strings - in some cases using `maxLength` can help mitigate it, but certain regular expressions can lead to exponential validation time even with relatively short strings (see [ReDoS attack](https://ajv.js.org/security.html#redos-attack)).
> - `patternProperties` for large property names - use `propertyNames` to mitigate, but some regular expressions can have exponential evaluation time as well.
> - `uniqueItems` for large non-scalar arrays - use `maxItems` to mitigate
>
> ### Do NOT use allErrors in production
> 
> The suggestions above to prevent slow validation would only work if you do NOT use `allErrors: true` in production code (using it would continue validation after validation errors).

Unfortunately, express-openapi-validator overrides whatever the user attempts to set for `allErrors`:
https://github.com/cdimascio/express-openapi-validator/blob/f20b1c9390c6d230db728f6bc528af5812f88312/src/framework/ajv/index.ts#L33-L37

Note: [`allErrors` is also set to `true` in `OpenAPISchemaValidator`](https://github.com/cdimascio/express-openapi-validator/blob/f20b1c9390c6d230db728f6bc528af5812f88312/src/framework/openapi.schema.validator.ts#L21), but that is less concerning since it is just used for OpenAPI schema validation and not end user requests. 

**Describe the solution you'd like**
It should be possible for developers to set `allErrors: false` and express-openapi-validator will respect it.

**Describe alternatives you've considered**
(none)

**Additional context**
This could help mitigate ReDOS attacks, at least to a small extent.

Pull request:
Change AJV allErrors default and support user setting
**BREAKING CHANGE**: Request and response validation stops after the first failure. Only one error will be reported even when multiple may exist. This follows best practices from AJV:
- [Security risks of trusted schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas)
- [`allErrors` option](https://ajv.js.org/options.html#allerrors)

To report all validation errors (only recommended in development), option `allErrors` can be set in express-openapi-validator request and response validation options. For example:
```ts
app.use(
  OpenApiValidator.middleware({
    apiSpec: 'path/to/openapi.json',
    validateRequests: {
      allErrors: true,
    },
    validateResponses: {
      allErrors: true,
    },
  })
);
```

---

AJV security recommendations advise that `allErrors` should not be set to `true` in production (more details in #954). Update `createAjv()` so that it does not set `allErrors` by default and add support for letting users define `allErrors` in request and response validation options.

Added tests to ensure that the number of errors reported (when multiple are expected) is correct based on how `allErrors` is set (or unset).

The `allErrors` configuration for `OpenAPISchemaValidator` is not changed by this commit since that validation is for trusted content.

Fixes #954

Patch:
diff --git a/src/framework/ajv/index.ts b/src/framework/ajv/index.ts
index 5dda5ff..dd43943 100644
--- 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
index 5bf32b0..bde6859 100644
--- a/src/framework/ajv/options.ts
+++ b/src/framework/ajv/options.ts
@@ -11,16 +11,18 @@ 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,
@@ -28,11 +30,12 @@ export class AjvOptions {
   }
 
   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
index 9d4d0c6..2df2ea0 100644
--- 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;
@@ -68,7 +82,7 @@ export type OperationHandlerOptions = {
     handlersPath: string,
     route: RouteMetadata,
     apiDoc: OpenAPIV3.Document,
-  ) => RequestHandler | Promise<RequestHandler>;
+  ) => unknown;
 };
 
 export type Format = {


Test patch:
diff --git a/test/699.spec.ts b/test/699.spec.ts
index 2997299..4e3399e 100644
--- 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
index 33bd3d1..7d72441 100644
--- 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
index e76d78e..4d6307f 100644
--- 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
index 5407dd5..70d9de2 100644
--- 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
new file mode 100644
index 0000000..4b96792
--- /dev/null
+++ 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
index e1a2792..7a152f0 100644
--- 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
index d9a634f..ac881a7 100644
--- 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
index f7ee0ee..4b1d355 100644
--- 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,
+        },
       }),
     );
 
diff --git a/test/multipart.disabled.spec.ts b/test/multipart.disabled.spec.ts
index dad4d24..2b715fd 100644
--- a/test/multipart.disabled.spec.ts
+++ b/test/multipart.disabled.spec.ts
@@ -9,15 +9,24 @@ describe(packageJson.name, () => {
   let app = null;
   before(async () => {
     const apiSpec = path.join('test', 'resources', 'multipart.yaml');
-    app = await createApp({ apiSpec, fileUploader: false }, 3003, app =>
-      app.use(
-        `${app.basePath}`,
-        express
-          .Router()
-          .post(`/sample_2`, (req, res) => res.json(req.body))
-          .post(`/sample_1`, (req, res) => res.json(req.body))
-          .get('/range', (req, res) => res.json(req.body)),
-      ),
+    app = await createApp(
+      {
+        apiSpec,
+        fileUploader: false,
+        validateRequests: {
+          allErrors: true,
+        },
+      },
+      3003,
+      (app) =>
+        app.use(
+          `${app.basePath}`,
+          express
+            .Router()
+            .post(`/sample_2`, (req, res) => res.json(req.body))
+            .post(`/sample_1`, (req, res) => res.json(req.body))
+            .get('/range', (req, res) => res.json(req.body)),
+        ),
     );
   });
   after(() => {
diff --git a/test/openapi.spec.ts b/test/openapi.spec.ts
index 5823b26..9726eb1 100644
--- a/test/openapi.spec.ts
+++ b/test/openapi.spec.ts
@@ -12,8 +12,24 @@ describe(packageJson.name, () => {
     const apiSpecPath = path.join('test', 'resources', 'openapi.yaml');
     const apiSpecJson = require('./resources/openapi.json');
     return Promise.all([
-      createApp({ apiSpec: apiSpecPath }, 3001),
-      createApp({ apiSpec: apiSpecJson }, 3002),
+      createApp(
+        {
+          apiSpec: apiSpecPath,
+          validateRequests: {
+            allErrors: true,
+          },
+        },
+        3001,
+      ),
+      createApp(
+        {
+          apiSpec: apiSpecJson,
+          validateRequests: {
+            allErrors: true,
+          },
+        },
+        3002,
+      ),
     ]).then(([a1, a2]) => {
       apps.push(a1);
       apps.push(a2);
diff --git a/test/path.level.parameters.spec.ts b/test/path.level.parameters.spec.ts
index 56d1f5e..7f61a47 100644
--- a/test/path.level.parameters.spec.ts
+++ b/test/path.level.parameters.spec.ts
@@ -15,13 +15,21 @@ describe(packageJson.name, () => {
       'resources',
       'path.level.parameters.yaml',
     );
-    app = await createApp({ apiSpec }, 3005, app =>
-      app.use(
-        `${app.basePath}`,
-        express
-          .Router()
-          .get(`/path_level_parameters`, (_req, res) => res.send()),
-      ),
+    app = await createApp(
+      {
+        apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
+      },
+      3005,
+      (app) =>
+        app.use(
+          `${app.basePath}`,
+          express
+            .Router()
+            .get(`/path_level_parameters`, (_req, res) => res.send()),
+        ),
     );
   });
 
diff --git a/test/request.bodies.ref.spec.ts b/test/request.bodies.ref.spec.ts
index 26e7d98..0a263c5 100644
--- a/test/request.bodies.ref.spec.ts
+++ b/test/request.bodies.ref.spec.ts
@@ -13,6 +13,9 @@ describe('request bodies', () => {
     app = await createApp(
       {
         apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
         validateResponses: true,
         unknownFormats: ['phone-number'],
       },
diff --git a/test/resources/multiple-validations.yaml b/test/resources/multiple-validations.yaml
new file mode 100644
index 0000000..ed6a8bc
--- /dev/null
+++ b/test/resources/multiple-validations.yaml
@@ -0,0 +1,53 @@
+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
+          content:
+            application/json:
+              schema:
+                type: object
+                required:
+                  - success
+                properties:
+                  success:
+                    type: boolean
+
+    get:
+      parameters:
+        - in: query
+          name: bname
+          schema:
+            type: string
+          required: true
+      responses:
+        200:
+          description: Success
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/Person"
+
+components:
+  schemas:
+    Person:
+      required:
+        - bname
+      type: object
+      properties:
+        bname:
+          type: string
+          format: starts-with-b
+          maxLength: 10
diff --git a/test/write.only.spec.ts b/test/write.only.spec.ts
index 565fcbf..324e3ca 100644
--- a/test/write.only.spec.ts
+++ b/test/write.only.spec.ts
@@ -10,38 +10,47 @@ describe(packageJson.name, () => {
   before(async () => {
     // Set up the express app
     const apiSpec = path.join('test', 'resources', 'write.only.yaml');
-    app = await createApp({ apiSpec, validateResponses: true }, 3005, (app) =>
-      app
-        .post(`${app.basePath}/products/inlined`, (req, res) => {
-          const body = req.body;
-          const excludeWriteOnly = req.query.exclude_write_only;
-          if (excludeWriteOnly) {
-            delete body.role;
-          }
-          res.json({
-            ...body,
-          });
-        })
-        .post(`${app.basePath}/products/nested`, (req, res) => {
-          const body = req.body;
-          const excludeWriteOnly = req.query.exclude_write_only;
-          body.id = 'test';
-          body.created_at = new Date().toISOString();
-          body.reviews = body.reviews.map((r) => ({
-            ...(excludeWriteOnly ? {} : { role_x: 'admin' }),
-            rating: r.rating ?? 2,
-          }));
+    app = await createApp(
+      {
+        apiSpec,
+        validateRequests: {
+          allErrors: true,
+        },
+        validateResponses: true,
+      },
+      3005,
+      (app) =>
+        app
+          .post(`${app.basePath}/products/inlined`, (req, res) => {
+            const body = req.body;
+            const excludeWriteOnly = req.query.exclude_write_only;
+            if (excludeWriteOnly) {
+              delete body.role;
+            }
+            res.json({
+              ...body,
+            });
+          })
+          .post(`${app.basePath}/products/nested`, (req, res) => {
+            const body = req.body;
+            const excludeWriteOnly = req.query.exclude_write_only;
+            body.id = 'test';
+            body.created_at = new Date().toISOString();
+            body.reviews = body.reviews.map((r) => ({
+              ...(excludeWriteOnly ? {} : { role_x: 'admin' }),
+              rating: r.rating ?? 2,
+            }));
 
-          if (excludeWriteOnly) {
-            delete body.role;
-            delete body.password;
-          }
-          res.json({
-            // id: 'xxxxx',
-            // created_at: '2024-02-09T17:32:28Z',
-            ...body,
-          });
-        }),
+            if (excludeWriteOnly) {
+              delete body.role;
+              delete body.password;
+            }
+            res.json({
+              // id: 'xxxxx',
+              // created_at: '2024-02-09T17:32:28Z',
+              ...body,
+            });
+          }),
     );
   });