Skip to content

fix: extract HTMLBlocks before parsing in stripComments - #1506

Merged
jarrod-lyra merged 6 commits into
nextfrom
jarrod/rm-16888-fix-stripcomments-htmlblock-crash
Jul 7, 2026
Merged

fix: extract HTMLBlocks before parsing in stripComments#1506
jarrod-lyra merged 6 commits into
nextfrom
jarrod/rm-16888-fix-stripcomments-htmlblock-crash

Conversation

@jarrod-lyra

@jarrod-lyra jarrod-lyra commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
🎫 Resolve RM-16888

🎯 What does this PR do?

stripComments with mdxish: true crashes on pages with multiline HTMLBlock content. CommonMark's htmlFlow intercepts unindented HTML tags inside the template literal, breaking the MDX expression parsing.

Fix:

Add the HTMLBlock micromark tokenizer from PR #1439 to claim <HTMLBlock> before htmlFlow can. Same pattern as jsxTable.

Files from #1439:

  • lib/micromark/html-block-component/syntax.ts
  • lib/micromark/html-block-component/index.ts
  • lib/mdast-util/html-block-component/index.ts

🧪 QA tips

For testing ticket behavior:

  • In the main app (ReadMe): create a page with a multiline HTMLBlock, and verify the .md endpoint returns markdown (not a full HTML page)

📸 Screenshot or Loom

Screen.Recording.2026-06-03.at.4.35.50.pm.mp4

github-advanced-security[bot]
github-advanced-security AI found potential problems Jun 3, 2026
View reviewed changes
Comment thread lib/stripComments.ts Fixed
@jarrod-lyra
fix(stripComments): add HTMLBlock tokenizer to prevent htmlFlow crash (…
984be17 
…RM-16888)

Adds the HTMLBlock micromark tokenizer from PR #1439 to the stripComments
pipeline, preventing multiline HTMLBlock content from crashing the parser
when htmlFlow intercepts inner HTML tags.
@jarrod-lyra
Merge branch 'next' of https://github.com/readmeio/markdown into jarrod/
cff52f2 
rm-16888-fix-stripcomments-htmlblock-crash
@jarrod-lyra
fix(stripComments): preserve HTML comments inside HTMLBlock content
301ca33 
HTMLBlock template literals contain authored HTML that should not have
comments stripped. Extracts HTMLBlocks before stripping and restores them
after, handling both top-level and div-wrapped HTMLBlocks.
@jarrod-lyra
Merge branch 'next' of https://github.com/readmeio/markdown into jarrod/
f32201f 
rm-16888-fix-stripcomments-htmlblock-crash
@jarrod-lyra
jarrod-lyra force-pushed the jarrod/rm-16888-fix-stripcomments-htmlblock-crash branch from 15f3ae6 to f32201f Compare June 5, 2026 05:50
github-advanced-security[bot]
github-advanced-security AI found potential problems Jun 5, 2026
View reviewed changes
Comment thread processor/transform/stripComments.ts Fixed
eaglethrost
eaglethrost reviewed Jun 5, 2026
View reviewed changes
Comment thread processor/transform/stripComments.ts

@eaglethrost eaglethrost Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why do we need to preserve comments in HTMLBlocks? I thought the point of this is to strip every comment, and also makes this code more complicated with these regexes.

If we do need to preserve them, then I don't think we need to extract the HTMLBlocks again? Since we already tokenise them, they should appear is 1 HTML node, so maybe we can just check if a visited node has <HTMLBlock> pattern, you can skip the comment stripping. Let me know if it makes sense.

@jarrod-lyra jarrod-lyra Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was leaning towards preserving comments after checking out your work in #1410, but CX-2511 doesn't say to strip them or not. That being said, if stripping comments in HTMLBlocks is fine, I'd rather do that.

@eaglethrost eaglethrost Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, comments get preserved in there & for magic blocks as well. I agree, I think stripping it in HTMLBlocks too sounds better, but would be good to confirm first.

@kevinports kevinports Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep for the stripComments pipeline we want to sanitize the entire page content of all comments, including HTMLBlocks.

@eaglethrost

eaglethrost commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Good use of the tokenizer!

@jarrod-lyra
Merge branch 'next' of github.com:readmeio/markdown into jarrod/rm-16888
66462ce 
-fix-stripcomments-htmlblock-crash
@jarrod-lyra
fix(stripComments): strip comments inside HTMLBlocks too
8da3726 
Remove the extract/restore logic that preserved HTML comments inside
HTMLBlock content. Comments are now stripped uniformly everywhere,
which is simpler and consistent with the security intent of CX-2511.
@jarrod-lyra
jarrod-lyra requested a review from rafegoldberg June 9, 2026 11:54
@jarrod-lyra
jarrod-lyra marked this pull request as ready for review June 9, 2026 11:54
@jarrod-lyra
jarrod-lyra requested a review from a team June 9, 2026 11:54
@gkoberger
gkoberger force-pushed the jarrod/rm-16888-fix-stripcomments-htmlblock-crash branch from 8da3726 to c514139 Compare June 11, 2026 07:00
@jboyens
jboyens force-pushed the jarrod/rm-16888-fix-stripcomments-htmlblock-crash branch from c514139 to 8da3726 Compare June 11, 2026 07:41
@erunion
erunion removed the request for review from a team June 16, 2026 21:08
@jarrod-lyra
jarrod-lyra requested a review from kevinports June 25, 2026 12:11
eaglethrost
eaglethrost approved these changes Jul 7, 2026
View reviewed changes

@eaglethrost eaglethrost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm but good get a readme approval as well!

kevinports
kevinports approved these changes Jul 7, 2026
View reviewed changes

@kevinports kevinports left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm.

@jarrod-lyra
jarrod-lyra merged commit 2c65c3f into next Jul 7, 2026
8 checks passed
@jarrod-lyra
jarrod-lyra deleted the jarrod/rm-16888-fix-stripcomments-htmlblock-crash branch July 7, 2026 23:42
rafegoldberg pushed a commit that referenced this pull request Jul 9, 2026
@semantic-release-bot
build(release): 🚀 v14.11.3 🦉
0ac6a0c 
## Version 14.11.3
### 🛠 Fixes & Updates

* **mdxish:** escape stray `<` in table cells ([#1535](#1535)) ([807f61a](807f61a))
* esm statements in JSX component child text crash rendering ([#1541](#1541)) ([6347bab](6347bab))
* extract HTMLBlocks before parsing in stripComments ([#1506](#1506)) ([2c65c3f](2c65c3f)), closes [#1439](#1439)
* release workflow breakage from npm engine drift ([#1542](#1542)) ([2288aa7](2288aa7))

<!--SKIP CI-->
@rafegoldberg

rafegoldberg commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

This PR was released!

🚀 Changes included in v14.11.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinports kevinports kevinports approved these changes
@eaglethrost eaglethrost eaglethrost approved these changes
@maximilianfalco maximilianfalco Awaiting requested review from maximilianfalco
@rafegoldberg rafegoldberg Awaiting requested review from rafegoldberg
+1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jarrod-lyra @eaglethrost @rafegoldberg @kevinports @github-advanced-security