Conversation
📝 WalkthroughWalkthroughAdds a protected internal reverse-proxy at Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser/Client
participant Nginx as Nginx Proxy
participant Trust as Trust Logic
participant Backend as Backend Server
Client->>Nginx: GET /server/... (with Sec-Fetch-Site or Referer)
Nginx->>Trust: Evaluate Sec-Fetch-Site
alt Sec-Fetch-Site == "same-origin"
Trust->>Nginx: TRUSTED
else Sec-Fetch-Site missing
Nginx->>Trust: Compare Referer host vs Host (sec_legacy)
alt Referer matches Host
Trust->>Nginx: TRUSTED
else
Trust->>Nginx: UNTRUSTED
end
else
Trust->>Nginx: UNTRUSTED
end
alt TRUSTED
Nginx->>Nginx: rewrite /server/... -> /...
Nginx->>Backend: proxy to 127.0.0.1:BACKEND_PORT
Backend->>Nginx: response (200/401/404/502)
Nginx->>Client: forward response
else UNTRUSTED
Nginx->>Client: 403 Forbidden
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/api_endpoints/test_nginx_proxy_security.py`:
- Around line 11-136: Tests currently call requests.get(...) without timeouts
(e.g., in test_nginx_proxy_security_modern_check,
test_nginx_proxy_security_block_cross_site,
test_nginx_proxy_security_block_no_headers, etc.), which can hang CI; replace
every direct requests.get call with a centralized http_get(url, headers=None)
helper that uses requests.get(url, headers=headers, timeout=REQUEST_TIMEOUT) and
read REQUEST_TIMEOUT from the environment with a default of 5 seconds; update
all tests to call http_get(...) instead (preserving existing try/except behavior
and assertions) and ensure the helper is defined/imported at the top of the test
file so all test_* functions use it.
🧹 Nitpick comments (1)
install/production-filesystem/services/start-nginx.sh (1)
45-67: Guard jq parsing to avoid startup aborts on malformed APP_CONF_OVERRIDE.With
set -e, a bad JSON payload (or missing jq) will terminate the script. A small guard keeps nginx startup resilient.♻️ Suggested hardening
-# Check override (highest priority) -if [ -n "${APP_CONF_OVERRIDE:-}" ]; then - override_port=$(echo "${APP_CONF_OVERRIDE}" | jq -r '.GRAPHQL_PORT // empty') - if [ -n "${override_port}" ]; then - export BACKEND_PORT="${override_port}" - fi -fi +# Check override (highest priority) +if [ -n "${APP_CONF_OVERRIDE:-}" ] && command -v jq >/dev/null 2>&1; then + override_port=$(echo "${APP_CONF_OVERRIDE}" | jq -r '.GRAPHQL_PORT // empty' 2>/dev/null || true) + if [ -n "${override_port}" ]; then + export BACKEND_PORT="${override_port}" + fi +fi
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/api_endpoints/test_nginx_proxy_security.py`:
- Around line 131-141: The test_nginx_proxy_security_allow_port currently
asserts only a 200 response which is brittle; update the assertion to accept any
of the allowed status codes (200, 401, 404) to match other "allowed" tests.
Locate test_nginx_proxy_security_allow_port and change the assertion on
response.status_code (from http_get(url, headers=headers)) to check that
response.status_code is in (200, 401, 404) and keep the same ConnectionError
handling and message.
🧹 Nitpick comments (1)
test/api_endpoints/test_nginx_proxy_security.py (1)
5-13: Optional: centralize allowed status codes to avoid drift.The allowed status list is repeated in multiple tests; consider a shared constant (e.g.,
ALLOWED_STATUS = {200, 401, 404, 502}) to keep expectations consistent.
|
Thanks for the PR! regarding def getBuildTimeStampAndVersion():
"""
Retrieves the build timestamp and version from files within the
application directory. Initializes them if missing.
Returns:
tuple: (int buildTimestamp, str version)
"""
files_defaults = [
('front/buildtimestamp.txt', '0'),
('.VERSION', 'unknown')
]
results = []
for filename, default in files_defaults:
path = os.path.join(applicationPath, filename)
if not os.path.exists(path):
with open(path, 'w') as f:
f.write(default)
with open(path, 'r') as f:
content = f.read().strip() or default
# Convert buildtimestamp to int, leave version as string
value = int(content) if filename.endswith('buildtimestamp.txt') else content
results.append(value)
return tuple(results)The backend is expecting these 2 files to be present if there are missing (not added during build) it tries to create them and that's why write permission is needed here. I know it's not great... so if we can ensure these files are available during build we can get rid of this check / fallback requiring write access. This error seems to be related to this particular PR as running the tests on this PR here works fine: https://github.com/netalertx/NetAlertX/actions/runs/21407901199/job/61691616057?pr=1466 |
This PR introduces security enhancements to the NetAlertX backend, focusing on protecting internal endpoints and modernizing request validation.
Refs
#1452
#1403
#1451
Likely others.
Sec-Fetch-Site. This provides robust protection against CSRF and unauthorized cross-origin requests by trusting browser-level security metadata.Referer-based fallback for legacy browsers to maintain compatibility while improving overall security for modern clients.mapdirectives for better performance and maintainability.docs/API.mdto explicitly mark the/serverendpoint as internal-only. This clarifies the API boundary and directs users towards the stable GraphQL and REST interfaces..devcontainersetup and Docker compose test scenarios (test_docker_compose_scenarios.py) for better stability and cleaner exits.test_nginx_proxy_security.py) to verify the new security logic in a live container environment, covering modern browsers, legacy browsers, and unauthorized cross-site requests.front/js/api.jsto ensure frontend requests align with the new security expectations and headers.Port and path semantics (post-merge)
Port 20211 = Nginx (frontend entrypoint)
http(s)://<host>:20211/server/*→ proxied tolocalhost:20212inside the container.Port 20212 = NetAlertX service (API/backend)
http://<host>:20212/docsImportant:
/serveris an internal convenience route via Nginx and is not a supported external integration endpoint./serverdirectly.Supported connection patterns
1) Default out-of-box Docker experience (web + optional direct API docs)
Use this to describe the default “it works after docker run” behavior, and mention
:20212/docsfor browser-based testing.flowchart LR B[Browser] subgraph NAC[NetAlertX Container] N[Nginx listening on port 20211] A[Service on port 20212] N -->|Proxy /server to localhost:20212| A end B -->|port 20211| NAC B -->|port 20212| NACDoc notes:
http://<host>:20211/http://<host>:20212/docs(browser convenience)/serveris for the UI ↔ backend flow through Nginx, not for external consumers.2) Direct API consumer to port 20212 (NOT recommended)
This is the “possible, but discouraged” pattern.
flowchart LR B[Browser] -->|HTTPS| S[Any API Consumer app] subgraph NAC[NetAlertX Container] N[Nginx listening on port 20211] N -->|Proxy /server to localhost:20212| A[Service on port 20212] end S -->|Port 20212| NACDoc notes (include a warning callout):
3) Recommended: HTTPS/Auth reverse proxy in front of NetAlertX (proxy to 20211)
This is the “standard” deployment recommendation for web access.
flowchart LR B[Browser] -->|HTTPS| S[Any Auth/SSL proxy] subgraph NAC[NetAlertX Container] N[Nginx listening on port 20211] N -->|Proxy /server to localhost:20212| A[Service on port 20212] end S -->|port 20211| NACDoc notes:
4) Recommended: Proxied API consumer (TLS + source limiting) to port 20212
Use this for “external tool integrates with NetAlertX API securely”.
flowchart LR B[Browser] -->|HTTPS| S[Any API Consumer app] C[HTTPS/source-limiting Proxy] subgraph NAC[NetAlertX Container] N[Nginx listening on port 20211] N -->|Proxy /server to localhost:20212| A[Service on port 20212] end S -->|Port 443| C C -->|Port 20212| NACDoc notes:
One-line documentation rule to add everywhere
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.