From 9377f53cf4005812c250dd412931ee2fced763fb Mon Sep 17 00:00:00 2001 From: Ioan Moldovan Date: Tue, 26 Nov 2024 01:34:11 +0200 Subject: [PATCH] feat: use built in dompurify --- conf/tsconfig.content_scripts.json | 2 +- .../compose-send-btn-module.ts | 2 +- extension/js/common/message-renderer.ts | 20 +-- extension/js/common/platform/xss.ts | 40 +++-- extension/types/purify.d.ts | 138 ------------------ extension/types/trusted-types.d.ts | 6 +- package-lock.json | 11 -- package.json | 1 - tsconfig.json | 2 +- 9 files changed, 44 insertions(+), 178 deletions(-) delete mode 100644 extension/types/purify.d.ts diff --git a/conf/tsconfig.content_scripts.json b/conf/tsconfig.content_scripts.json index c41966f8be7..c5015fb4228 100644 --- a/conf/tsconfig.content_scripts.json +++ b/conf/tsconfig.content_scripts.json @@ -16,7 +16,7 @@ "outDir": "../build/_/content_scripts", "baseUrl": "../extension", "paths": { - "dompurify": ["types/purify.d.ts"], + "dompurify": ["../node_modules/dompurify/dist/purify.cjs.d.ts"], "openpgp": ["../node_modules/openpgp/openpgp.d.ts"], "@openpgp/web-stream-tools": ["../node_modules/@openpgp/web-stream-tools/lib/index.d.ts"], "squire-rte": ["../node_modules/squire-rte/dist/types/Squire.d.ts"], diff --git a/extension/chrome/elements/compose-modules/compose-send-btn-module.ts b/extension/chrome/elements/compose-modules/compose-send-btn-module.ts index db50449d83f..66b11892408 100644 --- a/extension/chrome/elements/compose-modules/compose-send-btn-module.ts +++ b/extension/chrome/elements/compose-modules/compose-send-btn-module.ts @@ -179,7 +179,7 @@ export class ComposeSendBtnModule extends ViewModule { return; } if ('src' in node) { - const img: Element = node; + const img = node as HTMLImageElement; // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const src = img.getAttribute('src')!; const { mimeType, data } = this.parseInlineImageSrc(src); diff --git a/extension/js/common/message-renderer.ts b/extension/js/common/message-renderer.ts index 375b03adb25..7fc34704869 100644 --- a/extension/js/common/message-renderer.ts +++ b/extension/js/common/message-renderer.ts @@ -83,14 +83,17 @@ export class MessageRenderer { const inlineCIDAttachments = new Set(); // Define the hook function for DOMPurify to process image elements after sanitizing attributes - const processImageElements = (node: Element | null) => { - // Ensure the node exists and has a 'src' attribute - if (!node || !('src' in node)) return; - const imageSrc = node.getAttribute('src'); + DOMPurify.addHook('afterSanitizeAttributes', currentNode => { + // Ensure the node is an Element + if (!(currentNode instanceof Element)) return; + + // Check if the node has a 'src' attribute + const imageSrc = currentNode.getAttribute('src'); if (!imageSrc) return; + const matches = imageSrc.match(CID_PATTERN); - // Check if the src attribute contains a CID + // If the src attribute contains a CID if (matches?.[1]) { const contentId = matches[1]; const contentIdAttachment = attachments.find(attachment => attachment.cid === `<${contentId}>`); @@ -98,13 +101,10 @@ export class MessageRenderer { // Replace the src attribute with a base64 encoded string if (contentIdAttachment) { inlineCIDAttachments.add(contentIdAttachment); - node.setAttribute('src', `data:${contentIdAttachment.type};base64,${contentIdAttachment.getData().toBase64Str()}`); + currentNode.setAttribute('src', `data:${contentIdAttachment.type};base64,${contentIdAttachment.getData().toBase64Str()}`); } } - }; - - // Add the DOMPurify hook - DOMPurify.addHook('afterSanitizeAttributes', processImageElements); + }); // Sanitize the HTML and remove the DOMPurify hooks const sanitizedHtml = Xss.htmlSanitize(html); diff --git a/extension/js/common/platform/xss.ts b/extension/js/common/platform/xss.ts index 89e285eeaf9..aa5155f398d 100644 --- a/extension/js/common/platform/xss.ts +++ b/extension/js/common/platform/xss.ts @@ -107,23 +107,28 @@ export class Xss { // used whenever untrusted remote content (eg html email) is rendered, but we still want to preserve html DOMPurify.removeAllHooks(); DOMPurify.addHook('afterSanitizeAttributes', node => { - if (!node) { + // Ensure the node is an Element + if (!(node instanceof Element)) { return; } - if ('style' in node) { + + // Handle style attributes + if (node.hasAttribute('style')) { // mitigation rather than a fix, which will involve updating CSP, see https://github.com/FlowCrypt/flowcrypt-browser/issues/2648 - const style = (node as Element).getAttribute('style')?.toLowerCase(); + const style = node.getAttribute('style')?.toLowerCase(); if (style && (style.includes('url(') || style.includes('@import'))) { - (node as Element).removeAttribute('style'); // don't want any leaks through css url() + node.removeAttribute('style'); // don't want any leaks through css url() } // strip css styles that could use to overlap with the extension UI if (style && Xss.FORBID_CSS_STYLE.test(style)) { const updatedStyle = style.replace(Xss.FORBID_CSS_STYLE, ''); - (node as HTMLElement).setAttribute('style', updatedStyle); + node.setAttribute('style', updatedStyle); } } - if ('src' in node) { - const img = node as HTMLImageElement; + + // Handle image attributes + if (node.tagName === 'IMG') { + const img = node as HTMLImageElement; // Narrow type to HTMLImageElement const src = img.getAttribute('src'); if (imgHandling === 'IMG-DEL') { img.remove(); // just skip images @@ -131,20 +136,27 @@ export class Xss { img.remove(); // src that exists but is null is suspicious } else if (imgHandling === 'IMG-KEEP' && checkValidURL(src)) { // replace remote image with remote_image_container - const remoteImgEl = `
Authenticity of this remote image cannot be verified.
`; + const remoteImgEl = ` +
+ Authenticity of this remote image cannot be verified. +
`; Xss.replaceElementDANGEROUSLY(img, remoteImgEl); // xss-safe-value } } + + // Handle custom containers or CID-patterned src if ((node.classList.contains('remote_image_container') || CID_PATTERN.test(node.getAttribute('src') ?? '')) && imgHandling === 'IMG-TO-PLAIN-TEXT') { - Xss.replaceElementDANGEROUSLY(node, node.getAttribute('data-src') ?? node.getAttribute('alt') ?? ''); // xss-safe-value + const replacement = node.getAttribute('data-src') ?? node.getAttribute('alt') ?? ''; + Xss.replaceElementDANGEROUSLY(node, replacement); // xss-safe-value } - if ('target' in node) { - // open links in new window - (node as Element).setAttribute('target', '_blank'); - // prevents https://www.owasp.org/index.php/Reverse_Tabnabbing - (node as Element).setAttribute('rel', 'noopener noreferrer'); + + // Handle links (target and rel attributes) + if (node.tagName === 'A') { + node.setAttribute('target', '_blank'); // prevents https://www.owasp.org/index.php/Reverse_Tabnabbing + node.setAttribute('rel', 'noopener noreferrer'); } }); + const cleanHtml = Xss.htmlSanitize(dirtyHtml, true); DOMPurify.removeAllHooks(); return cleanHtml; diff --git a/extension/types/purify.d.ts b/extension/types/purify.d.ts deleted file mode 100644 index f7328784c90..00000000000 --- a/extension/types/purify.d.ts +++ /dev/null @@ -1,138 +0,0 @@ -/// - -export as namespace DOMPurify; -export = DOMPurify; - -declare const DOMPurify: createDOMPurifyI; - -type WindowLike = Pick< - typeof globalThis, - | "NodeFilter" - | "Node" - | "Element" - | "HTMLTemplateElement" - | "DocumentFragment" - | "HTMLFormElement" - | "DOMParser" - | "NamedNodeMap" ->; - -interface createDOMPurifyI extends DOMPurify.DOMPurifyI { - (window?: Window | WindowLike): DOMPurify.DOMPurifyI; -} - -declare namespace DOMPurify { - interface DOMPurifyI { - sanitize(source: string | Node): string; - // sanitize(source: string | Node, config: Config & { RETURN_TRUSTED_TYPE: true }): TrustedHTML; - sanitize( - source: string | Node, - config: Config & { RETURN_DOM_FRAGMENT?: false | undefined; RETURN_DOM?: false | undefined }, - ): string; - sanitize(source: string | Node, config: Config & { RETURN_DOM_FRAGMENT: true }): DocumentFragment; - sanitize(source: string | Node, config: Config & { RETURN_DOM: true }): HTMLElement; - sanitize(source: string | Node, config: Config): string | HTMLElement | DocumentFragment; - - addHook( - hook: "uponSanitizeElement", - cb: (currentNode: Element, data: SanitizeElementHookEvent, config: Config) => void, - ): void; - addHook( - hook: "uponSanitizeAttribute", - cb: (currentNode: Element, data: SanitizeAttributeHookEvent, config: Config) => void, - ): void; - addHook(hook: HookName, cb: (currentNode: Element, data: HookEvent, config: Config) => void): void; - - setConfig(cfg: Config): void; - clearConfig(): void; - isValidAttribute(tag: string, attr: string, value: string): boolean; - - removeHook(entryPoint: HookName): void; - removeHooks(entryPoint: HookName): void; - removeAllHooks(): void; - - version: string; - removed: any[]; - isSupported: boolean; - } - - interface Config { - ADD_ATTR?: string[] | undefined; - ADD_DATA_URI_TAGS?: string[] | undefined; - ADD_TAGS?: string[] | undefined; - ADD_URI_SAFE_ATTR?: string[] | undefined; - ALLOW_ARIA_ATTR?: boolean | undefined; - ALLOW_DATA_ATTR?: boolean | undefined; - ALLOW_UNKNOWN_PROTOCOLS?: boolean | undefined; - ALLOW_SELF_CLOSE_IN_ATTR?: boolean | undefined; - ALLOWED_ATTR?: string[] | undefined; - ALLOWED_TAGS?: string[] | undefined; - ALLOWED_NAMESPACES?: string[] | undefined; - ALLOWED_URI_REGEXP?: RegExp | undefined; - FORBID_ATTR?: string[] | undefined; - FORBID_CONTENTS?: string[] | undefined; - FORBID_TAGS?: string[] | undefined; - FORCE_BODY?: boolean | undefined; - IN_PLACE?: boolean | undefined; - KEEP_CONTENT?: boolean | undefined; - /** - * change the default namespace from HTML to something different - */ - NAMESPACE?: string | undefined; - PARSER_MEDIA_TYPE?: string | undefined; - RETURN_DOM_FRAGMENT?: boolean | undefined; - /** - * This defaults to `true` starting DOMPurify 2.2.0. Note that setting it to `false` - * might cause XSS from attacks hidden in closed shadowroots in case the browser - * supports Declarative Shadow: DOM https://web.dev/declarative-shadow-dom/ - */ - RETURN_DOM_IMPORT?: boolean | undefined; - RETURN_DOM?: boolean | undefined; - RETURN_TRUSTED_TYPE?: boolean | undefined; - SAFE_FOR_TEMPLATES?: boolean | undefined; - SANITIZE_DOM?: boolean | undefined; - /** @default false */ - SANITIZE_NAMED_PROPS?: boolean | undefined; - USE_PROFILES?: - | false - | { - mathMl?: boolean | undefined; - svg?: boolean | undefined; - svgFilters?: boolean | undefined; - html?: boolean | undefined; - } - | undefined; - WHOLE_DOCUMENT?: boolean | undefined; - CUSTOM_ELEMENT_HANDLING?: { - tagNameCheck?: RegExp | ((tagName: string) => boolean) | null | undefined; - attributeNameCheck?: RegExp | ((lcName: string) => boolean) | null | undefined; - allowCustomizedBuiltInElements?: boolean | undefined; - }; - } - - type HookName = - | "beforeSanitizeElements" - | "uponSanitizeElement" - | "afterSanitizeElements" - | "beforeSanitizeAttributes" - | "uponSanitizeAttribute" - | "afterSanitizeAttributes" - | "beforeSanitizeShadowDOM" - | "uponSanitizeShadowNode" - | "afterSanitizeShadowDOM"; - - type HookEvent = SanitizeElementHookEvent | SanitizeAttributeHookEvent | null; - - interface SanitizeElementHookEvent { - tagName: string; - allowedTags: { [key: string]: boolean }; - } - - interface SanitizeAttributeHookEvent { - attrName: string; - attrValue: string; - keepAttr: boolean; - allowedAttributes: { [key: string]: boolean }; - forceKeepAttr?: boolean | undefined; - } -} \ No newline at end of file diff --git a/extension/types/trusted-types.d.ts b/extension/types/trusted-types.d.ts index bfefff8c4f3..27317a429a4 100644 --- a/extension/types/trusted-types.d.ts +++ b/extension/types/trusted-types.d.ts @@ -1,3 +1,7 @@ interface Window { trustedTypes: any; -} \ No newline at end of file +} + +interface TrustedHTML {} + +interface TrustedTypePolicy {} diff --git a/package-lock.json b/package-lock.json index 796af1e9233..2041a4f2515 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,6 @@ "@types/chai": "4.3.19", "@types/chai-as-promised": "7.1.8", "@types/chrome": "0.0.284", - "@types/dompurify": "3.2.0", "@types/jquery": "3.5.32", "@types/mailparser": "3.4.5", "ava": "5.3.1", @@ -798,16 +797,6 @@ "@types/har-format": "*" } }, - "node_modules/@types/dompurify": { - "version": "3.2.0", - "resolved": "https://registry.npmjs.org/@types/dompurify/-/dompurify-3.2.0.tgz", - "integrity": "sha512-Fgg31wv9QbLDA0SpTOXO3MaxySc4DKGLi8sna4/Utjo4r3ZRPdCt4UQee8BWr+Q5z21yifghREPJGYaEOEIACg==", - "deprecated": "This is a stub types definition. dompurify provides its own type definitions, so you do not need this installed.", - "dev": true, - "dependencies": { - "dompurify": "*" - } - }, "node_modules/@types/eslint": { "version": "9.6.1", "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-9.6.1.tgz", diff --git a/package.json b/package.json index 7e76a304932..ca789023ebd 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,6 @@ "@types/chai": "4.3.19", "@types/chai-as-promised": "7.1.8", "@types/chrome": "0.0.284", - "@types/dompurify": "3.2.0", "@types/jquery": "3.5.32", "@types/mailparser": "3.4.5", "ava": "5.3.1", diff --git a/tsconfig.json b/tsconfig.json index 97a568a1972..73227600f2f 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -24,7 +24,7 @@ "pdfjs": ["lib/pdf.min.mjs", "types/pdf.js.d.ts"], "squire-rte": ["../node_modules/squire-rte/dist/types/Squire.d.ts"], "@openpgp/web-stream-tools": ["../node_modules/@openpgp/web-stream-tools/lib/index.d.ts", "lib/streams/streams.js"], - "dompurify": ["types/purify.d.ts", "lib/purify.js", "COMMENT"], + "dompurify": ["../node_modules/dompurify/dist/purify.cjs.d.ts", "lib/purify.js", "COMMENT"], "fine-uploader": ["lib/fine-uploader.js", "COMMENT"], "clipboard": ["lib/clipboard.js", "COMMENT"], "filesize": ["lib/filesize.js", "COMMENT"],