From e4fe0fc8c4241b19fd1ba99ce45e66e4e8cca484 Mon Sep 17 00:00:00 2001 From: Loren Eteval Date: Wed, 9 Sep 2026 12:18:00 +0800 Subject: [PATCH] Improve best-effort system proxy handling Signed-off-by: Loren Eteval --- Furious/Controllers/AGENTS.md | 8 +- Furious/Controllers/ConnectionController.py | 6 ++ Furious/Frozenlib/AGENTS.md | 9 +- Furious/Frozenlib/SystemProxy.py | 35 ++++++- tests/test_controllers.py | 46 +++++++++ tests/test_frozenlib.py | 106 ++++++++++++++++++++ 6 files changed, 197 insertions(+), 13 deletions(-) diff --git a/Furious/Controllers/AGENTS.md b/Furious/Controllers/AGENTS.md index b59c527..a4fdf09 100644 --- a/Furious/Controllers/AGENTS.md +++ b/Furious/Controllers/AGENTS.md @@ -13,7 +13,7 @@ transitions. Services own execution resources; existing prompts are presentation exposed during `Connecting`; successful runtime commit precedes System Proxy setup and `Connected`. Failure resets the active profile. Disconnect/reconnect cancels the exact in-flight generation and ignores stale completion. - Preserve state and signal ordering, interaction gating, the exact selected `ServerProfile`, runtime snapshots, - reconnect preference, and rollback after validation, runtime, TUN, System Proxy, cancellation, or unexpected-exit + reconnect preference, and rollback after validation, runtime, TUN, System Proxy exceptions, cancellation, or unexpected-exit failure. Worker/native callbacks cross to the controller’s Qt thread before transition. - The active live profile is not the prepared document used by an already-started runtime. Resolve identity and generation before changing state or host effects, and preserve typed runtime failures; cancellation and supersession @@ -25,9 +25,9 @@ transitions. Services own execution resources; existing prompts are presentation - `SettingsController` is the shared policy path used by Home, Settings, tray, and platform integration. Startup registration persists only after host success; other preferences may apply immediately or on the next connection. Preserve each setting's actual application timing instead of imposing one transaction order on all preferences. -- System Proxy helpers currently log some host failures without raising. Controller exception-path tests prove - recovery when an error reaches the controller, not that every OS failure is propagated. Keep desired proxy mode - distinct from observed host state when evolving this boundary. +- System Proxy configuration is best effort. Its helper logs host failures; an explicit False return does not roll + back an otherwise usable committed runtime or prevent Connected. Preserve exception-path recovery separately. + Exercise actual helper results with mocked OS boundaries; Connected does not guarantee the OS proxy was applied. ## Verification and evolution diff --git a/Furious/Controllers/ConnectionController.py b/Furious/Controllers/ConnectionController.py index a475975..89abd05 100644 --- a/Furious/Controllers/ConnectionController.py +++ b/Furious/Controllers/ConnectionController.py @@ -76,6 +76,8 @@ def validateProxyServer(server) -> bool: if int(port) < 0 or int(port) > 65535: raise ValueError except Exception: + # Any non-exit exceptions + return False return True @@ -382,6 +384,8 @@ class ConnectionController(QtCore.QObject): ) try: + # System proxy setup is best effort; the helper logs host failures. + # Ignore even False so an otherwise usable core stays connected. SystemProxy.set(httpProxy, proxyServerBypass) except Exception as ex: # Any non-exit exceptions @@ -515,6 +519,8 @@ class ConnectionController(QtCore.QObject): try: self._actionQueue.get_nowait() except Exception: + # Any non-exit exceptions + pass Mixins.ConnectionAware.callDisconnectedCallback() diff --git a/Furious/Frozenlib/AGENTS.md b/Furious/Frozenlib/AGENTS.md index edd2337..c4e27e3 100644 --- a/Furious/Frozenlib/AGENTS.md +++ b/Furious/Frozenlib/AGENTS.md @@ -14,10 +14,11 @@ for unrelated application orchestration to accumulate in a broad helper namespac - Keep proxy, DNS, routing, TUN, startup registration, session callbacks, external commands, and platform detection here or behind a runtime boundary so tests can replace them completely. Windows, macOS, Linux, Flatpak, AppImage, and older platform paths are distinct capabilities; never generalize from the current host. -- Check each helper's real result contract. Startup registration and some routing helpers return Booleans; System - Proxy set/off currently log failures and return no success value. Script-mode startup registration intentionally - does nothing. Do not infer confirmed host state from absence of an exception or generalize one helper's semantics - to all. New mutation APIs should report actionable success/failure to the owning controller/service. +- Check each helper's real result contract. System Proxy set/off/pac return True for reported host success, False for + failure, and None when policy deliberately leaves host settings unchanged. Startup registration and some routing + helpers return Booleans; script-mode startup registration intentionally does nothing. Preserve these distinctions + at callers instead of treating absence of an exception as confirmed host state. Check every native command result, + including each enabled macOS network service, and bound host-command waits at this boundary. - Prefer argument vectors over shell strings. Each caller owns any responsiveness/cleanup timeout appropriate to its context; build-time commands and GUI-time host mutation do not share one universal timeout policy. - Windows proxy calls, Linux desktop settings/host bridging, and macOS network-service operations are distinct diff --git a/Furious/Frozenlib/SystemProxy.py b/Furious/Frozenlib/SystemProxy.py index b90c137..3abe827 100644 --- a/Furious/Frozenlib/SystemProxy.py +++ b/Furious/Frozenlib/SystemProxy.py @@ -33,6 +33,8 @@ __all__ = ['SystemProxy'] logger = logging.getLogger(__name__) +HOST_PROXY_COMMAND_TIMEOUT = 5.0 + def handleAppSystemProxyMode() -> bool: """Handle app system proxy mode.""" @@ -68,6 +70,7 @@ def linuxProxyConfig(proxy_args, arg0, arg1): stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=True, + timeout=HOST_PROXY_COMMAND_TIMEOUT, ) @@ -81,6 +84,7 @@ def darwinProxyConfig(operation, *args): stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, + timeout=HOST_PROXY_COMMAND_TIMEOUT, ) # Replace with command.stdout.decode('utf-8', 'replace')...? @@ -89,13 +93,20 @@ def darwinProxyConfig(operation, *args): return service[1:] for serviceName in getNetworkServices(): + if serviceName.startswith('*'): + continue + runExternalCommand( [ 'networksetup', f'-{operation}', serviceName, *args, - ] + ], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=True, + timeout=HOST_PROXY_COMMAND_TIMEOUT, ) @@ -166,14 +177,18 @@ class _SystemProxy: return - if _pac(): + success = bool(_pac()) + + if success: logger.info('set proxy PAC success') else: logger.error('set proxy PAC failed') + return success + @staticmethod def set(server, bypass): - """Set data managed by the system proxy.""" + """Return host success/failure, or None when policy deliberately ignores it.""" def _set(): """Return the set value used by the system proxy.""" @@ -226,11 +241,15 @@ class _SystemProxy: return - if _set(): + success = bool(_set()) + + if success: logger.info(f'set proxy server {server} success') else: logger.error(f'set proxy server {server} failed') + return success + @staticmethod def off(): """Disable the system proxy.""" @@ -274,11 +293,15 @@ class _SystemProxy: return - if _off(): + success = bool(_off()) + + if success: logger.info('turn off proxy success') else: logger.error('turn off proxy failed') + return success + def daemonOn_(self): """Return the daemon on value used by the system proxy.""" @@ -309,6 +332,8 @@ class _SystemProxy: try: thread.start() except Exception: + # Any non-exit exceptions + if self._daemonThread is thread: self._daemonThread = None diff --git a/tests/test_controllers.py b/tests/test_controllers.py index 0f5cd60..b6b2e29 100644 --- a/tests/test_controllers.py +++ b/tests/test_controllers.py @@ -35,6 +35,7 @@ from PySide6 import QtCore from tests.support import application, isolatedSettings, processQtEvents import unittest +import importlib from types import SimpleNamespace from unittest import mock @@ -498,6 +499,51 @@ class ConnectionControllerTest(unittest.TestCase): controller.deleteLater() + def testActualProxyHelperFailurePreservesCommittedStartup(self): + """Keep a usable core connected when best-effort system proxy setup fails.""" + proxyModule = importlib.import_module('Furious.Frozenlib.SystemProxy') + + for asynchronous in (False, True): + with self.subTest(asynchronous=asynchronous), isolatedSettings(): + core = ( + FixtureAsyncCoreManager() if asynchronous else FixtureCoreManager() + ) + controller = ConnectionController( + coreManager=core, updatesManager=FixtureUpdatesManager() + ) + self.addCleanup(controller.deleteLater) + + states = [] + errors = [] + + controller.stateChanged.connect(states.append) + controller.errorOccurred.connect(errors.append) + + with mock.patch.object( + proxyModule, 'PLATFORM', 'Linux' + ), mock.patch.object( + proxyModule, 'handleAppSystemProxyMode', return_value=True + ), mock.patch.object( + proxyModule, + 'linuxProxyConfig', + side_effect=OSError('host rejected operation'), + ), mock.patch.object( + controller, '_runPostConnectTasksOnce' + ) as postConnect: + self.assertTrue(controller.startConnection(self.profile)) + + if asynchronous: + core.operations[0][0].succeed() + + processQtEvents() + + self.assertEqual(controller.state, ConnectionState.Connected) + self.assertEqual(states.count(ConnectionState.Connected), 1) + self.assertIs(controller.activeProfile, self.profile) + self.assertEqual(core.stopCalls, 0) + self.assertEqual(errors, []) + postConnect.assert_called_once() + def testStartAndProxyExceptionsReturnToStableDisconnectedState(self): """Clean every partially acquired resource after injected failures.""" for core, proxySideEffect in ( diff --git a/tests/test_frozenlib.py b/tests/test_frozenlib.py index ab7e24e..2aca439 100644 --- a/tests/test_frozenlib.py +++ b/tests/test_frozenlib.py @@ -431,6 +431,112 @@ class FrozenlibUtilityTest(unittest.TestCase): class MockedPlatformHelperTest(unittest.TestCase): """Exercise platform helpers without touching host startup or networking.""" + def testProxyMutationsReturnHostFailureWithoutDisclosingSecrets(self): + """Preserve false results from each native boundary for callers to handle.""" + proxy = SystemProxyModule._SystemProxy() + native = types.SimpleNamespace( + set=mock.Mock(return_value=False), + off=mock.Mock(return_value=False), + pac=mock.Mock(return_value=False), + ) + + for platform in ('Windows', 'Linux', 'Darwin'): + with self.subTest(platform=platform), mock.patch.object( + SystemProxyModule, 'PLATFORM', platform + ), mock.patch.object( + SystemProxyModule, 'handleAppSystemProxyMode', return_value=True + ), mock.patch.dict( + sys.modules, {'sysproxy': native} + ), mock.patch.object( + SystemProxyModule, + 'linuxProxyConfig', + side_effect=OSError('private-token'), + ), mock.patch.object( + SystemProxyModule, + 'darwinProxyConfig', + side_effect=OSError('private-token'), + ), self.assertLogs( + SystemProxyModule.logger, level='ERROR' + ) as captured: + self.assertIs(proxy.set('127.0.0.1:10809', 'localhost'), False) + self.assertIs(proxy.off(), False) + self.assertIs(proxy.pac('https://invalid.test/private-token'), False) + + self.assertNotIn('private-token', '\n'.join(captured.output)) + + def testIgnoredProxyMutationsDoNotTouchHost(self): + """Do-not-change remains a deliberate no-op rather than a failed mutation.""" + with mock.patch.object( + SystemProxyModule, 'handleAppSystemProxyMode', return_value=False + ), mock.patch.object(SystemProxyModule, 'runExternalCommand') as command: + self.assertIsNone(SystemProxyModule.SystemProxy.set('127.0.0.1:10809', '')) + self.assertIsNone(SystemProxyModule.SystemProxy.off()) + self.assertIsNone(SystemProxyModule.SystemProxy.pac('https://invalid.test')) + + command.assert_not_called() + + def testDarwinProxyChecksEveryEnabledServiceCommand(self): + """Check enabled services and propagate even a later service rejection.""" + for fail in (False, True): + commands = [] + + def run(command, **kwargs): + commands.append(command) + + self.assertTrue(kwargs.get('check')) + self.assertEqual(kwargs.get('timeout'), 5.0) + + if '-listallnetworkservices' in command: + return subprocess.CompletedProcess( + command, + 0, + stdout=b'Header\nWi-Fi\n*Disabled service\nEthernet\n', + ) + + if fail and 'Ethernet' in command: + raise subprocess.CalledProcessError(1, command) + + return subprocess.CompletedProcess(command, 0) + + with self.subTest(fail=fail), mock.patch.object( + SystemProxyModule, 'runExternalCommand', side_effect=run + ): + if fail: + with self.assertRaises(subprocess.CalledProcessError): + SystemProxyModule.darwinProxyConfig( + 'setwebproxy', '127.0.0.1', '10809' + ) + else: + SystemProxyModule.darwinProxyConfig( + 'setwebproxy', '127.0.0.1', '10809' + ) + + self.assertEqual( + [command[2] for command in commands[1:]], ['Wi-Fi', 'Ethernet'] + ) + + def testLinuxProxyCommandTimeoutBecomesExplicitFailure(self): + """A hung host settings command has a deadline and cannot claim success.""" + with ( + mock.patch.object(SystemProxyModule, 'PLATFORM', 'Linux'), + mock.patch.object( + SystemProxyModule, 'handleAppSystemProxyMode', return_value=True + ), + mock.patch.object( + SystemProxyModule.SystemRuntime, 'flatpakID', return_value='' + ), + mock.patch.object( + SystemProxyModule, + 'runExternalCommand', + side_effect=subprocess.TimeoutExpired('gsettings', 5.0), + ) as command, + self.assertLogs(SystemProxyModule.logger, level='ERROR'), + ): + self.assertIs(SystemProxyModule.SystemProxy.off(), False) + + self.assertTrue(command.call_args.kwargs['check']) + self.assertEqual(command.call_args.kwargs['timeout'], 5.0) + def testPacDiagnosticsDoNotExposeConfiguredUrl(self): """Avoid logging credential-bearing PAC URLs on any outcome.""" proxy = SystemProxyModule._SystemProxy()