From 74c1736372bcbdc73d228b69022ce72cc59b5025 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Thu, 26 Mar 2026 17:16:42 -0500 Subject: [PATCH] fix(remote-questions): empty-key entry in auth.json shadows valid Discord bot token (#2737) * fix(remote-questions): empty-key entry in auth.json shadows valid Discord bot token removeProviderToken() called auth.set(provider, { key: '' }) instead of auth.remove(provider). Since AuthStorage.set() appends for api_key type (deduplicating by exact key match), this inserted an empty-key entry at index 0. Every credential lookup (.get(), .find()) matched the empty entry first, shadowing valid tokens at later indices. Fixes: - remote-command.ts: use auth.remove() instead of auth.set() with empty key - config.ts: hydrateRemoteTokensFromAuth .find() now requires non-empty key - wizard.ts: loadStoredEnvKeys uses getCredentialsForProvider + .find() instead of .get() which returns creds[0] - onboarding.ts: check existing tokens via .some() over full credentials array instead of .get() which only returns first entry - key-manager.ts: filter empty-key entries in getAllKeyStatuses, add/remove/ rotate provider pickers, and doctor env-conflict check Tests: 3186 pass, 0 fail across full GSD test suite * fix(config): ignore empty shadowing tool keys --- src/onboarding.ts | 10 ++++--- .../extensions/gsd/commands-config.ts | 16 ++++++---- src/resources/extensions/gsd/key-manager.ts | 23 +++++---------- .../gsd/tests/commands-config.test.ts | 24 +++++++++++++++ .../extensions/gsd/tests/key-manager.test.ts | 17 ++++++++++- .../gsd/tests/remote-questions.test.ts | 29 +++++++++++++++++++ .../extensions/remote-questions/config.ts | 2 +- .../remote-questions/remote-command.ts | 2 +- src/wizard.ts | 9 ++++-- 9 files changed, 101 insertions(+), 31 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/commands-config.test.ts diff --git a/src/onboarding.ts b/src/onboarding.ts index eafe1d443..93e39d0f5 100644 --- a/src/onboarding.ts +++ b/src/onboarding.ts @@ -669,10 +669,12 @@ async function runRemoteQuestionsStep( pc: PicoModule, authStorage: AuthStorage, ): Promise { - // Check existing config - const hasDiscord = authStorage.has('discord_bot') && !!(authStorage.get('discord_bot') as any)?.key - const hasSlack = authStorage.has('slack_bot') && !!(authStorage.get('slack_bot') as any)?.key - const hasTelegram = authStorage.has('telegram_bot') && !!(authStorage.get('telegram_bot') as any)?.key + // Check existing config — use getCredentialsForProvider to skip empty-key entries + const hasValidKey = (provider: string) => + authStorage.getCredentialsForProvider(provider).some((c: any) => c.type === 'api_key' && c.key) + const hasDiscord = hasValidKey('discord_bot') + const hasSlack = hasValidKey('slack_bot') + const hasTelegram = hasValidKey('telegram_bot') const existingChannel = hasDiscord ? 'Discord' : hasSlack ? 'Slack' : hasTelegram ? 'Telegram' : null type RemoteOption = { value: string; label: string; hint?: string } diff --git a/src/resources/extensions/gsd/commands-config.ts b/src/resources/extensions/gsd/commands-config.ts index ec5a8b596..01cf58c14 100644 --- a/src/resources/extensions/gsd/commands-config.ts +++ b/src/resources/extensions/gsd/commands-config.ts @@ -22,6 +22,12 @@ export const TOOL_KEYS = [ { id: "groq", env: "GROQ_API_KEY", label: "Groq Voice", hint: "console.groq.com" }, ] as const; +function getStoredToolKey(auth: AuthStorage, providerId: string): string | undefined { + const creds = auth.getCredentialsForProvider(providerId); + const cred = creds.find((c) => c.type === "api_key" && c.key); + return cred?.type === "api_key" ? cred.key : undefined; +} + /** * Load tool API keys from auth.json into environment variables. * Called at session startup to ensure tools have access to their credentials. @@ -33,9 +39,9 @@ export function loadToolApiKeys(): void { const auth = AuthStorage.create(authPath); for (const tool of TOOL_KEYS) { - const cred = auth.get(tool.id); - if (cred && cred.type === "api_key" && cred.key && !process.env[tool.env]) { - process.env[tool.env] = cred.key; + const key = getStoredToolKey(auth, tool.id); + if (key && !process.env[tool.env]) { + process.env[tool.env] = key; } } } catch { @@ -55,14 +61,14 @@ export async function handleConfig(ctx: ExtensionCommandContext): Promise // Show current status const statusLines = ["GSD Tool Configuration\n"]; for (const tool of TOOL_KEYS) { - const hasKey = !!process.env[tool.env] || !!(auth.get(tool.id) as { key?: string })?.key; + const hasKey = !!process.env[tool.env] || !!getStoredToolKey(auth, tool.id); statusLines.push(` ${hasKey ? "\u2713" : "\u2717"} ${tool.label}${hasKey ? "" : ` \u2014 get key at ${tool.hint}`}`); } ctx.ui.notify(statusLines.join("\n"), "info"); // Ask which tools to configure const options = TOOL_KEYS.map(t => { - const hasKey = !!process.env[t.env] || !!(auth.get(t.id) as { key?: string })?.key; + const hasKey = !!process.env[t.env] || !!getStoredToolKey(auth, t.id); return `${t.label} ${hasKey ? "(configured \u2713)" : "(not set)"}`; }); options.push("(done)"); diff --git a/src/resources/extensions/gsd/key-manager.ts b/src/resources/extensions/gsd/key-manager.ts index db67fd81b..17bd3cb31 100644 --- a/src/resources/extensions/gsd/key-manager.ts +++ b/src/resources/extensions/gsd/key-manager.ts @@ -150,22 +150,13 @@ export interface KeyStatus { */ export function getAllKeyStatuses(auth: AuthStorage): KeyStatus[] { return PROVIDER_REGISTRY.map((provider) => { - const creds = auth.getCredentialsForProvider(provider.id); + const rawCreds = auth.getCredentialsForProvider(provider.id); + // Filter out empty-key entries (left by legacy removeProviderToken or skipped onboarding) + const creds = rawCreds.filter((c) => !(c.type === "api_key" && !(c as ApiKeyCredential).key)); const envKey = provider.envVar ? process.env[provider.envVar] : undefined; if (creds.length > 0) { const firstCred = creds[0]; - // Skip empty keys (from skipped onboarding) - if (firstCred.type === "api_key" && !(firstCred as ApiKeyCredential).key) { - return { - provider, - configured: false, - source: "none" as const, - credentialCount: 0, - description: "empty key (skipped setup)", - backedOff: false, - }; - } const desc = creds.length > 1 ? `${creds.length} keys (round-robin)` @@ -275,7 +266,7 @@ export async function handleAddKey( } else { // Interactive provider picker const options = PROVIDER_REGISTRY.map((p) => { - const creds = auth.getCredentialsForProvider(p.id); + const creds = auth.getCredentialsForProvider(p.id).filter((c) => !(c.type === "api_key" && !(c as ApiKeyCredential).key)); const existing = creds.length > 0 ? " (configured)" : ""; return `[${p.category}] ${p.label}${existing}`; }); @@ -360,7 +351,7 @@ export async function handleRemoveKey( } else { // Show only configured providers const configured = PROVIDER_REGISTRY.filter((p) => { - const creds = auth.getCredentialsForProvider(p.id); + const creds = auth.getCredentialsForProvider(p.id).filter((c) => !(c.type === "api_key" && !(c as ApiKeyCredential).key)); return creds.length > 0; }); @@ -619,7 +610,7 @@ export async function handleRotateKey( // Show only configured API key providers const configured = PROVIDER_REGISTRY.filter((p) => { const creds = auth.getCredentialsForProvider(p.id); - return creds.some((c) => c.type === "api_key"); + return creds.some((c) => c.type === "api_key" && (c as ApiKeyCredential).key); }); if (configured.length === 0) { @@ -788,7 +779,7 @@ export function runKeyDoctor(auth: AuthStorage): DoctorFinding[] { if (!envValue) continue; const creds = auth.getCredentialsForProvider(provider.id); - const apiKey = creds.find((c) => c.type === "api_key") as ApiKeyCredential | undefined; + const apiKey = creds.find((c) => c.type === "api_key" && (c as ApiKeyCredential).key) as ApiKeyCredential | undefined; if (apiKey?.key && apiKey.key !== envValue) { findings.push({ severity: "warning", diff --git a/src/resources/extensions/gsd/tests/commands-config.test.ts b/src/resources/extensions/gsd/tests/commands-config.test.ts new file mode 100644 index 000000000..4a0756e32 --- /dev/null +++ b/src/resources/extensions/gsd/tests/commands-config.test.ts @@ -0,0 +1,24 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +test("commands-config source-level: tool key lookup skips empty api_key entries", () => { + const source = readFileSync(join(__dirname, "..", "commands-config.ts"), "utf-8"); + assert.ok( + source.includes('getCredentialsForProvider(providerId)'), + "commands-config should read the full credential list", + ); + assert.ok( + source.includes('c.type === "api_key" && c.key'), + "commands-config should require a non-empty api_key when resolving stored tool keys", + ); + assert.ok( + !source.includes("auth.get(tool.id)"), + "commands-config should not rely on auth.get(tool.id), which can return an empty shadowing entry", + ); +}); diff --git a/src/resources/extensions/gsd/tests/key-manager.test.ts b/src/resources/extensions/gsd/tests/key-manager.test.ts index 54d66ae19..785c34945 100644 --- a/src/resources/extensions/gsd/tests/key-manager.test.ts +++ b/src/resources/extensions/gsd/tests/key-manager.test.ts @@ -189,7 +189,22 @@ test("getAllKeyStatuses detects empty keys as not configured", () => { const statuses = getAllKeyStatuses(auth); const groq = statuses.find((s) => s.provider.id === "groq"); assert.equal(groq?.configured, false); - assert.ok(groq?.description.includes("empty")); + // Empty-key entries are filtered out, so provider appears unconfigured + assert.equal(groq?.source, "none"); +}); + +test("getAllKeyStatuses finds valid keys even when empty-key entry exists at index 0", () => { + const auth = makeAuth({ + groq: [ + { type: "api_key", key: "" }, + { type: "api_key", key: "gsk-real-key" }, + ], + }); + const statuses = getAllKeyStatuses(auth); + const groq = statuses.find((s) => s.provider.id === "groq"); + assert.equal(groq?.configured, true); + assert.equal(groq?.source, "auth.json"); + assert.equal(groq?.credentialCount, 1); // only the valid key counts }); test("getAllKeyStatuses detects env var keys", () => { diff --git a/src/resources/extensions/gsd/tests/remote-questions.test.ts b/src/resources/extensions/gsd/tests/remote-questions.test.ts index 6d0550a32..23432a2c0 100644 --- a/src/resources/extensions/gsd/tests/remote-questions.test.ts +++ b/src/resources/extensions/gsd/tests/remote-questions.test.ts @@ -724,3 +724,32 @@ test("resolveRemoteConfig returns null when preferences are absent (no env side- if (savedTelegram !== undefined) process.env.TELEGRAM_BOT_TOKEN = savedTelegram; } }); + +test("config source-level: hydration skips api_key entries with empty keys", () => { + const configSrc = readFileSync( + join(__dirname, "..", "..", "remote-questions", "config.ts"), + "utf-8", + ); + // The find() call in hydrateRemoteTokensFromAuth must filter for non-empty keys, + // not just match on type === "api_key". This prevents stale empty-key entries + // (left by removeProviderToken) from shadowing valid tokens. + assert.ok( + configSrc.includes('c.type === "api_key" && !!c.key'), + "hydrateRemoteTokensFromAuth find() should require a non-empty key", + ); +}); + +test("config source-level: removeProviderToken uses auth.remove not auth.set with empty key", () => { + const commandSrc = readFileSync( + join(__dirname, "..", "..", "remote-questions", "remote-command.ts"), + "utf-8", + ); + // removeProviderToken should call auth.remove(provider), not auth.set(provider, { key: "" }). + // Setting an empty key pollutes the credentials array and shadows valid tokens. + const fnStart = commandSrc.indexOf("function removeProviderToken"); + assert.ok(fnStart !== -1, "removeProviderToken should exist"); + const fnEnd = commandSrc.indexOf("\n}", fnStart); + const fnBody = commandSrc.slice(fnStart, fnEnd); + assert.ok(fnBody.includes("auth.remove("), "removeProviderToken should call auth.remove()"); + assert.ok(!fnBody.includes('key: ""'), "removeProviderToken should not set an empty key"); +}); diff --git a/src/resources/extensions/remote-questions/config.ts b/src/resources/extensions/remote-questions/config.ts index b0f4e3138..e34249601 100644 --- a/src/resources/extensions/remote-questions/config.ts +++ b/src/resources/extensions/remote-questions/config.ts @@ -59,7 +59,7 @@ function hydrateRemoteTokensFromAuth(): void { for (const [providerId, envVar] of needed) { try { const creds = auth.getCredentialsForProvider(providerId); - const apiKeyCred = creds.find((c: { type: string }) => c.type === "api_key") as + const apiKeyCred = creds.find((c: { type: string; key?: string }) => c.type === "api_key" && !!c.key) as | { type: "api_key"; key: string } | undefined; if (apiKeyCred?.key) { diff --git a/src/resources/extensions/remote-questions/remote-command.ts b/src/resources/extensions/remote-questions/remote-command.ts index 6934d534a..ea5278904 100644 --- a/src/resources/extensions/remote-questions/remote-command.ts +++ b/src/resources/extensions/remote-questions/remote-command.ts @@ -312,7 +312,7 @@ function saveProviderToken(provider: string, token: string): void { function removeProviderToken(provider: string): void { const auth = getAuthStorage(); - auth.set(provider, { type: "api_key", key: "" }); + auth.remove(provider); } export function saveRemoteQuestionsConfig(channel: "slack" | "discord" | "telegram", channelId: string): void { diff --git a/src/wizard.ts b/src/wizard.ts index 1b11e1e8d..f156161ff 100644 --- a/src/wizard.ts +++ b/src/wizard.ts @@ -23,9 +23,12 @@ export function loadStoredEnvKeys(authStorage: AuthStorage): void { ] for (const [provider, envVar] of providers) { if (!process.env[envVar]) { - const cred = authStorage.get(provider) - if (cred?.type === 'api_key' && cred.key) { - process.env[envVar] = cred.key as string + // Use getCredentialsForProvider to skip empty-key entries at index 0 + // (left by legacy removeProviderToken which used set() with empty key) + const creds = authStorage.getCredentialsForProvider(provider) + const cred = creds.find((c: any) => c.type === 'api_key' && c.key) + if (cred?.type === 'api_key' && (cred as any).key) { + process.env[envVar] = (cred as any).key as string } } }