Skip to content

Commit 4d950e7

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 efe2e0e commit 4d950e7

File tree

4 files changed

+109
-6
lines changed

4 files changed

+109
-6
lines changed

‎pkg/provider/gitea/gitea.go‎

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

7071
func (v *Provider) Client() *forgejo.Client {
@@ -102,6 +103,22 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
102103
re := regexp.MustCompile(updateMarker)
103104
for _, comment := range comments {
104105
if re.MatchString(comment.Body) {
106+
// Get the UserID for the PAC user.
107+
if v.pacUserID == 0 {
108+
pacUser, _, err := v.Client().GetMyUserInfo()
109+
if err != nil {
110+
return fmt.Errorf("unable to fetch user info: %w", err)
111+
}
112+
v.pacUserID = pacUser.ID
113+
}
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("This comment was not created by PAC, skipping comment edit :%d, created by user %d, PAC user: %d",
118+
comment.ID, comment.Poster.ID, v.pacUserID)
119+
continue
120+
}
121+
105122
_, _, err := v.Client().EditIssueComment(event.Organization, event.Repository, comment.ID, forgejo.EditIssueCommentOption{
106123
Body: commit,
107124
})

‎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: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ type Provider struct {
6767
// current provider instance lifecycle to avoid repeated API calls.
6868
memberCache map[int64]bool
6969
cachedChangedFiles *changedfiles.ChangedFiles
70+
pacUserID int64 // user login used by PAC
7071
}
7172

7273
var defaultGitlabListOptions = gitlab.ListOptions{
@@ -115,6 +116,22 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
115116

116117
for _, comment := range comments {
117118
if commentRe.MatchString(comment.Body) {
119+
// Get the UserID for the PAC user.
120+
if v.pacUserID == 0 {
121+
pacUser, _, err := v.Client().Users.CurrentUser()
122+
if err != nil {
123+
return fmt.Errorf("unable to fetch user info: %w", err)
124+
}
125+
v.pacUserID = pacUser.ID
126+
}
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("This comment was not created by PAC, skipping comment edit :%d, created by user %d, PAC user: %d",
131+
comment.ID, comment.Author.ID, v.pacUserID)
132+
continue
133+
}
134+
118135
_, _, err := v.Client().Notes.UpdateMergeRequestNote(event.TargetProjectID, int64(event.PullRequestNumber), comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
119136
Body: &commit,
120137
})

‎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)
⚡