From 980b54f62bb1d23cc61e0a0b2e3f0a105167ccef Mon Sep 17 00:00:00 2001 From: Christofer Roth Date: Wed, 15 May 2024 16:00:46 +0200 Subject: [PATCH 1/2] fix(extension-link): use whitelist for allowed href values --- packages/extension-link/src/link.ts | 14 +- .../integration/extensions/link.spec.ts | 213 +++++++++++++++--- 2 files changed, 189 insertions(+), 38 deletions(-) diff --git a/packages/extension-link/src/link.ts b/packages/extension-link/src/link.ts index e3af421de65..1492a7182db 100644 --- a/packages/extension-link/src/link.ts +++ b/packages/extension-link/src/link.ts @@ -95,6 +95,15 @@ declare module '@tiptap/core' { } } +// From DOMPurify +// https://github.com/cure53/DOMPurify/blob/main/src/regexp.js +const ATTR_WHITESPACE = /[\u0000-\u0020\u00A0\u1680\u180E\u2000-\u2029\u205F\u3000]/g // eslint-disable-line no-control-regex +const IS_ALLOWED_URI = /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|sms|cid|xmpp):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i // eslint-disable-line no-useless-escape + +function isAllowedUri(uri: string | undefined) { + return !uri || uri.replace(ATTR_WHITESPACE, '').match(IS_ALLOWED_URI) +} + /** * This extension allows you to create links. * @see https://www.tiptap.dev/api/marks/link @@ -161,12 +170,11 @@ export const Link = Mark.create({ }, renderHTML({ HTMLAttributes }) { - // False positive; we're explicitly checking for javascript: links to ignore them - // eslint-disable-next-line no-script-url - if (HTMLAttributes.href?.startsWith('javascript:')) { + if (!isAllowedUri(HTMLAttributes.href)) { // strip out the href return ['a', mergeAttributes(this.options.HTMLAttributes, { ...HTMLAttributes, href: '' }), 0] } + return ['a', mergeAttributes(this.options.HTMLAttributes, HTMLAttributes), 0] }, diff --git a/tests/cypress/integration/extensions/link.spec.ts b/tests/cypress/integration/extensions/link.spec.ts index edb729093d5..ee1354a54ae 100644 --- a/tests/cypress/integration/extensions/link.spec.ts +++ b/tests/cypress/integration/extensions/link.spec.ts @@ -20,45 +20,188 @@ describe('extension-link', () => { } const getEditorEl = () => document.querySelector(`.${editorElClass}`) - it('does not output src tag for javascript schema', () => { - editor = new Editor({ - element: createEditorEl(), - extensions: [ - Document, - Text, - Paragraph, - Link, - ], - content: { - type: 'doc', - content: [ - { - type: 'paragraph', - content: [ - { - type: 'text', - text: 'hello world!', - marks: [ - { - type: 'link', - attrs: { - // We have to disable the eslint rule here because we're trying to purposely test eval urls - // eslint-disable-next-line no-script-url - href: 'javascript:alert(window.origin)', - }, - }, - ], - }, - ], - }, + const validUrls = [ + 'https://example.com', + 'http://example.com', + '/same-site/index.html', + '../relative.html', + 'mailto:info@example.com', + 'ftp://info@example.com', + ] + + validUrls.forEach(url => { + it('does output href tag for valid schemas', () => { + editor = new Editor({ + element: createEditorEl(), + extensions: [ + Document, + Text, + Paragraph, + Link, ], - }, + content: { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'hello world!', + marks: [ + { + type: 'link', + attrs: { + href: url, + }, + }, + ], + }, + ], + }, + ], + }, + }) + + expect(editor.getHTML()).to.include(url) + + editor?.destroy() + getEditorEl()?.remove() }) + }) + + // We have to disable the eslint rule here because we're trying to purposely test eval urls + // Examples inspired by: https://portswigger.net/web-security/cross-site-scripting/cheat-sheet#protocols + const invalidUrls = [ + // A standard JavaScript protocol + // eslint-disable-next-line no-script-url + 'javascript:alert(window.origin)', + + // The protocol is not case sensitive + // eslint-disable-next-line no-script-url + 'jAvAsCrIpT:alert(window.origin)', + + // Characters \x01-\x20 are allowed before the protocol + // eslint-disable-next-line no-script-url + '\x00javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x01javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x02javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x03javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x04javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x05javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x06javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x07javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x08javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x09javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0ajavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0bjavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0cjavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0djavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0ejavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x0fjavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x10javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x11javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x12javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x13javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x14javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x15javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x16javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x17javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x18javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x19javascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1ajavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1bjavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1cjavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1djavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1ejavascript:alert(window.origin)', + // eslint-disable-next-line no-script-url + '\x1fjavascript:alert(window.origin)', + // Characters \x09,\x0a,\x0d are allowed inside the protocol + // eslint-disable-next-line no-script-url + 'java\x09script:alert(window.origin)', + // eslint-disable-next-line no-script-url + 'java\x0ascript:alert(window.origin)', // eslint-disable-next-line no-script-url - expect(editor.getHTML()).to.not.include('javascript:alert(window.origin)') + 'java\x0dscript:alert(window.origin)', - editor?.destroy() - getEditorEl()?.remove() + // Characters \x09,\x0a,\x0d are allowed after protocol name before the colon + // eslint-disable-next-line no-script-url + 'javascript\x09:alert(window.origin)', + // eslint-disable-next-line no-script-url + 'javascript\x0a:alert(window.origin)', + // eslint-disable-next-line no-script-url + 'javascript\x0d:alert(window.origin)', + ] + + invalidUrls.forEach(url => { + it('does not output src tag for javascript schema', () => { + editor = new Editor({ + element: createEditorEl(), + extensions: [ + Document, + Text, + Paragraph, + Link, + ], + content: { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'hello world!', + marks: [ + { + type: 'link', + attrs: { + href: url, + }, + }, + ], + }, + ], + }, + ], + }, + }) + + expect(editor.getHTML()).to.not.include(url) + + editor?.destroy() + getEditorEl()?.remove() + }) }) }) From 738c436a9ff4af39d1d6abd52208eb7d46616106 Mon Sep 17 00:00:00 2001 From: Nick the Sick Date: Thu, 16 May 2024 17:10:15 +0200 Subject: [PATCH 2/2] fix: disable parsing `javascript:` links, add tests --- packages/extension-link/src/link.ts | 14 ++++- .../integration/extensions/link.spec.ts | 54 +++++++++++++++++-- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/packages/extension-link/src/link.ts b/packages/extension-link/src/link.ts index 1492a7182db..4aad82dd4fd 100644 --- a/packages/extension-link/src/link.ts +++ b/packages/extension-link/src/link.ts @@ -166,10 +166,22 @@ export const Link = Mark.create({ }, parseHTML() { - return [{ tag: 'a[href]:not([href *= "javascript:" i])' }] + return [{ + tag: 'a[href]', + getAttrs: dom => { + const href = (dom as HTMLElement).getAttribute('href') + + // prevent XSS attacks + if (!href || !isAllowedUri(href)) { + return false + } + return { href } + }, + }] }, renderHTML({ HTMLAttributes }) { + // prevent XSS attacks if (!isAllowedUri(HTMLAttributes.href)) { // strip out the href return ['a', mergeAttributes(this.options.HTMLAttributes, { ...HTMLAttributes, href: '' }), 0] diff --git a/tests/cypress/integration/extensions/link.spec.ts b/tests/cypress/integration/extensions/link.spec.ts index ee1354a54ae..b6f03442cd9 100644 --- a/tests/cypress/integration/extensions/link.spec.ts +++ b/tests/cypress/integration/extensions/link.spec.ts @@ -29,8 +29,8 @@ describe('extension-link', () => { 'ftp://info@example.com', ] - validUrls.forEach(url => { - it('does output href tag for valid schemas', () => { + it('does output href tag for valid JSON schemas', () => { + validUrls.forEach(url => { editor = new Editor({ element: createEditorEl(), extensions: [ @@ -64,6 +64,28 @@ describe('extension-link', () => { }) expect(editor.getHTML()).to.include(url) + expect(JSON.stringify(editor.getJSON())).to.include(url) + + editor?.destroy() + getEditorEl()?.remove() + }) + }) + + it('does output href tag for valid HTML schemas', () => { + validUrls.forEach(url => { + editor = new Editor({ + element: createEditorEl(), + extensions: [ + Document, + Text, + Paragraph, + Link, + ], + content: `

