Conversation
使用 DOMPurify 对服务端下发的公告 HTML 进行白名单过滤,防止潜在的 UI 注入风险。只允许基础标签和安全的 CSS 属性(颜色、字体相关)。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
该 PR 针对弹窗里“更新公告/通知”由服务端下发并以 dangerouslySetInnerHTML 渲染的场景,引入 DOMPurify 白名单清理,降低扩展 UI 被远程 HTML 注入的安全风险(对应 issue #1273)。
Changes:
- 新增
src/pkg/utils/sanitize.ts:封装基于 DOMPurify 的 HTML 清理与 style 属性白名单过滤 - Popup 公告渲染前对
checkUpdate.notice做清理后再注入 - 增加
dompurify依赖并更新锁文件
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/pkg/utils/sanitize.ts | 新增 DOMPurify 清理逻辑与 CSS 白名单 hook |
| src/pages/popup/App.tsx | 公告内容渲染前调用 sanitizeHTML |
| package.json | 添加 dompurify 运行时依赖 |
| pnpm-lock.yaml | 锁定新增依赖及其可选依赖 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| // 过滤不安全的 CSS 属性,只保留白名单中的属性 | ||
| DOMPurify.addHook("afterSanitizeAttributes", (node) => { | ||
| if (node instanceof HTMLElement && node.hasAttribute("style")) { | ||
| const { style } = node; | ||
| for (let i = style.length - 1; i >= 0; i--) { | ||
| if (!ALLOWED_CSS_PROPERTIES.includes(style[i])) { | ||
| style.removeProperty(style[i]); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
这里在模块顶层调用 DOMPurify.addHook 会产生全局副作用:任何后续对 DOMPurify 的使用都会被这个 style 白名单逻辑影响,并且在热重载/多实例场景可能重复注册 hook。更稳妥的做法是创建专用的 DOMPurify 实例(例如基于当前 window 创建实例)并只在该实例上注册 hook,或至少加一次性初始化保护。
| // 对 HTML 进行清理,只保留安全的标签和属性 | ||
| export function sanitizeHTML(html: string): string { | ||
| return DOMPurify.sanitize(html, { | ||
| ALLOWED_TAGS: ["b", "i", "a", "br", "p", "strong", "em", "span"], | ||
| ALLOWED_ATTR: ["href", "target", "style"], | ||
| }); | ||
| } |
There was a problem hiding this comment.
新增的 sanitizeHTML 属于安全关键逻辑,但目前在 src/pkg/utils/ 下没有对应单测覆盖。建议补充 sanitize.test.ts:至少覆盖(1)移除 <script>/事件处理属性(2)过滤非白名单标签与属性(3)style 仅保留白名单 CSS 属性,且能正确剔除如 position/background/top 等。
src/pages/popup/App.tsx
Outdated
| style={{ display: showAlert ? "flex" : "none" }} | ||
| type="info" | ||
| content={<div dangerouslySetInnerHTML={{ __html: checkUpdate.notice || "" }} />} | ||
| content={<div dangerouslySetInnerHTML={{ __html: sanitizeHTML(checkUpdate.notice || "") }} />} |
There was a problem hiding this comment.
这里在组件每次 render 时都会调用一次 DOMPurify 清理,DOMPurify 相对开销较大,可能导致 popup 频繁重渲染时的性能抖动。建议用 useMemo 基于 checkUpdate.notice 缓存清理后的字符串(或在拿到 notice 时就预先清理并存储)。
| return DOMPurify.sanitize(html, { | ||
| ALLOWED_TAGS: ["b", "i", "a", "br", "p", "strong", "em", "span"], | ||
| ALLOWED_ATTR: ["href", "target", "style"], | ||
| }); |
There was a problem hiding this comment.
sanitizeHTML 允许 target 但未同时允许/补齐 rel="noopener noreferrer"。当公告里存在 <a target="_blank"> 时可能引入反向 tabnabbing 风险(新页面可通过 window.opener 操作原扩展页面)。建议:要么不允许 target,由外层统一用安全方式打开链接;要么在 DOMPurify hook 中对 A 标签设置 rel=noopener noreferrer(并将 rel 加入允许属性)。
cyfung1031
left a comment
There was a problem hiding this comment.
@CodFrm 你直接改 JSON 吧。内容在ScriptCat扩充做Render
资讯下载量也会较小
这些 DOMPurify不太可靠
什么意思?改JSON? |
打錯字。改用JSON 由「接收服务端下发的公告 HTML 」改成 「接收服务端下发的公告 JSON 」 而不是直接下载 HTML内容 直接放在 React |
呀。现在已经是 json 把这个 notice 做成物件吧 例如 |
|
或者你想简单点 可以用这个 https://github.com/lemonadejs/html-to-json 把 type 不合适的都删掉,只保留 div, span, b, p, br 等 |
呀 这个 DOMPurify 看来也不错 |
|
你改到sw环境去了,好像出问题了 |
呀。它可能要用 document 来做处理。。。 sw 不能用sanitizeHTML |
概述 Descriptions
close #1273
变更内容 Changes
src/pkg/utils/sanitize.ts封装清理逻辑b,i,a,br,p,strong,em,spancolor,font-size,font-weight,font-styleposition、background)会被自动过滤src/pkg/utils/sanitize.tsto encapsulate sanitization logicb,i,a,br,p,strong,em,spancolor,font-size,font-weight,font-styleposition,background) are automatically stripped截图 Screenshots
N/A