feat: add NetAlertX ProxmoxVE helper scripts#1491
feat: add NetAlertX ProxmoxVE helper scripts#1491JVKeller wants to merge 26 commits intonetalertx:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a Proxmox VE deployment for NetAlertX: new LXC template script, in-container installer, Proxmox README and INSTALLATION_GUIDE, nginx FastCGI env propagation, persistent runtime directories and systemd unit, bridge autodetection/selection, update routine, and a new Python dependency. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Proxmox as "Proxmox VE (host)"
participant Orchestrator as "ct/netalertx.sh"
participant Container as "LXC Container"
participant Installer as "netalertx-install.sh"
participant System as "Systemd / OS services"
participant Nginx as "NGINX & PHP-FPM"
participant App as "NetAlertX"
User->>Orchestrator: run template (local installer or mirror)
Orchestrator->>Proxmox: create/configure LXC
Orchestrator->>Container: inject installer or mirror refs
Container->>Installer: execute installation script
Installer->>System: install packages, PHP, venv, apply capabilities
Installer->>Container: clone /app, create persistent dirs, set perms
Installer->>Nginx: deploy config (FastCGI envs) and restart
Installer->>System: create start script & systemd unit, enable service
System->>App: start NetAlertX
Nginx->>App: forward requests via FastCGI
App-->>User: service reachable at host:PORT
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Fix all issues with AI agents
In `@install/proxmox/install/netalertx-install.sh`:
- Around line 199-203: The chmod -R a+rwx "$INSTALL_DIR" makes the whole install
world-writable; instead restrict write access to owner/group (www-data) and
leave others read/execute only: remove the chmod -R a+rwx line and replace it
with setting file/dir modes like u=rwX,g=rwX,o=rX for the tree and make
directories setgid so new files inherit the www-data group (e.g. set directory
mode to 2775), ensure the web-writable parts referenced (front/ and the DB file
${INSTALL_DIR}/db/app.db) are owned by or writable for www-data (chgrp/chown to
www-data and ensure group write), and keep the existing chgrp and chown commands
but apply them only to the needed paths rather than the entire INSTALL_DIR.
In `@install/proxmox/INSTALLATION_GUIDE.md`:
- Around line 68-70: The fenced code block containing the example URL
("http://YOUR_SERVER_IP:YOUR_PORT") is missing a language tag which triggers
MD040; update that fenced block by adding a language identifier (e.g., use
```text) immediately after the opening backticks so the block becomes a fenced
code block with a language tag, preserving the existing content exactly.
In `@install/proxmox/proxmox-install-netalertx.sh`:
- Around line 289-298: The mkdir step creates "${INSTALL_DIR}/log" and
"${INSTALL_DIR}/api" as root so the subsequent sudo -u www-data touch calls can
fail; after creating the directories with mkdir -p, change ownership of those
directories to www-data (chown -R www-data:www-data "${INSTALL_DIR}/log"
"${INSTALL_DIR}/api") before calling sudo -u www-data touch, or alternatively
create the directories as www-data (sudo -u www-data mkdir -p ...) so that the
later touch commands succeed; ensure chown/chmod ordering is:
mkdir/mkdir-as-www-data -> chown (if needed) -> sudo -u www-data touch -> chmod.
In `@install/proxmox/README.md`:
- Around line 14-18: Update the Proxmox VE Helper Script (Recommended) command
in the README to use the canonical helper-script URL instead of the personal
fork; replace the wget URL string
"https://github.com/JVKeller/NetAlertX/raw/proxmox-baremetal-installer/install/proxmox/ct/netalertx.sh"
used in the bash -c "$(wget -qLO - <URL>)" line with the canonical URL
referenced in INSTALLATION_GUIDE.md so the primary install path points to the
official repository.
🧹 Nitpick comments (1)
install/proxmox/install/netalertx-install.sh (1)
237-246: Deduplicate NETALERTX_TMP/PORT exports.The repeated lines add noise without changing behavior.
🧹 Suggested cleanup
export NETALERTX_TMP=/app export PORT=${PORT} -export NETALERTX_TMP=/app -export PORT=${PORT} export PYTHONPATH=/app
… and fix permissions in legacy script
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/proxmox/proxmox-install-netalertx.sh (1)
369-369:⚠️ Potential issue | 🟠 MajorSystemd service
PORTvariable will not be expanded.The heredoc on line 369 uses
'EOF'(quoted), which prevents shell variable expansion. This means${PORT}on line 392 will be written literally as${PORT}instead of the actual port number (e.g.,20211).In contrast, the start script heredoc (line 333) uses unquoted
EOF, so${PORT}expands correctly there.🔧 Suggested fix - Remove quotes from EOF delimiter
-cat > /etc/systemd/system/netalertx.service << 'EOF' +cat > /etc/systemd/system/netalertx.service << EOF [Unit] Description=NetAlertX ServiceNote: After removing the quotes, you'll also need to escape any literal
$characters in the service file that should NOT be expanded (if any).Also applies to: 392-392
🧹 Nitpick comments (2)
install/proxmox/proxmox-install-netalertx.sh (2)
336-343: Environment variables are duplicated between start script and systemd service.The
NETALERTX_*environment variables are defined in both the generated start script (lines 336-343) and the systemd service file (lines 386-393). While this redundancy ensures the variables are set regardless of how the script is invoked, consider consolidating to a single source of truth (preferably the systemd service) to simplify maintenance.Also applies to: 386-393
345-348: Self-healing__init__.pycreation may mask packaging issues.The
__init__.pyfiles for/app/front/and/app/front/plugins/do not exist in the repository but are being created at startup. This defensive measure could hide legitimate packaging problems. These files should either be committed to the repository as part of proper package structure, or their creation should be removed if they're genuinely not needed for the application's import structure.
|
looks like I made the bunny happy. |
|
Is this using hardware install instead of docker install? |
|
I was really hoping we could do something more than a debian 13 hardware install next time. I can't get ProxMox working in a VM and my servers are all reserved for other tasks. Can you try something like this and see if it works? Docker installation makes it way easier to upgrade, manage, maintain, and even uninstall if absolutely required. This project can't support hardware installation, but if we can get it to work in a container, then we can enhance user experience and provide support. Step 1: Create the ContainerWhere: Run on Proxmox Node Shell. bash -c "$(wget -qLO - https://github.com/community-scripts/ProxmoxVE/raw/main/ct/docker.sh)"
Step 2: Install NetAlertXWhere: Run inside the new Container Console. mkdir -p /opt/netalertx/data && cd /opt/netalertx
wget -qO docker-compose.yml https://raw.githubusercontent.com/netalertx/rpi-image/refs/heads/master/root_filesystem/opt/netalertx/docker-compose.yml
docker compose up -d
Access: |
|
Yes. This will install at the hardware level outside the containers. Super easy if someone wants to work on the code base! I think it just boils down to options and what different people are used to running. Docker isn't harder to use, it just uses a different tool set that a different user might want to use, just like running in HomeAssistant. With the PVE Helper scripts line this one, all you have to do is literally type "update" in the console and it takes care of it. Also, the way it's set up, the helper script just builds the LXC container, with options for an advanced config, and then calls a new version of the 'baremetal' installer to actual populate and build the environment. That script (install/proxmox/install/netalertx-install.sh) could be ran by itself on deb/Ubuntu server directly. Btw, of you want to set up Proxmox in a VM, you can start with a vanilla Debian13 container, and install Proxmox on top of it. |
|
I’m happy to keep the Proxmox/Hardware install scripts around if the community helps support them - just note there’s no official support, and Docker is still the recommended way to install. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@install/proxmox/ct/netalertx.sh`:
- Around line 33-54: After performing the sed/eval override of build_container
(both the REPOS_URL mirror branch and the LOCAL_INSTALLER branch), immediately
re-read the function (e.g., new_func=$(declare -f build_container)) and verify
it contains the expected unique substring from your replacement (for the mirror
branch check for the ${REPOS_URL}/install/proxmox/install/${var_install}.sh
pattern or another fixed marker; for the local branch check for the pct push
"/root/install.sh" or lxc-attach ... /root/install.sh fragment); if the check
fails, log a clear error via msg_error including the attempted replacement and
exit non-zero so the script does not continue using the unmodified
build_container; use the existing symbols original_func, build_container,
REPOS_URL, and LOCAL_INSTALLER to locate and validate the change.
- Around line 103-135: The update_script() currently runs a bare git pull and an
unsuppressed pip install and then always restarts the service; fix it by making
git update deterministic and error-checked (e.g., inside update_script() use git
stash push --include-untracked or run git fetch && git reset --hard origin/main
to force a known state, or explicitly restore config/app.conf after update), run
pip install while suppressing output with the existing $STD variable (activate
venv at /opt/netalertx-env/bin/activate then pip install -r
install/proxmox/requirements.txt $STD), and add error handling: check the exit
status of the git and pip commands and if either fails call msg_error and exit
without restarting the service (do not run systemctl start netalertx.service on
failure); reference update_script, git pull, pip install,
/opt/netalertx-env/bin/activate, config/app.conf, and systemctl start/stop
netalertx.service when applying the changes.
In `@install/proxmox/install/netalertx-install.sh`:
- Around line 150-157: The PHP-FPM socket detection using PHP_FPM_SOCKET=$(find
/run/php/ -name "php*-fpm.sock" | head -n 1) can run before PHP-FPM is started
and thus leave /etc/nginx/conf.d/netalertx.conf pointing to a non-existent
socket; update the install script to perform this detection after PHP-FPM is
started (or wait/retry briefly) and add a deterministic fallback (e.g.,
unix:/run/php/php8.4-fpm.sock) when PHP_FPM_SOCKET is empty so the sed
replacement against netalertx.conf always sets a valid socket; adjust the block
that calls msg_info/msg_warn and the sed invocation accordingly to use
PHP_FPM_SOCKET or the fallback.
- Around line 66-72: The REPO_URL is used unsafely in the git clone call;
validate REPO_URL (e.g., ensure it matches a safe URL/ssh/https pattern) before
use, then call git clone with a double-dash to separate options from positional
args and remove the trailing slash on INSTALL_DIR; specifically update the git
clone invocation in the script that uses REPO_URL and INSTALL_DIR to: validate
REPO_URL, then run git clone -- "$REPO_URL" "$INSTALL_DIR" (ensure quoting), and
keep the existing rm -rf "$INSTALL_DIR" and cd "$INSTALL_DIR" logic unchanged.
- Around line 101-116: Remove the setcap call that grants network capabilities
to the Python interpreter (the BINARY_PYTHON / setcap
cap_net_raw,cap_net_admin+eip invocation) to avoid elevating every Python
process; instead rely on the existing sudo configuration or create a small
dedicated wrapper with capabilities for the specific scanner commands
(nmap/arp-scan/nbtscan/traceroute). Also remove the duplicated status message by
deleting the extra msg_ok "Installed Python Dependencies" so it only prints
once. Ensure references to BINARY_PYTHON, setcap, and msg_ok are updated
accordingly.
- Around line 234-263: Remove the duplicate export lines for NETALERTX_TMP and
PORT in the startup heredoc that writes INSTALL_DIR/start.netalertx.sh
(duplicates currently set via the heredoc content referencing NETALERTX_TMP and
PORT); also decide whether runtime values should be captured at install time —
if you want the script to retain variable references for later, change the
heredoc to a literal form (use <<'EOF' or escape ${SERVER_IP}/${PORT}) so
${SERVER_IP} and ${PORT} are not expanded at install time, and add a brief
comment in the heredoc clarifying the intended behavior.
- Around line 206-212: The sudoers heredoc currently hardcodes paths; replace
those literals with the dynamically resolved path variables used earlier (e.g.,
the variables that were set when locating binaries for nmap, arp-scan, nbtscan,
traceroute, the venv python and system python — use their exact variable names
from the script such as NMAP_BIN, ARP_SCAN_BIN, NBTSCAN_BIN, TRACEROUTE_BIN,
VENV_PYTHON, PYTHON3_BIN), ensure each variable is non-empty (or fallback to
command -v) before writing /etc/sudoers.d/netalertx, and keep the existing chmod
440 and msg_ok/msg_info calls. This guarantees the sudoers file grants sudo to
the actual resolved binary paths rather than hardcoded locations.
🧹 Nitpick comments (6)
install/proxmox/install/netalertx-install.sh (3)
174-176: Unquoted brace expansion intouchcommand.Line 175 uses brace expansion
${INSTALL_DIR}/log/{app.log,...}which works, but if any path component contained spaces, it would break. More importantly, ifsudo -u www-data touchfails (e.g., permission issue), the error is silently swallowed since there's no error checking.This is a minor robustness concern given the controlled environment.
267-300: Systemd service configuration is reasonable but consider adding security hardening directives.The service runs as
www-datawhich is good. However, for a network monitoring tool, consider adding systemd sandboxing directives to limit the blast radius (e.g.,ProtectSystem=,ProtectHome=,NoNewPrivileges=).NoNewPrivileges=truewould conflict withsetcap/sudousage, so that needs careful thought, but directives likeProtectHome=yesare safe here.
22-58: Large dependency list — consider documenting why each package is needed.This installs ~30 packages. A brief inline comment grouping them by purpose (e.g.,
# Network scanning tools,# Build tools,# Runtime) would help future maintainers understand which are essential vs. optional.Minor:
tiniis typically used as a Docker init process; it's unusual in a systemd-managed LXC container. Is it actually needed here?install/proxmox/ct/netalertx.sh (3)
65-101: Bridge detection logic is thorough with good fallbacks.The multi-method approach (ip link → sysfs → pvesh) with user prompting for multiple bridges is well thought out. The error handling for no-bridge-found is appropriate.
One minor note: line 69 concatenates two sources without deduplication by interface type —
ip -o link show type bridgecould return non-vmbrbridges (e.g.,docker0,br-*). Thegrep vmbron the sysfs path filters correctly, but theipcommand output isn't filtered. Thesort -udedup on line 72 handles exact duplicates but not irrelevant bridges.Consider filtering ip output to vmbr* bridges only
- BRIDGES=($(ip -o link show type bridge | awk -F': ' '{print $2}') $(ls /sys/class/net | grep vmbr | grep -v "vmbr0")) + BRIDGES=($(ip -o link show type bridge | awk -F': ' '{print $2}' | grep '^vmbr') $(ls /sys/class/net 2>/dev/null | grep '^vmbr' | grep -v '^vmbr0$'))
12-13: Sourcing remote code from an unpinned branch over HTTPS.
source <(curl -fsSL https://...ProxmoxVE/raw/main/misc/build.func)fetches and executes arbitrary code frommainwith no integrity verification. This is standard practice for Proxmox community scripts, but be aware that any upstream compromise or breaking change will silently propagate. Consider pinning to a commit SHA for production use.
56-57: ExportingREPO_URLwithout validation.
REPO_URLis exported and eventually consumed bygit clonein the install script. If a user setsREPO_URLto a value containing shell metacharacters, it flows through unchecked. The install script side was noted separately, but the export here is the origin point.
| INSTALL_DIR="/app" | ||
| # Default repository if not specified | ||
| REPO_URL="${REPO_URL:-https://github.com/netalertx/NetAlertX.git}" | ||
| # Ensure directory is empty | ||
| rm -rf "$INSTALL_DIR" | ||
| git clone "$REPO_URL" "$INSTALL_DIR/" --quiet | ||
| cd "$INSTALL_DIR" || exit |
There was a problem hiding this comment.
Unsanitized REPO_URL is passed directly to git clone.
If REPO_URL is set by an external caller (e.g., environment variable from the CT script), a malicious value could inject arguments into the git clone command. Use -- to separate options from the positional argument, and validate the URL format.
Proposed fix
-git clone "$REPO_URL" "$INSTALL_DIR/" --quiet
+git clone --quiet -- "$REPO_URL" "$INSTALL_DIR"Also note the trailing / on "$INSTALL_DIR/" is unnecessary and inconsistent with the rest of the script.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSTALL_DIR="/app" | |
| # Default repository if not specified | |
| REPO_URL="${REPO_URL:-https://github.com/netalertx/NetAlertX.git}" | |
| # Ensure directory is empty | |
| rm -rf "$INSTALL_DIR" | |
| git clone "$REPO_URL" "$INSTALL_DIR/" --quiet | |
| cd "$INSTALL_DIR" || exit | |
| INSTALL_DIR="/app" | |
| # Default repository if not specified | |
| REPO_URL="${REPO_URL:-https://github.com/netalertx/NetAlertX.git}" | |
| # Ensure directory is empty | |
| rm -rf "$INSTALL_DIR" | |
| git clone --quiet -- "$REPO_URL" "$INSTALL_DIR" | |
| cd "$INSTALL_DIR" || exit |
🤖 Prompt for AI Agents
In `@install/proxmox/install/netalertx-install.sh` around lines 66 - 72, The
REPO_URL is used unsafely in the git clone call; validate REPO_URL (e.g., ensure
it matches a safe URL/ssh/https pattern) before use, then call git clone with a
double-dash to separate options from positional args and remove the trailing
slash on INSTALL_DIR; specifically update the git clone invocation in the script
that uses REPO_URL and INSTALL_DIR to: validate REPO_URL, then run git clone --
"$REPO_URL" "$INSTALL_DIR" (ensure quoting), and keep the existing rm -rf
"$INSTALL_DIR" and cd "$INSTALL_DIR" logic unchanged.
|
FYI @DJKatastrof , @JVKeller is working on a PR here |
install/proxmox/ct/netalertx.sh
Outdated
| # Source: https://github.com/ProxmoxVE | ||
|
|
||
| # Import main orchestrator | ||
| source <(curl -fsSL https://github.com/community-scripts/ProxmoxVE/raw/main/misc/build.func) |
There was a problem hiding this comment.
You are loading a community-scripts framework.
Therefore it is mandatory that
/ct/netalertx.sh
/install/netalertx-install.sh
are part of the community-scripts repository.
Hosting on netalertx repro will NOT work!
There was a problem hiding this comment.
The plan is to have it working in the NAX repo and then submit it to the PVE Community repo.
The script works by pulling sum sub scripts so they all stick to the same framework so it will need to link back to that repo as well.
There was a problem hiding this comment.
The read.me has instructions for an over-ride to get around the repo change for testing.
|
I have copy & paste into my community-scripts dev environment. LXC setup indicate no mayor installation errors, but netalertx.service is failing: |
|
My simple view: Split your pull request into two independant pull requests: |
…d hardcoded paths
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@install/proxmox/ct/netalertx.sh`:
- Around line 103-138: The update_script function currently hardcodes the branch
with git reset --hard origin/main and uses plain exit on the missing /app path;
change the update logic to determine the branch dynamically (e.g., detect the
current branch via git rev-parse --abbrev-ref HEAD or read a configurable
default like REPO_BRANCH/REPO_URL) and use that branch name in the git
fetch/reset commands instead of origin/main, and replace the bare exit after the
"No ${APP} Installation Found!" error with exit 1 to return a failure status.
In `@install/proxmox/install/netalertx-install.sh`:
- Around line 206-212: The current here-doc writing /etc/sudoers.d/netalertx
uses the ${BINARY_*} variables directly and can produce an invalid sudoers line
if any are empty; change the logic in netalertx-install.sh to build the
allowed-command list dynamically by collecting non-empty BINARY_NMAP,
BINARY_ARPSCAN, BINARY_NBTSCAN, BINARY_TRACEROUTE and the fixed entries
(/opt/netalertx-env/bin/python, /usr/bin/python3) into an array/string, join
with ", " and only create the sudoers content if the resulting list is
non-empty; before replacing /etc/sudoers.d/netalertx run visudo -cf on a
temporary file to validate syntax and abort (with msg_info/msg_ok) if visudo
reports an error, and only then atomically write the file and set chmod 440.
🧹 Nitpick comments (2)
install/proxmox/install/netalertx-install.sh (2)
234-263: Unquoted heredoc expands${SERVER_IP}and${PORT}at install time.Since
<<EOF(not<<'EOF') is used, these values are baked into the startup script. If the container's IP changes (e.g., DHCP), the banner message will be stale. The environment variablePORTis also expanded, so changing it later requires editing the script. This is likely intentional for a static LXC setup, but worth a comment in the script for maintainers.
267-299: Environment variables are set both in the systemd unit and instart.netalertx.sh.The
Environment=directives in the service file (lines 285–291) duplicate the exports in the startup script (lines 239–246). The startup script's exports will take precedence at runtime, making the systemd entries effectively dead config. Consider keeping them in only one place to avoid drift.
|
This command will run the installer script from my fork which I just updated from the main branch and tested. If you don't want to fork to test, use mine, or update these lines as needed. |
|
Once this PR is ready I will merge and then split it out into a separate repo, e.g. https://github.com/netalertx/NetAlertX-proxmoxve |
|
@jokob-sk I think it's ready. I have someone who can test it later to make sure. To note, the last changes I just pushed may need to be added to the docker version as well to keep it from getting IP conflicts and ARP errors... /etc/sysctl.d/99-arp-fix.conf What it does:Stops the Linux kernel from answering ARP requests for IPs that don't belong to the interface.
|
|
hi @JVKeller - thanks for the tip - added it here: https://github.com/netalertx/NetAlertX/blob/main/install/production-filesystem/entrypoint.d/37-host-optimization.sh |
|
@heinemannj with the idea of separate repos - let me know how you would like to proceed, e.g.:
|
|
I can split them and make a new pull request. I'll have to update the links in the scripts. You are moving to a new repo? |
|
Please make your changes and merge ... I will test again the PROXMOX story. But all of my previous test was failing ... |
|
No progress: PROXMOX LXC installation failed - 🧩 Using Advanced Install on node pve1 💡 PVE Version 9.1.6 (Kernel: 6.17.9-1-pve) 🚀 NetAlertX setup has been successfully initialized! root@netalertx:~# root@netalertx:~#
|
|
@heinemannj The Let me fix the links... /app is empty because it's not cloning the repo. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/proxmox/proxmox-install-netalertx.sh (1)
369-393:⚠️ Potential issue | 🟠 Major
PORTenvironment variable is written literally in the systemd unit file.Line 369 uses
<< 'EOF'(single-quoted heredoc), which prevents shell variable expansion. This causes line 392 to writeEnvironment=PORT=${PORT}as a literal string to the unit file. When systemd loads the service, it will not expand the${PORT}reference (systemd does not perform shell variable expansion in Environment directives), leaving the service with an invalid port setting.Since
PORTis exported at line 342 before the heredoc is created, it should be expanded at script execution time. Remove the quotes from the heredoc delimiter to fix this.🔧 Suggested fix
-cat > /etc/systemd/system/netalertx.service << 'EOF' +cat > /etc/systemd/system/netalertx.service << EOF [Unit] Description=NetAlertX Service ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/proxmox-install-netalertx.sh` around lines 369 - 393, The systemd unit file for netalertx.service is written using a single-quoted heredoc so ${PORT} is not expanded and the unit ends up with a literal "Environment=PORT=${PORT}"; change the heredoc to an unquoted delimiter so shell variables expand when creating the file (i.e., write the here-doc for netalertx.service without single quotes) so the exported PORT value is substituted into the Environment=PORT line; verify the netalertx.service content (Environment=PORT=...) now contains the actual port value before reloading systemd.
♻️ Duplicate comments (3)
install/proxmox/ct/netalertx.sh (1)
40-47:⚠️ Potential issue | 🟠 Major
build_containerrewriting viased+evalis too fragile without post-checks.If upstream
build_containerchanges format, these substitutions can no-op and the script continues with unintended behavior.🔍 Suggested hardening
eval "$(echo "$modified_func" | sed "s|lxc-attach|${export_header} lxc-attach|g; s|pct exec|${export_header} pct exec|g")" + new_func=$(declare -f build_container) + if ! grep -Fq "${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh" <<< "$new_func"; then + msg_error "Failed to override build_container for REPOS_URL=${REPOS_URL}" + exit 1 + fi fi ... eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")" + new_func=$(declare -f build_container) + if ! grep -Fq '/root/install.sh' <<< "$new_func"; then + msg_error "Failed to override build_container with LOCAL_INSTALLER=${LOCAL_INSTALLER}" + exit 1 + fi fiAlso applies to: 55-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 40 - 47, The sed+eval rewrite of build_container is fragile and may silently no-op if upstream changes; modify the flow to (1) verify the original_func contains the expected call sites ("lxc-attach" or "pct exec") before attempting substitions, (2) perform substitutions into modified_func as now, and then (3) assert that the resulting modified_func actually contains the injected export header string (export_header) adjacent to the exec call and the replaced REPOS_URL pattern; if any check fails, print a clear error and abort instead of eval-ing, using the existing build_container symbol names (original_func, modified_func, export_header, lxc-attach, pct exec) so the script fails fast when rewrites did not apply.install/proxmox/install/netalertx-install.sh (2)
217-219:⚠️ Potential issue | 🟠 MajorRestrict
/apppermissions; Line 218 reintroduces a world-writable install tree.
chmod -R a+rwx "$INSTALL_DIR"allows any local user/process to modify app code, config, and DB.🔒 Suggested fix
chgrp -R www-data "$INSTALL_DIR" -# NetAlertX needs write access to front/ for some features, and broad access to /app -chmod -R a+rwx "$INSTALL_DIR" +# Keep write access to owner/group only +chmod -R ug+rwX,o-rwx "$INSTALL_DIR" +# Ensure group inheritance for newly created directories/files +find "$INSTALL_DIR" -type d -exec chmod g+s {} + chown -R www-data:www-data "${INSTALL_DIR}/db/app.db"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/install/netalertx-install.sh` around lines 217 - 219, The current chmod -R a+rwx "$INSTALL_DIR" makes the whole install tree world-writable; replace this with a minimum-privilege approach: ensure the app tree is owned by the web user (use the existing chown for the DB and extend to "${INSTALL_DIR}"), set directory permissions to 755 and regular file permissions to 644 (or use u=rwX,go=rX semantics) and set the DB file to 660 owned by www-data:www-data; specifically update the chmod/chown usage around INSTALL_DIR and the existing chown for "${INSTALL_DIR}/db/app.db" to apply these tighter permissions instead of a+rwx.
165-172:⚠️ Potential issue | 🟠 MajorPHP-FPM socket handling can still leave NGINX pointed at a stale upstream.
When the socket is absent at Line 167 (common before php-fpm starts), the script only warns, then never reapplies socket replacement after Line 252 starts php-fpm.
🔧 Suggested fix
-# Postpone PHP-FPM socket detection until after service start, or use a fallback. -# For now, we configure a default and assume the standard Debian 13/Ubuntu 24 location. -if [ -S "/run/php/php8.4-fpm.sock" ]; then - sed -i "s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" /etc/nginx/conf.d/netalertx.conf -else - # Fallback pattern for detection during startup if possible - msg_warn "PHP-FPM socket not found at standard location, will rely on service startup" -fi +# Use deterministic socket path for this installer target +sed -i "s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" /etc/nginx/conf.d/netalertx.conf ... systemctl enable php8.4-fpm systemctl start php8.4-fpm +systemctl restart nginx msg_ok "Started PHP-FPM"Also applies to: 250-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/install/netalertx-install.sh` around lines 165 - 172, The current logic updates /etc/nginx/conf.d/netalertx.conf only if /run/php/php8.4-fpm.sock exists and otherwise just msg_warns, which can leave NGINX pointed at a stale upstream; after the script starts PHP-FPM (the block that starts php-fpm around the later start/daemon step), re-check for the socket and re-run the same sed replacement used earlier (the sed "s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" against netalertx.conf) and then reload nginx (systemctl reload nginx or nginx -s reload) so the replacement is applied when the socket appears—add this retry/check immediately after the php-fpm start block and keep the msg_warn for the initial absence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/proxmox/ct/netalertx.sh`:
- Line 50: The LOCAL_INSTALLER path uses an extra "install" segment causing it
to resolve to .../install/install/...; update the assignment for LOCAL_INSTALLER
to remove the redundant "/install" so it points to the top-level installer (i.e.
change the suffix from "/../../install/${NSAPP:-netalertx}-install.sh" to
"/../../${NSAPP:-netalertx}-install.sh"), keeping the existing use of
LOCAL_INSTALLER and NSAPP unchanged.
- Around line 43-47: The code injects unescaped REPOS_URL, REPO_URL, and
REPO_BRANCH into strings passed to eval (see export_header, modified_func and
the eval of modified_func), enabling command injection; fix by shell-escaping
these env variables before interpolation (use a safe quoting/escaping utility
like printf '%q' or a proper escape function) and apply the same escaping when
constructing the replacement variable used with sed/pct exec/lxc-attach so no
untrusted characters can terminate the export statement or inject commands;
update export_header, modified_func (and any replacement variable) to use the
escaped versions of REPOS_URL/REPO_URL/REPO_BRANCH.
In `@install/proxmox/install/netalertx-install.sh`:
- Around line 224-234: The sudoers snippet currently includes Python
interpreters via SUDO_CMDS (initialized to "/opt/netalertx-env/bin/python,
/usr/bin/python3"), which grants www-data passwordless root via arbitrary Python
execution; remove those interpreter paths from SUDO_CMDS and instead whitelist
only the specific scanner binaries (BINARY_NMAP, BINARY_ARPSCAN, BINARY_NBTSCAN,
BINARY_TRACEROUTE) or a single root-owned wrapper script with a fixed path and
fixed args if privileged Python functionality is absolutely required (do not
allow the interpreter itself). Update the SUDO_CMDS initialization and the
generated /etc/sudoers.d/netalertx.tmp content to reference the wrapper script
path (owned by root) or the explicit binaries, and ensure no raw python
interpreter paths remain in SUDO_CMDS.
---
Outside diff comments:
In `@install/proxmox/proxmox-install-netalertx.sh`:
- Around line 369-393: The systemd unit file for netalertx.service is written
using a single-quoted heredoc so ${PORT} is not expanded and the unit ends up
with a literal "Environment=PORT=${PORT}"; change the heredoc to an unquoted
delimiter so shell variables expand when creating the file (i.e., write the
here-doc for netalertx.service without single quotes) so the exported PORT value
is substituted into the Environment=PORT line; verify the netalertx.service
content (Environment=PORT=...) now contains the actual port value before
reloading systemd.
---
Duplicate comments:
In `@install/proxmox/ct/netalertx.sh`:
- Around line 40-47: The sed+eval rewrite of build_container is fragile and may
silently no-op if upstream changes; modify the flow to (1) verify the
original_func contains the expected call sites ("lxc-attach" or "pct exec")
before attempting substitions, (2) perform substitutions into modified_func as
now, and then (3) assert that the resulting modified_func actually contains the
injected export header string (export_header) adjacent to the exec call and the
replaced REPOS_URL pattern; if any check fails, print a clear error and abort
instead of eval-ing, using the existing build_container symbol names
(original_func, modified_func, export_header, lxc-attach, pct exec) so the
script fails fast when rewrites did not apply.
In `@install/proxmox/install/netalertx-install.sh`:
- Around line 217-219: The current chmod -R a+rwx "$INSTALL_DIR" makes the whole
install tree world-writable; replace this with a minimum-privilege approach:
ensure the app tree is owned by the web user (use the existing chown for the DB
and extend to "${INSTALL_DIR}"), set directory permissions to 755 and regular
file permissions to 644 (or use u=rwX,go=rX semantics) and set the DB file to
660 owned by www-data:www-data; specifically update the chmod/chown usage around
INSTALL_DIR and the existing chown for "${INSTALL_DIR}/db/app.db" to apply these
tighter permissions instead of a+rwx.
- Around line 165-172: The current logic updates
/etc/nginx/conf.d/netalertx.conf only if /run/php/php8.4-fpm.sock exists and
otherwise just msg_warns, which can leave NGINX pointed at a stale upstream;
after the script starts PHP-FPM (the block that starts php-fpm around the later
start/daemon step), re-check for the socket and re-run the same sed replacement
used earlier (the sed
"s|unix:/var/run/php/php-fpm.sock;|unix:/run/php/php8.4-fpm.sock;|g" against
netalertx.conf) and then reload nginx (systemctl reload nginx or nginx -s
reload) so the replacement is applied when the socket appears—add this
retry/check immediately after the php-fpm start block and keep the msg_warn for
the initial absence.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
install/proxmox/ct/netalertx.shinstall/proxmox/install/netalertx-install.shinstall/proxmox/proxmox-install-netalertx.shinstall/proxmox/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- install/proxmox/requirements.txt
| fi | ||
|
|
||
| # Define local installer path for testing | ||
| LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh" |
There was a problem hiding this comment.
LOCAL_INSTALLER path is off by one directory level.
From install/proxmox/ct/netalertx.sh, Line 50 resolves to .../install/install/<name>-install.sh, so the local-installer override can be skipped unintentionally.
🔧 Suggested fix
-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh"
+LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/proxmox/ct/netalertx.sh` at line 50, The LOCAL_INSTALLER path uses an
extra "install" segment causing it to resolve to .../install/install/...; update
the assignment for LOCAL_INSTALLER to remove the redundant "/install" so it
points to the top-level installer (i.e. change the suffix from
"/../../install/${NSAPP:-netalertx}-install.sh" to
"/../../${NSAPP:-netalertx}-install.sh"), keeping the existing use of
LOCAL_INSTALLER and NSAPP unchanged.
| SUDO_CMDS="/opt/netalertx-env/bin/python, /usr/bin/python3" | ||
| for cmd in "$BINARY_NMAP" "$BINARY_ARPSCAN" "$BINARY_NBTSCAN" "$BINARY_TRACEROUTE"; do | ||
| if [[ -n "$cmd" ]]; then | ||
| SUDO_CMDS="${SUDO_CMDS}, ${cmd}" | ||
| fi | ||
| done | ||
|
|
||
| # Write to temp file for validation | ||
| cat > /etc/sudoers.d/netalertx.tmp <<EOF | ||
| www-data ALL=(ALL) NOPASSWD: ${SUDO_CMDS} | ||
| EOF |
There was a problem hiding this comment.
Do not grant passwordless sudo to python interpreters.
Line 224 + Line 233 allow www-data to run arbitrary Python as root, which is effectively full root escalation if the web process is compromised.
🛡️ Suggested fix
-SUDO_CMDS="/opt/netalertx-env/bin/python, /usr/bin/python3"
+SUDO_CMDS=""
for cmd in "$BINARY_NMAP" "$BINARY_ARPSCAN" "$BINARY_NBTSCAN" "$BINARY_TRACEROUTE"; do
if [[ -n "$cmd" ]]; then
SUDO_CMDS="${SUDO_CMDS}, ${cmd}"
fi
doneIf privileged Python execution is truly required, whitelist a root-owned wrapper script with fixed command/args instead of the interpreter binary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SUDO_CMDS="/opt/netalertx-env/bin/python, /usr/bin/python3" | |
| for cmd in "$BINARY_NMAP" "$BINARY_ARPSCAN" "$BINARY_NBTSCAN" "$BINARY_TRACEROUTE"; do | |
| if [[ -n "$cmd" ]]; then | |
| SUDO_CMDS="${SUDO_CMDS}, ${cmd}" | |
| fi | |
| done | |
| # Write to temp file for validation | |
| cat > /etc/sudoers.d/netalertx.tmp <<EOF | |
| www-data ALL=(ALL) NOPASSWD: ${SUDO_CMDS} | |
| EOF | |
| SUDO_CMDS="" | |
| for cmd in "$BINARY_NMAP" "$BINARY_ARPSCAN" "$BINARY_NBTSCAN" "$BINARY_TRACEROUTE"; do | |
| if [[ -n "$cmd" ]]; then | |
| SUDO_CMDS="${SUDO_CMDS:+$SUDO_CMDS, }${cmd}" | |
| fi | |
| done | |
| # Write to temp file for validation | |
| cat > /etc/sudoers.d/netalertx.tmp <<EOF | |
| www-data ALL=(ALL) NOPASSWD: ${SUDO_CMDS} | |
| EOF |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/proxmox/install/netalertx-install.sh` around lines 224 - 234, The
sudoers snippet currently includes Python interpreters via SUDO_CMDS
(initialized to "/opt/netalertx-env/bin/python, /usr/bin/python3"), which grants
www-data passwordless root via arbitrary Python execution; remove those
interpreter paths from SUDO_CMDS and instead whitelist only the specific scanner
binaries (BINARY_NMAP, BINARY_ARPSCAN, BINARY_NBTSCAN, BINARY_TRACEROUTE) or a
single root-owned wrapper script with a fixed path and fixed args if privileged
Python functionality is absolutely required (do not allow the interpreter
itself). Update the SUDO_CMDS initialization and the generated
/etc/sudoers.d/netalertx.tmp content to reference the wrapper script path (owned
by root) or the explicit binaries, and ensure no raw python interpreter paths
remain in SUDO_CMDS.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
install/proxmox/ct/netalertx.sh (3)
66-67:⚠️ Potential issue | 🔴 CriticalSame command injection vulnerability in local installer override.
The
replacementvariable also interpolates unescapedREPOS_URL,REPO_URL, andREPO_BRANCHinto anevalcontext.🔒 Proposed fix
msg_info "Using local installer from $LOCAL_INSTALLER" original_func=$(declare -f build_container) - replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" + printf -v q_REPOS_URL '%q' "${REPOS_URL:-}" + printf -v q_REPO_URL '%q' "${REPO_URL:-}" + printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}" + replacement="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 66 - 67, The replacement string assigned to replacement and then injected into eval (via eval "$(echo "$original_func" | sed ... )") is vulnerable because REPOS_URL, REPO_URL and REPO_BRANCH are interpolated unescaped; fix by stopping direct interpolation into the eval string: build the command without expanding those variables (use quoted placeholders) or use a safe-quoting mechanism (e.g. shell-quote/printf %q) when composing replacement, and update the sed substitution that references original_func so it inserts the safely-quoted/placeholdered values instead of raw REPOS_URL/REPO_URL/REPO_BRANCH; ensure the final command executed by eval only receives properly quoted values or, better, avoid eval and call pct/lxc-attach with arguments assembled as arrays or via a here-doc to eliminate command injection risks.
60-60:⚠️ Potential issue | 🟠 Major
LOCAL_INSTALLERpath resolves incorrectly.From
install/proxmox/ct/, going../../install/lands atinstall/install/, notinstall/proxmox/install/. The local installer override will never trigger because the file won't be found at the wrong path.🔧 Proposed fix
-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh" +LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` at line 60, The LOCAL_INSTALLER path in install/proxmox/ct/netalertx.sh is resolving to the wrong directory (../../install/... yields install/install/) so the override never finds the installer; update the assignment for LOCAL_INSTALLER to point to the proxmox install directory (use ../install/${NSAPP:-netalertx}-install.sh) by changing the path expression around the LOCAL_INSTALLER variable in netalertx.sh so it looks up one directory (../install) instead of two (../../install).
53-56:⚠️ Potential issue | 🔴 CriticalUnescaped environment variables in
evalcontext enable command injection.
REPOS_URL,REPO_URL, andREPO_BRANCHare interpolated directly into strings passed toeval. A crafted value like"; malicious_command && #breaks out of the export statement and executes arbitrary shell code on the Proxmox host.🔒 Proposed fix using printf escaping
if [[ -n "${REPOS_URL}" ]]; then # Only show info message in verbose mode to avoid UI overlap if [[ "${VERBOSE:-no}" == "yes" ]]; then msg_info "Using custom repository: ${REPOS_URL}" fi # Override build_container to pass environment variables and use the custom repo URL original_func=$(declare -f build_container) - # Prepend exports to the execution command within build_container - # This ensures REPOS_URL, REPO_URL, and REPO_BRANCH are available inside the container during install. - export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};" + # Safely escape variables before interpolation + printf -v q_REPOS_URL '%q' "${REPOS_URL:-}" + printf -v q_REPO_URL '%q' "${REPO_URL:-}" + printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}" + export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};" modified_func=$(echo "$original_func" | sed "s|https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main/install/\${var_install}.sh|${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh|g") # Inject the exports before the lxc-attach/pct exec call eval "$(echo "$modified_func" | sed "s|lxc-attach|${export_header} lxc-attach|g; s|pct exec|${export_header} pct exec|g")" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 53 - 56, The code builds export_header and modified_func by directly interpolating REPOS_URL/REPO_URL/REPO_BRANCH and then passes the result to eval, which allows command injection; fix by first escaping those env values (e.g., use printf %q into safe_REPOS_URL, safe_REPO_URL, safe_REPO_BRANCH) and use the escaped variables when constructing export_header and when performing the sed replacements for modified_func, and eliminate the unsafe eval by writing modified_func to a temp file and sourcing it or invoking it with bash -c with the function body passed as a single safe argument; update the usages of export_header, modified_func, original_func and the eval call accordingly.
🧹 Nitpick comments (1)
install/proxmox/ct/netalertx.sh (1)
93-93: Consider usingjqfor JSON parsing if available.The
grep -oPpattern for extracting bridge names from JSON is fragile if the output format changes. Sincejqis used elsewhere in the script ecosystem and is commonly available on Proxmox, it would be more robust.♻️ Proposed alternative with jq fallback
if command -v pvesh >/dev/null 2>&1; then - # shellcheck disable=SC2207,SC2046 # Working pattern for pvesh output - BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | grep -oP '"iface":"\K[^"]+')) + # shellcheck disable=SC2207,SC2046 # Working pattern for pvesh output + if command -v jq >/dev/null 2>&1; then + BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | jq -r '.[].iface')) + else + BRIDGES=($(pvesh get /nodes/$(hostname)/network --type bridge --output-format json | grep -oP '"iface":"\K[^"]+')) + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` at line 93, Replace the fragile grep extraction used when setting BRIDGES by using jq if available: detect jq with a command -v check, then run the existing pvesh get /nodes/$(hostname)/network --output-format json and pipe to jq -r '.[].iface' (or the appropriate JSON path) to populate the BRIDGES array; if jq is missing fall back to the current grep -oP '"iface":"\K[^"]+' pipeline. Update the BRIDGES assignment and any error handling so the script uses the jq result when present and the grep fallback otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@install/proxmox/ct/netalertx.sh`:
- Around line 66-67: The replacement string assigned to replacement and then
injected into eval (via eval "$(echo "$original_func" | sed ... )") is
vulnerable because REPOS_URL, REPO_URL and REPO_BRANCH are interpolated
unescaped; fix by stopping direct interpolation into the eval string: build the
command without expanding those variables (use quoted placeholders) or use a
safe-quoting mechanism (e.g. shell-quote/printf %q) when composing replacement,
and update the sed substitution that references original_func so it inserts the
safely-quoted/placeholdered values instead of raw
REPOS_URL/REPO_URL/REPO_BRANCH; ensure the final command executed by eval only
receives properly quoted values or, better, avoid eval and call pct/lxc-attach
with arguments assembled as arrays or via a here-doc to eliminate command
injection risks.
- Line 60: The LOCAL_INSTALLER path in install/proxmox/ct/netalertx.sh is
resolving to the wrong directory (../../install/... yields install/install/) so
the override never finds the installer; update the assignment for
LOCAL_INSTALLER to point to the proxmox install directory (use
../install/${NSAPP:-netalertx}-install.sh) by changing the path expression
around the LOCAL_INSTALLER variable in netalertx.sh so it looks up one directory
(../install) instead of two (../../install).
- Around line 53-56: The code builds export_header and modified_func by directly
interpolating REPOS_URL/REPO_URL/REPO_BRANCH and then passes the result to eval,
which allows command injection; fix by first escaping those env values (e.g.,
use printf %q into safe_REPOS_URL, safe_REPO_URL, safe_REPO_BRANCH) and use the
escaped variables when constructing export_header and when performing the sed
replacements for modified_func, and eliminate the unsafe eval by writing
modified_func to a temp file and sourcing it or invoking it with bash -c with
the function body passed as a single safe argument; update the usages of
export_header, modified_func, original_func and the eval call accordingly.
---
Nitpick comments:
In `@install/proxmox/ct/netalertx.sh`:
- Line 93: Replace the fragile grep extraction used when setting BRIDGES by
using jq if available: detect jq with a command -v check, then run the existing
pvesh get /nodes/$(hostname)/network --output-format json and pipe to jq -r
'.[].iface' (or the appropriate JSON path) to populate the BRIDGES array; if jq
is missing fall back to the current grep -oP '"iface":"\K[^"]+' pipeline. Update
the BRIDGES assignment and any error handling so the script uses the jq result
when present and the grep fallback otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
install/proxmox/ct/netalertx.sh (3)
32-36:⚠️ Potential issue | 🟠 MajorAdd a post-override assertion so function patching cannot fail silently.
At Line 35 and Line 46,
build_containeris rewritten by pattern-matching its body. If upstream formatting changes, the rewrite can no-op and continue with unintended install behavior. Fail fast after each override.Suggested hardening
if [[ -n "${REPOS_URL}" ]]; then # Override build_container surgically by replacing ONLY the URL string. # This is much safer than re-declaring the whole function. source <(declare -f build_container | sed "s|https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main/install/\${var_install}.sh|${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh|g") + if ! declare -f build_container | grep -Fq '/install/proxmox/install/${var_install}-install.sh'; then + msg_error "Failed to override build_container for REPOS_URL=${REPOS_URL}" + exit 1 + fi fi @@ if [[ -f "$LOCAL_INSTALLER" ]]; then msg_info "Using local installer from $LOCAL_INSTALLER" original_func=$(declare -f build_container) replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")" + if ! declare -f build_container | grep -Fq '/root/install.sh'; then + msg_error "Failed to override build_container for LOCAL_INSTALLER=${LOCAL_INSTALLER}" + exit 1 + fi fiAlso applies to: 42-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 32 - 36, The override of the build_container function via source <(declare -f build_container | sed ...) can silently no-op if the sed pattern fails; after each such override (the blocks that reference build_container and ${REPOS_URL}/${var_install}), add a post-override assertion that verifies the function was actually patched—e.g., capture declare -f build_container and test that the body contains the new ${REPOS_URL}/install/proxmox/install/${var_install}-install.sh URL (or otherwise differs from the original), and if the check fails emit a clear error (mentioning build_container and REPOS_URL/var_install) and exit non-zero so the script fails fast instead of continuing with an unmodified function.
45-46:⚠️ Potential issue | 🔴 CriticalEscape repo variables before injecting into eval-generated function content.
At Line 45-Line 46,
REPOS_URL/REPO_URL/REPO_BRANCHare interpolated unescaped into content later executed viaeval. Crafted values can inject commands into the generated function body.Minimal escaping fix
if [[ -f "$LOCAL_INSTALLER" ]]; then msg_info "Using local installer from $LOCAL_INSTALLER" original_func=$(declare -f build_container) - replacement="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" + printf -v q_REPOS_URL '%q' "${REPOS_URL:-}" + printf -v q_REPO_URL '%q' "${REPO_URL:-}" + printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}" + replacement="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH}; pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 45 - 46, The replacement currently injects unescaped REPOS_URL/REPO_URL/REPO_BRANCH into an eval-generated function (via original_func, replacement, eval and sed), allowing command injection; fix by escaping those variables before building replacement (e.g. compute safe_REPOS_URL=$(printf '%q' "$REPOS_URL") and likewise for REPO_URL and REPO_BRANCH) and use the escaped names when composing the replacement string so pct push / lxc-attach receive only quoted/escaped values instead of raw interpolated input. Ensure you update the code that constructs replacement (the sed/eval step) to reference the escaped variables rather than the original ones.
39-39:⚠️ Potential issue | 🟠 MajorFix LOCAL_INSTALLER path resolution (currently points to
install/install/...).At Line 39, from
install/proxmox/ct, using../../install/...resolves to<repo>/install/install/..., so the local override can be skipped even when the installer exists.Path fix
-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh" +LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` at line 39, The LOCAL_INSTALLER path wrongly includes an extra "install" segment causing install/install/...; update the assignment in install/proxmox/ct/netalertx.sh (the LOCAL_INSTALLER variable) to point to the correct relative path by removing the redundant "install" directory, i.e. use ../../${NSAPP:-netalertx}-install.sh so it resolves to repo/install/<name>-install.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/proxmox/ct/netalertx.sh`:
- Around line 107-127: The script stops netalertx.service before performing git
and pip steps (see systemctl stop netalertx.service, git fetch/reset, and pip
install) but uses exit 1 on failures, which can leave the service down; modify
the update block to ensure the service is always restarted on any failure or at
script exit—add a trap or a finally-style cleanup that calls systemctl start
netalertx.service (and optionally logs via msg_info/msg_ok) and invoke that
restart both on error paths after failed git fetch/reset or pip install and at
normal completion; ensure BRANCH detection and virtualenv
activation/deactivation remain the same but do not use immediate exit 1 without
first attempting to restart the service.
---
Duplicate comments:
In `@install/proxmox/ct/netalertx.sh`:
- Around line 32-36: The override of the build_container function via source
<(declare -f build_container | sed ...) can silently no-op if the sed pattern
fails; after each such override (the blocks that reference build_container and
${REPOS_URL}/${var_install}), add a post-override assertion that verifies the
function was actually patched—e.g., capture declare -f build_container and test
that the body contains the new
${REPOS_URL}/install/proxmox/install/${var_install}-install.sh URL (or otherwise
differs from the original), and if the check fails emit a clear error
(mentioning build_container and REPOS_URL/var_install) and exit non-zero so the
script fails fast instead of continuing with an unmodified function.
- Around line 45-46: The replacement currently injects unescaped
REPOS_URL/REPO_URL/REPO_BRANCH into an eval-generated function (via
original_func, replacement, eval and sed), allowing command injection; fix by
escaping those variables before building replacement (e.g. compute
safe_REPOS_URL=$(printf '%q' "$REPOS_URL") and likewise for REPO_URL and
REPO_BRANCH) and use the escaped names when composing the replacement string so
pct push / lxc-attach receive only quoted/escaped values instead of raw
interpolated input. Ensure you update the code that constructs replacement (the
sed/eval step) to reference the escaped variables rather than the original ones.
- Line 39: The LOCAL_INSTALLER path wrongly includes an extra "install" segment
causing install/install/...; update the assignment in
install/proxmox/ct/netalertx.sh (the LOCAL_INSTALLER variable) to point to the
correct relative path by removing the redundant "install" directory, i.e. use
../../${NSAPP:-netalertx}-install.sh so it resolves to
repo/install/<name>-install.sh.
| msg_info "Stopping ${APP} Service" | ||
| systemctl stop netalertx.service | ||
| msg_ok "Stopped ${APP} Service" | ||
|
|
||
| msg_info "Updating ${APP}" | ||
| cd /app || exit 1 | ||
| # Get current branch (default to main if detection fails) | ||
| BRANCH=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "main") | ||
|
|
||
| # Ensure clean state before pulling from the detected branch | ||
| git fetch origin "${BRANCH}" || exit 1 | ||
| git reset --hard "origin/${BRANCH}" || exit 1 | ||
| msg_ok "Updated ${APP} (Branch: ${BRANCH})" | ||
|
|
||
| msg_info "Updating Python Dependencies" | ||
| # shellcheck disable=SC1091 # venv activation script | ||
| source /opt/netalertx-env/bin/activate | ||
| # Suppress pip output unless verbose | ||
| $STD pip install -r install/proxmox/requirements.txt || exit 1 | ||
| deactivate | ||
| msg_ok "Updated Python Dependencies" |
There was a problem hiding this comment.
Don’t leave the service down on update failure.
At Line 107 onward, the service is stopped before git/pip operations, and failure paths exit 1 immediately. A transient fetch/pip error can leave NetAlertX offline until manual intervention.
Fail-safe restart pattern
function update_script() {
header_info
check_container_storage
check_container_resources
+ local restart_needed=0
@@
msg_info "Stopping ${APP} Service"
- systemctl stop netalertx.service
+ systemctl stop netalertx.service || exit 1
+ restart_needed=1
+ trap '[[ "$restart_needed" -eq 1 ]] && systemctl start netalertx.service >/dev/null 2>&1 || true' EXIT
msg_ok "Stopped ${APP} Service"
@@
msg_info "Starting ${APP} Service"
systemctl start netalertx.service
+ restart_needed=0
+ trap - EXIT
msg_ok "Started ${APP} Service"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/proxmox/ct/netalertx.sh` around lines 107 - 127, The script stops
netalertx.service before performing git and pip steps (see systemctl stop
netalertx.service, git fetch/reset, and pip install) but uses exit 1 on
failures, which can leave the service down; modify the update block to ensure
the service is always restarted on any failure or at script exit—add a trap or a
finally-style cleanup that calls systemctl start netalertx.service (and
optionally logs via msg_info/msg_ok) and invoke that restart both on error paths
after failed git fetch/reset or pip install and at normal completion; ensure
BRANCH detection and virtualenv activation/deactivation remain the same but do
not use immediate exit 1 without first attempting to restart the service.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
install/proxmox/ct/netalertx.sh (4)
111-131:⚠️ Potential issue | 🟠 MajorAvoid leaving
netalertx.servicedown on update failure.Line 112 stops the service, but failure paths in Line 121/122/129
exit 1before restart. Add a trap/finally-style restart guard.🛟 Proposed fail-safe pattern
function update_script() { header_info check_container_storage check_container_resources + local restart_needed=0 @@ msg_info "Stopping ${APP} Service" - systemctl stop netalertx.service + systemctl stop netalertx.service || exit 1 + restart_needed=1 + trap '[[ "$restart_needed" -eq 1 ]] && systemctl start netalertx.service >/dev/null 2>&1 || true' EXIT msg_ok "Stopped ${APP} Service" @@ $STD pip install -r install/proxmox/requirements.txt || exit 1 deactivate msg_ok "Updated Python Dependencies" @@ msg_info "Starting ${APP} Service" systemctl start netalertx.service + restart_needed=0 + trap - EXIT msg_ok "Started ${APP} Service"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 111 - 131, The script currently stops netalertx.service (systemctl stop netalertx.service) early and exits on failures (git fetch/reset, pip install) which can leave the service down; wrap the update block with a fail-safe that always attempts to restart the service (systemctl start netalertx.service) on any exit or error—implement a trap or finally-style cleanup around the sequence that includes BRANCH detection, git fetch/reset, venv activation (source /opt/netalertx-env/bin/activate), pip install ($STD pip install ...), and deactivate so that regardless of which command fails the trap will restart netalertx.service and return the original exit code; reference netalertx.service, BRANCH, git fetch/reset, source /opt/netalertx-env/bin/activate, and $STD pip install when adding the guard.
35-39:⚠️ Potential issue | 🔴 CriticalEscape repo variables before
evalto prevent command injection.Line 35 and Line 48 interpolate
REPOS_URL/REPO_URL/REPO_BRANCHdirectly into strings later executed byeval(Line 38 and Line 50). If those values are attacker-controlled, this is host-side command execution.🔐 Proposed fix
if [[ -n "${REPOS_URL}" ]]; then + printf -v q_REPOS_URL '%q' "${REPOS_URL:-}" + printf -v q_REPO_URL '%q' "${REPO_URL:-}" + printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}" # Inject exports and override the installer URL surgically. # This pattern matches the exact curl -fsSL call in build.func - export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};" + export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};" original_func=$(declare -f build_container) modified_func=$(echo "$original_func" | sed "s|bash -c \"\$(curl -fsSL [^\"]*)|bash -c \"${export_header} \$(curl -fsSL ${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh)|g") eval "$modified_func" fi @@ if [[ -f "$LOCAL_INSTALLER" ]]; then msg_info "Using local installer from $LOCAL_INSTALLER" original_func=$(declare -f build_container) - export_header="export REPOS_URL=${REPOS_URL}; export REPO_URL=${REPO_URL}; export REPO_BRANCH=${REPO_BRANCH};" + printf -v q_REPOS_URL '%q' "${REPOS_URL:-}" + printf -v q_REPO_URL '%q' "${REPO_URL:-}" + printf -v q_REPO_BRANCH '%q' "${REPO_BRANCH:-}" + export_header="export REPOS_URL=${q_REPOS_URL}; export REPO_URL=${q_REPO_URL}; export REPO_BRANCH=${q_REPO_BRANCH};" replacement="${export_header} pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")" fi#!/bin/bash set -euo pipefail file="install/proxmox/ct/netalertx.sh" echo "Inspecting eval-related interpolation:" nl -ba "$file" | sed -n '32,52p' echo echo "Checking whether shell-escaping is used before eval:" rg -n 'printf -v q_REPOS_URL|printf -v q_REPO_URL|printf -v q_REPO_BRANCH|export_header=.*REPOS_URL=.*REPO_URL=.*REPO_BRANCH=.*|eval "\$modified_func"|eval "\$\(echo \"\$original_func\" \| sed' "$file"Also applies to: 48-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 35 - 39, The export_header and modified_func interpolation (variables export_header, original_func, modified_func, and the later eval of modified_func inside build_container) inject unescaped REPOS_URL/REPO_URL/REPO_BRANCH into code passed to eval; fix by shell-escaping/quoting those variables before embedding them (e.g., create escaped versions like q_REPOS_URL/q_REPO_URL/q_REPO_BRANCH using printf -v ... %q or an equivalent safe-quoting routine) and use the escaped names when constructing export_header and when substituting into modified_func so the eval only executes safe, quoted strings rather than attacker-controlled content.
36-38:⚠️ Potential issue | 🟠 Major
sed-rewritingbuild_containeris brittle without post-check validation.The override silently falls back to upstream behavior if the
sedpattern no longer matches. Add an immediate verification after each override and fail fast if replacement did not apply.🧩 Proposed hardening
modified_func=$(echo "$original_func" | sed "s|bash -c \"\$(curl -fsSL [^\"]*)|bash -c \"${export_header} \$(curl -fsSL ${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh)|g") eval "$modified_func" + new_func=$(declare -f build_container) + if ! grep -Fq "${REPOS_URL}/install/proxmox/install/\${var_install}-install.sh" <<< "$new_func"; then + msg_error "Failed to override build_container for REPOS_URL mirror." + exit 1 + fi fi @@ replacement="${export_header} pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh && lxc-attach -n \"\$CTID\" -- bash /root/install.sh" eval "$(echo "$original_func" | sed "s|lxc-attach.*install/\${var_install}.sh.*|$replacement|")" + new_func=$(declare -f build_container) + if ! grep -Fq "pct push \"\$CTID\" \"$LOCAL_INSTALLER\" /root/install.sh" <<< "$new_func"; then + msg_error "Failed to override build_container for LOCAL_INSTALLER." + exit 1 + fi fiAlso applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` around lines 36 - 38, The sed-based override of build_container is brittle because if the sed pattern doesn't match it silently leaves the function unchanged; after creating modified_func (and the second override at the other occurrence), immediately verify the replacement actually applied (for example by comparing modified_func to original_func or grepping for the injected ${export_header} / ${REPOS_URL} token) and if the check fails log a clear error message and exit non‑zero to fail fast; apply this verification for the build_container override at the current block and the similar block at the other occurrence (lines 47-50) and ensure eval is only run when the verification passes.
42-42:⚠️ Potential issue | 🟠 MajorFix
LOCAL_INSTALLERpath (currently resolves toinstall/install/...).From
install/proxmox/ct, Line 42 currently builds an extrainstallpath segment and can skip the local-installer override unintentionally.📌 Proposed fix
-LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../../install/${NSAPP:-netalertx}-install.sh" +LOCAL_INSTALLER="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../install/${NSAPP:-netalertx}-install.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/proxmox/ct/netalertx.sh` at line 42, LOCAL_INSTALLER currently appends an extra /install segment causing paths like install/install/...; change the concatenation so it targets the installer file in the top-level install directory from the script's location by removing the redundant "/install" suffix—update the LOCAL_INSTALLER assignment (the variable name LOCAL_INSTALLER in the install/proxmox/ct script) to use .../../../${NSAPP:-netalertx}-install.sh instead of .../../../install/${NSAPP:-netalertx}-install.sh so the override works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@install/proxmox/ct/netalertx.sh`:
- Around line 111-131: The script currently stops netalertx.service (systemctl
stop netalertx.service) early and exits on failures (git fetch/reset, pip
install) which can leave the service down; wrap the update block with a
fail-safe that always attempts to restart the service (systemctl start
netalertx.service) on any exit or error—implement a trap or finally-style
cleanup around the sequence that includes BRANCH detection, git fetch/reset,
venv activation (source /opt/netalertx-env/bin/activate), pip install ($STD pip
install ...), and deactivate so that regardless of which command fails the trap
will restart netalertx.service and return the original exit code; reference
netalertx.service, BRANCH, git fetch/reset, source
/opt/netalertx-env/bin/activate, and $STD pip install when adding the guard.
- Around line 35-39: The export_header and modified_func interpolation
(variables export_header, original_func, modified_func, and the later eval of
modified_func inside build_container) inject unescaped
REPOS_URL/REPO_URL/REPO_BRANCH into code passed to eval; fix by
shell-escaping/quoting those variables before embedding them (e.g., create
escaped versions like q_REPOS_URL/q_REPO_URL/q_REPO_BRANCH using printf -v ...
%q or an equivalent safe-quoting routine) and use the escaped names when
constructing export_header and when substituting into modified_func so the eval
only executes safe, quoted strings rather than attacker-controlled content.
- Around line 36-38: The sed-based override of build_container is brittle
because if the sed pattern doesn't match it silently leaves the function
unchanged; after creating modified_func (and the second override at the other
occurrence), immediately verify the replacement actually applied (for example by
comparing modified_func to original_func or grepping for the injected
${export_header} / ${REPOS_URL} token) and if the check fails log a clear error
message and exit non‑zero to fail fast; apply this verification for the
build_container override at the current block and the similar block at the other
occurrence (lines 47-50) and ensure eval is only run when the verification
passes.
- Line 42: LOCAL_INSTALLER currently appends an extra /install segment causing
paths like install/install/...; change the concatenation so it targets the
installer file in the top-level install directory from the script's location by
removing the redundant "/install" suffix—update the LOCAL_INSTALLER assignment
(the variable name LOCAL_INSTALLER in the install/proxmox/ct script) to use
.../../../${NSAPP:-netalertx}-install.sh instead of
.../../../install/${NSAPP:-netalertx}-install.sh so the override works
correctly.
|
@heinemannj The installer works, it's just a pain trying to use strings to have it pull from a temp location for testing... If you could please try this one it should work now. |
I'm using for the Web access
|
4 participants
@jokob-sk @ingoratsdorf @adamoutler
This PR introduces the NetAlertX helper scripts to provide a streamlined, automated installation of the NetAlertX network security monitoring tool on Proxmox VE.
📝 Description
NetAlertX is a comprehensive WiFi / LAN intruder detector and network monitor. These scripts automate the creation of a Debian-based LXC container, the installation of all necessary runtimes (PHP 8.4, Python 3.13+), and the configuration of environment variables to ensure a "plug-and-play" experience.
✨ Features
setup_phphelper) and NGINX./opt/netalertx-envwith automated dependency management.setcapon key binaries (nmap,arp-scan,python3) to allow network scanning without requiring full root privileges.PYTHONPATH, creates missing package indicators (__init__.py), and ensures the correct working directory.update_script()in the CT script, allowing users to pull latest releases and rebuild dependencies via the helper menu.🛠️ Compliance & Standards
/ctand/installtemplates.source /dev/stdin <<<"$FUNCTIONS_FILE_PATH").$STD.🚀 How to Test
Summary by CodeRabbit
Documentation
New Features
Chores