Fix input validation in PR details and issue creation endpoints (#988)

* Fix input validation in PR details and issue creation endpoints

* Coderabbit review

* PR feedback

* Harden input validation for edge cases

Made-with: Cursor

---------

Co-authored-by: Nevyana Angelova <nevyangelova@192.168.100.29>

Author: nevyangelova

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) {
@@ -1726,9 +1772,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
@@ -1821,7 +1869,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")
 	}
 

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)