From 4277079d2651ef8324dc76fb226033f19273aab6 Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Wed, 2 Sep 2020 11:44:23 -0500 Subject: [PATCH] fix: dont validate schema properties called 'responses' as responses (#194) --- .travis.yml | 1 + src/plugins/utils/index.js | 1 + src/plugins/utils/is-response.js | 35 ++++++++++++ .../items-required-for-array-objects.js | 9 +-- .../2and3/semantic-validators/responses.js | 5 +- .../oas3/semantic-validators/responses.js | 10 ++-- test/plugins/utils/is-response.test.js | 55 +++++++++++++++++++ .../plugins/validation/oas3/responses.test.js | 8 ++- 8 files changed, 111 insertions(+), 13 deletions(-) create mode 100644 src/plugins/utils/is-response.js create mode 100644 test/plugins/utils/is-response.test.js diff --git a/.travis.yml b/.travis.yml index 3b6bb4a68..9b7515933 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,7 @@ cache: npm: false script: - npm run test-travis +- npm run lint after_success: - npm run report-coverage deploy: diff --git a/src/plugins/utils/index.js b/src/plugins/utils/index.js index f62669957..367a343c3 100644 --- a/src/plugins/utils/index.js +++ b/src/plugins/utils/index.js @@ -3,4 +3,5 @@ module.exports.checkCase = require('./caseConventionCheck'); module.exports.walk = require('./walk'); module.exports.isParameterObject = require('./isParameter'); +module.exports.isResponseObject = require('./is-response'); module.exports.hasRefProperty = require('./hasRefProperty'); diff --git a/src/plugins/utils/is-response.js b/src/plugins/utils/is-response.js new file mode 100644 index 000000000..b5cd04b7e --- /dev/null +++ b/src/plugins/utils/is-response.js @@ -0,0 +1,35 @@ +module.exports = (path, isOAS3 = true) => { + const operations = [ + 'get', + 'put', + 'post', + 'delete', + 'options', + 'head', + 'patch', + 'trace' + ]; + + const pathLength = path.length; + + // a necessary but not sufficient check for a response object + // without these, a schema property named "responses" + // may be validated as a response + const isResponsesProperty = path[pathLength - 1] === 'responses'; + + // three scenarios: + // 1) inside of an operation + const isOperationResponse = operations.includes(path[pathLength - 2]); + + // 2) inside of components -> responses (oas 3) + const isResponseInComponents = + pathLength === 2 && path[pathLength - 2] === 'components' && isOAS3; + + // 3) top level responses (swagger 2) + const isTopLevelResponse = pathLength === 1 && !isOAS3; + + return ( + isResponsesProperty && + (isOperationResponse || isResponseInComponents || isTopLevelResponse) + ); +}; diff --git a/src/plugins/validation/2and3/semantic-validators/items-required-for-array-objects.js b/src/plugins/validation/2and3/semantic-validators/items-required-for-array-objects.js index 399d11155..3a4e55a87 100644 --- a/src/plugins/validation/2and3/semantic-validators/items-required-for-array-objects.js +++ b/src/plugins/validation/2and3/semantic-validators/items-required-for-array-objects.js @@ -28,7 +28,7 @@ const checkReqProp = function(jsSpec, obj, requiredProp) { } else if (Array.isArray(obj.anyOf) || Array.isArray(obj.oneOf)) { const childList = obj.anyOf || obj.oneOf; let reqPropDefined = true; - childList.forEach((childObj) => { + childList.forEach(childObj => { if (!checkReqProp(jsSpec, childObj, requiredProp)) { reqPropDefined = false; } @@ -36,7 +36,7 @@ const checkReqProp = function(jsSpec, obj, requiredProp) { return reqPropDefined; } else if (Array.isArray(obj.allOf)) { let reqPropDefined = false; - obj.allOf.forEach((childObj) => { + obj.allOf.forEach(childObj => { if (checkReqProp(jsSpec, childObj, requiredProp)) { reqPropDefined = true; } @@ -44,7 +44,7 @@ const checkReqProp = function(jsSpec, obj, requiredProp) { return reqPropDefined; } return false; -} +}; module.exports.validate = function({ jsSpec }, config) { const messages = new MessageCarrier(); @@ -71,7 +71,8 @@ module.exports.validate = function({ jsSpec }, config) { } // Assertation 2 - const undefinedRequiredProperties = config.schemas.undefined_required_properties; + const undefinedRequiredProperties = + config.schemas.undefined_required_properties; if (Array.isArray(obj.required)) { obj.required.forEach((requiredProp, i) => { if (!checkReqProp(jsSpec, obj, requiredProp)) { diff --git a/src/plugins/validation/2and3/semantic-validators/responses.js b/src/plugins/validation/2and3/semantic-validators/responses.js index f9a95a308..221847684 100644 --- a/src/plugins/validation/2and3/semantic-validators/responses.js +++ b/src/plugins/validation/2and3/semantic-validators/responses.js @@ -1,5 +1,5 @@ const each = require('lodash/each'); -const { walk } = require('../../../utils'); +const { walk, isResponseObject } = require('../../../utils'); const MessageCarrier = require('../../../utils/messageCarrier'); const INLINE_SCHEMA_MESSAGE = @@ -11,10 +11,9 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) { config = config.responses; walk(jsSpec, [], function(obj, path) { - const contentsOfResponsesObject = path[path.length - 1] === 'responses'; const isRef = !!obj.$ref; - if (contentsOfResponsesObject && !isRef) { + if (isResponseObject(path, isOAS3) && !isRef) { each(obj, (response, responseKey) => { if (isOAS3) { each(response.content, (mediaType, mediaTypeKey) => { diff --git a/src/plugins/validation/oas3/semantic-validators/responses.js b/src/plugins/validation/oas3/semantic-validators/responses.js index ad2fb9a9f..ac04da4a3 100644 --- a/src/plugins/validation/oas3/semantic-validators/responses.js +++ b/src/plugins/validation/oas3/semantic-validators/responses.js @@ -15,7 +15,7 @@ // Assertation 5. Response bodies with application/json content should not use schema // type: string, format: binary. -const { walk } = require('../../../utils'); +const { walk, isResponseObject } = require('../../../utils'); const MessageCarrier = require('../../../utils/messageCarrier'); const findOctetSequencePaths = require('../../../utils/findOctetSequencePaths') .findOctetSequencePaths; @@ -27,10 +27,10 @@ module.exports.validate = function({ resolvedSpec }, config) { config = config.responses; walk(resolvedSpec, [], function(obj, path) { - const contentsOfResponsesObject = - path[0] === 'paths' && path[path.length - 1] === 'responses'; - - if (contentsOfResponsesObject) { + // because we are using the resolved spec here, + // and only want to validate the responses inside of operations, + // check that we are within the `paths` object + if (isResponseObject(path) && path[0] === 'paths') { const [statusCodes, successCodes] = getResponseCodes(obj); const binaryStringStatus = configSchemas.json_or_param_binary_string; diff --git a/test/plugins/utils/is-response.test.js b/test/plugins/utils/is-response.test.js new file mode 100644 index 000000000..8a841142d --- /dev/null +++ b/test/plugins/utils/is-response.test.js @@ -0,0 +1,55 @@ +const expect = require('expect'); +const { isResponseObject } = require('../../../src/plugins/utils'); + +describe('is response object - util', () => { + describe('OpenAPI 3', () => { + const isOas3 = true; + + it('should return false for top-level responses objects', () => { + const path = ['responses']; + expect(isResponseObject(path, isOas3)).toBe(false); + }); + + it('should return true for components responses objects', () => { + const path = ['components', 'responses']; + + // the second argument, `isOas3`, is optional. test that optionality + expect(isResponseObject(path)).toBe(true); + }); + + it('should return true for operation responses', () => { + const path = ['paths', '/resource', 'post', 'responses']; + expect(isResponseObject(path, isOas3)).toBe(true); + }); + + it('should return false for non responses', () => { + const path = ['paths', '/resource', 'post', 'parameters']; + expect(isResponseObject(path, isOas3)).toBe(false); + }); + + it('should return false for schemas with properties named responses', () => { + const path = [ + 'components', + 'schemas', + 'MySchema', + 'properties', + 'responses' + ]; + expect(isResponseObject(path, isOas3)).toBe(false); + }); + }); + + describe('Swagger 2', () => { + const isOas3 = false; + + it('should return true for top-level responses objects', () => { + const path = ['responses']; + expect(isResponseObject(path, isOas3)).toBe(true); + }); + + it('should return false for components responses objects', () => { + const path = ['components', 'responses']; + expect(isResponseObject(path, isOas3)).toBe(false); + }); + }); +}); diff --git a/test/plugins/validation/oas3/responses.test.js b/test/plugins/validation/oas3/responses.test.js index be10c190a..3c1ca77d4 100644 --- a/test/plugins/validation/oas3/responses.test.js +++ b/test/plugins/validation/oas3/responses.test.js @@ -110,7 +110,13 @@ describe('validation plugin - semantic - responses - oas3', function() { const res = validate({ resolvedSpec: spec }, config); expect(res.warnings.length).toEqual(0); expect(res.errors.length).toEqual(1); - expect(res.errors[0].path).toEqual(['paths', '/pets', 'get', 'responses', '200']); + expect(res.errors[0].path).toEqual([ + 'paths', + '/pets', + 'get', + 'responses', + '200' + ]); expect(res.errors[0].message).toEqual( 'All responses must include a description.' );