Merge pull request #771 from chrispyles/submit-pdf-first

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)

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,32 +96,43 @@ 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:
                 pdf_path = subm_pdfs[0]
             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):
         """

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():

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:

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()