PUId fixes #1387
PUId fixes #1387
Conversation
📝 WalkthroughWalkthroughThis PR adds pre-emptive permission fixes across multiple entrypoint scripts, broadens allowed Linux capabilities (adds CHOWN, SETUID, SETGID), centralizes error reporting in the root entrypoint, tweaks Docker/devcontainer build steps, and updates tests to exercise and validate the permission-fix and startup behaviors. Changes
Sequence DiagramsequenceDiagram
participant Client as Container runtime
participant Root as root-entrypoint.sh
participant EntryD as entrypoint.d/* scripts
participant FS as Filesystem
Client->>Root: start container (root or non-root)
rect rgb(200,220,255)
Note over Root: Validate IDs & Capabilities
Root->>Root: _validate_id -> _error_msg
Root->>Root: CAP checks (explicit bitmask: NET_*, CHOWN, SETUID, SETGID)
end
rect rgb(220,240,220)
Note over Root: Path priming (root path prep)
Root->>FS: chown -R /data, /tmp -> PUID:PGID
end
rect rgb(240,220,220)
Note over EntryD: Permission pre-checks before init
Root->>EntryD: invoke entrypoint.d scripts
EntryD->>FS: if path exists -> chmod u+rwX (suppress errors)
EntryD->>FS: ensure config/db files writable (suppress errors)
end
rect rgb(255,240,200)
Note over Root: Non-root runtime verification
Root->>FS: test write access to /tmp and /data
alt writeable
Root->>Client: continue startup
else not writeable
Root->>Root: emit structured _error_msg (non-fatal / may fail)
Root->>Client: fail or proceed with NETALERTX_CHECK_ONLY adjusted
end
end
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 1
Fix all issues with AI Agents 🤖
In @test/docker_tests/test_mount_diagnostics_pytest.py:
- Around line 357-373: The TestScenario named "data_noread" has expected_issues
set to [""] (a truthy list) but should be an empty list like "db_noread" so the
conditional if not test_scenario.expected_issues triggers; update the
TestScenario instance with name="data_noread" to use expected_issues=[] (an
empty list) to match the intended behavior in the validation logic that checks
test_scenario.expected_issues.
🧹 Nitpick comments (4)
install/production-filesystem/entrypoint.d/20-first-run-config.sh (1)
4-8: Consider adding existence check for app.conf before chmod.The pre-flight permission fixes are a good defensive measure. However, line 8 attempts to
chmoda file that might not exist yet (existence is checked later at line 10). While the suppressed error makes this harmless, adding a conditional check would be slightly more efficient and clearer.🔎 Optional refinement
# Fix permissions if config directory exists but is unreadable if [ -d "${NETALERTX_CONFIG}" ]; then chmod u+rwX "${NETALERTX_CONFIG}" 2>/dev/null || true fi -chmod u+rw "${NETALERTX_CONFIG}/app.conf" 2>/dev/null || true +if [ -f "${NETALERTX_CONFIG}/app.conf" ]; then + chmod u+rw "${NETALERTX_CONFIG}/app.conf" 2>/dev/null || true +fiinstall/production-filesystem/entrypoint.d/25-first-run-db.sh (1)
5-9: Consider adding existence check for DB file before chmod.Similar to the pattern in
20-first-run-config.sh, line 9 attempts tochmoda file that might not exist yet (existence is checked later at line 44). While the suppressed error makes this harmless, adding a conditional check would be slightly more efficient and maintain consistency with the directory check above.🔎 Optional refinement
# Fix permissions if DB directory exists but is unreadable if [ -d "${NETALERTX_DB}" ]; then chmod u+rwX "${NETALERTX_DB}" 2>/dev/null || true fi -chmod u+rw "${NETALERTX_DB_FILE}" 2>/dev/null || true +if [ -f "${NETALERTX_DB_FILE}" ]; then + chmod u+rw "${NETALERTX_DB_FILE}" 2>/dev/null || true +fiinstall/production-filesystem/root-entrypoint.sh (2)
66-73: Inconsistent indentation in error message.Line 72 uses a tab character for indentation while the surrounding lines use spaces. This may cause misaligned output in the terminal.
Proposed fix
_error_msg "INVALID ${name} VALUE (non-numeric)" \ " Startup halted because the provided ${name} environmental variable contains non-digit characters. Action: set a numeric ${name} (for example: ${name}=1000) in your environment or docker-compose file. Default: 20211." - exit 1 + exit 1
112-125: Error message suggestschmod 755which may be too permissive for/data.The suggested fix
chmod 755 ${path}in the error message would make the directory world-readable/executable. For paths containing sensitive data like/data, this might not be the best advice. Consider suggestingchmod 750or ownership-based fixes instead.Proposed fix
Action: - 1. Update Host Volume permissions (e.g. 'chmod 755 ${path}' on host). + 1. Update Host Volume permissions (e.g. 'chown $(id -u):$(id -g) ${path}' on host). 2. Or, run container as root (user: 0) and let PUID/PGID logic handle it."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/docker_tests/configurations/test_results.logis excluded by!**/*.log
📒 Files selected for processing (15)
.devcontainer/DockerfileDockerfiledocker-compose.ymlinstall/production-filesystem/entrypoint.d/10-capabilities-audit.shinstall/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/entrypoint.d/25-first-run-db.shinstall/production-filesystem/entrypoint.d/30-mandatory-folders.shinstall/production-filesystem/entrypoint.d/90-excessive-capabilities.shinstall/production-filesystem/root-entrypoint.shtest/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymltest/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.ymltest/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.ymltest/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.pytest/docker_tests/test_mount_diagnostics_pytest.py
💤 Files with no reviewable changes (1)
- docker-compose.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: For time utilities useserver/utils/datetime_utils.py(timeNowDB); for MAC addresses usefront/plugins/plugin_helper.py(normalize_mac); for string sanitization useserver/helper.pysanitizers. Always validate MACs before database writes
When running subprocesses, ALWAYS set explicit timeouts. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout
Files:
test/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.pytest/docker_tests/test_mount_diagnostics_pytest.py
**/*.{py,php,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use environment variables (
NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding/data/dbor relative paths. Use/datafor persistent config/db,/tmpfor runtime logs/api/nginx state
Files:
test/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.pytest/docker_tests/test_mount_diagnostics_pytest.py
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Never `chmod` or `chown` during operations in devcontainer. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead
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.
📚 Learning: 2026-01-04T01:25:43.080Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Never `chmod` or `chown` during operations in devcontainer. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead
Applied to files:
install/production-filesystem/entrypoint.d/30-mandatory-folders.shinstall/production-filesystem/entrypoint.d/90-excessive-capabilities.shinstall/production-filesystem/root-entrypoint.shtest/docker_tests/test_container_environment.py
📚 Learning: 2025-11-24T00:56:58.733Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1304
File: .hadolint.yaml:1-2
Timestamp: 2025-11-24T00:56:58.733Z
Learning: In the NetAlertX repository, the .hadolint.yaml config file is intentionally configured to only globally ignore DL3018 (Alpine package pinning). Rules DL3006 (explicit image tagging) and SC2114 (deleting system directories) are handled via inline `# hadolint ignore` directives in specific Dockerfiles where needed, rather than being globally ignored, to ensure new Dockerfiles are still checked for these rules.
Applied to files:
Dockerfile.devcontainer/Dockerfile
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
Dockerfileinstall/production-filesystem/entrypoint.d/90-excessive-capabilities.shtest/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX devcontainer setup, the `python -m venv /opt/venv` command works successfully on Alpine 3.22 despite the typical Alpine behavior of not providing a /usr/bin/python symlink by default. The build completes successfully and pytest runs without issues.
Applied to files:
Dockerfile.devcontainer/Dockerfile
📚 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:
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymlinstall/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/entrypoint.d/90-excessive-capabilities.shinstall/production-filesystem/root-entrypoint.shtest/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml.devcontainer/Dockerfile
📚 Learning: 2026-01-04T01:25:43.080Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Applies to **/*.{py,php,js} : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding `/data/db` or relative paths. Use `/data` for persistent config/db, `/tmp` for runtime logs/api/nginx state
Applied to files:
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymlinstall/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-11-01T19:02:10.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1263
File: install/production-filesystem/entrypoint.sh:60-86
Timestamp: 2025-11-01T19:02:10.635Z
Learning: In the NetAlertX project (install/production-filesystem/entrypoint.sh), when fail-fast behavior is adopted for entrypoint checks in the future, be lenient during review because tests will be removed or changed to accommodate that behavior. The current continue-on-failure approach exists to support the existing test suite, but this is expected to change along with corresponding test adjustments.
Applied to files:
install/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-10-26T16:45:41.247Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/scripts/check-ramdisk.sh:0-0
Timestamp: 2025-10-26T16:45:41.247Z
Learning: In NetAlertX check scripts (install/production-filesystem/services/scripts/check-*.sh), not all checks should exit with non-zero status. Some checks, like check-ramdisk.sh, are warning-only and exit 0 even when issues are detected, allowing the application to start despite suboptimal configuration.
Applied to files:
install/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-10-26T17:09:18.621Z
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.
Applied to files:
install/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/entrypoint.d/90-excessive-capabilities.shinstall/production-filesystem/root-entrypoint.shtest/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml
📚 Learning: 2025-10-26T15:39:36.707Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: install/production-filesystem/services/scripts/check-first-run-config.sh:0-0
Timestamp: 2025-10-26T15:39:36.707Z
Learning: In NetAlertX startup scripts, critical initialization failures (e.g., unable to create config directory or copy default config files) should exit with non-zero status to fail fast and provide clear error messages, rather than continuing in a broken state.
Applied to files:
install/production-filesystem/entrypoint.d/20-first-run-config.shinstall/production-filesystem/root-entrypoint.shinstall/production-filesystem/entrypoint.d/25-first-run-db.sh
📚 Learning: 2025-10-19T15:29:46.423Z
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.
Applied to files:
install/production-filesystem/entrypoint.d/90-excessive-capabilities.shinstall/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-09-20T14:08:48.256Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:90-96
Timestamp: 2025-09-20T14:08:48.256Z
Learning: In the NetAlertX devcontainer setup, the setup.sh script intentionally removes user_notifications.json from the API directory during development environment initialization to prevent notification clutter that accumulates during container launches and development work.
Applied to files:
install/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-09-20T14:08:44.292Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/stream-logs.sh:5-6
Timestamp: 2025-09-20T14:08:44.292Z
Learning: The .devcontainer/scripts/stream-logs.sh script in NetAlertX is designed as a diagnostic tool for troubleshooting devcontainer startup issues. When log files don't exist, this indicates that the executable/services didn't start properly, which is valuable diagnostic information. Pre-creating missing files would mask this diagnostic behavior.
Applied to files:
install/production-filesystem/root-entrypoint.sh
📚 Learning: 2025-09-20T02:56:24.501Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/devcontainer.json:5-8
Timestamp: 2025-09-20T02:56:24.501Z
Learning: In the NetAlertX devcontainer setup, the final .devcontainer/Dockerfile is generated by combining the root Dockerfile with .devcontainer/resources/devcontainer-Dockerfile using the generate-dockerfile.sh script. The devcontainer.json should reference the generated file, not the root Dockerfile.
Applied to files:
.devcontainer/Dockerfile
🧬 Code graph analysis (1)
test/docker_tests/test_container_environment.py (1)
test/docker_tests/test_ports_available.py (1)
_assert_contains(174-182)
🪛 Ruff (0.14.10)
test/docker_tests/test_docker_compose_scenarios.py
716-716: subprocess call: check for execution of untrusted input
(S603)
728-728: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (2)
- GitHub Check: docker_dev
- GitHub Check: docker-tests
🔇 Additional comments (22)
Dockerfile (1)
45-48: Verify intentional pip caching for base tools.Adding DL3042 to the ignore list acknowledges that line 46 intentionally omits
--no-cache-dirwhen upgrading pip/setuptools/wheel. This caches these base tools across builds for performance, while line 47 still uses--no-cache-dirfor application dependencies.Confirm this caching strategy is intentional, as cached pip versions may become stale between builds.
.devcontainer/Dockerfile (1)
1-1: Auto-generated file - skipping detailed review.This file is auto-generated by
.devcontainer/scripts/generate-configs.sh. Changes reflect updates to the root Dockerfile and generation scripts. Review comments about dependencies, build steps, or linter exemptions should be directed at the root Dockerfile.Based on learnings, devcontainer Dockerfiles are not reviewed directly.
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml (1)
40-40: LGTM: Tmpfs ownership explicitly set for PUID compatibility.The tmpfs mode change to 1777 (world-writable with sticky bit) and explicit uid/gid ownership aligns with the PR's PUID support improvements. This ensures the /tmp mount is accessible when running with arbitrary UIDs.
install/production-filesystem/entrypoint.d/90-excessive-capabilities.sh (1)
24-25: LGTM: Capability expansion necessary for PUID permission handling.The addition of CHOWN, SETGID, and SETUID capabilities is essential for the PR's PUID support improvements, enabling the container to fix permissions on startup when running with arbitrary UIDs. The bitmask calculation is correct:
0x34C1(bits 0, 6, 7, 10, 12, 13).test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.yml (2)
17-18: LGTM: Capabilities aligned with permission handling requirements.Adding SETGID and SETUID capabilities is consistent with the expanded capability set in
90-excessive-capabilities.shand necessary for PUID-based permission handling.
36-60: LGTM: Explicit tmpfs mounts improve test clarity and resource control.The three new tmpfs mounts (/tmp/log, /tmp/api, /tmp/run) with explicit size limits (64m) and ownership (uid=20211, gid=20211) enhance test reliability and align with the PR's PUID improvements. The mode 1777 ensures accessibility while the sticky bit provides basic isolation.
test/docker_tests/test_container_environment.py (3)
479-479: LGTM: Check-only mode improves test predictability.Adding
NETALERTX_CHECK_ONLY=1to the root phase ensures the test focuses on permission handling without full application startup, reducing test flakiness.
1360-1374: LGTM: Root-startup path validation strengthens permission-fix testing.The change to explicitly exercise the root-startup permission-fix path (line 1360) with pre-verification assertions (lines 1370-1374) ensures the test validates the PR's core PUID fix logic. This approach prevents false negatives by confirming root startup succeeds before checking downstream effects.
1395-1401: LGTM: Verification logging improves CI debuggability.The mandatory logging of verification commands and output aligns with project requirements (per file header) and aids in diagnosing CI failures related to permission fixes.
test/docker_tests/test_docker_compose_scenarios.py (4)
702-728: LGTM: Failure detection helper improves test reliability.The
_wait_for_unwritable_failurehelper properly polls container status with a 45-second deadline, ensuring the test waits for the expected failure condition rather than racing with container shutdown.
737-742: LGTM: Full startup mode validates nginx config generation.Running with
NETALERTX_CHECK_ONLY=0(full startup) correctly exercises the nginx configuration generation on tmpfs, which is the target of this test scenario. The detached mode with post_up hook ensures proper failure detection.
750-752: LGTM: Precise failure assertions validate expected behavior.The assertions correctly validate the expected failure mode: non-zero exit code and specific error messages about inability to write nginx config files. The ANSI escape stripping ensures reliable string matching.
825-825: Verify timeout reduction doesn't cause CI flakiness.The timeout reduction from 60 to 8 seconds in
test_normal_startup_no_warnings_composeis significant (87% reduction). While the detached mode with post_up port checking should handle timing, ensure this is sufficient in slower CI environments.Consider monitoring CI stability after this change. If flakiness occurs, the timeout may need adjustment or environment-specific tuning via
NETALERTX_COMPOSE_PORT_WAIT_TIMEOUT.Also applies to: 881-881
install/production-filesystem/entrypoint.d/10-capabilities-audit.sh (1)
6-7: LGTM! Documentation clarity improved.The updated comment clearly delineates the scope of this script from excessive capability checking, improving maintainability.
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml (1)
41-41: LGTM! Explicit tmpfs ownership for test consistency.Adding
uid=20211,gid=20211to the tmpfs mount ensures consistent ownership with the configured user (line 11) and PUID/PGID variables (lines 23-24), improving test reliability for permission scenarios.install/production-filesystem/entrypoint.d/30-mandatory-folders.sh (1)
23-28: LGTM! Pre-flight permission fix handles existing directories gracefully.The added pre-check correctly handles the scenario where a directory exists but has restrictive permissions. The
chmod u+rwXis appropriate (execute permission needed for directory traversal), and error suppression aligns with the defensive approach taken throughout the entrypoint scripts.test/docker_tests/test_mount_diagnostics_pytest.py (2)
440-452: LGTM on the new noread validation logic.The explicit table row assertions for
data_noreadanddb_noreadscenarios correctly verify the expected state after the root-entrypoint permission fixes:readable=False,writeable=True,mount=True,dataloss=True.However, this block will only be reached if the
expected_issuesinconsistency noted above is fixed.
678-680: LGTM on unconditional table validation.Calling
validate_scenario_table_outputfor every scenario ensures diagnostic table output is validated regardless of whether issues are expected. This improves test coverage.install/production-filesystem/root-entrypoint.sh (4)
46-60: LGTM on centralized error handling.The
_error_msghelper provides consistent, visually distinct error output with color formatting. Using>&2for stderr and heredoc for multiline messages is a clean approach.
79-107: LGTM on capability checking logic.The bitwise capability checks are cleaner than the previous awk-based parsing. Using explicit numeric constants with comments documenting the bit positions (
SETGID_MASK=64,SETUID_MASK=128,NET_MASK=13312) improves readability and avoids shell parsing issues.
138-154: LGTM on path priming logic.The approach of:
- Recursively chowning
/dataand/tmpupfront- Using
install -d -o -gfor directory creation with ownership- Skipping
/tmpin the loop (already handled)- Suppressing errors with
|| truefor best-effort behavior...aligns well with the fail-soft philosophy documented in the script header. The removal of chmod operations is appropriate since non-root runtime can handle modes after ownership is set.
157-163: Verify the intended behavior ofNETALERTX_CHECK_ONLY=0on fallback.When
su-execfails,NETALERTX_CHECK_ONLYdefaults to0. The AI summary indicates this changed from defaulting to1. Confirm this is intentional—setting it to0means the entrypoint will proceed with normal startup rather than check-only mode when privilege drop fails.This aligns with the fail-soft philosophy but changes the behavior for environments where
su-execcannot work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/docker_tests/test_docker_compose_scenarios.py (1)
79-80: Use numeric types for timeout constants.The timeout constants are defined as strings but are used in contexts expecting integers (e.g., line 260's
timeout: intparameter and line 323'stime.sleep()). While Python's duck typing allows this to work, it violates type hints and Python conventions for numeric values.🔎 Proposed fix
-COMPOSE_PORT_WAIT_TIMEOUT = "30" -COMPOSE_SETTLE_WAIT_SECONDS = "20" +COMPOSE_PORT_WAIT_TIMEOUT = 30 +COMPOSE_SETTLE_WAIT_SECONDS = 20
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymltest/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.pytest/docker_tests/test_mount_diagnostics_pytest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/docker_tests/test_mount_diagnostics_pytest.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: For time utilities useserver/utils/datetime_utils.py(timeNowDB); for MAC addresses usefront/plugins/plugin_helper.py(normalize_mac); for string sanitization useserver/helper.pysanitizers. Always validate MACs before database writes
When running subprocesses, ALWAYS set explicit timeouts. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout
Files:
test/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.py
**/*.{py,php,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use environment variables (
NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding/data/dbor relative paths. Use/datafor persistent config/db,/tmpfor runtime logs/api/nginx state
Files:
test/docker_tests/test_container_environment.pytest/docker_tests/test_docker_compose_scenarios.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Never `chmod` or `chown` during operations in devcontainer. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead
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.
📚 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:
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymltest/docker_tests/test_container_environment.py
📚 Learning: 2026-01-04T01:25:43.080Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Applies to **/*.{py,php,js} : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding `/data/db` or relative paths. Use `/data` for persistent config/db, `/tmp` for runtime logs/api/nginx state
Applied to files:
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.ymltest/docker_tests/test_docker_compose_scenarios.py
📚 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:
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.yml
📚 Learning: 2026-01-04T01:25:43.080Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-04T01:25:43.080Z
Learning: Never `chmod` or `chown` during operations in devcontainer. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead
Applied to files:
test/docker_tests/test_container_environment.py
🧬 Code graph analysis (1)
test/docker_tests/test_container_environment.py (1)
test/docker_tests/test_ports_available.py (1)
_assert_contains(174-182)
🪛 Ruff (0.14.10)
test/docker_tests/test_docker_compose_scenarios.py
716-716: subprocess call: check for execution of untrusted input
(S603)
728-728: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.active_config_unwritable.yml (2)
17-18: LGTM! Capabilities align with PUID fix requirements.Adding SETGID and SETUID capabilities is appropriate for testing the root-to-user transition path that this PR introduces. These capabilities enable the entrypoint to drop privileges from root (UID 0) to the application user (UID 20211).
41-45: Excellent permission isolation for tmpfs mounts.The explicit per-directory tmpfs mounts with
uid=20211,gid=20211,mode=1777provide strong permission boundaries for testing. Mode 1777 (sticky bit + rwx) is correct for /tmp directories, and the security flags (noexec, nosuid, nodev) follow container hardening best practices.test/docker_tests/test_container_environment.py (3)
1360-1374: Good alignment with PUID fix testing requirements.The test now correctly exercises the root-to-user permission-fix path by running as
user="0:0"instead of a non-root user. The added assertions ensure the root startup succeeds without permission errors before verification, which validates the permission-fix logic introduced in this PR.
1395-1401: Mandatory logging aids CI debugging.The added logging for the permission-fix verification command and its output provides essential diagnostics for CI failures. This follows the file's mandatory logging policy and is appropriate for debugging permission-related issues.
668-668: Timeout change is safe — all test invocations use sleep_seconds well below SUBPROCESS_TIMEOUT_SECONDS.All actual sleep_seconds values in the test file (5, 8, 15, 20 seconds) are strictly less than the default SUBPROCESS_TIMEOUT_SECONDS of 60 seconds. Therefore,
max(SUBPROCESS_TIMEOUT_SECONDS, sleep_seconds)always evaluates to 60 for all current tests, meaning the removed 30-second buffer was never actually applied and has no practical impact on test reliability.Likely an incorrect or invalid review comment.
test/docker_tests/test_docker_compose_scenarios.py (3)
702-728: Well-structured helper for failure detection.The
_wait_for_unwritable_failurefunction appropriately polls the container state until exit or timeout. The 45-second deadline and 2-second poll interval balance responsiveness with overhead. The mandatory logging of poll results aids CI debugging.
699-742: Test correctly validates unwritable nginx config scenario.The changes properly set up the test to detect nginx config write failures:
- Clearing stale port success data ensures clean test state
- Detached mode with the failure-wait hook correctly waits for container exit
NETALERTX_CHECK_ONLY=0enables full startup to exercise nginx config generation- 8-second timeout is appropriate since the test expects quick failure
745-752: Specific assertions improve test reliability.The updated assertions check for exact error messages from the unwritable nginx config scenario, making test failures easier to diagnose. The ANSI escape sanitization improves log readability.
2 participants
This addresses some issues during startup with PUID support and root-entrypoint.
Summary by CodeRabbit
Chores
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.