Merge branch 'main' into fix/server-side-app-name-validation

internal/handlers/app.go

@@ -32,11 +32,7 @@ func (h *Handlers) HandleAppNew() http.HandlerFunc {
 	return func(writer http.ResponseWriter, request *http.Request) {
 		data := h.addGlobals(map[string]any{}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "app_new.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "app_new.html", data)
 	}
 }
 
@@ -66,7 +62,7 @@ func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // valid
 
 		if name == "" || repoURL == "" {
 			data["Error"] = "Name and repository URL are required"
-			_ = tmpl.ExecuteTemplate(writer, "app_new.html", data)
+			h.renderTemplate(writer, tmpl, "app_new.html", data)
 
 			return
 		}
@@ -99,7 +95,7 @@ func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // valid
 		if createErr != nil {
 			h.log.Error("failed to create app", "error", createErr)
 			data["Error"] = "Failed to create app: " + createErr.Error()
-			_ = tmpl.ExecuteTemplate(writer, "app_new.html", data)
+			h.renderTemplate(writer, tmpl, "app_new.html", data)
 
 			return
 		}
@@ -160,11 +156,7 @@ func (h *Handlers) HandleAppDetail() http.HandlerFunc {
 			"Success":          request.URL.Query().Get("success"),
 		}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "app_detail.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "app_detail.html", data)
 	}
 }
 
@@ -193,11 +185,7 @@ func (h *Handlers) HandleAppEdit() http.HandlerFunc {
 			"App": application,
 		}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "app_edit.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "app_edit.html", data)
 	}
 }
 
@@ -266,7 +254,7 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid
 				"App":   application,
 				"Error": "Failed to update app",
 			}, request)
-			_ = tmpl.ExecuteTemplate(writer, "app_edit.html", data)
+			h.renderTemplate(writer, tmpl, "app_edit.html", data)
 
 			return
 		}
@@ -390,11 +378,7 @@ func (h *Handlers) HandleAppDeployments() http.HandlerFunc {
 			"Deployments": deployments,
 		}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "deployments.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "deployments.html", data)
 	}
 }
 

internal/handlers/auth.go

@@ -13,11 +13,7 @@ func (h *Handlers) HandleLoginGET() http.HandlerFunc {
 	return func(writer http.ResponseWriter, request *http.Request) {
 		data := h.addGlobals(map[string]any{}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "login.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "login.html", data)
 	}
 }
 
@@ -42,7 +38,7 @@ func (h *Handlers) HandleLoginPOST() http.HandlerFunc {
 
 		if username == "" || password == "" {
 			data["Error"] = "Username and password are required"
-			_ = tmpl.ExecuteTemplate(writer, "login.html", data)
+			h.renderTemplate(writer, tmpl, "login.html", data)
 
 			return
 		}
@@ -50,7 +46,7 @@ func (h *Handlers) HandleLoginPOST() http.HandlerFunc {
 		user, authErr := h.auth.Authenticate(request.Context(), username, password)
 		if authErr != nil {
 			data["Error"] = "Invalid username or password"
-			_ = tmpl.ExecuteTemplate(writer, "login.html", data)
+			h.renderTemplate(writer, tmpl, "login.html", data)
 
 			return
 		}
@@ -60,7 +56,7 @@ func (h *Handlers) HandleLoginPOST() http.HandlerFunc {
 			h.log.Error("failed to create session", "error", sessionErr)
 
 			data["Error"] = "Failed to create session"
-			_ = tmpl.ExecuteTemplate(writer, "login.html", data)
+			h.renderTemplate(writer, tmpl, "login.html", data)
 
 			return
 		}

internal/handlers/dashboard.go

@@ -69,10 +69,6 @@ func (h *Handlers) HandleDashboard() http.HandlerFunc {
 			"AppStats": appStats,
 		}, request)
 
-		execErr := tmpl.ExecuteTemplate(writer, "dashboard.html", data)
-		if execErr != nil {
-			h.log.Error("template execution failed", "error", execErr)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "dashboard.html", data)
 	}
 }

internal/handlers/handlers.go

