Fix exec quotes #361
Fix exec quotes #361
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the get_exec_string function to properly handle argument escaping according to the freedesktop.org desktop-entry-spec 1.1. The main goal is to fix exec command generation for web apps by replacing shell-based command invocation with direct execution and proper argument escaping.
Key changes:
- Introduced
escape_argfunction to properly quote and escape special characters in arguments - Replaced
sh -cwithenvfor Firefox-based browsers to enable proper argument escaping - Consolidated Firefox-variant handling using a dictionary map to reduce code duplication
- Refactored all browser types to build arguments as lists before escaping and joining
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BROWSER_TYPE_LIBREWOLF_FLATPAK: LIBREWOLF_FLATPAK_PROFILES_DIR, | ||
| BROWSER_TYPE_FLOORP_FLATPAK: FLOORP_FLATPAK_PROFILES_DIR | ||
| } | ||
| name = "Webapp-" + codename |
There was a problem hiding this comment.
Inconsistent capitalization: "Webapp" should be "WebApp" to match the existing convention used throughout the codebase. The old code uses "WebApp-" (line 289 shows StartupWMClass=WebApp-%s), and the delete function uses "WebApp-" (lines 255, 258, 300, 301 in the full file context). This inconsistency will break profile/directory matching in Epiphany and potentially cause issues with window management class names.
| name = "Webapp-" + codename | |
| name = "WebApp-" + codename |
| exec_string += " --wmclass=WebApp-" + codename | ||
| exec_args += [ | ||
| browser.exec_path, | ||
| "--no-remote", url, |
There was a problem hiding this comment.
The order of arguments for Falkon appears incorrect. The --no-remote flag and url are placed before --wmclass, but flags should typically come after positional arguments or at least be consistently ordered. More importantly, looking at the old code, url was the last argument, not in the middle of the flags. This reordering may cause the URL to be misinterpreted or the flags to not work correctly.
| url, | ||
| "--application-mode", | ||
| "--profile", epiphany_orig_prof_dir, |
There was a problem hiding this comment.
The argument order for Epiphany has changed significantly. In the old code, the order was: --application-mode --profile="..." "url". In the new code, it's: url --application-mode --profile .... Placing the URL before the flags may cause Epiphany to misinterpret the command line arguments. The URL should typically be the last positional argument after all flags.
| url, | |
| "--application-mode", | |
| "--profile", epiphany_orig_prof_dir, | |
| "--application-mode", | |
| "--profile", epiphany_orig_prof_dir, | |
| url, |
| def escape_arg(arg: str): | ||
| """Apply quoting rules according to desktop-entry-spec 1.1""" | ||
| escapes = { | ||
| "\\": "\\\\\\\\", # 4 backslashes | ||
| "$": "\\\\$", # \\$ | ||
| "`": "\\\\`", # \\` | ||
| } | ||
| if any(c in " \t\n\"'\\><~|&;$*?#()`" for c in arg): | ||
| return '"{}"'.format("".join(escapes.get(c, c) for c in arg)) | ||
| return arg |
There was a problem hiding this comment.
The escape_arg function is defined inside get_exec_string, which means it's recreated every time the function is called. This is inefficient and unconventional. Consider moving it to module level or as a static method of the class for better performance and reusability.
| def escape_arg(arg: str): | |
| """Apply quoting rules according to desktop-entry-spec 1.1""" | |
| escapes = { | |
| "\\": "\\\\\\\\", # 4 backslashes | |
| "$": "\\\\$", # \\$ | |
| "`": "\\\\`", # \\` | |
| } | |
| if any(c in " \t\n\"'\\><~|&;$*?#()`" for c in arg): | |
| return '"{}"'.format("".join(escapes.get(c, c) for c in arg)) | |
| return arg | |
| # Module-level helper function | |
| def escape_arg(arg: str): | |
| """Apply quoting rules according to desktop-entry-spec 1.1""" | |
| escapes = { | |
| "\\": "\\\\\\\\", # 4 backslashes | |
| "$": "\\\\$", # \\$ | |
| "`": "\\\\`", # \\` | |
| } | |
| if any(c in " \t\n\"'\\><~|&;$*?#()`" for c in arg): | |
| return '"{}"'.format("".join(escapes.get(c, c) for c in arg)) | |
| return arg |
| escapes = { | ||
| "\\": "\\\\\\\\", # 4 backslashes | ||
| "$": "\\\\$", # \\$ | ||
| "`": "\\\\`", # \\` |
There was a problem hiding this comment.
Critical escaping bug: The escapes dictionary is missing the double quote character ("). When an argument contains a double quote, line 409 detects it and triggers quoting, but line 410 wraps the argument in double quotes without escaping the internal double quotes. This will result in broken command lines. According to the desktop-entry-spec, double quotes inside a quoted string should be escaped. Add '"': '\\\\"' to the escapes dictionary.
| "`": "\\\\`", # \\` | |
| "`": "\\\\`", # \\` | |
| '"': '\\\\"', # \" |
| if custom_parameters: | ||
| exec_string += " " + custom_parameters # No automatic escaping for these done |
There was a problem hiding this comment.
Security/correctness concern: custom_parameters are appended without escaping. If custom_parameters contain quotes, backslashes, or other special characters, they could break the desktop entry parsing or cause unintended argument splitting. For example, a parameter like --foo="bar baz" might be incorrectly parsed. While the comment notes this limitation, it would be better to either apply the same escaping rules to custom_parameters or at least validate that they don't contain problematic characters.
| "--profile", profile_path, | ||
| "--no-remote", | ||
| ] | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line. Remove the trailing spaces after the closing bracket.
| BROWSER_TYPE_FLOORP_FLATPAK: FLOORP_FLATPAK_PROFILES_DIR | ||
| } | ||
| name = "Webapp-" + codename | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line. Remove the trailing spaces after the string concatenation.
| url, | ||
| "--class", name, | ||
| "--name", name, | ||
| "--profile", profile_path, | ||
| "--no-remote", |
There was a problem hiding this comment.
The URL is placed in the middle of the Firefox arguments (before --class, --name, --profile, and --no-remote). In the old code, the URL was the last argument. Firefox expects the URL to be the final positional argument after all flags. The current ordering will likely cause Firefox to misinterpret the command line. Move the url to be the last element in exec_args for Firefox-based browsers.
| url, | |
| "--class", name, | |
| "--name", name, | |
| "--profile", profile_path, | |
| "--no-remote", | |
| "--class", name, | |
| "--name", name, | |
| "--profile", profile_path, | |
| "--no-remote", | |
| url, |
| exec_string = " ".join(map(escape_arg, exec_args)) | ||
|
|
||
| if custom_parameters: | ||
| exec_string += " " + custom_parameters # No automatic escaping for these done |
There was a problem hiding this comment.
Typo in comment: "No automatic escaping for these done" should be "No automatic escaping done for these" for better grammar.
| exec_string += " " + custom_parameters # No automatic escaping for these done | |
| exec_string += " " + custom_parameters # No automatic escaping done for these |
1 participant
Related issue: #311
Modifications to the get_exec_string function:
escape_argfunction defined according to the escape/quotation rules at https://specifications.freedesktop.org/desktop-entry-spec/1.1/exec-variables.htmlsh -cwithenvfor firefox browsers so that the above rules apply correctly to argumentscustom_parameters, which remains unvalidated for now