diff --git a/.claude/skills/mft-configurator/SKILL.md b/.claude/skills/mft-configurator/SKILL.md index 62a641c..b7cf4f0 100644 --- a/.claude/skills/mft-configurator/SKILL.md +++ b/.claude/skills/mft-configurator/SKILL.md @@ -76,7 +76,7 @@ Plans expire after 15 minutes. If the plan expired or the device may have change ## Apply is currently disabled `mft-config apply` refuses in the current release while the write-path defects in -[issue #14](https://github.com/oveddan/mft-api/issues/14) are open — a plan file edited to set `applyEligibility.eligible=true` keeps a valid plan ID and bypasses the firmware allowlist, and `.mft-state` resolves against the current working directory. +[issue #14](https://github.com/oveddan/mft-api/issues/14) are open — `.mft-state` resolves against the current working directory, so an apply run from a different directory consults a different journal, and a plan whose apply died mid-write can be replayed. When a user asks to change settings: still export, still plan, still report the plan. Then tell them the change cannot be written yet, name the issue, and offer the vendor MIDI Fighter Utility as the way to make it by hand. Do not look for a way around the block — there is an environment variable that lifts it and it is deliberately not for agent use. diff --git a/README.md b/README.md index 7a3ccff..1adf898 100644 --- a/README.md +++ b/README.md @@ -128,13 +128,14 @@ Only `apply` can write configuration. System commands, reset commands, and bootloader commands are always blocked. Live writes currently require firmware `2026-07-02` and a strong command-`0x05` device identity. -> **`apply` is disabled in the current release.** Two defects in the write path -> ([#14](https://github.com/oveddan/mft-api/issues/14)) are open: a plan file -> edited to set `applyEligibility.eligible=true` keeps a valid plan ID, which -> bypasses the firmware allowlist; and `.mft-state` — the backups and the -> single-use plan journal — resolves against the current working directory, so -> applying from a different directory consults a different journal. That second -> one became easy to hit once the CLI could be installed and run from anywhere. +> **`apply` is disabled in the current release** while the write-path defects in +> [#14](https://github.com/oveddan/mft-api/issues/14) are open. Still +> outstanding: `.mft-state` — the backups and the single-use plan journal — +> resolves against the current working directory, so applying from a different +> directory consults a different journal; and a plan whose apply died mid-write +> can be replayed, because only a *completed* plan is recorded as consumed. +> The first became easy to hit once the CLI could be installed and run from +> anywhere. > > `list`, `export`, and `plan` are unaffected and are the whole read path. To > write settings meanwhile, use the vendor MIDI Fighter Utility. @@ -438,8 +439,21 @@ Before sending any configuration frame, `apply`: 7. records pending, verified, failed, or unknown outcomes in an append-only journal. -Completed plans are single-use. See [`docs/write-safety.md`](docs/write-safety.md) -for protocol-level details and remaining limitations. +Completed plans are single-use. + +**Write to a controller from one process at a time.** Nothing enforces this — a +lock would bring its own failure modes for what is an edge case on a single-user +desk device. + +Do not rely on the stale-value check to catch an overlap. Every write carries +the whole record, rebuilt from the snapshot that writer read, so if two applies +both export before either writes, the second one reverts the first's changes — +and because each verifies against its own expectation, both report success and +nothing records that a change was lost. Close the vendor MIDI Fighter Utility +before writing from here, too. + +See [`docs/write-safety.md`](docs/write-safety.md) for protocol-level details +and remaining limitations. ## Protocol coverage diff --git a/docs/write-safety.md b/docs/write-safety.md index 8941de1..6e1a947 100644 --- a/docs/write-safety.md +++ b/docs/write-safety.md @@ -31,3 +31,37 @@ The write implementation must preserve these invariants: Before support broadens beyond the current allowlist, tests must additionally cover journal recovery, two attached devices, timeouts before and after send, normalized fields, and confirmed restore. + +## One writer at a time + +**Nothing prevents two processes from writing to the same controller at once, +and this is deliberate.** A Twister is a single controller on one person's desk; +concurrent writers are an edge case, and defending against them would mean a +cross-process lock and its own failure modes — a stale lock is a device you +cannot write to. + +If it does happen, the failure is quiet, and the precondition check does not +save you. Every write carries the **whole** record — all of a global block, all +fifteen tags of an encoder — rebuilt from the snapshot that writer read. So two +applies that never interleave a single frame still lose data: + +1. A and B both export. Both hold snapshot `S`, and both preconditions pass, + because neither has written yet. +2. A writes its record and reads back. Its own change is there. A reports + success. +3. B writes its record, rebuilt from `S` — which still carries A's tags at + their *old* values. A's change is silently reverted. +4. B reads back and compares against what `S` plus B's own change implies. + It matches. B reports success too. + +Both processes report success, verification passes for both, and one of the two +changes is gone with nothing recorded to say so. Interleaving frames mid-record +is a further way to corrupt a single record, but it is not the main risk — this +is, and it needs no unlucky timing, only two overlapping reads. + +Read-back cannot catch it: each writer verifies against its own expectation, and +both expectations are individually satisfied. + +So: don't run two applies against the same controller simultaneously, and don't +build tooling that does. The same applies to the vendor MIDI Fighter Utility — +close it before writing from here. diff --git a/schema/patch-plan.schema.json b/schema/patch-plan.schema.json index 1dc6b57..c3b8158 100644 --- a/schema/patch-plan.schema.json +++ b/schema/patch-plan.schema.json @@ -1,12 +1,12 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": "https://djtechtools.com/schemas/mft-patch-plan-v1.json", + "$id": "https://djtechtools.com/schemas/mft-patch-plan-v2.json", "title": "MIDI Fighter Twister patch plan", "type": "object", "additionalProperties": false, "required": ["schemaVersion", "planId", "createdAt", "expiresAt", "snapshotHash", "deviceBinding", "applyEligibility", "changes", "frames"], "properties": { - "schemaVersion": { "const": "djtt.mft.patch-plan.v1" }, + "schemaVersion": { "const": "djtt.mft.patch-plan.v2" }, "planId": { "type": "string", "pattern": "^sha256:[0-9a-f]{64}$" }, "createdAt": { "type": "string", "format": "date-time" }, "expiresAt": { "type": "string", "format": "date-time" }, diff --git a/src/applier.ts b/src/applier.ts index 7f91dfb..44a7a13 100644 --- a/src/applier.ts +++ b/src/applier.ts @@ -2,9 +2,9 @@ import type { DeviceDescriptor, ConfigExport } from "./model.js"; import type { ConfigurationWriteConnection } from "./midi.js"; import { exportConfiguration } from "./exporter.js"; import { appendJournal } from "./journal.js"; -import { assertValidPlan, expectedSnapshotAfterChanges, type PatchPlan } from "./planner.js"; +import { assertValidPlan, deriveFrames, evaluateApplyEligibility, expectedSnapshotAfterChanges, type PatchPlan } from "./planner.js"; import { snapshotHash } from "./snapshot.js"; -import { encodeGlobalDryRun } from "./write-codec.js"; +import { encodeGlobalDryRun, type DryRunFrame } from "./write-codec.js"; import { firmwarePolicy } from "./compatibility.js"; interface ApplyOptions { @@ -23,6 +23,41 @@ function delay(milliseconds: number): Promise { return new Promise((resolve) => setTimeout(resolve, milliseconds)); } +/** + * Returns the frames to send, derived from the live snapshot and the plan's + * declared changes, having confirmed they are the frames the plan carries. + * + * The comparison is over `bytes`, because `bytes` is what gets sent. Comparing + * the `hex` rendering instead left a hole: a plan could keep the legitimate hex + * string for display while carrying different bytes, pass the check, and have + * the modified bytes written — read-back would notice only after they reached + * the device. + * + * The derived frames are also what the caller sends, so the plan's own array is + * never the source of any byte. It exists to be reviewed, and to be checked. + */ +function resolveFramesToSend(snapshot: ConfigExport, plan: PatchPlan): DryRunFrame[] { + const derived = deriveFrames(snapshot, plan.changes, firmwarePolicy(snapshot.device.firmware.date)); + const matches = + derived.length === plan.frames.length && + derived.every((frame, index) => { + const declared = plan.frames[index]; + return ( + declared !== undefined && + frame.target === declared.target && + frame.hex === declared.hex && + frame.bytes.length === declared.bytes.length && + frame.bytes.every((byte, position) => byte === declared.bytes[position]) + ); + }); + if (!matches) { + throw new Error( + "Patch plan frames do not match the frames its changes imply for this device; refusing to send bytes the plan does not describe", + ); + } + return derived; +} + function assertDeviceBinding(snapshot: ConfigExport, plan: PatchPlan): void { const binding = plan.deviceBinding; if ( @@ -47,10 +82,27 @@ export async function applyPatchPlan( const before = await exportConfiguration(connection, device, { timeoutMs, retries: 1 }); assertDeviceBinding(before, plan); if (snapshotHash(before) !== plan.snapshotHash) throw new Error("Device configuration changed since this plan was created"); + + // Everything above trusts the plan file. These two checks do not, and both + // have to run here rather than in assertValidPlan, because both need the + // device state that only exists after the export above. + // + // The plan's own applyEligibility is attacker-editable and the content hash + // is unkeyed, so re-deriving it from the device in front of us is what + // actually enforces the firmware allowlist. + const liveEligibility = evaluateApplyEligibility(before); + if (!liveEligibility.eligible) { + throw new Error(`Device is not eligible for live writes: ${liveEligibility.reasons.join("; ")}`); + } + // And the frames are only meaningful if they are the frames these changes + // imply. Encoder writes carry the whole record, so a plan could declare one + // change and ship bytes that rewrite fourteen other tags. Everything below + // sends these derived frames, never plan.frames. + const framesToSend = resolveFramesToSend(before, plan); const backupPath = await options.saveBackup(before); await appendJournal(options.journalPath, { planId: plan.planId, outcome: "started", detail: `backup=${backupPath}` }); - const targets = [...new Set(plan.frames.map((frame) => frame.target))]; + const targets = [...new Set(framesToSend.map((frame) => frame.target))]; const policy = firmwarePolicy(device.identity.firmwareDate); // Encoder writes need a trailing global-settings write to re-enable the display // (see below). Validate that frame's tag layout before the first write lands, so @@ -60,7 +112,7 @@ export async function applyPatchPlan( if (needsDisplayRefresh) encodeGlobalDryRun(before.globals.rawTags, policy); let progressive = before; for (const target of targets) { - const frames = plan.frames.filter((frame) => frame.target === target); + const frames = framesToSend.filter((frame) => frame.target === target); const targetChanges = plan.changes.filter((change) => change.target === target); const expected = expectedSnapshotAfterChanges(progressive, targetChanges); try { diff --git a/src/cli.ts b/src/cli.ts index e608d7b..b8c5c3b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -104,10 +104,10 @@ function assertApplyEnabled(): void { "apply is disabled in this release while the write-path defects in", "https://github.com/oveddan/mft-api/issues/14 are open.", "", - "Specifically: a plan file edited to set applyEligibility.eligible=true keeps a", - "valid planId, which bypasses the firmware allowlist; and .mft-state (backups and", - "the single-use plan journal) resolves against the current working directory, so", - "applying from a different directory consults a different journal.", + "Still outstanding: .mft-state (backups and the single-use plan journal) resolves", + "against the current working directory, so applying from a different directory", + "consults a different journal; and a plan whose apply died mid-write can be", + "replayed, because only a completed plan is recorded as consumed.", "", "list, export, and plan are unaffected. To write settings meanwhile, use the", "vendor MIDI Fighter Utility.", diff --git a/src/planner.ts b/src/planner.ts index 35609eb..ee282c0 100644 --- a/src/planner.ts +++ b/src/planner.ts @@ -1,4 +1,4 @@ -import { firmwarePolicy } from "./compatibility.js"; +import { firmwarePolicy, type FirmwarePolicy } from "./compatibility.js"; import type { ConfigExport } from "./model.js"; import { sha256, snapshotHash } from "./snapshot.js"; import { encodeEncoderDryRun, encodeGlobalDryRun, type DryRunFrame } from "./write-codec.js"; @@ -19,8 +19,10 @@ export interface PlannedChange { rawDesired: number; } +export const PATCH_PLAN_SCHEMA = "djtt.mft.patch-plan.v2"; + export interface PatchPlan { - schemaVersion: "djtt.mft.patch-plan.v1"; + schemaVersion: typeof PATCH_PLAN_SCHEMA; planId: string; createdAt: string; expiresAt: string; @@ -153,12 +155,114 @@ function parseTarget(config: ConfigExport, path: string): { }; } +/** + * Re-checks an already-resolved value against its field rule. + * + * `resolveValue` enforces this while planning, but planning is not a trust + * boundary — applying is. Without this, a re-signed plan could set a 1..16 + * field to 100: the value is still 7-bit so the codec accepts it, and + * `expectedSnapshotAfterChanges` expects the same number, so read-back + * verification agrees too. + */ +function assertWithinRule(value: number | boolean, rule: FieldRule, path: string): void { + if (rule.kind === "boolean") { + if (typeof value !== "boolean") throw new Error(`${path} expects a boolean, received ${String(value)}`); + return; + } + const minimum = rule.min ?? 0; + const maximum = rule.max ?? 127; + if (typeof value !== "number" || !Number.isInteger(value) || value < minimum || value > maximum) { + throw new Error(`${path} value ${String(value)} is outside ${minimum}..${maximum}`); + } +} + +function recordForTarget( + config: ConfigExport, + target: string, +): { rawTags: Record; bank?: number; encoder?: number } { + if (target === "globals") return { rawTags: config.globals.rawTags }; + const match = target.match(/^bank\.(\d+)\.encoder\.(\d+)$/); + if (!match) throw new Error(`Invalid plan target ${target}`); + const bank = Number(match[1]); + const encoder = Number(match[2]); + const record = config.banks + .find((candidate) => candidate.number === bank) + ?.encoders.find((candidate) => candidate.number === encoder); + if (!record) throw new Error(`Plan target ${target} is absent from snapshot`); + return { rawTags: record.rawTags, bank, encoder }; +} + +/** + * Builds the exact write frames a set of changes implies, given the snapshot + * they apply to. + * + * Encoder writes are whole-record bulk transfers, so a frame is only derivable + * together with the fourteen tags the change does not touch. Those live in the + * snapshot, never in the change list — which is why a plan's frames cannot be + * checked against its own changes in isolation, and why the applier calls this + * with the freshly-read device state rather than with anything the plan + * carries. + */ +export function deriveFrames(config: ConfigExport, changes: PlannedChange[], policy: FirmwarePolicy): DryRunFrame[] { + const changedRecords = new Map; bank?: number; encoder?: number }>(); + for (const change of changes) { + // Derive from `path` and `desired` — the two fields a human actually reads + // — rather than from `target`, `tag`, and `rawDesired`. Those three are + // redundant, and trusting them let a plan display an innocuous path while + // writing a different tag entirely: `global.brightness.rgb` in the change + // list, tag 3 in the bytes. They are now cross-checked, not obeyed. + const resolved = parseTarget(config, change.path); + assertWithinRule(change.desired, resolved.rule, change.path); + const rawDesired = desiredRaw(change.desired, resolved.rule, policy.shiftedChannelIsOneBased); + if ( + resolved.target !== change.target || + resolved.rule.tag !== change.tag || + resolved.normalizedPath !== change.path || + rawDesired !== change.rawDesired + ) { + throw new Error( + `Change ${change.path} does not agree with the target it declares (target ${change.target}, tag ${change.tag})`, + ); + } + + let changed = changedRecords.get(resolved.target); + if (!changed) { + const record = recordForTarget(config, resolved.target); + changed = { rawTags: { ...record.rawTags }, bank: record.bank, encoder: record.encoder }; + changedRecords.set(resolved.target, changed); + } + changed.rawTags[String(resolved.rule.tag)] = rawDesired; + } + + const frames: DryRunFrame[] = []; + for (const [target, changed] of changedRecords) { + if (target === "globals") frames.push(...encodeGlobalDryRun(changed.rawTags, policy)); + else frames.push(...encodeEncoderDryRun(changed.bank!, changed.encoder!, config.capabilities.bankCount, policy, changed.rawTags)); + } + return frames; +} + +/** + * Whether live writes are permitted for the device this snapshot describes. + * + * Deliberately a pure function of a snapshot rather than a stored field. The + * plan carries its answer for reporting, but the applier must recompute it from + * the device in front of it: the plan's copy is attacker-editable, and the + * content hash is unkeyed so re-signing an edited plan is trivial. + */ +export function evaluateApplyEligibility(config: ConfigExport): { eligible: boolean; reasons: string[] } { + const policy = firmwarePolicy(config.device.firmware.date); + const reasons: string[] = []; + if (!policy.liveWriteAllowed) reasons.push(`Live writes are not allowlisted for firmware ${policy.firmwareDate}`); + if (!config.device.unitId?.hex) reasons.push("Snapshot has no strong per-device identity"); + return { eligible: reasons.length === 0, reasons }; +} + export function createPatchPlan(config: ConfigExport, inputs: PatchInput[], now = new Date()): PatchPlan { if (config.schemaVersion !== "djtt.mft.config-export.v1") throw new Error("Unsupported snapshot schema"); if (inputs.length === 0) throw new Error("At least one --set operation is required"); const policy = firmwarePolicy(config.device.firmware.date); const palette = config.globals.colorMap.name === "mf64" ? "mf64" : "classic"; - const changedRecords = new Map; bank?: number; encoder?: number }>(); const changes: PlannedChange[] = []; for (const input of inputs) { @@ -172,20 +276,10 @@ export function createPatchPlan(config: ConfigExport, inputs: PatchInput[], now const desired = resolveValue(input.value, target.rule, palette); if (desired === expected) throw new Error(`${target.normalizedPath} is already ${String(desired)}`); const rawDesired = desiredRaw(desired, target.rule, policy.shiftedChannelIsOneBased); - let changed = changedRecords.get(target.target); - if (!changed) { - changed = { rawTags: { ...target.rawTags }, bank: target.bank, encoder: target.encoder }; - changedRecords.set(target.target, changed); - } - changed.rawTags[String(target.rule.tag)] = rawDesired; changes.push({ path: target.normalizedPath, target: target.target, tag: target.rule.tag, expected, desired, rawExpected, rawDesired }); } - const frames: DryRunFrame[] = []; - for (const [target, changed] of changedRecords) { - if (target === "globals") frames.push(...encodeGlobalDryRun(changed.rawTags, policy)); - else frames.push(...encodeEncoderDryRun(changed.bank!, changed.encoder!, config.capabilities.bankCount, policy, changed.rawTags)); - } + const frames = deriveFrames(config, changes, policy); const createdAt = now.toISOString(); const expiresAt = new Date(now.getTime() + 15 * 60_000).toISOString(); @@ -197,15 +291,17 @@ export function createPatchPlan(config: ConfigExport, inputs: PatchInput[], now modelId: config.device.modelId, identityStrength: (config.device.unitId ? "strong" : "weak") as "strong" | "weak", }; - const reasons: string[] = []; - if (!policy.liveWriteAllowed) reasons.push(`Live writes are not allowlisted for firmware ${policy.firmwareDate}`); - if (!deviceBinding.unitId) reasons.push("Snapshot has no strong per-device identity"); - const body = { createdAt, expiresAt, snapshotHash: stateHash, deviceBinding, changes, frames }; + const applyEligibility = evaluateApplyEligibility(config); + // applyEligibility is inside the hashed body: outside it, flipping + // `eligible` to true left planId valid and bypassed the firmware allowlist. + // Hashing it is not the real defence — the hash is unkeyed, so an editor can + // recompute it — but leaving a load-bearing field unhashed is indefensible. + // The defence is the applier recomputing eligibility from the live device. + const body = { createdAt, expiresAt, snapshotHash: stateHash, deviceBinding, applyEligibility, changes, frames }; return { - schemaVersion: "djtt.mft.patch-plan.v1", + schemaVersion: PATCH_PLAN_SCHEMA, planId: `sha256:${sha256(body)}`, ...body, - applyEligibility: { eligible: reasons.length === 0, reasons }, }; } @@ -215,15 +311,20 @@ function planBody(plan: PatchPlan): unknown { expiresAt: plan.expiresAt, snapshotHash: plan.snapshotHash, deviceBinding: plan.deviceBinding, + applyEligibility: plan.applyEligibility, changes: plan.changes, frames: plan.frames, }; } export function assertValidPlan(plan: PatchPlan, now = new Date()): void { - if (plan.schemaVersion !== "djtt.mft.patch-plan.v1") throw new Error("Unsupported patch plan schema"); + if (plan.schemaVersion !== PATCH_PLAN_SCHEMA) throw new Error("Unsupported patch plan schema"); if (plan.planId !== `sha256:${sha256(planBody(plan))}`) throw new Error("Patch plan content hash is invalid"); - if (new Date(plan.expiresAt).getTime() <= now.getTime()) throw new Error(`Patch plan expired at ${plan.expiresAt}`); + const expiresAt = new Date(plan.expiresAt).getTime(); + // An unparseable date yields NaN, and `NaN <= now` is false — so without this + // an unparseable expiry read as "not expired" and the plan never aged out. + if (!Number.isFinite(expiresAt)) throw new Error(`Patch plan has an unreadable expiry ${plan.expiresAt}`); + if (expiresAt <= now.getTime()) throw new Error(`Patch plan expired at ${plan.expiresAt}`); if (!plan.applyEligibility.eligible) throw new Error(`Patch plan is not eligible: ${plan.applyEligibility.reasons.join("; ")}`); } diff --git a/test/write-boundary.test.ts b/test/write-boundary.test.ts new file mode 100644 index 0000000..69956bb --- /dev/null +++ b/test/write-boundary.test.ts @@ -0,0 +1,241 @@ +import assert from "node:assert/strict"; +import { mkdtemp } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; + +import { applyPatchPlan } from "../src/applier.js"; +import { exportConfiguration } from "../src/exporter.js"; +import type { ConfigurationWriteConnection, MessageHandler } from "../src/midi.js"; +import type { DeviceDescriptor } from "../src/model.js"; +import { assertValidPlan, createPatchPlan, type PatchPlan, type PlannedChange } from "../src/planner.js"; +import { encodeGlobalDryRun } from "../src/write-codec.js"; +import { firmwarePolicy } from "../src/compatibility.js"; +import type { ConfigExport } from "../src/model.js"; +import { assertConfigurationWrite, assertReadOnlyRequest } from "../src/protocol.js"; +import { sha256 } from "../src/snapshot.js"; + +// Regression tests for the apply-boundary defects in issue #14. Each one is a +// path that previously reached the device. + +const GLOBALS = [0, 4, 1, 1, 2, 0, 3, 9, 4, 0, 5, 0, 6, 8, 7, 0, 8, 63, 9, 127, 31, 127, 32, 127, 33, 0, 34, 6, 35, 3, 36, 7, 37, 1, 38, 1]; +const LEGACY_GLOBALS = [0, 4, 1, 1, 2, 0, 3, 9, 4, 0, 5, 0, 6, 8, 7, 0, 8, 63, 9, 127, 31, 127, 32, 127]; +const BASE_ENCODER = [10, 0, 11, 0, 12, 1, 13, 2, 14, 0, 15, 0, 16, 1, 17, 0, 18, 1, 19, 25, 20, 5, 21, 63, 22, 2, 23, 0, 24, 5]; + +function merge(stored: number[], incoming: number[]): number[] { + const current = new Map(); + for (let index = 0; index < stored.length; index += 2) current.set(stored[index]!, stored[index + 1]!); + for (let index = 0; index < incoming.length; index += 2) current.set(incoming[index]!, incoming[index + 1]!); + return [...current.entries()].sort(([a], [b]) => a - b).flatMap(([tag, value]) => [tag, value]); +} + +class FakeTwister implements ConfigurationWriteConnection { + readonly writes: number[][] = []; + private readonly handlers = new Set(); + private readonly encoders = new Map(); + + private globals: number[]; + + constructor(globals: number[] = GLOBALS) { + this.globals = [...globals]; + for (let tag = 1; tag <= 64; tag += 1) this.encoders.set(tag, [...BASE_ENCODER]); + } + + send(message: ArrayLike): void { + assertReadOnlyRequest(message); + const bytes = Array.from(message); + if (bytes[4] === 5) this.emit([0xf0, 0, 1, 0x79, 5, 1, 1, 2, 3, 4, 5, 6, 7, 8, 0xf7]); + if (bytes[4] === 2) this.emit([0xf0, 0, 1, 0x79, 2, 1, ...this.globals, 0xf7]); + if (bytes[4] === 4) { + const requested = bytes[6]!; + if (requested === 65) { + this.emit([0xf0, 0, 1, 0x79, 4, 0, 65, 1, 1, 0, 0xf7]); + return; + } + const ordinal = requested === 0 ? 64 : requested; + const data = this.encoders.get(ordinal)!; + this.emit([0xf0, 0, 1, 0x79, 4, 0, requested, 1, 2, 24, ...data.slice(0, 24), 0xf7]); + this.emit([0xf0, 0, 1, 0x79, 4, 0, requested, 2, 2, 6, ...data.slice(24), 0xf7]); + } + } + + // Writes have to mutate the fake's state, not just be recorded: the applier + // reads the device back and compares hashes, so a write-only fake would fail + // verification and mask whether a guard or the round trip rejected the plan. + sendConfigurationWrite(message: ArrayLike): void { + assertConfigurationWrite(message); + const bytes = Array.from(message); + this.writes.push(bytes); + if (bytes[4] === 1) { + this.globals = merge(this.globals, bytes.slice(5, -1)); + return; + } + const ordinal = bytes[6] === 0 ? 64 : bytes[6]!; + const size = bytes[9]!; + this.encoders.set(ordinal, merge(this.encoders.get(ordinal)!, bytes.slice(10, 10 + size))); + } + + subscribe(handler: MessageHandler): () => void { + this.handlers.add(handler); + return () => this.handlers.delete(handler); + } + + close(): void {} + + private emit(message: number[]): void { + for (const handler of [...this.handlers]) handler(message); + } +} + +const device: DeviceDescriptor = { + inputPort: { index: 0, name: "Twister" }, + outputPort: { index: 0, name: "Twister" }, + identity: { manufacturerId: [0, 1, 121], familyId: 5, modelId: 1, firmwareBytes: [32, 38, 7, 2], firmwareDate: "2026-07-02" }, +}; + +const legacyDevice: DeviceDescriptor = { + ...device, + identity: { ...device.identity, firmwareBytes: [32, 23, 6, 8], firmwareDate: "2023-06-08" }, +}; + +// The plan hash is unkeyed, so anyone editing a plan can recompute it. Tests +// that only tamper without re-signing would prove far less than they appear to. +function resign(plan: PatchPlan): PatchPlan { + const { schemaVersion: _schema, planId: _id, ...body } = plan; + return { ...plan, planId: `sha256:${sha256(body)}` }; +} + +// Builds the global frame a forged change list implies, bypassing deriveFrames +// so the test can present a *self-consistent* plan. Otherwise the guard could +// reject it on a frame mismatch and the test would never exercise the change +// check it is actually about. +function globalFramesFor(snapshot: ConfigExport, changes: PlannedChange[]) { + const rawTags = { ...snapshot.globals.rawTags }; + for (const change of changes) rawTags[String(change.tag)] = change.rawDesired; + return encodeGlobalDryRun(rawTags, firmwarePolicy(snapshot.device.firmware.date)); +} + +async function applyTo(connection: ConfigurationWriteConnection, target: DeviceDescriptor, plan: PatchPlan): Promise { + const directory = await mkdtemp(join(tmpdir(), "mft-boundary-")); + await applyPatchPlan(connection, target, plan, { + journalPath: join(directory, "journal.ndjson"), + timeoutMs: 100, + pacingMs: 0, + saveBackup: async () => join(directory, "backup.json"), + }); +} + +test("0a: flipping applyEligibility now breaks the content hash", async () => { + // Legacy firmware, so the plan is genuinely ineligible and flipping the flag + // is a real edit. On an already-eligible plan the same assignment is a no-op + // and would pass for no reason. + const connection = new FakeTwister(LEGACY_GLOBALS); + const before = await exportConfiguration(connection, legacyDevice, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: 43 }]); + assert.equal(plan.applyEligibility.eligible, false); + + // Edit the field without re-signing, as a plain text edit of the file would. + const forged: PatchPlan = { ...plan, applyEligibility: { eligible: true, reasons: [] } }; + + assert.throws(() => assertValidPlan(forged), /content hash is invalid/); +}); + +test("0a: a re-signed forged plan is still refused by the live device check", async () => { + // Legacy firmware: live writes are not allowlisted, so planning marks the + // plan ineligible. Forge the flag and re-sign, which defeats every check that + // reads only the plan file. + const connection = new FakeTwister(LEGACY_GLOBALS); + const before = await exportConfiguration(connection, legacyDevice, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: 43 }]); + assert.equal(plan.applyEligibility.eligible, false); + + const forged = resign({ ...plan, applyEligibility: { eligible: true, reasons: [] } }); + assert.doesNotThrow(() => assertValidPlan(forged), "the re-signed plan must pass file-only validation"); + + await assert.rejects(applyTo(connection, legacyDevice, forged), /not eligible for live writes/); + assert.equal(connection.writes.length, 0, "no bytes may reach a device on non-allowlisted firmware"); +}); + +test("0b: frames that do not match the declared changes are refused", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: "green" }]); + + // Declare a colour change, but ship the bytes of a different one. An encoder + // frame carries the whole 15-tag record, so this rewrites tags the plan never + // mentions — exactly what a reviewer reading `changes` would not catch. + const smuggled = createPatchPlan(before, [{ path: "bank.1.encoder.1.encoder.midiNumber", value: 99 }]); + const forged = resign({ ...plan, frames: smuggled.frames }); + + assert.doesNotThrow(() => assertValidPlan(forged)); + await assert.rejects(applyTo(connection, device, forged), /frames do not match/); + assert.equal(connection.writes.length, 0); +}); + +test("0b: a change cannot write a different tag than its path names", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "global.brightness.rgb", value: 100 }]); + + // Keep the path a reviewer reads, retarget the tag the bytes touch. Tags 2-7 + // are real globals that no supported path can reach, so this writes a setting + // the plan does not admit to touching. The frames still match the changes — + // it is the changes themselves that lie. + const retargeted = [{ ...plan.changes[0]!, tag: 3, rawExpected: before.globals.rawTags["3"]!, rawDesired: 77 }]; + const forged = resign({ ...plan, changes: retargeted, frames: globalFramesFor(before, retargeted) }); + + await assert.rejects(applyTo(connection, device, forged), /does not agree with the target it declares/); + assert.equal(connection.writes.length, 0); +}); + +test("0b: frames whose bytes differ from their hex are refused", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: "green" }]); + + // `hex` is only a rendering. Leave it legitimate so the plan reads correctly, + // and modify the bytes that actually get sent. + const tampered = plan.frames.map((frame) => ({ ...frame, bytes: [...frame.bytes] })); + tampered[0]!.bytes[11] = 0x7f; + const forged = resign({ ...plan, frames: tampered }); + + await assert.rejects(applyTo(connection, device, forged), /frames do not match/); + assert.equal(connection.writes.length, 0); +}); + +test("0b: a value outside its field's range is refused at apply time", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "global.midiChannel", value: 5 }]); + + // The rule is 1..16, and planning enforces it. Applying is the trust + // boundary though, and 100 is still 7-bit, so the codec would encode it + // happily and read-back would agree with the forged expectation. + const outOfRange = [{ ...plan.changes[0]!, desired: 100, rawDesired: 100 }]; + const forged = resign({ ...plan, changes: outOfRange, frames: globalFramesFor(before, outOfRange) }); + + await assert.rejects(applyTo(connection, device, forged), /outside 1\.\.16/); + assert.equal(connection.writes.length, 0); +}); + +test("0f: an unreadable expiry is rejected rather than treated as unexpired", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: "green" }]); + + // new Date("never").getTime() is NaN, and `NaN <= now` is false, so this + // previously sailed through the expiry check and never aged out. + const forged = resign({ ...plan, expiresAt: "never" }); + + assert.throws(() => assertValidPlan(forged), /unreadable expiry/); +}); + +test("an untampered plan still applies", async () => { + const connection = new FakeTwister(); + const before = await exportConfiguration(connection, device, { timeoutMs: 100, retries: 0 }); + const plan = createPatchPlan(before, [{ path: "bank.1.encoder.1.colors.active", value: "green" }]); + + await applyTo(connection, device, plan); + + assert.ok(connection.writes.length > 0, "the guards must not block legitimate applies"); +});