fix(security): remediate 2026-05-12 audit findings (Stages 2–5)

Remediation of the 2026-05-12/13 audits (78 findings + cluster gaps), tracked in deploy-k3s/SECURITY.md, plus fixes from two independent post-remediation reviews. Auth & sessions: - SHA-256 hashed auth-token storage (C1); prior-token cache eviction on re-login (MEDIUM-1) - local Google JWKS verification, iss/aud/exp checks (C2/C3) - constant-time login + generic errors (L1/LIVE-L11/LIVE-L13) - per-account login lockout keyed on distinct source IPs (M5/MEDIUM-3) - verified-email gating, login rate limiting (LIVE-L19, H1-H3) IAP & webhooks: - Apple/Google cross-account replay protection (C5/C6/C10/C13, H5/H6) - migrations 000003-000006 (token hashing, IAP replay, audit_log + webhook_event_log table creation, append-only audit log) Authorization & races: - file-ownership owner-OR-member fix (C7), atomic share-code join (C9/H9), device-token reassignment (C8/LOW-3) Secrets & deploy: - secrets file-mounted at /etc/honeydue/secrets, not env (F8); Redis password out of the ConfigMap (HIGH-1); B2 keys reconciled - digest-pinned images, admin ingress hardening, CSP/HSTS, /metrics lockdown; kubeconfig 0600, etcd secrets-encryption, fail2ban + unattended-upgrades at provision; secret-rotation runbook Build, vet, and the full test suite (incl. -race) pass; the goose migration chain is verified against PostgreSQL 16. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-16 22:28:33 -05:00
parent 2004f9c5b2
commit c77ff07ce9
59 changed files with 2819 additions and 1245 deletions
@@ -1,6 +1,7 @@
 package handlers

 import (
+	"context"
 	"errors"
 	"net/http"

@@ -55,8 +56,15 @@ func (h *AuthHandler) SetAuditService(auditService *services.AuditService) {
 	h.auditService = auditService
 }

+// noStore marks a response as non-cacheable (audit L2) — auth responses
+// carry tokens and user data that must never sit in any cache.
+func noStore(c echo.Context) {
+	c.Response().Header().Set("Cache-Control", "no-store")
+}
+
 // Login handles POST /api/auth/login/
 func (h *AuthHandler) Login(c echo.Context) error {
+	noStore(c)
 	var req requests.LoginRequest
 	if err := c.Bind(&req); err != nil {
 		return apperrors.BadRequest("error.invalid_request")
@@ -65,9 +73,11 @@ func (h *AuthHandler) Login(c echo.Context) error {
 		return c.JSON(http.StatusBadRequest, validator.FormatValidationErrors(err))
 	}

-	response, err := h.authService.Login(c.Request().Context(), &req)
+	response, err := h.authService.Login(c.Request().Context(), &req, c.RealIP())
 	if err != nil {
-		log.Debug().Err(err).Str("identifier", req.Username).Msg("Login failed")
+		log.Debug().Err(err).Str("identifier", req.Username).
+			Str("ip", c.RealIP()).Str("user_agent", c.Request().UserAgent()).
+			Msg("Login failed")
 		if h.auditService != nil {
 			h.auditService.LogEvent(c, nil, services.AuditEventLoginFailed, map[string]interface{}{
 				"identifier": req.Username,
@@ -86,6 +96,7 @@ func (h *AuthHandler) Login(c echo.Context) error {

 // Register handles POST /api/auth/register/
 func (h *AuthHandler) Register(c echo.Context) error {
+	noStore(c)
 	var req requests.RegisterRequest
 	if err := c.Bind(&req); err != nil {
 		return apperrors.BadRequest("error.invalid_request")
@@ -157,6 +168,7 @@ func (h *AuthHandler) Logout(c echo.Context) error {

 // CurrentUser handles GET /api/auth/me/
 func (h *AuthHandler) CurrentUser(c echo.Context) error {
+	noStore(c)
 	user, err := middleware.MustGetAuthUser(c)
 	if err != nil {
 		return err
@@ -276,31 +288,7 @@ func (h *AuthHandler) ForgotPassword(c echo.Context) error {
 		return c.JSON(http.StatusBadRequest, validator.FormatValidationErrors(err))
 	}

-	code, user, err := h.authService.ForgotPassword(c.Request().Context(), req.Email)
-	if err != nil {
-		var appErr *apperrors.AppError
-		if errors.As(err, &appErr) && appErr.Code == http.StatusTooManyRequests {
-			// Only reveal rate limit errors
-			return err
-		}
-
-		log.Error().Err(err).Str("email", req.Email).Msg("Forgot password failed")
-		// Don't reveal other errors to prevent email enumeration
-	}
-
-	// Send password reset email (async) - only if user found
-	if h.emailService != nil && code != "" && user != nil {
-		go func() {
-			defer func() {
-				if r := recover(); r != nil {
-					log.Error().Interface("panic", r).Str("email", user.Email).Msg("Panic in password reset email goroutine")
-				}
-			}()
-			if err := h.emailService.SendPasswordResetEmail(user.Email, user.FirstName, code); err != nil {
-				log.Error().Err(err).Str("email", user.Email).Msg("Failed to send password reset email")
-			}
-		}()
-	}
+	noStore(c)

 	if h.auditService != nil {
 		h.auditService.LogEvent(c, nil, services.AuditEventPasswordReset, map[string]interface{}{
@@ -308,7 +296,33 @@ func (h *AuthHandler) ForgotPassword(c echo.Context) error {
 		})
 	}

-	// Always return success to prevent email enumeration
+	// Audit LIVE-L13: run the user lookup, code generation, and email send
+	// entirely in the background, then return the generic response
+	// immediately. This makes the response time identical whether or not
+	// the email belongs to a real account, defeating timing-based user
+	// enumeration. context.Background() is used because the request context
+	// is cancelled the moment this handler returns. Per-account rate
+	// limiting still runs inside the service; the edge auth-rate-limit
+	// middleware covers per-IP abuse.
+	email := req.Email
+	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				log.Error().Interface("panic", r).Str("email", email).Msg("Panic in forgot-password goroutine")
+			}
+		}()
+		code, user, err := h.authService.ForgotPassword(context.Background(), email)
+		if err != nil || code == "" || user == nil {
+			return
+		}
+		if h.emailService != nil {
+			if sendErr := h.emailService.SendPasswordResetEmail(user.Email, user.FirstName, code); sendErr != nil {
+				log.Error().Err(sendErr).Str("email", user.Email).Msg("Failed to send password reset email")
+			}
+		}
+	}()
+
+	// Always return success to prevent email enumeration.
 	return c.JSON(http.StatusOK, responses.ForgotPasswordResponse{
 		Message: "Password reset email sent",
 	})
@@ -365,6 +379,7 @@ func (h *AuthHandler) ResetPassword(c echo.Context) error {

 // AppleSignIn handles POST /api/auth/apple-sign-in/
 func (h *AuthHandler) AppleSignIn(c echo.Context) error {
+	noStore(c)
 	var req requests.AppleSignInRequest
 	if err := c.Bind(&req); err != nil {
 		return apperrors.BadRequest("error.invalid_request")
@@ -412,6 +427,7 @@ func (h *AuthHandler) AppleSignIn(c echo.Context) error {

 // GoogleSignIn handles POST /api/auth/google-sign-in/
 func (h *AuthHandler) GoogleSignIn(c echo.Context) error {
+	noStore(c)
 	var req requests.GoogleSignInRequest
 	if err := c.Bind(&req); err != nil {
 		return apperrors.BadRequest("error.invalid_request")
@@ -459,6 +475,7 @@ func (h *AuthHandler) GoogleSignIn(c echo.Context) error {

 // RefreshToken handles POST /api/auth/refresh/
 func (h *AuthHandler) RefreshToken(c echo.Context) error {
+	noStore(c)
 	user, err := middleware.MustGetAuthUser(c)
 	if err != nil {
 		return err
@@ -650,14 +650,14 @@ func TestAuthHandler_RefreshToken(t *testing.T) {
 	authGroup.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) error {
 			c.Set("auth_user", user)
-			c.Set("auth_token", authToken.Key)
+			c.Set("auth_token", authToken.Plaintext) // raw token — repo hashes for lookup (audit C1)
 			return next(c)
 		}
 	})
 	authGroup.POST("/refresh/", handler.RefreshToken)

 	t.Run("successful refresh", func(t *testing.T) {
-		w := testutil.MakeRequest(e, "POST", "/api/auth/refresh/", nil, authToken.Key)
+		w := testutil.MakeRequest(e, "POST", "/api/auth/refresh/", nil, authToken.Plaintext)
 		testutil.AssertStatusCode(t, w, http.StatusOK)

 		var response map[string]interface{}
@@ -37,6 +37,23 @@ func NewMediaHandler(
 	}
 }

+// safeContentDisposition builds an inline Content-Disposition header value
+// with a sanitized filename (audit M1). Control characters (including CR/LF),
+// double-quote and backslash are stripped so an attacker-controlled upload
+// filename cannot inject additional response headers (CWE-113).
+func safeContentDisposition(filename string) string {
+	cleaned := strings.Map(func(r rune) rune {
+		if r < 0x20 || r == 0x7f || r == '"' || r == '\\' {
+			return -1
+		}
+		return r
+	}, filename)
+	if cleaned == "" {
+		cleaned = "download"
+	}
+	return `inline; filename="` + cleaned + `"`
+}
+
 // ServeDocument serves a document file with access control
 // GET /api/media/document/:id
 func (h *MediaHandler) ServeDocument(c echo.Context) error {
@@ -71,7 +88,7 @@ func (h *MediaHandler) ServeDocument(c echo.Context) error {
 	// Set caching and disposition headers
 	c.Response().Header().Set("Cache-Control", "private, max-age=3600")
 	if doc.FileName != "" {
-		c.Response().Header().Set("Content-Disposition", "inline; filename=\""+doc.FileName+"\"")
+		c.Response().Header().Set("Content-Disposition", safeContentDisposition(doc.FileName))
 	}
 	return c.Blob(http.StatusOK, mimeType, data)
 }
@@ -114,7 +131,7 @@ func (h *MediaHandler) ServeDocumentImage(c echo.Context) error {
 	}

 	c.Response().Header().Set("Cache-Control", "private, max-age=3600")
-	c.Response().Header().Set("Content-Disposition", "inline; filename=\""+filepath.Base(img.ImageURL)+"\"")
+	c.Response().Header().Set("Content-Disposition", safeContentDisposition(filepath.Base(img.ImageURL)))
 	return c.Blob(http.StatusOK, mimeType, data)
 }

@@ -162,7 +179,7 @@ func (h *MediaHandler) ServeCompletionImage(c echo.Context) error {
 	}

 	c.Response().Header().Set("Cache-Control", "private, max-age=3600")
-	c.Response().Header().Set("Content-Disposition", "inline; filename=\""+filepath.Base(img.ImageURL)+"\"")
+	c.Response().Header().Set("Content-Disposition", safeContentDisposition(filepath.Base(img.ImageURL)))
 	return c.Blob(http.StatusOK, mimeType, data)
 }

@@ -8,6 +8,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io"
 	"math/big"
@@ -20,6 +21,7 @@ import (
 	"github.com/golang-jwt/jwt/v5"
 	"github.com/labstack/echo/v4"
 	"github.com/rs/zerolog/log"
+	"gorm.io/gorm"

 	"github.com/treytartt/honeydue-api/internal/config"
 	"github.com/treytartt/honeydue-api/internal/models"
@@ -165,9 +167,13 @@ func (h *SubscriptionWebhookHandler) HandleAppleWebhook(c echo.Context) error {
 	if notification.NotificationUUID != "" {
 		alreadyProcessed, err := h.webhookEventRepo.HasProcessed("apple", notification.NotificationUUID)
 		if err != nil {
-			log.Error().Err(err).Msg("Apple Webhook: Failed to check dedup")
-			// Continue processing on dedup check failure (fail-open)
-		} else if alreadyProcessed {
+			// Audit H6: fail closed. A dedup-check failure must not let a
+			// possibly-duplicate event through (duplicate refunds/grants).
+			// Return 500 so Apple retries once the DB is healthy.
+			log.Error().Err(err).Msg("Apple Webhook: dedup check failed — returning 500 for retry")
+			return c.JSON(http.StatusInternalServerError, map[string]interface{}{"error": "dedup check failed"})
+		}
+		if alreadyProcessed {
 			log.Info().Str("uuid", notification.NotificationUUID).Msg("Apple Webhook: Duplicate event, skipping")
 			return c.JSON(http.StatusOK, map[string]interface{}{"status": "duplicate"})
 		}
@@ -352,10 +358,24 @@ func (h *SubscriptionWebhookHandler) processAppleNotification(
 }

 func (h *SubscriptionWebhookHandler) findUserByAppleTransaction(originalTransactionID string) (*models.User, error) {
-	// Look up user subscription by stored receipt data
-	subscription, err := h.subscriptionRepo.FindByAppleReceiptContains(originalTransactionID)
+	// Audit C13: exact match on the indexed apple_original_transaction_id
+	// column. Falls back to the legacy escaped-LIKE scan over
+	// apple_receipt_data only for subscriptions created before that column
+	// existed (and thus not yet populated).
+	subscription, err := h.subscriptionRepo.FindByAppleOriginalTransactionID(originalTransactionID)
 	if err != nil {
-		return nil, err
+		// Only fall back to the legacy substring scan when the exact-match
+		// column genuinely had no row (a subscription created before the
+		// column existed). A real DB error must propagate — masking it as
+		// "not found" could bind the webhook to the wrong account via the
+		// LIKE scan, or silently drop a legitimate event.
+		if !errors.Is(err, gorm.ErrRecordNotFound) {
+			return nil, err
+		}
+		subscription, err = h.subscriptionRepo.FindByAppleReceiptContains(originalTransactionID)
+		if err != nil {
+			return nil, err
+		}
 	}

 	user, err := h.userRepo.FindByID(subscription.UserID)
@@ -566,9 +586,12 @@ func (h *SubscriptionWebhookHandler) HandleGoogleWebhook(c echo.Context) error {
 	if messageID != "" {
 		alreadyProcessed, err := h.webhookEventRepo.HasProcessed("google", messageID)
 		if err != nil {
-			log.Error().Err(err).Msg("Google Webhook: Failed to check dedup")
-			// Continue processing on dedup check failure (fail-open)
-		} else if alreadyProcessed {
+			// Audit H6: fail closed — see the Apple handler. Return 500 so
+			// Google Pub/Sub redelivers once the DB is healthy.
+			log.Error().Err(err).Msg("Google Webhook: dedup check failed — returning 500 for retry")
+			return c.JSON(http.StatusInternalServerError, map[string]interface{}{"error": "dedup check failed"})
+		}
+		if alreadyProcessed {
 			log.Info().Str("message_id", messageID).Msg("Google Webhook: Duplicate event, skipping")
 			return c.JSON(http.StatusOK, map[string]interface{}{"status": "duplicate"})
 		}