Skip to content

Commit 58cdfb7

Browse files
theakshaypanttheakshaypant
committed
fix(providers): restrict comment editing to PAC-owned comments
Add check to ensure PAC only edits comments created by its own credentials, preventing accidental modification of comments from other users or bot accounts. Jira: https://issues.redhat.com/browse/SRVKP-10857 Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com> Assisted-by: Claude Sonnet 4.5 (via Claude Code)
1 parent 5b07990 commit 58cdfb7

4 files changed

Lines changed: 111 additions & 6 deletions

File tree

‎pkg/provider/gitea/gitea.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ type Provider struct {
6464
eventEmitter *events.EventEmitter
6565
run *params.Run
6666
triggerEvent string
67+
pacUserID int64 // user login used by PAC
6768
}
6869

6970
func (v *Provider) Client() *forgejo.Client {
@@ -93,6 +94,15 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
9394

9495
// List comments of the PR
9596
if updateMarker != "" {
97+
// Get the UserID for the PAC user.
98+
if v.pacUserID == 0 {
99+
pacUser, _, err := v.Client().GetMyUserInfo()
100+
if err != nil {
101+
return fmt.Errorf("unable to fetch user info: %w", err)
102+
}
103+
v.pacUserID = pacUser.ID
104+
}
105+
96106
comments, _, err := v.Client().ListIssueComments(event.Organization, event.Repository, int64(event.PullRequestNumber), forgejo.ListIssueCommentOptions{})
97107
if err != nil {
98108
return err
@@ -101,6 +111,14 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
101111
re := regexp.MustCompile(updateMarker)
102112
for _, comment := range comments {
103113
if re.MatchString(comment.Body) {
114+
// Only edit comments created by this PAC installation's credentials.
115+
// Prevents accidentally modifying comments from other users/bots.
116+
if comment.Poster.ID != v.pacUserID {
117+
v.Logger.Debugf("Skipping comment %d: created by user ID %d, PAC user ID is %d",
118+
comment.ID, comment.Poster.ID, v.pacUserID)
119+
continue
120+
}
121+
104122
_, _, err := v.Client().EditIssueComment(event.Organization, event.Repository, comment.ID, forgejo.EditIssueCommentOption{
105123
Body: commit,
106124
})

‎pkg/provider/gitea/gitea_test.go‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -781,9 +781,12 @@ func TestCreateComment(t *testing.T) {
781781
commit: "Updated Comment",
782782
updateMarker: "MARKER",
783783
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
784+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
785+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
786+
},
784787
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
785788
if r.Method == http.MethodGet {
786-
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`)
789+
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER", "user": {"id": 100}}]`)
787790
return
788791
}
789792
},
@@ -800,15 +803,42 @@ func TestCreateComment(t *testing.T) {
800803
commit: "New Comment",
801804
updateMarker: "MARKER",
802805
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
806+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
807+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
808+
},
809+
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
810+
if r.Method == http.MethodGet {
811+
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH", "user": {"id": 200}}]`)
812+
return
813+
}
814+
assert.Equal(t, r.Method, http.MethodPost)
815+
rw.WriteHeader(http.StatusCreated)
816+
fmt.Fprint(rw, `{}`)
817+
},
818+
},
819+
},
820+
{
821+
name: "skip comment from different user and create new",
822+
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
823+
commit: "Updated Comment",
824+
updateMarker: "MARKER",
825+
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
826+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
827+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
828+
},
803829
"/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) {
804830
if r.Method == http.MethodGet {
805-
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`)
831+
fmt.Fprint(rw, `[{"id": 555, "body": "Old MARKER", "user": {"id": 999}}]`)
806832
return
807833
}
808834
assert.Equal(t, r.Method, http.MethodPost)
809835
rw.WriteHeader(http.StatusCreated)
810836
fmt.Fprint(rw, `{}`)
811837
},
838+
"/repos/org/repo/issues/comments/555": func(rw http.ResponseWriter, _ *http.Request) {
839+
t.Error("edit endpoint should not be called for comment from different user")
840+
rw.WriteHeader(http.StatusOK)
841+
},
812842
},
813843
},
814844
}
@@ -817,6 +847,8 @@ func TestCreateComment(t *testing.T) {
817847
t.Run(tt.name, func(t *testing.T) {
818848
fakeclient, mux, teardown := tgitea.Setup(t)
819849
defer teardown()
850+
observer, _ := zapobserver.New(zap.InfoLevel)
851+
fakelogger := zap.New(observer).Sugar()
820852

821853
if tt.clientNil {
822854
p := &Provider{}
@@ -829,7 +861,7 @@ func TestCreateComment(t *testing.T) {
829861
mux.HandleFunc(endpoint, handler)
830862
}
831863

832-
p := &Provider{giteaClient: fakeclient}
864+
p := &Provider{giteaClient: fakeclient, Logger: fakelogger}
833865
err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker)
834866
if tt.wantErr != "" {
835867
assert.ErrorContains(t, err, tt.wantErr)

‎pkg/provider/gitlab/gitlab.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ type Provider struct {
6666
// current provider instance lifecycle to avoid repeated API calls.
6767
memberCache map[int64]bool
6868
cachedChangedFiles *changedfiles.ChangedFiles
69+
pacUserID int64 // user login used by PAC
6970
}
7071

7172
var defaultGitlabListOptions = gitlab.ListOptions{
@@ -106,6 +107,15 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
106107
commentRe := regexp.MustCompile(updateMarker)
107108
options := []gitlab.RequestOptionFunc{}
108109

110+
// Get the UserID for the PAC user.
111+
if v.pacUserID == 0 {
112+
pacUser, _, err := v.Client().Users.CurrentUser()
113+
if err != nil {
114+
return fmt.Errorf("unable to fetch user info: %w", err)
115+
}
116+
v.pacUserID = pacUser.ID
117+
}
118+
109119
for {
110120
comments, resp, err := v.Client().Notes.ListMergeRequestNotes(event.TargetProjectID, int64(event.PullRequestNumber), &gitlab.ListMergeRequestNotesOptions{ListOptions: defaultGitlabListOptions}, options...)
111121
if err != nil {
@@ -114,6 +124,14 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
114124

115125
for _, comment := range comments {
116126
if commentRe.MatchString(comment.Body) {
127+
// Only edit comments created by this PAC installation's credentials.
128+
// Prevents accidentally modifying comments from other users/bots.
129+
if comment.Author.ID != v.pacUserID {
130+
v.Logger.Debugf("Skipping comment %d: created by user ID %d, PAC user ID is %d",
131+
comment.ID, comment.Author.ID, v.pacUserID)
132+
continue
133+
}
134+
117135
_, _, err := v.Client().Notes.UpdateMergeRequestNote(event.TargetProjectID, int64(event.PullRequestNumber), comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
118136
Body: &commit,
119137
})

‎pkg/provider/gitlab/gitlab_test.go‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1576,9 +1576,12 @@ func TestGitLabCreateComment(t *testing.T) {
15761576
commit: "Updated Comment",
15771577
updateMarker: "MARKER",
15781578
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1579+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
1580+
fmt.Fprint(rw, `{"id": 100}`)
1581+
},
15791582
"/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) {
15801583
if r.Method == http.MethodGet {
1581-
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`)
1584+
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER", "author": {"id": 100}}]`)
15821585
return
15831586
}
15841587
},
@@ -1595,9 +1598,12 @@ func TestGitLabCreateComment(t *testing.T) {
15951598
commit: "New Comment",
15961599
updateMarker: "MARKER",
15971600
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1601+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
1602+
fmt.Fprint(rw, `{"id": 100}`)
1603+
},
15981604
"/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) {
15991605
if r.Method == http.MethodGet {
1600-
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`)
1606+
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH", "author": {"id": 200}}]`)
16011607
return
16021608
}
16031609
assert.Equal(t, r.Method, http.MethodPost)
@@ -1606,6 +1612,30 @@ func TestGitLabCreateComment(t *testing.T) {
16061612
},
16071613
},
16081614
},
1615+
{
1616+
name: "skip comment from different user and create new",
1617+
event: &info.Event{PullRequestNumber: 123, TargetProjectID: 666},
1618+
commit: "Updated Comment",
1619+
updateMarker: "MARKER",
1620+
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
1621+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
1622+
fmt.Fprint(rw, `{"id": 100}`)
1623+
},
1624+
"/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) {
1625+
if r.Method == http.MethodGet {
1626+
fmt.Fprint(rw, `[{"id": 555, "body": "Old MARKER", "author": {"id": 999}}]`)
1627+
return
1628+
}
1629+
assert.Equal(t, r.Method, http.MethodPost)
1630+
rw.WriteHeader(http.StatusCreated)
1631+
fmt.Fprint(rw, `{}`)
1632+
},
1633+
"/projects/666/merge_requests/123/notes/555": func(rw http.ResponseWriter, _ *http.Request) {
1634+
t.Error("edit endpoint should not be called for comment from different user")
1635+
rw.WriteHeader(http.StatusOK)
1636+
},
1637+
},
1638+
},
16091639
}
16101640

