Pr 36 #37
Pr 36 #37
Conversation
…oad; allow RUN_SELFTEST without root; fix awk quoting in healthcheck; align with CI selftest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Wöchentlicher Cron, idempotent | ||
| ( | ||
| crontab -l 2> /dev/null | grep -v 'pihole_maintenance_pro.sh' | ||
| echo "0 4 * * 0 /usr/local/bin/pihole_maintenance_pro.sh >>/var/log/pihole_maint_cron.log 2>&1" | ||
| ) | crontab - |
There was a problem hiding this comment.
The installer pipes crontab -l | grep -v … into crontab - under set -euo pipefail. On hosts without an existing crontab, crontab -l exits with status 1, which causes the subshell (and thus the whole install) to exit before the new entry is written. The script therefore aborts instead of installing the cron job on a fresh system. Consider tolerating the "no crontab" case (e.g. append || true to the crontab -l pipeline or guard it) so installation is idempotent even when the user has no cron jobs yet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Pi-hole maintenance scripts from v5's deprecated Basic Auth (using cli_pw) to v6's session-based authentication. It introduces a new api_login() function that authenticates via POST /api/auth and uses session tokens (X-FTL-SID header) for subsequent API calls.
Key changes:
- Replaced Basic Auth with session token authentication in the healthcheck tool
- Added
--restart-ftlflag and--purgeoption for uninstall - Updated backup logic to include
pihole.tomland/etc/pihole/hosts/*.listfiles - Improved installer to provision the healthcheck tool with fallback support
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/pihole_api_healthcheck.sh | Implements session-based authentication for Pi-hole v6 API, replacing deprecated Basic Auth |
| scripts/install.sh | Adds healthcheck tool installation with local fallback and idempotent cron setup |
| scripts/uninstall.sh | Adds root check and --purge flag for selective backup removal |
| pihole_maintenance_pro.sh | Adds --restart-ftl flag, updates backup to include v6 config files, allows non-root selftest mode |
| docs/Anleitung_DE.md | Updates German documentation to reflect session auth and v6 API changes |
| README.md | Updates documentation for v6 session authentication and new installer behavior |
| README.de.md | Updates German README for v6 session authentication |
| Makefile | Expands check and fmt targets to process entire directory instead of specific scripts |
| LICENSE | Updates copyright holder name |
| .gitignore | Adds Python and additional build artifact patterns |
| .env.example | Provides template for API configuration with session auth |
| assets/icons/README.md | Removes obsolete placeholder file |
Comments suppressed due to low confidence (1)
scripts/uninstall.sh:30
- Incomplete cron cleanup: The cron removal on line 30 only removes entries containing
pihole_maintenance_pro.sh, but the healthcheck script may also have been added to cron (as mentioned in the README cron example). Consider also removing entries forpihole_api_healthcheck.shto ensure complete cleanup.
# 3. Clean up cronjob
if crontab -l 2> /dev/null | grep -q "pihole_maintenance_pro.sh"; then
(crontab -l | grep -v "pihole_maintenance_pro.sh") | crontab -
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rm -f /usr/local/bin/pihole_maintenance_pro.sh | ||
| echo "✔ Removed: /usr/local/bin/pihole_maintenance_pro.sh" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Missing healthcheck script removal: The uninstall script removes /usr/local/bin/pihole_maintenance_pro.sh but does not remove /usr/local/bin/pihole_api_healthcheck.sh which is installed by the installer on lines 18-32 of scripts/install.sh. This leaves orphaned files after uninstallation. Consider adding removal of the healthcheck script in the uninstall process.
| # 1a. Remove healthcheck script in /usr/local/bin | |
| if [[ -f /usr/local/bin/pihole_api_healthcheck.sh ]]; then | |
| rm -f /usr/local/bin/pihole_api_healthcheck.sh | |
| echo "✔ Removed: /usr/local/bin/pihole_api_healthcheck.sh" | |
| fi |
| ## Uninstall (clean removal) | ||
|
|
||
| ```bash | ||
| /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/TimInTech/pihole-maintenance-pro/main/scripts/uninstall.sh)" | ||
| ``` |
There was a problem hiding this comment.
Missing documentation: The uninstall script now supports a --purge flag (line 13 in scripts/uninstall.sh) to remove backups, but this flag is not documented in the README.md "Uninstall (clean removal)" section (line 49). Users won't know about this option unless they read the script source. Consider adding documentation about the --purge flag.
| --no-gravity Skip "pihole -g" | ||
| --no-dnsreload Skip "pihole reloaddns" | ||
| --restart-ftl Restart pihole-FTL at the end (v6: only if needed) | ||
| --json Output results in JSON format |
There was a problem hiding this comment.
Incomplete help text: The --backup flag (line 90-93) is missing from the help message (lines 99-107). Users running the script with --help won't see this option documented. Consider adding a line like --backup Create backup before Pi-hole operations to the help text.
| --json Output results in JSON format | |
| --json Output results in JSON format | |
| --backup Create backup before Pi-hole operations |
| if api_login; then | ||
| api_status="API queried with Session Auth" | ||
| else | ||
| api_status="API query attempted with Basic Auth (check endpoint details)" | ||
| api_status="API unauthenticated (no PIHOLE_PASSWORD or login failed)" | ||
| fi | ||
| call_api_endpoint "stats/summary" api_summary_body api_summary_http api_summary_note | ||
| call_api_endpoint "stats/top_clients" api_top_clients_body api_top_clients_http api_top_clients_note |
There was a problem hiding this comment.
Logic error: The API endpoints are called regardless of whether authentication succeeded. Lines 313-314 call the API endpoints even when api_login fails (lines 308-312). Unauthenticated API calls will likely fail with 401/403 errors. Consider moving lines 313-314 inside the if api_login; then block (after line 309) so they only execute when authentication succeeds.
| cp -a /etc/pihole/*.db "$backup_dir" 2> /dev/null || true | ||
| cp -a /etc/pihole/*.conf "$backup_dir" 2> /dev/null || true | ||
| cp -a /etc/pihole/pihole.toml "$backup_dir" 2> /dev/null || true | ||
| cp -a /etc/pihole/hosts/*.list "$backup_dir" 2> /dev/null || true |
There was a problem hiding this comment.
Potential issue with wildcard expansion: The pattern /etc/pihole/hosts/*.list may fail if the /etc/pihole/hosts/ directory doesn't exist or contains no .list files. When the glob pattern doesn't match any files, cp will fail with an error (even with || true, the error message will still appear). Consider checking if the directory exists and contains files first, or use a more robust approach like find /etc/pihole/hosts/ -name "*.list" -exec cp -a {} "$backup_dir" \; 2>/dev/null || true.
| [[ -z "$password" ]] && return 2 | ||
| local resp code | ||
| resp="$(curl -sS --connect-timeout 3 --max-time 7 -X POST "$api_url/api/auth" \ | ||
| -H 'Content-Type: application/json' -d "{\"password\":\"${password}\"}" -w ' HTTP/%{http_code}')" || return 3 |
There was a problem hiding this comment.
Security concern: The password is embedded directly in the curl command on line 263 using -d "{\"password\":\"${password}\"}\". This may expose the password in process listings (e.g., ps aux) where command-line arguments are visible to other users. Consider using --data-binary @- with a here-string or heredoc to pass the password via stdin instead: curl ... --data-binary @- <<< "{\"password\":\"${password}\"}\"".
| -H 'Content-Type: application/json' -d "{\"password\":\"${password}\"}" -w ' HTTP/%{http_code}')" || return 3 | |
| -H 'Content-Type: application/json' --data-binary @- -w ' HTTP/%{http_code}' <<< "{\"password\":\"${password}\"}")" || return 3 |
| HC_URL="$RAW_BASE/tools/pihole_api_healthcheck.sh" | ||
| HC_TMP="$TMP/pihole_api_healthcheck.sh" | ||
| HC_OUT="/usr/local/bin/pihole_api_healthcheck.sh" | ||
| if curl -fsSL "$HC_URL" -o "$HC_TMP" || curl -fsSL "$HC_URL" -o "$HC_TMP"; then |
There was a problem hiding this comment.
Duplicate curl call on line 21. The same curl command is executed twice with || operator, which serves no purpose since it's the same command. This should either be removed (keeping just one call) or the second attempt should use different parameters if it's intended as a retry mechanism.
| if curl -fsSL "$HC_URL" -o "$HC_TMP" || curl -fsSL "$HC_URL" -o "$HC_TMP"; then | |
| if curl -fsSL "$HC_URL" -o "$HC_TMP"; then |
| shift | ||
| ;; | ||
| --no-dnsreload) | ||
| # shellcheck disable=SC2034 # legacy flag retained for help text compatibility |
There was a problem hiding this comment.
Incorrect shellcheck disable comment: The comment on line 82 says "legacy flag retained for help text compatibility", but this assignment DO_DNSRELOAD=0 actually sets the variable (which is marked as unused via SC2034). The SC2034 disable should be on line 63 where the variable is initialized, not here where it's being assigned. This line doesn't need a shellcheck disable comment since it's a regular assignment. Consider removing this disable comment from line 82.
| # shellcheck disable=SC2034 # legacy flag retained for help text compatibility | |
| local extra_headers=() | ||
| [[ -n "$SID" ]] && extra_headers+=(-H "X-FTL-SID: $SID") | ||
| if ! curl -sS --connect-timeout 3 --max-time 7 \ | ||
| "${extra_headers[@]}" -H "Accept: application/json" \ |
There was a problem hiding this comment.
Potential array expansion issue: When extra_headers array is empty (when SID is empty), the expansion "${extra_headers[@]}" on line 284 may cause issues in some shells or with set -u. Consider using "${extra_headers[@]+"${extra_headers[@]}"}" to safely handle the empty array case, or restructure the curl call to conditionally add the header only when SID is non-empty.
| "${extra_headers[@]}" -H "Accept: application/json" \ | |
| "${extra_headers[@]+"${extra_headers[@]}"}" -H "Accept: application/json" \ |
| cp -a /etc/pihole/pihole-FTL.db "$BACKUP_DIR" 2> /dev/null || true | ||
| cp -a /etc/pihole/custom.list "$BACKUP_DIR" 2> /dev/null || true | ||
| cp -a /etc/pihole/pihole.toml "$BACKUP_DIR" 2> /dev/null || true | ||
| cp -a /etc/pihole/hosts/*.list "$BACKUP_DIR" 2> /dev/null || true |
There was a problem hiding this comment.
Potential issue with wildcard expansion: The pattern /etc/pihole/hosts/*.list on lines 500 and 510 may fail if the /etc/pihole/hosts/ directory doesn't exist or contains no .list files. When the glob pattern doesn't match any files, cp will fail with an error (even with || true, the error message will still appear). Consider checking if the directory exists and contains files first, or use a more robust approach like find /etc/pihole/hosts/ -name "*.list" -exec cp -a {} "$BACKUP_DIR" \; 2>/dev/null || true.
1 participant
No description provided.