Conversation
|
Lmk when good v to merge |
|
Where's CodeRabbit? ;-) Anyway, I'd say all good to merge. Test ran it and it's not modifying any actual project files. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds Debian 13 (Trixie) install/start scripts and an Nginx site config for NetAlertX, introduces end-to-end setup and startup flows, and updates a Dockerfile comment path. Includes package installation, environment prep, service management, repository cloning/updating, config linking, and process launch steps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Root User
participant I as install/debian13/install.debian13.sh
participant G as GitHub Repo
participant FS as Filesystem (/app, /config, /db)
participant S as start.debian13.sh
participant APT as APT/OS
participant N as NGINX
participant P as PHP-FPM 8.4
participant X as NetAlertX Server
U->>I: Execute installer (Debian 13)
I->>APT: Update, install prerequisites (sudo, git)
alt /app exists
I->>U: Prompt: install | update | start
opt install
I->>N: stop
I->>X: terminate running processes
I->>FS: unmount tmpfs (if mounted), clean /app
I->>G: clone repo to /app
end
opt update
I->>N: stop
I->>G: git pull in /app
end
opt start
Note over I: No code changes
end
else /app missing
I->>G: clone repo to /app
end
I->>FS: ensure front/buildtimestamp.txt
I->>S: chmod +x and invoke start.debian13.sh
rect rgba(200,240,255,0.2)
Note right of S: System setup and launch
S->>APT: Install system deps (nginx, php8.4-fpm, python, arp-scan, sqlite3, cron, etc.)
S->>FS: Create venv, pip install Python deps
S->>N: disable default site, link netalertx.conf
S->>FS: Prepare web UI, tmpfs logs/api, configs/DB, permissions
S->>P: start php-fpm
S->>N: start nginx
S->>X: launch NetAlertX (background)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/debian13/start.debian13.sh (1)
215-228: Fix Python entrypoint: use module form, absolute venv path, and update process-kill patternsVerified: server/main.py and init.py exist — running a directory path is fragile. Replace the relative venv activation and directory-start with a module invocation and update any pkill/pgrep that expect the old argv.
Apply (still relevant):
-# Activate the virtual python environment -source myenv/bin/activate +source "$INSTALLER_DIR/myenv/bin/activate" @@ -# Start the NetAlertX python script -# All error and console output being diverted to null, -# otherwise we can get critical errors re I/O -python "$INSTALL_PATH/server/" 2>/dev/null 1>/dev/null & +# Start the NetAlertX python app +cd "$INSTALL_PATH" +# Prefer module form to support packages with __main__.py +python -m server 1>/dev/null 2>/dev/null &Also update/replace any pkill -f "python ${INSTALL_DIR}/server" (example locations: install/debian13/start.debian13.sh, install/ubuntu24/start.ubuntu24.sh, install/debian12/start.debian12.sh, dockerfiles/start.sh, dockerfiles/init.sh) to match the new invocation (or switch to pidfile/systemd-managed service) so stop/restart behavior remains correct.
🧹 Nitpick comments (10)
Dockerfile.debian (1)
30-30: Clarify cross-file sync note (include Debian 13; verify path spelling).The comment references only Debian 12 and uses “dependecies” (likely typo). Suggest clarifying and including Debian 13 scripts to avoid drift.
Apply:
-# ❗ IMPORTANT - if you modify this file modify the /install/debian12/install_dependecies.debian12.sh file as well ❗ +# ❗ IMPORTANT: keep Dockerfile and installer scripts in sync. If you modify this file, also update: ❗ +# - /install/debian12/install_dependecies.debian12.sh # (verify exact path/name) +# - /install/debian13/install.debian13.sh +# - /install/debian13/start.debian13.shPlease confirm the Debian 12 filename is indeed “install_dependecies.debian12.sh”; if it’s “install_dependencies…”, fix the comment accordingly to avoid confusion.
install/debian13/netalertx.conf (2)
6-7: Remove proxy header in FastCGI context; pass it via FastCGI if needed.
proxy_set_headerhas no effect in a FastCGI location. If the app expects X-Forwarded-Prefix, pass it as a FastCGI param.Apply:
- add_header X-Forwarded-Prefix "/netalertx" always; - proxy_set_header X-Forwarded-Prefix "/netalertx"; + add_header X-Forwarded-Prefix "/netalertx" always; @@ location ~* \.php$ { + fastcgi_param HTTP_X_FORWARDED_PREFIX "/netalertx";Also applies to: 9-16
10-12: Make Cache-Control header unconditional.Without
always, the header won’t apply to non-2xx/3xx responses.Apply:
- add_header Cache-Control "no-store"; + add_header Cache-Control "no-store" always;install/debian13/install.debian13.sh (3)
1-6: Enable strict mode for safer installs.Fail fast and catch unset vars to avoid partial/unsafe installs.
Apply:
#!/usr/bin/env bash + +set -Eeuo pipefail +IFS=$'\n\t'
53-73: Harden destructive cleanup (SC2115).Use “${var:?}” guards and a compact, dotglob-safe pattern.
Apply:
- # Remove all contents safely - rm -rf -- "$INSTALL_DIR"/* "$INSTALL_DIR"/.[!.]* "$INSTALL_DIR"/..?* 2>/dev/null + # Remove all contents safely (dotfiles included) + rm -rf -- "${INSTALL_DIR:?}"/{.,}* 2>/dev/null || true
82-85: Unify process stop pattern (python vs python3).Use a single, robust pkill that matches both python and python3.
Apply:
- pkill -f "python ${INSTALL_DIR}/server" 2>/dev/null + pkill -f "python.*${INSTALL_DIR}/server" 2>/dev/nullinstall/debian13/start.debian13.sh (4)
51-61: Run apt-get update and use noninteractive mode.Precede installs with update; avoid prompts on fresh systems.
Apply:
-# Install dependencies -apt-get install -y \ +export DEBIAN_FRONTEND=noninteractive +apt-get update -yq +apt-get install -yq \ @@ -apt-get install nginx-core php${PHPVERSION} php${PHPVERSION}-sqlite3 php php-cgi php-fpm php-sqlite3 php-curl php-fpm php${PHPVERSION}-fpm php-cli -y +apt-get install -yq nginx-core php${PHPVERSION} php${PHPVERSION}-sqlite3 php php-cgi php-fpm php-sqlite3 php-curl php${PHPVERSION}-fpm php-cli
97-109: Quote paths and ensure idempotent symlink creation.Avoid failures when re-running the script.
Apply:
-if [ -d $WEB_UI_DIR ]; then +if [ -d "$WEB_UI_DIR" ]; then @@ - rm -R $WEB_UI_DIR + rm -rf -- "$WEB_UI_DIR" fi @@ -ln -s $INSTALL_PATH/front $WEB_UI_DIR +ln -sfn "$INSTALL_PATH/front" "$WEB_UI_DIR" @@ -ln -s "${INSTALLER_DIR}/$NGINX_CONF_FILE" $NGINX_CONFIG_FILE +ln -sfn "${INSTALLER_DIR}/$NGINX_CONF_FILE" "$NGINX_CONFIG_FILE"
148-158: Make unmounts quiet/idempotent.Suppress noise if not mounted; don’t fail.
Apply:
-umount "${INSTALL_DIR}/log" -umount "${INSTALL_DIR}/api" +umount "${INSTALL_DIR}/log" 2>/dev/null || true +umount "${INSTALL_DIR}/api" 2>/dev/null || true
185-192: Guard destructive wipes during ALWAYS_FRESH_INSTALL.Add safety braces and quiet errors.
Apply:
- rm -rf "${INSTALL_PATH}/config/"* + rm -rf -- "${INSTALL_PATH}/config/"* 2>/dev/null || true @@ - rm -rf "${INSTALL_PATH}/db/"* + rm -rf -- "${INSTALL_PATH}/db/"* 2>/dev/null || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile.debian(1 hunks)install/debian13/install.debian13.sh(1 hunks)install/debian13/netalertx.conf(1 hunks)install/debian13/start.debian13.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
install/debian13/install.debian13.sh
[warning] 72-72: Use "${var:?}" to ensure this never expands to /* .
(SC2115)
| echo "[INSTALL] Fixing file permissions" | ||
| chown root:www-data "${INSTALL_DIR}"/api/user_notifications.json | ||
|
|
||
| echo "[INSTALL] Fixing WEB_UI_DIR: ${WEB_UI_DIR}" | ||
|
|
||
| chmod -R a+rwx $WEB_UI_DIR | ||
|
|
||
| echo "[INSTALL] Fixing INSTALL_DIR: ${INSTALL_DIR}" | ||
|
|
||
| chmod -R a+rw $INSTALL_PATH/log | ||
| chmod -R a+rwx $INSTALL_DIR | ||
|
|
There was a problem hiding this comment.
Overly permissive 0777 ACLs on web and app dirs.
Setting a+rwx on the whole tree is a security risk. Tighten permissions and ownership.
Apply:
-chown root:www-data "${INSTALL_DIR}"/api/user_notifications.json
-
-echo "[INSTALL] Fixing WEB_UI_DIR: ${WEB_UI_DIR}"
-
-chmod -R a+rwx $WEB_UI_DIR
-
-echo "[INSTALL] Fixing INSTALL_DIR: ${INSTALL_DIR}"
-
-chmod -R a+rw $INSTALL_PATH/log
-chmod -R a+rwx $INSTALL_DIR
+chown -R www-data:www-data "${INSTALL_DIR}/api"
+echo "[INSTALL] Fixing WEB_UI_DIR: ${WEB_UI_DIR}"
+chown -R www-data:www-data "$WEB_UI_DIR"
+find "$WEB_UI_DIR" -type d -exec chmod 755 {} +
+find "$WEB_UI_DIR" -type f -exec chmod 644 {} +
+echo "[INSTALL] Fixing INSTALL_DIR: ${INSTALL_DIR}"
+chmod -R 664 "$INSTALL_PATH/log" || true
+chown -R www-data:www-data "$INSTALL_PATH/log"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In the file install/debian13/start.debian13.sh around lines 169 to 180, the code
sets overly permissive 0777 access control lists (ACLs) on the web and app
directories, which is a security risk. Instead, you should tighten the
permissions and ownership on these directories. For the user_notifications.json
file, set the ownership to root:www-data. For the $WEB_UI_DIR directory, use
more restrictive permissions like 755 instead of 0777. And for the $INSTALL_DIR
directory, use 0775 or similar rather than 0777.
install/debian13/start.debian13.sh
Outdated
| cp --update=none "${INSTALL_PATH}/back/$CONF_FILE" "${INSTALL_PATH}/config/$CONF_FILE" | ||
| cp --update=none "${INSTALL_PATH}/back/$DB_FILE" "$FILEDB" | ||
|
|
There was a problem hiding this comment.
cp uses invalid flag (--update=none); starter files may not be copied.
This likely fails silently (no set -e), leaving missing config/DB on first run.
Apply:
-# Copy starter $DB_FILE and $CONF_FILE if they don't exist
-cp --update=none "${INSTALL_PATH}/back/$CONF_FILE" "${INSTALL_PATH}/config/$CONF_FILE"
-cp --update=none "${INSTALL_PATH}/back/$DB_FILE" "$FILEDB"
+#!/usr/bin/env bash
+# Copy starter $DB_FILE and $CONF_FILE if they don't exist (no overwrite)
+install -D -m 664 -o www-data -g www-data "${INSTALL_PATH}/back/$CONF_FILE" "${INSTALL_PATH}/config/$CONF_FILE"
+install -D -m 660 -o www-data -g www-data "${INSTALL_PATH}/back/$DB_FILE" "$FILEDB"
@@
-chmod -R a+rwx $INSTALL_DIR # second time after we copied the files
-chmod -R a+rw $INSTALL_PATH/config
-chgrp -R www-data $INSTALL_PATH
+find "$INSTALL_PATH/config" -type d -exec chmod 755 {} +
+find "$INSTALL_PATH/config" -type f -exec chmod 644 {} +
+chown -R www-data:www-data "$INSTALL_PATH/config" "$FILEDB"Also applies to: 201-208
🤖 Prompt for AI Agents
In install/debian13/start.debian13.sh around lines 196-198 (and also apply same
fix to lines 201-208), the cp invocations use an invalid flag (--update=none) so
starter files may not be copied; replace the invalid flag with the intended
behavior (either --no-clobber/-n to avoid overwriting existing files or
--update/-u to only copy when source is newer) and update all similar cp lines
accordingly, and ensure the script enables strict failure handling (add set -e
or set -o errexit near the top) so cp errors don't fail silently.
Updates to cp -u
|
Ah, sweet. That took longer than usual ;-) The overly permissive file modes are an issue across all installers. |
|
Since it's a docker container, wouldn't
|
For docker, the whole thing does not matter as there is no existing config. Should never be. And even if there is, there's no point making a copy for a self-contained temporary system. Everything will be deleted and recreated with a restart. For the bare metal installers, I have changed this to
|
|
Oh sure... After I spent all day working on it. Lol JK. I'll try it out on Monday. Some of the things I was working on was adding a requirements.txt and some other automations. |
|
I like the requitrements.txt approach, not sure if it can be backported to all installation methods |
|
As long as pip exists you can use the one-file-per-line Maybe it's best to support single build like alpine or Debian instead of maintaining several build systems. With devcontainer coming, they can develop and use tools on any system they want, as long as it doesn't break the container. |
Can be backported, no issue. |
|
is this ready to merge @ingoratsdorf / @adamoutler - WDYT? |
|
I think the chmod a+rwx is excessive. As Coderabit said. That's chmod 777. No need for that. |
|
OK, I'll backport the If @JVKeller would like to maintain the Trixie installer? I'll look at the permission suggestion and see what runs and what does not. |
|
I'd like to propose we stick to performing all resource installation and static configuration within the My reasoning is that building a self-contained, immutable image is a core principle of containerization. This approach gives us several key advantages:
Runtime scripts (e.g.,
Moving static installation into a runtime script makes the container's state less predictable and slows down its startup. Would you be open to keeping the installation steps within the |
|
I mean, as it is, it appears the NetAlertX Debian package doesn't ship with NetAlertX 😄. What's the point of a container? Just make a installer script and let people choose to run on bare metal or in a container. |
|
I'm not in a position to maintain it, but thanks. I was thinking more along the lines of the Debian installer as a bare metal, as the initial installer did. Not containerized... so I had the script make the requirements.txt file. The last few projects I played with seemed to lean that way. It's still all in one installer script. Makes for a good proxmox lxc setup for testing before deployment. |
|
I'd say move it all to the script and delete the Debian non-NetAlertX Dockerfile.debian. A proper container includes everything needed and can be made read-only with some select tmpfs mounts in various places. You should use
But treating a container as a VM is not a good practice. Currently the Dockerfile.debian doesn't contain NetAlertX so it's really just a custom Debian distribution and should be removed. All the installs should occur in the script as previously stated. @jokob-sk thoughts? |
|
Following on from @JVKeller 's comment, I agree that the whole thread was about providing a bare metal Trixie installer along with Ubuntu. This was never about docker really. Personally, I only use docker in an LXC if there is really no install option other than docker available. Otherwise, I prefer a bare metal. I do understand that docker may be the easier option for a lot of (most) people. In which case many people will prefer the Alpine base image version due to resource savings - mostly disk space though. I def see the need for docker, whether Debian or Alpine, however, I'd be contributing to the bare metal installers only, I do not really use docker a lot and have insufficient knowledge. Obviously happy to collab wherever needed. :-) |
|
Sure. But do put the newline at the end of the dockerfile you removed. All files should end with a newline allowing an |
Sweet, thanks for the heads-up. |
|
My stance is: I support whatever makes the app easy to maintain, and adhere to general best practices (e.g. the DRY principle). I can't really chip in discussions around Proxmox, LXC, or the different bare-metal scripts as I don't actively use them (implemented the initial bare-metal install script, but maintaining all this by myself is stretching me really thin, so really appreciate the help from everyone here). I trust users and contributors like you in this thread, who are using these deployment approaches, as they have more knowledge and experience. Happy to make a call if necessary, but overall those are my thoughts :) |
|
After the rework of the Ubuntu24 installer, I'd suggest to cancel this one and rewrite the installer as well following the Ubuntu24 model. |
|
I'd suggest a unified installer. Ubuntu is based on Debian with a few different package names and some version differences. It would be a good idea to have a single file where |
|
Obviously replace the tests with if then blocks to properly handle failures instead of just |
4 participants
Created Debian13 (Trixie) installer, test run it.
nginx and php version fixes.
Trixie comes by default with PHP 8.4.
Summary by CodeRabbit
New Features
Improvements
Documentation