From 719dc62b0d455f46292d038d2bfbfe1b15e8c828 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 6 Oct 2026 08:30:46 -0400 Subject: [PATCH] fix(auth): set the session cookie's Secure flag behind a TLS-terminating proxy (M462 #4977) The cookie set Secure from r.TLS, which is nil whenever TLS terminates at Traefik or Cloudflare, so a public deployment's session cookie went out without Secure. auth.IsHTTPS now decides it from X-Forwarded-Proto, believed only through the trusted-hop count (rule 94) and read positionally like X-Forwarded-For. Plain-HTTP and LAN logins are unchanged; nothing redirects. Co-Authored-By: Claude Opus 5.5 --- internal/api/auth.go | 2 +- internal/api/auth_register.go | 2 +- internal/auth/clientip.go | 30 ++++++++++++++++++++++++ internal/auth/clientip_test.go | 43 ++++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/internal/api/auth.go b/internal/api/auth.go index 6624ed39..a36dea87 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -140,7 +140,7 @@ func (h *handlers) handleLogin(w http.ResponseWriter, r *http.Request) { Value: token, Path: "/", HttpOnly: true, - Secure: r.TLS != nil, // dev over http stays functional; prod over https gets Secure + Secure: auth.IsHTTPS(r, h.netSettings.Hops()), // plain HTTP and LAN stay functional (rule 94) SameSite: http.SameSiteStrictMode, MaxAge: int(sessionCookieMaxAge.Seconds()), }) diff --git a/internal/api/auth_register.go b/internal/api/auth_register.go index 710f7602..4ea5cddd 100644 --- a/internal/api/auth_register.go +++ b/internal/api/auth_register.go @@ -195,7 +195,7 @@ func (h *handlers) handleRegister(w http.ResponseWriter, r *http.Request) { Value: sessionToken, Path: "/", HttpOnly: true, - Secure: r.TLS != nil, + Secure: auth.IsHTTPS(r, h.netSettings.Hops()), // plain HTTP and LAN stay functional (rule 94) SameSite: http.SameSiteStrictMode, MaxAge: int(sessionCookieMaxAge.Seconds()), }) diff --git a/internal/auth/clientip.go b/internal/auth/clientip.go index 2be913cb..3b741dc4 100644 --- a/internal/auth/clientip.go +++ b/internal/auth/clientip.go @@ -65,6 +65,36 @@ func ClientIP(r *http.Request, trustedProxyHops int) string { return remote } +// IsHTTPS reports whether the client reached us over HTTPS, reading through +// trustedProxyHops reverse proxies by the same rule as ClientIP. +// +// TLS terminates at the operator's proxy (rule 94), so r.TLS is nil in every +// real deployment and only X-Forwarded-Proto can say what the client used. +// That header is believed only when hops >= 1: with no trusted proxy it is +// just something any client can send. Proxies that append rather than +// overwrite produce a comma list, read positionally like X-Forwarded-For. +// +// This decides only HTTPS-only behaviour (the cookie Secure flag, HSTS). It +// must never drive a redirect: plain-HTTP and LAN use keep working. +func IsHTTPS(r *http.Request, trustedProxyHops int) bool { + if r.TLS != nil { + return true + } + if trustedProxyHops <= 0 { + return false + } + raw := r.Header.Get("X-Forwarded-Proto") + if strings.TrimSpace(raw) == "" { + return false + } + parts := strings.Split(raw, ",") + idx := len(parts) - trustedProxyHops + if idx < 0 { + idx = 0 + } + return strings.EqualFold(strings.TrimSpace(parts[idx]), "https") +} + // forwardedChain returns the X-Forwarded-For entries in wire order, or the // single X-Real-IP value when XFF is absent. // diff --git a/internal/auth/clientip_test.go b/internal/auth/clientip_test.go index fba50fb9..50a330d5 100644 --- a/internal/auth/clientip_test.go +++ b/internal/auth/clientip_test.go @@ -1,6 +1,7 @@ package auth import ( + "crypto/tls" "net/http" "testing" ) @@ -168,3 +169,45 @@ func TestClientIP(t *testing.T) { }) } } + +func TestIsHTTPS(t *testing.T) { + tests := []struct { + name string + hops int + proto string + tls bool + want bool + }{ + // The three cases rule 94 names. + {name: "hops 0 ignores a forwarded https", hops: 0, proto: "https", want: false}, + {name: "hops 1 believes a forwarded https", hops: 1, proto: "https", want: true}, + {name: "plain request is not https", hops: 1, want: false}, + + {name: "forwarded http is not https", hops: 1, proto: "http", want: false}, + {name: "case-insensitive", hops: 1, proto: "HTTPS", want: true}, + {name: "direct TLS is https whatever the hops", hops: 0, tls: true, want: true}, + // An appending chain is read positionally: with one trusted hop the + // last entry is our proxy's word, and a client-supplied first entry + // cannot flip it. + {name: "appended chain reads our proxy's entry", hops: 1, proto: "https, http", want: false}, + {name: "two hops read the CDN's entry", hops: 2, proto: "https, http", want: true}, + {name: "over-counted hops clamp to the first entry", hops: 5, proto: "https", want: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r, err := http.NewRequest(http.MethodGet, "/api/auth/login", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + if tc.proto != "" { + r.Header.Set("X-Forwarded-Proto", tc.proto) + } + if tc.tls { + r.TLS = &tls.ConnectionState{} + } + if got := IsHTTPS(r, tc.hops); got != tc.want { + t.Errorf("IsHTTPS(hops=%d, proto=%q) = %v, want %v", tc.hops, tc.proto, got, tc.want) + } + }) + } +}