feat(preview): add plugins for enhanced markdown preview#6
feat(preview): add plugins for enhanced markdown preview#6kerwin2046 wants to merge 2 commits intoBunsDev:mainfrom
Conversation
|
@kerwin2046 is attempting to deploy a commit to the 0xBuns Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Code Review
This pull request introduces several new plugins for the markdown previewer, including KaTeX for math rendering, a copy-to-clipboard button for code blocks, heading anchors, and a table of contents generator. Feedback highlights critical security concerns regarding XSS vulnerabilities in the copy-button and KaTeX plugins due to unescaped user input. Additionally, there are logic errors in HTML attribute injection for heading anchors, concurrency issues with shared state in the TOC plugin, and accessibility concerns regarding the TOC's flat list structure.
| if (textEl) textEl.textContent = '${opts.copiedText}'; | ||
| btn.setAttribute('data-copied', 'true'); | ||
| setTimeout(function() { | ||
| var textEl = btn.querySelector('.${copyTextClass}'); | ||
| if (textEl) textEl.textContent = '${opts.buttonText}'; | ||
| btn.removeAttribute('data-copied'); | ||
| }, 2000); | ||
| }).catch(function() { | ||
| var textEl = btn.querySelector('.${copyTextClass}'); | ||
| if (textEl) textEl.textContent = 'Failed'; |
There was a problem hiding this comment.
There is a potential XSS vulnerability here. The opts.copiedText, opts.buttonText, and opts.classPrefix values are injected directly into a script string without escaping. If these options are ever derived from user-provided configuration, an attacker could inject arbitrary JavaScript by using single quotes and semicolons (e.g., '); alert(1); //).
Since these values are used as string literals in the generated JavaScript, you should at least escape single quotes or use JSON.stringify() to safely embed the strings.
|
|
||
| if (parts.length !== 2) return null; | ||
|
|
||
| return `${parts[0]}${idAttr}${anchorHtml}${closeTag}${parts[1]}`; |
There was a problem hiding this comment.
The logic for injecting the id attribute is incorrect and produces invalid HTML. By splitting on the closing tag and appending idAttr to parts[0], the attribute is placed inside the tag's content rather than within the opening tag. For example, <h1>Title</h1> becomes <h1>Title id="slug"<a...>...</a></h1>.
You should use a regex to inject the id into the opening tag.
const htmlWithId = defaultHtml.replace(/^<h[1-6]/i, (m) => `${m}${idAttr}`);
const closeTag = `</h${headingBlock.props.level}>`;
return htmlWithId.replace(new RegExp(closeTag, 'i'), `${anchorHtml}${closeTag}`);| macros: opts.macros, | ||
| }); | ||
| } catch { | ||
| return `<span class="${prefix}katex-error">${expr}</span>`; |
There was a problem hiding this comment.
| const opts = { ...DEFAULT_OPTIONS, ...options }; | ||
| const prefix = opts.classPrefix; | ||
|
|
||
| const tocItems: TocItem[] = []; |
There was a problem hiding this comment.
The tocItems array is shared across all renders using this plugin instance. Since renderAsync is asynchronous, concurrent calls using the same plugin instance will lead to race conditions where headings from different documents are mixed together or cleared prematurely.
To fix this, consider passing a context object through the rendering pipeline or ensuring that state is scoped to a single render cycle.
packages/preview/src/plugins/toc.ts
Outdated
| tocItems.push({ | ||
| level, | ||
| text, | ||
| anchor: slugify(text), |
There was a problem hiding this comment.
The slugify implementation here does not support an anchorPrefix, whereas the headingAnchorsPlugin does. If a user configures a prefix for their heading anchors, the TOC links generated by this plugin will be broken because they won't include that prefix. These two plugins should share a consistent slugification utility and configuration.
| includeShikiBlocks: true, | ||
| }; | ||
|
|
||
| let scriptInjected = false; |
There was a problem hiding this comment.
The scriptInjected flag is defined at the module level. This causes issues if multiple preview instances are used on the same page with different classPrefix values. The script will only be injected once using the configuration of the first plugin instance. Subsequent instances with different prefixes will have buttons that do not match the event listener's selector, rendering them non-functional.
packages/preview/src/plugins/toc.ts
Outdated
| const indent = (item.level - parentLevel) * opts.indentWidth; | ||
| const indentStyle = indent > 0 ? ` style="padding-left: ${indent}em"` : ''; | ||
|
|
||
| result += `<li class="${prefix}${opts.itemClass}"${indentStyle}> | ||
| <a class="${prefix}${opts.linkClass}" href="#${item.anchor}">${escapeHtml(item.text)}</a> | ||
| </li>\n`; |
There was a problem hiding this comment.
- copy-button: escape strings for JS to prevent XSS - heading-anchors: fix HTML injection logic for id attribute - katex: escape error fallback expressions to prevent XSS - toc: use nested <ul> for accessibility, add anchorPrefix option - copy-button: use Set to support multiple instances with different prefixes
Summary
Add four new preview plugins for enhanced markdown rendering:
copyButtonPlugin- Adds copy-to-clipboard button to code blocks with hover reveal and "Copied!" feedbackheadingAnchorsPlugin- Adds anchor links to headings (# icon) for deep linking and table of contents navigationkatexPlugin- Math rendering support using KaTeX for$inline$and$$block$$expressionstocPlugin- Automatic table of contents generation from headings, withextractToc()andrenderToc()utility functionsMotivation
These plugins address frequently requested features from the community (see ROADMAP.md):
Implementation
All plugins follow the existing
PreviewPlugininterface:classPrefixoption (default:cm-)Usage