Skip to content

Commit 25064f5

Browse files
asheshvasheshv
committed
fix: enforce data isolation and harden shared servers in server mode
pgAdmin 4 in server mode had no data isolation between users — any authenticated user could access other users' private servers, background processes, and debugger state by guessing object IDs. The shared server feature (21 audit issues) leaked owner credentials, saved passwords to wrong records, and corrupted owner data via SQLAlchemy session mutations. Centralized access control: - New server_access.py with get_server(), get_server_group(), get_user_server_query() replacing ~20 scattered unfiltered queries - connection_manager() raises ObjectGone (HTTP 410) in server mode when access is denied — fixes 155+ unguarded callers - UserScopedMixin.for_user() on 10 models replaces scattered user_id=current_user.id filters Shared server isolation (addresses all 21 audit issues): - get_shared_server_properties() expunges server from session, suppresses passexec_cmd/post_connection_sql, overrides all 6 SSL/passfile keys, strips owner-only keys - _is_non_owner() helper centralises 15+ inline checks - SENSITIVE_CONN_KEYS module-level constant (DRY) - Sanitizes connection_params on SharedServer creation - Tunnel/DB password save branches on ownership - change_password() checks SharedServer.password for non-owners - clear_saved/sshtunnel_password() use get_shared_server() - update_connection_parameter() routes to SharedServer copy - Only owner can trigger delete_shared_server (unshare) - SharedServer lookup uses (osid, user_id) not name (Issue 20) - Unique constraint on SharedServer(osid, user_id) with migration - ServerManager suppresses passexec/post_connection_sql for non-owners at creation time - Session restore includes shared servers - wal_replay()/check_pgpass() use get_server() - tunnel_port/tunnel_keep_alive copied from owner - delete_shared_server() accepts user_id parameter - get_shared_server() raises on None, catches IntegrityError - create_shared_server() exception handler uses rollback Tool/module hardening: - All tool endpoints use get_server() - Debugger function arguments scoped by user_id (migration) - Background processes use Process.for_user() - Workspace adhoc servers scoped by user_id Migration (schema version 49 -> 50): - Add user_id to debugger_function_arguments composite PK - Add indexes on server, sharedserver, servergroup - Add unique constraint on sharedserver(osid, user_id) - Fix ca00ec32581b to use raw SQL
1 parent 872d5ac commit 25064f5

21 files changed

Lines changed: 920 additions & 311 deletions

File tree

