From 2bad98a121069b3e09c347dbd177c3f3c5a506b8 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 5 Jun 2024 08:31:30 +0200 Subject: [PATCH] fix: disallow invalid tag values (#7268) This PR fixes how Unleash handles tag values. Specifically, it does these things: 1. Trims leading and trailing whitespace from tag values before inserting them into the database 2. Updates OpenAPI validation to not allow whitespace-only and to ignore leading and trailing whitespace Additionally, it moves the tag length constants into the constants file from the Joi tag schema file. This is because importing the values previously rendered them as undefined (probably due to a circular dependency somewhere in the system). This means that the previous values were also ignored by OpenAPI. UI updates reflecting this wil follow. ## Background When you tag a flag, there's nothing stopping you from using an entirely empty tag or a tag with leading/trailing whitespace. Empty tags make little sense and leading trailing whitespace differences are incredibly subtle: ![image](https://github.com/Unleash/unleash/assets/17786332/ec8fe193-8837-4c6a-b7bf-8766eff34eed) Additionally, leading and trailing whitespace is not shown in the dropdown list, so you'd have to guess at which is the right one. ![image](https://github.com/Unleash/unleash/assets/17786332/a14698ab-2bfd-4ec3-8814-b8e876d0aadb) --- .../openapi/spec/create-tag-schema.test.ts | 101 ++++++++++++++++++ src/lib/openapi/spec/create-tag-schema.ts | 24 +++++ src/lib/openapi/spec/index.ts | 1 + src/lib/openapi/spec/tag-schema.ts | 5 +- src/lib/routes/admin-api/tag.ts | 5 +- src/lib/services/tag-schema.ts | 3 +- src/lib/services/tag-service.test.ts | 29 +++++ src/lib/services/tag-service.ts | 6 +- src/lib/util/constants.ts | 3 + 9 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 src/lib/openapi/spec/create-tag-schema.test.ts create mode 100644 src/lib/openapi/spec/create-tag-schema.ts create mode 100644 src/lib/services/tag-service.test.ts diff --git a/src/lib/openapi/spec/create-tag-schema.test.ts b/src/lib/openapi/spec/create-tag-schema.test.ts new file mode 100644 index 0000000000..2a5cc63f6e --- /dev/null +++ b/src/lib/openapi/spec/create-tag-schema.test.ts @@ -0,0 +1,101 @@ +import { TAG_MAX_LENGTH, TAG_MIN_LENGTH } from '../../util'; +import { validateSchema } from '../validate'; + +describe('tag value validation', () => { + test.each([ + ['minimum', TAG_MIN_LENGTH], + ['maximum', TAG_MAX_LENGTH], + ])(`names with the %s length are valid`, (_, length) => { + const data = { + value: 'a'.repeat(length), + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toBeUndefined(); + }); + + test(`names can not be only whitespace`, () => { + const space = ' '.repeat(TAG_MIN_LENGTH); + const data = { + value: space, + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toMatchObject({ + errors: [{ keyword: 'pattern', instancePath: '/value' }], + }); + }); + + test(`names must be at least ${TAG_MIN_LENGTH} characters long, not counting leading and trailing whitespace`, () => { + const space = ' '.repeat(TAG_MIN_LENGTH); + const data = { + value: space + 'a'.repeat(TAG_MIN_LENGTH - 1) + space, + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toMatchObject({ + errors: [{ keyword: 'pattern', instancePath: '/value' }], + }); + }); + + test(`spaces within a tag value counts towards its maximum length`, () => { + const space = ' '.repeat(TAG_MAX_LENGTH); + const data = { + value: `a${space}z`, + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toMatchObject({ + errors: [{ keyword: 'pattern', instancePath: '/value' }], + }); + }); + + test(`leading and trailing whitespace does not count towards a name's maximum length`, () => { + const space = ' '.repeat(TAG_MAX_LENGTH); + const data = { + value: space + 'a'.repeat(TAG_MAX_LENGTH) + space, + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toBeUndefined(); + }); + + test(`tag names can contain spaces`, () => { + const data = { + value: 'tag name with spaces', + type: 'simple', + }; + + const validationResult = validateSchema( + '#/components/schemas/createTagSchema', + data, + ); + + expect(validationResult).toBeUndefined(); + }); +}); diff --git a/src/lib/openapi/spec/create-tag-schema.ts b/src/lib/openapi/spec/create-tag-schema.ts new file mode 100644 index 0000000000..e9fd8e0832 --- /dev/null +++ b/src/lib/openapi/spec/create-tag-schema.ts @@ -0,0 +1,24 @@ +import type { FromSchema } from 'json-schema-to-ts'; +import { TAG_MAX_LENGTH, TAG_MIN_LENGTH } from '../../util'; +import { tagSchema } from './tag-schema'; + +export const createTagSchema = { + ...tagSchema, + $id: '#/components/schemas/createTagSchema', + description: + 'Data used to create a new [tag](https://docs.getunleash.io/reference/tags)', + properties: { + ...tagSchema.properties, + value: { + type: 'string', + pattern: `^\\s*\\S.{${TAG_MIN_LENGTH - 2},${ + TAG_MAX_LENGTH - 2 + }}\\S\\s*$`, + description: `The value of the tag. The value must be between ${TAG_MIN_LENGTH} and ${TAG_MAX_LENGTH} characters long. Leading and trailing whitespace is ignored and will be trimmed before saving the tag value.`, + example: 'a-tag-value', + }, + }, + components: {}, +} as const; + +export type CreateTagSchema = FromSchema; diff --git a/src/lib/openapi/spec/index.ts b/src/lib/openapi/spec/index.ts index fdd1b04235..90e255c40f 100644 --- a/src/lib/openapi/spec/index.ts +++ b/src/lib/openapi/spec/index.ts @@ -54,6 +54,7 @@ export * from './create-invited-user-schema'; export * from './create-pat-schema'; export * from './create-strategy-schema'; export * from './create-strategy-variant-schema'; +export * from './create-tag-schema'; export * from './create-user-response-schema'; export * from './create-user-schema'; export * from './date-schema'; diff --git a/src/lib/openapi/spec/tag-schema.ts b/src/lib/openapi/spec/tag-schema.ts index 21cb8cec2c..060a80c092 100644 --- a/src/lib/openapi/spec/tag-schema.ts +++ b/src/lib/openapi/spec/tag-schema.ts @@ -1,5 +1,5 @@ import type { FromSchema } from 'json-schema-to-ts'; -import { TAG_MAX_LENGTH, TAG_MIN_LENGTH } from '../../services/tag-schema'; +import { TAG_MAX_LENGTH, TAG_MIN_LENGTH } from '../../util'; export const tagSchema = { $id: '#/components/schemas/tagSchema', @@ -11,16 +11,15 @@ export const tagSchema = { properties: { value: { type: 'string', + description: `The value of the tag.`, minLength: TAG_MIN_LENGTH, maxLength: TAG_MAX_LENGTH, - description: 'The value of the tag', example: 'a-tag-value', }, type: { type: 'string', minLength: TAG_MIN_LENGTH, maxLength: TAG_MAX_LENGTH, - default: 'simple', description: 'The [type](https://docs.getunleash.io/reference/tags#tag-types) of the tag', example: 'simple', diff --git a/src/lib/routes/admin-api/tag.ts b/src/lib/routes/admin-api/tag.ts index a7363f5563..d76ebf0804 100644 --- a/src/lib/routes/admin-api/tag.ts +++ b/src/lib/routes/admin-api/tag.ts @@ -27,6 +27,7 @@ import { } from '../../openapi/util/standard-responses'; import type FeatureTagService from '../../services/feature-tag-service'; import type { IFlagResolver } from '../../types'; +import type { CreateTagSchema } from '../../openapi'; const version = 1; @@ -94,7 +95,7 @@ class TagController extends Controller { ), ...getStandardResponses(400, 401, 403, 409, 415), }, - requestBody: createRequestSchema('tagSchema'), + requestBody: createRequestSchema('createTagSchema'), }), ], }); @@ -196,7 +197,7 @@ class TagController extends Controller { } async createTag( - req: IAuthRequest, + req: IAuthRequest, res: Response, ): Promise { const userName = extractUsername(req); diff --git a/src/lib/services/tag-schema.ts b/src/lib/services/tag-schema.ts index de541adc4a..280a3c06c8 100644 --- a/src/lib/services/tag-schema.ts +++ b/src/lib/services/tag-schema.ts @@ -1,9 +1,8 @@ import Joi from 'joi'; import { customJoi } from '../routes/util'; +import { TAG_MAX_LENGTH, TAG_MIN_LENGTH } from '../util'; -export const TAG_MIN_LENGTH = 2; -export const TAG_MAX_LENGTH = 50; export const tagSchema = Joi.object() .keys({ value: Joi.string().min(TAG_MIN_LENGTH).max(TAG_MAX_LENGTH), diff --git a/src/lib/services/tag-service.test.ts b/src/lib/services/tag-service.test.ts new file mode 100644 index 0000000000..7bfa4972f4 --- /dev/null +++ b/src/lib/services/tag-service.test.ts @@ -0,0 +1,29 @@ +import type { IUnleashConfig } from '../types/option'; +import { createTestConfig } from '../../test/config/test-config'; +import type EventService from '../features/events/event-service'; +import FakeTagStore from '../../test/fixtures/fake-tag-store'; +import TagService from './tag-service'; + +const config: IUnleashConfig = createTestConfig(); + +test('should trim tag values before saving them', async () => { + const tagStore = new FakeTagStore(); + const service = new TagService({ tagStore }, config, { + storeEvent: async () => {}, + } as unknown as EventService); + + await service.createTag( + { + value: ' test ', + type: 'simple', + }, + { id: 1, username: 'audit user', ip: '' }, + ); + + expect(tagStore.tags).toMatchObject([ + { + value: 'test', + type: 'simple', + }, + ]); +}); diff --git a/src/lib/services/tag-service.ts b/src/lib/services/tag-service.ts index 41990de4b7..0e90916c43 100644 --- a/src/lib/services/tag-service.ts +++ b/src/lib/services/tag-service.ts @@ -52,7 +52,11 @@ export default class TagService { } async createTag(tag: ITag, auditUser: IAuditUser): Promise { - const data = await this.validate(tag); + const trimmedTag = { + ...tag, + value: tag.value.trim(), + }; + const data = await this.validate(trimmedTag); await this.tagStore.createTag(data); await this.eventService.storeEvent( new TagCreatedEvent({ diff --git a/src/lib/util/constants.ts b/src/lib/util/constants.ts index af28acb4cb..976b8aecd4 100644 --- a/src/lib/util/constants.ts +++ b/src/lib/util/constants.ts @@ -17,6 +17,9 @@ export const PREDEFINED_ROLE_TYPES = [ROOT_ROLE_TYPE, PROJECT_ROLE_TYPE]; export const ROOT_ROLE_TYPES = [ROOT_ROLE_TYPE, CUSTOM_ROOT_ROLE_TYPE]; export const PROJECT_ROLE_TYPES = [PROJECT_ROLE_TYPE, CUSTOM_PROJECT_ROLE_TYPE]; +export const TAG_MIN_LENGTH = 2; +export const TAG_MAX_LENGTH = 50; + /* CONTEXT FIELD OPERATORS */ export const NOT_IN = 'NOT_IN';