diff --git a/CLAUDE.md b/CLAUDE.md index 5ee0e8e..42261c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,17 +11,19 @@ All source lives in `evaluation_function/`: | `main.py` | IPC server entry point; registers `evaluation_function` and `preview_function` with lf_toolkit | | `evaluation.py` | Core evaluation pipeline: security check → subprocess execution → output comparison → plot upload (GCS/S3 via lf_toolkit) → structured feedback | | `preview.py` | AST-based pre-execution security validator (`_SecurityVisitor`) | +| `s3_files.py` | Downloads `params["answer_files"]`/`params["response_files"]` objects into the per-request working directory | | `dev.py` | CLI wrapper for local manual testing | ### Evaluation pipeline (`evaluation.py`) 1. Run AST security check on student code -2. Dispatch by `params["mode"]` (required): +2. Gather file specs from params via `_collect_file_specs`: `params["answer_files"]` (teacher, plus legacy `params["files"]`) and `params["response_files"]` (student); teacher files win on a name clash. The response and answer are plain code strings. If any files are listed, download the listed objects once into a per-request working directory (see `s3_files.py`), used as the subprocess `cwd` for every run in this request +3. Dispatch by `params["mode"]` (required): - **`demo`**: execute code with no stdin; return stdout/plots as `output` feedback (no pass/fail) - **`io_test`**: for each test in `params["tests"]`, execute with `test["input"]` as stdin and compare stdout against `test["expected_output"]`; upload matplotlib plots on pass or fail - **`unit_test`**: append `params["test_code"]` + unit-runner harness to student code; execute once; parse JSON results; supports plain `test_*` functions, `unittest.TestCase` subclasses, and Hypothesis-based tests -3. Upload any captured matplotlib figures via `lf_toolkit` `upload_image` (`_UPLOAD_FOLDER = "evaluatePython"`); backend is GCS or S3 per `IMAGE_UPLOAD_BACKEND` -4. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary` +4. Upload any captured matplotlib figures via `lf_toolkit` `upload_image` (`_UPLOAD_FOLDER = "evaluatePython"`); backend is GCS or S3 per `IMAGE_UPLOAD_BACKEND` +5. Return a `Result` with feedback tags: `pass`, `fail`, `hidden_fail`, `error`, `output`, `summary` ### Request shape @@ -90,16 +92,41 @@ All source lives in `evaluation_function/`: "pep8_feedback": ["E225", "E231"], # custom rule list "tests": [...] } + +# answer_files / response_files — optional, work with all modes +# answer_files: the teacher's files, saved in the response area's gradeParams. +# response_files: the student's uploads, sent with each check as additionalParams. +# params["files"] is still accepted as a legacy alias for answer_files. +# All listed files are downloaded into one per-request working directory +# (the subprocess's cwd) before student code runs, given a pre-signed or +# public HTTPS URL per file (fetched directly with a GET — no AWS +# credentials needed here). On a name clash the teacher's file wins. Entries +# may be dicts or JSON strings of dicts. Data files can be read with +# open()/pandas.read_csv()/etc.; .py files are importable since they're +# co-located with the generated script. The same files are also available +# to the answer code when use_answer_as_expected_output/use_answer_as_test_code is set. +{ + "mode": "demo", + "answer_files": [ + {"url": "https://.../data.csv?X-Amz-Signature=...", "name": "data.csv"}, + {"url": "https://.../helper.py?X-Amz-Signature=...", "name": "helper.py"}, + ], + "response_files": [ + {"url": "https://.../mine.csv?X-Amz-Signature=...", "name": "mine.csv"}, + ] +} ``` ### Security model (`preview.py`) -`_SecurityVisitor` walks the AST before any execution and blocks: +`_SecurityVisitor` walks the AST and blocks: -- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `pathlib`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins` -- **Builtins**: `exec`, `eval`, `compile`, `open`, `__import__` +- **Modules**: `os`, `sys`, `subprocess`, `socket`, `urllib`, `http`, `requests`, `shutil`, `ftplib`, `smtplib`, `ctypes`, `multiprocessing`, `threading`, `importlib`, `pickle`, `builtins` +- **Builtins**: `exec`, `eval`, `compile`, `__import__` - **Dunder attribute access**: any `__attr__` style attribute +`open`/`pathlib` are intentionally **not** blocked here — they're needed to read files loaded via `params["answer_files"]`/`params["response_files"]` (see above). **Important caveat**: `preview_function` (this check) and `evaluation_function` (actual grading) are registered as two independent RPC methods in `main.py`; `evaluation.py` never calls `preview.py`. This check only powers editor-time linting feedback — it does not gate what code can do at grading time. The real, load-bearing control for file access is a runtime-injected restricted `open`/`io.open` in `evaluation.py`'s subprocess preamble (`_safe_open`), which blocks *write* access to anything inside the per-run files directory. It is not a hard sandbox boundary — since `os`/`subprocess` remain fully importable and runnable at grading time regardless of this feature, a student can bypass file restrictions entirely via `os`. Treat this as scoping the intended file-access path, not as isolation. + ## Key commands ```bash @@ -153,7 +180,7 @@ CI runs on Python 3.12 and uploads JUnit XML results (`.github/workflows/test-li | `LOG_LEVEL` | `debug` | Logging verbosity | | `IMAGE_UPLOAD_BACKEND` | `gcs` | Plot upload backend in lf_toolkit (`gcs` set in Dockerfile; override to `s3` on the service to use AWS) | | `GCS_BUCKET` | Runtime env | Target bucket for matplotlib plot uploads; set per-environment on the Cloud Run service. Auth is via the runtime service account (ADC) — no keys | -| `AWS_*` / `S3_BUCKET_URI` | Runtime env | Only for the legacy S3 plot-upload backend (`IMAGE_UPLOAD_BACKEND=s3`) | +| `AWS_*` / `S3_BUCKET_URI` | Runtime env | Only for the legacy S3 plot-upload backend (`IMAGE_UPLOAD_BACKEND=s3`). Not needed for `answer_files` / `response_files` downloads — those are plain HTTPS GETs from a pre-signed/public URL | | `SANDBOX_ENABLED` | `true` | Wrap the worker in shimmy's nsjail sandbox (needs `--privileged` at run time) | | `SANDBOX_SECCOMP` | `true` | nsjail seccomp syscall filter | | `SANDBOX_RO_BINDS` | `/usr:/lib:/lib64:/bin:/sbin:/etc:/app` | Read-only bind mounts visible inside the jail | diff --git a/evaluation_function/evaluation.py b/evaluation_function/evaluation.py index 7a67f59..60b61e4 100755 --- a/evaluation_function/evaluation.py +++ b/evaluation_function/evaluation.py @@ -3,7 +3,9 @@ import os import shutil import subprocess +import sys import tempfile +import traceback from typing import Any import pycodestyle @@ -11,6 +13,7 @@ from lf_toolkit.evaluation import Result, Params from lf_toolkit.evaluation.image_upload import upload_image, ImageUploadError +from .s3_files import download_files from .security import check_code_safety _TIMEOUT = 25 @@ -32,10 +35,27 @@ def error(self, line_number, offset, text, check): _PREAMBLE_TEMPLATE = """\ import os as _os +import io as _io +import builtins as _builtins _plot_dir = {plot_dir!r} _plot_idx = [0] +_files_dir = _os.path.realpath({files_dir!r}) +_real_open = _builtins.open + +def _safe_open(file, mode="r", *args, **kwargs): + if isinstance(file, (str, _os.PathLike)) and any(m in mode for m in ("w", "a", "x", "+")): + _target = _os.path.realpath(_os.path.join(_files_dir, _os.fspath(file))) + if _os.path.commonpath([_target, _files_dir]) == _files_dir: + raise PermissionError("Provided files are read-only and cannot be modified.") + return _real_open(file, mode, *args, **kwargs) + +# pathlib.Path.open()/read_text()/write_text() call io.open(...) directly, +# not the builtins.open name, so both bindings must be patched. +_builtins.open = _safe_open +_io.open = _safe_open + def _capture_plots(): import sys as _sys if 'matplotlib.pyplot' not in _sys.modules: @@ -109,19 +129,22 @@ def _add_repl_print(code: str) -> str: return code + f"\nprint(repr({ast.unparse(node)}))" -def _run_code(code: str, stdin: str) -> tuple[str, str, bool, list[Image.Image]]: +def _run_code(code: str, stdin: str, files_dir: str | None = None) -> tuple[str, str, bool, list[Image.Image]]: plot_dir = tempfile.mkdtemp() - preamble = _PREAMBLE_TEMPLATE.format(plot_dir=plot_dir) - with tempfile.NamedTemporaryFile(mode="w", suffix=".py", delete=False) as f: + own_run_dir = files_dir is None + run_dir = files_dir if files_dir is not None else tempfile.mkdtemp() + preamble = _PREAMBLE_TEMPLATE.format(plot_dir=plot_dir, files_dir=run_dir) + script_path = os.path.join(run_dir, "_submission.py") + with open(script_path, "w") as f: f.write(preamble + "\n" + code + "\n" + _CAPTURE_CALL) - tmpfile = f.name try: proc = subprocess.run( - ["python", tmpfile], + ["python", "_submission.py"], input=stdin, capture_output=True, text=True, timeout=_TIMEOUT, + cwd=run_dir, env={**os.environ, "MPLBACKEND": "Agg", "MPLCONFIGDIR": "/tmp"}, ) images = [] @@ -135,8 +158,10 @@ def _run_code(code: str, stdin: str) -> tuple[str, str, bool, list[Image.Image]] except subprocess.TimeoutExpired: return "", "", True, [] finally: - os.unlink(tmpfile) + os.unlink(script_path) shutil.rmtree(plot_dir, ignore_errors=True) + if own_run_dir: + shutil.rmtree(run_dir, ignore_errors=True) def _code_block(label: str, content: str) -> str: @@ -169,9 +194,9 @@ def _check_pep8(code: str, select: list[str]) -> list[str]: return [f"Line {ln}: {text}" for ln, text in checker.report.violations] -def _evaluate_demo(response: str, result: Result) -> Result: +def _evaluate_demo(response: str, result: Result, files_dir: str | None = None) -> Result: response = _add_repl_print(response) - stdout, stderr, timed_out, images = _run_code(response, "") + stdout, stderr, timed_out, images = _run_code(response, "", files_dir) if timed_out: result.add_feedback("error", f"Code timed out after {_TIMEOUT}s.") elif stderr and not stdout: @@ -183,7 +208,7 @@ def _evaluate_demo(response: str, result: Result) -> Result: return result -def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") -> Result: +def _evaluate_io(response: str, tests: list, result: Result, answer: str = "", files_dir: str | None = None) -> Result: passed = 0 response = _add_repl_print(response) @@ -206,12 +231,12 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") - if answer: ans_code = _add_repl_print(answer) ans_run_code = (prefix + ans_code) if inject else ans_code - ans_stdout, _, _, _ = _run_code(ans_run_code, run_stdin) + ans_stdout, _, _, _ = _run_code(ans_run_code, run_stdin, files_dir) expected = ans_stdout.rstrip() else: expected = test.get("expected_output", "").rstrip() - stdout, stderr, timed_out, images = _run_code(run_code, run_stdin) + stdout, stderr, timed_out, images = _run_code(run_code, run_stdin, files_dir) actual = stdout.rstrip() label = f"Hidden test {i}" if hidden else f"Test {i}" @@ -248,7 +273,7 @@ def _evaluate_io(response: str, tests: list, result: Result, answer: str = "") - return result -def _evaluate_unit(response: str, test_code: str, result: Result) -> Result: +def _evaluate_unit(response: str, test_code: str, result: Result, files_dir: str | None = None) -> Result: if not test_code.strip(): result.add_feedback("error", "No test code provided for unit_test mode.") return result @@ -256,7 +281,7 @@ def _evaluate_unit(response: str, test_code: str, result: Result) -> Result: results_path = tempfile.mktemp(suffix=".json") runner = _UNIT_RUNNER_TEMPLATE.format(results_path=results_path) combined = _add_repl_print(response) + "\n\n" + test_code + runner - stdout, stderr, timed_out, _ = _run_code(combined, "") + stdout, stderr, timed_out, _ = _run_code(combined, "", files_dir) test_results = None try: @@ -298,6 +323,41 @@ def _evaluate_unit(response: str, test_code: str, result: Result) -> Result: return result +def _coerce_file_specs(raw: Any) -> list: + """Normalise a raw files value into a list of {url, name} dicts. + + Entries may already be dicts, or JSON-encoded strings — the LF web + client may serialise each upload entry to a string. + """ + if not isinstance(raw, (list, tuple)): + return [] + specs = [] + for entry in raw: + if isinstance(entry, str): + try: + entry = json.loads(entry) + except (ValueError, TypeError): + continue + if isinstance(entry, dict): + specs.append(entry) + return specs + + +def _collect_file_specs(params: Params) -> list: + """Gather the files to make available for this request. + + The LF client passes the teacher's files as params["answer_files"] + (saved in the response area's grade params) and the student's uploads + as params["response_files"] (sent with each check). params["files"] is + accepted as a legacy alias for answer_files. All files land in one + working directory; on a name clash the teacher's file wins. + """ + teacher = _coerce_file_specs(params.get("answer_files")) + _coerce_file_specs(params.get("files")) + student = _coerce_file_specs(params.get("response_files")) + teacher_names = {spec.get("name") for spec in teacher} + return [spec for spec in student if spec.get("name") not in teacher_names] + teacher + + def evaluation_function(response: Any, answer: Any, params: Params) -> Result: result = Result() mode = params.get("mode") @@ -305,7 +365,10 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: result.add_feedback("error", f"Unknown or missing mode: {mode!r}. Expected 'demo', 'io_test', or 'unit_test'.") return result - violations = check_code_safety(str(response)) + code = str(response) + file_specs = _collect_file_specs(params) + + violations = check_code_safety(code) if violations: result.add_feedback( "error", @@ -313,23 +376,44 @@ def evaluation_function(response: Any, answer: Any, params: Params) -> Result: ) return result - if mode == "demo": - result = _evaluate_demo(str(response), result) - elif mode == "io_test": - ans = str(answer) if params.get("use_answer_as_expected_output") else "" - result = _evaluate_io(str(response), params.get("tests", []), result, answer=ans) - else: - test_code = str(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "") - result = _evaluate_unit(str(response), test_code, result) - - pep8_param = params.get("pep8_feedback") - if pep8_param: - select = pep8_param if isinstance(pep8_param, list) else _PEP8_SELECT - violations = _check_pep8(str(response), select) - if violations: - body = "Style suggestions (PEP8):\n" + "\n".join(f"- {v}" for v in violations) + files_dir = None + try: + file_warnings: list[str] = [] + if file_specs: + files_dir = tempfile.mkdtemp() + file_warnings = download_files(file_specs, files_dir) + + if mode == "demo": + result = _evaluate_demo(code, result, files_dir) + elif mode == "io_test": + ans = str(answer) if params.get("use_answer_as_expected_output") else "" + result = _evaluate_io(code, params.get("tests", []), result, answer=ans, files_dir=files_dir) else: - body = "No style issues found." - result.add_feedback("style", body) + test_code = str(answer) if params.get("use_answer_as_test_code") else params.get("test_code", "") + result = _evaluate_unit(code, test_code, result, files_dir=files_dir) + + for warning in file_warnings: + result.add_feedback("error", warning) + + pep8_param = params.get("pep8_feedback") + if pep8_param: + select = pep8_param if isinstance(pep8_param, list) else _PEP8_SELECT + violations = _check_pep8(code, select) + if violations: + body = "Style suggestions (PEP8):\n" + "\n".join(f"- {v}" for v in violations) + else: + body = "No style issues found." + result.add_feedback("style", body) + except Exception: + traceback.print_exc(file=sys.stderr) + result = Result() + result.add_feedback( + "error", + "An unexpected internal error occurred while evaluating this submission. " + "Please contact a course organizer.", + ) + finally: + if files_dir is not None: + shutil.rmtree(files_dir, ignore_errors=True) return result \ No newline at end of file diff --git a/evaluation_function/evaluation_test.py b/evaluation_function/evaluation_test.py index 610a6c2..ef82f2c 100755 --- a/evaluation_function/evaluation_test.py +++ b/evaluation_function/evaluation_test.py @@ -1,3 +1,6 @@ +import json +import os +import tempfile import unittest from unittest.mock import patch @@ -302,6 +305,226 @@ def test_hypothesis_fail_shows_minimal_example(self): self.assertIn("square(", result["feedback"]) +def _stub_download(content_by_filename): + def fake_download(files, dest_dir): + for filename, content in content_by_filename.items(): + with open(os.path.join(dest_dir, filename), "w") as f: + f.write(content) + return [] + return fake_download + + +class TestFileDownloads(unittest.TestCase): + + @patch("evaluation_function.evaluation.download_files") + def test_demo_mode_can_read_downloaded_file(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "1,2,3"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + + self.assertIn("1,2,3", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_io_test_downloads_once_for_all_tests(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "42"}) + params = { + "mode": "io_test", + "files": [{"url": "https://example.com/k", "name": "data.csv"}], + "tests": [_test("", "42\n"), _test("", "42\n")], + } + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + + self.assertTrue(result["is_correct"]) + mock_download.assert_called_once() + + @patch("evaluation_function.evaluation.download_files") + def test_answer_code_receives_same_files(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "7"}) + params = { + "mode": "io_test", + "use_answer_as_expected_output": True, + "files": [{"url": "https://example.com/k", "name": "data.csv"}], + "tests": [{"input": ""}], + } + code = "print(open('data.csv').read())" + result = evaluation_function(code, code, params).to_dict() + + self.assertTrue(result["is_correct"]) + + @patch("evaluation_function.evaluation.download_files") + def test_missing_file_reported_as_warning(self, mock_download): + mock_download.return_value = ["File 'data.csv' could not be found."] + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + result = evaluation_function("print('hi')", None, params).to_dict() + + self.assertIn("could not be found", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_import_of_uploaded_module(self, mock_download): + mock_download.side_effect = _stub_download({"helper.py": "def square(n):\n return n * n\n"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "helper.py"}]} + result = evaluation_function("import helper\nprint(helper.square(4))", None, params).to_dict() + + self.assertIn("16", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_file_entries_as_json_strings(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "42"}) + params = {"mode": "demo", "files": [json.dumps({"url": "https://example.com/k", "name": "data.csv"})]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + + self.assertIn("42", result["feedback"]) + passed_specs = mock_download.call_args[0][0] + self.assertEqual(passed_specs, [{"url": "https://example.com/k", "name": "data.csv"}]) + + @patch("evaluation_function.evaluation.download_files") + def test_answer_files_param(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "teacher"}) + params = {"mode": "demo", "answer_files": [{"url": "https://example.com/t", "name": "data.csv"}]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + + self.assertIn("teacher", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_response_files_param(self, mock_download): + mock_download.side_effect = _stub_download({"mine.csv": "student"}) + params = {"mode": "demo", "response_files": [{"url": "https://example.com/s", "name": "mine.csv"}]} + result = evaluation_function("print(open('mine.csv').read())", None, params).to_dict() + + self.assertIn("student", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_answer_and_response_files_combined(self, mock_download): + mock_download.return_value = [] + params = { + "mode": "demo", + "answer_files": [{"url": "https://example.com/t", "name": "data.csv"}], + "response_files": [ + {"url": "https://example.com/s1", "name": "mine.csv"}, + {"url": "https://example.com/s2", "name": "data.csv"}, + ], + } + evaluation_function("print('hi')", None, params) + + mock_download.assert_called_once() + passed_specs = mock_download.call_args[0][0] + self.assertEqual(passed_specs, [ + {"url": "https://example.com/s1", "name": "mine.csv"}, + {"url": "https://example.com/t", "name": "data.csv"}, + ]) + + def test_no_files_param_no_download_call(self): + with patch("evaluation_function.evaluation.download_files") as mock_download: + evaluation_function("print('hi')", None, {"mode": "demo"}) + mock_download.assert_not_called() + + +class TestMalformedFileSpec(unittest.TestCase): + + def test_legacy_filename_key_does_not_crash(self): + # Reproduces the real-world crash report shape: a client sending the + # old/wrong "filename" key instead of "name". Must not crash. + params = { + "mode": "demo", + "files": [{ + "url": "https://example.com/k", + "filename": "score_utils.py", + "type": "text/x-python-script", + "size": 237, + }], + } + result = evaluation_function("print('hi')", None, params).to_dict() + + self.assertIn("hi", result["feedback"]) + self.assertIn("missing", result["feedback"].lower()) + + +class TestUnexpectedExceptionHandling(unittest.TestCase): + + @patch("evaluation_function.evaluation._run_code") + def test_unexpected_exception_during_evaluation_is_caught(self, mock_run): + mock_run.side_effect = RuntimeError("boom") + + result = evaluation_function("print('hi')", None, {"mode": "demo"}).to_dict(include_test_data=True) + + self.assertFalse(result["is_correct"]) + self.assertIn("error", result["tags"]) + + @patch("evaluation_function.evaluation.download_files") + def test_exception_in_download_files_becomes_error_result(self, mock_download): + # Simulates a bug in download_files() itself (defense-in-depth, + # independent of the s3_files.py validation fix). + mock_download.side_effect = KeyError("name") + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + + result = evaluation_function("print('hi')", None, params).to_dict(include_test_data=True) + + self.assertFalse(result["is_correct"]) + self.assertIn("error", result["tags"]) + + def test_files_dir_cleaned_up_even_on_exception(self): + created_dirs = [] + real_mkdtemp = tempfile.mkdtemp + + def tracking_mkdtemp(*args, **kwargs): + d = real_mkdtemp(*args, **kwargs) + created_dirs.append(d) + return d + + with patch("evaluation_function.evaluation.download_files", side_effect=RuntimeError("boom")), \ + patch("evaluation_function.evaluation.tempfile.mkdtemp", side_effect=tracking_mkdtemp): + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + evaluation_function("print('hi')", None, params) + + self.assertTrue(created_dirs) + self.assertFalse(os.path.exists(created_dirs[0])) + + +class TestFileAccessSandbox(unittest.TestCase): + + @patch("evaluation_function.evaluation.download_files") + def test_read_downloaded_file_succeeds(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "hello"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + result = evaluation_function("print(open('data.csv').read())", None, params).to_dict() + + self.assertIn("hello", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_write_mode_to_provided_file_blocked(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "hello"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + result = evaluation_function("open('data.csv', 'w')", None, params).to_dict() + + self.assertIn("read-only", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_write_new_file_in_run_dir_blocked(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "hello"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + result = evaluation_function("open('output.txt', 'w')", None, params).to_dict() + + self.assertIn("read-only", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_pathlib_read_respects_sandbox(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "world"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + code = "from pathlib import Path\nprint(Path('data.csv').read_text())" + result = evaluation_function(code, None, params).to_dict() + + self.assertIn("world", result["feedback"]) + + @patch("evaluation_function.evaluation.download_files") + def test_pathlib_write_respects_sandbox(self, mock_download): + mock_download.side_effect = _stub_download({"data.csv": "world"}) + params = {"mode": "demo", "files": [{"url": "https://example.com/k", "name": "data.csv"}]} + code = "from pathlib import Path\nPath('data.csv').write_text('nope')" + result = evaluation_function(code, None, params).to_dict() + + self.assertIn("read-only", result["feedback"]) + + class TestPep8Feedback(unittest.TestCase): def test_violations_reported(self): diff --git a/evaluation_function/preview_test.py b/evaluation_function/preview_test.py index a8d7509..94bb20e 100755 --- a/evaluation_function/preview_test.py +++ b/evaluation_function/preview_test.py @@ -53,4 +53,25 @@ def test_input_is_allowed(self): result = preview_function(response, params) self.assertIn("preview", result) - self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) \ No newline at end of file + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_open_is_allowed(self): + response, params = "f = open('data.csv')\nf.read()", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_pathlib_import_allowed(self): + response, params = "from pathlib import Path\nPath('data.csv').read_text()", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertNotIn("Unsafe", result["preview"].get("feedback", "")) + + def test_os_still_blocked(self): + response, params = "import os\nos.system('ls')", Params() + result = preview_function(response, params) + + self.assertIn("preview", result) + self.assertIn("Unsafe", result["preview"].get("feedback", "")) \ No newline at end of file diff --git a/evaluation_function/s3_files.py b/evaluation_function/s3_files.py new file mode 100644 index 0000000..169c19e --- /dev/null +++ b/evaluation_function/s3_files.py @@ -0,0 +1,127 @@ +import os +from typing import TypedDict +from urllib.parse import urlparse + +import requests + +_MAX_FILE_BYTES = 5 * 1024 * 1024 +_MAX_TOTAL_BYTES = 20 * 1024 * 1024 +_DOWNLOAD_TIMEOUT = 10 +_CHUNK_SIZE = 65536 + + +class FileSpec(TypedDict): + url: str + name: str + + +def _valid_filename(filename: str) -> bool: + if not filename or filename in (".", ".."): + return False + return os.path.basename(filename) == filename + + +class _FileTooLarge(Exception): + pass + + +def _download_one(url: str, target: str, remaining_budget: int) -> int: + """Stream url into target. Returns bytes written. + + Raises _FileTooLarge (and removes any partial file) if the download + exceeds _MAX_FILE_BYTES or remaining_budget, or requests.RequestException + for network/HTTP errors — both handled by the caller. + """ + resp = requests.get(url, stream=True, timeout=_DOWNLOAD_TIMEOUT) + resp.raise_for_status() + + content_length = resp.headers.get("Content-Length") + cap = min(_MAX_FILE_BYTES, remaining_budget) + if content_length is not None and int(content_length) > cap: + raise _FileTooLarge() + + written = 0 + try: + with open(target, "wb") as f: + for chunk in resp.iter_content(chunk_size=_CHUNK_SIZE): + written += len(chunk) + if written > cap: + raise _FileTooLarge() + f.write(chunk) + except _FileTooLarge: + if os.path.exists(target): + os.unlink(target) + raise + return written + + +def download_files(files: list[FileSpec], dest_dir: str) -> list[str]: + """Download each file into dest_dir. + + Returns a list of warning strings for files that were skipped (invalid + filename/URL, too large, or errored) — never raises. + """ + if not files: + return [] + + real_dest_dir = os.path.realpath(dest_dir) + warnings: list[str] = [] + total_bytes = 0 + + for spec in files: + if not isinstance(spec, dict): + warnings.append( + "One of the provided files is missing required information (url/name) and was skipped." + ) + continue + + url = spec.get("url") + filename = spec.get("name") + has_url = isinstance(url, str) and bool(url) + has_filename = isinstance(filename, str) and bool(filename) + + if not has_url or not has_filename: + if has_filename: + warnings.append(f"File '{filename}' is missing a valid URL and was not made available.") + else: + warnings.append( + "One of the provided files is missing required information (url/name) and was skipped." + ) + continue + + if not _valid_filename(filename): + warnings.append(f"File '{filename}' has an invalid filename and was not made available.") + continue + + target = os.path.realpath(os.path.join(real_dest_dir, filename)) + if os.path.commonpath([target, real_dest_dir]) != real_dest_dir: + warnings.append(f"File '{filename}' has an invalid filename and was not made available.") + continue + + if urlparse(url).scheme != "https": + warnings.append(f"File '{filename}' has an invalid URL and was not made available.") + continue + + remaining_budget = _MAX_TOTAL_BYTES - total_bytes + if remaining_budget <= 0: + warnings.append( + f"File '{filename}' was skipped because it would exceed the total " + f"{_MAX_TOTAL_BYTES // (1024 * 1024)}MB size limit for this run." + ) + continue + + try: + written = _download_one(url, target, remaining_budget) + except _FileTooLarge: + warnings.append( + f"File '{filename}' exceeds the {_MAX_FILE_BYTES // (1024 * 1024)}MB size limit " + "and was not made available." + ) + continue + except requests.exceptions.RequestException as e: + warnings.append(f"File '{filename}' could not be downloaded ({e}).") + continue + + total_bytes += written + + return warnings diff --git a/evaluation_function/s3_files_test.py b/evaluation_function/s3_files_test.py new file mode 100644 index 0000000..1ec27d4 --- /dev/null +++ b/evaluation_function/s3_files_test.py @@ -0,0 +1,166 @@ +import os +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +import requests + +from .s3_files import download_files, _MAX_FILE_BYTES + +_URL = "https://example-bucket.s3.amazonaws.com/data.csv?X-Amz-Signature=abc" + + +def _fake_response(content: bytes, content_length: int | None = None, status_code: int = 200): + resp = MagicMock() + resp.status_code = status_code + resp.headers = {} + if content_length is not None: + resp.headers["Content-Length"] = str(content_length) + + def raise_for_status(): + if status_code >= 400: + raise requests.exceptions.HTTPError(f"{status_code} error") + + resp.raise_for_status.side_effect = raise_for_status + + chunk_size = 65536 + + def iter_content(chunk_size=chunk_size): + for i in range(0, len(content), chunk_size): + yield content[i:i + chunk_size] + + resp.iter_content.side_effect = iter_content + return resp + + +class TestDownloadFiles(unittest.TestCase): + + def setUp(self): + self.dest_dir = tempfile.mkdtemp() + + def test_no_files_returns_empty(self): + self.assertEqual(download_files([], self.dest_dir), []) + + @patch("evaluation_function.s3_files.requests.get") + def test_successful_download_writes_file(self, mock_get): + mock_get.return_value = _fake_response(b"hello", content_length=5) + + warnings = download_files([{"url": _URL, "name": "data.csv"}], self.dest_dir) + + self.assertEqual(warnings, []) + with open(os.path.join(self.dest_dir, "data.csv"), "rb") as f: + self.assertEqual(f.read(), b"hello") + + @patch("evaluation_function.s3_files.requests.get") + def test_oversized_via_header_skipped(self, mock_get): + mock_get.return_value = _fake_response(b"x" * 10, content_length=_MAX_FILE_BYTES + 1) + + warnings = download_files([{"url": _URL, "name": "big.csv"}], self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("big.csv", warnings[0]) + self.assertFalse(os.path.exists(os.path.join(self.dest_dir, "big.csv"))) + + @patch("evaluation_function.s3_files.requests.get") + def test_oversized_via_streaming_skipped(self, mock_get): + # Content-Length lies (claims small), actual streamed bytes exceed the cap. + big_content = b"x" * (_MAX_FILE_BYTES + 1) + mock_get.return_value = _fake_response(big_content, content_length=10) + + warnings = download_files([{"url": _URL, "name": "big.csv"}], self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("big.csv", warnings[0]) + self.assertFalse(os.path.exists(os.path.join(self.dest_dir, "big.csv"))) + + @patch("evaluation_function.s3_files.requests.get") + def test_total_size_cap_skips_later_files(self, mock_get): + # 5 files at exactly the per-file cap: the first 4 sum to exactly + # _MAX_TOTAL_BYTES (allowed), the 5th is skipped without a request. + mock_get.return_value = _fake_response(b"x" * _MAX_FILE_BYTES, content_length=_MAX_FILE_BYTES) + + files = [{"url": _URL, "name": f"{i}.csv"} for i in range(5)] + warnings = download_files(files, self.dest_dir) + + self.assertEqual(mock_get.call_count, 4) + self.assertEqual(len(warnings), 1) + self.assertIn("4.csv", warnings[0]) + + @patch("evaluation_function.s3_files.requests.get") + def test_http_error_skipped_others_continue(self, mock_get): + def side_effect(url, stream, timeout): + if url == "https://example.com/missing": + return _fake_response(b"", status_code=404) + return _fake_response(b"ok", content_length=2) + + mock_get.side_effect = side_effect + + files = [ + {"url": "https://example.com/missing", "name": "missing.csv"}, + {"url": "https://example.com/ok", "name": "ok.csv"}, + ] + warnings = download_files(files, self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("missing.csv", warnings[0]) + self.assertTrue(os.path.exists(os.path.join(self.dest_dir, "ok.csv"))) + + @patch("evaluation_function.s3_files.requests.get") + def test_network_error_skipped(self, mock_get): + mock_get.side_effect = requests.exceptions.ConnectionError("boom") + + warnings = download_files([{"url": _URL, "name": "data.csv"}], self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertIn("data.csv", warnings[0]) + + def test_rejects_non_https_url(self): + for bad_url in ("http://example.com/data.csv", "file:///etc/passwd", "ftp://example.com/data.csv"): + warnings = download_files([{"url": bad_url, "name": "data.csv"}], self.dest_dir) + self.assertEqual(len(warnings), 1, f"expected a warning for url={bad_url!r}") + + def test_filename_validation_rejects_traversal(self): + for bad_name in ("../evil.py", "/etc/passwd", "", ".", ".."): + warnings = download_files([{"url": _URL, "name": bad_name}], self.dest_dir) + self.assertEqual(len(warnings), 1, f"expected a warning for name={bad_name!r}") + + def test_legacy_filename_key_skipped_not_raised(self): + # Reproduces the real-world crash report: client actually sends "name", + # not the old "filename" key. A spec using the wrong/legacy key must + # not raise KeyError — it should be skipped with a warning. + warnings = download_files( + [{"url": _URL, "filename": "score_utils.py", "type": "text/x-python-script", "size": 237}], + self.dest_dir, + ) + self.assertEqual(len(warnings), 1) + self.assertIn("missing", warnings[0].lower()) + + def test_missing_url_key_skipped_not_raised(self): + warnings = download_files([{"name": "data.csv"}], self.dest_dir) + self.assertEqual(len(warnings), 1) + self.assertIn("data.csv", warnings[0]) + + def test_non_dict_spec_skipped_not_raised(self): + warnings = download_files(["not-a-dict", 42, None], self.dest_dir) + self.assertEqual(len(warnings), 3) + + def test_empty_spec_generic_message(self): + warnings = download_files([{}], self.dest_dir) + self.assertEqual(len(warnings), 1) + self.assertIn("missing", warnings[0].lower()) + + @patch("evaluation_function.s3_files.requests.get") + def test_malformed_spec_skipped_others_continue(self, mock_get): + mock_get.return_value = _fake_response(b"ok", content_length=2) + files = [ + {"url": _URL, "filename": "score_utils.py"}, # wrong/legacy key, missing "name" + {"url": _URL, "name": "ok.csv"}, + ] + warnings = download_files(files, self.dest_dir) + + self.assertEqual(len(warnings), 1) + self.assertTrue(os.path.exists(os.path.join(self.dest_dir, "ok.csv"))) + + +if __name__ == "__main__": + unittest.main() diff --git a/evaluation_function/security.py b/evaluation_function/security.py index aaaac34..df3f485 100644 --- a/evaluation_function/security.py +++ b/evaluation_function/security.py @@ -1,13 +1,17 @@ import ast +# NOTE: `open`/`pathlib` are intentionally NOT blocked -- student code needs +# them to read files supplied via params["files"] / the response payload. +# Write access into the per-run files dir is still blocked at runtime by +# `_safe_open` in evaluation.py's subprocess preamble. _BLOCKED_MODULES = { "os", "sys", "subprocess", "socket", "urllib", "http", - "requests", "shutil", "pathlib", "ftplib", "smtplib", + "requests", "shutil", "ftplib", "smtplib", "ctypes", "multiprocessing", "threading", "importlib", "pickle", "builtins", } -_BLOCKED_BUILTINS = {"exec", "eval", "compile", "open", "__import__"} +_BLOCKED_BUILTINS = {"exec", "eval", "compile", "__import__"} class _SecurityVisitor(ast.NodeVisitor): diff --git a/evaluation_function/security_test.py b/evaluation_function/security_test.py index 9e6adcf..374389f 100644 --- a/evaluation_function/security_test.py +++ b/evaluation_function/security_test.py @@ -35,8 +35,14 @@ def test_dunder_attribute_access(self): def test_syntax_error_is_not_a_violation(self): self.assertEqual(check_code_safety("def f(:\n"), []) + def test_open_and_pathlib_are_allowed(self): + # Student code needs these to read files supplied via params["files"] + # / the response payload; write access is blocked at runtime instead. + self.assertEqual(check_code_safety("open('data.csv').read()"), []) + self.assertEqual(check_code_safety("import pathlib\npathlib.Path('data.csv')"), []) + def test_multiple_violations_collected(self): - violations = check_code_safety("import os\nimport socket\nopen('/etc/passwd')") + violations = check_code_safety("import os\nimport socket\nexec('x = 1')") self.assertIn("import of 'os' is not allowed", violations) self.assertIn("import of 'socket' is not allowed", violations) - self.assertIn("use of 'open()' is not allowed", violations) + self.assertIn("use of 'exec()' is not allowed", violations)