‎web/migrations/versions/add_user_id_to_debugger_func_args_.py‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
##########################################################################
2+
#
3+
# pgAdmin 4 - PostgreSQL Tools
4+
#
5+
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
6+
# This software is released under the PostgreSQL Licence
7+
#
8+
##########################################################################
9+
10+
"""Add user_id to debugger_function_arguments and indexes for data isolation
11+
12+
Revision ID: add_user_id_dbg_args
13+
Revises: add_tools_ai_perm
14+
Create Date: 2026-04-08
15+
16+
"""
17+
from alembic import op
18+
import sqlalchemy as sa
19+
20+
# revision identifiers, used by Alembic.
21+
revision = 'add_user_id_dbg_args'
22+
down_revision = 'add_tools_ai_perm'
23+
branch_labels = None
24+
depends_on = None
25+
26+
27+
def upgrade():
28+
conn = op.get_bind()
29+
dialect = conn.dialect.name
30+
31+
# --- DebuggerFunctionArguments: add user_id to composite PK ---
32+
if dialect == 'sqlite':
33+
# SQLite cannot ALTER composite PKs. Recreate the table.
34+
# Existing debugger argument data is ephemeral (cached function
35+
# args) so dropping is acceptable.
36+
op.execute(
37+
'DROP TABLE IF EXISTS debugger_function_arguments'
38+
)
39+
op.create_table(
40+
'debugger_function_arguments',
41+
sa.Column('user_id', sa.Integer(),
42+
sa.ForeignKey('user.id'), nullable=False),
43+
sa.Column('server_id', sa.Integer(), nullable=False),
44+
sa.Column('database_id', sa.Integer(), nullable=False),
45+
sa.Column('schema_id', sa.Integer(), nullable=False),
46+
sa.Column('function_id', sa.Integer(), nullable=False),
47+
sa.Column('arg_id', sa.Integer(), nullable=False),
48+
sa.Column('is_null', sa.Integer(), nullable=False),
49+
sa.Column('is_expression', sa.Integer(), nullable=False),
50+
sa.Column('use_default', sa.Integer(), nullable=False),
51+
sa.Column('value', sa.String(), nullable=True),
52+
sa.PrimaryKeyConstraint(
53+
'user_id', 'server_id', 'database_id',
54+
'schema_id', 'function_id', 'arg_id'
55+
),
56+
sa.CheckConstraint('is_null >= 0 AND is_null <= 1'),
57+
sa.CheckConstraint(
58+
'is_expression >= 0 AND is_expression <= 1'),
59+
sa.CheckConstraint(
60+
'use_default >= 0 AND use_default <= 1'),
61+
)
62+
else:
63+
# PostgreSQL: add column, backfill from server owner, recreate
64+
# PK using batch_alter_table for portability.
65+
op.add_column(
66+
'debugger_function_arguments',
67+
sa.Column('user_id', sa.Integer(),
68+
sa.ForeignKey('user.id'), nullable=True)
69+
)
70+
# Backfill: assign user_id from the server's owner
71+
op.execute(
72+
'UPDATE debugger_function_arguments '
73+
'SET user_id = s.user_id '
74+
'FROM server s '
75+
'WHERE debugger_function_arguments.server_id = s.id'
76+
)
77+
# Delete orphans (rows with no matching server)
78+
op.execute(
79+
'DELETE FROM debugger_function_arguments '
80+
'WHERE user_id IS NULL'
81+
)
82+
op.alter_column(
83+
'debugger_function_arguments', 'user_id', nullable=False
84+
)
85+
# Recreate PK with user_id using batch_alter_table
86+
with op.batch_alter_table(
87+
'debugger_function_arguments'
88+
) as batch:
89+
batch.drop_constraint(
90+
'debugger_function_arguments_pkey', type_='primary'
91+
)
92+
batch.create_primary_key(
93+
'debugger_function_arguments_pkey',
94+
['user_id', 'server_id', 'database_id',
95+
'schema_id', 'function_id', 'arg_id']
96+
)
97+
98+
# --- Indexes for data isolation query performance ---
99+
# Only create indexes on tables that exist (sharedserver may be
100+
# absent in older schemas that haven't run all prior migrations).
101+
inspector = sa.inspect(conn)
102+
index_stmts = [
103+
('server',
104+
'CREATE INDEX IF NOT EXISTS ix_server_user_id '
105+
'ON server (user_id)'),
106+
('server',
107+
'CREATE INDEX IF NOT EXISTS ix_server_servergroup_id '
108+
'ON server (servergroup_id)'),
109+
('sharedserver',
110+
'CREATE INDEX IF NOT EXISTS ix_sharedserver_user_id '
111+
'ON sharedserver (user_id)'),
112+
('sharedserver',
113+
'CREATE INDEX IF NOT EXISTS ix_sharedserver_osid '
114+
'ON sharedserver (osid)'),
115+
('servergroup',
116+
'CREATE INDEX IF NOT EXISTS ix_servergroup_user_id '
117+
'ON servergroup (user_id)'),
118+
]
119+
for table_name, stmt in index_stmts:
120+
if inspector.has_table(table_name):
121+
op.execute(stmt)
122+
123+
# --- Unique constraint on SharedServer(osid, user_id) ---
124+
# Prevents duplicate SharedServer records from TOCTOU race.
125+
# First remove duplicates (keep lowest id per osid+user_id).
126+
if inspector.has_table('sharedserver'):
127+
if dialect == 'sqlite':
128+
op.execute(
129+
'DELETE FROM sharedserver WHERE id NOT IN '
130+
'(SELECT MIN(id) FROM sharedserver '
131+
'GROUP BY osid, user_id)'
132+
)
133+
else:
134+
op.execute(
135+
'DELETE FROM sharedserver s1 USING '
136+
'sharedserver s2 WHERE s1.osid = s2.osid '
137+
'AND s1.user_id = s2.user_id '
138+
'AND s1.id > s2.id'
139+
)
140+
with op.batch_alter_table('sharedserver') as batch:
141+
batch.create_unique_constraint(
142+
'uq_sharedserver_osid_user',
143+
['osid', 'user_id']
144+
)
145+
146+
147+
def downgrade():
148+
# pgAdmin only upgrades, downgrade not implemented.
149+
pass

