diff --git a/CHANGELOG.md b/CHANGELOG.md index 4feb10ed..e3c9cb75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +**v5.4.0 (unreleased):** + +* Updated submission runners to export and submit the PDF and then remove all copies of the API token before executing the submission to ensure that student code can't gain access to the token +* Updated Otter Run to skip PDF export and upload in debug mode + **v5.3.0:** * Updated Otter Assign to throw a `ValueError` when invalid Python code is encountered in test cells per [#756](https://github.com/ucbds-infra/otter-grader/issues/756) diff --git a/otter/run/run_autograder/runners/abstract_runner.py b/otter/run/run_autograder/runners/abstract_runner.py index 062b386f..5598ea4e 100644 --- a/otter/run/run_autograder/runners/abstract_runner.py +++ b/otter/run/run_autograder/runners/abstract_runner.py @@ -1,5 +1,6 @@ """Abstract runner class for the autograder""" +import gc import json import os import shutil @@ -11,6 +12,7 @@ from ..autograder_config import AutograderConfig from ..utils import OtterRuntimeError, print_output, write_blank_page_to_stare_at_before_you +from ....generate.token import APIClient from ....nbmeta_config import NBMetadataConfig @@ -84,7 +86,7 @@ def get_notebook_assignment_name(self, nb): nbmc = NBMetadataConfig.from_notebook(nb) return nbmc.assignment_name - def write_and_maybe_submit_pdf(self, client, submission_path, submit, scores): + def write_and_maybe_submit_pdf(self, submission_path): """ Upload a PDF to a Gradescope assignment for manual grading. @@ -94,12 +96,16 @@ def write_and_maybe_submit_pdf(self, client, submission_path, submit, scores): This method should be called, if appropriate, by ``self.run``. Args: - client (``otter.generate.token.APIClient``): the Gradescope client submission_path (``str``): path to the submission - submit (``bool``): whether to submit the PDF with ``client`` - scores (``otter.test_files.GradingResults``): the grading results (so an error can be - appended if needed) + + Returns: + ``Exception | None``: an error thrown while trying to write or submit the PDF, if any; + a return value of ``None`` indicates a successful export and upload """ + # Don't export or submit a PDF in debug mode. + if self.ag_config.debug: + return None + try: subm_pdfs = glob("*.pdf") if self.ag_config.use_submission_pdf and subm_pdfs: @@ -107,19 +113,26 @@ def write_and_maybe_submit_pdf(self, client, submission_path, submit, scores): else: pdf_path = self.write_pdf(submission_path) - if submit: + if self.ag_config.token: + client = APIClient(token=self.ag_config.token) self.submit_pdf(client, pdf_path) + # ensure the client gets garbage collected so that the token can't be accessed + # by student code + del client + gc.collect() + except Exception as e: print_output(f"\n\nError encountered while generating and submitting PDF:\n{e}") - scores.set_pdf_error(e) - if self.ag_config.submit_blank_pdf_on_export_failure and submit: + if self.ag_config.submit_blank_pdf_on_export_failure and self.ag_config.token: print_output("\nUploading a blank PDF due to export failure") with tempfile.NamedTemporaryFile(suffix=".pdf") as ntf: write_blank_page_to_stare_at_before_you(ntf.name) self.submit_pdf(client, ntf.name) + return e + def submit_pdf(self, client, pdf_path): """ Upload the PDF at ``pdf_path`` to the Gradescope assignment specified in the config. Does @@ -157,6 +170,22 @@ def submit_pdf(self, client, pdf_path): print_output("\n\nSuccessfully uploaded submissions for: {}".format( ", ".join(student_emails))) + def sanitize_tokens(self): + """ + Sanitize any references to the PDF submission upload token to prevent unauthorized access + when executing student code. + + This method should be invokved from inside ``{self.ag_config.autograder_dir}/submission`` + as part of ``run``. + """ + self.ag_config.token = None + if not os.path.exists("../source/otter_config.json"): return + with open("../source/otter_config.json") as f: + c = json.load(f) + if "token" in c: del c["token"] + with open("../source/otter_config.json", "w") as f: + json.dump(c, f, indent=2) + @abstractmethod def validate_submission(self, submission_path): """ diff --git a/otter/run/run_autograder/runners/python_runner.py b/otter/run/run_autograder/runners/python_runner.py index 9f4ebba2..b2087bf6 100644 --- a/otter/run/run_autograder/runners/python_runner.py +++ b/otter/run/run_autograder/runners/python_runner.py @@ -14,7 +14,6 @@ from ....check.notebook import _OTTER_LOG_FILENAME from ....execute import grade_notebook from ....export import export_notebook -from ....generate.token import APIClient from ....plugins import PluginCollection from ....utils import chdir, print_full_width @@ -94,15 +93,11 @@ def run(self): if plugin_collection: plugin_collection.run("before_grading", self.ag_config) - if self.ag_config.token is not None: - client = APIClient(token=self.ag_config.token) - generate_pdf = True - has_token = True + pdf_error = None + if self.ag_config.token is not None or self.ag_config.pdf: + pdf_error = self.write_and_maybe_submit_pdf(subm_path) - else: - generate_pdf = self.ag_config.pdf - has_token = False - client = None + self.sanitize_tokens() if os.path.isfile(_OTTER_LOG_FILENAME): try: @@ -137,6 +132,8 @@ def run(self): force_python3_kernel = not self.ag_config._otter_run, ) + if pdf_error: scores.set_pdf_error(pdf_error) + # verify the scores against the log if self.ag_config.print_summary: print_output("\n\n\n\n", end="") @@ -158,9 +155,6 @@ def run(self): else: print_output("No log found with which to verify student scores.") - if generate_pdf: - self.write_and_maybe_submit_pdf(client, subm_path, has_token, scores) - if plugin_collection: report = plugin_collection.generate_report() if report.strip(): diff --git a/otter/run/run_autograder/runners/r_runner.py b/otter/run/run_autograder/runners/r_runner.py index 395276bb..0e3cc3dd 100644 --- a/otter/run/run_autograder/runners/r_runner.py +++ b/otter/run/run_autograder/runners/r_runner.py @@ -16,7 +16,6 @@ from ..utils import OtterRuntimeError from ....export import export_notebook -from ....generate.token import APIClient from ....test_files import GradingResults from ....utils import chdir, get_source, knit_rmd_file, NBFORMAT_VERSION @@ -198,23 +197,18 @@ def run(self): os.environ["PATH"] = f"{self.ag_config.miniconda_path}/bin:" + os.environ.get("PATH") with chdir("./submission"): - if self.ag_config.token is not None: - client = APIClient(token=self.ag_config.token) - generate_pdf = True - has_token = True + pdf_error = None + if self.ag_config.token is not None or self.ag_config.pdf: + pdf_error = self.write_and_maybe_submit_pdf(None) - else: - generate_pdf = self.ag_config.pdf - has_token = False - client = None + self.sanitize_tokens() subm_path = self.resolve_submission_path() output = R_PACKAGES["ottr"].run_autograder( subm_path, ignore_errors = not self.ag_config.debug, test_dir = "./tests")[0] scores = GradingResults.from_ottr_json(output) - if generate_pdf: - self.write_and_maybe_submit_pdf(client, None, has_token, scores) + if pdf_error: scores.set_pdf_error(pdf_error) # delete the script if necessary if self.subm_path_deletion_required: diff --git a/test/test_run/test_integration.py b/test/test_run/test_integration.py index 381a4d44..54dd5c30 100644 --- a/test/test_run/test_integration.py +++ b/test/test_run/test_integration.py @@ -7,8 +7,10 @@ import os import pytest import re +import shutil from contextlib import contextmanager, nullcontext +from textwrap import dedent from unittest import mock from otter.generate.token import APIClient @@ -24,6 +26,10 @@ @pytest.fixture(autouse=True) def cleanup_output(cleanup_enabled): + with FILE_MANAGER.open("autograder/source/otter_config.json") as f: + cpy = f.read() + with FILE_MANAGER.open("rmd-autograder/source/otter_config.json") as f: + crmd = f.read() yield if cleanup_enabled: delete_paths([ @@ -43,6 +49,10 @@ def cleanup_output(cleanup_enabled): FILE_MANAGER.get_path("rmd-autograder/submission/__init__.py"), FILE_MANAGER.get_path("rmd-autograder/submission/.OTTER_LOG"), ]) + with FILE_MANAGER.open("autograder/source/otter_config.json", "w") as f: + f.write(cpy) + with FILE_MANAGER.open("rmd-autograder/source/otter_config.json", "w") as f: + f.write(crmd) @pytest.fixture(autouse=True) @@ -136,9 +146,11 @@ def alternate_config(config_path, new_config): contents = f.read() with open(config_path, "w") as f: json.dump(new_config, f) - yield - with open(config_path, "w") as f: - f.write(contents) + try: + yield + finally: + with open(config_path, "w") as f: + f.write(contents) def get_expected_error_results(error): @@ -154,6 +166,33 @@ def get_expected_error_results(error): } +@contextmanager +def alternate_submission(subm_path, new_nb): + with open(subm_path) as f: + contents = f.read() + nbformat.write(new_nb, subm_path) + try: + yield + finally: + with open(subm_path, "w") as f: + f.write(contents) + + +@contextmanager +def alternate_tests(test_files): + src, dst = FILE_MANAGER.get_path("autograder/source/tests"), FILE_MANAGER.get_path("autograder/source/tests_orig") + os.rename(src, dst) + os.makedirs(src, exist_ok=True) + for fn, contents in test_files.items(): + with open(os.path.join(src, fn), "w+") as f: + f.write(contents) + try: + yield + finally: + shutil.rmtree(src) + os.rename(dst, src) + + @pytest.fixture(autouse=True) def mock_export_notebook(cleanup_enabled): empty_pdfs = [] @@ -257,9 +296,8 @@ def test_use_submission_pdf( def test_force_public_test_summary(get_config_path, load_config): - config = load_config() - def perform_test(show_hidden, force_public_test_summary, expect_summary): + config = load_config() config["show_hidden"] = show_hidden config["force_public_test_summary"] = force_public_test_summary with alternate_config(get_config_path(), config): @@ -284,7 +322,6 @@ def test_script(load_config, expected_results, get_config_path): config = load_config() nb_path = FILE_MANAGER.get_path("autograder/submission/fails2and6H.ipynb") nb = nbformat.read(nb_path, as_version=NBFORMAT_VERSION) - os.remove(nb_path) # Remove the token so that we don't try to generate a PDF of a script. config.pop("token") @@ -295,6 +332,7 @@ def test_script(load_config, expected_results, get_config_path): # remove magic commands py = "\n".join(l for l in py.split("\n") if not l.startswith("get_ipython")) + os.remove(nb_path) with FILE_MANAGER.open("autograder/submission/fails2and6H.py", "w+") as f: f.write(py) @@ -408,3 +446,67 @@ def perform_test(rmd, expected_results, error=None, **kwargs): delete_paths([rmd_path]) with open(rmd_path, "w+") as f: f.write(orig_rmd) + + +@mock.patch.object(APIClient, "upload_pdf_submission") +def test_token_sanitization(mocked_upload, get_config_path, load_config, expected_results): + """ + Tests that the PDF upload token can't be accessed by the submission. + """ + config = load_config() + config["token"] = "abc123" + config.pop("plugins") + + nb = nbformat.v4.new_notebook(cells=[nbformat.v4.new_code_cell(dedent("""\ + import json + with open("../source/otter_config.json") as f: + config = json.load(f) + token = config.get("token") + """))]) + + tests = {"q1.py": dedent("""\ + OK_FORMAT = False + + from otter.test_files import test_case + + name = "q1" + + @test_case() + def q1_1(env): + assert env["token"] is None + """)} + + expected_results = { + "tests": [ + { + "name": "Public Tests", + "visibility": "visible", + "output": "q1 results: All test cases passed!", + "status": "passed", + }, + { + "name": "q1", + "score": 1.0, + "max_score": 1.0, + "visibility": "hidden", + "output": "q1 results: All test cases passed!" + }, + ], + } + + FILE_MANAGER.open("autograder/submission/fails2and6H.pdf", "wb+").close() + + mocked_upload.return_value.status_code = 200 + + with alternate_config(get_config_path(), config), \ + alternate_submission(FILE_MANAGER.get_path("autograder/submission/fails2and6H.ipynb"), nb), \ + alternate_tests(tests): + run_autograder(config['autograder_dir']) + + with FILE_MANAGER.open("autograder/results/results.json") as f: + actual_results = json.load(f) + + assert actual_results == expected_results, \ + f"Actual results did not matched expected:\n{actual_results}" + + mocked_upload.assert_called()