Uh oh!
There was an error while loading. Please reload this page.
refactor: Shared lib - #19
Merged
Merged
Conversation
Contributor
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughShared reusable Bash libraries are added and sourced by main scripts. Backup and restore modules are introduced with Docker-aware DB dump/restore flows, Telegram notifications, cron-based backup service management, temp/env/GitHub helpers, and README install examples updated to inline ChangesPasarGuard scripts (shared libs, backup/restore, wiring, README)
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Backup as "lib/pasarguard-backup.sh"
participant Docker as "Docker (DB containers)"
participant FS as "Filesystem (/tmp, backup dir)"
participant Telegram as "Telegram API"
User->>Backup: run backup_command or cron job
Backup->>Docker: detect DB type & request DB dump
Docker-->>Backup: return DB dump artifact
Backup->>FS: collect config/data, create (split) archive
Backup->>Telegram: upload archive parts and captions
Telegram-->>Backup: upload responses
Backup->>User: summary or error notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/docker.sh`:
- Around line 3-6: The install_docker function can report success even if curl
failed because the pipeline lacks pipefail; modify install_docker (and related
shell context) to fail when the download fails by enabling pipeline failure
detection (e.g., set -o pipefail) or by separating the download and install
steps: download the script with curl and verify curl's exit code (or that the
file is non-empty) before piping to sh, and only call colorized_echo green
"Docker installed successfully" after the curl and sh steps both return success;
reference install_docker and colorized_echo when applying the change.
In `@lib/github.sh`:
- Around line 19-25: The github_install_script_from_repo function currently
pipes curl into install which can create a truncated/zero-byte binary on partial
downloads; change it to download to a temporary file first (use mktemp), run
curl -fSL to write into that temp file while checking curl's exit status, set
the executable mode (chmod 755) on the temp file, then atomically move/install
it into /usr/local/bin/$install_name (using install or mv) and cleanup the temp
file via trap; reference github_install_script_from_repo and mirror the pattern
used by install_shared_libs_from_repo/github_download_file for robust error
handling.
In `@lib/system.sh`:
- Around line 47-52: install_package() may run before OS is set which causes
downstream callers like ensure_acme_dependencies() to hit "Unsupported operating
system"; update install_package (and any helper
detect_and_update_package_manager) to ensure detect_os() is invoked when the OS
variable is empty/undefined so OS is initialized before PKG_MANAGER logic runs —
check for an empty "${OS:-}" and call detect_os() early in install_package (or
at start of detect_and_update_package_manager) so package installation paths
have a valid OS value.
In `@pasarguard.sh`:
- Around line 4-26: The script exits if shared libs are missing, preventing the
self-hosting installer from ever fetching them; update pasarguard.sh so that
instead of immediately exiting when the for-loop (which checks
SHARED_LIB_DIR/$shared_lib) detects missing files, it invokes the bootstrap
installer (e.g., call install_pasarguard_script() or equivalent) to
download/install the shared libs into SHARED_LIB_DIR (or a temp dir), then
re-check and only error if the install fails; ensure the subsequent source lines
(source "$SHARED_LIB_DIR/common.sh", system.sh, docker.sh, github.sh, env.sh)
run after a successful install so curl | bash installs work on fresh machines.
In `@pg-node.sh`:
- Around line 4-24: The loader aborts when the shared libs aren't present
(SCRIPT_DIR/SHARED_LIB_DIR check and the for loop that exits), preventing
first-time installs; change the logic so that if required files under
SHARED_LIB_DIR are missing you do not exit immediately but set a flag (e.g.,
need_install=true), skip sourcing, perform or invoke a bootstrap installer
(download/copy the shared libs) and only then source the four files (common.sh,
system.sh, docker.sh, github.sh); specifically modify the SHARED_LIB_DIR
resolution and the existence check loop (the for shared_lib in ... block and
subsequent source calls) to defer exit and call the bootstrap/installation
routine when need_install is true, then re-check and source the libraries once
installed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c398e22-aa0b-4652-875c-58b3f2786c5f
📒 Files selected for processing (8)
README.mdlib/common.shlib/docker.shlib/env.shlib/github.shlib/system.shpasarguard.shpg-node.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
pg-node.sh (1)
4-24:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftShared-lib loader still blocks first-time remote installs.
This still exits before any bootstrap path when
lib/*.shis missing (fresh hosts runningbash -c "$(curl ...)"). The script cannot self-heal because it never gets far enough to source/install shared libs.🧩 Suggested flow change
-SHARED_LIB_DIR="${SCRIPT_DIR}/lib"-if [ ! -f "$SHARED_LIB_DIR/common.sh" ]; then- SHARED_LIB_DIR="/usr/local/lib/pasarguard-scripts/lib"-fi--for shared_lib in common.sh system.sh docker.sh github.sh; do- if [ ! -f "$SHARED_LIB_DIR/$shared_lib" ]; then- printf 'Missing shared library: %s\n' "$SHARED_LIB_DIR/$shared_lib" >&2- exit 1- fi-done+SHARED_LIB_DIR="${SCRIPT_DIR}/lib"+[ -f "$SHARED_LIB_DIR/common.sh" ] || SHARED_LIB_DIR="/usr/local/lib/pasarguard-scripts/lib"++need_install=false+for shared_lib in common.sh system.sh docker.sh github.sh; do+ if [ ! -f "$SHARED_LIB_DIR/$shared_lib" ]; then+ need_install=true+ break+ fi+done++if [ "$need_install" = true ]; then+ bootstrap_shared_libs # download lib files without requiring sourced helpers+fi++# re-check and then source🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pg-node.sh` around lines 4 - 24, The current loader exits immediately if shared libs are missing (the for loop checking "$SHARED_LIB_DIR/$shared_lib"), which prevents first-time bootstrap; update the logic in the block around SCRIPT_DIR/SHARED_LIB_DIR and the for shared_lib loop so that on missing libs it attempts a self-heal bootstrap (e.g., invoke an installer/bootstrap routine or download the missing lib files) and then re-checks the files before sourcing; only call exit 1 if the second check still fails. Refer to SCRIPT_DIR, SHARED_LIB_DIR, the for shared_lib in common.sh/system.sh/docker.sh/github.sh existence checks, and the source "$SHARED_LIB_DIR/..." lines to place the bootstrap attempt and re-validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pasarguard-backup.sh`:
- Around line 693-695: In remove_backup_service, crontab -l can exit non‑zero
when no crontab exists and with set -e will abort; change the logic around the
temporary file creation (temp_cron / mktemp and the crontab -l invocation) to
tolerate that failure by capturing crontab -l’s output into the temp file but
treating a non‑zero exit as "no crontab" (e.g., use crontab -l || true or an if
check) so the function proceeds to remove the job and restore crontab safely
even when no existing crontab is present.
- Around line 142-170: The summary block is unreachable because the array
uploaded_files is never initialized or populated; initialize uploaded_files as
an empty array (e.g., uploaded_files=() or local uploaded_files=() before the
loop that sends parts) and, inside the success branch where the script detects
'"ok":true' (the block that currently prints "Backup part $custom_filename
successfully sent to Telegram."), append the sent filename to the array (e.g.,
uploaded_files+=("$custom_filename")); this ensures the later conditional if [
${`#uploaded_files`[@]} -gt 0 ] will be true and the info_message summary will
include the uploaded files.
- Around line 785-797: The script currently writes SQLALCHEMY_DATABASE_URL
(which may contain credentials) to the log_file; instead create a redacted copy
and log that. In the block using SQLALCHEMY_DATABASE_URL and log_file, add a
step to produce a safe variable (e.g. safe_sqlalchemy_url) which strips or
replaces the userinfo portion (the username:password between "://" and "@") with
a fixed token like "REDACTED" (you can use shell parameter expansion or sed to
remove/replace the segment), then write safe_sqlalchemy_url to "$log_file" and
use it in the "Parsing ..." echo; leave the original SQLALCHEMY_DATABASE_URL for
actual DB operations but never log it. Ensure references to
SQLALCHEMY_DATABASE_URL in echo/log statements are replaced with
safe_sqlalchemy_url.
- Line 1: The file starts with a UTF-8 BOM which breaks shebang recognition;
remove the BOM so the file begins exactly with the shebang "#!/usr/bin/env bash"
(save the file as UTF-8 without BOM or strip the BOM), and apply the same fix to
the other affected files pasarguard-restore.sh and pasarguard.sh.
In `@lib/pasarguard-restore.sh`:
- Around line 835-867: The backups currently copy from the extracted payload
($temp_restore_dir) instead of the live files, so change the cp source to the
existing live files under $APP_DIR when creating backups (e.g., replace cp
"$temp_restore_dir/.env" "$APP_DIR/.env.backup.$(...)" with cp "$APP_DIR/.env"
"$APP_DIR/.env.backup.$(...)" and similarly for docker-compose.yml), and wrap
each backup cp in an existence check (if [ -f "$APP_DIR/.env" ] / if [ -f
"$APP_DIR/docker-compose.yml" ]) so you only back up files that actually exist
before overwriting with the payload.
- Line 1: The file contains a UTF-8 BOM before the shebang which can break
interpreter recognition; remove the BOM so the file begins exactly with
"#!/usr/bin/env bash" (ensure the first bytes are 23 21 2F ... for the shebang)
by saving the file without BOM (e.g., re-encode as UTF-8 without BOM) so scripts
like pasarguard-restore.sh execute and source correctly.
- Around line 723-728: The three psql -c commands currently interpolating
$target_db_name and $target_db_owner are vulnerable to injection and broken by
special characters; update those SQL strings to use PostgreSQL's quote_ident()
for identifiers (and quote_literal() if you ever need literals) instead of raw
bash expansion — e.g., in the SELECT pg_terminate_backend(...), DROP DATABASE IF
EXISTS ..., and CREATE DATABASE ... OWNER ... replace datname =
'$target_db_name' / "$target_db_name" / "$target_db_owner" with datname =
quote_ident('$target_db_name') / quote_ident('$target_db_name') /
quote_ident('$target_db_owner') respectively; likewise adjust subsequent psql
invocations that use "$target_db_name" as a connection target (the later docker
exec psql -d "$target_db_name" calls referenced) to instead connect to postgres
and run SQL that references the target via quote_ident('$target_db_name') so
identifiers are safely escaped (references: the SELECT pg_terminate_backend
call, DROP DATABASE IF EXISTS, CREATE DATABASE ... OWNER, and later psql -d
uses).
In `@lib/system.sh`:
- Around line 10-13: The check for /etc/lsb-release calls the lsb_release binary
unguarded which can fail on minimal images; change the condition around the OS
assignment so you only run lsb_release when the command exists (e.g. use command
-v or type to test lsb_release) and fall back to the /etc/os-release awk branch
otherwise; update the block that sets OS (the lsb_release -si usage and the
/etc/os-release awk fallback) to first verify lsb_release is available before
invoking it.
- Around line 187-194: The install currently writes the remote yq binary
directly to /usr/local/bin/yq (using the yq_url variable) without integrity
checks; update the flow to download the corresponding checksum file (prefer the
checksums-bsd or the tarball + its checksum), save downloads to temporary files,
compute the expected digest (using shasum/sha256sum or the appropriate algorithm
matching the checksums file), compare the computed checksum against the
published value and call die on mismatch, and only then move the verified binary
to /usr/local/bin/yq and run chmod +x and colorized_echo; ensure both curl and
wget branches download the checksum and binary to temp paths and verify before
replacing the target.
---
Duplicate comments:
In `@pg-node.sh`:
- Around line 4-24: The current loader exits immediately if shared libs are
missing (the for loop checking "$SHARED_LIB_DIR/$shared_lib"), which prevents
first-time bootstrap; update the logic in the block around
SCRIPT_DIR/SHARED_LIB_DIR and the for shared_lib loop so that on missing libs it
attempts a self-heal bootstrap (e.g., invoke an installer/bootstrap routine or
download the missing lib files) and then re-checks the files before sourcing;
only call exit 1 if the second check still fails. Refer to SCRIPT_DIR,
SHARED_LIB_DIR, the for shared_lib in common.sh/system.sh/docker.sh/github.sh
existence checks, and the source "$SHARED_LIB_DIR/..." lines to place the
bootstrap attempt and re-validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19a514dc-3f37-405e-89da-6a9543c750e7
📒 Files selected for processing (12)
docker-compose/node.ymldocker-compose/pasarguard-mariadb.ymldocker-compose/pasarguard-mysql.ymldocker-compose/pasarguard-postgresql.ymldocker-compose/pasarguard-timescaledb.ymllib/docker.shlib/github.shlib/pasarguard-backup.shlib/pasarguard-restore.shlib/system.shpasarguard.shpg-node.sh
✅ Files skipped from review due to trivial changes (1)
- lib/docker.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/github.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
M03ED
commented
May 1, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
lib/system.sh (1)
203-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify the yq checksum parser against the upstream release format.
awk '$2 == name { print $1 }'assumes a two-column checksum file. If thechecksumsasset is not in that format,expected_checksumstays empty or resolves the wrong field andinstall_yq()fails on clean machines. Prefer the release asset meant for standard SHA-256 verification, or parse the upstream format explicitly instead of assuming$1/$2.How is the mikefarah/yq GitHub release `checksums` asset formatted, and which release asset (`checksums` vs `checksums-bsd`) should a shell installer use to verify `yq_linux_amd64` with `sha256sum`?🤖 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 `@lib/system.sh` around lines 203 - 204, The current awk lookup in install_yq assumes a two-column file and can fail for upstream formats; instead fetch and use the release "checksums" asset (the one compatible with sha256sum for Linux) when verifying yq_linux_amd64, and parse checksum_tmp explicitly by matching the yq_binary filename and extracting the hex checksum field (e.g., find the line containing yq_binary in checksum_tmp and pull the SHA-256 token) before assigning expected_checksum; update references in install_yq, expected_checksum, and checksum_tmp to use this robust lookup so checksums-bsd or other formats don't break verification.
🤖 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.
Inline comments:
In `@lib/pasarguard-backup.sh`:
- Around line 721-729: The script currently uses global shared paths
(temp_dir="/tmp/pasarguard_backup" and
log_file="/var/log/pasarguard_backup_error.log") which allows concurrent runs to
clobber each other; change to create per-run temp and log files (use mktemp -d
to set temp_dir and mktemp for log_file or derive one from timestamp/$$) and
update uses of temp_dir, log_file, and final_backup_paths accordingly, and add a
simple mutual-exclusion guard (e.g., a lock using flock or a PID lockfile)
around the main backup routine so concurrent invocations are serialized and each
run cleans up only its own temp/log artifacts.
- Around line 744-748: Currently the script removes all previous backups (rm -f
"$backup_dir"/backup_*.tar.gz, zip, z[0-9][0-9]) before the
dump/archive/Telegram steps, risking no recoverable backup if later steps fail;
change the flow so that deletion/retention happens only after a successful
archive creation: move the rm -f retention cleanup (and any mkdir -p if needed)
to run after the archive creation step completes successfully (check the archive
command exit status or wrap with a success conditional), or instead implement a
retention routine that, after successful archive creation, lists existing
"backup_*.tar.gz" and "backup_*.zip" files in "$backup_dir" and deletes only the
oldest files beyond the desired N recent backups.
- Around line 270-272: The script currently prints the full secret in
colorized_echo calls (e.g., where telegram_bot_key is echoed); change those to
print a masked version instead by transforming telegram_bot_key into a short
visible fragment (for example show first 4 and last 4 chars with the middle
replaced by asterisks, or just show "****<last6>") before passing to
colorized_echo. Update every occurrence that prints the token (the
colorized_echo call that outputs "Telegram Bot API Key: $telegram_bot_key" and
the similar prints mentioned around the same areas) so they use a helper/masked
variable (e.g., masked_telegram_bot_key) created by slicing and replacing the
middle with asterisks, then pass that masked value to colorized_echo.
- Around line 460-462: The crontab removal currently uses grep -v "$command"
which can accidentally remove unrelated lines; change the dedupe to filter by
the stable marker instead (the comment "# pasarguard-backup-service") and use a
literal/fgrep style match to avoid regex surprises: read crontab into the temp
file (temp_cron), write a filtered version by removing any line containing the
marker, then mv the temp back and append the new schedule/command (variables
schedule and command) with the marker; update the grep invocation that produces
"${temp_cron}.tmp" to target the marker (and use a literal match option like -F)
rather than "$command".
In `@lib/pasarguard-restore.sh`:
- Around line 691-694: The restore logic sets restore_user/restore_password from
the backup metadata but should prefer the current installation identity when
available; update the restore_user, restore_password (and restore_db/name if
applicable) assignments to use current_db_user, current_db_password, and
current_db_name as the preferred values (falling back to db_user/DB_USER and
db_password/DB_PASSWORD from the backup) so non-Timescale PostgreSQL restores
run against the local/rotated credentials; apply the same change in the other
restore block referenced around the 772-786 region so both restore paths behave
consistently.
- Around line 393-401: The script currently prints the first 50 chars of
SQLALCHEMY_DATABASE_URL which can leak credentials; update the success message
that uses colorized_echo to print a redacted version of SQLALCHEMY_DATABASE_URL
instead of ${SQLALCHEMY_DATABASE_URL:0:50}—reuse the existing redaction helper
used elsewhere in this script/logging (instead of slicing) to strip or mask
credentials (e.g., replace password/user or show only scheme+host) before
passing into colorized_echo, keeping the checks around extracted_env and
temp_restore_dir unchanged.
- Around line 843-867: The restore logic sets preserve_db_credentials and copies
current_db_user/current_db_name/current_db_password/current_sqlalchemy_url back
into the ENV_FILE but omits preserving the captured MYSQL_ROOT_PASSWORD; add
handling to write the preserved root password when preserve_db_credentials is
true: after checking current_db_password (and before/with
current_sqlalchemy_url) call replace_or_append_env_var for "MYSQL_ROOT_PASSWORD"
using the captured variable (e.g., current_mysql_root_password or the name used
earlier) with the appropriate sensitive flag and ENV_FILE so the restored .env
retains the active DB root password.
---
Duplicate comments:
In `@lib/system.sh`:
- Around line 203-204: The current awk lookup in install_yq assumes a two-column
file and can fail for upstream formats; instead fetch and use the release
"checksums" asset (the one compatible with sha256sum for Linux) when verifying
yq_linux_amd64, and parse checksum_tmp explicitly by matching the yq_binary
filename and extracting the hex checksum field (e.g., find the line containing
yq_binary in checksum_tmp and pull the SHA-256 token) before assigning
expected_checksum; update references in install_yq, expected_checksum, and
checksum_tmp to use this robust lookup so checksums-bsd or other formats don't
break verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9eda172-6c63-413b-adf6-38e70e08b7de
📒 Files selected for processing (4)
lib/pasarguard-backup.shlib/pasarguard-restore.shlib/system.shpasarguard.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
M03ED
commented
May 6, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@lib/pasarguard-backup.sh`:
- Around line 1322-1334: The rsync exclude list currently only omits "mysql"
causing raw DB files for other engines to be copied; update the rsync call that
references "$DATA_DIR/" and "$temp_dir/pasarguard_data/" to exclude all DB
engine directories (e.g. "mysql", "mariadb", "postgresql", "timescaledb") or
dynamically build the exclude list from the deployment DB type (the variable
that identifies DB engine), so raw on-disk DB folders are never copied alongside
the SQL dump and the archive stays consistent.
In `@lib/pasarguard-restore.sh`:
- Around line 828-845: The restore currently uses rsync -a which merges instead
of overwriting; update the restore logic around extracted_data_dir/DATA_DIR in
the pasarguard-restore flow to (1) create a timestamped backup of the current
$DATA_DIR (similar to how .env and docker-compose.yml are backed up) into
$temp_restore_dir or a backups dir, and then (2) either run rsync with deletion
semantics (add --delete and any necessary --delete-excluded) when invoking
rsync, or remove the destination contents (e.g., rm -rf "$DATA_DIR"/*) before
rsync to ensure stale files are removed; keep existing logging to $log_file and
use colorized_echo for status messages, and retain detect_os/install_package
checks for rsync as-is.
- Around line 855-882: The preserve_db_credentials flag logic fails to detect
when only the MySQL root password changed; update the conditional that sets
preserve_db_credentials (the block referencing current_db_user, current_db_name,
current_db_password) to also check current_mysql_root_password vs
${MYSQL_ROOT_PASSWORD:-} so the flag is true when the rotated root password
differs; ensure the same variable name current_mysql_root_password is used and
that when preserve_db_credentials is true the existing code still calls
replace_or_append_env_var "MYSQL_ROOT_PASSWORD" "$current_mysql_root_password"
to keep the live MYSQL_ROOT_PASSWORD intact in the ENV_FILE.
- Around line 786-803: The postgres-superuser fallback currently connects to -d
"$db_name" (parsed from the backup) which can target the wrong DB; change the
superuser attempt in the docker exec command that uses -U postgres to connect to
-d "$restore_db_name" (the resolved target that prefers current_db_name) instead
of "$db_name", and ensure any success/failure messages still reference
$restore_db_name (symbols: $restore_user, $restore_db_name, $db_name,
temp_restore_dir/db_backup.sql, the docker exec psql lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78b2314b-775b-46db-93b8-3fd8e3e46336
📒 Files selected for processing (2)
lib/pasarguard-backup.shlib/pasarguard-restore.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
lib/pasarguard-restore.sh (4)
681-686: 💤 Low valueIndentation drift in the empty-backup-file guard.
The body of the
if [ ! -s ... ]check at Line 682 is indented three additional levels deeper than the openingif, which makes the block hard to read and risks getting visually attached to the wrong scope during a future edit. Re-aligning to the surrounding indentation would help.♻️ Quick reformat
- if [ ! -s "$temp_restore_dir/db_backup.sql" ]; then- colorized_echo red "Database backup file is empty or unreadable."- rm -rf "$temp_restore_dir"- exit 1- fi+ if [ ! -s "$temp_restore_dir/db_backup.sql" ]; then+ colorized_echo red "Database backup file is empty or unreadable."+ rm -rf "$temp_restore_dir"+ exit 1+ fi🤖 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 `@lib/pasarguard-restore.sh` around lines 681 - 686, The if-block that checks the database backup file size (if [ ! -s "$temp_restore_dir/db_backup.sql" ]) has inconsistent indentation: bring the body lines (colorized_echo red "Database backup file is empty or unreadable.", rm -rf "$temp_restore_dir", and exit 1) back to the same indentation level as the opening if to match surrounding code and avoid visual scope errors; locate the snippet referencing "$temp_restore_dir/db_backup.sql" and colorized_echo and re-align those three lines so they are not indented extra levels.
585-592: 💤 Low value
is_mariadbis set but never read (SC2034).The flag is correctly initialized and toggled, but downstream logic only reads
mysql_cmd/db_type_name. Either drop the flag or use it for the few places (e.g.,mysqldump --column-statistics=0is needed against MySQL 8 but not MariaDB) where the distinction matters.♻️ Quick fix (drop unused flag)
- local is_mariadb=false local mysql_cmd="mysql" local db_type_name="MySQL" if docker exec "$container_name" mariadb --version >/dev/null 2>&1; then - is_mariadb=true mysql_cmd="mariadb" db_type_name="MariaDB" fi🤖 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 `@lib/pasarguard-restore.sh` around lines 585 - 592, The local variable is_mariadb is set in the block that checks docker exec "$container_name" mariadb --version but never used; either remove is_mariadb entirely and rely only on mysql_cmd and db_type_name, or use is_mariadb to gate MySQL-vs-MariaDB specific behavior (for example when invoking mysqldump with --column-statistics=0 for MySQL 8). Update the code around the docker exec check and all downstream usages (references to mysql_cmd, db_type_name, and any mysqldump invocation) so the unused flag is removed or applied consistently to select the correct flags/commands.
67-74: ⚡ Quick winShared
/tmp/pasarguard_restorepath is vulnerable to clobber/symlink races.Hardcoding
/tmp/pasarguard_restoreand unconditionallyrm -rf-ing it has two consequences: (1) two restore invocations stomp each other and (2) anyone with write access to/tmpcan pre-create that path (or a symlink) before the privileged script runs and influence what gets removed/written. The backup script switched tomktemp -dfor exactly these reasons; the restore path is interactive and admin-only, so the urgency is lower, but the same fix is cheap.♻️ Suggested fix
- local backup_dir="$APP_DIR/backup"- local temp_restore_dir="/tmp/pasarguard_restore"+ local backup_dir="$APP_DIR/backup"+ local temp_restore_dir+ temp_restore_dir=$(mktemp -d "${TMPDIR:-/tmp}/pasarguard_restore.XXXXXX") || {+ colorized_echo red "Failed to create restore temp directory."+ exit 1+ } local log_file="/var/log/pasarguard_restore_error.log" - >"$log_file"+ : >"$log_file" echo "Restore Log - $(date)" >>"$log_file" -- # Clean up temp directory- rm -rf "$temp_restore_dir"- mkdir -p "$temp_restore_dir"(also addresses the SC2188 hint on Line 69.)
🤖 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 `@lib/pasarguard-restore.sh` around lines 67 - 74, The hardcoded temp_restore_dir and unconditional rm -rf introduce TOCTOU/symlink race and clobber issues; change the creation of temp_restore_dir to use a secure temporary directory (e.g., mktemp -d) instead of "/tmp/pasarguard_restore", remove the unsafe rm -rf on that shared path, and ensure the script uses that created directory variable (temp_restore_dir) consistently; also ensure log_file initialization (log_file="/var/log/pasarguard_restore_error.log") remains but avoid truncating files insecurely—open/write to it safely after creating the secure temp dir to address the race and the SC2188 hint.
511-523: 💤 Low valueConfirmation regex doesn't accept all-caps responses like
YES/NO.
^[Yy](es)?$matchesy,Y,yes,Yes, but notYES. Likewise^[Nn](o)?$rejectsNO. Operators typing in caps will be told "Please answer yes or no." indefinitely. A case-insensitive shell glob is simpler:♻️ Suggested fix
- if [[ "$confirm" =~ ^[Yy](es)?$ ]]; then+ shopt -s nocasematch+ if [[ "$confirm" =~ ^y(es)?$ ]]; then+ shopt -u nocasematch break - elif [[ "$confirm" =~ ^[Nn](o)?$ ]]; then+ elif [[ "$confirm" =~ ^n(o)?$ ]]; then+ shopt -u nocasematch colorized_echo yellow "Restore cancelled."Or simply expand the character class:
^[YyNn][EeOo]?[Ss]?$after splitting yes/no.🤖 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 `@lib/pasarguard-restore.sh` around lines 511 - 523, The confirmation regex in the while true loop rejects all-caps answers; modify the loop around the read -r confirm to enable case-insensitive matching (e.g., run shopt -s nocasematch before the while and restore it after) and then change the tests to use a simpler pattern like =~ ^(yes|no|y|n)$ against the confirm variable; keep the existing branches (colorized_echo "Restore cancelled.", rm -rf "$temp_restore_dir", exit 0) and ensure you unset or restore nocasematch after the loop so other code behavior is unchanged.lib/pasarguard-backup.sh (4)
1097-1097: 💤 Low value
$databasesis intentionally unquoted but vulnerable to globbing/IFS surprises.
docker exec ... "$dump_cmd" -u root -p"..." --databases $databases ...relies on word-splitting to expand the database list, but it's also subject to glob expansion and arbitraryIFS. With the rest of the script underset -eand untrusted DB names theoretically possible, a name containing*or?could glob to host filesystem entries. Safer to read the names into an array.♻️ Suggested change
- databases=$(docker exec "$container_name" "$mysql_cmd" -u root -p"$MYSQL_ROOT_PASSWORD" -e "SHOW DATABASES;" 2>>"$log_file" | grep -Ev "^(Database|mysql|performance_schema|information_schema|sys)$" || true)+ mapfile -t db_list < <(docker exec "$container_name" "$mysql_cmd" -u root -p"$MYSQL_ROOT_PASSWORD" -e "SHOW DATABASES;" 2>>"$log_file" | grep -Ev "^(Database|mysql|performance_schema|information_schema|sys)$" || true) @@ - elif ! docker exec "$container_name" "$dump_cmd" -u root -p"$MYSQL_ROOT_PASSWORD" --databases $databases --events --triggers >"$temp_dir/db_backup.sql" 2>>"$log_file"; then+ elif ! docker exec "$container_name" "$dump_cmd" -u root -p"$MYSQL_ROOT_PASSWORD" --databases "${db_list[@]}" --events --triggers >"$temp_dir/db_backup.sql" 2>>"$log_file"; then(adjust the
[ -z "$databases" ]check to[ ${#db_list[@]} -eq 0 ]accordingly.)🤖 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 `@lib/pasarguard-backup.sh` at line 1097, The unquoted $databases used in the docker exec mysqldump call can be globbed or split; change the code to read database names into an array (e.g., db_list) and pass them as a quoted array expansion to the docker exec invocation (use "${db_list[@]}") instead of $databases, update the empty-check from [ -z "$databases" ] to [ ${`#db_list`[@]} -eq 0 ], and ensure the mysqldump invocation that currently references databases, dump_cmd, container_name, temp_dir and log_file uses the safe array expansion to avoid IFS/globbing issues.
466-473: 💤 Low valueUse direct exit-status check instead of
$?(SC2181).Minor style: testing
$?separately is brittle if any command (including a future addition between the call and the check) intervenes.♻️ Quick fix
- backup_command- if [ $? -eq 0 ]; then+ if backup_command; then colorized_echo green "Initial backup completed successfully." else colorized_echo yellow "Initial backup completed with warnings. Check logs if needed." fi🤖 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 `@lib/pasarguard-backup.sh` around lines 466 - 473, Replace the separate call to backup_command followed by a separate "$?" check with a direct conditional that runs backup_command in the if statement (e.g., if backup_command; then ... else ... fi) so the command's exit status is tested immediately; update the block that currently calls backup_command and checks "$?" to instead use the single if backup_command form and keep the existing colorized_echo green/yellow messages inside the then/else branches, referencing the backup_command invocation and colorized_echo function names.
461-468: ⚡ Quick winLocal
backup_commandvariable shadows thebackup_command()function name.Inside
backup_service,local backup_command="PATH=... bash $script_path backup"shares the exact name of the function defined at line 751. Bash keeps separate namespaces for functions and variables, so this works today, but it's fragile and confusing to readers — a future refactor that calls"$backup_command"(or evaluates the variable as a command) will silently invoke the cron string instead of the function. The same shadow appears inedit_backup_serviceat Line 644.♻️ Suggested rename
- local backup_command="PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin bash $script_path backup"- add_cron_job "$cron_schedule" "$backup_command"+ local backup_cron_command="PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin bash $script_path backup"+ add_cron_job "$cron_schedule" "$backup_cron_command"(apply the same rename around Line 644.)
🤖 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 `@lib/pasarguard-backup.sh` around lines 461 - 468, The local variable backup_command inside backup_service shadows the existing backup_command() function (also used in edit_backup_service) and can cause confusing behavior; rename the variable to something like cron_backup_command or backup_command_str, update its uses (e.g., add_cron_job "$cron_schedule" "$cron_backup_command"), and ensure the initial backup still invokes the function by leaving the function call as backup_command (or explicitly call backup_command() if preferred); apply the same rename and usage changes in edit_backup_service to remove the shadowing.
818-819: 💤 Low value
>"$log_file"is a redirection without a command (SC2188).This works in practice (bash truncates the file), but ShellCheck flags it because it isn't an actual command. Use a noop to make the intent explicit and silence the warning.
♻️ Quick fix
- >"$log_file"- echo "Backup Log - $(date)" >>"$log_file"+ : >"$log_file"+ echo "Backup Log - $(date)" >>"$log_file"🤖 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 `@lib/pasarguard-backup.sh` around lines 818 - 819, Replace the bare redirection line (>"$log_file") that triggers ShellCheck SC2188 with an explicit no-op command redirected into "$log_file" so the intent to truncate the file is clear and the warning is silenced; keep the existing echo "Backup Log - $(date)" >>"$log_file" line and continue using the log_file variable.
🤖 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.
Inline comments:
In `@lib/pasarguard-backup.sh`:
- Around line 872-878: The failure branch for missing SQLALCHEMY_DATABASE_URL
currently appends to error_messages and returns but doesn't set keep_log_file or
invoke send_backup_error_to_telegram, so the RETURN trap will delete the log and
no Telegram alert is sent; update the branch handling in the
SQLALCHEMY_DATABASE_URL check to mirror the earlier error block (lines
~849-853): set keep_log_file=true, append the same detailed error message, call
send_backup_error_to_telegram, and then return 1 so the log is preserved and a
Telegram notification is dispatched; reference the SQLALCHEMY_DATABASE_URL
variable, error_messages array, colorized_echo, keep_log_file flag,
send_backup_error_to_telegram function, and the RETURN trap behavior when making
the change.
- Around line 920-960: The URL-parsed credentials and database name (db_user,
db_password, db_name) may be percent-encoded and must be percent-decoded before
being passed to dump/restore tools; add a small urldecode helper (e.g.,
urldecode()) and call db_user=$(urldecode "$db_user"), db_password=$(urldecode
"$db_password"), and db_name=$(urldecode "$db_name") immediately after they are
extracted in the parsing block shown (the local url_part/auth_part extraction
and the host/port/db_name assignment), and apply the same change in the
corresponding restore parsing block so tools receive decoded credentials.
In `@lib/pasarguard-restore.sh`:
- Around line 597-651: The restore chooses the backup's db_name first
(app_db_target="${db_name:-${current_db_name:-}}") which causes MySQL/MariaDB
restores to target the old DB name; change the assignment to prefer
current_db_name first (app_db_target="${current_db_name:-${db_name:-}}") to
mirror Postgres/Timescale behavior, and update the final fallback conditional
that checks credential rotation (the if block using
current_db_user/current_db_password and comparing them to
backup_restore_user/backup_restore_password) to also compare the DB name (i.e.,
ensure it triggers when current_db_name != db_name) so a renamed database with
same credentials will fall back to the current installation values; adjust
references to app_db_target, db_name, current_db_name, backup_restore_user,
backup_restore_password, current_db_user, current_db_password and
restore_success accordingly.
- Around line 691-818: The error message "Remote $db_type restore not supported
yet." is misleading when db_host is local but container_name is empty; update
the restore branch that checks [[ "$db_host" == "127.0.0.1" || "$db_host" ==
"localhost" || "$db_host" == "::1" ]] && [ -n "$container_name" ] to distinguish
two cases: if db_host is local but verify_and_start_container (container_name)
returned empty, emit a clear message (via colorized_echo) indicating the
container wasn't found/started and instruct to start the DB container or pass a
container name, clean up temp_restore_dir and exit non-zero; otherwise (true
remote host) keep the existing "Remote $db_type restore not supported yet."
path. Reference the variables container_name, db_host,
verify_and_start_container, temp_restore_dir and the colorized_echo call so you
change only the else/fallback handling to branch on container presence.
---
Nitpick comments:
In `@lib/pasarguard-backup.sh`:
- Line 1097: The unquoted $databases used in the docker exec mysqldump call can
be globbed or split; change the code to read database names into an array (e.g.,
db_list) and pass them as a quoted array expansion to the docker exec invocation
(use "${db_list[@]}") instead of $databases, update the empty-check from [ -z
"$databases" ] to [ ${`#db_list`[@]} -eq 0 ], and ensure the mysqldump invocation
that currently references databases, dump_cmd, container_name, temp_dir and
log_file uses the safe array expansion to avoid IFS/globbing issues.
- Around line 466-473: Replace the separate call to backup_command followed by a
separate "$?" check with a direct conditional that runs backup_command in the if
statement (e.g., if backup_command; then ... else ... fi) so the command's exit
status is tested immediately; update the block that currently calls
backup_command and checks "$?" to instead use the single if backup_command form
and keep the existing colorized_echo green/yellow messages inside the then/else
branches, referencing the backup_command invocation and colorized_echo function
names.
- Around line 461-468: The local variable backup_command inside backup_service
shadows the existing backup_command() function (also used in
edit_backup_service) and can cause confusing behavior; rename the variable to
something like cron_backup_command or backup_command_str, update its uses (e.g.,
add_cron_job "$cron_schedule" "$cron_backup_command"), and ensure the initial
backup still invokes the function by leaving the function call as backup_command
(or explicitly call backup_command() if preferred); apply the same rename and
usage changes in edit_backup_service to remove the shadowing.
- Around line 818-819: Replace the bare redirection line (>"$log_file") that
triggers ShellCheck SC2188 with an explicit no-op command redirected into
"$log_file" so the intent to truncate the file is clear and the warning is
silenced; keep the existing echo "Backup Log - $(date)" >>"$log_file" line and
continue using the log_file variable.
In `@lib/pasarguard-restore.sh`:
- Around line 681-686: The if-block that checks the database backup file size
(if [ ! -s "$temp_restore_dir/db_backup.sql" ]) has inconsistent indentation:
bring the body lines (colorized_echo red "Database backup file is empty or
unreadable.", rm -rf "$temp_restore_dir", and exit 1) back to the same
indentation level as the opening if to match surrounding code and avoid visual
scope errors; locate the snippet referencing "$temp_restore_dir/db_backup.sql"
and colorized_echo and re-align those three lines so they are not indented extra
levels.
- Around line 585-592: The local variable is_mariadb is set in the block that
checks docker exec "$container_name" mariadb --version but never used; either
remove is_mariadb entirely and rely only on mysql_cmd and db_type_name, or use
is_mariadb to gate MySQL-vs-MariaDB specific behavior (for example when invoking
mysqldump with --column-statistics=0 for MySQL 8). Update the code around the
docker exec check and all downstream usages (references to mysql_cmd,
db_type_name, and any mysqldump invocation) so the unused flag is removed or
applied consistently to select the correct flags/commands.
- Around line 67-74: The hardcoded temp_restore_dir and unconditional rm -rf
introduce TOCTOU/symlink race and clobber issues; change the creation of
temp_restore_dir to use a secure temporary directory (e.g., mktemp -d) instead
of "/tmp/pasarguard_restore", remove the unsafe rm -rf on that shared path, and
ensure the script uses that created directory variable (temp_restore_dir)
consistently; also ensure log_file initialization
(log_file="/var/log/pasarguard_restore_error.log") remains but avoid truncating
files insecurely—open/write to it safely after creating the secure temp dir to
address the race and the SC2188 hint.
- Around line 511-523: The confirmation regex in the while true loop rejects
all-caps answers; modify the loop around the read -r confirm to enable
case-insensitive matching (e.g., run shopt -s nocasematch before the while and
restore it after) and then change the tests to use a simpler pattern like =~
^(yes|no|y|n)$ against the confirm variable; keep the existing branches
(colorized_echo "Restore cancelled.", rm -rf "$temp_restore_dir", exit 0) and
ensure you unset or restore nocasematch after the loop so other code behavior is
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39af5089-5286-477f-95ea-1dadc7e53aa1
📒 Files selected for processing (2)
lib/pasarguard-backup.shlib/pasarguard-restore.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features
Refactor