‎web/migrations/versions/ca00ec32581b_.py‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
1616
"""
1717
from alembic import op
18-
from sqlalchemy.orm.session import Session
19-
from pgadmin.model import DebuggerFunctionArguments
2018

2119
# revision identifiers, used by Alembic.
2220
revision = 'ca00ec32581b'
@@ -26,11 +24,10 @@
2624

2725

2826
def upgrade():
29-
session = Session(bind=op.get_bind())
30-
31-
debugger_records = session.query(DebuggerFunctionArguments).all()
32-
if debugger_records:
33-
session.delete(debugger_records)
27+
# Use raw SQL instead of importing the model class, because
28+
# model changes in later migrations (e.g. adding user_id) would
29+
# cause this migration to fail on fresh databases.
30+
op.execute('DELETE FROM debugger_function_arguments')
3431

3532

3633
def downgrade():

‎web/pgadmin/browser/server_groups/__init__.py‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
from pgadmin.model import db, ServerGroup, Server
2626
import config
2727
from pgadmin.utils.preferences import Preferences
28+
from pgadmin.utils.server_access import get_server_group, \
29+
get_server_groups_for_user
2830

2931

3032
def get_icon_css_class(group_id, group_user_id,
@@ -286,7 +288,7 @@ def update(self, gid):
286288
def properties(self, gid):
287289
"""Update the server-group properties"""
288290

289-
sg = ServerGroup.query.filter(ServerGroup.id == gid).first()
291+
sg = get_server_group(gid)
290292

291293
if sg is None:
292294
return make_json_response(
@@ -296,7 +298,8 @@ def properties(self, gid):
296298
)
297299
else:
298300
return ajax_response(
299-
response={'id': sg.id, 'name': sg.name, 'user_id': sg.user_id},
301+
response={'id': sg.id, 'name': sg.name,
302+
'user_id': sg.user_id},
300303
status=200
301304
)
302305

@@ -373,8 +376,9 @@ def dependents(self, gid):
373376
@staticmethod
374377
def get_all_server_groups():
375378
"""
376-
Returns the list of server groups to show in server mode and
377-
if there is any shared server in the group.
379+
Returns the list of server groups to show in server mode.
380+
Includes groups owned by the user and groups containing
381+
shared servers accessible to this user.
378382
:return: server groups
379383
"""
380384

@@ -383,17 +387,18 @@ def get_all_server_groups():
383387
pref = Preferences.module('browser')
384388
hide_shared_server = pref.preference('hide_shared_server').get()
385389

386-
server_groups = ServerGroup.query.all()
387-
groups = []
388-
for group in server_groups:
389-
if hide_shared_server and \
390-
ServerGroupModule.has_shared_server(group.id) and \
391-
group.user_id != current_user.id:
392-
continue
393-
if group.user_id == current_user.id or \
394-
ServerGroupModule.has_shared_server(group.id):
390+
server_groups = get_server_groups_for_user()
391+
392+
if hide_shared_server:
393+
groups = []
394+
for group in server_groups:
395+
if group.user_id != current_user.id and \
396+
ServerGroupModule.has_shared_server(group.id):
397+
continue
395398
groups.append(group)
396-
return groups
399+
return groups
400+
401+
return server_groups
397402

398403
@pga_login_required
399404
def nodes(self, gid=None):
@@ -421,7 +426,7 @@ def nodes(self, gid=None):
421426
)
422427
)
423428
else:
424-
group = ServerGroup.query.filter(ServerGroup.id == gid).first()
429+
group = get_server_group(gid)
425430

426431
if not group:
427432
return gone(

0 commit comments

Comments
 (0)