From 6dda9d77f94abe00baa8ff56cdf9412eb8cde721 Mon Sep 17 00:00:00 2001 From: ok-diy1 Date: Tue, 1 Sep 2026 16:11:11 +0000 Subject: [PATCH] fix: stop refusing private-key writes on a config-mode guess Advanced refused to send when the store's isConfigMode read false, showing "Put your OnlyKey in config mode (flashing red LED)" to users whose key was in config mode with the LED flashing. The flag is an inference - the firmware reports the same UNLOCKED status either way - and it reads false whenever the app missed the one transition that sets it, such as starting up with the key already in config mode. There was no way to recover from inside the app. The legacy app carries the same flag but never gated a write on it, so this was a regression introduced by the rewrite. Removing the gate restores that behaviour: send, and let the device answer. Its refusal already routes through the existing catch blocks, and formatDeviceLockedError turns "Error not in config mode" into the instructions - so nothing there needs changing now that the firmware names the right reason. Measured end to end against the emulator: in config mode the wipe reports "Private key wiped from slot 101", and outside it the user gets the config-mode instructions rather than "unlock your device". The protocol side is pinned by onlykey-testing 01-protocol/27-config-mode-observability. Co-Authored-By: Claude Opus 5 --- src/components/Advanced.tsx | 21 ++++---- src/components/__tests__/Advanced.ui.test.tsx | 53 +++++++++++++++++-- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/src/components/Advanced.tsx b/src/components/Advanced.tsx index 5db0439..6382b6e 100644 --- a/src/components/Advanced.tsx +++ b/src/components/Advanced.tsx @@ -2,7 +2,7 @@ import React, { useState } from 'react'; import { useDeviceStore } from '../store/useDeviceStore'; import { KEY_SLOTS } from '../api/device/keyParser'; import { hexStringToByteArray } from '../api/device/utils'; -import { CONFIG_MODE_REQUIRED, configModeTooltipText } from '../data/configMode'; +import { configModeTooltipText } from '../data/configMode'; import { CautionButton, CriticalText, SetButton } from './ui/forms'; import { Tooltip } from './ui/Tooltip'; @@ -22,7 +22,7 @@ const ECC_SLOTS = [ const KEY_MODIFIERS = { Backup: 128, Signature: 64, Decryption: 32 }; const Advanced: React.FC = () => { - const { device, deviceType, isConfigMode, setWorking } = useDeviceStore(); + const { device, deviceType, setWorking } = useDeviceStore(); const [yubiForm, setYubiForm] = useState({ publicId: '', privateId: '', secretKey: '' }); const [eccType, setEccType] = useState(1); const [eccSlot, setEccSlot] = useState(101); @@ -33,12 +33,15 @@ const Advanced: React.FC = () => { if (!device) return null; - const requireConfigMode = (): boolean => { - if (isConfigMode) return true; - setStatus(null); - setError(CONFIG_MODE_REQUIRED); - return false; - }; + // No client-side config-mode gate. The firmware reports the same UNLOCKED + // status in config mode as out of it (okcore.cpp set_time), so the store's + // isConfigMode is an inference that reads false whenever the app missed the + // transition that sets it - a reconnect, or starting up with the key already + // in config mode. Gating writes on that guess refused them on a key that WAS + // in config mode, with no way to recover. The device is the authority: send, + // and let it answer. Its refusal already routes through the catch blocks + // below, and OnlyKeyDevice.formatDeviceLockedError turns "Error not in config + // mode" into the instructions. The legacy app never gated on this either. return (
@@ -203,7 +206,6 @@ const Advanced: React.FC = () => { onClick={async () => { setError(null); setStatus(null); - if (!requireConfigMode()) return; const maxLen = eccType === 9 ? 40 : 64; const key = eccKey.replace(/\s/g, '').slice(0, maxLen); if (!key || key.length !== maxLen) { @@ -233,7 +235,6 @@ const Advanced: React.FC = () => { if (!window.confirm(`Wipe private key from slot ${eccSlot}?`)) return; setError(null); setStatus(null); - if (!requireConfigMode()) return; setWorking(true, `Wiping private key from slot ${eccSlot}…`); try { await device.wipePrivateKey(eccSlot); diff --git a/src/components/__tests__/Advanced.ui.test.tsx b/src/components/__tests__/Advanced.ui.test.tsx index 94e9759..44a3b5b 100644 --- a/src/components/__tests__/Advanced.ui.test.tsx +++ b/src/components/__tests__/Advanced.ui.test.tsx @@ -38,7 +38,23 @@ describe('Advanced page', () => { expect(screen.getByText(/yubikey security info saved/i)).toBeInTheDocument(); }); - it('blocks private-key save outside config mode', async () => { + /* + * THE APP MUST NOT PRE-JUDGE CONFIG MODE. + * + * A user reported "unable to wipe key, says to put into config mode even when + * it is in config mode". The cause was a client-side gate here that refused to + * send when the store's isConfigMode read false - and it reads false whenever + * the app missed the one transition that sets it, such as starting up with the + * key already in config mode. The device was willing the whole time; + * onlykey-testing/test/01-protocol/27-config-mode-observability.test.js + * measures the firmware accepting OKWIPEPRIV in exactly that state. + * + * The legacy app never gated on this - it sent and let the device answer - so + * the gate was a regression introduced by the rewrite. These pin the fix: the + * command goes out regardless of the flag, and the device's own refusal is + * what the user sees. + */ + it('sends a private-key save even when isConfigMode reads false', async () => { const user = userEvent.setup(); const device = createMockDeviceClient(); seedDeviceStore({ device, isConfigMode: false }); @@ -50,8 +66,39 @@ describe('Advanced page', () => { ); await user.click(screen.getAllByRole('button', { name: /save to onlykey/i })[1]); - expect(screen.getByText(/flashing red led/i)).toBeInTheDocument(); - expect(device.setPrivateKey).not.toHaveBeenCalled(); + expect(device.setPrivateKey).toHaveBeenCalledWith(101, 1, expect.any(Array)); + expect(screen.queryByText(/flashing red led/i)).not.toBeInTheDocument(); + }); + + it('sends a private-key wipe even when isConfigMode reads false', async () => { + const user = userEvent.setup(); + vi.spyOn(window, 'confirm').mockReturnValue(true); + const device = createMockDeviceClient(); + seedDeviceStore({ device, isConfigMode: false }); + renderWithProviders(); + + await user.click(screen.getAllByRole('button', { name: /wipe from onlykey/i })[1]); + + expect(device.wipePrivateKey).toHaveBeenCalledWith(101); + expect(screen.queryByText(/flashing red led/i)).not.toBeInTheDocument(); + }); + + it("surfaces the device's own config-mode refusal instead of guessing", async () => { + const user = userEvent.setup(); + vi.spyOn(window, 'confirm').mockReturnValue(true); + const device = createMockDeviceClient(); + /* What OnlyKeyDevice.formatDeviceLockedError already makes of the firmware's + * "Error not in config mode" - which OKWIPEPRIV now returns for this state. */ + device.wipePrivateKey = vi + .fn() + .mockRejectedValue(new Error('OnlyKey must be in config mode (flashing red LED) for this operation.')); + seedDeviceStore({ device, isConfigMode: false }); + renderWithProviders(); + + await user.click(screen.getAllByRole('button', { name: /wipe from onlykey/i })[1]); + + expect(device.wipePrivateKey).toHaveBeenCalledWith(101); + expect(await screen.findByText(/flashing red led/i)).toBeInTheDocument(); }); it('saves an ECC key in config mode', async () => {