Hardening fixes - #1235
Conversation
WalkthroughFixes PYTHONPATH and Python binary resolution; expands devcontainer tooling, permissions, and docker-socket group handling; writes build timestamps; parameterizes compose networking/volumes; adds many startup/storage/network/permission validation scripts; adjusts service startup/entrypoints and adds container-focused pytest tests. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,250,252)
participant Host as Developer Host
participant DevCt as DevContainer
participant Setup as setup.sh
participant Docker as Docker Socket
Host->>DevCt: start devcontainer (mount `/var/run/docker.sock`)
DevCt->>Setup: run postCreateCommand (pip installs)
DevCt->>Setup: run postStartCommand -> setup.sh
Setup->>Docker: stat `/var/run/docker.sock` (get gid)
Setup->>Setup: ensure_docker_socket_access() → resolve/create group, add `netalertx` user
Setup->>DevCt: set permissions, write `NETALERTX_FRONT/buildtimestamp.txt`
DevCt-->>Host: switch to `netalertx` user, `sleep infinity`
end
sequenceDiagram
participant Entrypoint as entrypoint.sh
participant Checker as check-*.sh
participant Services as backend/nginx/php-fpm
participant Vendor as update_vendors.sh
Entrypoint->>Checker: execute each check script
Checker-->>Entrypoint: return per-check status
Entrypoint->>Entrypoint: record non-zero into FAILED_STATUS (string)
alt FAILED_STATUS non-empty
Entrypoint->>Entrypoint: print failures, exit with code
else NETALERTX_CHECK_ONLY == 1
Entrypoint->>Entrypoint: exit after checks with status
else
Entrypoint->>Services: start services (background)
Entrypoint->>Vendor: run vendor update via `bash` (background)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-26T16:45:41.228ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_safe_builder_unit.py (1)
15-153: Do not reimplement SafeConditionBuilder in tests; import the production class to avoid security drift.Redefining SafeConditionBuilder here risks divergence from server/db/sql_safe_builder.py and weakens guarantees of your SQLi tests. Import the real class and keep the lightweight logger shim.
Apply:
@@ -# Standalone version of SafeConditionBuilder for testing -class SafeConditionBuilder: - """ - Test version of SafeConditionBuilder with mock logger. - """ - ... - def get_safe_condition_legacy(self, condition_setting): - """Convert legacy condition settings to safe parameterized queries.""" - if not condition_setting or not condition_setting.strip(): - return "", {} - - try: - return self.build_safe_condition(condition_setting) - except ValueError: - # Log the error and return empty condition for safety - return "", {} +# Use the production implementation to ensure tests cover real behavior +from server.db.sql_safe_builder import SafeConditionBuilderKeep
sys.modules['logger'] = Mock()sofrom logger import mylogin the prod module resolves to a harmless mock. Based on learnings.
🧹 Nitpick comments (12)
server/api_server/nettools_endpoint.py (1)
5-5: Remove unused import.The
speedtestmodule is imported with an alias but never used in the code. The function usessubprocessto call thespeedtest-clicommand instead.Apply this diff to remove the unused import:
-import speedtest as speedtest_cliinstall/production-filesystem/services/scripts/check-nginx-config.sh (1)
48-48: Consider trap-based cleanup for the temp file.If the script is interrupted between lines 30 and 48, the temp file may remain. While unlikely to cause issues, a trap handler would ensure cleanup in all scenarios.
Add cleanup on exit:
+trap 'rm -f "${TMP_FILE}"' EXIT + TMP_FILE="${CONF_ACTIVE_DIR}/.netalertx-write-test" if ! ( : >"${TMP_FILE}" ) 2>/dev/null; thenThen remove the explicit
rm -f "${TMP_FILE}"on line 48 since the trap handles it..devcontainer/resources/devcontainer-Dockerfile (1)
10-10: Consider using a variable for the Python version path.The hardcoded
python3.12in PYTHONPATH will break when Alpine updates to Python 3.13+. Consider using a shell variable derived frompython3 --versionor a glob pattern in the entrypoint script to maintain forward compatibility.test/test_compound_conditions.py (1)
236-236: Address unused variable warnings from static analysis.Several test functions unpack both
sqlandparamsbut only use one of them. Use underscore-prefixed variables for unused values to signal intent and satisfy linters.Apply these changes:
Line 236 (in try block - params is used in except):
try: - sql, params = builder.build_safe_condition(condition) + sql, _params = builder.build_safe_condition(condition)Line 249:
- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)Line 303:
- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)Line 314:
- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)Also applies to: 249-249, 303-303, 314-314
install/production-filesystem/services/scripts/check-persistent_storage.sh (1)
38-40: Minor: Inconsistent indentation (tabs vs spaces).Line 39 uses a tab character while the rest of the script uses spaces. Consider standardizing on spaces for consistency.
test/test_safe_builder_unit.py (3)
271-279: Silence Ruff RUF059: prefix unusedsqlwith underscore.
sqlis not used; prefer_to signal intentional discard.- sql, params = builder.build_safe_condition("AND devName = 'Device1'") + _, params = builder.build_safe_condition("AND devName = 'Device1'")
311-314: Silence Ruff RUF059: prefix unusedsqlwith underscore.Same nit here.
- sql, params = builder.build_safe_condition(f"AND devName = '{unicode_str}'") + _, params = builder.build_safe_condition(f"AND devName = '{unicode_str}'")
326-333: Avoid broadexcept Exception; let pytest surface unexpected errors.Catching all exceptions hides real failures. Either remove the try/except or catch a specific error.
- for case in edge_cases: - try: - sql, params = builder.get_safe_condition_legacy(case) - # Should either return valid result or empty safe result - assert isinstance(sql, str) - assert isinstance(params, dict) - except Exception: - pytest.fail(f"Unexpected exception for edge case: {case}") + for case in edge_cases: + sql, params = builder.get_safe_condition_legacy(case) + # Should either return valid result or empty safe result + assert isinstance(sql, str) + assert isinstance(params, dict)test/docker_tests/test_container_environment.py (4)
286-307: Use pytest tmp_path instead of a hardcoded /tmp directory.Hardcoding /tmp is brittle and flagged by static analysis. Let pytest manage a temporary directory via tmp_path.
-def test_second_run_starts_clean() -> None: +def test_second_run_starts_clean(tmp_path: pathlib.Path) -> None: @@ - base = pathlib.Path("/tmp/NETALERTX_SECOND_RUN_CLEAN_TEST_MOUNT_INTENTIONAL") - paths = _setup_fixed_mount_tree(base) + base = tmp_path / "NETALERTX_SECOND_RUN_CLEAN_TEST_MOUNT_INTENTIONAL" + paths = _setup_fixed_mount_tree(base) @@ - finally: - shutil.rmtree(base, ignore_errors=True) + finally: + shutil.rmtree(base, ignore_errors=True)
106-120: Avoid devcontainer-specific hardcoded paths; resolve repo root dynamically.Tests currently assume /workspaces/NetAlertX; compute the repository root relative to this test file or via an env var override.
+REPO_ROOT = pathlib.Path(os.environ.get("NETALERTX_REPO_ROOT", "")).resolve() \ + if os.environ.get("NETALERTX_REPO_ROOT") \ + else pathlib.Path(__file__).resolve().parents[2] @@ - shutil.copyfile( - "/workspaces/NetAlertX/back/app.conf", - config_file, - ) + shutil.copyfile(str(REPO_ROOT / "back/app.conf"), config_file) @@ - shutil.copyfile( - "/workspaces/NetAlertX/db/app.db", - db_file, - ) + shutil.copyfile(str(REPO_ROOT / "db/app.db"), db_file) @@ - shutil.copyfile("/workspaces/NetAlertX/back/app.conf", paths["app_config"] / "app.conf") - shutil.copyfile("/workspaces/NetAlertX/db/app.db", paths["app_db"] / "app.db") + shutil.copyfile(str(REPO_ROOT / "back/app.conf"), paths["app_config"] / "app.conf") + shutil.copyfile(str(REPO_ROOT / "db/app.db"), paths["app_db"] / "app.db")Also applies to: 296-301
32-50: Remove or use unused Docker volume helpers._helpers
_create_docker_volumeand_remove_docker_volumeare currently unused; consider removing to reduce noise or add tests that use named volumes as per TODO.
25-25: Optional: guard on docker availability to skip gracefully.Consider skipping when
dockerisn’t present to reduce CI flakiness on non-docker runners.-pytestmark = [pytest.mark.docker, pytest.mark.feature_complete] +pytestmark = [pytest.mark.docker, pytest.mark.feature_complete] + +if shutil.which("docker") is None: + pytest.skip("docker not available on PATH; skipping docker integration tests", allow_module_level=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.devcontainer/Dockerfile(3 hunks).devcontainer/devcontainer.json(2 hunks).devcontainer/resources/devcontainer-Dockerfile(1 hunks).devcontainer/scripts/setup.sh(2 hunks).gitignore(1 hunks)README.md(2 hunks)docker-compose.yml(3 hunks)install/production-filesystem/entrypoint.sh(1 hunks)install/production-filesystem/services/scripts/check-app-permissions.sh(1 hunks)install/production-filesystem/services/scripts/check-capabilities.sh(1 hunks)install/production-filesystem/services/scripts/check-first-run-config.sh(1 hunks)install/production-filesystem/services/scripts/check-first-run-db.sh(1 hunks)install/production-filesystem/services/scripts/check-mandatory-folders.sh(1 hunks)install/production-filesystem/services/scripts/check-network-mode.sh(1 hunks)install/production-filesystem/services/scripts/check-nginx-config.sh(1 hunks)install/production-filesystem/services/scripts/check-permissions.sh(0 hunks)install/production-filesystem/services/scripts/check-persistent_storage.sh(1 hunks)install/production-filesystem/services/scripts/check-ramdisk.sh(1 hunks)install/production-filesystem/services/scripts/check-root.sh(1 hunks)install/production-filesystem/services/scripts/check-storage-extra.sh(1 hunks)install/production-filesystem/services/scripts/check-storage.sh(0 hunks)install/production-filesystem/services/scripts/check-user-netalertx.sh(1 hunks)install/production-filesystem/services/start-backend.sh(1 hunks)install/production-filesystem/services/start-nginx.sh(1 hunks)pyproject.toml(1 hunks)scripts/db_cleanup/regenerate-database.sh(0 hunks)server/api_server/nettools_endpoint.py(2 hunks)test/docker_tests/dockerfiles/no-error-compose.yml(1 hunks)test/docker_tests/test_container_environment.py(1 hunks)test/test_compound_conditions.py(1 hunks)test/test_safe_builder_unit.py(2 hunks)
💤 Files with no reviewable changes (3)
- scripts/db_cleanup/regenerate-database.sh
- install/production-filesystem/services/scripts/check-storage.sh
- install/production-filesystem/services/scripts/check-permissions.sh
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/test_compound_conditions.pyserver/api_server/nettools_endpoint.pytest/test_safe_builder_unit.pytest/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/test_compound_conditions.pytest/test_safe_builder_unit.pytest/docker_tests/test_container_environment.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When introducing new server functionality, add endpoints only within 'server/api_server/*' and keep authorization checks consistent.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (6)
📚 Learning: 2025-10-19T01:40:57.071Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.071Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Applied to files:
test/docker_tests/dockerfiles/no-error-compose.yml.devcontainer/Dockerfile
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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-Dockerfile.devcontainer/Dockerfile
📚 Learning: 2025-09-20T02:56:24.501Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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/resources/devcontainer-Dockerfile.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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
PR: jokob-sk/NetAlertX#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/Dockerfile
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile.devcontainer/Dockerfile
🧬 Code graph analysis (2)
test/test_compound_conditions.py (2)
test/test_safe_builder_unit.py (3)
SafeConditionBuilder(15-152)builder(156-158)build_safe_condition(81-96)server/db/sql_safe_builder.py (2)
SafeConditionBuilder(23-623)build_safe_condition(122-151)
test/test_safe_builder_unit.py (2)
server/db/sql_safe_builder.py (7)
SafeConditionBuilder(23-623)_sanitize_string(64-84)_validate_column_name(86-96)_validate_operator(98-108)_build_simple_condition(414-438)build_safe_condition(122-151)get_safe_condition_legacy(604-623)test/test_compound_conditions.py (1)
builder(21-23)
🪛 Ruff (0.14.1)
test/test_compound_conditions.py
236-236: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
249-249: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
303-303: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
314-314: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
server/api_server/nettools_endpoint.py
84-84: Starting a process with a partial executable path
(S607)
test/test_safe_builder_unit.py
274-274: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
312-312: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
332-332: Do not catch blind exception: Exception
(BLE001)
test/docker_tests/test_container_environment.py
34-34: subprocess call: check for execution of untrusted input
(S603)
35-35: Starting a process with a partial executable path
(S607)
44-44: subprocess call: check for execution of untrusted input
(S603)
45-45: Starting a process with a partial executable path
(S607)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
76-76: subprocess call: check for execution of untrusted input
(S603)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
214-214: f-string without any placeholders
Remove extraneous f prefix
(F541)
221-221: subprocess call: check for execution of untrusted input
(S603)
293-293: Probable insecure usage of temporary file or directory: "/tmp/NETALERTX_SECOND_RUN_CLEAN_TEST_MOUNT_INTENTIONAL"
(S108)
🪛 Shellcheck (0.11.0)
install/production-filesystem/services/start-backend.sh
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
⏰ 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 (34)
.gitignore (1)
1-1: Add coverage ignore entry to align with pytest workflow.The addition of
.coverageis appropriate given the PR's test modernization to pytest, which generates coverage reports via coverage.py. This prevents build artifacts from being committed to version control.install/production-filesystem/services/scripts/check-root.sh (1)
32-32: Sleep duration seems reasonable for user readability.The 5-second pause gives users adequate time to read the warning before the container stops.
install/production-filesystem/services/scripts/check-ramdisk.sh (1)
48-48: Directory check is appropriate for conf.active.The change from file (
-f) to directory (-d) check correctly reflects thatconf.activeis a directory containing configuration files rather than a single file.install/production-filesystem/services/scripts/check-user-netalertx.sh (1)
1-43: User validation logic is sound but exits non-fatally.The implementation correctly validates the running user against expected UID/GID with a reasonable fallback to hardcoded defaults. However, exiting with code 0 after the validation failure warning (line 43) means the container continues starting even when running as the wrong user.
Given the warning message states this "loses crucial safeguards," verify this is the intended behavior. If enforcement is desired, change line 43 to
exit 1. Otherwise, ensure the entrypoint or orchestration layer can detect and respond to this condition appropriately.install/production-filesystem/services/scripts/check-nginx-config.sh (1)
1-50: Nginx config validation is well-implemented.The script correctly validates both the existence and writability of the config directory with clear error messages and remediation steps. The temp file write test (line 30) is a reliable approach.
Note: This script exits with code 1 on failures (lines 26, 46), while most other check scripts in this PR exit with code 0. Verify this is intentional - presumably nginx config is considered critical while other checks are advisory. Consider documenting this distinction in the script header.
README.md (2)
49-56: Deployment instructions are clear and actionable.The added git clone workflow provides a straightforward path for users to deploy from source. The use of
--force-recreate --buildflags ensures a clean build.Consider adding a brief note about customization options (environment variables, ports, volumes) either inline or with a link to the docker-compose.yml documentation.
13-31: Improved table of contents structure.The restructured TOC with dedicated subsections under Features and a new "Everything else" section improves navigation and discoverability.
install/production-filesystem/services/scripts/check-first-run-db.sh (1)
5-15: LGTM!The conditional logic correctly implements idempotent first-run behavior: ALWAYS_FRESH_INSTALL forces a fresh database rebuild, and existing installations exit early to preserve data.
.devcontainer/scripts/setup.sh (1)
28-65: Solid Docker socket discovery and group handling.The ensure_docker_socket_access() function correctly detects the Docker socket, resolves its group with appropriate fallbacks, and idempotently adds the netalertx user to the group. Error handling is thorough.
pyproject.toml (1)
5-9: Good test infrastructure updates.The expanded testpaths and new markers enable selective execution of Docker integration tests and extended coverage suites without cluttering the default test run. Aligns well with the PR's testing strategy.
install/production-filesystem/services/start-nginx.sh (1)
51-55: LGTM!Adding error_log to /dev/stderr in the nginx -g directive improves container observability by routing errors to stdout for container log aggregation while maintaining the file log. Correct approach for containerized services.
install/production-filesystem/entrypoint.sh (2)
57-81: Excellent pre-startup health check framework.The guarded check loop correctly runs validation scripts only in production (NETALERTX_DEBUG != 1), captures and propagates exit codes, and provides per-script logging for troubleshooting. The check-only mode is useful for validation. Proper error handling throughout.
Verify that all check-*.sh scripts in ${SYSTEM_SERVICES_SCRIPTS} are intended to be discovered and executed by this loop, and that they all return appropriate exit codes (0 for success, non-zero for failure).
85-85: LGTM!Switching vendor update from sh to bash allows more robust scripting and running in the background avoids blocking startup. Good change for container readiness.
.devcontainer/devcontainer.json (1)
26-46: LGTM!Docker socket mount enables integration testing with Docker socket access, and the postCreateCommand installs essential test and debug tools (pytest, docker, debugpy) into the isolated venv. Both changes support the PR's testing modernization.
install/production-filesystem/services/scripts/check-app-permissions.sh (1)
38-47: LGTM for root permission fixes.The chown and find commands correctly ensure netalertx ownership and appropriate permissions (700 for dirs, 600 for files) when running as root. This is the right approach for initializing the container filesystem.
install/production-filesystem/services/scripts/check-network-mode.sh (3)
1-14: LGTM! Clean early-exit and interface detection.The script correctly:
- Uses portable
/bin/shfor Alpine compatibility- Respects the debug flag for development workflows
- Safely handles missing default routes by exiting early
17-42: Bridge detection heuristics are reasonable for a non-blocking warning.The MAC and IP pattern checks cover common Docker bridge scenarios (02:42:, 172.16-31., 192.168.65.*), and the "@if" check catches veth pairs. While these heuristics may have edge cases, they're appropriate for a warning-level check that doesn't block startup.
47-64: Clear, actionable warning with appropriate non-blocking behavior.The warning message correctly:
- Uses ANSI colors for visibility
- Explains the impact of bridge networking on ARP/discovery
- Provides concrete remediation (--network=host with required capabilities)
- Exits 0 to avoid blocking startup (warning-only behavior)
.devcontainer/resources/devcontainer-Dockerfile (2)
20-22: LGTM! Package additions support the Docker integration test suite.The addition of
py3-docker-pyanddocker-clialigns with the new Docker integration tests introduced in test/docker_tests/.
31-31: Verify that world-writable permissions on /opt/venv are intentional.The
chmod o+rwxon all/opt/venvdirectories grants write access to any user, which could allow malicious code or tooling to modify the virtual environment. While this is a devcontainer (lower risk), consider using group ownership withchown -R netalertx:netalertx /opt/venvandchmod -R g+winstead to limit write access to the netalertx user and group.install/production-filesystem/services/scripts/check-mandatory-folders.sh (2)
5-50: LGTM! Robust error handling with clear feedback.The function correctly:
- Guards each directory/file creation with existence checks
- Provides explicit error messages with the full path
- Returns non-zero on any failure
- Uses consistent messaging and structure
52-53: LGTM! Implicit exit status propagation is correct.The script correctly relies on shell behavior where the last command's exit status becomes the script's exit status, so failures in
check_mandatory_folders()will propagate as non-zero exits.test/docker_tests/dockerfiles/no-error-compose.yml (2)
1-16: LGTM! Security-hardened configuration follows best practices.The configuration correctly implements:
- Host networking for ARP/scanning requirements
- Read-only filesystem to prevent tampering
- Least-privilege capabilities (drop ALL, add only NET_ADMIN/NET_RAW/NET_BIND_SERVICE)
This aligns with CIS Docker benchmarks mentioned in the PR objectives.
17-41: LGTM! Volume strategy correctly separates persistent data from runtime configuration.The mix of named volumes (for config/db persistence) and bind mounts (for timezone) is appropriate, and the commented examples provide useful guidance for customization scenarios.
test/test_compound_conditions.py (2)
26-152: LGTM! Comprehensive test coverage for compound condition parsing.The tests correctly:
- Cover the user-reported Issue #1210 scenario
- Test single and compound conditions for backward compatibility
- Verify parameter generation and SQL structure
- Use clear, descriptive test names
The pytest migration maintains all original test logic while improving readability.
229-243: LGTM! SQL injection test correctly validates security posture.The test appropriately accepts two valid security outcomes:
- Raising ValueError to reject malicious input
- Sanitizing the input to remove dangerous constructs
The assertions verify that neither
DROPnor semicolons appear in the generated SQL.docker-compose.yml (3)
55-70: LGTM! Resource limits and environment variables are well-configured.The updates include:
- New GRAPHQL_PORT for the GraphQL API
- Concrete default for ALWAYS_FRESH_INSTALL (false) instead of a template
- Appropriate resource limits (2GB max, 1GB reservation) to prevent resource exhaustion
- Clear logging configuration for troubleshooting
72-78: LGTM! Restart policy and volume definitions are appropriate.The
restart: unless-stoppedpolicy ensures the container survives host reboots while allowing manual stops, and the minimal volume definitions are sufficient for local Docker installations.
18-27: Update documentation to clarify migration path and align examples with named volumes approach.The change from
${APP_DATA_LOCATION}bind mounts to named volumes (netalertx_config,netalertx_db) is confirmed as breaking for existing deployments. However, comprehensive migration guidance exists in docs/MIGRATION.md with automatic database/config migration support, and storage validation is enforced bycheck-persistent_storage.sh.The concerns are documentation-related, not missing functionality:
- docs/DOCKER_COMPOSE.md still shows bind mount examples with environment variables as the primary approach, which contradicts the production docker-compose.yml now using named volumes
- MIGRATION.md exists but users upgrading from bind mounts may not discover it—add a banner or link in docker-compose.yml comments pointing to it
- Consider adding a brief upgrade note in docker-compose.yml explaining why named volumes are used and when users need to consult MIGRATION.md
The migration path is viable but documentation should be better integrated to guide users proactively.
install/production-filesystem/services/scripts/check-persistent_storage.sh (2)
48-52: Warning-only behavior is correctly implemented (once variable scope bug is fixed).The script appropriately:
- Provides a 5-second delay to ensure users see the warning
- Uses non-fatal exit (exit 0) to avoid blocking container startup
- Documents the warning-only intent in comments
This aligns with the script's purpose as a user-facing notification rather than a hard requirement.
4-35: Critical bug: failures variable scope prevents error detection.Line 12 assigns
failures=1inside the function, but in POSIX shell, this creates a local variable that doesn't affect the globalfailuresinitialized at line 42. The check at line 48 will never detect failures fromwarn_if_not_persistent_mount().Fix by having the function return a status code and accumulate failures in the caller:
warn_if_not_persistent_mount() { path="$1" # Check if the path is a mount point by looking for it in /proc/self/mountinfo # We are looking for an exact match in the mount point column (field 5) if awk -v target="${path}" '$5 == target {found=1} END {exit found ? 0 : 1}' /proc/self/mountinfo; then return 0 fi - failures=1 YELLOW=$(printf '\033[1;33m') RESET=$(printf '\033[0m') >&2 printf "%s" "${YELLOW}" >&2 cat <<EOF ══════════════════════════════════════════════════════════════════════════════ ⚠️ ATTENTION: ${path} is not a persistent mount. Your data in this directory may not persist across container restarts or upgrades. To ensure your settings and history are saved, you must mount this directory as a persistent volume. Fix: mount ${path} explicitly as a bind mount or a named volume: # Bind mount --mount type=bind,src=/path/on/host,dst=${path} # Named volume --mount type=volume,src=netalertx-data,dst=${path} Apply one of these mount options and restart the container. ══════════════════════════════════════════════════════════════════════════════ EOF >&2 printf "%s" "${RESET}" + return 1 } # If NETALERTX_DEBUG=1 then we will exit if [ "${NETALERTX_DEBUG}" = "1" ]; then exit 0 fi failures=0 # NETALERTX_DB is a file, so we check its directory -warn_if_not_persistent_mount "$(dirname "${NETALERTX_DB_FILE}")" -warn_if_not_persistent_mount "${NETALERTX_CONFIG}" +warn_if_not_persistent_mount "$(dirname "${NETALERTX_DB_FILE}")" || failures=$((failures + 1)) +warn_if_not_persistent_mount "${NETALERTX_CONFIG}" || failures=$((failures + 1))Likely an incorrect or invalid review comment.
install/production-filesystem/services/scripts/check-storage-extra.sh (1)
35-40: Exit behavior is correct (once variable scope bug is fixed).Unlike check-persistent_storage.sh, this script correctly exits with status 1 when failures are detected, making it a blocking check rather than a warning-only check. The 5-second delay provides time for users to read the error message.
.devcontainer/Dockerfile (2)
1-1: Auto-generated; defer review to generator and root Dockerfile.Per project learnings, this file is generated by .devcontainer/scripts/generate-configs.sh; review should target the generator and root Dockerfile, not this artifact. Skipping detailed review. Based on learnings.
148-156: Confirm intent: granting NET_RAW/NET_ADMIN caps to the venv Python binary.
setcap ... $(readlink -f ${VIRTUAL_ENV_BIN}/python)enables raw sockets for Python (e.g., scapy). Confirm this is required in Runner and remains safe post-hardened perms. If only needed in devcontainer, consider limiting to that stage.Would you like me to trace call-sites needing raw sockets and suggest stage-scoped caps?
|
Coderabbit is a bit overly verbose suddenly. It's good to have more info but it takes effort to scroll to the bottom of each comment. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
install/production-filesystem/services/start-backend.sh (1)
14-15: SC2046: confirm intent for extra launch parameters; quote if single token.If the parameters file is a single argument, quote the substitution; if it’s multiple args, keep as-is. Also mirror the choice in the echo for accuracy.
Option if single token:
-echo "Starting python3 $(cat /services/config/python/backend-extra-launch-parameters 2>/dev/null) -m server > /app/log/stdout.log 2> >(tee /app/log/stderr.log >&2)" -exec python3 $(cat /services/config/python/backend-extra-launch-parameters 2>/dev/null) -m server > /app/log/stdout.log 2> >(tee /app/log/stderr.log >&2) +echo "Starting python3 \"$(cat /services/config/python/backend-extra-launch-parameters 2>/dev/null)\" -m server > /app/log/stdout.log 2> >(tee /app/log/stderr.log >&2)" +exec python3 "$(cat /services/config/python/backend-extra-launch-parameters 2>/dev/null)" -m server > /app/log/stdout.log 2> >(tee /app/log/stderr.log >&2).devcontainer/Dockerfile (1)
188-190: Remove redundant directory creation loop.This file contains the same redundant for-loop issue as the main Dockerfile (lines 185-187). Since this file is auto-generated from the root Dockerfile and
.devcontainer/resources/devcontainer-Dockerfile, fixing the issue in the source Dockerfile will resolve it here as well.Based on learnings
🧹 Nitpick comments (3)
install/production-filesystem/services/scripts/check-app-permissions.sh (1)
54-54: Minor: remove unused variable.
ALL_PATHSisn’t used.-ALL_PATHS="${READ_ONLY_PATHS} ${READ_WRITE_PATHS}"test/test_compound_conditions.py (1)
237-237: Consider prefixing unused unpacked variables with underscore.When unpacking tuples but only using one value, prefix unused variables with
_to signal intent and silence linter warnings. For example,_sql, params = builder.build_safe_condition(condition)orsql, _params = ....Apply these diffs to address the linter hints:
- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)- sql, params = builder.build_safe_condition(condition) + _sql, params = builder.build_safe_condition(condition)Also applies to: 250-250, 304-304, 315-315
Dockerfile (1)
179-190: Remove redundant directory creation loop.The
install -dcommand at line 182 already creates all directories inREAD_WRITE_FOLDERSwith the correct ownership and permissions. The subsequent for-loop (lines 185-187) repeats the same operation, which is redundant and adds unnecessary build time.Apply this diff to remove the redundant loop:
RUN chown -R ${READ_ONLY_USER}:${READ_ONLY_GROUP} ${READ_ONLY_FOLDERS} && \ chmod -R 004 ${READ_ONLY_FOLDERS} && \ find ${READ_ONLY_FOLDERS} -type d -exec chmod 005 {} + && \ install -d -o ${NETALERTX_USER} -g ${NETALERTX_GROUP} -m 700 ${READ_WRITE_FOLDERS} && \ chown -R ${NETALERTX_USER}:${NETALERTX_GROUP} ${READ_WRITE_FOLDERS} && \ chmod -R 600 ${READ_WRITE_FOLDERS} && \ find ${READ_WRITE_FOLDERS} -type d -exec chmod 700 {} + && \ chown ${READ_ONLY_USER}:${READ_ONLY_GROUP} /entrypoint.sh /opt /opt/venv && \ chmod 005 /entrypoint.sh ${SYSTEM_SERVICES}/*.sh /app /opt /opt/venv && \ - for dir in ${READ_WRITE_FOLDERS}; do \ - install -d -o ${NETALERTX_USER} -g ${NETALERTX_GROUP} -m 700 "$dir"; \ - done && \ apk del apk-tools && \ rm -Rf /var /etc/sudoers.d/* /etc/shadow /etc/gshadow /etc/sudoers \ /lib/apk /lib/firmware /lib/modules-load.d /lib/sysctl.d /mnt /home/ /root \ /srv /media && \ sed -i "/^\(${READ_ONLY_USER}\|${NETALERTX_USER}\):/!d" /etc/passwd && \ sed -i "/^\(${READ_ONLY_GROUP}\|${NETALERTX_GROUP}\):/!d" /etc/group && \ echo -ne '#!/bin/sh\n"$@"\n' > /usr/bin/sudo && chmod +x /usr/bin/sudo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.devcontainer/Dockerfile(4 hunks).devcontainer/devcontainer.json(2 hunks).devcontainer/resources/devcontainer-Dockerfile(1 hunks).devcontainer/scripts/setup.sh(2 hunks).vscode/tasks.json(1 hunks)Dockerfile(1 hunks)install/production-filesystem/entrypoint.sh(1 hunks)install/production-filesystem/services/scripts/check-app-permissions.sh(1 hunks)install/production-filesystem/services/scripts/check-capabilities.sh(1 hunks)install/production-filesystem/services/scripts/check-first-run-config.sh(2 hunks)install/production-filesystem/services/scripts/check-nonpersistent-storage.sh(1 hunks)install/production-filesystem/services/scripts/check-persistent-storage.sh(1 hunks)install/production-filesystem/services/scripts/check-ramdisk.sh(1 hunks)install/production-filesystem/services/scripts/check-root.sh(1 hunks)install/production-filesystem/services/scripts/check-user-netalertx.sh(1 hunks)install/production-filesystem/services/scripts/update_vendors.sh(1 hunks)install/production-filesystem/services/start-backend.sh(1 hunks)install/production-filesystem/services/start-crond.sh(1 hunks)install/production-filesystem/services/start-nginx.sh(1 hunks)install/production-filesystem/services/start-php-fpm.sh(1 hunks)test/docker_tests/test_container_environment.py(1 hunks)test/test_compound_conditions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .devcontainer/resources/devcontainer-Dockerfile
- install/production-filesystem/services/scripts/check-user-netalertx.sh
- install/production-filesystem/services/scripts/check-first-run-config.sh
- install/production-filesystem/services/scripts/check-root.sh
- install/production-filesystem/services/scripts/check-capabilities.sh
- install/production-filesystem/services/start-nginx.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/test_compound_conditions.pytest/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/test_compound_conditions.pytest/docker_tests/test_container_environment.py
🧠 Learnings (7)
📚 Learning: 2025-10-26T15:39:36.689Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1235
File: install/production-filesystem/services/scripts/check-first-run-config.sh:0-0
Timestamp: 2025-10-26T15:39:36.689Z
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
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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
PR: jokob-sk/NetAlertX#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-10-19T01:40:57.095Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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-09-20T14:13:33.635Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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
PR: jokob-sk/NetAlertX#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-09-20T14:09:29.159Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
.devcontainer/Dockerfile
🧬 Code graph analysis (3)
install/production-filesystem/services/scripts/check-persistent-storage.sh (1)
install/production-filesystem/services/scripts/check-nonpersistent-storage.sh (1)
warn_if_not_persistent_mount(9-34)
install/production-filesystem/services/scripts/check-nonpersistent-storage.sh (1)
install/production-filesystem/services/scripts/check-persistent-storage.sh (1)
warn_if_not_persistent_mount(38-67)
test/test_compound_conditions.py (1)
server/db/sql_safe_builder.py (2)
SafeConditionBuilder(23-623)build_safe_condition(122-151)
🪛 Ruff (0.14.1)
test/test_compound_conditions.py
237-237: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
250-250: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
304-304: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
315-315: Unpacked variable sql is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
test/docker_tests/test_container_environment.py
34-34: subprocess call: check for execution of untrusted input
(S603)
35-35: Starting a process with a partial executable path
(S607)
44-44: subprocess call: check for execution of untrusted input
(S603)
45-45: Starting a process with a partial executable path
(S607)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
76-76: subprocess call: check for execution of untrusted input
(S603)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
180-180: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
225-225: subprocess call: check for execution of untrusted input
(S603)
242-243: try-except-pass detected, consider logging the exception
(S110)
242-242: Do not catch blind exception: Exception
(BLE001)
249-249: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
252-256: Avoid specifying long messages outside the exception class
(TRY003)
698-698: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Shellcheck (0.11.0)
install/production-filesystem/services/scripts/check-nonpersistent-storage.sh
[warning] 5-5: In POSIX sh, == in place of = is undefined.
(SC3014)
install/production-filesystem/services/start-backend.sh
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
install/production-filesystem/services/scripts/check-app-permissions.sh
[warning] 54-54: ALL_PATHS appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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 (6)
.vscode/tasks.json (1)
165-167: Confirmdocker buildxchoice.The task uses
docker buildx buildrather than the simplerdocker build. Verify this is intentional (e.g., for multi-platform builds) or consider usingdocker buildfor standard single-platform scenarios.install/production-filesystem/services/scripts/check-ramdisk.sh (1)
40-48: Scoping explanation is incorrect; refactored diffs are flawed. Core issue exists but proposed solution is wrong.The review comment contains a critical error in its analysis:
Scoping claim is incorrect: The statement that
warn_if_not_dedicated_mountsetsfailures=1"locally (scoped to the function)" is wrong. Without thelocalkeyword,failures=1inside the function modifies the globalfailuresvariable. The variable IS properly propagated.Core issue is real: The script does exit unconditionally with
exit 0(line 48) regardless offailuresvalue, and the conf.active check (lines 43–45) doesn't contribute tofailures. This contradicts security hardening objectives.Proposed diffs are flawed: The refactored
warn_if_not_dedicated_mountfunction removes the successful mount detection logic (theif awk... return 0block). This would make the function always fail and print warnings even for properly configured mounts, causing false positives.Correct minimal fix: Change
exit 0toexit "${failures}"and setfailures=1when conf.active is not writable. The function itself needs no refactoring.Likely an incorrect or invalid review comment.
install/production-filesystem/services/start-crond.sh (1)
26-26: Echo looks good; keeps command visibility without behavior change..devcontainer/scripts/setup.sh (1)
28-65: LGTM—robust docker socket access configuration.The
ensure_docker_socket_accessfunction is well-structured with proper error handling, fallback mechanisms for group resolution, and idempotent group membership updates. The approach of resolving the socket GID and ensuring the netalertx user is in the corresponding group is sound..devcontainer/devcontainer.json (1)
26-28: LGTM—devcontainer lifecycle configuration is well-structured.The docker socket mount enables integration testing, and the lifecycle commands properly separate dependency installation (postCreateCommand) from environment setup (postStartCommand). This aligns well with the setup.sh changes and supports the PR's testing objectives.
Also applies to: 46-51
install/production-filesystem/services/scripts/check-persistent-storage.sh (1)
76-77: The review comment is incorrect and should be dismissed.The variables
NETALERTX_DB_FILEandNETALERTX_CONFIGare defined in the Dockerfile asENVvariables (lines 46 and 51), which automatically makes them available to all processes within the container at runtime. The entrypoint.sh sources check-persistent-storage.sh via a loop that runs allcheck-*.shscripts, and these variables are inherited by that process.This approach is consistent across the codebase: other scripts in the same directory (check-first-run-db.sh, check-first-run-config.sh, check-app-permissions.sh) use these same variables without explicit validation. Docker's
ENVdirective provides implicit guarantee of availability, eliminating the need for additional checks.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
install/production-filesystem/entrypoint.sh (2)
86-86: Quote variable expansion to protect against spaces.
${SYSTEM_SERVICES_SCRIPTS}should be quoted to prevent word splitting if the path contains spaces.-bash ${SYSTEM_SERVICES_SCRIPTS}/update_vendors.sh & +bash "${SYSTEM_SERVICES_SCRIPTS}/update_vendors.sh" &
56-56: Initialize FAILED_STATUS to "0" for numeric consistency.Line 56 initializes
FAILED_STATUS=""(empty), but line 280 uses it with the numeric-eqoperator. When no service fails, FAILED_STATUS remains empty, and[ "" -eq 0 ]is fragile. Initializing to"0"clarifies the numeric intent and ensures consistent behavior.-FAILED_STATUS="" +FAILED_STATUS="0"Then at line 280, the logic remains the same but is now robust:
-if [ "${FAILED_STATUS}" -eq 0 ] && [ "${FAILED_NAME}" != "signal" ]; then +if [ "${FAILED_STATUS}" -eq 0 ] && [ "${FAILED_NAME}" != "signal" ]; then FAILED_STATUS=1 fiAlso applies to: 280-282
test/docker_tests/test_container_environment.py (6)
1-8: Use logger.mylog for debug output; drop broad except.Aligns with coding_guidelines and reduces noisy prints.
@@ -import os +import os @@ -import pytest +import pytest +import logger @@ - # Print the full Docker command for debugging - print("\n--- DOCKER CMD ---\n", " ".join(cmd), "\n--- END CMD ---\n") + # Debug: full Docker command + logger.mylog("debug", f"DOCKER CMD: {' '.join(cmd)}") @@ - # Print container output for debugging in every test run. - try: - print("\n--- CONTAINER out ---\n", result.output) - except Exception: - pass + # Debug: container output + logger.mylog("debug", f"CONTAINER OUT:\n{result.output}")Also applies to: 223-243
631-641: Make readonly nginx_conf test actually read-only.Currently simulates zero perms, not a read-only mount. Match other readonly tests.
def test_readonly_nginx_conf_mount(tmp_path: pathlib.Path) -> None: @@ - paths = _setup_mount_tree(tmp_path, "readonly_nginx_conf") - _setup_zero_perm_dir(paths, "nginx_conf") - volumes = _build_volume_args(paths) - try: - result = _run_container("readonly-nginx-conf", volumes) - _assert_contains(result, "Write permission denied", result.args) - _assert_contains(result, "/services/config/nginx/conf.active", result.args) - assert result.returncode != 0 - finally: - _restore_zero_perm_dir(paths, "nginx_conf") + paths = _setup_mount_tree(tmp_path, "readonly_nginx_conf") + volumes = _build_volume_args(paths, read_only={"nginx_conf"}) + result = _run_container("readonly-nginx-conf", volumes) + _assert_contains(result, "Write permission denied", result.args) + _assert_contains(result, str(VOLUME_MAP["nginx_conf"]), result.args) + assert result.returncode != 0
637-638: Use VOLUME_MAP for asserted paths to avoid drift.Prefer VOLUME_MAP lookups over hard-coded strings.
- _assert_contains(result, "/services/config/nginx/conf.active", result.args) + _assert_contains(result, str(VOLUME_MAP["nginx_conf"]), result.args) @@ - _assert_contains(result, "/services/config/nginx/conf.active", result.args) + _assert_contains(result, str(VOLUME_MAP["nginx_conf"]), result.args) @@ - _assert_contains(result, "/app/config", result.args) + _assert_contains(result, str(VOLUME_MAP["app_config"]), result.args) @@ - _assert_contains(result, "/app/db", result.args) + _assert_contains(result, str(VOLUME_MAP["app_db"]), result.args)Also applies to: 679-680, 905-907, 924-926
223-231: Optional: gate verbose logs behind env var.Keeps CI output clean; enable with NETALERTX_TEST_DEBUG=1.
- logger.mylog("debug", f"DOCKER CMD: {' '.join(cmd)}") + if os.environ.get("NETALERTX_TEST_DEBUG"): + logger.mylog("debug", f"DOCKER CMD: {' '.join(cmd)}") @@ - logger.mylog("debug", f"CONTAINER OUT:\n{result.output}") + if os.environ.get("NETALERTX_TEST_DEBUG"): + logger.mylog("debug", f"CONTAINER OUT:\n{result.output}")
25-26: Optional: autouse check for Docker CLI, skip if unavailable.Prevents confusing failures on dev machines/CI runners without Docker.
Add near top of file:
@pytest.fixture(autouse=True, scope="module") def _require_docker(): if shutil.which("docker") is None: pytest.skip("Docker CLI not found in PATH; skipping docker tests.")Want a PR to add this across docker test modules?
9-11: Track TODOs with issues.ALWAYS_FRESH_INSTALL and named volume mount still untested.
I can open issues and scaffold tests for these. Proceed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.devcontainer/devcontainer.json(2 hunks).vscode/tasks.json(1 hunks)install/production-filesystem/entrypoint.sh(1 hunks)server/api_server/nettools_endpoint.py(3 hunks)test/docker_tests/test_container_environment.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/tasks.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_container_environment.pyserver/api_server/nettools_endpoint.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_container_environment.py
server/api_server/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When introducing new server functionality, add endpoints only within 'server/api_server/*' and keep authorization checks consistent.
Files:
server/api_server/nettools_endpoint.py
🧠 Learnings (3)
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#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/devcontainer.json
📚 Learning: 2025-10-26T16:45:41.228Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1235
File: install/production-filesystem/services/scripts/check-ramdisk.sh:0-0
Timestamp: 2025-10-26T16:45:41.228Z
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-10-26T15:39:36.689Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1235
File: install/production-filesystem/services/scripts/check-first-run-config.sh:0-0
Timestamp: 2025-10-26T15:39:36.689Z
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
🪛 Ruff (0.14.1)
test/docker_tests/test_container_environment.py
34-34: subprocess call: check for execution of untrusted input
(S603)
35-35: Starting a process with a partial executable path
(S607)
44-44: subprocess call: check for execution of untrusted input
(S603)
45-45: Starting a process with a partial executable path
(S607)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
76-76: subprocess call: check for execution of untrusted input
(S603)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
180-180: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
225-225: subprocess call: check for execution of untrusted input
(S603)
242-243: try-except-pass detected, consider logging the exception
(S110)
242-242: Do not catch blind exception: Exception
(BLE001)
249-249: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
252-256: Avoid specifying long messages outside the exception class
(TRY003)
698-698: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/nettools_endpoint.py
22-24: Avoid specifying long messages outside the exception class
(TRY003)
26-26: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Do not catch blind exception: Exception
(BLE001)
115-115: subprocess call: check for execution of untrusted input
(S603)
133-133: Do not catch blind exception: Exception
(BLE001)
⏰ 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 (8)
install/production-filesystem/entrypoint.sh (1)
56-77: Shell test bug fixed.The prior review flagged the unsafe
[ ${FAILED_STATUS} ]pattern underset -u. Line 74 now correctly uses[ -n "${FAILED_STATUS}" ]with proper quoting and the non-empty check. ✓.devcontainer/devcontainer.json (3)
26-28: Verify Docker socket mounting intent and document security implications.Binding
/var/run/docker.sockgrants the container significant privileges (effectively root access to the host Docker daemon). Confirm this is necessary for integration tests and document the security trade-off.Consider adding a comment explaining why this is required:
"mounts": [ - "source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind" //used for testing various conditions in docker + "source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind" // WARNING: grants root-equivalent access for docker integration tests ],
49-52: Verify postStartCommand execution order and timing.The background build process (
nohup screen docker buildx build...) may have ordering issues or race conditions with other setup steps. Verify that:
- The setup.sh script (
${containerWorkspaceFolder}/.devcontainer/scripts/setup.sh) completes before dependent tasks- The docker buildx command properly handles Docker socket initialization from setup.sh
Consider making the docker buildx build sequential if test execution depends on the image being ready:
- "postStartCommand": { - "Start Environment":"${containerWorkspaceFolder}/.devcontainer/scripts/setup.sh", - "Build test-container":"nohup screen docker buildx build --platform linux/amd64 --tag netalertx-test . & disown" + "postStartCommand": { + "Start Environment":"${containerWorkspaceFolder}/.devcontainer/scripts/setup.sh && docker buildx build --platform linux/amd64 --tag netalertx-test ." },
46-48: Verify pip package versions.Ensure the installed packages (
pytest,docker,debugpy) are pinned to compatible versions or explicitly allow any version. Currently unpinned packages may introduce unexpected behavior changes.Consider pinning versions:
- "Install Pip Requirements": "/opt/venv/bin/pip3 install pytest docker debugpy" + "Install Pip Requirements": "/opt/venv/bin/pip3 install pytest>=7.0 docker>=6.0 debugpy>=1.6"server/api_server/nettools_endpoint.py (4)
5-6: LGTM: Appropriate imports for path resolution.
105-111: LGTM: Proper graceful degradation.Returning 503 when the CLI is unavailable clearly communicates service unavailability to callers.
113-120: LGTM: Security concern resolved.Using the validated absolute path
SPEEDTEST_CLI_PATHeliminates the PATH hijacking risk identified in the previous review. The S603 static analysis warning is a false positive here.
133-138: LGTM: Reasonable defensive error handling.The broad exception handler ensures unexpected errors return a proper HTTP 500 response rather than crashing the endpoint.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/docker_tests/test_container_environment.py (1)
114-127: Hardcoded seed paths still break portability — unresolved from prior review.These absolute
/workspaces/NetAlertX/...paths prevent running tests outside the devcontainer. Resolve paths relative to the repo root or use an environment variable override, and skip gracefully if source files are missing.Apply this diff:
+ repo_root = pathlib.Path(os.environ.get("NETALERTX_REPO_ROOT", str(pathlib.Path(__file__).resolve().parents[2]))) + if seed_config: config_file = paths["app_config"] / "app.conf" + cfg_src = repo_root / "back" / "app.conf" + if not cfg_src.exists(): + pytest.skip(f"Seed file not found: {cfg_src}") shutil.copyfile( - "/workspaces/NetAlertX/back/app.conf", + cfg_src, config_file, ) config_file.chmod(0o600) if seed_db: db_file = paths["app_db"] / "app.db" + db_src = repo_root / "db" / "app.db" + if not db_src.exists(): + pytest.skip(f"Seed file not found: {db_src}") shutil.copyfile( - "/workspaces/NetAlertX/db/app.db", + db_src, db_file, ) db_file.chmod(0o600)
🧹 Nitpick comments (2)
test/docker_tests/test_container_environment.py (2)
823-841: Consider asserting expected warning message.The test validates exit code but doesn't verify the expected "NetAlertX is running as UID 1000:1000" warning mentioned in the docstring. Adding an output assertion would make the test more robust.
result = _run_container( "run-as-1000", volumes, user="1000:1000", ) + _assert_contains(result, "NetAlertX is running as UID", result.args) assert result.returncode != 0
844-862: Consider asserting expected warning message.The test validates exit code but doesn't verify the expected "not running with --network=host" warning mentioned in the docstring. Adding an output assertion would strengthen the test.
result = _run_container( "missing-host-network", volumes, network_mode=None, ) + _assert_contains(result, "not running with --network=host", result.args) assert result.returncode != 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/devcontainer.json(2 hunks)test/docker_tests/test_container_environment.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/devcontainer.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_container_environment.py
🪛 Ruff (0.14.1)
test/docker_tests/test_container_environment.py
42-42: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
84-84: subprocess call: check for execution of untrusted input
(S603)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
188-188: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
233-233: subprocess call: check for execution of untrusted input
(S603)
250-251: try-except-pass detected, consider logging the exception
(S110)
250-250: Do not catch blind exception: Exception
(BLE001)
257-257: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
260-264: Avoid specifying long messages outside the exception class
(TRY003)
706-706: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
test/docker_tests/test_container_environment.py (9)
9-16: Add skip guards for non‑Linux or missing Docker.Prevents hard errors in CI/dev where Docker/host networking isn’t available.
@@ -import os +import os +import sys @@ -pytestmark = [pytest.mark.docker, pytest.mark.feature_complete] +pytestmark = [ + pytest.mark.docker, + pytest.mark.feature_complete, + pytest.mark.skipif(shutil.which("docker") is None, reason="Docker not installed"), + pytest.mark.skipif(sys.platform != "linux", reason="Requires Linux host (--network=host)"), +]Also applies to: 33-34
100-113: Silence unused loop var.
targetisn’t used in this loop.- for key, target in VOLUME_MAP.items(): + for key, _ in VOLUME_MAP.items():
114-146: Repo‑relative seed paths look good.Nice removal of hard‑coded /workspaces; fallback discovery is sane. Consider skipping with pytest.skip instead of printing when seeds are absent to avoid false positives.
83-91: Bound the chown helper.Add a timeout so a hung
docker rundoesn’t stall the suite.subprocess.run( cmd, - check=True, + check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=60, )
249-270: Use logger.mylog instead of print.Per coding_guidelines for **/*.py. Gate verbosity (e.g., minimal/debug) to keep CI logs readable.
275-283: Modernize Optional typing.Align with usage elsewhere in this file.
-def _assert_contains(result, snippet: str, cmd: list[str] = None) -> None: +def _assert_contains(result, snippet: str, cmd: list[str] | None = None) -> None:
187-229: Minor: make docker args explicit and resilient.Optionally add
--pull=neverto avoid slow accidental pulls and set--initto reap child if entrypoint forks.
316-436: Reduce duplication with parametrization.These blocks share the same structure. Parametrize over mount key, user, and expected snippet to cut runtime and maintenance.
Also applies to: 438-588, 590-682, 710-798, 883-911, 913-952
40-49: Remove unused test helper functions.
_create_docker_volumeand_remove_docker_volumeare not referenced anywhere in the codebase. Remove them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/docker_tests/test_container_environment.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_container_environment.py
🪛 Ruff (0.14.1)
test/docker_tests/test_container_environment.py
42-42: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
84-84: subprocess call: check for execution of untrusted input
(S603)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
206-206: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
251-251: subprocess call: check for execution of untrusted input
(S603)
268-269: try-except-pass detected, consider logging the exception
(S110)
268-268: Do not catch blind exception: Exception
(BLE001)
275-275: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
278-282: Avoid specifying long messages outside the exception class
(TRY003)
724-724: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
test/docker_tests/test_container_environment.py (1)
710-798: Path assertions now match skipped mounts.Good cleanup on mismatched paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/docker_tests/test_container_environment.py (6)
129-146: Use logger.mylog; avoid unconditional prints.Replace prints with logger.mylog per guidelines and drop the broad except.
As per coding guidelines.@@ - print(f"[WARN] Seed file not found: {config_src}. Set NETALERTX_REPO_ROOT or run from repo root. Skipping copy.") + logger.mylog("minimal", f"Seed file not found: {config_src}. Set NETALERTX_REPO_ROOT or run from repo root. Skipping copy.") @@ - print(f"[WARN] Seed file not found: {db_src}. Set NETALERTX_REPO_ROOT or run from repo root. Skipping copy.") + logger.mylog("minimal", f"Seed file not found: {db_src}. Set NETALERTX_REPO_ROOT or run from repo root. Skipping copy.") @@ - print("\n--- DOCKER CMD ---\n", " ".join(cmd), "\n--- END CMD ---\n") + logger.mylog("debug", " ".join(cmd)) @@ - try: - print("\n--- CONTAINER out ---\n", result.output) - except Exception: - pass + try: + logger.mylog("trace", result.output) + except UnicodeEncodeError: + logger.mylog("trace", result.output.encode("utf-8", "replace").decode("utf-8"))Add near imports:
import logger # for logger.mylogAlso applies to: 249-251, 266-269
205-207: Use sticky bit for /tmp tmpfs./tmp should be 1777, not 0777.
- cmd.extend(["--tmpfs", "/tmp:mode=777"]) + cmd.extend(["--tmpfs", "/tmp:mode=1777"])
275-275: Explicit Optional typing for cmd.Fix implicit Optional per Ruff RUF013.
-def _assert_contains(result, snippet: str, cmd: list[str] = None) -> None: +def _assert_contains(result, snippet: str, cmd: list[str] | None = None) -> None:
100-101: Rename unused loop variable.Avoid shadowing and satisfy Ruff B007.
- for key, target in VOLUME_MAP.items(): + for key, _ in VOLUME_MAP.items():
20-23: Preflight skip when Docker or image missing.Add a session fixture to skip the module if
dockerorIMAGEis unavailable.# Add in this module or conftest.py import shutil, subprocess, pytest @pytest.fixture(scope="session", autouse=True) def _require_docker_image(): if not shutil.which("docker"): pytest.skip("Docker CLI not found") image = os.environ.get("NETALERTX_TEST_IMAGE", "netalertx-test") if subprocess.run(["docker","image","inspect",image], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode != 0: pytest.skip(f"Test image not present: {image}")
710-725: Assert non‑zero exit for misconfig scenarios.Harden tests: also assert
result.returncode != 0to catch regressions.@@ def test_missing_mount_app_db(... - raise AssertionError("Expected persistent mount warning for /app/db in container output.") + raise AssertionError("Expected persistent mount warning for /app/db in container output.") + assert result.returncode != 0 @@ def test_missing_mount_app_config(... - _assert_contains(result, "/app/config", result.args) + _assert_contains(result, "/app/config", result.args) + assert result.returncode != 0 @@ def test_missing_mount_app_log(... - _assert_contains(result, "/app/log", result.args) + _assert_contains(result, "/app/log", result.args) + assert result.returncode != 0 @@ def test_missing_mount_app_api(... - _assert_contains(result, "/app/api", result.args) + _assert_contains(result, "/app/api", result.args) + assert result.returncode != 0 @@ def test_missing_mount_services_run(... - _assert_contains(result, "Container startup checks failed with exit code", result.args) + _assert_contains(result, "Container startup checks failed with exit code", result.args) + assert result.returncode != 0 @@ def test_tmpfs_config_mount_warns(... - _assert_contains(result, "/app/config", result.args) + _assert_contains(result, "/app/config", result.args) + assert result.returncode != 0Also applies to: 734-739, 749-753, 762-767, 791-797, 925-935
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/docker_tests/test_container_environment.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_container_environment.py
🪛 Ruff (0.14.1)
test/docker_tests/test_container_environment.py
42-42: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
84-84: subprocess call: check for execution of untrusted input
(S603)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
206-206: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
251-251: subprocess call: check for execution of untrusted input
(S603)
268-269: try-except-pass detected, consider logging the exception
(S110)
268-268: Do not catch blind exception: Exception
(BLE001)
275-275: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
278-282: Avoid specifying long messages outside the exception class
(TRY003)
724-724: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/docker_tests/test_container_environment.py (2)
100-112: Rename unused loop variable to follow Python convention.The loop variable
targetis not used within the loop body. Per Python convention, unused variables should be prefixed with underscore.Apply this diff:
- for key, target in VOLUME_MAP.items(): + for key, _target in VOLUME_MAP.items(): folder_name = f"{label}_{key.upper()}_INTENTIONAL_NETALERTX_TEST" host_path = base / folder_name host_path.mkdir(parents=True, exist_ok=True)
275-275: Make Optional type hint explicit.The parameter
cmduses implicitOptionalwhich is prohibited by PEP 484.Apply this diff:
-def _assert_contains(result, snippet: str, cmd: list[str] = None) -> None: +def _assert_contains(result, snippet: str, cmd: list[str] | None = None) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/docker_tests/test_container_environment.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_container_environment.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_container_environment.py
🪛 Ruff (0.14.1)
test/docker_tests/test_container_environment.py
42-42: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
53-53: Starting a process with a partial executable path
(S607)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
84-84: subprocess call: check for execution of untrusted input
(S603)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Loop control variable target not used within loop body
Rename unused target to _target
(B007)
206-206: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
251-251: subprocess call: check for execution of untrusted input
(S603)
268-269: try-except-pass detected, consider logging the exception
(S110)
268-268: Do not catch blind exception: Exception
(BLE001)
275-275: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
278-282: Avoid specifying long messages outside the exception class
(TRY003)
720-720: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (2)
test/docker_tests/test_container_environment.py (2)
706-720: LGTM! Robust assertion with helpful debug output.The custom assertion logic properly checks for both the warning message and the specific path, with clear debug output on failure. The use of root user (line 713) correctly bypasses permission checks to isolate the missing mount detection.
316-950: Comprehensive test coverage for container security hardening scenarios.The test suite thoroughly covers all security hardening scenarios outlined in the PR objectives:
- Root-owned mounts detection
- Zero-permission handling
- Read-only mount validation
- Missing mount warnings
- Capability requirements
- User context validation
- Network mode requirements
- Automatic seeding behavior
- Tmpfs vs persistent storage warnings
The consistent test structure and clear documentation make this excellent integration test coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/production-filesystem/entrypoint.sh (1)
291-293: Numeric comparison will fail on empty string.If the service monitoring loop exits naturally without failures,
FAILED_STATUSremains an empty string (initialized at line 56). The numeric comparison-eqwill error: "integer expression expected."Initialize with a number or use a safer comparison:
-if [ "${FAILED_STATUS}" -eq 0 ] && [ "${FAILED_NAME}" != "signal" ]; then +if [ -z "${FAILED_STATUS}" ] && [ "${FAILED_NAME}" != "signal" ]; then FAILED_STATUS=1 fi
♻️ Duplicate comments (1)
test/docker_tests/test_entrypoint.py (1)
36-82: Test coverage is comprehensive.The four test cases effectively verify entrypoint behavior for check-skipping, APP_CONF_OVERRIDE logic, and environment interactions. Well-structured with appropriate markers.
Note: The
SKIP_TESTSvs.DISABLE_STARTUP_CHECKSmismatch is addressed in the entrypoint.sh review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/production-filesystem/entrypoint.sh(1 hunks)test/docker_tests/test_entrypoint.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 'logger.mylog' for logging at levels: none, minimal, verbose, debug, or trace.
Files:
test/docker_tests/test_entrypoint.py
test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests must reside under 'test/' and use pytest.
Files:
test/docker_tests/test_entrypoint.py
🧠 Learnings (1)
📚 Learning: 2025-10-26T16:45:41.228Z
Learnt from: adamoutler
PR: jokob-sk/NetAlertX#1235
File: install/production-filesystem/services/scripts/check-ramdisk.sh:0-0
Timestamp: 2025-10-26T16:45:41.228Z
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
🪛 Ruff (0.14.1)
test/docker_tests/test_entrypoint.py
21-21: Probable insecure usage of temporary file or directory: "/tmp:mode=777"
(S108)
33-33: 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). (1)
- GitHub Check: docker_dev
🔇 Additional comments (4)
install/production-filesystem/entrypoint.sh (3)
83-87: LGTM!The APP_CONF_OVERRIDE logic correctly checks both conditions and the tests verify all scenarios properly.
90-93: LGTM!The check-only mode implementation is correct and properly integrated with the tests.
97-97: LGTM!The switch from
shtobashaligns with the broader shell-tooling improvements in this PR.test/docker_tests/test_entrypoint.py (1)
15-33: LGTM!The helper function is well-implemented for Docker integration testing. The static analysis warnings about tmpfs security and subprocess usage are false positives—both are appropriate in test containers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Pull Request: Security Hardening and Proactive Issue Resolution
Summary
This PR implements critical security hardening measures and proactive user experience enhancements for NetAlertX, focusing on container security, SQL injection prevention, robust testing frameworks, and automated responses to common deployment and configuration issues. As a network monitoring tool handling sensitive device data, these changes significantly reduce the attack surface, improve resilience against exploitation vectors, and provide users with fail-safe mechanisms that prevent broken experiences through early detection and corrective guidance.
Security Impact Assessment
🔒 Container Security Hardening
Risk Addressed: Docker deployments with insecure configurations can expose host networks, lose persistent data, or allow privilege escalation. Common user issues include data loss from improper volume mounting, network isolation failures, and silent configuration errors leading to broken functionality.
Changes:
test/test_docker_integration.pyto validate secure configurations by systematically testing container configurations against identified and potential failure modes--network=hostrequirement for ARP scanning (prevents network isolation bypasses that cause scanning failures)/app/configand/app/db(prevents data loss attacks and common user data loss issues)Impact: Prevents unauthorized network access, ensures data integrity in containerized deployments, and eliminates common Docker-related support tickets by failing fast with clear guidance.
🛡️ SQL Injection Prevention
Risk Addressed: The reporting system was vulnerable to SQL injection attacks through user-controlled filter conditions, potentially allowing unauthorized data access or manipulation.
Changes:
Impact: Eliminates SQL injection vectors in device filtering and reporting features, protecting against data breaches.
🧪 Testing Framework Modernization
Risk Addressed: Legacy testing frameworks may miss security edge cases and provide inadequate coverage, leading to undetected vulnerabilities in production.
Changes:
Impact: Ensures ongoing security validation and reduces likelihood of regression vulnerabilities that could affect users.
Files Changed
test/test_docker_integration.py- New Docker security validation tests addressing common deployment issuestest/test_compound_conditions.py- Converted to pytest, enhanced SQL safety teststest/test_safe_builder_unit.py- Converted to pytest, expanded injection prevention testsValidation Steps
Risk Assessment
Proactive Issue Resolution
These changes implement a "fail-fast with guidance" approach to common user issues:
Compliance Considerations
These changes align with security best practices for:
Breaking Changes
None. All changes are backward-compatible and enhance existing functionality.
Future Enhancements
/install/production-filesystem/service/scripts/check-*.shscripts to halt users on misconfigurations and provide corrective guidance, reducing support overhead and improving user success ratesSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests