Skip to content

[chores] Added loud failure message if existing database is detected #562 #568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Srinath0916 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[chores] Added loud failure message if existing database is detected #562 #568

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 59 additions & 3 deletions deploy/auto-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,44 @@ setup_docker_openwisp() {
echo -ne ${GRN}"Do you have .env file? Enter filepath (leave blank for ad-hoc configuration): "${NON}
read env_path
if [[ ! -f "$env_path" ]]; then
# Validate backup has required credentials
backup_has_credentials=false
if [[ -f "$ENV_BACKUP" ]]; then
backup_has_credentials=true
for config in DB_USER DB_PASS DJANGO_SECRET_KEY; do
if [[ -z "$(get_env "$config" "$ENV_BACKUP")" ]]; then
backup_has_credentials=false
break
fi
done
fi

if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ "$backup_has_credentials" != true ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't let an incomplete live .env bypass the volume-safety abort.

Line 118 only checks whether $INSTALL_PATH/.env exists, not whether it still contains DB_USER, DB_PASS, and DJANGO_SECRET_KEY. If that file is partial or corrupted, download_docker_openwisp() will move it to $ENV_BACKUP, restore_from_backup will stay false, and Lines 216-217 will regenerate fresh credentials against the existing docker-openwisp_postgres_data volume. That recreates the auth mismatch this PR is trying to prevent. Reuse the same required-key validation for the live .env, or abort again immediately before the regeneration fallback.

Based on learnings, the .env file contains critical configurations that must be present for safe operation, and silent failures with empty values would be more dangerous than failing explicitly when the file is missing.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@deploy/auto-install.sh` at line 118, The check at the top of the install flow
only tests for the existence of "$INSTALL_PATH/.env" and can allow a
partial/corrupted file to bypass the volume-safety abort; update the condition
to validate that the live .env contains DB_USER, DB_PASS and DJANGO_SECRET_KEY
(same validation used elsewhere where required-key checks are applied) and treat
it as missing if any are absent so that download_docker_openwisp() will not move
an incomplete file into $ENV_BACKUP and cause restore_from_backup to remain
false; ensure this change interacts with the existing backup_has_credentials
logic and the docker-openwisp_postgres_data volume check so the script aborts
rather than proceeds to regenerate credentials against the existing volume.

{
echo -e "${RED}CRITICAL: Existing database volume detected!${NON}"
echo ""
echo "The Docker volume \"docker-openwisp_postgres_data\" already exists on this system."
echo "This likely means there is database data from a previous OpenWISP installation."
echo ""
echo "The auto-install script generates new database credentials during fresh installations."
echo "If it proceeds while this volume exists, the newly generated credentials will not"
echo "match the credentials stored in the existing database, making the database"
echo "inaccessible to OpenWISP."
echo ""
echo -e "${RED}⚠️ WARNING: The commands below will permanently delete the database volume and all"
echo -e "stored data. Run them only if you intentionally want to wipe the previous installation"
echo -e "or have a verified backup. Proceed at your own discretion.${NON}"
echo ""
echo "Cleanup commands:"
echo -e " ${YLW}cd /opt/openwisp/docker-openwisp && docker compose down --volumes${NON}"
echo "or"
echo -e " ${YLW}docker volume rm docker-openwisp_postgres_data${NON}"
echo ""
echo "Aborting installation to prevent credential mismatch."
echo -e "${RED}Check logs at $LOG_FILE${NON}"
} | tee -a "$LOG_FILE"
Copy link
Copy Markdown

@kilo-code-bot kilo-code-bot bot Mar 6, 2026 •
edited by nemesifier
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: ANSI escape codes will be written to the log file

The tee -a "$LOG_FILE" call pipes the raw output — including ANSI color codes (\033[1;31m, \033[1;33m, \033[0m) — directly into the log file. When users inspect the log with tail -n 50 -f /opt/openwisp/autoinstall.log (as documented in docs/user/quickstart.rst), they will see garbled escape sequences instead of readable text.

Strip the color codes before writing to the log:

Suggested change
} | tee -a "$LOG_FILE"
} | tee >(sed 's/\x1b\[[0-9;]*m//g' >> "$LOG_FILE") # strips ANSI color codes

Alternatively, use script -q -c or simply write a plain-text version to the log separately.

exit 1
fi
# Dashboard Domain
echo -ne ${GRN}"(1/5) Enter dashboard domain: "${NON}
read dashboard_domain
Expand Down Expand Up @@ -157,9 +195,27 @@ setup_docker_openwisp() {
fi
# Site manager email
set_env "EMAIL_DJANGO_DEFAULT" "$django_default_email"
# Set random secret values
python3 $INSTALL_PATH/build.py change-secret-key >/dev/null
python3 $INSTALL_PATH/build.py change-database-credentials >/dev/null
# Re-validate backup credentials after download
restore_from_backup=false
if [[ -f "$ENV_BACKUP" ]]; then
restore_from_backup=true
for config in DB_USER DB_PASS DJANGO_SECRET_KEY; do
if [[ -z "$(get_env "$config" "$ENV_BACKUP")" ]]; then
restore_from_backup=false
break
fi
done
fi
# Set random secret values only if no previous credentials exist
if [[ "$restore_from_backup" == true ]]; then
for config in DB_USER DB_PASS DJANGO_SECRET_KEY; do
value=$(get_env "$config" "$ENV_BACKUP")
set_env "$config" "$value"
done
Comment on lines +210 to +214
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -name "auto-install.sh" -type f

Repository: openwisp/docker-openwisp

Length of output: 92

🏁 Script executed:

find . -name "build.py" -type f

Repository: openwisp/docker-openwisp

Length of output: 78

🏁 Script executed:

# Let's also check the structure
git ls-files | head -20

Repository: openwisp/docker-openwisp

Length of output: 552

🏁 Script executed:

wc -l deploy/auto-install.sh build.py

Repository: openwisp/docker-openwisp

Length of output: 123

🏁 Script executed:

# Read auto-install.sh to find set_env() function and the backup restore logic
cat -n deploy/auto-install.sh

Repository: openwisp/docker-openwisp

Length of output: 14757

🏁 Script executed:

cat -n build.py

Repository: openwisp/docker-openwisp

Length of output: 1907

Fix unsafe secret restoration through set_env() eval.

The set_env() function (line 27) uses eval to execute an awk command with unquoted variable substitution. When restoring DB_USER, DB_PASS, and DJANGO_SECRET_KEY from the backup file in lines 210-214, values containing special characters like $, backticks, quotes, or backslashes (which build.py intentionally generates in secrets) can be expanded or reinterpreted by the shell rather than written literally. This corrupts the restored credentials and could execute arbitrary commands from a malicious backup file.

The same vulnerability exists in the upgrade path (lines 254-257). Replace set_env() calls for secret restoration with a method that writes values without shell evaluation.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@deploy/auto-install.sh` around lines 210 - 214, The restoration uses
set_env() which relies on eval and unquoted substitution, allowing shell
expansion of secrets; replace the set_env() calls for DB_USER, DB_PASS, and
DJANGO_SECRET_KEY (and the similar upgrade-path calls) with a safe write that
does not invoke eval or unquoted variable interpolation—either add a new
safe_set_env() or extend set_env() to accept a mode that uses awk/sed with -v
(or printf with no interpretation) to pass the secret value as a bound variable
and write it literally to the env file; keep using get_env() to read the
backed-up value but ensure the writer uses a safe mechanism (no eval, no
unquoted "$value") so special characters in secrets are preserved and never
executed.

else
python3 $INSTALL_PATH/build.py change-secret-key >/dev/null
python3 $INSTALL_PATH/build.py change-database-credentials >/dev/null
fi
# SSL Configuration
use_letsencrypt_lower=$(echo "$use_letsencrypt" | tr '[:upper:]' '[:lower:]')
if [[ "$use_letsencrypt_lower" == "y" || "$use_letsencrypt_lower" == "yes" ]]; then
Expand Down