fix(sandbox): TOCTOU race condition in git_worktree.py #8178

Merged
HAL9000 merged 13 commits from fix/toctou-race-git-worktree into master 2026-04-28 04:22:33 +00:00
8 changed files with 411 additions and 7 deletions
+12
View File
1
@@ -30,6 +30,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
project milestone planning. Includes BDD test coverage for the new workflow
documentation and permission configuration.
- **Git Worktree TOCTOU Race Condition** (#7507): Fixed a Time-Of-Check-To-Time-Of-Use
(TOCTOU) race condition in `git_worktree.py` that could cause `git worktree add`
operations to fail under concurrent execution. The fix replaces the unsafe
`mkdtemp()` + `rmdir()` pattern with a parent-directory approach that maintains
the OS-level uniqueness guarantee throughout the entire operation. The parent
temporary directory is now persisted and properly cleaned up on both success and
failure paths. Comprehensive BDD test coverage validates the fix under concurrent
execution and confirms proper cleanup behavior.
### Fixed
- **Atomic `server_connect` config writes** (#993): Fixed `server_connect` in
@@ -315,6 +324,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
are also protected. The DI container registration as `providers.Singleton`
is now correct and safe.
- **TOCTOU Race Condition in Git Worktree Sandbox** (#7507): Fixed Time-Of-Check-To-Time-Of-Use race condition in `GitWorktreeSandbox.create()` by replacing unsafe mkdtemp+rmdir pattern with persistent parent directory approach. Parent directory is now held throughout operation lifetime and properly cleaned up in all error paths (timeout, CalledProcessError, OSError) and in the cleanup() method, eliminating race window where another process could claim the worktree path. Comprehensive BDD coverage added for all error-path cleanup branches.
- **Validation Gate Empty-Run Guard** (#7508): Fixed `ApplyValidationSummary.all_required_passed`
returning `True` when zero validations were run, silently bypassing the apply gate. The property
now returns `False` when the validation result set is empty (`is_empty` is `True`), ensuring
@@ -400,3 +411,4 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
renders permission requests directly in the conversation stream for single-file
operations. Users can allow/reject with single-key shortcuts (`a`/`A`/`r`/`R`),
navigate with arrow keys, confirm with `Enter`, or press `v` to open the full
+1 -1
View File
1
@@ -24,4 +24,4 @@ Below are some of the specific details of various contributions.
* HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system.
* HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559).
* HAL 9000 has contributed the architecture-pool-supervisor milestone assignment feature (PR #8188 / issue #7521): added `forgejo_update_pull_request` permission and documented the PR workflow for major spec changes, enabling automatic milestone assignment for specification PRs.
* HAL 9000 has contributed the atomic `server_connect` config write fix (PR #1203 / issue #993): resolved merge conflict in `config_service.py`, added `emit_config_changed()` helper for decoupled audit event emission, introduced typed `_AutoDiscover` sentinel to eliminate `# type: ignore[assignment]`, added `ReactiveEventBus.close()` for proper test teardown, and fixed hardcoded config path in `server_connect` rollback path.
* HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations.
@@ -106,3 +106,44 @@ Feature: Git worktree sandbox coverage boost
And gwtcb _run_git is mocked to fail on branch delete
When gwtcb cleanup is called
Then the gwtcb sandbox should be in the "cleaned_up" state
# --- create: timeout after parent dir is created (TOCTOU fix coverage) ---
Scenario: Create cleans up parent dir on timeout during git worktree add
Given a gwtcb sandbox with mocked _run_git that times out on worktree add
When gwtcb create is called expecting a timeout error
Then a gwtcb SandboxCreationError should be raised with message "timed out"
And the gwtcb sandbox should be in the "errored" state
# --- create: CalledProcessError after parent dir is created (TOCTOU fix coverage) ---
Scenario: Create cleans up parent dir on CalledProcessError during git worktree add
Given a gwtcb sandbox with mocked _run_git that fails on worktree add
When gwtcb create is called expecting a process error
Then a gwtcb SandboxCreationError should be raised
And the gwtcb sandbox should be in the "errored" state
# --- commit: timeout with parent dir set (TOCTOU fix coverage) ---
Scenario: Commit cleans up parent dir on timeout
Given a gwtcb sandbox in ACTIVE state ready to commit with parent dir set
And gwtcb _run_git is mocked to raise TimeoutExpired on commit
When gwtcb commit is called expecting a timeout error
Then a gwtcb SandboxCommitError should be raised with message "timed out"
And the gwtcb sandbox should be in the "errored" state
# --- rollback: timeout with parent dir set (TOCTOU fix coverage) ---
Scenario: Rollback cleans up parent dir on timeout
Given a gwtcb sandbox in ACTIVE state ready to rollback with parent dir set
And gwtcb _run_git is mocked to raise TimeoutExpired on rollback
When gwtcb rollback is called expecting a timeout error
Then a gwtcb SandboxRollbackError should be raised with message "timed out"
And the gwtcb sandbox should be in the "errored" state
# --- cleanup: OSError when removing parent dir (TOCTOU fix coverage) ---
Scenario: Cleanup continues when parent dir removal raises OSError
Given a gwtcb sandbox with a parent dir that raises OSError on removal
When gwtcb cleanup is called
Then the gwtcb sandbox should be in the "cleaned_up" state
@@ -0,0 +1,89 @@
Feature: Git worktree TOCTOU race condition fix
As a security-conscious developer
I want the git worktree sandbox to avoid TOCTOU race conditions
So that another process cannot claim the worktree path between deletion and git creation
Background:
Given a gwt test git repository is initialised
# --- TOCTOU Race Condition Tests ---
Scenario: Worktree path is created atomically without race window
When a gwt sandbox is created for plan "plan-toctou-001"
Then the gwt sandbox worktree path should exist
And the gwt sandbox should be in the "created" state
And the gwt sandbox context should reference plan "plan-toctou-001"
Scenario: Worktree path uses parent directory approach
When a gwt sandbox is created for plan "plan-toctou-002"
Then the gwt sandbox worktree path should exist
And the gwt toctou sandbox should have a parent temp directory
And the gwt toctou worktree should be a subdirectory of its parent
Scenario: Multiple concurrent sandboxes do not collide
When a gwt sandbox is created for plan "plan-toctou-concurrent-1"
And the gwt toctou sandbox is saved as "first"
And a gwt sandbox is created for plan "plan-toctou-concurrent-2"
And the gwt toctou sandbox is saved as "second"
Then the gwt toctou saved sandbox "first" worktree path should exist
And the gwt toctou saved sandbox "second" worktree path should exist
And the gwt toctou saved sandbox paths "first" and "second" should differ
Scenario: Rapid successive worktree creations all succeed
When a gwt sandbox is created for plan "plan-toctou-rapid-1"
And the gwt toctou sandbox is saved as "r1"
And a gwt sandbox is created for plan "plan-toctou-rapid-2"
And the gwt toctou sandbox is saved as "r2"
And a gwt sandbox is created for plan "plan-toctou-rapid-3"
And the gwt toctou sandbox is saved as "r3"
Then the gwt toctou saved sandbox "r1" worktree path should exist
And the gwt toctou saved sandbox "r2" worktree path should exist
And the gwt toctou saved sandbox "r3" worktree path should exist
Scenario: Worktree path isolation is maintained across operations
When a gwt sandbox is created for plan "plan-toctou-isolation"
And a gwt file "test.txt" is created in the worktree with content "isolation test"
And the gwt path "test.txt" is resolved
And the gwt sandbox is committed with message "isolation test commit"
Then the gwt commit result should indicate success
And the gwt file "test.txt" should exist in the original repo
Scenario: Cleanup properly removes parent directory structure
When a gwt sandbox is created for plan "plan-toctou-cleanup"
And the gwt toctou parent directory path is recorded
And the gwt sandbox is cleaned up
Then the gwt sandbox worktree path should not exist
And the gwt toctou recorded parent directory should not exist
Scenario: Branch sanitisation works with parent directory approach
When a gwt sandbox is created for plan "plan with spaces!@#$%^&*()"
Then the gwt sandbox worktree path should exist
And the gwt sandbox branch name should be safe for git
And the gwt sandbox should be in the "created" state
Scenario: Worktree paths are unique across sandboxes
When a gwt sandbox is created for plan "plan-toctou-unique-1"
And the gwt toctou sandbox is saved as "u1"
And a gwt sandbox is created for plan "plan-toctou-unique-2"
And the gwt toctou sandbox is saved as "u2"
Then the gwt toctou saved sandbox "u1" worktree path should exist
And the gwt toctou saved sandbox "u2" worktree path should exist
And the gwt toctou saved sandbox paths "u1" and "u2" should differ
Scenario: Rollback works correctly with parent directory approach
When a gwt sandbox is created for plan "plan-toctou-rollback"
And a gwt file "rollback_test.txt" is created in the worktree with content "rollback data"
And the gwt path "rollback_test.txt" is resolved
And the gwt sandbox is rolled back
Then the gwt sandbox should be in the "rolled_back" state
And the gwt file "rollback_test.txt" should not exist in the worktree
Scenario: Commit and rollback sequence works with parent directory approach
When a gwt sandbox is created for plan "plan-toctou-commit-rollback"
And a gwt file "commit_rollback.txt" is created in the worktree with content "test data"
And the gwt sandbox is committed with message "commit rollback test"
Then the gwt commit result should indicate success
And the gwt file "commit_rollback.txt" should exist in the original repo
When the gwt sandbox is rolled back
Then the gwt sandbox should be in the "rolled_back" state
And the gwt file "commit_rollback.txt" should not exist in the original repo
@@ -575,3 +575,137 @@ def step_gwtcb_rollback_error_msg(ctx: Context, msg: str) -> None:
assert msg in str(ctx.gwtcb_error), (
f"Expected '{msg}' in error message, got: {ctx.gwtcb_error}"
)
# ---------------------------------------------------------------------------
# create: timeout after parent dir is created (TOCTOU fix coverage)
# ---------------------------------------------------------------------------
@given("a gwtcb sandbox with mocked _run_git that times out on worktree add")
def step_gwtcb_create_timeout_after_mkdtemp(ctx: Context) -> None:
repo_dir = _init_test_repo()
ctx.gwtcb_sandbox = _make_sandbox(repo_dir)
ctx.gwtcb_error = None
def mock_run_git(args, cwd, timeout=10):
if args and args[0] == "worktree" and "add" in args:
raise subprocess.TimeoutExpired(cmd="git worktree add", timeout=timeout)
result = MagicMock()
result.stdout = repo_dir
result.stderr = ""
return result
patcher = patch(f"{_MODULE}._run_git", side_effect=mock_run_git)
ctx.gwtcb_patcher = patcher
patcher.start()
@given("a gwtcb sandbox with mocked _run_git that fails on worktree add")
def step_gwtcb_create_process_error_after_mkdtemp(ctx: Context) -> None:
repo_dir = _init_test_repo()
ctx.gwtcb_sandbox = _make_sandbox(repo_dir)
ctx.gwtcb_error = None
def mock_run_git(args, cwd, timeout=10):
if args and args[0] == "worktree" and "add" in args:
raise subprocess.CalledProcessError(
returncode=1,
cmd="git worktree add",
stderr="worktree add failed",
)
result = MagicMock()
result.stdout = repo_dir
result.stderr = ""
return result
patcher = patch(f"{_MODULE}._run_git", side_effect=mock_run_git)
ctx.gwtcb_patcher = patcher
patcher.start()
@when("gwtcb create is called expecting a process error")
def step_gwtcb_create_process_error(ctx: Context) -> None:
try:
ctx.gwtcb_sandbox.create("plan-process-error")
except SandboxCreationError as exc:
ctx.gwtcb_error = exc
finally:
if hasattr(ctx, "gwtcb_patcher"):
ctx.gwtcb_patcher.stop()
# ---------------------------------------------------------------------------
# commit: timeout with parent dir set (TOCTOU fix coverage)
# ---------------------------------------------------------------------------
@given("a gwtcb sandbox in ACTIVE state ready to commit with parent dir set")
def step_gwtcb_active_ready_commit_with_parent(ctx: Context) -> None:
import tempfile as _tempfile
repo_dir = _init_test_repo()
ctx.gwtcb_sandbox = _make_sandbox(repo_dir)
ctx.gwtcb_error = None
ctx.gwtcb_commit_result = None
ctx.gwtcb_sandbox._status = SandboxStatus.ACTIVE
ctx.gwtcb_sandbox._worktree_path = repo_dir
ctx.gwtcb_sandbox._branch_name = "cleveragents/plan-test"
ctx.gwtcb_sandbox._base_commit = "abc123"
ctx.gwtcb_sandbox._original_branch = "main"
ctx.gwtcb_sandbox._parent_temp_dir = _tempfile.mkdtemp(prefix="gwtcb-parent-")
# ---------------------------------------------------------------------------
# rollback: timeout with parent dir set (TOCTOU fix coverage)
# ---------------------------------------------------------------------------
@given("a gwtcb sandbox in ACTIVE state ready to rollback with parent dir set")
def step_gwtcb_active_ready_rollback_with_parent(ctx: Context) -> None:
import tempfile as _tempfile
repo_dir = _init_test_repo()
ctx.gwtcb_sandbox = _make_sandbox(repo_dir)
ctx.gwtcb_error = None
ctx.gwtcb_sandbox._status = SandboxStatus.ACTIVE
ctx.gwtcb_sandbox._worktree_path = repo_dir
ctx.gwtcb_sandbox._base_commit = "abc123"
ctx.gwtcb_sandbox._parent_temp_dir = _tempfile.mkdtemp(prefix="gwtcb-parent-")
# ---------------------------------------------------------------------------
# cleanup: OSError when removing parent dir (TOCTOU fix coverage)
# ---------------------------------------------------------------------------
@given("a gwtcb sandbox with a parent dir that raises OSError on removal")
def step_gwtcb_cleanup_parent_oserror(ctx: Context) -> None:
import shutil as _shutil
import tempfile as _tempfile
repo_dir = _init_test_repo()
ctx.gwtcb_sandbox = _make_sandbox(repo_dir)
ctx.gwtcb_error = None
parent_dir = _tempfile.mkdtemp(prefix="gwtcb-parent-oserr-")
ctx.gwtcb_sandbox._status = SandboxStatus.ACTIVE
ctx.gwtcb_sandbox._worktree_path = "/tmp/nonexistent-gwtcb-worktree-oserr"
ctx.gwtcb_sandbox._branch_name = "cleveragents/plan-oserr"
ctx.gwtcb_sandbox._parent_temp_dir = parent_dir
real_rmtree = _shutil.rmtree
def mock_rmtree(path, ignore_errors=False, **kwargs):
if path == parent_dir and not ignore_errors:
raise OSError("Permission denied")
return real_rmtree(path, ignore_errors=ignore_errors, **kwargs)
patcher = patch(
"cleveragents.infrastructure.sandbox.git_worktree.shutil.rmtree",
side_effect=mock_rmtree,
)
ctx.gwtcb_patcher = patcher
patcher.start()
@@ -0,0 +1,89 @@
"""Step definitions for TOCTOU race condition fix in git worktree sandbox.
All steps use the ``gwt toctou`` prefix to avoid collisions with the
existing ``gwt`` steps in ``git_worktree_sandbox_steps.py``.
"""
from __future__ import annotations
import os
from behave import then, when
from behave.runner import Context
def _ensure_saved_sandboxes(ctx: Context) -> dict[str, object]:
"""Return the saved-sandboxes dict, creating it if absent."""
if not hasattr(ctx, "gwt_toctou_saved"):
ctx.gwt_toctou_saved: dict[str, object] = {}
return ctx.gwt_toctou_saved # type: ignore[no-any-return]
# ---------------------------------------------------------------------------
# When — save / record helpers
# ---------------------------------------------------------------------------
@when('the gwt toctou sandbox is saved as "{label}"')
def step_gwt_toctou_save(ctx: Context, label: str) -> None:
saved = _ensure_saved_sandboxes(ctx)
saved[label] = ctx.gwt_sandbox
@when("the gwt toctou parent directory path is recorded")
def step_gwt_toctou_record_parent(ctx: Context) -> None:
parent = getattr(ctx.gwt_sandbox, "_parent_temp_dir", None)
assert parent is not None, "Sandbox has no _parent_temp_dir attribute"
ctx.gwt_toctou_parent_path: str = parent
# ---------------------------------------------------------------------------
# Then — parent directory assertions
# ---------------------------------------------------------------------------
@then("the gwt toctou sandbox should have a parent temp directory")
def step_gwt_toctou_has_parent(ctx: Context) -> None:
parent = getattr(ctx.gwt_sandbox, "_parent_temp_dir", None)
assert parent is not None, "Expected _parent_temp_dir to be set"
assert os.path.isdir(parent), f"Parent temp dir does not exist: {parent}"
@then("the gwt toctou worktree should be a subdirectory of its parent")
def step_gwt_toctou_worktree_is_child(ctx: Context) -> None:
parent = ctx.gwt_sandbox._parent_temp_dir
worktree = ctx.gwt_sandbox.context.sandbox_path
assert worktree.startswith(parent + os.sep), (
f"Worktree {worktree} is not under parent {parent}"
)
@then("the gwt toctou recorded parent directory should not exist")
def step_gwt_toctou_parent_gone(ctx: Context) -> None:
parent = ctx.gwt_toctou_parent_path
assert not os.path.exists(parent), (
f"Parent directory should have been removed: {parent}"
)
# ---------------------------------------------------------------------------
# Then — saved-sandbox assertions
# ---------------------------------------------------------------------------
@then('the gwt toctou saved sandbox "{label}" worktree path should exist')
def step_gwt_toctou_saved_exists(ctx: Context, label: str) -> None:
saved = _ensure_saved_sandboxes(ctx)
sandbox = saved[label]
assert sandbox.context is not None
assert os.path.isdir(sandbox.context.sandbox_path), (
f"Worktree for '{label}' does not exist"
)
@then('the gwt toctou saved sandbox paths "{a}" and "{b}" should differ')
def step_gwt_toctou_saved_differ(ctx: Context, a: str, b: str) -> None:
saved = _ensure_saved_sandboxes(ctx)
path_a = saved[a].context.sandbox_path
path_b = saved[b].context.sandbox_path
assert path_a != path_b, f"Paths should differ but both are {path_a}"
@@ -709,7 +709,10 @@ def step_prepare_slow_executor_bg_loop(context):
context.graph.state_manager.update_state = MagicMock()
async def slow_execute(state):
await asyncio.sleep(10)
# Use the original (un-patched) asyncio.sleep so the test-infrastructure
# 10 ms sleep cap does not race with the 0.01 s executor timeout.
_real_sleep = getattr(asyncio, "_original_sleep", asyncio.sleep)
await _real_sleep(0.5)
return {"messages": ["never"]}
context.graph.nodes["worker"].execute = slow_execute
@@ -745,7 +748,10 @@ def step_prepare_slow_executor_tp(context):
context.graph.state_manager.update_state = MagicMock()
async def slow_execute(state):
await asyncio.sleep(10)
# Use the original (un-patched) asyncio.sleep so the test-infrastructure
# 10 ms sleep cap does not race with the 0.01 s executor timeout.
_real_sleep = getattr(asyncio, "_original_sleep", asyncio.sleep)
await _real_sleep(0.5)
return {"messages": ["never"]}
context.graph.nodes["worker"].execute = slow_execute
@@ -140,6 +140,7 @@ class GitWorktreeSandbox:
# Set after create()
self._worktree_path: str | None = None
self._parent_temp_dir: str | None = None
self._branch_name: str | None = None
self._original_branch: str | None = None
self._base_commit: str | None = None
@@ -349,10 +350,15 @@ class GitWorktreeSandbox:
safe_plan_id = _sanitise_branch_name(plan_id)
self._branch_name = f"cleveragents/plan-{safe_plan_id}"
# Create a temporary directory for the worktree
self._worktree_path = tempfile.mkdtemp(prefix=f"ca-sandbox-{safe_plan_id}-")
# mkdtemp creates the dir; git worktree add needs it to not exist
os.rmdir(self._worktree_path)
# Create a temporary parent directory for the worktree.
# We use a parent directory approach to avoid TOCTOU race condition:
# mkdtemp() creates the parent atomically, then git worktree add
# creates the child worktree directory itself. This eliminates the
# race window that would exist if we created and then deleted the
# worktree path before git claims it.
parent_dir = tempfile.mkdtemp(prefix=f"ca-sandbox-{safe_plan_id}-")
self._parent_temp_dir = parent_dir
self._worktree_path = os.path.join(parent_dir, "worktree")
# Create the worktree with a new branch
_run_git(
@@ -370,12 +376,18 @@ class GitWorktreeSandbox:
except subprocess.TimeoutExpired as exc:
self._status = SandboxStatus.ERRORED
# Clean up parent directory on creation failure
if self._parent_temp_dir is not None:
shutil.rmtree(self._parent_temp_dir, ignore_errors=True)
raise SandboxCreationError(
f"Git command timed out after {self._git_timeout}s "
f"while creating worktree for resource {self._resource_id}"
) from exc
except subprocess.CalledProcessError as exc:
self._status = SandboxStatus.ERRORED
# Clean up parent directory on creation failure
if self._parent_temp_dir is not None:
shutil.rmtree(self._parent_temp_dir, ignore_errors=True)
raise SandboxCreationError(
f"Failed to create git worktree for resource "
f"{self._resource_id}: {exc.stderr.strip()}"
@@ -560,6 +572,9 @@ class GitWorktreeSandbox:
except subprocess.TimeoutExpired as exc:
self._pre_merge_commit = None
# Clean up parent directory on creation failure
if self._parent_temp_dir is not None:
shutil.rmtree(self._parent_temp_dir, ignore_errors=True)
self._status = SandboxStatus.ERRORED
raise SandboxCommitError(
f"Git command timed out after {self._git_timeout}s "
@@ -648,6 +663,9 @@ class GitWorktreeSandbox:
)
except subprocess.TimeoutExpired as exc:
self._status = SandboxStatus.ERRORED
# Clean up parent directory on creation failure
if self._parent_temp_dir is not None:
shutil.rmtree(self._parent_temp_dir, ignore_errors=True)
raise SandboxRollbackError(
f"Git command timed out after {self._git_timeout}s "
f"while rolling back sandbox {self._sandbox_id}"
@@ -700,6 +718,21 @@ class GitWorktreeSandbox:
)
shutil.rmtree(self._worktree_path, ignore_errors=True)
# Remove the parent temporary directory
if self._parent_temp_dir is not None and os.path.exists(self._parent_temp_dir):
try:
shutil.rmtree(self._parent_temp_dir, ignore_errors=False)
logger.debug(
"Removed parent temporary directory: %s",
self._parent_temp_dir,
)
except OSError as exc:
logger.warning(
"Failed to remove parent temporary directory %s: %s",
self._parent_temp_dir,
exc,
)
# Delete the sandbox branch
if self._branch_name is not None:
try: