Skip to content

Commit 40d65ee

Browse files
authored
feat(kiloclaw): add generic patchSecrets endpoint with catalog-driven validation (#947)
<!-- PR title format: type(scope): description — e.g., feat(auth): add SSO login --> <!-- Keep the title under 72 characters, use imperative mood, no trailing period. --> ## Summary feat(kiloclaw): add generic patchSecrets endpoint with catalog-driven validation <!-- What changed and why? Keep this brief and outcome-focused. --> <!-- Include any architectural changes and enough context for a reviewer unfamiliar with this code area. --> Adds patchSecrets tRPC mutation and PATCH /api/platform/secrets worker endpoint alongside existing patchChannels. Uses secret catalog for key validation, pattern matching, maxLength enforcement, and allFieldsRequired enforcement. Makes updateChannels delegate to updateSecrets internally so both storage fields (channels + encryptedSecrets) stay in sync regardless of which endpoint is called. Eliminates interleave drift risk. Key design decisions: - Key remapping at storage boundary: encryptedSecrets stores env var names (e.g., TELEGRAM_BOT_TOKEN) so the existing buildEnvVars/mergeEnvVarsWithSecrets pipeline works unchanged. A reverse map (ENV_VAR_TO_FIELD_KEY) converts back to field keys when reading into the working set. - allFieldsRequired enforced at DO level: The DO validates post-merge state (existing secrets + patch), not just the incoming patch. This allows single-field rotations (e.g., rotating slackBotToken when slackAppToken is already stored) while still rejecting invalid partial clears. - secretCount excludes channel env vars: Channel tokens are dual-written into encryptedSecrets but excluded from secretCount (via CHANNEL_ENV_VARS filter) since they're already counted by channelCount. Applied in getStatus(), getDebugState(), and /api/kiloclaw/config. - Validation errors return 400: DO validation errors carry status: 400 and an Invalid secret patch: prefix so the route handler returns the actual message instead of a generic 500. ## Verification - Catalog package tests pass (37 tests) — includes new allFieldsRequired contract, maxLength contract, and unknown key rejection tests - DO updateSecrets tests pass (8 tests) — set, remove, merge, legacy migration, Slack dual-token set/clear, channel-only filtering - DO updateChannels tests pass (8 tests) — all 6 existing tests pass through delegation path, plus 2 new interleave consistency tests - updateChannels delegation produces identical boolean return shape as original implementation - Worker builds without errors (wrangler types + wrangler deploy --dry-run) - Integration: provision instance via patchChannels, update via patchSecrets, verify both endpoints see consistent state ## Visual Changes N/A ## Reviewer Notes - Dual-write strategy: updateSecrets writes to both channels (legacy, field-keyed) and encryptedSecrets (new, env-var-keyed). Channel-category keys go to both; future non-channel secrets (tools, providers) only go to encryptedSecrets. - updateChannels delegation: The old method now delegates to updateSecrets. This is the fix for the interleave drift edge case; there's now a single write path. Existing updateChannels callers see no behavior change. - tRPC defers allFieldsRequired to DO: The tRPC layer validates key membership, patterns, and maxLength, but does not check allFieldsRequired — only the DO can see existing state to validate post-merge correctness. - configured response filtered: The configured array returned by updateSecrets is filtered to ALL_SECRET_FIELD_KEYS only, preventing non-catalog env var names from leaking into the response. - Rate limiting: Not yet applied to patchSecrets. Should match whatever strategy patchChannels uses. Can be added as a follow-up. - tRPC-level integration tests: Validation logic is tested via catalog contract tests and DO-level tests rather than full tRPC caller tests, since the router test infrastructure requires real DB + user setup + worker client mocking. - Depends on: PR #857 (@kilocode/kiloclaw-secret-catalog package)
2 parents f1a4612 + 044eeb8 commit 40d65ee

14 files changed

Lines changed: 595 additions & 43 deletions

File tree

kiloclaw/packages/secret-catalog/src/__tests__/catalog.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
SECRET_CATALOG_MAP,
55
ALL_SECRET_FIELD_KEYS,
66
FIELD_KEY_TO_ENV_VAR,
7+
ENV_VAR_TO_FIELD_KEY,
78
FIELD_KEY_TO_ENTRY,
89
getEntriesByCategory,
910
} from '../catalog.js';
@@ -119,6 +120,20 @@ describe('Secret Catalog', () => {
119120
expect(FIELD_KEY_TO_ENV_VAR.get('slackBotToken')).toBe('SLACK_BOT_TOKEN');
120121
expect(FIELD_KEY_TO_ENV_VAR.get('slackAppToken')).toBe('SLACK_APP_TOKEN');
121122
});
123+
124+
it('ENV_VAR_TO_FIELD_KEY is the exact reverse of FIELD_KEY_TO_ENV_VAR', () => {
125+
expect(ENV_VAR_TO_FIELD_KEY.size).toBe(FIELD_KEY_TO_ENV_VAR.size);
126+
for (const [fieldKey, envVar] of FIELD_KEY_TO_ENV_VAR) {
127+
expect(ENV_VAR_TO_FIELD_KEY.get(envVar)).toBe(fieldKey);
128+
}
129+
});
130+
131+
it('ENV_VAR_TO_FIELD_KEY has correct reverse mappings', () => {
132+
expect(ENV_VAR_TO_FIELD_KEY.get('TELEGRAM_BOT_TOKEN')).toBe('telegramBotToken');
133+
expect(ENV_VAR_TO_FIELD_KEY.get('DISCORD_BOT_TOKEN')).toBe('discordBotToken');
134+
expect(ENV_VAR_TO_FIELD_KEY.get('SLACK_BOT_TOKEN')).toBe('slackBotToken');
135+
expect(ENV_VAR_TO_FIELD_KEY.get('SLACK_APP_TOKEN')).toBe('slackAppToken');
136+
});
122137
});
123138

