From 7d8f77fd9d86ddd64c7d7d0d32ad525e57eba8d3 Mon Sep 17 00:00:00 2001
From: Dustin Popp <dpopp07@gmail.com>
Date: Tue, 17 Sep 2024 11:53:58 -0500
Subject: [PATCH] fix(ibm-string-attributes): expand rule to apply to response
 schemas

The rule only applied to "input" schemas, which are more important to check for
the attributes, but the API Handbook recommends the attributes for repsonse
schemas as well. This commit expands the rule to check for those scenarios.

Signed-off-by: Dustin Popp <dpopp07@gmail.com>
---
 docs/ibm-cloud-rules.md                       |  1 -
 .../src/functions/string-attributes.js        | 24 +-------
 .../ruleset/test/string-attributes.test.js    | 57 ++++++++++++-------
 packages/ruleset/test/utils/root-document.js  | 14 ++++-
 .../mock-files/oas3/clean-with-tabs.yml       |  9 +++
 .../cli-validator/mock-files/oas3/clean.yml   |  9 +++
 .../cli-validator/mock-files/oas31/clean.yml  |  9 +++
 7 files changed, 79 insertions(+), 44 deletions(-)

diff --git a/docs/ibm-cloud-rules.md b/docs/ibm-cloud-rules.md
index 5c052f43..300bc90a 100644
--- a/docs/ibm-cloud-rules.md
+++ b/docs/ibm-cloud-rules.md
@@ -6522,7 +6522,6 @@ fields in order to clearly define the set of valid values for the property.
 [<a href="https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-types#string">1</a>].
 <p>Note that these checks are bypassed for the following scenarios:
 <ul>
-<li>All checks are bypassed for string schemas that are used only within an operation response.
 <li>All checks are bypassed for string schemas that contain an <code>enum</code> field.</li>
 <li>The check for the <code>pattern</code> field is bypassed if <code>format</code> is set to 
 <code>binary</code>, <code>byte</code>, <code>date</code>, <code>date-time</code>, or <code>url</code>.</li>
diff --git a/packages/ruleset/src/functions/string-attributes.js b/packages/ruleset/src/functions/string-attributes.js
index e6aea44b..ce380d26 100644
--- a/packages/ruleset/src/functions/string-attributes.js
+++ b/packages/ruleset/src/functions/string-attributes.js
@@ -9,11 +9,7 @@ const {
   validateNestedSchemas,
 } = require('@ibm-cloud/openapi-ruleset-utilities');
 
-const {
-  getCompositeSchemaAttribute,
-  LoggerFactory,
-  pathMatchesRegexp,
-} = require('../utils');
+const { getCompositeSchemaAttribute, LoggerFactory } = require('../utils');
 
 let ruleId;
 let logger;
@@ -55,7 +51,7 @@ function stringBoundaryErrors(schema, path) {
 
   // Only check for the presence of validation keywords on input schemas
   // (i.e. those used for parameters and request bodies).
-  if (isStringSchema(schema) && isInputSchema(path)) {
+  if (isStringSchema(schema)) {
     logger.debug('schema is a string schema');
 
     // Perform these checks only if enum is not defined.
@@ -95,10 +91,7 @@ function stringBoundaryErrors(schema, path) {
         });
       }
     }
-  }
-
-  // Make sure string attributes aren't used for non-strings.
-  if (!isStringSchema(schema)) {
+  } else {
     // Make sure that string-related fields are not present in a non-string schema.
     if (schemaContainsAttribute(schema, 'pattern')) {
       errors.push({
@@ -135,14 +128,3 @@ function schemaContainsAttribute(schema, attrName) {
     s => attrName in s && isDefined(s[attrName])
   );
 }
-
-function isInputSchema(path) {
-  // Output schemas are much simpler to check for with regex.
-  // Use the inverse of that to determine input schemas.
-  const isOutputSchema = pathMatchesRegexp(
-    path,
-    /^paths,.*,responses,.+,(content|headers),.+,schema/
-  );
-
-  return !isOutputSchema;
-}
diff --git a/packages/ruleset/test/string-attributes.test.js b/packages/ruleset/test/string-attributes.test.js
index 68c60f48..79dbdba4 100644
--- a/packages/ruleset/test/string-attributes.test.js
+++ b/packages/ruleset/test/string-attributes.test.js
@@ -212,27 +212,6 @@ describe(`Spectral rule: ${ruleId}`, () => {
       expect(results).toHaveLength(0);
     });
 
-    it('Response body string schema has no keywords', async () => {
-      const testDocument = makeCopy(rootDocument);
-      testDocument.paths['/v1/movies/{movie_id}'].get.responses['200'] = {
-        content: {
-          'application/json': {
-            schema: {
-              properties: {
-                name: {
-                  type: 'string',
-                  description: 'no validation',
-                },
-              },
-            },
-          },
-        },
-      };
-
-      const results = await testRule(ruleId, rule, testDocument);
-      expect(results).toHaveLength(0);
-    });
-
     it('Response header string schema has no keywords', async () => {
       const testDocument = makeCopy(rootDocument);
       testDocument.paths['/v1/movies/{movie_id}'].get.responses.headers = {
@@ -615,5 +594,41 @@ describe(`Spectral rule: ${ruleId}`, () => {
       );
       expect(validation.severity).toBe(expectedSeverity);
     });
+
+    it('Response body string schema has no keywords', async () => {
+      const testDocument = makeCopy(rootDocument);
+      testDocument.paths['/v1/movies/{movie_id}'].get.responses['200'] = {
+        content: {
+          'application/json': {
+            schema: {
+              properties: {
+                name: {
+                  type: 'string',
+                  description: 'no validation',
+                },
+              },
+            },
+          },
+        },
+      };
+
+      const results = await testRule(ruleId, rule, testDocument);
+      expect(results).toHaveLength(3);
+
+      const expectedPath =
+        'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.name';
+      const expectedMessages = [
+        `String schemas should define property 'pattern'`,
+        `String schemas should define property 'minLength'`,
+        `String schemas should define property 'maxLength'`,
+      ];
+
+      for (let i = 0; i < results.length; i++) {
+        expect(results[i].code).toBe(ruleId);
+        expect(results[i].message).toBe(expectedMessages[i]);
+        expect(results[i].severity).toBe(expectedSeverity);
+        expect(results[i].path.join('.')).toBe(expectedPath);
+      }
+    });
   });
 });
diff --git a/packages/ruleset/test/utils/root-document.js b/packages/ruleset/test/utils/root-document.js
index 3830f43a..536f6baa 100644
--- a/packages/ruleset/test/utils/root-document.js
+++ b/packages/ruleset/test/utils/root-document.js
@@ -248,6 +248,7 @@ module.exports = {
                   type: 'string',
                   minLength: 0,
                   maxLength: 512,
+                  pattern: '^[a-zA-Z0-9 ]+$',
                 },
               },
             },
