From 972636de77f9481629c246c5e647a93cc330ed18 Mon Sep 17 00:00:00 2001 From: Quinn Slack Date: Tue, 28 Mar 2023 18:30:30 -0700 Subject: [PATCH] use DOMPurify instead of sanitize-html for smaller bundle (#50002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also sanitize HTML more strictly. Previously we allowed SVG and `data:` URIs in some cases (for some functionality from the legacy Sourcegraph extension API). This is no longer needed, and getting stricter in HTML sanitization is generally good. ## Test plan Test callers of renderMarkdown in UI. Co-authored-by: Juliana Peña --- client/branded/BUILD.bazel | 2 - .../components/CommitSearchResultMatch.tsx | 4 +- ...eamingProgressSkippedPopover.test.tsx.snap | 10 -- client/cody-shared/package.json | 3 + client/cody-shared/src/chat/markdown.ts | 37 ++---- .../src/chat/recipes/chat-question.ts | 4 +- .../src/chat/recipes/explain-code-detailed.ts | 3 +- .../chat/recipes/explain-code-high-level.ts | 5 +- .../src/chat/recipes/generate-docstring.ts | 5 +- .../src/chat/recipes/generate-test.ts | 5 +- .../cody-shared/src/chat/recipes/git-log.ts | 3 +- .../chat/recipes/improve-variable-names.ts | 5 +- .../cody-shared/src/chat/recipes/translate.ts | 5 +- .../cody-shared/src/chat/transcript/index.ts | 4 +- .../src/chat/transcript/transcript.test.ts | 4 +- client/cody-shared/tsconfig.json | 1 + client/cody/src/chat/ChatViewProvider.ts | 3 +- .../src/integration-test/extension.test.ts | 6 +- client/cody/src/integration-test/index.ts | 2 +- client/cody/webviews/Chat.tsx | 14 +- .../common/src/util/markdown/markdown.test.ts | 33 ++--- client/common/src/util/markdown/markdown.ts | 122 +++--------------- .../__snapshots__/HoverOverlay.test.tsx.snap | 12 -- .../__snapshots__/Notices.test.tsx.snap | 4 - package.json | 4 +- pnpm-lock.yaml | 100 ++++++-------- third-party-licenses/ThirdPartyLicenses.csv | 1 - 27 files changed, 115 insertions(+), 286 deletions(-) diff --git a/client/branded/BUILD.bazel b/client/branded/BUILD.bazel index 9b69e16715e..e02f48202e0 100644 --- a/client/branded/BUILD.bazel +++ b/client/branded/BUILD.bazel @@ -169,7 +169,6 @@ ts_project( "//:node_modules/@types/node", "//:node_modules/@types/react", "//:node_modules/@types/react-dom", - "//:node_modules/@types/sanitize-html", "//:node_modules/classnames", "//:node_modules/comlink", #keep "//:node_modules/copy-to-clipboard", @@ -184,7 +183,6 @@ ts_project( "//:node_modules/react-sticky-box", "//:node_modules/react-visibility-sensor", "//:node_modules/rxjs", - "//:node_modules/sanitize-html", "//:node_modules/ts-key-enum", "//:node_modules/use-resize-observer", "//client/shared:shared_lib", #keep diff --git a/client/branded/src/search-ui/components/CommitSearchResultMatch.tsx b/client/branded/src/search-ui/components/CommitSearchResultMatch.tsx index 97ff22f57cd..9cf40c5aab5 100644 --- a/client/branded/src/search-ui/components/CommitSearchResultMatch.tsx +++ b/client/branded/src/search-ui/components/CommitSearchResultMatch.tsx @@ -1,10 +1,10 @@ import React, { useEffect, useMemo, useRef, useState } from 'react' import classNames from 'classnames' +import DOMPurify from 'dompurify' import { range } from 'lodash' import { of } from 'rxjs' import { catchError } from 'rxjs/operators' -import sanitizeHtml from 'sanitize-html' import { highlightNode, logger } from '@sourcegraph/common' import { PlatformContextProps } from '@sourcegraph/shared/src/platform/context' @@ -47,7 +47,7 @@ export const CommitSearchResultMatch: React.FunctionComponent { logger.log(error) - return of('
' + sanitizeHtml(item.content) + '
') + return of('
' + DOMPurify.sanitize(item.content) + '
') }) ) .subscribe(highlightedCommitContent => { diff --git a/client/branded/src/search-ui/results/progress/__snapshots__/StreamingProgressSkippedPopover.test.tsx.snap b/client/branded/src/search-ui/results/progress/__snapshots__/StreamingProgressSkippedPopover.test.tsx.snap index 1e63129251c..7897219220e 100644 --- a/client/branded/src/search-ui/results/progress/__snapshots__/StreamingProgressSkippedPopover.test.tsx.snap +++ b/client/branded/src/search-ui/results/progress/__snapshots__/StreamingProgressSkippedPopover.test.tsx.snap @@ -67,8 +67,6 @@ exports[`StreamingProgressSkippedPopover should render correctly 1`] = `

There was a network error retrieving search results. Check your Internet connection and try again.

- -
Search timed out

- -
10k forked repositories excluded

- -
60k archived repositories excluded

- -
in your query.

- -
- // - span: ['class'], - code: ['class'], - }, -} - -export function renderMarkdown(text: string): string { +/** + * Render Markdown to safe HTML. + * + * NOTE: This only works when called in an environment with the DOM. In the VS Code extension, it + * only works in the webview context, not in the extension host context, because the latter lacks a + * DOM. We could use isomorphic-dompurify for that, but that adds needless complexity for now. If + * that becomes necessary, we can add that. + */ +export function renderMarkdown(markdown: string): string { registerHighlightContributions() - return sanitize(marked.parse(text, { gfm: true, highlight: highlightCode, breaks: true }), sanitizeOptions) + + // Add Cody-specific Markdown rendering if needed. + return renderMarkdownCommon(markdown) } diff --git a/client/cody-shared/src/chat/recipes/chat-question.ts b/client/cody-shared/src/chat/recipes/chat-question.ts index cf89721ac62..b25a88faf8b 100644 --- a/client/cody-shared/src/chat/recipes/chat-question.ts +++ b/client/cody-shared/src/chat/recipes/chat-question.ts @@ -6,7 +6,6 @@ import { MAX_CURRENT_FILE_TOKENS, MAX_HUMAN_INPUT_TOKENS } from '../../prompt/co import { populateCodeContextTemplate } from '../../prompt/templates' import { truncateText } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { Recipe } from './recipe' @@ -24,11 +23,10 @@ export class ChatQuestion implements Recipe { ): Promise { const timestamp = getShortTimestamp() const truncatedText = truncateText(humanChatInput, MAX_HUMAN_INPUT_TOKENS) - const displayText = renderMarkdown(humanChatInput) return Promise.resolve( new Interaction( - { speaker: 'human', text: truncatedText, displayText, timestamp }, + { speaker: 'human', text: truncatedText, displayText: humanChatInput, timestamp }, { speaker: 'assistant', text: '', displayText: '', timestamp }, this.getContextMessages(truncatedText, editor, intentDetector, codebaseContext) ) diff --git a/client/cody-shared/src/chat/recipes/explain-code-detailed.ts b/client/cody-shared/src/chat/recipes/explain-code-detailed.ts index 257fe7d1598..20a7184397f 100644 --- a/client/cody-shared/src/chat/recipes/explain-code-detailed.ts +++ b/client/cody-shared/src/chat/recipes/explain-code-detailed.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS, MAX_RECIPE_SURROUNDING_TOKENS } from '../../prompt/constants' import { truncateText, truncateTextStart } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { getContextMessagesFromSelection, getNormalizedLanguageName, MARKDOWN_FORMAT_PROMPT } from './helpers' @@ -33,7 +32,7 @@ export class ExplainCodeDetailed implements Recipe { const languageName = getNormalizedLanguageName(selection.fileName) const promptMessage = `Please explain the following ${languageName} code. Be very detailed and specific, and indicate when it is not clear to you what is going on. Format your response as an ordered list.\n\`\`\`\n${truncatedSelectedText}\n\`\`\`\n${MARKDOWN_FORMAT_PROMPT}` - const displayText = renderMarkdown(`Explain the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\``) + const displayText = `Explain the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\`` return new Interaction( { speaker: 'human', text: promptMessage, displayText, timestamp }, diff --git a/client/cody-shared/src/chat/recipes/explain-code-high-level.ts b/client/cody-shared/src/chat/recipes/explain-code-high-level.ts index 8cb05e51946..baef6aa53fd 100644 --- a/client/cody-shared/src/chat/recipes/explain-code-high-level.ts +++ b/client/cody-shared/src/chat/recipes/explain-code-high-level.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS, MAX_RECIPE_SURROUNDING_TOKENS } from '../../prompt/constants' import { truncateText, truncateTextStart } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { getContextMessagesFromSelection, getNormalizedLanguageName, MARKDOWN_FORMAT_PROMPT } from './helpers' @@ -33,9 +32,7 @@ export class ExplainCodeHighLevel implements Recipe { const languageName = getNormalizedLanguageName(selection.fileName) const promptMessage = `Explain the following ${languageName} code at a high level. Only include details that are essential to an overal understanding of what's happening in the code.\n\`\`\`\n${truncatedSelectedText}\n\`\`\`\n${MARKDOWN_FORMAT_PROMPT}` - const displayText = renderMarkdown( - `Explain the following code at a high level:\n\`\`\`\n${selection.selectedText}\n\`\`\`` - ) + const displayText = `Explain the following code at a high level:\n\`\`\`\n${selection.selectedText}\n\`\`\`` return new Interaction( { speaker: 'human', text: promptMessage, displayText, timestamp }, diff --git a/client/cody-shared/src/chat/recipes/generate-docstring.ts b/client/cody-shared/src/chat/recipes/generate-docstring.ts index 37ed8d8cd13..1d579aadfd6 100644 --- a/client/cody-shared/src/chat/recipes/generate-docstring.ts +++ b/client/cody-shared/src/chat/recipes/generate-docstring.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS, MAX_RECIPE_SURROUNDING_TOKENS } from '../../prompt/constants' import { truncateText, truncateTextStart } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { @@ -56,9 +55,7 @@ export class GenerateDocstring implements Recipe { docStart = '// ' } - const displayText = renderMarkdown( - `Generate documentation for the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\`` - ) + const displayText = `Generate documentation for the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\`` const assistantResponsePrefix = `Here is the generated documentation:\n\`\`\`${extension}\n${docStart}` return new Interaction( diff --git a/client/cody-shared/src/chat/recipes/generate-test.ts b/client/cody-shared/src/chat/recipes/generate-test.ts index f859034d518..7d14088098b 100644 --- a/client/cody-shared/src/chat/recipes/generate-test.ts +++ b/client/cody-shared/src/chat/recipes/generate-test.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS, MAX_RECIPE_SURROUNDING_TOKENS } from '../../prompt/constants' import { truncateText, truncateTextStart } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { @@ -41,9 +40,7 @@ export class GenerateTest implements Recipe { const promptMessage = `Generate a unit test in ${languageName} for the following code:\n\`\`\`${extension}\n${truncatedSelectedText}\n\`\`\`\n${MARKDOWN_FORMAT_PROMPT}` const assistantResponsePrefix = `Here is the generated unit test:\n\`\`\`${extension}\n` - const displayText = renderMarkdown( - `Generate a unit test for the following code:\n\`\`\`${extension}\n${selection.selectedText}\n\`\`\`` - ) + const displayText = `Generate a unit test for the following code:\n\`\`\`${extension}\n${selection.selectedText}\n\`\`\`` return new Interaction( { speaker: 'human', text: promptMessage, displayText, timestamp }, diff --git a/client/cody-shared/src/chat/recipes/git-log.ts b/client/cody-shared/src/chat/recipes/git-log.ts index 140f098b870..8b7f893568d 100644 --- a/client/cody-shared/src/chat/recipes/git-log.ts +++ b/client/cody-shared/src/chat/recipes/git-log.ts @@ -7,7 +7,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS } from '../../prompt/constants' import { truncateText } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { Recipe } from './recipe' @@ -76,7 +75,7 @@ export class GitHistory implements Recipe { const promptMessage = `Summarize these commits:\n${truncatedGitLogOutput}\n\nProvide your response in the form of a bulleted list. Do not mention the commit hashes.` const assistantResponsePrefix = 'Here is a summary of recent changes:\n- ' return new Interaction( - { speaker: 'human', text: promptMessage, displayText: renderMarkdown(rawDisplayText), timestamp }, + { speaker: 'human', text: promptMessage, displayText: rawDisplayText, timestamp }, { speaker: 'assistant', prefix: assistantResponsePrefix, diff --git a/client/cody-shared/src/chat/recipes/improve-variable-names.ts b/client/cody-shared/src/chat/recipes/improve-variable-names.ts index a55b524012c..2275d06e64f 100644 --- a/client/cody-shared/src/chat/recipes/improve-variable-names.ts +++ b/client/cody-shared/src/chat/recipes/improve-variable-names.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS, MAX_RECIPE_SURROUNDING_TOKENS } from '../../prompt/constants' import { truncateText, truncateTextStart } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { @@ -37,9 +36,7 @@ export class ImproveVariableNames implements Recipe { const truncatedFollowingText = truncateText(selection.followingText, MAX_RECIPE_SURROUNDING_TOKENS) const extension = getFileExtension(selection.fileName) - const displayText = renderMarkdown( - `Improve the variable names in the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\`` - ) + const displayText = `Improve the variable names in the following code:\n\`\`\`\n${selection.selectedText}\n\`\`\`` const languageName = getNormalizedLanguageName(selection.fileName) const promptMessage = `Improve the variable names in this ${languageName} code by replacing the variable names with new identifiers which succinctly capture the purpose of the variable. We want the new code to be a drop-in replacement, so do not change names bound outside the scope of this code, like function names or members defined elsewhere. Only change the names of local variables and parameters:\n\n\`\`\`${extension}\n${truncatedSelectedText}\n\`\`\`\n${MARKDOWN_FORMAT_PROMPT}` diff --git a/client/cody-shared/src/chat/recipes/translate.ts b/client/cody-shared/src/chat/recipes/translate.ts index 54299277ae4..7b420815867 100644 --- a/client/cody-shared/src/chat/recipes/translate.ts +++ b/client/cody-shared/src/chat/recipes/translate.ts @@ -4,7 +4,6 @@ import { IntentDetector } from '../../intent-detector' import { MAX_RECIPE_INPUT_TOKENS } from '../../prompt/constants' import { truncateText } from '../../prompt/truncation' import { getShortTimestamp } from '../../timestamp' -import { renderMarkdown } from '../markdown' import { Interaction } from '../transcript/interaction' import { languageMarkdownID, languageNames } from './langs' @@ -37,9 +36,7 @@ export class TranslateToLanguage implements Recipe { const truncatedSelectedText = truncateText(selection.selectedText, MAX_RECIPE_INPUT_TOKENS) const promptMessage = `Translate the following code into ${toLanguage}\n\`\`\`\n${truncatedSelectedText}\n\`\`\`` - const displayText = renderMarkdown( - `Translate the following code into ${toLanguage}\n\`\`\`\n${selection.selectedText}\n\`\`\`` - ) + const displayText = `Translate the following code into ${toLanguage}\n\`\`\`\n${selection.selectedText}\n\`\`\`` const markdownID = languageMarkdownID[toLanguage] || '' const assistantResponsePrefix = `Here is the code translated to ${toLanguage}:\n\`\`\`${markdownID}\n` diff --git a/client/cody-shared/src/chat/transcript/index.ts b/client/cody-shared/src/chat/transcript/index.ts index 53066124e83..a9b890126f4 100644 --- a/client/cody-shared/src/chat/transcript/index.ts +++ b/client/cody-shared/src/chat/transcript/index.ts @@ -19,11 +19,11 @@ export class Transcript { return this.interactions.length > 0 ? this.interactions[this.interactions.length - 1] : null } - public addAssistantResponse(text: string, displayText: string): void { + public addAssistantResponse(text: string): void { this.getLastInteraction()?.setAssistantMessage({ speaker: 'assistant', text, - displayText, + displayText: text, timestamp: getShortTimestamp(), }) } diff --git a/client/cody-shared/src/chat/transcript/transcript.test.ts b/client/cody-shared/src/chat/transcript/transcript.test.ts index 740e759ddf5..b4a3c6ac981 100644 --- a/client/cody-shared/src/chat/transcript/transcript.test.ts +++ b/client/cody-shared/src/chat/transcript/transcript.test.ts @@ -95,7 +95,7 @@ describe('Transcript', () => { transcript.addInteraction(firstInteraction) const assistantResponse = 'By setting the Authorization header.' - transcript.addAssistantResponse(assistantResponse, assistantResponse) + transcript.addAssistantResponse(assistantResponse) const secondInteraction = await chatQuestionRecipe.getInteraction( 'how to create a batch change', @@ -135,7 +135,7 @@ describe('Transcript', () => { transcript.addInteraction(interaction) const assistantResponse = 'EFGH'.repeat(256) // 256 tokens - transcript.addAssistantResponse(assistantResponse, assistantResponse) + transcript.addAssistantResponse(assistantResponse) } const tokensPerInteraction = 512 // 256 for question + 256 for response. diff --git a/client/cody-shared/tsconfig.json b/client/cody-shared/tsconfig.json index f8d224ed2d4..db2fea3039c 100644 --- a/client/cody-shared/tsconfig.json +++ b/client/cody-shared/tsconfig.json @@ -8,4 +8,5 @@ }, "include": ["**/*", ".*"], "exclude": ["out"], + "references": [{ "path": "../common" }], } diff --git a/client/cody/src/chat/ChatViewProvider.ts b/client/cody/src/chat/ChatViewProvider.ts index 0b693ec2157..e706744fec7 100644 --- a/client/cody/src/chat/ChatViewProvider.ts +++ b/client/cody/src/chat/ChatViewProvider.ts @@ -1,7 +1,6 @@ import * as vscode from 'vscode' import { ChatClient } from '@sourcegraph/cody-shared/src/chat/chat' -import { renderMarkdown } from '@sourcegraph/cody-shared/src/chat/markdown' import { getRecipe } from '@sourcegraph/cody-shared/src/chat/recipes' import { Transcript } from '@sourcegraph/cody-shared/src/chat/transcript' import { ChatMessage } from '@sourcegraph/cody-shared/src/chat/transcript/messages' @@ -214,7 +213,7 @@ export class ChatViewProvider implements vscode.WebviewViewProvider { } private async onBotMessageChange(text: string): Promise { - this.transcript.addAssistantResponse(text, renderMarkdown(text)) + this.transcript.addAssistantResponse(text) await this.sendTranscript() } diff --git a/client/cody/src/integration-test/extension.test.ts b/client/cody/src/integration-test/extension.test.ts index 2caf36c4b58..4b914a57baa 100644 --- a/client/cody/src/integration-test/extension.test.ts +++ b/client/cody/src/integration-test/extension.test.ts @@ -79,8 +79,8 @@ suite('End-to-end', () => { // Check the chat transcript contains markdown const message = await getTranscript(api, 0) - assert.ok(message.displayText.startsWith('

Explain the following code')) - assert.ok(message.displayText.includes('public')) + assert.match(message.displayText, /^Explain the following code/) + assert.match(message.displayText, /public/) // Check the server response was handled // "hello world" is a canned response from the server @@ -90,7 +90,7 @@ suite('End-to-end', () => { return assistantMessage.displayText.length > 0 }) const assistantMessage = await getTranscript(api, 1) - assert.ok(assistantMessage.displayText.includes('hello, world')) + assert.match(assistantMessage.displayText, /hello, world/) // Clean up. await ensureExecuteCommand('cody.delete-access-token') diff --git a/client/cody/src/integration-test/index.ts b/client/cody/src/integration-test/index.ts index 111ae3febbe..734824cd05c 100644 --- a/client/cody/src/integration-test/index.ts +++ b/client/cody/src/integration-test/index.ts @@ -10,7 +10,7 @@ export function run(): Promise { color: true, }) // To debug tests interactively, extend this timeout. - mocha.timeout(2000) + mocha.timeout(15000) const testsRoot = __dirname diff --git a/client/cody/webviews/Chat.tsx b/client/cody/webviews/Chat.tsx index a4a1343662a..df578c13621 100644 --- a/client/cody/webviews/Chat.tsx +++ b/client/cody/webviews/Chat.tsx @@ -6,6 +6,8 @@ import React, { useCallback, useEffect, useRef, useState } from 'react' import { VSCodeButton, VSCodeTextArea } from '@vscode/webview-ui-toolkit/react' +import { renderMarkdown } from '@sourcegraph/cody-shared/src/chat/markdown' + import { Tips } from './Tips' import { SubmitSvg } from './utils/icons' import { ChatMessage } from './utils/types' @@ -118,7 +120,11 @@ export const Chat: React.FunctionComponent className={`bubble-content ${bubbleClassName(message.speaker)}-bubble-content`} > {message.displayText && ( -

+

)} {message.contextFiles && message.contextFiles.length > 0 && ( @@ -138,7 +144,11 @@ export const Chat: React.FunctionComponent

{messageInProgress.displayText ? ( -

+

) : (

diff --git a/client/common/src/util/markdown/markdown.test.ts b/client/common/src/util/markdown/markdown.test.ts index cb00d766f0e..444225b0e90 100644 --- a/client/common/src/util/markdown/markdown.test.ts +++ b/client/common/src/util/markdown/markdown.test.ts @@ -69,40 +69,35 @@ describe('renderMarkdown', () => { B -

\\"image

- " +

\\"image

" `) }) it('renders to plain text with plainText: true', () => { - expect(renderMarkdown('A **b**', { plainText: true })).toBe('A b\n') + expect(renderMarkdown('A **b**', { plainText: true })).toBe('A b') }) it('sanitizes script tags', () => { expect(renderMarkdown('')).toBe('') }) it('sanitizes event handlers', () => { - expect(renderMarkdown('')).toBe('

\n') + expect(renderMarkdown('test')).toBe('

test

') }) it('does not allow arbitrary tags', () => { - expect(renderMarkdown('')).toBe('

\n') + expect(renderMarkdown('')).toBe('

') }) it('drops SVG tags', () => { - expect(renderMarkdown('')).toBe('

\n') + expect(renderMarkdown('')).toBe('

') }) - it('allows tags', () => { + it('forbids tags', () => { const input = '/' - expect(renderMarkdown(input)).toBe(`

${input}

\n`) + expect(renderMarkdown(input)).toBe('

') }) - - describe('allowDataUriLinksAndDownloads option', () => { - const MARKDOWN_WITH_DOWNLOAD = 'D\n[D2](data:text/plain,foobar)' - test('default disabled', () => { - expect(renderMarkdown(MARKDOWN_WITH_DOWNLOAD)).toBe('

D\nD2

\n') - }) - test('enabled', () => { - expect(renderMarkdown(MARKDOWN_WITH_DOWNLOAD, { allowDataUriLinksAndDownloads: true })).toBe( - '

D\nD2

\n' - ) - }) + it('forbids rel and style attributes', () => { + const input = 'Link' + expect(renderMarkdown(input)).toBe('

Link

') + }) + test('forbids data URI links', () => { + const input = 'D\n[D2](data:text/plain,foobar)' + expect(renderMarkdown(input)).toBe('

D\nD2

') }) }) diff --git a/client/common/src/util/markdown/markdown.ts b/client/common/src/util/markdown/markdown.ts index dfaccd4579a..15da9459982 100644 --- a/client/common/src/util/markdown/markdown.ts +++ b/client/common/src/util/markdown/markdown.ts @@ -1,10 +1,8 @@ +import DOMPurify, { Config as DOMPurifyConfig } from 'dompurify' import { highlight, highlightAuto } from 'highlight.js/lib/core' -import { without } from 'lodash' // This is the only file allowed to import this module, all other modules must use renderMarkdown() exported from here // eslint-disable-next-line no-restricted-imports import { marked } from 'marked' -import sanitize from 'sanitize-html' -import { Overwrite } from 'utility-types' import { logger } from '../logger' @@ -44,15 +42,11 @@ export const highlightCodeSafe = (code: string, language?: string): string => { } } -const svgPresentationAttributes = ['fill', 'stroke', 'stroke-width'] as const -const ALL_VALUES_ALLOWED = [/.*/] - /** * Renders the given markdown to HTML, highlighting code and sanitizing dangerous HTML. * Can throw an exception on parse errors. * * @param markdown The markdown to render - * @param options Options object for passing additional parameters */ export const renderMarkdown = ( markdown: string, @@ -63,19 +57,9 @@ export const renderMarkdown = ( disableAutolinks?: boolean renderer?: marked.Renderer headerPrefix?: string - } & ( - | { - /** Strip off any HTML and return a plain text string, useful for previews */ - plainText: true - } - | { - /** Following options apply to non-plaintext output only. */ - plainText?: false - - /** Allow links to data: URIs and that download files. */ - allowDataUriLinksAndDownloads?: boolean - } - ) = {} + /** Strip off any HTML and return a plain text string, useful for previews */ + plainText?: boolean + } = {} ): string => { const tokenizer = new marked.Tokenizer() if (options.disableAutolinks) { @@ -96,94 +80,18 @@ export const renderMarkdown = ( tokenizer, }) - let sanitizeOptions: Overwrite - if (options.plainText) { - sanitizeOptions = { - ...sanitize.defaults, - allowedAttributes: {}, - allowedSchemes: [], - allowedSchemesByTag: {}, - allowedTags: [], - selfClosing: [], - } - } else { - sanitizeOptions = { - ...sanitize.defaults, - // Ensure must have type attribute set - exclusiveFilter: ({ tag, attribs }) => tag === 'object' && !attribs.type, + const dompurifyConfig: DOMPurifyConfig & { RETURN_DOM_FRAGMENT?: false; RETURN_DOM?: false } = options.plainText + ? { + ALLOWED_TAGS: [], + ALLOWED_ATTR: [], + KEEP_CONTENT: true, + } + : { + USE_PROFILES: { html: true }, + FORBID_ATTR: ['rel', 'style'], + } - allowedTags: [ - ...without(sanitize.defaults.allowedTags, 'iframe'), - 'img', - 'picture', - 'source', - 'svg', - 'rect', - 'defs', - 'pattern', - 'mask', - 'circle', - 'path', - 'title', - ], - allowedAttributes: { - ...sanitize.defaults.allowedAttributes, - a: [ - ...sanitize.defaults.allowedAttributes.a, - 'title', - 'class', - { name: 'rel', values: ['noopener', 'noreferrer'] }, - ], - img: [...sanitize.defaults.allowedAttributes.img, 'alt', 'width', 'height', 'align', 'style'], - // Support different images depending on media queries (e.g. color theme, reduced motion) - source: ['srcset', 'media'], - svg: ['width', 'height', 'viewbox', 'version', 'preserveaspectratio', 'style'], - rect: ['x', 'y', 'width', 'height', 'transform', ...svgPresentationAttributes], - path: ['d', ...svgPresentationAttributes], - circle: ['cx', 'cy', ...svgPresentationAttributes], - pattern: ['id', 'width', 'height', 'patternunits', 'patterntransform'], - mask: ['id'], - // Allow highligh.js styles, e.g. - // - // - span: ['class'], - code: ['class'], - // Support deep-linking - h1: ['id'], - h2: ['id'], - h3: ['id'], - h4: ['id'], - h5: ['id'], - h6: ['id'], - }, - allowedStyles: { - img: { - padding: ALL_VALUES_ALLOWED, - 'padding-left': ALL_VALUES_ALLOWED, - 'padding-right': ALL_VALUES_ALLOWED, - 'padding-top': ALL_VALUES_ALLOWED, - 'padding-bottom': ALL_VALUES_ALLOWED, - }, - // SVGs are usually for charts in code insights. - // Allow them to be responsive. - svg: { - flex: ALL_VALUES_ALLOWED, - 'flex-grow': ALL_VALUES_ALLOWED, - 'flex-shrink': ALL_VALUES_ALLOWED, - 'flex-basis': ALL_VALUES_ALLOWED, - }, - }, - } - if (options.allowDataUriLinksAndDownloads) { - sanitizeOptions.allowedAttributes.a = [...sanitizeOptions.allowedAttributes.a, 'download'] - sanitizeOptions.allowedSchemesByTag = { - ...sanitizeOptions.allowedSchemesByTag, - a: [...(sanitizeOptions.allowedSchemesByTag.a || sanitizeOptions.allowedSchemes), 'data'], - } - } - } - - return sanitize(rendered, sanitizeOptions) + return DOMPurify.sanitize(rendered, dompurifyConfig).trim() } export const markdownLexer = (markdown: string): marked.TokensList => marked.lexer(markdown) diff --git a/client/shared/src/hover/__snapshots__/HoverOverlay.test.tsx.snap b/client/shared/src/hover/__snapshots__/HoverOverlay.test.tsx.snap index b7b4ae77722..d7a972cd122 100644 --- a/client/shared/src/hover/__snapshots__/HoverOverlay.test.tsx.snap +++ b/client/shared/src/hover/__snapshots__/HoverOverlay.test.tsx.snap @@ -98,8 +98,6 @@ exports[`HoverOverlay actions and hover present 1`] = `

v

- -
v

- -
v

- -
v

- -
v

- -
v2

- -
a

- -
a

- -