From 1889b1531c97c7892349a4d6069df080e28e63f0 Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Wed, 25 Mar 2026 11:47:38 +0200 Subject: [PATCH 1/4] Fix input validation in PR details and issue creation endpoints --- server/plugin/api.go | 33 ++++++++++++++++++++++----- server/plugin/api_test.go | 47 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/server/plugin/api.go b/server/plugin/api.go index 52ae04105..847f0b3f8 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -765,6 +765,13 @@ func (p *Plugin) getPrsDetails(c *UserContext, w http.ResponseWriter, r *http.Re return } + for _, pr := range prList { + if _, _, err := getRepoOwnerAndNameFromURL(pr.URL); err != nil { + p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "invalid PR URL: " + pr.URL, StatusCode: http.StatusBadRequest}) + return + } + } + prDetails := make([]*PRDetails, len(prList)) var wg sync.WaitGroup for i, pr := range prList { @@ -786,7 +793,16 @@ func (p *Plugin) fetchPRDetails(c *UserContext, client *github.Client, prURL str requestedReviewers := []*string{} reviewsList := []*github.PullRequestReview{} - repoOwner, repoName := getRepoOwnerAndNameFromURL(prURL) + repoOwner, repoName, err := getRepoOwnerAndNameFromURL(prURL) + if err != nil { + c.Log.WithError(err).Warnf("Invalid PR URL") + return &PRDetails{ + URL: prURL, + Number: prNumber, + RequestedReviewers: requestedReviewers, + Reviews: reviewsList, + } + } var wg sync.WaitGroup @@ -841,9 +857,12 @@ func fetchReviews(c *UserContext, client *github.Client, repoOwner string, repoN return reviewsList, nil } -func getRepoOwnerAndNameFromURL(url string) (string, string) { +func getRepoOwnerAndNameFromURL(url string) (string, string, error) { splitted := strings.Split(url, "/") - return splitted[len(splitted)-2], splitted[len(splitted)-1] + if len(splitted) < 2 { + return "", "", fmt.Errorf("invalid URL %q: expected at least owner/repo", url) + } + return splitted[len(splitted)-2], splitted[len(splitted)-1], nil } func (p *Plugin) searchIssues(c *UserContext, w http.ResponseWriter, r *http.Request) { @@ -1724,9 +1743,11 @@ func (p *Plugin) createIssue(c *UserContext, w http.ResponseWriter, r *http.Requ return } - splittedRepo := strings.Split(issue.Repo, "/") - owner := splittedRepo[0] - repoName := splittedRepo[1] + owner, repoName, err := parseRepo(issue.Repo) + if err != nil { + p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "invalid repository: " + issue.Repo, StatusCode: http.StatusBadRequest}) + return + } githubClient := p.githubConnectUser(c.Ctx, c.GHInfo) var resp *github.Response diff --git a/server/plugin/api_test.go b/server/plugin/api_test.go index 4b662c4cc..5bd1ee180 100644 --- a/server/plugin/api_test.go +++ b/server/plugin/api_test.go @@ -514,6 +514,53 @@ func TestParseRepo(t *testing.T) { } } +func TestGetRepoOwnerAndNameFromURL(t *testing.T) { + tests := []struct { + name string + url string + expectedOwner string + expectedRepo string + expectError bool + }{ + { + name: "Valid full GitHub URL", + url: "https://api.github.com/repos/owner/repo/pulls/1", + expectedOwner: "pulls", + expectedRepo: "1", + }, + { + name: "Simple owner/repo", + url: "owner/repo", + expectedOwner: "owner", + expectedRepo: "repo", + }, + { + name: "No slashes - should error", + url: "crash", + expectError: true, + }, + { + name: "Empty string - should error", + url: "", + expectError: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + owner, repo, err := getRepoOwnerAndNameFromURL(tc.url) + if tc.expectError { + assert.Error(t, err) + assert.Empty(t, owner) + assert.Empty(t, repo) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expectedOwner, owner) + assert.Equal(t, tc.expectedRepo, repo) + } + }) + } +} + func TestUpdateSettings(t *testing.T) { mockKvStore, mockAPI, mockLogger, mockLoggerWith, _ := GetTestSetup(t) p := getPluginTest(mockAPI, mockKvStore) From 7e8757f9d1aa2eab1c172a637b93ffffad6495b5 Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Wed, 25 Mar 2026 12:02:20 +0200 Subject: [PATCH 2/4] Coderabbit review --- server/plugin/api.go | 32 +++++++++++++++++++++++++++----- server/plugin/api_test.go | 23 ++++++++++++++++++++--- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/server/plugin/api.go b/server/plugin/api.go index 847f0b3f8..bfbf977fa 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -857,12 +857,34 @@ func fetchReviews(c *UserContext, client *github.Client, repoOwner string, repoN return reviewsList, nil } -func getRepoOwnerAndNameFromURL(url string) (string, string, error) { - splitted := strings.Split(url, "/") - if len(splitted) < 2 { - return "", "", fmt.Errorf("invalid URL %q: expected at least owner/repo", url) +func getRepoOwnerAndNameFromURL(rawURL string) (string, string, error) { + if rawURL == "" { + return "", "", fmt.Errorf("URL must not be empty") } - return splitted[len(splitted)-2], splitted[len(splitted)-1], nil + + var segments []string + if parsed, err := url.Parse(rawURL); err == nil && parsed.Scheme != "" { + segments = strings.Split(strings.Trim(parsed.Path, "/"), "/") + } else { + segments = strings.Split(strings.Trim(rawURL, "/"), "/") + } + + hasReposSegment := false + for i, seg := range segments { + if seg == "repos" { + hasReposSegment = true + if i+2 < len(segments) { + return segments[i+1], segments[i+2], nil + } + break + } + } + + if !hasReposSegment && len(segments) == 2 && segments[0] != "" && segments[1] != "" { + return segments[0], segments[1], nil + } + + return "", "", fmt.Errorf("invalid repository URL %q: expected owner/repo or a GitHub API URL containing /repos/owner/repo", rawURL) } func (p *Plugin) searchIssues(c *UserContext, w http.ResponseWriter, r *http.Request) { diff --git a/server/plugin/api_test.go b/server/plugin/api_test.go index 5bd1ee180..a3316119c 100644 --- a/server/plugin/api_test.go +++ b/server/plugin/api_test.go @@ -523,10 +523,16 @@ func TestGetRepoOwnerAndNameFromURL(t *testing.T) { expectError bool }{ { - name: "Valid full GitHub URL", + name: "GitHub API repository URL", + url: "https://api.github.com/repos/owner/repo", + expectedOwner: "owner", + expectedRepo: "repo", + }, + { + name: "GitHub API repository URL with trailing path", url: "https://api.github.com/repos/owner/repo/pulls/1", - expectedOwner: "pulls", - expectedRepo: "1", + expectedOwner: "owner", + expectedRepo: "repo", }, { name: "Simple owner/repo", @@ -544,6 +550,17 @@ func TestGetRepoOwnerAndNameFromURL(t *testing.T) { url: "", expectError: true, }, + { + name: "Single segment path - should error", + url: "https://api.github.com/repos/owner", + expectError: true, + }, + { + name: "Enterprise GitHub API URL", + url: "https://github.example.com/api/v3/repos/myorg/myrepo", + expectedOwner: "myorg", + expectedRepo: "myrepo", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { From 4ad4b8556e2b53291a362b0b9cfd7a35029150b0 Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Thu, 26 Mar 2026 12:49:26 +0200 Subject: [PATCH 3/4] PR feedback --- server/plugin/api.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/server/plugin/api.go b/server/plugin/api.go index bfbf977fa..c3eb8ee32 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -765,16 +765,18 @@ func (p *Plugin) getPrsDetails(c *UserContext, w http.ResponseWriter, r *http.Re return } + var validPRs []*PRDetails for _, pr := range prList { if _, _, err := getRepoOwnerAndNameFromURL(pr.URL); err != nil { - p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "invalid PR URL: " + pr.URL, StatusCode: http.StatusBadRequest}) - return + c.Log.WithError(err).Warnf("Skipping PR with invalid URL") + continue } + validPRs = append(validPRs, pr) } - prDetails := make([]*PRDetails, len(prList)) + prDetails := make([]*PRDetails, len(validPRs)) var wg sync.WaitGroup - for i, pr := range prList { + for i, pr := range validPRs { wg.Go(func() { prDetail := p.fetchPRDetails(c, githubClient, pr.URL, pr.Number) prDetails[i] = prDetail From 23b6e3d531bde622e6434e4bd255acd98bb7542c Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Thu, 26 Mar 2026 18:39:57 +0200 Subject: [PATCH 4/4] Harden input validation for edge cases Made-with: Cursor --- server/plugin/api.go | 7 +++++-- server/plugin/api_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/server/plugin/api.go b/server/plugin/api.go index c3eb8ee32..50ce2a3e5 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -767,6 +767,9 @@ func (p *Plugin) getPrsDetails(c *UserContext, w http.ResponseWriter, r *http.Re var validPRs []*PRDetails for _, pr := range prList { + if pr == nil { + continue + } if _, _, err := getRepoOwnerAndNameFromURL(pr.URL); err != nil { c.Log.WithError(err).Warnf("Skipping PR with invalid URL") continue @@ -875,7 +878,7 @@ func getRepoOwnerAndNameFromURL(rawURL string) (string, string, error) { for i, seg := range segments { if seg == "repos" { hasReposSegment = true - if i+2 < len(segments) { + if i+2 < len(segments) && segments[i+1] != "" && segments[i+2] != "" { return segments[i+1], segments[i+2], nil } break @@ -1864,7 +1867,7 @@ func parseRepo(repoParam string) (owner, repo string, err error) { } splitted := strings.Split(repoParam, "/") - if len(splitted) != 2 { + if len(splitted) != 2 || splitted[0] == "" || splitted[1] == "" { return "", "", errors.New("invalid repository") } diff --git a/server/plugin/api_test.go b/server/plugin/api_test.go index a3316119c..024897483 100644 --- a/server/plugin/api_test.go +++ b/server/plugin/api_test.go @@ -502,6 +502,26 @@ func TestParseRepo(t *testing.T) { assert.EqualError(t, err, "invalid repository") }, }, + { + name: "Empty owner", + repoParam: "/repo", + setup: func() {}, + assertions: func(t *testing.T, owner, repo string, err error) { + assert.Equal(t, "", owner) + assert.Equal(t, "", repo) + assert.EqualError(t, err, "invalid repository") + }, + }, + { + name: "Empty repo", + repoParam: "owner/", + setup: func() {}, + assertions: func(t *testing.T, owner, repo string, err error) { + assert.Equal(t, "", owner) + assert.Equal(t, "", repo) + assert.EqualError(t, err, "invalid repository") + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -561,6 +581,16 @@ func TestGetRepoOwnerAndNameFromURL(t *testing.T) { expectedOwner: "myorg", expectedRepo: "myrepo", }, + { + name: "Empty owner after repos segment", + url: "https://api.github.com/repos//repo", + expectError: true, + }, + { + name: "Empty repo after repos segment", + url: "https://api.github.com/repos/owner/", + expectError: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {