docker: read PGADMIN_CONFIG_CONFIG_DATABASE_URI safely via os.environ (fixes #9984)

[asheshv]

Closes #9984.

Problem

The container entrypoint constructed a Python -c argument by shell-substituting ${PGADMIN_CONFIG_CONFIG_DATABASE_URI} inside a double-quoted Python string:

val = check_external_config_db("${PGADMIN_CONFIG_CONFIG_DATABASE_URI}")

That collided with the long-standing config_distro.py convention, which requires the env var's value to be a Python literal (so the entrypoint's CONFIG_DATABASE_URI = $val write produces valid Python). Users therefore set the env var to 'postgresql+psycopg://...'. The entrypoint then re-wrapped that literal in another set of quotes, producing:

check_external_config_db("'postgresql+psycopg://...'")

— a string with literal single quotes inside. SQLAlchemy can't parse it:

sqlalchemy.exc.ArgumentError: Could not parse SQLAlchemy URL from given URL string

The Python crash made $(...) capture an empty string, so the downstream check [ "${external_config_db_exists}" = "False" ] also failed — silently skipping the entire first-launch user-setup block. That is the second symptom on the issue: PGADMIN_DEFAULT_EMAIL and PGADMIN_DEFAULT_PASSWORD getting ignored.

Regression history

