mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(kanban): restrict managed-scratch roots to workspaces/ dirs only
Copilot review on PR #28819 flagged that `_is_managed_scratch_path` accepted the entire `<kanban_home>/kanban` subtree as managed scratch storage. With that, a task whose `workspace_kind='scratch'` and `workspace_path` was mis-set to `<kanban_home>/kanban`, `.../kanban/logs`, or a board's metadata directory (e.g. `.../kanban/boards/<slug>` without the `workspaces/` child) would pass the containment guard and let task completion `shutil.rmtree` Hermes' own DB, metadata, and log subtrees. Tighten the guard: * Allowed roots are now exclusively `workspaces/` directories — the `HERMES_KANBAN_WORKSPACES_ROOT` override, `<kanban_home>/kanban/workspaces`, and each `<kanban_home>/kanban/boards/<slug>/workspaces` discovered on disk. * Require strict descendancy: a path equal to a root itself is rejected too, because deleting a workspaces root would wipe every task's scratch dir at once. Add a regression test covering the three Copilot-named attack paths (kanban root, kanban/logs, board root without `workspaces/`) plus the workspaces-root-itself case, and confirm the inner task-id dir still matches.
This commit is contained in:
parent
80ad1609c8
commit
23115b5c0f
2 changed files with 83 additions and 7 deletions
|
|
@ -3038,15 +3038,24 @@ def complete_task(
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _is_managed_scratch_path(p: Path) -> bool:
|
||||
"""Return True iff *p* lives inside a kanban-managed scratch root.
|
||||
"""Return True iff *p* is a strict descendant of a kanban-managed scratch root.
|
||||
|
||||
A managed root is one of:
|
||||
A managed root is exclusively a ``workspaces/`` directory — never the
|
||||
broader kanban home, a board root, or sibling subtrees like ``logs/`` or
|
||||
``boards/<slug>/`` itself. Allowed roots:
|
||||
|
||||
* ``HERMES_KANBAN_WORKSPACES_ROOT`` when set (worker-side override
|
||||
injected by the dispatcher).
|
||||
* The current kanban home's ``kanban/`` subtree, which covers both the
|
||||
legacy default-board scratch root (``<kanban_home>/kanban/workspaces``)
|
||||
and per-board roots (``<kanban_home>/kanban/boards/<slug>/workspaces``).
|
||||
* ``<kanban_home>/kanban/workspaces`` — legacy default-board scratch root.
|
||||
* ``<kanban_home>/kanban/boards/<slug>/workspaces`` for each board slug
|
||||
that currently exists on disk.
|
||||
|
||||
The check requires strict descendancy: a path equal to one of these
|
||||
roots is NOT managed (deleting the workspaces root would wipe every
|
||||
task's scratch dir at once), and a path that resolves to ``<kanban_home>
|
||||
/kanban`` itself, ``<kanban_home>/kanban/logs``, or
|
||||
``<kanban_home>/kanban/boards/<slug>`` is rejected because those
|
||||
subtrees hold Hermes' own DB, metadata, and logs, not task workspaces.
|
||||
|
||||
Used by :func:`_cleanup_workspace` to refuse to ``shutil.rmtree`` paths
|
||||
outside Hermes-managed storage. A board ``default_workdir`` pointing at a
|
||||
|
|
@ -3065,10 +3074,36 @@ def _is_managed_scratch_path(p: Path) -> bool:
|
|||
except OSError:
|
||||
pass
|
||||
try:
|
||||
roots.append((kanban_home() / "kanban").resolve(strict=False))
|
||||
home = kanban_home()
|
||||
except OSError:
|
||||
pass
|
||||
home = None
|
||||
if home is not None:
|
||||
try:
|
||||
roots.append((home / "kanban" / "workspaces").resolve(strict=False))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
boards_parent = (home / "kanban" / "boards").resolve(strict=False)
|
||||
except OSError:
|
||||
boards_parent = None
|
||||
if boards_parent is not None:
|
||||
try:
|
||||
entries = list(boards_parent.iterdir())
|
||||
except OSError:
|
||||
entries = []
|
||||
for entry in entries:
|
||||
try:
|
||||
if not entry.is_dir():
|
||||
continue
|
||||
except OSError:
|
||||
continue
|
||||
try:
|
||||
roots.append((entry / "workspaces").resolve(strict=False))
|
||||
except OSError:
|
||||
continue
|
||||
for root in roots:
|
||||
if p_abs == root:
|
||||
continue
|
||||
try:
|
||||
if p_abs.is_relative_to(root):
|
||||
return True
|
||||
|
|
|
|||
|
|
@ -1561,6 +1561,47 @@ def test_is_managed_scratch_path_rejects_real_source_tree(kanban_home, tmp_path)
|
|||
assert not kb._is_managed_scratch_path(real)
|
||||
|
||||
|
||||
def test_is_managed_scratch_path_rejects_kanban_metadata_subtrees(kanban_home):
|
||||
"""Hermes' own DB/metadata/log subtrees under ``<kanban_home>/kanban`` are NOT managed.
|
||||
|
||||
Regression guard for the Copilot finding on #28819: a scratch task whose
|
||||
``workspace_path`` was mis-set to the kanban home, the logs dir, or a
|
||||
board's metadata dir (i.e. the board root itself, not its ``workspaces/``
|
||||
child) must be refused. Without this, the containment check would happily
|
||||
``shutil.rmtree`` Hermes' DB/metadata/logs on task completion.
|
||||
"""
|
||||
kanban_root = kanban_home / "kanban"
|
||||
kanban_root.mkdir(parents=True, exist_ok=True)
|
||||
assert not kb._is_managed_scratch_path(kanban_root)
|
||||
|
||||
logs_dir = kanban_root / "logs"
|
||||
logs_dir.mkdir(parents=True, exist_ok=True)
|
||||
assert not kb._is_managed_scratch_path(logs_dir)
|
||||
|
||||
board_root = kanban_root / "boards" / "my-board"
|
||||
board_root.mkdir(parents=True, exist_ok=True)
|
||||
# The board root itself is NOT a managed scratch dir — only the
|
||||
# ``workspaces/`` child (and its descendants) are.
|
||||
assert not kb._is_managed_scratch_path(board_root)
|
||||
|
||||
# Sibling subtrees of ``workspaces/`` under a board (e.g. its kanban.db
|
||||
# or board.json living next to ``workspaces/``) are also not managed.
|
||||
board_logs = board_root / "logs"
|
||||
board_logs.mkdir(parents=True, exist_ok=True)
|
||||
assert not kb._is_managed_scratch_path(board_logs)
|
||||
|
||||
# Now create the board's workspaces dir and a task scratch dir under it —
|
||||
# the latter is the only thing the guard should allow.
|
||||
board_workspaces = board_root / "workspaces"
|
||||
board_workspaces.mkdir(parents=True, exist_ok=True)
|
||||
# The workspaces root itself is also NOT managed — deleting it would
|
||||
# wipe every task's scratch dir at once.
|
||||
assert not kb._is_managed_scratch_path(board_workspaces)
|
||||
task_dir = board_workspaces / "task-42"
|
||||
task_dir.mkdir(parents=True, exist_ok=True)
|
||||
assert kb._is_managed_scratch_path(task_dir)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tenancy
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue