fix(#2041): address code+security review findings
- add typeof guard so a non-string override passes through verbatim instead of crashing on .startsWith (preserves pre-fix no-crash behaviour) [LOW-1] - use Object.hasOwn() for the alias lookup so __proto__/constructor cannot return a truthy non-string from the plain object literal [LOW-D3] - cap the unmappable-override stderr warning at 64 chars so an oversized or secret-shaped value cannot leak in full to stderr/logs [LOW-D4] - remove the unused mapClaudeOverrideForRuntime export (helpers are covered behaviourally via resolveModelInternal/resolveModelForTier) [NIT] - add resolveModelForTier unmappable-override fall-through test (closes the mutation-score gap) [MEDIUM-1] - add case-sensitivity contract test (Claude-Sonnet-5 passes through verbatim) [LOW-2] Both orthogonal reviews returned APPROVE with no Critical/High findings.
This commit is contained in:
@@ -111,9 +111,12 @@ function warnModelOverrideUnmappable(agentType: string, overrideValue: string):
|
||||
const key = `${agentType}::${overrideValue}`;
|
||||
if (_modelOverrideUnmappableWarned.has(key)) return;
|
||||
_modelOverrideUnmappableWarned.add(key);
|
||||
// MUST go to stderr — resolve-model's JSON result is parsed from stdout.
|
||||
// Cap emission length so an oversized or secret-shaped value cannot leak in
|
||||
// full to stderr/logs (#2041 security review). MUST go to stderr — resolve-
|
||||
// model's JSON result is parsed from stdout.
|
||||
const safe = overrideValue.length > 64 ? overrideValue.slice(0, 64) + '…' : overrideValue;
|
||||
process.stderr.write(
|
||||
`gsd: warning — model_overrides value "${overrideValue}" for ${agentType} ` +
|
||||
`gsd: warning — model_overrides value "${safe}" for ${agentType} ` +
|
||||
`has no Claude agent alias; falling through to tier resolution.\n`,
|
||||
);
|
||||
}
|
||||
@@ -132,16 +135,28 @@ function _resetModelOverrideWarningCacheForTests(): void {
|
||||
* through to normal tier/dynamic-routing resolution" (used when a Claude full
|
||||
* ID has no alias — matches model_policy's warn-and-fall-through). Non-Claude
|
||||
* runtimes and non-Claude values always pass through verbatim.
|
||||
*
|
||||
* Hardening (code+security review): a `typeof` guard preserves the pre-fix
|
||||
* no-crash behavior if a malformed config surfaces a non-string value, and an
|
||||
* `Object.hasOwn` lookup defeats `__proto__`/`constructor` lookups on the plain
|
||||
* object literal so those reserved keys cannot return a truthy non-string.
|
||||
*/
|
||||
function mapClaudeOverrideForRuntime(
|
||||
override: string,
|
||||
configRuntime: string | null | undefined,
|
||||
agentType: string,
|
||||
): string | null {
|
||||
// Defensive: model_overrides is typed Record<string,string> but a malformed
|
||||
// config could surface a non-string; pass through verbatim (preserving the
|
||||
// pre-fix no-crash behaviour) and let the downstream Agent tool reject it.
|
||||
if (typeof override !== 'string') return override;
|
||||
const onClaude = !configRuntime || configRuntime === 'claude';
|
||||
if (!onClaude) return override;
|
||||
const alias = CLAUDE_POLICY_ID_TO_ALIAS[override];
|
||||
if (alias) return alias;
|
||||
// Object.hasOwn guards against __proto__/constructor returning a truthy
|
||||
// non-string from the plain object literal (#2041 security review).
|
||||
if (Object.hasOwn(CLAUDE_POLICY_ID_TO_ALIAS, override)) {
|
||||
return CLAUDE_POLICY_ID_TO_ALIAS[override];
|
||||
}
|
||||
if (CLAUDE_AGENT_ALIASES.has(override)) return override;
|
||||
if (override.startsWith('claude-')) {
|
||||
warnModelOverrideUnmappable(agentType, override);
|
||||
@@ -562,7 +577,6 @@ export = {
|
||||
resolveModelInternal,
|
||||
_resetModelPolicyWarningCacheForTests,
|
||||
_resetModelOverrideWarningCacheForTests,
|
||||
mapClaudeOverrideForRuntime,
|
||||
VALID_GRANULARITIES,
|
||||
resolveGranularityInternal,
|
||||
assertValidGranularityOverride,
|
||||
|
||||
@@ -3909,6 +3909,32 @@ describe('#2041 model_overrides: Claude full ID → alias on claude runtime', ()
|
||||
assert.strictEqual(resolveModelForTier(tmpDir, 'gsd-executor', 0), 'claude-sonnet-5');
|
||||
});
|
||||
|
||||
// MEDIUM-1 (review): exercise the unmappable-override fall-through branch in
|
||||
// resolveModelForTier (closes the mutation-score gap — a future refactor that
|
||||
// accidentally returned the verbatim override instead of falling through
|
||||
// would otherwise survive the suite).
|
||||
test('resolveModelForTier unmappable claude ID falls through to tier alias on claude', () => {
|
||||
resetRuntimeWarningCaches();
|
||||
writeConfig(tmpDir, {
|
||||
runtime: 'claude',
|
||||
model_profile: 'balanced',
|
||||
model_overrides: { 'gsd-planner': 'claude-opus-4-5' },
|
||||
});
|
||||
// unmappable override → fall through → no dynamic_routing → resolveModelInternal → 'opus'
|
||||
assert.strictEqual(resolveModelForTier(tmpDir, 'gsd-planner', 0), 'opus');
|
||||
});
|
||||
|
||||
// LOW-2 (review): pin the case-sensitive contract — a case-variant like
|
||||
// "Claude-Sonnet-5" is NOT mapped (alias keys are case-sensitive, matching
|
||||
// the model_policy path and the Claude API).
|
||||
test('model_overrides case-variant "Claude-Sonnet-5" passes through verbatim (case-sensitive contract)', () => {
|
||||
writeConfig(tmpDir, {
|
||||
runtime: 'claude',
|
||||
model_overrides: { 'gsd-executor': 'Claude-Sonnet-5' },
|
||||
});
|
||||
assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'Claude-Sonnet-5');
|
||||
});
|
||||
|
||||
// Regression guard: non-Claude custom / vendor values still pass through verbatim
|
||||
// on the claude runtime (the fix must NOT touch values that aren't Claude IDs).
|
||||
test('model_overrides non-Claude custom model passes through verbatim on runtime:claude', () => {
|
||||
|
||||
Reference in New Issue
Block a user