124139
describe('Lookup helpers', () => {
@@ -264,4 +279,57 @@ describe('Secret Catalog', () => {
264279
);
265280
});
266281
});
282+
283+
describe('allFieldsRequired contract', () => {
284+
it('slack entry has allFieldsRequired set', () => {
285+
const slack = SECRET_CATALOG_MAP.get('slack');
286+
expect(slack?.allFieldsRequired).toBe(true);
287+
});
288+
289+
it('slack entry has exactly 2 fields', () => {
290+
const slack = SECRET_CATALOG_MAP.get('slack');
291+
expect(slack?.fields.length).toBe(2);
292+
expect(slack?.fields.map(f => f.key)).toEqual(['slackBotToken', 'slackAppToken']);
293+
});
294+
295+
it('telegram and discord do not have allFieldsRequired', () => {
296+
expect(SECRET_CATALOG_MAP.get('telegram')?.allFieldsRequired).toBeFalsy();
297+
expect(SECRET_CATALOG_MAP.get('discord')?.allFieldsRequired).toBeFalsy();
298+
});
299+
300+
it('ALL_SECRET_FIELD_KEYS rejects unknown keys', () => {
301+
expect(ALL_SECRET_FIELD_KEYS.has('telegramBotToken')).toBe(true);
302+
expect(ALL_SECRET_FIELD_KEYS.has('unknownKey')).toBe(false);
303+
expect(ALL_SECRET_FIELD_KEYS.has('')).toBe(false);
304+
});
305+
306+
it('FIELD_KEY_TO_ENTRY maps both slack fields to the same entry', () => {
307+
const botEntry = FIELD_KEY_TO_ENTRY.get('slackBotToken');
308+
const appEntry = FIELD_KEY_TO_ENTRY.get('slackAppToken');
309+
expect(botEntry).toBeDefined();
310+
expect(botEntry).toBe(appEntry);
311+
expect(botEntry?.allFieldsRequired).toBe(true);
312+
});
313+
});
314+
315+
describe('maxLength contract', () => {
316+
it('all maxLength values are within the global 500 ceiling', () => {
317+
for (const entry of SECRET_CATALOG) {
318+
for (const field of entry.fields) {
319+
expect(field.maxLength).toBeLessThanOrEqual(500);
320+
}
321+
}
322+
});
323+
324+
it('field-specific maxLength values are set correctly', () => {
325+
const telegram = FIELD_KEY_TO_ENTRY.get('telegramBotToken');
326+
const discord = FIELD_KEY_TO_ENTRY.get('discordBotToken');
327+
const slackBot = FIELD_KEY_TO_ENTRY.get('slackBotToken');
328+
329+
expect(telegram?.fields[0].maxLength).toBe(100);
330+
expect(discord?.fields[0].maxLength).toBe(200);
331+
expect(slackBot?.fields.find(f => f.key === 'slackBotToken')?.maxLength).toBe(300);
332+
expect(slackBot?.fields.find(f => f.key === 'slackAppToken')?.maxLength).toBe(300);
333+
});
334+
});
267335
});

