fix(highlight): ReDoS 检测降级为编辑器提示（实测漏判，不可作拒绝闸门）
上一提交把 isReDoSProne 当作 compileRules 的拒绝闸门，声称"9/9 危险全抓"。
独立复查证伪了这个结论：
## 实测漏判 —— 危险规则仍会进热路径
| pattern | 静态检测 | 实测 |
|---|---|---|
| [(](a+)+$ | 放行 | 30 字符 4065ms |
| (?:a\|a)+$ | 放行 | 26 字符 7547ms |
| ((a\|a))+$ | 放行 | 24 字符 16212ms |
| (\w\|\d)+$ | 放行 | 26 字符 14451ms |
根因三条：外层扫描不跟踪字符类状态（[(] 里的 ( 被当组起始）、遇到不闭合
组时 return false 让整个函数交白卷、分支比较只做字面相等抓不到语义重叠。
## 实测误判 —— 合法规则被拒
(?:a{2,3})+ 等把 {n,m} 范围重复当成 {n} 固定重复。
尝试修复后每修一处引入新问题（补字符类状态后又误杀 (?:\d{2})+ 与 ipv4
预设），证明手写正则语法扫描有本质缺陷。
## 三个替代方案均否决（有实测依据）
1. 运行时熔断（循环内查时钟）：**原理上无效**。单次 exec 阻塞 28426ms，
时钟检查根本执行不到
2. re2：不支持 lookbehind，perm/okstate/http 三条预设依赖它
3. Worker + 超时：**可靠**（6/6 危险全抓、806ms 内 tick 51 次主线程未阻塞）
但往返 0.11ms/块 vs 直接 0.035ms，且终端高亮必须同步（WriteBatcher 消费
同步染色结果）——只适合规则的异步校验，不适合热路径
## 本次改动
- compileRules 不再因 isReDoSProne 跳过规则：拒绝闸门会静默禁用合法规则
（已证实会误杀）却仍放进危险规则（已证实会漏判），两头都不对
- isReDoSProne 保留给编辑器提示：漏判不比改动前差，误判只是多一个警告而非
禁用功能
- 注释与 AGENTS.md 如实标注"只有提示没有根治"，并记录三个方案的否决理由，
避免后来者重复踩坑
热路径的实际边界仍是 MAX_LINE_LEN（>4KB 行不进用户正则）+ MAX_PER_RULE。
真正的解法（Worker 异步校验 + 存盘前把关）列为未完成项。
This commit is contained in:
Bill committed
1 parent
c6aca15134
commit
68fa46b2e3
4 files changed
+66
-29
No files matched your search
@@ -148,10 +148,9 @@ Electron + electron-vite + React 终端工具（本地终端 / SSH / SFTP）。
|
||||
- **恢复串只插在「可见间隙」前后**（`MIN_GAP_FOR_RESTORE=3` 个可见字符），不是每个 span 后。Kimi 这类全彩 TUI 一行 ~19 个 span，每 span 都重放会让 9.5KB 输出膨胀到 40.8KB（**4.30x**，无恢复时 2.78x），下游 xterm 解析 + WebGL 字形图集（背景图模式 MCR=4.5 逐字形算对比度，真彩色让缓存全 miss）被压垮，实测渲染线程卡 **18.4 秒**（日志 `[renderer] main thread stalled ~18384ms`）。现在的实现：最后一个 span 后必恢复，中间间隙够长才恢复，膨胀 3.05x 且长间隙着色正常
|
||||
- **改以上任何参数必须重跑 `[restore cost budget]` 基准段**（`tests/hl-rules.mjs`，断言密集真彩色 TUI 膨胀 < 3.2x）。功能性用例全绿不代表热路径没回归——`hl-split-smoke` 的样本每行以 reset 结尾，恰好绕过了这类膨胀
|
||||
- **CSI 参数类必须含私有前缀与中间字节**：`ESCAPE_RE` 的 CSI 分支是 `[0-9;:?<>=]*[ -/]*[A-Za-z]`。漏掉 `<`/`>`/`=` 和中间字节（`ESC[>4;1m` modifyOtherKeys、`ESC[<0;1;2M` SGR 鼠标、`ESC[=1;1$y` DECRQM）会让整行被当成含裸 ESC 的文本透传，**该行之后直到下一个换行的所有关键词全部失去高亮**——vim/tmux 在鼠标与按键上报时正好发这些序列
|
||||
- **ReDoS 用静态结构分析根治**（`highlightEngine.ts` 的 `isReDoSProne`）：编译期扫描两种可证危险的形状——**嵌套量词**（`(a+)+`、`(.*)*`、`([a-z]+)*`）与**同分支重复**（`(a|a)*`），命中即**拒绝该规则**（不进编译集，永远到不了热路径）。实测 9/9 危险全抓、6/6 安全无误判、22 条出厂预设全部通过，开销是纯字符串遍历
|
||||
- **有界量词不是危险**：`(?:\d{1,3}\.){3}`（ipv4 预设）外层 `{3}` 固定重复，不会指数回溯——判定只对 `*`、`+`、`{n,}` 这类开放式重复生效。曾因把 `{n}` 也算进去而误杀 ipv4
|
||||
- **两个被否决的方案**（都有实测依据，别再试）：`re2` 不支持 lookbehind，会废掉 `perm`/`okstate`/`http` 三条依赖它的预设；Worker + 超时往返开销 0.11ms/块（直接执行 0.035ms，4.1x），且终端高亮必须同步（`WriteBatcher` 消费同步染色结果），异步化会打乱 `HighlightStream` 语义
|
||||
- `MAX_PATTERN_LEN=500` 保留，但那只是限制粘贴量；`MAX_LINE_LEN=4KB` 限的是长度不是回溯成本（30 字符的 `(a+)+$` 实测 8.2 秒），**真正的防线是 `isReDoSProne`**
|
||||
- **ReDoS：只有「提示」没有「根治」，别误以为已防护**（`highlightEngine.ts` 的 `isReDoSProne`，**仅编辑器使用**）：它能抓典型的嵌套量词（`(a+)+`、`(.*)*`）与字面重复分支（`(a|a)*`），但**实测有漏判**——`[(](a+)+`（外层循环不跟踪字符类状态，30 字符卡 4 秒）、`(?:a|a)+` / `((a|a))+`（只做字面相等比较，抓不到语义重叠，7.5s / 16s）——也有误判（`(?:a{2,3})+` 这类范围重复被当成固定重复）。**因此它绝不能当拒绝闸门**：`compileRules` 不因它跳过规则，否则会静默禁用合法规则却仍放进危险规则
|
||||
- **尝试过的三个方案与为什么不行**（都有实测依据，别再试）：① `re2` 不支持 lookbehind，会废掉 `perm`/`okstate`/`http` 三条预设；② 运行时熔断（循环里查时钟）在原理上无效——单次 `exec` 就阻塞 28 秒，时钟检查根本执行不到；③ Worker + 超时**可靠**（6/6 危险全抓、主线程保持响应），但往返 0.11ms/块（直接执行 0.035ms）且终端高亮必须同步（`WriteBatcher` 消费同步染色结果），**只适合规则的异步校验，不适合热路径**
|
||||
- 当前热路径的实际边界就是 `MAX_LINE_LEN`（>4KB 的行不进用户正则）与 `MAX_PER_RULE`（每块每规则 300 命中）。真正的解法仍是 Worker 异步校验 + 规则存盘前把关，属未完成项
|
||||
- **fit 耗时埋点**：`TerminalView.scheduleFit` 记录每次 `fit.fit()` 的耗时，超 `FIT_SLOW_LOG_MS=200` 就写诊断日志（含 cols/rows/scrollback）。`fit` 是渲染层唯一代价随会话历史增长的操作（reflow 全量 scrollback），而通用 stall 看门狗只会说「渲染层卡了」，不可行动；有这条日志才能判断卡顿是否由 fit 引起、多大、多少行
|
||||
- **超长行按行降级**：run 无 >4KB 行时走整段扫描（保留跨行匹配）；有则按 `\n` 逐行，>4KB 的行原样透传不上色（该 run 内跨行匹配放弃）。回溯防护不变式：**任何 >4KB 的行不进用户正则**
|
||||
- **统计按 pane 实例聚合**：`highlightStats.ts` 按 pane id（`crypto.randomUUID()`，**不是 sessionId**——SSH 分屏同一 session 挂两 pane）登记各自 sink，订阅方拿跨 pane 求和；卸载与关统计开关都要注销，重绑定（rebind）**不**注销（React 先跑完 cleanups 再 setups，在那里注销会让 pane 从汇总消失一拍）。重置清各 pane 的真实计数器
|
||||
|
||||
@@ -179,3 +179,33 @@
|
||||
- i18n 新文案四语齐备（zh-CN/zh-TW/en/ja，缺译回退简体）
|
||||
- 渲染层新依赖一律 devDependencies；`@xterm/*` 版本必须与锁定的 6.1.0-beta.304 体系兼容，改依赖后必须真实 `npm install` 并跑 `node scripts/verify-deps.cjs`
|
||||
- 改动若触碰 AGENTS.md 已记载的行为，同步更新 AGENTS.md
|
||||
|
||||
## 最终 review 结论（2026-10-17 收尾）
|
||||
|
||||
对 `c6aca15` 做了独立复查，**发现 ReDoS 静态检测不可靠，已从「拒绝闸门」降级为「编辑器提示」**。
|
||||
|
||||
### 实测漏判（危险规则仍会进热路径）
|
||||
| pattern | 静态检测 | 实测耗时 |
|
||||
|---|---|---|
|
||||
| `[(](a+)+$` | 放行 | 30 字符 **4065ms** |
|
||||
| `(?:a|a)+$` | 放行 | 26 字符 **7547ms** |
|
||||
| `((a|a))+$` | 放行 | 24 字符 **16212ms** |
|
||||
| `(\w|\d)+$` | 放行 | 26 字符 **14451ms** |
|
||||
|
||||
根因：外层扫描不跟踪字符类状态（`[(` 里的 `(` 被当组起始）、遇到不闭合组时整个函数提前返回、分支比较只做字面相等。
|
||||
|
||||
### 实测误判（合法规则被拒）
|
||||
`(?:a{2,3})+`、`(?:\d{2})+` —— 把 `{n,m}` 范围重复当成 `{n}` 固定重复。
|
||||
|
||||
### 尝试修复但失败
|
||||
补字符类状态后又误杀 `(?:\d{2})+` 与 `ipv4` 预设。**每修一处引入新问题**，证明手写正则语法扫描有本质缺陷。
|
||||
|
||||
### 否决的三个替代方案
|
||||
1. **运行时熔断**（循环里查时钟）：**原理上无效**。实测单次 `exec` 阻塞 **28426ms**，时钟检查根本执行不到
|
||||
2. **re2**：不支持 lookbehind，`perm`/`okstate`/`http` 三条预设依赖它
|
||||
3. **Worker + 超时**：**可靠**（6/6 危险全抓、主线程 806ms 内 tick 51 次未阻塞），但往返 0.11ms/块 vs 直接 0.035ms。**只适合规则异步校验，不适合热路径**
|
||||
|
||||
### 当前状态与遗留
|
||||
`isReDoSProne` 保留为**编辑器提示**（漏判不比改动前差，误判只是多一个警告而非禁用规则）。热路径边界仍是 `MAX_LINE_LEN`（>4KB 行不进用户正则）+ `MAX_PER_RULE`。
|
||||
|
||||
**未完成**：真正解法是 Worker 异步校验 + 规则存盘前把关。工作量中等，需单独排期。
|
||||
@@ -103,30 +103,32 @@ const MAX_LINE_LEN = 4 * 1024
|
||||
|
||||
/**
|
||||
* Longest user pattern accepted. Bounds the damage a stored rule can do and
|
||||
* keeps the editor's live preview from choking on a paste; the real backtracking
|
||||
* defence is `isReDoSProne` below.
|
||||
* keeps the editor's live preview from choking on a paste.
|
||||
*/
|
||||
export const MAX_PATTERN_LEN = 500
|
||||
|
||||
/**
|
||||
* Static ReDoS check: reject patterns whose structure admits exponential
|
||||
* backtracking, WITHOUT running them.
|
||||
* Advisory ReDoS heuristic for the rule EDITOR: flags patterns whose shape
|
||||
* admits exponential backtracking.
|
||||
*
|
||||
* Why not the two obvious alternatives:
|
||||
* - `re2` would fix it at the root, but it does not support lookbehind, and
|
||||
* three shipped presets (`perm`, `okstate`, `http`) depend on it — the cure
|
||||
* would break the feature.
|
||||
* - Moving execution to a Worker with a timeout costs ~0.11ms per round trip
|
||||
* (measured; direct execution is ~0.035ms), and terminal highlighting must
|
||||
* stay synchronous because `WriteBatcher` consumes the coloured text inline.
|
||||
* Scope is deliberately limited to a warning. Measured misses — `[(](a+)+`
|
||||
* (4s on 30 chars) and `(?:a|a)+` (7.5s) / `((a|a))+` (16s) both slip through —
|
||||
* mean it must NOT gate the hot path: rejecting on a heuristic would silently
|
||||
* disable legitimate rules while still letting hostile ones past. It is useful
|
||||
* where a false negative costs nothing and a hit saves the user from typing a
|
||||
* pattern that would freeze the settings page.
|
||||
*
|
||||
* A string scan has none of those problems: it is nanoseconds, runs at compile
|
||||
* time, and rejects only shapes that are provably dangerous —
|
||||
* nested quantifiers `(a+)+` `(.*)*` `([a-z]+)*`
|
||||
* duplicated branches `(a|a)*` `(.|\n)+`
|
||||
* Measured against a mixed corpus it flags every exponential case and no
|
||||
* legitimate one, which is why this is a reject rather than a warning: a rule
|
||||
* that reaches the hot path must be safe to run there.
|
||||
* Why not the two obvious alternatives, both measured:
|
||||
* - `re2` fixes it at the root but does not support lookbehind, and `perm`,
|
||||
* `okstate` and `http` presets depend on it — the cure breaks the feature.
|
||||
* - A Worker with a timeout is reliable (6/6 dangerous caught, main thread
|
||||
* responsive) but costs ~0.11ms per round trip versus ~0.035ms direct, and
|
||||
* terminal highlighting must stay synchronous because `WriteBatcher`
|
||||
* consumes the coloured text inline. It remains the right tool for rule
|
||||
* VALIDATION (low frequency, may be async) — just not for the hot path.
|
||||
*
|
||||
* Runtime cut-offs cannot help either: one `exec` on a catastrophic pattern
|
||||
* blocks for 28s, so a clock check inside the match loop never gets to run.
|
||||
*/
|
||||
export function isReDoSProne(pattern: string): boolean {
|
||||
for (let i = 0; i < pattern.length; i++) {
|
||||
@@ -288,9 +290,12 @@ export function compileRules(rules: HighlightRule[]): CompiledRule[] {
|
||||
// Bounded like the other per-rule guards: a huge pattern only costs compile
|
||||
// time, but it also means someone pasted a blob into the editor.
|
||||
if (pattern.length > MAX_PATTERN_LEN) continue
|
||||
// Reject exponential-backtracking shapes outright: the hot path runs this
|
||||
// pattern on every output line, so an unsafe one could freeze the renderer.
|
||||
if (isReDoSProne(pattern)) continue
|
||||
// NOTE: the rule is deliberately NOT rejected for a ReDoS-prone shape here.
|
||||
// `isReDoSProne` is an advisory heuristic used by the editor only — it has
|
||||
// measured misses (`[(](a+)+`, `(?:a|a)+`) and false positives, so using it
|
||||
// as a gate would silently disable legitimate rules while still letting
|
||||
// some hostile ones through. The real bounds on this path remain
|
||||
// MAX_LINE_LEN (no line over 4KB reaches a user regex) and MAX_PER_RULE.
|
||||
let regex: RegExp
|
||||
try {
|
||||
// 'g' always; 'i' when the rule asks for case-insensitive matching; 'm'
|
||||
|
||||
+7
-4
@@ -1238,12 +1238,15 @@ console.log('[ReDoS shapes]')
|
||||
const flaggedPresets = DEFAULT_HIGHLIGHT_RULES.filter((r) => isReDoSProne(r.pattern)).map((r) => r.id)
|
||||
ok(flaggedPresets.length === 0, `no shipped preset is rejected (flagged: ${flaggedPresets.join(', ') || 'none'})`)
|
||||
|
||||
// A rejected rule must not reach the compiled set at all.
|
||||
// The heuristic is ADVISORY: it must not gate the hot path. Rejecting on it
|
||||
// would silently disable legitimate rules (measured false positives) while
|
||||
// still admitting hostile ones (measured misses: `[(](a+)+`, `(?:a|a)+`), so
|
||||
// compileRules keeps both and only the editor surfaces the warning.
|
||||
const compiled = compileRules([
|
||||
{ id: 'bad', pattern: '(a+)+$', priority: 1, color: { fg: '#ff0000' } },
|
||||
{ id: 'good', pattern: 'ERROR', priority: 2, color: { fg: '#00ff00' } }
|
||||
{ id: 'flagged', pattern: '(a+)+$', priority: 1, color: { fg: '#ff0000' } },
|
||||
{ id: 'plain', pattern: 'ERROR', priority: 2, color: { fg: '#00ff00' } }
|
||||
])
|
||||
ok(compiled.length === 1 && compiled[0].id === 'good', 'compileRules drops the unsafe rule and keeps the safe one')
|
||||
ok(compiled.length === 2, 'compileRules keeps flagged rules (the check is advisory, not a gate)')
|
||||
}
|
||||
|
||||
console.log(failed === 0 ? '\n[hl-rules] ALL CHECKS PASSED' : `\n[hl-rules] ${failed} CHECK(S) FAILED`)
|
||||
|
||||
Reference in new issue
Block a user