From 1d6646d1e03efdd247a0ee29fd406b51915f1995 Mon Sep 17 00:00:00 2001 From: "eric.quintero@trailofbits.com" Date: Tue, 2 Jun 2026 16:26:20 +0000 Subject: [PATCH] Quote remote release tool commands --- run_release.py | 56 +++++--- tests/test_run_release.py | 193 ++++++++++++++++++++++++++++ tests/test_windows_merge_upload.py | 100 ++++++++++++++ windows-release/merge-and-upload.py | 39 +++++- 4 files changed, 363 insertions(+), 25 deletions(-) create mode 100644 tests/test_windows_merge_upload.py diff --git a/run_release.py b/run_release.py index b7cfe0e3..fbe3da47 100755 --- a/run_release.py +++ b/run_release.py @@ -45,6 +45,11 @@ DOWNLOADS_SERVER = "downloads.nyc1.psf.io" DOCS_SERVER = "docs.nyc1.psf.io" + +def quote_remote_shell(value: object) -> str: + return shlex.quote(str(value)) + + WHATS_NEW_TEMPLATE = """ ***************************** What's new in Python {version} @@ -755,7 +760,7 @@ def upload_files_to_server(db: ReleaseShelf, server: str) -> None: ftp_client = MySFTPClient.from_transport(transport) assert ftp_client is not None, f"SFTP client to {server} is None" - client.exec_command(f"rm -rf {destination}") + client.exec_command(f"rm -rf {quote_remote_shell(destination)}") with contextlib.suppress(OSError): ftp_client.mkdir(str(destination)) @@ -810,28 +815,29 @@ def execute_command(command: str) -> None: raise ReleaseException(channel.recv_stderr(1000)) def copy_and_set_permissions(source_glob: str, destination: str) -> None: - execute_command(f"mkdir -p {destination}") - execute_command(f"cp {source_glob} {destination}") + quoted_destination = quote_remote_shell(destination) + execute_command(f"mkdir -p {quoted_destination}") + execute_command(f"cp {source_glob} {quoted_destination}") # Skip chgrp/chmod if already correct: another RM may have created # the directory, and only the owner can change group or permissions. execute_command( - f"find {destination} -maxdepth 0 ! -group downloads " + f"find {quoted_destination} -maxdepth 0 ! -group downloads " f"-exec chgrp downloads {{}} +" ) execute_command( - f"find {destination} -maxdepth 0 ! -perm 775 -exec chmod 775 {{}} +" + f"find {quoted_destination} -maxdepth 0 ! -perm 775 -exec chmod 775 {{}} +" ) execute_command( - f"find {destination} -type f ! -perm 664 -exec chmod 664 {{}} +" + f"find {quoted_destination} -type f ! -perm 664 -exec chmod 664 {{}} +" ) - copy_and_set_permissions(f"{source}/downloads/*", destination) + copy_and_set_permissions(f"{quote_remote_shell(source)}/downloads/*", destination) # Docs release_tag = db["release"] if release_tag.is_final or release_tag.is_release_candidate: copy_and_set_permissions( - f"{source}/docs/*", + f"{quote_remote_shell(source)}/docs/*", f"/srv/www.python.org/ftp/python/doc/{release_tag}", ) @@ -870,13 +876,20 @@ def execute_command(command: str) -> None: raise ReleaseException(channel.recv_stderr(1000)) docs_filename = f"python-{release_tag}-docs-html" - execute_command(f"mkdir -p {destination}") - execute_command(f"unzip {source}/docs/{docs_filename}.zip -d {destination}") - execute_command(f"mv /{destination}/{docs_filename}/* {destination}") - execute_command(f"rm -rf /{destination}/{docs_filename}") - execute_command(f"chgrp -R docs {destination}") - execute_command(f"chmod -R 775 {destination}") - execute_command(f"find {destination} -type f -exec chmod 664 {{}} \\;") + quoted_destination = quote_remote_shell(destination) + execute_command(f"mkdir -p {quoted_destination}") + execute_command( + f"unzip {quote_remote_shell(f'{source}/docs/{docs_filename}.zip')} " + f"-d {quoted_destination}" + ) + execute_command( + f"mv {quote_remote_shell(f'/{destination}/{docs_filename}')}/* " + f"{quoted_destination}" + ) + execute_command(f"rm -rf {quote_remote_shell(f'/{destination}/{docs_filename}')}") + execute_command(f"chgrp -R docs {quoted_destination}") + execute_command(f"chmod -R 775 {quoted_destination}") + execute_command(f"find {quoted_destination} -type f -exec chmod 664 {{}} \\;") @functools.cache @@ -1088,12 +1101,19 @@ def run_add_to_python_dot_org(db: ReleaseShelf) -> None: # Do the interactive flow to get an identity for Sigstore issuer = sigstore.oidc.Issuer(sigstore.oidc.DEFAULT_OAUTH_ISSUER_URL) - identity_token = issuer.identity_token() + identity_token = str(issuer.identity_token()) print("Adding files to python.org...") - stdin, stdout, stderr = client.exec_command( - f"AUTH_INFO={auth_info} SIGSTORE_IDENTITY_TOKEN={identity_token} python3 add_to_pydotorg.py {db['release']}" + command = " ".join( + [ + f"AUTH_INFO={quote_remote_shell(auth_info)}", + f"SIGSTORE_IDENTITY_TOKEN={quote_remote_shell(identity_token)}", + "python3", + "add_to_pydotorg.py", + quote_remote_shell(db["release"]), + ] ) + stdin, stdout, stderr = client.exec_command(command) stderr_text = stderr.read().decode() if stderr_text: raise paramiko.SSHException(f"Failed to execute the command: {stderr_text}") diff --git a/tests/test_run_release.py b/tests/test_run_release.py index 82830a85..d9387c3f 100644 --- a/tests/test_run_release.py +++ b/tests/test_run_release.py @@ -248,3 +248,196 @@ def test_update_whatsnew_toctree(tmp_path: Path) -> None: # Assert new_contents = toctree__file.read_text() assert " 3.15.rst\n 3.14.rst\n" in new_contents + + +def test_run_add_to_python_dot_org_quotes_remote_environment(monkeypatch) -> None: + commands = [] + + class FakeSFTPClient: + def put(self, source: str, destination: str) -> None: + pass + + def close(self) -> None: + pass + + class FakeSSHClient: + def load_system_host_keys(self) -> None: + pass + + def set_missing_host_key_policy(self, policy) -> None: + pass + + def connect(self, *args, **kwargs) -> None: + pass + + def get_transport(self): + return object() + + def exec_command(self, command: str): + commands.append(command) + return None, io.BytesIO(b"ok"), io.BytesIO() + + class FakeIssuer: + def __init__(self, issuer_url: str) -> None: + self.issuer_url = issuer_url + + def identity_token(self) -> str: + return "token; touch /tmp/pwned" + + monkeypatch.setattr(run_release.paramiko, "SSHClient", FakeSSHClient) + monkeypatch.setattr( + run_release.MySFTPClient, + "from_transport", + staticmethod(lambda transport: FakeSFTPClient()), + ) + monkeypatch.setattr(run_release.sigstore.oidc, "Issuer", FakeIssuer) + + db = { + "auth_info": "user:key; echo pwned", + "release": Tag("3.15.0a1"), + "ssh_key": None, + "ssh_user": "release-manager", + } + + run_release.run_add_to_python_dot_org(cast(ReleaseShelf, db)) + + assert commands == [ + "AUTH_INFO='user:key; echo pwned' " + "SIGSTORE_IDENTITY_TOKEN='token; touch /tmp/pwned' " + "python3 add_to_pydotorg.py 3.15.0a1" + ] + + +def test_upload_files_to_server_quotes_remote_cleanup_path( + monkeypatch, tmp_path: Path +) -> None: + commands = [] + + class FakeSFTPClient: + def mkdir(self, path: str) -> None: + pass + + def put_dir(self, source: Path, target: str, progress) -> None: + pass + + def close(self) -> None: + pass + + class FakeSSHClient: + def load_system_host_keys(self) -> None: + pass + + def set_missing_host_key_policy(self, policy) -> None: + pass + + def connect(self, *args, **kwargs) -> None: + pass + + def get_transport(self): + return object() + + def exec_command(self, command: str) -> None: + commands.append(command) + + @contextlib.contextmanager + def fake_alive_bar(total: int): + yield lambda *args, **kwargs: None + + release = Tag("3.15.0a1") + artifacts_path = tmp_path / str(release) + (artifacts_path / "downloads").mkdir(parents=True) + + monkeypatch.setattr(run_release.paramiko, "SSHClient", FakeSSHClient) + monkeypatch.setattr( + run_release.MySFTPClient, + "from_transport", + staticmethod(lambda transport: FakeSFTPClient()), + ) + monkeypatch.setattr(run_release, "alive_bar", fake_alive_bar) + + db = { + "git_repo": tmp_path, + "release": release, + "ssh_key": None, + "ssh_user": "release-manager; touch /tmp/pwned #", + } + + run_release.upload_files_to_server( + cast(ReleaseShelf, db), run_release.DOWNLOADS_SERVER + ) + + assert commands == [ + "rm -rf '/home/psf-users/release-manager; touch /tmp/pwned #/3.15.0a1'" + ] + + +def test_release_file_placement_quotes_remote_paths(monkeypatch) -> None: + commands = [] + + class FakeChannel: + def exec_command(self, command: str) -> None: + commands.append(command) + + def recv_exit_status(self) -> int: + return 0 + + def recv_stderr(self, size: int) -> bytes: + return b"" + + class FakeTransport: + def open_session(self) -> FakeChannel: + return FakeChannel() + + class FakeSSHClient: + def load_system_host_keys(self) -> None: + pass + + def set_missing_host_key_policy(self, policy) -> None: + pass + + def connect(self, *args, **kwargs) -> None: + pass + + def get_transport(self) -> FakeTransport: + return FakeTransport() + + monkeypatch.setattr(run_release.paramiko, "SSHClient", FakeSSHClient) + + db = { + "release": Tag("3.15.0rc1"), + "ssh_key": None, + "ssh_user": "release-manager; touch /tmp/pwned #", + } + + run_release.place_files_in_download_folder(cast(ReleaseShelf, db)) + run_release.unpack_docs_in_the_docs_server(cast(ReleaseShelf, db)) + + assert commands == [ + "mkdir -p /srv/www.python.org/ftp/python/3.15.0", + "cp '/home/psf-users/release-manager; touch /tmp/pwned #/3.15.0rc1'/downloads/* " + "/srv/www.python.org/ftp/python/3.15.0", + "find /srv/www.python.org/ftp/python/3.15.0 -maxdepth 0 ! -group downloads " + "-exec chgrp downloads {} +", + "find /srv/www.python.org/ftp/python/3.15.0 -maxdepth 0 ! -perm 775 " + "-exec chmod 775 {} +", + "find /srv/www.python.org/ftp/python/3.15.0 -type f ! -perm 664 " + "-exec chmod 664 {} +", + "mkdir -p /srv/www.python.org/ftp/python/doc/3.15.0rc1", + "cp '/home/psf-users/release-manager; touch /tmp/pwned #/3.15.0rc1'/docs/* " + "/srv/www.python.org/ftp/python/doc/3.15.0rc1", + "find /srv/www.python.org/ftp/python/doc/3.15.0rc1 -maxdepth 0 ! -group downloads " + "-exec chgrp downloads {} +", + "find /srv/www.python.org/ftp/python/doc/3.15.0rc1 -maxdepth 0 ! -perm 775 " + "-exec chmod 775 {} +", + "find /srv/www.python.org/ftp/python/doc/3.15.0rc1 -type f ! -perm 664 " + "-exec chmod 664 {} +", + "mkdir -p /srv/docs.python.org/release/3.15.0rc1", + "unzip '/home/psf-users/release-manager; touch /tmp/pwned #/3.15.0rc1/docs/" + "python-3.15.0rc1-docs-html.zip' -d /srv/docs.python.org/release/3.15.0rc1", + "mv //srv/docs.python.org/release/3.15.0rc1/python-3.15.0rc1-docs-html/* " + "/srv/docs.python.org/release/3.15.0rc1", + "rm -rf //srv/docs.python.org/release/3.15.0rc1/python-3.15.0rc1-docs-html", + "chgrp -R docs /srv/docs.python.org/release/3.15.0rc1", + "chmod -R 775 /srv/docs.python.org/release/3.15.0rc1", + "find /srv/docs.python.org/release/3.15.0rc1 -type f -exec chmod 664 {} \\;", + ] diff --git a/tests/test_windows_merge_upload.py b/tests/test_windows_merge_upload.py new file mode 100644 index 00000000..c89e4a06 --- /dev/null +++ b/tests/test_windows_merge_upload.py @@ -0,0 +1,100 @@ +import importlib.util +from pathlib import Path +from typing import Any + +import pytest + + +def load_merge_upload_module(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Any: + for name in ( + "INDEX_FILE", + "LOCAL_INDEX", + "MAKECAT", + "MANIFEST_FILE", + "NO_UPLOAD", + "PLINK", + "PSCP", + "SIGN_COMMAND", + "UPLOAD_HOST", + "UPLOAD_HOST_KEY", + "UPLOAD_KEYFILE", + "UPLOAD_PATH_PREFIX", + "UPLOAD_URL_PREFIX", + "UPLOAD_USER", + ): + monkeypatch.delenv(name, raising=False) + + monkeypatch.chdir(tmp_path) + script = Path(__file__).parents[1] / "windows-release" / "merge-and-upload.py" + spec = importlib.util.spec_from_file_location("merge_and_upload_for_test", script) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + + with pytest.raises(SystemExit) as exc_info: + spec.loader.exec_module(module) + + assert exc_info.value.code == 1 + return module + + +def test_remote_upload_commands_quote_url_derived_paths( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + module = load_merge_upload_module(monkeypatch, tmp_path) + calls: list[tuple[tuple[object, ...], bool]] = [] + + def fake_run(*args: object, single_cmd: bool = False) -> str: + calls.append((args, single_cmd)) + return "" + + module._run = fake_run + module.PLINK = "plink.exe" + module.PSCP = "pscp.exe" + module.UPLOAD_HOST = "downloads.example.org" + module.UPLOAD_USER = "release-manager" + + dest = module.url2path( + "https://www.python.org/ftp/python/3.14.0;touch PTP/python-3.14.0-amd64.exe" + ) + + module.prepare_upload_dir(dest) + module.upload_ssh("python-3.14.0-amd64.exe", dest) + + assert calls == [ + ( + ( + "plink.exe", + "-batch", + "release-manager@downloads.example.org", + "mkdir '/srv/www.python.org/ftp/python/3.14.0;touch PTP' && " + "chgrp downloads '/srv/www.python.org/ftp/python/3.14.0;touch PTP' && " + "chmod a+rx '/srv/www.python.org/ftp/python/3.14.0;touch PTP'", + ), + False, + ), + ( + ( + "pscp.exe", + "-batch", + "python-3.14.0-amd64.exe", + "release-manager@downloads.example.org:" + "'/srv/www.python.org/ftp/python/3.14.0;touch PTP/" + "python-3.14.0-amd64.exe'", + ), + False, + ), + ( + ( + "plink.exe", + "-batch", + "release-manager@downloads.example.org", + "chgrp downloads " + "'/srv/www.python.org/ftp/python/3.14.0;touch PTP/" + "python-3.14.0-amd64.exe' && chmod g-x,o+r " + "'/srv/www.python.org/ftp/python/3.14.0;touch PTP/" + "python-3.14.0-amd64.exe'", + ), + False, + ), + ] diff --git a/windows-release/merge-and-upload.py b/windows-release/merge-and-upload.py index 5e00a6d1..65152d16 100644 --- a/windows-release/merge-and-upload.py +++ b/windows-release/merge-and-upload.py @@ -2,6 +2,7 @@ import json import os import re +import shlex import subprocess import sys from pathlib import Path @@ -67,6 +68,10 @@ class RunError(Exception): pass +def quote_remote_shell(value): + return shlex.quote(str(value)) + + def _run(*args, single_cmd=False): if single_cmd: args = args[0] @@ -101,8 +106,13 @@ def upload_ssh(source, dest): if not UPLOAD_HOST or NO_UPLOAD or LOCAL_INDEX: print("Skipping upload of", source, "because UPLOAD_HOST is missing") return - _run(*_std_args(PSCP), source, f"{UPLOAD_USER}@{UPLOAD_HOST}:{dest}") - call_ssh(f"chgrp downloads {dest} && chmod g-x,o+r {dest}") + _run( + *_std_args(PSCP), + source, + f"{UPLOAD_USER}@{UPLOAD_HOST}:{quote_remote_shell(dest)}", + ) + quoted_dest = quote_remote_shell(dest) + call_ssh(f"chgrp downloads {quoted_dest} && chmod g-x,o+r {quoted_dest}") def download_ssh(source, dest): @@ -110,7 +120,21 @@ def download_ssh(source, dest): print("Skipping download of", source, "because UPLOAD_HOST is missing") return Path(dest).parent.mkdir(exist_ok=True, parents=True) - _run(*_std_args(PSCP), f"{UPLOAD_USER}@{UPLOAD_HOST}:{source}", dest) + _run( + *_std_args(PSCP), + f"{UPLOAD_USER}@{UPLOAD_HOST}:{quote_remote_shell(source)}", + dest, + ) + + +def prepare_upload_dir(dest): + destdir = dest.rpartition("/")[0] + quoted_destdir = quote_remote_shell(destdir) + call_ssh( + f"mkdir {quoted_destdir} && " + f"chgrp downloads {quoted_destdir} && " + f"chmod a+rx {quoted_destdir}" + ) def url2path(url): @@ -296,7 +320,9 @@ def find_missing_from_index(url, installs): INDEX_PATH = url2path(INDEX_URL) try: - INDEX_MTIME = int(call_ssh("stat", "-c", "%Y", INDEX_PATH) or "0") + INDEX_MTIME = int( + call_ssh("stat", "-c", "%Y", quote_remote_shell(INDEX_PATH)) or "0" + ) except ValueError: pass @@ -356,8 +382,7 @@ def find_missing_from_index(url, installs): # Upload last to ensure we've got a valid index first for i, src, dest, sbom, sbom_dest in UPLOADS: print("Uploading", src, "to", dest) - destdir = dest.rpartition("/")[0] - call_ssh(f"mkdir {destdir} && chgrp downloads {destdir} && chmod a+rx {destdir}") + prepare_upload_dir(dest) upload_ssh(src, dest) if sbom and sbom_dest: upload_ssh(sbom, sbom_dest) @@ -366,7 +391,7 @@ def find_missing_from_index(url, installs): # Check that nobody else has published while we were uploading if INDEX_FILE and INDEX_MTIME: try: - mtime = int(call_ssh("stat", "-c", "%Y", INDEX_PATH) or "0") + mtime = int(call_ssh("stat", "-c", "%Y", quote_remote_shell(INDEX_PATH)) or "0") except ValueError: mtime = 0 if mtime > INDEX_MTIME: