feat: harden prepared firmware updates - #850
Conversation
cb6c3f8 to
e853104
Compare
e853104 to
ce94a5e
Compare
originalix
left a comment
There was a problem hiding this comment.
三仓固件升级最小化加固 — 只读复审结论(hardware-js-sdk 部分)
base 对齐已核实:codex/pr845-base-1ae35c6b == 1ae35c6b2a6397441d3b0084b5b6f69914a4fb2b == PR #845 当前 head,分支恰好 3 个 commit。#845 的其余修复(ROMLOADER fail-fast、assertProtocolV2ReconnectIdentity、verifyProtocolV2StagedFile)均在 5eaa6596 的删除范围之外,未丢失。但 #845 的 validateProtocolV2FilesystemPath 在新 prepared 分支上被绕过,见 FirmwareUpdateV4.ts:765 的意见。
mutation barrier 逐条追踪,结构全部成立:
- V2:
:514 acquireFirmwareSource()→ resource:517-598→:602 DeviceBackToBoot - V3:
:175-192备齐 resource/firmware/ble/bootloader →:197 enterBootloaderMode() - V4:
:784-785备源 →executeProtocolV2Phases,首个enterProtocolV2BootloaderMode在~:1439 - DeviceUpdateBootloader:
:175-186先开源,再checkBootloaderSourceLength与任何 EmmcFileWrite
已实跑:cd packages/core && npx jest --coverage=false → 54 suites / 643 tests 全绿;node scripts/check-versions.js → 30 个包统一在 1.2.0-alpha.22。
本 PR 上共 4 条行内意见:P0 ×1、P1 ×3(其中一条是与 app-monorepo 共同构成 P0 的 SDK 侧)。App 侧 5 条见 OneKeyHQ/app-monorepo#12656,native 侧 4 条见 OneKeyHQ/app-modules#79。
范围收敛:三个只在本文件内被引用的新增
|
| 符号 | 定义 | 全部引用点 |
|---|---|---|
digestFirmwareUpdatePlan |
packages/core/src/api/firmware/FirmwareUpdatePlan.ts:152 |
同文件 :328、:619 |
assertFirmwareArtifactRead |
packages/core/src/api/firmware/FirmwareArtifactSource.ts:61 |
同文件 :97、:155 |
FirmwareUpdatePreparedBinding |
packages/core/src/api/firmware/FirmwareUpdatePreparedPlan.ts:298 |
同文件 :326、:337 |
(rg 全仓、排除 node_modules 与 .d.ts 后各自只命中一个路径。)
同时确认它们没有从 packages/core/src/index.ts 对外导出——本轮 index.ts 新增的公开导出只有 getFirmwareUpdateHostBindingGeneration / registerFirmwareUpdateHostBinding / unregisterFirmwareUpdateHostBinding 三件套。所以这不是「删掉死代码」,而是去掉 export 关键字即可,行为零变化,只是让本次评审引入的模块间 API 增量与实际使用情况一致。
优先级最低,不影响发布,放在这里只是为了让「代码量」这条评审信号有据可依,而不是笼统地说代码多。
App 侧还有 5 条同类观察(含一条死测试与一处全局日志语义被放宽),已单独评论在 OneKeyHQ/app-monorepo#12656。
originalix
left a comment
There was a problem hiding this comment.
P2 批次(经对抗验证后存活的 1 条)
上一轮 14 条未验证的 P2 候选,这轮去重成 11 条后全部送了双透镜对抗验证,7 条存活、4 条被推翻。本 PR 上存活 1 条。
被推翻的与本仓相关的一条:「resource artifact 在 plan builder 与 V2/V3 executor 用了两套选择规则,prepared Touch/Pro 更新会以 FirmwareArtifactsNotPrepared 硬失败」→ 推翻。DataManager.getFirmwareLatestRelease(DataManager.ts:298-304,本 PR 未改、与 base 一致)在最新 release 条目缺 resource 时会用 getSysResourcesLatestRelease({features, firmwareType}) 回填,而这正是 executor 侧 gate 用的同一个查询,两条「规则」收敛到同一个 URL。验证者用 jest 构造了 Touch 4.9.0 + config firmware-v8 为 [4.10.0 带 resource / 4.11.0 不带 resource] 的精确触发场景实跑,plan.targetsToUpdate 仍为 ['firmware','resource']。
ce94a5eb(即当前 PR head)。我注意到本地 SDK worktree 正处于 rebase 中途(rebase (start): checkout pr845-r5),工作区相对 PR head 已漂移约 16 行。如果这个 rebase 会 force-push 上来,上面所有行号需要重新定位,结论本身也需要在新 base 上复核——尤其是那条 P0(validateProtocolV2FilesystemPath 被绕过),要确认新 base 没有改变 legacy/prepared 两条路径的关系。
ce94a5e to
7cadb40
Compare
|
针对上面的“范围收敛”补充:已在 |
originalix
left a comment
There was a problem hiding this comment.
第二轮复核:修复确认 + 2 条新引入的问题 + 2 条流程提醒
逐条核到 7cadb40c。实跑 packages/core jest → 55 suites / 657 tests 全绿。
| 原 finding | 判定 |
|---|---|
| P0 prepared devicePath 绕过路径校验 | FIXED |
P1 deviceIdentity 自比恒真 |
PARTIALLY(见下) |
P1 buildFirmwareUpdatePlan 无 try/catch |
FIXED,但见 R4 |
P0(跨仓)asIntegrity 丢字段 |
已在 App 侧修复,SDK 保持不变是正确分工 |
| P2 装后版本断言误报 | FIXED,但见 R7 |
devicePath 那条修得很干净::755 在打开 artifact source 之前校验,三条 devicePath 生产路径(prepared / 手工 resourceBundleFiles / remote-config bundles)现已全覆盖;我另外核了 getProtocolV2InstallItemStagingPath、verifyProtocolV2StagedFile、fileWriteChunk 三处入参,确认都不是远端可控(staging path 由硬编码 volume + 白名单表内的 fileName 拼成)。新增测试真调 prepareProtocolV2ResourceSources 并断言 source opener 未被调用,是有效的方法级证明。
身份绑定为什么只给 PARTIALLY:自比恒真确实消除了,四条路径(V2 :415-420、V3 :168-173、DeviceUpdateBootloader :167-170、V4 :942-946)的校验也都严格早于各自的第一次设备变异,deviceIdentity 为空时是抛错而非跳过。两点保留意见:
- V4 用的是
DeviceInfoGet实读,而 V2/V3/DeviceUpdateBootloader 用的是features.serialNo(SDK 侧缓存对象)。后者证明的是「plan 与 SDK 当前认定的设备对象一致」,不是「与线上物理设备一致」。V1 协议下没有便宜的 fresh serial 读法,这是可接受折中,但结论强度不能等同 V4,建议在文档里写明。 - 测试完全没覆盖:新增的 13 行只测 helper 的 match/mismatch,连
deviceIdentity: undefined的 fail-closed 分支都没测,四条执行器路径零覆盖。这不满足「在线设备身份与 preparedPlan 不一致时,在任何typedCall写设备之前就抛错」这个要求 —— 至少需要一条断言 typedCall 调用次数为 0 的用例。
一条需要记录进架构文档的后果:FirmwareUpdatePreparedPlan.ts:119-125 的 receipt-vs-plan 完整性绑定用的也是「存在才比对」,而线上 manifest 恰恰不提供 expectedSize/fingerprint。这是把 P0 修在 App 侧的必然结果,方向正确,但意味着信任模型已经变成:App 内置 catalog 是唯一完整性锚点,SDK 不提供任何独立校验。这不应该是隐含的。
流程提醒 1 —— PR 的 base 分支没跟着 rebase 动。 分支确实已 rebase 到 #845 的新 head 9434a5c7,但 PR 的 base 分支 codex/pr845-base-1ae35c6b 仍指向 1ae35c6b(merge-base(1ae35c6b, 7cadb40c) = 1ae35c6b)。于是 GitHub 算出的 diff 是 105 files / +5553 / -1419,把 #845 自己的 35 文件增量(+878/-685)混了进来;真实 PR 内容是 9434a5c7...HEAD = 72 files / +4675 / -734。评审者在网页上看到的不是本 PR 的改动。建议把 base 分支重指到 9434a5c7,或新建 codex/pr845-base-9434a5c7 并改 PR base。
流程提醒 2 —— 新 base 把一处固件相关的安全校验放松了,方向与本次修复相反。 不是本 PR 干的,但 #850 现在坐在它上面:assertProtocolV2ReconnectIdentity 在旧 base 1ae35c6b 是「任一侧 serial 读不到就抛 DeviceNotFound」,新 base(来自 cb642276 fix: align Pro2 runtime protocol contracts)改成了 return 静默放行,影响的是设备变异之后的重连身份校验。缓解事实:prepared 路径下 captureProtocolV2PhysicalIdentity 会在 serial 缺失时抛错,所以 protocolV2ExpectedSerialNumber 一定是真实 serial、重连校验仍有效;被放松的只有非 prepared 的 V4 流程 —— 而按 app-monorepo#12656 的现状,那恰恰是今天 Pro2 唯一会走的路径。合起来是「入口强、过程弱」,且 base 里还把这个行为写进了测试固化下来。建议向 #845 反馈。
顺带:expectedDeviceId 现在是死参数 —— App 侧始终与 preparedPlan 同时传,而 V4 有 preparedPlan 时走 prepared 分支、完全忽略它。不是缺陷,但它暗示了一个实际不存在的绑定,建议删掉。
20bb98b to
79eae70
Compare
01354bd to
4213263
Compare
Dynamic PR #845 base alignmentRebased this PR onto the current PR #845 head Current firmware delta against PR #845:
Validation:
The repository-wide PR profile still reports pre-existing full-lint failures in unrelated or unbuilt packages. No remaining full-lint error is in this PR delta. |
4213263 to
17fd02a
Compare
|
过度编程收敛已在 acf53cc 完成:移除了 FirmwareUpdateArtifactParams 和 FirmwareUpdateV2 内不可消费的 resourceArtifact;V2 prepared resource 继续只接受 preparedPlan.materializedEntries 对应的 resourceEntries,并补了 archive receipt 不能替代 entry binding 的回归断言。expectedDeviceId 保留,因为不带 preparedPlan 的 V4 路径会用它绑定初始物理身份;三份 checkDeviceToBootloader 不在本 PR 合并,避免扩大固件升级架构重构范围。验证:agent:check commit 与 pr 均通过,Core 67 suites / 838 tests、full lint、full test、full build 全绿。 |
Automated code review found blocking issuesReviewed commit Review summaryThe PR separates firmware acquisition from device mutation through device-bound update plans, prepared artifact receipts, generation-bound host readers, and streaming byte sources, then integrates those inputs into the Protocol V1 and Protocol V2 firmware orchestrators. It also hardens React Native BLE connection setup, packet-write timeouts, protocol reprobe behavior, and firmware reconnect cleanup. What needs attention: Before merging, complete the public Protocol V2 planning path, apply Neo capability checks to artifact-derived targets, and restore confirmed-offset recovery for large Protocol V2 transfers. Issues to address
Validation gaps
|
…ade-sdk-minimal # Conflicts: # packages/connect-examples/electron-example/package.json # packages/connect-examples/expo-example/package.json # packages/connect-examples/expo-playground/package.json # packages/connect-examples/hwk-demo/package.json # packages/core/package.json # packages/hd-ble-sdk/package.json # packages/hd-cli/package.json # packages/hd-common-connect-sdk/package.json # packages/hd-transport-electron/package.json # packages/hd-transport-emulator/package.json # packages/hd-transport-http/package.json # packages/hd-transport-lowlevel/package.json # packages/hd-transport-react-native/package.json # packages/hd-transport-usb/package.json # packages/hd-transport-web-device/package.json # packages/hd-transport/package.json # packages/hd-web-sdk/package.json # packages/hwk-adapter-core/package.json # packages/hwk-ledger-adapter/package.json # packages/hwk-ledger-connector-ble/package.json # packages/hwk-ledger-connector-webhid/package.json # packages/hwk-trezor-adapter/package.json # packages/hwk-trezor-connector-electron-ble/package.json # packages/hwk-trezor-connector-rn-ble/package.json # packages/hwk-trezor-connector-webusb/package.json # packages/hwk-trezor-connector/package.json # packages/hwk-trezor-core/package.json # packages/hwk-trezor-protobuf/package.json # packages/hwk-trezor-protocol/package.json # packages/hwk-trezor-schema-utils/package.json # packages/hwk-trezor-transport/package.json # packages/hwk-trezor-type-utils/package.json # packages/hwk-trezor-utils/package.json # packages/shared/package.json
Scope
Dynamic base
1ae35c6b2a6397441d3b0084b5b6f69914a4fb2bcodex/pr845-base-1ae35c6bVerification
yarn workspace @onekeyfe/hd-core test --runInBand: 54 suites, 643 tests passedyarn agent:check --profile commitwith Node 24: passedDiff
Release
1.2.0-alpha.22