diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts index 57dcb60207..39bdc4c9c1 100644 --- 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 { 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) { - 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 index 71a6d69384..9363cefd0a 100644 --- 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({ - body: { 'test-answer': '' }, - }); + test.each([ + 'automationGoalDevops', + 'companyIndustryExtended', + 'otherCompanyIndustryExtended', + 'automationGoalSm', + 'usageModes', + ])('should throw BadRequestError on XSS attempt for an array field %s', async (fieldName) => { + const req = mock(); + req.body = { + version: 'v4', + personalization_survey_n8n_version: '1.0.0', + personalization_survey_submitted_at: new Date().toISOString(), + [fieldName]: [''], + }; + + 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(); + req.body = { + version: 'v4', + personalization_survey_n8n_version: '1.0.0', + personalization_survey_submitted_at: new Date().toISOString(), + [fieldName]: '', + }; 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 index 19429228ed..f931feecef 100644 --- 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,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 }; diff --git a/packages/cli/src/controllers/survey-answers.dto.ts b/packages/cli/src/controllers/survey-answers.dto.ts new file mode 100644 index 0000000000..f115a6992b --- /dev/null +++ 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 index 56149a1e9e..8e4fb444c6 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/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', }); }); diff --git a/packages/cli/src/events/relay-event-map.ts b/packages/cli/src/events/relay-event-map.ts index 8f0efc2fed..ffc9c9d716 100644 --- 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; + answers: IPersonalizationSurveyAnswersV4; }; 'user-deleted': { diff --git a/packages/cli/src/events/telemetry-event-relay.ts b/packages/cli/src/events/telemetry-event-relay.ts index d09481f911..3389e24b3f 100644 --- 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; - 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 index a847281d7d..bae7990c1a 100644 --- 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 | {}>; + export type SurveyAnswers = AuthenticatedRequest<{}, {}, IPersonalizationSurveyAnswersV4>; } export interface UserSetupPayload { diff --git a/packages/cli/src/validators/__tests__/no-xss.validator.test.ts b/packages/cli/src/validators/__tests__/no-xss.validator.test.ts index 33821787ec..75eaeb9072 100644 --- a/packages/cli/src/validators/__tests__/no-xss.validator.test.ts +++ b/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', '', 'another-valid-string'], + ['', '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' }); + }); + } + }); }); diff --git a/packages/cli/src/validators/no-xss.validator.ts b/packages/cli/src/validators/no-xss.validator.ts index 7c65f02dfe..69960c39dd 100644 --- a/packages/cli/src/validators/no-xss.validator.ts +++ b/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, { diff --git a/packages/cli/test/integration/me.api.test.ts b/packages/cli/test/integration/me.api.test.ts index ef9757dc44..b644d61550 100644 --- a/packages/cli/test/integration/me.api.test.ts +++ b/packages/cli/test/integration/me.api.test.ts @@ -1,7 +1,6 @@ import { Container } from 'typedi'; import { IsNull } from '@n8n/typeorm'; import validator from 'validator'; -import { randomString } from 'n8n-workflow'; import type { User } from '@db/entities/User'; import { UserRepository } from '@db/repositories/user.repository'; @@ -15,6 +14,7 @@ import { addApiKey, createOwner, createUser, createUserShell } from './shared/db import type { SuperAgentTest } from './shared/types'; import { mockInstance } from '@test/mocking'; import { GlobalConfig } from '@n8n/config'; +import type { IPersonalizationSurveyAnswersV4 } from 'n8n-workflow'; const testServer = utils.setupTestServer({ endpointGroups: ['me'] }); @@ -145,16 +145,16 @@ describe('Owner shell', () => { }); test('POST /me/survey should succeed with valid inputs', async () => { - const validPayloads = [SURVEY, {}]; + const validPayloads = [SURVEY, EMPTY_SURVEY]; for (const validPayload of validPayloads) { const response = await authOwnerShellAgent.post('/me/survey').send(validPayload); - expect(response.statusCode).toBe(200); expect(response.body).toEqual(SUCCESS_RESPONSE_BODY); + expect(response.statusCode).toBe(200); const storedShellOwner = await Container.get(UserRepository).findOneOrFail({ - where: { email: IsNull() }, + where: { id: ownerShell.id }, }); expect(storedShellOwner.personalizationAnswers).toEqual(validPayload); @@ -300,7 +300,7 @@ describe('Member', () => { }); test('POST /me/survey should succeed with valid inputs', async () => { - const validPayloads = [SURVEY, {}]; + const validPayloads = [SURVEY, EMPTY_SURVEY]; for (const validPayload of validPayloads) { const response = await authMemberAgent.post('/me/survey').send(validPayload); @@ -392,16 +392,31 @@ describe('Owner', () => { }); }); -const SURVEY = [ - 'codingSkill', - 'companyIndustry', - 'companySize', - 'otherCompanyIndustry', - 'otherWorkArea', - 'workArea', -].reduce>((acc, cur) => { - return (acc[cur] = randomString(2, 10)), acc; -}, {}); +const SURVEY: IPersonalizationSurveyAnswersV4 = { + version: 'v4', + personalization_survey_submitted_at: '2024-08-21T13:05:51.709Z', + personalization_survey_n8n_version: '1.0.0', + automationGoalDevops: ['test'], + automationGoalDevopsOther: 'test', + companyIndustryExtended: ['test'], + otherCompanyIndustryExtended: ['test'], + companySize: 'test', + companyType: 'test', + automationGoalSm: ['test'], + automationGoalSmOther: 'test', + usageModes: ['test'], + email: 'test@email.com', + role: 'test', + roleOther: 'test', + reportedSource: 'test', + reportedSourceOther: 'test', +}; + +const EMPTY_SURVEY: IPersonalizationSurveyAnswersV4 = { + version: 'v4', + personalization_survey_submitted_at: '2024-08-21T13:05:51.709Z', + personalization_survey_n8n_version: '1.0.0', +}; const VALID_PATCH_ME_PAYLOADS = [ { diff --git a/packages/editor-ui/src/Interface.ts b/packages/editor-ui/src/Interface.ts index 05c40402f1..a3f17a5582 100644 --- a/packages/editor-ui/src/Interface.ts +++ b/packages/editor-ui/src/Interface.ts @@ -48,6 +48,7 @@ import type { NodeConnectionType, INodeCredentialsDetails, StartNodeData, + IPersonalizationSurveyAnswersV4, } from 'n8n-workflow'; import type { BulkCommand, Undoable } from '@/models/history'; import type { PartialBy, TupleToUnion } from '@/utils/typeHelpers'; @@ -648,24 +649,6 @@ export type IPersonalizationSurveyAnswersV3 = { email?: string | null; }; -export type IPersonalizationSurveyAnswersV4 = { - version: 'v4'; - automationGoalDevops?: string[] | null; - automationGoalDevopsOther?: string | null; - companyIndustryExtended?: string[] | null; - otherCompanyIndustryExtended?: string[] | null; - companySize?: string | null; - companyType?: string | null; - automationGoalSm?: string[] | null; - automationGoalSmOther?: string | null; - usageModes?: string[] | null; - email?: string | null; - role?: string | null; - roleOther?: string | null; - reportedSource?: string | null; - reportedSourceOther?: string | null; -}; - export type IPersonalizationLatestVersion = IPersonalizationSurveyAnswersV4; export type IPersonalizationSurveyVersions = diff --git a/packages/editor-ui/src/components/PersonalizationModal.vue b/packages/editor-ui/src/components/PersonalizationModal.vue index 3f74e06f4e..cd4693565e 100644 --- a/packages/editor-ui/src/components/PersonalizationModal.vue +++ b/packages/editor-ui/src/components/PersonalizationModal.vue @@ -140,7 +140,6 @@ import { import { useToast } from '@/composables/useToast'; import Modal from '@/components/Modal.vue'; import type { IFormInputs, IPersonalizationLatestVersion } from '@/Interface'; -import type { GenericValue } from 'n8n-workflow'; import { useUIStore } from '@/stores/ui.store'; import { useSettingsStore } from '@/stores/settings.store'; import { useRootStore } from '@/stores/root.store'; @@ -696,19 +695,16 @@ export default defineComponent({ this.isSaving = true; try { - const survey: Record = { + const survey: IPersonalizationLatestVersion = { ...values, version: SURVEY_VERSION, personalization_survey_submitted_at: new Date().toISOString(), personalization_survey_n8n_version: this.rootStore.versionCli, }; - await this.externalHooks.run( - 'personalizationModal.onSubmit', - survey as IPersonalizationLatestVersion, - ); + await this.externalHooks.run('personalizationModal.onSubmit', survey); - await this.usersStore.submitPersonalizationSurvey(survey as IPersonalizationLatestVersion); + await this.usersStore.submitPersonalizationSurvey(survey); this.posthogStore.setMetadata(survey, 'user'); diff --git a/packages/editor-ui/src/utils/userUtils.ts b/packages/editor-ui/src/utils/userUtils.ts index 3d8aa21637..ff862ecfd8 100644 --- a/packages/editor-ui/src/utils/userUtils.ts +++ b/packages/editor-ui/src/utils/userUtils.ts @@ -65,11 +65,11 @@ import type { IPersonalizationSurveyAnswersV1, IPersonalizationSurveyAnswersV2, IPersonalizationSurveyAnswersV3, - IPersonalizationSurveyAnswersV4, IPersonalizationSurveyVersions, IUser, ILogInStatus, } from '@/Interface'; +import type { IPersonalizationSurveyAnswersV4 } from 'n8n-workflow'; /* Utility functions used to handle users in n8n diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 3faa825f7b..a66dcc5eb4 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -2789,3 +2789,23 @@ export type Functionality = 'regular' | 'configuration-node' | 'pairedItem'; export type Result = { ok: true; result: T } | { ok: false; error: E }; export type CallbackManager = CallbackManagerLC; + +export type IPersonalizationSurveyAnswersV4 = { + version: 'v4'; + personalization_survey_submitted_at: string; + personalization_survey_n8n_version: string; + automationGoalDevops?: string[] | null; + automationGoalDevopsOther?: string | null; + companyIndustryExtended?: string[] | null; + otherCompanyIndustryExtended?: string[] | null; + companySize?: string | null; + companyType?: string | null; + automationGoalSm?: string[] | null; + automationGoalSmOther?: string | null; + usageModes?: string[] | null; + email?: string | null; + role?: string | null; + roleOther?: string | null; + reportedSource?: string | null; + reportedSourceOther?: string | null; +};