From 00e0446b99b9d18faac9f40ae5731891b978ccf6 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 3 Apr 2026 11:19:42 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20four=20bug=20fixes=20=E2=80=94=20bold=20?= =?UTF-8?q?plan=20checkboxes,=20BACKLOG=20recommendations,=20executor=5Fmo?= =?UTF-8?q?del=20regression=20tests,=20session=5Fid=20path=20traversal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- get-shit-done/bin/lib/init.cjs | 2 + get-shit-done/bin/lib/phase.cjs | 3 +- get-shit-done/bin/lib/roadmap.cjs | 5 +- hooks/gsd-context-monitor.js | 7 +++ hooks/gsd-statusline.js | 5 +- tests/init-manager.test.cjs | 27 +++++++++ tests/init.test.cjs | 34 ++++++++++++ tests/phase.test.cjs | 38 +++++++++++++ tests/security.test.cjs | 91 +++++++++++++++++++++++++++++++ 9 files changed, 207 insertions(+), 5 deletions(-) diff --git a/get-shit-done/bin/lib/init.cjs b/get-shit-done/bin/lib/init.cjs index 989f3ef84..48e340c1e 100644 --- a/get-shit-done/bin/lib/init.cjs +++ b/get-shit-done/bin/lib/init.cjs @@ -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({ diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index d6080375a..11f49b0ad 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -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'); diff --git a/get-shit-done/bin/lib/roadmap.cjs b/get-shit-done/bin/lib/roadmap.cjs index e817c53e3..b0cd2f357 100644 --- a/get-shit-done/bin/lib/roadmap.cjs +++ b/get-shit-done/bin/lib/roadmap.cjs @@ -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, diff --git a/hooks/gsd-context-monitor.js b/hooks/gsd-context-monitor.js index ae1bbf9a3..505141072 100644 --- a/hooks/gsd-context-monitor.js +++ b/hooks/gsd-context-monitor.js @@ -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'); diff --git a/hooks/gsd-statusline.js b/hooks/gsd-statusline.js index 32742693e..e745350c8 100755 --- a/hooks/gsd-statusline.js +++ b/hooks/gsd-statusline.js @@ -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({ diff --git a/tests/init-manager.test.cjs b/tests/init-manager.test.cjs index ebdb508e7..02a86eceb 100644 --- a/tests/init-manager.test.cjs +++ b/tests/init-manager.test.cjs @@ -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'); + }); }); diff --git a/tests/init.test.cjs b/tests/init.test.cjs index 8550728be..c625091f2 100644 --- a/tests/init.test.cjs +++ b/tests/init.test.cjs @@ -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 }); diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index c45ddb54d..6ba019389 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -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'); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/security.test.cjs b/tests/security.test.cjs index f6a12d38b..a4294f2b8 100644 --- a/tests/security.test.cjs +++ b/tests/security.test.cjs @@ -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 */ } + }); +});