From 33474f36522a7119927046f754d039d962a42f65 Mon Sep 17 00:00:00 2001 From: Sven Date: Thu, 7 Nov 2019 19:37:23 +0100 Subject: [PATCH 1/3] add a test to reproduce #111 --- test/resources/security.top.level.yaml | 15 +++++++++++++++ test/routes.spec.ts | 1 + test/security.top.level.spec.ts | 15 +++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/test/resources/security.top.level.yaml b/test/resources/security.top.level.yaml index ef8cdad0..7f6e9534 100644 --- a/test/resources/security.top.level.yaml +++ b/test/resources/security.top.level.yaml @@ -39,6 +39,21 @@ paths: '401': description: unauthorized + /api_query_keys: + get: + security: + - ApiKeyQueryAuth: [] + parameters: + - name: param1 + in: query + schema: + type: string + responses: + '200': + description: OK + '401': + description: unauthorized + /bearer: get: security: diff --git a/test/routes.spec.ts b/test/routes.spec.ts index 7e54a8d8..eb651d18 100644 --- a/test/routes.spec.ts +++ b/test/routes.spec.ts @@ -355,5 +355,6 @@ describe(packageJson.name, () => { }); }); }); + }); }); diff --git a/test/security.top.level.spec.ts b/test/security.top.level.spec.ts index a3f77000..b07a6917 100644 --- a/test/security.top.level.spec.ts +++ b/test/security.top.level.spec.ts @@ -26,6 +26,7 @@ describe(packageJson.name, () => { .Router() .get(`/api_key`, (req, res) => res.json({ logged_in: true })) .get(`/api_query_key`, (req, res) => res.json({ logged_in: true })) + .get(`/api_query_keys`, (req, res) => res.json({ logged_in: true })) .get(`/api_key_or_anonymous`, (req, res) => res.json({ logged_in: true }), ) @@ -95,6 +96,20 @@ describe(packageJson.name, () => { .expect(200) ); + it('should return 200 if apikey exist as query param with another query parmeter in the request', async () => + request(app) + .get(`${basePath}/api_query_keys`) + .query({ "APIKey": 'test' }) + .query({ "param1": 'anotherTest' }) + .expect(200) + ); + + it('should return 200 if apikey exist as query param with another query parmeter in the request', async () => + request(app) + .get(`${basePath}/api_query_keys`) + .query({ "APIKey": 'test' }) + .expect(200) + ); it('should return 200 if apikey or anonymous', async () => request(app) .get(`${basePath}/api_key_or_anonymous`) From 55440a82bd3a91d335247d32f14814f59802417b Mon Sep 17 00:00:00 2001 From: Sven Date: Fri, 8 Nov 2019 16:40:47 +0100 Subject: [PATCH 2/3] fix #111 by introducing a whiteList for sercurity query param --- src/middlewares/openapi.request.validator.ts | 18 ++++++++++++++++-- test/security.top.level.spec.ts | 2 +- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/middlewares/openapi.request.validator.ts b/src/middlewares/openapi.request.validator.ts index 249433b7..eeefa2ef 100644 --- a/src/middlewares/openapi.request.validator.ts +++ b/src/middlewares/openapi.request.validator.ts @@ -64,6 +64,8 @@ export class RequestValidator { private buildMiddleware(path, pathSchema, contentType) { const parameters = this.parametersToSchema(path, pathSchema.parameters); + const securityQueryParameter = this.getSecurityQueryParams(pathSchema, this._apiDocs.components.securitySchemes); + let requestBody = pathSchema.requestBody; if (requestBody && requestBody.hasOwnProperty('$ref')) { @@ -85,7 +87,7 @@ export class RequestValidator { const validator = this.ajv.compile(schema); return (req, res, next) => { - this.rejectUnknownQueryParams(req.query, schema.properties.query); + this.rejectUnknownQueryParams(req.query, schema.properties.query, securityQueryParameter); const shouldUpdatePathParams = Object.keys(req.openapi.pathParams).length > 0; @@ -155,9 +157,10 @@ export class RequestValidator { }; } - private rejectUnknownQueryParams(query, schema) { + private rejectUnknownQueryParams(query, schema, whiteList = []) { if (!schema.properties) return; const knownQueryParams = new Set(Object.keys(schema.properties)); + whiteList.forEach ( item => knownQueryParams.add(item)); const queryParams = Object.keys(query); for (const q of queryParams) { if (!knownQueryParams.has(q)) { @@ -185,6 +188,17 @@ export class RequestValidator { return {}; } + private getSecurityQueryParams(pathSchema, securitySchema) { + return (pathSchema.security && securitySchema) ? pathSchema.security + .filter( obj => Object.entries(obj).length !== 0 ) + .map( sec => { + const securityKey = Object.keys(sec)[0]; + return securitySchema[securityKey]; + }) + .filter(sec => sec && sec.in && sec.in === 'query') + .map(sec => sec.name) : []; + } + private parametersToSchema(path, parameters = []) { const schema = { query: {}, headers: {}, params: {}, cookies: {} }; const reqFields = { diff --git a/test/security.top.level.spec.ts b/test/security.top.level.spec.ts index b07a6917..92943d23 100644 --- a/test/security.top.level.spec.ts +++ b/test/security.top.level.spec.ts @@ -104,7 +104,7 @@ describe(packageJson.name, () => { .expect(200) ); - it('should return 200 if apikey exist as query param with another query parmeter in the request', async () => + it('should return 200 if apikey exist as query param with no query parmeter in the request but in the spec', async () => request(app) .get(`${basePath}/api_query_keys`) .query({ "APIKey": 'test' }) From 82c4bed8302ce3f4d6d0af887fef42c52942c7f4 Mon Sep 17 00:00:00 2001 From: Carmine DiMascio Date: Fri, 8 Nov 2019 22:33:02 -0500 Subject: [PATCH 3/3] apply linter/formatter --- src/middlewares/openapi.request.validator.ts | 31 +++++++++++------- test/security.top.level.spec.ts | 33 +++++++------------- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/src/middlewares/openapi.request.validator.ts b/src/middlewares/openapi.request.validator.ts index eeefa2ef..9d4b4f69 100644 --- a/src/middlewares/openapi.request.validator.ts +++ b/src/middlewares/openapi.request.validator.ts @@ -64,7 +64,10 @@ export class RequestValidator { private buildMiddleware(path, pathSchema, contentType) { const parameters = this.parametersToSchema(path, pathSchema.parameters); - const securityQueryParameter = this.getSecurityQueryParams(pathSchema, this._apiDocs.components.securitySchemes); + const securityQueryParameter = this.getSecurityQueryParams( + pathSchema, + this._apiDocs.components.securitySchemes, + ); let requestBody = pathSchema.requestBody; @@ -87,7 +90,11 @@ export class RequestValidator { const validator = this.ajv.compile(schema); return (req, res, next) => { - this.rejectUnknownQueryParams(req.query, schema.properties.query, securityQueryParameter); + this.rejectUnknownQueryParams( + req.query, + schema.properties.query, + securityQueryParameter, + ); const shouldUpdatePathParams = Object.keys(req.openapi.pathParams).length > 0; @@ -160,7 +167,7 @@ export class RequestValidator { private rejectUnknownQueryParams(query, schema, whiteList = []) { if (!schema.properties) return; const knownQueryParams = new Set(Object.keys(schema.properties)); - whiteList.forEach ( item => knownQueryParams.add(item)); + whiteList.forEach(item => knownQueryParams.add(item)); const queryParams = Object.keys(query); for (const q of queryParams) { if (!knownQueryParams.has(q)) { @@ -189,14 +196,16 @@ export class RequestValidator { } private getSecurityQueryParams(pathSchema, securitySchema) { - return (pathSchema.security && securitySchema) ? pathSchema.security - .filter( obj => Object.entries(obj).length !== 0 ) - .map( sec => { - const securityKey = Object.keys(sec)[0]; - return securitySchema[securityKey]; - }) - .filter(sec => sec && sec.in && sec.in === 'query') - .map(sec => sec.name) : []; + return pathSchema.security && securitySchema + ? pathSchema.security + .filter(obj => Object.entries(obj).length !== 0) + .map(sec => { + const securityKey = Object.keys(sec)[0]; + return securitySchema[securityKey]; + }) + .filter(sec => sec && sec.in && sec.in === 'query') + .map(sec => sec.name) + : []; } private parametersToSchema(path, parameters = []) { diff --git a/test/security.top.level.spec.ts b/test/security.top.level.spec.ts index 92943d23..9fdf4dd3 100644 --- a/test/security.top.level.spec.ts +++ b/test/security.top.level.spec.ts @@ -48,18 +48,14 @@ describe(packageJson.name, () => { const body = r.body; expect(body.errors).to.be.an('array'); expect(body.errors).to.have.length(1); - expect(body.errors[0].message).to.equals( - "'X-API-Key' header required", - ); + expect(body.errors[0].message).to.equals("'X-API-Key' header required"); })); - it('should return 200 if apikey exists', async () => request(app) .get(`${basePath}/api_key`) .set('X-API-Key', 'test') - .expect(200) - ); + .expect(200)); it('should return 404 if apikey exist, but path doesnt exist', async () => request(app) @@ -70,9 +66,7 @@ describe(packageJson.name, () => { const body = r.body; expect(body.errors).to.be.an('array'); expect(body.errors).to.have.length(1); - expect(body.errors[0].message).to.equals( - 'not found', - ); + expect(body.errors[0].message).to.equals('not found'); })); it('should return 405 if apikey exist, but invalid method used', async () => @@ -84,32 +78,27 @@ describe(packageJson.name, () => { const body = r.body; expect(body.errors).to.be.an('array'); expect(body.errors).to.have.length(1); - expect(body.errors[0].message).to.equals( - 'POST method not allowed', - ); + expect(body.errors[0].message).to.equals('POST method not allowed'); })); it('should return 200 if apikey exist as query param', async () => request(app) .get(`${basePath}/api_query_key`) - .query({ "APIKey": 'test' }) - .expect(200) - ); + .query({ APIKey: 'test' }) + .expect(200)); it('should return 200 if apikey exist as query param with another query parmeter in the request', async () => request(app) .get(`${basePath}/api_query_keys`) - .query({ "APIKey": 'test' }) - .query({ "param1": 'anotherTest' }) - .expect(200) - ); + .query({ APIKey: 'test' }) + .query({ param1: 'anotherTest' }) + .expect(200)); it('should return 200 if apikey exist as query param with no query parmeter in the request but in the spec', async () => request(app) .get(`${basePath}/api_query_keys`) - .query({ "APIKey": 'test' }) - .expect(200) - ); + .query({ APIKey: 'test' }) + .expect(200)); it('should return 200 if apikey or anonymous', async () => request(app) .get(`${basePath}/api_key_or_anonymous`)