feat: harden firmware artifact storage - #79
Conversation
originalix
left a comment
There was a problem hiding this comment.
三仓固件升级最小化加固 — 只读复审结论(app-modules 部分)
本 PR 上共 4 条行内意见,全部 P1(其中 3 条 iOS、1 条 Android)。App 侧 5 条见 OneKeyHQ/app-monorepo#12656,SDK 侧 4 条见 OneKeyHQ/hardware-js-sdk#850。
核查后认为稳妥、没有提意见的部分(避免后续重复讨论):三端 verify-before-promote 的 SHA-256;artifactRef 是摘要派生的不透明 token,无法逃出 store root;ZIP 路径安全规则集(POSIX/Windows/UNC/ADS/控制字符/规范化/大小写折叠,拒符号链接与目录,拒嵌套归档,压缩比与展开上限,CRC 校验);64 KiB 固定缓冲、不按 Content-Length 或 uncompressed_size 分配;maxBytes == expectedSize 三端一致;Content-Range 精确匹配与 resume 收到 200 时截断;256 KiB reader 上限;进程级单例所有权;TLS 校验从未被关闭;pinned resolver registry 的引用计数与 32 条上限;SNI 文本路径 10 MiB 上限在 pinned transport 抽取后未被改变(纯重构)。
测试证据现状:本 PR 生产代码 +3573 行,测试只 +101 行。iOS 侧对 FirmwareArtifactStore.swift(1198 行)零测试——tests/swiftpm 的唯一 target 只符号链接了 RangeDownloadLogic.swift;Android 只新增 FirmwareArchiveRulesTest.kt(75) 与 FirmwareArtifactDeadlineTest.kt(26)。计划 Task 10 的 conformance 服务器(cross-host-redirect、wrong-hostname-cert)从未创建,conformance/ 下只有 README.md 与 ios-simulator/。下面 4 条里有 3 条正是因此没有任何测试会失败。
待你方确认的一项 ops 前置:线上签名 config.json 是否真的带 exact-host 的 domains['*.onekey-asset.com'] 条目——这决定 pinned-IP 候选在生产是否真的可达。这是配置问题不是代码问题,我无权限核对。
originalix
left a comment
There was a problem hiding this comment.
P2 批次(经对抗验证后存活的 2 条)
上一轮 14 条未验证的 P2 候选,这轮去重成 11 条后全部送了双透镜(correctness + reachability)对抗验证,7 条存活、4 条被推翻。本 PR 上存活 2 条,都在 iOS 侧,都不阻塞。
另有 1 条与本仓相关的候选我判断证据不足、没有发布:「三端重定向策略分歧,Android/Desktop 对任何 3xx 都是不可重试的硬失败」。反方论据成立——catalog 生成器在构建期就用 redirect: 'error' 拉取过每个 artifact URL(app-monorepo development/scripts/firmware/generateFirmwareCatalog.mjs:99, :252),所以「catalog URL 必须是终态」这个不变量已经在构建期被强制;它的触发依赖未来的 CDN 配置变更,且 iOS 侧的部分已被上面那条 pinned 重定向 P1 覆盖。留作 pending,不作为 finding。
本轮结论锚定 commit 56a98dc8。
originalix
left a comment
There was a problem hiding this comment.
第二轮复核:修复确认 + 3 条新引入的问题
先说确认的部分。逐条核到 1989d3f1:
| 原 finding | 判定 |
|---|---|
| P1 pinned 不拒重定向 / 不校验最终 URL | FIXED |
| P1 pinned 无 wall-clock deadline | FIXED |
P1 Swift 错误丢 ARTIFACT_* 码 |
FIXED |
P1 Android Dispatchers.Default |
FIXED |
| P2 去重串扰 | PARTIALLY(见下方 R5) |
| P2 逐字节消费 | FIXED,但见 R1 / R2 |
重定向那条修得完整:SniConnectClient.swift:114-121 实现 willPerformHTTPRedirection → completionHandler(nil),pinned session 复用同一 delegate;最终 URL 断言三要素齐全(scheme==https / host 相等 / port 为 nil 或 443)。错误码那条的新测试确实断言了 String(describing:) 含 ARTIFACT_,不是空断言。Android 的 12 个固件 API 全部改用模块自有的 SupervisorJob() + Dispatchers.IO,且 cancel 不进 downloadLocks monitor,可调度性有结构性保证。实跑:Swift 33 tests 全绿,Android 14 个测试类 71 用例全绿。三端 capability 已统一为 2。
对我上一轮一条结论的修正:我说「iOS 错误码全部丢失」说重了。Nitro 的 String(describing:) 对带关联值的 enum 会保留完整消息,只有 case transferFailed 这类无 payload 的 case 才退化成裸名。所以 FirmwareArtifactStoreError.downloadFailed("ARTIFACT_...") 一直是好的,坏的只是 background session 那条。修复仍然必要且正确,但影响面比我原先描述的小。
另外澄清一条别处可能出现的误判:有说法认为 native 把 TLS 失败统一归成 ARTIFACT_NETWORK_FAILED、导致证书错误可重试。按本 PR 实际内容不成立 —— iOS FirmwareArtifactStore.swift:994/:1077、Android FirmwareArtifactStore.kt:462 都产出 ARTIFACT_TLS_FAILED,isFirmwareArtifactTLSError 覆盖 8 个 NSURLError 证书码并沿 NSUnderlyingErrorKey 链遍历。该误判来自读了 App 当前 pin 的旧包(3.0.81-alpha.1)而非本 PR。
下面 3 条是本次修复新引入的,都已逐行复核。
一点测试覆盖上的观察
修复确实是「为了能被测到」而把逻辑挪进了 RangeDownloadLogic.swift(SwiftPM 唯一能编到的 target),新增 4 个符号里 3 个一一对应新测试 —— 这个做法本身是对的。但本次 P1 修复的最终 URL 三要素断言(FirmwareArtifactStore.swift:1005-1014)本身没有被挪出去,因此零测试覆盖:最该抽成纯谓词来测的东西反而没抽。同样零覆盖的还有 validateDownloadParams、validateContentRange,以及 iOS 侧全部 ZIP 规则(Android 那边已抽成 FirmwareArchiveRules.kt 并有 4 个用例)。
一个必须真机验证的高风险点
session.download(for:) 与 EMASCurl 自定义 URLProtocol 的组合在真机上是否正常工作。自定义 URLProtocol + download task 理论上支持,但 EMASCurl 的验证面主要在 data task。这一点无法静态确认,若不工作则 pinned 路由整条失效 —— 而 pinned 路由正是大陆可用性的全部意义。建议列为真机验证清单第一项。
Firmware artifact Promise settlement follow-upPublished Scope is intentionally narrow:
Validation:
|
Scope
Runtime ownership
The store is a process-level native singleton shared by independently initialized
mainandbgJS runtimes. Firmware coordination and readers remain inbg; native files are not copied into both JS heaps.Verification
Diff
Release sequencing
All public packages are versioned as
3.0.81-alpha.2. Publish through GitHub CI with the prereleasenextdist-tag after review.