Skip to content

Replace free-text server-mode password exec commands with an administrator allowlist - #10506

Closed
dpage wants to merge 20 commits into
pgadmin-org:masterfrom
dpage:feature/server-passexec-allowlist
Closed

dpage wants to merge 20 commits into
pgadmin-org:masterfrom
dpage:feature/server-passexec-allowlist

Conversation

@dpage

@dpage dpage commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

In server mode, PasswordExecCommand was free text run through a shell, so anyone able to edit a server could run arbitrary commands as the pgAdmin service account. This replaces it with a list of named commands that the administrator defines in config.py, which users can only pick from. Desktop mode is unchanged. Addresses #10249, and replaces #10328.

What changes in server mode

  • SERVER_PASSEXEC_COMMANDS maps names to argv lists. Commands run with shell=False, and the connection details reach them through the PGADMIN_PASSEXEC_HOST, _PORT, _USERNAME, _DATABASE, _PGADMIN_USER and _AUTH_SOURCE environment variables rather than placeholder substitution.
  • Commands get a reduced environment (PATH, HOME, locale and similar, plus anything named in the new SERVER_PASSEXEC_ENV_PASSTHROUGH), so other users' PGPASSWORD or container setup passwords never reach them.
  • The server dialog offers a drop-down of the configured names, hidden when none are configured. On a shared server, a non-owner can inherit the owner's command, choose none, or pick their own.
  • Failures report the command name only, never its arguments.
  • Existing free-text values that exactly match a configured command are converted to its name on first web start; everything else is cleared, and passexec_cmd is never run in server mode. ENABLE_SERVER_PASS_EXEC_CMD is deprecated and logs a warning.
  • servers.json import and export carry the command by name.

Upgrade impact: administrators who used ENABLE_SERVER_PASS_EXEC_CMD must list their commands in SERVER_PASSEXEC_COMMANDS, and commands relying on %HOSTNAME%-style placeholders or on environment variables outside the base list need adjusting. The upgrade notes in server_dialog.rst cover this.

The server dialog changed, so run-feature-tests-pg is the check to watch.

Summary by CodeRabbit

  • New Features
    • Server administrators can configure named password-retrieval commands. In server mode, users can select an available command or, on shared servers, inherit the owner’s selection or choose a per-user setting.
    • Server-mode commands run without a shell in a restricted environment. Password expiration is available with a configured command.
    • Server-mode import and export use configured command names; desktop mode continues to support free-text commands.
  • Bug Fixes
    • Invalid or unavailable server-mode commands are skipped with a warning that does not reveal command text.
  • Documentation
    • Added guidance on command configuration, shared-server behavior, environment variables, and upgrading existing settings.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pgadmin-org/pgadmin4/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ec685b8c-b852-49f4-9837-48c7abc5251a

📥 Commits

Reviewing files that changed from the base of the PR and between 687a743 and c30cfd8.


📒 Files selected for processing (5)
  • docs/en_US/server_dialog.rst
  • web/config.py
  • web/pgadmin/utils/__init__.py
  • web/pgadmin/utils/passexec.py
  • web/pgadmin/utils/tests/test_passexec_convert.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.



Walkthrough

Server mode now uses administrator-configured password commands by name. The change adds command-name storage, startup conversion of legacy values, shared-server inheritance and overrides, and mode-specific API, browser, import, and export handling. Commands run without a shell in a reduced environment; Desktop mode retains free-text command handling.

Changes

Named Server Password Execution

