Skip to content

fix: reject path separators in worktree prefixes - #219

Merged
shiqui merged 2 commits into
mainfrom
codex/vuln-605-prefix-policy
Nov 4, 2026
Merged

shiqui merged 2 commits into
mainfrom
codex/vuln-605-prefix-policy

Conversation

@shiqui

@shiqui shiqui commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Description

Treat gtr.worktrees.prefix as a folder-name prefix by rejecting POSIX and Windows path separators before worktree operations. Previously, ../planted- plus branch feature/auth created a worktree beside the configured base directory; dev- correctly created dev-feature-auth inside it.

Validate the effective prefix after configuration precedence, and guard direct creation and target-resolution helpers. The shared context check also covers native/fallback PR checkout and rename. Empty, dotted, and spaced prefixes remain supported.

Motivation

Related: VULN-605.

The behavior reproduces on upstream commit 2d4852d97d28fe9d51fbd2b877555c6292a50e3b, including an untrusted committed .gtrconfig prefix with a locally configured worktree directory. Disposable controls confirmed that occupied files and directories are refused without overwriting their contents. The demonstrated impact is unintended worktree placement; execution and cross-user impact were not established.

Directory placement remains the responsibility of gtr.worktrees.dir. This change does not alter its configuration authority or fix the separate identifier-selection behavior in resolve_target.

Type of Change

  • Bug fix
  • Breaking change for prefixes containing path separators
  • Documentation update

Testing

  • Added 11 tests for prefix preservation and rejection, configuration sources and precedence, porcelain output, direct resolution, rename, and PR checkout. The native checkout success test now also exercises an ordinary prefix.
  • All six rejection regressions fail against the untouched baseline.
  • Full BATS suite under Bash 5.3: 614/616 passed initially. The two Fish helper failures also reproduced on baseline because the sandbox prevented Fish from writing its data/cache directories; both passed when rerun with temporary writable XDG directories.
  • Bash 3.2: all 13 prefix-focused tests passed, plus isolated new, go, list, run, mv, and rm smoke tests.
  • ShellCheck, completion-generation check, and git diff --check passed.

Manual Testing

Tested on macOS with Git 2.50.1. Linux, Windows Git Bash, and Git 2.17 were not exercised.

  1. In disposable repositories, compare empty and dev- prefixes with ../planted-, group/../../planted-, nested, and backslash-containing prefixes.
  2. Confirm ordinary prefixes retain their exact names and valid porcelain output through creation and lifecycle operations.
  3. Confirm invalid prefixes fail before creating destinations or branches, emit no porcelain success records, and leave rename sources intact.

Breaking Changes

  • This PR introduces breaking changes.
  • Migration guidance is included in the configuration documentation.

Prefixes such as team/dev- are now rejected. Use prefix dev- and a gtr.worktrees.dir ending in team. Existing worktrees are not relocated automatically.

Checklist

  • Read CONTRIBUTING.md and followed the project's Bash and error-handling conventions.
  • Performed manual testing on macOS.
  • Updated README, configuration documentation, and CLI help.
  • Verified generated completions are unchanged.
  • Introduced no external dependencies.

Additional Context

Three existing SSH URL-normalization tests fail under Bash 3.2 on both baseline and patched code, but pass under Bash 5.3. They are outside this prefix-validation change.

License Acknowledgment

This contribution is made under the terms of the Apache License 2.0.

Summary by CodeRabbit

  • New Features
    • Worktree prefixes are applied literally to folder names, including empty prefixes and prefixes with spaces or dots. Use the worktree directory setting to choose the parent folder.
  • Bug Fixes
    • Prefixes containing / or \ are rejected to prevent invalid paths during worktree creation, checkout, and rename.
  • Documentation
    • Clarified prefix rules and configuration options in the documentation and help text.

@linear

linear Bot commented Nov 4, 2026

Copy link
Copy Markdown

VULN-605

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The CLI rejects worktree prefixes containing / or \ before worktree operations. Empty, dotted, spaced, and ordinary prefixes remain supported. Documentation directs users to gtr.worktrees.dir to select the parent directory.

Changes

Worktree prefix handling

Layer / File(s) Summary
Prefix contract and validation
README.md, docs/configuration.md, lib/commands/help.sh, lib/core.sh, tests/core_*
Documentation defines the prefix restrictions and parent-directory setting. Core helpers reject prefixes containing path separators. Core tests cover accepted prefixes and rejection during creation and target resolution.
Command integration coverage
tests/cmd_create_integration.bats, tests/cmd_pr.bats, tests/cmd_rename.bats
Tests cover configuration sources and precedence, creation, PR checkout, and rename. They check that invalid prefixes fail before the tested command side effects.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: natoboram, scarf005

Merge Risk: Minimal · up to 3f18f

The change consistently rejects unsafe path separators before worktree operations while preserving supported prefix behavior, so it is mergeable with normal checks.

Security architecture risk: Low · up to 3f18f

The new validation blocks path-bearing prefixes before worktree mutations. Existing configurations containing separators must be corrected before normal worktree commands can resume. No introduced security vulnerability was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reviewed boundary concerns repository configuration influencing local worktree placement and Git metadata operations under the invoking user's authority. The inspected change narrows prefix-derived path influence; it does not establish execution, cross-user exposure, or comprehensive confinement of directory configuration.

Security Findings and Attack Paths

  • inferred — A separator-containing prefix selected from repository configuration can no longer reach the inspected prefix-based creation and checkout sinks: validation returns failure first. This conclusion is limited to the prefix channel and does not assert that other path-selection inputs are safe.

Trust Boundaries and Controls

  • observed — The routed public-entrypoint ranges are test code, not newly exposed production APIs. Their arguments select existing checkout modes, and TEST_GH_LOG observes a mock. The resolver regression asserts rejection even when the traversal destination exists; it does not add a production bypass.
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 "fix: reject path separators in worktree prefixes" matches the main change. The changeset adds _validate_worktree_prefix in lib/core.sh, which rejects gtr.worktrees.prefix values contain…
Docstring Coverage Passed Docstring coverage is 100.00% 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. Documentat…
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 "fix: reject path separators in worktree prefixes" matches the main change. The changeset adds _validate_worktree_prefix in lib/core.sh, which rejects gtr.worktrees.prefix values containing / or \. It calls this validation from resolve_target, create_worktree, and resolve_repo_context. The docs, help text, and tests all cover the same rejection. The title is concise, specific, and uses a conventional commit prefix. It is a single sentence with no file list or vague wording.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% 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 4 functions across 7 files. (1 skipped: 1 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 each path with care,
No slash can lead the worktree there.
A dotted prefix stays in place,
And spaces keep their chosen space.
The parent path sets where roots grow,
While safe new worktrees come and go.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between b5b3bd5 and 3f18fdb.

Files selected for processing: 5
  • README.md
  • lib/commands/help.sh
  • lib/core.sh
  • tests/cmd_pr.bats
  • tests/core_resolve_target.bats
Files skipped from review as they are similar to previous changes: 1
  • lib/commands/help.sh

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

@shiqui
shiqui merged commit 588b09c into main Nov 4, 2026
5 checks passed
@shiqui
shiqui deleted the codex/vuln-605-prefix-policy branch November 4, 2026 18:51
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