fix: address PR 162 security regression blockers#163
Conversation
…e, remove path traversal vuln, harden workflows for supplychain attacks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@NatoBoram just checking in. Need any support on this? |
lib/hooks.sh
Outdated
| [ ! -f "$config_file" ] && return 0 | ||
|
|
||
| local trust_path | ||
| trust_path=$(_hooks_current_trust_path "$config_file") || return 0 # no hooks = trusted |
There was a problem hiding this comment.
Blocker: _hooks_are_trusted fails open on any infrastructure error
The comment on this line says "no hooks = trusted" — that describes the intended case where _hooks_current_trust_path returns non-zero because the file has no hooks.* keys. But || return 0 fires on any failure: shasum unavailable, trust directory unreadable, or any other unexpected error. In those cases the function silently returns 0 (trusted) and .gtrconfig hooks execute without user approval, defeating the entire trust model.
The no-hooks case is still handled correctly downstream: _hooks_get_trusted reads hooks.$phase from the config file independently. If there are no hooks, that returns empty and nothing runs regardless of what _hooks_are_trusted returns.
The generated POSIX wrapper in init.sh already does the right thing for the equivalent operation:
_gtr_trust_key="$(__FUNC___hooks_current_trust_key ...)" || return 1The runtime and the generated helper are inconsistent — the generated wrapper is the correct model.
Fix: || return 0 → || return 1
lib/adapters.sh
Outdated
| _configured_command_uses_path_arg() { | ||
| local arg="$1" | ||
| case "$arg" in | ||
| /* | ./* | ../* | ~* | *\\*) |
There was a problem hiding this comment.
Blocker: path-argument filter misses --flag=./value form
The patterns here only match standalone path tokens (./foo, /abs, ../up, ~home):
case "$arg" in
/* | ./* | ../* | ~* | *\\*)
return 0
;;
esacA --flag=./value token like --package=./evil starts with --, matches none of these patterns, and passes through as safe.
This matters because .gtrconfig can set adapter defaults without trust review — lib/launch.sh calls cfg_default gtr.editor.default GTR_EDITOR_DEFAULT "none" defaults.editor, which reads [defaults] editor = ... from .gtrconfig with no hook-style gating. A .gtrconfig containing:
[defaults]
editor = npx --package=./malicious evilbinparses to ["npx", "--package=./malicious", "evilbin"]. npx is not in the wrapper list; --package=./malicious doesn't match any bare-path pattern; npx is found in PATH. _run_configured_command then executes npx --package=./malicious evilbin /path/to/worktree, which npm resolves against the local ./malicious package.
Similar vectors exist for any package-runner or tool that accepts --import, --require, --config, or --manifest-path with relative paths (pip, cargo, bundle exec, etc.).
Fix options:
- Strip the
=valuesuffix from--key=valuetokens before pattern-matching, or - Also reject any arg containing
./or../as a substring (not just prefix), or - Trust-gate
defaults.editor/defaults.aifrom.gtrconfigthe same way hooks are gated
| 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") |
There was a problem hiding this comment.
Fish regex over-escaped — linked-worktree .gtrconfig discovery silently broken
printf '%s/.gtrconfig\n' (string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir")This is inside a <<'FISH' heredoc (no Bash interpolation), so Fish receives the literal pattern /\\.git$. In Fish PCRE, \\ is an escaped backslash — it matches a literal \ character. Unix paths from git rev-parse --git-common-dir never contain backslashes, so the pattern never matches. The /.git suffix is not stripped; the function returns /path/to/repo/.git/.gtrconfig instead of /path/to/repo/.gtrconfig. The file doesn't exist, so .gtrconfig hooks are silently skipped for any Fish user in a linked worktree.
The equivalent POSIX template uses ${_gtr_git_common_dir%/.git} (parameter expansion, not regex) and works correctly.
Fix:
printf '%s/.gtrconfig\n' (string replace -r '/\.git$' '' -- "$_gtr_git_common_dir")One backslash before the dot in the regex correctly escapes the . as a literal character in PCRE.
No existing test exercises this branch (Fish + linked worktree), so this wouldn't be caught by the test suite.
averyjennings
left a comment
averyjennings
left a comment
There was a problem hiding this comment.
The trust model architecture is well-designed: repo-scoped key (SHA256(repo_root + "\n" + hook_content_hash)), atomic marker writes via mktemp + mv, and TOCTOU detection in cmd_trust are all solid. The eval removals throughout (printenv for config lookup, printf -v for UI input, hand-written tokenizer for adapter parsing) are correct and address the original PR 162 blockers.
Note on the CodeRabbit temp-file-leak concern: false positive in current code.
_hooks_canonical_config_pathis resolved on line 153 beforemktempon line 154 — there is no temp file to leak if canonical path resolution fails.
Three blockers remain from prior review rounds — inline comments with specific fixes attached.
F1 — Fish regex (lib/commands/init.sh:179): /\\.git$ in a <<'FISH' heredoc matches a literal backslash, not a dot. The .git suffix is never stripped for linked worktrees; Fish users see hooks permanently reported as untrusted even after running git gtr trust. The Bash/Zsh helpers (parameter expansion) are unaffected.
F2 — Trust gate fails open (lib/hooks.sh:140,172): _hooks_are_trusted and _hooks_mark_trusted use || return 0 as a catch-all that fires on any error, not just "no hooks defined." A stale worktree path that makes _hooks_repo_root fail is enough to trigger silent trust bypass for repos that have hook definitions.
F3 — .gtrconfig [defaults] bypasses the trust gate (lib/adapters.sh:299, lib/launch.sh:7-12): defaults.ai / defaults.editor are read from .gtrconfig via cfg_default without a trust check, and the adapter safety validator passes --flag=./value tokens. A repo-committed defaults.ai = npx --package=./malicious evilbin reaches _run_configured_command "$@" and executes without any git gtr trust prompt.
| 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") |
There was a problem hiding this comment.
Blocker: Fish regex over-escaped — linked-worktree .gtrconfig discovery silently broken
This line is inside <<'FISH' (single-quoted heredoc), so Bash passes the literal string /\\.git$ to Fish. In Fish PCRE:
\\\\= escaped backslash → matches a literal\\character- The full pattern
/\\.git$matches/\\<any_char>gitat end of string
Unix paths from git rev-parse --git-common-dir (e.g., /path/to/repo/.git) contain no backslashes, so the pattern never matches.
For a Fish user in a linked worktree:
$_gtr_git_common_dir=/path/to/repo/.gitstring replace -r '/\\.git$' ''does nothing- Function returns
/path/to/repo/.git/.gtrconfig(wrong path) - Config not found → trust check fails → hooks are permanently reported as untrusted, even after
git gtr trustwas run
The Bash/Zsh helpers use parameter expansion (${_gtr_git_common_dir%/.git}) and are unaffected.
Fix: change /\\.git$ to /\.git$ (single backslash = PCRE-escaped dot = matches literal .).
lib/hooks.sh
Outdated
| [ ! -f "$config_file" ] && return 0 | ||
|
|
||
| local trust_path | ||
| trust_path=$(_hooks_current_trust_path "$config_file") || return 0 # no hooks = trusted |
There was a problem hiding this comment.
Blocker: _hooks_are_trusted (and _hooks_mark_trusted at line 172) fail open on any infrastructure error
The comment says "no hooks = trusted" — correct intent. When _hooks_current_trust_path returns 1 because there are no hooks.* keys, vacuous trust is right.
But || return 0 fires on any non-zero return from the chain: if _hooks_repo_root cannot cd to the config file's directory (e.g., after git worktree move leaves a stale symlink), the whole chain returns 1 and _hooks_are_trusted returns 0 (trusted) — even when .gtrconfig does have hook definitions. Those hooks then execute without user approval.
_hooks_mark_trusted at line 172 has the same pattern: failure in _hooks_current_trust_path silently returns 0, so cmd_trust can appear to succeed without writing a marker.
Fix: separate the "no hooks" case from errors:
_hooks_are_trusted() {
local config_file="$1"
[ ! -f "$config_file" ] && return 0
# No hooks defined — nothing to gate
local hook_content
hook_content=$(_hooks_read_definitions "$config_file") || return 0
# Hooks exist — any key-computation failure means untrusted, not trusted
local trust_path
trust_path=$(_hooks_reviewed_trust_path "$config_file" "$hook_content") || return 1
_hooks_marker_matches_config "$trust_path" "$config_file"
}Apply the same pattern to _hooks_mark_trusted.
lib/adapters.sh
Outdated
| _configured_command_uses_path_arg() { | ||
| local arg="$1" | ||
| case "$arg" in | ||
| /* | ./* | ../* | ~* | *\\*) |
There was a problem hiding this comment.
Blocker: --flag=./value form bypasses the path-argument filter; .gtrconfig [defaults] reaches this validator with no trust gate
Gap 1 — filter miss: --package=./malicious starts with --, so none of the patterns (/* | ./* | ../* | ~* | *\\*) match. The argument passes as safe, is stored in GTR_*_CMD_ARGS, and reaches _run_configured_command "$@".
Gap 2 — no trust gate for .gtrconfig [defaults]: lib/launch.sh:7-12 reads defaults.ai / defaults.editor from .gtrconfig via cfg_default and passes the value directly to load_ai_adapter / load_editor_adapter. The hook trust gate covers hooks.* keys only — defaults.* keys have no equivalent gate.
Combined, a repo can commit:
[defaults]
ai = npx --package=./malicious evilbin_configured_command_is_safe "npx" "--package=./malicious" "evilbin" returns 0, and _run_configured_command executes npx --package=./malicious evilbin — running evilbin from the local ./malicious package directory — without git gtr trust ever being invoked.
Fix for the filter gap: add *=./*|*=../*|*=/* to _configured_command_uses_path_arg.
Fix for the trust gate gap: apply the same trust check to .gtrconfig defaults.* keys, or document clearly that [defaults] values are not trust-gated.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/commands/init.sh (1)
179-179:⚠️ Potential issue | 🟠 MajorFix the Fish regex on Line 179; it still won't strip
/.git.Because this code is emitted from a single-quoted heredoc, Fish receives
/\\.git$literally. In Fish regex syntax that matches a backslash, not the.gitsuffix, so linked worktrees resolve.gtrconfigunder.../.git/.gtrconfigand trust-gated hooks stay broken in Fish.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")#!/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' FISH🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commands/init.sh` at line 179, The Fish regex used in the printf call still contains an escaped backslash ('/\\.git$') so Fish matches a literal backslash instead of the '.git' suffix; update the regex in the printf/string replace invocation (the line calling string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir") to use '/\.git$' (i.e., remove the extra backslash) so Fish correctly strips '/.git' from linked worktree paths.
🧹 Nitpick comments (1)
tests/adapters.bats (1)
131-151: Consider extracting mock binary creation to a test helper (DRY).The
mktemp + cat + chmod + PATHscaffold repeats in several tests and can be centralized for easier maintenance.♻️ Optional refactor sketch
+create_mock_bin() { + local name="$1" + local body="$2" + mock_bin_dir="${mock_bin_dir:-$(mktemp -d)}" + cat > "$mock_bin_dir/$name" <<EOF +#!/usr/bin/env bash +$body +EOF + chmod +x "$mock_bin_dir/$name" + PATH="$mock_bin_dir:$PATH" +} `@test` "_load_adapter allows generic commands with slash-bearing arguments" { - mock_bin_dir="$(mktemp -d)" - cat > "$mock_bin_dir/bunx" <<'EOF' -#!/usr/bin/env bash -exit 0 -EOF - chmod +x "$mock_bin_dir/bunx" - PATH="$mock_bin_dir:$PATH" + create_mock_bin "bunx" "exit 0" _load_adapter "ai" "bunx `@github/copilot`@latest" "AI tool" "$(_list_registry_names "$_AI_REGISTRY")" "bunx, gpt" [ "$GTR_AI_CMD" = "bunx `@github/copilot`@latest" ] [ "$GTR_AI_CMD_NAME" = "bunx" ] }Also applies to: 159-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/adapters.bats` around lines 131 - 151, Extract the repeated "mktemp + cat + chmod + PATH" scaffold into a reusable test helper (e.g., create_mock_bin or setup_mock_bin) and call it from the tests instead of inlining the block; specifically replace the repeated scaffolding around mock_bin_dir, creating an executable (tool/bunx) and prepending it to PATH in the tests named "_load_adapter ..." and "_load_adapter allows path-like literal arguments", ensure the helper accepts the binary name and content, makes it executable, updates PATH, and performs cleanup (or returns the temp dir for later cleanup) so the duplicate code at the other occurrence (lines 159-165) is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/commands/init.sh`:
- Line 179: The Fish regex used in the printf call still contains an escaped
backslash ('/\\.git$') so Fish matches a literal backslash instead of the '.git'
suffix; update the regex in the printf/string replace invocation (the line
calling string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir") to use
'/\.git$' (i.e., remove the extra backslash) so Fish correctly strips '/.git'
from linked worktree paths.
---
Nitpick comments:
In `@tests/adapters.bats`:
- Around line 131-151: Extract the repeated "mktemp + cat + chmod + PATH"
scaffold into a reusable test helper (e.g., create_mock_bin or setup_mock_bin)
and call it from the tests instead of inlining the block; specifically replace
the repeated scaffolding around mock_bin_dir, creating an executable (tool/bunx)
and prepending it to PATH in the tests named "_load_adapter ..." and
"_load_adapter allows path-like literal arguments", ensure the helper accepts
the binary name and content, makes it executable, updates PATH, and performs
cleanup (or returns the temp dir for later cleanup) so the duplicate code at the
other occurrence (lines 159-165) is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2746854b-2562-4c9b-93fd-641eae58f1ff
📒 Files selected for processing (7)
lib/adapters.shlib/commands/init.shlib/config.shlib/hooks.shtests/adapters.batstests/hooks.batstests/init.bats
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/config.sh
- tests/hooks.bats
- tests/init.bats
- lib/adapters.sh
3 participants
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.CodeRabbit resolution commit:
475b9f0.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
.git.Improvements
Tests