From 703c65395cf1603ebc9c49c68b28e5a3a9d87fc5 Mon Sep 17 00:00:00 2001 From: Bananaman <38923130+Bananaman@users.noreply.github.com> Date: Fri, 24 Mar 2023 21:44:31 +0100 Subject: [PATCH] Improved game executable launcher, and added argument validation Now uses proper shell escaping and parsing for all executable arguments, for more robust game launching. All existing game JSON files with the old string values will automatically be converted to the new format on app launch. The executable parsing uses the "shlex" library, which guarantees accurate parsing. We now also use direct process launching (without any intermediary shell) by default, but the old "shell"-based launch method still exists in the code via an alternative flag in `run_command.py` (if we ever need to restore it for some reason). Furthermore, if the user attempts to manually write an improperly escaped argument into the game's details (such as missing closing quotation marks), the GUI will now alert the user that their executable argument is invalid, along with telling them the exact reason why it's invalid. --- src/utils/bottles_parser.py | 5 ++-- src/utils/create_details_window.py | 20 +++++++++++++-- src/utils/get_games.py | 29 ++++++++++++++++++++-- src/utils/heroic_parser.py | 12 ++++----- src/utils/run_command.py | 39 ++++++++++++++++++++++-------- src/utils/steam_parser.py | 4 +-- 6 files changed, 85 insertions(+), 24 deletions(-) diff --git a/src/utils/bottles_parser.py b/src/utils/bottles_parser.py index 85fd3c1..18778a1 100644 --- a/src/utils/bottles_parser.py +++ b/src/utils/bottles_parser.py @@ -112,9 +112,10 @@ def bottles_parser(parent_widget, action): continue values["name"] = game["name"] - values["executable"] = "xdg-open " + shlex.quote( + values["executable"] = [ + "xdg-open", f'bottles:run/{game["bottle"]["name"]}/{game["name"]}' - ) + ] values["hidden"] = False values["source"] = "bottles" values["added"] = current_time diff --git a/src/utils/create_details_window.py b/src/utils/create_details_window.py index 0ca5137..40fc371 100644 --- a/src/utils/create_details_window.py +++ b/src/utils/create_details_window.py @@ -19,6 +19,7 @@ import json import os +import shlex import time from gi.repository import Adw, GdkPixbuf, Gio, GLib, GObject, Gtk @@ -52,7 +53,7 @@ def create_details_window(parent_widget, game_id=None): ) name = Gtk.Entry.new_with_buffer(Gtk.EntryBuffer.new(games[game_id].name, -1)) executable = Gtk.Entry.new_with_buffer( - Gtk.EntryBuffer.new((games[game_id].executable), -1) + Gtk.EntryBuffer.new(shlex.join(games[game_id].executable), -1) ) apply_button = Gtk.Button.new_with_label(_("Apply")) @@ -201,6 +202,21 @@ def create_details_window(parent_widget, game_id=None): final_developer = developer.get_buffer().get_text() final_executable = executable.get_buffer().get_text() + try: + # Attempt to parse using shell parsing rules (doesn't verify executable existence). + final_executable_split = shlex.split( + final_executable, comments=False, posix=True + ) + except Exception as e: + create_dialog( + window, + _("Couldn't Add Game") + if game_id is None + else _("Couldn't Apply Preferences"), + f'{_("Executable")}: {e}.', # e = Shell parsing error message. Not translatable. + ) + return + if game_id is None: if final_name == "": create_dialog( @@ -252,7 +268,7 @@ def create_details_window(parent_widget, game_id=None): values["name"] = final_name values["developer"] = final_developer or None - values["executable"] = final_executable + values["executable"] = final_executable_split path = os.path.join( os.path.join( diff --git a/src/utils/get_games.py b/src/utils/get_games.py index d2e0423..73acfe9 100644 --- a/src/utils/get_games.py +++ b/src/utils/get_games.py @@ -19,6 +19,9 @@ import json import os +import shlex + +from .game_data_to_json import game_data_to_json def get_games(game_ids=None): @@ -39,8 +42,30 @@ def get_games(game_ids=None): game_files = os.listdir(games_dir) for game in game_files: - with open(os.path.join(games_dir, game), "r") as open_file: + with open(os.path.join(games_dir, game), "r+") as open_file: data = json.loads(open_file.read()) + + # Convert any outdated JSON values to our newest data format. + needs_rewrite = False + if "executable" in data and isinstance(data["executable"], str): + needs_rewrite = True + try: + # Use shell parsing to determine what the individual components are. + executable_split = shlex.split( + data["executable"], comments=False, posix=True + ) + except: + # Fallback: Split once at earliest space (1 part if no spaces, else 2 parts). + executable_split = data["executable"].split(" ", 1) + data["executable"] = executable_split + + if needs_rewrite: + open_file.seek(0) + open_file.truncate() + open_file.write(game_data_to_json(data)) + open_file.close() - games[data["game_id"]] = data + + games[data["game_id"]] = data + return games diff --git a/src/utils/heroic_parser.py b/src/utils/heroic_parser.py index 494f6c4..a579391 100644 --- a/src/utils/heroic_parser.py +++ b/src/utils/heroic_parser.py @@ -129,9 +129,9 @@ def heroic_parser(parent_widget, action): values["name"] = game["title"] values["developer"] = game["developer"] values["executable"] = ( - f"start heroic://launch/{app_name}" + ["start", f"heroic://launch/{app_name}"] if os.name == "nt" - else f"xdg-open heroic://launch/{app_name}" + else ["xdg-open", f"heroic://launch/{app_name}"] ) values["hidden"] = False values["source"] = "heroic_epic" @@ -195,9 +195,9 @@ def heroic_parser(parent_widget, action): break values["executable"] = ( - f"start heroic://launch/{app_name}" + ["start", f"heroic://launch/{app_name}"] if os.name == "nt" - else f"xdg-open heroic://launch/{app_name}" + else ["xdg-open", f"heroic://launch/{app_name}"] ) values["hidden"] = False values["source"] = "heroic_gog" @@ -230,9 +230,9 @@ def heroic_parser(parent_widget, action): values["name"] = item["title"] values["executable"] = ( - f"start heroic://launch/{app_name}" + ["start", f"heroic://launch/{app_name}"] if os.name == "nt" - else f"xdg-open heroic://launch/{app_name}" + else ["xdg-open", f"heroic://launch/{app_name}"] ) values["hidden"] = False values["source"] = "heroic_sideload" diff --git a/src/utils/run_command.py b/src/utils/run_command.py index 1400128..4614c2d 100644 --- a/src/utils/run_command.py +++ b/src/utils/run_command.py @@ -18,6 +18,7 @@ # SPDX-License-Identifier: GPL-3.0-or-later import os +import shlex import subprocess import sys @@ -25,15 +26,33 @@ from gi.repository import Gio def run_command(executable): - subprocess.Popen( - [f"flatpak-spawn --host {executable}"] - if os.getenv("FLATPAK_ID") == "hu.kramo.Cartridges" - else executable.split() - if os.name == "nt" - else [executable], - shell=True, - start_new_session=True, - creationflags=subprocess.CREATE_NEW_PROCESS_GROUP if os.name == "nt" else 0, - ) + use_shell = False + if not use_shell: + # The host environment is automatically passed through by Popen. + subprocess.Popen( + ["flatpak-spawn", "--host", *executable] # Flatpak + if os.getenv("FLATPAK_ID") == "hu.kramo.Cartridges" + else executable # Windows + if os.name == "nt" + else executable, # Linux/Others + shell=False, # If true, the extra arguments would incorrectly be given to the shell instead. + start_new_session=True, + creationflags=subprocess.CREATE_NEW_PROCESS_GROUP if os.name == "nt" else 0, + ) + else: + # When launching as a shell, we must pass 1 string with the exact command + # line exactly as we would type it in a shell (with escaped arguments). + subprocess.Popen( + shlex.join( + ["flatpak-spawn", "--host", *executable] # Flatpak + if os.getenv("FLATPAK_ID") == "hu.kramo.Cartridges" + else executable # Windows + if os.name == "nt" + else executable # Linux/Others + ), + shell=True, + start_new_session=True, + creationflags=subprocess.CREATE_NEW_PROCESS_GROUP if os.name == "nt" else 0, + ) if Gio.Settings.new("hu.kramo.Cartridges").get_boolean("exit-after-launch"): sys.exit() diff --git a/src/utils/steam_parser.py b/src/utils/steam_parser.py index bfcc1ef..f99a8e3 100644 --- a/src/utils/steam_parser.py +++ b/src/utils/steam_parser.py @@ -64,9 +64,9 @@ def get_game(task, datatypes, current_time, parent_widget, appmanifest, steam_di return values["executable"] = ( - f'start steam://rungameid/{values["appid"]}' + ["start", f'steam://rungameid/{values["appid"]}'] if os.name == "nt" - else f'xdg-open steam://rungameid/{values["appid"]}' + else ["xdg-open", f'steam://rungameid/{values["appid"]}'] ) values["hidden"] = False values["source"] = "steam"