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

fix: remove the marked.js warning about sanitize and add sanitizer #2591

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

PerfectPan
Copy link
Contributor

@PerfectPan PerfectPan commented Apr 16, 2023

Types

  • 🎉 New Features
  • 🐛 Bug Fixes
  • 📚 Documentation Changes
  • 💄 Code Style Changes
  • 💄 Style Changes
  • 🪚 Refactors
  • 🚀 Performance Improvements
  • 🏗️ Build System
  • ⏱ Tests
  • 🧹 Chores
  • Other Changes

Background or solution

🤖 Generated by Copilot at 80ad54f

  • Sanitize markdown content using dompurify library instead of deprecated and insecure sanitize option from marked library (link, link, link, link)
  • Import sanitize function from dompurify library in comments-body.tsx, markdown/index.tsx, and utils/marked.ts ( link, link)
  • Add dompurify library and its type definitions as dependencies to components/package.json (link, link)

Close #2256
Following the offical suggestion, import dom purify for sanitizer. https://marked.js.org/#usage

Changelog

🤖 Generated by Copilot at 80ad54f

This pull request adds dompurify as a dependency to sanitize HTML output from markdown content in various packages. It also removes the insecure sanitize option from the marked library and replaces it with dompurify in the comments and core-browser packages.

@opensumi opensumi bot added the 🐞 bug Something isn't working label Apr 16, 2023
@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Patch coverage: 83.33% and no project coverage change.

Comparison is base (7b72b4e) 57.62% compared to head (80ad54f) 57.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2591   +/-   ##
=======================================
  Coverage   57.62%   57.63%           
=======================================
  Files        1326     1326           
  Lines       83563    83578   +15     
  Branches    17403    17403           
=======================================
+ Hits        48157    48169   +12     
- Misses      32167    32170    +3     
  Partials     3239     3239           
Flag Coverage Δ
jsdom 52.65% <83.33%> (+<0.01%) ⬆️
node 16.76% <25.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core-browser/src/core-preferences.ts 92.85% <ø> (ø)
...ackages/core-browser/src/extensions/schema/menu.ts 100.00% <ø> (ø)
packages/i18n/src/common/contributes/en-US.lang.ts 100.00% <ø> (ø)
packages/i18n/src/common/contributes/zh-CN.lang.ts 100.00% <ø> (ø)
packages/i18n/src/common/en-US.lang.ts 100.00% <ø> (ø)
packages/i18n/src/common/zh-CN.lang.ts 100.00% <ø> (ø)
...s/main-layout/src/common/main-layout.definition.ts 100.00% <ø> (ø)
...erences/src/browser/preference-settings.service.ts 72.19% <ø> (ø)
packages/components/src/utils/marked.ts 85.71% <71.42%> (+3.89%) ⬆️
packages/menu-bar/src/browser/menu-bar.store.ts 93.02% <80.00%> (-4.20%) ⬇️
... and 4 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@erha19 erha19 merged commit db74351 into opensumi:main Apr 17, 2023
14 checks passed
@erha19 erha19 added this to the 2.24 milestone Apr 18, 2023
bytemain added a commit that referenced this pull request May 18, 2023
@bytemain
Copy link
Member

这个 PR 会带来一个问题:#2725,dompurify 在 Node 环境下需要配合 JSDOM 一起使用。

我尝试配合 JSDOM 使用了一下,在目前的编译配置下会编译报错(jsdom 20.x 以上都是 esm 包了,要在 2.24 修改编译配置的话是 breaking change,每个集成方都得添加一遍)。

我换了个 sanitize-html,它用的 htmlparser2 也是 esm 包。

那么看起来比较好的方案是 revert 这个 PR?

cc @erha19 @Aaaaash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Marked's deprecated warning message should be removed
3 participants