Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Markdown: Fixed markdown not working in NodeJS #2977

Merged
merged 1 commit into from Jul 2, 2021

Conversation

RunDevelopment
Copy link
Member

Markdown crashed on Node. The code-block-highlighting relyed on browser DOM to get the text content of an HTML string (see wrap hook function).

The fix employed by this PR is to replace the DOM-based approach with a custom function: textcontent(html: string). The function is implemented using Prism's markup tag regex and some custom logic to unescape HTML entities I stole (not literally) from lowdash. It works pretty well on Node and in browsers (notably IE11).

I also contemplated detecting whether we have DOM access and using the DOM approach when available. I decided against this because:

  1. Consistent behavior. Markdown working just fine in the browser but not on NodeJS is exactly what I tried to fix.
  2. Security. It's unexpected that Markdown inserts anything into the DOM. This might open the door for XSS attacks.

@mAAdhaTTah @LeaVerou @Golmote

This fix is urgent. Please review ASAP.

I want to release the fix right after it got merged.


This fixes #2973.

@github-actions
Copy link

github-actions bot commented Jul 1, 2021

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of +166 B (+9.2%).

file master pull size diff % diff
components/prism-markdown.min.js 1.8 KB 1.97 KB +166 B +9.2%

Generated by 🚫 dangerJS against 59e320a

@LeaVerou
Copy link
Member

LeaVerou commented Jul 2, 2021

This is not a regression, right? If not, why is it urgent?

@RunDevelopment
Copy link
Member Author

It is a regression. Markdown doesn't work on Node anymore. See #2973 and #2976.

@LeaVerou
Copy link
Member

LeaVerou commented Jul 2, 2021

What introduced this regression? git blame shows that these lines have existed for 5 months?

@RunDevelopment
Copy link
Member Author

The linked commit did introduce the regression. We just haven't had a release for 6 months, so it went unnoticed.

@LeaVerou
Copy link
Member

LeaVerou commented Jul 2, 2021

I see. I'm not a huge fan of stripping HTML with regexes (it worries me about potential security issues or incorrect results) but I suppose our best bet is to merge this in and remove the regression as soon as possible, while we figure out a better long-term fix.

@RunDevelopment
Copy link
Member Author

I'm not a huge fan of stripping HTML with regexes

Neither am I.

I'll open a separate issue for a sound way to get the text content of an HTML string.

@RunDevelopment
Copy link
Member Author

@LeaVerou, can I take your comment as approval? Just want to make sure :)

I plan on releasing a patch right after merging this.

@RunDevelopment
Copy link
Member Author

Actually, I want the patch to include a fix for #2974 as well.

@RunDevelopment RunDevelopment added this to the v1.24.1 milestone Jul 2, 2021
@RunDevelopment RunDevelopment merged commit 151121c into PrismJS:master Jul 2, 2021
@RunDevelopment RunDevelopment deleted the issue2973 branch July 2, 2021 16:02
@meteorlxy
Copy link

@RunDevelopment If #2974 could not be merged soon, would you mind to release a patch for this fix first?

@RunDevelopment
Copy link
Member Author

@meteorlxy v1.24.1 is now available. It includes both fixes.

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

Successfully merging this pull request may close these issues.

Unable to run in node when using markdown
3 participants