From 3636546c9842e219fc15a9b51214aaa905f9d03a Mon Sep 17 00:00:00 2001 From: Agastya Date: Sun, 9 Aug 2026 21:44:13 +0530 Subject: [PATCH 1/2] Wrap malformed info.toml failures in ManifestError load() previously let several failure modes escape as raw exceptions instead of ManifestError, so the CLI's top-level ManifestError handler (which prints a friendly `pythonlings: ...` message and exits 2) never caught them and a full traceback leaked to the user instead: - invalid TOML syntax raised tomllib.TOMLDecodeError - a missing/wrongly-typed `name` or `path` field raised KeyError or a downstream TypeError - an absolute or `..`-traversal path was not explicitly rejected and could reach outside the exercises/ tree All three now raise a contextual ManifestError before any unsafe filesystem access, matching the existing behavior for the already-handled cases (missing info.toml, bad format_version, empty exercises list, duplicate names, missing exercise/check files). Adds unit tests for each new rejection path plus two CLI integration tests asserting exit code 2 with no traceback text in stderr. --- pythonlings/core/manifest.py | 34 +++++++++++--- tests/integration/test_cli_verify.py | 24 ++++++++++ tests/unit/test_manifest.py | 69 ++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 6 deletions(-) diff --git a/pythonlings/core/manifest.py b/pythonlings/core/manifest.py index fcf41e0..49b4301 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,21 +66,40 @@ 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 if not abs_path.exists(): 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..ff1ed11 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -210,3 +210,72 @@ 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="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) From 4f041b115d5bfa727e32b16c9c4e3a73dd33f6e9 Mon Sep 17 00:00:00 2001 From: agu2347 <94227848+agu2347@users.noreply.github.com> Date: Mon, 10 Aug 2026 20:46:34 +0530 Subject: [PATCH 2/2] Address CodeRabbit review: reject symlink escapes, fix RUF043 The lexical path checks (no absolute path, no '..' components, starts with exercises/) accept exercises/link/file.py even when 'link' is a symlink pointing outside the workspace -- CodeRabbit correctly flagged that a value can look clean lexically and still resolve elsewhere. Now resolve() both the exercise path and the derived check path and confirm they stay within the resolved exercises/ and checks/ directories before touching the filesystem further, raising ManifestError otherwise. Added a regression test that creates a real symlink escaping the workspace and confirms it raises; reverting the fix makes this test fail (with a different, wrong error), proving it's a real check. Also fixed the RUF043 warning on the new test_load_rejects_invalid_toml_syntax test: match="info.toml" treated '.' as a regex wildcard; changed to the raw/escaped match=r"info\.toml". tests/unit/test_manifest.py + tests/integration/test_cli_verify.py: 31 passed. ruff check: clean (the one PLW1510 warning ruff reports is pre-existing in test_cli_verify.py's _run() helper, untouched by this diff, as already noted in the original PR). --- pythonlings/core/manifest.py | 24 ++++++++++++++++++++++++ tests/unit/test_manifest.py | 29 ++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/pythonlings/core/manifest.py b/pythonlings/core/manifest.py index 49b4301..f694ccb 100644 --- a/pythonlings/core/manifest.py +++ b/pythonlings/core/manifest.py @@ -102,12 +102,36 @@ def load(root: Path) -> Manifest: 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/unit/test_manifest.py b/tests/unit/test_manifest.py index ff1ed11..2858972 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -214,7 +214,7 @@ def test_real_curriculum_check_files_parse() -> None: 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="info.toml"): + with pytest.raises(ManifestError, match=r"info\.toml"): load(tmp_path) @@ -279,3 +279,30 @@ def test_load_rejects_traversal_path(tmp_path: Path) -> None: ) 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)