diff --git a/pythonlings/core/manifest.py b/pythonlings/core/manifest.py index fcf41e0..2727f6b 100644 --- a/pythonlings/core/manifest.py +++ b/pythonlings/core/manifest.py @@ -15,6 +15,14 @@ class ManifestError(ValueError): """info.toml is missing, malformed, or fails validation.""" +def _resolve_path(path: Path, description: str) -> Path: + """Resolve a manifest path and translate filesystem failures.""" + try: + return path.resolve() + except (OSError, RuntimeError) as e: + raise ManifestError(f"could not resolve {description} at {path}: {e}") from e + + @dataclass(frozen=True) class Manifest: exercises: list[Exercise] @@ -54,40 +62,116 @@ def load(root: Path) -> Manifest: "Run 'pythonlings init' to create one." ) - with info_path.open("rb") as f: - data = tomllib.load(f) + try: + with info_path.open("rb") as f: + data = tomllib.load(f) + except UnicodeDecodeError as e: + raise ManifestError(f"info.toml is not valid UTF-8: {e}") from e + except tomllib.TOMLDecodeError as e: + raise ManifestError(f"info.toml is not valid TOML: {e}") from e + except OSError as e: + raise ManifestError(f"could not read info.toml at {info_path}: {e}") from e if data.get("format_version") != 1: raise ManifestError( f"info.toml format_version must be 1, got {data.get('format_version')!r}" ) + welcome_message = data.get("welcome_message", "Welcome to pythonlings!") + final_message = data.get("final_message", "All exercises complete.") + for field, value in ( + ("welcome_message", welcome_message), + ("final_message", final_message), + ): + if not isinstance(value, str): + raise ManifestError(f"info.toml {field!r} must be a string") + 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") + resolved_root = _resolve_path(root, "workspace root") + exercises_root = _resolve_path(root / "exercises", "workspace exercises/ directory") + checks_root = _resolve_path(root / "checks", "workspace checks/ directory") + for directory, resolved_directory in ( + ("exercises", exercises_root), + ("checks", checks_root), + ): + if not resolved_directory.is_relative_to(resolved_root): + raise ManifestError( + f"workspace {directory}/ directory escapes the workspace root " + "via a symlink" + ) + 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 - if not abs_path.exists(): - raise ManifestError(f"exercise path does not exist: {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. + resolved_abs_path = _resolve_path(abs_path, f"exercise path for {name!r}") + if 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.is_file(): + raise ManifestError(f"exercise path is not a file: {rel_path}") # Derive the check path: exercises/<...> mirrors to checks/<...>. check_rel = Path("checks", *rel_path.parts[1:]) check_abs = root / check_rel - if not check_abs.exists(): - raise ManifestError(f"no check file for {name!r}: {check_rel}") + resolved_check_abs = _resolve_path(check_abs, f"check path for {name!r}") + if 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.is_file(): + raise ManifestError( + f"no check file for {name!r}; path is not a file: {check_rel}" + ) + + hint = entry.get("hint", "") + docs = entry.get("docs", "") + for field, value in (("hint", hint), ("docs", docs)): + if not isinstance(value, str): + raise ManifestError( + f"exercise {name!r} field {field!r} must be a string" + ) exercises.append( Exercise( @@ -95,8 +179,8 @@ def load(root: Path) -> Manifest: path=abs_path, check_path=check_abs, topic=rel_path.parent.name, - hint=entry.get("hint", ""), - docs=entry.get("docs", ""), + hint=hint, + docs=docs, root=root, rel_path=rel_path, check_rel_path=check_rel, @@ -105,6 +189,6 @@ def load(root: Path) -> Manifest: return Manifest( exercises=exercises, - welcome_message=data.get("welcome_message", "Welcome to pythonlings!"), - final_message=data.get("final_message", "All exercises complete."), + welcome_message=welcome_message, + final_message=final_message, ) diff --git a/tests/integration/test_cli_verify.py b/tests/integration/test_cli_verify.py index 0e3fc07..af525ba 100644 --- a/tests/integration/test_cli_verify.py +++ b/tests/integration/test_cli_verify.py @@ -64,3 +64,80 @@ 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_invalid_utf8_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_bytes(b"\xff") + result = _run("--root", str(tmp_path), "verify") + assert result.returncode == 2 + assert "valid UTF-8" in result.stderr + assert "Traceback" not in result.stderr + + +def test_verify_info_toml_read_error_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "info.toml").mkdir() + result = _run("--root", str(tmp_path), "verify") + assert result.returncode == 2 + assert "could not read info.toml" in result.stderr + assert "Traceback" not in result.stderr + + +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:") + + +def test_verify_directory_path_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "exercises" / "topic").mkdir(parents=True) + (tmp_path / "checks" / "topic").mkdir(parents=True) + (tmp_path / "info.toml").write_text( + 'format_version = 1\n' + '[[exercises]]\n' + 'name = "a"\n' + 'path = "exercises/topic"\n', + encoding="utf-8", + ) + + result = _run("--root", str(tmp_path), "verify") + assert result.returncode == 2 + assert "exercise path is not a file" in result.stderr + assert "Traceback" not in result.stderr + + +def test_hint_non_string_field_exits_2_without_traceback(tmp_path: Path) -> None: + (tmp_path / "exercises").mkdir() + (tmp_path / "exercises" / "a.py").write_text("", encoding="utf-8") + (tmp_path / "checks").mkdir() + (tmp_path / "checks" / "a.py").write_text("", encoding="utf-8") + (tmp_path / "info.toml").write_text( + 'format_version = 1\n' + '[[exercises]]\n' + 'name = "a"\n' + 'path = "exercises/a.py"\n' + 'hint = 1\n', + encoding="utf-8", + ) + + result = _run("--root", str(tmp_path), "hint", "a") + assert result.returncode == 2 + assert "hint" in result.stderr + assert "Traceback" not in result.stderr diff --git a/tests/unit/test_manifest.py b/tests/unit/test_manifest.py index 43823bf..6837f3f 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -210,3 +210,253 @@ 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_invalid_utf8(tmp_path: Path) -> None: + (tmp_path / "info.toml").write_bytes(b"\xff") + with pytest.raises(ManifestError, match="valid UTF-8"): + load(tmp_path) + + +def test_load_wraps_info_toml_read_errors(tmp_path: Path) -> None: + (tmp_path / "info.toml").mkdir() + with pytest.raises(ManifestError, match="could not read 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) + + +def test_load_rejects_check_symlink_escape(tmp_path: Path) -> None: + outside = tmp_path.parent / f"{tmp_path.name}_outside_checks" + outside.mkdir() + (outside / "a.py").write_text("assert True\n", encoding="utf-8") + + (tmp_path / "exercises" / "topic").mkdir(parents=True) + (tmp_path / "exercises" / "topic" / "a.py").write_text("", encoding="utf-8") + (tmp_path / "checks").mkdir() + (tmp_path / "checks" / "topic").symlink_to(outside, target_is_directory=True) + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/topic/a.py"\n' + 'hint = "h"\n', + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match="check path.*escapes the workspace"): + load(tmp_path) + + +def test_load_rejects_top_level_exercises_symlink_escape(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + outside = tmp_path / "outside_exercises" + workspace.mkdir() + outside.mkdir() + (outside / "a.py").write_text("", encoding="utf-8") + (workspace / "exercises").symlink_to(outside, target_is_directory=True) + (workspace / "checks").mkdir() + (workspace / "checks" / "a.py").write_text("", encoding="utf-8") + (workspace / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/a.py"\n', + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match="exercises/ directory escapes"): + load(workspace) + + +def test_load_rejects_top_level_checks_symlink_escape(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + outside = tmp_path / "outside_checks" + workspace.mkdir() + outside.mkdir() + (outside / "a.py").write_text("", encoding="utf-8") + (workspace / "checks").symlink_to(outside, target_is_directory=True) + (workspace / "exercises").mkdir() + (workspace / "exercises" / "a.py").write_text("", encoding="utf-8") + (workspace / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/a.py"\n', + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match="checks/ directory escapes"): + load(workspace) + + +def test_load_rejects_directory_exercise_path(tmp_path: Path) -> None: + (tmp_path / "exercises" / "topic").mkdir(parents=True) + (tmp_path / "checks" / "topic").mkdir(parents=True) + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/topic"\n', + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match="exercise path is not a file"): + load(tmp_path) + + +def test_load_rejects_directory_check_path(tmp_path: Path) -> None: + (tmp_path / "exercises" / "topic").mkdir(parents=True) + (tmp_path / "exercises" / "topic" / "a.py").write_text("", encoding="utf-8") + (tmp_path / "checks" / "topic" / "a.py").mkdir(parents=True) + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/topic/a.py"\n', + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match="path is not a file"): + load(tmp_path) + + +def test_load_wraps_symlink_resolution_errors( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + exercise_path = tmp_path / "exercises" / "a.py" + _write_curriculum(tmp_path, "a") + original_resolve = Path.resolve + + def fail_exercise_resolution(path: Path, strict: bool = False) -> Path: + if path == exercise_path: + raise RuntimeError("symlink loop") + return original_resolve(path, strict=strict) + + monkeypatch.setattr(Path, "resolve", fail_exercise_resolution) + + with pytest.raises(ManifestError, match="could not resolve exercise path"): + load(tmp_path) + + +@pytest.mark.parametrize("field", ["hint", "docs"]) +def test_load_rejects_non_string_exercise_text(tmp_path: Path, field: str) -> None: + _write_curriculum(tmp_path, "a") + (tmp_path / "info.toml").write_text( + "format_version = 1\n" + "[[exercises]]\n" + 'name = "a"\n' + 'path = "exercises/a.py"\n' + f"{field} = 1\n", + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match=field): + load(tmp_path) + + +@pytest.mark.parametrize("field", ["welcome_message", "final_message"]) +def test_load_rejects_non_string_manifest_text(tmp_path: Path, field: str) -> None: + _write_curriculum(tmp_path, "a") + info_path = tmp_path / "info.toml" + info_path.write_text( + f"{field} = 1\n" + info_path.read_text(encoding="utf-8"), + encoding="utf-8", + ) + + with pytest.raises(ManifestError, match=field): + load(tmp_path)