Trust the proxy's last X-Forwarded-For hop, and decide trust once

docs/install.md

@@ -105,7 +105,8 @@ Otherwise every change made in the UI is refused with "it didn't come from this
 Traefik send both by default; with nginx, add `proxy_set_header X-Forwarded-Host $http_host;`
 (`$http_host` keeps the port, which the check needs) and
 `proxy_set_header X-Forwarded-Proto $scheme;`. The audit log records the client's address from
-`X-Forwarded-For`. YardMaster believes these headers only from the trusted proxy: a request carrying
+the last value of `X-Forwarded-For`, the one your proxy appends (with nginx,
+`proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;`). YardMaster believes these headers only from the trusted proxy: a request carrying
 the shared secret, or one from an address in `YARDMASTER_PROXY_ADDRESSES`.
 
 ## Configuration

internal/httpapi/httpapi_test.go

@@ -292,7 +292,8 @@ func TestForwardedHeadersTrustedLikeIdentity(t *testing.T) {
 	req := func(remote string, headers ...string) *http.Request {
 		r := httptest.NewRequest("GET", "http://ym.test/api/session", nil)
 		r.RemoteAddr = remote
-		r.Header.Set("X-Forwarded-For", "192.0.2.7, 10.9.0.5")
+		// The client sent a made-up address; the proxy appended the one it saw.
+		r.Header.Set("X-Forwarded-For", "198.51.100.66, 192.0.2.7")
 		r.Header.Set("X-Forwarded-Proto", "https")
 		for i := 0; i+1 < len(headers); i += 2 {
 			r.Header.Set(headers[i], headers[i+1])
@@ -320,3 +321,26 @@ func TestForwardedHeadersTrustedLikeIdentity(t *testing.T) {
 		t.Error("untrusted: forwarded https believed")
 	}
 }
+
+func TestHTTPSStateIsReadOnceAndSaved(t *testing.T) {
+	s, _ := newServer(t, settings.Settings{})
+	ctx := t.Context()
+	s.DB.SetSetting(ctx, settingHTTPS, httpsState{Name: "ym.office.lan"})
+	if st := s.httpsState(ctx); st.Name != "ym.office.lan" {
+		t.Fatalf("not loaded from the database: %+v", st)
+	}
+	// Later reads come from memory, not the database.
+	s.DB.SetSetting(ctx, settingHTTPS, httpsState{Name: "elsewhere.lan"})
+	if st := s.httpsState(ctx); st.Name != "ym.office.lan" {
+		t.Errorf("read again from the database: %+v", st)
+	}
+	// Changes are kept in memory and saved.
+	if err := s.setHTTPSState(ctx, httpsState{Name: "ym.office.lan", Confirmed: true}); err != nil {
+		t.Fatal(err)
+	}
+	var saved httpsState
+	s.DB.GetSetting(ctx, settingHTTPS, &saved)
+	if st := s.httpsState(ctx); !st.Confirmed || !saved.Confirmed {
+		t.Errorf("in memory %+v, saved %+v", st, saved)
+	}
+}

internal/httpapi/https.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	_ "embed"
 	"html/template"
+	"log/slog"
 	"net/http"
 	"strconv"
 
@@ -23,10 +24,31 @@ type httpsState struct {
 
 const settingHTTPS = "https"
 
+// httpsState returns the stored HTTPS state. It's read from the database once and then
+// kept in memory: port 8080 asks for it on every request, and only setHTTPSState changes it.
 func (s *Server) httpsState(ctx context.Context) httpsState {
-	var st httpsState
-	s.DB.GetSetting(ctx, settingHTTPS, &st)
-	return st
+	s.httpsMu.Lock()
+	defer s.httpsMu.Unlock()
+	if !s.httpsLoaded {
+		var st httpsState
+		if _, err := s.DB.GetSetting(ctx, settingHTTPS, &st); err != nil {
+			slog.Error("reading the HTTPS state", "err", err)
+			return httpsState{}
+		}
+		s.https, s.httpsLoaded = st, true
+	}
+	return s.https
+}
+
+// setHTTPSState stores the HTTPS state.
+func (s *Server) setHTTPSState(ctx context.Context, st httpsState) error {
+	s.httpsMu.Lock()
+	defer s.httpsMu.Unlock()
+	if err := s.DB.SetSetting(ctx, settingHTTPS, st); err != nil {
+		return err
+	}
+	s.https, s.httpsLoaded = st, true
+	return nil
 }
 
 // httpsActive reports whether YardMaster serves HTTPS on its HTTPS port, and the name
@@ -62,7 +84,7 @@ func (s *Server) SeedHTTPSName(ctx context.Context) {
 		return
 	}
 	if _, err := s.TLS.SetName(s.Settings.PublicHost); err == nil {
-		s.DB.SetSetting(ctx, settingHTTPS, httpsState{Name: s.Settings.PublicHost})
+		s.setHTTPSState(ctx, httpsState{Name: s.Settings.PublicHost})
 	}
 }
 
@@ -111,7 +133,7 @@ func (s *Server) handleHTTPSName(w http.ResponseWriter, r *http.Request, u *auth
 	if st.Name != in.Name {
 		st = httpsState{Name: in.Name}
 	}
-	if err := s.DB.SetSetting(r.Context(), settingHTTPS, st); err != nil {
+	if err := s.setHTTPSState(r.Context(), st); err != nil {
 		logErr(w, err)
 		return
 	}
@@ -143,7 +165,7 @@ func (s *Server) handleHTTPSConfirm(w http.ResponseWriter, r *http.Request, u *a
 		return
 	}
 	st.Confirmed = true
-	if err := s.DB.SetSetting(r.Context(), settingHTTPS, st); err != nil {
+	if err := s.setHTTPSState(r.Context(), st); err != nil {
 		logErr(w, err)
 		return
 	}

internal/httpapi/server.go

@@ -4,6 +4,7 @@
 package httpapi
 
 import (
+	"context"
 	"crypto/rand"
 	"embed"
 	"encoding/hex"
@@ -62,6 +63,10 @@ type Server struct {
 
 	setupMu   sync.Mutex
 	setupCode string // first-run code printed to the log; "" once used
+
+	httpsMu     sync.Mutex
+	https       httpsState
+	httpsLoaded bool
 }
 
 func New(d Deps) *Server {
@@ -150,7 +155,7 @@ func (s *Server) Handler() http.Handler {
 	mux.Handle("/v1/", s.gateway)
 	mux.HandleFunc("/", s.handleStatic)
 
-	return s.securityHeaders(s.originGuard(mux))
+	return s.securityHeaders(s.withForwarded(s.originGuard(mux)))
 }
 
 // PlainHandler serves port 8080. Until HTTPS is active it is the full application. Once
@@ -292,24 +297,51 @@ func (s *Server) isSiteHost(r *http.Request, host string) bool {
 	return fwd != "" && strings.EqualFold(host, fwd)
 }
 
-// fromTrustedProxy reports whether r came through the trusted proxy, by its shared secret
-// or a configured address: the same trust that lets it name the user, so its
-// X-Forwarded-* headers are believed too.
-func (s *Server) fromTrustedProxy(r *http.Request) bool {
-	return s.Settings.Auth == settings.AuthProxy && auth.ProxyTrusted(r, s.Settings.Proxy, remoteAddr(r), true)
+// forwarded is what the trusted proxy reports about a request: the host and scheme the
+// browser used and the client's address. It's empty unless the request came through the
+// proxy, by its shared secret or a configured address: the same trust that lets it name
+// the user.
+type forwarded struct {
+	host, proto, client string
+}
+
+type forwardedKey struct{}
+
+// withForwarded decides once per request whether to believe its X-Forwarded-* headers.
+func (s *Server) withForwarded(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := context.WithValue(r.Context(), forwardedKey{}, s.readForwarded(r))
+		next.ServeHTTP(w, r.WithContext(ctx))
+	})
+}
+
+// forwardedFor returns what withForwarded decided for r.
+func (s *Server) forwardedFor(r *http.Request) forwarded {
+	if f, ok := r.Context().Value(forwardedKey{}).(forwarded); ok {
+		return f
+	}
+	return s.readForwarded(r)
 }
 
-// forwarded is the first value of an X-Forwarded-* header from the trusted proxy; empty
-// otherwise.
-func (s *Server) forwarded(r *http.Request, header string) string {
-	if !s.fromTrustedProxy(r) {
-		return ""
+func (s *Server) readForwarded(r *http.Request) forwarded {
+	if s.Settings.Auth != settings.AuthProxy || !auth.ProxyTrusted(r, s.Settings.Proxy, remoteAddr(r), true) {
+		return forwarded{}
+	}
+	// The proxy reports the browser's host and scheme, so their first value is its own. It
+	// appends the address it saw to whatever X-Forwarded-For the client sent, so only the
+	// last value is.
+	host := strings.Split(r.Header.Get("X-Forwarded-Host"), ",")
+	proto := strings.Split(r.Header.Get("X-Forwarded-Proto"), ",")
+	client := strings.Split(r.Header.Get("X-Forwarded-For"), ",")
+	return forwarded{
+		host:   strings.TrimSpace(host[0]),
+		proto:  strings.TrimSpace(proto[0]),
+		client: strings.TrimSpace(client[len(client)-1]),
 	}
-	return strings.TrimSpace(strings.Split(r.Header.Get(header), ",")[0])
 }
 
 // forwardedHost is the host the browser used, as the trusted proxy reports it.
-func (s *Server) forwardedHost(r *http.Request) string { return s.forwarded(r, "X-Forwarded-Host") }
+func (s *Server) forwardedHost(r *http.Request) string { return s.forwardedFor(r).host }
 
 // ---- helpers ----
 
@@ -371,8 +403,8 @@ func remoteAddr(r *http.Request) netip.Addr {
 // clientIP is the address recorded in the audit log. Behind another proxy it's the
 // address the proxy reports.
 func (s *Server) clientIP(r *http.Request) string {
-	if f := s.forwarded(r, "X-Forwarded-For"); f != "" {
-		return f
+	if c := s.forwardedFor(r).client; c != "" {
+		return c
 	}
 	return remoteAddr(r).String()
 }
@@ -403,5 +435,5 @@ func (s *Server) isHTTPS(r *http.Request) bool {
 	if r.TLS != nil {
 		return true
 	}
-	return strings.EqualFold(s.forwarded(r, "X-Forwarded-Proto"), "https")
+	return strings.EqualFold(s.forwardedFor(r).proto, "https")
 }