Skip to content

validate variable names in copyToGlobals debug handler - #1537

Open
sahvx655-wq wants to merge 2 commits into
ipython:mainfrom
sahvx655-wq:copytoglobals-validate-names
Open

validate variable names in copyToGlobals debug handler#1537
sahvx655-wq wants to merge 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.

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