From f2ba82199b4047f0ce5794c148fcd91baa412550 Mon Sep 17 00:00:00 2001 From: Carmine DiMascio Date: Tue, 31 Dec 2019 13:53:40 -0500 Subject: [PATCH] allowReserved not validated properly. #158 --- .../parsers/req.parameter.mutator.ts | 38 ++++++++++- test/common/app.common.ts | 6 +- test/nullable.spec.ts | 2 +- test/openapi.spec.ts | 64 +++++++++++++++++-- test/query.params.spec.ts | 30 +++++++-- test/resources/query.params.yaml | 41 ++++++++++++ tsconfig.json | 5 +- 7 files changed, 170 insertions(+), 16 deletions(-) diff --git a/src/middlewares/parsers/req.parameter.mutator.ts b/src/middlewares/parsers/req.parameter.mutator.ts index be5500d4..6a5112fd 100644 --- a/src/middlewares/parsers/req.parameter.mutator.ts +++ b/src/middlewares/parsers/req.parameter.mutator.ts @@ -5,6 +5,7 @@ import { OpenApiRequestMetadata, ValidationSchema, } from '../../framework/types'; +import * as url from 'url'; import { validationError } from '../util'; import { dereferenceParameter, normalizeParameter } from './util'; import * as mediaTypeParser from 'media-typer'; @@ -14,6 +15,8 @@ type SchemaObject = OpenAPIV3.SchemaObject; type ReferenceObject = OpenAPIV3.ReferenceObject; type ParameterObject = OpenAPIV3.ParameterObject; +const RESERVED_CHARS = /[\:\/\?#\[\]@!\$&\'()\*\+,;=]/; + const ARRAY_DELIMITER = { form: ',', spaceDelimited: ' ', @@ -56,6 +59,10 @@ export class RequestParameterMutator { */ public modifyRequest(req: OpenApiRequest): void { const { parameters } = (req.openapi).schema; + const rawQuery = this.parseQueryStringUndecoded( + url.parse(req.originalUrl).query, + ); + parameters.forEach(p => { const parameter = dereferenceParameter(this._apiDocs, p); const { name, schema } = normalizeParameter(parameter); @@ -64,6 +71,10 @@ export class RequestParameterMutator { const i = req.originalUrl.indexOf('?'); const queryString = req.originalUrl.substr(i + 1); + if (parameter.in === 'query' && !parameter.allowReserved) { + this.validateReservedCharacters(name, rawQuery); + } + if (parameter.content) { this.handleContent(req, name, parameter); } else if (parameter.in === 'query' && this.isObjectOrXOf(schema)) { @@ -260,7 +271,32 @@ export class RequestParameterMutator { ): void { if (!delimiter) { const message = `Parameter 'style' has incorrect value '${parameter.style}' for [${parameter.name}]`; - throw validationError(400, this.path, message); + throw validationError(400, `.query.${parameter.name}`, message); } } + + private validateReservedCharacters( + name: string, + pairs: { [key: string]: string[] }, + ) { + const vs = pairs[name]; + if (!vs) return; + for (const v of vs) { + if (v?.match(RESERVED_CHARS)) { + const message = `Parameter '${name}' must be url encoded. It's value may not contain reserved characters.`; + throw validationError(400, `.query.${name}`, message); + } + } + } + + private parseQueryStringUndecoded(qs: string) { + if (!qs) return {}; + const q = qs.replace('?', ''); + return q.split('&').reduce((m, p) => { + const [k, v] = p.split('='); + m[k] = m[k] ?? []; + m[k].push(v); + return m; + }, {}); + } } diff --git a/test/common/app.common.ts b/test/common/app.common.ts index 74d88930..4a8c14d6 100644 --- a/test/common/app.common.ts +++ b/test/common/app.common.ts @@ -44,13 +44,15 @@ export function routes(app) { app.get(`${basePath}/pets`, function(req: Request, res: Response): void { res.json({ test: 'hi', - ...req.body, + body: req.body, + query: req.query, }); }); app.post(`${basePath}/pets`, function(req: Request, res: Response): void { res.json({ - ...req.body, + body: req.body, + query: req.query, id: 'new-id', }); }); diff --git a/test/nullable.spec.ts b/test/nullable.spec.ts index 64919a54..9e90f7a1 100644 --- a/test/nullable.spec.ts +++ b/test/nullable.spec.ts @@ -52,7 +52,7 @@ describe(packageJson.name, () => { }) .expect(200) .then(r => { - expect(r.body.tag).to.equal('my default value'); + expect(r.body.body.tag).to.equal('my default value'); })); it('should fail if required and not provided (nullable true)', async () => diff --git a/test/openapi.spec.ts b/test/openapi.spec.ts index 2b1b7c14..3d890e2b 100644 --- a/test/openapi.spec.ts +++ b/test/openapi.spec.ts @@ -81,18 +81,40 @@ describe(packageJson.name, () => { expect(e[0].message).to.equal('should be >= 5'); })); - it('should return 200 when JSON in query param', async () => + it('should return 400 when non-urlencoded JSON in query param', async () => request(apps[i]) .get(`${basePath}/pets`) .query(`limit=10&test=one&testJson={"foo": "bar"}`) .set('Accept', 'application/json') .expect('Content-Type', /json/) + .then(r => { + expect(r.body) + .to.have.property('message') + .that.equals( + "Parameter 'testJson' must be url encoded. It's value may not contain reserved characters.", + ); + })); + + it('should return 200 when JSON in query param', async () => + request(apps[i]) + .get(`${basePath}/pets`) + .query( + `limit=10&test=one&testJson=${encodeURIComponent( + '{"foo": "bar"}', + )}`, + ) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) .expect(200)); it('should return 400 when improper JSON in query param', async () => request(apps[i]) .get(`${basePath}/pets`) - .query(`limit=10&test=one&testJson={"foo": "test"}`) + .query({ + limit: 10, + test: 'one', + testJson: { foo: 'test' }, + }) .set('Accept', 'application/json') .expect('Content-Type', /json/) .expect(400) @@ -105,22 +127,56 @@ describe(packageJson.name, () => { ); })); - it('should return 200 when separated array in query param', async () => + it('should return 400 when comma separated array in query param', async () => + request(apps[i]) + .get(`${basePath}/pets`) + .query({ + limit: 10, + test: 'one', + testArray: 'foo,bar,baz', + }) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200)); + + it('should return 400 when comma separated array in query param is not url encoded', async () => request(apps[i]) .get(`${basePath}/pets`) .query(`limit=10&test=one&testArray=foo,bar,baz`) .set('Accept', 'application/json') .expect('Content-Type', /json/) + .expect(400) + .then(r => { + expect(r.body) + .to.have.property('message') + .that.equals( + "Parameter 'testArray' must be url encoded. It's value may not contain reserved characters.", + ); + })); + + it('should return 200 when separated array in query param', async () => + request(apps[i]) + .get(`${basePath}/pets`) + .query( + `limit=10&test=one&testArray=${encodeURIComponent('foo,bar,baz')}`, + ) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) .expect(200)); it('should return 400 when improper separated array in query param', async () => request(apps[i]) .get(`${basePath}/pets`) - .query(`limit=10&test=one&testArray=foo,bar,test`) + .query({ + limit: 10, + test: 'one', + testArray: 'foo,bar,test', + }) .set('Accept', 'application/json') .expect('Content-Type', /json/) .expect(400) .then(r => { + console.log(r.body); const e = r.body.errors; expect(e).to.have.length(1); expect(e[0].path).to.contain('testArray'); diff --git a/test/query.params.spec.ts b/test/query.params.spec.ts index 6dec8289..69ca63b2 100644 --- a/test/query.params.spec.ts +++ b/test/query.params.spec.ts @@ -16,7 +16,9 @@ describe(packageJson.name, () => { `${app.basePath}`, express .Router() - .post(`/pets/nullable`, (req, res) => res.json(req.body)), + .post(`/pets/nullable`, (req, res) => res.json(req.body)) + .get(`/no_reserved`, (req, res) => res.json(req.body)) + .get(`/allow_reserved`, (req, res) => res.json(req.body)), ), ); }); @@ -86,8 +88,26 @@ describe(packageJson.name, () => { }) .expect(200)); - it("should succeed when query param 'name' has empty value and sets allowEmptyValue: true", async () => - request(app) - .get(`${app.basePath}/pets?name=&tags=one&limit=10&breed=german_shepherd&owner_name=carmine`) - .expect(200)); + it("should succeed when query param 'name' has empty value and sets allowEmptyValue: true", async () => + request(app) + .get( + `${app.basePath}/pets?name=&tags=one&limit=10&breed=german_shepherd&owner_name=carmine`, + ) + .expect(200)); + + it('should not allow reserved characters', async () => + request(app) + .get(`${app.basePath}/no_reserved?value=ThisHas$ReservedChars!`) + .expect(400) + .then(r => { + const body = r.body; + expect(body.message).equals( + "Parameter 'value' must be url encoded. It's value may not contain reserved characters.", + ); + })); + + it('should may allow reserved characters when allowedReserved: true', async () => + request(app) + .get(`${app.basePath}/allow_reserved?value=ThisHas$ReservedChars!`) + .expect(200)); }); diff --git a/test/resources/query.params.yaml b/test/resources/query.params.yaml index a1d1472a..6869def6 100644 --- a/test/resources/query.params.yaml +++ b/test/resources/query.params.yaml @@ -30,6 +30,46 @@ servers: enum: - v1 paths: + /no_reserved: + get: + parameters: + - name: value + in: query + required: true + schema: + type: string + responses: + '200': + description: success + /allow_reserved: + get: + parameters: + - name: value + in: query + required: true + schema: + type: string + allowReserved: true + responses: + '200': + description: success + + /allow_reserved/array_explode: + get: + parameters: + - name: value + in: query + required: true + schema: + type: array + items: + type: string + explode: true + allowReserved: true + responses: + '200': + description: success + /pets: get: description: | @@ -45,6 +85,7 @@ paths: type: array items: type: string + # allowReserved: true - name: limit in: query description: maximum number of results to return diff --git a/tsconfig.json b/tsconfig.json index 7b9278ab..0c529e7a 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -9,11 +9,10 @@ "resolveJsonModule": true, "typeRoots": ["./node_modules/@types", "./typings"] }, - "exclude": ["node_modules", "a_reference"], + "exclude": ["node_modules"], "include": [ "typings/**/*.d.ts", "src/**/*.ts", - "src/framework/modded.express.mung.ts", - "a_reference/query.parser.ts" + "src/framework/modded.express.mung.ts" ] }