From 6b5f2623e63214d6433851b90af5a051433bf617 Mon Sep 17 00:00:00 2001 From: kipavy Date: Sun, 11 Oct 2026 09:16:47 +0000 Subject: [PATCH] fix(editor): a failed secret save no longer duplicates the new object Creating a host, key or identity stores the object first and its secrets second. When a secret failed to store, the save threw before the editor learned the new object's id, so it stayed in "create" mode and every later autosave created the object again. The three form saves now announce the created object before storing its secrets, through a required callback. The editor switches to editing on that call, so the next autosave updates the same object and retries its secrets. The shared create-then-store step lives in createWithSecrets. Refs #608 --- src/components/hosts/HostsPage.tsx | 3 +- src/components/keychain/KeychainPage.tsx | 9 ++-- .../mobile/screens/MobileHostEditScreen.tsx | 5 +-- .../screens/MobileIdentityEditScreen.tsx | 10 ++--- .../mobile/screens/MobileKeyEditScreen.tsx | 6 +-- src/services/editorMoveSecrets.test.ts | 6 +-- src/services/hostForm.test.ts | 30 ++++++++++--- src/services/hostForm.ts | 16 ++++--- src/services/keychainForm.test.ts | 43 ++++++++++++++++--- src/services/keychainForm.ts | 28 ++++++------ src/services/vaultObjectSecrets.ts | 12 +++++- 11 files changed, 110 insertions(+), 58 deletions(-) diff --git a/src/components/hosts/HostsPage.tsx b/src/components/hosts/HostsPage.tsx index 2339a2bbd..303a09891 100644 --- a/src/components/hosts/HostsPage.tsx +++ b/src/components/hosts/HostsPage.tsx @@ -589,8 +589,7 @@ export default function HostsPage() { const handleSubmit = async (data: ConnectionFormData, secrets: HostFormSecrets) => { try { - const saved = await saveHostFromForm(editing, data, secrets, selectedVaultIds[0] ?? "personal"); - if (!editing && saved) setEditingId(saved.id); + await saveHostFromForm(editing, data, secrets, selectedVaultIds[0] ?? "personal", (created) => setEditingId(created.id)); } catch (err) { reportSaveError(err); } diff --git a/src/components/keychain/KeychainPage.tsx b/src/components/keychain/KeychainPage.tsx index 6ab390f7b..cb9bcd06d 100644 --- a/src/components/keychain/KeychainPage.tsx +++ b/src/components/keychain/KeychainPage.tsx @@ -461,8 +461,9 @@ export default function KeychainPage() { const handleKeySubmit = async (data: SshKeyFormData, privateKey: string | null, publicKey: string | null, passphrase: string | null) => { try { - const key = await saveKeyFromForm(editingKey, data, privateKey, publicKey, passphrase, selectedVaultIds[0] ?? "personal"); - if (!editingKey) setEditingKeyId(key.id); + await saveKeyFromForm( + editingKey, data, privateKey, publicKey, passphrase, selectedVaultIds[0] ?? "personal", (created) => setEditingKeyId(created.id), + ); } catch (err) { reportSaveError(err); } @@ -474,10 +475,10 @@ export default function KeychainPage() { inlineKeyMaterial?: { label?: string; privateKey: string; publicKey: string }, ) => { try { - const identity = await saveIdentityFromForm( + await saveIdentityFromForm( editingIdentity, data, password, inlineKeyMaterial, inlineKeyIdRef, selectedVaultIds[0] ?? "personal", + (created) => setEditingIdentityId(created.id), ); - if (!editingIdentity) setEditingIdentityId(identity.id); } catch (err) { reportSaveError(err); } diff --git a/src/components/mobile/screens/MobileHostEditScreen.tsx b/src/components/mobile/screens/MobileHostEditScreen.tsx index cec7adf51..4d9100836 100644 --- a/src/components/mobile/screens/MobileHostEditScreen.tsx +++ b/src/components/mobile/screens/MobileHostEditScreen.tsx @@ -41,10 +41,7 @@ export default function MobileHostEditScreen({ hostId }: { hostId?: string }) { ref={formRef} hideChrome initial={editing ?? undefined} - onSubmit={async (data, secrets) => { - const saved = await saveHostFromForm(editing, data, secrets, createVaultId); - if (!editing && saved) setEditingId(saved.id); - }} + onSubmit={(data, secrets) => saveHostFromForm(editing, data, secrets, createVaultId, (created) => setEditingId(created.id))} onClose={pop} vaults={[]} /> diff --git a/src/components/mobile/screens/MobileIdentityEditScreen.tsx b/src/components/mobile/screens/MobileIdentityEditScreen.tsx index d0ff08f76..420992722 100644 --- a/src/components/mobile/screens/MobileIdentityEditScreen.tsx +++ b/src/components/mobile/screens/MobileIdentityEditScreen.tsx @@ -37,12 +37,10 @@ export default function MobileIdentityEditScreen({ identityId }: { identityId?: hideChrome initial={editing ?? undefined} flushRef={flushRef} - onSubmit={async (data, password, inlineKeyMaterial) => { - const saved = await saveIdentityFromForm( - editing, data, password, inlineKeyMaterial, inlineKeyIdRef, createVaultId, - ); - if (!editing) setEditingId(saved.id); - }} + onSubmit={(data, password, inlineKeyMaterial) => + saveIdentityFromForm( + editing, data, password, inlineKeyMaterial, inlineKeyIdRef, createVaultId, (created) => setEditingId(created.id), + )} onClose={pop} /> diff --git a/src/components/mobile/screens/MobileKeyEditScreen.tsx b/src/components/mobile/screens/MobileKeyEditScreen.tsx index 29bf953e2..84910834c 100644 --- a/src/components/mobile/screens/MobileKeyEditScreen.tsx +++ b/src/components/mobile/screens/MobileKeyEditScreen.tsx @@ -39,10 +39,8 @@ export default function MobileKeyEditScreen({ keyId, mode }: { keyId?: string; m initial={editing ?? undefined} initialMode={mode} flushRef={flushRef} - onSubmit={async (data, privateKey, publicKey, passphrase) => { - const saved = await saveKeyFromForm(editing, data, privateKey, publicKey, passphrase, createVaultId); - if (!editing) setEditingId(saved.id); - }} + onSubmit={(data, privateKey, publicKey, passphrase) => + saveKeyFromForm(editing, data, privateKey, publicKey, passphrase, createVaultId, (created) => setEditingId(created.id))} onClose={pop} /> diff --git a/src/services/editorMoveSecrets.test.ts b/src/services/editorMoveSecrets.test.ts index a31e7d085..c481acfa3 100644 --- a/src/services/editorMoveSecrets.test.ts +++ b/src/services/editorMoveSecrets.test.ts @@ -70,17 +70,17 @@ const editors = [ { kind: "host", key: "password:c1", save: (vault_id: string, value: string, from = "personal") => - saveHostFromForm({ id: "c1", vault_id: from } as Connection, { tags: [], vault_id }, { ...none, password: value }, "personal"), + saveHostFromForm({ id: "c1", vault_id: from } as Connection, { tags: [], vault_id }, { ...none, password: value }, "personal", () => {}), }, { kind: "key", key: "key:k1:private", save: (vault_id: string, value: string, from = "personal") => - saveKeyFromForm({ id: "k1", vault_id: from } as SshKey, { tags: [], vault_id }, value, null, null, "personal"), + saveKeyFromForm({ id: "k1", vault_id: from } as SshKey, { tags: [], vault_id }, value, null, null, "personal", () => {}), }, { kind: "identity", key: "identity:i1:password", save: (vault_id: string, value: string, from = "personal") => - saveIdentityFromForm({ id: "i1", vault_id: from } as Identity, { vault_id } as never, value, undefined, { current: null }, "personal"), + saveIdentityFromForm({ id: "i1", vault_id: from } as Identity, { vault_id } as never, value, undefined, { current: null }, "personal", () => {}), }, ]; diff --git a/src/services/hostForm.test.ts b/src/services/hostForm.test.ts index b6ce9f026..9aa627f1a 100644 --- a/src/services/hostForm.test.ts +++ b/src/services/hostForm.test.ts @@ -39,7 +39,7 @@ describe("saveHostFromForm", () => { storeSecret.mockImplementation(async (k: string) => { calls.push(`store ${k}`); }); const personal = { id: "c1", vault_id: "personal" } as never; - await saveHostFromForm(personal, { tags: [], vault_id: "v-other" }, { ...none, password: "new" }, "personal"); + await saveHostFromForm(personal, { tags: [], vault_id: "v-other" }, { ...none, password: "new" }, "personal", () => {}); expect(moveWithSecrets).toHaveBeenCalledWith("connection", personal, "v-other", expect.any(Function), [ ["password:c1", "new"], ["key:c1", null], ["passphrase:c1", null], ["proxy_password:c1", null], ["knock_sequence:c1", null], @@ -49,31 +49,49 @@ describe("saveHostFromForm", () => { }); it("stores the proxy password locally", async () => { - await saveHostFromForm(editing, { tags: [] }, { ...none, proxy_password: "pp" }, "personal"); + await saveHostFromForm(editing, { tags: [] }, { ...none, proxy_password: "pp" }, "personal", () => {}); expect(storeSecret).toHaveBeenCalledWith("proxy_password:c1", "pp"); }); it("stores the knock sequence locally", async () => { - await saveHostFromForm(editing, { tags: [] }, { ...none, knock_sequence: "666/tcp" }, "personal"); + await saveHostFromForm(editing, { tags: [] }, { ...none, knock_sequence: "666/tcp" }, "personal", () => {}); expect(storeSecret).toHaveBeenCalledWith("knock_sequence:c1", "666/tcp"); }); it("clears the proxy password when emptied", async () => { - await saveHostFromForm(editing, { tags: [] }, { ...none, proxy_password: "" }, "personal"); + await saveHostFromForm(editing, { tags: [] }, { ...none, proxy_password: "" }, "personal", () => {}); expect(deleteSecret).toHaveBeenCalledWith("proxy_password:c1"); }); it("an edit that clears a password rejects when deleteSecret rejects", async () => { deleteSecret.mockRejectedValueOnce(new Error("withdraw failed")); await expect( - saveHostFromForm(editing, { tags: [] }, { ...none, password: "" }, "personal"), + saveHostFromForm(editing, { tags: [] }, { ...none, password: "" }, "personal", () => {}), ).rejects.toThrow("withdraw failed"); }); it("an upload failure while saving a host form reaches the caller", async () => { storeSecret.mockRejectedValue(new TeamSecretUploadError("password:c1", new Error("403"))); await expect( - saveHostFromForm(editing, { tags: [] }, { ...none, password: "pw" }, "v1"), + saveHostFromForm(editing, { tags: [] }, { ...none, password: "pw" }, "v1", () => {}), ).rejects.toBeInstanceOf(TeamSecretUploadError); }); + + it("a new host whose secret save fails is announced first, so the next save edits it instead of creating another", async () => { + saveConnection.mockResolvedValue({ id: "n1" }); + storeSecret.mockRejectedValue(new TeamSecretUploadError("password:n1", new Error("403"))); + const created = vi.fn(); + + await expect( + saveHostFromForm(null, { tags: [] }, { ...none, password: "pw" }, "team-1", created), + ).rejects.toBeInstanceOf(TeamSecretUploadError); + + expect(created).toHaveBeenCalledWith({ id: "n1" }); + }); + + it("an edit announces nothing", async () => { + const created = vi.fn(); + await saveHostFromForm(editing, { tags: [] }, none, "personal", created); + expect(created).not.toHaveBeenCalled(); + }); }); diff --git a/src/services/hostForm.ts b/src/services/hostForm.ts index 1f20b219c..776259e65 100644 --- a/src/services/hostForm.ts +++ b/src/services/hostForm.ts @@ -1,7 +1,7 @@ import type { Connection, ConnectionFormData } from "@/types"; import { useConnectionStore } from "@/stores/connectionStore"; import { CONNECTION_SECRET_FIELDS, CONNECTION_SECRET_KEYS, type ConnectionSecretField } from "@/services/teamVaultSecretKeys"; -import { moveWithSecrets, storeNewSecrets, type SecretEdit } from "@/services/vaultObjectSecrets"; +import { moveWithSecrets, createWithSecrets, type SecretEdit } from "@/services/vaultObjectSecrets"; export type HostFormSecrets = Record; @@ -17,13 +17,15 @@ export async function saveHostFromForm( data: ConnectionFormData, secrets: HostFormSecrets, fallbackVaultId: string, -): Promise { + onCreated: (created: Connection) => void, +): Promise { const { updateConnection, saveConnection } = useConnectionStore.getState(); if (editing) { - await moveWithSecrets("connection", editing, data.vault_id, () => updateConnection(editing.id, data), secretEdits(editing.id, secrets)); - return editing; + return moveWithSecrets("connection", editing, data.vault_id, () => updateConnection(editing.id, data), secretEdits(editing.id, secrets)); } - const conn = await saveConnection({ ...data, vault_id: data.vault_id ?? fallbackVaultId }); - if (conn) await storeNewSecrets(secretEdits(conn.id, secrets)); - return conn ?? null; + await createWithSecrets( + () => saveConnection({ ...data, vault_id: data.vault_id ?? fallbackVaultId }), + (id) => secretEdits(id, secrets), + onCreated, + ); } diff --git a/src/services/keychainForm.test.ts b/src/services/keychainForm.test.ts index 882f683dd..87cf78f02 100644 --- a/src/services/keychainForm.test.ts +++ b/src/services/keychainForm.test.ts @@ -47,7 +47,7 @@ beforeEach(() => { describe("editing hands the edited fields to the move", () => { test("a key passes its halves, written after a same-location update", async () => { - await saveKeyFromForm(existing, { tags: [], vault_id: "v-other" }, null, null, "pass", "personal"); + await saveKeyFromForm(existing, { tags: [], vault_id: "v-other" }, null, null, "pass", "personal", () => {}); expect(h.moveWithSecrets).toHaveBeenCalledWith("key", existing, "v-other", expect.any(Function), [ ["key:k1:private", null], ["key:k1:public", null], ["key:k1:passphrase", "pass"], ]); @@ -57,7 +57,7 @@ describe("editing hands the edited fields to the move", () => { test("an identity passes its password, written after a same-location update", async () => { const identity = { id: "i1", vault_id: "v-other" } as Identity; - await saveIdentityFromForm(identity, { vault_id: "personal" } as never, "pw", undefined, { current: null }, "personal"); + await saveIdentityFromForm(identity, { vault_id: "personal" } as never, "pw", undefined, { current: null }, "personal", () => {}); expect(h.moveWithSecrets).toHaveBeenCalledWith("identity", identity, "personal", expect.any(Function), [ ["identity:i1:password", "pw"], ]); @@ -68,7 +68,7 @@ describe("editing hands the edited fields to the move", () => { describe("saveKeyFromForm", () => { test("creates the key, then stores each half under its own secret name", async () => { - await saveKeyFromForm(null, { tags: [] }, "PRIV", "PUB", "pass", "personal"); + await saveKeyFromForm(null, { tags: [] }, "PRIV", "PUB", "pass", "personal", () => {}); expect(saveKey).toHaveBeenCalledWith({ tags: [], vault_id: "personal" }); expect(storeSecret.mock.calls).toEqual([ ["key:new:private", "PRIV"], @@ -79,14 +79,14 @@ describe("saveKeyFromForm", () => { }); test("a null half is left untouched, an emptied one is deleted on edit", async () => { - await saveKeyFromForm(existing, { tags: [] }, null, "", "pass", "personal"); + await saveKeyFromForm(existing, { tags: [] }, null, "", "pass", "personal", () => {}); expect(updateKey).toHaveBeenCalledWith("k1", { tags: [] }); expect(storeSecret.mock.calls).toEqual([["key:k1:passphrase", "pass"]]); expect(deleteSecret.mock.calls).toEqual([["key:k1:public"]]); }); test("an empty half on create writes nothing rather than deleting", async () => { - await saveKeyFromForm(null, { tags: [] }, "PRIV", "", "", "personal"); + await saveKeyFromForm(null, { tags: [] }, "PRIV", "", "", "personal", () => {}); expect(storeSecret.mock.calls).toEqual([["key:new:private", "PRIV"]]); expect(deleteSecret).not.toHaveBeenCalled(); }); @@ -95,11 +95,42 @@ describe("saveKeyFromForm", () => { describe("saveIdentityFromForm", () => { test("an inline key is filed in the identity's vault, not the personal one", async () => { h.saveIdentity.mockResolvedValue({ id: "i-new" }); - await saveIdentityFromForm(null, { username: "root", tags: [] }, null, { privateKey: "PRIV", publicKey: "" }, { current: null }, "team-1"); + await saveIdentityFromForm(null, { username: "root", tags: [] }, null, { privateKey: "PRIV", publicKey: "" }, { current: null }, "team-1", () => {}); expect(saveKey).toHaveBeenCalledWith(expect.objectContaining({ vault_id: "team-1" })); }); }); +describe("a new object whose secret save fails", () => { + const refused = new TeamSecretUploadError("k", new Error("403")); + + test("a key is announced first, so the next save edits it instead of creating another", async () => { + h.storeSecret.mockRejectedValue(refused); + const created = vi.fn(); + + await expect(saveKeyFromForm(null, { tags: [] }, "PRIV", null, null, "team-1", created)).rejects.toBe(refused); + + expect(created).toHaveBeenCalledWith({ id: "new", vault_id: "personal" }); + }); + + test("an identity is announced first too", async () => { + h.saveIdentity.mockResolvedValue({ id: "i-new" }); + h.storeSecret.mockRejectedValue(refused); + const created = vi.fn(); + + await expect( + saveIdentityFromForm(null, { username: "root", tags: [] }, "pw", undefined, { current: null }, "team-1", created), + ).rejects.toBe(refused); + + expect(created).toHaveBeenCalledWith({ id: "i-new" }); + }); + + test("an edit announces nothing", async () => { + const created = vi.fn(); + await saveKeyFromForm(existing, { tags: [] }, null, null, null, "personal", created); + expect(created).not.toHaveBeenCalled(); + }); +}); + describe("unlinkIdentityFromHost", () => { const identity = { id: "i1", username: "root", key_id: "k1" } as Identity; const host = { id: "c1", name: "web", host: "h", port: 22, tags: [] } as never; diff --git a/src/services/keychainForm.ts b/src/services/keychainForm.ts index d80b02826..60147a09d 100644 --- a/src/services/keychainForm.ts +++ b/src/services/keychainForm.ts @@ -3,7 +3,7 @@ import { useIdentityStore } from "@/stores/identityStore"; import { connectionToFormData } from "@/stores/connectionStore"; import { storeSecret, getSecret } from "@/services/vault"; import { reportUploadFailure } from "@/services/secretRouting"; -import { moveWithSecrets, storeNewSecrets, type SecretEdit } from "@/services/vaultObjectSecrets"; +import { moveWithSecrets, createWithSecrets, type SecretEdit } from "@/services/vaultObjectSecrets"; import type { AuthType, Connection, ConnectionFormData, Identity, IdentityFormData, SshKey, SshKeyFormData } from "@/types"; type InlineKeyMaterial = { label?: string; privateKey: string; publicKey: string }; @@ -15,20 +15,16 @@ export async function saveKeyFromForm( publicKey: string | null, passphrase: string | null, fallbackVaultId: string, -): Promise { + onCreated: (created: SshKey) => void, +): Promise { const { saveKey, updateKey } = useKeyStore.getState(); const edits = (id: string): SecretEdit[] => [ [`key:${id}:private`, privateKey], [`key:${id}:public`, publicKey], [`key:${id}:passphrase`, passphrase], ]; - if (editing) { - await moveWithSecrets("key", editing, data.vault_id, () => updateKey(editing.id, data), edits(editing.id)); - return editing; - } - const key = await saveKey({ ...data, vault_id: data.vault_id ?? fallbackVaultId }); - await storeNewSecrets(edits(key.id)); - return key; + if (editing) return moveWithSecrets("key", editing, data.vault_id, () => updateKey(editing.id, data), edits(editing.id)); + await createWithSecrets(() => saveKey({ ...data, vault_id: data.vault_id ?? fallbackVaultId }), edits, onCreated); } /** `inlineKeyId` carries the key a previous autosave pass created, so repeated @@ -40,7 +36,8 @@ export async function saveIdentityFromForm( inlineKeyMaterial: InlineKeyMaterial | undefined, inlineKeyId: { current: string | null }, fallbackVaultId: string, -): Promise { + onCreated: (created: Identity) => void, +): Promise { const { saveIdentity, updateIdentity } = useIdentityStore.getState(); let resolvedData = data; @@ -57,12 +54,13 @@ export async function saveIdentityFromForm( const edits = (id: string): SecretEdit[] => [[`identity:${id}:password`, password]]; if (editing) { - await moveWithSecrets("identity", editing, resolvedData.vault_id, () => updateIdentity(editing.id, resolvedData), edits(editing.id)); - return editing; + return moveWithSecrets("identity", editing, resolvedData.vault_id, () => updateIdentity(editing.id, resolvedData), edits(editing.id)); } - const identity = await saveIdentity({ ...resolvedData, vault_id: resolvedData.vault_id ?? fallbackVaultId }); - await storeNewSecrets(edits(identity.id)); - return identity; + await createWithSecrets( + () => saveIdentity({ ...resolvedData, vault_id: resolvedData.vault_id ?? fallbackVaultId }), + edits, + onCreated, + ); } export async function unlinkIdentityFromHost( diff --git a/src/services/vaultObjectSecrets.ts b/src/services/vaultObjectSecrets.ts index 5c6f10781..6057294e6 100644 --- a/src/services/vaultObjectSecrets.ts +++ b/src/services/vaultObjectSecrets.ts @@ -96,7 +96,17 @@ async function applyEdits( } } -export const storeNewSecrets = (edits: readonly SecretEdit[]) => applyEdits(edits, storeSecret, async () => {}); +export async function createWithSecrets( + create: () => Promise, + edits: (id: string) => readonly SecretEdit[], + onCreated: (created: T) => void, +): Promise { + const created = await create(); + if (!created) return; + // Announced before its secrets: a caller that never learns the id creates the object again on its next save. + onCreated(created); + await applyEdits(edits(created.id), storeSecret, async () => {}); +} export async function moveWithSecrets( kind: SecretObjectKind,