kiloclaw/packages/secret-catalog/src/catalog.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from 'zod';
2-
import type { SecretCatalogEntry, SecretCategory } from './types.js';
3-
import { SecretCatalogEntrySchema } from './types.js';
2+
import type { SecretCatalogEntry, SecretCategory } from './types';
3+
import { SecretCatalogEntrySchema } from './types';
44

55
/**
66
* Secret Catalog — declarative registry of all secret types.
@@ -104,6 +104,9 @@ export const SECRET_CATALOG_MAP: ReadonlyMap<string, SecretCatalogEntry> = new M
104104
SECRET_CATALOG.map(entry => [entry.id, entry])
105105
);
106106

107+
/** Union type of all secret field keys in the catalog */
108+
export type SecretFieldKey = (typeof SECRET_CATALOG_RAW)[number]['fields'][number]['key'];
109+
107110
/** Set of all field keys across all entries */
108111
export const ALL_SECRET_FIELD_KEYS: ReadonlySet<string> = new Set(
109112
SECRET_CATALOG.flatMap(entry => entry.fields.map(field => field.key))
@@ -114,6 +117,11 @@ export const FIELD_KEY_TO_ENV_VAR: ReadonlyMap<string, string> = new Map(
114117
SECRET_CATALOG.flatMap(entry => entry.fields.map(field => [field.key, field.envVar]))
115118
);
116119

120+
/** Reverse map: env var name → field key (for reading encryptedSecrets back to working set) */
121+
export const ENV_VAR_TO_FIELD_KEY: ReadonlyMap<string, string> = new Map(
122+
SECRET_CATALOG.flatMap(entry => entry.fields.map(field => [field.envVar, field.key]))
123+
);
124+
117125
/** Map of field key → owning entry (used for allFieldsRequired checks) */
118126
export const FIELD_KEY_TO_ENTRY: ReadonlyMap<string, SecretCatalogEntry> = new Map(
119127
SECRET_CATALOG.flatMap(entry => entry.fields.map(field => [field.key, entry]))

kiloclaw/packages/secret-catalog/src/index.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ export {
55
InjectionMethodSchema,
66
SecretFieldDefinitionSchema,
77
SecretCatalogEntrySchema,
8-
} from './types.js';
8+
} from './types';
99

1010
// Types
1111
export type {
@@ -14,20 +14,23 @@ export type {
1414
InjectionMethod,
1515
SecretFieldDefinition,
1616
SecretCatalogEntry,
17-
} from './types.js';
17+
} from './types';
1818

19-
export { DEFAULT_INJECTION_METHOD, getInjectionMethod } from './types.js';
19+
export { DEFAULT_INJECTION_METHOD, getInjectionMethod } from './types';
2020

2121
// Catalog and lookup helpers
2222
export {
2323
SECRET_CATALOG,
2424
SECRET_CATALOG_MAP,
2525
ALL_SECRET_FIELD_KEYS,
2626
FIELD_KEY_TO_ENV_VAR,
27+
ENV_VAR_TO_FIELD_KEY,
2728
FIELD_KEY_TO_ENTRY,
2829
ALL_SECRET_ENV_VARS,
2930
getEntriesByCategory,
30-
} from './catalog.js';
31+
} from './catalog';
32+
33+
export type { SecretFieldKey } from './catalog';
3134

