Skip to content

Commit 392f1dd

Browse files
authored
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
1 parent 826ba62 commit 392f1dd

17 files changed

+336
-84
lines changed

src/framework/ajv/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ function createAjv(
3232
const { ajvFormats, ...ajvOptions } = options;
3333
const ajv = new AjvDraft4({
3434
...ajvOptions,
35-
allErrors: true,
3635
formats: formats,
3736
});
3837

src/framework/ajv/options.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,28 +11,31 @@ export class AjvOptions {
1111
constructor(options: NormalizedOpenApiValidatorOpts) {
1212
this.options = options;
1313
}
14+
1415
get preprocessor(): Options {
1516
return this.baseOptions();
1617
}
1718

1819
get response(): Options {
19-
const { coerceTypes, removeAdditional } = <ValidateResponseOpts>(
20+
const { allErrors, coerceTypes, removeAdditional } = <ValidateResponseOpts>(
2021
this.options.validateResponses
2122
);
2223
return {
2324
...this.baseOptions(),
25+
allErrors,
2426
useDefaults: false,
2527
coerceTypes,
2628
removeAdditional,
2729
};
2830
}
2931

3032
get request(): RequestValidatorOptions {
31-
const { allowUnknownQueryParameters, coerceTypes, removeAdditional } = <
33+
const { allErrors, allowUnknownQueryParameters, coerceTypes, removeAdditional } = <
3234
ValidateRequestOpts
3335
>this.options.validateRequests;
3436
return {
3537
...this.baseOptions(),
38+
allErrors,
3639
allowUnknownQueryParameters,
3740
coerceTypes,
3841
removeAdditional,
@@ -44,8 +47,13 @@ export class AjvOptions {
4447
}
4548

4649
private baseOptions(): Options {
47-
const { coerceTypes, formats, validateFormats, serDes, ajvFormats } =
48-
this.options;
50+
const {
51+
coerceTypes,
52+
formats,
53+
validateFormats,
54+
serDes,
55+
ajvFormats,
56+
} = this.options;
4957
const serDesMap = {};
5058
for (const serDesObject of serDes) {
5159
if (!serDesMap[serDesObject.format]) {
@@ -69,7 +77,7 @@ export class AjvOptions {
6977
coerceTypes,
7078
useDefaults: true,
7179
removeAdditional: false,
72-
validateFormats: validateFormats,
80+
validateFormats,
7381
formats,
7482
serDesMap,
7583
ajvFormats,

src/framework/types.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,26 @@ export interface Options extends ajv.Options {
4747
export interface RequestValidatorOptions extends Options, ValidateRequestOpts { }
4848

4949
export type ValidateRequestOpts = {
50+
/**
51+
* Whether AJV should check all rules and collect all errors or return after the first error.
52+
*
53+
* This should not be set to `true` in production. See [AJV: Security risks of trusted
54+
* schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas).
55+
*/
56+
allErrors?: boolean;
5057
allowUnknownQueryParameters?: boolean;
5158
coerceTypes?: boolean | 'array';
5259
removeAdditional?: boolean | 'all' | 'failing';
5360
};
5461

5562
export type ValidateResponseOpts = {
63+
/**
64+
* Whether AJV should check all rules and collect all errors or return after the first error.
65+
*
66+
* This should not be set to `true` in production. See [AJV: Security risks of trusted
67+
* schemas](https://ajv.js.org/security.html#security-risks-of-trusted-schemas).
68+
*/
69+
allErrors?: boolean;
5670
removeAdditional?: boolean | 'all' | 'failing';
5771
coerceTypes?: boolean | 'array';
5872
onError?: (err: InternalServerError, json: any, req: Request) => void;

test/699.spec.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,6 @@ describe('699 serialize response components only', () => {
182182
3005,
183183
(app) => {
184184
app.get([`${app.basePath}/users/:id?`], (req, res) => {
185-
debugger;
186185
if (typeof req.params.id !== 'string') {
187186
throw new Error("Should be not be deserialized to ObjectId object");
188187
}

test/881.spec.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ function createServer() {
5050
app.use(
5151
OpenApiValidator.middleware({
5252
apiSpec,
53+
validateRequests: {
54+
allErrors: true,
55+
},
5356
validateResponses: true,
5457
}),
5558
);

test/additional.props.spec.ts

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,22 @@ describe(packageJson.name, () => {
1515
'resources',
1616
'additional.properties.yaml',
1717
);
18-
app = await createApp({ apiSpec }, 3005, (app) =>
19-
app.use(
20-
`${app.basePath}/additional_props`,
21-
express
22-
.Router()
23-
.post(`/false`, (req, res) => res.json(req.body))
24-
.post(`/true`, (req, res) => res.json(req.body)),
25-
),
18+
app = await createApp(
19+
{
20+
apiSpec,
21+
validateRequests: {
22+
allErrors: true,
23+
},
24+
},
25+
3005,
26+
(app) =>
27+
app.use(
28+
`${app.basePath}/additional_props`,
29+
express
30+
.Router()
31+
.post(`/false`, (req, res) => res.json(req.body))
32+
.post(`/true`, (req, res) => res.json(req.body)),
33+
),
2634
);
2735
});
2836

test/ajv.options.spec.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ describe('AjvOptions', () => {
1010
apiSpec: './spec',
1111
validateApiSpec: false,
1212
validateRequests: {
13-
allowUnknownQueryParameters: false,
14-
coerceTypes: false,
13+
allowUnknownQueryParameters: false,
14+
coerceTypes: false,
1515
},
1616
validateResponses: {
1717
coerceTypes: false,
@@ -44,7 +44,7 @@ describe('AjvOptions', () => {
4444
expect(options.validateSchema).to.be.false;
4545
});
4646

47-
it('should not validate schema for multipar since schema is validated on startup', async () => {
47+
it('should not validate schema for multipart since schema is validated on startup', async () => {
4848
const ajv = new AjvOptions(baseOptions);
4949
const options = ajv.multipart;
5050
expect(options.validateSchema).to.be.false;
@@ -60,7 +60,7 @@ describe('AjvOptions', () => {
6060
},
6161
],
6262
});
63-
const options = ajv.multipart;
63+
const options = ajv.preprocessor;
6464
expect(options.serDesMap['custom-1']).has.property('deserialize');
6565
expect(options.serDesMap['custom-1']).does.not.have.property('serialize');
6666
});
@@ -75,7 +75,7 @@ describe('AjvOptions', () => {
7575
},
7676
],
7777
});
78-
const options = ajv.multipart;
78+
const options = ajv.preprocessor;
7979
expect(options.serDesMap).has.property('custom-1');
8080
expect(options.serDesMap['custom-1']).has.property('serialize');
8181
expect(options.serDesMap['custom-1']).does.not.have.property('deserialize');
@@ -92,7 +92,7 @@ describe('AjvOptions', () => {
9292
},
9393
],
9494
});
95-
const options = ajv.multipart;
95+
const options = ajv.preprocessor;
9696
expect(options.serDesMap).has.property('custom-1');
9797
expect(options.serDesMap['custom-1']).has.property('serialize');
9898
expect(options.serDesMap['custom-1']).has.property('deserialize');
@@ -116,7 +116,7 @@ describe('AjvOptions', () => {
116116
},
117117
],
118118
});
119-
const options = ajv.multipart;
119+
const options = ajv.preprocessor;
120120
expect(options.serDesMap).has.property('custom-1');
121121
expect(options.serDesMap['custom-1']).has.property('serialize');
122122
expect(options.serDesMap['custom-1']).has.property('deserialize');

test/all-errors.spec.ts

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import * as path from 'path';
2+
import * as request from 'supertest';
3+
import { expect } from 'chai';
4+
import { createApp } from './common/app';
5+
6+
describe('request body validation with and without allErrors', () => {
7+
let allErrorsApp;
8+
let notAllErrorsApp;
9+
10+
const defineRoutes = (app) => {
11+
app.post(`${app.basePath}/persons`, (req, res) => {
12+
res.send({ success: true });
13+
});
14+
app.get(`${app.basePath}/persons`, (req, res) => {
15+
res.send({ bname: req.query.bname });
16+
});
17+
};
18+
19+
before(async () => {
20+
const apiSpec = path.join('test', 'resources', 'multiple-validations.yaml');
21+
22+
allErrorsApp = await createApp(
23+
{
24+
apiSpec,
25+
formats: {
26+
'starts-with-b': (v) => /^b/i.test(v),
27+
},
28+
validateRequests: {
29+
allErrors: true,
30+
},
31+
validateResponses: {
32+
allErrors: true,
33+
},
34+
},
35+
3005,
36+
defineRoutes,
37+
true,
38+
);
39+
40+
notAllErrorsApp = await createApp(
41+
{
42+
apiSpec,
43+
formats: {
44+
'starts-with-b': (v) => /^b/i.test(v),
45+
},
46+
validateResponses: true,
47+
},
48+
3006,
49+
defineRoutes,
50+
true,
51+
);
52+
});
53+
54+
after(() => {
55+
allErrorsApp.server.close();
56+
notAllErrorsApp.server.close();
57+
});
58+
59+
it('should return 200 if short b-name is posted', async () =>
60+
request(allErrorsApp)
61+
.post(`${allErrorsApp.basePath}/persons`)
62+
.set('content-type', 'application/json')
63+
.send({ bname: 'Bob' })
64+
.expect(200));
65+
66+
it('should return 200 if short b-name is fetched', async () =>
67+
request(allErrorsApp)
68+
.get(`${allErrorsApp.basePath}/persons?bname=Bob`)
69+
.expect(200));
70+
71+
it('should include all request validation errors when allErrors=true', async () =>
72+
request(allErrorsApp)
73+
.post(`${allErrorsApp.basePath}/persons`)
74+
.set('content-type', 'application/json')
75+
.send({ bname: 'Maximillian' })
76+
.expect(400)
77+
.then((res) => {
78+
expect(res.body.errors).to.have.lengthOf(2);
79+
}));
80+
81+
it('should include only first request validation error when allErrors=false', async () =>
82+
request(notAllErrorsApp)
83+
.post(`${notAllErrorsApp.basePath}/persons`)
84+
.set('content-type', 'application/json')
85+
.send({ bname: 'Maximillian' })
86+
.expect(400)
87+
.then((res) => {
88+
expect(res.body.errors).to.have.lengthOf(1);
89+
}));
90+
91+
it('should include all response validation errors when allErrors=true', async () =>
92+
request(allErrorsApp)
93+
.get(`${allErrorsApp.basePath}/persons?bname=Maximillian`)
94+
.expect(500)
95+
.then((res) => {
96+
expect(res.body.errors).to.have.lengthOf(2);
97+
}));
98+
99+
it('should include only first response validation error when allErrors=false', async () =>
100+
request(notAllErrorsApp)
101+
.get(`${notAllErrorsApp.basePath}/persons?bname=Maximillian`)
102+
.expect(500)
103+
.then((res) => {
104+
expect(res.body.errors).to.have.lengthOf(1);
105+
}));
106+
});

test/all.of.spec.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,19 @@ describe(packageJson.name, () => {
1111
before(async () => {
1212
// Set up the express app
1313
const apiSpec = path.join('test', 'resources', 'all.of.yaml');
14-
app = await createApp({ apiSpec }, 3005, (app) =>
15-
app.use(
16-
`${app.basePath}`,
17-
express.Router().post(`/all_of`, (req, res) => res.json(req.body)),
18-
),
14+
app = await createApp(
15+
{
16+
apiSpec,
17+
validateRequests: {
18+
allErrors: true,
19+
},
20+
},
21+
3005,
22+
(app) =>
23+
app.use(
24+
`${app.basePath}`,
25+
express.Router().post(`/all_of`, (req, res) => res.json(req.body)),
26+
),
1927
);
2028
});
2129

test/empty.servers.spec.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,19 @@ describe('empty servers', () => {
1010
before(async () => {
1111
// Set up the express app
1212
const apiSpec = path.join('test', 'resources', 'empty.servers.yaml');
13-
app = await createApp({ apiSpec }, 3007, app =>
14-
app.use(
15-
``,
16-
express
17-
.Router()
18-
.get(`/pets`, (req, res) => res.json(req.body)),
19-
),
13+
app = await createApp(
14+
{
15+
apiSpec,
16+
validateRequests: {
17+
allErrors: true,
18+
},
19+
},
20+
3007,
21+
(app) =>
22+
app.use(
23+
``,
24+
express.Router().get(`/pets`, (req, res) => res.json(req.body)),
25+
),
2026
);
2127
});
2228

@@ -28,7 +34,7 @@ describe('empty servers', () => {
2834
request(app)
2935
.get(`/pets`)
3036
.expect(400)
31-
.then(r => {
37+
.then((r) => {
3238
expect(r.body.errors).to.be.an('array');
3339
expect(r.body.errors).to.have.length(2);
3440
}));

0 commit comments

Comments
 (0)