* fix(#2997): mask SECRET_CONFIG_KEYS in SDK config-set/get and init responses The CJS→TS port at sdk/src/query/config-mutation.ts:240,243 and config-query.ts:122,128,132 dropped the masking layer that secrets.cjs spec defines for brave_search/firecrawl/exa_search. Result: the SDK echoed plaintext API keys into machine-readable JSON output (stdout, transcripts, CI logs). Adjacent leak in init.ts:673-675 / init.cjs:728-730: the init bundle passed config.brave_search through raw, leaking the API key whenever the user had stored one. Fix: - New sdk/src/query/secrets.ts ports SECRET_CONFIG_KEYS, isSecretKey, maskSecret, maskIfSecret. Exact CJS parity (verified by 17 tests in secrets.test.ts that import secrets.cjs and compare). - config-set masks value + previousValue in response; on-disk plaintext intact (key stays usable). - config-get masks read response. --default flows through unmasked (user's own input, not stored secret). - init.ts/init.cjs mask string values only; booleans (availability flags) pass through unchanged so the typed contract is preserved. Tests: 17 in secrets.test.ts (including CJS parity), 5 in config-mutation.test.ts (#2997 block — covers on-disk-preserved, previousValue masking, short-value, unset, non-secret pass-through), 4 in config-query.test.ts. Closes #2997 * chore(#2997): add changeset fragment for PR #2999 * chore(#2997): add changeset fragment for PR #2999 * chore(#2999): drop direct CHANGELOG.md edit; release entry now lives in .changeset/ The changeset-fragment workflow (#2975) renders fragments into CHANGELOG.md at release time. Direct edits to [Unreleased] on each PR caused merge conflicts on every concurrent PR. This commit restores CHANGELOG.md to match origin/main; the release entry for this fix is preserved in the .changeset/*.md fragment(s) on this branch, which the release workflow consolidates.
This commit is contained in:
5
.changeset/merry-foxes-climb.md
Normal file
5
.changeset/merry-foxes-climb.md
Normal file
@@ -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.
|
||||
5
.changeset/plucky-moles-roam.md
Normal file
5
.changeset/plucky-moles-roam.md
Normal file
@@ -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.
|
||||
@@ -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,
|
||||
|
||||
@@ -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)');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
updated: true,
|
||||
key: keyPath,
|
||||
value: parsedValue,
|
||||
value: maskIfSecret(keyPath, parsedValue),
|
||||
};
|
||||
if (previousValue !== undefined) {
|
||||
data.previousValue = previousValue;
|
||||
data.previousValue = maskIfSecret(keyPath, previousValue);
|
||||
}
|
||||
return { data };
|
||||
};
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 ─────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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,
|
||||
|
||||
66
sdk/src/query/secrets.test.ts
Normal file
66
sdk/src/query/secrets.test.ts
Normal file
@@ -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 ****<last-4> 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));
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
43
sdk/src/query/secrets.ts
Normal file
43
sdk/src/query/secrets.ts
Normal file
@@ -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<string> = new Set([
|
||||
'brave_search',
|
||||
'firecrawl',
|
||||
'exa_search',
|
||||
]);
|
||||
|
||||
export function isSecretKey(keyPath: string): boolean {
|
||||
return SECRET_CONFIG_KEYS.has(keyPath);
|
||||
}
|
||||
|
||||
/**
|
||||
* Convention: ≥8 chars → `****<last-4>`; <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<T>(keyPath: string, value: T): T | string {
|
||||
return isSecretKey(keyPath) ? maskSecret(value) : value;
|
||||
}
|
||||
Reference in New Issue
Block a user