3235
// Validation
33-
export { validateFieldValue } from './validation.js';
36+
export { validateFieldValue } from './validation';

kiloclaw/src/durable-objects/kiloclaw-instance.test.ts

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1412,6 +1412,230 @@ describe('updateChannels', () => {
14121412
expect(result.telegram).toBe(true);
14131413
expect(result.discord).toBe(true);
14141414
});
1415+
1416+
it('updateChannels dual-writes to encryptedSecrets (no interleave drift)', async () => {
1417+
const { instance, storage } = createInstance();
1418+
await seedProvisioned(storage);
1419+
1420+
// Write via legacy path
1421+
await instance.updateChannels({ telegramBotToken: fakeEnvelope });
1422+
1423+
// channels uses field keys, encryptedSecrets uses env var names
1424+
const channels = storage._store.get('channels') as Record<string, unknown>;
1425+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1426+
expect(channels.telegramBotToken).toEqual(fakeEnvelope);
1427+
expect(secrets.TELEGRAM_BOT_TOKEN).toEqual(fakeEnvelope);
1428+
});
1429+
1430+
it('interleaving updateChannels and updateSecrets keeps storage in sync', async () => {
1431+
const discordEnvelope = { ...fakeEnvelope, encryptedData: 'discord-data' };
1432+
const { instance, storage } = createInstance();
1433+
await seedProvisioned(storage);
1434+
1435+
// Step 1: set telegram via updateSecrets (new path)
1436+
await instance.updateSecrets({ telegramBotToken: fakeEnvelope });
1437+
1438+
// Step 2: set discord via updateChannels (legacy path, delegates to updateSecrets)
1439+
const result = await instance.updateChannels({ discordBotToken: discordEnvelope });
1440+
1441+
// Both should be present via legacy response
1442+
expect(result.telegram).toBe(true);
1443+
expect(result.discord).toBe(true);
1444+
1445+
// channels uses field keys, encryptedSecrets uses env var names
1446+
const channels = storage._store.get('channels') as Record<string, unknown>;
1447+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1448+
expect(channels.telegramBotToken).toEqual(fakeEnvelope);
1449+
expect(channels.discordBotToken).toEqual(discordEnvelope);
1450+
expect(secrets.TELEGRAM_BOT_TOKEN).toEqual(fakeEnvelope);
1451+
expect(secrets.DISCORD_BOT_TOKEN).toEqual(discordEnvelope);
1452+
});
1453+
});
1454+
1455+
// ============================================================================
1456+
// updateSecrets
1457+
// ============================================================================
1458+
1459+
describe('updateSecrets', () => {
1460+
const fakeEnvelope = {
1461+
encryptedData: 'data',
1462+
encryptedDEK: 'dek',
1463+
algorithm: 'rsa-aes-256-gcm' as const,
1464+
version: 1 as const,
1465+
};
1466+
1467+
it('stores env var names in encryptedSecrets but field keys in channels', async () => {
1468+
const { instance, storage } = createInstance();
1469+
await seedProvisioned(storage);
1470+
1471+
const result = await instance.updateSecrets({ telegramBotToken: fakeEnvelope });
1472+
1473+
expect(result.configured).toContain('telegramBotToken');
1474+
// channels uses field keys (for decryptChannelTokens backward compat)
1475+
const channels = storage._store.get('channels') as Record<string, unknown>;
1476+
expect(channels.telegramBotToken).toEqual(fakeEnvelope);
1477+
// encryptedSecrets uses env var names (for buildEnvVars/mergeEnvVarsWithSecrets)
1478+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1479+
expect(secrets.TELEGRAM_BOT_TOKEN).toEqual(fakeEnvelope);
1480+
expect(secrets.telegramBotToken).toBeUndefined();
1481+
});
1482+
1483+
it('removes a secret when null is passed', async () => {
1484+
const { instance, storage } = createInstance();
1485+
await seedProvisioned(storage, {
1486+
channels: { telegramBotToken: fakeEnvelope },
1487+
encryptedSecrets: { TELEGRAM_BOT_TOKEN: fakeEnvelope },
1488+
});
1489+
1490+
const result = await instance.updateSecrets({ telegramBotToken: null });
1491+
1492+
expect(result.configured).not.toContain('telegramBotToken');
1493+
expect(storage._store.get('channels')).toBeNull();
1494+
expect(storage._store.get('encryptedSecrets')).toBeNull();
1495+
});
1496+
1497+
it('merges with existing secrets — setting telegram preserves discord', async () => {
1498+
const discordEnvelope = { ...fakeEnvelope, encryptedData: 'discord-data' };
1499+
const { instance, storage } = createInstance();
1500+
await seedProvisioned(storage, {
1501+
channels: { discordBotToken: discordEnvelope },
1502+
encryptedSecrets: { DISCORD_BOT_TOKEN: discordEnvelope },
1503+
});
1504+
1505+
const result = await instance.updateSecrets({ telegramBotToken: fakeEnvelope });
1506+
1507+
expect(result.configured).toContain('telegramBotToken');
1508+
expect(result.configured).toContain('discordBotToken');
1509+
const channels = storage._store.get('channels') as Record<string, unknown>;
1510+
expect(channels.telegramBotToken).toEqual(fakeEnvelope);
1511+
expect(channels.discordBotToken).toEqual(discordEnvelope);
1512+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1513+
expect(secrets.TELEGRAM_BOT_TOKEN).toEqual(fakeEnvelope);
1514+
expect(secrets.DISCORD_BOT_TOKEN).toEqual(discordEnvelope);
1515+
});
1516+
1517+
it('reads from legacy channels field when encryptedSecrets is empty', async () => {
1518+
const { instance, storage } = createInstance();
1519+
// Simulate legacy state: only channels field, no encryptedSecrets
1520+
await seedProvisioned(storage, {
1521+
channels: { telegramBotToken: fakeEnvelope },
1522+
});
1523+
1524+
const discordEnvelope = { ...fakeEnvelope, encryptedData: 'discord-data' };
1525+
const result = await instance.updateSecrets({ discordBotToken: discordEnvelope });
1526+
1527+
// Should see both: legacy telegram + new discord
1528+
expect(result.configured).toContain('telegramBotToken');
1529+
expect(result.configured).toContain('discordBotToken');
1530+
});
1531+
1532+
it('removing all secrets sets both storage fields to null', async () => {
1533+
const { instance, storage } = createInstance();
1534+
await seedProvisioned(storage, {
1535+
channels: { telegramBotToken: fakeEnvelope },
1536+
encryptedSecrets: { TELEGRAM_BOT_TOKEN: fakeEnvelope },
1537+
});
1538+
1539+
const result = await instance.updateSecrets({ telegramBotToken: null });
1540+
1541+
expect(result.configured).toEqual([]);
1542+
expect(storage._store.get('channels')).toBeNull();
1543+
expect(storage._store.get('encryptedSecrets')).toBeNull();
1544+
});
1545+
1546+
it('sets both slack tokens and dual-writes to channels', async () => {
1547+
const slackBotEnvelope = { ...fakeEnvelope, encryptedData: 'slack-bot' };
1548+
const slackAppEnvelope = { ...fakeEnvelope, encryptedData: 'slack-app' };
1549+
const { instance, storage } = createInstance();
1550+
await seedProvisioned(storage);
1551+
1552+
const result = await instance.updateSecrets({
1553+
slackBotToken: slackBotEnvelope,
1554+
slackAppToken: slackAppEnvelope,
1555+
});
1556+
1557+
expect(result.configured).toContain('slackBotToken');
1558+
expect(result.configured).toContain('slackAppToken');
1559+
const channels = storage._store.get('channels') as Record<string, unknown>;
1560+
expect(channels.slackBotToken).toEqual(slackBotEnvelope);
1561+
expect(channels.slackAppToken).toEqual(slackAppEnvelope);
1562+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1563+
expect(secrets.SLACK_BOT_TOKEN).toEqual(slackBotEnvelope);
1564+
expect(secrets.SLACK_APP_TOKEN).toEqual(slackAppEnvelope);
1565+
});
1566+
1567+
it('clears both slack tokens simultaneously', async () => {
1568+
const slackBotEnvelope = { ...fakeEnvelope, encryptedData: 'slack-bot' };
1569+
const slackAppEnvelope = { ...fakeEnvelope, encryptedData: 'slack-app' };
1570+
const { instance, storage } = createInstance();
1571+
await seedProvisioned(storage, {
1572+
channels: { slackBotToken: slackBotEnvelope, slackAppToken: slackAppEnvelope },
1573+
encryptedSecrets: { SLACK_BOT_TOKEN: slackBotEnvelope, SLACK_APP_TOKEN: slackAppEnvelope },
1574+
});
1575+
1576+
const result = await instance.updateSecrets({
1577+
slackBotToken: null,
1578+
slackAppToken: null,
1579+
});
1580+
1581+
expect(result.configured).not.toContain('slackBotToken');
1582+
expect(result.configured).not.toContain('slackAppToken');
1583+
expect(storage._store.get('channels')).toBeNull();
1584+
expect(storage._store.get('encryptedSecrets')).toBeNull();
1585+
});
1586+
1587+
it('second updateSecrets call does not accumulate phantom entries', async () => {
1588+
const { instance, storage } = createInstance();
1589+
await seedProvisioned(storage);
1590+
1591+
// First call: set telegram
1592+
await instance.updateSecrets({ telegramBotToken: fakeEnvelope });
1593+
1594+
// Second call: set discord — telegram should persist, no phantom keys
1595+
const discordEnvelope = { ...fakeEnvelope, encryptedData: 'discord-data' };
1596+
const result = await instance.updateSecrets({ discordBotToken: discordEnvelope });
1597+
1598+
expect(result.configured).toEqual(
1599+
expect.arrayContaining(['telegramBotToken', 'discordBotToken'])
1600+
);
1601+
expect(result.configured).toHaveLength(2);
1602+
1603+
// encryptedSecrets should have exactly 2 env var keys, no field key duplicates
1604+
const secrets = storage._store.get('encryptedSecrets') as Record<string, unknown>;
1605+
const secretKeys = Object.keys(secrets).sort();
1606+
expect(secretKeys).toEqual(['DISCORD_BOT_TOKEN', 'TELEGRAM_BOT_TOKEN']);
1607+
1608+
// channels should have exactly 2 field keys
1609+
const channels = storage._store.get('channels') as Record<string, unknown>;
1610+
const channelKeys = Object.keys(channels).sort();
1611+
expect(channelKeys).toEqual(['discordBotToken', 'telegramBotToken']);
1612+
});
1613+
1614+
it('configured return uses field keys not env var names', async () => {
1615+
const { instance, storage } = createInstance();
1616+
await seedProvisioned(storage);
1617+
1618+
const result = await instance.updateSecrets({ telegramBotToken: fakeEnvelope });
1619+
1620+
// Should return field keys, not env var names
1621+
expect(result.configured).toContain('telegramBotToken');
1622+
expect(result.configured).not.toContain('TELEGRAM_BOT_TOKEN');
1623+
});
1624+
1625+
it('rejects partial clear of allFieldsRequired entry', async () => {
1626+
const slackBotEnvelope = { ...fakeEnvelope, encryptedData: 'slack-bot' };
1627+
const slackAppEnvelope = { ...fakeEnvelope, encryptedData: 'slack-app' };
1628+
const { instance, storage } = createInstance();
1629+
await seedProvisioned(storage, {
1630+
channels: { slackBotToken: slackBotEnvelope, slackAppToken: slackAppEnvelope },
1631+
encryptedSecrets: { SLACK_BOT_TOKEN: slackBotEnvelope, SLACK_APP_TOKEN: slackAppEnvelope },
1632+
});
1633+
1634+
// Removing only one Slack token should fail — allFieldsRequired
1635+
await expect(instance.updateSecrets({ slackBotToken: null })).rejects.toThrow(
1636+
'Invalid secret patch: Slack requires all fields to be set together'
1637+
);
1638+
});
14151639
});
14161640

14171641
// ============================================================================

0 commit comments

Comments
 (0)