Skip to content

Fix trusted proxy client IP resolution for rate limiting - #18

Merged
osama1998H merged 1 commit into
masterfrom
codex/fix-security-report-issue
Mar 27, 2026
Merged

Fix trusted proxy client IP resolution for rate limiting#18
osama1998H merged 1 commit into
masterfrom
codex/fix-security-report-issue

Conversation

@osama1998H

@osama1998H osama1998H commented Mar 27, 2026

Copy link
Copy Markdown
Owner

Summary

  • add trusted proxy-aware client IP resolution and remove unconditional forwarded-header trust from global middleware
  • route auth handlers, request logging, and rate limiting through the shared resolved client IP
  • add TRUSTED_PROXY_CIDRS configuration and document the fail-closed default in user-facing docs
  • update the security report to mark finding 7 as fixed and reduce the remaining open issue count to 5

Testing

  • go test ./...

Summary by CodeRabbit

  • New Features

    • Added TRUSTED_PROXY_CIDRS environment variable to configure trusted proxy IP addresses for accurate client IP detection in rate limiting and audit trails.
  • Bug Fixes

    • Fixed security vulnerability: client IP is now determined safely based on trusted proxy configuration instead of unconditionally trusting forwarded headers.
  • Documentation

    • Updated configuration guides and security documentation with proxy CIDR configuration details and deployment requirements.

@coderabbitai

coderabbitai Bot commented Mar 27, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

A new trusted proxy configuration (TRUSTED_PROXY_CIDRS) is introduced to selectively trust forwarded IP headers based on the immediate peer's CIDR range. A ClientIPResolver middleware replaces unconditional forwarded-header processing, resolving effective client IP by validating peer trust before honoring X-Forwarded-For, X-Real-IP, or True-Client-IP headers.

Changes

Cohort / File(s) Summary
Environment & Configuration Documentation
.env.example, README.md
Added TRUSTED_PROXY_CIDRS environment variable definition with guidance on comma-separated proxy CIDR/IP configuration and default (empty) behavior.
Operational & Security Documentation
docs/horizontal-scaling.md, docs/security-report.md
Updated documentation to describe trusted-proxy client IP resolution, marked forwarded-header security finding as fixed, expanded production checklist to require explicit proxy CIDR configuration.
Core Client IP Resolution
internal/api/middleware/clientip.go
New middleware module implementing ClientIPResolver with trusted-proxy-aware IP parsing; exports PopulateClientIP, ClientIP, and GetClientIP utilities; includes IPv6 and host:port normalization logic.
Middleware & Handler Updates
internal/api/middleware/auth.go, internal/api/middleware/logger.go, internal/api/handlers/auth.go, internal/api/router.go
Added ContextKeyClientIP constant, replaced RealIPFromRequest with ClientIP() resolver across rate limiting, logging, auth handlers; integrated ClientIPResolver into router middleware chain.
Rate Limiting Refactor
internal/api/middleware/ratelimit.go
Generalized rate-limit dependency from *cache.Cache to rateLimitCounter interface; removed legacy realIP/RealIPFromRequest helpers; delegated client IP derivation to external ClientIP resolver.
Configuration Parsing
internal/config/config.go, internal/config/config_test.go
Extended ServerConfig with TrustedProxyCIDRs and TrustedProxyRanges fields; added parseTrustedProxyCIDRs and singleIPNet helpers to parse comma-separated entries and convert single IPs to /32//128 blocks; comprehensive unit tests for CIDR parsing.
Rate Limiting Tests
internal/api/middleware/ratelimit_test.go
Replaced TestRealIPFromRequest with resolver-focused TestClientIPResolverResolve and TestRateLimitUsesResolvedClientIP; added fake rate-limit counter for integration testing; covers untrusted peer rejection, trusted proxy chain traversal, and header spoofing defense.

Sequence Diagram

sequenceDiagram
    participant Client as Client
    participant Proxy as Trusted Proxy/LB
    participant Server as UniAuth Server
    participant ConfigDB as Config (TrustedProxyRanges)
    
    Client->>Proxy: HTTP Request
    Proxy->>Proxy: Add X-Forwarded-For: client-ip
    Proxy->>Server: Request + XFF Header
    
    Server->>Server: Extract Peer IP from RemoteAddr
    Server->>ConfigDB: Is Peer in TrustedProxyRanges?
    ConfigDB-->>Server: Yes (proxy is trusted)
    
    Server->>Server: Parse X-Forwarded-For list (R→L)
    Server->>Server: Select rightmost untrusted IP
    Server-->>Server: Resolved Client IP = parsed-from-header
    
    alt Untrusted Peer / No Forwarded Header
        Server-->>Server: Resolved Client IP = peer IP (direct)
    end
    
    Server->>Server: Store in Context[ClientIP]
    Server->>Server: Use for rate limiting, audit logs
    Server-->>Client: Response
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

size/XL

Poem

🐰 A proxy so trusty, with CIDR in hand,
We parse the forwarded, both false and grand,
The rabbit now knows who's really out there—
No spoofed IPs slip through our security snare! 🔒

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers main changes but is missing required template sections: no explicit Type of change selection, no Related issues link, and no detailed Testing checklist entries. Add Type of change checkboxes, specify Related issues (if any), and complete Testing section with specific test names and results.
Docstring Coverage ⚠️ Warning Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: implementing trusted proxy-aware client IP resolution and fixing rate limiting to use it.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-security-report-issue

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@osama1998H
osama1998H merged commit 96b3e37 into master Mar 27, 2026
3 of 4 checks passed
@osama1998H
osama1998H deleted the codex/fix-security-report-issue branch March 27, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant