Easy permissions

[adamoutler]

Fixes discord post: https://discord.com/channels/1274490466481602755/1432752989872848896/1432752989872848896

This PR addresses a critical user experience issue for NetAlertX users migrating from previous versions that allowed running as root or any UID. With the new security constraints requiring UID 20211, existing deployments may have incorrect file permissions that prevent proper operation.

Primary Solution: One-Time Root Permission Fix

When a container starts as root (common during migration), NetAlertX now:

• Automatically detects root execution (modifying existing logic) and provides a clear security warning.
• Fixes all file ownership and permissions for critical paths (db, config, log, etc.) to UID 20211.
• Guides users to restart with the proper UID 20211 for ongoing operation.
• Hangs indefinitely (sleep infinity) after corrections, forcing a manual restart.

This enables a seamless migration path: run once as root to fix permissions, then switch to UID 20211 for secure, ongoing use.

Migration Workflow

1. User starts container as root (existing configuration)
2. NetAlertX detects root, displays warning, fixes permissions
3. Container hangs with guidance to stop and restart as UID 20211. User may docker copy files in or out of the container using this mode.
4. User updates configuration to use UID 20211:20211
5. Container starts normally with correct permissions

Supporting Changes

• Enhanced Permission Script:
  • Modified the existing root detection logic to improve the user experience.
  • Updated the warning message to use magenta for high visibility.
  • Refined the existing chown and chmod logic to explicitly set ownership to 20211 and permissions to u+rwx (user-only), ensuring least-required permissions.
• Devcontainer Updates:
  • Added docker-cli-compose for improved development workflow
• Test Coverage:
  • Updated tests to validate root detection and permission fixing
  • Corrected user IDs to match new security requirements
  • Added assertions for new warning messages and exit behaviors

Security Benefits

• Maintains security by requiring UID 20211 for normal operation
• Provides safe migration path without permanent root access
• Ensures consistent, least-required permissions across all deployments

Backward Compatibility

• Existing root-based configs continue to work for permission fixing
• No breaking changes for properly configured UID 20211 deployments
• Clear migration path prevents user frustration

This PR transforms a potential migration blocker into a guided, secure upgrade experience.

How to correct permissions:

docker run -it --rm --name netalertx --user "0" \
  -v netalertx_config:/app/config \
  -v netalertx_db:/app/db \
  netalertx:latest

Summary by CodeRabbit

• Bug Fixes

  • Fixed application execution to prevent running as root, improving security posture.
  • Corrected permission handling for read-write application paths with proper ownership assignment.

• Chores

  • Updated development container configuration with additional Docker tools.

[coderabbitai]

Walkthrough

Adds docker-cli-compose to devcontainer installations. Refactors permission-check script to enforce non-root execution with security alerts and granular permission handling. Extensively updates container tests to run as netalertx user with revised expectations for output messages and exit codes.

Changes

Cohort / File(s)	Summary
Devcontainer Setup .devcontainer/Dockerfile, .devcontainer/resources/devcontainer-Dockerfile	Added docker-cli-compose package to APK install command in both Dockerfile images, extending the development tools available in the devcontainer environment.
Permission Enforcement & Security install/production-filesystem/services/scripts/check-app-permissions.sh	Introduced security alert (MAGENTA color) for root execution. Changed ownership model from dual (netalertx:netalertx) to single (netalertx). Updated permission commands to use granular per-item modes (u+rwx for directories, u+rw for files) with error suppression. Replaced dirname calls with explicit path variables. Added remediation block and exit with status 211 when running as root.
Container Environment Tests test/docker_tests/test_container_environment.py	Migrated all test scenarios from root-based execution to netalertx user context (UID 20211:20211). Replaced dynamic seeding with fixed mount tree setup. Updated expected output messages to reflect new permission-check behavior and security alerts. Revised exit code expectations and assertions to match non-root execution model.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Permission script (check-app-permissions.sh): Logic density around new security alerts, ownership changes, and permission handling requires careful validation that security-critical paths are correct.
• Test file (test_container_environment.py): Extensive updates across multiple test cases; each test's revised expectations and setup changes need individual verification for consistency with the new non-root model.
• Devcontainer files: Trivial package additions; minimal review effort.

Poem

🐰 A compose tool joins the devcontainer's nest,
While permissions dance to a non-root request,
Security magenta blooms with alerts so bright,
Tests now run as netalertx—no more root's might!
Granular modes and fixed trees unite,
The container sings with newfound tight! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title Check	❓ Inconclusive	The title "Easy permissions" is related to the changeset in that it references permissions, which are indeed a core part of the changes. The PR modifies permission handling, detection, and fixing logic. However, the title is quite vague and generic, lacking specificity about what the actual changes accomplish. A teammate scanning PR history would not clearly understand from this title that the change involves root container detection, automatic permission remediation, migration support from root-based deployments, or enforcement of non-root operation. The title is too broad to convey meaningful information about the primary change.	Consider using a more specific and descriptive title that clarifies the main change, such as "Auto-fix permissions when container runs as root" or "Support migration from root to non-root container deployments" or "Add root detection and automatic permission remediation". This would help teammates quickly understand the nature and purpose of the change from the PR history.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

install/production-filesystem/services/scripts/check-app-permissions.sh (1)

82-109: Bug: failures won’t propagate due to pipeline subshell

Using echo ... | while ...; do failures=1; done assigns failures in a subshell; the outer failures remains 0, masking errors.

Replace with a non-pipeline loop:

