From b479f23af09b92a1afa086701e06eb5dc14f16cb Mon Sep 17 00:00:00 2001 From: Jamkris Date: Mon, 27 Apr 2026 12:39:03 +0900 Subject: [PATCH 1/2] feat: port block-no-verify hook and workflow-security CI validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts two security additions from everything-claude-code (upstream 4e66b28). Both are protocol-portable: block-no-verify accepts the { tool_input: { command } } JSON shape that Gemini CLI BeforeTool already produces, and validate-workflow-security is a standalone GitHub Actions guard that has no agent-protocol dependencies. scripts/hooks/block-no-verify.js - PreToolUse-style hook: blocks --no-verify (and -n shorthand on commit) plus -c core.hooksPath= overrides for git commit / push / merge / cherry-pick / rebase / am - Detects subcommand even in chained shells (git log -n 5 && git commit ...) without false-positiving on -n that belongs to git log - Returns exit 2 with stderr explanation when blocking; exit 0 in all other cases hooks/hooks.json - Registered as the first BeforeTool hook on tool == run_shell_command, matching upstream behavior of "always on" — same blast radius as the Claude side scripts/ci/validate-workflow-security.js - Rejects unsafe github.event.{workflow_run,pull_request}.head.* refs inside actions/checkout steps when triggered from privileged events (workflow_run / pull_request_target) - Wired into .github/workflows/reusable-validate.yml as a 5th validation step Tests - tests/hooks/block-no-verify.test.js (12 cases) — adapted from the upstream suite. Upstream uses run-with-flags.js indirection that this project does not have, so tests pipe JSON to the hook directly via spawnSync stdin - tests/ci/validate-workflow-security.test.js (4 cases) — copied as-is; upstream already supports ECC_WORKFLOWS_DIR for fixture isolation - tests/run-all.js — both registered Lint clean / 181 tests pass (was 165) / 16 hook matchers (was 15) / 5 workflow files clean. Skipped from upstream and why: - gateguard-fact-force.js + skills/gateguard: hardcoded Claude tool names and Claude permissionDecision JSON output protocol; would need a near-rewrite to fit Gemini's stderr+exit hook contract - bash-hook-dispatcher.js + pre/post-bash-dispatcher.js: dispatcher pattern works around Claude's single-hook-per-event limit; Gemini hooks.json already supports multiple entries per event natively --- .github/workflows/reusable-validate.yml | 3 + hooks/hooks.json | 10 + scripts/ci/validate-workflow-security.js | 138 ++++++++++ scripts/hooks/block-no-verify.js | 269 ++++++++++++++++++++ tests/ci/validate-workflow-security.test.js | 90 +++++++ tests/hooks/block-no-verify.test.js | 120 +++++++++ tests/run-all.js | 2 + 7 files changed, 632 insertions(+) create mode 100644 scripts/ci/validate-workflow-security.js create mode 100644 scripts/hooks/block-no-verify.js create mode 100644 tests/ci/validate-workflow-security.test.js create mode 100644 tests/hooks/block-no-verify.test.js diff --git a/.github/workflows/reusable-validate.yml b/.github/workflows/reusable-validate.yml index 1c0adfb..4f2a965 100644 --- a/.github/workflows/reusable-validate.yml +++ b/.github/workflows/reusable-validate.yml @@ -35,3 +35,6 @@ jobs: - name: Validate skills run: node scripts/ci/validate-skills.js + + - name: Validate workflow security + run: node scripts/ci/validate-workflow-security.js diff --git a/hooks/hooks.json b/hooks/hooks.json index f0d79af..b63174c 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -2,6 +2,16 @@ "$schema": "https://json.schemastore.org/gemini-code-settings.json", "hooks": { "BeforeTool": [ + { + "matcher": "tool == \"run_shell_command\"", + "hooks": [ + { + "type": "command", + "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/block-no-verify.js\"" + } + ], + "description": "Block git --no-verify and core.hooksPath= overrides on commit/push/merge/cherry-pick/rebase/am — protects pre-commit, commit-msg, and pre-push hooks from being skipped" + }, { "matcher": "tool == \"run_shell_command\" && tool_input.command matches \"(npm run dev|pnpm( run)? dev|yarn dev|bun run dev)\"", "hooks": [ diff --git a/scripts/ci/validate-workflow-security.js b/scripts/ci/validate-workflow-security.js new file mode 100644 index 0000000..7f6183b --- /dev/null +++ b/scripts/ci/validate-workflow-security.js @@ -0,0 +1,138 @@ +#!/usr/bin/env node +/** + * Reject unsafe GitHub Actions patterns that execute or checkout untrusted PR code + * from privileged events such as workflow_run or pull_request_target. + */ + +const fs = require('fs'); +const path = require('path'); + +const DEFAULT_WORKFLOWS_DIR = path.join(__dirname, '../../.github/workflows'); + +const RULES = [ + { + event: 'workflow_run', + eventPattern: /\bworkflow_run\s*:/m, + description: 'workflow_run must not checkout an untrusted workflow_run head ref/repository', + expressionPattern: /\$\{\{\s*github\.event\.workflow_run\.(?:head_branch|head_sha|head_repository(?:\.[A-Za-z0-9_.]+)?)\s*\}\}|\$\{\{\s*github\.event\.workflow_run\.pull_requests\[\d+\]\.head\.(?:ref|sha|repo\.full_name)\s*\}\}/g, + }, + { + event: 'pull_request_target', + eventPattern: /\bpull_request_target\s*:/m, + description: 'pull_request_target must not checkout an untrusted pull_request head ref/repository', + expressionPattern: /\$\{\{\s*github\.event\.pull_request\.head\.(?:ref|sha|repo\.full_name)\s*\}\}/g, + }, +]; + +function getWorkflowFiles(workflowsDir) { + if (!fs.existsSync(workflowsDir)) { + return []; + } + + return fs.readdirSync(workflowsDir) + .filter(file => /\.(?:yml|yaml)$/i.test(file)) + .map(file => path.join(workflowsDir, file)) + .sort(); +} + +function getLineNumber(source, index) { + return source.slice(0, index).split(/\r?\n/).length; +} + +function extractCheckoutSteps(source) { + const blocks = []; + const lines = source.split(/\r?\n/); + let current = null; + + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const stepStart = line.match(/^(\s*)-\s+/); + + if (stepStart) { + if (current) { + blocks.push(current); + } + + current = { + indent: stepStart[1].length, + startLine: i + 1, + lines: [line], + }; + continue; + } + + if (current) { + current.lines.push(line); + } + } + + if (current) { + blocks.push(current); + } + + return blocks + .map(block => ({ + startLine: block.startLine, + text: block.lines.join('\n'), + })) + .filter(block => /uses:\s*actions\/checkout@/m.test(block.text)); +} + +function findViolations(filePath, source) { + const violations = []; + const checkoutSteps = extractCheckoutSteps(source); + + for (const rule of RULES) { + if (!rule.eventPattern.test(source)) { + continue; + } + + for (const step of checkoutSteps) { + for (const match of step.text.matchAll(rule.expressionPattern)) { + violations.push({ + filePath, + event: rule.event, + description: rule.description, + expression: match[0], + line: step.startLine + getLineNumber(step.text, match.index) - 1, + }); + } + } + } + + return violations; +} + +function validateWorkflowSecurity(workflowsDir = DEFAULT_WORKFLOWS_DIR) { + const files = getWorkflowFiles(workflowsDir); + const violations = []; + + for (const filePath of files) { + const source = fs.readFileSync(filePath, 'utf8'); + violations.push(...findViolations(filePath, source)); + } + + if (violations.length > 0) { + for (const violation of violations) { + console.error( + `ERROR: ${path.basename(violation.filePath)}:${violation.line} - ${violation.description}`, + ); + console.error(` Unsafe expression: ${violation.expression}`); + } + return 1; + } + + console.log(`Validated workflow security for ${files.length} workflow files`); + return 0; +} + +if (require.main === module) { + process.exit(validateWorkflowSecurity(process.env.ECC_WORKFLOWS_DIR || DEFAULT_WORKFLOWS_DIR)); +} + +module.exports = { + DEFAULT_WORKFLOWS_DIR, + extractCheckoutSteps, + findViolations, + validateWorkflowSecurity, +}; diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js new file mode 100644 index 0000000..864b628 --- /dev/null +++ b/scripts/hooks/block-no-verify.js @@ -0,0 +1,269 @@ +#!/usr/bin/env node +/** + * PreToolUse Hook: Block --no-verify flag + * + * Blocks git hook-bypass flags (--no-verify, -c core.hooksPath=) to protect + * pre-commit, commit-msg, and pre-push hooks from being skipped by AI agents. + * + * Replaces the previous npx-based invocation that failed in pnpm-only projects + * (EBADDEVENGINES) and could not be disabled via ECC_DISABLED_HOOKS. + * + * Exit codes: + * 0 = allow (not a git command or no bypass flags) + * 2 = block (bypass flag detected) + */ + +'use strict'; + +const MAX_STDIN = 1024 * 1024; +let raw = ''; + +/** + * Git commands that support the --no-verify flag. + */ +const GIT_COMMANDS_WITH_NO_VERIFY = [ + 'commit', + 'push', + 'merge', + 'cherry-pick', + 'rebase', + 'am', +]; + +/** + * Characters that can appear immediately before 'git' in a command string. + */ +const VALID_BEFORE_GIT = ' \t\n\r;&|$`(<{!"\']/.~\\'; + +/** + * Check if a position in the input is inside a shell comment. + */ +function isInComment(input, idx) { + const lineStart = input.lastIndexOf('\n', idx - 1) + 1; + const before = input.slice(lineStart, idx); + for (let i = 0; i < before.length; i++) { + if (before.charAt(i) === '#') { + const prev = i > 0 ? before.charAt(i - 1) : ''; + if (prev !== '$' && prev !== '\\') return true; + } + } + return false; +} + +/** + * Find the next 'git' token in the input starting from a position. + */ +function findGit(input, start) { + let pos = start; + while (pos < input.length) { + const idx = input.indexOf('git', pos); + if (idx === -1) return null; + + const isExe = input.slice(idx + 3, idx + 7).toLowerCase() === '.exe'; + const len = isExe ? 7 : 3; + const after = input[idx + len] || ' '; + if (!/[\s"']/.test(after)) { + pos = idx + 1; + continue; + } + + const before = idx > 0 ? input[idx - 1] : ' '; + if (VALID_BEFORE_GIT.includes(before)) return { idx, len }; + pos = idx + 1; + } + return null; +} + +/** + * Detect which git subcommand (commit, push, etc.) is being invoked. + * Returns { command, offset } where offset is the position right after the + * subcommand keyword, so callers can scope flag checks to only that portion. + */ +function detectGitCommand(input) { + let start = 0; + while (start < input.length) { + const git = findGit(input, start); + if (!git) return null; + + if (isInComment(input, git.idx)) { + start = git.idx + git.len; + continue; + } + + // Find the first matching subcommand token after "git". + // We pick the one closest to "git" so that argument values like + // "git push origin commit" don't misclassify "commit" as the subcommand. + let bestCmd = null; + let bestIdx = Infinity; + + for (const cmd of GIT_COMMANDS_WITH_NO_VERIFY) { + let searchPos = git.idx + git.len; + while (searchPos < input.length) { + const cmdIdx = input.indexOf(cmd, searchPos); + if (cmdIdx === -1) break; + + const before = cmdIdx > 0 ? input[cmdIdx - 1] : ' '; + const after = input[cmdIdx + cmd.length] || ' '; + if (!/\s/.test(before)) { searchPos = cmdIdx + 1; continue; } + if (!/[\s;&#|>)\]}"']/.test(after) && after !== '') { searchPos = cmdIdx + 1; continue; } + if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) break; + if (isInComment(input, cmdIdx)) { searchPos = cmdIdx + 1; continue; } + + // Verify this token is the first non-flag word after "git" — i.e. the + // actual subcommand, not an argument value to a different subcommand. + const gap = input.slice(git.idx + git.len, cmdIdx); + const tokens = gap.trim().split(/\s+/).filter(Boolean); + // Every token before the candidate must be a flag or a flag argument. + // Git global flags like -c take a value argument (e.g. -c key=value). + let onlyFlagsAndArgs = true; + let expectFlagArg = false; + for (const t of tokens) { + if (expectFlagArg) { expectFlagArg = false; continue; } + if (t.startsWith('-')) { + // -c is a git global flag that takes the next token as its argument + if (t === '-c' || t === '-C' || t === '--work-tree' || t === '--git-dir' || + t === '--namespace' || t === '--super-prefix') { + expectFlagArg = true; + } + continue; + } + onlyFlagsAndArgs = false; + break; + } + if (!onlyFlagsAndArgs) { searchPos = cmdIdx + 1; continue; } + + if (cmdIdx < bestIdx) { + bestIdx = cmdIdx; + bestCmd = cmd; + } + break; + } + } + + if (bestCmd) { + return { command: bestCmd, offset: bestIdx + bestCmd.length }; + } + + start = git.idx + git.len; + } + return null; +} + +/** + * Check if the input contains a --no-verify flag for a specific git command. + * Only inspects the portion of the input starting at `offset` (the position + * right after the detected subcommand keyword) so that flags belonging to + * earlier commands in a chain are not falsely matched. + */ +function hasNoVerifyFlag(input, command, offset) { + const region = input.slice(offset); + if (/--no-verify\b/.test(region)) return true; + + // For commit, -n is shorthand for --no-verify + if (command === 'commit') { + if (/\s-n(?:\s|$)/.test(region) || /\s-n[a-zA-Z]/.test(region)) return true; + } + + return false; +} + +/** + * Check if the input contains a -c core.hooksPath= override. + */ +function hasHooksPathOverride(input) { + return /-c\s+["']?core\.hooksPath\s*=/.test(input); +} + +/** + * Check a command string for git hook bypass attempts. + */ +function checkCommand(input) { + const detected = detectGitCommand(input); + if (!detected) return { blocked: false }; + + const { command: gitCommand, offset } = detected; + + if (hasNoVerifyFlag(input, gitCommand, offset)) { + return { + blocked: true, + reason: `BLOCKED: --no-verify flag is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, + }; + } + + if (hasHooksPathOverride(input)) { + return { + blocked: true, + reason: `BLOCKED: Overriding core.hooksPath is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`, + }; + } + + return { blocked: false }; +} + +/** + * Extract the command string from hook input (JSON or plain text). + */ +function extractCommand(rawInput) { + const trimmed = rawInput.trim(); + if (!trimmed.startsWith('{')) return trimmed; + + try { + const parsed = JSON.parse(trimmed); + if (typeof parsed !== 'object' || parsed === null) return trimmed; + + // Claude Code format: { tool_input: { command: "..." } } + const cmd = parsed.tool_input?.command; + if (typeof cmd === 'string') return cmd; + + // Generic JSON formats + for (const key of ['command', 'cmd', 'input', 'shell', 'script']) { + if (typeof parsed[key] === 'string') return parsed[key]; + } + + return trimmed; + } catch { + return trimmed; + } +} + +/** + * Exportable run() for in-process execution via run-with-flags.js. + */ +function run(rawInput) { + const command = extractCommand(rawInput); + const result = checkCommand(command); + + if (result.blocked) { + return { + exitCode: 2, + stderr: result.reason, + }; + } + + return { exitCode: 0 }; +} + +module.exports = { run }; + +// Stdin fallback for spawnSync execution — only when invoked directly, not via require() +if (require.main === module) { + process.stdin.setEncoding('utf8'); + process.stdin.on('data', chunk => { + if (raw.length < MAX_STDIN) { + const remaining = MAX_STDIN - raw.length; + raw += chunk.substring(0, remaining); + } + }); + + process.stdin.on('end', () => { + const command = extractCommand(raw); + const result = checkCommand(command); + + if (result.blocked) { + process.stderr.write(result.reason + '\n'); + process.exit(2); + } + + process.stdout.write(raw); + }); +} diff --git a/tests/ci/validate-workflow-security.test.js b/tests/ci/validate-workflow-security.test.js new file mode 100644 index 0000000..b1e69a3 --- /dev/null +++ b/tests/ci/validate-workflow-security.test.js @@ -0,0 +1,90 @@ +#!/usr/bin/env node +/** + * Validate workflow security guardrails for privileged GitHub Actions events. + */ + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const SCRIPT_PATH = path.join(__dirname, '..', '..', 'scripts', 'ci', 'validate-workflow-security.js'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` Error: ${error.message}`); + return false; + } +} + +function runValidator(files) { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-workflow-security-')); + try { + for (const [name, contents] of Object.entries(files)) { + fs.writeFileSync(path.join(tempDir, name), contents); + } + + return spawnSync('node', [SCRIPT_PATH], { + encoding: 'utf8', + env: { + ...process.env, + ECC_WORKFLOWS_DIR: tempDir, + }, + }); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } +} + +function run() { + console.log('\n=== Testing workflow security validation ===\n'); + + let passed = 0; + let failed = 0; + + if (test('allows safe workflow_run workflow that only checks out the base repository', () => { + const result = runValidator({ + 'safe.yml': `name: Safe\non:\n workflow_run:\n workflows: ["CI"]\n types: [completed]\njobs:\n repair:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n - run: echo safe\n`, + }); + assert.strictEqual(result.status, 0, result.stderr || result.stdout); + })) passed++; else failed++; + + if (test('rejects workflow_run checkout using github.event.workflow_run.head_branch', () => { + const result = runValidator({ + 'unsafe-workflow-run.yml': `name: Unsafe\non:\n workflow_run:\n workflows: ["CI"]\n types: [completed]\njobs:\n repair:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n ref: \${{ github.event.workflow_run.head_branch }}\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail'); + assert.match(result.stderr, /workflow_run must not checkout an untrusted workflow_run head ref\/repository/); + assert.match(result.stderr, /head_branch/); + })) passed++; else failed++; + + if (test('rejects workflow_run checkout using github.event.workflow_run.head_repository.full_name', () => { + const result = runValidator({ + 'unsafe-repository.yml': `name: Unsafe\non:\n workflow_run:\n workflows: ["CI"]\n types: [completed]\njobs:\n repair:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n repository: \${{ github.event.workflow_run.head_repository.full_name }}\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail'); + assert.match(result.stderr, /head_repository\.full_name/); + })) passed++; else failed++; + + if (test('rejects pull_request_target checkout using github.event.pull_request.head.sha', () => { + const result = runValidator({ + 'unsafe-pr-target.yml': `name: Unsafe\non:\n pull_request_target:\n branches: [main]\njobs:\n inspect:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n ref: \${{ github.event.pull_request.head.sha }}\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail'); + assert.match(result.stderr, /pull_request_target must not checkout an untrusted pull_request head ref\/repository/); + assert.match(result.stderr, /pull_request\.head\.sha/); + })) passed++; else failed++; + + console.log(`\nPassed: ${passed}`); + console.log(`Failed: ${failed}`); + + process.exit(failed > 0 ? 1 : 0); +} + +run(); diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js new file mode 100644 index 0000000..8c9accd --- /dev/null +++ b/tests/hooks/block-no-verify.test.js @@ -0,0 +1,120 @@ +/** + * Tests for scripts/hooks/block-no-verify.js + * + * Invokes the hook script directly via stdin (Gemini CLI BeforeTool runs + * hooks as standalone Node processes). Upstream's test harness goes through + * a `run-with-flags.js` indirection that this project does not have. + */ + +'use strict'; + +const assert = require('assert'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const HOOK = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'block-no-verify.js'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` Error: ${error.message}`); + return false; + } +} + +function runHook(input) { + const rawInput = typeof input === 'string' ? input : JSON.stringify(input); + const result = spawnSync('node', [HOOK], { + input: rawInput, + encoding: 'utf8', + timeout: 15000, + stdio: ['pipe', 'pipe', 'pipe'] + }); + return { + code: Number.isInteger(result.status) ? result.status : 1, + stdout: result.stdout || '', + stderr: result.stderr || '' + }; +} + +let passed = 0; +let failed = 0; + +console.log('\nblock-no-verify hook tests'); +console.log('─'.repeat(50)); + +if (test('allows plain git commit', () => { + const r = runHook({ tool_input: { command: 'git commit -m "hello"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks --no-verify on git commit', () => { + const r = runHook({ tool_input: { command: 'git commit --no-verify -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('BLOCKED'), `stderr should contain BLOCKED: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks -n shorthand on git commit', () => { + const r = runHook({ tool_input: { command: 'git commit -n -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('BLOCKED'), `stderr should contain BLOCKED: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks core.hooksPath override', () => { + const r = runHook({ tool_input: { command: 'git -c core.hooksPath=/dev/null commit -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('core.hooksPath'), `stderr should mention core.hooksPath: ${r.stderr}`); +})) passed++; else failed++; + +// Chained-command false-positive prevention +if (test('does not false-positive on -n belonging to git log in a chain', () => { + const r = runHook({ tool_input: { command: 'git log -n 10 && git commit -m "msg"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('does not false-positive on --no-verify in a prior non-git command', () => { + const r = runHook({ tool_input: { command: 'echo --no-verify && git commit -m "msg"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('still blocks --no-verify on the git commit part of a chain', () => { + const r = runHook({ tool_input: { command: 'git log -n 5 && git commit --no-verify -m "msg"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + +// Subcommand detection +if (test('does not misclassify "commit" as subcommand when it is a refspec arg to push', () => { + const r = runHook({ tool_input: { command: 'git push origin commit' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks --no-verify on git push', () => { + const r = runHook({ tool_input: { command: 'git push --no-verify' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); + assert.ok(r.stderr.includes('git push'), `stderr should mention git push: ${r.stderr}`); +})) passed++; else failed++; + +// Pass-through +if (test('allows non-git commands', () => { + const r = runHook({ tool_input: { command: 'npm test' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('handles plain text input (no tool_input wrapper)', () => { + const r = runHook('git commit -m "hello"'); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('blocks plain text input with --no-verify', () => { + const r = runHook('git commit --no-verify -m "msg"'); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + +console.log('─'.repeat(50)); +console.log(`Passed: ${passed} Failed: ${failed}`); + +process.exit(failed > 0 ? 1 : 0); diff --git a/tests/run-all.js b/tests/run-all.js index 934e482..b2916ad 100644 --- a/tests/run-all.js +++ b/tests/run-all.js @@ -17,6 +17,8 @@ const testFiles = [ 'lib/session-aliases.test.js', 'lib/gemini-tools.test.js', 'hooks/hooks.test.js', + 'hooks/block-no-verify.test.js', + 'ci/validate-workflow-security.test.js', 'lint/validators.test.js' ]; From ce2ca963fb353bb4a28764e5622e6452390aba7d Mon Sep 17 00:00:00 2001 From: Jamkris Date: Mon, 27 Apr 2026 12:54:57 +0900 Subject: [PATCH 2/2] fix: address gemini-code-assist review on PR #47 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment 2 — block-no-verify: include & in the chained-command separator regex so detectGitCommand exits the inner subcommand search early on backgrounding/&&, matching the existing tokenizer behavior Comment 3 — block-no-verify: false positive on -n inside commit message Before: git commit -m "Fixed -n bug" → BLOCKED (incorrect) After: stripQuotedStrings() neutralizes single- and double-quoted args before flag-detection regexes run; the same fix covers --no-verify inside quoted messages Comment 4 — block-no-verify: same false-positive class for the core.hooksPath= regex, fixed by routing through stripQuotedStrings() Comment 6 — validate-workflow-security: extractCheckoutSteps regex required an unquoted `uses: actions/checkout@...`. A quoted form (`uses: "actions/checkout@v4"`) would have bypassed the security filter entirely. Regex now accepts an optional ' or " before the value. Tests - 5 new false-positive cases in tests/hooks/block-no-verify.test.js (12 → 17): --no-verify in double-quoted message, single-quoted message, -n in quoted message, core.hooksPath= in quoted message, plus a confirm-still-blocks case for a real flag combined with a decoy quoted string - 1 new fixture in tests/ci/validate-workflow-security.test.js (4 → 5): quoted uses: value with an unsafe ref Lint clean / 187 tests pass (was 181) / 5 workflow files clean. Comments 1 (detectGitCommand exceeds 50-line soft target) and 5 (bracket notation / pull_requests[*] bypass) are not addressed in this commit; rebuttals posted on the PR thread. --- scripts/ci/validate-workflow-security.js | 2 +- scripts/hooks/block-no-verify.js | 22 ++++++++++++++--- tests/ci/validate-workflow-security.test.js | 11 +++++++++ tests/hooks/block-no-verify.test.js | 27 +++++++++++++++++++++ 4 files changed, 58 insertions(+), 4 deletions(-) diff --git a/scripts/ci/validate-workflow-security.js b/scripts/ci/validate-workflow-security.js index 7f6183b..0393613 100644 --- a/scripts/ci/validate-workflow-security.js +++ b/scripts/ci/validate-workflow-security.js @@ -75,7 +75,7 @@ function extractCheckoutSteps(source) { startLine: block.startLine, text: block.lines.join('\n'), })) - .filter(block => /uses:\s*actions\/checkout@/m.test(block.text)); + .filter(block => /uses:\s*['"]?actions\/checkout@/m.test(block.text)); } function findViolations(filePath, source) { diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 864b628..4562630 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -106,7 +106,7 @@ function detectGitCommand(input) { const after = input[cmdIdx + cmd.length] || ' '; if (!/\s/.test(before)) { searchPos = cmdIdx + 1; continue; } if (!/[\s;&#|>)\]}"']/.test(after) && after !== '') { searchPos = cmdIdx + 1; continue; } - if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) break; + if (/[;|&]/.test(input.slice(git.idx + git.len, cmdIdx))) break; if (isInComment(input, cmdIdx)) { searchPos = cmdIdx + 1; continue; } // Verify this token is the first non-flag word after "git" — i.e. the @@ -149,6 +149,20 @@ function detectGitCommand(input) { return null; } +/** + * Replace the contents of single- and double-quoted strings with empty + * quotes. Used to neutralize commit messages and other quoted args before + * we run the flag-detection regexes — without this, a benign command like + * git commit -m "fix: --no-verify edge case" + * would falsely match `--no-verify` inside the message and block the + * commit. Backslash-escaped quotes inside the string are honored. + */ +function stripQuotedStrings(input) { + return input + .replace(/'(?:[^'\\]|\\.)*'/g, "''") + .replace(/"(?:[^"\\]|\\.)*"/g, '""'); +} + /** * Check if the input contains a --no-verify flag for a specific git command. * Only inspects the portion of the input starting at `offset` (the position @@ -156,7 +170,7 @@ function detectGitCommand(input) { * earlier commands in a chain are not falsely matched. */ function hasNoVerifyFlag(input, command, offset) { - const region = input.slice(offset); + const region = stripQuotedStrings(input.slice(offset)); if (/--no-verify\b/.test(region)) return true; // For commit, -n is shorthand for --no-verify @@ -169,9 +183,11 @@ function hasNoVerifyFlag(input, command, offset) { /** * Check if the input contains a -c core.hooksPath= override. + * Quoted strings are stripped first to avoid false positives on commit + * messages or argument values that happen to contain the phrase. */ function hasHooksPathOverride(input) { - return /-c\s+["']?core\.hooksPath\s*=/.test(input); + return /-c\s+["']?core\.hooksPath\s*=/.test(stripQuotedStrings(input)); } /** diff --git a/tests/ci/validate-workflow-security.test.js b/tests/ci/validate-workflow-security.test.js index b1e69a3..d50e6a2 100644 --- a/tests/ci/validate-workflow-security.test.js +++ b/tests/ci/validate-workflow-security.test.js @@ -81,6 +81,17 @@ function run() { assert.match(result.stderr, /pull_request\.head\.sha/); })) passed++; else failed++; + // Quoted action names must still be inspected (gemini-code-assist review + // on PR #47 — `uses: "actions/checkout@v4"` would have bypassed the + // checkout-step filter under the original regex). + if (test('detects unsafe checkout when uses: value is quoted', () => { + const result = runValidator({ + 'unsafe-quoted.yml': `name: Unsafe\non:\n pull_request_target:\n branches: [main]\njobs:\n inspect:\n runs-on: ubuntu-latest\n steps:\n - uses: "actions/checkout@v4"\n with:\n ref: \${{ github.event.pull_request.head.sha }}\n`, + }); + assert.notStrictEqual(result.status, 0, 'Expected validator to fail on quoted uses:'); + assert.match(result.stderr, /pull_request\.head\.sha/); + })) passed++; else failed++; + console.log(`\nPassed: ${passed}`); console.log(`Failed: ${failed}`); diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index 8c9accd..de3ec2b 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -114,6 +114,33 @@ if (test('blocks plain text input with --no-verify', () => { assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); })) passed++; else failed++; +// False-positive prevention: flag-like text inside quoted commit messages +// must not trigger a block (gemini-code-assist review on PR #47). +if (test('does not false-positive on --no-verify inside double-quoted message', () => { + const r = runHook({ tool_input: { command: 'git commit -m "fix: --no-verify edge case"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('does not false-positive on --no-verify inside single-quoted message', () => { + const r = runHook({ tool_input: { command: "git commit -m 'fix: --no-verify edge case'" } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('does not false-positive on -n inside quoted commit message', () => { + const r = runHook({ tool_input: { command: 'git commit -m "Fixed -n bug in module"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('does not false-positive on core.hooksPath= inside quoted message', () => { + const r = runHook({ tool_input: { command: 'git commit -m "doc: explain core.hooksPath= setting"' } }); + assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); +})) passed++; else failed++; + +if (test('still blocks --no-verify when both quoted message and real flag are present', () => { + const r = runHook({ tool_input: { command: 'git commit --no-verify -m "fix: -n edge case"' } }); + assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); +})) passed++; else failed++; + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`);