Allow other users (Non-Synology) #1363
Conversation
WalkthroughReplaces hard-coded container UIDs/GIDs with build- and runtime-configurable ARGs/ENV (NETALERTX_UID/GID, READONLY_UID/GID); updates Dockerfiles, docker-compose, entrypoint scripts, docs, tests, and devcontainer to use the new ownership model and adjusts permission, venv copy, first-run cleanup, and mount diagnostics accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Build stage
participant Image as Runner image
participant Orchestrator as Docker / Compose
participant Entrypoint as Container entrypoint scripts
participant MountDiag as Mount diagnostics
participant App as NetAlertX application
Builder->>Image: Accept ARGs NETALERTX_UID/GID, READONLY_UID/GID
Image->>Image: create group/user with NETALERTX_UID/GID
Image->>Image: COPY venv & version files chown=${READONLY_UID}:${READONLY_GID}
Image->>Image: create READ_WRITE_FOLDERS (mode 0777)
Orchestrator->>Image: run container (user env via NETALERTX_UID/GID)
Entrypoint->>Entrypoint: honor SKIP_TESTS / per-script skip rules
Entrypoint->>MountDiag: run mount diagnostics and write-test
MountDiag-->>Entrypoint: report writable state
Entrypoint->>Entrypoint: apply TARGET_USER ownership where needed
Entrypoint->>Entrypoint: derive PORT_GQL (from APP_CONF_OVERRIDE or env) and check conflicts
Entrypoint->>App: start service with verified permissions and ports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
install/production-filesystem/entrypoint.d/30-apply-conf-override.sh (1)
1-33: Critical: Script cannot be executed by netalertx user due to permission model.The hardened stage applies
chmod 005to/entrypoint.dscripts, making them executable only for the "others" category. However, scripts are owned by thereadonlyuser, and the container runs asnetalertx(UID 20211), which is neither the owner nor in the readonly group. This causes exit code 126 (permission denied) at container startup.Either grant
netalertxownership of the scripts, add it to the readonly group, or adjust the permission model to allownetalertxexecution.install/production-filesystem/entrypoint.sh (1)
330-334: Potential shell arithmetic error whenFAILED_STATUSis empty.
FAILED_STATUSis initialized to""(line 68), but the comparison at line 332 uses-eq 0, which expects an integer. If we reach this point without any failure,FAILED_STATUSwill be empty and the comparison may fail with a shell error.🔎 Proposed fix
-if [ "${FAILED_STATUS}" -eq 0 ] && [ "${FAILED_NAME}" != "signal" ]; then +if [ "${FAILED_STATUS:-0}" -eq 0 ] && [ "${FAILED_NAME}" != "signal" ]; then
🧹 Nitpick comments (9)
install/production-filesystem/entrypoint.d/10-mounts.py (1)
98-112: Enhanced writability check with actual write test.The improved logic now performs an actual write test when the target is a directory, addressing OverlayFS/copy-up scenarios where
os.access()may incorrectly report writability. The test file is properly cleaned up after verification.Optional: Make the control flow more explicit
The static analysis tool suggests making the final
return Truepart of an explicitelseblock for clarity. While the current logic is correct, this would make the intent more obvious:if os.path.isdir(current): test_file = os.path.join(current, f".netalertx_write_test_{os.getpid()}") try: with open(test_file, "w") as f: f.write("test") os.remove(test_file) return True except OSError: return False - - return True + else: + return TrueThis is purely a style preference and doesn't affect functionality.
install/production-filesystem/entrypoint.d/99-ports-available.sh (1)
9-14: Crude JSON parsing may fail for valid configurations.The regex
GRAPHQL_PORT"*[:=]\"*[0-9]\+is fragile and may miss valid JSON formats with whitespace, nested structures, or alternate quoting. For example,"GRAPHQL_PORT": 20212(with space after colon) won't match.Consider a more robust pattern:
🔎 Suggested improvement
# crude parse: look for GRAPHQL_PORT in JSON-like string - PORT_GQL=$(printf '%s' "${APP_CONF_OVERRIDE}" | grep -o 'GRAPHQL_PORT"*[:=]\"*[0-9]\+' | tr -cd '0-9' || true) + PORT_GQL=$(printf '%s' "${APP_CONF_OVERRIDE}" | grep -oE 'GRAPHQL_PORT"?\s*[:=]\s*"?[0-9]+' | tr -cd '0-9' || true)docker-compose.yml (1)
49-54: Consider updating the comment to reflect dynamic UID/GID.Lines 51-52 still reference hardcoded
uid=20211andgid=20211, but the actual values are now dynamic. Consider updating for clarity.🔎 Suggested update
# tmpfs mounts for writable directories in a read-only container and improve system performance # All writes now live under /tmp/* subdirectories which are created dynamically by entrypoint.d scripts - # uid=20211 and gid=20211 is the netalertx user inside the container - # mode=1700 gives rwx------ permissions to the netalertx user only + # uid/gid default to 20211 (netalertx user) but can be overridden via NETALERTX_UID/NETALERTX_GID + # mode=1700 gives rwx------ permissions to the configured user only tmpfs:install/production-filesystem/entrypoint.d/0-storage-permission.sh (1)
63-63: Quote the echo message to prevent word splitting.If
TARGET_USERcontains spaces or special characters, this could produce unexpected output.🔎 Proposed fix
- echo Permissions fixed for read-write paths. Please restart the container as user ${TARGET_USER}. + echo "Permissions fixed for read-write paths. Please restart the container as user ${TARGET_USER}."test/docker_tests/test_entrypoint_security.py (2)
4-31: Consider removing unusedtmp_pathfixture parameters.The
tmp_pathparameter is unused in all three test functions. While not a functional issue, removing them would clean up the code.🔎 Proposed fix
-def test_entrypoint_injection_puid(tmp_path): +def test_entrypoint_injection_puid(): """Attempt to inject commands via PUID environment variable.""" ... -def test_entrypoint_injection_tz(tmp_path): +def test_entrypoint_injection_tz(): """Attempt to inject commands via TZ environment variable.""" ... -def test_entrypoint_fuzzing_puid(tmp_path): +def test_entrypoint_fuzzing_puid(): """Fuzz PUID with random strings.""" ...
33-34: Move imports to the top of the file.Imports for
randomandstringshould be at the top of the file with other imports, per PEP 8.🔎 Proposed fix
import pytest +import random +import string from .test_container_environment import _run_container, IMAGE ... -import random -import string - def test_entrypoint_fuzzing_puid(tmp_path):test/docker_tests/test_container_environment.py (3)
1168-1168: Remove unused variabledata_host_path.This variable is assigned but never used.
🔎 Proposed fix
- data_host_path = paths["data"] - check_cmd = [
1178-1179: Rename unused loop variablereadonlyto_readonly.The
readonlyvariable is not used within the loop body.🔎 Proposed fix
- for host_path, target, readonly in volumes: + for host_path, target, _readonly in volumes: check_cmd.extend(["-v", f"{host_path}:{target}"])
1111-1188: Good addition of restrictive permissions handling test.This test properly validates both the non-root failure case and the root permission-fix case. The workaround for potential devcontainer hang by avoiding
--userns hostin the helper is pragmatic.Note: The
_chown_root_safehelper duplicates logic from_chown_path. Consider reusing_chown_pathwith an optionaluserns_modeparameter if this pattern is needed elsewhere.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.devcontainer/Dockerfile(9 hunks).devcontainer/resources/devcontainer-Dockerfile(1 hunks)Dockerfile(6 hunks)docker-compose.yml(2 hunks)docs/DOCKER_COMPOSE.md(2 hunks)docs/DOCKER_INSTALLATION.md(2 hunks)docs/docker-troubleshooting/incorrect-user.md(1 hunks)front/plugins/plugin_helper.py(1 hunks)install/production-filesystem/entrypoint.d/0-storage-permission.sh(2 hunks)install/production-filesystem/entrypoint.d/10-mounts.py(2 hunks)install/production-filesystem/entrypoint.d/15-first-run-config.sh(1 hunks)install/production-filesystem/entrypoint.d/30-apply-conf-override.sh(1 hunks)install/production-filesystem/entrypoint.d/35-writable-config.sh(1 hunks)install/production-filesystem/entrypoint.d/60-user-netalertx.sh(1 hunks)install/production-filesystem/entrypoint.d/99-ports-available.sh(2 hunks)install/production-filesystem/entrypoint.sh(2 hunks)test/docker_tests/debug_storage_permission.sh(1 hunks)test/docker_tests/test_container_environment.py(11 hunks)test/docker_tests/test_entrypoint.py(1 hunks)test/docker_tests/test_entrypoint_security.py(1 hunks)test/docker_tests/test_mount_diagnostics_pytest.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.devcontainer/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never
chmodorchownduring operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix.devcontainer/scripts/setup.shor.devcontainer/resources/devcontainer-Dockerfileinstead.
Files:
.devcontainer/resources/devcontainer-Dockerfile.devcontainer/Dockerfile
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
install/production-filesystem/entrypoint.d/10-mounts.pyfront/plugins/plugin_helper.pytest/docker_tests/test_entrypoint.pytest/docker_tests/test_container_environment.pytest/docker_tests/test_mount_diagnostics_pytest.pytest/docker_tests/test_entrypoint_security.py
🧠 Learnings (20)
📓 Common learnings
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:
install/production-filesystem/entrypoint.d/0-storage-permission.shinstall/production-filesystem/entrypoint.d/15-first-run-config.shtest/docker_tests/debug_storage_permission.shdocs/docker-troubleshooting/incorrect-user.mdinstall/production-filesystem/entrypoint.d/60-user-netalertx.shdocs/DOCKER_COMPOSE.mddocker-compose.ymldocs/DOCKER_INSTALLATION.md.devcontainer/DockerfileDockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.
Applied to files:
install/production-filesystem/entrypoint.d/0-storage-permission.shdocs/DOCKER_COMPOSE.mddocker-compose.ymldocs/DOCKER_INSTALLATION.md.devcontainer/Dockerfile
📚 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/0-storage-permission.shtest/docker_tests/debug_storage_permission.shdocs/docker-troubleshooting/incorrect-user.mddocs/DOCKER_COMPOSE.mddocker-compose.ymldocs/DOCKER_INSTALLATION.md.devcontainer/DockerfileDockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to .devcontainer/**/* : Never `chmod` or `chown` during operations in devcontainer. Everything is already writable. 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/0-storage-permission.shtest/docker_tests/debug_storage_permission.sh.devcontainer/DockerfileDockerfile
📚 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/35-writable-config.shinstall/production-filesystem/entrypoint.d/15-first-run-config.shinstall/production-filesystem/entrypoint.d/60-user-netalertx.shinstall/production-filesystem/entrypoint.sh
📚 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 repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile.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:
.devcontainer/resources/devcontainer-Dockerfile.devcontainer/DockerfileDockerfile
📚 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:
.devcontainer/resources/devcontainer-Dockerfiledocs/DOCKER_COMPOSE.mddocker-compose.yml.devcontainer/DockerfileDockerfile
📚 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:
.devcontainer/resources/devcontainer-Dockerfile.devcontainer/Dockerfile
📚 Learning: 2025-10-10T22:16:02.770Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1214
File: install/ubuntu24/uninstall.sh:129-141
Timestamp: 2025-10-10T22:16:02.770Z
Learning: NetAlertX uninstall procedures should only remove files from the repository (specifically /app files) and should not touch system packages like PHP, nginx, avahi, or other shared system components to avoid damaging user systems.
Applied to files:
install/production-filesystem/entrypoint.d/15-first-run-config.shinstall/production-filesystem/entrypoint.d/60-user-netalertx.sh.devcontainer/DockerfileDockerfile
📚 Learning: 2025-11-23T16:05:31.339Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1302
File: back/cron_script.sh:6-18
Timestamp: 2025-11-23T16:05:31.339Z
Learning: The `/back/cron_script.sh` file is legacy code maintained only for compatibility with other components. The active/production version is in `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` directory are typically outside the scope of PRs unless specifically addressing compatibility.
Applied to files:
install/production-filesystem/entrypoint.d/15-first-run-config.shinstall/production-filesystem/entrypoint.sh
📚 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/15-first-run-config.shinstall/production-filesystem/entrypoint.d/60-user-netalertx.shinstall/production-filesystem/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:
test/docker_tests/debug_storage_permission.shinstall/production-filesystem/entrypoint.d/60-user-netalertx.shinstall/production-filesystem/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:
docs/docker-troubleshooting/incorrect-user.mdinstall/production-filesystem/entrypoint.d/60-user-netalertx.sh
📚 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:
docs/DOCKER_COMPOSE.mddocker-compose.yml.devcontainer/Dockerfile
📚 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:
docker-compose.yml.devcontainer/DockerfileDockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Devcontainer philosophy: brutal simplicity. One user, everything writable, completely idempotent. No permission checks, no conditional logic, no sudo needed.
Applied to files:
.devcontainer/DockerfileDockerfile
📚 Learning: 2025-09-20T14:08:44.152Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:82-92
Timestamp: 2025-09-20T14:08:44.152Z
Learning: In devcontainer builds, source repository files are not available during the Docker build phase. The source code gets mounted into the container after it's built and started, so COPY commands referencing source files will fail. Configuration files need to be handled at runtime (e.g., in setup scripts) rather than during the build stage.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-19T15:29:49.532Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: install/production-filesystem/build/init-nginx.sh:3-3
Timestamp: 2025-10-19T15:29:49.532Z
Learning: In Docker build-time initialization scripts (like those in install/production-filesystem/build/), environment variables set in the Dockerfile do not need explicit validation within the scripts. The Docker build process will fail if required environment variables are undefined, providing implicit validation.
Applied to files:
Dockerfile
🧬 Code graph analysis (2)
test/docker_tests/test_container_environment.py (1)
test/docker_tests/test_ports_available.py (3)
_setup_mount_tree(60-98)_run_container(106-167)_assert_contains(170-178)
test/docker_tests/test_entrypoint_security.py (1)
test/docker_tests/test_container_environment.py (1)
_run_container(434-565)
🪛 GitHub Actions: Code checks
install/production-filesystem/entrypoint.d/30-apply-conf-override.sh
[error] 82-82: Permission denied during container startup: /entrypoint.d/30-apply-conf-override.sh could not be executed (exit code 126).
test/docker_tests/test_entrypoint_security.py
[error] 2-2: ImportError: attempted relative import with no known parent package
🪛 LanguageTool
docs/docker-troubleshooting/incorrect-user.md
[style] ~17-~17: Consider using a different verb for a more formal wording.
Context: ...re owned by a different UID. ## How to Correct the Issue Option A: Use defaults (reco...
(FIX_RESOLVE)
🪛 Ruff (0.14.8)
install/production-filesystem/entrypoint.d/10-mounts.py
108-108: Consider moving this statement to an else block
(TRY300)
test/docker_tests/test_container_environment.py
73-73: Probable insecure usage of temporary file or directory: "/tmp/pytest-docker-mounts"
(S108)
87-87: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
89-90: try-except-pass detected, consider logging the exception
(S110)
89-89: Do not catch blind exception: Exception
(BLE001)
95-95: Unused function argument: tmp_path
(ARG001)
240-240: Avoid specifying long messages outside the exception class
(TRY003)
261-261: subprocess call: check for execution of untrusted input
(S603)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
280-280: subprocess call: check for execution of untrusted input
(S603)
281-281: Starting a process with a partial executable path
(S607)
289-289: subprocess call: check for execution of untrusted input
(S603)
290-290: Starting a process with a partial executable path
(S607)
312-312: subprocess call: check for execution of untrusted input
(S603)
313-326: Starting a process with a partial executable path
(S607)
367-367: subprocess call: check for execution of untrusted input
(S603)
379-379: subprocess call: check for execution of untrusted input
(S603)
380-392: Starting a process with a partial executable path
(S607)
409-409: Unused function argument: tmp_path
(ARG001)
459-459: Do not catch blind exception: Exception
(BLE001)
491-491: Probable insecure usage of temporary file or directory: "/tmp:mode=1700,uid="
(S108)
741-741: Unused function argument: tmp_path
(ARG001)
765-765: Unused function argument: tmp_path
(ARG001)
799-799: Unused function argument: tmp_path
(ARG001)
833-833: Unused function argument: tmp_path
(ARG001)
857-857: Unused function argument: tmp_path
(ARG001)
1131-1131: subprocess call: check for execution of untrusted input
(S603)
1168-1168: Local variable data_host_path is assigned to but never used
Remove assignment to unused variable data_host_path
(F841)
1178-1178: Loop control variable readonly not used within loop body
Rename unused readonly to _readonly
(B007)
1181-1181: subprocess call: check for execution of untrusted input
(S603)
test/docker_tests/test_entrypoint_security.py
4-4: Unused function argument: tmp_path
(ARG001)
19-19: Unused function argument: tmp_path
(ARG001)
36-36: Unused function argument: tmp_path
(ARG001)
39-39: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
41-41: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
⏰ 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 (31)
.devcontainer/resources/devcontainer-Dockerfile (1)
27-27: LGTM - py3-psutil addition.The addition of
py3-psutilto the devcontainer tooling is appropriate for development and testing scenarios where process/system utilities may be needed.front/plugins/plugin_helper.py (1)
40-43: LGTM - Defensive config access.The change from direct dictionary access to
.get()with a fallback prevents KeyError exceptions when TIMEZONE is missing from the config file. The validation that follows (lines 41-42) ensures the value is still checked againstall_timezones.test/docker_tests/test_mount_diagnostics_pytest.py (1)
8-15: LGTM - Future robustness roadmap.The TODO block documents planned test improvements for edge cases (symlink attacks, OverlayFS/copy-up scenarios, and text-based output for broader terminal compatibility). This provides clear guidance for future enhancements.
install/production-filesystem/entrypoint.d/10-mounts.py (1)
3-16: LGTM - Clear module documentation.The module docstring effectively describes the tool's purpose and documents planned enhancements (text-based output, OverlayFS improvements, root-to-user context handling).
install/production-filesystem/entrypoint.d/60-user-netalertx.sh (1)
11-23: Simplified user validation aligns with arbitrary UID/GID support.The changes transform this script from a strict validation check to an informational logger:
Fallback behavior (lines 12, 16): When
getentlookup fails, the script now defaults to the current UID/GID instead of leaving them empty, which prevents spurious mismatches.Always exits successfully (line 23): The script now always exits 0 and prints a brief note regardless of whether IDs match, removing the previous warning behavior.
This aligns with the PR's goal of supporting arbitrary user configurations without strict enforcement of the default
netalertxuser.test/docker_tests/debug_storage_permission.sh (1)
1-66: LGTM - Debug script for testing storage permission scenarios.This debug script appropriately tests the root-to-user permission-fixing workflow in a container environment. Key observations:
- Root-only execution: Properly gates permission fixing behind a root check (line 29)
- Verbose output: Uses
-vflags for debugging visibility (lines 60-63)- Path iteration: Processes each path individually with error handling (lines 57-63)
- Container lifecycle: The
sleep infinitypattern (line 65) keeps the container alive for inspectionThe script serves as a test utility for validating the ownership and permission handling introduced in this PR's dynamic UID/GID support.
install/production-filesystem/entrypoint.d/15-first-run-config.sh (1)
10-10: Execution order clarification: 0-storage-permission.sh handles ownership before config creation.The removal of
-oand-gflags is acceptable. The script0-storage-permission.shruns first and corrects ownership of the config directory if the container starts as root, then halts execution. When restarted as the non-rootnetalertxuser (the recommended and documented approach),app.confis created with correct ownership. The config file is only generated when it doesn't exist, ensuring proper ownership in the standard deployment path..devcontainer/Dockerfile (2)
1-2: Auto-generated file — directing review to source files.Based on learnings, this file is auto-generated by combining the root Dockerfile with
.devcontainer/resources/devcontainer-Dockerfile. Any feedback on the shared portions is provided in the root Dockerfile review.
275-277: Good addition ofpy3-psutilfor testing.Adding
py3-psutilaligns with the PR objective of fixing a previously missing test dependency that caused tests to be skipped.Dockerfile (4)
53-58: Good parameterization of UID/GID.The new build arguments enable flexible UID/GID configuration at build time while maintaining sensible defaults. The separation of
NETALERTX_UID/GID(runtime service account) fromREADONLY_UID/GID(lock owner) is a clean design that prevents UID/GID collisions.
221-221: Intentional permission relaxation for arbitrary UID support.Changing to mode
0777allows containers to run with arbitraryuser:settings (e.g.,user: "1000:1000"). This is appropriate given the read-only container filesystem and the PR's goal of supporting non-Synology UIDs. The container isolation provides the security boundary.
224-231: Good practice: cleaning first-run artifacts.Removing
app.confand database files prevents Docker volume copy-up from persisting build-time artifacts with restrictive ownership into fresh named volumes. This ensures arbitrary UID/GID runs work correctly on first start.
196-210: ARG re-declaration is correct.Docker ARGs don't persist across stages, so re-declaring them in the hardened stage is necessary. The readonly user creation correctly uses the parameterized UIDs.
docker-compose.yml (1)
11-12: Good: Runtime UID/GID is now configurable.Using
${NETALERTX_UID:-20211}:${NETALERTX_GID:-20211}allows users to run the container with custom UIDs (e.g.,NETALERTX_UID=1000) while maintaining backward compatibility with the default values.install/production-filesystem/entrypoint.d/0-storage-permission.sh (1)
26-27: LGTM!The
TARGET_USERvariable correctly defaults tonetalertxwhile allowing override viaNETALERTX_USERenvironment variable. This aligns with the PR's goal of supporting arbitrary user IDs.test/docker_tests/test_entrypoint.py (1)
50-57: LGTM!Test expectations correctly updated to match the new entrypoint behavior where
APP_CONF_OVERRIDEdetection message is emitted to stderr rather than stdout. The separation of assertions for stdout and stderr is appropriate.docs/DOCKER_COMPOSE.md (2)
54-58: LGTM!Documentation correctly updated to reflect environment-variable-driven UID/GID handling for tmpfs mounts. The comments explain the default service user and permission mode clearly.
97-98: LGTM!The new TIP block provides clear guidance on runtime UID/GID alignment for users who override the default service user. This is helpful for preventing permission issues.
docs/DOCKER_INSTALLATION.md (2)
30-37: LGTM!Documentation correctly updated to use environment-variable-driven UID/GID values in the
docker runexample. The explanatory note about runtime UID/GID and the readonly lock owner provides clear guidance for users.
88-89: LGTM!The
chowncommand example correctly uses environment variable interpolation with defaults, matching the approach used elsewhere in the documentation.docs/docker-troubleshooting/incorrect-user.md (1)
5-31: LGTM!The troubleshooting documentation is well-structured with clear explanations of the issue, security ramifications, root causes, and two actionable correction options. The resource links point to the appropriate documentation.
install/production-filesystem/entrypoint.sh (3)
53-62: LGTM!Clean refactoring of the banner rendering. Using a variable for the ASCII art and a single
printfcall with format specifiers is cleaner and easier to maintain.
71-80: LGTM!The
SKIP_TESTSandSKIP_STARTUP_CHECKSenvironment variables provide useful flexibility for testing and troubleshooting. The per-script skip logic usinggreppattern matching is appropriate.
121-124: LGTM!The
APP_CONF_OVERRIDEis now exported without printing to stdout, which aligns with the test expectation changes intest_entrypoint.py. The logic correctly only sets the override whenGRAPHQL_PORTis set andAPP_CONF_OVERRIDEis not already defined.test/docker_tests/test_container_environment.py (7)
48-62: LGTM!The
_repo_root()function correctly discovers the repository root using multiple fallback strategies: environment variable, pyproject.toml, .git directory, or back/db directories.
65-91: LGTM!The
_docker_visible_tmp_root()and_docker_visible_path()functions correctly handle the devcontainer scenario where pytest's tmp_path may not be visible to the Docker daemon. Using/tmp/pytest-docker-mountsis acceptable for test infrastructure.
236-268: LGTM!The
_chown_path()helper correctly uses Docker with--userns hostand root user to change ownership of host paths, which is necessary for testing ownership scenarios across different UID/GID combinations.
297-331: LGTM!The volume management helpers (
_fresh_named_volume,_ensure_volume_copy_up,_seed_volume_text_file) provide clean abstractions for docker-based test orchestration. The pattern of using helper containers with Alpine for volume operations is appropriate.
401-431: LGTM!The
test_nonroot_custom_uid_logs_noteparametrized test correctly validates that arbitrary non-root UID/GID combinations can run with compose-like mounts and produce the expected note message.
452-463: LGTM!The dynamic tmpfs UID/GID parsing from the
userparameter is a good enhancement that matches the docker-compose UX. The fallback to defaults (20211) when parsing fails is appropriate.
474-484: LGTM!Dynamically assigning random ports for
PORTandGRAPHQL_PORTwhen running in host network mode avoids flakiness due to port conflicts. The collision check between the two ports is a nice touch.
| import pytest | ||
| from .test_container_environment import _run_container, IMAGE |
There was a problem hiding this comment.
Fix import to resolve pipeline failure.
The pipeline reports an ImportError: attempted relative import with no known parent package. This occurs when the test file is run directly or when the package structure isn't recognized.
🔎 Proposed fix
import pytest
-from .test_container_environment import _run_container, IMAGE
+try:
+ from .test_container_environment import _run_container, IMAGE
+except ImportError:
+ from test_container_environment import _run_container, IMAGE🧰 Tools
🪛 GitHub Actions: Code checks
[error] 2-2: ImportError: attempted relative import with no known parent package
🤖 Prompt for AI Agents
In test/docker_tests/test_entrypoint_security.py lines 1-2, the file uses a
relative import "from .test_container_environment import _run_container, IMAGE"
which causes ImportError when run directly; change the import to an absolute one
(e.g. "from test.test_container_environment import _run_container, IMAGE") or
otherwise reference the module by its full package path so tests run both under
pytest and when invoked directly; update the import line accordingly and ensure
tests' package layout or PYTHONPATH is adjusted if you prefer the package-style
import.
| def test_entrypoint_fuzzing_puid(tmp_path): | ||
| """Fuzz PUID with random strings.""" | ||
| for _ in range(3): | ||
| fuzz = ''.join(random.choices(string.ascii_letters + string.digits + string.punctuation, k=20)) | ||
|
|
||
| result = _run_container( | ||
| "fuzz-puid", | ||
| volumes=[], | ||
| env={"PUID": fuzz}, | ||
| sleep_seconds=1 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fuzzing test result is never checked.
The result variable is assigned but never used. Add assertions to verify the container didn't crash or produce unexpected output.
🔎 Proposed fix
def test_entrypoint_fuzzing_puid(tmp_path):
"""Fuzz PUID with random strings."""
for _ in range(3):
fuzz = ''.join(random.choices(string.ascii_letters + string.digits + string.punctuation, k=20))
result = _run_container(
"fuzz-puid",
volumes=[],
env={"PUID": fuzz},
sleep_seconds=1
)
+ # Ensure fuzzing doesn't cause injection or crash
+ assert "INJECTED" not in (result.stdout or "")
+ assert "INJECTED" not in (result.stderr or "")🧰 Tools
🪛 Ruff (0.14.8)
36-36: Unused function argument: tmp_path
(ARG001)
39-39: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
41-41: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
In test/docker_tests/test_entrypoint_security.py around lines 36 to 47, the
fuzzing test assigns result = _run_container(...) but never asserts anything;
add assertions to validate the container did not crash and produced expected
output. After the _run_container call, assert the container exit code is zero
(or matches the expected code), assert stderr is empty or does not contain
keywords like "traceback" or "error", and optionally assert stdout contains the
expected startup marker; include these assertions inside the loop so each fuzz
input is verified.
adamoutler
force-pushed
the
allow-other-users
branch
2 times, most recently
from
53eb5b3 to
d951921
Compare
adamoutler
force-pushed
the
allow-other-users
branch
from
d951921 to
f9b7249
Compare
|
Force push to remove security testing added. It was experimental tests which are no intended for production. |
There was a problem hiding this comment.
Actionable comments posted: 9
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/entrypoint.sh (1)
71-80: Usegrep -winstead of\bfor POSIX-compatible word boundary matching.Line 77 uses
\bword boundaries, which are not supported by POSIX grep or BusyBox grep (Alpine Linux). Sincescript_namecan contain spaces (from the sed transformation on line 75), the pattern\bnetwork check\bwill fail on Alpine. BusyBox grep supports the -w flag for whole word matching. Replacegrep -q "\b${script_name}\b"withgrep -qw "${script_name}"to ensure compatibility across all environments.
🧹 Nitpick comments (7)
install/production-filesystem/entrypoint.d/10-mounts.py (2)
104-110: Ensure test file cleanup even on removal failure.If
os.remove()raises an exception, the test file will remain in the filesystem. Consider using a try-finally block to ensure cleanup.🔎 Proposed fix for robust cleanup
if os.path.isdir(current): test_file = os.path.join(current, f".netalertx_write_test_{os.getpid()}") try: with open(test_file, "w") as f: f.write("test") - os.remove(test_file) - return True + try: + os.remove(test_file) + return True + finally: + # Ensure cleanup attempt even if remove fails + if os.path.exists(test_file): + try: + os.remove(test_file) + except OSError: + pass except OSError: return FalseAlternatively, simplify with unconditional cleanup in a finally block:
if os.path.isdir(current): test_file = os.path.join(current, f".netalertx_write_test_{os.getpid()}") + file_created = False try: with open(test_file, "w") as f: f.write("test") - os.remove(test_file) + file_created = True return True except OSError: return False + finally: + if file_created: + try: + os.remove(test_file) + except OSError: + pass
108-108: Optional: Consider static analysis suggestion for code structure.Ruff suggests moving the
return Truestatement to anelseblock after the try-except for improved code structure, though the current implementation is functionally correct.🔎 Alternative structure per static analysis
test_file = os.path.join(current, f".netalertx_write_test_{os.getpid()}") try: with open(test_file, "w") as f: f.write("test") os.remove(test_file) - return True except OSError: return False + else: + return Truetest/docker_tests/test_container_environment.py (4)
75-79: Consider logging the permission error for debugging.The silent exception handling here could make it harder to diagnose test failures if the permission issue causes problems downstream. Even for best-effort operations, consider logging at debug level.
🔎 Proposed fix to add logging
try: root.chmod(0o777) except PermissionError: - # Best-effort; the directory only needs to be writable by the current user. - pass + # Best-effort; the directory only needs to be writable by the current user. + print(f"Warning: Could not chmod {root} to 0o777, proceeding anyway")
86-91: Blind exception catching may hide unexpected errors.Catching
Exceptionbroadly can mask bugs. Consider being more specific (e.g., catchingValueErrororAttributeError) or at least logging when the exception occurs.
401-432: Remove unusedtmp_pathparameter.The
tmp_pathparameter is no longer used after switching from bind mounts to named volumes. Consider removing it from the function signature.🔎 Proposed fix
@pytest.mark.parametrize( "uid_gid", [ (1001, 1001), (1502, 1502), ], ) def test_nonroot_custom_uid_logs_note( - tmp_path: pathlib.Path, uid_gid: tuple[int, int], ) -> None:
741-741: Remove unusedtmp_pathparameters from multiple test functions.After switching from bind mounts to named volumes, several test functions no longer use their
tmp_pathparameter:
test_missing_app_conf_triggers_seed(line 741)test_missing_app_db_triggers_seed(line 765)test_custom_port_without_writable_conf(line 799)test_excessive_capabilities_warning(line 833)test_appliance_integrity_read_write_mode(line 857)🔎 Example fix for test_missing_app_conf_triggers_seed
-def test_missing_app_conf_triggers_seed(tmp_path: pathlib.Path) -> None: +def test_missing_app_conf_triggers_seed() -> None:Apply similar changes to the other test functions.
Also applies to: 765-765, 799-799, 833-833, 857-857
.devcontainer/Dockerfile (1)
209-211: Rephrase unprofessional comment.The comment "AI may claim this is stupid" is unprofessional. Consider rephrasing to focus on the technical rationale, e.g., "This ownership-as-a-lock pattern prevents modification by ensuring the readonly user has no login, sudo, or write permissions."
Note: Apply the fix to the source Dockerfile, as this file is auto-generated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.devcontainer/Dockerfile(9 hunks)Dockerfile(6 hunks)front/plugins/plugin_helper.py(1 hunks)install/production-filesystem/entrypoint.d/10-mounts.py(2 hunks)install/production-filesystem/entrypoint.d/15-first-run-config.sh(1 hunks)install/production-filesystem/entrypoint.d/30-apply-conf-override.sh(1 hunks)install/production-filesystem/entrypoint.sh(2 hunks)test/docker_tests/debug_storage_permission.sh(1 hunks)test/docker_tests/test_container_environment.py(11 hunks)test/docker_tests/test_entrypoint.py(1 hunks)test/docker_tests/test_mount_diagnostics_pytest.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/docker_tests/test_mount_diagnostics_pytest.py
- front/plugins/plugin_helper.py
- install/production-filesystem/entrypoint.d/15-first-run-config.sh
- install/production-filesystem/entrypoint.d/30-apply-conf-override.sh
- Dockerfile
🧰 Additional context used
📓 Path-based instructions (2)
.devcontainer/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never
chmodorchownduring operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix.devcontainer/scripts/setup.shor.devcontainer/resources/devcontainer-Dockerfileinstead.
Files:
.devcontainer/Dockerfile
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
test/docker_tests/test_container_environment.pyinstall/production-filesystem/entrypoint.d/10-mounts.pytest/docker_tests/test_entrypoint.py
🧠 Learnings (17)
📚 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:
test/docker_tests/debug_storage_permission.sh.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/debug_storage_permission.sh.devcontainer/Dockerfile
📚 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:
test/docker_tests/debug_storage_permission.sh
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to .devcontainer/**/* : Never `chmod` or `chown` during operations in devcontainer. Everything is already writable. 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/debug_storage_permission.sh.devcontainer/Dockerfile
📚 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:
test/docker_tests/debug_storage_permission.shinstall/production-filesystem/entrypoint.sh
📚 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:
.devcontainer/Dockerfile
📚 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
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Devcontainer philosophy: brutal simplicity. One user, everything writable, completely idempotent. No permission checks, no conditional logic, no sudo needed.
Applied to files:
.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 repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.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:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-10T22:16:02.770Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1214
File: install/ubuntu24/uninstall.sh:129-141
Timestamp: 2025-10-10T22:16:02.770Z
Learning: NetAlertX uninstall procedures should only remove files from the repository (specifically /app files) and should not touch system packages like PHP, nginx, avahi, or other shared system components to avoid damaging user systems.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 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.sh
📚 Learning: 2025-11-23T16:05:31.339Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1302
File: back/cron_script.sh:6-18
Timestamp: 2025-11-23T16:05:31.339Z
Learning: The `/back/cron_script.sh` file is legacy code maintained only for compatibility with other components. The active/production version is in `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` directory are typically outside the scope of PRs unless specifically addressing compatibility.
Applied to files:
install/production-filesystem/entrypoint.sh
📚 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.sh
🧬 Code graph analysis (1)
test/docker_tests/test_container_environment.py (1)
test/docker_tests/test_ports_available.py (3)
_setup_mount_tree(60-98)_run_container(106-167)_assert_contains(170-178)
🪛 Ruff (0.14.8)
test/docker_tests/test_container_environment.py
73-73: Probable insecure usage of temporary file or directory: "/tmp/pytest-docker-mounts"
(S108)
87-87: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
89-90: try-except-pass detected, consider logging the exception
(S110)
89-89: Do not catch blind exception: Exception
(BLE001)
95-95: Unused function argument: tmp_path
(ARG001)
240-240: Avoid specifying long messages outside the exception class
(TRY003)
261-261: subprocess call: check for execution of untrusted input
(S603)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
280-280: subprocess call: check for execution of untrusted input
(S603)
281-281: Starting a process with a partial executable path
(S607)
289-289: subprocess call: check for execution of untrusted input
(S603)
290-290: Starting a process with a partial executable path
(S607)
312-312: subprocess call: check for execution of untrusted input
(S603)
313-326: Starting a process with a partial executable path
(S607)
367-367: subprocess call: check for execution of untrusted input
(S603)
379-379: subprocess call: check for execution of untrusted input
(S603)
380-392: Starting a process with a partial executable path
(S607)
409-409: Unused function argument: tmp_path
(ARG001)
459-459: Do not catch blind exception: Exception
(BLE001)
491-491: Probable insecure usage of temporary file or directory: "/tmp:mode=1700,uid="
(S108)
741-741: Unused function argument: tmp_path
(ARG001)
765-765: Unused function argument: tmp_path
(ARG001)
799-799: Unused function argument: tmp_path
(ARG001)
833-833: Unused function argument: tmp_path
(ARG001)
857-857: Unused function argument: tmp_path
(ARG001)
1131-1131: subprocess call: check for execution of untrusted input
(S603)
1168-1168: Local variable data_host_path is assigned to but never used
Remove assignment to unused variable data_host_path
(F841)
1178-1178: Loop control variable readonly not used within loop body
Rename unused readonly to _readonly
(B007)
1181-1181: subprocess call: check for execution of untrusted input
(S603)
install/production-filesystem/entrypoint.d/10-mounts.py
108-108: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (13)
install/production-filesystem/entrypoint.d/10-mounts.py (1)
3-16: LGTM: Clear documentation of purpose and roadmap.The module docstring effectively explains the tool's purpose and outlines future enhancements, improving maintainability.
test/docker_tests/test_container_environment.py (2)
452-462: LGTM: Good defensive parsing.The user string parsing with fallback to defaults is well-implemented and handles unexpected formats gracefully.
474-485: LGTM: Smart conflict avoidance.The dynamic port allocation for host network mode effectively prevents test flakiness from port conflicts. The collision check between PORT and GRAPHQL_PORT is a nice touch.
install/production-filesystem/entrypoint.sh (2)
53-62: Banner refactoring looks good.The consolidation of the ASCII art into a
NAXvariable and singleprintfstatement improves readability and maintainability.
122-124: Silent APP_CONF_OVERRIDE setting aligns with test expectations.The removal of the echo statement when setting
APP_CONF_OVERRIDEcorrectly matches the updated test expectations intest_entrypoint.py(which now expects the message in stderr from a different script).test/docker_tests/test_entrypoint.py (1)
50-57: Test expectations correctly updated.The test now properly validates that:
APP_CONF_OVERRIDEis set silently inentrypoint.sh(no stdout)- The "APP_CONF_OVERRIDE detected" message appears in stderr from the downstream script
This aligns with the changes in
entrypoint.shwhere the echo was removed..devcontainer/Dockerfile (7)
56-61: LGTM! Parameterized UID/GID enables flexible user configuration.The ARG-based approach correctly enables build-time customization of user and group IDs while maintaining separate readonly lock ownership. This directly supports the PR objective of allowing non-Synology user IDs.
141-143: LGTM! User creation now uses parameterized UIDs/GIDs.The user and group creation correctly uses the ARG variables, enabling the flexible UID/GID configuration.
200-203: LGTM! ARG re-declaration necessary for multi-stage builds.ARGs must be re-declared in each stage where they're used. This is correct Docker behavior.
227-234: LGTM! Proper handling of first-run artifacts.The comment and implementation correctly prevent baking first-run artifacts into the image, avoiding ownership/permission issues when using arbitrary UIDs with Docker volume copy-up.
170-175: LGTM! Version file handling with readonly ownership.The loop correctly ensures both
.VERSIONand.VERSION_PREVfiles exist with appropriate readonly ownership, maintaining consistency with the new ownership model.
277-277: py3-psutil correctly addresses the missing test dependency.The test file
test/backend/test_sql_security.pycontainstest_memory_usage_parameter_generation, which skips when psutil is unavailable. Addingpy3-psutilto the Alpine-based devcontainer image provides the required psutil library, allowing this test to run instead of being skipped.
162-163: No action required—readonly venv ownership already verified to work correctly.The venv is copied with readonly ownership in the runner stage, but the devcontainer stage explicitly grants write permissions via
chmod o+rwxon all venv directories. The learnings confirm pytest runs without issues in this configuration, demonstrating that Python bytecode compilation functions properly. No verification or changes needed.
| ${NETALERTX_DB_FILE} | ||
| " | ||
|
|
||
| TARGET_USER="${NETALERTX_USER:-netalertx}" |
There was a problem hiding this comment.
Validate TARGET_USER is non-empty.
If NETALERTX_USER is explicitly set to an empty string, TARGET_USER will be empty, causing chown to fail with confusing error messages.
🔎 Proposed fix
TARGET_USER="${NETALERTX_USER:-netalertx}"
+if [ -z "${TARGET_USER}" ]; then
+ echo "ERROR: TARGET_USER cannot be empty"
+ exit 1
+fi🤖 Prompt for AI Agents
In test/docker_tests/debug_storage_permission.sh around line 26, TARGET_USER is
set directly from NETALERTX_USER which can be an empty string; validate that
TARGET_USER is non-empty and handle the empty-case: if NETALERTX_USER is set but
empty, fallback to the default user (netalertx) or exit with a clear error
before running chown. Update the assignment to check for an empty value and
either assign the literal default or print a descriptive error and exit so chown
is never invoked with an empty user.
| IMMEDIATE ACTION REQUIRED: Switch to the dedicated 'netalertx' user: | ||
| * Remove any 'user:' directive specifying UID 0 from docker-compose.yml or | ||
| * switch to the default USER in the image (20211:20211) |
There was a problem hiding this comment.
Use dynamic TARGET_USER in warning message.
The warning message hardcodes 'netalertx' user on lines 40-42, but the script uses the dynamic TARGET_USER variable elsewhere. This creates an inconsistency when NETALERTX_USER is set to a different value.
🔎 Proposed fix
- IMMEDIATE ACTION REQUIRED: Switch to the dedicated 'netalertx' user:
+ IMMEDIATE ACTION REQUIRED: Switch to the dedicated '${TARGET_USER}' user:
* Remove any 'user:' directive specifying UID 0 from docker-compose.yml or
* switch to the default USER in the image (20211:20211)
IMPORTANT: This corrective mode automatically adjusts ownership of
- /data/db and /data/config directories to the netalertx user, ensuring
+ /data/db and /data/config directories to the ${TARGET_USER} user, ensuring
proper operation in subsequent runs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IMMEDIATE ACTION REQUIRED: Switch to the dedicated 'netalertx' user: | |
| * Remove any 'user:' directive specifying UID 0 from docker-compose.yml or | |
| * switch to the default USER in the image (20211:20211) | |
| IMMEDIATE ACTION REQUIRED: Switch to the dedicated '${TARGET_USER}' user: | |
| * Remove any 'user:' directive specifying UID 0 from docker-compose.yml or | |
| * switch to the default USER in the image (20211:20211) | |
| IMPORTANT: This corrective mode automatically adjusts ownership of | |
| /data/db and /data/config directories to the ${TARGET_USER} user, ensuring | |
| proper operation in subsequent runs. |
🤖 Prompt for AI Agents
In test/docker_tests/debug_storage_permission.sh around lines 40 to 42, the
warning message currently hardcodes the username 'netalertx' while the script
uses the dynamic TARGET_USER/NETALERTX_USER elsewhere; update the message to
interpolate the actual TARGET_USER variable instead of the literal string so the
warning reflects the configured user (use the shell variable expansion used
elsewhere in the script, preserve quoting/escaping as needed), and ensure any
related occurrences in those lines are changed to use TARGET_USER.
| def test_restrictive_permissions_handling(tmp_path: pathlib.Path) -> None: | ||
| """Test handling of restrictive permissions on bind mounts. | ||
|
|
||
| Simulates a user mounting a directory with restrictive permissions (e.g., 755 root:root). | ||
| The container should either fail gracefully or handle it if running as root (which triggers fix). | ||
| If running as non-root (default), it should fail to write if it doesn't have access. | ||
| """ | ||
| paths = _setup_mount_tree(tmp_path, "restrictive_perms") | ||
|
|
||
| # Helper to chown without userns host (workaround for potential devcontainer hang) | ||
| def _chown_root_safe(host_path: pathlib.Path) -> None: | ||
| cmd = [ | ||
| "docker", "run", "--rm", | ||
| # "--userns", "host", # Removed to avoid hang | ||
| "--user", "0:0", | ||
| "--entrypoint", "/bin/chown", | ||
| "-v", f"{host_path}:/mnt", | ||
| IMAGE, | ||
| "-R", "0:0", "/mnt", | ||
| ] | ||
| subprocess.run(cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
|
|
||
| # Set up a restrictive directory (root owned, 755) | ||
| target_dir = paths["app_db"] | ||
| _chown_root_safe(target_dir) | ||
| target_dir.chmod(0o755) | ||
|
|
||
| # Mount ALL volumes to avoid 'find' errors in 0-storage-permission.sh | ||
| keys = {"data", "app_db", "app_config", "app_log", "app_api", "services_run", "nginx_conf"} | ||
| volumes = _build_volume_args_for_keys(paths, keys) | ||
|
|
||
| # Case 1: Running as non-root (default) - Should fail to write | ||
| # We disable host network/userns to avoid potential hangs in devcontainer environment | ||
| result = _run_container( | ||
| "restrictive-perms-user", | ||
| volumes, | ||
| user="20211:20211", | ||
| sleep_seconds=5, | ||
| network_mode=None, | ||
| userns_mode=None | ||
| ) | ||
| assert result.returncode != 0 or "Permission denied" in result.output or "Unable to write" in result.output | ||
|
|
||
| # Case 2: Running as root - Should trigger the fix script | ||
| result_root = _run_container( | ||
| "restrictive-perms-root", | ||
| volumes, | ||
| user="0:0", | ||
| sleep_seconds=5, | ||
| network_mode=None, | ||
| userns_mode=None | ||
| ) | ||
|
|
||
| _assert_contains(result_root, "NetAlertX is running as ROOT", result_root.args) | ||
| _assert_contains(result_root, "Permissions fixed for read-write paths", result_root.args) | ||
|
|
||
| # Verify the fix actually happened | ||
| data_host_path = paths["data"] | ||
|
|
||
| check_cmd = [ | ||
| "docker", "run", "--rm", | ||
| "--entrypoint", "/bin/sh", | ||
| "--user", "20211:20211", | ||
| IMAGE, | ||
| "-c", "ls -ldn /data/db && touch /data/db/test_write_after_fix" | ||
| ] | ||
| # Add all volumes to check_cmd too | ||
| for host_path, target, readonly in volumes: | ||
| check_cmd.extend(["-v", f"{host_path}:{target}"]) | ||
|
|
||
| check_result = subprocess.run(check_cmd, capture_output=True, text=True) | ||
|
|
||
| if check_result.returncode != 0: | ||
| print(f"Check command failed. Cmd: {check_cmd}") | ||
| print(f"Stderr: {check_result.stderr}") | ||
| print(f"Stdout: {check_result.stdout}") | ||
|
|
||
| assert check_result.returncode == 0, f"Should be able to write after root fix script runs. Stderr: {check_result.stderr}. Stdout: {check_result.stdout}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix unused variables and add timeouts to subprocess calls.
This test has several issues:
- Line 1168:
data_host_pathis assigned but never used - remove it. - Line 1178: Loop variable
readonlyis not used - rename to_readonlyper convention. - Lines 1131, 1181: Subprocess calls missing explicit timeouts (required by coding guidelines).
Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fixes
- # Verify the fix actually happened
- data_host_path = paths["data"]
-
check_cmd = [
"docker", "run", "--rm",
"--entrypoint", "/bin/sh",
"--user", "20211:20211",
IMAGE,
"-c", "ls -ldn /data/db && touch /data/db/test_write_after_fix"
]
# Add all volumes to check_cmd too
- for host_path, target, readonly in volumes:
+ for host_path, target, _readonly in volumes:
check_cmd.extend(["-v", f"{host_path}:{target}"])
- check_result = subprocess.run(check_cmd, capture_output=True, text=True)
+ check_result = subprocess.run(check_cmd, capture_output=True, text=True, timeout=60)Also add timeout to the subprocess.run call at line 1131:
subprocess.run(cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+ subprocess.run(cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=60)🧰 Tools
🪛 Ruff (0.14.8)
1131-1131: subprocess call: check for execution of untrusted input
(S603)
1168-1168: Local variable data_host_path is assigned to but never used
Remove assignment to unused variable data_host_path
(F841)
1178-1178: Loop control variable readonly not used within loop body
Rename unused readonly to _readonly
(B007)
1181-1181: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
In test/docker_tests/test_container_environment.py lines 1111-1188: remove the
unused assignment data_host_path (line ~1168), rename loop variable readonly to
_readonly in the for-host_path, target, readonly loop (around line ~1178) to
avoid unused-variable warnings, and add explicit timeouts (e.g., timeout=60) to
the two subprocess.run calls: the one in _chown_root_safe (around line ~1131)
and the one running check_cmd (around line ~1181) so subprocesses cannot hang.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
install/production-filesystem/entrypoint.d/99-ports-available.sh (1)
61-73: Variable escaping issue has been addressed.The current code correctly escapes
\$APP_CONF_OVERRIDEand\$GRAPHQL_PORTon line 67, which is consistent with the escaping pattern used earlier in the file (lines 22-23, 54). This addresses the previous review feedback about inconsistent escaping.The explicit "always emit if listening, even if same port" behavior means users may see multiple warnings when
PORT_APP == PORT_GQLand the port is in use, but this is acceptable for diagnostic completeness.test/docker_tests/test_container_environment.py (8)
236-269: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter, which can cause indefinite hangs if the Docker command encounters issues.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
try: subprocess.run( cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, ) except subprocess.CalledProcessError as exc: raise RuntimeError(f"Failed to chown {host_path} to {uid}:{gid}") from exc
279-285: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter, which can cause indefinite hangs.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
def _docker_volume_rm(volume_name: str) -> None: subprocess.run( ["docker", "volume", "rm", "-f", volume_name], check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
288-294: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
def _docker_volume_create(volume_name: str) -> None: subprocess.run( ["docker", "volume", "create", volume_name], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
304-331: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
subprocess.run( [ "docker", "run", "--rm", "--userns", "host", "-v", f"{volume_name}:/data", "--entrypoint", "/bin/sh", IMAGE, "-c", "true", ], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
333-375: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
subprocess.run( cmd, input=content, text=True, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
377-399: Add explicit timeout to subprocess call.The
subprocess.runcall lacks a timeout parameter.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
return ( subprocess.run( [ "docker", "run", "--rm", "--userns", "host", "-v", f"{volume_name}:/data", "alpine:3.22", "sh", "-c", f"test -f '{container_path}'", ], check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, ).returncode == 0 )
1127-1137: Add explicit timeout to subprocess call in nested helper.The
subprocess.runcall in the_chown_root_safehelper lacks a timeout parameter.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
subprocess.run(cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + subprocess.run(cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=60)
1174-1194: Fix unused variables and add timeout to subprocess call.Three issues in this code segment:
- Line 1174:
data_host_pathis assigned but never used- Line 1184: Loop variable
readonlyis not used- Line 1187: Subprocess call missing explicit timeout
Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fixes
- # Verify the fix actually happened - data_host_path = paths["data"] - check_cmd = [ "docker", "run", "--rm", "--entrypoint", "/bin/sh", "--user", "20211:20211", IMAGE, "-c", "ls -ldn /data/db && touch /data/db/test_write_after_fix" ] # Add all volumes to check_cmd too - for host_path, target, readonly in volumes: + for host_path, target, _readonly in volumes: check_cmd.extend(["-v", f"{host_path}:{target}"]) - check_result = subprocess.run(check_cmd, capture_output=True, text=True) + check_result = subprocess.run(check_cmd, capture_output=True, text=True, timeout=60)
🧹 Nitpick comments (9)
test/docker_tests/test_container_environment.py (9)
48-62: Consider raising an exception instead of returning an arbitrary parent.The fallback
cur.parents[2]may return an incorrect path if repository detection fails. Consider raising aRuntimeErrorto make detection failures explicit rather than silently proceeding with a potentially wrong path.🔎 Proposed fix
def _repo_root() -> pathlib.Path: env = os.environ.get("NETALERTX_REPO_ROOT") if env: return pathlib.Path(env) cur = pathlib.Path(__file__).resolve() for parent in cur.parents: if any( [ (parent / "pyproject.toml").exists(), (parent / ".git").exists(), (parent / "back").exists() and (parent / "db").exists(), ] ): return parent - return cur.parents[2] + raise RuntimeError(f"Could not locate repository root from {cur}")
94-100: Remove unusedtmp_pathparameter.The
tmp_pathparameter is no longer used since the function now calls_docker_visible_tmp_root()directly. Consider removing it from the signature.🔎 Proposed fix
def _setup_mount_tree( - tmp_path: pathlib.Path, prefix: str, *, seed_config: bool = True, seed_db: bool = True, ) -> dict[str, pathlib.Path]:Note: This change requires updating all callers of this function throughout the test suite.
408-432: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
@pytest.mark.parametrize( "uid_gid", [ (1001, 1001), (1502, 1502), ], ) def test_nonroot_custom_uid_logs_note( - tmp_path: pathlib.Path, uid_gid: tuple[int, int], ) -> None:
465-470: Add timeout to cleanup subprocess call.While this cleanup command uses
check=Falsemaking a hang less critical, it should still include a timeout per coding guidelines.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
🔎 Proposed fix
subprocess.run( ["docker", "rm", "-f", name], check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
741-763: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
-def test_missing_app_conf_triggers_seed(tmp_path: pathlib.Path) -> None: +def test_missing_app_conf_triggers_seed() -> None:
765-797: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
-def test_missing_app_db_triggers_seed(tmp_path: pathlib.Path) -> None: +def test_missing_app_db_triggers_seed() -> None:
799-831: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
-def test_custom_port_without_writable_conf(tmp_path: pathlib.Path) -> None: +def test_custom_port_without_writable_conf() -> None:
833-856: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
-def test_excessive_capabilities_warning(tmp_path: pathlib.Path) -> None: +def test_excessive_capabilities_warning() -> None:
857-879: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but never used in this test.🔎 Proposed fix
-def test_appliance_integrity_read_write_mode(tmp_path: pathlib.Path) -> None: +def test_appliance_integrity_read_write_mode() -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
install/production-filesystem/entrypoint.d/35-writable-config.sh(1 hunks)install/production-filesystem/entrypoint.d/99-ports-available.sh(2 hunks)install/production-filesystem/entrypoint.sh(3 hunks)test/docker_tests/test_container_environment.py(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- install/production-filesystem/entrypoint.d/35-writable-config.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
test/docker_tests/test_container_environment.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Devcontainer philosophy: brutal simplicity. One user, everything writable, completely idempotent. No permission checks, no conditional logic, no sudo needed.
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
test/docker_tests/test_container_environment.py
📚 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.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.sh
📚 Learning: 2025-11-23T16:05:31.339Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1302
File: back/cron_script.sh:6-18
Timestamp: 2025-11-23T16:05:31.339Z
Learning: The `/back/cron_script.sh` file is legacy code maintained only for compatibility with other components. The active/production version is in `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` directory are typically outside the scope of PRs unless specifically addressing compatibility.
Applied to files:
install/production-filesystem/entrypoint.sh
📚 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.sh
🧬 Code graph analysis (1)
test/docker_tests/test_container_environment.py (1)
test/docker_tests/test_ports_available.py (3)
_setup_mount_tree(60-98)_run_container(106-167)_assert_contains(170-178)
🪛 Ruff (0.14.8)
test/docker_tests/test_container_environment.py
73-73: Probable insecure usage of temporary file or directory: "/tmp/pytest-docker-mounts"
(S108)
87-87: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
89-90: try-except-pass detected, consider logging the exception
(S110)
89-89: Do not catch blind exception: Exception
(BLE001)
95-95: Unused function argument: tmp_path
(ARG001)
240-240: Avoid specifying long messages outside the exception class
(TRY003)
261-261: subprocess call: check for execution of untrusted input
(S603)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
280-280: subprocess call: check for execution of untrusted input
(S603)
281-281: Starting a process with a partial executable path
(S607)
289-289: subprocess call: check for execution of untrusted input
(S603)
290-290: Starting a process with a partial executable path
(S607)
312-312: subprocess call: check for execution of untrusted input
(S603)
313-326: Starting a process with a partial executable path
(S607)
367-367: subprocess call: check for execution of untrusted input
(S603)
379-379: subprocess call: check for execution of untrusted input
(S603)
380-392: Starting a process with a partial executable path
(S607)
409-409: Unused function argument: tmp_path
(ARG001)
459-459: Do not catch blind exception: Exception
(BLE001)
491-491: Probable insecure usage of temporary file or directory: "/tmp:mode=1700,uid="
(S108)
741-741: Unused function argument: tmp_path
(ARG001)
765-765: Unused function argument: tmp_path
(ARG001)
799-799: Unused function argument: tmp_path
(ARG001)
833-833: Unused function argument: tmp_path
(ARG001)
857-857: Unused function argument: tmp_path
(ARG001)
1137-1137: subprocess call: check for execution of untrusted input
(S603)
1174-1174: Local variable data_host_path is assigned to but never used
Remove assignment to unused variable data_host_path
(F841)
1184-1184: Loop control variable readonly not used within loop body
Rename unused readonly to _readonly
(B007)
1187-1187: subprocess call: check for execution of untrusted input
(S603)
⏰ 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-tests
- GitHub Check: docker_dev
🔇 Additional comments (10)
install/production-filesystem/entrypoint.sh (2)
53-62: Clean refactor of banner display.Storing the ASCII art in a variable and using a single
printfwith format specifiers is cleaner than multiple print statements. The%bspecifier correctly interprets escape sequences for colors while%spreserves the banner content literally.
124-124: Appropriate use of stderr for diagnostic output.Redirecting this detection message to stderr separates diagnostic information from normal stdout output, which aligns with the test expectations mentioned in the PR summary regarding stdout/stderr separation.
install/production-filesystem/entrypoint.d/99-ports-available.sh (2)
9-14: The JSON parsing is fragile but functional for expected input.The regex assumes a specific JSON structure. For the expected format
{"GRAPHQL_PORT":"20212"}, this works:
grep -o 'GRAPHQL_PORT"*[:=]\"*[0-9]\+'matchesGRAPHQL_PORT":"20212tr -cd '0-9'extracts20212The
|| truecorrectly prevents grep's non-zero exit when no match is found, and the fallback chain${PORT_GQL:-${GRAPHQL_PORT:-20212}}handles missing values gracefully. The comment acknowledging "crude parse" is appropriate given this is a startup diagnostic, not critical path logic.
16-28: Port conflict detection logic is correct.The arithmetic comparison
-eqcorrectly checks for identical port numbers, and the escaped variables (\$PORT,\$GRAPHQL_PORT) in the here-doc properly display literal variable names as configuration hints to users.test/docker_tests/test_container_environment.py (6)
65-81: LGTM!The function correctly handles Docker daemon visibility requirements. The best-effort chmod approach is appropriate since the directory only needs to be writable by the current user.
83-92: LGTM!The defensive exception handling is appropriate for this utility function. The fallback behavior (returning the original path) ensures robustness.
169-210: LGTM!The integration with
_docker_visible_path()and best-effort permission handling correctly supports the new Docker-based test infrastructure.
452-484: LGTM!The UID/GID parsing and dynamic port allocation correctly support arbitrary user IDs while avoiding port conflicts in host network mode.
708-730: LGTM!The conversion to named volumes and proper cleanup handling improves test isolation and reliability.
972-1010: LGTM!The modifications correctly test the non-regular file scenario and improve test clarity with explicit directory creation and ownership handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/docker_tests/test_container_environment.py (1)
1188-1213: Fix unused variables flagged in previous review.Two issues from the previous review remain unresolved:
- Line 1188:
data_host_pathis assigned but never used - remove this assignment.- Line 1198: Loop variable
readonlyis not used - rename to_readonlyper Python convention.🔎 Proposed fixes
_assert_contains(result_root, "NetAlertX is running as ROOT", result_root.args) _assert_contains(result_root, "Permissions fixed for read-write paths", result_root.args) - # Verify the fix actually happened - data_host_path = paths["data"] - check_cmd = [ "docker", "run", "--rm", "--entrypoint", "/bin/sh", "--user", "20211:20211", IMAGE, "-c", "ls -ldn /data/db && touch /data/db/test_write_after_fix" ] # Add all volumes to check_cmd too - for host_path, target, readonly in volumes: + for host_path, target, _readonly in volumes: check_cmd.extend(["-v", f"{host_path}:{target}"]).devcontainer/Dockerfile (1)
12-12: Typo: "uses who wish" should be "users who wish".This issue was previously flagged. Since this file is auto-generated, apply the fix to the source file (the root
Dockerfilewhere this comment originates).
🧹 Nitpick comments (2)
test/docker_tests/test_container_environment.py (2)
87-92: Consider logging the exception in_docker_visible_path().The bare
except Exception: passsilently swallows errors. While this is best-effort logic, logging the exception would aid debugging per coding guidelines: "Always leave logging enabled."🔎 Suggested improvement
try: if str(path).startswith("/tmp/"): return _docker_visible_tmp_root() / path.name - except Exception: - pass + except Exception as e: + # Best-effort path mapping; fall back to original path + print(f"Warning: Could not map path {path} to docker-visible space: {e}") return path
459-469: User parsing logic is robust with safe fallback.The user string parsing safely handles unexpected formats by falling back to defaults (20211:20211). The bare exception catch is acceptable for test code, though a more specific exception would be slightly cleaner.
🔎 Optional: More specific exception handling
if user: try: u_str, g_str = user.split(":", 1) tmp_uid = int(u_str) tmp_gid = int(g_str) - except Exception: + except (ValueError, AttributeError): # Keep defaults if user format is unexpected. tmp_uid = 20211 tmp_gid = 20211
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/Dockerfile(9 hunks)test/docker_tests/test_container_environment.py(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.devcontainer/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never
chmodorchownduring operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix.devcontainer/scripts/setup.shor.devcontainer/resources/devcontainer-Dockerfileinstead.
Files:
.devcontainer/Dockerfile
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
test/docker_tests/test_container_environment.py
🧠 Learnings (16)
📚 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:
.devcontainer/Dockerfile
📚 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
📚 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 repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Devcontainer philosophy: brutal simplicity. One user, everything writable, completely idempotent. No permission checks, no conditional logic, no sudo needed.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to .devcontainer/**/* : Never `chmod` or `chown` during operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead.
Applied to files:
.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:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:08:44.152Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:82-92
Timestamp: 2025-09-20T14:08:44.152Z
Learning: In devcontainer builds, source repository files are not available during the Docker build phase. The source code gets mounted into the container after it's built and started, so COPY commands referencing source files will fail. Configuration files need to be handled at runtime (e.g., in setup scripts) rather than during the build stage.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-10T22:16:02.770Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1214
File: install/ubuntu24/uninstall.sh:129-141
Timestamp: 2025-10-10T22:16:02.770Z
Learning: NetAlertX uninstall procedures should only remove files from the repository (specifically /app files) and should not touch system packages like PHP, nginx, avahi, or other shared system components to avoid damaging user systems.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Applied to files:
test/docker_tests/test_container_environment.py
🪛 Ruff (0.14.8)
test/docker_tests/test_container_environment.py
74-74: Probable insecure usage of temporary file or directory: "/tmp/pytest-docker-mounts"
(S108)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
90-91: try-except-pass detected, consider logging the exception
(S110)
90-90: Do not catch blind exception: Exception
(BLE001)
96-96: Unused function argument: tmp_path
(ARG001)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
262-262: subprocess call: check for execution of untrusted input
(S603)
270-270: Avoid specifying long messages outside the exception class
(TRY003)
282-282: subprocess call: check for execution of untrusted input
(S603)
283-283: Starting a process with a partial executable path
(S607)
292-292: subprocess call: check for execution of untrusted input
(S603)
293-293: Starting a process with a partial executable path
(S607)
316-316: subprocess call: check for execution of untrusted input
(S603)
317-330: Starting a process with a partial executable path
(S607)
372-372: subprocess call: check for execution of untrusted input
(S603)
385-385: subprocess call: check for execution of untrusted input
(S603)
386-398: Starting a process with a partial executable path
(S607)
416-416: Unused function argument: tmp_path
(ARG001)
466-466: Do not catch blind exception: Exception
(BLE001)
472-472: subprocess call: check for execution of untrusted input
(S603)
473-473: Starting a process with a partial executable path
(S607)
499-499: Probable insecure usage of temporary file or directory: "/tmp:mode=1700,uid="
(S108)
749-749: Unused function argument: tmp_path
(ARG001)
773-773: Unused function argument: tmp_path
(ARG001)
807-807: Unused function argument: tmp_path
(ARG001)
841-841: Unused function argument: tmp_path
(ARG001)
865-865: Unused function argument: tmp_path
(ARG001)
1145-1145: subprocess call: check for execution of untrusted input
(S603)
1188-1188: Local variable data_host_path is assigned to but never used
Remove assignment to unused variable data_host_path
(F841)
1198-1198: Loop control variable readonly not used within loop body
Rename unused readonly to _readonly
(B007)
1201-1201: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (13)
test/docker_tests/test_container_environment.py (5)
20-20: Excellent addition of centralized timeout configuration.The
SUBPROCESS_TIMEOUT_SECONDSconstant provides a single point of configuration for subprocess timeouts across all tests, with a sensible 60-second default that aligns with coding guidelines.
95-167: Well-designed mount tree setup for arbitrary UID/GID testing.The function correctly:
- Uses docker-visible paths to avoid bind mount issues
- Sets world-writable permissions (0o777/0o666) to support arbitrary UID/GID runs
- Seeds config/db from repository when requested
- Ensures all mount points and children have appropriate permissions
The unused
tmp_pathparameter is acceptable as it's a pytest fixture.
237-405: Excellent resolution of previous timeout concerns.All subprocess calls in these volume helper functions now include explicit timeouts using
SUBPROCESS_TIMEOUT_SECONDS. This addresses the concerns raised in previous reviews about subprocess calls potentially hanging indefinitely.
408-438: Excellent test coverage for arbitrary UID/GID support.This test validates the PR's primary objective of supporting non-Synology user IDs. The parametrization tests multiple UIDs, and the finally block ensures proper cleanup.
749-887: Tests successfully migrated to volume-based approach.These tests have been properly updated to use named volumes instead of bind mounts, which:
- Aligns with docker-compose UX
- Avoids docker-visible path issues
- Ensures proper cleanup via try/finally blocks
The volume-based approach is more realistic for testing container behavior.
.devcontainer/Dockerfile (8)
57-62: Excellent: Dynamic UID/GID configuration.The introduction of build-time ARGs for user and group IDs enables flexible deployment across different environments and achieves the PR's goal of supporting non-Synology users. The separation between service account and read-only lock owner is a thoughtful security design.
141-142: LGTM: User creation now uses configurable IDs.The addgroup/adduser commands correctly reference the ARG variables instead of hardcoded values.
160-175: LGTM: Version file handling is robust.The addition of
.VERSION_PREVtracking, dynamic ownership using ARG variables, and defensive initialization loop ensure version files are properly configured regardless of build context. The use of read-only ownership for these files is appropriate.
199-203: LGTM: Correct ARG re-declaration for multi-stage build.Docker requires build arguments to be re-declared in each stage where they're used. This follows the correct pattern.
212-213: LGTM: Read-only user creation with configurable IDs.Correctly uses ARG variables for the read-only lock owner account.
224-224: LGTM: This chmod is in the production stage, not devcontainer.The previous review comment about line 224 violating devcontainer guidelines was misplaced. This line is in the
hardenedproduction stage (not thenetalertx-devcontainerstage that begins at line 261). Setting0777permissions onREAD_WRITE_FOLDERSin production is appropriate to ensure the service can write to these directories with arbitrary runtime UIDs.The devcontainer guideline about avoiding chmod/chown applies only to the devcontainer stage (lines 261-305).
227-234: Excellent: Prevents volume permission issues with arbitrary UIDs.Removing first-run artifacts from the image is critical for the PR's goal of supporting arbitrary user IDs. Without this cleanup, Docker's volume copy-up would persist restrictive ownership from the image into fresh volumes, breaking runs with non-default UIDs. The detailed comment clearly documents the rationale.
277-277: py3-psutil is required for test dependencies.The
py3-psutilpackage is necessary for the test suite. Specifically,test/backend/test_sql_security.pyuses psutil to monitor memory usage during parameter generation testing viapsutil.Process().memory_info(). Using the Alpine package (py3-psutil) is the correct approach to avoid compilation issues on Alpine Linux.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.devcontainer/Dockerfile (1)
12-12: Typo remains unfixed.The typo "uses who wish" (should be "users who wish") is still present. Since this is an auto-generated file, apply the fix to the source (root Dockerfile or
.devcontainer/resources/devcontainer-Dockerfile).
🧹 Nitpick comments (3)
test/docker_tests/test_container_environment.py (3)
95-101: Remove unusedtmp_pathparameter.The
tmp_pathparameter is no longer used since the function now creates its mount tree under_docker_visible_tmp_root()(line 105). Consider removing it from the signature.🔎 Proposed fix
def _setup_mount_tree( - tmp_path: pathlib.Path, prefix: str, *, seed_config: bool = True, seed_db: bool = True, ) -> dict[str, pathlib.Path]:Update all call sites to remove the
tmp_pathargument.
408-438: Clean test for arbitrary UID support.This test properly validates the PR's core objective of supporting arbitrary non-root UIDs. However, the
tmp_pathparameter is unused and can be removed.🔎 Proposed fix
@pytest.mark.parametrize( "uid_gid", [ (1001, 1001), (1502, 1502), ], ) def test_nonroot_custom_uid_logs_note( - tmp_path: pathlib.Path, uid_gid: tuple[int, int], ) -> None:
749-750: Optional: Remove unusedtmp_pathparameters from volume-based tests.Several test functions that switched to named volumes no longer use their
tmp_pathparameter:
test_missing_app_conf_triggers_seed(line 749)test_missing_app_db_triggers_seed(line 773)test_custom_port_without_writable_conf(line 807)test_excessive_capabilities_warning(line 841)test_appliance_integrity_read_write_mode(line 865)Consider removing these unused parameters unless required by pytest fixtures.
Also applies to: 773-774, 807-808, 841-842, 865-866
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/Dockerfile(9 hunks)test/docker_tests/test_container_environment.py(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.devcontainer/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never
chmodorchownduring operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix.devcontainer/scripts/setup.shor.devcontainer/resources/devcontainer-Dockerfileinstead.
Files:
.devcontainer/Dockerfile
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
test/docker_tests/test_container_environment.py
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to .devcontainer/**/* : Never `chmod` or `chown` during operations in devcontainer. Everything is already writable. 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: 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: .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-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:
.devcontainer/Dockerfile
📚 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
📚 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 repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.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:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Devcontainer philosophy: brutal simplicity. One user, everything writable, completely idempotent. No permission checks, no conditional logic, no sudo needed.
Applied to files:
.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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to .devcontainer/**/* : Never `chmod` or `chown` during operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix `.devcontainer/scripts/setup.sh` or `.devcontainer/resources/devcontainer-Dockerfile` instead.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:08:44.152Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:82-92
Timestamp: 2025-09-20T14:08:44.152Z
Learning: In devcontainer builds, source repository files are not available during the Docker build phase. The source code gets mounted into the container after it's built and started, so COPY commands referencing source files will fail. Configuration files need to be handled at runtime (e.g., in setup scripts) rather than during the build stage.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-10-10T22:16:02.770Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1214
File: install/ubuntu24/uninstall.sh:129-141
Timestamp: 2025-10-10T22:16:02.770Z
Learning: NetAlertX uninstall procedures should only remove files from the repository (specifically /app files) and should not touch system packages like PHP, nginx, avahi, or other shared system components to avoid damaging user systems.
Applied to files:
.devcontainer/Dockerfile
📚 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:
.devcontainer/Dockerfile
📚 Learning: 2025-12-08T01:59:00.903Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
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 (3)
_setup_mount_tree(60-98)_run_container(106-167)_assert_contains(170-178)
🪛 Ruff (0.14.8)
test/docker_tests/test_container_environment.py
74-74: Probable insecure usage of temporary file or directory: "/tmp/pytest-docker-mounts"
(S108)
88-88: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
90-91: try-except-pass detected, consider logging the exception
(S110)
90-90: Do not catch blind exception: Exception
(BLE001)
96-96: Unused function argument: tmp_path
(ARG001)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
262-262: subprocess call: check for execution of untrusted input
(S603)
270-270: Avoid specifying long messages outside the exception class
(TRY003)
282-282: subprocess call: check for execution of untrusted input
(S603)
283-283: Starting a process with a partial executable path
(S607)
292-292: subprocess call: check for execution of untrusted input
(S603)
293-293: Starting a process with a partial executable path
(S607)
316-316: subprocess call: check for execution of untrusted input
(S603)
317-330: Starting a process with a partial executable path
(S607)
372-372: subprocess call: check for execution of untrusted input
(S603)
385-385: subprocess call: check for execution of untrusted input
(S603)
386-398: Starting a process with a partial executable path
(S607)
416-416: Unused function argument: tmp_path
(ARG001)
466-466: Do not catch blind exception: Exception
(BLE001)
472-472: subprocess call: check for execution of untrusted input
(S603)
473-473: Starting a process with a partial executable path
(S607)
499-499: Probable insecure usage of temporary file or directory: "/tmp:mode=1700,uid="
(S108)
749-749: Unused function argument: tmp_path
(ARG001)
773-773: Unused function argument: tmp_path
(ARG001)
807-807: Unused function argument: tmp_path
(ARG001)
841-841: Unused function argument: tmp_path
(ARG001)
865-865: Unused function argument: tmp_path
(ARG001)
1145-1145: subprocess call: check for execution of untrusted input
(S603)
1198-1198: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (16)
test/docker_tests/test_container_environment.py (6)
20-20: LGTM: Centralized timeout configuration.The
SUBPROCESS_TIMEOUT_SECONDSconstant properly centralizes timeout configuration for all subprocess calls, and I've verified that all subprocess calls throughout the file now include explicit timeouts. This satisfies the coding guidelines requirement.Based on coding guidelines: "Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise."
49-93: LGTM: Docker-visible path helpers.The new helper functions (
_repo_root,_docker_visible_tmp_root,_docker_visible_path) properly address the issue where pytest's tmp_path may not be visible to the Docker daemon. The defensive exception handling with fallbacks is appropriate for test infrastructure.
281-405: LGTM: Volume management utilities.The new volume management functions (
_docker_volume_rm,_docker_volume_create,_fresh_named_volume,_ensure_volume_copy_up,_seed_volume_text_file,_volume_has_file) provide a clean abstraction for named-volume testing and all include proper timeouts as required by coding guidelines.
441-573: LGTM: Enhanced container run helper.The updates to
_run_containerproperly support the extended test scenarios including arbitrary UIDs, pre-entrypoint scripts, and user namespace configuration. The user string parsing with defensive exception handling (lines 462-469) is appropriate - it safely falls back to defaults if the format is unexpected.
980-1018: LGTM: Refined config validation test.The test correctly exercises the "not a regular file" warning path by explicitly creating a directory at the config path location. This ensures the test validates the specific error condition.
1125-1210: LGTM: Comprehensive restrictive permissions test.This test properly validates the handling of restrictive permissions on bind mounts, covering both non-root failure and root fix scenarios. The nested
_chown_root_safehelper without--userns host(line 1138) is an appropriate workaround for potential devcontainer environment hangs.All issues from previous reviews have been addressed:
- Explicit timeouts added (lines 1150, 1202)
- Loop variable renamed to
_readonly(line 1195).devcontainer/Dockerfile (10)
46-48: LGTM!Using
--prefer-binaryreduces build time by favoring pre-built wheels over source compilation.
57-61: LGTM!The ARG-based UID/GID approach cleanly supports custom user IDs while maintaining separation between the service account and read-only lock owner.
141-142: LGTM!Dynamic user/group creation using ARGs correctly implements the configurable UID/GID feature.
160-160: LGTM!Adding
.VERSION_PREVenables version tracking across container updates.
162-163: LGTM!Assigning venv ownership to the read-only lock owner prevents runtime modification—a solid hardening practice.
170-175: LGTM!Version file initialization with read-only ownership ensures consistent versioning metadata.
199-203: LGTM!Re-declaring ARGs in the hardened stage is required—ARG scope is limited to the stage where declared.
212-213: LGTM!Read-only user creation with ARG-based IDs maintains consistency with the configurable UID/GID design.
227-234: LGTM!Removing first-run artifacts prevents Docker's volume copy-up from baking restrictive ownership into named volumes, enabling arbitrary UID/GID usage.
277-277: LGTM!Adding
py3-psutilprovides process and system monitoring utilities for development.
| RUN apk add --no-cache \ | ||
| bash \ | ||
| shadow \ | ||
| python3 \ | ||
| python3-dev \ | ||
| gcc \ | ||
| musl-dev \ | ||
| libffi-dev \ | ||
| openssl-dev \ | ||
| git \ | ||
| rust \ | ||
| cargo \ | ||
| && python -m venv /opt/venv |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for requirements.txt files and check their content
echo "=== Finding requirements.txt files ==="
fd -e requirements.txt
echo ""
echo "=== Checking for pydantic and Rust-dependent packages ==="
if [ -f "requirements.txt" ]; then
echo "=== Contents of requirements.txt ==="
cat requirements.txt
else
echo "No requirements.txt found in root"
# Search in subdirectories
find . -name "requirements.txt" -type f | head -10
fiRepository: jokob-sk/NetAlertX
Length of output: 582
Remove unnecessary Rust toolchain from builder stage.
The requirements.txt contains no packages requiring Rust compilation. Cryptography<40 uses C/OpenSSL bindings, not Rust. Removing rust and cargo from lines 42-43 will reduce build time and image size.
🤖 Prompt for AI Agents
.devcontainer/Dockerfile lines 32 to 44: the builder stage currently installs
the Rust toolchain (rust and cargo) although requirements.txt has no crates
needing Rust; remove the rust and cargo entries from the apk add list to avoid
unnecessary build time and image size, leaving the other packages intact and
then rebuild the image to verify the container still installs Python venv and
required C build deps for cryptography.
2 participants
Does not fix Synology.
userto be set touser: "1000"or other arbitrary userSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.