From f659015be6de8e6f0c322c5ff4d1a4532d2f29a2 Mon Sep 17 00:00:00 2001 From: "Umang G. Patel" <23169768+robonetphy@users.noreply.github.com> Date: Tue, 22 Nov 2022 01:58:53 +0530 Subject: [PATCH 01/21] fix(tools-api): pasteConfig.tags now supports a sanitize config (#2100) * event handlers function added * santization config added * integrate with paste event * lint removed * remove old changes * object based sanitization configuration support * paste config updated * logic updated * extract tag name from paste-config * tool tags added * multi tag sanitization added * the comments added * lint removed * Update types/configs/paste-config.d.ts Co-authored-by: Peter Savchenko * update the changes * lint removed\ * return empty array by get tags * submoduble reset * Update src/components/modules/paste.ts Co-authored-by: Jorge <46056498+jorgectf@users.noreply.github.com> * changelog added * tool comments added * chore: docs, code comments updated * fix: xss in processDataTransfer * base tests added * test added * rm 'only' from test suite * rm log * reorder test Co-authored-by: Peter Savchenko Co-authored-by: Jorge <46056498+jorgectf@users.noreply.github.com> --- docs/CHANGELOG.md | 3 + docs/tools.md | 24 +- src/components/dom.ts | 16 +- src/components/modules/paste.ts | 174 +++++++++-- test/cypress/tests/api/tools.spec.ts | 433 ++++++++++++++++++++++++++- types/configs/paste-config.d.ts | 14 +- types/configs/sanitizer-config.d.ts | 6 +- types/index.d.ts | 1 + 8 files changed, 627 insertions(+), 44 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 392e2f10..c34289c7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,9 @@ - `Deprecated` — *Styles API* — CSS classes `.cdx-settings-button` and `.cdx-settings-button--active` are not recommended to use. Consider configuring your block settings with new JSON API instead. - `Fix` — Wrong element not highlighted anymore when popover opened. - `Fix` — When Tunes Menu open keydown events can not be handled inside plugins. +- `Fix` — If a Tool specifies some tags to substitute on paste, all attributes of that tags will be removed before passing them to the tool. Possible XSS vulnerability fixed. +- `Fix` — Workaround for the HTMLJanitor bug with Tables (https://github.com/guardian/html-janitor/issues/3) added +- `Improvement` — *Tools API* — `pasteConfig().tags` now support sanitizing configuration. It allows you to leave some explicitly specified attributes for pasted content. ### 2.25.0 diff --git a/docs/tools.md b/docs/tools.md index ec0dd7d7..7df5ceb6 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -151,7 +151,7 @@ To handle pasted HTML elements object returned from `pasteConfig` getter should For correct work you MUST provide `onPaste` handler at least for `defaultBlock` Tool. -> Example +#### Example Header Tool can handle `H1`-`H6` tags using paste handling API @@ -163,7 +163,27 @@ static get pasteConfig() { } ``` -> Same tag can be handled by one (first specified) Tool only. +**Note. Same tag can be handled by one (first specified) Tool only.** + +**Note. All attributes of pasted tag will be removed. To leave some attribute, you should explicitly specify them. Se below** + +Let's suppose you want to leave the 'src' attribute when handle pasting of the `img` tags. Your config should look like this: + +```javascript +static get pasteConfig() { + return { + tags: [ + { + img: { + src: true + } + } + ], + } +} +``` + +[Read more](https://editorjs.io/sanitizer) about the sanitizing configuration. ### RegExp patterns handling diff --git a/src/components/dom.ts b/src/components/dom.ts index 75628ea2..7c907aed 100644 --- a/src/components/dom.ts +++ b/src/components/dom.ts @@ -53,7 +53,7 @@ export default class Dom { * * @returns {HTMLElement} */ - public static make(tagName: string, classNames: string|string[] = null, attributes: object = {}): HTMLElement { + public static make(tagName: string, classNames: string | string[] = null, attributes: object = {}): HTMLElement { const el = document.createElement(tagName); if (Array.isArray(classNames)) { @@ -109,8 +109,8 @@ export default class Dom { * @param {Element|Element[]|DocumentFragment|Text|Text[]} elements - element or elements list */ public static append( - parent: Element|DocumentFragment, - elements: Element|Element[]|DocumentFragment|Text|Text[] + parent: Element | DocumentFragment, + elements: Element | Element[] | DocumentFragment | Text | Text[] ): void { if (Array.isArray(elements)) { elements.forEach((el) => parent.appendChild(el)); @@ -125,7 +125,7 @@ export default class Dom { * @param {Element} parent - where to append * @param {Element|Element[]} elements - element or elements list */ - public static prepend(parent: Element, elements: Element|Element[]): void { + public static prepend(parent: Element, elements: Element | Element[]): void { if (Array.isArray(elements)) { elements = elements.reverse(); elements.forEach((el) => parent.prepend(el)); @@ -168,7 +168,7 @@ export default class Dom { * * @returns {Element} */ - public static find(el: Element|Document = document, selector: string): Element { + public static find(el: Element | Document = document, selector: string): Element { return el.querySelector(selector); } @@ -192,7 +192,7 @@ export default class Dom { * * @returns {NodeList} */ - public static findAll(el: Element|Document = document, selector: string): NodeList { + public static findAll(el: Element | Document = document, selector: string): NodeList { return el.querySelectorAll(selector); } @@ -523,6 +523,8 @@ export default class Dom { 'ruby', 'section', 'table', + 'tbody', + 'thead', 'tr', 'tfoot', 'ul', @@ -619,7 +621,7 @@ export default class Dom { * @todo handle case when editor initialized in scrollable popup * @param el - element to compute offset */ - public static offset(el): {top: number; left: number; right: number; bottom: number} { + public static offset(el): { top: number; left: number; right: number; bottom: number } { const rect = el.getBoundingClientRect(); const scrollLeft = window.pageXOffset || document.documentElement.scrollLeft; const scrollTop = window.pageYOffset || document.documentElement.scrollTop; diff --git a/src/components/modules/paste.ts b/src/components/modules/paste.ts index e901897b..5a8d12f8 100644 --- a/src/components/modules/paste.ts +++ b/src/components/modules/paste.ts @@ -4,7 +4,9 @@ import * as _ from '../utils'; import { BlockAPI, PasteEvent, - PasteEventDetail + PasteEventDetail, + SanitizerConfig, + SanitizerRule } from '../../../types'; import Block from '../block'; import { SavedData } from '../../../types/data-formats'; @@ -20,6 +22,12 @@ interface TagSubstitute { * */ tool: BlockTool; + + /** + * If a Tool specifies just a tag name, all the attributes will be sanitized. + * But Tool can explicitly specify sanitizer configuration for supported tags + */ + sanitizationConfig?: SanitizerRule; } /** @@ -112,12 +120,12 @@ export default class Paste extends Module { /** * Tags` substitutions parameters */ - private toolsTags: {[tag: string]: TagSubstitute} = {}; + private toolsTags: { [tag: string]: TagSubstitute } = {}; /** * Store tags to substitute by tool name */ - private tagsByTool: {[tools: string]: string[]} = {}; + private tagsByTool: { [tools: string]: string[] } = {}; /** Patterns` substitutions parameters */ private toolsPatterns: PatternSubstitute[] = []; @@ -186,7 +194,7 @@ export default class Paste extends Module { this.insertEditorJSData(JSON.parse(editorJSData)); return; - } catch (e) {} // Do nothing and continue execution as usual if error appears + } catch (e) { } // Do nothing and continue execution as usual if error appears } /** @@ -198,7 +206,11 @@ export default class Paste extends Module { /** Add all tags that can be substituted to sanitizer configuration */ const toolsTags = Object.keys(this.toolsTags).reduce((result, tag) => { - result[tag.toLowerCase()] = true; + /** + * If Tool explicitly specifies sanitizer configuration for the tag, use it. + * Otherwise, remove all attributes + */ + result[tag.toLowerCase()] = this.toolsTags[tag].sanitizationConfig ?? {}; return result; }, {}); @@ -306,31 +318,69 @@ export default class Paste extends Module { } } + /** + * Get tags name list from either tag name or sanitization config. + * + * @param {string | object} tagOrSanitizeConfig - tag name or sanitize config object. + * @returns {string[]} array of tags. + */ + private collectTagNames(tagOrSanitizeConfig: string | SanitizerConfig): string[] { + /** + * If string, then it is a tag name. + */ + if (_.isString(tagOrSanitizeConfig)) { + return [ tagOrSanitizeConfig ]; + } + /** + * If object, then its keys are tags. + */ + if (_.isObject(tagOrSanitizeConfig)) { + return Object.keys(tagOrSanitizeConfig); + } + + /** Return empty tag list */ + return []; + } + /** * Get tags to substitute by Tool * * @param tool - BlockTool object */ private getTagsConfig(tool: BlockTool): void { - const tags = tool.pasteConfig.tags || []; + const tagsOrSanitizeConfigs = tool.pasteConfig.tags || []; + const toolTags = []; - tags.forEach((tag) => { - if (Object.prototype.hasOwnProperty.call(this.toolsTags, tag)) { - _.log( - `Paste handler for «${tool.name}» Tool on «${tag}» tag is skipped ` + - `because it is already used by «${this.toolsTags[tag].tool.name}» Tool.`, - 'warn' - ); + tagsOrSanitizeConfigs.forEach((tagOrSanitizeConfig) => { + const tags = this.collectTagNames(tagOrSanitizeConfig); - return; - } + /** + * Add tags to toolTags array + */ + toolTags.push(...tags); + tags.forEach((tag) => { + if (Object.prototype.hasOwnProperty.call(this.toolsTags, tag)) { + _.log( + `Paste handler for «${tool.name}» Tool on «${tag}» tag is skipped ` + + `because it is already used by «${this.toolsTags[tag].tool.name}» Tool.`, + 'warn' + ); - this.toolsTags[tag.toUpperCase()] = { - tool, - }; + return; + } + /** + * Get sanitize config for tag. + */ + const sanitizationConfig = _.isObject(tagOrSanitizeConfig) ? tagOrSanitizeConfig[tag] : null; + + this.toolsTags[tag.toUpperCase()] = { + tool, + sanitizationConfig, + }; + }); }); - this.tagsByTool[tool.name] = tags.map((t) => t.toUpperCase()); + this.tagsByTool[tool.name] = toolTags.map((t) => t.toUpperCase()); } /** @@ -449,7 +499,7 @@ export default class Paste extends Module { private async processFiles(items: FileList): Promise { const { BlockManager } = this.Editor; - let dataToInsert: {type: string; event: PasteEvent}[]; + let dataToInsert: { type: string; event: PasteEvent }[]; dataToInsert = await Promise.all( Array @@ -473,7 +523,7 @@ export default class Paste extends Module { * * @param {File} file - file to process */ - private async processFile(file: File): Promise<{event: PasteEvent; type: string}> { + private async processFile(file: File): Promise<{ event: PasteEvent; type: string }> { const extension = _.getFileExtension(file); const foundConfig = Object @@ -515,6 +565,19 @@ export default class Paste extends Module { */ private processHTML(innerHTML: string): PasteData[] { const { Tools } = this.Editor; + + /** + * @todo Research, do we really need to always wrap innerHTML to a div: + * - tag could be processed separately, but for now it becomes div-wrapped + * and then .getNodes() returns strange: [document-fragment, img] + * (description of the method says that it should should return only block tags or fragments, + * but there are inline-block element along with redundant empty fragment) + * - probably this is a reason of bugs with unexpected new block creation instead of inline pasting: + * - https://github.com/codex-team/editor.js/issues/1427 + * - https://github.com/codex-team/editor.js/issues/1244 + * - https://github.com/codex-team/editor.js/issues/740 + * + */ const wrapper = $.make('DIV'); wrapper.innerHTML = innerHTML; @@ -543,16 +606,65 @@ export default class Paste extends Module { break; } - const { tags } = tool.pasteConfig; + const { tags: tagsOrSanitizeConfigs } = tool.pasteConfig; - const toolTags = tags.reduce((result, tag) => { - result[tag.toLowerCase()] = {}; + /** + * Reduce the tags or sanitize configs to a single array of sanitize config. + * For example: + * If sanitize config is + * [ 'tbody', + * { + * table: { + * width: true, + * height: true, + * }, + * }, + * { + * td: { + * colspan: true, + * rowspan: true, + * }, + * tr: { // <-- the second tag + * height: true, + * }, + * }, + * ] + * then sanitize config will be + * [ + * 'table':{}, + * 'tbody':{width: true, height: true} + * 'td':{colspan: true, rowspan: true}, + * 'tr':{height: true} + * ] + */ + const toolTags = tagsOrSanitizeConfigs.reduce((result, tagOrSanitizeConfig) => { + const tags = this.collectTagNames(tagOrSanitizeConfig); + + tags.forEach((tag) => { + const sanitizationConfig = _.isObject(tagOrSanitizeConfig) ? tagOrSanitizeConfig[tag] : null; + + result[tag] = sanitizationConfig || {}; + }); return result; }, {}); + const customConfig = Object.assign({}, toolTags, tool.baseSanitizeConfig); - content.innerHTML = clean(content.innerHTML, customConfig); + /** + * A workaround for the HTMLJanitor bug with Tables (incorrect sanitizing of table.innerHTML) + * https://github.com/guardian/html-janitor/issues/3 + */ + if (content.tagName.toLowerCase() === 'table') { + const cleanTableHTML = clean(content.outerHTML, customConfig); + const tmpWrapper = $.make('div', undefined, { + innerHTML: cleanTableHTML, + }); + + content = tmpWrapper.firstChild; + } else { + content.innerHTML = clean(content.innerHTML, customConfig); + } const event = this.composePasteEvent('tag', { data: content, @@ -565,7 +677,12 @@ export default class Paste extends Module { event, }; }) - .filter((data) => !$.isNodeEmpty(data.content) || $.isSingleTag(data.content)); + .filter((data) => { + const isEmpty = $.isEmpty(data.content); + const isSingleTag = $.isSingleTag(data.content); + + return !isEmpty || isSingleTag; + }); } /** @@ -576,7 +693,7 @@ export default class Paste extends Module { * @returns {PasteData[]} */ private processPlain(plain: string): PasteData[] { - const { defaultBlock } = this.config as {defaultBlock: string}; + const { defaultBlock } = this.config as { defaultBlock: string }; if (!plain) { return []; @@ -681,7 +798,7 @@ export default class Paste extends Module { * * @returns {Promise<{event: PasteEvent, tool: string}>} */ - private async processPattern(text: string): Promise<{event: PasteEvent; tool: string}> { + private async processPattern(text: string): Promise<{ event: PasteEvent; tool: string }> { const pattern = this.toolsPatterns.find((substitute) => { const execResult = substitute.pattern.exec(text); @@ -878,3 +995,4 @@ export default class Paste extends Module { }) as PasteEvent; } } + diff --git a/test/cypress/tests/api/tools.spec.ts b/test/cypress/tests/api/tools.spec.ts index aa73a933..6064f183 100644 --- a/test/cypress/tests/api/tools.spec.ts +++ b/test/cypress/tests/api/tools.spec.ts @@ -1,5 +1,5 @@ -import { ToolboxConfig, BlockToolData, ToolboxConfigEntry } from '../../../../types'; -import { TunesMenuConfig } from '../../../../types/tools'; +import { ToolboxConfig, BlockToolData, ToolboxConfigEntry, PasteConfig } from '../../../../types'; +import { HTMLPasteEvent, PasteEvent, TunesMenuConfig } from '../../../../types/tools'; /* eslint-disable @typescript-eslint/no-empty-function */ @@ -270,7 +270,7 @@ describe('Editor Tools Api', () => { }); }); - context('Tunes', () => { + context('Tunes — renderSettings()', () => { it('should contain a single block tune configured in tool\'s renderSettings() method', () => { /** Tool with single tunes menu entry configured */ class TestTool { @@ -490,4 +490,431 @@ describe('Editor Tools Api', () => { .should('contain.text', sampleText); }); }); + + /** + * @todo cover all the pasteConfig properties + */ + context('Paste — pasteConfig()', () => { + context('tags', () => { + /** + * tags: ['H1', 'H2'] + */ + it('should use corresponding tool when the array of tag names specified', () => { + /** + * Test tool with pasteConfig.tags specified + */ + class TestImgTool { + /** config specified handled tag */ + public static get pasteConfig(): PasteConfig { + return { + tags: [ 'img' ], // only tag name specified. Attributes should be sanitized + }; + } + + /** onPaste callback will be stubbed below */ + public onPaste(): void {} + + /** save is required for correct implementation of the BlockTool class */ + public save(): void {} + + /** render is required for correct implementation of the BlockTool class */ + public render(): HTMLElement { + return document.createElement('img'); + } + } + + const toolsOnPaste = cy.spy(TestImgTool.prototype, 'onPaste'); + + cy.createEditor({ + tools: { + testTool: TestImgTool, + }, + }).as('editorInstance'); + + cy.get('[data-cy=editorjs]') + .get('div.ce-block') + .click() + .paste({ + 'text/html': '', + }) + .then(() => { + expect(toolsOnPaste).to.be.called; + }); + }); + + /** + * tags: ['img'] -> + */ + it('should sanitize all attributes from tag, if only tag name specified ', () => { + /** + * Variable used for spying the pasted element we are passing to the Tool + */ + let pastedElement; + + /** + * Test tool with pasteConfig.tags specified + */ + class TestImageTool { + /** config specified handled tag */ + public static get pasteConfig(): PasteConfig { + return { + tags: [ 'img' ], // only tag name specified. Attributes should be sanitized + }; + } + + /** onPaste callback will be stubbed below */ + public onPaste(): void {} + + /** save is required for correct implementation of the BlockTool class */ + public save(): void {} + + /** render is required for correct implementation of the BlockTool class */ + public render(): HTMLElement { + return document.createElement('img'); + } + } + + /** + * Stub the onPaste method to access the PasteEvent data for assertion + */ + cy.stub(TestImageTool.prototype, 'onPaste').callsFake((event: HTMLPasteEvent) => { + pastedElement = event.detail.data; + }); + + cy.createEditor({ + tools: { + testImageTool: TestImageTool, + }, + }); + + cy.get('[data-cy=editorjs]') + .get('div.ce-block') + .click() + .paste({ + 'text/html': '', // all attributes should be sanitized + }) + .then(() => { + expect(pastedElement).not.to.be.undefined; + expect(pastedElement.tagName.toLowerCase()).eq('img'); + expect(pastedElement.attributes.length).eq(0); + }); + }); + + /** + * tags: [{ + * img: { + * src: true + * } + * }] + * -> + * + */ + it('should leave attributes if entry specified as a sanitizer config ', () => { + /** + * Variable used for spying the pasted element we are passing to the Tool + */ + let pastedElement; + + /** + * Test tool with pasteConfig.tags specified + */ + class TestImageTool { + /** config specified handled tag */ + public static get pasteConfig(): PasteConfig { + return { + tags: [ + { + img: { + src: true, + }, + }, + ], + }; + } + + /** onPaste callback will be stubbed below */ + public onPaste(): void {} + + /** save is required for correct implementation of the BlockTool class */ + public save(): void {} + + /** render is required for correct implementation of the BlockTool class */ + public render(): HTMLElement { + return document.createElement('img'); + } + } + + /** + * Stub the onPaste method to access the PasteEvent data for assertion + */ + cy.stub(TestImageTool.prototype, 'onPaste').callsFake((event: HTMLPasteEvent) => { + pastedElement = event.detail.data; + }); + + cy.createEditor({ + tools: { + testImageTool: TestImageTool, + }, + }); + + cy.get('[data-cy=editorjs]') + .get('div.ce-block') + .click() + .paste({ + 'text/html': '', + }) + .then(() => { + expect(pastedElement).not.to.be.undefined; + + /** + * Check that the has only "src" attribute + */ + expect(pastedElement.tagName.toLowerCase()).eq('img'); + expect(pastedElement.getAttribute('src')).eq('foo'); + expect(pastedElement.attributes.length).eq(1); + }); + }); + + /** + * tags: [ + * 'video', + * { + * source: { + * src: true + * } + * } + * ] + */ + it('should support mixed tag names and sanitizer config ', () => { + /** + * Variable used for spying the pasted element we are passing to the Tool + */ + let pastedElement; + + /** + * Test tool with pasteConfig.tags specified + */ + class TestTool { + /** config specified handled tag */ + public static get pasteConfig(): PasteConfig { + return { + tags: [ + 'video', // video should not have attributes + { + source: { // source should have only src attribute + src: true, + }, + }, + ], + }; + } + + /** onPaste callback will be stubbed below */ + public onPaste(): void {} + + /** save is required for correct implementation of the BlockTool class */ + public save(): void {} + + /** render is required for correct implementation of the BlockTool class */ + public render(): HTMLElement { + return document.createElement('tbody'); + } + } + + /** + * Stub the onPaste method to access the PasteEvent data for assertion + */ + cy.stub(TestTool.prototype, 'onPaste').callsFake((event: HTMLPasteEvent) => { + pastedElement = event.detail.data; + }); + + cy.createEditor({ + tools: { + testTool: TestTool, + }, + }); + + cy.get('[data-cy=editorjs]') + .get('div.ce-block') + .click() + .paste({ + 'text/html': '', + }) + .then(() => { + expect(pastedElement).not.to.be.undefined; + + /** + * Check that