feat: add validator warning for binary string in "application/json" o…

…r parameter (#139) A binary string (type: string, format: binary) should not be included in JSON objects or used for parameters. This PR adds a validation to warn about that, `json_or_param_binary_string`. This PR also addresses a bug where $refs to valid responses were incorrectly flagged for problems.
IBM · Feb 18, 2020 · 9497333 · 9497333
1 parent ab2bc90
commit 9497333
Show file tree

Hide file tree

Showing 11 changed files with 1,103 additions and 105 deletions.
diff --git a/README.md b/README.md
@@ -253,6 +253,7 @@ The supported rules are described below:
 | array_of_arrays             | Flag any schema with a 'property' of type `array` with items of type `array`. | shared   |
 | property_case_convention    | Flag any property with a `name` that does not follow a given case convention. snake_case_only must be 'off' to use. | shared |
 | enum_case_convention        | Flag any enum with a `value` that does not follow a given case convention. snake_case_only must be 'off' to use.    | shared |
+| json_or_param_binary_string | Flag parameters or application/json request/response bodies with schema type: string, format: binary. | oas3 |
 
 ##### security_definitions
 | Rule                        | Description                                                                           | Spec   |
@@ -361,6 +362,11 @@ The default values for each rule are described below.
 | no_success_response_codes | warning |
 | no_response_body          | warning |
 
+##### schemas
+
+| Rule                        | Default |
+| --------------------------- | ------- |
+| json_or_param_binary_string | warning |
 
 ##### shared
 

diff --git a/src/.defaultsForValidator.js b/src/.defaultsForValidator.js
@@ -96,6 +96,9 @@ const defaults = {
       'no_response_codes': 'error',
       'no_success_response_codes': 'warning',
       'no_response_body': 'warning'
+    },
+    'schemas': {
+      'json_or_param_binary_string': 'warning'
     }
   }
 };

diff --git a/src/plugins/utils/findOctetSequencePaths.js b/src/plugins/utils/findOctetSequencePaths.js
@@ -0,0 +1,75 @@
+// Finds octet sequences (type: string, format: binary) in schemas including
+// nested arrays, objects, nested arrays of type object, objects with properties
+// that are nested arrays, and objects with properties that are objects This
+// function takes a resolved schema object (no refs) and returns a list of
+// paths to octet sequences (empty list if none found). The function accepts
+// the path both as an array and a string and returns the path in the same
+// format received:
+// typeof(path) === 'array' => [[path1, get], [path2, get], ...]
+// typeof(path) === 'string' => ['path1.get', path2.get, ...]
+
+const findOctetSequencePaths = (resolvedSchema, path) => {
+  if (!resolvedSchema) {
+    // schema is empty, no octet sequence
+    return [];
+  }
+
+  const pathsToOctetSequence = [];
+
+  if (resolvedSchema.type === 'string' && resolvedSchema.format === 'binary') {
+    pathsToOctetSequence.push(path);
+  } else if (resolvedSchema.type === 'array') {
+    pathsToOctetSequence.push(...arrayOctetSequences(resolvedSchema, path));
+  } else if (resolvedSchema.type === 'object') {
+    pathsToOctetSequence.push(...objectOctetSequences(resolvedSchema, path));
+  }
+
+  return pathsToOctetSequence;
+};
+
+function arrayOctetSequences(resolvedSchema, path) {
+  const arrayPathsToOctetSequence = [];
+  const arrayItems = resolvedSchema.items;
+  if (arrayItems !== undefined) {
+    // supports both array and string (delimited by .) paths
+    const pathToSchema = Array.isArray(path)
+      ? path.concat('items')
+      : `${path}.items`;
+    if (arrayItems.type === 'string' && arrayItems.format === 'binary') {
+      arrayPathsToOctetSequence.push(pathToSchema);
+    } else if (arrayItems.type === 'object' || arrayItems.type === 'array') {
+      arrayPathsToOctetSequence.push(
+        ...findOctetSequencePaths(arrayItems, pathToSchema)
+      );
+    }
+  }
+  return arrayPathsToOctetSequence;
+}
+
+function objectOctetSequences(resolvedSchema, path) {
+  const objectPathsToOctetSequence = [];
+  const objectProperties = resolvedSchema.properties;
+  if (objectProperties) {
+    Object.keys(objectProperties).forEach(function(prop) {
+      const propPath = Array.isArray(path)
+        ? path.concat(['properties', prop])
+        : `${path}.properties.${prop}`;
+      if (
+        objectProperties[prop].type === 'string' &&
+        objectProperties[prop].format === 'binary'
+      ) {
+        objectPathsToOctetSequence.push(propPath);
+      } else if (
+        objectProperties[prop].type === 'object' ||
+        objectProperties[prop].type === 'array'
+      ) {
+        objectPathsToOctetSequence.push(
+          ...findOctetSequencePaths(objectProperties[prop], propPath)
+        );
+      }
+    });
+  }
+  return objectPathsToOctetSequence;
+}
+
+module.exports.findOctetSequencePaths = findOctetSequencePaths;
diff --git a/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js b/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js
@@ -181,7 +181,7 @@ function formatValid(obj, isOAS3) {
       return (
         !schema.format ||
         includes(
-          ['byte', 'binary', 'date', 'date-time', 'password'],
+          ['byte', 'date', 'date-time', 'password'],
           schema.format.toLowerCase()
         )
       );

diff --git a/src/plugins/validation/oas3/semantic-validators/operations.js b/src/plugins/validation/oas3/semantic-validators/operations.js
@@ -4,15 +4,21 @@
 // Assertation 2. Operations with non-form request bodies should set the `x-codegen-request-body-name`
 // annotation (for code generation purposes)
 
+// Assertation 3. Request bodies with application/json content should not use schema
+// type: string, format: binary.
+
 const pick = require('lodash/pick');
 const each = require('lodash/each');
 const { hasRefProperty } = require('../../../utils');
+const findOctetSequencePaths = require('../../../utils/findOctetSequencePaths')
+  .findOctetSequencePaths;
 
 module.exports.validate = function({ resolvedSpec, jsSpec }, config) {
   const result = {};
   result.error = [];
   result.warning = [];
 
+  const configSchemas = config.schemas;
   config = config.operations;
 
   const REQUEST_BODY_NAME = 'x-codegen-request-body-name';
@@ -82,6 +88,28 @@ module.exports.validate = function({ resolvedSpec, jsSpec }, config) {
               });
             }
           }
