diff --git a/server/plugin/api.go b/server/plugin/api.go index 52ae04105..50ce2a3e5 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -765,9 +765,21 @@ func (p *Plugin) getPrsDetails(c *UserContext, w http.ResponseWriter, r *http.Re return } - prDetails := make([]*PRDetails, len(prList)) + 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 + } + validPRs = append(validPRs, pr) + } + + 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 @@ -786,7 +798,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 +862,34 @@ func fetchReviews(c *UserContext, client *github.Client, repoOwner string, repoN return reviewsList, nil } -func getRepoOwnerAndNameFromURL(url string) (string, string) { - splitted := strings.Split(url, "/") - return splitted[len(splitted)-2], splitted[len(splitted)-1] +func getRepoOwnerAndNameFromURL(rawURL string) (string, string, error) { + if rawURL == "" { + return "", "", fmt.Errorf("URL must not be empty") + } + + 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) && segments[i+1] != "" && segments[i+2] != "" { + 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) { @@ -1724,9 +1770,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 @@ -1819,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 4b662c4cc..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) { @@ -514,6 +534,80 @@ func TestParseRepo(t *testing.T) { } } +func TestGetRepoOwnerAndNameFromURL(t *testing.T) { + tests := []struct { + name string + url string + expectedOwner string + expectedRepo string + expectError bool + }{ + { + 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: "owner", + expectedRepo: "repo", + }, + { + 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, + }, + { + 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", + }, + { + 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) { + 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)