validate variable names in copyToGlobals debug handler - #1537
validate variable names in copyToGlobals debug handler#1537sahvx655-wq wants to merge 2 commits into
Conversation
|
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. |
|
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. |
Expression injection via unvalidated variable names in copyToGlobals
While reading through the debug handlers I noticed
copyToGlobalsdrops the client-supplieddstVariableNamestraight intoglobals()['{dst_var_name}']and forwards that tosetExpression, and handssrcVariableNameover as the value expression, with neither of them checked. Its siblingrichInspectVariablesalready runsstr.isidentifierover the name it evaluates, so the omission here stood out. A name such asx'] or __import__('os').system('id') or globals()['ycloses the quoted string and injects an arbitrary expression that debugpy then evaluates in the debuggee frame; the forwardedsetExpressionpayload 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/srcVariableNamethat 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.