fix(cli): add path traversal guard to context_remove --all loop #1297

New issue

Open

opened 2026-04-02 09:17:54 +00:00 by freemo · 0 comments

freemo commented

2026-04-02 09:17:54 +00:00

Owner

Metadata

Branch: bugfix/m5-actor-context-remove-path-traversal
Commit Message: fix(cli): add path traversal guard to context_remove --all loop
Milestone: v3.6.0
Parent Epic: (needs manual linking — no open CLI/security Epic identified at creation time)

Background and Context

The context_remove command in src/cleveragents/cli/commands/actor_context.py was introduced in issue #869. When the --all flag is used, the function calls _list_context_names(base) to enumerate all subdirectory names under the context base directory, then passes each name directly to shutil.rmtree(base / cname) without any path sanitisation.

Because cname is read directly from the filesystem (via Path.iterdir()), a maliciously crafted directory name containing path traversal characters (e.g., .. or ../../etc) could cause shutil.rmtree to delete files and directories outside the intended context base directory. This is a classic path traversal vulnerability.

The same risk exists in the single-context removal path (target = base / name) where name is supplied by the user as a CLI argument, though the attack surface there is lower since the user controls the input directly.

Current Behavior

# src/cleveragents/cli/commands/actor_context.py:163
for cname in names:
    shutil.rmtree(base / cname)

cname is used without resolving or validating that (base / cname).resolve() is still a descendant of base.resolve(). A directory named .. or ../../sensitive on the filesystem would cause deletion of an arbitrary path.

Expected Behavior

Before calling shutil.rmtree, the resolved path must be verified to be within the context base directory. Any name that escapes the base should be rejected with a clear error message and skipped (or cause a hard abort), never deleted.

resolved_base = base.resolve()
for cname in names:
    target_path = (base / cname).resolve()
    if not target_path.is_relative_to(resolved_base):
        console.print(
            f"[red]Error:[/red] Skipping '{cname}' — path traversal detected."
        )
        continue
    shutil.rmtree(target_path)

The same guard should be applied to the single-context removal path (target = base / name).

Acceptance Criteria

context_remove --all resolves each candidate path and verifies it is a descendant of base.resolve() before calling shutil.rmtree
Any context name that resolves outside base is rejected with a descriptive error message and skipped; no deletion occurs for that entry
Single-context removal (context_remove <NAME>) applies the same guard before delegating to ContextManager.delete()
All existing context_remove BDD scenarios continue to pass
New BDD scenario covers the path-traversal rejection case (directory named .. or similar)
nox (all default sessions) passes with no errors
Coverage ≥ 97%

Supporting Information

Vulnerable file: src/cleveragents/cli/commands/actor_context.py
Vulnerable function: context_remove
Vulnerable lines: ~163 (shutil.rmtree(base / cname)) and ~175 (target = base / name)
Introduced by: #869
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Severity: Medium — requires a maliciously named directory to already exist under the context base; not remotely exploitable without prior filesystem access, but violates defence-in-depth

Subtasks

Add resolved_base = base.resolve() before the --all deletion loop in context_remove
Wrap shutil.rmtree(base / cname) with a is_relative_to guard; skip and warn on violation
Apply the same guard to the single-context removal path (target = base / name) before ctx_mgr.delete()
Tests (Behave): Add scenario context remove --all skips path-traversal directory names
Tests (Behave): Add scenario context remove rejects traversal name in single-context path
Verify coverage ≥ 97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.
All nox stages pass (lint, typecheck, unit_tests, integration_tests, coverage_report).
Coverage ≥ 97%.

## Metadata - **Branch**: `bugfix/m5-actor-context-remove-path-traversal` - **Commit Message**: `fix(cli): add path traversal guard to context_remove --all loop` - **Milestone**: v3.6.0 - **Parent Epic**: *(needs manual linking — no open CLI/security Epic identified at creation time)* ## Background and Context The `context_remove` command in `src/cleveragents/cli/commands/actor_context.py` was introduced in issue #869. When the `--all` flag is used, the function calls `_list_context_names(base)` to enumerate all subdirectory names under the context base directory, then passes each name directly to `shutil.rmtree(base / cname)` without any path sanitisation. Because `cname` is read directly from the filesystem (via `Path.iterdir()`), a maliciously crafted directory name containing path traversal characters (e.g., `..` or `../../etc`) could cause `shutil.rmtree` to delete files and directories **outside** the intended context base directory. This is a classic path traversal vulnerability. The same risk exists in the single-context removal path (`target = base / name`) where `name` is supplied by the user as a CLI argument, though the attack surface there is lower since the user controls the input directly. ## Current Behavior ```python # src/cleveragents/cli/commands/actor_context.py:163 for cname in names: shutil.rmtree(base / cname) ``` `cname` is used without resolving or validating that `(base / cname).resolve()` is still a descendant of `base.resolve()`. A directory named `..` or `../../sensitive` on the filesystem would cause deletion of an arbitrary path. ## Expected Behavior Before calling `shutil.rmtree`, the resolved path must be verified to be within the context base directory. Any name that escapes the base should be rejected with a clear error message and skipped (or cause a hard abort), never deleted. ```python resolved_base = base.resolve() for cname in names: target_path = (base / cname).resolve() if not target_path.is_relative_to(resolved_base): console.print( f"[red]Error:[/red] Skipping '{cname}' — path traversal detected." ) continue shutil.rmtree(target_path) ``` The same guard should be applied to the single-context removal path (`target = base / name`). ## Acceptance Criteria - [ ] `context_remove --all` resolves each candidate path and verifies it is a descendant of `base.resolve()` before calling `shutil.rmtree` - [ ] Any context name that resolves outside `base` is rejected with a descriptive error message and skipped; no deletion occurs for that entry - [ ] Single-context removal (`context_remove <NAME>`) applies the same guard before delegating to `ContextManager.delete()` - [ ] All existing `context_remove` BDD scenarios continue to pass - [ ] New BDD scenario covers the path-traversal rejection case (directory named `..` or similar) - [ ] `nox` (all default sessions) passes with no errors - [ ] Coverage ≥ 97% ## Supporting Information - **Vulnerable file**: `src/cleveragents/cli/commands/actor_context.py` - **Vulnerable function**: `context_remove` - **Vulnerable lines**: ~163 (`shutil.rmtree(base / cname)`) and ~175 (`target = base / name`) - **Introduced by**: #869 - **CWE**: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') - **Severity**: Medium — requires a maliciously named directory to already exist under the context base; not remotely exploitable without prior filesystem access, but violates defence-in-depth ## Subtasks - [ ] Add `resolved_base = base.resolve()` before the `--all` deletion loop in `context_remove` - [ ] Wrap `shutil.rmtree(base / cname)` with a `is_relative_to` guard; skip and warn on violation - [ ] Apply the same guard to the single-context removal path (`target = base / name`) before `ctx_mgr.delete()` - [ ] Tests (Behave): Add scenario `context remove --all skips path-traversal directory names` - [ ] Tests (Behave): Add scenario `context remove rejects traversal name in single-context path` - [ ] Verify coverage ≥ 97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. - All nox stages pass (`lint`, `typecheck`, `unit_tests`, `integration_tests`, `coverage_report`). - Coverage ≥ 97%.

freemo added the

labels

2026-04-02 09:18:18 +00:00

freemo self-assigned this

2026-04-02 18:45:25 +00:00

freemo removed the