Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check marking schemes for attachments to prevent leakage #1783

Merged
merged 1 commit into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions nbgrader/preprocessors/clearmarkingscheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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. ###
Expand All @@ -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
Expand All @@ -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
tuncbkose marked this conversation as resolved.
Show resolved Hide resolved
"""
)

# add lines as long as it's not in the hidden tests region
elif not in_ms:
new_lines.append(line)
Expand Down
74 changes: 74 additions & 0 deletions nbgrader/tests/preprocessors/test_clearmarkscheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
tuncbkose marked this conversation as resolved.
Show resolved Hide resolved
### 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
Comment on lines +142 to +148
Copy link
Contributor

@brichet brichet Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### BEGIN MARK SCHEME
Text text text text text.
![](attachment:image1.jpg)
Grade grade grade.
![](attachment:image2.png)
Mark mark mark mark.
### END MARK SCHEME
### BEGIN MARK SCHEME
Text text text text text.
![alternative text](attachment:image1.jpg)
Grade grade grade.
### END MARK SCHEME

Thanks @tuncbkose, I was more thinking about alternative text to the image, to test the regular expression.
But if you think it is relevant to keep also a test with 2 images inserted we can also keep it (maybe keeping text around is also a good thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I'll add one with an alternative text. As for this example with two attachments and text around them, I don't think it matters too much but it doesn't hurt to keep it.

"""
).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)"