fix(github): restrict comment editing to PAC-owned comments

theakshaypant · chmouel · commit 95bb453d97c1 · 2026-03-03T07:52:44.000+01:00
Add check to ensure PAC only edits comments created by its own credentials, preventing accidental modification of comments from other users or bot accounts. For GitHub Apps, fetch bot login from app slug if needed on comment match. 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)
diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go
@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"github.com/gobwas/glob"
+	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/go-github/v81/github"
 	"github.com/jonboulle/clockwork"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
@@ -62,6 +63,7 @@ type Provider struct {
 	triggerEvent       string
 	cachedChangedFiles *changedfiles.ChangedFiles
 	commitInfo         *github.Commit
+	pacUserLogin       string // user/bot login used by PAC
 }
 
 type skippedRun struct {
@@ -898,6 +900,16 @@ func (v *Provider) listCommentsByMarker(
 	matchedComments := make([]*github.IssueComment, 0, len(comments))
 	for _, comment := range comments {
 		if re.MatchString(comment.GetBody()) {
+			if err = v.getUserLogin(ctx, event); err != nil {
+				return nil, fmt.Errorf("unable to fetch user info: %w", err)
+			}
+			// Only edit comments created by this PAC installation's credentials.
+			// Prevents accidentally modifying comments from other users/bots.
+			if comment.GetUser().GetLogin() != v.pacUserLogin {
+				v.Logger.Debugf("This comment was not created by PAC, skipping comment edit :%d, created by user %s, PAC user: %s",
+					comment.GetID(), comment.GetUser().GetLogin(), v.pacUserLogin)
+				continue
+			}
 			matchedComments = append(matchedComments, comment)
 		}
 	}
@@ -989,3 +1001,61 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
 	)
 	return nil
 }
+
+func (v *Provider) getUserLogin(ctx context.Context, event *info.Event) error {
+	if v.pacUserLogin != "" {
+		return nil
+	}
+
+	// Get the PAC user/bot login.
+	if event.InstallationID > 0 {
+		// For Apps, get the app slug.
+		slug, err := v.fetchAppSlug(ctx, event.Provider.URL)
+		if err != nil {
+			return fmt.Errorf("failed to fetch app slug: %w", err)
+		}
+
+		v.pacUserLogin = fmt.Sprintf("%s[bot]", slug)
+	} else {
+		// For PATs, get the authenticated user.
+		pacUser, _, err := v.Client().Users.Get(ctx, "")
+		if err != nil {
+			return fmt.Errorf("unable to fetch user info: %w", err)
+		}
+		v.pacUserLogin = pacUser.GetLogin()
+	}
+
+	return nil
+}
+
+// Refer https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/authenticating-as-a-github-app-installation
+func (v *Provider) fetchAppSlug(ctx context.Context, apiURL string) (string, error) {
+	ns := info.GetNS(ctx)
+	applicationID, privateKey, err := v.GetAppIDAndPrivateKey(ctx, ns, v.Run.Clients.Kube)
+	if err != nil {
+		return "", err
+	}
+	parsedPK, err := jwt.ParseRSAPrivateKeyFromPEM(privateKey)
+	if err != nil {
+		return "", fmt.Errorf("failed to parse private key: %w", err)
+	}
+
+	token := jwt.NewWithClaims(jwt.SigningMethodRS256, &jwt.RegisteredClaims{
+		Issuer:    fmt.Sprintf("%d", applicationID),
+		IssuedAt:  jwt.NewNumericDate(time.Now()),
+		ExpiresAt: jwt.NewNumericDate(time.Now().Add(5 * time.Minute)),
+	})
+
+	tokenString, err := token.SignedString(parsedPK)
+	if err != nil {
+		return "", fmt.Errorf("failed to sign private key: %w", err)
+	}
+
+	client, _, _ := MakeClient(ctx, apiURL, tokenString)
+	app, _, err := client.Apps.Get(ctx, "")
+	if err != nil {
+		return "", fmt.Errorf("failed to get app info: %w", err)
+	}
+
+	return app.GetSlug(), nil
+}
diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go
@@ -1468,9 +1468,13 @@ func TestCreateComment(t *testing.T) {
 			updateMarker: "MARKER",
 			setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
 				t.Helper()
+				mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
+					fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
+				},
+				)
 				mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
 					if r.Method == http.MethodGet {
-						fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`)
+						fmt.Fprint(rw, `[{"id": 555, "body": "MARKER", "user": {"id": 100, "login": "pac-user"}}]`)
 						return
 					}
 				})
@@ -1487,6 +1491,9 @@ func TestCreateComment(t *testing.T) {
 			updateMarker: "MARKER",
 			setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
 				t.Helper()
+				mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
+					fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
+				})
 				mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
 					if r.Method == http.MethodGet {
 						fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`)
