From ecfed58f552d9ef3d03bd52c2f25cad8f26c515c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 26 Mar 2026 19:13:07 -0600 Subject: [PATCH] fix: extend security sanitization + add 10 tests for merged community PRs - Extend json_safe() to ERROR_CLASS and FAILED_STEP fields - Improve ERROR_MESSAGE escaping to handle backslashes and newlines - Replace python3 with bun for JSON validation in gstack-review-log - Add 7 telemetry injection prevention tests - Add 2 review-log JSON validation tests - Add 1 discover-skills hidden directory filtering test Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/gstack-review-log | 2 +- bin/gstack-telemetry-log | 8 ++-- test/gen-skill-docs.test.ts | 24 ++++++++++++ test/review-log.test.ts | 77 +++++++++++++++++++++++++++++++++++++ test/telemetry.test.ts | 76 ++++++++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 test/review-log.test.ts diff --git a/bin/gstack-review-log b/bin/gstack-review-log index 0ebc162c..62c9e171 100755 --- a/bin/gstack-review-log +++ b/bin/gstack-review-log @@ -9,7 +9,7 @@ mkdir -p "$GSTACK_HOME/projects/$SLUG" # Validate: input must be parseable JSON (reject malformed or injection attempts) INPUT="$1" -if ! printf '%s' "$INPUT" | python3 -c "import json,sys; json.load(sys.stdin)" 2>/dev/null; then +if ! printf '%s' "$INPUT" | bun -e "JSON.parse(await Bun.stdin.text())" 2>/dev/null; then # Not valid JSON — refuse to append echo "gstack-review-log: invalid JSON, skipping" >&2 exit 1 diff --git a/bin/gstack-telemetry-log b/bin/gstack-telemetry-log index 2573f29d..da371c38 100755 --- a/bin/gstack-telemetry-log +++ b/bin/gstack-telemetry-log @@ -159,15 +159,15 @@ SESSION_ID="$(json_safe "$SESSION_ID")" SOURCE="$(json_safe "$SOURCE")" EVENT_TYPE="$(json_safe "$EVENT_TYPE")" -# Escape null fields +# Escape null fields — sanitize ERROR_CLASS and FAILED_STEP via json_safe() ERR_FIELD="null" -[ -n "$ERROR_CLASS" ] && ERR_FIELD="\"$ERROR_CLASS\"" +[ -n "$ERROR_CLASS" ] && ERR_FIELD="\"$(json_safe "$ERROR_CLASS")\"" ERR_MSG_FIELD="null" -[ -n "$ERROR_MESSAGE" ] && ERR_MSG_FIELD="\"$(echo "$ERROR_MESSAGE" | head -c 200 | sed 's/"/\\"/g')\"" +[ -n "$ERROR_MESSAGE" ] && ERR_MSG_FIELD="\"$(printf '%s' "$ERROR_MESSAGE" | head -c 200 | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' -e 's/ /\\t/g' | tr '\n\r' ' ')\"" STEP_FIELD="null" -[ -n "$FAILED_STEP" ] && STEP_FIELD="\"$(echo "$FAILED_STEP" | head -c 30)\"" +[ -n "$FAILED_STEP" ] && STEP_FIELD="\"$(json_safe "$FAILED_STEP")\"" # Cap unreasonable durations if [ -n "$DURATION" ] && [ "$DURATION" -gt 86400 ] 2>/dev/null; then diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index cab12413..274c558f 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -3,6 +3,7 @@ import { COMMAND_DESCRIPTIONS } from '../browse/src/commands'; import { SNAPSHOT_FLAGS } from '../browse/src/snapshot'; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; const ROOT = path.resolve(import.meta.dir, '..'); const MAX_SKILL_DESCRIPTION_LENGTH = 1024; @@ -1599,6 +1600,29 @@ describe('setup script validation', () => { }); }); +describe('discover-skills hidden directory filtering', () => { + test('discoverTemplates skips dot-prefixed directories', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-discover-')); + try { + // Create a hidden dir with a template (should be excluded) + fs.mkdirSync(path.join(tmpDir, '.hidden'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.hidden', 'SKILL.md.tmpl'), '---\nname: evil\n---\ntest'); + // Create a visible dir with a template (should be included) + fs.mkdirSync(path.join(tmpDir, 'visible'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, 'visible', 'SKILL.md.tmpl'), '---\nname: good\n---\ntest'); + + const { discoverTemplates } = require('../scripts/discover-skills'); + const results = discoverTemplates(tmpDir); + const dirs = results.map((r: { tmpl: string }) => r.tmpl); + + expect(dirs).toContain('visible/SKILL.md.tmpl'); + expect(dirs).not.toContain('.hidden/SKILL.md.tmpl'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); +}); + describe('telemetry', () => { test('generated SKILL.md contains telemetry start block', () => { const content = fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8'); diff --git a/test/review-log.test.ts b/test/review-log.test.ts new file mode 100644 index 00000000..f418fa29 --- /dev/null +++ b/test/review-log.test.ts @@ -0,0 +1,77 @@ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { execSync, ExecSyncOptionsWithStringEncoding } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const BIN = path.join(ROOT, 'bin'); + +let tmpDir: string; +let slugDir: string; + +function run(input: string, opts: { expectFail?: boolean } = {}): { stdout: string; exitCode: number } { + const execOpts: ExecSyncOptionsWithStringEncoding = { + cwd: ROOT, + env: { ...process.env, GSTACK_HOME: tmpDir }, + encoding: 'utf-8', + timeout: 10000, + }; + try { + const stdout = execSync(`${BIN}/gstack-review-log '${input.replace(/'/g, "'\\''")}'`, execOpts).trim(); + return { stdout, exitCode: 0 }; + } catch (e: any) { + if (opts.expectFail) { + return { stdout: e.stderr?.toString() || '', exitCode: e.status || 1 }; + } + throw e; + } +} + +beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-revlog-')); + // gstack-review-log uses gstack-slug which needs a git repo — create the projects dir + // with a predictable slug by pre-creating the directory structure + slugDir = path.join(tmpDir, 'projects'); + fs.mkdirSync(slugDir, { recursive: true }); +}); + +afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe('gstack-review-log', () => { + test('appends valid JSON to review JSONL file', () => { + const input = '{"skill":"plan-eng-review","status":"clean"}'; + const result = run(input); + expect(result.exitCode).toBe(0); + + // Find the JSONL file that was written + const projectDirs = fs.readdirSync(slugDir); + expect(projectDirs.length).toBeGreaterThan(0); + const projectDir = path.join(slugDir, projectDirs[0]); + const jsonlFiles = fs.readdirSync(projectDir).filter(f => f.endsWith('.jsonl')); + expect(jsonlFiles.length).toBeGreaterThan(0); + + const content = fs.readFileSync(path.join(projectDir, jsonlFiles[0]), 'utf-8').trim(); + const parsed = JSON.parse(content); + expect(parsed.skill).toBe('plan-eng-review'); + expect(parsed.status).toBe('clean'); + }); + + test('rejects non-JSON input with non-zero exit code', () => { + const result = run('not json at all', { expectFail: true }); + expect(result.exitCode).not.toBe(0); + + // Verify nothing was written + const projectDirs = fs.readdirSync(slugDir); + if (projectDirs.length > 0) { + const projectDir = path.join(slugDir, projectDirs[0]); + const jsonlFiles = fs.readdirSync(projectDir).filter(f => f.endsWith('.jsonl')); + if (jsonlFiles.length > 0) { + const content = fs.readFileSync(path.join(projectDir, jsonlFiles[0]), 'utf-8').trim(); + expect(content).toBe(''); + } + } + }); +}); diff --git a/test/telemetry.test.ts b/test/telemetry.test.ts index a3050631..40e6df88 100644 --- a/test/telemetry.test.ts +++ b/test/telemetry.test.ts @@ -125,6 +125,82 @@ describe('gstack-telemetry-log', () => { expect(events[0]).toHaveProperty('_branch'); }); + // ─── json_safe() injection prevention tests ──────────────── + test('sanitizes skill name with quote injection attempt', () => { + setConfig('telemetry', 'anonymous'); + run(`${BIN}/gstack-telemetry-log --skill 'review","injected":"true' --duration 10 --outcome success --session-id inj-1`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + // Must be valid JSON (no injection — quotes stripped, so no field injection possible) + const event = JSON.parse(lines[0]); + // The key check: no injected top-level property was created + expect(event).not.toHaveProperty('injected'); + // Skill field should have quotes stripped but content preserved + expect(event.skill).not.toContain('"'); + }); + + test('truncates skill name exceeding 200 chars', () => { + setConfig('telemetry', 'anonymous'); + const longSkill = 'a'.repeat(250); + run(`${BIN}/gstack-telemetry-log --skill '${longSkill}' --duration 10 --outcome success --session-id trunc-1`); + + const events = parseJsonl(); + expect(events[0].skill.length).toBeLessThanOrEqual(200); + }); + + test('sanitizes outcome with newline injection attempt', () => { + setConfig('telemetry', 'anonymous'); + // Use printf to pass actual newline in the argument + run(`bash -c 'OUTCOME=$(printf "success\\nfake\\":\\"true"); ${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome "$OUTCOME" --session-id inj-2'`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event).not.toHaveProperty('fake'); + }); + + test('sanitizes session_id with backslash-quote injection', () => { + setConfig('telemetry', 'anonymous'); + run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome success --session-id 'id\\\\"","x":"y'`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event).not.toHaveProperty('x'); + }); + + test('sanitizes error_class with quote injection', () => { + setConfig('telemetry', 'anonymous'); + run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-class 'timeout","extra":"val' --session-id inj-3`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event).not.toHaveProperty('extra'); + }); + + test('sanitizes failed_step with quote injection', () => { + setConfig('telemetry', 'anonymous'); + run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --failed-step 'step1","hacked":"yes' --session-id inj-4`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event).not.toHaveProperty('hacked'); + }); + + test('escapes error_message quotes and preserves content', () => { + setConfig('telemetry', 'anonymous'); + run(`${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'Error: file "test.txt" not found' --session-id inj-5`); + + const lines = readJsonl(); + expect(lines).toHaveLength(1); + const event = JSON.parse(lines[0]); + expect(event.error_message).toContain('file'); + expect(event.error_message).toContain('not found'); + }); + test('creates analytics directory if missing', () => { // Remove analytics dir const analyticsDir = path.join(tmpDir, 'analytics');