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

ivov · 2024-08-15T08:01:38Z

tomi · 2024-08-15T08:39:10Z

packages/cli/src/databases/utils/no-xss.validator.ts

+
+		if (sanitized !== value) return false;
+
+		return !URL_REGEX.test(value);


I feel like this should be a separate validator. Having a URL is not a XSS. I guess for backwards compat we could then include that validator also in places that are currently using the NoXss, if they need that

Agree! Not sure why it was ever included.

tomi · 2024-08-15T08:44:30Z

packages/cli/src/databases/utils/no-xss.validator.ts

@@ -0,0 +1,32 @@
+import type { ValidationOptions, ValidatorConstraintInterface } from 'class-validator';


Should this file be somewhere else than under @/databases?

Agree, not sure why but it has been like this since forever. Changed.

tomi · 2024-08-16T06:21:57Z

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

+	const entity = new Entity();
+
+	describe('Scripts and URLs', () => {
+		const MALICIOUS_STRINGS = ['http://google.com', '<script src/>', 'www.domain.tld'];


Maybe also <script>alert('xss')</script>?

tomi

Nice, thank you for addressing 👏 The comment about test.each can also be done as a follow up

tomi · 2024-08-16T08:22:52Z

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

+		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 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' });

}

)

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?

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

tomi · 2024-08-16T08:23:14Z

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

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


Same in this test suite

github-actions · 2024-08-16T08:35:43Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2024-08-16T08:35:52Z

Test summary

337 • 1 • 0 • 0 • 0

Run details


Project	n8n
Status	Failed
Commit	`2b5bac0`
Started	Aug 16, 2024 8:31 AM
Ended	Aug 16, 2024 8:35 AM
Duration	04:46 💡
OS	Linux Debian -
Browser	Electron 118

View run in Cypress Cloud ➡️

Failures

	e2e/14-mapping.cy.ts		1
1	Data mapping > maps expressions from table header

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

mutdmour · 2024-08-16T12:10:02Z

This seems to be causing e2e tests to fail here 9bfc074

ivov · 2024-08-16T12:18:06Z

22606e6

