diff --git a/internal/auth/apple.go b/internal/auth/apple.go index 9b4ff60..3ca1d9a 100644 --- a/internal/auth/apple.go +++ b/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, diff --git a/internal/auth/auth.go b/internal/auth/auth.go index f9fd84b..ef464f3 100644 --- a/internal/auth/auth.go +++ b/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 { diff --git a/internal/auth/store.go b/internal/auth/store.go index 5f8ba26..679f491 100644 --- a/internal/auth/store.go +++ b/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) } diff --git a/internal/config/config.go b/internal/config/config.go index 0b3a2a3..911dba9 100644 --- a/internal/config/config.go +++ b/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 == "" { diff --git a/internal/forgejo/client.go b/internal/forgejo/client.go index 616e70e..eaa4f3d 100644 --- a/internal/forgejo/client.go +++ b/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) diff --git a/internal/handlers/public/auth.go b/internal/handlers/public/auth.go index 2b32abc..d214058 100644 --- a/internal/handlers/public/auth.go +++ b/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") +} diff --git a/internal/handlers/public/routes.go b/internal/handlers/public/routes.go index 2a38eef..24f67bb 100644 --- a/internal/handlers/public/routes.go +++ b/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) diff --git a/internal/handlers/public/sso.go b/internal/handlers/public/sso.go index 5adbb47..7ad9046 100644 --- a/internal/handlers/public/sso.go +++ b/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) diff --git a/internal/handlers/public/tickets.go b/internal/handlers/public/tickets.go index a8ebfcc..d5d6cf7 100644 --- a/internal/handlers/public/tickets.go +++ b/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) diff --git a/internal/middleware/flash.go b/internal/middleware/flash.go index fd3167c..81545a1 100644 --- a/internal/middleware/flash.go +++ b/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, }) } diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 4ac42be..79385dd 100644 --- a/internal/middleware/middleware.go +++ b/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") } diff --git a/web/templates/pages/change-password.html b/web/templates/pages/change-password.html new file mode 100644 index 0000000..92b78bc --- /dev/null +++ b/web/templates/pages/change-password.html @@ -0,0 +1,49 @@ +{{define "title"}}Change Password{{end}} + +{{define "content"}} +
+

Change password

+ + {{with .Data}} + {{if .Error}} +
+

{{.Error}}

+
+ {{end}} + + {{if not .HasPassword}} +
+

Your account uses social login and does not have a password set.

+
+ {{else}} +
+ + +
+ + +
+ +
+ + +
+ +
+ + +
+ + +
+ {{end}} + {{end}} + +

+ Back to tickets +

+
+{{end}} diff --git a/web/templates/partials/nav.html b/web/templates/partials/nav.html index fe26aad..a9ea16d 100644 --- a/web/templates/partials/nav.html +++ b/web/templates/partials/nav.html @@ -17,6 +17,7 @@ {{if .User}} My Tickets New Ticket + Password {{.User.Name}} {{if .User.IsAdmin}}Admin{{end}}