fix: address PR 162 security regression blockers#163
Conversation
…e, remove path traversal vuln, harden workflows for supplychain attacks
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as git gtr (CLI)
participant Git as git (config/.gtrconfig)
participant Trust as TrustStore\n(XDG_CONFIG_HOME)
participant Hooks as Hook Runner
rect rgba(200,230,255,0.5)
U->>CLI: git gtr trust
CLI->>Git: locate `.gtrconfig` and read `hooks.*`
end
rect rgba(220,255,200,0.5)
CLI->>CLI: compute SHA‑256 of hooks content
CLI->>Trust: check for existing trust marker
Trust-->>CLI: marker present? (yes/no)
end
alt not trusted
CLI->>U: display hook contents + prompt_yes_no
U-->>CLI: confirm
CLI->>Trust: write trust marker for content hash
Trust-->>CLI: success / failure
CLI->>U: exit 0 on success / exit 1 on failure
else already trusted
CLI->>U: report hooks already trusted
end
rect rgba(255,240,200,0.5)
Note right of Hooks: Later hook execution (e.g., run_hooks)\nreads hooks via Trust-aware helper
Hooks->>Trust: request hooks for phase
Trust-->>Hooks: return trusted `.gtrconfig` hooks + local `gtr.hook.*`
Hooks->>Hooks: execute approved hooks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/adapters.sh (1)
307-360:⚠️ Potential issue | 🟠 MajorReject path separators in adapter names to prevent local command execution.
The metacharacter filter (lines 354–360) blocks shell operators but not slashes, so relative paths like
./tooland../toolpass through. Sincecommand -vaccepts relative paths and the full name is later passed toevalin_run_configured_command(), a user-controlled config value (e.g.,git gtr config set gtr.editor.default ./malicious) would execute arbitrary binaries from the current directory instead of only PATH-resolved commands. Reject/and\in the first token, or validate that the resolved command is in PATH only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/adapters.sh` around lines 307 - 360, The metacharacter check currently blocks shell operators but allows path separators, letting relative paths like "./tool" pass; update the validation in the adapter loader to reject path separators for the configured command (check either "name" or the extracted "cmd_name" for '/' and '\' characters) before accepting it, and return an error like other checks; alternatively, resolve the command with "command -v" and ensure the resolved path is a PATH-resolved command (not a relative/containment path) before proceeding — reference the variables "name" and "cmd_name" and the caller "_run_configured_command" when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/commands/init.sh`:
- Around line 275-277: The Fish wrapper's hardcoded subcommand list is out of
sync with the Bash list (the Bash block uses COMP_CWORD/COMPREPLY with compgen
and includes "trust"); update the Fish completion template in this file to match
the Bash subcommand list (add "trust" and any other missing entries) so that
running the init fish generator (git gtr init fish) produces the same
completions as the COMPREPLY/compgen block; alternatively, factor the subcommand
list into a single variable and reference it from both the COMP_CWORD/COMPREPLY
block and the Fish template so they stay synchronized.
In `@lib/commands/trust.sh`:
- Around line 13-40: cmd_trust reads and displays hooks then calls
_hooks_mark_trusted which re-hashes the file after the prompt; compute the
reviewed marker (e.g., call the hashing function/_get_marker used by
_hooks_mark_trusted) immediately after reading hook_content and before calling
prompt_yes_no, store that marker (and reviewed path if needed), show the hooks
as before, then after prompt_yes_no pass that stored marker into a new variant
of the marker-writer (or change _hooks_mark_trusted to accept a marker argument)
so you persist the exact reviewed marker instead of re-computing it
post-confirmation; keep _hooks_are_trusted usage unchanged but compare against
the stored marker where appropriate.
In `@lib/hooks.sh`:
- Around line 16-32: The trust marker key is currently content-only (via
_hooks_file_hash) which makes markers shared across repos; update
_hooks_trust_path to incorporate a repository-unique identifier (e.g.,
repository root path, remote URL, or git object like commit/slug) into the
computed key so the path becomes "$_GTR_TRUST_DIR/<repo-id>-<hash>" instead of
just "<hash>", and likewise update the wrapper generation in
lib/commands/init.sh to include the same repo identifier when creating trust
markers so markers are repo-scoped; keep using _hooks_file_hash for the content
part and prepend or append the chosen repo-id consistently.
---
Outside diff comments:
In `@lib/adapters.sh`:
- Around line 307-360: The metacharacter check currently blocks shell operators
but allows path separators, letting relative paths like "./tool" pass; update
the validation in the adapter loader to reject path separators for the
configured command (check either "name" or the extracted "cmd_name" for '/' and
'\' characters) before accepting it, and return an error like other checks;
alternatively, resolve the command with "command -v" and ensure the resolved
path is a PATH-resolved command (not a relative/containment path) before
proceeding — reference the variables "name" and "cmd_name" and the caller
"_run_configured_command" when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28b78bd9-7e2a-429d-863e-69b3767fa1e4
📒 Files selected for processing (24)
.github/workflows/homebrew.yml.github/workflows/lint.ymlREADME.mdbin/git-gtrcompletions/_git-gtrcompletions/git-gtr.fishcompletions/gtr.bashlib/adapters.shlib/args.shlib/commands/help.shlib/commands/init.shlib/commands/trust.shlib/config.shlib/copy.shlib/hooks.shlib/platform.shlib/ui.shscripts/generate-completions.shtests/adapters.batstests/cmd_trust.batstests/copy_safety.batstests/hooks.batstests/init.batstests/platform.bats
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/hooks.sh (1)
22-29:⚠️ Potential issue | 🟠 MajorPath scoping still leaves stale approvals across content changes.
Hashing
repo_root + hook hashfixes the old cross-path reuse, but it still does not re-prompt when the same checkout path starts executing different repo-local code. A trusted./scripts/bootstrapsurvives branch updates or a fresh clone into the same directory as long as.gtrconfigis unchanged, andrun_hooks()will then execute a payload the user never reviewed. This trust key needs a repository-state component that changes with the checked-out content, not just the path and hook string.Also applies to: 41-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks.sh` around lines 22 - 29, The current hook trust key uses only path+hook content, so a different commit checked out at the same path reuses approvals; update _hooks_file_hash to incorporate repository state by including a stable commit identifier (e.g., git rev-parse --verify HEAD) or a tree hash of the working tree when present, then pass that combined string into _hooks_content_hash so any change to checked-out content invalidates prior approvals; adjust callers like run_hooks to continue using _hooks_file_hash unchanged so the trust key now reflects both .gtrconfig hook entries and the current repository commit/tree state.
🧹 Nitpick comments (1)
lib/hooks.sh (1)
41-79: Keep the trust-key recipe single-sourced.
lib/hooks.shand the generated helper inlib/commands/init.sh:129-155now need to stay byte-for-byte aligned again. Since the last regression came from those drifting, I'd strongly prefer generating the wrapper helper from one shared source or template instead of maintaining two manual implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks.sh` around lines 41 - 79, The trust-key logic is duplicated between lib/hooks.sh (functions _hooks_trust_key, _hooks_trust_key_for_content, _hooks_trust_path, _hooks_trust_path_for_content) and the generated helper in lib/commands/init.sh; consolidate to a single source by moving the core trust-key recipe into one canonical implementation (either a shared template file or a single library function) and change the generator to emit a thin wrapper that calls that canonical implementation so both runtime and generated code remain byte-for-byte aligned; ensure the generator references the canonical symbol name (e.g., the single source function) when producing the wrapper and remove the duplicate manual implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/adapters.sh`:
- Around line 307-318: The override-loading code currently sources the adapter
file using adapter_selector/adapter_file then returns early (return 0), which
drops the original full name (e.g., "nano -w") so flags never reach the generic
runner; change the flow to source the override but do NOT return
immediately—instead set a marker (e.g., ADAPTER_OVERRIDE_LOADED=1 or set
adapter_override_file="$adapter_file") and let the function continue so the full
name variable (the original name variable) is preserved and passed to the
generic runner; apply the same change for the second override branch that
mirrors this logic (the block that uses adapter_selector/adapter_file again).
- Around line 183-193: The validation allows shell builtins and wrapper commands
with flags which can be exploited because _run_configured_command uses eval "set
-- $command_string"; change the validation to use type -P instead of command -v
so only PATH executables (not builtins) are accepted, and add a check that the
command string contains no option flags (tokens starting with -) or disallowed
wrapper executables (e.g., sh, bash, eval, builtin) before allowing it to be
stored/used; update the validation logic that currently checks metacharacters
(refer to the validation function called before _run_configured_command) to
explicitly reject tokens beginning with '-' and to reject commands where type -P
returns empty or where the resolved command name matches a blacklist of shell
wrappers.
In `@lib/hooks.sh`:
- Around line 91-102: _hooks_write_trust_marker() currently creates the target
file path then writes to it, which can leave a partial/empty marker on failure;
change it to write atomically by creating a temp file in the same directory
(e.g. "${trust_path}.tmp"), write the payload to that temp file, flush it (fsync
the file) and then mv the temp into place, and optionally fsync the directory
(using _GTR_TRUST_DIR) to ensure durability; additionally, tighten
_hooks_are_trusted() to validate the stored payload (e.g. check file is
non-empty and matches the expected config_file or pattern) before treating it as
trusted so a truncated/empty marker cannot be accepted.
---
Duplicate comments:
In `@lib/hooks.sh`:
- Around line 22-29: The current hook trust key uses only path+hook content, so
a different commit checked out at the same path reuses approvals; update
_hooks_file_hash to incorporate repository state by including a stable commit
identifier (e.g., git rev-parse --verify HEAD) or a tree hash of the working
tree when present, then pass that combined string into _hooks_content_hash so
any change to checked-out content invalidates prior approvals; adjust callers
like run_hooks to continue using _hooks_file_hash unchanged so the trust key now
reflects both .gtrconfig hook entries and the current repository commit/tree
state.
---
Nitpick comments:
In `@lib/hooks.sh`:
- Around line 41-79: The trust-key logic is duplicated between lib/hooks.sh
(functions _hooks_trust_key, _hooks_trust_key_for_content, _hooks_trust_path,
_hooks_trust_path_for_content) and the generated helper in lib/commands/init.sh;
consolidate to a single source by moving the core trust-key recipe into one
canonical implementation (either a shared template file or a single library
function) and change the generator to emit a thin wrapper that calls that
canonical implementation so both runtime and generated code remain byte-for-byte
aligned; ensure the generator references the canonical symbol name (e.g., the
single source function) when producing the wrapper and remove the duplicate
manual implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc37536c-f564-4ebe-adc2-f74003eb714e
📒 Files selected for processing (8)
lib/adapters.shlib/commands/init.shlib/commands/trust.shlib/hooks.shtests/adapters.batstests/cmd_trust.batstests/hooks.batstests/init.bats
✅ Files skipped from review due to trivial changes (2)
- tests/init.bats
- lib/commands/trust.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/hooks.bats
- tests/adapters.bats
- lib/commands/init.sh
- tests/cmd_trust.bats
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks.sh`:
- Around line 117-118: If _hooks_canonical_config_path fails after creating the
temp file, the temp file referenced by temp_path (created via mktemp
"$_GTR_TRUST_DIR/.trust.XXXXXX") is leaked; update the failure path so that when
canonical_config_file=$(_hooks_canonical_config_path "$config_file") returns
non-zero you remove the temp file (rm -f "$temp_path") before returning (or set
a local cleanup/trap that removes temp_path on early exit) to ensure no
temporary files are left behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42772e88-3740-46a9-bd38-764b54ca23f5
📒 Files selected for processing (9)
adapters/ai/claude.shadapters/ai/cursor.shadapters/editor/nano.shlib/adapters.shlib/commands/init.shlib/hooks.shtests/adapters.batstests/hooks.batstests/init.bats
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/hooks.bats
- tests/adapters.bats
- lib/adapters.sh
- tests/init.bats
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/adapters.sh`:
- Around line 296-304: Remove the overly restrictive pre-parse validations:
delete the helper functions _configured_command_uses_path_arg and
_configured_command_has_safe_syntax and stop rejecting arguments like
"~/.toolrc" or raw metacharacters, and instead only validate the adapter command
name inside _load_adapter(); ensure _load_adapter() no longer calls those two
functions and only checks the command name token (leaving GTR_*_CMD_ARGS
handling to the adapter overrides) so argv-style execution via "$@" can accept
literal path-like args and metacharacters as safe data.
In `@lib/commands/init.sh`:
- Around line 174-180: The regex passed to string replace in the branch handling
linked worktrees is over-escaped ('/\\.git$') so Fish treats the backslash
literally; change the pattern to '/\.git$' (single backslash) in the string
replace -r call that operates on _gtr_git_common_dir so the trailing '/.git'
suffix is correctly stripped (i.e., update the expression string replace -r
'/\\.git$' '' -- "$_gtr_git_common_dir" to use '/\.git$' instead).
In `@lib/hooks.sh`:
- Around line 135-175: The code currently treats failures from
_hooks_current_trust_path as success (returning 0), merging “no hooks” with real
errors; change the error handling so that when _hooks_current_trust_path fails
it returns non-zero (fail closed). Specifically, in _hooks_are_trusted() replace
the trust_path assignment branch that uses "|| return 0" with "|| return 1" so
resolution errors mark hooks untrusted, and in _hooks_mark_trusted() do the same
(replace "trust_path=$(_hooks_current_trust_path \"$config_file\") || return 0"
with "|| return 1") so marker writes are aborted on resolution errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf8e3b93-1ea4-40b2-a342-d1d7f67142a8
📒 Files selected for processing (8)
README.mdlib/adapters.shlib/commands/help.shlib/commands/init.shlib/commands/trust.shlib/hooks.shtests/adapters.batstests/init.bats
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- lib/commands/help.sh
- tests/init.bats
- lib/commands/trust.sh
| _configured_command_uses_path_arg() { | ||
| local arg="$1" | ||
| case "$arg" in | ||
| /* | ./* | ../* | ~* | *\\*) | ||
| return 0 | ||
| ;; | ||
| esac | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- validation helpers ---'
sed -n '296,351p' lib/adapters.sh
echo
echo '--- load_adapter validation chain ---'
sed -n '470,478p' lib/adapters.sh
echo
echo '--- override adapters consuming configured args ---'
sed -n '9,28p' adapters/editor/nano.sh
echo
sed -n '42,62p' adapters/ai/claude.shRepository: coderabbitai/git-worktree-runner
Length of output: 2889
🏁 Script executed:
# Check where these validation functions are called
rg '_configured_command_uses_path_arg|_configured_command_has_safe_syntax|_configured_command_is_safe' lib/ adapters/ --no-headingRepository: coderabbitai/git-worktree-runner
Length of output: 451
🏁 Script executed:
# Test the pattern matching behavior
bash -c '
arg="~/.toolrc"
case "$arg" in
~*)
echo "~/.toolrc matches ~* pattern (would be rejected)"
;;
*)
echo "~/.toolrc does not match pattern"
;;
esac
arg="%s\n"
case "$arg" in
\;* | \`* | '"'"'\$(\'' | \|* | \&* | '\''>'\'\'* | '\''<'\''*)
echo "%s\n contains metacharacters (would be rejected)"
;;
*)
echo "%s\n is safe from _configured_command_has_safe_syntax"
;;
esac
'Repository: coderabbitai/git-worktree-runner
Length of output: 286
🏁 Script executed:
# Test the pattern matching behavior more simply
bash -c '
arg="~/.toolrc"
case "$arg" in
~*)
echo "RESULT: ~/.toolrc matches ~* pattern"
;;
*)
echo "RESULT: ~/.toolrc does not match pattern"
;;
esac
'
bash -c '
arg="%s\n"
case "$arg" in
*\;* | *\`* | *\|\* | *\&* | *\>* | *\<*)
echo "RESULT: %s\n contains metacharacters"
;;
*)
echo "RESULT: %s\n is safe from syntax checks"
;;
esac
'
bash -c '
arg="foo & bar"
case "$arg" in
*\;* | *\`* | *\|\* | *\&* | *\>* | *\<*)
echo "RESULT: foo & bar contains metacharacters"
;;
*)
echo "RESULT: foo & bar is safe"
;;
esac
'Repository: coderabbitai/git-worktree-runner
Length of output: 194
Validation is overly restrictive for safe argv-based execution.
With command execution shifted from eval to parsed argv via "$@", literal path-like arguments and raw metacharacters in config values are now safe data, not shell syntax. Yet _configured_command_uses_path_arg() still rejects arguments matching ~* (e.g., ~/.toolrc), and _configured_command_has_safe_syntax() rejects raw metacharacters like & in the unparsed command string. These validations block legitimate configured commands before they reach the override adapters that correctly consume GTR_*_CMD_ARGS separately.
Suggested fix
Remove the path-argument check and pre-parse syntax validation. Keep only command-name validation in _load_adapter():
_configured_command_is_safe() {
[ "$#" -gt 0 ] || return 1
local cmd_name="$1"
shift
case "$cmd_name" in
*/* | *\\*)
return 1
;;
esac
if _configured_command_is_wrapper "$cmd_name" && [ "$#" -gt 0 ]; then
return 1
fi
+
+ return 0
}- if ! _configured_command_has_safe_syntax "$name" \
- || ! _parse_configured_command parsed_args "$name" \
+ if ! _parse_configured_command parsed_args "$name" \
|| ! _configured_command_is_safe "${parsed_args[@]}"; thenThen remove _configured_command_uses_path_arg() and _configured_command_has_safe_syntax() entirely (lines 296–327).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/adapters.sh` around lines 296 - 304, Remove the overly restrictive
pre-parse validations: delete the helper functions
_configured_command_uses_path_arg and _configured_command_has_safe_syntax and
stop rejecting arguments like "~/.toolrc" or raw metacharacters, and instead
only validate the adapter command name inside _load_adapter(); ensure
_load_adapter() no longer calls those two functions and only checks the command
name token (leaving GTR_*_CMD_ARGS handling to the adapter overrides) so
argv-style execution via "$@" can accept literal path-like args and
metacharacters as safe data.
| if test "$_gtr_git_common_dir" = ".git" | ||
| set -l _gtr_repo_root (git rev-parse --show-toplevel 2>/dev/null) | ||
| test -n "$_gtr_repo_root"; or return 1 | ||
| printf '%s/.gtrconfig\n' "$_gtr_repo_root" | ||
| else | ||
| printf '%s/.gtrconfig\n' (string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir") | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
command -v fish >/dev/null 2>&1 || exit 0
fish <<'FISH'
printf 'current: '
string replace -r '/\\.git$' '' -- '/tmp/repo/.git'
printf '\nfixed: '
string replace -r '/\.git$' '' -- '/tmp/repo/.git'
printf '\n'
FISHRepository: coderabbitai/git-worktree-runner
Length of output: 58
🏁 Script executed:
# Check if the file exists and read the relevant lines
cat -n lib/commands/init.sh | sed -n '170,185p'Repository: coderabbitai/git-worktree-runner
Length of output: 828
🏁 Script executed:
# Also check the surrounding context to understand the purpose better
cat -n lib/commands/init.sh | sed -n '150,190p'Repository: coderabbitai/git-worktree-runner
Length of output: 1750
🏁 Script executed:
# Search for other occurrences of the regex pattern
rg '/\\\\.git' lib/commands/init.shRepository: coderabbitai/git-worktree-runner
Length of output: 164
🏁 Script executed:
# Check git documentation references or comments about linked worktrees
rg -i 'worktree|linked' lib/commands/init.sh | head -20Repository: coderabbitai/git-worktree-runner
Length of output: 1317
🏁 Script executed:
# Verify the git-common-dir behavior for linked worktrees
# Check if there are any tests or documentation about this
rg -i 'git-common-dir|git.*common.*dir' lib/commands/init.sh -B2 -A2Repository: coderabbitai/git-worktree-runner
Length of output: 915
🏁 Script executed:
# Check if there's any test coverage for this function
fd -name "*.fish" -o -name "*test*" | xargs grep -l "gtrconfig_path" 2>/dev/null || echo "No test files found"Repository: coderabbitai/git-worktree-runner
Length of output: 343
Fish worktree path regex is over-escaped and breaks linked worktree config resolution.
In Fish, single-quoted strings keep backslashes literal, so the pattern /\\.git$ matches a literal backslash followed by a dot—not the .git suffix that git rev-parse --git-common-dir actually returns. The shell version of this function correctly uses %/.git parameter expansion, confirming the intent is to strip the /.git suffix. Linked worktrees will fail to find .gtrconfig at the repo root because the suffix is never stripped.
Suggested fix
- printf '%s/.gtrconfig\n' (string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir")
+ printf '%s/.gtrconfig\n' (string replace -r '/\.git$' '' -- "$_gtr_git_common_dir")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/commands/init.sh` around lines 174 - 180, The regex passed to string
replace in the branch handling linked worktrees is over-escaped ('/\\.git$') so
Fish treats the backslash literally; change the pattern to '/\.git$' (single
backslash) in the string replace -r call that operates on _gtr_git_common_dir so
the trailing '/.git' suffix is correctly stripped (i.e., update the expression
string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir" to use '/\.git$'
instead).
| _hooks_are_trusted() { | ||
| local config_file="$1" | ||
| [ ! -f "$config_file" ] && return 0 | ||
|
|
||
| local trust_path | ||
| trust_path=$(_hooks_current_trust_path "$config_file") || return 0 # no hooks = trusted | ||
| _hooks_marker_matches_config "$trust_path" "$config_file" | ||
| } | ||
|
|
||
| # Write a trust marker that matches the reviewed hooks | ||
| # Usage: _hooks_write_trust_marker <trust_path> [config_file] | ||
| _hooks_write_trust_marker() { | ||
| local trust_path="$1" | ||
| local config_file="${2:-}" | ||
| local temp_path="" | ||
| local canonical_config_file="" | ||
|
|
||
| mkdir -p "$_GTR_TRUST_DIR" || return 1 | ||
| canonical_config_file=$(_hooks_canonical_config_path "$config_file") || return 1 | ||
| temp_path="$(mktemp "$_GTR_TRUST_DIR/.trust.XXXXXX")" || return 1 | ||
|
|
||
| if ! printf '%s\n' "$canonical_config_file" > "$temp_path"; then | ||
| rm -f "$temp_path" | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! mv -f "$temp_path" "$trust_path"; then | ||
| rm -f "$temp_path" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| # Mark .gtrconfig hooks as trusted | ||
| # Usage: _hooks_mark_trusted <config_file> | ||
| _hooks_mark_trusted() { | ||
| local config_file="$1" | ||
| local trust_path | ||
| trust_path=$(_hooks_current_trust_path "$config_file") || return 0 | ||
|
|
||
| _hooks_write_trust_marker "$trust_path" "$config_file" | ||
| } |
There was a problem hiding this comment.
Fail closed when trust-key resolution breaks.
_hooks_are_trusted() and _hooks_mark_trusted() both treat _hooks_current_trust_path() failure as success. That collapses “no hooks” and real errors into the same path, so untrusted hooks can run and trust persistence can silently no-op.
Suggested fix
_hooks_are_trusted() {
local config_file="$1"
[ ! -f "$config_file" ] && return 0
- local trust_path
- trust_path=$(_hooks_current_trust_path "$config_file") || return 0 # no hooks = trusted
+ local hook_content trust_path
+ hook_content=$(_hooks_read_definitions "$config_file") || return 0
+ trust_path=$(_hooks_reviewed_trust_path "$config_file" "$hook_content") || return 1
_hooks_marker_matches_config "$trust_path" "$config_file"
}
@@
_hooks_mark_trusted() {
local config_file="$1"
- local trust_path
- trust_path=$(_hooks_current_trust_path "$config_file") || return 0
+ local hook_content trust_path
+ hook_content=$(_hooks_read_definitions "$config_file") || return 0
+ trust_path=$(_hooks_reviewed_trust_path "$config_file" "$hook_content") || return 1
_hooks_write_trust_marker "$trust_path" "$config_file"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks.sh` around lines 135 - 175, The code currently treats failures from
_hooks_current_trust_path as success (returning 0), merging “no hooks” with real
errors; change the error handling so that when _hooks_current_trust_path fails
it returns non-zero (fail closed). Specifically, in _hooks_are_trusted() replace
the trust_path assignment branch that uses "|| return 0" with "|| return 1" so
resolution errors mark hooks untrusted, and in _hooks_mark_trusted() do the same
(replace "trust_path=$(_hooks_current_trust_path \"$config_file\") || return 0"
with "|| return 1") so marker writes are aborted on resolution errors.
This supersedes #162 because the original PR branch cannot be updated by maintainers (
maintainerCanModify: false).It keeps the intended hardening from #162, fixes the security and regression blockers found during review, and includes a final cleanup pass to reduce long-term Bash maintenance risk without changing user-facing behavior.
What changed:
git gtr trustand generatedinitwrappersgit gtr trustif hooks change during confirmation.gtrconfigcorrectly from worktrees in generated wrappers./toolsh -c ...evalparser that preserves quoted arguments safelyclaude --continue,cursor --resume, andnano -wnode_modules/*and*/.*cmd_trustfail when trust persistence failstrustValidation:
bats tests/shellcheck bin/gtr bin/git-gtr lib/*.sh lib/commands/*.sh adapters/editor/*.sh adapters/ai/*.sh./scripts/generate-completions.sh --checkOriginal replacement-PR fix commit:
8bbf922.Follow-up hardening commits:
f2b7249,825820e,e63c8a9,7b90ec6.Cleanup commit:
01d23d3.Summary by CodeRabbit
New Features
git gtr trustto review/approve repository.gtrconfighooks before they run; trust is stored per-repo+content and requires re-approval on change.Security & Bug Fixes
.gitdeletion.Improvements
Tests