@@ -1039,7 +1040,9 @@ module.exports = {
           trace: {
             description: 'The error trace information.',
             type: 'string',
-            format: 'uuid',
+            format: 'identifier',
+            pattern: '^[a-zA-Z0-9 ]+$',
+            maxLength: 30,
           },
         },
       },
@@ -1055,10 +1058,16 @@ module.exports = {
           message: {
             description: 'The error message.',
             type: 'string',
+            pattern: '^[a-zA-Z0-9 ]+$',
+            minLength: 1,
+            maxLength: 128,
           },
           more_info: {
             description: 'Additional info about the error.',
             type: 'string',
+            pattern: '^[a-zA-Z0-9 ]+$',
+            minLength: 1,
+            maxLength: 128,
           },
           target: {
             $ref: '#/components/schemas/ErrorTarget',
@@ -1078,6 +1087,9 @@ module.exports = {
             description:
               'The name of the field/header/query parameter associated with the error.',
             type: 'string',
+            pattern: '^[a-zA-Z0-9 ]+$',
+            minLength: 1,
+            maxLength: 30,
           },
         },
       },
diff --git a/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml b/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml
index 00cc42f9..9f0abbc3 100644
--- a/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml
+++ b/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml
@@ -48,6 +48,9 @@ paths:
 							schema:
 								type: string
 								description: A link to the next page of responses
+								pattern: '^[a-zA-Z0-9]+$'
+								minLength: 1
+								maxLength: 128
 					content:
 						application/json:
 							schema:
@@ -159,6 +162,9 @@ components:
 				next_token:
 					type: string
 					description: next token
+					pattern: '^[a-zA-Z0-9_]+$'
+					minLength: 1
+					maxLength: 128
 				first:
 					allOf:
 						- $ref: '#/components/schemas/PageLink'
@@ -190,6 +196,9 @@ components:
 				href:
 					description: Request URL string
 					type: string
+					pattern: '^[a-zA-Z0-9]+$'
+					minLength: 1
+					maxLength: 128
 		Error:
 			type: object
 			description:
diff --git a/packages/validator/test/cli-validator/mock-files/oas3/clean.yml b/packages/validator/test/cli-validator/mock-files/oas3/clean.yml
index 7a8fd7ba..763dd131 100644
--- a/packages/validator/test/cli-validator/mock-files/oas3/clean.yml
+++ b/packages/validator/test/cli-validator/mock-files/oas3/clean.yml
@@ -48,6 +48,9 @@ paths:
               schema:
                 type: string
                 description: A link to the next page of responses
+                pattern: '^[a-zA-Z0-9]+$'
+                minLength: 1
+                maxLength: 128
           content:
             application/json:
               schema:
@@ -159,6 +162,9 @@ components:
         next_token:
           type: string
           description: next token
+          pattern: '^[a-zA-Z0-9_]+$'
+          minLength: 1
+          maxLength: 128
         first:
           allOf:
             - $ref: '#/components/schemas/PageLink'
@@ -190,6 +196,9 @@ components:
         href:
           description: Request URL string
           type: string
+          pattern: '^[a-zA-Z0-9]+$'
+          minLength: 1
+          maxLength: 128
     Error:
       type: object
       description:
diff --git a/packages/validator/test/cli-validator/mock-files/oas31/clean.yml b/packages/validator/test/cli-validator/mock-files/oas31/clean.yml
index aaed48ae..e1d498c9 100644
--- a/packages/validator/test/cli-validator/mock-files/oas31/clean.yml
+++ b/packages/validator/test/cli-validator/mock-files/oas31/clean.yml
@@ -48,6 +48,9 @@ paths:
               schema:
                 type: string
                 description: A link to the next page of responses
+                pattern: '^[a-zA-Z0-9]+$'
+                minLength: 1
+                maxLength: 128
           content:
             application/json:
               schema:
@@ -159,6 +162,9 @@ components:
         next_token:
           type: string
           description: next token
+          pattern: '^[a-zA-Z0-9_]+$'
+          minLength: 1
+          maxLength: 128
         first:
           allOf:
             - $ref: '#/components/schemas/PageLink'
@@ -190,6 +196,9 @@ components:
         href:
           description: Request URL string
           type: string
+          pattern: '^[a-zA-Z0-9]+$'
+          minLength: 1
+          maxLength: 128
     Error:
       type: object
       description: