Fix security vulnerabilities identified in pre-deployment audit

- Regenerate session ID on login to prevent session fixation (H1)
- Add mutex to login lockout counters to fix race condition (H2)
- Validate issuer/audience claims on Apple ID tokens (M2)
- Verify comment belongs to ticket's issue to prevent attachment IDOR (M4)
- Stop SSO from re-approving admin-disapproved users (M3)
- Add Content-Security-Policy header (M1)
- Configure trusted proxies via TRUSTED_PROXIES env var (M6)
- Cap password length at 128 for bcrypt truncation (L1)
- Set Secure flag on flash cookies over HTTPS (L2)
- Rate-limit POST /reset-password (L3)
- Add authenticated password change at /account/password (L4)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

internal/auth/apple.go

@@ -105,8 +105,13 @@ func (p *AppleProvider) getUserInfo(ctx context.Context, token *oauth2.Token) (*
 		return nil, fmt.Errorf("decode apple JWKS: %w", err)
 	}
 
-	// Parse and verify the token
-	parsed, err := jwt.Parse(idToken, func(t *jwt.Token) (interface{}, error) {
+	// Parse and verify the token with issuer and audience validation
+	type appleIDClaims struct {
+		Email string `json:"email"`
+		jwt.RegisteredClaims
+	}
+	claims := &appleIDClaims{}
+	parsed, err := jwt.ParseWithClaims(idToken, claims, func(t *jwt.Token) (interface{}, error) {
 		kid, ok := t.Header["kid"].(string)
 		if !ok {
 			return nil, fmt.Errorf("missing kid header")
@@ -136,18 +141,20 @@ func (p *AppleProvider) getUserInfo(ctx context.Context, token *oauth2.Token) (*
 			}
 		}
 		return nil, fmt.Errorf("key %s not found in JWKS", kid)
-	})
+	},
+		jwt.WithIssuer("https://appleid.apple.com"),
+		jwt.WithAudience(p.Config.ClientID),
+	)
 	if err != nil {
 		return nil, fmt.Errorf("verify id_token: %w", err)
 	}
 