Bisected to commit 1fe840fca ("Allow to pass PGADMIN_CONFIG_CONFIG_DATABASE_URI from docker secrets", #5869, Oct 2024), which incidentally added "…" shell quoting around the expansion in a PR whose actual scope was docker-secrets support (file_env helper). The pre-regression form (ed211a2bb, #7811, Sep 2024) used a bare expansion and worked correctly with the convention.

Both env-var forms have been broken in shipped images since Oct 2024:

Env var form	Symptom
PGADMIN_CONFIG_CONFIG_DATABASE_URI="'postgresql+psycopg://…'" (with quotes, documented)	SQLAlchemy ArgumentError + silent PGADMIN_DEFAULT_EMAIL skip
PGADMIN_CONFIG_CONFIG_DATABASE_URI=postgresql+psycopg://… (without quotes, naive)	config_distro.py SyntaxError on startup → container exits

Fix

Rather than restore the bare-expansion form (a 7-year Chesterton's fence that the next shellcheck-style cleanup would reintroduce), remove the shell from the quoting question altogether:

• Read the env var inside Python via os.environ — the shell no longer participates in Python-literal handling.
• Use ast.literal_eval to unwrap the legacy 'url' / "url" form when present; on ValueError/SyntaxError, fall through to the raw value (so users who set the env var without quotes also work).
• Default external_config_db_exists stays "False" on any Python failure (only overwritten when the Python call produces non-empty stdout) — fixes the secondary PGADMIN_DEFAULT_EMAIL-ignored regression.

Verification (manual)

Form	Result
'postgresql+psycopg://…' (legacy single-quoted)	ast.literal_eval unwraps → clean URI ✓
"postgresql+psycopg://…" (double-quoted)	ast.literal_eval unwraps → clean URI ✓
postgresql+psycopg://… (raw, no quotes)	ast.literal_eval raises → fall through to raw ✓
End-to-end against live Postgres 17, no server table	returns False (first-launch path runs) ✓
End-to-end against live Postgres 17, with server table seeded	returns True (skip first-launch as designed for #7811) ✓
Malformed env var	Python fails → external_config_db_exists stays "False" → first-launch path runs (no silent skip) ✓

Notes

• ast.literal_eval safety: parses Python literals only; cannot execute arbitrary code. Available since Python 2.6 (2008); pgAdmin's Docker image uses python:3-alpine (currently 3.12+), so no version-availability concern.
• No new dependencies, no docs changes, no behavior change for users with already-working configs (no working configs exist on master — both forms fail).
• 1 file changed, +20/-1.

Test plan

• Bash syntax check (bash -n pkg/docker/entrypoint.sh) clean.
• All three env-var forms validated against a live Postgres 17 (Tests 1-5 above).
• Container test (recommended for maintainers): rebuild the docker image off this branch, set PGADMIN_CONFIG_CONFIG_DATABASE_URI="'postgresql+psycopg://…'" + PGADMIN_DEFAULT_EMAIL + PGADMIN_DEFAULT_PASSWORD, run a fresh container → verify (1) no ArgumentError in container logs, (2) default admin user is created against the external DB, (3) restart with server table populated → first-launch is skipped as in Servers are re-imported at every container startup #7811.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of Docker configuration database initialization by fixing potential issues with special characters in database connection strings.

[coderabbitai]

Walkthrough

The Docker entrypoint's external configuration database existence check is refactored to pass the database URI via environment variable instead of as an inline shell argument. The Python snippet now reads the URI from os.environ, applies ast.literal_eval for legacy quoted forms, and falls back gracefully on parse errors, eliminating shell quoting issues that caused SQLAlchemy parsing failures.

Changes

Database Configuration Check

Layer / File(s)	Summary
External config DB existence check via environment variable pkg/docker/entrypoint.sh	The Python helper invocation for external_config_db_exists (lines 198–219) replaces a single-line command with a multi-line snippet that reads PGADMIN_CONFIG_CONFIG_DATABASE_URI from os.environ, uses ast.literal_eval to unwrap legacy single-quoted forms, preserves the "False" default on ValueError/SyntaxError, and suppresses stderr.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• pgadmin-org/pgadmin4#9833: Both PRs modify pkg/docker/entrypoint.sh's Python helper invocation flow—main PR rewrites the external config DB existence check and its argument handling (via os.environ[...]), while the retrieved PR wraps Python helper checks under SU_EXEC for root/non-root execution—so they overlap in the same helper-check logic.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing the unsafe shell-based reading of PGADMIN_CONFIG_CONFIG_DATABASE_URI by switching to os.environ in Python, and references the fixed issue #9984.
Linked Issues check	✅ Passed	The PR correctly addresses issue #9984 by moving env-var reading into Python via os.environ and using ast.literal_eval to safely unwrap legacy quoted literals, resolving the double-quoting bug that caused SQLAlchemy ArgumentError.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the PGADMIN_CONFIG_CONFIG_DATABASE_URI handling in the entrypoint script; no unrelated modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/docker/entrypoint.sh (1)

197-220: Significant improvement in correctness and safety.

The refactored approach successfully addresses the double-quoting regression from commit 1fe840f and implements a robust safe-fallback strategy. Key improvements:

1. Eliminates shell-quoting issues: Reading via os.environ prevents shell expansion and double-quoting of Python literals.
2. Backward compatible: ast.literal_eval unwraps legacy 'url' forms while supporting raw values via the fallback.
3. Safe default on failure: Initializing to "False" and conditionally updating only on success ensures first-launch setup is never silently skipped (#9984).

Trade-off: The 2>/dev/null stderr suppression prioritizes safety over observability—errors are silently absorbed to preserve the safe default. This is intentional per the PR objectives, but may complicate debugging if the check fails unexpectedly in production. Consider whether operational monitoring or logging could detect repeated check failures without compromising the safety guarantee.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/docker/entrypoint.sh` around lines 197 - 220, The current stderr
suppression (the "2>/dev/null" on the inline Python invocation) hides failures
of check_external_config_db and makes debugging harder; change the call that
sets result to capture stderr instead of discarding it (e.g., redirect stderr
into a variable or pipe it to the system logger with "logger -t
pgadmin-external-config") while keeping the existing behavior of initializing
external_config_db_exists="False" and only updating it if result is non-empty;
update the invocation that uses SU_EXEC and the inline python (the command that
currently ends with 2>/dev/null) to preserve the safe default but emit the
captured stderr to logs (or logger) when the check returns empty so operators
can observe repeated failures without changing the success/failure logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 197-220: The current stderr suppression (the "2>/dev/null" on the
inline Python invocation) hides failures of check_external_config_db and makes
debugging harder; change the call that sets result to capture stderr instead of
discarding it (e.g., redirect stderr into a variable or pipe it to the system
logger with "logger -t pgadmin-external-config") while keeping the existing
behavior of initializing external_config_db_exists="False" and only updating it
if result is non-empty; update the invocation that uses SU_EXEC and the inline
python (the command that currently ends with 2>/dev/null) to preserve the safe
default but emit the captured stderr to logs (or logger) when the check returns
empty so operators can observe repeated failures without changing the
success/failure logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3fccd3bb-7292-492f-b695-05c8199b4536

📥 Commits

Reviewing files that changed from the base of the PR and between 1487059 and 3b4da7e.

📒 Files selected for processing (1)

• pkg/docker/entrypoint.sh