From 7b510c886e40ff14a08e0e8ddff5445fa36c3f32 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Wed, 25 Mar 2026 03:36:36 -0400 Subject: [PATCH] fix: harden session hook guards and session ID handling --- hooks/hooks.json | 2 +- scripts/lib/session-manager.js | 15 ++++++++-- scripts/lib/utils.js | 17 ++++++++--- tests/hooks/config-protection.test.js | 4 +-- tests/hooks/hooks.test.js | 43 ++++++++++++++++++++++++--- tests/integration/hooks.test.js | 16 +++++----- tests/lib/session-manager.test.js | 23 +++++++------- tests/lib/utils.test.js | 12 ++++++-- 8 files changed, 96 insertions(+), 36 deletions(-) diff --git a/hooks/hooks.json b/hooks/hooks.json index 56da376b..8b7dade7 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -136,7 +136,7 @@ "hooks": [ { "type": "command", - "command": "node -e \"const fs=require('fs');const path=require('path');const {spawnSync}=require('child_process');const raw=fs.readFileSync(0,'utf8');const root=(()=>{const envRoot=process.env.CLAUDE_PLUGIN_ROOT||'';if(envRoot.trim())return envRoot.trim();const home=require('os').homedir();const claudeDir=path.join(home,'.claude');const probe=path.join('scripts','lib','utils.js');if(fs.existsSync(path.join(claudeDir,probe)))return claudeDir;for(const candidate of [path.join(claudeDir,'plugins','everything-claude-code'),path.join(claudeDir,'plugins','everything-claude-code@everything-claude-code'),path.join(claudeDir,'plugins','marketplace','everything-claude-code')]){if(fs.existsSync(path.join(candidate,probe)))return candidate;}try{const cacheBase=path.join(claudeDir,'plugins','cache','everything-claude-code');for(const org of fs.readdirSync(cacheBase,{withFileTypes:true})){if(!org.isDirectory())continue;for(const version of fs.readdirSync(path.join(cacheBase,org.name),{withFileTypes:true})){if(!version.isDirectory())continue;const candidate=path.join(cacheBase,org.name,version.name);if(fs.existsSync(path.join(candidate,probe)))return candidate;}}}catch{}return claudeDir;})();const script=path.join(root,'scripts','hooks','run-with-flags.js');if(fs.existsSync(script)){const result=spawnSync(process.execPath,[script,'session:start','scripts/hooks/session-start.js','minimal,standard,strict'],{input:raw,encoding:'utf8',env:process.env,cwd:process.cwd(),timeout:30000});if(result.stdout)process.stdout.write(result.stdout);if(result.stderr)process.stderr.write(result.stderr);process.exit(Number.isInteger(result.status)?result.status:0);}process.stderr.write('[SessionStart] WARNING: could not resolve ECC plugin root; skipping session-start hook'+String.fromCharCode(10));process.stdout.write(raw);\"" + "command": "node -e \"const fs=require('fs');const path=require('path');const {spawnSync}=require('child_process');const raw=fs.readFileSync(0,'utf8');const rel=path.join('scripts','hooks','run-with-flags.js');const hasRunnerRoot=candidate=>{const value=typeof candidate==='string'?candidate.trim():'';return value.length>0&&fs.existsSync(path.join(path.resolve(value),rel));};const root=(()=>{const envRoot=process.env.CLAUDE_PLUGIN_ROOT||'';if(hasRunnerRoot(envRoot))return path.resolve(envRoot.trim());const home=require('os').homedir();const claudeDir=path.join(home,'.claude');if(hasRunnerRoot(claudeDir))return claudeDir;for(const candidate of [path.join(claudeDir,'plugins','everything-claude-code'),path.join(claudeDir,'plugins','everything-claude-code@everything-claude-code'),path.join(claudeDir,'plugins','marketplace','everything-claude-code')]){if(hasRunnerRoot(candidate))return candidate;}try{const cacheBase=path.join(claudeDir,'plugins','cache','everything-claude-code');for(const org of fs.readdirSync(cacheBase,{withFileTypes:true})){if(!org.isDirectory())continue;for(const version of fs.readdirSync(path.join(cacheBase,org.name),{withFileTypes:true})){if(!version.isDirectory())continue;const candidate=path.join(cacheBase,org.name,version.name);if(hasRunnerRoot(candidate))return candidate;}}}catch{}return claudeDir;})();const script=path.join(root,rel);if(fs.existsSync(script)){const result=spawnSync(process.execPath,[script,'session:start','scripts/hooks/session-start.js','minimal,standard,strict'],{input:raw,encoding:'utf8',env:process.env,cwd:process.cwd(),timeout:30000});const stdout=typeof result.stdout==='string'?result.stdout:'';if(stdout)process.stdout.write(stdout);else process.stdout.write(raw);if(result.stderr)process.stderr.write(result.stderr);if(result.error||result.status===null||result.signal){const reason=result.error?result.error.message:(result.signal?'signal '+result.signal:'missing exit status');process.stderr.write('[SessionStart] ERROR: session-start hook failed: '+reason+String.fromCharCode(10));process.exit(1);}process.exit(Number.isInteger(result.status)?result.status:0);}process.stderr.write('[SessionStart] WARNING: could not resolve ECC plugin root; skipping session-start hook'+String.fromCharCode(10));process.stdout.write(raw);\"" } ], "description": "Load previous context and detect package manager on new session" diff --git a/scripts/lib/session-manager.js b/scripts/lib/session-manager.js index e057e774..49a44307 100644 --- a/scripts/lib/session-manager.js +++ b/scripts/lib/session-manager.js @@ -322,6 +322,15 @@ function getAllSessions(options = {}) { * @returns {object|null} Session object or null if not found */ function getSessionById(sessionId, includeContent = false) { + if (typeof sessionId !== 'string') { + return null; + } + + const normalizedSessionId = sessionId.trim(); + if (!normalizedSessionId) { + return null; + } + const sessions = getSessionCandidates(); for (const session of sessions) { @@ -334,9 +343,9 @@ function getSessionById(sessionId, includeContent = false) { }; // Check if session ID matches (short ID or full filename without .tmp) - const shortIdMatch = sessionId.length > 0 && metadata.shortId !== 'no-id' && metadata.shortId.startsWith(sessionId); - const filenameMatch = filename === sessionId || filename === `${sessionId}.tmp`; - const noIdMatch = metadata.shortId === 'no-id' && filename === `${sessionId}-session.tmp`; + const shortIdMatch = metadata.shortId !== 'no-id' && metadata.shortId.startsWith(normalizedSessionId); + const filenameMatch = filename === normalizedSessionId || filename === `${normalizedSessionId}.tmp`; + const noIdMatch = metadata.shortId === 'no-id' && filename === `${normalizedSessionId}-session.tmp`; if (!shortIdMatch && !filenameMatch && !noIdMatch) { continue; diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index 9f0b175e..7a5796a3 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -13,6 +13,13 @@ const { execSync, spawnSync } = require('child_process'); const isWindows = process.platform === 'win32'; const isMacOS = process.platform === 'darwin'; const isLinux = process.platform === 'linux'; +const SESSION_DATA_DIR_NAME = 'session-data'; +const LEGACY_SESSIONS_DIR_NAME = 'sessions'; +const WINDOWS_RESERVED_SESSION_IDS = new Set([ + 'CON', 'PRN', 'AUX', 'NUL', + 'COM1', 'COM2', 'COM3', 'COM4', 'COM5', 'COM6', 'COM7', 'COM8', 'COM9', + 'LPT1', 'LPT2', 'LPT3', 'LPT4', 'LPT5', 'LPT6', 'LPT7', 'LPT8', 'LPT9' +]); /** * Get the user's home directory (cross-platform) @@ -32,14 +39,14 @@ function getClaudeDir() { * Get the sessions directory */ function getSessionsDir() { - return path.join(getClaudeDir(), 'session-data'); + return path.join(getClaudeDir(), SESSION_DATA_DIR_NAME); } /** * Get the legacy sessions directory used by older ECC installs */ function getLegacySessionsDir() { - return path.join(getClaudeDir(), 'sessions'); + return path.join(getClaudeDir(), LEGACY_SESSIONS_DIR_NAME); } /** @@ -143,9 +150,11 @@ function sanitizeSessionId(raw) { .replace(/^-+|-+$/g, ''); if (sanitized.length > 0) { - if (!hasNonAscii) return sanitized; - const suffix = crypto.createHash('sha256').update(normalized).digest('hex').slice(0, 6); + if (WINDOWS_RESERVED_SESSION_IDS.has(sanitized.toUpperCase())) { + return `${sanitized}-${suffix}`; + } + if (!hasNonAscii) return sanitized; return `${sanitized}-${suffix}`; } diff --git a/tests/hooks/config-protection.test.js b/tests/hooks/config-protection.test.js index 049090ff..8c7654ab 100644 --- a/tests/hooks/config-protection.test.js +++ b/tests/hooks/config-protection.test.js @@ -35,7 +35,7 @@ function runHook(input, env = {}) { }); return { - code: result.status ?? 0, + code: Number.isInteger(result.status) ? result.status : 1, stdout: result.stdout || '', stderr: result.stderr || '' }; @@ -98,4 +98,4 @@ function runTests() { process.exit(failed > 0 ? 1 : 0); } -runTests(); \ No newline at end of file +runTests(); diff --git a/tests/hooks/hooks.test.js b/tests/hooks/hooks.test.js index 2837271c..7f171b74 100644 --- a/tests/hooks/hooks.test.js +++ b/tests/hooks/hooks.test.js @@ -91,10 +91,7 @@ function getLegacySessionsDir(homeDir) { } function getSessionStartAdditionalContext(stdout) { - if (!stdout.trim()) { - return ''; - } - + assert.ok(stdout.trim(), 'Expected SessionStart hook to emit stdout payload'); const payload = JSON.parse(stdout); assert.strictEqual(payload.hookSpecificOutput?.hookEventName, 'SessionStart', 'Should emit SessionStart hook payload'); assert.strictEqual(typeof payload.hookSpecificOutput?.additionalContext, 'string', 'Should include additionalContext text'); @@ -435,6 +432,42 @@ async function runTests() { passed++; else failed++; + if ( + await asyncTest('prefers canonical session-data content over legacy duplicates', async () => { + const isoHome = path.join(os.tmpdir(), `ecc-canonical-start-${Date.now()}`); + const canonicalDir = getCanonicalSessionsDir(isoHome); + const legacyDir = getLegacySessionsDir(isoHome); + const filename = '2026-02-11-dupe1234-session.tmp'; + const canonicalFile = path.join(canonicalDir, filename); + const legacyFile = path.join(legacyDir, filename); + const sameTime = new Date('2026-02-11T12:00:00Z'); + + fs.mkdirSync(canonicalDir, { recursive: true }); + fs.mkdirSync(legacyDir, { recursive: true }); + fs.mkdirSync(path.join(isoHome, '.claude', 'skills', 'learned'), { recursive: true }); + + fs.writeFileSync(canonicalFile, '# Canonical Session\n\nUse the canonical session-data copy.\n'); + fs.writeFileSync(legacyFile, '# Legacy Session\n\nDo not prefer the legacy duplicate.\n'); + fs.utimesSync(canonicalFile, sameTime, sameTime); + fs.utimesSync(legacyFile, sameTime, sameTime); + + try { + const result = await runScript(path.join(scriptsDir, 'session-start.js'), '', { + HOME: isoHome, + USERPROFILE: isoHome + }); + assert.strictEqual(result.code, 0); + const additionalContext = getSessionStartAdditionalContext(result.stdout); + assert.ok(additionalContext.includes('canonical session-data copy')); + assert.ok(!additionalContext.includes('legacy duplicate')); + } finally { + fs.rmSync(isoHome, { recursive: true, force: true }); + } + }) + ) + passed++; + else failed++; + if ( await asyncTest('strips ANSI escape codes from injected session content', async () => { const isoHome = path.join(os.tmpdir(), `ecc-ansi-start-${Date.now()}`); @@ -1924,6 +1957,8 @@ async function runTests() { assert.ok(sessionStartHook.command.includes("plugins','everything-claude-code@everything-claude-code'"), 'Should probe the namespaced legacy plugin root'); assert.ok(sessionStartHook.command.includes("plugins','marketplace','everything-claude-code'"), 'Should probe the marketplace legacy plugin root'); assert.ok(sessionStartHook.command.includes("plugins','cache','everything-claude-code'"), 'Should retain cache lookup fallback'); + assert.ok(sessionStartHook.command.includes('if(hasRunnerRoot(envRoot))return path.resolve(envRoot.trim())'), 'Should validate CLAUDE_PLUGIN_ROOT before trusting it'); + assert.ok(sessionStartHook.command.includes('else process.stdout.write(raw)'), 'Should fall back to raw stdout when the child emits no stdout'); assert.ok(!sessionStartHook.command.includes('find '), 'Should not scan arbitrary plugin paths with find'); assert.ok(!sessionStartHook.command.includes('head -n 1'), 'Should not pick the first matching plugin path'); }) diff --git a/tests/integration/hooks.test.js b/tests/integration/hooks.test.js index 45c8895d..d7753325 100644 --- a/tests/integration/hooks.test.js +++ b/tests/integration/hooks.test.js @@ -91,11 +91,10 @@ function runHookWithInput(scriptPath, input = {}, env = {}, timeoutMs = 10000) { } function getSessionStartPayload(stdout) { - if (!stdout.trim()) { - return null; - } - - return JSON.parse(stdout); + assert.ok(stdout.trim(), 'Expected SessionStart hook to emit stdout payload'); + const payload = JSON.parse(stdout); + assert.strictEqual(payload.hookSpecificOutput?.hookEventName, 'SessionStart'); + return payload; } /** @@ -262,10 +261,9 @@ async function runTests() { // Session-start should write info to stderr assert.ok(result.stderr.length > 0, 'Should have stderr output'); assert.ok(result.stderr.includes('[SessionStart]'), 'Should have [SessionStart] prefix'); - if (result.stdout.trim()) { - const payload = getSessionStartPayload(result.stdout); - assert.strictEqual(payload.hookSpecificOutput?.hookEventName, 'SessionStart'); - } + const payload = getSessionStartPayload(result.stdout); + assert.ok(payload.hookSpecificOutput, 'Should include hookSpecificOutput'); + assert.strictEqual(payload.hookSpecificOutput.hookEventName, 'SessionStart'); })) passed++; else failed++; if (await asyncTest('PreCompact hook logs to stderr', async () => { diff --git a/tests/lib/session-manager.test.js b/tests/lib/session-manager.test.js index 50fe2d66..116ff602 100644 --- a/tests/lib/session-manager.test.js +++ b/tests/lib/session-manager.test.js @@ -477,6 +477,12 @@ src/main.ts assert.strictEqual(result, null, 'Empty string should not match any session'); })) passed++; else failed++; + if (test('getSessionById returns null for non-string IDs', () => { + assert.strictEqual(sessionManager.getSessionById(null), null); + assert.strictEqual(sessionManager.getSessionById(undefined), null); + assert.strictEqual(sessionManager.getSessionById(42), null); + })) passed++; else failed++; + if (test('getSessionById metadata and stats populated when includeContent=true', () => { const result = sessionManager.getSessionById('abcd1234', true); assert.ok(result, 'Should find session'); @@ -1601,18 +1607,13 @@ src/main.ts 'Null search should return sessions (confirming they exist but space filtered them)'); })) passed++; else failed++; - // ── Round 98: getSessionById with null sessionId throws TypeError ── - console.log('\nRound 98: getSessionById (null sessionId — crashes at line 297):'); + // ── Round 98: getSessionById with null sessionId returns null ── + console.log('\nRound 98: getSessionById (null sessionId — guarded null return):'); - if (test('getSessionById(null) throws TypeError when session files exist', () => { - // session-manager.js line 297: `sessionId.length > 0` — calling .length on null - // throws TypeError because there's no early guard for null/undefined input. - // This only surfaces when valid .tmp files exist in the sessions directory. - assert.throws( - () => sessionManager.getSessionById(null), - { name: 'TypeError' }, - 'null.length should throw TypeError (no input guard at function entry)' - ); + if (test('getSessionById(null) returns null when session files exist', () => { + // Keep a populated sessions directory so the early input guard is exercised even when + // candidate files are present. + assert.strictEqual(sessionManager.getSessionById(null), null); })) passed++; else failed++; // Cleanup test environment for Rounds 95-98 that needed sessions diff --git a/tests/lib/utils.test.js b/tests/lib/utils.test.js index d05bbc18..8040acec 100644 --- a/tests/lib/utils.test.js +++ b/tests/lib/utils.test.js @@ -74,8 +74,8 @@ function runTests() { if (test('getSessionSearchDirs includes canonical and legacy paths', () => { const searchDirs = utils.getSessionSearchDirs(); - assert.ok(searchDirs.includes(utils.getSessionsDir()), 'Should include canonical session dir'); - assert.ok(searchDirs.includes(utils.getLegacySessionsDir()), 'Should include legacy session dir'); + assert.strictEqual(searchDirs[0], utils.getSessionsDir(), 'Canonical session dir should be searched first'); + assert.strictEqual(searchDirs[1], utils.getLegacySessionsDir(), 'Legacy session dir should be searched second'); })) passed++; else failed++; if (test('getTempDir returns valid temp directory', () => { @@ -184,6 +184,14 @@ function runTests() { } })) passed++; else failed++; + if (test('sanitizeSessionId avoids Windows reserved device names', () => { + const con = utils.sanitizeSessionId('CON'); + const aux = utils.sanitizeSessionId('aux'); + assert.ok(con.startsWith('CON-'), `Expected CON to get a suffix, got: ${con}`); + assert.ok(aux.startsWith('aux-'), `Expected aux to get a suffix, got: ${aux}`); + assert.notStrictEqual(utils.sanitizeSessionId('COM1'), 'COM1'); + })) passed++; else failed++; + // Session ID tests console.log('\nSession ID Functions:');