-	claims, ok := parsed.Claims.(jwt.MapClaims)
-	if !ok {
-		return nil, fmt.Errorf("invalid claims")
+	if !parsed.Valid {
+		return nil, fmt.Errorf("invalid id_token")
 	}
 
-	sub, _ := claims["sub"].(string)
-	email, _ := claims["email"].(string)
+	sub := claims.Subject
+	email := claims.Email
 
 	return &OAuthUserInfo{
 		ProviderUserID: sub,

internal/auth/auth.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/gorilla/sessions"
 	"github.com/mattnite/forgejo-tickets/internal/email"
 	"github.com/mattnite/forgejo-tickets/internal/models"
 	"golang.org/x/crypto/bcrypt"
@@ -20,6 +21,7 @@ const (
 )
 
 type loginAttempt struct {
+	mu       sync.Mutex
 	count    int
 	lastFail time.Time
 }
@@ -64,11 +66,14 @@ func (s *Service) Login(ctx context.Context, emailAddr, password string) (*model
 	// Check if the account is locked due to too many failed attempts
 	if val, ok := s.loginAttempts.Load(emailAddr); ok {
 		attempt := val.(*loginAttempt)
-		if attempt.count >= maxLoginAttempts && time.Since(attempt.lastFail) < lockoutDuration {
+		attempt.mu.Lock()
+		locked := attempt.count >= maxLoginAttempts && time.Since(attempt.lastFail) < lockoutDuration
+		expired := time.Since(attempt.lastFail) >= lockoutDuration
+		attempt.mu.Unlock()
+		if locked {
 			return nil, fmt.Errorf("account temporarily locked, try again later")
 		}
-		// Reset if the lockout window has expired
-		if time.Since(attempt.lastFail) >= lockoutDuration {
+		if expired {
 			s.loginAttempts.Delete(emailAddr)
 		}
 	}
@@ -105,19 +110,20 @@ func (s *Service) Login(ctx context.Context, emailAddr, password string) (*model
 func (s *Service) recordFailedAttempt(emailAddr string) {
 	val, _ := s.loginAttempts.LoadOrStore(emailAddr, &loginAttempt{})
 	attempt := val.(*loginAttempt)
+	attempt.mu.Lock()
 	attempt.count++
 	attempt.lastFail = time.Now()
+	attempt.mu.Unlock()
 }
 
 func (s *Service) CreateSession(r *http.Request, w http.ResponseWriter, userID uuid.UUID) error {
-	session, err := s.store.Get(r, sessionCookieName)
-	if err != nil {
-		session, err = s.store.New(r, sessionCookieName)
-		if err != nil {
-			return err
-		}
-	}
+	// Always destroy any existing session first to prevent session fixation.
+	s.DestroySession(r, w)
 
+	// Create a brand new session with a fresh ID (bypass cookie reuse).
+	session := sessions.NewSession(s.store, sessionCookieName)
+	session.Options = s.store.Options()
+	session.IsNew = true
 	session.Values["user_id"] = userID.String()
 	return s.store.Save(r, w, session)
 }
@@ -132,6 +138,29 @@ func (s *Service) DestroySession(r *http.Request, w http.ResponseWriter) error {
 	return s.store.Save(r, w, session)
 }
 
+func (s *Service) ChangePassword(ctx context.Context, userID uuid.UUID, currentPassword, newPassword string) error {
+	var user models.User
+	if err := s.db.WithContext(ctx).First(&user, "id = ?", userID).Error; err != nil {
+		return fmt.Errorf("user not found")
+	}
+
+	if user.PasswordHash == nil {
+		return fmt.Errorf("this account uses social login and has no password to change")
+	}
+
+	if err := bcrypt.CompareHashAndPassword([]byte(*user.PasswordHash), []byte(currentPassword)); err != nil {
+		return fmt.Errorf("current password is incorrect")
+	}
+
+	hash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost)
+	if err != nil {
+		return fmt.Errorf("hash password: %w", err)
+	}
+
+	hashStr := string(hash)
+	return s.db.WithContext(ctx).Model(&user).Update("password_hash", hashStr).Error
+}
+
 func (s *Service) CreateUserWithPassword(ctx context.Context, emailAddr, password, name string, verified bool, approved bool) (*models.User, error) {
 	hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
 	if err != nil {

internal/auth/store.go

@@ -42,6 +42,17 @@ func NewPGStore(db *gorm.DB, secure bool, keyPairs ...[]byte) *PGStore {
 	}
 }
 
+// Options returns a copy of the store's session options.
+func (s *PGStore) Options() *sessions.Options {
+	return &sessions.Options{
+		Path:     s.options.Path,
+		MaxAge:   s.options.MaxAge,
+		HttpOnly: s.options.HttpOnly,
+		SameSite: s.options.SameSite,
+		Secure:   s.options.Secure,
+	}
+}
+
 func (s *PGStore) Get(r *http.Request, name string) (*sessions.Session, error) {
 	return sessions.GetRegistry(r).Get(s, name)
 }

internal/config/config.go

@@ -39,6 +39,8 @@ type Config struct {
 	AppleKeyID     string
 	AppleKeyPath   string
 
+	// Proxy
+	TrustedProxies string
 }
 
 func Load() (*Config, error) {
@@ -60,6 +62,7 @@ func Load() (*Config, error) {
 		AppleTeamID:           getEnv("APPLE_TEAM_ID", ""),
 		AppleKeyID:            getEnv("APPLE_KEY_ID", ""),
 		AppleKeyPath:          getEnv("APPLE_KEY_PATH", ""),
+		TrustedProxies:        getEnv("TRUSTED_PROXIES", ""),
 	}
 
 	if cfg.DatabaseURL == "" {

internal/forgejo/client.go

@@ -86,6 +86,7 @@ type Comment struct {
 	User      APIUser      `json:"user"`
 	Assets    []Attachment `json:"assets"`
 	CreatedAt time.Time    `json:"created_at"`
+	IssueURL  string       `json:"issue_url"` // e.g. "https://forgejo/api/v1/repos/owner/repo/issues/123"
 }
 
 type APIUser struct {
@@ -94,6 +95,18 @@ type APIUser struct {
 	Email    string `json:"email"`
 }
 
+// IssueNumber extracts the issue number from the comment's issue_url field.
+func (c *Comment) IssueNumber() (int64, error) {
+	if c.IssueURL == "" {
+		return 0, fmt.Errorf("comment has no issue_url")
+	}
+	parts := strings.Split(c.IssueURL, "/")
+	if len(parts) == 0 {
+		return 0, fmt.Errorf("invalid issue_url: %s", c.IssueURL)
+	}
+	return strconv.ParseInt(parts[len(parts)-1], 10, 64)
+}
+
 // DisplayName returns the best human-readable name for the user.
 func (u APIUser) DisplayName() string {
 	if u.FullName != "" {
@@ -510,6 +523,34 @@ func (c *Client) GetIssue(owner, repo string, number int64) (*Issue, error) {
 	return &issue, nil
 }
 
+// GetComment fetches a single comment by ID. Used to verify comment ownership.
+func (c *Client) GetComment(owner, repo string, commentID int64) (*Comment, error) {
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d", c.baseURL, owner, repo, commentID)
+
+	httpReq, err := http.NewRequest("GET", reqURL, nil)
+	if err != nil {
+		return nil, err
+	}
+	httpReq.Header.Set("Authorization", "token "+c.apiToken)
+
+	resp, err := c.httpClient.Do(httpReq)
+	if err != nil {
+		return nil, fmt.Errorf("forgejo API request failed: %w", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		respBody, _ := io.ReadAll(resp.Body)
+		return nil, fmt.Errorf("forgejo API returned %d: %s", resp.StatusCode, string(respBody))
+	}
+
+	var comment Comment
+	if err := json.NewDecoder(resp.Body).Decode(&comment); err != nil {
+		return nil, err
+	}
+	return &comment, nil
+}
+
 func (c *Client) ListIssueComments(owner, repo string, number int64) ([]Comment, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/comments", c.baseURL, owner, repo, number)
 

internal/handlers/public/auth.go

@@ -17,6 +17,9 @@ func validatePassword(password string) string {
 	if len(password) < 8 {
 		return "Password must be at least 8 characters"
 	}
+	if len(password) > 128 {
+		return "Password must be at most 128 characters"
+	}
 	var hasUpper, hasLower, hasDigit bool
 	for _, r := range password {
 		switch {
@@ -226,3 +229,47 @@ func (h *AuthHandler) ResetPassword(c *gin.Context) {
 	middleware.SetFlash(c, "success", "Password reset successfully. You can now log in.")
 	c.Redirect(http.StatusSeeOther, "/login")
 }
+
+func (h *AuthHandler) ChangePasswordForm(c *gin.Context) {
+	user := auth.CurrentUser(c)
+	hasPassword := user.PasswordHash != nil
+	h.deps.Renderer.Render(c.Writer, c.Request, "change-password", map[string]interface{}{
+		"HasPassword": hasPassword,
+	})
+}
+
+func (h *AuthHandler) ChangePassword(c *gin.Context) {
+	user := auth.CurrentUser(c)
+	currentPassword := c.PostForm("current_password")
+	newPassword := c.PostForm("new_password")
+	confirmPassword := c.PostForm("confirm_password")
+
+	hasPassword := user.PasswordHash != nil
+
+	if newPassword != confirmPassword {
+		h.deps.Renderer.Render(c.Writer, c.Request, "change-password", map[string]interface{}{
+			"HasPassword": hasPassword,
+			"Error":       "New passwords do not match",
+		})
+		return
+	}
+
+	if errMsg := validatePassword(newPassword); errMsg != "" {
+		h.deps.Renderer.Render(c.Writer, c.Request, "change-password", map[string]interface{}{
+			"HasPassword": hasPassword,
+			"Error":       errMsg,
+		})
+		return
+	}
+
+	if err := h.deps.Auth.ChangePassword(c.Request.Context(), user.ID, currentPassword, newPassword); err != nil {
+		h.deps.Renderer.Render(c.Writer, c.Request, "change-password", map[string]interface{}{
+			"HasPassword": hasPassword,
+			"Error":       err.Error(),
+		})
+		return
+	}
+
+	middleware.SetFlash(c, "success", "Password changed successfully.")
+	c.Redirect(http.StatusSeeOther, "/tickets")
+}

internal/handlers/public/routes.go

@@ -30,6 +30,13 @@ func NewRouter(deps Dependencies) *gin.Engine {
 	r := gin.New()
 	r.MaxMultipartMemory = 10 << 20 // 10 MB
 
+	// Configure trusted proxies to prevent X-Forwarded-For spoofing for rate limits
+	if deps.Config.TrustedProxies != "" {
+		r.SetTrustedProxies(strings.Split(deps.Config.TrustedProxies, ","))
+	} else {
+		r.SetTrustedProxies(nil)
+	}
+
 	r.Use(middleware.RequestID)
 	r.Use(middleware.Logging)
 	r.Use(middleware.Recovery)
@@ -70,7 +77,7 @@ func NewRouter(deps Dependencies) *gin.Engine {
 		csrf.GET("/forgot-password", authHandler.ForgotPasswordForm)
 		csrf.POST("/forgot-password", authRateLimiter.Middleware(), authHandler.ForgotPassword)
 		csrf.GET("/reset-password", authHandler.ResetPasswordForm)
-		csrf.POST("/reset-password", authHandler.ResetPassword)
+		csrf.POST("/reset-password", authRateLimiter.Middleware(), authHandler.ResetPassword)
 
 		oauthHandler := &OAuthHandler{deps: deps}
 		csrf.GET("/auth/:provider/login", oauthHandler.Login)
@@ -80,6 +87,9 @@ func NewRouter(deps Dependencies) *gin.Engine {
 		authenticated := csrf.Group("/")
 		authenticated.Use(auth.RequireAuth)
 		{
+			authenticated.GET("/account/password", authHandler.ChangePasswordForm)
+			authenticated.POST("/account/password", authHandler.ChangePassword)
+
 			ticketHandler := &TicketHandler{deps: deps}
 			authenticated.GET("/tickets", ticketHandler.List)
 			authenticated.GET("/tickets/new", ticketHandler.NewForm)

internal/handlers/public/sso.go

@@ -105,7 +105,7 @@ func (h *SSOHandler) HandleSSO(c *gin.Context) {
 		log.Info().Str("email", email).Str("repo", slug).Msg("SSO: existing user logged in")
 	}
 
-	// Update existing user if needed
+	// Update existing user if needed (never re-approve a user that was disapproved by admin)
 	updates := map[string]interface{}{}
 	if user.Name != name {
 		updates["name"] = name
@@ -113,13 +113,15 @@ func (h *SSOHandler) HandleSSO(c *gin.Context) {
 	if !user.EmailVerified {
 		updates["email_verified"] = true
 	}
-	if !user.Approved {
-		updates["approved"] = true
-	}
 	if len(updates) > 0 {
 		h.deps.DB.Model(&user).Updates(updates)
 	}
 
+	if !user.Approved {
+		c.String(http.StatusForbidden, "your account is not approved")
+		return
+	}
+
 	// Assign user to repo if not already
 	var count int64
 	h.deps.DB.Model(&models.UserRepo{}).Where("user_id = ? AND repo_id = ?", user.ID, repo.ID).Count(&count)

internal/handlers/public/tickets.go

@@ -557,7 +557,7 @@ func (h *TicketHandler) DownloadIssueAttachment(c *gin.Context) {
 
 // DownloadCommentAttachment proxies a comment-level attachment download via Forgejo API.
 func (h *TicketHandler) DownloadCommentAttachment(c *gin.Context) {
-	_, repo, ok := h.verifyTicketOwnership(c)
+	ticket, repo, ok := h.verifyTicketOwnership(c)
 	if !ok {
 		return
 	}
@@ -566,6 +566,26 @@ func (h *TicketHandler) DownloadCommentAttachment(c *gin.Context) {
 	attachmentID := c.Param("attachmentId")
 	filename := c.Param("filename")
 
+	// Validate that the comment belongs to this ticket's issue to prevent IDOR
+	commentIDInt, err := strconv.ParseInt(commentID, 10, 64)
+	if err != nil {
+		h.deps.Renderer.RenderError(c.Writer, c.Request, http.StatusBadRequest, "Invalid comment ID")
+		return
+	}
+
+	comment, err := h.deps.ForgejoClient.GetComment(repo.ForgejoOwner, repo.ForgejoRepo, commentIDInt)
+	if err != nil {
+		log.Error().Err(err).Int64("commentID", commentIDInt).Msg("failed to verify comment ownership")
+		h.deps.Renderer.RenderError(c.Writer, c.Request, http.StatusNotFound, "Comment not found")
+		return
+	}
+
+	issueNumber, err := comment.IssueNumber()
+	if err != nil || issueNumber != ticket.ForgejoIssueNumber {
+		h.deps.Renderer.RenderError(c.Writer, c.Request, http.StatusForbidden, "Access denied")
+		return
+	}
+
 	assetURL := h.deps.ForgejoClient.BaseURL() + "/api/v1/repos/" + repo.ForgejoOwner + "/" + repo.ForgejoRepo + "/issues/comments/" + commentID + "/assets/" + attachmentID
 
 	h.proxyAssetDownload(c, assetURL, filename)

internal/middleware/flash.go

@@ -18,6 +18,7 @@ func SetFlash(c *gin.Context, flashType, message string) {
 		Path:     "/",
 		MaxAge:   60,
 		HttpOnly: true,
+		Secure:   c.Request.TLS != nil || c.GetHeader("X-Forwarded-Proto") == "https",
 		SameSite: http.SameSiteLaxMode,
 	})
 }

internal/middleware/middleware.go

@@ -40,6 +40,7 @@ func SecurityHeaders(secure bool) gin.HandlerFunc {
 		c.Header("X-Content-Type-Options", "nosniff")
 		c.Header("X-Frame-Options", "DENY")
 		c.Header("Referrer-Policy", "strict-origin-when-cross-origin")
+		c.Header("Content-Security-Policy", "default-src 'self'; script-src 'self' https://cdn.jsdelivr.net; style-src 'self' 'unsafe-inline'; img-src 'self' data:; frame-ancestors 'none'")
 		if secure {
 			c.Header("Strict-Transport-Security", "max-age=63072000; includeSubDomains")
 		}

web/templates/pages/change-password.html

@@ -0,0 +1,49 @@
+{{define "title"}}Change Password{{end}}
+
+{{define "content"}}
+<div class="mx-auto max-w-sm">
+    <h2 class="text-2xl font-bold text-gray-900 text-center">Change password</h2>
+
+    {{with .Data}}
+        {{if .Error}}
+        <div class="mt-4 rounded-md bg-red-50 p-4">
+            <p class="text-sm text-red-800">{{.Error}}</p>
+        </div>
+        {{end}}
+
+        {{if not .HasPassword}}
+        <div class="mt-4 rounded-md bg-yellow-50 p-4">
+            <p class="text-sm text-yellow-800">Your account uses social login and does not have a password set.</p>
+        </div>
+        {{else}}
+        <form method="POST" action="/account/password" class="mt-8 space-y-6">
+            <input type="hidden" name="gorilla.csrf.Token" value="{{$.CSRFToken}}">
+
+            <div>
+                <label for="current_password" class="block text-sm font-medium text-gray-700">Current Password</label>
+                <input type="password" name="current_password" id="current_password" required
+                       class="mt-1 block w-full rounded-md border border-gray-300 px-3 py-2 shadow-sm focus:border-blue-500 focus:outline-none focus:ring-1 focus:ring-blue-500">
+            </div>
+
+            <div>
+                <label for="new_password" class="block text-sm font-medium text-gray-700">New Password</label>
+                <input type="password" name="new_password" id="new_password" required minlength="8"
+                       class="mt-1 block w-full rounded-md border border-gray-300 px-3 py-2 shadow-sm focus:border-blue-500 focus:outline-none focus:ring-1 focus:ring-blue-500">
+            </div>
+
+            <div>
+                <label for="confirm_password" class="block text-sm font-medium text-gray-700">Confirm New Password</label>
+                <input type="password" name="confirm_password" id="confirm_password" required
+                       class="mt-1 block w-full rounded-md border border-gray-300 px-3 py-2 shadow-sm focus:border-blue-500 focus:outline-none focus:ring-1 focus:ring-blue-500">
+            </div>
+
+            <button type="submit" class="w-full rounded-md bg-blue-600 px-4 py-2 text-sm font-semibold text-white shadow hover:bg-blue-500">Change password</button>
+        </form>
+        {{end}}
+    {{end}}
+
+    <p class="mt-4 text-center text-sm text-gray-600">
+        <a href="/tickets" class="font-medium text-blue-600 hover:text-blue-500">Back to tickets</a>
+    </p>
+</div>
+{{end}}

web/templates/partials/nav.html

@@ -17,6 +17,7 @@
                 {{if .User}}
                     <a href="/tickets" class="text-sm font-medium text-gray-700 hover:text-gray-900">My Tickets</a>
                     <a href="/tickets/new" class="text-sm font-medium text-white bg-blue-600 hover:bg-blue-700 px-3 py-1.5 rounded-md">New Ticket</a>
+                    <a href="/account/password" class="text-sm font-medium text-gray-500 hover:text-gray-700">Password</a>
                     <span class="text-sm text-gray-500">{{.User.Name}}</span>
                     {{if .User.IsAdmin}}<span class="inline-flex items-center rounded-full bg-blue-100 px-2 py-0.5 text-xs font-medium text-blue-700">Admin</span>{{end}}
                     <form method="POST" action="/logout" class="inline">