Http sec fetch #1468
Merged
Http sec fetch #1468
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
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.
Collaborator
|
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 |
jokob-sk
reviewed
Jan 28, 2026
This was referenced Jan 28, 2026
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.