fix(#2003): address code+security review findings

- warn (don't silently ignore) when --runtime is an unknown runtime that
  canonicalizeRuntimeName rejects; the warning surfaces via warnings[] so a
  typo like --runtime cluade or a runtime known to runtime-homes but not the
  alias manifest (e.g. grok) no longer silently resolves to the persisted
  runtime's config dir on this diagnostic command [M-1]
- add end-to-end CLI test for loop render-hooks --runtime (the exact command
  the bug report calls out as silently no-op'ing) [L-2]
- add closed-vocabulary rejection test: crafted --runtime values
  (../../etc/passwd, __proto__, --config-dir, garbage) are rejected, warn,
  and fall through to the persisted runtime — pins the security-load-bearing
  contract [NIT-01]
- add boundary tests: --config-dir wins over --runtime (precedence); missing
  --runtime value errors with USAGE [N-1]

Both orthogonal reviews returned APPROVE with no Critical/High findings.
Security review confirmed --runtime cannot coerce getGlobalConfigDir into an
arbitrary path (closed-vocabulary Map lookup + registry hash-key gate) and
does not expand the trust surface beyond the existing operator-controlled
--config-dir flag.
This commit is contained in:
Tom Boucher
2026-07-06 23:32:06 -04:00
parent def745fa6b
commit 327b6409e8
2 changed files with 93 additions and 0 deletions

View File

@@ -477,6 +477,16 @@ function resolveCapabilityRuntimeState(
const canonical = runtimeNamePolicy.canonicalizeRuntimeName(runtimeOverride);
if (canonical) {
resolvedConfigDir = runtimeHomes.getGlobalConfigDir(canonical);
} else {
// #2003: unknown runtime override — warn (don't silently ignore the
// explicit input) and fall through to persisted-runtime resolution.
// Avoids a silent-wrong-result on this diagnostic command for typos
// (e.g. "cluade") or runtimes known to runtime-homes but not yet to
// the alias manifest (e.g. "grok"). The warning surfaces via the
// `warnings[]` channel consumed by cmdCapabilityState/cmdLoopRenderHooks.
warnings.push(
`--runtime "${runtimeOverride}" is not a known runtime; falling back to auto-detected/persisted runtime resolution`,
);
}
}
if (!resolvedConfigDir) {

View File

@@ -1900,4 +1900,87 @@ describe('regressions: --runtime override bypasses persisted runtime (#2003)', (
cleanup(tmpDir);
}
});
// L-2 (review): end-to-end CLI test for loop render-hooks --runtime — the exact
// command the bug report calls out as silently no-op'ing. Guards the copy-pasted
// arg parsing in gsd-tools.cjs from diverging from the capability-state branch.
test('CLI: `loop render-hooks verify:post --runtime claude` resolves against the Claude config dir', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rt-loop-cli-'));
try {
writePersistedRuntime(tmpDir, 'codex');
const result = runGsdTools('loop render-hooks verify:post --runtime claude --raw', tmpDir);
assert.ok(result.success, `loop render-hooks --runtime should succeed: ${result.error || ''}`);
const parsed = JSON.parse(result.output);
// The envelope carries activeHooks/rendered; the key assertion is that it
// ran without silently no-op'ing against the codex dir. A non-empty
// point + a rendered string (even "_No active hooks..._") proves the
// command executed against the resolved (claude) config dir rather than
// erroring or emitting nothing.
assert.strictEqual(parsed.point, 'verify:post', 'render-hooks must echo the requested point');
assert.ok(typeof parsed.rendered === 'string', 'render-hooks must produce a rendered string');
} finally {
cleanup(tmpDir);
}
});
// M-1 + NIT-01 (review): unknown / crafted --runtime values are REJECTED by the
// closed-vocabulary canonicalizer, warn, and fall through to the persisted
// runtime (never embedded into a path). This is the security-load-bearing
// contract — pin it so a future refactor can't silently break it.
test('resolveCapabilityRuntimeState: unknown/crafted --runtime is rejected (closed vocabulary), warns, and falls through to persisted runtime', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rt-reject-'));
const savedGsdRuntime = process.env.GSD_RUNTIME;
delete process.env.GSD_RUNTIME;
try {
writePersistedRuntime(tmpDir, 'codex');
const runtimeHomes = require('../gsd-core/bin/lib/runtime-homes.cjs');
const expectedCodexDir = runtimeHomes.getGlobalConfigDir('codex');
const crafted = ['../../etc/passwd', '__proto__', 'constructor', '--config-dir', 'garbage', 'foo bar', 'cluade'];
for (const bad of crafted) {
const result = resolveCapabilityRuntimeState(tmpDir, undefined, undefined, bad);
assert.strictEqual(result.runtimeConfigDir, expectedCodexDir,
`crafted --runtime "${bad}" must fall through to persisted codex dir, not be embedded in a path`);
assert.ok(result.warnings.some((w) => w.includes('--runtime') && w.includes(bad)),
`crafted --runtime "${bad}" must emit a warning naming the rejected value`);
}
} finally {
if (savedGsdRuntime === undefined) delete process.env.GSD_RUNTIME;
else process.env.GSD_RUNTIME = savedGsdRuntime;
cleanup(tmpDir);
}
});
// N-1 (review): CLI arg-parsing boundary — --config-dir wins over --runtime
// (most-explicit input takes precedence), and missing --runtime value errors.
test('CLI: --config-dir takes precedence over --runtime when both are given', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rt-precedence-'));
const explicitDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rt-explicit-cfg-'));
try {
writePersistedRuntime(tmpDir, 'codex');
const result = runGsdTools(
`capability state --config-dir ${explicitDir} --runtime claude --raw`,
tmpDir,
);
assert.ok(result.success, `capability state with both flags should succeed: ${result.error || ''}`);
const parsed = JSON.parse(result.output);
assert.strictEqual(parsed.runtimeConfigDir, explicitDir,
'--config-dir (explicit path) must win over --runtime when both are present');
} finally {
cleanup(explicitDir);
cleanup(tmpDir);
}
});
test('CLI: `capability state --runtime` with no value errors with USAGE', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rt-missing-'));
try {
writePersistedRuntime(tmpDir, 'codex');
const result = runGsdTools('capability state --runtime', tmpDir);
assert.ok(!result.success, 'missing --runtime value must produce a non-zero exit');
assert.ok(/Missing value for --runtime/.test(result.error || ''),
`error must name the missing --runtime value: ${result.error || ''}`);
} finally {
cleanup(tmpDir);
}
});
});