From 0f4ebcba100b994242b8695e4c594280823789fc Mon Sep 17 00:00:00 2001 From: yuvrajangadsingh Date: Mon, 9 Mar 2026 14:56:45 +0530 Subject: [PATCH] fix: address review - add archive check, verify repo ownership, remove path fallback --- models/issues/commit_comment.go | 15 +++++++++++++-- routers/web/repo/commit_comment.go | 5 +---- routers/web/web.go | 4 ++-- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/models/issues/commit_comment.go b/models/issues/commit_comment.go index 297b97ce6c..45d5b1bafb 100644 --- a/models/issues/commit_comment.go +++ b/models/issues/commit_comment.go @@ -120,8 +120,19 @@ func DeleteCommitComment(ctx context.Context, commentID int64) error { }) } -// GetCommitCommentByID returns a commit comment by loading the Comment entry. -func GetCommitCommentByID(ctx context.Context, commentID int64) (*Comment, error) { +// GetCommitCommentByID returns a commit comment by loading the Comment entry, +// verifying it belongs to the given repository via the junction table. +func GetCommitCommentByID(ctx context.Context, repoID, commentID int64) (*Comment, error) { + exists, err := db.GetEngine(ctx).Table("commit_comment"). + Where("repo_id = ? AND comment_id = ?", repoID, commentID). + Exist() + if err != nil { + return nil, err + } + if !exists { + return nil, db.ErrNotExist{Resource: "CommitComment", ID: commentID} + } + c := &Comment{} has, err := db.GetEngine(ctx).ID(commentID).Get(c) if err != nil { diff --git a/routers/web/repo/commit_comment.go b/routers/web/repo/commit_comment.go index 10f772363c..a3761daaf3 100644 --- a/routers/web/repo/commit_comment.go +++ b/routers/web/repo/commit_comment.go @@ -40,9 +40,6 @@ func CreateCommitComment(ctx *context.Context) { content := ctx.FormString("content") treePath := ctx.FormString("tree_path") - if treePath == "" { - treePath = ctx.FormString("path") - } side := ctx.FormString("side") line := ctx.FormInt64("line") @@ -132,7 +129,7 @@ func DeleteCommitComment(ctx *context.Context) { return } - comment, err := issues_model.GetCommitCommentByID(ctx, commentID) + comment, err := issues_model.GetCommitCommentByID(ctx, ctx.Repo.Repository.ID, commentID) if err != nil { ctx.NotFound(err) return diff --git a/routers/web/web.go b/routers/web/web.go index be1ce1b1cb..01cc33814f 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1686,8 +1686,8 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Get("/commit/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) m.Get("/commit/{sha:([a-f0-9]{7,64})$}/load-branches-and-tags", repo.LoadBranchesAndTags) m.Get("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, repo.RenderNewCommitCommentForm) - m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, repo.CreateCommitComment) - m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment/{id}/delete", reqSignIn, repo.DeleteCommitComment) + m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment", reqSignIn, context.RepoMustNotBeArchived(), repo.CreateCommitComment) + m.Post("/commit/{sha:([a-f0-9]{7,64})$}/comment/{id}/delete", reqSignIn, context.RepoMustNotBeArchived(), repo.DeleteCommitComment) // FIXME: this route `/cherry-pick/{sha}` doesn't seem useful or right, the new code always uses `/_cherrypick/` which could handle branch name correctly m.Get("/cherry-pick/{sha:([a-f0-9]{7,64})$}", repo.SetEditorconfigIfExists, context.RepoRefByDefaultBranch(), repo.CherryPick)