Block invalid identifier (#895)

* block leading and trailing whitespace

* block leading and trailing whitespace, tabs and line feeds, and include code comments explaining restrictions

* update tests for updated invalid value message, add test for invalid padding

Author: carrolp

app/apollo/models/const.js

@@ -81,7 +81,15 @@ const DIRECTIVE_LIMITS = {
   MAX_JSON_ITEMS: config.has('directive_limits.max_json_items') ? config.get('directive_limits.max_json_items') : 128,
   MAX_CLUSTER_ARRAY_LENGTH: CLUSTER_MAX_TOTAL_LIMIT,
   MAX_GROUP_ARRAY_LENGTH: config.has('directive_limits.max_group_array_length') ? config.get('directive_limits.max_group_array_length') : 32,
-  INVALID_PATTERN: /[<>$%&!@()}{"#]{1,}/,
+  /*
+  A string is invalid if starts with whitespace, OR contains an invalid character from the list, OR ends with whitespace
+  Currently the same restriction is applied to all fields (see directives.js), but not all attributes need to restrict the characterset the same way.
+  Additional code refactoring would be required to explicitly test identifiers with different patterns than other fields.
+  The error messages emitted suggest only "alphabets, numbers, underscore and hyphen" are allowed, but this pattern does not accurately enforce that.
+  Consider adding '`\[\]\\\/*^. as additional invalid chars for identifiers, but until refactored thoroughly this will negatively affect other values.
+  E.g. ConfigurationVersion "type" attribute value "application/yaml" needs to contain a "/" and "description" attributes can be more freeform.
+  */
+  INVALID_PATTERN: /^\s|[<>$%&!@()}{"#\t\n\r]{1,}|\s$/,
 };
 
 // console.log('NODE_ENV: ' + config.util.getEnv('NODE_ENV') + `, DIRECTIVE_LIMITS: ${JSON.stringify(DIRECTIVE_LIMITS)}`);

app/apollo/test/channel.spec.js

@@ -320,7 +320,25 @@ describe('channel graphql test suite', () => {
         name: 'a_illegal_char#',
       });
       console.log(`${JSON.stringify(data.data)}`);
-      expect(data.data.errors[0].message).to.have.string('should only contain alphabets, numbers, underscore and hyphen');
+      expect(data.data.errors[0].message).to.have.string('should avoid leading or trailing whitespace and only contain alphabets, numbers, underscore and hyphen');
+    } catch (error) {
+      if (error.response) {
+        console.error('error encountered:  ', error.response.data);
+      } else {
+        console.error('error encountered:  ', error);
+      }
+      throw error;
+    }
+  });
+
+  it('add a channel with illegal whitespace', async () => {
+    try {
+      const data = await channelApi.addChannel(adminToken, {
+        orgId: org01._id,
+        name: ' a_illegal_pad ',
+      });
+      console.log(`${JSON.stringify(data.data)}`);
+      expect(data.data.errors[0].message).to.have.string('should avoid leading or trailing whitespace and only contain alphabets, numbers, underscore and hyphen');
     } catch (error) {
       if (error.response) {
         console.error('error encountered:  ', error.response.data);

app/apollo/test/group.spec.js

@@ -603,7 +603,7 @@ describe('groups graphql test suite', () => {
         uuid: group_02_uuid,
         clusters: ['cluster_01', 'cluster_04$']
       });
-      expect(data.data.errors[0].message).to.have.string('should only contain alphabets, numbers, underscore and hyphen');
+      expect(data.data.errors[0].message).to.have.string('should avoid leading or trailing whitespace and only contain alphabets, numbers, underscore and hyphen');
     } catch (error) {
       if (error.response) {
         console.error('error encountered:  ', error.response.data);

app/apollo/utils/directives.js

@@ -72,7 +72,7 @@ class IdentifierSanitizer extends Sanitizer {
     }
     if (this.arg !== 'content') {
       if (DIRECTIVE_LIMITS.INVALID_PATTERN.test(value)) {
-        throw new ValidationError(`The ${this.arg}'s value '${value}' should only contain alphabets, numbers, underscore and hyphen`);
+        throw new ValidationError(`The ${this.arg}'s value '${value}' should avoid leading or trailing whitespace and only contain alphabets, numbers, underscore and hyphen`);
       }
     }
   }
@@ -137,7 +137,7 @@ class JsonSanitizer extends Sanitizer {
           throw new ValidationError(`The json object element ${child} exceeded the value length ${DIRECTIVE_LIMITS.MAX_JSON_VALUE_LENGTH}`);
         }
         if (DIRECTIVE_LIMITS.INVALID_PATTERN.test(parent[child])) {
-          throw new ValidationError(`The ${this.arg} value ${parent[child]} should only contain alphabets, numbers, underscore and hyphen`);
+          throw new ValidationError(`The ${this.arg} value ${parent[child]} should avoid leading or trailing whitespace and only contain alphabets, numbers, underscore and hyphen`);
         }
       }
     }
@@ -185,4 +185,4 @@ class JsonDirective extends SchemaDirectiveVisitor {
   }
 }
 
-module.exports = { IdentifierDirective, JsonDirective };
+module.exports = { IdentifierDirective, JsonDirective };