From 7e9049a3ff86b91e8e7ef633107f2470de1d9ac1 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:22:24 +0100 Subject: [PATCH 1/2] fix: refresh stale ServerManager when a server id is reused connection_manager() cached ServerManager objects keyed only by the Flask session id and the numeric server id. If the configuration database is reset or restored without restarting pgAdmin, a freshly created server can reuse the id of a deleted one, and the cached manager - still pointing at the old server's host/port/credentials - gets returned as though it belonged to the new row, including reporting a stale "connected" status. Compare the manager against the current Server row's identifying fields (host, port, database, user, service, tunnel host) before reusing it, and rebuild via the existing update()/release() path when they no longer match. --- web/pgadmin/utils/driver/psycopg3/__init__.py | 39 ++++++- .../psycopg3/tests/test_manager_is_stale.py | 101 ++++++++++++++++++ 2 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index 60d7f4c2d54..cecacdbc849 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -98,6 +98,27 @@ def _restore_connections_from_session(self): return {} + @staticmethod + def _manager_is_stale(manager, server_data): + """ + A cached manager is normally kept in sync with edits to its + Server row via explicit manager.update() calls from the + server-edit endpoints. It can still go stale in place if the + row itself was swapped out from under it, e.g. a numeric + server id reused by an unrelated row after the configuration + database was reset or restored without restarting pgAdmin, so + compare against what actually identifies the target rather + than trusting the id match alone. + """ + return ( + manager.host != server_data.host or + manager.port != server_data.port or + manager.db != server_data.maintenance_db or + manager.user != server_data.username or + manager.service != server_data.service or + manager.tunnel_host != server_data.tunnel_host + ) + def connection_manager(self, sid=None): """ connection_manager(...) @@ -144,8 +165,22 @@ def connection_manager(self, sid=None): if str(sid) in managers: manager = managers[str(sid)] with connection_restore_lock: - manager._restore_connections() - manager.update_session() + if self._manager_is_stale(manager, server_data): + # The id has been reused by an unrelated Server + # row (e.g. the configuration database was reset + # or restored without restarting pgAdmin), so the + # cached manager still points at whatever server + # it was originally built from. Drop it rather + # than report a live connection to a server that, + # from this row's perspective, was never opened. + manager.release() + manager.update(server_data) + if config.SERVER_MODE and server_data.shared and \ + server_data.user_id != current_user.id: + manager.passexec = None + else: + manager._restore_connections() + manager.update_session() managers['pinged'] = datetime.datetime.now() if str(sid) not in managers: diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py b/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py new file mode 100644 index 00000000000..aef0253c6d4 --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py @@ -0,0 +1,101 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +""" +Unit tests for Driver._manager_is_stale. + +These are pure attribute-comparison tests, run as plain unittest +TestCases without needing a Postgres server connection. +""" + +import unittest +from types import SimpleNamespace + +from pgadmin.utils.route import BaseTestGenerator +from pgadmin.utils.driver.psycopg3 import Driver + + +def make_manager(**overrides): + fields = dict( + host='old-host', port=5432, db='postgres', user='old-user', + service=None, tunnel_host=None, + ) + fields.update(overrides) + return SimpleNamespace(**fields) + + +def make_server_data(**overrides): + fields = dict( + host='old-host', port=5432, maintenance_db='postgres', + username='old-user', service=None, tunnel_host=None, + ) + fields.update(overrides) + return SimpleNamespace(**fields) + + +class _PureUnitTestSetupMixin: + """setUp here calls unittest.TestCase.setUp directly, skipping + BaseTestGenerator.setUp's Postgres connection.""" + + def setUp(self): + unittest.TestCase.setUp(self) + + +class TestManagerIsStaleMatchesUnchanged( + _PureUnitTestSetupMixin, BaseTestGenerator): + """A manager whose identity fields still match the current Server + row is not stale, even if the row was legitimately edited via the + normal manager.update() flow elsewhere.""" + + scenarios = [('default', dict())] + + def runTest(self): + manager = make_manager() + server_data = make_server_data() + + self.assertFalse(Driver._manager_is_stale(manager, server_data)) + + +class TestManagerIsStaleDetectsReusedId( + _PureUnitTestSetupMixin, BaseTestGenerator): + """A manager built from a Server row that no longer matches the + current row for this id (e.g. the id was reused after the + configuration database was reset) must be treated as stale.""" + + scenarios = [('default', dict())] + + def runTest(self): + manager = make_manager(host='deleted-server.example.com') + server_data = make_server_data(host='new-server.example.com') + + self.assertTrue(Driver._manager_is_stale(manager, server_data)) + + +class TestManagerIsStaleChecksEachIdentityField( + _PureUnitTestSetupMixin, BaseTestGenerator): + """Any one of host/port/db/user/service/tunnel_host differing is + enough to mark the manager stale.""" + + scenarios = [('default', dict())] + + def runTest(self): + server_data = make_server_data() + + for field, value in ( + ('port', 5433), + ('maintenance_db', 'template1'), + ('username', 'new-user'), + ('service', 'myservice'), + ('tunnel_host', 'bastion.example.com'), + ): + manager = make_manager() + changed_server_data = make_server_data(**{field: value}) + self.assertTrue( + Driver._manager_is_stale(manager, changed_server_data), + "expected stale manager when %s changes" % field) From f6bbcb0cd185d38a8fffb62d8dcdc8d9d33af9f8 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:18:12 +0100 Subject: [PATCH 2/2] fix: validate identity on first restore and refresh access-control metadata CodeRabbit review on #10312 identified two real gaps left by the _manager_is_stale check: - _restore_connections_from_session() (the path taken on a worker's first request for a session, e.g. after a restart) restored serialized password/connection state from the Flask session purely by numeric server id, with no identity check at all - so a reused id would have the previous row's serialized state applied before any manager existed to run _manager_is_stale against. ServerManager.as_dict() now persists the same six identity fields alongside the serialized state, and _restore_connections_from_session() checks them (via the new _saved_state_is_stale) before restoring, discarding the blob instead of restoring it when they don't match. - The non-stale (fast) path in connection_manager() kept a cached manager's shared/passexec suppression as of whenever it was last built, since manager.update() - the only place shared/passexec get refreshed - is only called on the stale path. A reused id whose new row happens to share every identity field but differs in shared/ ownership would keep serving the previous owner's passexec to a non-owner. shared/passexec are now refreshed unconditionally from the current server row regardless of which path was taken. --- web/pgadmin/utils/driver/psycopg3/__init__.py | 56 ++++++++++-- .../utils/driver/psycopg3/server_manager.py | 12 +++ .../psycopg3/tests/test_manager_is_stale.py | 85 ++++++++++++++++++- 3 files changed, 146 insertions(+), 7 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index cecacdbc849..0264c5ae08f 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -91,13 +91,47 @@ def _restore_connections_from_session(self): server.user_id != current_user.id: manager.passexec = None if server.id in session_managers: - manager._restore( - session_managers[server.id]) - manager.update_session() + saved = session_managers[server.id] + if self._saved_state_is_stale(saved, server): + # The persisted blob was serialized under + # this numeric id by whatever Server row + # held it before (e.g. the configuration + # database was reset or restored without + # restarting pgAdmin), so it no longer + # describes this row. Restoring it would + # hand the new row the previous row's + # password/connection state. Drop it and + # let the manager start clean. + manager.update_session() + else: + manager._restore(saved) + manager.update_session() return managers return {} + @staticmethod + def _saved_state_is_stale(saved, server_data): + """ + Same identity check as _manager_is_stale, applied to the + serialized ServerManager state carried across worker + restarts/new sessions in the Flask session + ('__pgsql_server_managers'), before it is restored onto a + manager that was just built fresh from the current Server row. + Without this, a reused server id would have its old serialized + password/connections restored onto the new row on the very + first request, before any manager exists to run + _manager_is_stale against. + """ + return ( + saved.get('host') != server_data.host or + saved.get('port') != server_data.port or + saved.get('db') != server_data.maintenance_db or + saved.get('user') != server_data.username or + saved.get('service') != server_data.service or + saved.get('tunnel_host') != server_data.tunnel_host + ) + @staticmethod def _manager_is_stale(manager, server_data): """ @@ -175,12 +209,22 @@ def connection_manager(self, sid=None): # from this row's perspective, was never opened. manager.release() manager.update(server_data) - if config.SERVER_MODE and server_data.shared and \ - server_data.user_id != current_user.id: - manager.passexec = None else: manager._restore_connections() manager.update_session() + # Identity (host/port/db/user/service/tunnel) + # still matches, so the live connection is kept, + # but access-control-relevant metadata such as + # shared/ownership is not part of that identity + # check and manager.update() was skipped above - + # refresh it here too, otherwise a row whose + # sharing/ownership changed via the same reused-id + # path could keep serving the previous owner's + # passexec to a new, non-owning user. + manager.shared = server_data.shared + if config.SERVER_MODE and server_data.shared and \ + server_data.user_id != current_user.id: + manager.passexec = None managers['pinged'] = datetime.datetime.now() if str(sid) not in managers: diff --git a/web/pgadmin/utils/driver/psycopg3/server_manager.py b/web/pgadmin/utils/driver/psycopg3/server_manager.py index 00738355ee4..9a9320cdbd1 100644 --- a/web/pgadmin/utils/driver/psycopg3/server_manager.py +++ b/web/pgadmin/utils/driver/psycopg3/server_manager.py @@ -154,6 +154,18 @@ def as_dict(self): res['ver'] = self.ver res['sversion'] = self.sversion + # Persisted alongside the connection state so a later restore + # (e.g. after a worker restart) can tell whether this blob still + # belongs to the Server row for this id, or whether the id was + # reused by an unrelated row after the configuration database + # was reset/restored - see Driver._manager_is_stale. + res['host'] = self.host + res['port'] = self.port + res['db'] = self.db + res['user'] = self.user + res['service'] = self.service + res['tunnel_host'] = self.tunnel_host + self._set_password(res) if self.use_ssh_tunnel: diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py b/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py index aef0253c6d4..a78f00908bd 100644 --- a/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_manager_is_stale.py @@ -8,7 +8,7 @@ ########################################################################## """ -Unit tests for Driver._manager_is_stale. +Unit tests for Driver._manager_is_stale and Driver._saved_state_is_stale. These are pure attribute-comparison tests, run as plain unittest TestCases without needing a Postgres server connection. @@ -39,6 +39,18 @@ def make_server_data(**overrides): return SimpleNamespace(**fields) +def make_saved_state(**overrides): + """Mimics the identity fields ServerManager.as_dict() persists into + the Flask session ('__pgsql_server_managers') alongside the + serialized password/connections.""" + fields = dict( + host='old-host', port=5432, db='postgres', user='old-user', + service=None, tunnel_host=None, + ) + fields.update(overrides) + return fields + + class _PureUnitTestSetupMixin: """setUp here calls unittest.TestCase.setUp directly, skipping BaseTestGenerator.setUp's Postgres connection.""" @@ -99,3 +111,74 @@ def runTest(self): self.assertTrue( Driver._manager_is_stale(manager, changed_server_data), "expected stale manager when %s changes" % field) + + +class TestSavedStateIsStaleMatchesUnchanged( + _PureUnitTestSetupMixin, BaseTestGenerator): + """Serialized session state whose identity fields still match the + current Server row is safe to restore onto a freshly built + manager.""" + + scenarios = [('default', dict())] + + def runTest(self): + saved = make_saved_state() + server_data = make_server_data() + + self.assertFalse(Driver._saved_state_is_stale(saved, server_data)) + + +class TestSavedStateIsStaleDetectsReusedId( + _PureUnitTestSetupMixin, BaseTestGenerator): + """Serialized state left over from a deleted Server row (e.g. after + the configuration database was reset/restored without restarting + pgAdmin) must not be restored onto the row that reused its id.""" + + scenarios = [('default', dict())] + + def runTest(self): + saved = make_saved_state(host='deleted-server.example.com') + server_data = make_server_data(host='new-server.example.com') + + self.assertTrue(Driver._saved_state_is_stale(saved, server_data)) + + +class TestSavedStateIsStaleMissingFieldsAreStale( + _PureUnitTestSetupMixin, BaseTestGenerator): + """State serialized before identity fields were added to + ServerManager.as_dict() (i.e. a dict without host/port/etc. keys) + cannot be verified, so it must be treated as stale rather than + trusted blindly.""" + + scenarios = [('default', dict())] + + def runTest(self): + saved = {'sid': 1, 'ver': '18.0', 'sversion': 180000, + 'connections': {}} + server_data = make_server_data() + + self.assertTrue(Driver._saved_state_is_stale(saved, server_data)) + + +class TestSavedStateIsStaleChecksEachIdentityField( + _PureUnitTestSetupMixin, BaseTestGenerator): + """Any one of host/port/db/user/service/tunnel_host differing is + enough to discard the serialized state.""" + + scenarios = [('default', dict())] + + def runTest(self): + server_data = make_server_data() + + for field, value in ( + ('port', 5433), + ('maintenance_db', 'template1'), + ('username', 'new-user'), + ('service', 'myservice'), + ('tunnel_host', 'bastion.example.com'), + ): + saved = make_saved_state() + changed_server_data = make_server_data(**{field: value}) + self.assertTrue( + Driver._saved_state_is_stale(saved, changed_server_data), + "expected stale saved state when %s changes" % field)