From fd34cf5f1f6e243243c738c6e0cf62eb682c4d68 Mon Sep 17 00:00:00 2001 From: Jacob Evan Shreve Date: Tue, 7 Feb 2023 23:36:52 -0500 Subject: [PATCH 1/3] Swallow potential exceptions from showtraceback() The nbgrader project is aware of a form of cheating where students disrupt `InteractiveShell.showtraceback` in hopes of hiding exceptions to avoid losing points. They have implemented a solution to prevent this cheating from working on the client side, and have some tests to demonstrate this technique: https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb In essence, these attacks import the interactive shell and erase the traceback handler so that their failing tests won't report failures. import IPython.core.interactiveshell IPython.core.interactiveshell.InteractiveShell.showtraceback = None The problem is that this causes an exception inside the kernel, leading to a stalled execution. The kernel has stopped working, but the client continues to wait for messages. So far, nbgrader's solution to this is to require a timeout value so the client can eventually decide it is done. This prevents allowing a value of `None` for `Execute.timeout` because this would cause a test case to infinitely hang. This commit addresses the problem by making `InteractiveShell._run_cell` a little more protective around it's call to `showtraceback()`. There is already a try/except block around running the cell. This commit adds a finally clause so that the method will _always_ return an `ExecutionResult`, even if a new exception is thrown within the except clause. For the record, the exception thrown is: TypeError: 'NoneType' object is not callable Accepting this change will allow nbgrader to update `nbgrader.preprocessors.Execute` to support a type of `Integer(allow_none=True)` as the parent `NotebookClient` intended. Discussion about this is ongoing in jupyter/nbgrader#1690. --- IPython/core/interactiveshell.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/IPython/core/interactiveshell.py b/IPython/core/interactiveshell.py index 66ceee05dd6..45ed4e23a19 100644 --- a/IPython/core/interactiveshell.py +++ b/IPython/core/interactiveshell.py @@ -3013,7 +3013,7 @@ def _run_cell( runner = _pseudo_sync_runner try: - return runner(coro) + result = runner(coro) except BaseException as e: info = ExecutionInfo( raw_cell, store_history, silent, shell_futures, cell_id @@ -3021,6 +3021,7 @@ def _run_cell( result = ExecutionResult(info) result.error_in_exec = e self.showtraceback(running_compiled_code=True) + finally: return result def should_run_async( From c7a9470e540392c575aac46c3ee5cf4fe5123eb1 Mon Sep 17 00:00:00 2001 From: Jacob Evan Shreve Date: Wed, 8 Feb 2023 01:29:06 -0500 Subject: [PATCH 2/3] Add some regression tests for this change --- IPython/core/tests/test_interactiveshell.py | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/IPython/core/tests/test_interactiveshell.py b/IPython/core/tests/test_interactiveshell.py index 982bd5a3b22..3fb9fdf5b5d 100644 --- a/IPython/core/tests/test_interactiveshell.py +++ b/IPython/core/tests/test_interactiveshell.py @@ -1110,3 +1110,43 @@ def foo(*args, **kwargs): # clean up ip.Completer.custom_matchers.pop() + + +class TestShowTracebacksAttack(unittest.TestCase): + """Test that the interactive shell is resilient against the client attack of + manipulating the showtracebacks method. These attacks shouldn't result in an + unhandled exception in the kernel.""" + + def test_set_show_tracebacks_none(self): + """Test the case of the client setting showtracebacks to None""" + + result = ip.run_cell( + """ + import IPython.core.interactiveshell + IPython.core.interactiveshell.InteractiveShell.showtraceback = None + + assert False, "This should not raise an exception" + """ + ) + print(result) + + assert result.result is None + assert isinstance(result.error_in_exec, TypeError) + assert str(result.error_in_exec) == "'NoneType' object is not callable" + + def test_set_show_tracebacks_noop(self): + """Test the case of the client setting showtracebacks to a no op lambda""" + + result = ip.run_cell( + """ + import IPython.core.interactiveshell + IPython.core.interactiveshell.InteractiveShell.showtraceback = lambda *args, **kwargs: None + + assert False, "This should not raise an exception" + """ + ) + print(result) + + assert result.result is None + assert isinstance(result.error_in_exec, AssertionError) + assert str(result.error_in_exec) == "This should not raise an exception" From a011765b44febfb11bae122d2ed7db763621ac8f Mon Sep 17 00:00:00 2001 From: Jacob Evan Shreve Date: Wed, 8 Feb 2023 01:50:16 -0500 Subject: [PATCH 3/3] Isolate the attack tests with setUp and tearDown methods --- IPython/core/tests/test_interactiveshell.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/IPython/core/tests/test_interactiveshell.py b/IPython/core/tests/test_interactiveshell.py index 3fb9fdf5b5d..641dcef3cf4 100644 --- a/IPython/core/tests/test_interactiveshell.py +++ b/IPython/core/tests/test_interactiveshell.py @@ -1112,11 +1112,17 @@ def foo(*args, **kwargs): ip.Completer.custom_matchers.pop() -class TestShowTracebacksAttack(unittest.TestCase): +class TestShowTracebackAttack(unittest.TestCase): """Test that the interactive shell is resilient against the client attack of manipulating the showtracebacks method. These attacks shouldn't result in an unhandled exception in the kernel.""" + def setUp(self): + self.orig_showtraceback = interactiveshell.InteractiveShell.showtraceback + + def tearDown(self): + interactiveshell.InteractiveShell.showtraceback = self.orig_showtraceback + def test_set_show_tracebacks_none(self): """Test the case of the client setting showtracebacks to None"""