Add IP filtering feature with configuration options #7
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds an IP-based firewall and middleware to enforce allowed IPs/CIDR ranges (optionally trusting proxy headers), wires it into server startup, and documents two environment variables for configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReverseProxy
participant Server
participant Firewall
Client->>ReverseProxy: HTTP request
ReverseProxy->>Server: Forward request (adds X-Real-IP / X-Forwarded-For)
Server->>Firewall: IPFilter.IsAllowed(request)
Firewall-->>Server: allowed / denied
alt allowed
Server->>Server: pass to router/handler chain
Server-->>Client: 200/handler response
else denied
Server-->>Client: 403 {"error":"access denied"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
|
@coderabbitai generate docstrings |
|
Warning Docstrings generation is disabled for your repository or organization. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13287 @nDmitry, I'll generate comprehensive docstrings for the exported entities in the IP filtering feature. Here are the improved docstrings: internal/api/rest/telegram.go// Scraper defines the interface for scraping Telegram channel data.
// Implementations must handle fetching channel information and posts from Telegram.
type Scraper interface {
Scrape(ctx context.Context, username string) (*entity.Channel, error)
}
// Generator defines the interface for generating RSS/Atom feeds from channel data.
// Implementations must support multiple feed formats and filtering options.
type Generator interface {
Generate(channel *entity.Channel, params *entity.FeedParams) ([]byte, error)
}
// IPFilter defines the interface for IP-based access control.
// Implementations determine whether an HTTP request should be allowed based on
// the client's IP address and optional reverse proxy headers.
type IPFilter interface {
IsAllowed(r *http.Request) bool
}internal/api/rest/ipfilter.go// Firewall implements IP-based access control for HTTP requests.
// It maintains a list of allowed IP addresses and CIDR ranges, and can
// optionally trust reverse proxy headers (X-Real-IP, X-Forwarded-For) to
// determine the client's real IP address.
type Firewall struct {
allowedNets []*net.IPNet
trustProxy bool
}
// NewFirewall creates a new Firewall instance with the specified configuration.
// The allowedIPsStr parameter accepts a comma-separated list of IP addresses
// and/or CIDR ranges (e.g., "10.0.0.0/24,192.168.1.1,2001:db8::/32").
// If allowedIPsStr is empty, all IP addresses are allowed by default.
// When trustProxy is true, the firewall will check X-Real-IP and X-Forwarded-For
// headers to determine the client's IP address, which is necessary when the
// application runs behind a reverse proxy.
// Returns an error if any IP address or CIDR notation is invalid.
func NewFirewall(allowedIPsStr string, trustProxy bool) (*Firewall, error) {
// ... existing implementation
}
// IsAllowed checks if the request originates from an allowed IP address.
// When trustProxy is enabled, it first checks X-Real-IP and X-Forwarded-For headers
// before falling back to RemoteAddr. If no IP restrictions are configured (empty allowlist),
// all requests are allowed. Returns false if the IP cannot be extracted or is not in the allowlist.
func (f *Firewall) IsAllowed(r *http.Request) bool {
// ... existing implementation
}internal/api/rest/middleware.go// IPFilterMiddleware wraps an http.Handler with IP-based access control.
// If the filter is nil, the middleware passes all requests through unchanged.
// When a filter is provided, each request is validated using filter.IsAllowed.
// Denied requests receive a 403 Forbidden response with a JSON error message.
// The middleware logs warnings for denied requests including the remote address and path.
func IPFilterMiddleware(filter IPFilter) func(http.Handler) http.Handler {
// ... existing implementation
}internal/api/rest/server.go// NewServer creates a new REST API server with the specified dependencies.
// The ipFilter parameter controls IP-based access restrictions; pass nil to disable filtering.
// The port parameter specifies the TCP port to listen on (e.g., "8080").
// The server is pre-configured with secure timeout values to mitigate common attacks.
func NewServer(c cache.Cache, s Scraper, g Generator, ipFilter IPFilter, port string) *Server {
// ... existing implementation
}These docstrings follow Go conventions and provide clear explanations of:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/api/rest/server.go (1)
65-73: Middleware ordering: confirm this is the desired logging behaviorThe current chain:
handler := muxhandler = IPFilterMiddleware(s.ipFilter)(handler)handler = Logger(handler)means
LoggerwrapsIPFilterMiddleware, so even denied requests are fully logged by the HTTP logger (plus a warn from the filter). If you’d prefer not to emit the full request/response logging for blocked IPs (keeping only the filter’s warning), you could invert the order:- handler := http.Handler(s.mux) - handler = IPFilterMiddleware(s.ipFilter)(handler) - handler = Logger(handler) + handler := http.Handler(s.mux) + handler = Logger(handler) + handler = IPFilterMiddleware(s.ipFilter)(handler)As implemented, behavior is coherent; just ensure it matches your observability/privacy expectations.
internal/api/rest/ipfilter.go (3)
47-97: IP extraction and proxy‑header handling look correct and defensive
IsAllowedshort‑circuits totruewhenallowedNetsis empty, then usesextractClientIP+isIPAllowedotherwise, which matches the doc comment.- With
trustProxy == true, you first honor a validX-Real-IP, then the first entry inX-Forwarded-For, falling back toRemoteAddronly if headers are missing/invalid. WithtrustProxy == false, onlyRemoteAddris used. This is a sensible pattern and avoids trusting spoofed headers when not configured to.- If no valid IP can be extracted, you fail closed (
IsAllowedreturnsfalse), which is the safer default.If you later want stricter diagnostics, you could differentiate “no IP extracted” vs “IP not in allowlist” in logs, but that’s not required for correctness.
99-114:extractFromRemoteAddris correct; consider handling bare IPs as a nicetyUsing
net.SplitHostPortonremoteAddrand thennet.ParseIPon the host is the right approach for typicalip:portand[ip]:portformats used bynet/http. When parsing fails or IP is invalid, you return a wrapped error andIsAlloweddenies the request, which is a safe failure mode.If you expect environments/tests that sometimes set
RemoteAddrto a bare IP without a port, you might optionally handle that case, for example:func extractFromRemoteAddr(remoteAddr string) (string, error) { - host, _, err := net.SplitHostPort(remoteAddr) - - if err != nil { - return "", fmt.Errorf("invalid remote address: %w", err) - } + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + // Fallback: treat remoteAddr as a bare IP without port. + host = remoteAddr + } ip := net.ParseIP(host) if ip == nil { return "", fmt.Errorf("invalid IP address: %s", host) }Not mandatory, but can reduce surprises if upstream occasionally omits the port.
116-188: Allowlist parsing and membership checks are straightforward and robust
parseAllowedIPscorrectly trims each comma‑separated part, skips empties (tolerating stray commas/whitespace), and aborts on the first invalid entry, returning the underlying error.parseIPOrCIDRappropriately distinguishes CIDR vs single IP, normalizing plain IPs into/32or/128CIDRs, which keeps everything in a single*net.IPNetrepresentation.isIPAllowedsimply parses the candidate IP and linearly scansallowedNetsusingContains, which is perfectly adequate for the expected small config lists.If you ever grow the allowlist significantly, you might consider structuring the data (e.g., grouping v4/v6 or using a trie), but for typical deployment sizes this is more than sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/rest/ipfilter.go(1 hunks)internal/api/rest/middleware.go(2 hunks)internal/api/rest/server.go(3 hunks)internal/api/rest/telegram.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/rest/telegram.go
- internal/api/rest/middleware.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/rest/server.go (2)
internal/api/rest/telegram.go (1)
IPFilter(28-30)internal/api/rest/middleware.go (2)
IPFilterMiddleware(76-105)Logger(12-44)
🔇 Additional comments (4)
internal/api/rest/server.go (2)
16-25: Server struct wiring foripFilteris consistent and minimalAdding
ipFilter IPFilterhere cleanly models the new dependency and keepsServer’s state cohesive with the constructor changes; no issues spotted.
27-45: Constructor docs and dependency wiring look accurateThe expanded
NewServersignature, doc comments, and field initialization (includingipFilter) line up correctly with the described behavior (nil to disable filtering, preconfigured timeouts). The handler being leftnilhere and set later inRunis also consistent with the comment.internal/api/rest/ipfilter.go (2)
10-18: Firewall state design is simple and concurrency‑safeStoring
allowedNets []*net.IPNetplus atrustProxyflag is minimal and read‑only after construction, so it’s safe for concurrent use from HTTP handlers. The docstring correctly explains the responsibilities and proxy‑trust behavior.
19-45: The "empty means allow all" behavior is explicitly documented as intendedWhen
allowedIPsStris empty,allowedNetsis set tonil, andIsAllowedtreatslen(f.allowedNets) == 0as "no restrictions → allow all requests." This behavior is already documented in theNewFirewalldocstring ("If allowedIPsStr is empty, all IP addresses are allowed by default") and theIsAlloweddocstring ("If no IP restrictions are configured (empty allowlist), all requests are allowed").The same behavior applies to whitespace-only strings:
parseAllowedIPstrims whitespace and skips empty parts, resulting in an empty slice thatIsAllowedtreats as unrestricted access.This is not a hidden assumption—it is the explicitly stated design. If the intended security posture differs from this, the documentation and default behavior should be aligned accordingly.
Addresses #5
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.