From b46b5dfb2aa3a1f5320a7a3420b9186bb8fc655f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= <f@miniflux.net>
Date: Sat, 11 Mar 2023 20:36:52 -0800
Subject: [PATCH] Use r.RemoteAddr to check /metrics endpoint network access

HTTP headers like X-Forwarded-For or X-Real-Ip can be easily spoofed. As
such, it cannot be used to test if the client IP is allowed.

The recommendation is to use HTTP Basic authentication to protect the
metrics endpoint, or run Miniflux behind a trusted reverse-proxy.
---
 http/request/client_ip.go | 23 ++++++++++++++---------
 service/httpd/httpd.go    |  9 +++++----
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/http/request/client_ip.go b/http/request/client_ip.go
index 22086081..83d3a577 100644
--- a/http/request/client_ip.go
+++ b/http/request/client_ip.go
@@ -10,15 +10,7 @@ import (
 	"strings"
 )
 
-func dropIPv6zone(address string) string {
-	i := strings.IndexByte(address, '%')
-	if i != -1 {
-		address = address[:i]
-	}
-	return address
-}
-
-// FindClientIP returns client real IP address.
+// FindClientIP returns the client real IP address based on trusted Reverse-Proxy HTTP headers.
 func FindClientIP(r *http.Request) string {
 	headers := []string{"X-Forwarded-For", "X-Real-Ip"}
 	for _, header := range headers {
@@ -36,6 +28,11 @@ func FindClientIP(r *http.Request) string {
 	}
 
 	// Fallback to TCP/IP source IP address.
+	return FindRemoteIP(r)
+}
+
+// FindRemoteIP returns remote client IP address.
+func FindRemoteIP(r *http.Request) string {
 	remoteIP, _, err := net.SplitHostPort(r.RemoteAddr)
 	if err != nil {
 		remoteIP = r.RemoteAddr
@@ -49,3 +46,11 @@ func FindClientIP(r *http.Request) string {
 
 	return remoteIP
 }
+
+func dropIPv6zone(address string) string {
+	i := strings.IndexByte(address, '%')
+	if i != -1 {
+		address = address[:i]
+	}
+	return address
+}
diff --git a/service/httpd/httpd.go b/service/httpd/httpd.go
index db3c26b4..eac45b46 100644
--- a/service/httpd/httpd.go
+++ b/service/httpd/httpd.go
@@ -208,7 +208,7 @@ func setupHandler(store *storage.Storage, pool *worker.Pool) *mux.Router {
 
 				// Returns a 404 if the client is not authorized to access the metrics endpoint.
 				if route.GetName() == "metrics" && !isAllowedToAccessMetricsEndpoint(r) {
-					logger.Error(`[Metrics] Client not allowed: %s`, request.ClientIP(r))
+					logger.Error(`[Metrics] [ClientIP=%s] Client not allowed (%s)`, request.ClientIP(r), r.RemoteAddr)
 					http.NotFound(w, r)
 					return
 				}
@@ -222,9 +222,8 @@ func setupHandler(store *storage.Storage, pool *worker.Pool) *mux.Router {
 }
 
 func isAllowedToAccessMetricsEndpoint(r *http.Request) bool {
-	clientIP := request.ClientIP(r)
-
 	if config.Opts.MetricsUsername() != "" && config.Opts.MetricsPassword() != "" {
+		clientIP := request.ClientIP(r)
 		username, password, authOK := r.BasicAuth()
 		if !authOK {
 			logger.Info("[Metrics] [ClientIP=%s] No authentication header sent", clientIP)
@@ -248,7 +247,9 @@ func isAllowedToAccessMetricsEndpoint(r *http.Request) bool {
 			logger.Fatal(`[Metrics] Unable to parse CIDR %v`, err)
 		}
 
-		if network.Contains(net.ParseIP(clientIP)) {
+		// We use r.RemoteAddr in this case because HTTP headers like X-Forwarded-For can be easily spoofed.
+		// The recommendation is to use HTTP Basic authentication.
+		if network.Contains(net.ParseIP(request.FindRemoteIP(r))) {
 			return true
 		}
 	}