fix(core): Use class-validator with XSS check for survey answers (#10490)

Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>

packages/cli/src/GenericHelpers.ts

@@ -1,4 +1,4 @@
-import { ValidationError, validate } from 'class-validator';
+import { validate } from 'class-validator';
 import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
 import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
 import type { TagEntity } from '@db/entities/TagEntity';
@@ -9,7 +9,7 @@ import type {
 	UserUpdatePayload,
 } from '@/requests';
 import { BadRequestError } from './errors/response-errors/bad-request.error';
-import { NoXss } from '@/validators/no-xss.validator';
+import type { PersonalizationSurveyAnswersV4 } from './controllers/survey-answers.dto';
 
 export async function validateEntity(
 	entity:
@@ -19,7 +19,8 @@ export async function validateEntity(
 		| User
 		| UserUpdatePayload
 		| UserRoleChangePayload
-		| UserSettingsUpdatePayload,
+		| UserSettingsUpdatePayload
+		| PersonalizationSurveyAnswersV4,
 ): Promise<void> {
 	const errors = await validate(entity);
 
@@ -37,37 +38,3 @@ export async function validateEntity(
 }
 
 export const DEFAULT_EXECUTIONS_GET_ALL_LIMIT = 20;
-
-class StringWithNoXss {
-	@NoXss()
-	value: string;
-
-	constructor(value: string) {
-		this.value = value;
-	}
-}
-
-// Temporary solution until we implement payload validation middleware
-export async function validateRecordNoXss(record: Record<string, string>) {
-	const errors: ValidationError[] = [];
-
-	for (const [key, value] of Object.entries(record)) {
-		const stringWithNoXss = new StringWithNoXss(value);
-		const validationErrors = await validate(stringWithNoXss);
-
-		if (validationErrors.length > 0) {
-			const error = new ValidationError();
-			error.property = key;
-			error.constraints = validationErrors[0].constraints;
-			errors.push(error);
-		}
-	}
-
-	if (errors.length > 0) {
-		const errorMessages = errors
-			.map((error) => `${error.property}: ${Object.values(error.constraints ?? {}).join(', ')}`)
-			.join(' | ');
-
-		throw new BadRequestError(errorMessages);
-	}
-}

packages/cli/src/controllers/__tests__/me.controller.test.ts

@@ -349,10 +349,40 @@ describe('MeController', () => {
 			);
 		});
 
-		it('should throw BadRequestError on XSS attempt', async () => {
-			const req = mock<MeRequest.SurveyAnswers>({
-				body: { 'test-answer': '<script>alert("XSS")</script>' },
-			});
+		test.each([
+			'automationGoalDevops',
+			'companyIndustryExtended',
+			'otherCompanyIndustryExtended',
+			'automationGoalSm',
+			'usageModes',
+		])('should throw BadRequestError on XSS attempt for an array field %s', async (fieldName) => {
+			const req = mock<MeRequest.SurveyAnswers>();
+			req.body = {
+				version: 'v4',
+				personalization_survey_n8n_version: '1.0.0',
+				personalization_survey_submitted_at: new Date().toISOString(),
+				[fieldName]: ['<script>alert("XSS")</script>'],
+			};
+
+			await expect(controller.storeSurveyAnswers(req)).rejects.toThrowError(BadRequestError);
+		});
+
+		test.each([
+			'automationGoalDevopsOther',
+			'companySize',
+			'companyType',
+			'automationGoalSmOther',
+			'roleOther',
+			'reportedSource',
+			'reportedSourceOther',
+		])('should throw BadRequestError on XSS attempt for a string field %s', async (fieldName) => {
+			const req = mock<MeRequest.SurveyAnswers>();
+			req.body = {
+				version: 'v4',
+				personalization_survey_n8n_version: '1.0.0',
+				personalization_survey_submitted_at: new Date().toISOString(),
+				[fieldName]: '<script>alert("XSS")</script>',
+			};
 
 			await expect(controller.storeSurveyAnswers(req)).rejects.toThrowError(BadRequestError);
 		});

packages/cli/src/controllers/me.controller.ts

@@ -6,7 +6,7 @@ import { randomBytes } from 'crypto';
 import { AuthService } from '@/auth/auth.service';
 import { Delete, Get, Patch, Post, RestController } from '@/decorators';
 import { PasswordUtility } from '@/services/password.utility';
-import { validateEntity, validateRecordNoXss } from '@/GenericHelpers';
+import { validateEntity } from '@/GenericHelpers';
 import type { User } from '@db/entities/User';
 import {
 	AuthenticatedRequest,
@@ -25,6 +25,7 @@ import { isApiEnabled } from '@/PublicApi';
 import { EventService } from '@/events/event.service';
 import { MfaService } from '@/Mfa/mfa.service';
 import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error';
+import { PersonalizationSurveyAnswersV4 } from './survey-answers.dto';
 
 export const API_KEY_PREFIX = 'n8n_api_';
 
@@ -195,7 +196,7 @@ export class MeController {
 
 		if (!personalizationAnswers) {
 			this.logger.debug(
-				'Request to store user personalization survey failed because of empty payload',
+				'Request to store user personalization survey failed because of undefined payload',
 				{
 					userId: req.user.id,
 				},
@@ -203,12 +204,18 @@ export class MeController {
 			throw new BadRequestError('Personalization answers are mandatory');
 		}
 
-		await validateRecordNoXss(personalizationAnswers);
+		const validatedAnswers = plainToInstance(
+			PersonalizationSurveyAnswersV4,
+			personalizationAnswers,
+			{ excludeExtraneousValues: true },
+		);
+
+		await validateEntity(validatedAnswers);
 
 		await this.userRepository.save(
 			{
 				id: req.user.id,
-				personalizationAnswers,
+				personalizationAnswers: validatedAnswers,
 			},
 			{ transaction: false },
 		);
@@ -217,7 +224,7 @@ export class MeController {
 
 		this.eventService.emit('user-submitted-personalization-survey', {
 			userId: req.user.id,
-			answers: personalizationAnswers,
+			answers: validatedAnswers,
 		});
 
 		return { success: true };

packages/cli/src/controllers/survey-answers.dto.ts

@@ -0,0 +1,109 @@
+import { NoXss } from '@/validators/no-xss.validator';
+import { Expose } from 'class-transformer';
+import { IsString, IsArray, IsOptional, IsEmail, IsEnum } from 'class-validator';
+import type { IPersonalizationSurveyAnswersV4 } from 'n8n-workflow';
+
+export class PersonalizationSurveyAnswersV4 implements IPersonalizationSurveyAnswersV4 {
+	@NoXss()
+	@Expose()
+	@IsEnum(['v4'])
+	version: 'v4';
+
+	@NoXss()
+	@Expose()
+	@IsString()
+	personalization_survey_submitted_at: string;
+
+	@NoXss()
+	@Expose()
+	@IsString()
+	personalization_survey_n8n_version: string;
+
+	@Expose()
+	@IsOptional()
+	@IsArray()
+	@NoXss({ each: true })
+	@IsString({ each: true })
+	automationGoalDevops?: string[] | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	automationGoalDevopsOther?: string | null;
+
+	@NoXss({ each: true })
+	@Expose()
+	@IsOptional()
+	@IsArray()
+	@IsString({ each: true })
+	companyIndustryExtended?: string[] | null;
+
+	@NoXss({ each: true })
+	@Expose()
+	@IsOptional()
+	@IsString({ each: true })
+	otherCompanyIndustryExtended?: string[] | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	companySize?: string | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	companyType?: string | null;
+
+	@NoXss({ each: true })
+	@Expose()
+	@IsOptional()
+	@IsArray()
+	@IsString({ each: true })
+	automationGoalSm?: string[] | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	automationGoalSmOther?: string | null;
+
+	@NoXss({ each: true })
+	@Expose()
+	@IsOptional()
+	@IsArray()
+	@IsString({ each: true })
+	usageModes?: string[] | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsEmail()
+	email?: string | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	role?: string | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	roleOther?: string | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	reportedSource?: string | null;
+
+	@NoXss()
+	@Expose()
+	@IsOptional()
+	@IsString()
+	reportedSourceOther?: string | null;
+}

packages/cli/src/events/__tests__/telemetry-event-relay.test.ts

@@ -863,10 +863,10 @@ describe('TelemetryEventRelay', () => {
 			const event: RelayEventMap['user-submitted-personalization-survey'] = {
 				userId: 'user123',
 				answers: {
+					version: 'v4',
+					personalization_survey_n8n_version: '1.0.0',
+					personalization_survey_submitted_at: '2021-10-01T00:00:00.000Z',
 					companySize: '1-10',
-					workArea: 'IT',
-					automationGoal: 'Improve efficiency',
-					valueExpectation: 'Time savings',
 				},
 			};
 
@@ -874,10 +874,10 @@ describe('TelemetryEventRelay', () => {
 
 			expect(telemetry.track).toHaveBeenCalledWith('User responded to personalization questions', {
 				user_id: 'user123',
+				version: 'v4',
+				personalization_survey_n8n_version: '1.0.0',
+				personalization_survey_submitted_at: '2021-10-01T00:00:00.000Z',
 				company_size: '1-10',
-				work_area: 'IT',
-				automation_goal: 'Improve efficiency',
-				value_expectation: 'Time savings',
 			});
 		});
 

packages/cli/src/events/relay-event-map.ts

@@ -1,4 +1,9 @@
-import type { AuthenticationMethod, IRun, IWorkflowBase } from 'n8n-workflow';
+import type {
+	AuthenticationMethod,
+	IPersonalizationSurveyAnswersV4,
+	IRun,
+	IWorkflowBase,
+} from 'n8n-workflow';
 import type { IWorkflowDb, IWorkflowExecutionDataProcess } from '@/Interfaces';
 import type { ProjectRole } from '@/databases/entities/ProjectRelation';
 import type { GlobalRole } from '@/databases/entities/User';
@@ -106,7 +111,7 @@ export type RelayEventMap = {
 
 	'user-submitted-personalization-survey': {
 		userId: string;
-		answers: Record<string, string>;
+		answers: IPersonalizationSurveyAnswersV4;
 	};
 
 	'user-deleted': {

packages/cli/src/events/telemetry-event-relay.ts

@@ -945,11 +945,15 @@ export class TelemetryEventRelay extends EventRelay {
 		userId,
 		answers,
 	}: RelayEventMap['user-submitted-personalization-survey']) {
-		const camelCaseKeys = Object.keys(answers);
 		const personalizationSurveyData = { user_id: userId } as Record<string, string | string[]>;
-		camelCaseKeys.forEach((camelCaseKey) => {
-			personalizationSurveyData[snakeCase(camelCaseKey)] = answers[camelCaseKey];
-		});
+
+		// ESlint is wrong here
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+		for (const [camelCaseKey, value] of Object.entries(answers)) {
+			if (value) {
+				personalizationSurveyData[snakeCase(camelCaseKey)] = value;
+			}
+		}
 
 		this.telemetry.track('User responded to personalization questions', personalizationSurveyData);
 	}

packages/cli/src/requests.ts

@@ -8,6 +8,7 @@ import type {
 	INodeCredentials,
 	INodeParameters,
 	INodeTypeNameVersion,
+	IPersonalizationSurveyAnswersV4,
 	IUser,
 } from 'n8n-workflow';
 
@@ -235,7 +236,7 @@ export declare namespace MeRequest {
 		{},
 		{ currentPassword: string; newPassword: string; mfaCode?: string }
 	>;
-	export type SurveyAnswers = AuthenticatedRequest<{}, {}, Record<string, string> | {}>;
+	export type SurveyAnswers = AuthenticatedRequest<{}, {}, IPersonalizationSurveyAnswersV4>;
 }
 
 export interface UserSetupPayload {

packages/cli/src/validators/__tests__/no-xss.validator.test.ts

@@ -11,6 +11,9 @@ describe('NoXss', () => {
 
 		@NoXss()
 		version = '';
+
+		@NoXss({ each: true })
+		categories: string[] = [];
 	}
 
 	const entity = new Entity();
@@ -71,7 +74,7 @@ describe('NoXss', () => {
 		}
 	});
 
-	describe('Miscellanous strings', () => {
+	describe('Miscellaneous strings', () => {
 		const VALID_MISCELLANEOUS_STRINGS = ['CI/CD'];
 
 		for (const str of VALID_MISCELLANEOUS_STRINGS) {
@@ -81,4 +84,34 @@ describe('NoXss', () => {
 			});
 		}
 	});
+
+	describe('Array of strings', () => {
+		const VALID_STRING_ARRAYS = [
+			['cloud-infrastructure-orchestration', 'ci-cd', 'reporting'],
+			['automationGoalDevops', 'cloudComputing', 'containerization'],
+		];
+
+		for (const arr of VALID_STRING_ARRAYS) {
+			test(`should allow array: ${JSON.stringify(arr)}`, async () => {
+				entity.categories = arr;
+				await expect(validate(entity)).resolves.toBeEmptyArray();
+			});
+		}
+
+		const INVALID_STRING_ARRAYS = [
+			['valid-string', '<script>alert("xss")</script>', 'another-valid-string'],
+			['<img src="x" onerror="alert(\'XSS\')">', 'valid-string'],
+		];
+
+		for (const arr of INVALID_STRING_ARRAYS) {
+			test(`should reject array containing invalid string: ${JSON.stringify(arr)}`, async () => {
+				entity.categories = arr;
+				const errors = await validate(entity);
+				expect(errors).toHaveLength(1);
+				const [error] = errors;
+				expect(error.property).toEqual('categories');
+				expect(error.constraints).toEqual({ NoXss: 'Potentially malicious string' });
+			});
+		}
+	});
 });

packages/cli/src/validators/no-xss.validator.ts

@@ -4,7 +4,9 @@ import { registerDecorator, ValidatorConstraint } from 'class-validator';
 
 @ValidatorConstraint({ name: 'NoXss', async: false })
 class NoXssConstraint implements ValidatorConstraintInterface {
-	validate(value: string) {
+	validate(value: unknown) {
+		if (typeof value !== 'string') return false;
+
 		return (
 			value ===
 			xss(value, {