@@ -1498,18 +1505,54 @@ func TestCreateComment(t *testing.T) {
 				return nil
 			},
 		},
+		{
+			name:         "skip comment from different user and create new",
+			event:        &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
+			updateMarker: "MARKER",
+			commentBody:  "New Comment MARKER",
+			setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
+				t.Helper()
+				editEndpointCalled := false
+				commentCreated := false
+				mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
+					fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
+				})
+				mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
+					if r.Method == http.MethodGet {
+						fmt.Fprint(rw, `[{"id": 555, "body": "Old MARKER", "user": {"id": 999, "login": "other-user"}}]`)
+						return
+					}
+					assert.Equal(t, r.Method, http.MethodPost)
+					commentCreated = true
+					rw.WriteHeader(http.StatusCreated)
+					fmt.Fprint(rw, `{"id": 556}`)
+				})
+				mux.HandleFunc("/repos/org/repo/issues/comments/555", func(rw http.ResponseWriter, _ *http.Request) {
+					editEndpointCalled = true
+					t.Error("should not edit comment from different user")
+					rw.WriteHeader(http.StatusOK)
+				})
+				return func(t *testing.T) {
+					t.Helper()
+					assert.Assert(t, !editEndpointCalled, "edit endpoint should not be called for comment from different user")
+					assert.Assert(t, commentCreated, "new comment should be created")
+				}
+			},
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx, _ := rtesting.SetupFakeContext(t)
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			fakelogger := zap.New(observer).Sugar()
 
 			var provider *Provider
 			var postAssert func(t *testing.T)
 			if !tt.clientNil {
 				fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
 				defer teardown()
-				provider = &Provider{ghClient: fakeclient}
+				provider = &Provider{ghClient: fakeclient, Logger: fakelogger}
 
 				if tt.setup != nil {
 					postAssert = tt.setup(t, mux)
@@ -1577,9 +1620,12 @@ func TestCreateCommentDedupLogging(t *testing.T) {
 			updateMarker: "MARKER",
 			setup: func(t *testing.T, mux *http.ServeMux) {
 				t.Helper()
+				mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
+					fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
+				})
 				mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
 					assert.Equal(t, r.Method, http.MethodGet)
-					fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z"}]`)
+					fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z", "user": {"id": 100, "login": "pac-user"}}]`)
 				})
 				mux.HandleFunc("/repos/org/repo/issues/comments/111", func(rw http.ResponseWriter, r *http.Request) {
 					assert.Equal(t, r.Method, http.MethodPatch)
@@ -1598,9 +1644,12 @@ func TestCreateCommentDedupLogging(t *testing.T) {
 			updateMarker: "MARKER",
 			setup: func(t *testing.T, mux *http.ServeMux) {
 				t.Helper()
+				mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
+					fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
+				})
 				mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
 					assert.Equal(t, r.Method, http.MethodGet)
-					fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z"}, {"id": 222, "body": "MARKER old", "created_at": "2024-01-01T00:01:00Z"}]`)
+					fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z", "user": {"id": 100, "login": "pac-user"}}, {"id": 222, "body": "MARKER old", "created_at": "2024-01-01T00:01:00Z", "user": {"id": 100, "login": "pac-user"}}]`)
 				})
 				mux.HandleFunc("/repos/org/repo/issues/comments/111", func(rw http.ResponseWriter, r *http.Request) {
 					assert.Equal(t, r.Method, http.MethodPatch)
@@ -1905,3 +1954,135 @@ func TestSkipPushEventForPRCommits(t *testing.T) {
 		})
 	}
 }