+
+          // Assertation 3
+          const binaryStringStatus = configSchemas.json_or_param_binary_string;
+          if (binaryStringStatus !== 'off') {
+            for (const mimeType of requestBodyMimeTypes) {
+              if (mimeType === 'application/json') {
+                const schemaPath = `paths.${pathName}.${opName}.requestBody.content.${mimeType}.schema`;
+                const octetSequencePaths = findOctetSequencePaths(
+                  requestBodyContent[mimeType].schema,
+                  schemaPath
+                );
+                const message =
+                  'JSON request/response bodies should not contain binary (type: string, format: binary) values.';
+                for (const p of octetSequencePaths) {
+                  result[binaryStringStatus].push({
+                    path: p,
+                    message
+                  });
+                }
+              }
+            }
+          }
         }
       }
     });

diff --git a/src/plugins/validation/oas3/semantic-validators/parameters.js b/src/plugins/validation/oas3/semantic-validators/parameters.js
@@ -7,13 +7,20 @@
 
 // https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#parameterObject
 
+// Assertation 3:
+// A paramater should not use schema type: string, format: binary because there is now well-
+// defined way to encode an octet sequence in a URL.
+
 const { isParameterObject, walk } = require('../../../utils');
+const findOctetSequencePaths = require('../../../utils/findOctetSequencePaths')
+  .findOctetSequencePaths;
 
 module.exports.validate = function({ jsSpec }, config) {
   const result = {};
   result.error = [];
   result.warning = [];
 
+  const configSchemas = config.schemas;
   config = config.parameters;
 
   walk(jsSpec, [], function(obj, path) {
@@ -66,6 +73,40 @@ module.exports.validate = function({ jsSpec }, config) {
           });
         }
       }
+
+      const binaryStringStatus = configSchemas.json_or_param_binary_string;
+      if (binaryStringStatus !== 'off') {
+        const octetSequencePaths = [];
+        octetSequencePaths.push(
+          ...findOctetSequencePaths(obj.schema, path.concat(['schema']))
+        );
+        if (obj.content) {
+          Object.keys(obj.content).forEach(function(mimeType) {
+            if (mimeType === 'application/json') {
+              const paramContentPath = path.concat([
+                'content',
+                mimeType,
+                'schema'
+              ]);
+              octetSequencePaths.push(
+                ...findOctetSequencePaths(
+                  obj.content[mimeType].schema,
+                  paramContentPath
+                )
+              );
+            }
+          });
+        }
+
+        for (const p of octetSequencePaths) {
+          const message =
+            'Parameters should not contain binary (type: string, format: binary) values.';
+          result[binaryStringStatus].push({
+            path: p,
+            message
+          });
+        }
+      }
     }
   });
 

diff --git a/src/plugins/validation/oas3/semantic-validators/responses.js b/src/plugins/validation/oas3/semantic-validators/responses.js
@@ -12,30 +12,39 @@
 // Assertation 4:
 // A non-204 success response should define a response body
 
+// Assertation 5. Response bodies with application/json content should not use schema
+// type: string, format: binary.
+
 const { walk } = require('../../../utils');
