[fix] Stopped trusting X-Forwarded-For for the client IP address

[dee077]

Checklist

• I have read the OpenWISP Contributing Guidelines and OpenWISP Anti AI Spam Policy.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Related to https://github.com/openwisp/openwisp-radius/security/advisories/GHSA-hvmx-g7m2-vqx8

Description of Changes

Nginx passed the first value of the client-supplied X-Forwarded-For header to the application as REMOTE_ADDR (NINGX_REAL_REMOTE_ADDR='$real_ip'), so any client could choose the IP address seen by OpenWISP, including the addresses allowed to use the FreeRADIUS API.

• Changed the default of NINGX_REAL_REMOTE_ADDR to $remote_addr, the address of the connection to nginx.
• Added NGINX_TRUSTED_PROXIES: when set, nginx reads the client IP address from X-Forwarded-For only for requests coming from the listed reverse proxies (using set_real_ip_from, real_ip_header and real_ip_recursive).
• Each NGINX_TRUSTED_PROXIES entry is validated as an IP address or network; invalid entries stop the container with an error.
• A warning is logged at startup when NINGX_REAL_REMOTE_ADDR is still set to $real_ip or $http_x_forwarded_for.
• Updated the documentation of NINGX_REAL_REMOTE_ADDR and documented NGINX_TRUSTED_PROXIES.

Upgrade note: deployments behind a load balancer or an ingress that relied on the previous default must set NGINX_TRUSTED_PROXIES to the addresses of their proxies, otherwise the proxy address is used as the client IP address.

Screenshot

N/A

[coderabbitai]

📝 Walkthrough

The Nginx configuration now defaults NINGX_REAL_REMOTE_ADDR to $remote_addr and adds an empty NGINX_TRUSTED_PROXIES setting. At startup, the script validates each configured proxy address or network, exits on invalid entries, and generates real-IP directives for valid entries. The Nginx template includes those directives. The documentation describes forwarded-IP behavior and warns about trusting addresses that can set X-Forwarded-For.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant NGINX_TRUSTED_PROXIES
  participant init_command.sh
  participant nginx.template.conf
  participant Nginx
  NGINX_TRUSTED_PROXIES->>init_command.sh: Provide comma-separated proxy entries
  init_command.sh->>init_command.sh: Validate entries and generate real-IP directives
  init_command.sh->>nginx.template.conf: Export NGINX_REAL_IP_BLOCK
  nginx.template.conf->>Nginx: Include generated directives in HTTP configuration

Loading

Suggested reviewers: nemesifier

Merge Risk: ⚪ Minimal · up to 29ec9

No concrete merge-blocking behavior remains: forwarded addresses are opt-in through trusted proxies, and invalid proxy entries stop startup with a diagnostic.

Security architecture risk: 🔵 Low · up to 29ec9

The new default removes unconditional trust in forwarded client addresses. Residual risk depends on deployment configuration: legacy overrides remain accepted, and every host in a trusted proxy network can supply client identity. Actual proxy configuration and upgrade behavior were not available for verification.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The policy affects client-IP identity across server configurations inheriting the HTTP-level directives, including dashboard and API traffic. Its authorization consequences include the existing HTTP IP allow rule, the HTTPS admin network rule, and downstream application checks using REMOTE_ADDR. Every host admitted by a configured trusted network becomes an identity assertion source for this Nginx instance.

Security Findings and Attack Paths

• observed — The legacy raw-header address path remains available through explicit $real_ip or $http_x_forwarded_for overrides; startup warns but does not reject them. This exposure predates the PR, while the image default removes that selection. It is a residual deployment condition, not an established introduced or worsened concern.

Trust Boundaries and Controls

• inferred — Under the default templated configuration, an untrusted peer’s forwarded header no longer selects application client identity. Configuring trusted proxies delegates that authority to the listed peers and networks; security consequently depends on narrow network selection and correct forwarded-header handling by those proxies.

Resilience and Maintainability Implications

• observed — Custom-file mode starts Nginx before the new warning, validation, and rendering steps. This pre-existing mode leaves policy ownership with the custom configuration rather than applying the new environment-driven trust policy.

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

Failed checks: 1 error

Check name	Status	Explanation	Resolution
Ui Changes, Regression Test, Docs		The pull request changes application startup and nginx configuration behavior in images/common/init_command.sh, Dockerfile, and nginx.template.conf, but the authoritative diff changes no test fi…	Add regression tests to the pull request. Cover empty NGINX_TRUSTED_PROXIES, valid IPv4/IPv6 addresses and networks, invalid entries causing startup failure, generated set_real_ip_from/recursive configuration, and warnings for unsafe `N…

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title uses the required [fix] prefix and clearly describes the main security change: stopping trust in client-supplied X-Forwarded-For values by default.
Description check		The description includes the required checklist, issue reference, change summary, upgrade note, and screenshot section. It clearly explains the default change, trusted proxy configuration, validation …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Ui Changes, Regression Test, Docs

Explanation

The pull request changes application startup and nginx configuration behavior in images/common/init_command.sh, Dockerfile, and nginx.template.conf, but the authoritative diff changes no test file. Existing tests/runtests.py tests cover unrelated utility and startup paths; they do not verify the new trusted-proxy directives, invalid-entry failure, default behavior, or spoofing warning. The pull request does not provide the required regression test. Documentation was updated, and no visible UI change is present, so screenshots are not required.

Resolution

Add regression tests to the pull request. Cover empty NGINX_TRUSTED_PROXIES, valid IPv4/IPv6 addresses and networks, invalid entries causing startup failure, generated set_real_ip_from/recursive configuration, and warnings for unsafe NINGX_REAL_REMOTE_ADDR values. Keep the tests runnable in the repository's existing test suite.

• Fix all pre-merge checks with AI

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🔇 Additional comments: 2

images/openwisp_nginx/Dockerfile: 1

139-140: LGTM!

images/common/init_command.sh-61-65: 1

61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Add a comment before the validation and real-IP block.

The repository guidelines ask for a comment in shell scripts that explains opaque code. Add a short comment before this block. It must explain three points. set -f stops glob expansion of proxy entries. The warning covers the legacy spoofable values. The directives trust X-Forwarded-For only from listed proxies.

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: d0d205de-6de4-4319-bec5-fd96d35f582a

📥 Commits

Reviewing files that changed from the base of the PR and between 8ce26e5 and 29ec95b.

📒 Files selected for processing: 4

• docs/user/settings.rst
• images/common/init_command.sh
• images/openwisp_nginx/Dockerfile
• images/openwisp_nginx/nginx.template.conf

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

⏰ Context from checks skipped due to timeout: 4

• GitHub Check: auto-assign-issue / run-bot
• GitHub Check: CI Build
• GitHub Check: Analyze (actions)
• GitHub Check: Analyze (python)

🧰 Additional context used

📚 Code guidelines: 1

AGENTS.md — auto-discovered

📓 Path-based instructions: 4

Verify that documentation remains consistent with the implemented behavior and does not reference deprecated or removed functionality.

⚙️ CodeRabbit configuration file

Files:

• docs/user/settings.rst

Flag potential security vulnerabilities Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries Flag unused or redundant code Flag outdated or incorrect comments/docstrings Ensure new code handles err...

⚙️ CodeRabbit configuration file

Files:

• images/openwisp_nginx/Dockerfile
• images/openwisp_nginx/nginx.template.conf
• docs/user/settings.rst
• images/common/init_command.sh

Source excerpt: Be careful with shell scripts, Docker layers, permissions, entrypoints, health checks, and generated configuration.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• images/common/init_command.sh

Source excerpt: Use `README.rst` and `docs/` for setup, deployment, and usage.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• docs/user/settings.rst

🪛 Checkov (3.3.20)

images/openwisp_nginx/Dockerfile

[low] 1-155: Ensure that HEALTHCHECK instructions have been added to container images

(CKV_DOCKER_2)

---

[low] 1-155: Ensure that a user for the container has been created

(CKV_DOCKER_3)