+
+func TestFetchAppSlug(t *testing.T) {
+	testAppID := int64(12345)
+	validSlug := "my-github-app"
+
+	tests := []struct {
+		name             string
+		privateKey       []byte
+		applicationID    int64
+		setupMux         func(mux *http.ServeMux)
+		wantSlug         string
+		wantErrSubstring string
+	}{
+		{
+			name:          "success fetch app slug",
+			privateKey:    []byte(fakePrivateKey),
+			applicationID: testAppID,
+			setupMux: func(mux *http.ServeMux) {
+				mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
+					_, _ = fmt.Fprintf(w, `{"slug": "%s", "name": "My GitHub App"}`, validSlug)
+				})
+			},
+			wantSlug: validSlug,
+		},
+		{
+			name:          "app endpoint returns 404",
+			privateKey:    []byte(fakePrivateKey),
+			applicationID: testAppID,
+			setupMux: func(mux *http.ServeMux) {
+				mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(http.StatusNotFound)
+					_, _ = fmt.Fprint(w, `{"message": "Not Found"}`)
+				})
+			},
+			wantErrSubstring: "failed to get app info",
+		},
+		{
+			name:             "invalid private key",
+			privateKey:       []byte("invalid-key"),
+			applicationID:    testAppID,
+			setupMux:         func(_ *http.ServeMux) {},
+			wantErrSubstring: "failed to parse private key",
+		},
+		{
+			name:          "app returns empty slug",
+			privateKey:    []byte(fakePrivateKey),
+			applicationID: testAppID,
+			setupMux: func(mux *http.ServeMux) {
+				mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
+					_, _ = fmt.Fprint(w, `{"slug": "", "name": "My GitHub App"}`)
+				})
+			},
+			wantSlug: "",
+		},
+		{
+			name:          "app endpoint returns malformed JSON",
+			privateKey:    []byte(fakePrivateKey),
+			applicationID: testAppID,
+			setupMux: func(mux *http.ServeMux) {
+				mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
+					_, _ = fmt.Fprint(w, `{invalid json`)
+				})
+			},
+			wantErrSubstring: "failed to get app info",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, mux, serverURL, teardown := ghtesthelper.SetupGH()
+			defer teardown()
+
+			if tt.setupMux != nil {
+				tt.setupMux(mux)
+			}
+
+			// Set up context with namespace
+			ctx, _ := rtesting.SetupFakeContext(t)
+			testNamespace := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-namespace",
+				},
+			}
+			ctx = info.StoreNS(ctx, testNamespace.GetName())
+
+			// Create a secret with the test application ID and private key
+			appIDBytes := make([]byte, 0, 20)
+			appIDBytes = fmt.Appendf(appIDBytes, "%d", tt.applicationID)
+			testSecret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pac-secret",
+					Namespace: testNamespace.GetName(),
+				},
+				Data: map[string][]byte{
+					"github-application-id": appIDBytes,
+					"github-private-key":    tt.privateKey,
+				},
+			}
+
+			// Set up test data with kubernetes client
+			tdata := testclient.Data{
+				Namespaces: []*corev1.Namespace{testNamespace},
+				Secret:     []*corev1.Secret{testSecret},
+			}
+			stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+
+			// Create a provider with mocked kubernetes client
+			provider := &Provider{}
+			provider.Run = &params.Run{
+				Clients: clients.Clients{
+					Kube: stdata.Kube,
+				},
+				Info: info.Info{
+					Controller: &info.ControllerInfo{
+						Secret: testSecret.GetName(),
+					},
+				},
+			}
+
+			slug, err := provider.fetchAppSlug(ctx, serverURL)
+
+			if tt.wantErrSubstring != "" {
+				assert.Assert(t, err != nil, "expected error but got none")
+				assert.ErrorContains(t, err, tt.wantErrSubstring)
+				return
+			}
+
+			assert.NilError(t, err)
+			assert.Equal(t, slug, tt.wantSlug)
+		})
+	}
+}