-echo "${READ_ONLY_PATHS}" | while IFS= read -r path; do  
-    [ -z "$path" ] && continue
-    if [ ! -e "$path" ]; then
+for path in ${READ_ONLY_PATHS}; do
+    [ -n "$path" ] || continue
+    if [ ! -e "$path" ]; then
         failures=1
         >&2 printf "%s" "${RED}"
         >&2 cat <<EOF
@@
         >&2 printf "%s" "${RESET}"
-    elif [ ! -r "$path" ]; then
+    elif [ ! -r "$path" ]; then
         failures=1
         >&2 printf "%s" "${YELLOW}"
         >&2 cat <<EOF
@@
         >&2 printf "%s" "${RESET}"
     fi
-done
+done

🧹 Nitpick comments (4)

install/production-filesystem/services/scripts/check-app-permissions.sh (3)

42-65: Good: high-visibility, stderr-only root alert

Clear banner, actionable guidance, and no variable expansion in heredoc. Minor: wording “actively trying to get pwned” may be too informal for some environments.

---

111-129: Quote paths in write checks; tolerate empty entries

Minor hardening: quote $path and continue on empties for symmetry with read-only loop.

-for path in $READ_WRITE_PATHS; do
-    if [ -e "$path" ] && [ ! -w "$path" ]; then
+for path in ${READ_WRITE_PATHS}; do
+    [ -n "$path" ] || continue
+    if [ -e "$path" ] && [ ! -w "$path" ]; then
         failures=1
         >&2 printf "%s" "${YELLOW}"
         >&2 cat <<EOF

---

20-27: Potential false-positive on VIRTUAL_ENV

If VIRTUAL_ENV isn’t set in production, this will flag “Path does not exist.” Consider gating on non-empty before including in READ_ONLY_PATHS.

test/docker_tests/test_container_environment.py (1)

823-836: Root run test: return code assumption may be brittle

Test expects rc==0 after SIGTERM because the harness converts 143→0. If the entrypoint or permission script starts returning 211 on termination, this will fail. Either pin the behavior in docs or assert on the presence of the banner plus “Permissions fixed…” and allow rc∈{0,211}.

Example:

-    assert result.returncode == 0 # container must be forced to exit 0 by termination after warning
+    assert result.returncode in (0, 211)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5962312 and 8d4c7ea.

📒 Files selected for processing (4)

• .devcontainer/Dockerfile (1 hunks)
• .devcontainer/resources/devcontainer-Dockerfile (1 hunks)
• install/production-filesystem/services/scripts/check-app-permissions.sh (2 hunks)
• test/docker_tests/test_container_environment.py (5 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.

Files:

• test/docker_tests/test_container_environment.py

test/**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Tests must reside under 'test/' and use pytest.

Files:

• test/docker_tests/test_container_environment.py

🧠 Learnings (2)

📓 Common learnings

Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.613Z
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
PR: jokob-sk/NetAlertX#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/scripts/check-app-permissions.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 (9)

.devcontainer/Dockerfile (1)

227-228: LGTM! docker-cli-compose addition supports devcontainer workflows.

The addition of docker-cli-compose is appropriately grouped with other Docker CLI tools and supports multi-container development workflows in the devcontainer environment.

.devcontainer/resources/devcontainer-Dockerfile (1)

21-22: LGTM! Correct addition to devcontainer source.

The docker-cli-compose package is correctly added to the source file, enabling Docker Compose functionality in the devcontainer. The generated .devcontainer/Dockerfile reflects this change appropriately.

install/production-filesystem/services/scripts/check-app-permissions.sh (3)

15-16: LGTM: color additions

MAGENTA + RESET usage is correct and scoped to stderr messages.

---

35-37: LGTM: include specific files in READ_WRITE_PATHS

Adding NETALERTX_CONFIG_FILE and NETALERTX_DB_FILE ensures single-file mounts get fixed too.

---

66-74: Harden the root remediation: quote paths, per-path iteration, and trap for deterministic exit

The shell hardening concerns identified are substantively valid:

1. Word-splitting vulnerability: READ_WRITE_PATHS (lines 30–37) contains newlines and variable expansions. The unquoted ${READ_WRITE_PATHS} at lines 67, 70–71 will split on all whitespace, treating each path fragment as a separate argument.

2. Quoting: Wrapping with quotes (e.g., "${READ_WRITE_PATHS}") or iterating with proper quoting is required to preserve path integrity.

3. Exit code on SIGTERM: Without a trap, sleep infinity & wait $!; exit 211 exits with code 143 (128 + SIGTERM signal 15) when the container receives SIGTERM. The proposed trap 'exit 211' TERM INT ensures code 211 is returned to orchestrators.

Apply the suggested diff for robust remediation.

However, I cannot verify the test harness behavior: No test files or exit-code conversion logic were found in the codebase. Before merging, confirm that container stop tests handle the new exit code 211 correctly and that the orchestration layer (if applicable) expects this non-zero signal.

test/docker_tests/test_container_environment.py (4)

856-857: LGTM: wrong user warning message assertion

Asserts the exact UID:GID message; aligns with new user-check script.

---

889-895: LGTM: fixed mount tree + chown for config seeding

Deterministic setup reduces flakiness from repo-relative paths.

---

906-913: LGTM: fixed mount tree + chown for DB seeding

Same benefits as config seeding; explicit user improves reproducibility.

---

232-247: Nice diagnostics: list mount perms before entrypoint

This helps triage failures without reruns. Keep it.