Layer / File(s) Summary
Persist command names and convert legacy values
web/config.py, web/migrations/versions/server_passexec_name_.py, web/pgadmin/model/__init__.py, web/pgadmin/__init__.py, web/pgadmin/utils/passexec.py, web/pgadmin/utils/tests/test_passexec_convert.py, web/pgadmin/utils/tests/test_passexec_startup.py, docs/en_US/server_dialog.rst
Adds named-command and environment passthrough settings and nullable name columns for servers and shared servers. Server startup validates settings and converts matching legacy command values to names while clearing legacy values. The documentation describes server-mode selection and conversion behavior.
Resolve and execute configured commands
web/pgadmin/utils/passexec.py, web/pgadmin/utils/driver/psycopg3/server_manager.py, web/pgadmin/utils/driver/psycopg3/__init__.py, web/pgadmin/utils/tests/test_passexec_server.py, web/pgadmin/utils/tests/test_passexec_resolve.py
The runtime resolves owner and shared-server selections, builds a named executor, and runs configured arguments without a shell in a reduced environment. Tests cover resolution, environment handling, and execution failures.
Manage selections through the API and browser
web/pgadmin/browser/__init__.py, web/pgadmin/browser/server_groups/servers/__init__.py, web/pgadmin/browser/server_groups/servers/static/js/server.js, web/pgadmin/browser/server_groups/servers/static/js/server.ui.js, web/pgadmin/browser/templates/browser/js/utils.js, web/pgadmin/browser/server_groups/servers/tests/test_server_passexec_api.py, web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py, web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py, web/regression/javascript/schema_ui_files/server.ui.spec.js
The API validates owner and per-user shared-server selections and returns mode-appropriate properties. The browser displays configured choices, inheritance, and expiration controls, and adjusts copied shared-server selections. Tests cover API, shared-server, and browser behavior.
Import and export mode-specific values
web/pgadmin/utils/__init__.py, web/pgadmin/utils/tests/test_passexec_import_export.py, docs/en_US/import_export_servers.rst
Server-mode import accepts configured command names and export omits free-text commands. Desktop mode continues to import and export free-text commands.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ServerManager
  participant build_passexec
  participant resolve_server_passexec
  participant ServerPasswordExec
  participant subprocess
  ServerManager->>build_passexec: Build password executor
  build_passexec->>resolve_server_passexec: Resolve the user's command
  resolve_server_passexec-->>build_passexec: Return command name and connection details
  build_passexec->>ServerPasswordExec: Create executor with configured argv
  ServerPasswordExec->>subprocess: Run argv without a shell
Loading

Merge Risk: ⚪ Minimal · up to c30cf

Server-mode password commands now come from an administrator-defined allowlist and run without a shell, in a reduced environment. No concrete merge-blocking risk was identified in the supplied context. Administrators must move any existing commands into the new setting before the first start after upgrading.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 27.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 107 functions across 21 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: replacing free-text server-mode password exec commands with an administrator-defined allowlist.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 27.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 107 functions across 21 files. (1 skipped: 1 unsupported.)


  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dpage

dpage commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Note

Automated review by an AI agent (Claude Fable 5.1), not by @dpage. Dave asked a Claude Code agent with a clean context to review this PR; the findings below are the agent's own and have not been edited by Dave.

This does what it says: in server mode the free-text command path is gone from every entry point I could find (create, update, import, startup conversion and the ServerManager itself), replaced by an administrator-defined argv run with shell=False and a filtered environment. I read the full files behind the diff, ran every new and touched Python test in a worktree (all pass, 100 tests across the eight modules, including the server-mode API cases which CI skips), and found nothing that should block merge. There are no existing human or CodeRabbit review threads.

Security points I checked and am satisfied with

  • Connection details reach the command only through PGADMIN_PASSEXEC_* environment variables (web/pgadmin/utils/passexec.py:257-264), so a hostile host, port or username set by a server editor cannot become part of the argv. The docs and config.py both state that only _PGADMIN_USER and _AUTH_SOURCE are trustworthy.
  • The child environment is rebuilt from an allowlist (passexec.py:139-154), so PGPASSWORD left in os.environ by export_password_env (server_manager.py:540-548) for another user's backup never reaches a command; test_passexec_server.py:212-236 covers this.
  • Failures raise an exception naming only the command (passexec.py:269-292), and _log_stderr does not log argv; the subprocess exceptions, which embed the argument list, are swallowed with from None.
  • normalise_passexec_name (passexec.py:50-67) is the single validation point for both the owner and non-owner paths, and prepare_passexec_data (servers/__init__.py:70-100) refuses any non-empty passexec_cmd in server mode with a 400, so a client cannot smuggle free text in past the hidden field.
  • The legacy conversion runs after the schema upgrade inside the app context (pgadmin/__init__.py:545-547), is idempotent (a second start finds no passexec_cmd rows), and is skipped in CLI mode so setup.py --sqlite-path on a desktop database does not wipe anything.
  • Letting a non-owner of a shared server inherit or choose a configured command is a deliberate widening from the old blanket suppression, and is acceptable because the command is administrator-defined and receives the non-owner's pgAdmin identity; it is documented in server_dialog.rst.

