Skip to content
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
2 changes: 1 addition & 1 deletion src/debugpy/server/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ def attach_to_pid():
)
tmp_file.write(python_code.encode())

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.

@copilot This .replace("\\", "/") patches the symptom rather than the root cause and is the lone raw-string outlier in this function — script_dir/setup already use the escaping-immune encode = list(bytearray(...)) + codecs.utf_8_decode pattern a few lines above. The Skeptic verified on CPython 3.11.9 that .replace still leaves a quote gap (a " in the path raises SyntaxError) and a POSIX regression (a backslash path retargets os.remove to a different file than sys.remote_exec receives). Both are unreachable today given tempfile's name alphabet, but repr(tmp_file_path) or reusing the existing encode/decode pattern closes both simultaneously and restores a single representation of tmp_file_path.

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to {tmp_file_path!r} (per PR #2039) — this produces a valid Python string literal for any path without modifying it, closing both the Windows backslash and POSIX quote/backslash-filename gaps.

tmp_file.write(
"""import os;os.remove("{tmp_file_path}");""".format(
"""import os;os.remove({tmp_file_path!r});""".format(
tmp_file_path=tmp_file_path
).encode()
)
Expand Down
61 changes: 61 additions & 0 deletions tests/debugpy/server/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,64 @@ def test_script_parent_pid_with_listen_failure(cli):
cli(["--listen", "8888", "--parent-session-pid", "1234", "spam.py"])

assert "--parent-session-pid requires --connect" in str(ex.value)


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.

@copilot test_pep_768_code_injection_path_with_backslashes is tautological: it redeclares private copies of the broken/fixed templates and asserts only stdlib compile()/str.replace() behavior — it never imports or calls cli.py. The Skeptic and Architect both verified that reverting the production .replace("\\", "/") leaves this test green, so it provides false coverage. Delete it (Test 2 already covers the real path) or rewrite it to drive cli.attach_to_pid().

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deleted test_pep_768_code_injection_path_with_backslashes — it never called into cli.py so it provided false coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 843c0d16test_pep_768_code_injection_path_with_backslashes has been deleted.

def test_pep_768_remote_exec_called_with_backslash_path():
"""Test that attach_to_pid() calls sys.remote_exec and writes valid Python
to the temp file even when the temp path contains backslashes (Windows)."""
import contextlib
from debugpy.server import cli

mock_windows_tmp_path = r"C:\Users\test\AppData\Local\Temp\tmp0_vuee4s"
pid = os.getpid()

# A fake file object that captures writes via a side_effect list.
# Using MagicMock avoids the BytesIO.close() issue where getvalue()
# raises ValueError on a closed buffer.
written_chunks = []
fake_file = mock.MagicMock()
fake_file.name = mock_windows_tmp_path
fake_file.write.side_effect = written_chunks.append

fake_file_cm = mock.MagicMock()
fake_file_cm.__enter__ = mock.Mock(return_value=fake_file)
fake_file_cm.__exit__ = mock.Mock(return_value=False)

# Configure cli.options with the minimum fields attach_to_pid() needs.
original_options = {
attr: getattr(cli.options, attr)
for attr in ("mode", "address", "wait_for_client", "log_to",
"adapter_access_token", "disable_sys_remote_exec", "target")
}
try:
cli.options.mode = "connect"
cli.options.address = ("127.0.0.1", 5678)
cli.options.wait_for_client = False
cli.options.log_to = None
cli.options.adapter_access_token = None
cli.options.disable_sys_remote_exec = False
cli.options.target = pid

with contextlib.ExitStack() as stack:
stack.enter_context(
mock.patch("tempfile.NamedTemporaryFile", return_value=fake_file_cm)
)
mock_remote_exec = stack.enter_context(
mock.patch.object(sys, "remote_exec", create=True)
)
cli.attach_to_pid()

# sys.remote_exec must have been called with the original (backslash) path
# because that is the actual path created on disk.
mock_remote_exec.assert_called_once_with(pid, mock_windows_tmp_path)

# The Python code written to the temp file must be syntactically valid.
injected_code = b"".join(written_chunks).decode()
compile(injected_code, "<string>", "exec")

# The os.remove() call inside the injected code must use the repr of the path
# so that it is a valid Python string literal on all platforms.
assert "import os;os.remove({});".format(repr(mock_windows_tmp_path)) in injected_code
finally:
for attr, value in original_options.items():
setattr(cli.options, attr, value)
Loading