Skip to content

fix: reject unsafe Git configuration before running GTR - #225

Merged
shiqui merged 2 commits into
mainfrom
codex/fix-shared-git-config
Nov 4, 2026
Merged

shiqui merged 2 commits into
mainfrom
codex/fix-shared-git-config

Conversation

@shiqui

@shiqui shiqui commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Description

Reject unsafe native Git configuration before GTR uses it. For example, a group-writable .git/config containing gtr.hook.postCreate and gtr.worktrees.dir previously ran the hook and created a worktree at the supplied destination. It now produces an error before creating the branch, directory, or hook marker.

The runtime validator checks ownership, group/other write permissions, macOS write-capable ACLs, included files, symlinks, and parent directories. Protected configuration retains its existing behavior. CLI startup and hook dispatch enforce the check, and generated Bash/Zsh/Fish helpers embed the same validator. Shell cache schema 9 replaces older generated helpers when integration is refreshed.

Motivation

A shared writable configuration file must not implicitly authorize another OS user's GTR hooks or destination settings. This introduces an actual runtime refusal for that case.

Type of Change

  • Breaking change: unsafe configuration is now rejected.
  • Documentation update.
  • Other: security regressions.

Testing

Tested on macOS with Git 2.50.1, Bash 3.2.57 and 5.3.20, Zsh 5.9, and Fish 4.9.3.

  • All 21 new security regressions pass, including under system Bash 3.2.
  • Before the fix, the CLI attack fixture and all three generated-shell attack fixtures failed their security assertions; the injected commands ran.
  • All 732 BATS tests pass across 33 test files, using isolated Git configuration and XDG paths.
  • ShellCheck, completion generation, and whitespace checks pass.
  • Manual smoke passed for create, list, go, editor/AI launch using harmless stubs, remove, and config set/get/unset. Each lifecycle hook ran exactly once.
  • Cache tests cover replacement of schema 8 as well as older helpers.

Linux CI and the macOS Bash 3.2 CI check pass. Windows/Git Bash and a live second-UID fixture have not been tested; regression fixtures use disposable repositories and controlled permissions, with no account or system permission changes.

Breaking Changes

Group/other-writable Git configuration, unsafe parent paths, foreign ownership, and macOS write-capable ACL grants stop GTR. Fix the reported path's ownership/permissions or use a private clone. The documentation includes migration guidance; git gtr trust does not override the filesystem check. Reload shell setup after upgrading to replace already-loaded old functions.

Checklist

  • Followed project Bash style and Bash 3.2 compatibility.
  • Added runtime regression coverage and documentation.
  • Performed isolated manual testing on macOS.
  • Added no commands or flags requiring completion changes.
  • Noted validation limits and compatibility changes.

License Acknowledgment

This contribution is made under the Apache License 2.0.

Summary by CodeRabbit

  • Security
    • GTR now checks Git configuration files and their included files, symlink targets, and parent directories for unsafe ownership or write permissions. Commands and hook execution stop when configuration cannot be verified or is unsafe.
    • Shell integrations recheck configuration safety before running post-directory-change hooks.
  • Documentation
    • Updated configuration guidance with the safety checks, their impact on shared configurations, and steps to address rejected settings. git gtr trust does not bypass these checks.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

GTR now validates native Git configuration ownership and write access before using configuration. CLI startup, hook dispatch, and generated Bash, Zsh, and Fish helpers enforce the checks. The shell integration cache schema is now 9. Tests, CI setup, and documentation cover the new behavior.

Changes

Git configuration safety

