diff --git a/.changeset/merry-foxes-climb.md b/.changeset/merry-foxes-climb.md new file mode 100644 index 000000000..e21ee9950 --- /dev/null +++ b/.changeset/merry-foxes-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2997 +--- +SDK config-set/config-get and init responses no longer echo plaintext API keys. New sdk/src/query/secrets.ts ports SECRET_CONFIG_KEYS masking from CJS; init bundles only mask string values to preserve the boolean availability-flag contract. See #2997. diff --git a/.changeset/plucky-moles-roam.md b/.changeset/plucky-moles-roam.md new file mode 100644 index 000000000..e21ee9950 --- /dev/null +++ b/.changeset/plucky-moles-roam.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2997 +--- +SDK config-set/config-get and init responses no longer echo plaintext API keys. New sdk/src/query/secrets.ts ports SECRET_CONFIG_KEYS masking from CJS; init bundles only mask string values to preserve the boolean availability-flag contract. See #2997. diff --git a/get-shit-done/bin/lib/init.cjs b/get-shit-done/bin/lib/init.cjs index 1e2654729..3eaa42374 100644 --- a/get-shit-done/bin/lib/init.cjs +++ b/get-shit-done/bin/lib/init.cjs @@ -7,6 +7,7 @@ const path = require('path'); const { execSync } = require('child_process'); const { loadConfig, resolveModelInternal, findPhaseInternal, getRoadmapPhaseInternal, pathExistsInternal, generateSlugInternal, getMilestoneInfo, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, normalizePhaseName, toPosixPath, output, error, checkAgentsInstalled, phaseTokenMatches } = require('./core.cjs'); const { planningPaths, planningDir, planningRoot } = require('./planning-workspace.cjs'); +const { maskIfSecret } = require('./secrets.cjs'); // Accept all bold/colon variants of the Requirements header (#2769): // **Requirements:** / **Requirements**: / **Requirements** : render the @@ -725,9 +726,13 @@ function cmdInitPhaseOp(cwd, phase, raw) { const result = { // Config commit_docs: config.commit_docs, - brave_search: config.brave_search, - firecrawl: config.firecrawl, - exa_search: config.exa_search, + // #2997: secret config keys may be either booleans (availability flags) or + // string API keys (when user did `gsd-tools config-set brave_search XXX`). + // Pass booleans through; mask string values so the init bundle never echoes + // plaintext credentials. SDK init.ts mirrors this masking. + brave_search: typeof config.brave_search === 'string' ? maskIfSecret('brave_search', config.brave_search) : config.brave_search, + firecrawl: typeof config.firecrawl === 'string' ? maskIfSecret('firecrawl', config.firecrawl) : config.firecrawl, + exa_search: typeof config.exa_search === 'string' ? maskIfSecret('exa_search', config.exa_search) : config.exa_search, // Phase info phase_found: !!phaseInfo, diff --git a/sdk/src/query/config-mutation.test.ts b/sdk/src/query/config-mutation.test.ts index 44f544b72..e95d9b867 100644 --- a/sdk/src/query/config-mutation.test.ts +++ b/sdk/src/query/config-mutation.test.ts @@ -492,3 +492,54 @@ describe('configEnsureSection', () => { expect(raw.workflow).toEqual({ research: true }); }); }); + +// ─── #2997: Secret masking in configSet response ──────────────────────────── + +describe('configSet secret masking (#2997)', () => { + it('masks the response value for SECRET_CONFIG_KEYS, leaving on-disk plaintext intact', async () => { + const { configSet } = await import('./config-mutation.js'); + const apiKey = 'BSA-1234567890abcdef'; + const result = await configSet(['brave_search', apiKey], tmpDir); + const data = result.data as { value: string }; + // Response is masked + expect(data.value).toBe('****cdef'); + expect(data.value).not.toContain(apiKey); + // On-disk plaintext is intact (the key is usable) + const raw = JSON.parse(await readFile(join(tmpDir, '.planning', 'config.json'), 'utf-8')); + expect(raw.brave_search).toBe(apiKey); + }); + + it('masks previousValue when overwriting an existing secret', async () => { + const { configSet } = await import('./config-mutation.js'); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ brave_search: 'OLD-KEY-1234567890' }), + ); + const result = await configSet(['brave_search', 'NEW-KEY-abcdef9999'], tmpDir); + const data = result.data as { value: string; previousValue: string }; + expect(data.value).toBe('****9999'); + expect(data.previousValue).toBe('****7890'); + expect(data.previousValue).not.toContain('OLD-KEY'); + }); + + it('does NOT mask non-secret keys', async () => { + const { configSet } = await import('./config-mutation.js'); + const result = await configSet(['model_profile', 'quality'], tmpDir); + const data = result.data as { value: string }; + expect(data.value).toBe('quality'); + }); + + it('renders short secret values as **** (no tail leak)', async () => { + const { configSet } = await import('./config-mutation.js'); + const result = await configSet(['firecrawl', 'short'], tmpDir); + const data = result.data as { value: string }; + expect(data.value).toBe('****'); + }); + + it('renders unset/empty as (unset) in the response', async () => { + const { configSet } = await import('./config-mutation.js'); + const result = await configSet(['exa_search', ''], tmpDir); + const data = result.data as { value: string }; + expect(data.value).toBe('(unset)'); + }); +}); diff --git a/sdk/src/query/config-mutation.ts b/sdk/src/query/config-mutation.ts index 0c3aea73d..4ac28f7df 100644 --- a/sdk/src/query/config-mutation.ts +++ b/sdk/src/query/config-mutation.ts @@ -26,6 +26,7 @@ import { VALID_PROFILES, getAgentToModelMapForProfile } from './config-query.js' import { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS } from './config-schema.js'; import { planningPaths } from './helpers.js'; import { acquireStateLock, releaseStateLock } from './state-mutation.js'; +import { maskIfSecret } from './secrets.js'; import type { QueryHandler } from './utils.js'; /** @@ -233,14 +234,17 @@ export const configSet: QueryHandler = async (args, projectDir, workstream) => { await releaseStateLock(lockPath); } - // Match CJS JSON: `JSON.stringify` omits keys whose value is `undefined` + // Mask plaintext for keys in SECRET_CONFIG_KEYS to match CJS behavior at + // config.cjs:362-370 — without this, `gsd-sdk query config-set brave_search XXX` + // would echo the plaintext credential into machine-readable output. (#2997) + // The on-disk value is intentionally NOT masked — only the response. const data: Record = { updated: true, key: keyPath, - value: parsedValue, + value: maskIfSecret(keyPath, parsedValue), }; if (previousValue !== undefined) { - data.previousValue = previousValue; + data.previousValue = maskIfSecret(keyPath, previousValue); } return { data }; }; diff --git a/sdk/src/query/config-query.test.ts b/sdk/src/query/config-query.test.ts index 634363305..dca26ed61 100644 --- a/sdk/src/query/config-query.test.ts +++ b/sdk/src/query/config-query.test.ts @@ -219,3 +219,50 @@ describe('VALID_PROFILES', () => { expect(VALID_PROFILES).toEqual(['quality', 'balanced', 'budget', 'adaptive']); }); }); + +// ─── #2997: Secret masking in configGet response ──────────────────────────── + +describe('configGet secret masking (#2997)', () => { + it('masks the response data for SECRET_CONFIG_KEYS', async () => { + const { configGet } = await import('./config-query.js'); + const apiKey = 'BSA-1234567890abcdef'; + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ brave_search: apiKey }), + ); + const result = await configGet(['brave_search'], tmpDir); + expect(result.data).toBe('****cdef'); + expect(result.data).not.toBe(apiKey); + }); + + it('does NOT mask non-secret keys', async () => { + const { configGet } = await import('./config-query.js'); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'quality' }), + ); + const result = await configGet(['model_profile'], tmpDir); + expect(result.data).toBe('quality'); + }); + + it('renders short secret values as **** (no tail leak)', async () => { + const { configGet } = await import('./config-query.js'); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ firecrawl: 'abc' }), + ); + const result = await configGet(['firecrawl'], tmpDir); + expect(result.data).toBe('****'); + }); + + it('does not mask the user-supplied --default value (it is the user\'s own input, not a stored secret)', async () => { + const { configGet } = await import('./config-query.js'); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'balanced' }), + ); + const result = await configGet(['brave_search', '--default', 'placeholder'], tmpDir); + // Default flows through unchanged: the user typed it, the SDK echoed it. + expect(result.data).toBe('placeholder'); + }); +}); diff --git a/sdk/src/query/config-query.ts b/sdk/src/query/config-query.ts index 4c18bcd36..1d3e54725 100644 --- a/sdk/src/query/config-query.ts +++ b/sdk/src/query/config-query.ts @@ -20,6 +20,7 @@ import { readFile } from 'node:fs/promises'; import { GSDError, ErrorClassification } from '../errors.js'; import { loadConfig } from '../config.js'; import { planningPaths } from './helpers.js'; +import { maskIfSecret } from './secrets.js'; import type { QueryHandler } from './utils.js'; // ─── MODEL_PROFILES ───────────────────────────────────────────────────────── @@ -129,7 +130,10 @@ export const configGet: QueryHandler = async (args, projectDir, workstream) => { throw new GSDError(`Key not found: ${keyPath}`, ErrorClassification.Execution); } - return { data: current }; + // Mask plaintext for keys in SECRET_CONFIG_KEYS to match CJS behavior at + // config.cjs:440-441 — without this, `gsd-sdk query config-get brave_search` + // would echo the plaintext credential into machine-readable output. (#2997) + return { data: maskIfSecret(keyPath, current) }; }; // ─── configPath ───────────────────────────────────────────────────────────── diff --git a/sdk/src/query/init.ts b/sdk/src/query/init.ts index 0dacc300b..8f1488719 100644 --- a/sdk/src/query/init.ts +++ b/sdk/src/query/init.ts @@ -25,6 +25,7 @@ import { homedir } from 'node:os'; import { loadConfig, type GSDConfig } from '../config.js'; import { resolveModel, MODEL_PROFILES } from './config-query.js'; +import { maskIfSecret } from './secrets.js'; import { findPhase } from './phase.js'; import { roadmapGetPhase, getMilestoneInfo, extractCurrentMilestone, extractPhasesFromSection } from './roadmap.js'; import { planningPaths, normalizePhaseName, toPosixPath, resolveAgentsDir, detectRuntime } from './helpers.js'; @@ -670,9 +671,14 @@ export const initPhaseOp: QueryHandler = async (args, projectDir, workstream) => const result: Record = { commit_docs: config.commit_docs, - brave_search: config.brave_search, - firecrawl: config.firecrawl, - exa_search: config.exa_search, + // #2997: secret config keys (brave_search, firecrawl, exa_search) may be + // either boolean availability flags OR string API keys depending on how the + // user configured them. Pass booleans through; mask string values so the + // init bundle never echoes plaintext credentials. Mirrors the masking added + // to config-get/config-set in the same fix. + brave_search: typeof config.brave_search === 'string' ? maskIfSecret('brave_search', config.brave_search) : config.brave_search, + firecrawl: typeof config.firecrawl === 'string' ? maskIfSecret('firecrawl', config.firecrawl) : config.firecrawl, + exa_search: typeof config.exa_search === 'string' ? maskIfSecret('exa_search', config.exa_search) : config.exa_search, phase_found: phaseFound, phase_dir: (phaseInfo?.directory as string) ?? null, phase_number: phaseNumber, diff --git a/sdk/src/query/secrets.test.ts b/sdk/src/query/secrets.test.ts new file mode 100644 index 000000000..d1ec55487 --- /dev/null +++ b/sdk/src/query/secrets.test.ts @@ -0,0 +1,66 @@ +import { describe, it } from 'vitest'; +import assert from 'node:assert/strict'; +import { SECRET_CONFIG_KEYS, isSecretKey, maskSecret, maskIfSecret } from './secrets.js'; +// Parity check against the CJS module. +import secretsCjs from '../../../get-shit-done/bin/lib/secrets.cjs'; + +describe('Bug #2997: SDK secrets module', () => { + it('SECRET_CONFIG_KEYS exposes the documented set (locked)', () => { + assert.deepEqual([...SECRET_CONFIG_KEYS].sort(), ['brave_search', 'exa_search', 'firecrawl']); + }); + + it('isSecretKey returns true for each registered secret', () => { + for (const k of SECRET_CONFIG_KEYS) { + assert.equal(isSecretKey(k), true, `${k} should be a secret`); + } + }); + + it('isSecretKey returns false for unrelated keys', () => { + for (const k of ['model_profile', 'commit_docs', 'workflow.plan_bounce', 'unknown_key']) { + assert.equal(isSecretKey(k), false); + } + }); + + it('maskSecret renders the convention **** for ≥8-char strings', () => { + assert.equal(maskSecret('BSA-secret-key-abcd1234'), '****1234'); + assert.equal(maskSecret('12345678'), '****5678'); + }); + + it('maskSecret renders **** with no tail for <8-char strings', () => { + assert.equal(maskSecret('short'), '****'); + assert.equal(maskSecret('abc'), '****'); + assert.equal(maskSecret('1234567'), '****'); + }); + + it('maskSecret renders (unset) for null/undefined/empty', () => { + assert.equal(maskSecret(null), '(unset)'); + assert.equal(maskSecret(undefined), '(unset)'); + assert.equal(maskSecret(''), '(unset)'); + }); + + it('maskIfSecret passes non-secret values through unchanged', () => { + assert.equal(maskIfSecret('model_profile', 'quality'), 'quality'); + assert.equal(maskIfSecret('commit_docs', true), true); + }); + + it('maskIfSecret masks secret values', () => { + assert.equal(maskIfSecret('brave_search', 'BSA-1234567890'), '****7890'); + assert.equal(maskIfSecret('firecrawl', null), '(unset)'); + }); + + // Parity with the CJS module — single source of truth via test enforcement, + // not import. Ensures SDK and CJS can never drift on the masking rule. + describe('CJS parity (#2997)', () => { + it('SECRET_CONFIG_KEYS matches the CJS set exactly', () => { + const cjsKeys = [...secretsCjs.SECRET_CONFIG_KEYS].sort(); + const tsKeys = [...SECRET_CONFIG_KEYS].sort(); + assert.deepEqual(tsKeys, cjsKeys); + }); + + for (const sample of ['', 'a', 'abc', '12345678', 'BSA-1234567890', null, undefined]) { + it(`maskSecret(${JSON.stringify(sample)}) matches CJS output`, () => { + assert.equal(maskSecret(sample), secretsCjs.maskSecret(sample)); + }); + } + }); +}); diff --git a/sdk/src/query/secrets.ts b/sdk/src/query/secrets.ts new file mode 100644 index 000000000..9a5e2d758 --- /dev/null +++ b/sdk/src/query/secrets.ts @@ -0,0 +1,43 @@ +/** + * Secrets handling — TypeScript mirror of `get-shit-done/bin/lib/secrets.cjs`. + * + * Keys considered sensitive (`SECRET_CONFIG_KEYS`) are masked in any + * machine-readable response from `config-set` / `config-get` so plaintext + * credentials don't end up in workflow output, session transcripts, or + * shell histories. The on-disk value is unchanged; only the response is masked. + * + * Behavior must match `secrets.cjs` exactly. A parity test asserts the + * two modules expose the same set of secret keys and produce identical + * masked output for representative inputs. + * + * Tracked in #2997 (security: SDK port lost masking behavior). + */ + +export const SECRET_CONFIG_KEYS: ReadonlySet = new Set([ + 'brave_search', + 'firecrawl', + 'exa_search', +]); + +export function isSecretKey(keyPath: string): boolean { + return SECRET_CONFIG_KEYS.has(keyPath); +} + +/** + * Convention: ≥8 chars → `****`; <8 chars → `****`; null/empty/undefined → `(unset)`. + * Identical to `secrets.cjs` `maskSecret`. + */ +export function maskSecret(value: unknown): string { + if (value === null || value === undefined || value === '') return '(unset)'; + const s = String(value); + if (s.length < 8) return '****'; + return '****' + s.slice(-4); +} + +/** + * Helper: returns the value masked if `keyPath` is a secret, else the value + * unchanged. Use at response-construction boundaries in query handlers. + */ +export function maskIfSecret(keyPath: string, value: T): T | string { + return isSecretKey(keyPath) ? maskSecret(value) : value; +}