@@ -2,6 +2,7 @@
 package handlers
 
 import (
+	"bytes"
 	"encoding/json"
 	"log/slog"
 	"net/http"
@@ -18,6 +19,7 @@ import (
 	"git.eeqj.de/sneak/upaas/internal/service/auth"
 	"git.eeqj.de/sneak/upaas/internal/service/deploy"
 	"git.eeqj.de/sneak/upaas/internal/service/webhook"
+	"git.eeqj.de/sneak/upaas/templates"
 )
 
 // Params contains dependencies for Handlers.
@@ -80,6 +82,28 @@ func (h *Handlers) addGlobals(
 	return data
 }
 
+// renderTemplate executes the named template into a buffer first, then writes
+// to the ResponseWriter only on success. This prevents partial/corrupt HTML
+// responses when template execution fails partway through.
+func (h *Handlers) renderTemplate(
+	writer http.ResponseWriter,
+	tmpl *templates.TemplateExecutor,
+	name string,
+	data any,
+) {
+	var buf bytes.Buffer
+
+	err := tmpl.ExecuteTemplate(&buf, name, data)
+	if err != nil {
+		h.log.Error("template execution failed", "error", err)
+		http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
+
+		return
+	}
+
+	_, _ = buf.WriteTo(writer)
+}
+
 func (h *Handlers) respondJSON(
 	writer http.ResponseWriter,
 	_ *http.Request,

internal/handlers/render_template_test.go

@@ -0,0 +1,73 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestRenderTemplateBuffersOutput verifies that successful template rendering
+// produces a complete HTML response (not partial/corrupt).
+func TestRenderTemplateBuffersOutput(t *testing.T) {
+	t.Parallel()
+
+	testCtx := setupTestHandlers(t)
+
+	// The setup page is simple and has no DB dependencies
+	request := httptest.NewRequest(http.MethodGet, "/setup", nil)
+	recorder := httptest.NewRecorder()
+
+	handler := testCtx.handlers.HandleSetupGET()
+	handler.ServeHTTP(recorder, request)
+
+	assert.Equal(t, http.StatusOK, recorder.Code)
+
+	body := recorder.Body.String()
+	// A properly buffered response should contain the closing </html> tag,
+	// proving the full template was rendered before being sent.
+	assert.Contains(t, body, "</html>")
+	// Should NOT contain the error text that would be appended on failure
+	assert.NotContains(t, body, "Internal Server Error")
+}
+
+// TestDashboardRenderTemplateBuffersOutput verifies the dashboard handler
+// also uses buffered template rendering.
+func TestDashboardRenderTemplateBuffersOutput(t *testing.T) {
+	t.Parallel()
+
+	testCtx := setupTestHandlers(t)
+
+	request := httptest.NewRequest(http.MethodGet, "/", nil)
+	recorder := httptest.NewRecorder()
+
+	handler := testCtx.handlers.HandleDashboard()
+	handler.ServeHTTP(recorder, request)
+
+	assert.Equal(t, http.StatusOK, recorder.Code)
+
+	body := recorder.Body.String()
+	assert.Contains(t, body, "</html>")
+	assert.NotContains(t, body, "Internal Server Error")
+}
+
+// TestLoginRenderTemplateBuffersOutput verifies the login handler
+// uses buffered template rendering.
+func TestLoginRenderTemplateBuffersOutput(t *testing.T) {
+	t.Parallel()
+
+	testCtx := setupTestHandlers(t)
+
+	request := httptest.NewRequest(http.MethodGet, "/login", nil)
+	recorder := httptest.NewRecorder()
+
+	handler := testCtx.handlers.HandleLoginGET()
+	handler.ServeHTTP(recorder, request)
+
+	assert.Equal(t, http.StatusOK, recorder.Code)
+
+	body := recorder.Body.String()
+	assert.Contains(t, body, "</html>")
+	assert.NotContains(t, body, "Internal Server Error")
+}

internal/handlers/setup.go

@@ -18,11 +18,7 @@ func (h *Handlers) HandleSetupGET() http.HandlerFunc {
 	return func(writer http.ResponseWriter, request *http.Request) {
 		data := h.addGlobals(map[string]any{}, request)
 
-		err := tmpl.ExecuteTemplate(writer, "setup.html", data)
-		if err != nil {
-			h.log.Error("template execution failed", "error", err)
-			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
-		}
+		h.renderTemplate(writer, tmpl, "setup.html", data)
 	}
 }
 
@@ -62,7 +58,7 @@ func (h *Handlers) renderSetupError(
 		"Username": username,
 		"Error":    errorMsg,
 	}, request)
-	_ = tmpl.ExecuteTemplate(writer, "setup.html", data)
+	h.renderTemplate(writer, tmpl, "setup.html", data)
 }
 
 // HandleSetupPOST handles the setup form submission.