* master: fix(editor): Add workflow scopes when initializing workflow (#10455) feat(editor): Improve node label readability in new canvas (no-changelog) (#10432) fix(editor): Fix lazy loaded component not using suspense (no-changelog) (#10454) fix(editor): Buffer json chunks in stream response (#10439) refactor(editor): Remove `id` param from PATCH /me calls (no-changelog) (#10449) fix(core): Fix XSS validation and separate URL validation (#10424) fix(Respond to Webhook Node): Fix issue preventing the chat trigger from working (#9886) feat(editor): Add `registerCustomAction` to new canvas (no-changelog) (#10359)

janober · 2024-08-16T15:02:48Z

Got released with n8n@1.55.3

# [1.56.0](https://github.com/netroy/n8n/compare/n8n@1.55.0...n8n@1.56.0) (2024-08-20) ### Bug Fixes * Better errors in Switch, If and Filter nodes ([n8n-io#10457](https://github.com/netroy/n8n/issues/10457)) ([aea82cb](aea82cb)) * **core:** Fix payload property in `workflow-post-execute` event ([n8n-io#10413](https://github.com/netroy/n8n/issues/10413)) ([d98e29e](d98e29e)) * **core:** Fix XSS validation and separate URL validation ([n8n-io#10424](https://github.com/netroy/n8n/issues/10424)) ([91467ab](91467ab)) * **core:** Use explicit types in configs to ensure valid decorator metadata ([n8n-io#10433](https://github.com/netroy/n8n/issues/10433)) ([2043daa](2043daa)) * **editor:** Add workflow scopes when initializing workflow ([n8n-io#10455](https://github.com/netroy/n8n/issues/10455)) ([b857c2c](b857c2c)) * **editor:** Buffer json chunks in stream response ([n8n-io#10439](https://github.com/netroy/n8n/issues/10439)) ([37797f3](37797f3)) * **editor:** Fix flaky mapping tests ([n8n-io#10453](https://github.com/netroy/n8n/issues/10453)) ([fc6d413](fc6d413)) * **editor:** Highlight matching type in filter component ([n8n-io#10425](https://github.com/netroy/n8n/issues/10425)) ([6bca879](6bca879)) * **editor:** Show item count in output panel schema view ([n8n-io#10426](https://github.com/netroy/n8n/issues/10426)) ([4dee7cc](4dee7cc)) * **editor:** Truncate long data pill labels in schema view ([n8n-io#10427](https://github.com/netroy/n8n/issues/10427)) ([1bf2f4f](1bf2f4f)) * Filter component - improve errors ([n8n-io#10456](https://github.com/netroy/n8n/issues/10456)) ([61ac0c7](61ac0c7)) * **Google Sheets Node:** Better error when column to match on is empty ([n8n-io#10442](https://github.com/netroy/n8n/issues/10442)) ([ce46bf5](ce46bf5)) * **Google Sheets Node:** Update name and hint for useAppend option ([n8n-io#10443](https://github.com/netroy/n8n/issues/10443)) ([c5a0c04](c5a0c04)) * **Google Sheets Node:** Update to returnAllMatches option ([n8n-io#10440](https://github.com/netroy/n8n/issues/10440)) ([f7fb02e](f7fb02e)) * **Invoice Ninja Node:** Fix payment types ([n8n-io#10462](https://github.com/netroy/n8n/issues/10462)) ([129245d](129245d)) * **n8n Form Trigger Node:** Show basic authentication modal on wrong credentials ([n8n-io#10423](https://github.com/netroy/n8n/issues/10423)) ([0dc3e99](0dc3e99)) * **OpenAI Node:** Throw node operations error in case of openAi client error ([n8n-io#10448](https://github.com/netroy/n8n/issues/10448)) ([0d3ed46](0d3ed46)) * Project Viewer always seeing a connection error when testing credentials ([n8n-io#10417](https://github.com/netroy/n8n/issues/10417)) ([613cdd2](613cdd2)) * Remove unimplemented Postgres credentials options ([n8n-io#10461](https://github.com/netroy/n8n/issues/10461)) ([17ac784](17ac784)) * Require mfa code to change email ([n8n-io#10354](https://github.com/netroy/n8n/issues/10354)) ([39c8e50](39c8e50)) * **Respond to Webhook Node:** Fix issue preventing the chat trigger from working ([n8n-io#9886](https://github.com/netroy/n8n/issues/9886)) ([9d6ad88](9d6ad88)) * Show input names when node has multiple inputs ([n8n-io#10434](https://github.com/netroy/n8n/issues/10434)) ([973956c](973956c)) ### Features * **core:** Support bidirectional communication between specific mains and specific workers ([n8n-io#10377](https://github.com/netroy/n8n/issues/10377)) ([d0fc9de](d0fc9de)) * **Facebook Graph API Node:** Update node to support API v18 - v20 ([n8n-io#10419](https://github.com/netroy/n8n/issues/10419)) ([e7ee10f](e7ee10f)) Co-authored-by: netroy <196144+netroy@users.noreply.github.com>

refactor(core): Improve XSS validator

cfb719e

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Aug 15, 2024

tomi reviewed Aug 16, 2024

View reviewed changes

ivov added 3 commits August 16, 2024 09:41

Merge branch 'master' into pay-1845-improve-xss-validator

55acdc4

Extract URL validation from NoXss validator

ba5645b

Apply new decorator

2b5bac0

ivov changed the title ~~refactor(core): Improve XSS validator~~ fix(core): Fix XSS validation and separate URL validation Aug 16, 2024

ivov added the release/backport Changes that need to be backported to older releases. label Aug 16, 2024

ivov requested a review from tomi August 16, 2024 08:13

tomi approved these changes Aug 16, 2024

View reviewed changes

ivov merged commit 91467ab into master Aug 16, 2024
38 of 43 checks passed

ivov deleted the pay-1845-improve-xss-validator branch August 16, 2024 08:49

ivov added a commit that referenced this pull request Aug 16, 2024

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

4833fcb

github-actions bot mentioned this pull request Aug 16, 2024

🚀 Release 1.55.3 #10460

Merged

janober added the Released label Aug 16, 2024

github-actions bot mentioned this pull request Aug 21, 2024

🚀 Release 1.56.0 #10502

Merged

netroy mentioned this pull request Aug 21, 2024

🚀 Release 1.56.0 netroy/n8n#64

Closed

riascho pushed a commit that referenced this pull request Aug 26, 2024

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

9203e85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ivov commented Aug 15, 2024

tomi Aug 15, 2024

ivov Aug 16, 2024

tomi Aug 15, 2024

ivov Aug 16, 2024

tomi Aug 16, 2024

tomi left a comment

tomi Aug 16, 2024

ivov Aug 16, 2024

tomi Aug 16, 2024

tomi Aug 16, 2024

github-actions bot commented Aug 16, 2024

cypress bot commented Aug 16, 2024

mutdmour commented Aug 16, 2024

ivov commented Aug 16, 2024

janober commented Aug 16, 2024


		if (sanitized !== value) return false;

		return !URL_REGEX.test(value);

		@@ -0,0 +1,32 @@
		import type { ValidationOptions, ValidatorConstraintInterface } from 'class-validator';

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

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

Conversation

ivov commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2024

cypress bot commented Aug 16, 2024

Test summary

Run details

Failures

mutdmour commented Aug 16, 2024

ivov commented Aug 16, 2024

janober commented Aug 16, 2024