Log IP address extracted according to trust proxy setting

[nDmitry]

Summary by CodeRabbit

Release Notes

• New Features
  • Added support for trusting proxy headers to properly detect client IP addresses when the application runs behind reverse proxies or load balancers.

[coderabbitai]

Warning

Rate limit exceeded

@nDmitry has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Walkthrough

The changes introduce a trustProxy configuration flag propagated through the REST API stack to enable proxy-aware client IP extraction. A new ip.go file provides IP extraction utilities supporting proxy headers, while firewall logic is simplified to delegate IP containment checks to these utilities rather than handling proxy extraction directly.

Changes

Cohort / File(s)	Summary
Server initialization cmd/tgfeed/main.go, internal/api/rest/server.go	Added trustProxy parameter to NewServer constructor and Server struct; propagated flag to middleware invocations in the request handler setup.
Client IP extraction utilities internal/api/rest/ip.go	New file with four functions: extractClientIP (errors on parse failures), mustExtractClientIP (empty string on errors), tryExtractFromProxyHeaders (checks X-Real-IP and X-Forwarded-For), and extractFromRemoteAddr (parses host:port). Handles proxy header priority when trustProxy is enabled.
Middleware updates internal/api/rest/middleware.go	Updated Logger and IPFilterMiddleware signatures to accept trustProxy parameter; both now use mustExtractClientIP for remote_addr logging instead of direct r.RemoteAddr access.
Firewall simplification internal/api/rest/firewall.go	Removed standalone proxy extraction functions (extractClientIP, tryExtractFromProxyHeaders, extractFromRemoteAddr). Converted isIPAllowed from standalone function to Firewall method; IsAllowed now delegates to f.isIPAllowed for IP containment checks without proxy logic.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Middleware
    participant IPExtractor as IP Extractor
    participant Firewall
    
    Client->>Middleware: HTTP Request
    Middleware->>IPExtractor: mustExtractClientIP(request, trustProxy)
    alt trustProxy enabled
        IPExtractor->>IPExtractor: tryExtractFromProxyHeaders
        Note over IPExtractor: Check X-Real-IP,<br/>X-Forwarded-For
    else trustProxy disabled
        IPExtractor->>IPExtractor: extractFromRemoteAddr
        Note over IPExtractor: Parse RemoteAddr
    end
    IPExtractor-->>Middleware: clientIP (string)
    Middleware->>Firewall: IsAllowed(request)
    Firewall->>Firewall: f.isIPAllowed(clientIP)
    alt IP in allowedNets
        Firewall-->>Middleware: true
        Middleware-->>Client: ✓ Request allowed
    else IP blocked
        Firewall-->>Middleware: false
        Middleware-->>Client: ✗ Request denied
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Add IP filtering feature with configuration options #7: Both PRs modify the REST IP-filtering codepath by updating server and middleware signatures, and refactoring client IP extraction and proxy-trust handling logic across the same files.

Poem

🐰 A proxy flag hops through the code,
Trust headers now lighten the load,
IP extraction, clean and bright,
Middleware delegates it right!
Simple firewall, logic trimmed tight! 🎯

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately summarizes the main change: adding IP address extraction logic that respects a trust proxy setting for logging purposes. This is evident from the new ip.go file, middleware updates, and the trustProxy parameter propagation throughout the codebase.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (3)

internal/api/rest/firewall.go (1)

61-76: isIPAllowed takes allowedNets as a parameter but always receives f.allowedNets.

Since this is a method on Firewall, it can access f.allowedNets directly, which simplifies the signature and avoids the redundant parameter passing on line 58.

♻️ Proposed simplification