internal/middleware/middleware.go

@@ -111,10 +111,53 @@ func ipFromHostPort(hostPort string) string {
 	return host
 }
 
-// realIP extracts the client's real IP address from the request,
-// checking proxy headers first (trusted reverse proxy like Traefik),
-// then falling back to RemoteAddr.
+// trustedProxyNets are RFC1918 and loopback CIDRs whose proxy headers we trust.
+//
+//nolint:gochecknoglobals // package-level constant nets parsed once
+var trustedProxyNets = func() []*net.IPNet {
+	cidrs := []string{
+		"10.0.0.0/8",
+		"172.16.0.0/12",
+		"192.168.0.0/16",
+		"127.0.0.0/8",
+		"::1/128",
+		"fc00::/7",
+	}
+
+	nets := make([]*net.IPNet, 0, len(cidrs))
+
+	for _, cidr := range cidrs {
+		_, n, _ := net.ParseCIDR(cidr)
+		nets = append(nets, n)
+	}
+
+	return nets
+}()
+
+// isTrustedProxy reports whether ip is in an RFC1918, loopback, or ULA range.
+func isTrustedProxy(ip net.IP) bool {
+	for _, n := range trustedProxyNets {
+		if n.Contains(ip) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// realIP extracts the client's real IP address from the request.
+// Proxy headers (X-Real-IP, X-Forwarded-For) are only trusted when the
+// direct connection originates from an RFC1918/loopback address.
+// Otherwise, headers are ignored and RemoteAddr is used (fail closed).
 func realIP(r *http.Request) string {
+	addr := ipFromHostPort(r.RemoteAddr)
+	remoteIP := net.ParseIP(addr)
+
+	// Only trust proxy headers from private/loopback sources.
+	if remoteIP == nil || !isTrustedProxy(remoteIP) {
+		return addr
+	}
+
 	// 1. X-Real-IP (set by Traefik/nginx)
 	if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" {
 		return ip
@@ -130,7 +173,7 @@ func realIP(r *http.Request) string {
 	}
 
 	// 3. Fall back to RemoteAddr
-	return ipFromHostPort(r.RemoteAddr)
+	return addr
 }
 
 // CORS returns CORS middleware.

internal/middleware/realip_test.go

@@ -2,6 +2,7 @@ package middleware //nolint:testpackage // tests unexported realIP function
 
 import (
 	"context"
+	"net"
 	"net/http"
 	"testing"
 )
@@ -16,54 +17,98 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test
 		xff        string
 		want       string
 	}{
+		// === Trusted proxy (RFC1918 / loopback) — headers ARE honoured ===
 		{
-			name:       "X-Real-IP takes priority",
+			name:       "trusted: X-Real-IP from 10.x",
 			remoteAddr: "10.0.0.1:1234",
 			xRealIP:    "203.0.113.5",
 			xff:        "198.51.100.1, 10.0.0.1",
 			want:       "203.0.113.5",
 		},
 		{
-			name:       "X-Forwarded-For used when no X-Real-IP",
+			name:       "trusted: XFF from 10.x when no X-Real-IP",
 			remoteAddr: "10.0.0.1:1234",
 			xff:        "198.51.100.1, 10.0.0.1",
 			want:       "198.51.100.1",
 		},
 		{
-			name:       "X-Forwarded-For single IP",
+			name:       "trusted: XFF single IP from 10.x",
 			remoteAddr: "10.0.0.1:1234",
 			xff:        "203.0.113.10",
 			want:       "203.0.113.10",
 		},
 		{
-			name:       "falls back to RemoteAddr",
+			name:       "trusted: falls back to RemoteAddr (192.168.x)",
 			remoteAddr: "192.168.1.1:5678",
 			want:       "192.168.1.1",
 		},
 		{
-			name:       "RemoteAddr without port",
+			name:       "trusted: RemoteAddr without port",
 			remoteAddr: "192.168.1.1",
 			want:       "192.168.1.1",
 		},
 		{
-			name:       "X-Real-IP with whitespace",
+			name:       "trusted: X-Real-IP with whitespace from 10.x",
 			remoteAddr: "10.0.0.1:1234",
 			xRealIP:    "  203.0.113.5  ",
 			want:       "203.0.113.5",
 		},
 		{
-			name:       "X-Forwarded-For with whitespace",
+			name:       "trusted: XFF with whitespace from 10.x",
 			remoteAddr: "10.0.0.1:1234",
 			xff:        "  198.51.100.1 , 10.0.0.1",
 			want:       "198.51.100.1",
 		},
 		{
-			name:       "empty X-Real-IP falls through to XFF",
+			name:       "trusted: empty X-Real-IP falls through to XFF from 10.x",
 			remoteAddr: "10.0.0.1:1234",
 			xRealIP:    "  ",
 			xff:        "198.51.100.1",
 			want:       "198.51.100.1",
 		},
+		{
+			name:       "trusted: loopback honours X-Real-IP",
+			remoteAddr: "127.0.0.1:9999",
+			xRealIP:    "93.184.216.34",
+			want:       "93.184.216.34",
+		},
+		{
+			name:       "trusted: 172.16.x honours XFF",
+			remoteAddr: "172.16.0.1:4321",
+			xff:        "8.8.8.8",
+			want:       "8.8.8.8",
+		},
+
+		// === Untrusted proxy (public IP) — headers IGNORED, use RemoteAddr ===
+		{
+			name:       "untrusted: X-Real-IP ignored from public IP",
+			remoteAddr: "203.0.113.50:1234",
+			xRealIP:    "10.0.0.1",
+			want:       "203.0.113.50",
+		},
+		{
+			name:       "untrusted: XFF ignored from public IP",
+			remoteAddr: "198.51.100.99:5678",
+			xff:        "10.0.0.1, 192.168.1.1",
+			want:       "198.51.100.99",
+		},
+		{
+			name:       "untrusted: both headers ignored from public IP",
+			remoteAddr: "8.8.8.8:443",
+			xRealIP:    "1.2.3.4",
+			xff:        "5.6.7.8",
+			want:       "8.8.8.8",
+		},
+		{
+			name:       "untrusted: no headers, public RemoteAddr",
+			remoteAddr: "93.184.216.34:8080",
+			want:       "93.184.216.34",
+		},
+		{
+			name:       "untrusted: public RemoteAddr without port",
+			remoteAddr: "93.184.216.34",
+			want:       "93.184.216.34",
+		},
 	}
 
 	for _, tt := range tests {
@@ -88,3 +133,25 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test
 		})
 	}
 }
+
+func TestIsTrustedProxy(t *testing.T) {
+	t.Parallel()
+
+	trusted := []string{"10.0.0.1", "10.255.255.255", "172.16.0.1", "172.31.255.255",
+		"192.168.0.1", "192.168.255.255", "127.0.0.1", "127.255.255.255", "::1"}
+	untrusted := []string{"8.8.8.8", "203.0.113.1", "172.32.0.1", "11.0.0.1", "2001:db8::1"}
+
+	for _, addr := range trusted {
+		ip := net.ParseIP(addr)
+		if !isTrustedProxy(ip) {
+			t.Errorf("expected %s to be trusted", addr)
+		}
+	}
+
+	for _, addr := range untrusted {
+		ip := net.ParseIP(addr)
+		if isTrustedProxy(ip) {
+			t.Errorf("expected %s to be untrusted", addr)
+		}
+	}
+}