fixes #249: Do not trust client-provided forwarded headers for the IP address check - #260
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe REST API plugin now checks request remote addresses against configured IP allowlists. It also detects a forwarded-header configuration that can make the check spoofable, displays a warning on the REST API page, and documents proxy configuration. The minimum Openfire version is now 5.1.0. ChangesREST API allowlist and proxy trust
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Browser
participant RESTAPIJSP
participant RESTServicePlugin
participant AdminConsolePlugin
Browser->>RESTAPIJSP: Request REST API page
RESTAPIJSP->>RESTServicePlugin: Check whether allowlist check is spoofable
RESTServicePlugin->>AdminConsolePlugin: Read forwarded-header and trusted-proxy settings
AdminConsolePlugin-->>RESTServicePlugin: Return configured settings
RESTServicePlugin-->>RESTAPIJSP: Return spoofability result
RESTAPIJSP-->>Browser: Render warning when result is true
Merge Risk: ⚪ Minimal · up to The IP allowlist change is ready to merge after normal checks. Administrators must restart the admin console for proxy-setting changes to take effect, as documented. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change removes a direct way to spoof an allowed IP address. Protection still depends on the server and reverse proxies being configured to trust forwarded addresses only from the right proxies. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1e1171a to
c4c2b66
Compare
…ers for the IP address check The IP address used to check against the list of allowed IP addresses was taken from the 'X-Forwarded-For' header (and some variants) when present. As any client can set that header, the check could easily be bypassed. The address is now obtained from the request's remote address only. The REST API is served by the admin console's web server, which (when configured to do so) replaces that address with the value from forwarded headers. Since Openfire 5.1.0 (OF-3261), it can be configured to do so only for requests from trusted proxies. This plugin now requires Openfire 5.1.0 or later. The admin console page of the REST API shows a warning when the IP address check is enabled, while the admin console uses forwarded headers without a list of trusted proxies.
dc7c536 to
f005502
Compare
2 participants
The IP address used to check against the list of allowed IP addresses was taken from the 'X-Forwarded-For' header (and some variants) when present. As any client can set that header, the check could easily be bypassed.
The address is now obtained from the request's remote address only. The REST API is served by the admin console's web server, which (when configured to do so) replaces that address with the value from forwarded headers. Since Openfire 5.1.0 (OF-3261), it can be configured to do so only for requests from trusted proxies. This plugin now requires Openfire 5.1.0 or later.
The admin console page of the REST API shows a warning when the IP address check is enabled, while the admin console uses forwarded headers without a list of trusted proxies.
Note that the only relevant commit in this PR is the last one. The others are from #258 on which this PR builds. This PR should be rebased after that one gets merged.
Summary by CodeRabbit
Security
Documentation
Compatibility