mirror of
https://github.com/usestrix/strix.git
synced 2026-11-14 09:01:25 +00:00
fix(interface): Address PR review on collision and path guard
Check every assigned workspace subdirectory in assign_workspace_subdirs to prevent suffix collisions across sanitized target names, and add test coverage for symlinks resolving outside the temporary directory. - Make assign_workspace_subdirs ensure unique workspace subdirectory names across all assigned targets - Add test_clone_repository_rejects_symlink_pointing_outside_temp_dir - Add test_assign_workspace_subdirs_avoids_collisions_with_generated_suffixes
This commit is contained in:
parent
21f3f55a50
commit
ed92a726e5
2 changed files with 66 additions and 7 deletions
|
|
@ -1286,7 +1286,7 @@ def derive_local_base_name(path_str: str) -> str:
|
|||
|
||||
|
||||
def assign_workspace_subdirs(targets_info: list[dict[str, Any]]) -> None:
|
||||
name_counts: dict[str, int] = {}
|
||||
assigned_names: set[str] = set()
|
||||
|
||||
for target in targets_info:
|
||||
target_type = target["type"]
|
||||
|
|
@ -1301,12 +1301,14 @@ def assign_workspace_subdirs(targets_info: list[dict[str, Any]]) -> None:
|
|||
if base_name is None:
|
||||
continue
|
||||
|
||||
count = name_counts.get(base_name, 0) + 1
|
||||
name_counts[base_name] = count
|
||||
candidate = base_name
|
||||
count = 2
|
||||
while candidate in assigned_names:
|
||||
candidate = f"{base_name}-{count}"
|
||||
count += 1
|
||||
|
||||
workspace_subdir = base_name if count == 1 else f"{base_name}-{count}"
|
||||
|
||||
details["workspace_subdir"] = workspace_subdir
|
||||
assigned_names.add(candidate)
|
||||
details["workspace_subdir"] = candidate
|
||||
|
||||
|
||||
def is_whitebox_scan(targets_info: list[dict[str, Any]]) -> bool:
|
||||
|
|
|
|||
|
|
@ -2,12 +2,13 @@
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from strix.interface.utils import (
|
||||
assign_workspace_subdirs,
|
||||
clone_repository,
|
||||
derive_repo_base_name,
|
||||
infer_target_type,
|
||||
|
|
@ -92,3 +93,59 @@ def test_derive_repo_base_name_handles_dot_git() -> None:
|
|||
assert derive_repo_base_name("..git") == "target"
|
||||
assert derive_repo_base_name("https://github.com/foo/bar.git") == "bar"
|
||||
assert derive_repo_base_name("git@github.com:foo/bar.git") == "bar"
|
||||
|
||||
|
||||
def test_clone_repository_rejects_symlink_pointing_outside_temp_dir(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.setattr("tempfile.gettempdir", lambda: str(tmp_path))
|
||||
monkeypatch.setattr("shutil.which", lambda cmd: "/usr/bin/git" if cmd == "git" else None)
|
||||
|
||||
outside_dir = tmp_path / "sensitive_outside_directory"
|
||||
outside_dir.mkdir(parents=True)
|
||||
canary_file = outside_dir / "canary.txt"
|
||||
canary_file.write_text("critical data", encoding="utf-8")
|
||||
|
||||
run_temp_dir = tmp_path / "strix_repos" / "run_123"
|
||||
run_temp_dir.mkdir(parents=True)
|
||||
symlink_dest = run_temp_dir / "external_symlink"
|
||||
symlink_dest.symlink_to(outside_dir)
|
||||
|
||||
mock_run = MagicMock()
|
||||
with (
|
||||
patch("subprocess.run", mock_run),
|
||||
pytest.raises(ValueError, match="Invalid clone path outside temporary directory"),
|
||||
):
|
||||
clone_repository(
|
||||
"https://example.com/repo.git",
|
||||
"run_123",
|
||||
dest_name="external_symlink",
|
||||
)
|
||||
|
||||
# Verify external directory and canary were not touched by shutil.rmtree
|
||||
assert outside_dir.exists()
|
||||
assert canary_file.exists()
|
||||
assert canary_file.read_text(encoding="utf-8") == "critical data"
|
||||
mock_run.assert_not_called()
|
||||
|
||||
|
||||
def test_assign_workspace_subdirs_avoids_collisions_with_generated_suffixes() -> None:
|
||||
targets_info: list[dict[str, Any]] = [
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/foo.git"}},
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/.foo.git"}},
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/foo-2.git"}},
|
||||
]
|
||||
assign_workspace_subdirs(targets_info)
|
||||
subdirs = [t["details"]["workspace_subdir"] for t in targets_info]
|
||||
assert len(subdirs) == len(set(subdirs)), f"Collisions found: {subdirs}"
|
||||
assert subdirs == ["foo", "foo-2", "foo-2-2"]
|
||||
|
||||
# Also verify repeated names increment cleanly
|
||||
repeated_targets: list[dict[str, Any]] = [
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/bar.git"}},
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/bar.git"}},
|
||||
{"type": "repository", "details": {"target_repo": "https://example.com/bar.git"}},
|
||||
]
|
||||
assign_workspace_subdirs(repeated_targets)
|
||||
repeated_subdirs = [t["details"]["workspace_subdir"] for t in repeated_targets]
|
||||
assert repeated_subdirs == ["bar", "bar-2", "bar-3"]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue