From 3d9891857097d295c823dba751e83a3a78255ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tun=C3=A7=20Ba=C5=9Far=20K=C3=B6se?= Date: Mon, 8 May 2023 12:45:19 +0300 Subject: [PATCH] Check marking schemes for attachments to prevent leakage --- nbgrader/preprocessors/clearmarkingscheme.py | 27 ++++++- .../preprocessors/test_clearmarkscheme.py | 74 +++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/nbgrader/preprocessors/clearmarkingscheme.py b/nbgrader/preprocessors/clearmarkingscheme.py index 1ff1a5fa7..e3d71da60 100644 --- a/nbgrader/preprocessors/clearmarkingscheme.py +++ b/nbgrader/preprocessors/clearmarkingscheme.py @@ -14,12 +14,12 @@ class ClearMarkScheme(NbGraderPreprocessor): begin_mark_scheme_delimeter = Unicode( "BEGIN MARK SCHEME", - help="The delimiter marking the beginning of hidden tests cases" + help="The delimiter marking the beginning of a marking scheme region" ).tag(config=True) end_mark_scheme_delimeter = Unicode( "END MARK SCHEME", - help="The delimiter marking the end of hidden tests cases" + help="The delimiter marking the end of a marking scheme region" ).tag(config=True) enforce_metadata = Bool( @@ -35,6 +35,16 @@ class ClearMarkScheme(NbGraderPreprocessor): ) ).tag(config=True) + check_attachment_leakage = Bool( + True, + help=dedent( + """ + Whether or not to check if a marking scheme region contains an attachment, + in order to prevent leakage to student version of notebooks. + """ + ) + ).tag(config=True) + def _remove_mark_scheme_region(self, cell: NotebookNode) -> bool: """Find a region in the cell that is delimeted by `self.begin_mark_scheme_delimeter` and `self.end_mark_scheme_delimeter` (e.g. ### @@ -50,6 +60,7 @@ def _remove_mark_scheme_region(self, cell: NotebookNode) -> bool: new_lines = [] in_ms = False removed_ms = False + attachment_regex = r"!\[.*\]\(attachment:.+?\)" for line in lines: # begin the test area @@ -67,6 +78,18 @@ def _remove_mark_scheme_region(self, cell: NotebookNode) -> bool: elif self.end_mark_scheme_delimeter in line: in_ms = False + elif self.check_attachment_leakage and in_ms and re.search(attachment_regex, line): + raise RuntimeError( + """ + Encountered an attachment in a marking scheme. + This can leak to student notebooks. + You might want to embed your image instead, like here: + https://github.com/jupyter/nbgrader/issues/1782#issuecomment-1541493629. + You can suppress this check with ClearMarkScheme.check_attachment_leakage=False. + For more info: https://github.com/jupyter/nbgrader/issues/1782 + """ + ) + # add lines as long as it's not in the hidden tests region elif not in_ms: new_lines.append(line) diff --git a/nbgrader/tests/preprocessors/test_clearmarkscheme.py b/nbgrader/tests/preprocessors/test_clearmarkscheme.py index 6a8b754af..20c32042c 100644 --- a/nbgrader/tests/preprocessors/test_clearmarkscheme.py +++ b/nbgrader/tests/preprocessors/test_clearmarkscheme.py @@ -108,3 +108,77 @@ def test_preprocess_notebook(self, preprocessor): """Is the test notebook processed without error?""" nb = self._read_nb(os.path.join("files", "test_taskcell.ipynb")) preprocessor.preprocess(nb, {}) + + # attachment detection tests + def test_attachment_in_mark_scheme(self, preprocessor): + """Is an error raised when there is an attachment in the marking scheme?""" + source = dedent( + """ + assert True + ### BEGIN MARK SCHEME + ![](attachment:image.png) + ### END MARK SCHEME + """ + ).strip() + cell = create_task_cell(source, 'markdown', 'some-task-id', 2) + with pytest.raises(RuntimeError): + preprocessor._remove_mark_scheme_region(cell) + + source = dedent( + """ + assert True + ### BEGIN MARK SCHEME + ![alt text](attachment:answers.png) + ### END MARK SCHEME + """ + ).strip() + cell = create_task_cell(source, 'markdown', 'some-task-id', 2) + with pytest.raises(RuntimeError): + preprocessor._remove_mark_scheme_region(cell) + + source = dedent( + """ + assert True + ### BEGIN MARK SCHEME + Text text text text text. + ![](attachment:image1.jpg) + Grade grade grade. + ![](attachment:image2.png) + Mark mark mark mark. + ### END MARK SCHEME + """ + ).strip() + cell = create_task_cell(source, 'markdown', 'some-task-id', 2) + with pytest.raises(RuntimeError): + preprocessor._remove_mark_scheme_region(cell) + + def test_attachment_suppress_error(self, preprocessor): + """Can the error be suppressed?""" + source = dedent( + """ + assert True + ### BEGIN MARK SCHEME + ![](attachment:image.png) + ### END MARK SCHEME + """ + ).strip() + cell = create_task_cell(source, 'markdown', 'some-task-id', 2) + preprocessor.check_attachment_leakage = False + removed_test = preprocessor._remove_mark_scheme_region(cell) + assert removed_test + assert cell.source == "assert True" + + def test_attachment_not_in_mark_scheme(self, preprocessor): + """Attachments outside of marking schemes shouldn't be touched""" + source = dedent( + """ + ![](attachment:image.png) + ### BEGIN MARK SCHEME + assert True + ### END MARK SCHEME + """ + ).strip() + cell = create_task_cell(source, 'markdown', 'some-task-id', 2) + removed_test = preprocessor._remove_mark_scheme_region(cell) + assert removed_test + assert cell.source == "![](attachment:image.png)"