hello world!

`, + }) + + expect(editor.getHTML()).to.include(url) + expect(JSON.stringify(editor.getJSON())).to.include(url) editor?.destroy() getEditorEl()?.remove() @@ -164,8 +186,8 @@ describe('extension-link', () => { 'javascript\x0d:alert(window.origin)', ] - invalidUrls.forEach(url => { - it('does not output src tag for javascript schema', () => { + it('does not output href for :javascript links in JSON schema', () => { + invalidUrls.forEach(url => { editor = new Editor({ element: createEditorEl(), extensions: [ @@ -199,6 +221,30 @@ describe('extension-link', () => { }) expect(editor.getHTML()).to.not.include(url) + // Unfortunately, if the content is provided as JSON, it stays in the editor instance until it's destroyed + // At least, it cannot be outputted as HTML into a page + // expect(JSON.stringify(editor.getJSON())).to.not.include(url) + + editor?.destroy() + getEditorEl()?.remove() + }) + }) + + it('does not output href for :javascript links in HTML schema', () => { + invalidUrls.forEach(url => { + editor = new Editor({ + element: createEditorEl(), + extensions: [ + Document, + Text, + Paragraph, + Link, + ], + content: `

hello world!

`, + }) + + expect(editor.getHTML()).to.not.include(url) + expect(JSON.stringify(editor.getJSON())).to.not.include(url) editor?.destroy() getEditorEl()?.remove()