Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Fix XSS validation and separate URL validation #10424

Merged
merged 4 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/GenericHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 '@/validators/no-xss.validator';

export async function validateEntity(
entity:
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/src/databases/entities/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 '@/validators/no-xss.validator';
import { objectRetriever, lowerCaser } from '../utils/transformers';
import { WithTimestamps, jsonColumnType } from './AbstractEntity';
import type { IPersonalizationSurveyAnswers } from '@/Interfaces';
Expand All @@ -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<GlobalRole, 'global:owner'>;
Expand All @@ -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;
Expand Down

This file was deleted.

18 changes: 0 additions & 18 deletions packages/cli/src/databases/utils/customValidators.ts

This file was deleted.

5 changes: 4 additions & 1 deletion packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type {

import { Expose } from 'class-transformer';
import { IsBoolean, IsEmail, IsIn, IsOptional, IsString, Length } from 'class-validator';
import { NoXss } from '@db/utils/customValidators';
import { NoXss } from '@/validators/no-xss.validator';
import type { PublicUser, SecretsProvider, SecretsProviderState } from '@/Interfaces';
import { AssignableRole } from '@db/entities/User';
import type { GlobalRole, User } from '@db/entities/User';
Expand All @@ -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<User, 'email' | 'firstName' | 'lastName'> {
@Expose()
Expand All @@ -34,12 +35,14 @@ export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'la

@Expose()
@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;

@Expose()
@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;
Expand Down
26 changes: 26 additions & 0 deletions packages/cli/src/validators/__tests__/no-url.validator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { NoUrl } from '../no-url.validator';
import { validate } from 'class-validator';

describe('NoUrl', () => {
class Entity {
@NoUrl()
name = '';
}

const entity = new Entity();

describe('URLs', () => {
const URLS = ['http://google.com', 'www.domain.tld'];

for (const str of URLS) {
test(`should block ${str}`, async () => {
entity.name = str;
const errors = await validate(entity);
expect(errors).toHaveLength(1);
const [error] = errors;
expect(error.property).toEqual('name');
expect(error.constraints).toEqual({ NoUrl: 'Potentially malicious string' });
});
}
Comment on lines +15 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test.each is made for cases like this:

Suggested change
for (const str of URLS) {
test(`should block ${str}`, async () => {
entity.name = str;
const errors = await validate(entity);
expect(errors).toHaveLength(1);
const [error] = errors;
expect(error.property).toEqual('name');
expect(error.constraints).toEqual({ NoUrl: 'Potentially malicious string' });
});
}
test.each(['http://google.com', 'www.domain.tld'])(
'should block %s', async (str) => {
entity.name = str;
const errors = await validate(entity);
expect(errors).toHaveLength(1);
const [error] = errors;
expect(error.property).toEqual('name');
expect(error.constraints).toEqual({ NoUrl: 'Potentially malicious string' });
}
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've always found a simple loop more readable and easier to remember, for test.each I always have to look up the syntax 😓 Is test.each faster or better in some other way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. Personally I find test.each more readable :D There are subtle differences also, like mentioned in this SO comment

});
});
72 changes: 72 additions & 0 deletions packages/cli/src/validators/__tests__/no-xss.validator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { NoXss } from '../no-xss.validator';
import { validate } from 'class-validator';

describe('NoXss', () => {
class Entity {
@NoXss()
name = '';

@NoXss()
timestamp = '';

@NoXss()
version = '';
}

const entity = new Entity();

describe('Scripts', () => {
const XSS_STRINGS = ['<script src/>', "<script>alert('xss')</script>"];

for (const str of XSS_STRINGS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in this test suite

test(`should block ${str}`, async () => {
entity.name = str;
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' });
});
}
});

describe('Names', () => {
const VALID_NAMES = [
'Johann Strauß',
'Вагиф Сәмәдоғлу',
'René Magritte',
'সুকুমার রায়',
'མགོན་པོ་རྡོ་རྗེ།',
'عبدالحليم حافظ',
];

for (const name of VALID_NAMES) {
test(`should allow ${name}`, async () => {
entity.name = name;
expect(await validate(entity)).toBeEmptyArray();
});
}
});

describe('ISO-8601 timestamps', () => {
const VALID_TIMESTAMPS = ['2022-01-01T00:00:00.000Z', '2022-01-01T00:00:00.000+02:00'];

for (const timestamp of VALID_TIMESTAMPS) {
test(`should allow ${timestamp}`, async () => {
entity.timestamp = timestamp;
await expect(validate(entity)).resolves.toBeEmptyArray();
});
}
});

describe('Semver versions', () => {
const VALID_VERSIONS = ['1.0.0', '1.0.0-alpha.1'];

for (const version of VALID_VERSIONS) {
test(`should allow ${version}`, async () => {
entity.version = version;
await expect(validate(entity)).resolves.toBeEmptyArray();
});
}
});
});
27 changes: 27 additions & 0 deletions packages/cli/src/validators/no-url.validator.ts
Original file line number Diff line number Diff line change
@@ -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,
});
};
}
26 changes: 26 additions & 0 deletions packages/cli/src/validators/no-xss.validator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import type { ValidationOptions, ValidatorConstraintInterface } from 'class-validator';
import { registerDecorator, ValidatorConstraint } from 'class-validator';
import sanitizeHtml from 'sanitize-html';

@ValidatorConstraint({ name: 'NoXss', async: false })
class NoXssConstraint implements ValidatorConstraintInterface {
validate(value: string) {
return value === sanitizeHtml(value, { allowedTags: [], allowedAttributes: {} });
}

defaultMessage() {
return 'Potentially malicious string';
}
}

export function NoXss(options?: ValidationOptions) {
return function (object: object, propertyName: string) {
registerDecorator({
name: 'NoXss',
target: object.constructor,
propertyName,
options,
validator: NoXssConstraint,
});
};
}
5 changes: 4 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading