diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index cfcb324c4d8..03a76e5c2f2 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -30,6 +30,8 @@ from config import PG_DEFAULT_DRIVER from pgadmin.model import db, Server, ServerGroup, User, SharedServer from pgadmin.utils.driver import get_driver +from pgadmin.utils.driver.psycopg3 import shared_server_passexec +from pgadmin.utils.passexec import PasswordExec from pgadmin.utils.master_password import get_crypt_key from pgadmin.utils.exception import CryptKeyMissing, ConnectionLost from pgadmin.tools.schema_diff.node_registry import SchemaDiffRegistry @@ -957,10 +959,26 @@ def update(self, gid, sid): # which will affect the connections. if not conn.connected(): manager.update(server) - # Suppress passexec for non-owners so the manager - # never holds the owner's password-exec command. + # manager.update() rebuilds the manager from the server + # object alone, dropping any passexec. Recompute it so a + # non-owner's own SharedServer.passexec_cmd survives, while + # still never inheriting the owner's. if _is_non_owner(server): - manager.passexec = None + manager.passexec = shared_server_passexec(server) + elif 'passexec_cmd' in data or 'passexec_expiration' in data: + # manager.update() is skipped while connected, but a + # changed passexec_cmd/passexec_expiration is still + # committed above. manager.passexec is read lazily on + # the next reconnect (Connection.__attempt_execution_ + # reconnect), so refresh it now or a mid-session + # reconnect would keep using the pre-change command. + if _is_non_owner(server): + manager.passexec = shared_server_passexec(server) + else: + manager.passexec = PasswordExec( + server.passexec_cmd, server.host, server.port, + server.username, server.passexec_expiration) \ + if server.passexec_cmd else None return jsonify( node=self.blueprint.generate_browser_node( @@ -1007,10 +1025,14 @@ def _set_valid_attr_value(self, gid, data, config_param_map, server, raise CryptKeyMissing # Fields that non-owners must never set on their - # SharedServer — they enable command/SQL execution - # or are owner-level concepts not on SharedServer. + # SharedServer — owner-level concepts not on SharedServer. + # passexec_cmd/passexec_expiration are deliberately NOT + # here: a non-owner may set their own, which only ever runs + # in their own request context (see shared_server_passexec + # in pgadmin.utils.driver.psycopg3). Only inheriting the + # *owner's* passexec_cmd is blocked, and that is enforced in + # connection_manager(), not here. _owner_only_fields = frozenset({ - 'passexec_cmd', 'passexec_expiration', 'db_res', 'db_res_type', }) @@ -1628,12 +1650,14 @@ def connect(self, gid, sid, is_qt=False, server=None): # the API call is not made from SQL Editor or View/Edit Data tool if not manager.connection().connected() and not is_qt: manager.update(server) - # Re-suppress passexec after update() which rebuilds - # from the (overlaid) server object. Belt-and-suspenders: - # the overlay already defaults passexec to None, but this - # guards against direct DB edits. + # manager.update() rebuilds the manager from the (overlaid) + # server object alone, dropping any passexec. Recompute it + # so a non-owner's own SharedServer.passexec_cmd survives, + # while still never inheriting the owner's. server.id is + # preserved through the overlay, so this still resolves + # against the right SharedServer row. if _is_non_owner(server): - manager.passexec = None + manager.passexec = shared_server_passexec(server) conn = manager.connection() # Get enc key diff --git a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py index 49f6a2b93eb..5e2feb9894c 100644 --- a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py +++ b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py @@ -14,10 +14,13 @@ or HTTP infrastructure. """ +import inspect +import json from unittest.mock import MagicMock, patch, call from pgadmin.utils.route import BaseTestGenerator SRV_MODULE = 'pgadmin.browser.server_groups.servers' +DRIVER_MODULE = 'pgadmin.utils.driver.psycopg3' def _make_server(**overrides): @@ -511,8 +514,8 @@ class TestOwnerOnlyFieldsGuard(BaseTestGenerator): for non-owners.""" scenarios = [ - ('Non-owner cannot set passexec_cmd', - dict(test_method='test_nonowner_passexec_blocked')), + ('Non-owner can set their own passexec_cmd', + dict(test_method='test_nonowner_passexec_allowed')), ('Non-owner cannot set db_res or db_res_type', dict(test_method='test_nonowner_db_res_blocked')), ('Owner can set passexec_cmd', @@ -525,7 +528,7 @@ def runTest(self): @patch(SRV_MODULE + '.get_crypt_key', return_value=(True, b'key')) @patch(SRV_MODULE + '.current_user') - def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): + def test_nonowner_passexec_allowed(self, mock_cu, mock_ck): mock_cu.id = 200 # Non-owner from pgadmin.browser.server_groups.servers import \ ServerNode @@ -536,7 +539,7 @@ def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): node.delete_shared_server = MagicMock() data = { - 'passexec_cmd': '/evil/cmd', + 'passexec_cmd': '/usr/bin/my-own-cmd', 'post_connection_sql': 'SET role reader;', } config_map = { @@ -547,9 +550,10 @@ def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): node._set_valid_attr_value( 1, data, config_map, server, ss) - # passexec_cmd should be blocked for non-owners - self.assertIsNone(ss.passexec_cmd) - # post_connection_sql is allowed for non-owners + # Non-owners may set their own SharedServer.passexec_cmd -- + # only inheriting the *owner's* command is blocked, and + # that's enforced in connection_manager(), not here. + self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') self.assertEqual(ss.post_connection_sql, 'SET role reader;') @@ -697,3 +701,144 @@ def test_raises_on_none(self, mock_cu, mock_ss): self.assertIn( 'Failed to create shared server', str(ctx.exception)) + + +class TestUpdateRefreshesLivePassexec(BaseTestGenerator): + """Verify ServerNode.update() refreshes manager.passexec when + passexec_cmd/passexec_expiration changes on a *connected* server. + + manager.update(server) is skipped while connected (it would touch + live connection state), so without an explicit refresh here the + manager keeps serving the pre-change command to + Connection.__attempt_execution_reconnect() on the next automatic + reconnect (CodeRabbit finding on PR #10328). + """ + + scenarios = [ + ("Owner: passexec_cmd change while connected refreshes " + 'the manager', + dict(test_method='test_owner_connected_passexec_refreshed')), + ("Non-owner: passexec_cmd change while connected refreshes " + "the manager from their own SharedServer row", + dict(test_method='test_nonowner_connected_passexec_refreshed')), + ('Unrelated field change while connected leaves passexec ' + 'untouched', + dict(test_method='test_connected_unrelated_field_untouched')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + def _call_update( + self, server, data, connected, manager, sharedserver=None): + """Invoke the undecorated ServerNode.update(gid, sid) with + collaborators mocked. Returns (manager, result) -- callers + must assert on result too, so a test can't pass vacuously + by way of update() returning early (e.g. its "no parameters + were changed" guard) before ever reaching the code under + test.""" + from pgadmin.browser.server_groups.servers import ServerNode + + driver = MagicMock() + driver.connection_manager.return_value = manager + manager.connection.return_value.connected.return_value = connected + manager.user_info = {} + + node = ServerNode.__new__(ServerNode) + node.blueprint = MagicMock() + node.blueprint.generate_browser_node.return_value = {} + node.node_type = 'server' + node.delete_shared_server = MagicMock() + + with self.app.test_request_context( + '/', method='PUT', data=json.dumps(data), + content_type='application/json'), \ + patch(SRV_MODULE + '.get_server', return_value=server), \ + patch(SRV_MODULE + '.get_driver', return_value=driver), \ + patch(SRV_MODULE + '.get_crypt_key', + return_value=(True, b'key')), \ + patch(SRV_MODULE + '.db'), \ + patch(SRV_MODULE + '.jsonify', side_effect=lambda **kw: kw), \ + patch.object( + __import__(SRV_MODULE, fromlist=['ServerModule']). + ServerModule, 'get_shared_server', + return_value=sharedserver): + raw_update = inspect.unwrap(ServerNode.update) + result = raw_update(node, 1, 1) + + return manager, result + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_owner_connected_passexec_refreshed( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 100 # Owner + + server = _make_server() + manager = MagicMock() + data = {'passexec_cmd': '/usr/bin/new-owner-cmd', + 'passexec_expiration': 90} + + manager, result = self._call_update( + server, data, connected=True, manager=manager) + + self.assertIn('node', result) + self.assertIsNotNone(manager.passexec) + self.assertEqual(manager.passexec.cmd, '/usr/bin/new-owner-cmd') + self.assertEqual(manager.passexec.expiration_seconds, 90) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_nonowner_connected_passexec_refreshed( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 200 # Non-owner + mock_driver_cu.id = 200 + + server = _make_server() + ss = _make_shared_server() + # The driver's own SharedServer query must resolve to the + # same ss instance _set_valid_attr_value() just wrote to. + mock_driver_ss.query.filter_by.return_value.first.return_value = ss + + manager = MagicMock() + data = {'passexec_cmd': '/usr/bin/my-own-cmd', + 'passexec_expiration': 60} + + manager, result = self._call_update( + server, data, connected=True, manager=manager, + sharedserver=ss) + + self.assertIn('node', result) + self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') + self.assertEqual(ss.passexec_expiration, 60) + self.assertIsNotNone(manager.passexec) + self.assertEqual(manager.passexec.cmd, '/usr/bin/my-own-cmd') + self.assertEqual(manager.passexec.expiration_seconds, 60) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_connected_unrelated_field_untouched( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 100 # Owner + + server = _make_server() + manager = MagicMock() + sentinel = manager.passexec # whatever the manager already has + data = {'name': 'NewName'} + + manager, result = self._call_update( + server, data, connected=True, manager=manager) + + self.assertIn('node', result) + # No passexec field changed -- manager.passexec must be left + # exactly as it was, not recomputed or cleared. + self.assertIs(manager.passexec, sentinel) diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index 60d7f4c2d54..12626a021b2 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -23,10 +23,11 @@ from threading import Lock import config -from pgadmin.model import Server +from pgadmin.model import Server, SharedServer from pgadmin.utils.server_access import get_server, \ get_user_server_query from pgadmin.utils.exception import ObjectGone +from pgadmin.utils.passexec import PasswordExec from .keywords import scan_keyword from ..abstract import BaseDriver from .connection import Connection @@ -35,6 +36,31 @@ connection_restore_lock = Lock() +def shared_server_passexec(server): + """Return a PasswordExec built from a shared-server non-owner's + own SharedServer.passexec_cmd, or None. + + The owner's passexec_cmd is never honoured here for a non-owner + -- see #9830 / CVE-2026-7813, where any user able to own a shared + server could otherwise run an arbitrary command in every other + user's request context. A non-owner's own SharedServer.passexec_cmd + carries no such risk: it only ever runs in that same user's own + request context, exactly like an owned server's passexec_cmd would. + + Also used outside this module (browser.server_groups.servers) to + recompute passexec after a manager.update() call, which otherwise + rebuilds the manager from the server object alone and drops it. + """ + shared_server = SharedServer.query.filter_by( + user_id=current_user.id, osid=server.id).first() + if shared_server is None or not shared_server.passexec_cmd: + return None + return PasswordExec( + shared_server.passexec_cmd, server.host, server.port, + shared_server.username or server.username, + shared_server.passexec_expiration) + + class Driver(BaseDriver): """ class Driver(BaseDriver): @@ -84,12 +110,13 @@ def _restore_connections_from_session(self): for server in servers: manager = managers[str(server.id)] = \ ServerManager(server) - # Suppress passexec for non-owners of shared - # servers — it runs commands on the client - # machine and must not inherit the owner's. + # Never inherit the owner's passexec for + # non-owners of shared servers; only their own + # SharedServer.passexec_cmd, if any. if config.SERVER_MODE and server.shared and \ server.user_id != current_user.id: - manager.passexec = None + manager.passexec = \ + shared_server_passexec(server) if server.id in session_managers: manager._restore( session_managers[server.id]) @@ -152,12 +179,13 @@ def connection_manager(self, sid=None): # server_data was already access-checked above; # it cannot be None at this point. manager = ServerManager(server_data) - # Suppress passexec for non-owners of shared - # servers — it runs commands on the client machine - # and must not inherit the owner's. + # Never inherit the owner's passexec for non-owners + # of shared servers; only their own + # SharedServer.passexec_cmd, if any. if config.SERVER_MODE and server_data.shared and \ server_data.user_id != current_user.id: - manager.passexec = None + manager.passexec = \ + shared_server_passexec(server_data) managers[str(sid)] = manager return manager diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py b/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py new file mode 100644 index 00000000000..4abbaf65a8e --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py @@ -0,0 +1,116 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Unit tests for shared_server_passexec(), used by connection_manager(), +_restore_connections_from_session(), and by browser.server_groups.servers +to recompute a non-owner's passexec after manager.update() rebuilds the +manager from the server object alone (see #10249). +""" + +from unittest.mock import MagicMock, patch +from pgadmin.utils.route import BaseTestGenerator + +DRIVER_MODULE = 'pgadmin.utils.driver.psycopg3' + + +def _make_server(**overrides): + defaults = dict( + id=1, host='db.owner.com', port=5432, username='owner', + ) + defaults.update(overrides) + server = MagicMock() + for k, v in defaults.items(): + setattr(server, k, v) + return server + + +class TestSharedServerPassexec(BaseTestGenerator): + """Verify shared_server_passexec() resolves a non-owner's own + PasswordExec and never the owner's.""" + + scenarios = [ + ("Non-owner's own passexec_cmd is used", + dict(test_method='test_nonowner_own_cmd_used')), + ('No SharedServer row -> None', + dict(test_method='test_no_shared_server_row')), + ('SharedServer row with no passexec_cmd -> None', + dict(test_method='test_shared_server_no_cmd')), + ("Falls back to owner's server.username when " + 'SharedServer.username is blank', + dict(test_method='test_username_falls_back_to_server')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_nonowner_own_cmd_used(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock( + passexec_cmd='/usr/bin/my-own-cmd', + passexec_expiration=120, username='nonowner') + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + server = _make_server() + result = shared_server_passexec(server) + + mock_ss_cls.query.filter_by.assert_called_once_with( + user_id=200, osid=server.id) + self.assertIsNotNone(result) + self.assertEqual(result.cmd, '/usr/bin/my-own-cmd') + self.assertEqual(result.host, 'db.owner.com') + self.assertEqual(result.port, 5432) + self.assertEqual(result.username, 'nonowner') + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_no_shared_server_row(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = None + + self.assertIsNone(shared_server_passexec(_make_server())) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_shared_server_no_cmd(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock(passexec_cmd=None) + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + self.assertIsNone(shared_server_passexec(_make_server())) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_username_falls_back_to_server(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock( + passexec_cmd='/usr/bin/my-own-cmd', + passexec_expiration=None, username=None) + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + result = shared_server_passexec(_make_server(username='owner')) + + self.assertEqual(result.username, 'owner')