fix(core): Use class-validator with XSS check for survey answers (n8n…

…-io#10490) Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
netroy · Aug 21, 2024 · 547a606 · 547a606
1 parent d5acde5
commit 547a606
Show file tree

Hide file tree

Showing 15 changed files with 274 additions and 102 deletions.
diff --git a/packages/cli/src/GenericHelpers.ts b/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);
-	}
-}
diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/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);
 		});

diff --git a/packages/cli/src/controllers/me.controller.ts b/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,20 +196,26 @@ 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,
 				},
 			);
 			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 };

diff --git a/packages/cli/src/controllers/survey-answers.dto.ts b/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;
+}
diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts
@@ -863,21 +863,21 @@ 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',
 				},
 			};
 
 			eventService.emit('user-submitted-personalization-survey', event);
 
 			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',
 			});
 		});
 

diff --git a/packages/cli/src/events/relay-event-map.ts b/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': {

diff --git a/packages/cli/src/events/telemetry-event-relay.ts b/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);
 	}

diff --git a/packages/cli/src/requests.ts b/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 {