Easy permissions - #1248
Conversation
WalkthroughAdds 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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 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 subshellUsing
echo ... | while ...; do failures=1; doneassignsfailuresin a subshell; the outerfailuresremains 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 alertClear 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 entriesMinor hardening: quote
$pathand 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_ENVIf 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 brittleTest 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
📒 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-composeis 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-composepackage is correctly added to the source file, enabling Docker Compose functionality in the devcontainer. The generated.devcontainer/Dockerfilereflects this change appropriately.install/production-filesystem/services/scripts/check-app-permissions.sh (3)
15-16: LGTM: color additionsMAGENTA + RESET usage is correct and scoped to stderr messages.
35-37: LGTM: include specific files in READ_WRITE_PATHSAdding 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 exitThe shell hardening concerns identified are substantively valid:
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.Quoting: Wrapping with quotes (e.g.,
"${READ_WRITE_PATHS}") or iterating with proper quoting is required to preserve path integrity.Exit code on SIGTERM: Without a trap,
sleep infinity & wait $!; exit 211exits with code 143 (128 + SIGTERM signal 15) when the container receives SIGTERM. The proposedtrap 'exit 211' TERM INTensures 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 assertionAsserts the exact UID:GID message; aligns with new user-check script.
889-895: LGTM: fixed mount tree + chown for config seedingDeterministic setup reduces flakiness from repo-relative paths.
906-913: LGTM: fixed mount tree + chown for DB seedingSame benefits as config seeding; explicit user improves reproducibility.
232-247: Nice diagnostics: list mount perms before entrypointThis helps triage failures without reruns. Keep it.
2 participants
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:
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
Supporting Changes
chownandchmodlogic to explicitly set ownership to 20211 and permissions tou+rwx(user-only), ensuring least-required permissions.docker-cli-composefor improved development workflowSecurity Benefits
Backward Compatibility
This PR transforms a potential migration blocker into a guided, secure upgrade experience.
How to correct permissions:
Summary by CodeRabbit
Bug Fixes
Chores