Fix input validation in PR details and issue creation endpoints#988
Open
nevyangelova wants to merge 4 commits intomasterfrom
Open
Fix input validation in PR details and issue creation endpoints#988nevyangelova wants to merge 4 commits intomasterfrom
nevyangelova wants to merge 4 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughgetPrsDetails now filters out nil or unparsable PR entries; fetchPRDetails returns partial PRDetails and skips GitHub calls when URL parsing fails; getRepoOwnerAndNameFromURL now returns an error and uses URL-aware parsing; createIssue validates via parseRepo and returns 400 on invalid repo; tests added for URL parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server\n(plugin/api.go)
participant GH as GitHub API
Client->>Server: Request PR details (list of PRs)
Server->>Server: validate PR list, drop nil entries
alt PR URL parses
Server->>GH: fetch PR reviews & reviewers
GH-->>Server: return reviews & reviewers
Server-->>Client: include full PRDetails
else PR URL invalid
Server->>Server: log warning, build partial PRDetails (URL, Number, empty slices)
Server-->>Client: include partial PRDetails (no GH calls)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/api_test.go`:
- Around line 525-530: The test case "Valid full GitHub URL" asserts wrong
owner/repo values for the URL "https://api.github.com/repos/owner/repo/pulls/1";
update the test to expect expectedOwner="owner" and expectedRepo="repo" and
ensure the production function getRepoOwnerAndNameFromURL (used by
fetchPRDetails) is changed to parse the path segment pattern
"/repos/{owner}/{repo}/..." (i.e., find the "repos" segment and take the next
two segments as owner and repo) instead of returning the last two path segments;
adjust the test vector and confirm fetchPRDetails uses the corrected
getRepoOwnerAndNameFromURL behavior.
In `@server/plugin/api.go`:
- Around line 860-866: getRepoOwnerAndNameFromURL currently just returns the
last two path segments which breaks for full GitHub API URLs; update it to parse
the input as a URL (using net/url), split the path into segments, and if the
path contains a "repos" segment, take the two segments immediately after "repos"
as owner and repo; otherwise, if the path is just "owner/repo" (two segments),
return those; return an error if neither pattern matches. Ensure you update
references to getRepoOwnerAndNameFromURL accordingly and include clear error
messages when rejecting inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 506415de-16f9-46e3-9774-6c006d2438e4
📒 Files selected for processing (2)
server/plugin/api.goserver/plugin/api_test.go
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nevyangelova
added
2: Dev Review
Contributor
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Great improvement to the parsing of the repo owner and names! Only have one question about the API behavior, let me know what you think :)
server/plugin/api.go
Outdated
| 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 |
Contributor
There was a problem hiding this comment.
Looks like this early return causes the entire request to be treated as an error if a single item in the prList isn't valid
I'm wondering whether just excluding the invalid items from the response would be preferable from a UX perspective while still being able to address the cause of the issue
Contributor
Author
|
@avasconcelos114 thank you for the good suggestion, updated |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/api.go`:
- Around line 769-775: The loop over prList dereferences pr.URL without checking
for nil, which can panic for payloads like [null]; update the loop that builds
validPRs to first check that pr != nil (and optionally log a warning with c.Log)
before calling getRepoOwnerAndNameFromURL(pr.URL), and continue on nil entries
so only non-nil PRs are validated and appended to validPRs.
- Around line 1770-1774: The parseRepo function currently only checks the split
length which allows strings like "owner/" or "/repo" to be treated as valid;
update parseRepo to validate that both returned parts (owner and repo) are
non-empty and return a descriptive error when either is empty so createIssue's
error path triggers correctly; locate and modify parseRepo to trim whitespace,
split by '/', ensure len(parts)==2 and parts[0] != "" and parts[1] != "", and
return an error message such as "invalid repository: missing owner or repo" when
the check fails.
- Around line 875-880: The parser currently returns segments[i+1] and
segments[i+2] immediately when it sees the "repos" segment; update that logic to
validate that both segments[i+1] (owner) and segments[i+2] (repo) are non-empty
before returning. In the loop that sets hasReposSegment and returns owner/repo,
add checks for empty strings (and return an appropriate error) so inputs like
"/repos//repo/..." or "/repos/owner//..." are rejected rather than propagated
downstream; keep the existing bounds check (i+2 < len(segments)) and only return
after both content checks pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add bounds checking to getRepoOwnerAndNameFromURL to prevent potential runtime panic from malformed input. Validate PR URLs in getPrsDetails handler before processing. Replace manual string splitting in createIssue with the existing parseRepo function which already performs proper validation.
QA Steps
PR details endpoint handles invalid input gracefully
Issue creation handles invalid input gracefully
Ticket Link
https://mattermost.atlassian.net/browse/MM-68027
Change Impact: 🟡 Medium
Reasoning: The PR hardens input validation for GitHub repo/PR URL parsing and updates handlers to bail out or return errors on malformed inputs; changes are limited to API handlers and parsing helpers but modify handler behavior and a helper signature.
Regression Risk: Moderate — altering a shared helper's behavior and handler control flow can change responses for malformed inputs and could affect callers relying on previous permissive parsing; tests for the new helper were added but handler-level coverage remains limited.
** QA Recommendation:** Manual QA recommended: verify PR details load valid PRs and gracefully skip/log invalid PR URLs (no panics), and confirm
/github issue createsucceeds for valid repos and returns clear 400 errors for invalid repo strings. Skipping manual QA carries moderate risk.Generated by CodeRabbitAI