fix(kanban): guard _cleanup_workspace against paths outside workspaces_root#30718
fix(kanban): guard _cleanup_workspace against paths outside workspaces_root#30718SimoKiihamaki wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a safety guard to _cleanup_workspace so scratch workspace deletion is restricted to paths under workspaces_root, preventing accidental data loss when workspace_path points outside the kanban workspaces directory. Symlinks and relative traversal are resolved before the check.
Changes:
- Introduces
_is_safe_workspace_pathhelper that compares resolved paths against the resolved workspaces root. - Updates
_cleanup_workspaceto refuse deletion of paths outsideworkspaces_root, logging a warning and still cleaning up the tmux session. - Adds a comprehensive test module covering normal cleanup, outside-root paths, symlink escapes, traversal attempts, non-scratch kinds, and the helper itself.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| hermes_cli/kanban_db.py | Adds resolved-path safety guard before scratch workspace deletion and the _is_safe_workspace_path helper. |
| tests/hermes_cli/test_kanban_workspace_cleanup.py | New tests verifying the safety guard across inside/outside, symlink, traversal, non-scratch, and missing-task scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pass # best-effort — never block completion | ||
|
|
||
|
|
||
| def _is_safe_workspace_path(workspace_path: Path, workspaces_root: Path) -> bool: |
| def test_symlink_escaping_workspace_is_not_deleted(self, kanban_home, tmp_path): | ||
| """A symlink inside workspaces_root pointing outside must not be followed for deletion.""" |
| wp = Path(path).resolve() | ||
| root = workspaces_root().resolve() | ||
| if not _is_safe_workspace_path(wp, root): |
|
Closing as superseded by #31708 (merged: commits 80ad160 + 23115b5 + ce529d6 on main). Your containment-via-workspaces_root() approach is the same shape as @briandevans's salvaged fix. We picked #28819 because it submitted first AND covers more roots (env override, per-board sub-roots) with more thorough tests (kanban metadata sibling rejection, workspaces-root-itself rejection). Same outcome, broader coverage. Thanks for the catch on #28818 — independent confirmation of the data-loss vector helped prioritize this. |
Fixes #30151
Problem
_cleanup_workspace()calledshutil.rmtree()on any path inworkspace_pathwithout verifying it was under the Kanban workspaces root. Ifdefault_workdirpointed to a real directory, completing a scratch task would recursively delete it.Fix
Added path validation: workspace cleanup now resolves the workspace path and verifies it is under
workspaces_root()before allowing deletion. Paths outside the root are refused with a warning log. This guards against:default_workdirpointing to a user directory../../)Testing