From c91fb9a86b31ce81605656e4f5b71009ece82607 Mon Sep 17 00:00:00 2001 From: Paul Carroll Date: Tue, 18 May 2021 16:00:48 -0500 Subject: [PATCH 1/3] block leading and trailing whitespace --- app/apollo/models/const.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/apollo/models/const.js b/app/apollo/models/const.js index a160e4e4d..072ddcedf 100644 --- a/app/apollo/models/const.js +++ b/app/apollo/models/const.js @@ -76,7 +76,10 @@ 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 OR ends with whitespace, OR contains an invalid character from the list. + // DevNote: The error messages emitted suggest only "alphabets, numbers, underscore and hyphen" are allowed, but this pattern does not accurately enforce that. + // DevNote: Consider adding '`\[\]\\\/*^. as invalid chars, but this will affect some strings like "type":"application/yaml" and "description" (both from 'ConfigurationVersion' resources) + INVALID_PATTERN: /^\s|[<>$%&!@()}{"#\t\n\r]{1,}|\s$/, }; // console.log('NODE_ENV: ' + config.util.getEnv('NODE_ENV') + `, DIRECTIVE_LIMITS: ${JSON.stringify(DIRECTIVE_LIMITS)}`); From e28b8c355cc1b2b71ae5bc730a75a4562919dbf7 Mon Sep 17 00:00:00 2001 From: Paul Carroll Date: Wed, 19 May 2021 10:03:39 -0500 Subject: [PATCH 2/3] block leading and trailing whitespace, tabs and line feeds, and include code comments explaining restrictions --- app/apollo/models/const.js | 11 ++++++++--- app/apollo/utils/directives.js | 6 +++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/apollo/models/const.js b/app/apollo/models/const.js index 072ddcedf..48975944f 100644 --- a/app/apollo/models/const.js +++ b/app/apollo/models/const.js @@ -76,9 +76,14 @@ 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, - // A string is invalid if starts OR ends with whitespace, OR contains an invalid character from the list. - // DevNote: The error messages emitted suggest only "alphabets, numbers, underscore and hyphen" are allowed, but this pattern does not accurately enforce that. - // DevNote: Consider adding '`\[\]\\\/*^. as invalid chars, but this will affect some strings like "type":"application/yaml" and "description" (both from 'ConfigurationVersion' resources) + /* + 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$/, }; diff --git a/app/apollo/utils/directives.js b/app/apollo/utils/directives.js index 695545bb5..3dcbf72b5 100644 --- a/app/apollo/utils/directives.js +++ b/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 }; \ No newline at end of file +module.exports = { IdentifierDirective, JsonDirective }; From 12e18739281cdc2ec71031a87334768b191a1682 Mon Sep 17 00:00:00 2001 From: Paul Carroll Date: Wed, 19 May 2021 13:00:01 -0500 Subject: [PATCH 3/3] update tests for updated invalid value message, add test for invalid padding --- app/apollo/test/channel.spec.js | 20 +++++++++++++++++++- app/apollo/test/group.spec.js | 2 +- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/apollo/test/channel.spec.js b/app/apollo/test/channel.spec.js index 06a536e89..bc8e72c5b 100644 --- a/app/apollo/test/channel.spec.js +++ b/app/apollo/test/channel.spec.js @@ -316,7 +316,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); diff --git a/app/apollo/test/group.spec.js b/app/apollo/test/group.spec.js index f55acd8f4..11dd8ec15 100644 --- a/app/apollo/test/group.spec.js +++ b/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);