+const findOctetSequencePaths = require('../../../utils/findOctetSequencePaths')
+  .findOctetSequencePaths;
 
 module.exports.validate = function({ resolvedSpec }, config) {
   const result = {};
   result.error = [];
   result.warning = [];
 
+  const configSchemas = config.schemas;
   config = config.responses;
 
   walk(resolvedSpec, [], function(obj, path) {
     const contentsOfResponsesObject =
       path[0] === 'paths' && path[path.length - 1] === 'responses';
 
     if (contentsOfResponsesObject) {
-      if (obj['204'] && obj['204'].content) {
-        result.error.push({
-          path: path.concat(['204', 'content']),
-          message: `A 204 response MUST NOT include a message-body.`
-        });
+      const [statusCodes, successCodes] = getResponseCodes(obj);
+
+      const binaryStringStatus = configSchemas.json_or_param_binary_string;
+      if (binaryStringStatus !== 'off') {
+        validateNoBinaryStringsInResponse(
+          obj,
+          result,
+          path,
+          binaryStringStatus
+        );
       }
-      const responseCodes = Object.keys(obj).filter(code =>
-        isResponseCode(code)
-      );
-      if (!responseCodes.length) {
+
+      if (!statusCodes.length) {
         const message =
           'Each `responses` object MUST have at least one response code.';
         const checkStatus = config.no_response_codes;
@@ -45,36 +54,33 @@ module.exports.validate = function({ resolvedSpec }, config) {
             message
           });
         }
+      } else if (!successCodes.length) {
+        const message =
+          'Each `responses` object SHOULD have at least one code for a successful response.';
+        const checkStatus = config.no_success_response_codes;
+        if (checkStatus !== 'off') {
+          result[checkStatus].push({
+            path,
+            message
+          });
+        }
       } else {
-        const successCodes = responseCodes.filter(
-          code => code.slice(0, 1) === '2'
-        );
-        if (!successCodes.length) {
-          const message =
-            'Each `responses` object SHOULD have at least one code for a successful response.';
-          const checkStatus = config.no_success_response_codes;
-          if (checkStatus !== 'off') {
-            result[checkStatus].push({
-              path,
-              message
-            });
-          }
-        } else {
-          const checkStatus = config.no_response_body;
-          // if response body rule is on, loops through success codes and issues warning (by default)
-          // for non-204 success responses without a response body
-          if (checkStatus !== 'off') {
-            for (const successCode of successCodes) {
-              if (successCode != '204' && !obj[successCode].content) {
-                result[checkStatus].push({
-                  path: path.concat([successCode]),
-                  message:
-                    `A ` +
-                    successCode +
-                    ` response should include a response body. Use 204 for responses without content.`
-                });
-              }
+        // validate success codes
+        for (const successCode of successCodes) {
+          if (successCode !== '204' && !obj[successCode].content) {
+            const checkStatus = config.no_response_body;
+            if (checkStatus !== 'off') {
+              const message = `A ${successCode} response should include a response body. Use 204 for responses without content.`;
+              result[checkStatus].push({
+                path: path.concat([successCode]),
+                message
+              });
             }
+          } else if (successCode === '204' && obj[successCode].content) {
+            result.error.push({
+              path: path.concat(['204', 'content']),
+              message: `A 204 response MUST NOT include a message-body.`
+            });
           }
         }
       }
@@ -84,7 +90,50 @@ module.exports.validate = function({ resolvedSpec }, config) {
   return { errors: result.error, warnings: result.warning };
 };
 
-function isResponseCode(code) {
+function getResponseCodes(responseObj) {
+  const statusCodes = Object.keys(responseObj).filter(code =>
+    isStatusCode(code)
+  );
+  const successCodes = statusCodes.filter(code => code.slice(0, 1) === '2');
+  return [statusCodes, successCodes];
+}
+
+function isStatusCode(code) {
   const allowedFirstDigits = ['1', '2', '3', '4', '5'];
   return code.length === 3 && allowedFirstDigits.includes(code.slice(0, 1));
 }
+
+function validateNoBinaryStringsInResponse(
+  responseObj,
+  result,
+  path,
+  binaryStringStatus
+) {
+  Object.keys(responseObj).forEach(function(responseCode) {
+    const responseBodyContent = responseObj[responseCode].content;
+    if (responseBodyContent) {
+      Object.keys(responseBodyContent).forEach(function(mimeType) {
+        if (mimeType === 'application/json') {
+          const schemaPath = path.concat([
+            responseCode,
+            'content',
+            mimeType,
+            'schema'
+          ]);
+          const octetSequencePaths = findOctetSequencePaths(
+            responseBodyContent[mimeType].schema,
+            schemaPath
+          );
+          const message =
+            'JSON request/response bodies should not contain binary (type: string, format: binary) values.';
+          for (const p of octetSequencePaths) {
+            result[binaryStringStatus].push({
+              path: p,
+              message
+            });
+          }
+        }
+      });
+    }
+  });
+}
diff --git a/test/plugins/validation/2and3/parameters-ibm.js b/test/plugins/validation/2and3/parameters-ibm.js
@@ -594,7 +594,7 @@ describe('validation plugin - semantic - parameters-ibm', () => {
                 in: 'query',
                 schema: {
                   type: 'string',
-                  format: 'binary'
+                  format: 'byte'
                 }
               }
             ]