From 72ff9b1a7dace3063f95023a14f251362a9b9d29 Mon Sep 17 00:00:00 2001 From: LeonarthCG <33758848+LeonarthCG@users.noreply.github.com> Date: Tue, 10 Mar 2026 19:12:48 +0100 Subject: [PATCH] Saving Princess: Security fixes for issues detected by Bandit (#6013) * Saving Princess: absolute paths on suprocess.run * Saving Princess: more error handling for downloads * Saving Princess: rework launch_command setting Apparently subprocess.Popen requires a list for args instead of a string everywhere but in Windows, so the change was preventing the game from running on Linux. Additionally, the game is now launched using absolute paths. * Saving Princess: prevent bandit warnings * Saving Princess: remove unnecessary compare_digest * Saving Princess: fix Linux paths by using which * Saving Princess: rename launch command setting Previously, launch_command held a string. Now it holds a list of strings. Additionally, the defaults have changed. To prevent the wrong type from being used, the setting has been renamed, effectively abandoning the original launch_command setting. * Saving Princess: fix Linux default command return type Co-authored-by: black-sliver <59490463+black-sliver@users.noreply.github.com> --------- Co-authored-by: black-sliver <59490463+black-sliver@users.noreply.github.com> --- worlds/saving_princess/Client.py | 42 +++++++++++++++++++----------- worlds/saving_princess/__init__.py | 16 +++++++++--- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/worlds/saving_princess/Client.py b/worlds/saving_princess/Client.py index 29a97bb667..195a6a5703 100644 --- a/worlds/saving_princess/Client.py +++ b/worlds/saving_princess/Client.py @@ -1,4 +1,5 @@ import argparse +import ssl import zipfile from io import BytesIO @@ -8,12 +9,13 @@ import hashlib import json import logging import os + +import certifi import requests -import secrets import shutil -import subprocess +import subprocess # nosec from tkinter import messagebox -from typing import Any, Dict, Set +from typing import Any, Dict, Set, List import urllib import urllib.parse @@ -90,7 +92,7 @@ def get_timestamp(date: str) -> float: def send_request(request_url: str) -> UrlResponse: """Fetches status code and json response from given url""" - response = requests.get(request_url) + response = requests.get(request_url, timeout=10) if response.status_code == 200: # success try: data = response.json() @@ -129,13 +131,16 @@ def update(target_asset: str, url: str) -> bool: if update_available and messagebox.askyesnocancel(f"New {target_asset}", "Would you like to install the new version now?"): # unzip and patch - with urllib.request.urlopen(release_url) as download: + if not release_url.lower().startswith("https"): + raise ValueError(f'Unexpected scheme for url "{release_url}".') + context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=certifi.where()) + with urllib.request.urlopen(release_url, context=context) as download: # nosec with zipfile.ZipFile(BytesIO(download.read())) as zf: zf.extractall() patch_game() set_date(target_asset, newest_date) - except (ValueError, RuntimeError, urllib.error.HTTPError): - update_error = f"Failed to apply update." + except (ValueError, RuntimeError, urllib.error.HTTPError, urllib.error.URLError) as e: + update_error = f"Failed to apply update:\n{e}" messagebox.showerror("Failure", update_error) raise RuntimeError(update_error) return True @@ -158,8 +163,8 @@ def is_install_valid() -> bool: if not os.path.exists(file_name): return False with open(file_name, "rb") as clean: - current_hash = hashlib.md5(clean.read()).hexdigest() - if not secrets.compare_digest(current_hash, expected_hash): + current_hash = hashlib.md5(clean.read(), usedforsecurity=False).hexdigest() + if current_hash != expected_hash: return False return True @@ -189,12 +194,16 @@ def install() -> None: logging.info("Extracting files from cab archive.") if Utils.is_windows: - subprocess.run(["Extrac32", "/Y", "/E", "saving_princess.cab"]) + windows_path = os.environ["WINDIR"] + extractor_path = f"{windows_path}/System32/Extrac32" + subprocess.run([extractor_path, "/Y", "/E", "saving_princess.cab"]) #nosec else: - if shutil.which("wine") is not None: - subprocess.run(["wine", "Extrac32", "/Y", "/E", "saving_princess.cab"]) - elif shutil.which("7z") is not None: - subprocess.run(["7z", "e", "saving_princess.cab"]) + wine_path = shutil.which("wine") + p7zip_path = shutil.which("7z") + if wine_path is not None: + subprocess.run([wine_path, "Extrac32", "/Y", "/E", "saving_princess.cab"]) #nosec + elif p7zip_path is not None: + subprocess.run([p7zip_path, "e", "saving_princess.cab"]) #nosec else: error = "Could not find neither wine nor 7z.\n\nPlease install either the wine or the p7zip package." messagebox.showerror("Missing package!", f"Error: {error}") @@ -250,7 +259,10 @@ def launch(*args: str) -> Any: if SavingPrincessWorld.settings.launch_game: logging.info("Launching game.") try: - subprocess.Popen(f"{SavingPrincessWorld.settings.launch_command} {name} {password} {server}") + game: str = os.path.join(os.getcwd(), "Saving Princess v0_8.exe") + launch_command: List[str] = (SavingPrincessWorld.settings.launch_command_with_args + + [game, name, password, server]) + subprocess.Popen(launch_command) # nosec except FileNotFoundError: error = ("Could not run the game!\n\n" "Please check that launch_command in options.yaml or host.yaml is set up correctly.") diff --git a/worlds/saving_princess/__init__.py b/worlds/saving_princess/__init__.py index b4caf3828c..0c6208638a 100644 --- a/worlds/saving_princess/__init__.py +++ b/worlds/saving_princess/__init__.py @@ -1,3 +1,4 @@ +import shutil from typing import ClassVar, Dict, Any, Type, List, Union import Utils @@ -20,6 +21,15 @@ components.append( ) +def get_default_launch_command() -> List[str]: + """Returns platform-dependant default launch command for Saving Princess""" + if Utils.is_windows: + return [] + else: + wine_path = shutil.which("wine") + return [wine_path] if wine_path is not None else ["/usr/bin/wine"] + + class SavingPrincessSettings(Group): class GamePath(UserFilePath): """Path to the game executable from which files are extracted""" @@ -34,17 +44,17 @@ class SavingPrincessSettings(Group): class LaunchGame(Bool): """Set this to false to never autostart the game""" - class LaunchCommand(str): + class LaunchCommandWithArgs(List[str]): """ The console command that will be used to launch the game The command will be executed with the installation folder as the current directory + Additional items in the list will be passed in as arguments """ exe_path: GamePath = GamePath("Saving Princess.exe") install_folder: InstallFolder = InstallFolder("Saving Princess") launch_game: Union[LaunchGame, bool] = True - launch_command: LaunchCommand = LaunchCommand('"Saving Princess v0_8.exe"' if Utils.is_windows - else 'wine "Saving Princess v0_8.exe"') + launch_command_with_args: LaunchCommandWithArgs = LaunchCommandWithArgs(get_default_launch_command()) class SavingPrincessWeb(WebWorld):