Enforce write eligibility against the device, not the plan file - #16
Conversation
Four of the apply-boundary defects in #14. The theme is that the plan file was trusted for decisions only the device can answer. 0a, the sharp one. `applyEligibility` sat outside the hashed body while `assertValidPlan` gated on it, so editing a plan to set `eligible: true` left `planId` valid and bypassed the firmware allowlist. Demonstrated end to end against legacy firmware with a strong unit ID. Hashing the field is not the fix — the hash is unkeyed, so an editor can re-sign. The fix is `evaluateApplyEligibility`, a pure function of a snapshot, which the applier now runs against the *freshly read device* after export. The plan's copy is reporting only. The field goes into the hashed body as well, because leaving a load-bearing value unhashed is indefensible even when it is not the last line of defence; that changes the plan format, so `schemaVersion` moves to v2. 0b. Nothing checked that a plan's frames were the frames its changes imply. Encoder writes are whole-record bulk transfers, so a plan could declare one colour change and ship bytes rewriting fourteen other tags — invisible to anyone reading `changes`. `deriveFrames` is now shared between planning and applying, and the applier re-derives from the live snapshot and compares. This could not go in `assertValidPlan` as first planned: `PlannedChange` carries only the changed tag, encoder frames need all fifteen, and those live in the snapshot. `assertValidPlan` also runs before the export. So the check sits after the export and the snapshot-hash check, before backup, journal, or any write. 0f. `new Date(plan.expiresAt).getTime() <= now` is `NaN <= now` for an unparseable date, which is false — so a malformed expiry read as "not expired" and the plan never aged out. Rejected explicitly now. Tests in test/write-boundary.test.ts, one per defect. The forged plans are re-signed, since an unkeyed hash makes that trivial for an attacker and a test that only tampers would prove much less than it appears to. Verified each is load-bearing by reverting each guard and watching the matching test fail. A control case confirms untampered plans still apply. `apply` stays gated: 0c and 0d are still open. The gate message, README, and skill now name only what is actually outstanding. Refs #14 Co-Authored-By: Claude Opus 5 <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: d5dc0efba8
ℹ️ 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".
| const derived = deriveFrames(snapshot, plan.changes, firmwarePolicy(snapshot.device.firmware.date)); | ||
| const mismatch = | ||
| derived.length !== plan.frames.length || | ||
| derived.some((frame, index) => frame.hex !== plan.frames[index]?.hex || frame.target !== plan.frames[index]?.target); |
There was a problem hiding this comment.
When apply is enabled via MFT_UNSAFE_APPLY=1 or this API is called directly, a re-signed plan can modify frame.bytes while retaining the original hex and target. This check therefore passes, but line 97 sends the modified bytes; readback only detects the mismatch after the unauthorized write has reached the device. Compare the derived and planned byte arrays directly (and ideally reject inconsistent bytes/hex representations) before writing.
Useful? React with 👍 / 👎.
| for (const change of changes) { | ||
| let changed = changedRecords.get(change.target); | ||
| if (!changed) { | ||
| const record = recordForTarget(config, change.target); | ||
| changed = { rawTags: { ...record.rawTags }, bank: record.bank, encoder: record.encoder }; | ||
| changedRecords.set(change.target, changed); | ||
| } | ||
| changed.rawTags[String(change.tag)] = change.rawDesired; |
There was a problem hiding this comment.
For a re-signed plan, deriveFrames trusts the attacker-controlled target, tag, and rawDesired without verifying that they correspond to path, expected, and desired. For example, a change can still claim global.brightness.rgb while selecting one of the otherwise unsupported global tags 2–7; the derived frame then matches, expectedSnapshotAfterChanges expects the same hidden write, and verification succeeds. Re-parse each path against the live snapshot and derive the target, tag, and raw value from its field rule instead of accepting those redundant plan fields.
Useful? React with 👍 / 👎.
Codex found two ways past the frame-correspondence check added in the previous commit. Both are real; both confirmed by reproduction. The check compared `frame.hex`, but `frame.bytes` is what gets sent. A re-signed plan could keep the legitimate hex string for display and carry different bytes: the check passed, the modified bytes went to the device, and read-back noticed only afterwards. Compare the byte arrays, and send the *derived* frames rather than the plan's — the plan's array is now only something to review and to check against, never a source of bytes. The deeper one: checking frames against changes is worthless if the changes themselves are unconstrained. `deriveFrames` trusted `target`, `tag`, and `rawDesired`, which are redundant with `path` and `desired` — the two fields a human actually reads. So a change could display `global.brightness.rgb` while writing tag 3, one of the globals no supported path can reach. The frames matched the changes, and `expectedSnapshotAfterChanges` expected the same hidden write, so verification passed too. Reproduced before fixing. Now the target, tag, and raw value are re-derived by re-parsing `path` against the live snapshot, and the redundant fields are cross-checked rather than obeyed. Two regression tests, both verified load-bearing by reverting each guard and watching only the matching test fail. Refs #14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both P1s confirmed and fixed in 2b8d446. Bytes vs hex — right, and worse than a comparison bug: the plan's frames were also the ones being sent. Now the check compares byte arrays, and the applier sends the derived frames. Path vs target/tag — this was the better catch. Checking frames against changes is worthless if the changes are unconstrained. Reproduced it: a change displaying Two regression tests, each verified load-bearing by reverting its guard. |
Two processes can write to the same controller at once and nothing stops them. That stays true on purpose: a Twister is one controller on one person's desk, so concurrent writers are an edge case, and a cross-process lock brings failure modes of its own — a stale lock is a device you cannot write to at all. Document the boundary instead. Each write re-reads the device and rejects stale expected values, so a second writer is normally refused rather than silently clobbering. What is not defended is an encoder write, which is a multi-frame bulk transfer: frames interleaved from two writers can leave a record in a state neither intended, and read-back catches that only afterwards. Also names the vendor MIDI Fighter Utility, which is the likeliest second writer in practice. Refs #14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Added a docs note on concurrency rather than building a lock: one writer at a time, stated as an expectation with the actual failure mode (interleaved multi-frame encoder writes) spelled out. A cross-process lock isn't worth its own failure modes for a single-user desk device. @codex review |
Self-review found the same gap Codex found, one field over. The previous commit re-derived a change's target and tag by re-parsing its `path`, but took `desired` on trust. `global.midiChannel` has a documented range of 1..16, and `createPatchPlan` enforces it. Planning is not the trust boundary though — applying is. A re-signed plan could set that field to 100: still a 7-bit value, so the codec encodes it without complaint, and `expectedSnapshotAfterChanges` expects the same number, so read-back verification agrees as well. Nothing between the plan file and the device disagreed. `assertWithinRule` now re-checks the resolved value against its field rule inside `deriveFrames`, alongside the target and tag checks. Also drops the `changedRecords` map from `createPatchPlan`. It was left populated but unread once frame building moved into `deriveFrames` — duplicated logic that a later change would silently apply to only one of the two paths. Refs #14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Self-review found the same class of gap one field over: Also dropped a dead 30 tests passing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adf582ca17
ℹ️ 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".
| // 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); | ||
| const rawDesired = desiredRaw(change.desired, resolved.rule, policy.shiftedChannelIsOneBased); |
There was a problem hiding this comment.
A re-signed plan can set both desired and rawDesired to 127 for a field such as switch.action.code, whose planner range is 0–8. The new derivation calls desiredRaw directly without passing the value through resolveValue, so the forged frame matches the derived frame and the unsupported value reaches the device. Fresh evidence after the earlier review is that the new path-based derivation still bypasses the field rule's type and range validation; validate change.desired against resolved.rule before encoding it.
Useful? React with 👍 / 👎.
| If it does happen, the damage is bounded but real. Each write re-reads the | ||
| device and rejects stale expected values, so the second writer is normally | ||
| refused rather than silently clobbering. What is *not* defended: an encoder | ||
| write is a multi-frame bulk transfer, and frames from two writers interleaved | ||
| mid-record can leave that record in a state neither writer intended. Read-back |
There was a problem hiding this comment.
When two applies both finish their initial snapshot before either sends, neither observes stale state and neither is refused. Since each global or encoder write contains the whole record from its own snapshot, the writes can even run non-interleaved—A writes and verifies, then B writes and verifies—while B silently restores A's changed tags to their old values and both processes report success. The warning should describe this general stale-snapshot clobbering rather than limiting the undefended failure mode to interleaved encoder frames.
Useful? React with 👍 / 👎.
The warning added two commits ago said the undefended case was frames from two writers interleaving mid-record, and implied the stale-value check would normally refuse the second writer. Codex pointed out that is the lesser risk and the reassurance is wrong. Every write carries the whole record, rebuilt from the snapshot its writer read. So two applies that never interleave a single frame still lose data: both export, both preconditions pass because neither has written yet, A writes and verifies, then B writes a record built from a snapshot that still holds A's old values. A's change is reverted. B verifies against its own expectation, which is satisfied. Both report success and nothing records that a change was lost. No unlucky timing required — only two overlapping reads. Read-back cannot catch it, because each writer checks its own expectation and both are individually correct. Refs #14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both addressed. P1 (desired not validated) — already fixed. You reviewed P2 (docs) — you're right and my version was worse than wrong, it was reassuring. I described interleaved frames as the undefended case and implied the stale-value check would normally refuse the second writer. The real risk needs no interleaving at all: every write carries the whole record rebuilt from its writer's snapshot, so if both export before either writes, B reverts A, B verifies against its own expectation, and both report success with nothing recording the loss. Rewritten to lead with that. |
1 participant
Four of the apply-boundary defects in #14. The theme: the plan file was trusted for decisions only the device can answer.
0a —
applyEligibilitysat outside the hashed body whileassertValidPlangated on it, so editing a plan to seteligible: truekeptplanIdvalid and bypassed the firmware allowlist. Demonstrated end to end against legacy firmware.Hashing the field is not the fix — the hash is unkeyed, so an editor can re-sign. The fix is
evaluateApplyEligibility, run by the applier against the freshly read device. The plan's copy is now reporting only. The field is hashed too, which movesschemaVersionto v2.0b — nothing checked a plan's frames were the frames its changes imply. Encoder writes are whole-record transfers, so a plan could declare one colour change and ship bytes rewriting fourteen other tags, invisible to anyone reading
changes.deriveFramesis now shared between planning and applying.This could not live in
assertValidPlanas originally planned:PlannedChangecarries only the changed tag, encoder frames need all fifteen, and those are in the snapshot — whichassertValidPlandoes not have, since it runs before the export.0f —
new Date(plan.expiresAt).getTime() <= nowisNaN <= nowfor an unparseable date, which is false, so a malformed expiry never aged out.Tests
One per defect in
test/write-boundary.test.ts. The forged plans are re-signed, since an unkeyed hash makes that trivial and a test that only tampers proves much less than it appears to. Each verified load-bearing by reverting its guard and watching the matching test fail. A control case confirms untampered plans still apply.Not addressed
applystays gated — 0c (replayable plans) and 0d (cwd-relative state) are open, and both need design decisions rather than mechanical fixes. The gate message, README, and skill now name only what is actually outstanding.🤖 Generated with Claude Code