From 9b9458847f490d6d5502c60a657a8ac90df98e30 Mon Sep 17 00:00:00 2001 From: Song Yikun Date: Tue, 19 May 2026 18:22:22 +0800 Subject: [PATCH 1/2] fix(warehouse): normalize --paths input before tracked-set membership check (PER-184) `abc warehouse contribute --paths` was rejecting paths with `./` prefix or redundant `//` separators because the membership check was a direct string comparison against the tracked set. Add `_normalize_path()` helper that: - Strips `./` and collapses `//` via `os.path.normpath`. - Rejects absolute paths with a clear error. - Rejects parent-directory (`..`) traversal with a clear error. - Returns POSIX-style forward slashes for cross-platform consistency. Apply normalization to every `--paths` entry before checking membership against `tracked_set`, and use the normalized form as the commit-paths list so git sees the same canonical strings. Test suite extended with 4 new cases under `TestContributePaths`: leading `./`, double `//`, absolute rejection, and `..` rejection. Closes PER-184. --- .../beacon/domains/warehouse/contribute.py | 27 ++++++- .../tests/unit/warehouse/test_contribute.py | 74 +++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/libs/beacon/src/beacon/domains/warehouse/contribute.py b/libs/beacon/src/beacon/domains/warehouse/contribute.py index 8f45987..cb71a48 100644 --- a/libs/beacon/src/beacon/domains/warehouse/contribute.py +++ b/libs/beacon/src/beacon/domains/warehouse/contribute.py @@ -4,6 +4,7 @@ driven by a project's .agentic-beacon/config.toml and beacon.yaml. """ +import os import subprocess from dataclasses import dataclass from pathlib import Path @@ -36,6 +37,27 @@ def _run_git( ) +def _normalize_path(path: str) -> str: + """Normalize a user-supplied warehouse-relative path for membership checking. + + * Converts to POSIX-style forward slashes. + * Removes leading './' segments. + * Collapses redundant separators (e.g. 'skills//foo' -> 'skills/foo'). + * Rejects absolute paths and parent-directory traversal. + """ + p = Path(path) + if p.is_absolute(): + raise ValueError(f"Absolute paths are not allowed: {path!r}") + parts = p.parts + if ".." in parts: + raise ValueError(f"Parent-directory traversal is not allowed: {path!r}") + # normpath removes './' and collapses redundant separators + normalized = os.path.normpath(path) + # Ensure POSIX-style forward slashes for cross-platform consistency + normalized = normalized.replace(os.sep, "/") + return normalized + + def _count_dirty_outside_scope(warehouse_path: Path, tracked: list[str]) -> int: """Return number of dirty files outside the tracked set.""" full = _run_git(warehouse_path, ["status", "--porcelain"]) @@ -91,15 +113,16 @@ def contribute( tracked_paths = get_tracked_paths(warehouse_path, beacon_yaml) if paths is not None: + normalized_paths = [_normalize_path(p) for p in paths] tracked_set = set(tracked_paths) - untracked = [p for p in paths if p not in tracked_set] + untracked = [p for p in normalized_paths if p not in tracked_set] if untracked: raise ValueError( f"The following paths are not tracked by beacon.yaml and cannot be committed: " f"{', '.join(repr(p) for p in untracked)}" ) # Use the caller-supplied paths (preserving their order), scoped within tracked_paths - commit_paths = list(paths) + commit_paths = normalized_paths else: commit_paths = tracked_paths diff --git a/libs/beacon/tests/unit/warehouse/test_contribute.py b/libs/beacon/tests/unit/warehouse/test_contribute.py index 5a82d24..66494f0 100644 --- a/libs/beacon/tests/unit/warehouse/test_contribute.py +++ b/libs/beacon/tests/unit/warehouse/test_contribute.py @@ -448,6 +448,80 @@ def test_contribute_paths_sequential_groups_leave_others_dirty( assert "commit b" in log.stdout assert "commit a" in log.stdout + def test_contribute_paths_leading_dot_slash(self, contrib_project_multi): + """Leading './' is normalized away before membership check.""" + project, wh = contrib_project_multi + env = _git_env() + + (wh / "contexts" / "a.md").write_text("# a modified\n") + + result = contribute( + project, + message="commit with ./prefix", + push=False, + paths=("./contexts/a.md",), + ) + assert result.status == "committed" + + committed_files = subprocess.run( + ["git", "show", "--name-only", "--format=", "HEAD"], + cwd=wh, + env=env, + capture_output=True, + text=True, + check=True, + ) + assert "contexts/a.md" in committed_files.stdout + + def test_contribute_paths_double_slash(self, contrib_project_multi): + """Redundant '//' separators are collapsed.""" + project, wh = contrib_project_multi + env = _git_env() + + (wh / "contexts" / "a.md").write_text("# a modified\n") + + result = contribute( + project, + message="commit with double slash", + push=False, + paths=("contexts//a.md",), + ) + assert result.status == "committed" + + committed_files = subprocess.run( + ["git", "show", "--name-only", "--format=", "HEAD"], + cwd=wh, + env=env, + capture_output=True, + text=True, + check=True, + ) + assert "contexts/a.md" in committed_files.stdout + + def test_contribute_paths_absolute_rejected(self, contrib_project_multi): + """Absolute paths raise ValueError.""" + project, wh = contrib_project_multi + with pytest.raises(ValueError) as exc_info: + contribute( + project, + message="should fail", + push=False, + paths=("/absolute/path.md",), + ) + assert "absolute" in str(exc_info.value).lower() + + def test_contribute_paths_dotdot_rejected(self, contrib_project_multi): + """Paths with '..' raise ValueError.""" + project, wh = contrib_project_multi + with pytest.raises(ValueError) as exc_info: + contribute( + project, + message="should fail", + push=False, + paths=("../outside.md",), + ) + assert "parent-directory" in str(exc_info.value).lower() + class TestDirtyOutsideScope: """Tests for PER-159: out-of-scope dirty file count in contribute.""" From b8d7288ce0b9a7c1bcdac9fd6a3a02815d6ce7c5 Mon Sep 17 00:00:00 2001 From: Song Yikun Date: Wed, 20 May 2026 13:12:27 +0800 Subject: [PATCH 2/2] refactor(warehouse): move path normalization to utils/paths.py (PER-184) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per PR #151 review: `_normalize_path` is a stateless helper with no warehouse-domain knowledge — it belongs in utils/, not buried as a private domain helper. - New `beacon/utils/paths.py` with public `normalize_relative_path()`. - `contribute.py` imports it; drops the now-unused `import os`. - Add direct unit coverage in `tests/unit/utils/test_paths.py` (the contribute tests already cover it behaviorally). No behaviour change. Co-Authored-By: Claude Opus 4.7 --- .../beacon/domains/warehouse/contribute.py | 25 +---------- libs/beacon/src/beacon/utils/paths.py | 28 ++++++++++++ libs/beacon/tests/unit/utils/test_paths.py | 43 +++++++++++++++++++ 3 files changed, 73 insertions(+), 23 deletions(-) create mode 100644 libs/beacon/src/beacon/utils/paths.py create mode 100644 libs/beacon/tests/unit/utils/test_paths.py diff --git a/libs/beacon/src/beacon/domains/warehouse/contribute.py b/libs/beacon/src/beacon/domains/warehouse/contribute.py index cb71a48..c1b485f 100644 --- a/libs/beacon/src/beacon/domains/warehouse/contribute.py +++ b/libs/beacon/src/beacon/domains/warehouse/contribute.py @@ -4,7 +4,6 @@ driven by a project's .agentic-beacon/config.toml and beacon.yaml. """ -import os import subprocess from dataclasses import dataclass from pathlib import Path @@ -13,6 +12,7 @@ from beacon.domains.warehouse._tracked_paths import get_tracked_paths from beacon.domains.warehouse.preconditions import ensure_sync_ready +from beacon.utils.paths import normalize_relative_path @dataclass @@ -37,27 +37,6 @@ def _run_git( ) -def _normalize_path(path: str) -> str: - """Normalize a user-supplied warehouse-relative path for membership checking. - - * Converts to POSIX-style forward slashes. - * Removes leading './' segments. - * Collapses redundant separators (e.g. 'skills//foo' -> 'skills/foo'). - * Rejects absolute paths and parent-directory traversal. - """ - p = Path(path) - if p.is_absolute(): - raise ValueError(f"Absolute paths are not allowed: {path!r}") - parts = p.parts - if ".." in parts: - raise ValueError(f"Parent-directory traversal is not allowed: {path!r}") - # normpath removes './' and collapses redundant separators - normalized = os.path.normpath(path) - # Ensure POSIX-style forward slashes for cross-platform consistency - normalized = normalized.replace(os.sep, "/") - return normalized - - def _count_dirty_outside_scope(warehouse_path: Path, tracked: list[str]) -> int: """Return number of dirty files outside the tracked set.""" full = _run_git(warehouse_path, ["status", "--porcelain"]) @@ -113,7 +92,7 @@ def contribute( tracked_paths = get_tracked_paths(warehouse_path, beacon_yaml) if paths is not None: - normalized_paths = [_normalize_path(p) for p in paths] + normalized_paths = [normalize_relative_path(p) for p in paths] tracked_set = set(tracked_paths) untracked = [p for p in normalized_paths if p not in tracked_set] if untracked: diff --git a/libs/beacon/src/beacon/utils/paths.py b/libs/beacon/src/beacon/utils/paths.py new file mode 100644 index 0000000..c96dcdd --- /dev/null +++ b/libs/beacon/src/beacon/utils/paths.py @@ -0,0 +1,28 @@ +"""Stateless path helpers with no domain knowledge.""" + +import os +from pathlib import Path + + +def normalize_relative_path(path: str) -> str: + """Normalize a user-supplied relative path for membership checking. + + * Converts to POSIX-style forward slashes. + * Removes leading './' segments. + * Collapses redundant separators (e.g. 'skills//foo' -> 'skills/foo'). + * Rejects absolute paths and parent-directory traversal. + + Raises: + ValueError: if the path is absolute or contains a '..' component. + """ + p = Path(path) + if p.is_absolute(): + raise ValueError(f"Absolute paths are not allowed: {path!r}") + parts = p.parts + if ".." in parts: + raise ValueError(f"Parent-directory traversal is not allowed: {path!r}") + # normpath removes './' and collapses redundant separators + normalized = os.path.normpath(path) + # Ensure POSIX-style forward slashes for cross-platform consistency + normalized = normalized.replace(os.sep, "/") + return normalized diff --git a/libs/beacon/tests/unit/utils/test_paths.py b/libs/beacon/tests/unit/utils/test_paths.py new file mode 100644 index 0000000..288ab64 --- /dev/null +++ b/libs/beacon/tests/unit/utils/test_paths.py @@ -0,0 +1,43 @@ +"""Unit tests for path normalization utilities.""" + +import pytest +from beacon.utils.paths import normalize_relative_path + + +class TestNormalizeRelativePath: + """Test cases for normalize_relative_path (PER-184).""" + + def test_plain_path_unchanged(self): + """A clean relative path passes through unchanged.""" + assert normalize_relative_path("contexts/a.md") == "contexts/a.md" + + def test_leading_dot_slash_stripped(self): + """Leading './' is removed.""" + assert normalize_relative_path("./contexts/a.md") == "contexts/a.md" + + def test_double_slash_collapsed(self): + """Redundant '//' separators are collapsed.""" + assert normalize_relative_path("contexts//a.md") == "contexts/a.md" + + def test_nested_dot_segments_collapsed(self): + """Interior '.' segments are collapsed.""" + assert normalize_relative_path("contexts/./a.md") == "contexts/a.md" + + def test_absolute_path_rejected(self): + """Absolute paths raise ValueError.""" + with pytest.raises(ValueError, match="[Aa]bsolute"): + normalize_relative_path("/absolute/path.md") + + def test_parent_traversal_rejected(self): + """Paths containing '..' raise ValueError.""" + with pytest.raises(ValueError, match="[Pp]arent-directory"): + normalize_relative_path("../outside.md") + + def test_parent_traversal_interior_rejected(self): + """A '..' component anywhere in the path is rejected.""" + with pytest.raises(ValueError, match="[Pp]arent-directory"): + normalize_relative_path("contexts/../../../etc/passwd") + + def test_dotdot_in_filename_not_rejected(self): + """A literal '..' inside a filename is not a traversal component.""" + assert normalize_relative_path("contexts/foo..bar.md") == "contexts/foo..bar.md"