From 5b450107cd4cca4abe77eb5868653fd7697b05db Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:22:24 +0100 Subject: [PATCH 1/3] 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 d79ab1cbeb897715ea35962d0b05483a551b32ea Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:18:12 +0100 Subject: [PATCH 2/3] 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) From a8a72978133c129a933894f31bea06b48ba9a39b Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 15:23:22 +0100 Subject: [PATCH 3/3] fix: bind cached server managers to the pgAdmin user Issue #6090's own reproduction re-imports the same servers as a new pgAdmin user, so every connection field used by the staleness checks still matches. Nothing rotates the session id at login, so after a configuration database reset without a restart the new user would inherit the previous user's live connection and saved password. Record the fs_uniquifier of the user each ServerManager was built for, persist it with the serialized session state, and treat a manager or saved state belonging to a different user as stale. --- web/pgadmin/utils/driver/psycopg3/__init__.py | 43 ++++++++++++++++--- .../utils/driver/psycopg3/server_manager.py | 4 ++ .../psycopg3/tests/test_manager_is_stale.py | 38 ++++++++++++++++ 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index 0264c5ae08f..06edde02da4 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -81,9 +81,11 @@ def _restore_connections_from_session(self): session['__pgsql_server_managers'].copy() servers = get_user_server_query().filter( Server.is_adhoc == 0) + pga_user = self._current_pga_user() for server in servers: manager = managers[str(server.id)] = \ ServerManager(server) + manager.pga_user = pga_user # Suppress passexec for non-owners of shared # servers — it runs commands on the client # machine and must not inherit the owner's. @@ -92,7 +94,8 @@ def _restore_connections_from_session(self): manager.passexec = None if server.id in session_managers: saved = session_managers[server.id] - if self._saved_state_is_stale(saved, server): + if self._saved_state_is_stale( + saved, server, pga_user): # The persisted blob was serialized under # this numeric id by whatever Server row # held it before (e.g. the configuration @@ -100,7 +103,9 @@ def _restore_connections_from_session(self): # 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 + # password/connection state. The same goes + # for state saved by a different pgAdmin + # user on this browser session. Drop it and # let the manager start clean. manager.update_session() else: @@ -111,7 +116,17 @@ def _restore_connections_from_session(self): return {} @staticmethod - def _saved_state_is_stale(saved, server_data): + def _current_pga_user(): + """ + The fs_uniquifier of the logged-in pgAdmin user, which cached and + serialized ServerManager state is bound to. Unlike User.id it is + random, so it is not reused when the configuration database is + reset and a new user is created. + """ + return getattr(current_user, 'fs_uniquifier', None) + + @staticmethod + def _saved_state_is_stale(saved, server_data, pga_user=None): """ Same identity check as _manager_is_stale, applied to the serialized ServerManager state carried across worker @@ -122,8 +137,15 @@ def _saved_state_is_stale(saved, server_data): password/connections restored onto the new row on the very first request, before any manager exists to run _manager_is_stale against. + + The state is also stale if it was saved by a different pgAdmin + user: nothing rotates the session id at login, so a new user + logging in on the same browser session must not inherit the + previous user's password or connections, even for a server with + identical connection details. """ return ( + saved.get('pga_user') != pga_user or saved.get('host') != server_data.host or saved.get('port') != server_data.port or saved.get('db') != server_data.maintenance_db or @@ -133,7 +155,7 @@ def _saved_state_is_stale(saved, server_data): ) @staticmethod - def _manager_is_stale(manager, server_data): + def _manager_is_stale(manager, server_data, pga_user=None): """ A cached manager is normally kept in sync with edits to its Server row via explicit manager.update() calls from the @@ -142,9 +164,12 @@ def _manager_is_stale(manager, server_data): 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. + than trusting the id match alone. It is also stale if it was + built for a different pgAdmin user on the same browser session + (see _saved_state_is_stale). """ return ( + getattr(manager, 'pga_user', None) != pga_user or manager.host != server_data.host or manager.port != server_data.port or manager.db != server_data.maintenance_db or @@ -198,8 +223,10 @@ def connection_manager(self, sid=None): managers = self.managers[session.sid] if str(sid) in managers: manager = managers[str(sid)] + pga_user = self._current_pga_user() with connection_restore_lock: - if self._manager_is_stale(manager, server_data): + if self._manager_is_stale( + manager, server_data, pga_user): # The id has been reused by an unrelated Server # row (e.g. the configuration database was reset # or restored without restarting pgAdmin), so the @@ -207,8 +234,11 @@ def connection_manager(self, sid=None): # 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. + # The same applies to a manager built for another + # pgAdmin user on this browser session. manager.release() manager.update(server_data) + manager.pga_user = pga_user else: manager._restore_connections() manager.update_session() @@ -231,6 +261,7 @@ def connection_manager(self, sid=None): # server_data was already access-checked above; # it cannot be None at this point. manager = ServerManager(server_data) + manager.pga_user = self._current_pga_user() # Suppress passexec for non-owners of shared # servers — it runs commands on the client machine # and must not inherit the owner's. diff --git a/web/pgadmin/utils/driver/psycopg3/server_manager.py b/web/pgadmin/utils/driver/psycopg3/server_manager.py index 9a9320cdbd1..febcfca4feb 100644 --- a/web/pgadmin/utils/driver/psycopg3/server_manager.py +++ b/web/pgadmin/utils/driver/psycopg3/server_manager.py @@ -54,6 +54,9 @@ def __init__(self, server): self.tunnel_object = None self.tunnel_created = False self.display_connection_string = '' + # fs_uniquifier of the pgAdmin user this manager was built for; + # set by the driver, see Driver._current_pga_user. + self.pga_user = None self.update(server) @@ -165,6 +168,7 @@ def as_dict(self): res['user'] = self.user res['service'] = self.service res['tunnel_host'] = self.tunnel_host + res['pga_user'] = self.pga_user self._set_password(res) 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 a78f00908bd..77e74e07d0c 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 @@ -182,3 +182,41 @@ def runTest(self): self.assertTrue( Driver._saved_state_is_stale(saved, changed_server_data), "expected stale saved state when %s changes" % field) + + +class TestManagerIsStaleDetectsDifferentPgAdminUser( + _PureUnitTestSetupMixin, BaseTestGenerator): + """A manager built for one pgAdmin user must not be reused for + another user who logs in on the same browser session, even when + every connection field matches (issue #6090: the same servers + re-imported by a new user after the configuration database was + reset).""" + + scenarios = [('default', dict())] + + def runTest(self): + manager = make_manager(pga_user='old-user-uniquifier') + server_data = make_server_data() + + self.assertTrue(Driver._manager_is_stale( + manager, server_data, 'new-user-uniquifier')) + self.assertFalse(Driver._manager_is_stale( + manager, server_data, 'old-user-uniquifier')) + + +class TestSavedStateIsStaleDetectsDifferentPgAdminUser( + _PureUnitTestSetupMixin, BaseTestGenerator): + """Serialized state saved by one pgAdmin user must not be restored + for another user on the same browser session, even when every + connection field matches.""" + + scenarios = [('default', dict())] + + def runTest(self): + saved = make_saved_state(pga_user='old-user-uniquifier') + server_data = make_server_data() + + self.assertTrue(Driver._saved_state_is_stale( + saved, server_data, 'new-user-uniquifier')) + self.assertFalse(Driver._saved_state_is_stale( + saved, server_data, 'old-user-uniquifier'))