From 0ce1993970691823959d17c1d67bcecea020ef79 Mon Sep 17 00:00:00 2001 From: Mohammad Abdul Sahil <127765312+abdulsaheel@users.noreply.github.com> Date: Wed, 12 Aug 2026 07:42:14 +0530 Subject: [PATCH] protocol: forceTrim comment fix, gen5 dangerous opcodes, bad frame rev forceTrim body isnt a range/erase, its trim_page + wrap_count. dangerousCmds was missing 0x8E/0x8F/0x90 which is what gen5 acts on. gen5 frame with rev != 1 passes crc but shifts the field offsets, so opcode came back as a body byte. marks it not decodable now. --- lib/src/commands.dart | 2 +- lib/src/constants.dart | 57 ++++++++++++++------- lib/src/control.dart | 7 +++ lib/src/framing.dart | 28 +++++++++- lib/src/gen5_records.dart | 2 +- test/framing_test.dart | 76 ++++++++++++++++++++++++++++ test/whoop_protocol_update_test.dart | 8 +-- 7 files changed, 155 insertions(+), 25 deletions(-) diff --git a/lib/src/commands.dart b/lib/src/commands.dart index cfead99..fa4b516 100644 --- a/lib/src/commands.dart +++ b/lib/src/commands.dart @@ -159,7 +159,7 @@ Uint8List cmdToggleHr(int seq, bool on) => /// /// NOTE: sending this with payload `[0x00]` (i.e. `cmdSendR10R11(seq, false)`) /// is the REAL persistent raw-flood OFF-switch — the off state persists across -/// reconnects. STOP_RAW_DATA (0x52) does nothing. (PROTOCOL_FINDINGS.md:168-169) +/// reconnects. STOP_RAW_DATA (0x52) does nothing. Uint8List cmdSendR10R11(int seq, bool on) => buildCommand(seq, Cmd.sendR10R11Realtime, [on ? 0x01 : 0x00]); Uint8List cmdToggleImu(int seq, bool on) => diff --git a/lib/src/constants.dart b/lib/src/constants.dart index bd10c97..c439c20 100644 --- a/lib/src/constants.dart +++ b/lib/src/constants.dart @@ -35,7 +35,7 @@ class PacketType { class Cmd { static const int linkValid = 0x01; // Report the highest wire-protocol revision the strap understands. Response - // carries the max protocol version — used for firmware/feature gating. + // carries the max protocol version — used for feature gating. static const int getMaxProtocolVersion = 0x02; static const int toggleRealtimeHr = 0x03; static const int reportVersionInfo = 0x07; @@ -45,8 +45,10 @@ class Cmd { static const int abortHistoricalTransmits = 0x14; static const int sendHistoricalData = 0x16; static const int historicalDataResult = 0x17; // the batch ACK - // DANGER — never send. Body is NOT empty: two LE i32 range args - // (full erase = both 0xFEFEFEFE). See PROTOCOL_FINDINGS.md. + // DANGER — never send. Body is NOT empty and is NOT a range/erase: it is an + // 8-byte `[u32 trim_page LE][u32 wrap_count LE]` — the + // same 8 bytes as the HISTORY_END trim token. It advances the strap's flash + // trim pointer, so a wrong value discards unsynced records. static const int forceTrim = 0x19; static const int getBatteryLevel = 0x1A; static const int rebootStrap = 0x1D; // DANGER @@ -56,12 +58,18 @@ class Cmd { static const int setReadPointer = 0x21; static const int getDataRange = 0x22; static const int getHelloHarvard = 0x23; - // Firmware-load opcodes (Cmd opcode space — distinct from PacketType 0x24 - // COMMAND_RESPONSE, which is inner[0], not a command opcode). - // DANGER — never send (per PROTOCOL_FINDINGS.md destructive list). - static const int startFirmwareLoad = 0x24; // DANGER - static const int loadFirmwareData = 0x25; // DANGER - static const int processFirmwareImage = 0x26; // DANGER + // Device-update trio (Cmd opcode space — distinct from PacketType + // 0x24 COMMAND_RESPONSE, which is inner[0], not a command opcode). + // DANGER — never send. + // + // ⚠ These 0x24-0x26 values are the GEN4-EMPIRICAL numbering and could not + // be confirmed on gen5. A gen5 strap does not act on them; the real gen5 + // device-update trio is 0x8E/0x8F/0x90 (= 142/143/144), which + // [dangerousCmds] now also blocks. Keeping the gen4 entries because their + // numbering cannot be disproven, not because it is confirmed. + static const int startUpdateLoad = 0x24; // DANGER (gen4-empirical) + static const int loadUpdateData = 0x25; // DANGER (gen4-empirical) + static const int processUpdateImage = 0x26; // DANGER (gen4-empirical) static const int sendR10R11Realtime = 0x3F; // On-device haptic alarm. SET carries a wall-clock epoch + a haptic waveform // pattern (see cmdSetAlarm in commands.dart for the exact, hardware-verified @@ -112,7 +120,7 @@ class Cmd { /// Band-agnostic opcode safety classification, sourced from whoop-rs's /// hardware-tested command surface (kept SEPARATE from [dangerousCmds] above, /// which is OpenStrap's own, independently-curated gen4 list — the two do not -/// fully overlap, e.g. this list omits the firmware-load opcodes (0x24-0x26) +/// fully overlap, e.g. this list omits the device-update opcodes (0x24-0x26) /// that [dangerousCmds] already blocks, and adds a few whoop-rs flags ours /// didn't have, notably 120/SET_FF_VALUE — see the note on [forbidden] below). /// @@ -146,24 +154,39 @@ class OpcodeSafety { }; /// The subset of [forbidden] that is actively destructive (data loss / - /// bricking), not merely "don't auto-fire". Opcodes 142-144 have no named - /// meaning in either reference codebase — treat as permanently blocked, - /// unknown-but-dangerous. + /// bricking), not merely "don't auto-fire". Opcodes 142-144 (0x8E-0x90) are + /// the confirmed gen5 device-update trio — [dangerousCmds] + /// also lists them so they are actually REFUSED, because this set is + /// classification-only and self-enforces NOTHING (see the class doc). Treat + /// membership here as documentation; gate on [dangerousCmds] to block a send. static const Set destructive = {25, 45, 142, 143, 144}; static bool isForbidden(int opcode) => forbidden.contains(opcode); static bool isDestructive(int opcode) => destructive.contains(opcode); } -/// Commands that can brick the link / burn battery / brick flash. NEVER auto-fire. +/// Commands that can brick the link / burn battery / brick flash. NEVER +/// auto-fire. CALLER-ENFORCED: this package builds frames but has no transport +/// write path, so it does not itself block a send — a call site (edge, at the +/// point it writes a command) is expected to check membership here and refuse. +/// This is the set such a guard should gate on (contrast [OpcodeSafety], which +/// only sub-classifies and is likewise not self-enforcing). const Set dangerousCmds = { Cmd.forceTrim, Cmd.togglePersistentR21, Cmd.rebootStrap, Cmd.powerCycleStrap, - Cmd.startFirmwareLoad, - Cmd.loadFirmwareData, - Cmd.processFirmwareImage, + // gen4-empirical device-update numbering (unverified; inert on gen5). + Cmd.startUpdateLoad, + Cmd.loadUpdateData, + Cmd.processUpdateImage, + // CONFIRMED gen5 device-update trio (0x8E/0x8F/0x90 = + // 142/143/144). These are the opcodes a real gen5 strap actually acts on, so + // they must be in the enforced set — not merely classified in + // [OpcodeSafety.destructive]. No Cmd.* constant: we never construct these. + 0x8E, + 0x8F, + 0x90, }; /// Historical-data record type (inner[1] of a 0x2F / data packet). diff --git a/lib/src/control.dart b/lib/src/control.dart index 3dea80a..de8992e 100644 --- a/lib/src/control.dart +++ b/lib/src/control.dart @@ -826,6 +826,13 @@ class Decoded { /// scale, GET_HELLO opcode, historical-record version family); defaults to /// gen4 so every existing caller is unchanged. Decoded decodeFrame(Frame frame, {BandProfile profile = BandProfile.gen4}) { + // A frame that passed CRC but advertises a frame revision this decoder does + // not understand (see [Frame.frameRevOk]) must NOT be decoded with the rev-1 + // `inner[0]/[1]/[2]` field offsets — surface it instead of silently handing + // back a body byte as the packet type / opcode. + if (!frame.frameRevOk) { + return Decoded('unsupported_frame_rev', {'packet_type': frame.packetType}); + } final inner = frame.inner; final pt = frame.packetType; try { diff --git a/lib/src/framing.dart b/lib/src/framing.dart index 917318e..f037c01 100644 --- a/lib/src/framing.dart +++ b/lib/src/framing.dart @@ -23,12 +23,30 @@ class Frame { final bool crc8Ok; final bool crc32Ok; - Frame(this.inner, this.crc8Ok, this.crc32Ok); + /// Whether the frame header advertises the frame revision this decoder was + /// written for (rev-1). The `inner[0]/[1]/[2]` = packetType/seq/opcode field + /// offsets below assume rev-1 layout; a rev-2 frame can still pass both CRCs + /// yet shift those fields, so [opcode] would silently return a body byte. + /// + /// gen5 carries an explicit revision byte at header[1] (0x01 on every real + /// strap frame — see [BandProfile.buildHeader]); a rev-2 frame stamps 0x02 + /// there. gen4's 4-byte header has no revision byte, so it is always treated + /// as rev-1. Defaults true so directly-constructed frames and the entire + /// rev-1 path are unchanged; [parseFrame] sets it false for a gen5 frame + /// whose revision byte is not rev-1 — surfacing it instead of mis-decoding. + final bool frameRevOk; + + Frame(this.inner, this.crc8Ok, this.crc32Ok, {this.frameRevOk = true}); /// Band-neutral alias for the header-integrity result. bool get headerCrcOk => crc8Ok; bool get valid => crc8Ok && crc32Ok; + + /// Safe to read [packetType]/[seq]/[opcode] with the rev-1 field offsets: + /// CRCs pass AND the frame revision is one this decoder understands. Check + /// this (not just [valid]) before trusting [opcode] on an inbound frame. + bool get decodable => valid && frameRevOk; int get packetType => inner.isNotEmpty ? inner[0] : -1; int get seq => inner.length > 1 ? inner[1] : -1; int get opcode => inner.length > 2 ? inner[2] : -1; @@ -81,7 +99,13 @@ Frame? parseFrame(Uint8List raw, {BandProfile profile = BandProfile.gen4}) { final storedBd = raw.buffer.asByteData(raw.offsetInBytes + innerStart + declared - 4, 4); final stored = storedBd.getUint32(0, Endian.little); - return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner)); + // gen5 header[1] is the frame-revision byte (rev-1 = 0x01). A rev-2 frame can + // pass both CRCs but shifts the inner field offsets, so flag it rather than + // let the rev-1 getters return a body byte as the opcode. gen4 has no such + // byte and is always rev-1. + final frameRevOk = !profile.isGen5 || raw[1] == revision1; + return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner), + frameRevOk: frameRevOk); } /// Length-based reassembler. feed() returns every complete Frame it can carve diff --git a/lib/src/gen5_records.dart b/lib/src/gen5_records.dart index 97a0054..47d5d6e 100644 --- a/lib/src/gen5_records.dart +++ b/lib/src/gen5_records.dart @@ -104,7 +104,7 @@ abstract class Gen5HistoricalRecord { /// per-field below — several fields have OPEN semantic disagreements between /// the two reference implementations (whoop-rs vs noop) that could not be /// resolved from bytes alone; those are called out explicitly rather than -/// silently picking a side. See PROTOCOL_FINDINGS / the multiband spec §1.7. +/// silently picking a side. class Gen5HistorySample extends Gen5HistoricalRecord { /// bpm. 0 is a legitimate reading (device warming up), not absence. final int heartRate; diff --git a/test/framing_test.dart b/test/framing_test.dart index 06cc749..3e5160e 100644 --- a/test/framing_test.dart +++ b/test/framing_test.dart @@ -178,4 +178,80 @@ void main() { expect(frames.every((f) => f.valid), isTrue); }); }); + + group('dangerousCmds (enforced never-send set)', () { + test('includes the confirmed gen5 brick trio 0x8E/0x8F/0x90', () { + expect(dangerousCmds.containsAll({0x8E, 0x8F, 0x90}), isTrue); + }); + + test('keeps the gen4-empirical device-update trio + destructive singles', () { + expect( + dangerousCmds, + containsAll([ + Cmd.startUpdateLoad, + Cmd.loadUpdateData, + Cmd.processUpdateImage, + Cmd.forceTrim, + Cmd.rebootStrap, + Cmd.powerCycleStrap, + Cmd.togglePersistentR21, + ]), + ); + }); + + test('OpcodeSafety.destructive gen5 opcodes (142-144) are actually enforced', () { + final gen5Destructive = + OpcodeSafety.destructive.where((o) => o >= 142).toSet(); + expect(gen5Destructive, {142, 143, 144}); + expect(dangerousCmds.containsAll(gen5Destructive), isTrue, + reason: 'classification-only set must be blocked by the enforced set'); + }); + }); + + group('frame-rev-2 hardening', () { + test('gen5 rev-2 frame passes CRC but is flagged non-decodable', () { + final raw = Uint8List.fromList( + buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5)); + + final rev1 = parseFrame(raw, profile: BandProfile.gen5)!; + expect(rev1.valid, isTrue); + expect(rev1.frameRevOk, isTrue); + expect(rev1.decodable, isTrue); + + // Forge a rev-2 frame: bump the header revision byte (header[1]) and fix + // the header crc16 so the header-integrity check still passes — i.e. the + // frame stays .valid, exactly the latent case that used to mis-decode. + raw[1] = 0x02; + final c = crc16Modbus(raw.sublist(0, 6)); + raw[6] = c & 0xFF; + raw[7] = (c >> 8) & 0xFF; + + final rev2 = parseFrame(raw, profile: BandProfile.gen5)!; + expect(rev2.valid, isTrue, reason: 'both CRCs still pass'); + expect(rev2.frameRevOk, isFalse, reason: 'revision byte 0x02 is not rev-1'); + expect(rev2.decodable, isFalse, + reason: 'surfaced, not silently decoded as rev-1'); + }); + + test('gen4 has no revision byte and is always treated as rev-1', () { + final f = parseFrame(buildCommand(0, Cmd.getHelloHarvard, const [0x00]))!; + expect(f.frameRevOk, isTrue); + expect(f.decodable, isTrue); + }); + + test('decodeFrame surfaces a rev-2 frame instead of decoding rev-1 fields', () { + final raw = Uint8List.fromList( + buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5)); + raw[1] = 0x02; // bump frame revision + final c = crc16Modbus(raw.sublist(0, 6)); // keep header CRC valid + raw[6] = c & 0xFF; + raw[7] = (c >> 8) & 0xFF; + + final f = parseFrame(raw, profile: BandProfile.gen5)!; + expect(f.valid, isTrue); + final d = decodeFrame(f, profile: BandProfile.gen5); + expect(d.kind, 'unsupported_frame_rev', + reason: 'must not be decoded with rev-1 offsets'); + }); + }); } diff --git a/test/whoop_protocol_update_test.dart b/test/whoop_protocol_update_test.dart index df3a237..368e7e7 100644 --- a/test/whoop_protocol_update_test.dart +++ b/test/whoop_protocol_update_test.dart @@ -102,14 +102,14 @@ void main() { Cmd.rebootStrap, Cmd.powerCycleStrap, Cmd.togglePersistentR21, - Cmd.startFirmwareLoad, - Cmd.loadFirmwareData, - Cmd.processFirmwareImage, + Cmd.startUpdateLoad, + Cmd.loadUpdateData, + Cmd.processUpdateImage, ]), ); // 0x24 here is a Cmd opcode; PacketType.commandResponse (0x24) is a // separate namespace (inner[0], not inner[2]). - expect(Cmd.startFirmwareLoad, 0x24); + expect(Cmd.startUpdateLoad, 0x24); expect(Cmd.powerCycleStrap, 0x20); // 0x62 is GET_EXTENDED_BATTERY_INFO; there is no 0x63 command (the old // "reset fuel gauge" opcode was a decode artifact and has been removed).