-func (f *Firewall) isIPAllowed(ipStr string, allowedNets []*net.IPNet) bool {
+func (f *Firewall) isIPAllowed(ipStr string) bool {
 	ip := net.ParseIP(ipStr)
 
 	if ip == nil {
 		return false
 	}
 
-	for _, ipNet := range allowedNets {
+	for _, ipNet := range f.allowedNets {
 		if ipNet.Contains(ip) {
 			return true
 		}
 	}
 
 	return false
 }

And update the call site:

-	return f.isIPAllowed(clientIP, f.allowedNets)
+	return f.isIPAllowed(clientIP)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/rest/firewall.go` around lines 61 - 76, The isIPAllowed method
currently accepts allowedNets as a parameter but always uses the Firewall
receiver's f.allowedNets; remove the redundant parameter from the method
signature (keep func (f *Firewall) isIPAllowed(ipStr string) bool), update the
implementation to reference f.allowedNets directly, and update every call site
to stop passing allowedNets (e.g., calls that currently invoke isIPAllowed(...,
f.allowedNets) should be changed to isIPAllowed(...)). Ensure compilation by
adjusting any tests or uses that rely on the old signature.

internal/api/rest/ip.go (2)

46-56: Redundant len(ips) > 0 check — strings.Split always returns at least one element.

strings.Split(xff, ",") on a non-empty string will never return an empty slice, so this guard is dead code.

♻️ Simplified version

 	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		ips := strings.Split(xff, ",")
-
-		if len(ips) > 0 {
-			clientIP := strings.TrimSpace(ips[0])
-
-			if ip := net.ParseIP(clientIP); ip != nil {
-				return ip.String()
-			}
+		clientIP := strings.TrimSpace(strings.Split(xff, ",")[0])
+		if ip := net.ParseIP(clientIP); ip != nil {
+			return ip.String()
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/rest/ip.go` around lines 46 - 56, The len(ips) > 0 check is
redundant because strings.Split(xff, ",") on a non-empty xff always returns at
least one element; simplify the block in the handler that reads
r.Header.Get("X-Forwarded-For") by removing the if len(ips) > 0 guard and
directly using ips[0] (trim with strings.TrimSpace and validate with net.ParseIP
as is), so update the code around the ips, clientIP, and net.ParseIP usage to
drop the dead check while preserving behavior.

---

26-36: mustExtractClientIP duplicates extractClientIP — just delegate.

This function repeats the proxy-header + fallback logic from extractClientIP. If the extraction logic evolves, these two will diverge silently.

♻️ Proposed refactor

 func mustExtractClientIP(r *http.Request, trustProxy bool) string {
-	if trustProxy {
-		if clientIP := tryExtractFromProxyHeaders(r); clientIP != "" {
-			return clientIP
-		}
-	}
-
-	ip, _ := extractFromRemoteAddr(r.RemoteAddr)
-
-	return ip
+	ip, _ := extractClientIP(r, trustProxy)
+	return ip
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/rest/ip.go` around lines 26 - 36, mustExtractClientIP duplicates
the proxy-header + fallback logic present in extractClientIP; replace the
duplicate logic by delegating to extractClientIP to avoid divergence: change
mustExtractClientIP(r *http.Request, trustProxy bool) to simply return
extractClientIP(r, trustProxy) (or call extractClientIP and handle its return
exactly the same way mustExtractClientIP currently does), referencing the
functions mustExtractClientIP and extractClientIP so future changes to
extraction logic only live in extractClientIP.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/api/rest/firewall.go`:
- Around line 61-76: The isIPAllowed method currently accepts allowedNets as a
parameter but always uses the Firewall receiver's f.allowedNets; remove the
redundant parameter from the method signature (keep func (f *Firewall)
isIPAllowed(ipStr string) bool), update the implementation to reference
f.allowedNets directly, and update every call site to stop passing allowedNets
(e.g., calls that currently invoke isIPAllowed(..., f.allowedNets) should be
changed to isIPAllowed(...)). Ensure compilation by adjusting any tests or uses
that rely on the old signature.

In `@internal/api/rest/ip.go`:
- Around line 46-56: The len(ips) > 0 check is redundant because
strings.Split(xff, ",") on a non-empty xff always returns at least one element;
simplify the block in the handler that reads r.Header.Get("X-Forwarded-For") by
removing the if len(ips) > 0 guard and directly using ips[0] (trim with
strings.TrimSpace and validate with net.ParseIP as is), so update the code
around the ips, clientIP, and net.ParseIP usage to drop the dead check while
preserving behavior.
- Around line 26-36: mustExtractClientIP duplicates the proxy-header + fallback
logic present in extractClientIP; replace the duplicate logic by delegating to
extractClientIP to avoid divergence: change mustExtractClientIP(r *http.Request,
trustProxy bool) to simply return extractClientIP(r, trustProxy) (or call
extractClientIP and handle its return exactly the same way mustExtractClientIP
currently does), referencing the functions mustExtractClientIP and
extractClientIP so future changes to extraction logic only live in
extractClientIP.