From ad8a44faa9e4f4ebf6e5672310087b96b29852f5 Mon Sep 17 00:00:00 2001 From: ElGregor Date: Fri, 23 Oct 2026 14:57:08 +0200 Subject: [PATCH] fix(icon): pass file paths as plain argv to the icon helper --- additional/python/get_icon_path_for_file.py | 10 +- .../python/tests/test_icon_path_helper.py | 91 +++++++++++++ lib/services/icon_loader.dart | 47 +++++-- security-fixplan-42-50.md | 9 +- test/icon_loader_test.dart | 124 ++++++++++++++++++ 5 files changed, 264 insertions(+), 17 deletions(-) create mode 100644 additional/python/tests/test_icon_path_helper.py create mode 100644 test/icon_loader_test.dart diff --git a/additional/python/get_icon_path_for_file.py b/additional/python/get_icon_path_for_file.py index 38e1a017..00a484c0 100644 --- a/additional/python/get_icon_path_for_file.py +++ b/additional/python/get_icon_path_for_file.py @@ -7,11 +7,15 @@ import jessentials +# The path arrives as one argv element and is used as data, unchanged. +# +# It used to arrive quoted ("--file='/path'") because the caller ran the +# helper through a shell, and this line stripped the quotes again. The +# caller no longer adds them and no longer uses a shell, so stripping them +# here would only corrupt a path that legitimately ends in a quote +# character. file_path = jessentials.get_value_from_arguments("file") - -file_path = file_path.removeprefix("'").removesuffix("'") - info = Gio.File.new_for_path(file_path).query_info('standard::icon', Gio.FileQueryInfoFlags.NONE, None) icon = info.get_icon() diff --git a/additional/python/tests/test_icon_path_helper.py b/additional/python/tests/test_icon_path_helper.py new file mode 100644 index 00000000..d304d966 --- /dev/null +++ b/additional/python/tests/test_icon_path_helper.py @@ -0,0 +1,91 @@ +"""WP-B2 (#45): the icon helper must hand the path through unchanged. + +The icon loader used to pass the path to the Python helper as +`--file=''` — quotes embedded in the value — and the helper stripped +them again with `removeprefix("'")` / `removesuffix("'")`. The two sides only +agreed because the one caller happened to add exactly the quotes the one +helper removed. + +The fix makes the contract explicit: the caller owns the encoding, the +helper passes the value through. That is what these tests pin. + +Why it matters, concretely: the helper used to *rewrite* the value it was +given. A caller that passes the plain path — which is what the icon loader +does now — would have had a path ending in a quote character truncated by +the old shim. The pair therefore has to change together, and the tests below +hold both halves to that. + +The GTK stack is mocked: the helper only needs `gi` to exist, and the +assertion is about which path reaches `Gio.File.new_for_path`. +""" + +import contextlib +import importlib.util +import io +import os +import sys +import unittest +from unittest import mock + +SCRIPTS = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +HELPER = os.path.join(SCRIPTS, "get_icon_path_for_file.py") +sys.path.insert(0, SCRIPTS) + + +def load_helper(argv): + """Loads the helper with a mocked GTK stack, returns the mocked Gio.""" + fake_gi = mock.MagicMock() + fake_repository = mock.MagicMock() + fake_gi.repository = fake_repository + sys.modules["gi"] = fake_gi + sys.modules["gi.repository"] = fake_repository + + old_argv = sys.argv + sys.argv = argv + try: + spec = importlib.util.spec_from_file_location( + "get_icon_path_for_file_under_test", HELPER) + module = importlib.util.module_from_spec(spec) + # The helper prints the resolved icon path; that is not what this test + # is about, and the mock's repr would drown the test output. + with contextlib.redirect_stdout(io.StringIO()): + spec.loader.exec_module(module) + finally: + sys.argv = old_argv + sys.modules.pop("gi", None) + sys.modules.pop("gi.repository", None) + return fake_repository.Gio + + +class IconPathHelper(unittest.TestCase): + def path_reaching_gio(self, file_value): + """Runs the helper with `--file=`, returns that path.""" + gio = load_helper(["get_icon_path_for_file.py", "--file=" + file_value]) + gio.File.new_for_path.assert_called_once() + return gio.File.new_for_path.call_args[0][0] + + def test_plain_path_arrives_unchanged(self): + path = "/home/user/Documents/report.txt" + self.assertEqual(self.path_reaching_gio(path), path) + + def test_path_with_spaces_arrives_unchanged(self): + path = "/home/user/My Documents/annual report.txt" + self.assertEqual(self.path_reaching_gio(path), path) + + def test_path_ending_in_a_quote_is_not_truncated(self): + # The old shim ran removesuffix("'") over the value, so a value that + # ends in a quote character lost it. The caller no longer adds + # quotes, which is exactly the case the old shim would have broken. + path = "/home/user/odd name'" + self.assertEqual(self.path_reaching_gio(path), path) + + def test_surrounding_quotes_are_kept(self): + # The helper must not strip quotes: the value is data. Whether it + # carries quotes is the caller's decision — the icon loader passes + # the plain path, and adds no quotes of its own. + value = "'/tmp/foo'" + self.assertEqual(self.path_reaching_gio(value), value) + + +if __name__ == "__main__": + unittest.main() diff --git a/lib/services/icon_loader.dart b/lib/services/icon_loader.dart index 11e08257..6c402dbc 100644 --- a/lib/services/icon_loader.dart +++ b/lib/services/icon_loader.dart @@ -45,27 +45,48 @@ class IconLoader { } String iconPath = "not found"; + // The path goes into the argv as data: no quoting around it, and no + // shell in between. The two belong together — the quotes existed only + // to survive the shell, and the Python helper had to strip them again. + // + // The shell was never a split risk (Dart quotes every argv element, so + // a path with a space stays one argument); it was an unnecessary + // intermediary, and `runInShell: false` is the pattern the rest of this + // file uses. What the quotes did cost: the helper had to rewrite the + // value it was given, which breaks a path that legitimately ends in a + // quote character. The helper now passes the value through unchanged, + // so both sides agree on the format. + /// Load icon for folder if (appCode == "folder") { - iconPath = await Linux.runCommandWithCustomArguments("/usr/bin/python3", [ - "${Linux.pythonScriptsFolder}/get_icon_path_for_file.py", - "--file='/tmp/'", - ]); + iconPath = await Linux.runCommandWithCustomArguments( + "/usr/bin/python3", + [ + "${Linux.pythonScriptsFolder}/get_icon_path_for_file.py", + "--file=/tmp/", + ], + runInShell: false); /// Load icon for app } else if (appCode != "" && filePath == "") { - iconPath = await Linux.runCommandWithCustomArguments("/usr/bin/python3", [ - "${Linux.pythonScriptsFolder}/get_icon_path_for_application.py", - "--icon=$appCode", - "--path-to-alternative-logos=${Linux.additionalFolder}logos/" - ]); + iconPath = await Linux.runCommandWithCustomArguments( + "/usr/bin/python3", + [ + "${Linux.pythonScriptsFolder}/get_icon_path_for_application.py", + "--icon=$appCode", + "--path-to-alternative-logos=${Linux.additionalFolder}logos/" + ], + runInShell: false); /// Load icon for file type } else if (filePath != "") { - iconPath = await Linux.runCommandWithCustomArguments("/usr/bin/python3", [ - "${Linux.pythonScriptsFolder}/get_icon_path_for_file.py", - "--file='$filePath'", - ]); + iconPath = await Linux.runCommandWithCustomArguments( + "/usr/bin/python3", + [ + "${Linux.pythonScriptsFolder}/get_icon_path_for_file.py", + "--file=$filePath", + ], + runInShell: false); } iconPath = iconPath.trim(); diff --git a/security-fixplan-42-50.md b/security-fixplan-42-50.md index 82c9e94c..9b72df83 100644 --- a/security-fixplan-42-50.md +++ b/security-fixplan-42-50.md @@ -26,7 +26,7 @@ |---|---|---|---| | #49 | confirmed (Triage+Skeptiker) | **Medium — top priority** | WP-S1 (V0.8.2.5) | | #44 | confirmed | Medium (Defense-in-Depth) | WP-S2 (V0.8.2.5) | -| #45 | contested — Mechanik-Prämisse entkräftet | Low pending RV-1 | WP-B2 (V0.8.2) | +| #45 | contested — Mechanik-Prämisse entkräftet | Low pending RV-1 | WP-B2 (V0.8.2) | ✅ erledigt — `4049874` | | #50 | contested — Mechanik-Prämisse entkräftet | Low pending RV-1 | WP-S3 (V0.8.2.5) | | #42 | **rejected** (Prämisse empirisch widerlegt) | Low (Härtung+Korrektheit) | WP-S3 (V0.8.2.5) | | #47 | confirmed | Low | WP-B1 (V0.8.2) | @@ -169,6 +169,13 @@ Commit fließt in den V0.8.2-Commit „fix(launcher): …" oder eigener Commit ### WP-B2 — #45: Icon-Loader-Quote-Shim entfernen (nur als gekoppeltes Paar) +**Status:** umgesetzt am 2026-10-23 — der Pfad geht als Daten-Element ins argv +(`--file=`, keine eingebetteten Quotes) und ohne Shell (`runInShell: false` an allen drei +Aufrufstellen in `lib/services/icon_loader.dart`); der Quote-Strip-Shim in +`additional/python/get_icon_path_for_file.py` ist entfernt, der Helfer reicht den Wert unverändert +durch. TDD: `test/icon_loader_test.dart` (7 Tests) und +`additional/python/tests/test_icon_path_helper.py` (4 Tests). Commit `4049874`. + **Befund:** Sink `icon_loader.dart:64-68` (`--file='$filePath'`, drei Aufrufe :52/:59/:64) über runInShell:true-Default; Quote-Strip-Shim in `additional/python/get_icon_path_for_file.py` arbeitet unter Dart-Escaping korrekt. Entkräftet als Injection (RV-1 entscheidet final) — diff --git a/test/icon_loader_test.dart b/test/icon_loader_test.dart new file mode 100644 index 00000000..0bfb585a --- /dev/null +++ b/test/icon_loader_test.dart @@ -0,0 +1,124 @@ +// WP-B2 (#45): the icon loader must hand the path to the helper as plain +// argv. +// +// The loader used to embed the path in a quoted string inside the argument +// (`--file=''`), and the Python helper stripped the quotes again. The +// two only agreed because the one caller added exactly the quotes the one +// helper removed. +// +// The fix makes the contract explicit: the caller owns the encoding, the +// helper passes the value through. Two things follow from it: +// +// 1. the shell is no longer part of the path. It was never a split risk — +// Dart quotes every argv element, so a path with a space stayed one +// argument — but it was an unnecessary intermediary, and the repo's +// established pattern is `runInShell: false` (six call sites in +// `linux.dart`). +// 2. the helper no longer rewrites the value. A caller that passes the +// plain path — which is what the loader does now — would have had a +// path ending in a quote character truncated by the old shim. The pair +// therefore has to change together. +// +// These tests pin both halves of that contract: the argv the loader builds, +// and the `runInShell` flag it uses. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:linux_assistant/helpers/command_helper.dart'; +import 'package:linux_assistant/services/icon_loader.dart'; + +/// Records what the icon loader would have started instead of starting it. +class _RecordingRunner implements CommandRunner { + final List> argumentLists = >[]; + final List runInShellFlags = []; + + @override + Future run( + String command, + List arguments, { + Map? environment, + bool asRoot = false, + bool hostOnFlatpak = true, + bool runInShell = false, + }) async { + argumentLists.add(List.of(arguments)); + runInShellFlags.add(runInShell); + // "not found" keeps the loader on its default-icon branch, so the test + // never touches the file system for an icon. + return const CommandResult(true, "not found", ""); + } +} + +void main() { + late _RecordingRunner runner; + + setUp(() { + runner = _RecordingRunner(); + CommandHelper.runner = runner; + // The loader is a singleton with a widget cache; a cached entry would + // skip the command entirely and make the test pass for the wrong reason. + IconLoader().cache.clear(); + }); + + tearDown(() { + CommandHelper.runner = CommandHelper.processRunner; + IconLoader().cache.clear(); + }); + + group('IconLoader hands the path over as plain argv', () { + test('a file path arrives as one argument, without quotes', () async { + const String path = "/home/user/My Documents/annual report.txt"; + + await IconLoader().getIconForAppOrFile(path); + + expect(runner.argumentLists, hasLength(1)); + final List arguments = runner.argumentLists.single; + expect(arguments, contains("--file=$path")); + // The path must not be split over several arguments. + expect( + arguments.where((String a) => a.startsWith("--file=")), hasLength(1)); + // And it must not carry the quotes the old shim stripped again. + expect(arguments, isNot(contains("--file='$path'"))); + }); + + test('a path ending in a quote character survives unchanged', () async { + const String path = "/home/user/odd name'.txt"; + + await IconLoader().getIconForAppOrFile(path); + + expect(runner.argumentLists.single, contains("--file=$path")); + }); + + test('the folder lookup passes its path without quotes', () async { + await IconLoader().getIconForAppOrFile("folder"); + + expect(runner.argumentLists.single, contains("--file=/tmp/")); + expect(runner.argumentLists.single, isNot(contains("--file='/tmp/'"))); + }); + + test('the application lookup passes its code without quotes', () async { + await IconLoader().getIconForAppOrFile("firefox"); + + expect(runner.argumentLists.single, contains("--icon=firefox")); + }); + }); + + group('IconLoader does not run the helper through a shell', () { + test('the file lookup runs without a shell', () async { + await IconLoader().getIconForAppOrFile("/home/user/report.txt"); + + expect(runner.runInShellFlags.single, isFalse); + }); + + test('the folder lookup runs without a shell', () async { + await IconLoader().getIconForAppOrFile("folder"); + + expect(runner.runInShellFlags.single, isFalse); + }); + + test('the application lookup runs without a shell', () async { + await IconLoader().getIconForAppOrFile("firefox"); + + expect(runner.runInShellFlags.single, isFalse); + }); + }); +}