mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(security): disposition all 104 code-scanning + dependabot alerts (#375)
Fix the 4 real CodeQL path-injection alerts (open_conventions_pr trusted the API-settable project.workspace_path with no containment) plus defense-in-depth segment validation at the get_workspace_path chokepoint. Bump next 16.1.1->16.1.7 and transitive lockfile deps to clear 24 Dependabot alerts. Close the intake subagent-ban gap: the Claude intake driver still carried the Task tool and the prompter prompt told it to fan out research subagents, contradicting the fleet-wide ban. The remaining 47 CodeQL + 29 Dependabot alerts are dismissed on GitHub with per-alert justifications (guard patterns CodeQL can't model across call hops; next 16.2.x blocked by the verified tab-hostage router regression). Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -36,11 +36,15 @@ def _init_repo(repo: Path) -> None:
|
||||
_git(repo, "commit", "-m", "init")
|
||||
|
||||
|
||||
def _svc(monkeypatch: pytest.MonkeyPatch, repo: Path) -> GitService:
|
||||
def _svc(monkeypatch: pytest.MonkeyPatch, repo: Path, root: Path) -> GitService:
|
||||
svc = GitService.__new__(GitService)
|
||||
svc.session = AsyncMock()
|
||||
|
||||
# The workspace-scope guard requires workspace_path under
|
||||
# {workspaces_root}/{project.slug}; anchor the root at the test dir.
|
||||
monkeypatch.setattr(git_module.settings, "workspaces_root", str(root))
|
||||
project = MagicMock()
|
||||
project.slug = "g-proj"
|
||||
project.workspace_path = str(repo)
|
||||
project.default_branch = "master"
|
||||
project_service = MagicMock()
|
||||
@@ -69,12 +73,12 @@ async def test_dirty_tree_refused_before_any_checkout(
|
||||
"""A dirty working tree is the agent's active workspace — refuse, return
|
||||
None, and leave the tree exactly as it was (no scaffold branch, dirty
|
||||
change still uncommitted in the working tree)."""
|
||||
repo = tmp_path / "repo"
|
||||
repo = tmp_path / "g-proj" / "repo"
|
||||
_init_repo(repo)
|
||||
# Dirty the tree: modify a tracked file (the agent's in-progress work).
|
||||
(repo / "README.md").write_text("# dirty work in progress\n")
|
||||
|
||||
svc = _svc(monkeypatch, repo)
|
||||
svc = _svc(monkeypatch, repo, tmp_path)
|
||||
|
||||
result = await svc.open_conventions_pr(
|
||||
"g-proj", content="version: 1\n", title="scaffold", body="b"
|
||||
@@ -111,10 +115,10 @@ async def test_clean_tree_proceeds_and_commits_on_scaffold(
|
||||
"""A clean tree proceeds: the conventions file is committed on the
|
||||
scaffold branch cut from master, and master itself is untouched
|
||||
(regression guard for the fix not over-rejecting the clean case)."""
|
||||
repo = tmp_path / "repo"
|
||||
repo = tmp_path / "g-proj" / "repo"
|
||||
_init_repo(repo)
|
||||
|
||||
svc = _svc(monkeypatch, repo)
|
||||
svc = _svc(monkeypatch, repo, tmp_path)
|
||||
|
||||
result = await svc.open_conventions_pr(
|
||||
"g-proj", content="version: 1\n", title="scaffold", body="b"
|
||||
@@ -154,12 +158,13 @@ async def test_missing_base_branch_refused(
|
||||
not fall through to ``checkout -B <scaffold>`` from the current branch —
|
||||
that would base the scaffold on the agent's task branch. Refuse when the
|
||||
checkout doesn't actually land on base."""
|
||||
repo = tmp_path / "repo"
|
||||
repo = tmp_path / "g-proj" / "repo"
|
||||
_init_repo(repo)
|
||||
|
||||
svc = _svc(monkeypatch, repo)
|
||||
svc = _svc(monkeypatch, repo, tmp_path)
|
||||
# Override the project default to a branch that doesn't exist locally.
|
||||
project = MagicMock()
|
||||
project.slug = "g-proj"
|
||||
project.workspace_path = str(repo)
|
||||
project.default_branch = "nonexistent-base"
|
||||
project_service = MagicMock()
|
||||
|
||||
@@ -0,0 +1,131 @@
|
||||
"""``open_conventions_pr`` refuses a ``project.workspace_path`` outside the
|
||||
project's own workspace tree.
|
||||
|
||||
``workspace_path`` is settable through the PM-gated ``POST
|
||||
/projects/{id}/workspace`` route with no path validation, so a steered PM
|
||||
agent could point it at an arbitrary orchestrator directory (or another
|
||||
project's clone) and have the conventions flow write + commit there. Only a
|
||||
path under ``{workspaces_root}/{project.slug}`` may receive the scaffold
|
||||
commit; anything else is treated as "no usable workspace" (returns None).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import subprocess
|
||||
from typing import TYPE_CHECKING
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.services import git as git_module
|
||||
from roboco.services.git import GitService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
|
||||
_SCAFFOLD_BRANCH = "chore/roboco-conventions-scaffold"
|
||||
|
||||
|
||||
def _git(repo: Path, *args: str) -> None:
|
||||
subprocess.run(["git", *args], cwd=repo, check=True, capture_output=True)
|
||||
|
||||
|
||||
def _init_repo(repo: Path) -> None:
|
||||
repo.mkdir(parents=True, exist_ok=True)
|
||||
_git(repo, "init", "-b", "master")
|
||||
_git(repo, "config", "user.email", "t@example.com")
|
||||
_git(repo, "config", "user.name", "T")
|
||||
_git(repo, "config", "commit.gpgsign", "false")
|
||||
(repo / "README.md").write_text("# r\n")
|
||||
_git(repo, "add", "README.md")
|
||||
_git(repo, "commit", "-m", "init")
|
||||
|
||||
|
||||
def _svc(
|
||||
monkeypatch: pytest.MonkeyPatch, workspace_path: Path, root: Path
|
||||
) -> GitService:
|
||||
svc = GitService.__new__(GitService)
|
||||
svc.session = AsyncMock()
|
||||
monkeypatch.setattr(git_module.settings, "workspaces_root", str(root))
|
||||
project = MagicMock()
|
||||
project.slug = "g-proj"
|
||||
project.workspace_path = str(workspace_path)
|
||||
project.default_branch = "master"
|
||||
project_service = MagicMock()
|
||||
project_service.get_by_slug = AsyncMock(return_value=project)
|
||||
monkeypatch.setattr(git_module, "get_project_service", lambda _s: project_service)
|
||||
monkeypatch.setattr(svc, "_token_for_project", AsyncMock(return_value=None))
|
||||
return svc
|
||||
|
||||
|
||||
def _branch_exists(repo: Path, branch: str) -> bool:
|
||||
res = subprocess.run(
|
||||
["git", "rev-parse", "--verify", branch],
|
||||
cwd=repo,
|
||||
capture_output=True,
|
||||
check=False,
|
||||
)
|
||||
return res.returncode == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_workspace_path_outside_root_refused(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""A workspace_path outside workspaces_root entirely is refused and never
|
||||
receives a scaffold branch or file."""
|
||||
root = tmp_path / "workspaces"
|
||||
root.mkdir()
|
||||
outside = tmp_path / "outside-repo"
|
||||
_init_repo(outside)
|
||||
svc = _svc(monkeypatch, outside, root)
|
||||
|
||||
result = await svc.open_conventions_pr(
|
||||
"g-proj", content="version: 1\n", title="scaffold", body="b"
|
||||
)
|
||||
|
||||
assert result is None
|
||||
assert not _branch_exists(outside, _SCAFFOLD_BRANCH)
|
||||
assert not (outside / ".roboco" / "conventions.yml").exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_workspace_path_in_other_projects_tree_refused(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""A workspace_path under workspaces_root but inside ANOTHER project's
|
||||
tree is refused — cross-project commits are not allowed."""
|
||||
other = tmp_path / "other-proj" / "backend" / "be-dev-1"
|
||||
_init_repo(other)
|
||||
svc = _svc(monkeypatch, other, tmp_path)
|
||||
|
||||
result = await svc.open_conventions_pr(
|
||||
"g-proj", content="version: 1\n", title="scaffold", body="b"
|
||||
)
|
||||
|
||||
assert result is None
|
||||
assert not _branch_exists(other, _SCAFFOLD_BRANCH)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_explicit_workspace_argument_bypasses_db_path(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""The explicit ``workspace`` argument (internally constructed by callers
|
||||
from the validated workspace layout) is honored as before — the scope
|
||||
guard applies to the API-settable DB field, not the trusted argument."""
|
||||
repo = tmp_path / "elsewhere" / "clone"
|
||||
_init_repo(repo)
|
||||
# DB field points somewhere invalid; explicit arg wins.
|
||||
svc = _svc(monkeypatch, tmp_path / "bogus", tmp_path / "workspaces")
|
||||
|
||||
result = await svc.open_conventions_pr(
|
||||
"g-proj",
|
||||
content="version: 1\n",
|
||||
title="scaffold",
|
||||
body="b",
|
||||
workspace=repo,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result["branch"] == _SCAFFOLD_BRANCH
|
||||
assert _branch_exists(repo, _SCAFFOLD_BRANCH)
|
||||
@@ -0,0 +1,56 @@
|
||||
"""``get_workspace_path`` rejects traversal-capable path components.
|
||||
|
||||
Slugs/teams are regex- or enum-validated at creation, but every workspace path
|
||||
is built at this one chokepoint — pin the by-construction guard so a raw
|
||||
``../`` / absolute / NUL segment can never place a workspace outside the root,
|
||||
regardless of what upstream validation a future caller forgets.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from roboco.models.base import Team
|
||||
from roboco.services.workspace import WorkspaceError, WorkspaceService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
def _service(root: Path) -> WorkspaceService:
|
||||
svc = WorkspaceService(MagicMock())
|
||||
svc.session = AsyncMock()
|
||||
svc.root = root
|
||||
return svc
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
["", ".", "..", "../escape", "a/b", "a\\b", "bad\x00slug", "/etc"],
|
||||
)
|
||||
def test_rejects_unsafe_project_slug(tmp_path: Path, bad: str) -> None:
|
||||
svc = _service(tmp_path)
|
||||
with pytest.raises(WorkspaceError, match="unsafe project slug"):
|
||||
svc.get_workspace_path(bad, Team.BACKEND, "be-dev-1")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("bad", ["..", "back/end", ""])
|
||||
def test_rejects_unsafe_team_string(tmp_path: Path, bad: str) -> None:
|
||||
svc = _service(tmp_path)
|
||||
with pytest.raises(WorkspaceError, match="unsafe team"):
|
||||
svc.get_workspace_path("guard-core", bad, "be-dev-1")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("bad", ["..", "../../be-dev-1", "be\x00dev"])
|
||||
def test_rejects_unsafe_agent_slug(tmp_path: Path, bad: str) -> None:
|
||||
svc = _service(tmp_path)
|
||||
with pytest.raises(WorkspaceError, match="unsafe agent slug"):
|
||||
svc.get_workspace_path("guard-core", Team.BACKEND, bad)
|
||||
|
||||
|
||||
def test_valid_segments_unchanged(tmp_path: Path) -> None:
|
||||
svc = _service(tmp_path)
|
||||
path = svc.get_workspace_path("guard-core", Team.BACKEND, "be-dev-1")
|
||||
assert path == tmp_path / "guard-core" / "backend" / "be-dev-1"
|
||||
Reference in New Issue
Block a user