Skip to content

validate variable names in copyToGlobals debug handler - #1537

Merged
JohanMabille merged 2 commits into
ipython:mainfrom
sahvx655-wq:copytoglobals-validate-names
Sep 8, 2026
Merged

validate variable names in copyToGlobals debug handler#1537
JohanMabille merged 2 commits into
ipython:mainfrom
sahvx655-wq:copytoglobals-validate-names

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

Expression injection via unvalidated variable names in copyToGlobals

While reading through the debug handlers I noticed copyToGlobals drops the client-supplied dstVariableName straight into globals()['{dst_var_name}'] and forwards that to setExpression, and hands srcVariableName over as the value expression, with neither of them checked. Its sibling richInspectVariables already runs str.isidentifier over the name it evaluates, so the omission here stood out. A name such as x'] or __import__('os').system('id') or globals()['y closes the quoted string and injects an arbitrary expression that debugpy then evaluates in the debuggee frame; the forwarded setExpression payload showed the escaped quotes plainly once I logged it.

Left as is this lets a malformed or hostile debug request steer evaluation well beyond a variable copy. The change rejects any dstVariableName/srcVariableName that is not a valid identifier before the expression is built, mirroring the guard the sibling handler already applies. Added a regression under the existing debugger tests that confirms such a request is refused locally rather than forwarded.

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

Pushed a fix for the test_without_debugpy job. The new test asserted on reply["success"] unconditionally, but with debugpy uninstalled the debugger never starts, so process_request returns an empty dict and the whole thing failed with KeyError: 'success' rather than on anything to do with the guard itself.

It now branches on debugpy the same way the rest of that module does, asserting the rejection when debugpy is present and reply == {} when it isn't. Ran tests/test_debugger.py both ways locally, 12 pass in each.

@sahvx655-wq

sahvx655-wq commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Following on from the above, the two checks still showing red aren't from this branch. Test Lint is four "InteractiveShell | None" union-attr errors in debugger.py at lines 91, 446, 459 and 644, none of which this PR touches. I reproduced them against a clean 342cf58 worktree with the same mypy and IPython 9.16, so it looks like the unpinned ipython>=7.23.1 in the mypy hook picking up the newer optional get_ipython() return rather than anything here.

The windows qt5 3.14 job times out inside matplotlib's font_manager while it builds the font cache, which is environmental. Neither is something this branch can fix, but say the word if you'd rather I rebase once main is green.

@sahvx655-wq
sahvx655-wq force-pushed the copytoglobals-validate-names branch from c17d477 to 5e2f6db Compare September 8, 2026 08:06
@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

Rebased onto main. The four union-attr errors behind the Test Lint failure were fixed there by f53affa, which casts the get_ipython() sites to InteractiveShell, and the last five nightlies on 7c9f9b4 are green. The diff is unchanged, only the base moved; the mypy hook and ruff both pass against the rebased branch locally.

The new run is green across all 36 checks, including the windows qt5 3.14 job that previously timed out in matplotlib's font cache and test_without_debugpy on its new 3.11 matrix entry. Ran tests/test_debugger.py with and without debugpy against the rebased branch, 12 pass in each.

@JohanMabille JohanMabille left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix! I wonder if we should also return a response carrying the copyToGlobals command in case of success, but that's orthogonal to this PR.

@JohanMabille
JohanMabille merged commit 213cbf6 into ipython:main Sep 8, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants