diff --git a/pythonlings/core/manifest.py b/pythonlings/core/manifest.py index fcf41e0..f694ccb 100644 --- a/pythonlings/core/manifest.py +++ b/pythonlings/core/manifest.py @@ -55,7 +55,10 @@ def load(root: Path) -> Manifest: ) with info_path.open("rb") as f: - data = tomllib.load(f) + try: + data = tomllib.load(f) + except tomllib.TOMLDecodeError as e: + raise ManifestError(f"info.toml is not valid TOML: {e}") from e if data.get("format_version") != 1: raise ManifestError( @@ -63,29 +66,72 @@ def load(root: Path) -> Manifest: ) raw_exercises = data.get("exercises", []) - if not raw_exercises: + if not isinstance(raw_exercises, list) or not raw_exercises: raise ManifestError("info.toml must define a non-empty [[exercises]] array") seen: set[str] = set() exercises: list[Exercise] = [] for entry in raw_exercises: - name = entry["name"] + if not isinstance(entry, dict): + raise ManifestError( + f"info.toml [[exercises]] entries must be tables, got {entry!r}" + ) + + name = entry.get("name") + if not isinstance(name, str) or not name: + raise ManifestError( + f"info.toml exercise entry is missing a valid 'name': {entry!r}" + ) if name in seen: raise ManifestError(f"duplicate exercise name: {name!r}") seen.add(name) - rel_path = Path(entry["path"]) - if not rel_path.parts or rel_path.parts[0] != "exercises": + raw_path = entry.get("path") + if not isinstance(raw_path, str) or not raw_path: + raise ManifestError(f"exercise {name!r} is missing a valid 'path'") + + rel_path = Path(raw_path) + if ( + rel_path.is_absolute() + or ".." in rel_path.parts + or not rel_path.parts + or rel_path.parts[0] != "exercises" + ): raise ManifestError( - f"exercise path must be under exercises/: {rel_path}" + f"exercise {name!r} path must be a relative path under " + f"exercises/, with no '..' components: {raw_path!r}" ) abs_path = root / rel_path + # The lexical checks above reject '..' segments and absolute + # paths in the *written* path string, but a symlink inside + # exercises/ can still resolve outside the workspace even + # when the written path looks clean (e.g. exercises/link/a.py + # where "link" is a symlink pointing elsewhere). Resolve and + # confirm containment before touching the filesystem further. + exercises_root = (root / "exercises").resolve() + resolved_abs_path = abs_path.resolve() + if resolved_abs_path != exercises_root and not resolved_abs_path.is_relative_to( + exercises_root + ): + raise ManifestError( + f"exercise {name!r} path escapes the workspace exercises/ " + f"directory via a symlink: {raw_path!r}" + ) if not abs_path.exists(): raise ManifestError(f"exercise path does not exist: {rel_path}") # Derive the check path: exercises/<...> mirrors to checks/<...>. check_rel = Path("checks", *rel_path.parts[1:]) check_abs = root / check_rel + checks_root = (root / "checks").resolve() + resolved_check_abs = check_abs.resolve() + if resolved_check_abs != checks_root and not resolved_check_abs.is_relative_to( + checks_root + ): + raise ManifestError( + f"check path for {name!r} escapes the workspace checks/ " + f"directory via a symlink: {check_rel}" + ) if not check_abs.exists(): raise ManifestError(f"no check file for {name!r}: {check_rel}") diff --git a/tests/integration/test_cli_verify.py b/tests/integration/test_cli_verify.py index 0e3fc07..b38be26 100644 --- a/tests/integration/test_cli_verify.py +++ b/tests/integration/test_cli_verify.py @@ -64,3 +64,27 @@ def test_verify_reports_manifest_error_with_exit_2(tmp_path: Path) -> None: result = _run("--root", str(tmp_path), "verify") assert result.returncode == 2 assert "info.toml" in result.stderr + + +def test_verify_malformed_toml_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text("format_version = [1\n", encoding="utf-8") + result = _run("--root", str(tmp_path), "verify") + assert result.returncode == 2 + assert "info.toml" in result.stderr + assert "Traceback" not in result.stderr + assert result.stderr.startswith("pythonlings:") + + +def test_verify_traversal_path_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + 'format_version = 1\n' + '[[exercises]]\n' + 'name = "a"\n' + 'path = "exercises/../../etc/passwd"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + result = _run("--root", str(tmp_path), "verify") + assert result.returncode == 2 + assert "Traceback" not in result.stderr + assert result.stderr.startswith("pythonlings:") diff --git a/tests/unit/test_manifest.py b/tests/unit/test_manifest.py index 43823bf..2858972 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -210,3 +210,99 @@ def test_real_curriculum_check_files_parse() -> None: exercise.check_path.read_text(encoding="utf-8"), filename=str(exercise.check_path), ) + + +def test_load_rejects_invalid_toml_syntax(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text("format_version = [1\n", encoding="utf-8") + with pytest.raises(ManifestError, match=r"info\.toml"): + load(tmp_path) + + +def test_load_rejects_missing_name_field(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'path = "exercises/a.py"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="'name'"): + load(tmp_path) + + +def test_load_rejects_wrong_type_name_field(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + "name = 123\n" + 'path = "exercises/a.py"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="'name'"): + load(tmp_path) + + +def test_load_rejects_missing_path_field(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="'path'"): + load(tmp_path) + + +def test_load_rejects_absolute_path(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "/etc/passwd"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="under exercises/"): + load(tmp_path) + + +def test_load_rejects_traversal_path(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/../../etc/passwd"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="under exercises/"): + load(tmp_path) + + +def test_load_rejects_symlink_escape(tmp_path: Path) -> None: + """A path that is lexically clean (no '..', not absolute, starts with + exercises/) can still resolve outside the workspace via a symlink, e.g. + exercises/link/secret.py where "link" is a symlink to somewhere else. + The lexical checks alone don't catch this -- containment must be + verified against the *resolved* path. + """ + outside = tmp_path.parent / "outside_workspace" + outside.mkdir(exist_ok=True) + (outside / "secret.py").write_text("SECRET = 1\n", encoding="utf-8") + + (tmp_path / "exercises").mkdir() + (tmp_path / "exercises" / "link").symlink_to(outside, target_is_directory=True) + (tmp_path / "checks").mkdir() + + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/link/secret.py"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + with pytest.raises(ManifestError, match="escapes the workspace"): + load(tmp_path)