16111641
for _, tt := range tests {
@@ -1632,6 +1662,7 @@ func TestGitLabCreateComment(t *testing.T) {
16321662
p := &Provider{
16331663
sourceProjectID: 666,
16341664
gitlabClient: fakeclient,
1665+
Logger: logger,
16351666
}
16361667
err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker)
16371668
if tt.wantErr != "" {
@@ -1649,6 +1680,9 @@ func TestGitLabCreateCommentPaging(t *testing.T) {
16491680
commit := "Updated Comment"
16501681
updateMarker := "MARKER"
16511682
mockResponses := map[string]func(rw http.ResponseWriter, _ *http.Request){
1683+
"/user": func(rw http.ResponseWriter, _ *http.Request) {
1684+
fmt.Fprint(rw, `{"id": 100}`)
1685+
},
16521686
"/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) {
16531687
if r.Method == http.MethodGet {
16541688
page := thelp.SetPagingHeader(t, rw, r, 100)
@@ -1658,7 +1692,7 @@ func TestGitLabCreateCommentPaging(t *testing.T) {
16581692
} else if page > 10 {
16591693
t.Error("notes shouldn't be queries past the expected ID")
16601694
}
1661-
fmt.Fprintf(rw, `[{"id": %d, "body": "%s"}]`, page, note)
1695+
fmt.Fprintf(rw, `[{"id": %d, "body": "%s", "author": {"id": 100}}]`, page, note)
16621696
}
16631697
},
16641698
"/projects/666/merge_requests/123/notes/{id}": func(rw http.ResponseWriter, r *http.Request) {
@@ -1676,6 +1710,8 @@ func TestGitLabCreateCommentPaging(t *testing.T) {
16761710

16771711
fakeclient, mux, teardown := thelp.Setup(t)
16781712
defer teardown()
1713+
observer, _ := zapobserver.New(zap.InfoLevel)
1714+
logger := zap.New(observer).Sugar()
16791715

16801716
for endpoint, handler := range mockResponses {
16811717
mux.HandleFunc(endpoint, handler)
@@ -1684,6 +1720,7 @@ func TestGitLabCreateCommentPaging(t *testing.T) {
16841720
p := &Provider{
16851721
sourceProjectID: 666,
16861722
gitlabClient: fakeclient,
1723+
Logger: logger,
16871724
}
16881725
err := p.CreateComment(context.Background(), event, commit, updateMarker)
16891726
assert.NilError(t, err)

0 commit comments

Comments
 (0)