feat: add opt-in sanitize option to strip script tags - #1574
feat: add opt-in sanitize option to strip script tags#1574RAYMOND-LUO wants to merge 17 commits into
Conversation
…s' into raymond/cx-3169-strip-script-tags
WalkthroughAdded an optional Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/plugins/strip-tags.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Error while loading rule ' Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
processor/plugin/strip-scripts.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the lowercase-name regex.
isScriptNodecreates/^[a-z]/for every visited MDX JSX node. Declare it once at module scope.Proposed change
+const lowercaseFirstLetter = /^[a-z]/; + const isScriptNode = (node: Nodes): boolean => { if (node.type === 'element') return node.tagName.toLowerCase() === 'script'; if (node.type === 'mdxJsxFlowElement' || node.type === 'mdxJsxTextElement') { const name = node.name ?? ''; - return /^[a-z]/.test(name) && name.toLowerCase() === 'script'; + return lowercaseFirstLetter.test(name) && name.toLowerCase() === 'script'; }As per coding guidelines, “Hoist invariants such as regexes, constants, interfaces, and
unified()processors to module scope.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@processor/plugin/strip-scripts.ts` around lines 13 - 18, Hoist the invariant lowercase-name regex used by isScriptNode to module scope, then reuse that single regex when checking MDX JSX node names. Keep the existing script-node detection behavior unchanged.Source: Coding guidelines
__tests__/plugins/strip-scripts.test.tsx (1)
64-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the
mdxJsxTextElementbranch and parser boundaries.This test covers
mdxJsxFlowElement, butisScriptNodehas a separatemdxJsxTextElementbranch. Add lowercase, mixed-case, and uppercase text-node cases.Also add escaped-tag, blank-line, and surrounding-whitespace cases through each configured RMDX and MDXish engine.
As per coding guidelines, “Unit-test dense logic directly rather than testing it only through a consumer” and “Cover both RMDX and MDXish engines and test edge cases including … escapes, nesting, blank lines, whitespace, and formatting differences.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/plugins/strip-scripts.test.tsx` around lines 64 - 80, Extend the strip-scripts tests to cover mdxJsxTextElement nodes with lowercase, mixed-case, and uppercase script names, verifying only literal script nodes are removed while Script references remain. Add parser-boundary cases for escaped tags, blank lines, and surrounding whitespace across every configured RMDX and MDXish engine, including nesting and formatting variations where applicable. Exercise the dense isScriptNode logic directly in addition to consumer-level coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/plugins/strip-scripts.test.tsx`:
- Around line 64-80: Extend the strip-scripts tests to cover mdxJsxTextElement
nodes with lowercase, mixed-case, and uppercase script names, verifying only
literal script nodes are removed while Script references remain. Add
parser-boundary cases for escaped tags, blank lines, and surrounding whitespace
across every configured RMDX and MDXish engine, including nesting and formatting
variations where applicable. Exercise the dense isScriptNode logic directly in
addition to consumer-level coverage.
In `@processor/plugin/strip-scripts.ts`:
- Around line 13-18: Hoist the invariant lowercase-name regex used by
isScriptNode to module scope, then reuse that single regex when checking MDX JSX
node names. Keep the existing script-node detection behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e12884a-be74-4b02-9e68-092d4b349f60
📒 Files selected for processing (5)
__tests__/lib/mdxish/markdown-inside-single-line-html.test.ts__tests__/plugins/strip-scripts.test.tsxlib/compile.tslib/mdxish.tsprocessor/plugin/strip-scripts.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/plugins/strip-scripts.test.tsx`:
- Around line 105-110: Update the test around execute and appendMarker to
include a literal script element in the Markdown/MDX input, then assert the
rendered html excludes both the script markup and its content while retaining
the existing caller-plugin and heading-ID assertions. This must verify
rehypeStripScripts remains active when rehypePlugins is nonempty.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eef66dd-9883-4f1e-b1e2-f45207178918
📒 Files selected for processing (3)
__tests__/plugins/strip-scripts.test.tsxlib/compile.tsprocessor/plugin/strip-scripts.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- processor/plugin/strip-scripts.ts
- lib/compile.ts
eaglethrost
left a comment
eaglethrost
left a comment
There was a problem hiding this comment.
I think we should not strip unconditionally and should have an option for it, otherwise a lot of customer scripts suddenly would stop working. Have some comments as well!
There was a problem hiding this comment.
Can we make this plugin more general e.g. strip-tags, and then have script as one of the tags stripped at the moment so we can extend it to more tags?
| import { renderingEngines } from '../components/utils'; | ||
| import { execute, findElementByTagName } from '../helpers'; | ||
|
|
||
| const engines = [ |
There was a problem hiding this comment.
Nit: We have a renderingEngines variable for tests in __tests__/components/utils., can we move that to __tests__/helperts.ts and reuse it?
There was a problem hiding this comment.
Ahh I see, although it does touch imports for a bunch of files and adds noise here. Also this test specifically needs the new flag turned on sanitize: true and the shared renderingEngines takes no options. Happy either way - what do u think?
There was a problem hiding this comment.
Ah yeah that’s fine to keep then
There was a problem hiding this comment.
Overall comments:
- Nit: Can we wrap all of these tests in 1 big describe statement? Just for convention
- Can we add tests for when the script are inside tables, comments, and custom components?
Renames the plugin to strip-tags and drives it from a STRIPPED_TAG_NAMES set holding only `script` for now, so future vectors are a one-line change. Both pipelines default sanitize to false, preserving current rendering until a project opts in.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/plugins/strip-tags.test.tsx (1)
33-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd pipeline-level edge-case coverage.
The direct transformer test covers
<sCrIpT>, but no MDX or MDXish test passes that input through the full pipeline. Add table-driven cases for mixed-case tags, escaped tag text, emphasis, and whitespace or blank-line preservation. Cite CX-3169 in the regression suite.As per coding guidelines, cover both engines and test emphasis variants, escapes, nesting, blank lines, whitespace, and formatting differences.
Also applies to: 204-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/plugins/strip-tags.test.tsx` around lines 33 - 74, Expand the parameterized “strip tags” coverage in the describe.each(engines) suite to exercise the full MDX/MDXish render pipeline with mixed-case script tags, escaped tag text, emphasis variants, nested content, blank lines, whitespace, and formatting differences. Preserve the expected stripping or preservation behavior for each case, cover both engines through the existing table, and cite CX-3169 in the regression tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/plugins/strip-tags.test.tsx`:
- Around line 33-74: Expand the parameterized “strip tags” coverage in the
describe.each(engines) suite to exercise the full MDX/MDXish render pipeline
with mixed-case script tags, escaped tag text, emphasis variants, nested
content, blank lines, whitespace, and formatting differences. Preserve the
expected stripping or preservation behavior for each case, cover both engines
through the existing table, and cite CX-3169 in the regression tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02368f84-b12f-4a37-9eac-f86bae27d816
📒 Files selected for processing (4)
__tests__/plugins/strip-tags.test.tsxlib/compile.tslib/mdxish.tsprocessor/plugin/strip-tags.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
RAYMOND-LUO
changed the title
|
Heads up on a change of approach here. Rather than stripping for everyone, this is now gated behind a sanitize option that defaults to off, so nothing changes for existing projects. we don't know how many customers rely on raw html or script tags in their docs, so flipping it globally was too risky. The opt-in lives as a god mode project flag, so we can turn it on per customer for the ones asking for sanitisation:
Both are drafts and depend on this one landing first for the rmdx bump. Full sanitisation (javascript: urls, iframes, etc) is still in #1526's for a follow-up. This only covers script tags, and the plugin is now driven by a cc @flinehan |
2 participants
🎯 What does this PR do?
A raw
<script>alert(1)</script>in page content ends up verbatim in server-rendered HTML and executes on page load. Neither engine sanitizes it: in MDX, a literal<script>parses as JSX rather than raw HTML, so theformat: 'md'-onlyrehypeSanitizestep never sees it; in MDX-ish, the pipeline runsrehypeRawbut has no sanitize step at all. Legacy rdmd always stripped script tags via its sanitize schema, so this closes that one gap.Scope: this is deliberately narrow. It strips
<script>and nothing else, so it does not bring the two engines to full parity with themdsanitize schema.javascript:URLs,on*handlers,<iframe>,<object>and friends still render onmdxandmdxishwhilemdblocks them. Full sanitization for both engines is being handled separately in #1526; this lands the script fix without waiting on that larger change.Opt-in:
sanitizedefaults tofalse, so this PR changes nothing for current callers. ReadMe turns it on per project via a god-mode flag (readmeio/readme#20204), forwarded by readmeio/mdx-renderer#341.processor/plugin/strip-tags.tsrehype plugin removes any node that would render one of the literal DOM tags in itsSTRIPPED_TAG_NAMESset (justscripttoday, so future vectors are a one-line change):elementnodes with ascripttag name, and mdx-jsx nodes whose name is a lowercase-first case-variant ofscript(browsers parse tag names case-insensitively, so<sCrIpT>would execute). Capitalized<Script>component references are left alone.sanitizeoption:lib/compile.ts(all formats) andlib/mdxish.ts(afternormalizeMdxJsxNodes).HTMLBlocks are intentionally exempt, matching legacy: their scripts live in a string prop, never reach SSR HTML, and only run client-side behindrunScripts.🧪 QA tips
<script>alert(1)</script>to a page body and render via either engine withsanitize: true: no alert, no<script>in output HTML, surrounding content intact📸 Screenshot or Loom
Before
alertBefore.mov
After
alertAfter.mov