Non-blocking

  1. docs/en_US/server_dialog.rst:265-276 and web/config.py:1064-1083: the upgrade note explains the matching rule but not the ordering, and the conversion is irreversible. An administrator who upgrades, starts pgAdmin, and only then adds SERVER_PASSEXEC_COMMANDS will have every existing command cleared on that first start, and because convert_legacy_server_passexec (passexec.py:118-122) deliberately logs no command text, nothing can be recovered. Suggest one sentence in both places saying that SERVER_PASSEXEC_COMMANDS must be configured before the first start after upgrading, and optionally an extra startup warning when the dictionary is empty but rows with passexec_cmd exist.

  2. web/pgadmin/browser/server_groups/servers/tests/test_server_passexec_api.py:181,314: the two end-to-end server-mode cases skip unless the run is in server mode, and both run-python-tests-pg.yml jobs write SERVER_MODE = False into config_local.py, so CI never exercises them. They pass locally in a server-mode run, so this is a coverage observation rather than a defect; worth knowing when judging CI green on later changes to this code.

  3. web/pgadmin/utils/__init__.py:555-569: a servers.json exported from server mode carries the command name under PasswordExecCommand. Imported into a desktop install it becomes a free-text shell command of that name. Harmless (it is the user's own machine and the name is administrator-chosen) and the import docs already describe the two meanings, so no change needed; mentioned only so it is a known trade-off.

Note on verification: the branch is 25 commits behind upstream/master, but the only overlap is in server_manager.py and connection.py and SCHEMA_VERSION is still 52 on master with no competing migration, so the rebase should be clean. I could not run the Jest spec locally because the primary checkout's node_modules have moved past this branch's package.json; the PR's own run-javascript-tests, run-feature-tests-pg and style checks all pass in CI.

dpage added 20 commits October 12, 2026 20:03
…T in the reduced environment test on Windows.
…uns, since patching SERVER_MODE on a desktop app breaks its login checks.
…EC_COMMANDS is configured, and document that it must be set before the first start after upgrading.
@dpage
dpage force-pushed the feature/server-passexec-allowlist branch from 687a743 to c30cfd8 Compare October 12, 2026 19:11
@dpage

dpage commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Note

Changes made by an AI agent (Claude Fable 5.1), not by @dpage. Dave asked a Claude Code agent to fold in the non-blocking points from the automated review above.

Point 1: convert_legacy_server_passexec now runs only once at least one entry in SERVER_PASSEXEC_COMMANDS is valid; with an empty allowlist it leaves every legacy passexec_cmd row untouched (nothing in server mode reads, runs, returns or exports that column) and logs a warning per table giving the number of rows still waiting, without any command text. server_dialog.rst and the config.py comment now say the setting must be configured before the first start after upgrading and describe the postponement. test_passexec_convert.py gained a scenario for the postponed case. Points 2 and 3 left as they were.

Rebased onto current upstream/master (no competing migration; SCHEMA_VERSION is still 52 there, 53 here). Ran the passexec unit tests, the servers API, data-isolation and shared-server tests in desktop mode, and the API, conversion and startup tests again in server mode: all pass. pycodestyle clean on the changed files.

dpage added a commit that referenced this pull request Oct 12, 2026
…rator allowlist. (#10506)

In server mode, users choose a password exec command by name from
SERVER_PASSEXEC_COMMANDS rather than entering a command line. Existing
commands are converted on upgrade once the allowlist is configured; if
it is empty the conversion is postponed and a warning is logged.
@dpage

dpage commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Merged to master as c41b606.

@dpage dpage closed this Oct 12, 2026
dpage added a commit that referenced this pull request Oct 13, 2026
The #10330 branch added and then dropped a schema version bump; replayed
on top of #10506, which also took version 53, the squash set it back to
52 even though the server_passexec_name migration remains.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant