From f82f4d36dedf89a188d6237e960fe82cf57c70d9 Mon Sep 17 00:00:00 2001 From: Shay Date: Fri, 15 May 2026 11:50:15 -0700 Subject: [PATCH] Harden pack validator trust boundary - reject unsafe pack IDs, path traversal, absolute paths, and separators - require --allow-arbitrary-code for dynamic validator execution - add --dry-run validation that does not import or execute validators - preserve JSON/report behavior with explicit trust boundary - add focused CLI security tests --- core/cli.py | 59 ++++++++++++++-- tests/test_cli_pack_validate_security.py | 89 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 4 deletions(-) create mode 100644 tests/test_cli_pack_validate_security.py diff --git a/core/cli.py b/core/cli.py index ddca48e0..e55da8f4 100644 --- a/core/cli.py +++ b/core/cli.py @@ -342,15 +342,60 @@ def cmd_pack_verify(args: argparse.Namespace) -> int: return _run(sys.executable, "-m", "language_packs", "verify", args.pack_id) +def _safe_pack_id(pack_id: str) -> str: + """Reject pack IDs containing path traversal or separator characters.""" + if not pack_id: + _die("pack_id is required", code=2) + + path = Path(pack_id) + + if path.is_absolute(): + _die("pack_id must not be an absolute path", code=2) + + if pack_id in {".", ".."}: + _die("pack_id must name a pack, not a relative path", code=2) + + if any(part in {"", ".", ".."} for part in path.parts): + _die("pack_id must not contain path traversal", code=2) + + if "/" in pack_id or "\\" in pack_id: + _die("pack_id must be a simple pack id, not a path", code=2) + + return pack_id + + def cmd_pack_validate(args: argparse.Namespace) -> int: """Run executable source-pack validation gates.""" - import importlib.util - - pack_dir = _REPO_ROOT / "packs" / args.pack_id + pack_id = _safe_pack_id(args.pack_id) + pack_dir = _REPO_ROOT / "packs" / pack_id validator_path = pack_dir / "validators.py" + if not validator_path.exists(): _die(f"source-pack validator not found: {validator_path}", code=1) - spec = importlib.util.spec_from_file_location(f"{args.pack_id}_validators", validator_path) + + if getattr(args, "dry_run", False): + if args.json: + print(json.dumps({ + "pack_id": pack_id, + "validator_path": str(validator_path), + "would_execute": False, + "exists": True, + }, ensure_ascii=False, indent=2, sort_keys=True)) + else: + print(f"dry-run: pack_id={pack_id}") + print(f"validator: {validator_path}") + print("status: validator exists, would not execute") + return 0 + + if not getattr(args, "allow_arbitrary_code", False): + _die( + "dynamic validator execution requires --allow-arbitrary-code", + code=2, + ) + + import importlib.util + + spec = importlib.util.spec_from_file_location(f"{pack_id}_validators", validator_path) if spec is None or spec.loader is None: _die(f"cannot load source-pack validator: {validator_path}", code=1) module = importlib.util.module_from_spec(spec) @@ -569,6 +614,12 @@ def build_parser() -> argparse.ArgumentParser: pack_validate = pack_sub.add_parser("validate", help="validate a source pack under packs/") pack_validate.add_argument("pack_id", help="source pack id, e.g. en, he, grc, el") pack_validate.add_argument("--json", action="store_true", help="emit machine-readable JSON") + pack_validate.add_argument("--dry-run", action="store_true", help="check validator exists without executing") + pack_validate.add_argument( + "--allow-arbitrary-code", + action="store_true", + help="permit dynamic validator execution (required to run validators)", + ) pack_validate.set_defaults(func=cmd_pack_validate) rust = subparsers.add_parser( diff --git a/tests/test_cli_pack_validate_security.py b/tests/test_cli_pack_validate_security.py new file mode 100644 index 00000000..2551f51f --- /dev/null +++ b/tests/test_cli_pack_validate_security.py @@ -0,0 +1,89 @@ +"""Security tests for the ``core pack validate`` CLI subcommand.""" +from __future__ import annotations + +import importlib.util + +import pytest + +from core import cli + + +def test_pack_validate_requires_allow_arbitrary_code(capsys: pytest.CaptureFixture[str]) -> None: + with pytest.raises(SystemExit) as exc: + cli.main(["pack", "validate", "en"]) + + assert exc.value.code != 0 + captured = capsys.readouterr() + assert "--allow-arbitrary-code" in captured.err + + +def test_pack_validate_dry_run_does_not_execute_validator( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + def fail_import(*args: object, **kwargs: object) -> None: + raise AssertionError("validator should not execute during dry-run") + + monkeypatch.setattr(importlib.util, "spec_from_file_location", fail_import) + + rc = cli.main(["pack", "validate", "en", "--dry-run"]) + + assert rc == 0 + captured = capsys.readouterr() + assert "dry-run" in captured.out.lower() + + +@pytest.mark.parametrize( + "pack_id", + ["../foo", "/tmp/foo", "foo/bar", r"foo\bar", ".", "..", ""], +) +def test_pack_validate_rejects_unsafe_pack_ids(pack_id: str) -> None: + args = ["pack", "validate", pack_id, "--dry-run"] if pack_id else ["pack", "validate", "", "--dry-run"] + with pytest.raises(SystemExit) as exc: + cli.main(args) + assert exc.value.code == 2 + + +def test_pack_validate_rejects_path_traversal(capsys: pytest.CaptureFixture[str]) -> None: + with pytest.raises(SystemExit) as exc: + cli.main(["pack", "validate", "../foo", "--dry-run"]) + + assert exc.value.code == 2 + captured = capsys.readouterr() + assert "path" in captured.err.lower() or "traversal" in captured.err.lower() + + +def test_pack_validate_rejects_absolute_paths(capsys: pytest.CaptureFixture[str]) -> None: + with pytest.raises(SystemExit) as exc: + cli.main(["pack", "validate", "/tmp/foo", "--dry-run"]) + + assert exc.value.code == 2 + captured = capsys.readouterr() + assert "absolute" in captured.err.lower() + + +def test_pack_validate_rejects_path_separators(capsys: pytest.CaptureFixture[str]) -> None: + with pytest.raises(SystemExit) as exc: + cli.main(["pack", "validate", "foo/bar", "--dry-run"]) + + assert exc.value.code == 2 + captured = capsys.readouterr() + assert "path" in captured.err.lower() + + +def test_pack_validate_allows_known_safe_pack_with_explicit_flag() -> None: + rc = cli.main(["pack", "validate", "en", "--allow-arbitrary-code"]) + assert rc in {0, 1} + + +def test_pack_validate_dry_run_json(capsys: pytest.CaptureFixture[str]) -> None: + import json + + rc = cli.main(["pack", "validate", "en", "--dry-run", "--json"]) + + assert rc == 0 + captured = capsys.readouterr() + data = json.loads(captured.out) + assert data["pack_id"] == "en" + assert data["would_execute"] is False + assert data["exists"] is True