fix(onion): 落实 PR #32 Codex review 的 5 条修复（SSRF / 选路归因 / 守卫稳定性 / 钱包误报）#35
fix(onion): 落实 PR #32 Codex review 的 5 条修复（SSRF / 选路归因 / 守卫稳定性 / 钱包误报）#35v0id-byte wants to merge 5 commits into
Conversation
PR #32 的 Codex review（commit 066e263）提了 5 条，逐条核实均成立，逐条修复： P1 #5 可达性探测 SSRF：RelayReachability.probe 此前对链上目录里**任意** host 直开 WS， 恶意/陈旧描述符（127.0.0.1 / 10.x / 169.254.169.254）会让用户机器周期性向内网/云元数据 地址发 WS Upgrade。把 dialRelay 的 SSRF 守卫 + CF 隧道 wss 逻辑抽成 relaynode.resolveRelayWsUrl， dialRelay 与 probe **复用同一口径**（不漂移）；新增 allowPrivateHosts（默认 false=拒探私网， 仅本机回环自测传 true）经 makeHsDeps 传入。新增 scripts/reachability-ssrf-test.ts。 P1 #1 选路归因：建路到「WS 可达但转不动」的 exit 失败时，旧逻辑 markBad(middle) 会冤枉好 middle， 在小污染目录里把可达集压到只剩坏 exit、连去好 exit 的建路也建不成。改为**延迟归因**：仅当同一 exit 被别的 middle 走通（证明 exit 好）才回头判负此前失败的 middle；整条建路无人走通则不怪任何 middle。新增 scripts/buildcircuit-exit-blame-test.ts（已验证能捕获旧行为回归）。 P2 #2 守卫稳定性：buildCircuit 把**可达性过滤后**的 all 传给 GuardManager.currentGuard，会让一次 瞬时探测失败把持久守卫当作下线永久剔除并重写 guards.json（破坏入口守卫稳定性）。改为传**全量目录** pool 做持久化/采样；守卫此刻是否可达仍由 hop0 实际 connect + markUnreachable 冷却判定。 P2 #3 陈旧探测复活死中继：refresh 的无条件写缓存会被「早于 markBad 发起、晚于它返回」的陈旧探测 覆盖（WS 开≠能转发），让判负的死中继瞬间复活。改为记探测发起时刻，仅当期间无更新写入才落缓存。 P2 #4 钱包误报：NodeClient.onClose 对 gossip peer 失败也设了 lastError，WalletView 的 `connectionError ?? lastError` 兜底仍把瞬时 peer 错误显示到网络卡片（#31/#32 只清了 connectionError）。 改为仅 bootstrap 失败才记 lastError。 3 个本机回环自测（hs-socks-test / browser-core-test / demo-network）传 allowPrivateHosts:true。 typecheck（node+cli）、架构边界、10 个 relay 测试套件全过。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 227195938f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 持久守卫当作已下线而永久剔除并重写 guards.json——若传 all，一次瞬时探测失败 / markBad 就会把稳定守卫永久 | ||
| // 轮换掉，破坏入口守卫的稳定性（入口集被放大 → 削弱抗统计去匿名）。守卫“此刻是否可达”改由下面 hop0 的实际 | ||
| // connect 判定：连不上即 markUnreachable 冷却 + 换下一个钉住守卫（既有逻辑），到点自动恢复，绝不动持久集。 | ||
| const gid = guardManager.currentGuard(pool, new Set([exitRelayId, ...failed])); |
There was a problem hiding this comment.
When a GuardManager is present, this now samples/persists guards from the raw pool instead of the reachability-filtered all. In production RoleManager always supplies a GuardManager, so a polluted chain descriptor such as 127.0.0.1 or 169.254.169.254 can be chosen as hop0 even though RelayReachability rejected it; the next step calls c.connect(hopOf(guard)), and CircuitClient.connect opens ws://${guard.host}:${guard.port} directly without resolveRelayWsUrl. That leaves the hidden-service build path able to dial the user's private network (or fill the guard set with unusable private guards) despite the new SSRF guard.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| if (isIP(host)) { | ||
| cb(isPublicIpAddress(host) ? url(wsHost(host)) : null); |
There was a problem hiding this comment.
When an untrusted descriptor uses an IPv4-mapped IPv6 literal in hexadecimal form, e.g. ::ffff:0a00:1 for 10.0.0.1 or ::ffff:7f00:1 for loopback, isIP(host) is true but isPublicIpAddress only unwraps dotted ::ffff: forms. This branch therefore returns a WebSocket URL and both the reachability probe and shared dialer connect to the private address, bypassing the SSRF guard that this change adds for private IP literals.
Useful? React with 👍 / 👎.
fix(onion): resolve reachability merge conflict in RelayReachability
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62978c3653
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // middle 连得上但到不了 exit：**不立刻怪 middle**——可能是 exit 自己死了。先记下，待同一 exit 被别的 middle | ||
| // 走通再回头判负（见上）；整条建路都没走通则不冤枉任何 middle（避免一个坏 exit 把好 middle 逐个误剔，最终把 | ||
| // 可达集压到只剩坏 exit、令到好 exit 的建路也失败）。 | ||
| exitExtendFailed.add(middle.address); |
There was a problem hiding this comment.
When a dead exit is tried through a good middle that has not yet been marked proven, this catch records the middle for delayed attribution but the unchanged branch immediately below still calls markBad(middle.address). In fresh/small directories, a single unreachable exit can therefore still poison good middles before any other middle proves the exit, recreating the self-locking behavior this change is meant to prevent.
Useful? React with 👍 / 👎.
| if (kind === 6) { | ||
| const h = address.toLowerCase(); | ||
| if (h.startsWith('::ffff:') && h.includes('.')) return isPublicIpAddress(h.slice(7)); | ||
| if (h.startsWith('::ffff:')) { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier compressed-form issue: Node accepts expanded IPv4-mapped IPv6 literals such as 0:0:0:0:0:ffff:7f00:1 and 0000:0000:0000:0000:0000:ffff:0a00:0001 as IPv6 addresses, but this guard only unwraps strings starting with the compressed ::ffff: prefix. Those descriptors therefore fall through as public IPv6 and resolveRelayWsUrl will permit probes/dials to loopback or RFC1918 targets, leaving an SSRF bypass for the same private addresses in expanded notation.
Useful? React with 👍 / 👎.
| relays.map( | ||
| (d) => | ||
| new Promise<[RelayDescriptor, boolean]>((resolve) => { | ||
| resolveRelayWsUrl(d.host, d.port, allowPrivateHosts, (url) => resolve([d, url !== null])); |
There was a problem hiding this comment.
For hostname descriptors, this check only validates the single DNS answer seen by resolveRelayWsUrl, then discards the resolved target and keeps the original host on the descriptor that is later passed to CircuitClient.connect for hop0. With a rebinding or round-robin hostname that returns a public address here and a private address on the subsequent WebSocket dial, the descriptor passes sanitizedRelayPool but the client can still dial an internal address when it is chosen as the guard, bypassing the SSRF guard.
Useful? React with 👍 / 👎.
| relays.map( | ||
| (d) => | ||
| new Promise<[RelayDescriptor, boolean]>((resolve) => { | ||
| resolveRelayWsUrl(d.host, d.port, allowPrivateHosts, (url) => resolve([d, url !== null])); |
There was a problem hiding this comment.
This new sanitization pass awaits resolveRelayWsUrl for every descriptor without the 5s cap used by RelayReachability.probeOne. If an untrusted hostname's DNS lookup stalls or waits on an attacker-controlled nameserver, Promise.all here holds buildCircuit until the OS resolver gives up, which can hang hidden-service startup/maintenance paths that call build without an outer timeout even though the preceding reachability probe would have timed out promptly.
Useful? React with 👍 / 👎.
| const pool = dir().filter((d) => isRoutableHost(d.host)); | ||
| await reachability.refresh(pool); // 探测可达性（暖缓存即时返回，冷缓存一次并行探测 ~5s） | ||
| const all = reachability.knownUsable(pool); | ||
| const sanitizedPool = await sanitizedRelayPool(pool, opts?.allowPrivateHosts ?? false); |
There was a problem hiding this comment.
When the caller sets allowPrivateHosts: true for the local loopback test networks updated in this commit, the descriptors have already been removed by the unconditional isRoutableHost filter before this option is applied. As a result sanitizedPool is still empty for all-127.0.0.1 relay directories, so serveHiddenService cannot establish any introduction point even though the new option is meant to enable exactly that local test scenario.
Useful? React with 👍 / 👎.
1 participant
跟进 #32 的 Codex review（reviewed commit
066e263）。逐条核实，5 条全部成立，逐条修复并加测试。reachability.tshsbridge.tshsbridge.tscurrentGuard→ 瞬断即永久轮换持久守卫reachability.tsmarkBad→ 死中继瞬间复活NodeClient.swiftlastError仍显示到钱包卡片#5（P1）可达性探测 SSRF — 私网 host 探测前先拦
RelayReachability.probe此前对目录里任意host:port直接开 WS。描述符是不可信链上数据，恶意/陈旧条目（127.0.0.1/10.x/169.254.169.254）会让每个用户机器周期性向内网 / 云元数据地址发 WS Upgrade，绕过中继侧的私网拦截。修复：把
dialRelay的 SSRF 守卫 + CF 隧道 wss/按主机名逻辑抽成relaynode.resolveRelayWsUrl，dialRelay与probe复用同一函数（口径只有一处、绝不漂移）。新增allowPrivateHosts（默认 false = 生产拒探私网；仅本机回环自测传true）经makeHsDeps第三参传入。scripts/reachability-ssrf-test.ts：默认下对127.0.0.1中继0 次 TCP 连接 + 缓存不可达；allowPrivateHosts:true才真连；私网 IP 字面量同步判负（不耗 5s 超时）。#1（P1）选路归因 — 别替 exit 背锅怪 middle
建路到「WS 可达但实际转不动」的 exit 时，旧逻辑在 exit-EXTEND 失败处
markBad(middle)。但失败既可能是 middle 转不动、也可能是 exit 自己死了。小污染目录（3 好 + 1 坏 exit）下，建到坏 exit 会把好 middle 逐个判负，把可达集压到只剩坏 exit → 连去好 exit 的建路也建不成。修复：延迟归因。exit-EXTEND 失败的 middle 先记入
exitExtendFailed不判负；仅当同一 exit 被别的 middle 走通（证明 exit 好）才回头判负它们；整条建路无人走通 → 共同嫌疑是 exit → 谁都不判负。保留对非转发 middle 的检测能力，不冤枉好 middle。scripts/buildcircuit-exit-blame-test.ts：4 个真实 RelayNode，暖缓存后杀掉 exit；建到死 exit 失败后，连建 3 个好 exit 必须全部成功。已验证该测试能捕获旧行为（临时还原旧markBad→ 测试失败）。#2（P2）守卫稳定性 — 持久化喂全量目录
buildCircuit把可达性过滤后的all传给GuardManager.currentGuard。后者会把「不在所传目录里」的持久守卫当作下线永久剔除并重写guards.json→ 一次瞬时探测失败/markBad就把稳定守卫永久轮换掉，放大入口集、削弱抗去匿名。修复：守卫的持久化/采样改喂全量
pool；守卫「此刻是否可达」仍由 hop0 的实际connect+markUnreachable冷却判定（既有逻辑，到点自动恢复，绝不动持久集）。#3（P2）陈旧探测复活死中继
refresh无条件写缓存：一个早于markBad、晚于它返回的后台探测会把「连得上但转不动」的判负覆盖回ok:true（WS 开 ≠ 能转发）→ 死中继瞬间复活、建路反复重撞。修复：记探测发起时刻
startedAt，落缓存只在探测发起者这一处、且仅当期间无更新写入（cur.at <= startedAt）才写。#4（P2）钱包误报瞬时 gossip 错误
NodeClient.onClose对 gossip peer 失败也设lastError，而WalletView渲染connectionError ?? lastError→ #31/#32 只清了connectionError，瞬时 peer 错误仍经lastError显示到网络卡片。修复：仅 bootstrap（用户配置的种子）失败才记
lastError。验证
@v0idchain/node+@v0idchain/clitypecheck、架构边界检查通过。reachability-ssrf·buildcircuit-exit-blame·relay-integration·hs-socks·guards·relay-dos·mixnet·hs-rendezvous·hs-dht·rolemanager-smoke。relay-integration仍验证「公开 relay 拒绝 private descriptor EXTEND」→ dialRelay 重构行为不变。🤖 Generated with Claude Code