Layer / File(s) Summary
Configuration validation
lib/config_safety.sh
Adds checks for ownership, write permissions, macOS write-capable ACLs, symlinks, parent directories, configuration origins, and included files.
Runtime enforcement and generated shell helpers
bin/git-gtr, lib/hooks.sh, lib/commands/init.sh
CLI startup and hook dispatch stop when configuration checks fail. Generated Bash, Zsh, and Fish helpers check configuration after changing directories and before post-cd hooks. The init cache schema changes to 9.
Regression coverage and guidance
tests/config_safety.bats, tests/init.bats, .github/workflows/lint.yml, README.md, docs/configuration.md
Tests cover unsafe and protected configurations and cache updates. CI isolates Git and XDG settings. Documentation describes the checks, failure behavior, and remediation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant CLI as git-gtr
  participant Check as _gtr_config_check
  participant Git
  participant Hooks as run_hooks
  User->>CLI: Run a GTR command
  CLI->>Check: Validate configuration
  Check->>Git: Read configuration sources and includes
  Git-->>Check: Return configuration origins and paths
  Check-->>CLI: Return validation result
  CLI->>Hooks: Dispatch configured hooks
  Hooks->>Check: Recheck configuration
  Check->>Git: Read configuration for the current worktree
  Git-->>Check: Return configuration origins and paths
  Check-->>Hooks: Return validation result
Loading

Suggested reviewers: natoboram

Merge Risk: Minimal · up to 18f09

Group-writable repositories will require the documented permission change. No actionable merge-blocking issue remains on the supplied evidence.

Security architecture risk: Low · up to 18f09

The change strengthens configuration trust checks without establishing a newly introduced security weakness in the inspected paths. Existing shells must reload their integration to receive the protection. Incomplete coverage and unverified interruption behavior leave limited residual uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The protected boundary is local OS-user authority: native configuration can select worktree destinations and executable commands. Successful command execution would inherit the invoking process's accessible files, credentials, and privileges; this change does not create a sandbox or tenant-isolation boundary.

Security Findings and Attack Paths

  • observed — Regression fixtures model writable native configuration injecting hooks and redirecting worktree creation. They assert refusal without a hook marker, destination directory, or new branch, and separately exercise refreshed Bash, Zsh, and Fish helpers after configuration becomes writable. These are inspected test assertions, not independently executed results.

Trust Boundaries and Controls

  • observed — Process-supplied configuration authority is distinct from filesystem authority. Environment selectors determine checked configuration locations, while command-line and standard-input origins are accepted. Repository .gtrconfig hooks still require their separate definition-bound approval; that approval does not bypass native configuration validation.

Resilience and Maintainability Implications

  • inferred — Revalidation at hook collection reduces stale startup trust when dispatch enters another worktree. Validation and later Git reads are nevertheless separate operations, so the inspected design relies on protected ownership and paths rather than an immutable configuration snapshot. No newly introduced cross-user race exploit was established.

Hardening Proposals

  • proposed — Consider atomic cache replacement and interrupted-refresh recovery checks so future security-helper upgrades cannot leave a stamp-bearing partial cache. This hardens a preexisting cache mechanism; it is not an observed exploit introduced by this PR.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main change: GTR now rejects unsafe Git configuration before it runs. The raw summary and objectives confirm that the runtime validator, startup check, hook dispatch, and generated…
Docstring Coverage Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: GTR now rejects unsafe Git configuration before it runs. The raw summary and objectives confirm that the runtime validator, startup check, hook dispatch, and generated shell helpers all enforce this rule. The fix: prefix matches the described regression fix for group-writable .git/config. The title is concise, single-sentence, and specific enough for a teammate scanning history.

Full details: Docstring Coverage

Explanation

Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 5 files. (4 skipped: 4 unsupported.)

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the config trail,
For writable paths that might prevail.
Safe hooks wait until checks are through,
Fresh shell helpers join them too.
Then hops proceed on paths secure.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 7077fa9 and 18f0914.

Files selected for processing: 9
  • .github/workflows/lint.yml
  • README.md
  • bin/git-gtr
  • docs/configuration.md
  • lib/commands/init.sh
  • lib/config_safety.sh
  • lib/hooks.sh
  • tests/config_safety.bats
  • tests/init.bats

Included review availability: This review used your included allowance. Your plan provides up to 100 included reviews per hour; 98 remain after this review.

@shiqui
shiqui marked this pull request as ready for review November 4, 2026 20:09
@shiqui
shiqui merged commit ce66bb3 into main Nov 4, 2026
5 checks passed
@shiqui
shiqui deleted the codex/fix-shared-git-config branch November 4, 2026 20:53
@linear

linear Bot commented Nov 4, 2026

Copy link
Copy Markdown

VULN-601

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant