fix: loginLimiter.allow returns true (no rate limit) for empty IP #31

New issue

Closed

opened 2026-05-17 17:18:04 +02:00 by dries · 0 comments

dries commented

2026-05-17 17:18:04 +02:00

Owner

Problem

internal/server/auth.go:328-331:

func (l *loginLimiter) allow(ip string) bool {
    if ip == "" {
        return true   // ← no rate limit for empty IP
    }

clientIP() falls back to the raw r.RemoteAddr string when net.SplitHostPort fails, so reaching ip == "" requires r.RemoteAddr itself to be empty — which the standard net/http server never produces. However:

The GUI mode starts the server via gui.RunGUI, which may use a different listener type in the future.
A Unix-domain socket listener would produce r.RemoteAddr = "".
If a reverse proxy is ever introduced (e.g. an nginx in front for TLS termination), a misconfigured X-Forwarded-For stripping could arrive here as empty.

Returning true on empty IP silently bypasses the rate limiter entirely, turning every empty-IP source into an unlimited login attempt endpoint.

Suggested fix

Return false (deny) on empty IP, or use a synthetic sentinel bucket key so the rate limit still applies:

if ip == "" {
    ip = "<unknown>"   // rate-limit unknown-origin clients as a single bucket
}

This is conservative: a legitimate browser will always have a RemoteAddr, so an empty IP is almost certainly an infrastructure anomaly that shouldn't get a free pass.

References

internal/server/auth.go:328-331
internal/server/auth.go:357-367 (clientIP() — fallback path)

## Problem `internal/server/auth.go:328-331`: ```go func (l *loginLimiter) allow(ip string) bool { if ip == "" { return true // ← no rate limit for empty IP } ``` `clientIP()` falls back to the raw `r.RemoteAddr` string when `net.SplitHostPort` fails, so reaching `ip == ""` requires `r.RemoteAddr` itself to be empty — which the standard `net/http` server never produces. However: 1. The GUI mode starts the server via `gui.RunGUI`, which may use a different listener type in the future. 2. A Unix-domain socket listener would produce `r.RemoteAddr = ""`. 3. If a reverse proxy is ever introduced (e.g. an nginx in front for TLS termination), a misconfigured `X-Forwarded-For` stripping could arrive here as empty. Returning `true` on empty IP silently bypasses the rate limiter entirely, turning every empty-IP source into an unlimited login attempt endpoint. ## Suggested fix Return `false` (deny) on empty IP, or use a synthetic sentinel bucket key so the rate limit still applies: ```go if ip == "" { ip = "<unknown>" // rate-limit unknown-origin clients as a single bucket } ``` This is conservative: a legitimate browser will always have a RemoteAddr, so an empty IP is almost certainly an infrastructure anomaly that shouldn't get a free pass. ## References - `internal/server/auth.go:328-331` - `internal/server/auth.go:357-367` (`clientIP()` — fallback path)