feat: port block-no-verify hook + workflow-security CI validator#47
feat: port block-no-verify hook + workflow-security CI validator#47
Conversation
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
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security enhancements, including a git hook to prevent bypassing pre-commit checks and a CI validator for GitHub Actions workflows. Feedback identifies a style guide violation where the detectGitCommand function exceeds the 50-line limit. Furthermore, the reviewer suggests improving several regex patterns to handle command separators like &, reduce false positives in commit messages, and address potential bypasses in the workflow security script related to bracket notation and quoted strings.
| 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; | ||
| } |
There was a problem hiding this comment.
The detectGitCommand function is approximately 68 lines long, which exceeds the 50-line limit specified in the repository style guide. Consider refactoring the inner subcommand validation logic into a separate helper function to improve readability and maintainability.
References
- Functions should be under 50 lines. (link)
There was a problem hiding this comment.
Keeping the function as-is. Two reasons:
-
Vendored port.
block-no-verify.jsis a 1:1 port fromeverything-claude-code(upstream commit4e66b28). Refactoring just to satisfy the soft 50-line target creates merge friction on every future sync without changing behavior. -
Splits poorly.
detectGitCommandis a state machine with two nested loops (outer: walkgittokens; inner: scan candidate subcommands). The inner loop's gap-tokenization+flag-arg-tracking on lines 114–132 is tightly coupled to its enclosing scope (bestCmd,bestIdx,searchPos). Extracting it into a helper would require passing 4–5 parameters and a partial result back, which hurts readability more than the line count helps.
The styleguide entry says functions "should be" under 50 lines — a guideline, not a hard rule. I'd rather keep this one over-target and easy to compare against upstream than refactor it into something diffs poorly.
| 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; |
There was a problem hiding this comment.
The regex /[;|]/ used to detect command separators between git and its subcommand should also include the & character (e.g., && or backgrounding). Additionally, the check for the character following the subcommand on line 108 should also account for &.
| if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) break; | |
| if (/[;| &]/.test(input.slice(git.idx + git.len, cmdIdx))) break; |
|
|
||
| // 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; |
There was a problem hiding this comment.
This regex for the -n shorthand is prone to false positives as it will match the string -n anywhere in the command region after the subcommand, including inside commit messages (e.g., git commit -m "Fixed -n bug"). While difficult to solve perfectly with regex, consider using a more restrictive pattern that ensures it is treated as a flag.
| * Check if the input contains a -c core.hooksPath= override. | ||
| */ | ||
| function hasHooksPathOverride(input) { | ||
| return /-c\s+["']?core\.hooksPath\s*=/.test(input); |
There was a problem hiding this comment.
Similar to the -n check, this regex can trigger false positives if core.hooksPath= appears within a commit message or other argument. Since -c is a global git flag, it usually appears before the subcommand. You might consider checking for it only in the 'gap' between git and the subcommand identified in detectGitCommand.
| 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, |
There was a problem hiding this comment.
The regex for detecting unsafe expressions is not exhaustive. It misses bracket notation (e.g., github['event']...) and array wildcards (e.g., pull_requests[*]). While it covers the most common patterns, an attacker could potentially bypass this check using alternative expression syntax.
There was a problem hiding this comment.
Acknowledging this as a known limitation, deferring to a follow-up.
The current regex covers the dot-notation patterns that show up in real-world unsafe workflows. Bracket notation (github['event']['workflow_run']['head_branch']) and array wildcards (pull_requests[*]) are theoretical bypasses but I haven't seen them used in the wild for this attack class — most write-ups (Salus, Snyk, GitHub's own guidance) target the dot-notation form.
Doing this properly needs a small expression-language parser rather than another regex layer; bolting more alternations onto the existing pattern would just shift the bypass surface around. I'd rather track this as a follow-up issue than ship a half-fix that gives a false sense of completeness.
Comment 6 (the quoted uses: filter bypass) is a different class — that one I fixed in ce2ca96 because it broke the coverage of the validator, not just one expression form.
| startLine: block.startLine, | ||
| text: block.lines.join('\n'), | ||
| })) | ||
| .filter(block => /uses:\s*actions\/checkout@/m.test(block.text)); |
There was a problem hiding this comment.
The regex for identifying checkout steps does not account for quoted action names (e.g., uses: "actions/checkout@v4"). This could be used to bypass the security validation.
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.
Jamkris
added
🛡 Security
1 participant
Summary
Adopts two security additions from
everything-claude-code(upstream as of4e66b28) that are protocol-portable to Gemini CLI without rewriting:scripts/hooks/block-no-verify.js— registered as a BeforeTool hook onrun_shell_command. Blocks--no-verify(and-nshorthand oncommit) plus-c core.hooksPath=overrides forgit commit / push / merge / cherry-pick / rebase / am. Detects subcommand even in chained shells (git log -n 5 && git commit ...) without false-positiving on-nthat belongs togit log. Returns exit 2 with stderr explanation when blocking; exit 0 otherwise.scripts/ci/validate-workflow-security.js— rejects unsafegithub.event.{workflow_run,pull_request}.head.*refs insideactions/checkoutsteps when triggered from privileged events (workflow_run/pull_request_target). Wired intoreusable-validate.ymlas a 5th validation step.The
tool_input.commandJSON shape that block-no-verify reads is the same shape Gemini CLI's BeforeTool already produces, so no protocol translation was needed for the hook code itself.Hook registration (per session direction: same blast radius as the Claude side)
{ "matcher": "tool == \"run_shell_command\"", "hooks": [ { "type": "command", "command": "node \"$HOME/.gemini/extensions/everything-gemini-code/scripts/hooks/block-no-verify.js\"" } ] }Always-on: matches every shell command, with the hook returning exit 0 fast for non-git commands.
Skipped from upstream (with reasons)
gateguard-fact-force.js+skills/gateguard— hardcoded Claude tool names (Edit/Write/MultiEdit/Bash), ClaudepermissionDecisionJSON output protocol,CLAUDE_*env vars. Porting would be 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; Geminihooks.jsonalready supports multiple entries per event natively.Test plan
node scripts/hooks/block-no-verify.js— direct stdin invocation works (12 cases pass)node scripts/ci/validate-workflow-security.js— passes against this repo's 5 workflow files (and against the 4 fixture cases in the test)node scripts/ci/validate-hooks.js— 16 hook matchers (was 15)npm run lint— cleannpm test— 181/181 pass (was 165: +12 hook tests, +4 CI validator tests)git commit --no-verifyattempt is blocked once the extension is reinstalled