From bcfca6fe1ed42aa3d9d15ed4b3ae67c0620ed81e Mon Sep 17 00:00:00 2001 From: Mark Michaelis Date: Tue, 24 Oct 2023 13:09:47 +0200 Subject: [PATCH] feat: Test And Adjust Attribute Sanitation Adjusting the parameter order passed to the attribute predicate. Now that `attributeName` comes first, it is easier for predicates just to ignore the possibly irrelevant tag-name (instead of always having to deal with an unused parameter). --- .../__tests__/bbob/htmlSanitizer.test.ts | 79 +++++++++++++++++++ .../src/bbob/htmlSanitizer.ts | 18 +++-- 2 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 packages/ckeditor5-bbcode/__tests__/bbob/htmlSanitizer.test.ts diff --git a/packages/ckeditor5-bbcode/__tests__/bbob/htmlSanitizer.test.ts b/packages/ckeditor5-bbcode/__tests__/bbob/htmlSanitizer.test.ts new file mode 100644 index 0000000000..ffcfe0e29c --- /dev/null +++ b/packages/ckeditor5-bbcode/__tests__/bbob/htmlSanitizer.test.ts @@ -0,0 +1,79 @@ +import { AllowedAttributePredicate, htmlSanitizer } from "../../src/bbob/htmlSanitizer"; +import bbob from "@bbob/core/es"; +import { render } from "@bbob/html/es"; + +describe("htmlSanitizer", () => { + describe("Default Configuration", () => { + const plugin = htmlSanitizer(); + const processor = bbob(plugin); + const process = (bbCode: string): string => processor.process(bbCode, { render }).html; + + it.each` + input | expected | comment + ${``} | ${``} | ${`Empty string handling`} + ${`Lorem`} | ${`Lorem`} | ${`unsuspicious string handling`} + ${`[b]T[/b]`} | ${`T`} | ${`unsuspicious element handling`} + ${`T`} | ${`<b>T</b>`} | ${`encode HTML elements by default`} + ${`[b onclick=xss()]T[/b]`} | ${`T`} | ${`remove suspicious attributes`} + ${`[b oNcLick=xss()]T[/b]`} | ${`T`} | ${`remove suspicious attributes`} + `( + "[$#] $comment: Should process '$input' to '$expected'", + ({ input, expected }: { input: string; expected: string }) => { + expect(process(input)).toBe(expected); + }, + ); + }); + + /** + * While not recommended in context as CKEditor 5-Plugin, this customization + * option matches a similar feature of Markdown. And: Internally it clearly + * shows where escaping happens and why. + */ + describe("Allow HTML", () => { + const plugin = htmlSanitizer({ allowHtml: true }); + const processor = bbob(plugin); + const process = (bbCode: string): string => processor.process(bbCode, { render }).html; + + it.each` + input | expected | comment + ${``} | ${``} | ${`Empty string handling`} + ${`Lorem`} | ${`Lorem`} | ${`unsuspicious string handling`} + ${`[b]T[/b]`} | ${`T`} | ${`unsuspicious element handling`} + ${`T`} | ${`T`} | ${`enabled by option: let HTML elements pass through`} + ${`[b onclick=xss()]T[/b]`} | ${`T`} | ${`still: remove suspicious attributes`} + `( + "[$#] $comment: Should process '$input' to '$expected'", + ({ input, expected }: { input: string; expected: string }) => { + expect(process(input)).toBe(expected); + }, + ); + }); + + describe("Custom Attribute Filter", () => { + /** + * Allows only `class` attribute for `b` and `i` tags. Allows `id` + * attribute for any tag. + */ + const isAllowedAttribute: AllowedAttributePredicate = (attr: string, tag: string) => { + if (["b", "i"].includes(tag)) { + return attr === "class"; + } + return attr === "id"; + }; + const plugin = htmlSanitizer({ isAllowedAttribute }); + const processor = bbob(plugin); + const process = (bbCode: string): string => processor.process(bbCode, { render }).html; + + it.each` + input | expected | comment + ${`[b class=CLASS]T[/b]`} | ${`T`} | ${`b.class passes custom filter`} + ${`[u class=CLASS]T[/u]`} | ${`T`} | ${`u.class is blocked by custom filter`} + ${`[u class=CLASS id=ID]T[/u]`} | ${`T`} | ${`u.class is blocked by custom filter, only u.id passes`} + `( + "[$#] $comment: Should process '$input' to '$expected'", + ({ input, expected }: { input: string; expected: string }) => { + expect(process(input)).toBe(expected); + }, + ); + }); +}); diff --git a/packages/ckeditor5-bbcode/src/bbob/htmlSanitizer.ts b/packages/ckeditor5-bbcode/src/bbob/htmlSanitizer.ts index 38545933ff..36de941aa2 100644 --- a/packages/ckeditor5-bbcode/src/bbob/htmlSanitizer.ts +++ b/packages/ckeditor5-bbcode/src/bbob/htmlSanitizer.ts @@ -2,13 +2,19 @@ import { CorePlugin, CoreTree } from "@bbob/core/es"; import { escapeHTML, getUniqAttr, isTagNode } from "@bbob/plugin-helper/es"; /** - * Default attribute filter. Only forbids any `on*` handlers. + * Predicate for filtering attributes. This predicate is not applied to + * unique attributes. * - * @param tagName - owning tag name * @param attributeName - name of attribute + * @param tagName - owning tag name + */ +export type AllowedAttributePredicate = (attributeName: string, tagName: string) => boolean; + +/** + * Default attribute filter. Only forbids any `on*` handlers. */ -export const defaultIsAllowedAttribute = (tagName: string, attributeName: string): boolean => - !attributeName.startsWith("on"); +export const defaultIsAllowedAttribute: AllowedAttributePredicate = (attributeName: string): boolean => + !attributeName.toLowerCase().startsWith("on"); export interface HtmlSanitizerOptions { /** @@ -24,7 +30,7 @@ export interface HtmlSanitizerOptions { * @param tagName - owning tag name * @param attributeName - name of attribute */ - isAllowedAttribute?: typeof defaultIsAllowedAttribute; + isAllowedAttribute?: AllowedAttributePredicate; } const walk = ( @@ -51,7 +57,7 @@ const walk = ( // by object. item.attrs = Object.fromEntries( Object.entries(item.attrs).filter( - ([attrName]) => uniqAttr === attrName || options.isAllowedAttribute(item.tag, attrName), + ([attrName]) => uniqAttr === attrName || options.isAllowedAttribute(attrName, item.tag), ), ); } else {