diff --git a/docs/configuration.md b/docs/configuration.md index 3073533..59a62e6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -331,6 +331,16 @@ symlink itself removes only the link, including when the pattern ends in `/`; the link target is preserved. Exclusions are skipped when the copied directory or one of its path components is a symlink. +If directory copying fails, `git gtr new` and `git gtr pr` stop before running +post-create hooks or automatically launching tools. The created worktree is +left in place for inspection and cleanup. + +Keep copy sources, destination worktrees, and their parent directories writable +only by trusted users. A user who can replace a worktree path or modify a copied +tool can change what a hook executes. Hook approval covers command definitions; +it does not pin the contents of delegated scripts or dependencies. Copy path +checks cannot isolate execution from other users who can mutate those paths. + > [!WARNING] > Dependency directories may contain sensitive files (credentials, tokens, cached secrets). Always use `gtr.copy.excludeDirs` to exclude sensitive subdirectories if needed. diff --git a/lib/commands/copy.sh b/lib/commands/copy.sh index 4e49da7..53d999a 100644 --- a/lib/commands/copy.sh +++ b/lib/commands/copy.sh @@ -95,11 +95,15 @@ cmd_copy() { if [ "$dry_run" -eq 1 ]; then log_step "[dry-run] Would copy to: $dst_branch" [ -n "$patterns" ] && copy_patterns "$src_path" "$dst_path" "$patterns" "$excludes" "true" "true" - [ -n "$resolved_dirs" ] && _copy_resolved_directories "$src_path" "$dst_path" "$resolved_dirs" "$dir_excludes" "true" + if [ -n "$resolved_dirs" ]; then + _copy_resolved_directories "$src_path" "$dst_path" "$resolved_dirs" "$dir_excludes" "true" || return 1 + fi else log_step "Copying to: $dst_branch" [ -n "$patterns" ] && copy_patterns "$src_path" "$dst_path" "$patterns" "$excludes" "true" - [ -n "$resolved_dirs" ] && _copy_resolved_directories "$src_path" "$dst_path" "$resolved_dirs" "$dir_excludes" + if [ -n "$resolved_dirs" ]; then + _copy_resolved_directories "$src_path" "$dst_path" "$resolved_dirs" "$dir_excludes" || return 1 + fi fi copied_any=1 done diff --git a/lib/commands/create.sh b/lib/commands/create.sh index 4fca228..e57c17a 100644 --- a/lib/commands/create.sh +++ b/lib/commands/create.sh @@ -14,7 +14,7 @@ _post_create_copy() { if [ -n "$includes" ]; then log_step "Copying files..." - copy_patterns "$repo_root" "$worktree_path" "$includes" "$excludes" + copy_patterns "$repo_root" "$worktree_path" "$includes" "$excludes" || return 1 fi # Copy directories (typically git-ignored dirs like node_modules, .venv) @@ -24,7 +24,7 @@ _post_create_copy() { if [ -n "$dir_includes" ]; then log_step "Copying directories..." - copy_directories "$repo_root" "$worktree_path" "$dir_includes" "$dir_excludes" + copy_directories "$repo_root" "$worktree_path" "$dir_includes" "$dir_excludes" || return 1 fi } @@ -261,7 +261,10 @@ _cmd_create_impl() { # Copy files based on patterns if [ "$skip_copy" -eq 0 ]; then - _post_create_copy "$repo_root" "$worktree_path" + if ! _post_create_copy "$repo_root" "$worktree_path"; then + log_error "Worktree created at $worktree_path, but copying failed; hooks and tools were not started" + exit 1 + fi fi # Run post-create hooks (unless --no-hooks) diff --git a/lib/commands/pr.sh b/lib/commands/pr.sh index 9d2b012..20087fb 100644 --- a/lib/commands/pr.sh +++ b/lib/commands/pr.sh @@ -396,7 +396,10 @@ cmd_pr() { fi if [ "$skip_copy" -eq 0 ]; then - _post_create_copy "$repo_root" "$worktree_path" + if ! _post_create_copy "$repo_root" "$worktree_path"; then + log_error "Worktree created at $worktree_path, but copying failed; hooks and tools were not started" + return 1 + fi fi if [ "$skip_hooks" -eq 0 ]; then diff --git a/lib/copy.sh b/lib/copy.sh index 4ca636e..f9410e4 100644 --- a/lib/copy.sh +++ b/lib/copy.sh @@ -49,6 +49,20 @@ _copy_discovery_records() { done } +_copy_move_os="" + +# Plain mv treats a destination symlink to a directory as a container. The +# destination can change while copying to staging, so checking -L beforehand +# is insufficient. Use the platform's no-follow form when publishing entries. +# This protects the final component, not ancestors writable by other users. +_copy_move_entry() { + [ -n "$_copy_move_os" ] || _copy_move_os=$(detect_os) + case "$_copy_move_os" in + darwin) mv -fh -- "$1" "$2" ;; + *) mv -fT -- "$1" "$2" ;; + esac +} + # Merge without following existing destination links. Stage each leaf in its # destination directory, then rename it so existing hard links are not written. _copy_tree_entry() { @@ -77,7 +91,7 @@ _copy_tree_entry() { return 1 fi fi - if mv -f -- "$staged/leaf" "$dest"; then + if _copy_move_entry "$staged/leaf" "$dest"; then rmdir "$staged" return 0 fi @@ -192,7 +206,7 @@ _fast_copy_dir() { rm -rf -- "$staging" return 1 fi - if mv -- "${staging:?}/${src##*/}" "$target"; then + if _copy_move_entry "${staging:?}/${src##*/}" "$target"; then rmdir "$staging" else rm -rf -- "$staging" @@ -699,6 +713,7 @@ EOF # Copy already-resolved directories from a source root. # Usage: _copy_resolved_directories src_root dst_root resolved_dirs excludes [dry_run] +# Returns nonzero if any directory copy fails; callers must guard the status. _copy_resolved_directories() { local src_root="$1" local dst_root="$2" @@ -718,7 +733,7 @@ _copy_resolved_directories() { old_pwd=$(pwd) cd "$src_root" || return 1 - local copied_count=0 + local copied_count=0 failed=0 local dir_path while IFS= read -r dir_path; do [ -z "$dir_path" ] && continue @@ -741,7 +756,10 @@ _copy_resolved_directories() { continue fi - mkdir -p "$dest_parent" + if ! mkdir -p "$dest_parent"; then + failed=1 + continue + fi if { if _has_subdir_excludes "$dir_path" "$excludes"; then _selective_copy_dir "$dir_path" "$dst_root" "$excludes" @@ -752,6 +770,7 @@ _copy_resolved_directories() { copied_count=$((copied_count + 1)) else log_warn "Failed to copy directory $dir_path" + failed=1 fi done < "$TEST_REPO/deps/file" + git config --add gtr.copy.includeDirs deps + mv() { return 1; } + + run cmd_copy copy-target + + [ "$status" -ne 0 ] + [ ! -e "$TEST_WORKTREES_DIR/copy-target/deps" ] +} + @test "cmd_copy dry-run does not copy configured includeDirs" { mkdir -p "$TEST_REPO/.zed" echo "settings" > "$TEST_REPO/.zed/settings.json" diff --git a/tests/cmd_create_integration.bats b/tests/cmd_create_integration.bats index d6b0361..f95146b 100644 --- a/tests/cmd_create_integration.bats +++ b/tests/cmd_create_integration.bats @@ -129,6 +129,47 @@ _cmd_create_capture_stdout() { [ ! -s "$stdout_file" ] } +@test "cmd_create stops before hooks when a directory publication races with a symlink" { + mkdir -p "$TEST_REPO/deps" "$TEST_REPO/outside" + printf 'echo expected > hook-marker\n' > "$TEST_REPO/deps/tool" + printf 'echo redirected > hook-marker\n' > "$TEST_REPO/outside/tool" + git config --add gtr.copy.includeDirs deps + git config --add gtr.hook.postCreate 'sh deps/tool' + local race_worktree="$TEST_WORKTREES_DIR/copy-race" + local stdout_file="$TEST_REPO/create-stdout" + mv() { + local last + for last in "$@"; do :; done + if [ "$last" = "$race_worktree/deps" ]; then + ln -s "$TEST_REPO/outside" "$race_worktree/deps" + fi + command mv "$@" + } + + run _cmd_create_capture_stdout "$stdout_file" copy-race --from HEAD --no-fetch --porcelain + + [ "$status" -ne 0 ] + [ -L "$race_worktree/deps" ] + [ ! -e "$race_worktree/hook-marker" ] + [ ! -e "$TEST_REPO/outside/deps" ] + [ ! -s "$stdout_file" ] + [[ "$output" == *"copying failed"* ]] +} + +@test "cmd_create runs a copied tool once after successful directory publication" { + mkdir -p "$TEST_REPO/deps" + printf 'echo expected >> hook-marker\n' > "$TEST_REPO/deps/tool" + git config --add gtr.copy.includeDirs deps + git config --add gtr.hook.postCreate 'sh deps/tool' + + run cmd_create copy-success --from HEAD --no-fetch --porcelain + + [ "$status" -eq 0 ] + [ ! -L "$TEST_WORKTREES_DIR/copy-success/deps" ] + [ "$(cat "$TEST_WORKTREES_DIR/copy-success/hook-marker")" = expected ] + [[ "$output" == *$'hook_status\tran'* ]] +} + @test "cmd_create creates worktree with --track none" { cmd_create track-none --from HEAD --track none --no-fetch --yes [ -d "$TEST_WORKTREES_DIR/track-none" ] diff --git a/tests/cmd_pr.bats b/tests/cmd_pr.bats index e6957d4..ed703ad 100644 --- a/tests/cmd_pr.bats +++ b/tests/cmd_pr.bats @@ -40,6 +40,21 @@ teardown() { [ "$(git config branch.feature/from-pr.merge)" = "refs/heads/feature/from-pr" ] } +@test "cmd_pr does not run postCreate after a directory copy fails" { + mkdir -p "$TEST_REPO/deps" + echo value > "$TEST_REPO/deps/file" + git config --add gtr.copy.includeDirs deps + git config --add gtr.hook.postCreate 'touch hook-marker' + mv() { return 1; } + + run cmd_pr 123 --remote origin --yes + + [ "$status" -ne 0 ] + [ -d "$TEST_WORKTREES_DIR/feature-from-pr" ] + [ ! -e "$TEST_WORKTREES_DIR/feature-from-pr/hook-marker" ] + [[ "$output" == *"copying failed"* ]] +} + @test "cmd_pr treats an option-shaped remote as a fetch operand" { # Configure the name directly because `git remote add` rejects leading dashes. git config remote.--all.url "$TEST_REMOTE_REPO" diff --git a/tests/copy_safety.bats b/tests/copy_safety.bats index c47a384..f0a3459 100644 --- a/tests/copy_safety.bats +++ b/tests/copy_safety.bats @@ -670,7 +670,8 @@ teardown() { echo private > "$outside/secret" echo replacement > "$src/vendor/cache/secret" ln -s "$outside" "$dst/vendor/cache" - copy_directories "$src" "$dst" vendor '' + run copy_directories "$src" "$dst" vendor '' + [ "$status" -ne 0 ] [ "$(cat "$outside/secret")" = private ] } @@ -828,3 +829,63 @@ SH [ "$status" -ne 0 ] [ ! -e "$dst/cache" ] } + +@test "directory publication does not follow a symlink inserted at the move boundary" { + _test_tmpdir=$(mktemp -d) + local src="$_test_tmpdir/source space" dst="$_test_tmpdir/destination space" + local outside="$_test_tmpdir/outside" + mkdir -p "$src/cache" "$dst" "$outside" + echo value > "$src/cache/file" + echo sentinel > "$outside/keep" + # Deterministically schedule the other writer after all pre-copy checks. + # The actual system mv still performs the move with the production options. + mv() { + ln -s "$outside" "$dst/cache" + command mv "$@" + } + + run _fast_copy_dir "$src/cache" "$dst/" + + [ "$status" -ne 0 ] + [ -L "$dst/cache" ] + [ ! -e "$outside/cache" ] + [ "$(cat "$outside/keep")" = sentinel ] + [ "$(find "$dst" -name '.gtr-copy.*' | wc -l | tr -d ' ')" -eq 0 ] +} + +@test "directory copy failures reach the caller and restore its working directory" { + _test_tmpdir=$(mktemp -d) + local src="$_test_tmpdir/src" dst="$_test_tmpdir/dst" + mkdir -p "$src/cache" "$dst" + echo value > "$src/cache/file" + mv() { return 1; } + local before result=0 + before=$(pwd) + + copy_directories "$src" "$dst" cache '' || result=$? + + [ "$result" -ne 0 ] + [ "$(pwd)" = "$before" ] + [ ! -e "$dst/cache" ] + [ "$(find "$dst" -name '.gtr-copy.*' | wc -l | tr -d ' ')" -eq 0 ] +} + +@test "staged leaf publication replaces a raced symlink without writing its referent" { + _test_tmpdir=$(mktemp -d) + local src="$_test_tmpdir/src" dst="$_test_tmpdir/dst" + local outside="$_test_tmpdir/outside" + mkdir -p "$src" "$dst" "$outside" + echo value > "$src/file" + echo sentinel > "$outside/leaf" + mv() { + ln -s "$outside" "$dst/file" + command mv "$@" + } + + _copy_tree_entry "$src/file" "$dst/file" + + [ ! -L "$dst/file" ] + [ "$(cat "$dst/file")" = value ] + [ "$(cat "$outside/leaf")" = sentinel ] + [ "$(find "$dst" -name '.gtr-copy.*' | wc -l | tr -d ' ')" -eq 0 ] +}