From cfb719e321a8fce090e65941e4710b95442e6881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 15 Aug 2024 10:00:22 +0200 Subject: [PATCH 1/3] refactor(core): Improve XSS validator --- packages/cli/package.json | 1 + packages/cli/src/GenericHelpers.ts | 2 +- packages/cli/src/databases/entities/User.ts | 2 +- .../utils/__tests__/customValidators.test.ts | 43 ------------ .../utils/__tests__/no-xss.validator.test.ts | 70 +++++++++++++++++++ .../src/databases/utils/customValidators.ts | 18 ----- .../src/databases/utils/no-xss.validator.ts | 32 +++++++++ packages/cli/src/requests.ts | 2 +- pnpm-lock.yaml | 21 +++--- 9 files changed, 118 insertions(+), 73 deletions(-) delete mode 100644 packages/cli/src/databases/utils/__tests__/customValidators.test.ts create mode 100644 packages/cli/src/databases/utils/__tests__/no-xss.validator.test.ts delete mode 100644 packages/cli/src/databases/utils/customValidators.ts create mode 100644 packages/cli/src/databases/utils/no-xss.validator.ts diff --git a/packages/cli/package.json b/packages/cli/package.json index 8387f0ba41521..f12118ea91a9e 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -155,6 +155,7 @@ "reflect-metadata": "0.2.2", "replacestream": "4.0.3", "samlify": "2.8.9", + "sanitize-html": "2.12.1", "semver": "7.5.4", "shelljs": "0.8.5", "simple-git": "3.17.0", diff --git a/packages/cli/src/GenericHelpers.ts b/packages/cli/src/GenericHelpers.ts index 300f24f9f9355..00ecf3e0eb6b8 100644 --- a/packages/cli/src/GenericHelpers.ts +++ b/packages/cli/src/GenericHelpers.ts @@ -9,7 +9,7 @@ import type { UserUpdatePayload, } from '@/requests'; import { BadRequestError } from './errors/response-errors/bad-request.error'; -import { NoXss } from './databases/utils/customValidators'; +import { NoXss } from '@/databases/utils/no-xss.validator'; export async function validateEntity( entity: diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index d24af19742e6a..a27afb60efba4 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -13,7 +13,7 @@ import { IsEmail, IsString, Length } from 'class-validator'; import type { IUser, IUserSettings } from 'n8n-workflow'; import type { SharedWorkflow } from './SharedWorkflow'; import type { SharedCredentials } from './SharedCredentials'; -import { NoXss } from '../utils/customValidators'; +import { NoXss } from '@db/utils/no-xss.validator'; import { objectRetriever, lowerCaser } from '../utils/transformers'; import { WithTimestamps, jsonColumnType } from './AbstractEntity'; import type { IPersonalizationSurveyAnswers } from '@/Interfaces'; diff --git a/packages/cli/src/databases/utils/__tests__/customValidators.test.ts b/packages/cli/src/databases/utils/__tests__/customValidators.test.ts deleted file mode 100644 index df906b36d6a52..0000000000000 --- a/packages/cli/src/databases/utils/__tests__/customValidators.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { NoXss } from '@db/utils/customValidators'; -import { validate } from 'class-validator'; - -describe('customValidators', () => { - describe('NoXss', () => { - class Person { - @NoXss() - name: string; - } - const person = new Person(); - - const invalidNames = ['http://google.com', '"]; - for (const str of MALICIOUS_STRINGS) { + for (const str of XSS_STRINGS) { test(`should block ${str}`, async () => { entity.name = str; - const [error] = await validate(entity); + const errors = await validate(entity); + expect(errors).toHaveLength(1); + const [error] = errors; expect(error.property).toEqual('name'); expect(error.constraints).toEqual({ NoXss: 'Potentially malicious string' }); }); diff --git a/packages/cli/src/validators/no-url.validator.ts b/packages/cli/src/validators/no-url.validator.ts new file mode 100644 index 0000000000000..1df05fed5fa0d --- /dev/null +++ b/packages/cli/src/validators/no-url.validator.ts @@ -0,0 +1,27 @@ +import type { ValidationOptions, ValidatorConstraintInterface } from 'class-validator'; +import { registerDecorator, ValidatorConstraint } from 'class-validator'; + +const URL_REGEX = /^(https?:\/\/|www\.)/i; + +@ValidatorConstraint({ name: 'NoUrl', async: false }) +class NoUrlConstraint implements ValidatorConstraintInterface { + validate(value: string) { + return !URL_REGEX.test(value); + } + + defaultMessage() { + return 'Potentially malicious string'; + } +} + +export function NoUrl(options?: ValidationOptions) { + return function (object: object, propertyName: string) { + registerDecorator({ + name: 'NoUrl', + target: object.constructor, + propertyName, + options, + validator: NoUrlConstraint, + }); + }; +} diff --git a/packages/cli/src/databases/utils/no-xss.validator.ts b/packages/cli/src/validators/no-xss.validator.ts similarity index 76% rename from packages/cli/src/databases/utils/no-xss.validator.ts rename to packages/cli/src/validators/no-xss.validator.ts index ce71ebbc18144..8075309df9923 100644 --- a/packages/cli/src/databases/utils/no-xss.validator.ts +++ b/packages/cli/src/validators/no-xss.validator.ts @@ -2,16 +2,10 @@ import type { ValidationOptions, ValidatorConstraintInterface } from 'class-vali import { registerDecorator, ValidatorConstraint } from 'class-validator'; import sanitizeHtml from 'sanitize-html'; -const URL_REGEX = /^(https?:\/\/|www\.)/i; - @ValidatorConstraint({ name: 'NoXss', async: false }) class NoXssConstraint implements ValidatorConstraintInterface { validate(value: string) { - const sanitized = sanitizeHtml(value, { allowedTags: [], allowedAttributes: {} }); - - if (sanitized !== value) return false; - - return !URL_REGEX.test(value); + return value === sanitizeHtml(value, { allowedTags: [], allowedAttributes: {} }); } defaultMessage() { From 2b5bac0271f09507a30178aca952f5adefabc0c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 16 Aug 2024 10:01:03 +0200 Subject: [PATCH 3/3] Apply new decorator --- packages/cli/src/databases/entities/User.ts | 3 +++ packages/cli/src/requests.ts | 3 +++ 2 files changed, 6 insertions(+) diff --git a/packages/cli/src/databases/entities/User.ts b/packages/cli/src/databases/entities/User.ts index d79328208ae87..dad8bbbe8000c 100644 --- a/packages/cli/src/databases/entities/User.ts +++ b/packages/cli/src/databases/entities/User.ts @@ -25,6 +25,7 @@ import { } from '@/permissions/global-roles'; import { hasScope, type ScopeOptions, type Scope } from '@n8n/permissions'; import type { ProjectRelation } from './ProjectRelation'; +import { NoUrl } from '@/validators/no-url.validator'; export type GlobalRole = 'global:owner' | 'global:admin' | 'global:member'; export type AssignableRole = Exclude; @@ -51,12 +52,14 @@ export class User extends WithTimestamps implements IUser { @Column({ length: 32, nullable: true }) @NoXss() + @NoUrl() @IsString({ message: 'First name must be of type string.' }) @Length(1, 32, { message: 'First name must be $constraint1 to $constraint2 characters long.' }) firstName: string; @Column({ length: 32, nullable: true }) @NoXss() + @NoUrl() @IsString({ message: 'Last name must be of type string.' }) @Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' }) lastName: string; diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 8c28775a3aed7..a847281d7d760 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -26,6 +26,7 @@ import type { ProjectRole } from './databases/entities/ProjectRelation'; import type { Scope } from '@n8n/permissions'; import type { ScopesField } from './services/role.service'; import type { AiAssistantSDK } from '@n8n_io/ai-assistant-sdk'; +import { NoUrl } from '@/validators/no-url.validator'; export class UserUpdatePayload implements Pick { @Expose() @@ -34,12 +35,14 @@ export class UserUpdatePayload implements Pick