fix: four bug fixes — bold plan checkboxes, BACKLOG recommendations, executor_model regression tests, session_id path traversal
- fix(#1572): phase complete now marks bold-wrapped plan checkboxes in ROADMAP.md (`- [ ] **01-01**` format) by allowing optional `**` around plan IDs in the planCheckboxPattern regex in both phase.cjs and roadmap.cjs - fix(#1569): manager init no longer recommends 999.x (BACKLOG) phases as next actions; add guard in cmdManagerInit that skips phases matching /^999(?:\.|$)/ - fix(#1568): add regression tests confirming init execute-phase respects model_overrides for executor_model, including when resolve_model_ids is 'omit' - fix(#1533): reject session_id values containing path traversal sequences (../, /, \) in gsd-context-monitor and gsd-statusline before constructing /tmp file paths; add security tests covering both hooks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -956,9 +956,11 @@ function cmdInitManager(cwd, raw) {
|
||||
} catch { /* intentionally empty */ }
|
||||
|
||||
// Compute recommended actions (execute > plan > discuss)
|
||||
// Skip BACKLOG phases (999.x numbering) — they are parked ideas, not active work
|
||||
const recommendedActions = [];
|
||||
for (const phase of phases) {
|
||||
if (phase.disk_status === 'complete') continue;
|
||||
if (/^999(?:\.|$)/.test(phase.number)) continue;
|
||||
|
||||
if (phase.disk_status === 'planned' && phase.deps_satisfied) {
|
||||
recommendedActions.push({
|
||||
|
||||
@@ -743,12 +743,13 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
|
||||
);
|
||||
|
||||
// Mark completed plan checkboxes (safety net for missed per-plan updates)
|
||||
// Handles both plain IDs ("- [ ] 01-01-PLAN.md") and bold-wrapped IDs ("- [ ] **01-01**")
|
||||
for (const summaryFile of phaseInfo.summaries) {
|
||||
const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', '');
|
||||
if (!planId) continue;
|
||||
const planEscaped = escapeRegex(planId);
|
||||
const planCheckboxPattern = new RegExp(
|
||||
`(-\\s*\\[) (\\]\\s*${planEscaped})`,
|
||||
`(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`,
|
||||
'i'
|
||||
);
|
||||
roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
|
||||
|
||||
@@ -300,13 +300,13 @@ function cmdRoadmapUpdatePlanProgress(cwd, phaseNum, raw) {
|
||||
roadmapContent = replaceInCurrentMilestone(roadmapContent, checkboxPattern, `$1x$2 (completed ${today})`);
|
||||
}
|
||||
|
||||
// Mark completed plan checkboxes (e.g. "- [ ] 50-01-PLAN.md" or "- [ ] 50-01:")
|
||||
// Mark completed plan checkboxes (e.g. "- [ ] 50-01-PLAN.md", "- [ ] 50-01:", or "- [ ] **50-01**")
|
||||
for (const summaryFile of phaseInfo.summaries) {
|
||||
const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', '');
|
||||
if (!planId) continue;
|
||||
const planEscaped = escapeRegex(planId);
|
||||
const planCheckboxPattern = new RegExp(
|
||||
`(-\\s*\\[) (\\]\\s*${planEscaped})`,
|
||||
`(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`,
|
||||
'i'
|
||||
);
|
||||
roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
|
||||
@@ -314,7 +314,6 @@ function cmdRoadmapUpdatePlanProgress(cwd, phaseNum, raw) {
|
||||
|
||||
fs.writeFileSync(roadmapPath, roadmapContent, 'utf-8');
|
||||
});
|
||||
|
||||
output({
|
||||
updated: true,
|
||||
phase: phaseNum,
|
||||
|
||||
@@ -45,6 +45,13 @@ process.stdin.on('end', () => {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Reject session IDs that contain path traversal sequences or path separators.
|
||||
// session_id is used to construct file paths in /tmp — an unsanitized value
|
||||
// could escape the temp directory and read or write arbitrary files.
|
||||
if (/[/\\]|\.\./.test(sessionId)) {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Check if context warnings are disabled via config
|
||||
const cwd = data.cwd || process.cwd();
|
||||
const configPath = path.join(cwd, '.planning', 'config.json');
|
||||
|
||||
@@ -35,7 +35,10 @@ process.stdin.on('end', () => {
|
||||
|
||||
// Write context metrics to bridge file for the context-monitor PostToolUse hook.
|
||||
// The monitor reads this file to inject agent-facing warnings when context is low.
|
||||
if (session) {
|
||||
// Reject session IDs with path separators or traversal sequences to prevent
|
||||
// a malicious session_id from writing files outside the temp directory.
|
||||
const sessionSafe = session && !/[/\\]|\.\./.test(session);
|
||||
if (sessionSafe) {
|
||||
try {
|
||||
const bridgePath = path.join(os.tmpdir(), `claude-ctx-${session}.json`);
|
||||
const bridgeData = JSON.stringify({
|
||||
|
||||
@@ -479,4 +479,31 @@ describe('init manager', () => {
|
||||
assert.strictEqual(output.manager_flags.plan, '--valid-flag', 'valid flag should pass through');
|
||||
assert.strictEqual(output.manager_flags.execute, '', 'command substitution should be sanitized');
|
||||
});
|
||||
|
||||
test('does not recommend BACKLOG phases (999.x) as next actions', () => {
|
||||
writeState(tmpDir);
|
||||
// Regular phase (planned, deps met) plus a backlog phase (999.1) also planned
|
||||
writeRoadmap(tmpDir, [
|
||||
{ number: '1', name: 'Foundation' },
|
||||
{ number: '999.1', name: 'Nice to have feature (BACKLOG)' },
|
||||
]);
|
||||
// Phase 1: planned (has plan, no summary)
|
||||
scaffoldPhase(tmpDir, 1, { plans: 1 });
|
||||
// Phase 999.1: planned (has plan, no summary)
|
||||
const backlogDir = path.join(tmpDir, '.planning', 'phases', '999.1-backlog');
|
||||
fs.mkdirSync(backlogDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(backlogDir, '999.1-01-PLAN.md'), '# Backlog Plan');
|
||||
|
||||
const result = runGsdTools('init manager', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
const recommended = output.recommended_actions || [];
|
||||
const backlogRecs = recommended.filter(r => /^999/.test(r.phase));
|
||||
assert.strictEqual(backlogRecs.length, 0, 'no 999.x phases should appear in recommended_actions');
|
||||
|
||||
// Phase 1 (non-backlog) should still be recommended
|
||||
const activeRecs = recommended.filter(r => r.phase === '1');
|
||||
assert.strictEqual(activeRecs.length, 1, 'phase 1 should still be recommended');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -33,6 +33,40 @@ describe('init commands', () => {
|
||||
assert.strictEqual(output.config_path, '.planning/config.json');
|
||||
});
|
||||
|
||||
test('init execute-phase respects model_overrides for executor_model', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-foundation');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({
|
||||
model_profile: 'balanced',
|
||||
model_overrides: { 'gsd-executor': 'openai/o4-mini' },
|
||||
}));
|
||||
|
||||
const result = runGsdTools('init execute-phase 1 --raw', tmpDir, { HOME: tmpDir });
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.executor_model, 'openai/o4-mini',
|
||||
'model_overrides["gsd-executor"] must take precedence over profile');
|
||||
});
|
||||
|
||||
test('init execute-phase respects model_overrides when resolve_model_ids is omit', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-foundation');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({
|
||||
resolve_model_ids: 'omit',
|
||||
model_overrides: { 'gsd-executor': 'openai/o4-mini' },
|
||||
}));
|
||||
|
||||
const result = runGsdTools('init execute-phase 1 --raw', tmpDir, { HOME: tmpDir });
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.executor_model, 'openai/o4-mini',
|
||||
'model_overrides must take precedence even when resolve_model_ids is omit');
|
||||
});
|
||||
|
||||
test('init plan-phase returns file paths', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
|
||||
@@ -1743,6 +1743,44 @@ Plans:
|
||||
assert.ok(!roadmap.includes('[ ] 01-01-PLAN.md'), 'plan 01-01 should not remain unchecked');
|
||||
assert.ok(!roadmap.includes('[ ] 01-02-PLAN.md'), 'plan 01-02 should not remain unchecked');
|
||||
});
|
||||
|
||||
test('marks bold-wrapped plan-level checkboxes on phase complete', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap
|
||||
|
||||
- [ ] Phase 1: Foundation
|
||||
|
||||
### Phase 1: Foundation
|
||||
**Goal:** Setup
|
||||
**Plans:** 2 plans
|
||||
|
||||
Plans:
|
||||
- [ ] **01-01**: Schema migration
|
||||
- [ ] **01-02**: Auth setup
|
||||
`
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
`# State\n\n**Current Phase:** 01\n**Status:** In progress\n**Current Plan:** 01-02\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n`
|
||||
);
|
||||
|
||||
const p1 = path.join(tmpDir, '.planning', 'phases', '01-foundation');
|
||||
fs.mkdirSync(p1, { recursive: true });
|
||||
fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary');
|
||||
fs.writeFileSync(path.join(p1, '01-02-PLAN.md'), '# Plan');
|
||||
fs.writeFileSync(path.join(p1, '01-02-SUMMARY.md'), '# Summary');
|
||||
|
||||
const result = runGsdTools('phase complete 1', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
assert.ok(roadmap.includes('[x] **01-01**'), 'bold plan 01-01 checkbox should be checked');
|
||||
assert.ok(roadmap.includes('[x] **01-02**'), 'bold plan 01-02 checkbox should be checked');
|
||||
assert.ok(!roadmap.includes('[ ] **01-01**'), 'bold plan 01-01 should not remain unchecked');
|
||||
assert.ok(!roadmap.includes('[ ] **01-02**'), 'bold plan 01-02 should not remain unchecked');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -8,6 +8,7 @@ const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const fs = require('fs');
|
||||
|
||||
const {
|
||||
validatePath,
|
||||
@@ -414,3 +415,93 @@ describe('validateFieldName', () => {
|
||||
assert.ok(!validateFieldName('-field').valid);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Hook session_id path traversal (#1533) ────────────────────────────────
|
||||
// Verify that gsd-context-monitor and gsd-statusline reject session_id values
|
||||
// containing path traversal sequences before constructing temp file paths.
|
||||
|
||||
const { execFileSync } = require('child_process');
|
||||
|
||||
function runHook(hookPath, inputJson) {
|
||||
try {
|
||||
const result = execFileSync(process.execPath, [hookPath], {
|
||||
input: JSON.stringify(inputJson),
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
timeout: 3000,
|
||||
});
|
||||
return { exitCode: 0, stdout: result };
|
||||
} catch (err) {
|
||||
return { exitCode: err.status || 1, stdout: err.stdout || '', stderr: err.stderr || '' };
|
||||
}
|
||||
}
|
||||
|
||||
describe('gsd-context-monitor session_id path traversal', () => {
|
||||
const monitorPath = path.join(__dirname, '..', 'hooks', 'gsd-context-monitor.js');
|
||||
const tmpDir = os.tmpdir();
|
||||
|
||||
test('exits silently for session_id with ../ traversal', () => {
|
||||
const maliciousId = '../../../etc/passwd';
|
||||
const result = runHook(monitorPath, { session_id: maliciousId });
|
||||
assert.strictEqual(result.exitCode, 0, 'hook should exit 0 for malicious session_id');
|
||||
assert.strictEqual(result.stdout.trim(), '', 'hook should produce no output for malicious session_id');
|
||||
const escapedPath = path.join(tmpDir, 'claude-ctx-' + maliciousId + '.json');
|
||||
assert.ok(!fs.existsSync(escapedPath), 'traversal file must not be created');
|
||||
});
|
||||
|
||||
test('exits silently for session_id with / separator', () => {
|
||||
const maliciousId = 'foo/bar';
|
||||
const result = runHook(monitorPath, { session_id: maliciousId });
|
||||
assert.strictEqual(result.exitCode, 0);
|
||||
assert.strictEqual(result.stdout.trim(), '');
|
||||
});
|
||||
|
||||
test('exits silently for session_id with backslash', () => {
|
||||
const maliciousId = 'foo\\bar';
|
||||
const result = runHook(monitorPath, { session_id: maliciousId });
|
||||
assert.strictEqual(result.exitCode, 0);
|
||||
assert.strictEqual(result.stdout.trim(), '');
|
||||
});
|
||||
});
|
||||
|
||||
describe('gsd-statusline session_id path traversal', () => {
|
||||
const statuslinePath = path.join(__dirname, '..', 'hooks', 'gsd-statusline.js');
|
||||
const tmpDir = os.tmpdir();
|
||||
|
||||
const baseInput = {
|
||||
model: { display_name: 'Claude' },
|
||||
context_window: { remaining_percentage: 80 },
|
||||
workspace: { current_dir: os.tmpdir() },
|
||||
};
|
||||
|
||||
test('does not write bridge file for session_id with ../ traversal', () => {
|
||||
const maliciousId = '../../../etc/gsd-test';
|
||||
const bridgePath = path.join(tmpDir, 'claude-ctx-' + maliciousId + '.json');
|
||||
try { fs.unlinkSync(bridgePath); } catch { /* intentionally empty */ }
|
||||
|
||||
runHook(statuslinePath, { ...baseInput, session_id: maliciousId });
|
||||
|
||||
assert.ok(!fs.existsSync(bridgePath), 'bridge file must not be written for traversal session_id');
|
||||
});
|
||||
|
||||
test('does not write bridge file for session_id with forward slash', () => {
|
||||
const maliciousId = 'sub/path';
|
||||
const bridgePath = path.join(tmpDir, 'claude-ctx-' + maliciousId + '.json');
|
||||
try { fs.unlinkSync(bridgePath); } catch { /* intentionally empty */ }
|
||||
|
||||
runHook(statuslinePath, { ...baseInput, session_id: maliciousId });
|
||||
|
||||
assert.ok(!fs.existsSync(bridgePath), 'bridge file must not be written for session_id with /');
|
||||
});
|
||||
|
||||
test('writes bridge file for safe session_id', () => {
|
||||
const safeId = 'abc123-safe-session';
|
||||
const bridgePath = path.join(tmpDir, 'claude-ctx-' + safeId + '.json');
|
||||
try { fs.unlinkSync(bridgePath); } catch { /* intentionally empty */ }
|
||||
|
||||
runHook(statuslinePath, { ...baseInput, session_id: safeId });
|
||||
|
||||
assert.ok(fs.existsSync(bridgePath), 'bridge file must be written for safe session_id');
|
||||
try { fs.unlinkSync(bridgePath); } catch { /* intentionally empty */ }
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user