diff --git a/src/channels/accounts.ts b/src/channels/accounts.ts index cc562c1a62..7e73525d1b 100644 --- a/src/channels/accounts.ts +++ b/src/channels/accounts.ts @@ -84,9 +84,36 @@ interface ChannelAccountStore { export const LEGACY_CHANNEL_ACCOUNT_ID = "__legacy_migrated__"; const stores = new Map(); +const channelSecretOperationTails = new Map>(); const CHANNEL_SECRET_REFS_KEY = "__letta_secret_refs"; const SECRET_PRESENT_PLACEHOLDER = "__letta_channel_secret_present__"; -const pendingSecretWrites: Promise[] = []; + +async function runSerializedChannelSecretOperation( + channelId: string, + operation: () => Promise, +): Promise { + // The account file is shared by every account in a channel, so serialize the + // full keyring + in-memory + file commit in this process. Separate processes + // remain last-writer-wins because OS keyrings provide no conditional writes. + const previous = + channelSecretOperationTails.get(channelId) ?? Promise.resolve(); + let release: () => void = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + const tail = previous.then(() => gate); + channelSecretOperationTails.set(channelId, tail); + + await previous; + try { + return await operation(); + } finally { + release(); + if (channelSecretOperationTails.get(channelId) === tail) { + channelSecretOperationTails.delete(channelId); + } + } +} type ChannelAccountWithSecretRefs = ChannelAccount & { [CHANNEL_SECRET_REFS_KEY]?: Record; @@ -180,34 +207,29 @@ function applySecretPlaceholders(account: ChannelAccount): void { } } -function queueSecretWrite(promise: Promise): void { - pendingSecretWrites.push( - promise.catch(() => { - // Best-effort background secret persistence. Foreground commands that - // need to validate credentials surface errors explicitly; detached secret - // writes should not spam startup logs or crash the process. - }), - ); -} - -function prepareAccountForStorage(account: ChannelAccount): ChannelAccount { +function prepareAccountForStorage( + account: ChannelAccount, + options: { redactPersistedSecrets?: boolean } = {}, +): ChannelAccount { const cloned = cloneAccount(account) as ChannelAccountWithSecretRefs; if (getCachedChannelCredentialsStoreMode() !== "keyring") { delete cloned[CHANNEL_SECRET_REFS_KEY]; return cloned; } + const existingSecretRefs = cloned[CHANNEL_SECRET_REFS_KEY]; delete cloned[CHANNEL_SECRET_REFS_KEY]; for (const fieldPath of getSecretFieldPaths(cloned)) { const value = getSecretValueFromAccount(cloned, fieldPath); if (typeof value === "string" && value.trim().length > 0) { - markSecretRef(cloned, fieldPath); - if (!isSecretPlaceholder(value)) { - queueSecretWrite( - setChannelSecret(cloned.channel, cloned.accountId, fieldPath, value), - ); + if ( + isSecretPlaceholder(value) || + existingSecretRefs?.[fieldPath] === true || + options.redactPersistedSecrets + ) { + markSecretRef(cloned, fieldPath); + deleteSecretValueFromAccount(cloned, fieldPath); } - deleteSecretValueFromAccount(cloned, fieldPath); } } @@ -221,6 +243,134 @@ function normalizeInboundDebounceMs(value: unknown): number | undefined { return Math.trunc(Math.min(value, 10000)); } +interface AccountSecretWrite { + fieldPath: string; + value: string; +} + +function getAccountSecretWrites(account: ChannelAccount): AccountSecretWrite[] { + return getSecretFieldPaths(account).flatMap((fieldPath) => { + const value = getSecretValueFromAccount(account, fieldPath); + if ( + typeof value !== "string" || + value.trim().length === 0 || + isSecretPlaceholder(value) + ) { + return []; + } + return [{ fieldPath, value }]; + }); +} + +function getSecretPersistenceErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +async function persistAccountSecretsForKeyring( + account: ChannelAccount, +): Promise { + if (getCachedChannelCredentialsStoreMode() !== "keyring") { + return []; + } + + const accountSecrets = getAccountSecretWrites(account); + const existingAccount = getStore(account.channel).accounts.find( + (entry) => entry.accountId === account.accountId, + ); + const existingRefs = existingAccount ? getSecretRefs(existingAccount) : {}; + const writes = accountSecrets.filter(({ fieldPath, value }) => { + if (!existingAccount || existingRefs[fieldPath] !== true) { + return true; + } + return getSecretValueFromAccount(existingAccount, fieldPath) !== value; + }); + + // One field maps to one backend operation. Multi-field credentials need an + // application-level transaction because OS keyrings expose no shared commit. + if (writes.length === 1) { + const write = writes[0]; + if (write) { + await setChannelSecret( + account.channel, + account.accountId, + write.fieldPath, + write.value, + ); + } + return accountSecrets.map(({ fieldPath }) => fieldPath); + } + + if (writes.length > 1) { + const writesWithOldValues = await Promise.all( + writes.map(async (write) => ({ + ...write, + oldValue: await getChannelSecret( + account.channel, + account.accountId, + write.fieldPath, + ), + })), + ); + + const completedWrites: typeof writesWithOldValues = []; + try { + for (const write of writesWithOldValues) { + await setChannelSecret( + account.channel, + account.accountId, + write.fieldPath, + write.value, + ); + completedWrites.push(write); + } + } catch (error) { + const rollbackErrors: Error[] = []; + for (const { fieldPath, oldValue } of completedWrites.reverse()) { + try { + if (oldValue === null) { + await deleteChannelSecret( + account.channel, + account.accountId, + fieldPath, + ); + } else { + await setChannelSecret( + account.channel, + account.accountId, + fieldPath, + oldValue, + ); + } + } catch (rollbackError) { + rollbackErrors.push( + new Error( + `Failed to restore ${fieldPath}: ${getSecretPersistenceErrorMessage( + rollbackError, + )}`, + ), + ); + } + } + + if (rollbackErrors.length > 0) { + const originalError = + error instanceof Error + ? error + : new Error(getSecretPersistenceErrorMessage(error)); + throw new AggregateError( + [originalError, ...rollbackErrors], + `Failed to persist channel credentials: ${originalError.message}. Credential rollback also failed for ${rollbackErrors + .map((rollbackError) => rollbackError.message) + .join("; ")}`, + ); + } + throw error; + } + } + + return accountSecrets.map(({ fieldPath }) => fieldPath); +} + function cloneAccount(account: T): T { const cloned = { ...account, @@ -602,10 +752,13 @@ export function loadChannelAccounts(channelId: string): void { stores.set(channelId, { accounts: [] }); } -function saveChannelAccounts(channelId: string): void { +function saveChannelAccounts( + channelId: string, + options: { redactPersistedSecrets?: boolean } = {}, +): void { const store = getStore(channelId); const writeAccounts = store.accounts.map((account) => { - const cloned = prepareAccountForStorage(account); + const cloned = prepareAccountForStorage(account, options); // Canonicalize: convert camelCase keys to snake_case for storage for (const [snakeKey, camelKey] of Object.entries(SNAKE_TO_CAMEL)) { const value = (cloned as unknown as Record)[camelKey]; @@ -643,13 +796,11 @@ function saveChannelAccounts(channelId: string): void { } export async function flushPendingChannelSecretWrites(): Promise { - while (pendingSecretWrites.length > 0) { - const writes = pendingSecretWrites.splice(0, pendingSecretWrites.length); - await Promise.all(writes); - } + // Writes are awaited before redaction now. Keep this exported helper as a + // compatibility no-op for tests and callers from older channel code paths. } -export async function hydrateChannelAccountSecrets( +async function hydrateChannelAccountSecretsUnlocked( channelId: string, ): Promise { const mode = await getActiveChannelCredentialsStoreMode(); @@ -688,11 +839,19 @@ export async function hydrateChannelAccountSecrets( } if (migratedPlaintextSecrets) { - saveChannelAccounts(channelId); + saveChannelAccounts(channelId, { redactPersistedSecrets: true }); await flushPendingChannelSecretWrites(); } } +export async function hydrateChannelAccountSecrets( + channelId: string, +): Promise { + await runSerializedChannelSecretOperation(channelId, () => + hydrateChannelAccountSecretsUnlocked(channelId), + ); +} + export function listChannelAccounts(channelId: string): ChannelAccount[] { return getStore(channelId).accounts.map((account) => cloneAccount(account)); } @@ -722,9 +881,10 @@ export async function getChannelAccountWithSecrets( return getChannelAccount(channelId, accountId); } -export function upsertChannelAccount( +function upsertChannelAccountInternal( channelId: string, account: ChannelAccount, + saveOptions: { redactPersistedSecrets?: boolean } = {}, ): ChannelAccount { const store = getStore(channelId); const next = cloneAccount(account); @@ -736,18 +896,38 @@ export function upsertChannelAccount( } else { store.accounts.push(next); } - saveChannelAccounts(channelId); + saveChannelAccounts(channelId, saveOptions); return cloneAccount(next); } +export function upsertChannelAccount( + channelId: string, + account: ChannelAccount, +): ChannelAccount { + return upsertChannelAccountInternal(channelId, account); +} + export async function upsertChannelAccountWithSecrets( channelId: string, account: ChannelAccount, ): Promise { - await getActiveChannelCredentialsStoreMode(); - const next = upsertChannelAccount(channelId, account); - await flushPendingChannelSecretWrites(); - return next; + return runSerializedChannelSecretOperation(channelId, async () => { + await getActiveChannelCredentialsStoreMode(); + const persistedSecretFields = + await persistAccountSecretsForKeyring(account); + const nextAccount = cloneAccount(account); + for (const fieldPath of persistedSecretFields) { + // Keep runtime credentials hydrated while carrying refs through later + // account mutations such as route binding. + markSecretRef(nextAccount, fieldPath); + } + const next = upsertChannelAccountInternal(channelId, nextAccount, { + redactPersistedSecrets: + getCachedChannelCredentialsStoreMode() === "keyring", + }); + await flushPendingChannelSecretWrites(); + return next; + }); } export function removeChannelAccount( @@ -770,16 +950,23 @@ export async function removeChannelAccountWithSecrets( channelId: string, accountId: string, ): Promise { - await hydrateChannelAccountSecrets(channelId); - const account = getChannelAccount(channelId, accountId); - if (account && getCachedChannelCredentialsStoreMode() === "keyring") { - await Promise.all( - getSecretFieldPaths(account).map((fieldPath) => - deleteChannelSecret(channelId, accountId, fieldPath), - ), - ); - } - return removeChannelAccount(channelId, accountId); + return runSerializedChannelSecretOperation(channelId, async () => { + await getActiveChannelCredentialsStoreMode(); + const account = getChannelAccount(channelId, accountId); + const secretFieldPaths = + account && getCachedChannelCredentialsStoreMode() === "keyring" + ? getSecretFieldPaths(account) + : []; + const removed = removeChannelAccount(channelId, accountId); + if (removed && secretFieldPaths.length > 0) { + await Promise.all( + secretFieldPaths.map((fieldPath) => + deleteChannelSecret(channelId, accountId, fieldPath), + ), + ); + } + return removed; + }); } export function clearChannelAccountStores(): void { diff --git a/src/channels/credential-store.test.ts b/src/channels/credential-store.test.ts index 67cfd87af7..43aed4f795 100644 --- a/src/channels/credential-store.test.ts +++ b/src/channels/credential-store.test.ts @@ -15,6 +15,7 @@ import { getChannelAccountWithSecrets, hydrateChannelAccountSecrets, removeChannelAccountWithSecrets, + upsertChannelAccount, upsertChannelAccountWithSecrets, } from "@/channels/accounts"; import { __testOverrideChannelsRoot } from "@/channels/config"; @@ -26,6 +27,10 @@ import { buildChannelSecretName, getActiveChannelCredentialsStoreMode, } from "@/channels/credential-store"; +import { + bindChannelAccountLive, + createChannelAccountLiveWithSecrets, +} from "@/channels/service-accounts"; import type { SlackChannelAccount, TelegramChannelAccount, @@ -141,6 +146,323 @@ describe("channel credential storage", () => { expect(hydrated?.appToken).toBe("xapp-secret"); }); + test("secure Telegram create preserves refs through synchronous account binding", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + + await createChannelAccountLiveWithSecrets( + "telegram", + { + displayName: "Telegram Bot", + enabled: false, + dmPolicy: "pairing", + allowedUsers: [], + config: { + token: "telegram-secret", + transcribe_voice: false, + }, + }, + { accountId: "telegram-account" }, + ); + + bindChannelAccountLive( + "telegram", + "telegram-account", + "agent-1", + "conversation-1", + ); + + const persistedText = readFileSync( + join(channelsRoot, "telegram", "accounts.json"), + "utf-8", + ); + expect(persistedText).not.toContain("telegram-secret"); + const persisted = JSON.parse(persistedText) as { + accounts: Array>; + }; + expect(persisted.accounts[0]).toMatchObject({ + binding: { + agentId: "agent-1", + conversationId: "conversation-1", + }, + __letta_secret_refs: { token: true }, + }); + expect( + secrets.get( + buildChannelSecretName("telegram", "telegram-account", "token"), + ), + ).toBe("telegram-secret"); + }); + + test("failed multi-secret update restores keyring and persisted account state", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + await upsertChannelAccountWithSecrets("slack", makeSlackAccount()); + + const botTokenName = buildChannelSecretName( + "slack", + "slack-account", + "botToken", + ); + const appTokenName = buildChannelSecretName( + "slack", + "slack-account", + "appToken", + ); + __setChannelSecretStoreOverrideForTests({ + get: async (name) => secrets.get(name) ?? null, + set: async (name, value) => { + if (name === appTokenName && value === "xapp-new") { + throw new Error("second keyring write failed"); + } + secrets.set(name, value); + }, + delete: async (name) => secrets.delete(name), + }); + + await expect( + upsertChannelAccountWithSecrets("slack", { + ...makeSlackAccount(), + botToken: "xoxb-new", + appToken: "xapp-new", + }), + ).rejects.toThrow("second keyring write failed"); + + expect(secrets.get(botTokenName)).toBe("xoxb-secret"); + expect(secrets.get(appTokenName)).toBe("xapp-secret"); + const persistedText = readFileSync( + join(channelsRoot, "slack", "accounts.json"), + "utf-8", + ); + expect(persistedText).not.toContain("xoxb-secret"); + expect(persistedText).not.toContain("xapp-secret"); + expect(persistedText).not.toContain("xoxb-new"); + expect(persistedText).not.toContain("xapp-new"); + expect(JSON.parse(persistedText).accounts[0]).toMatchObject({ + __letta_secret_refs: { + botToken: true, + appToken: true, + }, + }); + + clearChannelAccountStores(); + const rehydrated = (await getChannelAccountWithSecrets( + "slack", + "slack-account", + )) as SlackChannelAccount | null; + expect(rehydrated?.botToken).toBe("xoxb-secret"); + expect(rehydrated?.appToken).toBe("xapp-secret"); + }); + + test("serializes concurrent updates before rollback and file commit", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + await upsertChannelAccountWithSecrets("slack", makeSlackAccount()); + + const botTokenName = buildChannelSecretName( + "slack", + "slack-account", + "botToken", + ); + const appTokenName = buildChannelSecretName( + "slack", + "slack-account", + "appToken", + ); + let signalFirstUpdateAtSecondWrite: () => void = () => {}; + const firstUpdateAtSecondWrite = new Promise((resolve) => { + signalFirstUpdateAtSecondWrite = resolve; + }); + let releaseFirstUpdateFailure: () => void = () => {}; + const allowFirstUpdateFailure = new Promise((resolve) => { + releaseFirstUpdateFailure = resolve; + }); + __setChannelSecretStoreOverrideForTests({ + get: async (name) => secrets.get(name) ?? null, + set: async (name, value) => { + if (name === appTokenName && value === "xapp-first") { + signalFirstUpdateAtSecondWrite(); + await allowFirstUpdateFailure; + throw new Error("first update second write failed"); + } + secrets.set(name, value); + }, + delete: async (name) => secrets.delete(name), + }); + + const firstUpdate = upsertChannelAccountWithSecrets("slack", { + ...makeSlackAccount(), + botToken: "xoxb-first", + appToken: "xapp-first", + }); + await firstUpdateAtSecondWrite; + + let secondUpdateSettled = false; + const secondUpdate = upsertChannelAccountWithSecrets("slack", { + ...makeSlackAccount(), + appToken: "xapp-second", + }).then( + (account) => { + secondUpdateSettled = true; + return account; + }, + (error) => { + secondUpdateSettled = true; + throw error; + }, + ); + await Promise.resolve(); + expect(secondUpdateSettled).toBe(false); + + releaseFirstUpdateFailure(); + await expect(firstUpdate).rejects.toThrow( + "first update second write failed", + ); + const secondAccount = (await secondUpdate) as SlackChannelAccount; + + expect(secondAccount.botToken).toBe("xoxb-secret"); + expect(secondAccount.appToken).toBe("xapp-second"); + expect(secrets.get(botTokenName)).toBe("xoxb-secret"); + expect(secrets.get(appTokenName)).toBe("xapp-second"); + const persistedText = readFileSync( + join(channelsRoot, "slack", "accounts.json"), + "utf-8", + ); + expect(persistedText).not.toContain("xoxb-secret"); + expect(persistedText).not.toContain("xoxb-first"); + expect(persistedText).not.toContain("xapp-first"); + expect(persistedText).not.toContain("xapp-second"); + expect(JSON.parse(persistedText).accounts[0]).toMatchObject({ + __letta_secret_refs: { + botToken: true, + appToken: true, + }, + }); + + clearChannelAccountStores(); + const rehydrated = (await getChannelAccountWithSecrets( + "slack", + "slack-account", + )) as SlackChannelAccount | null; + expect(rehydrated?.botToken).toBe("xoxb-secret"); + expect(rehydrated?.appToken).toBe("xapp-second"); + }); + + test("reports the original write error together with rollback failures", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + await upsertChannelAccountWithSecrets("slack", makeSlackAccount()); + + const botTokenName = buildChannelSecretName( + "slack", + "slack-account", + "botToken", + ); + const appTokenName = buildChannelSecretName( + "slack", + "slack-account", + "appToken", + ); + __setChannelSecretStoreOverrideForTests({ + get: async (name) => secrets.get(name) ?? null, + set: async (name, value) => { + if (name === appTokenName && value === "xapp-new") { + throw new Error("second keyring write failed"); + } + if (name === botTokenName && value === "xoxb-secret") { + throw new Error("bot token rollback failed"); + } + secrets.set(name, value); + }, + delete: async (name) => secrets.delete(name), + }); + + try { + await upsertChannelAccountWithSecrets("slack", { + ...makeSlackAccount(), + botToken: "xoxb-new", + appToken: "xapp-new", + }); + throw new Error("Expected secure account update to fail"); + } catch (error) { + expect(error).toBeInstanceOf(AggregateError); + expect(error).toHaveProperty( + "message", + expect.stringContaining("second keyring write failed"), + ); + expect(error).toHaveProperty( + "message", + expect.stringContaining("Failed to restore botToken"), + ); + expect((error as AggregateError).errors[0]).toHaveProperty( + "message", + "second keyring write failed", + ); + } + }); + + test("sync saves keep hydrated keyring credentials redacted", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + + await upsertChannelAccountWithSecrets("slack", makeSlackAccount()); + clearChannelAccountStores(); + const hydrated = (await getChannelAccountWithSecrets( + "slack", + "slack-account", + )) as SlackChannelAccount | null; + if (!hydrated) { + throw new Error("Expected hydrated Slack account"); + } + + upsertChannelAccount("slack", { ...hydrated, enabled: false }); + + const persistedText = readFileSync( + join(channelsRoot, "slack", "accounts.json"), + "utf-8", + ); + expect(persistedText).not.toContain("xoxb-secret"); + expect(persistedText).not.toContain("xapp-secret"); + expect(persistedText).toContain("__letta_secret_refs"); + }); + + test("failed multi-secret create removes newly written keyring values", async () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + const appTokenName = buildChannelSecretName( + "slack", + "slack-account", + "appToken", + ); + __setChannelSecretStoreOverrideForTests({ + get: async (name) => secrets.get(name) ?? null, + set: async (name, value) => { + if (name === appTokenName) { + throw new Error("keyring rejected second secret"); + } + secrets.set(name, value); + }, + delete: async (name) => secrets.delete(name), + }); + + await expect( + upsertChannelAccountWithSecrets("slack", makeSlackAccount()), + ).rejects.toThrow("keyring rejected second secret"); + + expect(secrets.size).toBe(0); + expect(existsSync(join(channelsRoot, "slack", "accounts.json"))).toBe( + false, + ); + }); + + test("sync keyring save keeps plaintext until secrets are persisted", () => { + __setActiveChannelCredentialsStoreModeForTests("keyring"); + + upsertChannelAccount("slack", makeSlackAccount()); + + const persistedText = readFileSync( + join(channelsRoot, "slack", "accounts.json"), + "utf-8", + ); + expect(persistedText).toContain("xoxb-secret"); + expect(persistedText).toContain("xapp-secret"); + expect(persistedText).not.toContain("__letta_secret_refs"); + }); + test("keyring mode migrates existing plaintext tokens out of accounts.json", async () => { __setActiveChannelCredentialsStoreModeForTests("keyring"); mkdirSync(join(channelsRoot, "slack"), { recursive: true }); diff --git a/src/channels/protocol-account-commands.ts b/src/channels/protocol-account-commands.ts index 7d597698bd..d9d9ef88c3 100644 --- a/src/channels/protocol-account-commands.ts +++ b/src/channels/protocol-account-commands.ts @@ -42,8 +42,8 @@ export async function handleAccountConfigLifecycleCommand( startChannelLive, stopChannelAccountLive, stopChannelLive, - createChannelAccountLive, - updateChannelAccountLive, + createChannelAccountLiveWithSecrets, + updateChannelAccountLiveWithSecrets, bindChannelAccountLive, unbindChannelAccountLive, } = service; @@ -167,7 +167,7 @@ export async function handleAccountConfigLifecycleCommand( const pluginConfig = getChannelPluginConfig(parsed.account as Record) ?? {}; - const created = createChannelAccountLive( + const created = await createChannelAccountLiveWithSecrets( effectiveChannelId, { displayName: @@ -251,7 +251,7 @@ export async function handleAccountConfigLifecycleCommand( allowedUsers: parsed.patch.allowed_users, config: pluginConfig, }; - const account = updateChannelAccountLive( + const account = await updateChannelAccountLiveWithSecrets( parsed.channel_id, parsed.account_id, accountPatch, diff --git a/src/channels/protocol-command-handler.test.ts b/src/channels/protocol-command-handler.test.ts index a349c8fcbc..a3031d7774 100644 --- a/src/channels/protocol-command-handler.test.ts +++ b/src/channels/protocol-command-handler.test.ts @@ -107,17 +107,17 @@ function findMessage( return parseMessages(socket).find((message) => message.type === type); } -async function expectCommandCompletesWithoutSecretFlush( +async function expectCommandWaitsForSecretFlush( commandPromise: Promise, ): Promise { const result = await Promise.race([ commandPromise.then(() => "completed" as const), - new Promise<"timed-out">((resolve) => { - setTimeout(() => resolve("timed-out"), 250); + new Promise<"pending">((resolve) => { + setTimeout(() => resolve("pending"), 250); }), ]); - expect(result).toBe("completed"); + expect(result).toBe("pending"); } describe("channel account list responses", () => { @@ -572,7 +572,7 @@ describe("channel account list responses", () => { } }); - test("Telegram account protocol commands complete while keyring writes are pending", async () => { + test("Telegram account protocol commands await keyring writes before responding", async () => { setupInMemoryChannelStores(); const pendingSecretOperations: Array<() => void> = []; @@ -618,7 +618,14 @@ describe("channel account list responses", () => { runtime, ); commandPromises.push(createPromise); - await expectCommandCompletesWithoutSecretFlush(createPromise); + await expectCommandWaitsForSecretFlush(createPromise); + + expect( + findMessage(socket, "channel_account_create_response"), + ).toBeUndefined(); + expect(pendingSecretOperations).toHaveLength(1); + pendingSecretOperations.shift()?.(); + await createPromise; expect( findMessage(socket, "channel_account_create_response"), @@ -644,7 +651,7 @@ describe("channel account list responses", () => { }, }, }); - expect(pendingSecretOperations).toHaveLength(1); + expect(pendingSecretOperations).toHaveLength(0); const updatePromise = sendChannelCommand( { @@ -667,7 +674,14 @@ describe("channel account list responses", () => { runtime, ); commandPromises.push(updatePromise); - await expectCommandCompletesWithoutSecretFlush(updatePromise); + await expectCommandWaitsForSecretFlush(updatePromise); + + expect( + findMessage(socket, "channel_account_update_response"), + ).toBeUndefined(); + expect(pendingSecretOperations).toHaveLength(1); + pendingSecretOperations.shift()?.(); + await updatePromise; expect( findMessage(socket, "channel_account_update_response"), @@ -687,7 +701,7 @@ describe("channel account list responses", () => { }, }, }); - expect(pendingSecretOperations).toHaveLength(2); + expect(pendingSecretOperations).toHaveLength(0); const deletePromise = sendChannelCommand( { @@ -700,7 +714,14 @@ describe("channel account list responses", () => { runtime, ); commandPromises.push(deletePromise); - await expectCommandCompletesWithoutSecretFlush(deletePromise); + await expectCommandWaitsForSecretFlush(deletePromise); + + expect( + findMessage(socket, "channel_account_delete_response"), + ).toBeUndefined(); + expect(pendingSecretOperations).toHaveLength(1); + pendingSecretOperations.shift()?.(); + await deletePromise; expect( findMessage(socket, "channel_account_delete_response"), @@ -711,9 +732,8 @@ describe("channel account list responses", () => { account_id: "telegram-bot", deleted: true, }); - // Delete is intentionally non-hydrating for the LCD command path, so it - // should not enqueue another keyring operation before responding. - expect(pendingSecretOperations).toHaveLength(2); + // Delete removes the keyring entry without reading it first. + expect(pendingSecretOperations).toHaveLength(0); } finally { for (const resolveSecretOperation of pendingSecretOperations.splice(0)) { resolveSecretOperation(); diff --git a/src/channels/service-account-removal.test.ts b/src/channels/service-account-removal.test.ts new file mode 100644 index 0000000000..c9bf7a0fcd --- /dev/null +++ b/src/channels/service-account-removal.test.ts @@ -0,0 +1,191 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { + __testOverrideLoadChannelAccounts, + __testOverrideSaveChannelAccounts, + clearChannelAccountStores, +} from "@/channels/accounts"; +import { + __setActiveChannelCredentialsStoreModeForTests, + __setChannelSecretStoreOverrideForTests, + buildChannelSecretName, +} from "@/channels/credential-store"; +import { + __testOverrideLoadPairingStore, + __testOverrideSavePairingStore, + clearPairingStores, + consumePairingCode, + createPairingCode, + getApprovedUsers, + getPendingPairings, +} from "@/channels/pairing"; +import { + __testOverrideLoadRoutes, + __testOverrideSaveRoutes, + addRoute, + clearAllRoutes, + getRoute, +} from "@/channels/routing"; +import { + createChannelAccountLiveWithSecrets, + getChannelAccountSnapshot, + removeChannelAccountLive, +} from "@/channels/service"; +import { + __testOverrideLoadTargetStore, + __testOverrideSaveTargetStore, + clearTargetStores, + getChannelTarget, + upsertChannelTarget, +} from "@/channels/targets"; + +describe("channel account removal", () => { + beforeEach(() => { + clearChannelAccountStores(); + clearAllRoutes(); + clearPairingStores(); + clearTargetStores(); + __testOverrideLoadChannelAccounts(() => []); + __testOverrideSaveChannelAccounts(() => {}); + __testOverrideLoadRoutes(() => null); + __testOverrideSaveRoutes(() => {}); + __testOverrideLoadPairingStore(() => null); + __testOverrideSavePairingStore(() => {}); + __testOverrideLoadTargetStore(() => {}); + __testOverrideSaveTargetStore(() => {}); + __setActiveChannelCredentialsStoreModeForTests("keyring"); + }); + + afterEach(() => { + clearChannelAccountStores(); + clearAllRoutes(); + clearPairingStores(); + clearTargetStores(); + __testOverrideLoadChannelAccounts(null); + __testOverrideSaveChannelAccounts(null); + __testOverrideLoadRoutes(null); + __testOverrideSaveRoutes(null); + __testOverrideLoadPairingStore(null); + __testOverrideSavePairingStore(null); + __testOverrideLoadTargetStore(null); + __testOverrideSaveTargetStore(null); + __setActiveChannelCredentialsStoreModeForTests(null); + __setChannelSecretStoreOverrideForTests(null); + }); + + test("removes local account state before reporting keyring cleanup failures", async () => { + const secrets = new Map(); + const deleteCalls: string[] = []; + const localStateRemovedBeforeSecretCleanup: boolean[] = []; + let secretReadCount = 0; + const accountId = "slack-app"; + const channelId = "slack"; + const chatId = "C-account"; + const targetId = "target-C-account"; + const botSecretName = buildChannelSecretName( + channelId, + accountId, + "botToken", + ); + const appSecretName = buildChannelSecretName( + channelId, + accountId, + "appToken", + ); + + __setChannelSecretStoreOverrideForTests({ + get: async (name) => { + secretReadCount++; + return secrets.get(name) ?? null; + }, + set: async (name, value) => { + secrets.set(name, value); + }, + delete: async (name) => { + deleteCalls.push(name); + localStateRemovedBeforeSecretCleanup.push( + getChannelAccountSnapshot(channelId, accountId) === null && + getRoute(channelId, chatId, accountId, null) === null && + getChannelTarget(channelId, targetId, accountId) === null && + getPendingPairings(channelId, accountId).length === 0 && + getApprovedUsers(channelId, accountId).length === 0, + ); + throw new Error("keyring delete failed"); + }, + }); + + await createChannelAccountLiveWithSecrets( + channelId, + { + enabled: false, + botToken: "xoxb-token", + appToken: "xapp-token", + dmPolicy: "pairing", + }, + { accountId }, + ); + addRoute(channelId, { + accountId, + chatId, + chatType: "channel", + threadId: null, + agentId: "agent-1", + conversationId: "conv-1", + enabled: true, + createdAt: "2026-04-11T00:00:00.000Z", + updatedAt: "2026-04-11T00:00:00.000Z", + }); + upsertChannelTarget(channelId, { + accountId, + targetId, + targetType: "channel", + chatId, + label: "#account", + discoveredAt: "2026-04-11T00:00:00.000Z", + lastSeenAt: "2026-04-11T00:00:00.000Z", + lastMessageId: "1712790000.000100", + }); + createPairingCode( + channelId, + "U-pending", + chatId, + "Pending User", + accountId, + ); + const approvedCode = createPairingCode( + channelId, + "U-approved", + chatId, + "Approved User", + accountId, + ); + expect( + consumePairingCode(channelId, approvedCode, accountId), + ).not.toBeNull(); + + expect(getChannelAccountSnapshot(channelId, accountId)).not.toBeNull(); + expect(getRoute(channelId, chatId, accountId, null)).not.toBeNull(); + expect(getChannelTarget(channelId, targetId, accountId)).not.toBeNull(); + expect(getPendingPairings(channelId, accountId)).toHaveLength(1); + expect(getApprovedUsers(channelId, accountId)).toHaveLength(1); + expect(secrets.get(botSecretName)).toBe("xoxb-token"); + expect(secrets.get(appSecretName)).toBe("xapp-token"); + secretReadCount = 0; + + await expect( + removeChannelAccountLive(channelId, accountId), + ).rejects.toThrow("keyring delete failed"); + + expect(secretReadCount).toBe(0); + expect([...deleteCalls].sort()).toEqual( + [appSecretName, botSecretName].sort(), + ); + expect(localStateRemovedBeforeSecretCleanup).toEqual([true, true]); + expect(getChannelAccountSnapshot(channelId, accountId)).toBeNull(); + expect(getRoute(channelId, chatId, accountId, null)).toBeNull(); + expect(getChannelTarget(channelId, targetId, accountId)).toBeNull(); + expect(getPendingPairings(channelId, accountId)).toEqual([]); + expect(getApprovedUsers(channelId, accountId)).toEqual([]); + expect(secrets.get(botSecretName)).toBe("xoxb-token"); + expect(secrets.get(appSecretName)).toBe("xapp-token"); + }); +}); diff --git a/src/channels/service-accounts.ts b/src/channels/service-accounts.ts index 15d879d54f..a8a5dd1dee 100644 --- a/src/channels/service-accounts.ts +++ b/src/channels/service-accounts.ts @@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto"; import { getChannelAccount, getChannelAccountWithSecrets, - removeChannelAccount, + removeChannelAccountWithSecrets, upsertChannelAccount, upsertChannelAccountWithSecrets, } from "./accounts"; @@ -63,7 +63,7 @@ export async function createChannelAccountLiveWithSecrets( ): Promise { assertSupportedChannelId(channelId); const accountId = options?.accountId?.trim() || randomUUID(); - const existing = await getChannelAccountWithSecrets(channelId, accountId); + const existing = getChannelAccount(channelId, accountId); if (existing) { throw new Error( `Channel account "${accountId}" already exists for ${channelId}.`, @@ -140,14 +140,14 @@ export async function updateChannelAccountLiveWithSecrets( patch: ChannelAccountPatch, ): Promise { assertSupportedChannelId(channelId); - const existing = await getChannelAccountWithSecrets(channelId, accountId); + let existing = getChannelAccount(channelId, accountId); if (!existing) { throw new Error( `Channel account "${accountId}" was not found for ${channelId}.`, ); } - const nextAccount = mergeAccountPatch(existing, patch); + let nextAccount = mergeAccountPatch(existing, patch); const shouldResetRoutes = (isSlackChannelAccount(existing) || isDiscordChannelAccount(existing) || @@ -158,6 +158,23 @@ export async function updateChannelAccountLiveWithSecrets( typeof nextAccount.agentId === "string" && nextAccount.agentId !== existing.agentId; + if (shouldResetRoutes) { + // Route-save rollback must restore the prior credential value if this + // update also rotates it. Ordinary account edits can preserve secret + // placeholders without reading the keyring. + const hydratedExisting = await getChannelAccountWithSecrets( + channelId, + accountId, + ); + if (!hydratedExisting) { + throw new Error( + `Channel account "${accountId}" was not found for ${channelId}.`, + ); + } + existing = hydratedExisting; + nextAccount = mergeAccountPatch(existing, patch); + } + const updated = await upsertChannelAccountWithSecrets(channelId, nextAccount); if (shouldResetRoutes) { @@ -425,7 +442,7 @@ export async function removeChannelAccountLive( removeRoutesForAccount(channelId, accountId); removeChannelTargetsForAccount(channelId, accountId); removePairingStateForAccount(channelId, accountId); - const removed = removeChannelAccount(channelId, accountId); + const removed = await removeChannelAccountWithSecrets(channelId, accountId); await refreshLoadedMessageChannelTool(); return removed; }