Security Focused Updates - Thresher.sh findings#162
Security Focused Updates - Thresher.sh findings#162shadowcodex wants to merge 2 commits intocoderabbitai:mainfrom
Conversation
…e, remove path traversal vuln, harden workflows for supplychain attacks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
WalkthroughAdds a repository-level trust mechanism for Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as "git gtr trust"
participant Config as ".gtrconfig"
participant Hash as "Hash/Trust Marker"
participant Prompt as "Prompt"
User->>CLI: run "git gtr trust"
CLI->>Config: resolve path & read hooks.*
Config-->>CLI: hook entries
CLI->>Hash: compute SHA-256 of hooks
Hash-->>CLI: hash value
CLI->>Hash: check ~/.config/gtr/trusted/<hash>
Hash-->>CLI: not found
CLI->>Prompt: show hooks + warning, ask confirm
Prompt->>User: request confirmation
User-->>Prompt: confirm
Prompt-->>CLI: confirmed
CLI->>Hash: create trust marker file
Hash-->>CLI: marked trusted
CLI-->>User: success message
sequenceDiagram
participant Init as "gtr init / post-cd"
participant Config as ".gtrconfig"
participant Hash as "Trust Marker Check"
participant Hooks as "Hook Merger/Runner"
Init->>Config: load hooks.postCd
Config-->>Init: hook list
Init->>Hash: check trust marker for hooks
Hash-->>Init: trusted? / untrusted?
alt trusted
Init->>Hooks: include .gtrconfig hooks + git-config hooks
else untrusted
Init-->>Init: log warning, suggest "git gtr trust"
Init->>Hooks: include only git-config hooks
end
Hooks->>Hooks: dedupe & execute postCd hooks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
🤖 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 301-307: The current validation rejects any "/" in the entire
"$name", which wrongly blocks generic commands like "bunx
`@github/copilot`@latest"; update the check to validate only the adapter selector
portion (e.g., the token that follows the command, not the executable) by
extracting the selector from "$name" (or use the existing selector variable if
present) and run the case pattern against that selector instead of "$name",
keeping the same log_error "$label name '...'" semantics but reporting the
selector; ensure the change targets the block that contains the case "$name"
check so generic fallback still accepts executables with "/" in their arguments.
In `@lib/commands/init.sh`:
- Around line 97-123: The cached init output keyed only on GTR_VERSION (see
cmd_init and the ~/.cache/gtr/init-* stamp) still serves the old pre-trust hook
bodies (and bin/git-gtr still reports 2.6.0), so update the caching/stamp logic
to invalidate or version the init cache whenever the shell-integration/hook
trust behavior changes: either include a new trust-related stamp (e.g., a
HOOKS_TRUST_VERSION or a hash of the new hook bodies) in the cache key used by
cmd_init and related init paths (also update the other init cache usages around
the blocks noted at 281-309 and 468-492), or bump GTR_VERSION reported by
bin/git-gtr so existing caches are treated as stale and regenerated with the
postCd trust checks enabled.
- Around line 107-123: The trust-check is building _gtr_config_file with git
rev-parse --show-toplevel (variable _gtr_config_file) which points to a worktree
root and thus misses the repo-level .gtrconfig; change the lookup to use the
repository common dir (e.g., use git rev-parse --git-common-dir or call the
existing _gtrconfig_path() helper from lib/config.sh) so _gtr_config_file points
at the main repo .gtrconfig before performing the trust/hash check in the
post-cd hook logic (the code block around _gtr_config_file, _gtr_file_hooks, and
_gtr_hook_hash).
In `@lib/commands/trust.sh`:
- Around line 35-37: The success message is printed unconditionally even if
_hooks_mark_trusted() failed; update the trust command to check the exit status
of _hooks_mark_trusted (call it and test its return code) and only call log_info
"Hooks marked as trusted" when _hooks_mark_trusted returns success; on failure,
surface an error via log_error or non-zero exit so the user knows persistence
failed. Ensure you reference the _hooks_mark_trusted function from the trust
flow and propagate its failure rather than always printing success.
In `@lib/copy.sh`:
- Around line 285-291: The current case in lib/copy.sh incorrectly rejects "*"
and ".*" suffixes (pattern_suffix) which prevents legitimate directory-exclude
patterns like "node_modules/*" and "*/.*" from being applied; update the
rejection logic so only truly empty suffixes are skipped (keep the "" branch)
and remove "*" and ".*" from the blanket reject list, and instead add an
explicit check that protects .git by detecting exclude_pattern values that
target ".git" (e.g., exactly ".git", end with "/.git", or patterns like
"*/.git") and log_warn/continue for those only; locate and change the case
handling around pattern_suffix plus ensure _apply_directory_excludes() and
copy_directories() still honor wildcard suffixes afterwards.
In `@lib/hooks.sh`:
- Around line 33-36: The current trust check uses only the content hash from
_hooks_file_hash and checks for a file in _GTR_TRUST_DIR/$hash, allowing
different repositories with identical hook text to be trusted; update the logic
to bind trust to the repository by incorporating a canonical repo identifier
(e.g., output of git rev-parse --show-toplevel or a resolved absolute path) as a
salt when generating the trust key and when creating/checking the trust file:
change where hash is computed in the trust check (the variable hash from
_hooks_file_hash and the check against _GTR_TRUST_DIR/$hash) to compute a
composite key like sha256("$hash:$repo_id") and use that filename, and ensure
the code path that writes the trust file (where it currently writes the current
path) writes the same composite key so the existence check validates both hook
content and repo identity.
- Around line 64-66: The code currently calls _hooks_are_trusted on $config_file
then re-reads $config_file with git config -f, allowing a TOCTOU race; instead,
create an immutable snapshot of the config and use that single snapshot for both
trust-check and loading hooks: copy $config_file to a temp file (e.g., snapshot)
immediately after the existence check, call _hooks_are_trusted with that
snapshot and then read hooks into file_hooks with git config -f "$snapshot"
--get-all "hooks.$phase"; ensure the temp snapshot is removed after use to avoid
leaks.
- Around line 16-23: _hooks_file_hash() currently captures git output via
command substitution then prints it with printf '%s', which removes the trailing
newline and yields a different digest than the pipeline used in init.sh; change
_hooks_file_hash to preserve the exact bytes (including the trailing newline)
when computing the SHA256 so trust tokens match — e.g., avoid command
substitution and instead pipe git config -f "$config_file" --get-regexp
'^hooks\.' 2>/dev/null directly into shasum (or ensure you print with a trailing
newline) so the hash produced by _hooks_file_hash matches the hash logic used in
lib/commands/init.sh and _hooks_mark_trusted().
In `@lib/platform.sh`:
- Around line 129-155: The code currently uses printf '%q' to build
safe_sh_cmd/safe_sh_path and safe_win_cmd/safe_win_path which produces
Bash-style quoting and passes the whole command as a single quoted word,
breaking multi-word commands; instead stop pre-quoting and pass the command and
path as positional parameters to the spawned shell/terminal: for
gnome-terminal/konsole/xterm invoke sh -c with a small wrapper like "cd \"$1\"
&& shift && exec \"$@\"" (pass path then the command and its args as subsequent
arguments) so the target shell receives argv properly; similarly, remove printf
'%q' for Windows paths/commands and pass the command as separate args to wt or
to cmd.exe /k (use cmd.exe to change to the directory then execute the passed
argv) so gnome-terminal/konsole/xterm, wt and cmd.exe receive the real argv
instead of a single Bash-escaped string (adjust usage around
safe_sh_cmd/safe_sh_path and safe_win_cmd/safe_win_path, and calls to
gnome-terminal, konsole, xterm, wt and cmd.exe).
🪄 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: 23ad21c6-be61-4a9b-915e-90ae3994f13d
📒 Files selected for processing (12)
.github/workflows/homebrew.yml.github/workflows/lint.ymlbin/git-gtrlib/adapters.shlib/args.shlib/commands/init.shlib/commands/trust.shlib/config.shlib/copy.shlib/hooks.shlib/platform.shlib/ui.sh
| # Reject adapter names containing path traversal characters | ||
| case "$name" in | ||
| */* | *..* | *\\*) | ||
| log_error "$label name '$name' contains invalid characters" | ||
| return 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Validate only the adapter selector, not the full generic command.
This check runs before generic fallback and rejects any / anywhere in name. That blocks legitimate commands like bunx @github/copilot@latest—the same form called out in the new example on Line 183—even though the executable being resolved is just bunx.
♻️ Proposed fix
_load_adapter() {
- local type="$1" name="$2" label="$3" builtin_list="$4" path_hint="$5"
+ local type="$1" name="$2" label="$3" builtin_list="$4" path_hint="$5"
+ local adapter_selector="${name%% *}"
# Reject adapter names containing path traversal characters
- case "$name" in
+ case "$adapter_selector" in
*/* | *..* | *\\*)
log_error "$label name '$name' contains invalid characters"
return 1
;;
esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/adapters.sh` around lines 301 - 307, The current validation rejects any
"/" in the entire "$name", which wrongly blocks generic commands like "bunx
`@github/copilot`@latest"; update the check to validate only the adapter selector
portion (e.g., the token that follows the command, not the executable) by
extracting the selector from "$name" (or use the existing selector variable if
present) and run the case pattern against that selector instead of "$name",
keeping the same log_error "$label name '...'" semantics but reporting the
selector; ensure the change targets the block that contains the case "$name"
check so generic fallback still accepts executables with "/" in their arguments.
| __FUNC___run_post_cd_hooks() { | ||
| local dir="$1" | ||
| local _gtr_trust_dir="${XDG_CONFIG_HOME:-$HOME/.config}/gtr/trusted" | ||
|
|
||
| cd "$dir" && { | ||
| local _gtr_hooks _gtr_hook _gtr_seen _gtr_config_file | ||
| _gtr_hooks="" | ||
| _gtr_seen="" | ||
| # Read from git config (local > global > system) | ||
| _gtr_hooks="$(git config --get-all gtr.hook.postCd 2>/dev/null)" || true | ||
| # Read from .gtrconfig if it exists | ||
| # Read from .gtrconfig if it exists — only if trusted | ||
| _gtr_config_file="$(git rev-parse --show-toplevel 2>/dev/null)/.gtrconfig" | ||
| if [ -f "$_gtr_config_file" ]; then | ||
| local _gtr_file_hooks | ||
| _gtr_file_hooks="$(git config -f "$_gtr_config_file" --get-all hooks.postCd 2>/dev/null)" || true | ||
| if [ -n "$_gtr_file_hooks" ]; then | ||
| if [ -n "$_gtr_hooks" ]; then | ||
| _gtr_hooks="$_gtr_hooks"$'\n'"$_gtr_file_hooks" | ||
| # Verify trust before including .gtrconfig hooks | ||
| local _gtr_hook_hash | ||
| _gtr_hook_hash="$(git config -f "$_gtr_config_file" --get-regexp '^hooks\.' 2>/dev/null | shasum -a 256 | cut -d' ' -f1)" || true | ||
| if [ -n "$_gtr_hook_hash" ] && [ -f "$_gtr_trust_dir/$_gtr_hook_hash" ]; then | ||
| if [ -n "$_gtr_hooks" ]; then | ||
| _gtr_hooks="$_gtr_hooks"$'\n'"$_gtr_file_hooks" | ||
| else | ||
| _gtr_hooks="$_gtr_file_hooks" | ||
| fi | ||
| else | ||
| _gtr_hooks="$_gtr_file_hooks" | ||
| echo "__FUNC__: Untrusted .gtrconfig hooks skipped — run 'git gtr trust' to approve" >&2 |
There was a problem hiding this comment.
These new shell-integration bodies are still hidden behind the old init cache.
cmd_init() only keys ~/.cache/gtr/init-* on GTR_VERSION (Lines 68-88), and bin/git-gtr still reports 2.6.0. Anyone who already has cached init output will keep getting the pre-trust hook script, so this security gate does not land until the cache is manually cleared or the stamp changes.
Also applies to: 281-309, 468-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/commands/init.sh` around lines 97 - 123, The cached init output keyed
only on GTR_VERSION (see cmd_init and the ~/.cache/gtr/init-* stamp) still
serves the old pre-trust hook bodies (and bin/git-gtr still reports 2.6.0), so
update the caching/stamp logic to invalidate or version the init cache whenever
the shell-integration/hook trust behavior changes: either include a new
trust-related stamp (e.g., a HOOKS_TRUST_VERSION or a hash of the new hook
bodies) in the cache key used by cmd_init and related init paths (also update
the other init cache usages around the blocks noted at 281-309 and 468-492), or
bump GTR_VERSION reported by bin/git-gtr so existing caches are treated as stale
and regenerated with the postCd trust checks enabled.
| # Read from .gtrconfig if it exists — only if trusted | ||
| _gtr_config_file="$(git rev-parse --show-toplevel 2>/dev/null)/.gtrconfig" | ||
| if [ -f "$_gtr_config_file" ]; then | ||
| local _gtr_file_hooks | ||
| _gtr_file_hooks="$(git config -f "$_gtr_config_file" --get-all hooks.postCd 2>/dev/null)" || true | ||
| if [ -n "$_gtr_file_hooks" ]; then | ||
| if [ -n "$_gtr_hooks" ]; then | ||
| _gtr_hooks="$_gtr_hooks"$'\n'"$_gtr_file_hooks" | ||
| # Verify trust before including .gtrconfig hooks | ||
| local _gtr_hook_hash | ||
| _gtr_hook_hash="$(git config -f "$_gtr_config_file" --get-regexp '^hooks\.' 2>/dev/null | shasum -a 256 | cut -d' ' -f1)" || true | ||
| if [ -n "$_gtr_hook_hash" ] && [ -f "$_gtr_trust_dir/$_gtr_hook_hash" ]; then | ||
| if [ -n "$_gtr_hooks" ]; then | ||
| _gtr_hooks="$_gtr_hooks"$'\n'"$_gtr_file_hooks" | ||
| else | ||
| _gtr_hooks="$_gtr_file_hooks" | ||
| fi | ||
| else | ||
| _gtr_hooks="$_gtr_file_hooks" | ||
| echo "__FUNC__: Untrusted .gtrconfig hooks skipped — run 'git gtr trust' to approve" >&2 |
There was a problem hiding this comment.
The post-cd trust check is looking in the worktree, not the main repo.
Inside a linked worktree, git rev-parse --show-toplevel points at the worktree root. _gtrconfig_path() in lib/config.sh:17-33 intentionally uses --git-common-dir to reach the main repo root, and git gtr trust stores trust against that file. With Lines 108, 293, and 481 still using --show-toplevel, trusted repo-level .gtrconfig hooks will never be found here.
Also applies to: 292-308, 480-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/commands/init.sh` around lines 107 - 123, The trust-check is building
_gtr_config_file with git rev-parse --show-toplevel (variable _gtr_config_file)
which points to a worktree root and thus misses the repo-level .gtrconfig;
change the lookup to use the repository common dir (e.g., use git rev-parse
--git-common-dir or call the existing _gtrconfig_path() helper from
lib/config.sh) so _gtr_config_file points at the main repo .gtrconfig before
performing the trust/hash check in the post-cd hook logic (the code block around
_gtr_config_file, _gtr_file_hooks, and _gtr_hook_hash).
| if prompt_yes_no "Trust these hooks?"; then | ||
| _hooks_mark_trusted "$config_file" | ||
| log_info "Hooks marked as trusted" |
There was a problem hiding this comment.
Don't report trust success when persistence failed.
_hooks_mark_trusted() in lib/hooks.sh:39-48 can fail on mkdir -p or the redirect. Line 37 still prints success unconditionally, so the user can be told the hooks are trusted even though no marker was written.
♻️ Proposed fix
if prompt_yes_no "Trust these hooks?"; then
- _hooks_mark_trusted "$config_file"
- log_info "Hooks marked as trusted"
+ if _hooks_mark_trusted "$config_file"; then
+ log_info "Hooks marked as trusted"
+ else
+ log_error "Failed to persist hook trust state"
+ return 1
+ fi
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if prompt_yes_no "Trust these hooks?"; then | |
| _hooks_mark_trusted "$config_file" | |
| log_info "Hooks marked as trusted" | |
| if prompt_yes_no "Trust these hooks?"; then | |
| if _hooks_mark_trusted "$config_file"; then | |
| log_info "Hooks marked as trusted" | |
| else | |
| log_error "Failed to persist hook trust state" | |
| return 1 | |
| fi | |
| else |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/commands/trust.sh` around lines 35 - 37, The success message is printed
unconditionally even if _hooks_mark_trusted() failed; update the trust command
to check the exit status of _hooks_mark_trusted (call it and test its return
code) and only call log_info "Hooks marked as trusted" when _hooks_mark_trusted
returns success; on failure, surface an error via log_error or non-zero exit so
the user knows persistence failed. Ensure you reference the _hooks_mark_trusted
function from the trust flow and propagate its failure rather than always
printing success.
| # Reject bare glob-only suffixes that would match everything | ||
| case "$pattern_suffix" in | ||
| ""|"*"|"**"|".*") | ||
| log_warn "Skipping overly broad exclude suffix: $exclude_pattern" | ||
| continue | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Rejecting */.* here makes existing directory excludes a no-op.
Lines 285-291 now skip the exact suffix shapes that Lines 263-264 still document as supported, including node_modules/* and */.*. Because _apply_directory_excludes() runs after copy_directories(), those patterns stop stripping copied content and the worktree keeps files the user explicitly excluded. If the goal is only protecting .git, handle that path explicitly instead of disabling the wildcard forms wholesale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/copy.sh` around lines 285 - 291, The current case in lib/copy.sh
incorrectly rejects "*" and ".*" suffixes (pattern_suffix) which prevents
legitimate directory-exclude patterns like "node_modules/*" and "*/.*" from
being applied; update the rejection logic so only truly empty suffixes are
skipped (keep the "" branch) and remove "*" and ".*" from the blanket reject
list, and instead add an explicit check that protects .git by detecting
exclude_pattern values that target ".git" (e.g., exactly ".git", end with
"/.git", or patterns like "*/.git") and log_warn/continue for those only; locate
and change the case handling around pattern_suffix plus ensure
_apply_directory_excludes() and copy_directories() still honor wildcard suffixes
afterwards.
| _hooks_file_hash() { | ||
| local config_file="$1" | ||
| local hook_content | ||
| hook_content=$(git config -f "$config_file" --get-regexp '^hooks\.' 2>/dev/null) || true | ||
| if [ -z "$hook_content" ]; then | ||
| return 1 | ||
| fi | ||
| printf '%s' "$hook_content" | shasum -a 256 | cut -d' ' -f1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "lib/hooks.sh:"
sed -n '16,24p' lib/hooks.sh
echo
echo "lib/commands/init.sh:"
sed -n '114,116p' lib/commands/init.sh
echo
sample=$'hooks.postCd ./scripts/post-cd.sh\n'
hash_via_hooks=$(
hook_content=$(printf '%s' "$sample")
printf '%s' "$hook_content" | shasum -a 256 | cut -d' ' -f1
)
hash_via_init=$(printf '%s' "$sample" | shasum -a 256 | cut -d' ' -f1)
printf 'hash_via_hooks=%s\n' "$hash_via_hooks"
printf 'hash_via_init=%s\n' "$hash_via_init"
test "$hash_via_hooks" != "$hash_via_init"Repository: coderabbitai/git-worktree-runner
Length of output: 705
🏁 Script executed:
# Find all usages of _hooks_file_hash to understand the call flow
rg '_hooks_file_hash' --type bash -B 2 -A 2
# Also check the trust verification flow
rg '_hooks_are_trusted|_hooks_mark_trusted' --type bash -B 1 -A 3Repository: coderabbitai/git-worktree-runner
Length of output: 133
🏁 Script executed:
# Search for all usages of the hash functions (no type filter)
rg '_hooks_file_hash' -B 2 -A 2
# Search for trust functions
rg '_hooks_are_trusted|_hooks_mark_trusted|_hooks_get_trusted' -B 1 -A 3
# Also view the full init.sh section around line 114
sed -n '110,120p' lib/commands/init.sh
# And full hooks.sh section around line 16
sed -n '10,30p' lib/hooks.shRepository: coderabbitai/git-worktree-runner
Length of output: 4236
🏁 Script executed:
# Get broader context around the init.sh hash computation
sed -n '100,130p' lib/commands/init.sh
# Also check if _gtr_trust_dir is set and if it matches _GTR_TRUST_DIR from hooks.sh
rg '_gtr_trust_dir|_GTR_TRUST_DIR' -B 1 -A 1
# Verify git config output behavior with a simple example
# Check what git config actually outputs (with or without trailing newline)
cat lib/commands/init.sh | head -150 | tail -50Repository: coderabbitai/git-worktree-runner
Length of output: 5380
Make _hooks_file_hash() hash computation match lib/commands/init.sh byte-for-byte.
Lines 19 and 23 compute the hash using command substitution followed by printf '%s', which strips the trailing newline that git config --get-regexp outputs. The direct pipeline in lib/commands/init.sh:114-116 hashes with that newline intact, so trust markers written by _hooks_mark_trusted() won't match the hash lookup in init.sh, breaking the trust system.
Suggested fix
_hooks_file_hash() {
local config_file="$1"
local hook_content
- hook_content=$(git config -f "$config_file" --get-regexp '^hooks\.' 2>/dev/null) || true
- if [ -z "$hook_content" ]; then
- return 1
- fi
- printf '%s' "$hook_content" | shasum -a 256 | cut -d' ' -f1
+ hook_content=$(git config -f "$config_file" --get-regexp '^hooks\.' 2>/dev/null) || return 1
+ printf '%s\n' "$hook_content" | shasum -a 256 | cut -d' ' -f1
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks.sh` around lines 16 - 23, _hooks_file_hash() currently captures git
output via command substitution then prints it with printf '%s', which removes
the trailing newline and yields a different digest than the pipeline used in
init.sh; change _hooks_file_hash to preserve the exact bytes (including the
trailing newline) when computing the SHA256 so trust tokens match — e.g., avoid
command substitution and instead pipe git config -f "$config_file" --get-regexp
'^hooks\.' 2>/dev/null directly into shasum (or ensure you print with a trailing
newline) so the hash produced by _hooks_file_hash matches the hash logic used in
lib/commands/init.sh and _hooks_mark_trusted().
| local hash | ||
| hash=$(_hooks_file_hash "$config_file") || return 0 # no hooks = trusted | ||
|
|
||
| [ -f "$_GTR_TRUST_DIR/$hash" ] |
There was a problem hiding this comment.
Bind trust to the repository, not just the hook text.
Line 36 trusts any .gtrconfig that hashes to the same content, and Line 47 writes the current path but never checks it. A second repo can reuse the same hook string (./scripts/postCreate.sh, make bootstrap, etc.) and inherit trust even though it runs different repo code.
Minimal hardening
_hooks_are_trusted() {
local config_file="$1"
[ ! -f "$config_file" ] && return 0
local hash
hash=$(_hooks_file_hash "$config_file") || return 0 # no hooks = trusted
- [ -f "$_GTR_TRUST_DIR/$hash" ]
+ [ -f "$_GTR_TRUST_DIR/$hash" ] &&
+ [ "$(cat "$_GTR_TRUST_DIR/$hash" 2>/dev/null)" = "$config_file" ]
}At minimum, validate the stored repo/config identity; ideally, salt the trust key with the canonical repo root.
Also applies to: 46-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks.sh` around lines 33 - 36, The current trust check uses only the
content hash from _hooks_file_hash and checks for a file in
_GTR_TRUST_DIR/$hash, allowing different repositories with identical hook text
to be trusted; update the logic to bind trust to the repository by incorporating
a canonical repo identifier (e.g., output of git rev-parse --show-toplevel or a
resolved absolute path) as a salt when generating the trust key and when
creating/checking the trust file: change where hash is computed in the trust
check (the variable hash from _hooks_file_hash and the check against
_GTR_TRUST_DIR/$hash) to compute a composite key like sha256("$hash:$repo_id")
and use that filename, and ensure the code path that writes the trust file
(where it currently writes the current path) writes the same composite key so
the existence check validates both hook content and repo identity.
| if [ -n "$config_file" ] && [ -f "$config_file" ]; then | ||
| if _hooks_are_trusted "$config_file"; then | ||
| file_hooks=$(git config -f "$config_file" --get-all "hooks.$phase" 2>/dev/null) || true |
There was a problem hiding this comment.
Use one snapshot for approval and execution.
Line 65 decides trust from one read of .gtrconfig, then Line 66 re-reads the file to load commands. If the file changes between those calls, you can approve version A and execute version B. Load the phase hooks from the same immutable snapshot you hashed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks.sh` around lines 64 - 66, The code currently calls
_hooks_are_trusted on $config_file then re-reads $config_file with git config
-f, allowing a TOCTOU race; instead, create an immutable snapshot of the config
and use that single snapshot for both trust-check and loading hooks: copy
$config_file to a temp file (e.g., snapshot) immediately after the existence
check, call _hooks_are_trusted with that snapshot and then read hooks into
file_hooks with git config -f "$snapshot" --get-all "hooks.$phase"; ensure the
temp snapshot is removed after use to avoid leaks.
| # Escape cmd and path for safe embedding in sh -c strings | ||
| local safe_sh_cmd safe_sh_path | ||
| safe_sh_cmd=$(printf '%q' "$cmd") | ||
| safe_sh_path=$(printf '%q' "$path") | ||
| # Try common terminal emulators | ||
| if command -v gnome-terminal >/dev/null 2>&1; then | ||
| gnome-terminal --working-directory="$path" --title="$title" -- sh -c "$cmd; exec \$SHELL" 2>/dev/null || true | ||
| gnome-terminal --working-directory="$path" --title="$title" -- sh -c "$safe_sh_cmd; exec \$SHELL" 2>/dev/null || true | ||
| elif command -v konsole >/dev/null 2>&1; then | ||
| konsole --workdir "$path" -p "tabtitle=$title" -e sh -c "$cmd; exec \$SHELL" 2>/dev/null || true | ||
| konsole --workdir "$path" -p "tabtitle=$title" -e sh -c "$safe_sh_cmd; exec \$SHELL" 2>/dev/null || true | ||
| elif command -v xterm >/dev/null 2>&1; then | ||
| xterm -T "$title" -e "cd \"$path\" && $cmd && exec \$SHELL" 2>/dev/null || true | ||
| xterm -T "$title" -e "cd $safe_sh_path && $safe_sh_cmd && exec \$SHELL" 2>/dev/null || true | ||
| else | ||
| log_warn "No supported terminal emulator found" | ||
| return 1 | ||
| fi | ||
| ;; | ||
| windows) | ||
| # Escape for safe embedding in cmd.exe strings | ||
| local safe_win_cmd safe_win_path | ||
| safe_win_cmd=$(printf '%q' "$cmd") | ||
| safe_win_path=$(printf '%q' "$path") | ||
| # Try Windows Terminal, then fallback to cmd | ||
| if command -v wt >/dev/null 2>&1; then | ||
| wt -d "$path" "$cmd" 2>/dev/null || true | ||
| else | ||
| cmd.exe /c start "$title" cmd.exe /k "cd /d \"$path\" && $cmd" 2>/dev/null || true | ||
| cmd.exe /c start "$title" cmd.exe /k "cd /d $safe_win_path && $safe_win_cmd" 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cmd='echo hello'
safe=$(printf '%q' "$cmd")
printf 'printf %%q => %s\n' "$safe"
sh -c "$safe" || trueRepository: coderabbitai/git-worktree-runner
Length of output: 146
🏁 Script executed:
cd /tmp && find . -name "platform.sh" -type f 2>/dev/null | head -5Repository: coderabbitai/git-worktree-runner
Length of output: 58
🏁 Script executed:
fd -t f "platform.sh" | head -5Repository: coderabbitai/git-worktree-runner
Length of output: 91
🏁 Script executed:
sed -n '129,155p' lib/platform.shRepository: coderabbitai/git-worktree-runner
Length of output: 1355
printf '%q' is escaping for Bash, not for the shells you launch.
On Linux, quoting the whole cmd string turns something like echo hello into a single shell word before sh -c sees it, so the terminal tries to execute a binary literally named echo hello. On Windows, the same %q output is Bash syntax, not cmd.exe quoting, so spaced paths/args break there too. Pass argv/positional parameters to the spawned shell instead of pre-quoting the full command string.
Any multi-word command will fail to execute across all three Linux terminal emulators and both Windows execution paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/platform.sh` around lines 129 - 155, The code currently uses printf '%q'
to build safe_sh_cmd/safe_sh_path and safe_win_cmd/safe_win_path which produces
Bash-style quoting and passes the whole command as a single quoted word,
breaking multi-word commands; instead stop pre-quoting and pass the command and
path as positional parameters to the spawned shell/terminal: for
gnome-terminal/konsole/xterm invoke sh -c with a small wrapper like "cd \"$1\"
&& shift && exec \"$@\"" (pass path then the command and its args as subsequent
arguments) so the target shell receives argv properly; similarly, remove printf
'%q' for Windows paths/commands and pass the command as separate args to wt or
to cmd.exe /k (use cmd.exe to change to the directory then execute the passed
argv) so gnome-terminal/konsole/xterm, wt and cmd.exe receive the real argv
instead of a single Bash-escaped string (adjust usage around
safe_sh_cmd/safe_sh_path and safe_win_cmd/safe_win_path, and calls to
gnome-terminal, konsole, xterm, wt and cmd.exe).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I couldn’t update this branch directly because maintainer edits are disabled () and the fork rejected pushes from my account. I opened #163 from , which carries the same security changes plus fixes for the current blockers in this PR (trust hash mismatch, init cache invalidation, worktree lookup, adapter fallback regression, exclude glob regression, terminal argv handling, and added regression tests). |
|
I couldn’t update this branch directly because maintainer edits are disabled ( I opened #163 from
|
|
Ok great, we can close this PR then and use the new one. |
2 participants
Security Remediation Summary
Based on automated security scan findings from Thresher (scan date: 2026-04-03), this PR resolves 0 Critical, 4 High, and 6 Medium severity findings — including hook trust model hardening, shell injection fixes, CI/CD supply chain hardening, and code-level security improvements.
Dependency Upgrades (0 packages)
No package-manager dependencies exist in this project. The entire dependency surface consists of GitHub Actions used only in CI.
Application Security Fixes
High
lib/hooks.sh,lib/commands/init.sh— Hooks from.gtrconfigfiles (committed to repositories) now require explicit user approval viagit gtr trustbefore execution. Trust is cached per content hash in~/.config/gtr/trusted/. Applies to all hook execution paths:run_hooks(),run_hooks_export(), and postCd hooks in generated shell functions (bash, zsh, fish).lib/adapters.sh— Replacedeval "$GTR_EDITOR_CMD"andeval "$GTR_AI_CMD"with array-based dispatch (read -ra+"${arr[@]}"). Added shell metacharacter validation (;,`,$(,|,&,>,<) for config-supplied command names in the generic fallback path.Medium
lib/adapters.sh— Added validation in_load_adapter()to reject adapter names containing/,.., or\before constructing file paths..github/workflows/lint.yml,.github/workflows/homebrew.yml— Pinnedactions/checkoutandmislav/bump-homebrew-formula-actionto full commit SHAs. Addedpermissions: read-allat the top level of both workflow files.lib/copy.sh— Added validation to reject overly broad exclude suffixes (*,**,.*). Added.gitdirectory protection in the exclude loop to prevent accidental removal.lib/platform.sh— Appliedprintf '%q'escaping to$cmdand$pathfor Linux and Windows terminal emulator invocations.lib/config.sh— Replacedeval "value=\${${env_name}:-}"withprintenv "$env_name"to eliminate eval from the config path.Low
lib/ui.sh— Replacedeval "$var_name=\"$input\""withprintf -v "$var_name" '%s' "$input".lib/args.sh— Replaced manual IFS save/restore withlocal IFS='|'for automatic scoping.Secrets Remediation
GitHub Actions Hardening
actions/checkout@v4to SHA34e114876b0b11c390a56381ad16ebd13914f8d5in lint.yml (3 occurrences)mislav/bump-homebrew-formula-action@v3to SHA56a283fa15557e9abaa4bdb63b8212abc68e655cin homebrew.ymlpermissions: read-allto both lint.yml and homebrew.yml to enforce least-privilege token accessScan Context
Known Remaining Items (not addressed in this PR)
These were flagged by AI analysts but require deeper architectural changes or stakeholder decisions:
lib/copy.sh) — requires microsecond race window + local write access; extremely low risklib/commands/init.sh) — cache contains only shell function definitions, not sensitive dataevalfor execution — the trust model gates access but does not sandbox execution; a future sandboxing mechanism could further reduce riskSummary by CodeRabbit
New Features
git gtr trustto review and approve repository-defined hooks; shell completions and help text updated.Bug Fixes
Documentation
Chores