Fixed a variable quoting issue in the Docker entrypoint script #9511
Conversation
WalkthroughAdds an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @pkg/docker/entrypoint.sh:
- Around line 89-92: ensure_quoted currently wraps unquoted URIs in single
quotes but fails to escape internal single quotes, breaking the Python
invocation in external_config_db_exists; update ensure_quoted to produce a
shell-safe quoted string by escaping any single quotes inside the value (replace
each ' with '"'"' or the equivalent '\'' sequence) or switch to a robust quoting
approach (e.g., wrap in double quotes and escape embedded double quotes and
backslashes) so that the substituted ${PGADMIN_CONFIG_CONFIG_DATABASE_URI} in
the python -c command remains valid; ensure the changed behavior is used where
external_config_db_exists is computed and in any other places that call
ensure_quoted.
- Around line 70-84: The ensure_quoted function fails if the unquoted value
contains single quotes and uses echo to return the value; update ensure_quoted
to escape any single quotes in the input by replacing each ' with '\'' (so the
final wrapped form is safe), then wrap the result in single quotes only if it
wasn’t already quoted, and return the value using printf '%s' instead of echo;
keep references to the ensure_quoted function and the existing regex checks for
already-quoted values when making this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
- GitHub Check: run-python-tests-pg (windows-latest, 18)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
for backwards compatibility, quotes are only added if the variable doesn't already contain single or double quotes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T20:14:11.407Z
Learning: In the pgadmin4 Dockerfile, the sudoers entry for `pgadminr` (line containing `echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix`) is intentional and not a typo. The `pgadminr` user is dynamically created by the docker entrypoint script when the container runs with a non-default UID (not 5050) and the user can write to /etc/passwd. Both `pgadmin` and `pgadminr` sudoers entries are needed to support different container execution scenarios.
📚 Learning: 2025-11-20T20:14:11.407Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T20:14:11.407Z
Learning: In the pgadmin4 Dockerfile, the sudoers entry for `pgadminr` (line containing `echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix`) is intentional and not a typo. The `pgadminr` user is dynamically created by the docker entrypoint script when the container runs with a non-default UID (not 5050) and the user can write to /etc/passwd. Both `pgadmin` and `pgadminr` sudoers entries are needed to support different container execution scenarios.
Applied to files:
pkg/docker/entrypoint.sh
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (15)
| # Ensures the variable starts and ends in quotes after trimming | ||
| ensure_quoted() { | ||
| local var="$1" | ||
|
|
||
| # Trim whitespaces | ||
| var="${var#"${var%%[![:space:]]*}"}" | ||
| var="${var%"${var##*[![:space:]]}"}" | ||
|
|
||
| # Check if the variable is already double or single quoted | ||
| if [[ ! $var =~ ^\"(.*)\"$ && ! $var =~ ^\'(.*)\'$ ]]; then | ||
| # Use docstrings in case the value contains quote characters inside | ||
| var="'''${var}'''" | ||
| fi | ||
|
|
||
| echo "${var}" | ||
| } |
There was a problem hiding this comment.
Critical: Shell metacharacters in URIs will be expanded despite Python triple-quotes.
The triple-quote approach (line 81) attempts to address single quotes in URIs using Python's triple-quoted string syntax. While this handles single quotes better than regular single quotes, it introduces critical security and correctness issues:
-
Shell variable expansion: When the sanitized value is substituted inside double quotes on line 93 (
"...${PGADMIN_CONFIG_CONFIG_DATABASE_URI}..."), the shell will still expand dollar signs, backticks, backslashes, and exclamation marks—even when they appear inside the triple quotes, because double quotes preserve these special meanings. For example, a URI likepostgresql://user:pa$word@host/dbbecomes'''postgresql://user:pa$word@host/db''', but when substituted inside double quotes,$wordis interpreted as a shell variable. -
Triple-quote breakage: If the URI contains
''', Python parsing will fail. -
Already-quoted check flaw: The regex on line 79 checks for quote characters in the value, but environment variables don't preserve shell quotes—if a user sets
PGADMIN_CONFIG_CONFIG_DATABASE_URI='value', the environment variable containsvalue(without quotes). This check only catches URIs that contain literal quote characters as part of the data.
🔎 Recommended fix: Use environment variables to pass the URI safely to Python
This approach avoids all shell quoting/escaping issues:
-# Ensures the variable starts and ends in quotes after trimming
-ensure_quoted() {
- local var="$1"
-
- # Trim whitespaces
- var="${var#"${var%%[![:space:]]*}"}"
- var="${var%"${var##*[![:space:]]}"}"
-
- # Check if the variable is already double or single quoted
- if [[ ! $var =~ ^\"(.*)\"$ && ! $var =~ ^\'(.*)\'$ ]]; then
- # Use docstrings in case the value contains quote characters inside
- var="'''${var}'''"
- fi
-
- echo "${var}"
-}
-
# Check whether the external configuration database exists if it is being used.
external_config_db_exists="False"
if [ -n "${PGADMIN_CONFIG_CONFIG_DATABASE_URI}" ]; then
- # Support both quoted and unquoted URIs for backwards compatibility
- PGADMIN_CONFIG_CONFIG_DATABASE_URI=$(ensure_quoted "${PGADMIN_CONFIG_CONFIG_DATABASE_URI}")
-
- external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 -c "from check_external_config_db import check_external_config_db; val = check_external_config_db(${PGADMIN_CONFIG_CONFIG_DATABASE_URI}); print(val)")
+ external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 -c "import os; from check_external_config_db import check_external_config_db; val = check_external_config_db(os.environ.get('PGADMIN_CONFIG_CONFIG_DATABASE_URI', '')); print(val)")
fiThis passes the URI through the environment, avoiding all shell metacharacter interpretation.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
-
When expanding variables inside double quotes, e.g.
"$var", the contents are treated as literal strings, no? The target that receives that string is the Python interpreter, which does no such expansions. -
The likelyhood is almsot 0.
-
Currently, people are using environmental variables with literal quotes, e.g.
PGADMIN_CONFIG_CONFIG_DATABASE_URI='"value"'.
Proposed solution. While the idea is actually good, we would be breaking all the existing code (see point 3 above), so can you propose a solution that accounts for that and keeps it backwards compatible?
There was a problem hiding this comment.
@Guiorgy, you're absolutely right about point 1—I was mistaken about the shell expansion behavior. When ${PGADMIN_CONFIG_CONFIG_DATABASE_URI} is substituted inside double quotes, the shell replaces it with the variable's value, but that value is treated as literal text (no further expansion of $, backticks, etc. within the value itself). My apologies for the confusion.
For backwards compatibility with users who have quotes embedded in their environment variable values, here's a revised approach using os.environ with Python-side quote handling:
# Check whether the external configuration database exists if it is being used.
external_config_db_exists="False"
if [ -n "${PGADMIN_CONFIG_CONFIG_DATABASE_URI}" ]; then
external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 << 'PYEOF'
import os
from check_external_config_db import check_external_config_db
uri = os.environ.get('PGADMIN_CONFIG_CONFIG_DATABASE_URI', '')
# Strip surrounding quotes for backwards compatibility with quoted env vars
uri = uri.strip()
if len(uri) >= 2:
if (uri[0] == '"' and uri[-1] == '"') or (uri[0] == "'" and uri[-1] == "'"):
uri = uri[1:-1]
val = check_external_config_db(uri)
print(val)
PYEOF
)
fiThis approach:
- Uses heredoc (
<< 'PYEOF') to avoid all shell quoting/escaping complexity - Passes the URI via environment (no injection risk)
- Strips surrounding quotes in Python for backwards compatibility with users who added quotes to work around the original bug
- Handles both quoted (
'"value"') and unquoted (value) environment variable values
1 participant
In the following line:
external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 -c "from check_external_config_db import check_external_config_db; val = check_external_config_db("${PGADMIN_CONFIG_CONFIG_DATABASE_URI}"); print(val)")We pass this to the Python interpreter:
Removing the noise we get:
I am assuming the value of the
PGADMIN_CONFIG_CONFIG_DATABASE_URIvariable was meant to be surrounded by quotes inside Python code, but since the Python code is passed to the interpreter as a double quoted shell argument, this resulted in the quotes being lost, moreover, if the variable contained any spaces, this would result in an error since those wouldn't be escaped.This has been reported in #8529, however it was marked as won't fix. Just in case you decide to revisit, here's one possible solution. Instead of just blindly adding the quotes, I added a function
ensure_quotedto check whether the value of the variable is already single/double quoted (I am assuming no one is using docstrings"""string"""since that would complicate the function). This is for backwards compatibility to support the already existing configurations that had to manually add the quotes for it to work previously.Edit: Coderabbitai made an interesting suggestion to move the parsing of the environmental variable inside Python itself, in other words, write Pyhton code instead of Bash code. I think that's a better approach. Let me know if you agree and I'll rewrite this PR.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.