fix: root access PHP & Nginx#1400
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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/production-filesystem/services/config/php/php-fpm.d/www.conf:
- Around line 494-495: The two php_admin_value directives
php_admin_value[upload_max_filesize] and php_admin_value[post_max_size] are
inconsistently formatted ("1 M" vs "1M"); update
php_admin_value[upload_max_filesize] to match the compact format used by
php_admin_value[post_max_size] (use "1M" without a space) so both directives use
the same unit formatting to avoid parsing inconsistencies.
- Around line 498-500: The file defines php_admin_value[realpath_cache_size]
twice (duplicate entries) — remove one of the
php_admin_value[realpath_cache_size] lines so only a single realpath_cache_size
setting remains, leaving php_admin_value[session.save_path] intact; ensure the
remaining php_admin_value[realpath_cache_size] retains the desired value
(4096K).
In @install/production-filesystem/services/start-php-fpm.sh:
- Around line 31-36: The script currently forces php-fpm to run as root by
appending -R to php_fpm_cmd when id -u == 0 which is a security risk; change the
logic in start-php-fpm.sh so php_fpm_cmd is NOT given -R for UID 0, instead
ensure the process drops privileges by launching php-fpm under a non-root user
(create/ensure a dedicated user like www-data and use su-exec/setpriv or
runit/systemd user directives), fix any socket/file ownership before start
(chown/chmod) so php-fpm can bind without root, remove or correct the misleading
comment about "legacy systems where we cannot provision root access to a
binary", and add a brief note in the script explaining the chosen non-root
approach and any legacy constraint justification for reviewer verification.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
install/production-filesystem/services/config/nginx/netalertx.conf.templateinstall/production-filesystem/services/config/php/php-fpm.d/www.confinstall/production-filesystem/services/start-nginx.shinstall/production-filesystem/services/start-php-fpm.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/start-php-fpm.sh:25-26
Timestamp: 2025-10-26T17:12:30.237Z
Learning: In NetAlertX's start-php-fpm.sh, PHP-FPM error logging is handled through the PHP-FPM configuration file (PHP_FPM_CONFIG_FILE) rather than through shell redirection. The current redirection pattern (stdout to LOG_APP_PHP_ERRORS, stderr to /dev/stderr) is intentional.
📚 Learning: 2025-10-19T01:40:57.095Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Applied to files:
install/production-filesystem/services/config/nginx/netalertx.conf.templateinstall/production-filesystem/services/start-nginx.sh
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
install/production-filesystem/services/config/nginx/netalertx.conf.templateinstall/production-filesystem/services/start-nginx.sh
📚 Learning: 2025-10-26T17:12:30.237Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/start-php-fpm.sh:25-26
Timestamp: 2025-10-26T17:12:30.237Z
Learning: In NetAlertX's start-php-fpm.sh, PHP-FPM error logging is handled through the PHP-FPM configuration file (PHP_FPM_CONFIG_FILE) rather than through shell redirection. The current redirection pattern (stdout to LOG_APP_PHP_ERRORS, stderr to /dev/stderr) is intentional.
Applied to files:
install/production-filesystem/services/start-php-fpm.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (3)
install/production-filesystem/services/config/php/php-fpm.d/www.conf (1)
496-496: Verify buffer size increase is addressing the root cause.The output buffer size has been doubled from 262144 to 524288. While the PR description mentions resolving "a PHP buffer problem," increasing buffer sizes can sometimes mask underlying issues rather than fix them.
Could you clarify:
- What specific buffer-related issue was encountered?
- Is 524288 (512KB) an appropriate size for the expected use case, or could there be a more targeted fix?
- Have you verified this doesn't mask issues like infinite loops or excessive data generation?
install/production-filesystem/services/config/nginx/netalertx.conf.template (1)
1-2: Template implementation is correct, but security implications remain.The addition of the
${NGINX_USER_DIRECTIVE}placeholder is technically correct and follows the envsubst pattern used elsewhere. However, this enables nginx to run as root when UID is 0, which carries the same security concerns raised in the start-nginx.sh and start-php-fpm.sh reviews.Please ensure the security implications of running nginx as root have been thoroughly evaluated and documented, as this significantly increases the attack surface if nginx or any served application has vulnerabilities.
install/production-filesystem/services/start-nginx.sh (1)
47-47: Implementation of envsubst variable addition is correct.The addition of
${NGINX_USER_DIRECTIVE}to the envsubst call is implemented correctly and follows the existing pattern for template variable substitution.
install/production-filesystem/services/config/php/php-fpm.d/www.conf
Outdated
Show resolved
Hide resolved
install/production-filesystem/services/config/php/php-fpm.d/www.conf
Outdated
Show resolved
Hide resolved
| #In the event PUID is 0 we need to run nginx as root | ||
| #This is useful on legacy systems where we cannot provision root access to a binary | ||
| export NGINX_USER_DIRECTIVE="" | ||
| if [ "$(id -u)" -eq 0 ]; then | ||
| NGINX_USER_DIRECTIVE="user root;" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n install/production-filesystem/services/start-nginx.shRepository: jokob-sk/NetAlertX
Length of output: 2723
🏁 Script executed:
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | head -20 | xargs grep -l "nginx\|root" 2>/dev/null | head -5Repository: jokob-sk/NetAlertX
Length of output: 121
🏁 Script executed:
rg -C 5 "legacy systems" . --type sh 2>/dev/nullRepository: jokob-sk/NetAlertX
Length of output: 2054
🏁 Script executed:
rg -C 3 "NGINX_USER_DIRECTIVE" . 2>/dev/nullRepository: jokob-sk/NetAlertX
Length of output: 2056
Clarify the intent and security implications of running nginx as root.
The logic itself is defensible in a containerized environment where the process inherits root UID from the container. However, the comment is misleading—it states "we cannot provision root access to a binary" when the actual situation is that the process is already running as root and cannot drop to a lower privilege level. When UID is not 0, NGINX_USER_DIRECTIVE correctly remains empty.
That said:
- Document why the container runs as root and why privilege separation cannot be configured (e.g., container design constraints)
- Consider alternatives: running the container as a non-root user, using Linux capabilities to allow port binding without full root, or explicit user provisioning within the container
- Update the comment to accurately reflect the scenario: "If the process is running as root (UID 0), explicitly configure nginx to run as root; otherwise use the default user"
There was a problem hiding this comment.
Root access is permitted now because Synology with AUFS cannot use capabilities assigned to a binary.
There was a problem hiding this comment.
@adamoutler, thank you for clarifying! That makes complete sense—AUFS's limitation with file capabilities on Synology systems is a legitimate technical constraint that justifies this approach.
To help future maintainers understand this constraint, consider updating the comment to be more specific:
# When running as root (PUID=0), configure nginx to run as root.
# This is required for Synology systems with AUFS, which cannot use
# Linux capabilities assigned to binaries for privilege management.
export NGINX_USER_DIRECTIVE=""
if [ "$(id -u)" -eq 0 ]; then
NGINX_USER_DIRECTIVE="user root;"
fiThis way, the comment clearly documents the Synology AUFS limitation rather than the more generic "legacy systems" phrasing.
✏️ Learnings added
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1400
File: install/production-filesystem/services/start-nginx.sh:38-43
Timestamp: 2026-01-10T06:37:57.050Z
Learning: In NetAlertX, nginx and PHP may run as root (when PUID=0) to support Synology NAS systems using AUFS (Another Union File System), which cannot use Linux capabilities assigned to binaries for privilege management. This is a known limitation of AUFS on Synology platforms.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: front/plugins/dhcp_servers/script.py:44-44
Timestamp: 2025-10-19T15:29:46.423Z
Learning: In the NetAlertX dhcp_servers plugin (front/plugins/dhcp_servers/script.py), the nmap command uses both 'sudo' and '--privileged' flag to maintain cross-platform compatibility. While the hardened Docker image stubs sudo and uses capabilities, hardware installations (Debian 12, Ubuntu 24) and the Debian Dockerfile require sudo for raw socket access. This approach ensures the plugin works across all deployment targets.
|
|
||
| #In the event PUID is 0 we need to run php-fpm as root | ||
| #This is useful on legacy systems where we cannot provision root access to a binary | ||
| if [[ $(id -u) -eq 0 ]]; then | ||
| php_fpm_cmd+=(-R) | ||
| fi |
There was a problem hiding this comment.
Critical security concern: Running PHP-FPM as root.
Running PHP-FPM as root violates the principle of least privilege and significantly increases the attack surface. If PHP or any PHP application has a vulnerability, an attacker would gain root privileges.
Additionally, the comment states "legacy systems where we cannot provision root access to a binary" which appears contradictory—if the process is already running as root (UID 0), root access has already been provisioned.
Could you clarify:
- What specific legacy system constraint requires this change?
- Have you considered alternative approaches (e.g., file system permissions, capabilities, or using rootless containers)?
- Is there security documentation or threat model analysis for running these services as root?
🤖 Prompt for AI Agents
In @install/production-filesystem/services/start-php-fpm.sh around lines 31 -
36, The script currently forces php-fpm to run as root by appending -R to
php_fpm_cmd when id -u == 0 which is a security risk; change the logic in
start-php-fpm.sh so php_fpm_cmd is NOT given -R for UID 0, instead ensure the
process drops privileges by launching php-fpm under a non-root user
(create/ensure a dedicated user like www-data and use su-exec/setpriv or
runit/systemd user directives), fix any socket/file ownership before start
(chown/chmod) so php-fpm can bind without root, remove or correct the misleading
comment about "legacy systems where we cannot provision root access to a
binary", and add a brief note in the script explaining the chosen non-root
approach and any legacy constraint justification for reviewer verification.
There was a problem hiding this comment.
Root access is permitted now because Synology with AUFS cannot use capabilities assigned to a binary.
There was a problem hiding this comment.
@adamoutler, thank you for the clarification! I understand now—the root access requirement for PHP-FPM (and nginx) is specifically to support Synology NAS systems using AUFS, which has a known limitation preventing the use of Linux capabilities assigned to binaries. This is consistent with the platform compatibility approach already documented for this project.
The comment about "legacy systems where we cannot provision root access to a binary" makes sense in this context—AUFS cannot leverage capabilities, so the only alternative for operations requiring elevated privileges is to run as root when PUID=0.
This is a valid architectural decision for maintaining Synology compatibility, and I've noted this context from our earlier discussion about the nginx changes in this PR.
🧠 Learnings used
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/start-php-fpm.sh:25-26
Timestamp: 2025-10-26T17:12:30.237Z
Learning: In NetAlertX's start-php-fpm.sh, PHP-FPM error logging is handled through the PHP-FPM configuration file (PHP_FPM_CONFIG_FILE) rather than through shell redirection. The current redirection pattern (stdout to LOG_APP_PHP_ERRORS, stderr to /dev/stderr) is intentional.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1400
File: install/production-filesystem/services/start-nginx.sh:38-43
Timestamp: 2026-01-10T06:37:57.050Z
Learning: In NetAlertX, nginx and PHP may run as root (when PUID=0) to support Synology NAS systems using AUFS (Another Union File System), which cannot use Linux capabilities assigned to binaries for privilege management. This is a known limitation of AUFS on Synology platforms.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: front/plugins/dhcp_servers/script.py:44-44
Timestamp: 2025-10-19T15:29:46.423Z
Learning: In the NetAlertX dhcp_servers plugin (front/plugins/dhcp_servers/script.py), the nmap command uses both 'sudo' and '--privileged' flag to maintain cross-platform compatibility. While the hardened Docker image stubs sudo and uses capabilities, hardware installations (Debian 12, Ubuntu 24) and the Debian Dockerfile require sudo for raw socket access. This approach ensures the plugin works across all deployment targets.
|
Build failed due to pre-existing selenium problem corrected in #1399 |
2 participants
This fixes root access for both nginx and php. Additionally it resolves the php buffer problem.
Summary by CodeRabbit
Configuration Updates
Chores
✏️ Tip: You can customize this high-level summary in your review settings.