From 926eba97c5f60daff854fe7f8241f567d9633f0d Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Thu, 12 Feb 2026 17:32:04 -0800 Subject: [PATCH] fix: add input validation, date range checks, and security hardening - validate-agents.js: reject invalid model names in agent frontmatter - package-manager.js: validate script/binary names against shell injection - session-manager.js: reject impossible month/day values in filenames - utils.js: support options.all for replaceInFile string patterns - strategic-compact/SKILL.md: fix hook matcher syntax and script reference - install.sh: warn when overwriting existing rule customizations - Add 24 new tests covering all validation and edge cases --- install.sh | 6 ++ scripts/ci/validate-agents.js | 7 ++ scripts/lib/package-manager.js | 20 +++++ scripts/lib/session-manager.js | 5 ++ scripts/lib/utils.js | 17 +++- skills/strategic-compact/SKILL.md | 19 +++-- tests/integration/hooks.test.js | 58 ++++++++++++++ tests/lib/package-manager.test.js | 125 ++++++++++++++++++++++++++++++ tests/lib/session-manager.test.js | 40 ++++++++++ tests/lib/utils.test.js | 25 ++++++ 10 files changed, 312 insertions(+), 10 deletions(-) diff --git a/install.sh b/install.sh index 6c868b20..0b468f7d 100755 --- a/install.sh +++ b/install.sh @@ -70,6 +70,12 @@ fi if [[ "$TARGET" == "claude" ]]; then DEST_DIR="${CLAUDE_RULES_DIR:-$HOME/.claude/rules}" + # Warn if destination already exists (user may have local customizations) + if [[ -d "$DEST_DIR" ]] && [[ "$(ls -A "$DEST_DIR" 2>/dev/null)" ]]; then + echo "Note: $DEST_DIR/ already exists. Existing files will be overwritten." + echo " Back up any local customizations before proceeding." + fi + # Always install common rules echo "Installing common rules -> $DEST_DIR/common/" mkdir -p "$DEST_DIR/common" diff --git a/scripts/ci/validate-agents.js b/scripts/ci/validate-agents.js index 29a82b3c..d6db58c4 100644 --- a/scripts/ci/validate-agents.js +++ b/scripts/ci/validate-agents.js @@ -8,6 +8,7 @@ const path = require('path'); const AGENTS_DIR = path.join(__dirname, '../../agents'); const REQUIRED_FIELDS = ['model', 'tools']; +const VALID_MODELS = ['haiku', 'sonnet', 'opus']; function extractFrontmatter(content) { // Strip BOM if present (UTF-8 BOM: \uFEFF) @@ -62,6 +63,12 @@ function validateAgents() { hasErrors = true; } } + + // Validate model is a known value + if (frontmatter.model && !VALID_MODELS.includes(frontmatter.model)) { + console.error(`ERROR: ${file} - Invalid model '${frontmatter.model}'. Must be one of: ${VALID_MODELS.join(', ')}`); + hasErrors = true; + } } if (hasErrors) { diff --git a/scripts/lib/package-manager.js b/scripts/lib/package-manager.js index f767282d..c2580d60 100644 --- a/scripts/lib/package-manager.js +++ b/scripts/lib/package-manager.js @@ -280,12 +280,24 @@ function setProjectPackageManager(pmName, projectDir = process.cwd()) { return config; } +// Allowed characters in script/binary names: alphanumeric, dash, underscore, dot, slash, @ +// This prevents shell metacharacter injection while allowing scoped packages (e.g., @scope/pkg) +const SAFE_NAME_REGEX = /^[@a-zA-Z0-9_.\/-]+$/; + /** * Get the command to run a script * @param {string} script - Script name (e.g., "dev", "build", "test") * @param {object} options - { projectDir } + * @throws {Error} If script name contains unsafe characters */ function getRunCommand(script, options = {}) { + if (!script || typeof script !== 'string') { + throw new Error('Script name must be a non-empty string'); + } + if (!SAFE_NAME_REGEX.test(script)) { + throw new Error(`Script name contains unsafe characters: ${script}`); + } + const pm = getPackageManager(options); switch (script) { @@ -306,8 +318,16 @@ function getRunCommand(script, options = {}) { * Get the command to execute a package binary * @param {string} binary - Binary name (e.g., "prettier", "eslint") * @param {string} args - Arguments to pass + * @throws {Error} If binary name contains unsafe characters */ function getExecCommand(binary, args = '', options = {}) { + if (!binary || typeof binary !== 'string') { + throw new Error('Binary name must be a non-empty string'); + } + if (!SAFE_NAME_REGEX.test(binary)) { + throw new Error(`Binary name contains unsafe characters: ${binary}`); + } + const pm = getPackageManager(options); return `${pm.config.execCmd} ${binary}${args ? ' ' + args : ''}`; } diff --git a/scripts/lib/session-manager.js b/scripts/lib/session-manager.js index c3331b50..89e68dc7 100644 --- a/scripts/lib/session-manager.js +++ b/scripts/lib/session-manager.js @@ -31,6 +31,11 @@ function parseSessionFilename(filename) { if (!match) return null; const dateStr = match[1]; + + // Validate date components are in valid ranges (not just format) + const [year, month, day] = dateStr.split('-').map(Number); + if (month < 1 || month > 12 || day < 1 || day > 31) return null; + // match[2] is undefined for old format (no ID) const shortId = match[2] || 'no-id'; diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index a0b548c9..ec145213 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -388,13 +388,26 @@ function getGitModifiedFiles(patterns = []) { /** * Replace text in a file (cross-platform sed alternative) + * @param {string} filePath - Path to the file + * @param {string|RegExp} search - Pattern to search for. String patterns replace + * the FIRST occurrence only; use a RegExp with the `g` flag for global replacement. + * @param {string} replace - Replacement string + * @param {object} options - Options + * @param {boolean} options.all - When true and search is a string, replaces ALL + * occurrences (uses String.replaceAll). Ignored for RegExp patterns. + * @returns {boolean} true if file was written, false on error */ -function replaceInFile(filePath, search, replace) { +function replaceInFile(filePath, search, replace, options = {}) { const content = readFile(filePath); if (content === null) return false; try { - const newContent = content.replace(search, replace); + let newContent; + if (options.all && typeof search === 'string') { + newContent = content.replaceAll(search, replace); + } else { + newContent = content.replace(search, replace); + } writeFile(filePath, newContent); return true; } catch (err) { diff --git a/skills/strategic-compact/SKILL.md b/skills/strategic-compact/SKILL.md index 562119bb..2e37f40a 100644 --- a/skills/strategic-compact/SKILL.md +++ b/skills/strategic-compact/SKILL.md @@ -29,7 +29,7 @@ Strategic compaction at logical boundaries: ## How It Works -The `suggest-compact.sh` script runs on PreToolUse (Edit/Write) and: +The `suggest-compact.js` script runs on PreToolUse (Edit/Write) and: 1. **Tracks tool calls** — Counts tool invocations in session 2. **Threshold detection** — Suggests at configurable threshold (default: 50 calls) @@ -42,13 +42,16 @@ Add to your `~/.claude/settings.json`: ```json { "hooks": { - "PreToolUse": [{ - "matcher": "tool == \"Edit\" || tool == \"Write\"", - "hooks": [{ - "type": "command", - "command": "~/.claude/skills/strategic-compact/suggest-compact.sh" - }] - }] + "PreToolUse": [ + { + "matcher": "Edit", + "hooks": [{ "type": "command", "command": "node ~/.claude/skills/strategic-compact/suggest-compact.js" }] + }, + { + "matcher": "Write", + "hooks": [{ "type": "command", "command": "node ~/.claude/skills/strategic-compact/suggest-compact.js" }] + } + ] } } ``` diff --git a/tests/integration/hooks.test.js b/tests/integration/hooks.test.js index eb6083e6..18610e95 100644 --- a/tests/integration/hooks.test.js +++ b/tests/integration/hooks.test.js @@ -445,6 +445,64 @@ async function runTests() { assert.ok(elapsed < 5000, `Should complete in <5s, took ${elapsed}ms`); })) passed++; else failed++; + if (await asyncTest('hooks survive stdin exceeding 1MB limit', async () => { + // The post-edit-console-warn hook reads stdin up to 1MB then passes through + // Send > 1MB to verify truncation doesn't crash the hook + const oversizedInput = JSON.stringify({ + tool_input: { file_path: '/test.js' }, + tool_output: { output: 'x'.repeat(1200000) } // ~1.2MB + }); + + const proc = spawn('node', [path.join(scriptsDir, 'post-edit-console-warn.js')], { + stdio: ['pipe', 'pipe', 'pipe'] + }); + + let code = null; + // MUST drain stdout/stderr to prevent backpressure blocking the child process + proc.stdout.on('data', () => {}); + proc.stderr.on('data', () => {}); + proc.stdin.on('error', (err) => { + if (err.code !== 'EPIPE' && err.code !== 'EOF') throw err; + }); + proc.stdin.write(oversizedInput); + proc.stdin.end(); + + await new Promise(resolve => { + proc.on('close', (c) => { code = c; resolve(); }); + }); + + assert.strictEqual(code, 0, 'Should exit 0 despite oversized input'); + })) passed++; else failed++; + + if (await asyncTest('hooks handle truncated JSON from overflow gracefully', async () => { + // session-end parses stdin JSON. If input is > 1MB and truncated mid-JSON, + // JSON.parse should fail and fall back to env var + const proc = spawn('node', [path.join(scriptsDir, 'session-end.js')], { + stdio: ['pipe', 'pipe', 'pipe'] + }); + + let code = null; + let stderr = ''; + // MUST drain stdout to prevent backpressure blocking the child process + proc.stdout.on('data', () => {}); + proc.stderr.on('data', data => stderr += data); + proc.stdin.on('error', (err) => { + if (err.code !== 'EPIPE' && err.code !== 'EOF') throw err; + }); + + // Build a string that will be truncated mid-JSON at 1MB + const bigValue = 'x'.repeat(1200000); + proc.stdin.write(`{"transcript_path":"/tmp/none","padding":"${bigValue}"}`); + proc.stdin.end(); + + await new Promise(resolve => { + proc.on('close', (c) => { code = c; resolve(); }); + }); + + // Should exit 0 even if JSON parse fails (falls back to env var or null) + assert.strictEqual(code, 0, 'Should not crash on truncated JSON'); + })) passed++; else failed++; + // Summary console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); diff --git a/tests/lib/package-manager.test.js b/tests/lib/package-manager.test.js index 57a1ef71..8e9a0e35 100644 --- a/tests/lib/package-manager.test.js +++ b/tests/lib/package-manager.test.js @@ -498,6 +498,131 @@ function runTests() { assert.ok(regex.test('bun run lint'), 'Should match bun run lint'); })) passed++; else failed++; + // getPackageManager robustness tests + console.log('\ngetPackageManager (robustness):'); + + if (test('falls through on corrupted project config JSON', () => { + const testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pm-robust-')); + const claudeDir = path.join(testDir, '.claude'); + fs.mkdirSync(claudeDir, { recursive: true }); + fs.writeFileSync(path.join(claudeDir, 'package-manager.json'), '{not valid json!!!'); + + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + delete process.env.CLAUDE_PACKAGE_MANAGER; + const result = pm.getPackageManager({ projectDir: testDir }); + // Should fall through to default (npm) since project config is corrupt + assert.ok(result.name, 'Should return a package manager'); + assert.ok(result.source !== 'project-config', 'Should not use corrupt project config'); + } finally { + if (originalEnv !== undefined) { + process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + } + fs.rmSync(testDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + + if (test('falls through on project config with unknown PM', () => { + const testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pm-robust-')); + const claudeDir = path.join(testDir, '.claude'); + fs.mkdirSync(claudeDir, { recursive: true }); + fs.writeFileSync(path.join(claudeDir, 'package-manager.json'), + JSON.stringify({ packageManager: 'nonexistent-pm' })); + + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + delete process.env.CLAUDE_PACKAGE_MANAGER; + const result = pm.getPackageManager({ projectDir: testDir }); + assert.ok(result.name, 'Should return a package manager'); + assert.ok(result.source !== 'project-config', 'Should not use unknown PM config'); + } finally { + if (originalEnv !== undefined) { + process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + } + fs.rmSync(testDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + + // getRunCommand validation tests + console.log('\ngetRunCommand (validation):'); + + if (test('rejects empty script name', () => { + assert.throws(() => pm.getRunCommand(''), /non-empty string/); + })) passed++; else failed++; + + if (test('rejects null script name', () => { + assert.throws(() => pm.getRunCommand(null), /non-empty string/); + })) passed++; else failed++; + + if (test('rejects script name with shell metacharacters', () => { + assert.throws(() => pm.getRunCommand('test; rm -rf /'), /unsafe characters/); + })) passed++; else failed++; + + if (test('rejects script name with backticks', () => { + assert.throws(() => pm.getRunCommand('test`whoami`'), /unsafe characters/); + })) passed++; else failed++; + + if (test('accepts scoped package names', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'npm'; + const cmd = pm.getRunCommand('@scope/my-script'); + assert.strictEqual(cmd, 'npm run @scope/my-script'); + } finally { + if (originalEnv !== undefined) { + process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + } else { + delete process.env.CLAUDE_PACKAGE_MANAGER; + } + } + })) passed++; else failed++; + + // getExecCommand validation tests + console.log('\ngetExecCommand (validation):'); + + if (test('rejects empty binary name', () => { + assert.throws(() => pm.getExecCommand(''), /non-empty string/); + })) passed++; else failed++; + + if (test('rejects null binary name', () => { + assert.throws(() => pm.getExecCommand(null), /non-empty string/); + })) passed++; else failed++; + + if (test('rejects binary name with shell metacharacters', () => { + assert.throws(() => pm.getExecCommand('prettier; cat /etc/passwd'), /unsafe characters/); + })) passed++; else failed++; + + if (test('accepts dotted binary names like tsc', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'npm'; + const cmd = pm.getExecCommand('tsc'); + assert.strictEqual(cmd, 'npx tsc'); + } finally { + if (originalEnv !== undefined) { + process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + } else { + delete process.env.CLAUDE_PACKAGE_MANAGER; + } + } + })) passed++; else failed++; + + if (test('ignores unknown env var package manager', () => { + const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; + try { + process.env.CLAUDE_PACKAGE_MANAGER = 'totally-fake-pm'; + const result = pm.getPackageManager(); + // Should ignore invalid env var and fall through + assert.notStrictEqual(result.name, 'totally-fake-pm', 'Should not use unknown PM'); + } finally { + if (originalEnv !== undefined) { + process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; + } else { + delete process.env.CLAUDE_PACKAGE_MANAGER; + } + } + })) passed++; else failed++; + // Summary console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); diff --git a/tests/lib/session-manager.test.js b/tests/lib/session-manager.test.js index 6a6a8096..831c074e 100644 --- a/tests/lib/session-manager.test.js +++ b/tests/lib/session-manager.test.js @@ -465,6 +465,20 @@ src/main.ts assert.ok(result, 'Should find old-format session by filename'); })) passed++; else failed++; + if (test('getSessionById returns null for empty string', () => { + const result = sessionManager.getSessionById(''); + assert.strictEqual(result, null, 'Empty string should not match any session'); + })) 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'); + assert.ok(result.metadata, 'Should have metadata'); + assert.ok(result.stats, 'Should have stats'); + assert.strictEqual(typeof result.stats.totalItems, 'number', 'stats.totalItems should be number'); + assert.strictEqual(typeof result.stats.lineCount, 'number', 'stats.lineCount should be number'); + })) passed++; else failed++; + // parseSessionMetadata edge cases console.log('\nparseSessionMetadata (edge cases):'); @@ -574,6 +588,32 @@ src/main.ts assert.strictEqual(result, null, 'Extra segments should be rejected'); })) passed++; else failed++; + if (test('rejects impossible month (13)', () => { + const result = sessionManager.parseSessionFilename('2026-13-01-abcd1234-session.tmp'); + assert.strictEqual(result, null, 'Month 13 should be rejected'); + })) passed++; else failed++; + + if (test('rejects impossible day (32)', () => { + const result = sessionManager.parseSessionFilename('2026-01-32-abcd1234-session.tmp'); + assert.strictEqual(result, null, 'Day 32 should be rejected'); + })) passed++; else failed++; + + if (test('rejects month 00', () => { + const result = sessionManager.parseSessionFilename('2026-00-15-abcd1234-session.tmp'); + assert.strictEqual(result, null, 'Month 00 should be rejected'); + })) passed++; else failed++; + + if (test('rejects day 00', () => { + const result = sessionManager.parseSessionFilename('2026-01-00-abcd1234-session.tmp'); + assert.strictEqual(result, null, 'Day 00 should be rejected'); + })) passed++; else failed++; + + if (test('accepts valid edge date (month 12, day 31)', () => { + const result = sessionManager.parseSessionFilename('2026-12-31-abcd1234-session.tmp'); + assert.ok(result, 'Month 12, day 31 should be accepted'); + assert.strictEqual(result.date, '2026-12-31'); + })) passed++; else failed++; + if (test('datetime field is a Date object', () => { const result = sessionManager.parseSessionFilename('2026-06-15-abcdef12-session.tmp'); assert.ok(result); diff --git a/tests/lib/utils.test.js b/tests/lib/utils.test.js index 9167c032..5f636c20 100644 --- a/tests/lib/utils.test.js +++ b/tests/lib/utils.test.js @@ -546,6 +546,31 @@ function runTests() { } })) passed++; else failed++; + if (test('replaces all occurrences with string when options.all is true', () => { + const testFile = path.join(utils.getTempDir(), `utils-test-${Date.now()}.txt`); + try { + utils.writeFile(testFile, 'hello world hello again hello'); + utils.replaceInFile(testFile, 'hello', 'goodbye', { all: true }); + const content = utils.readFile(testFile); + assert.strictEqual(content, 'goodbye world goodbye again goodbye'); + } finally { + fs.unlinkSync(testFile); + } + })) passed++; else failed++; + + if (test('options.all is ignored for regex patterns', () => { + const testFile = path.join(utils.getTempDir(), `utils-test-${Date.now()}.txt`); + try { + utils.writeFile(testFile, 'foo bar foo'); + // all option should be ignored for regex; only g flag matters + utils.replaceInFile(testFile, /foo/, 'qux', { all: true }); + const content = utils.readFile(testFile); + assert.strictEqual(content, 'qux bar foo', 'Regex without g should still replace first only'); + } finally { + fs.unlinkSync(testFile); + } + })) passed++; else failed++; + if (test('replaces with capture groups', () => { const testFile = path.join(utils.getTempDir(), `utils-test-${Date.now()}.txt`); try {