MM-67866 Improving connection logic#980
Merged
avasconcelos114 merged 3 commits intomasterfrom Mar 18, 2026
Merged
Conversation
avasconcelos114
added
2: Dev Review
|
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 (3)
📝 WalkthroughWalkthroughAdds OAuth scope parsing and validation to the GitHub OAuth flow, enforces the EnablePrivateRepo setting when "private" is requested, extends GitHub user info struct, and adds unit tests covering scope parsing, validation, and the private-repo-disabled API path. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Server API
participant GitHub as GitHub OAuth
Client->>API: GET /oauth/connect?private=true
API->>API: Check EnablePrivateRepo flag
alt Private Repos Disabled
API-->>Client: 403 Forbidden (private repos disabled)
else Private Repos Enabled
API->>GitHub: Exchange code / Fetch authenticated user
GitHub-->>API: User data + X-OAuth-Scopes header
API->>API: parseScopes() -> validateOAuthScopes(resp, privateAllowed)
alt Scopes Valid
API-->>Client: 200 OK (authentication complete)
else Scopes Invalid
API-->>Client: 403 Forbidden (scope validation failed)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (3)
server/plugin/plugin.go (2)
612-616: Useslices.Containsfor cleaner iteration.The static analysis tool flagged this loop as simplifiable. Since the
slicespackage is already imported, this can be modernized.♻️ Suggested refactor
- for _, s := range granted { - if s == string(github.ScopeRepo) { - return fmt.Errorf("token was granted %q scope but only %q was requested", github.ScopeRepo, github.ScopePublicRepo) - } - } + if slices.Contains(granted, string(github.ScopeRepo)) { + return fmt.Errorf("token was granted %q scope but only %q was requested", github.ScopeRepo, github.ScopePublicRepo) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/plugin.go` around lines 612 - 616, Replace the manual loop that checks for github.ScopeRepo in the granted slice with slices.Contains to simplify the check: in the block that iterates over granted and compares s == string(github.ScopeRepo), use slices.Contains(granted, string(github.ScopeRepo)) and keep the existing error return (fmt.Errorf("token was granted %q scope but only %q was requested", github.ScopeRepo, github.ScopePublicRepo)) when true; no other logic changes required.
621-630: Usestrings.SplitSeqfor more efficient ranging.The static analysis tool flagged that ranging over
SplitSeqis more efficient than splitting into a slice first.♻️ Suggested refactor
func parseScopes(header string) []string { var scopes []string - for _, s := range strings.Split(header, ",") { + for s := range strings.SplitSeq(header, ",") { s = strings.TrimSpace(s) if s != "" { scopes = append(scopes, s) } } return scopes }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/plugin.go` around lines 621 - 630, The parseScopes function currently builds a full slice via strings.Split then ranges it; change it to iterate the strings.SplitSeq for the header instead to avoid allocating the intermediate slice. Replace the strings.Split(header, ",") loop in parseScopes with a loop over strings.SplitSeq(header, ",") (using the sequence's iteration API to obtain each chunk), call strings.TrimSpace on each chunk, and append non-empty results to scopes so the function behavior remains the same while avoiding the temporary slice allocation.server/plugin/api.go (1)
432-444: Good security enhancement with OAuth scope validation.The scope validation ensures tokens don't have broader permissions than requested, preventing potential privilege escalation. The error handling and user messaging are clear.
One minor note: the static analysis flagged that
errat line 439 shadows the declaration at line 389. While this is functionally correct (the shadowed variable is from a different scope), you could rename it for clarity.♻️ Optional: Rename shadowed variable for clarity
- if err := p.validateOAuthScopes(resp, state.PrivateAllowed); err != nil { - c.Log.WithError(err).Warnf("Mismatching OAuth scopes, rejecting connection") + if scopeErr := p.validateOAuthScopes(resp, state.PrivateAllowed); scopeErr != nil { + c.Log.WithError(scopeErr).Warnf("Mismatching OAuth scopes, rejecting connection") rErr = errors.New("OAuth token scope does not match the requested permissions") p.writeAPIError(w, &APIErrorResponse{Message: "OAuth token scope does not match the requested permissions. Please reconnect your GitHub account.", StatusCode: http.StatusForbidden}) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/api.go` around lines 432 - 444, The variable err returned by githubClient.Users.Get shadows an earlier err; rename this inner error variable to something non-shadowing (e.g., getUserErr) in the block that calls githubClient.Users.Get and update the subsequent error checks and logging (c.Log.WithError and any references) to use that new name; ensure you similarly rename the err used in the validateOAuthScopes check if it conflicts, keeping calls to validateOAuthScopes, p.writeAPIError, rErr assignment, gitUser and resp unchanged.
🤖 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/utils_test.go`:
- Around line 322-328: The struct literal assigned to the variable "tests" in
utils_test.go is misformatted; run "gofmt -w server/plugin/utils_test.go" (or
apply gofmt formatting) to fix spacing/alignment for the fields (name, resp,
privateAllowed, enablePrivateRepo, expectError) so the file compiles and the
pipeline passes; ensure tabs/spaces and field alignment match gofmt's output for
the "tests" declaration and save the file.
---
Nitpick comments:
In `@server/plugin/api.go`:
- Around line 432-444: The variable err returned by githubClient.Users.Get
shadows an earlier err; rename this inner error variable to something
non-shadowing (e.g., getUserErr) in the block that calls githubClient.Users.Get
and update the subsequent error checks and logging (c.Log.WithError and any
references) to use that new name; ensure you similarly rename the err used in
the validateOAuthScopes check if it conflicts, keeping calls to
validateOAuthScopes, p.writeAPIError, rErr assignment, gitUser and resp
unchanged.
In `@server/plugin/plugin.go`:
- Around line 612-616: Replace the manual loop that checks for github.ScopeRepo
in the granted slice with slices.Contains to simplify the check: in the block
that iterates over granted and compares s == string(github.ScopeRepo), use
slices.Contains(granted, string(github.ScopeRepo)) and keep the existing error
return (fmt.Errorf("token was granted %q scope but only %q was requested",
github.ScopeRepo, github.ScopePublicRepo)) when true; no other logic changes
required.
- Around line 621-630: The parseScopes function currently builds a full slice
via strings.Split then ranges it; change it to iterate the strings.SplitSeq for
the header instead to avoid allocating the intermediate slice. Replace the
strings.Split(header, ",") loop in parseScopes with a loop over
strings.SplitSeq(header, ",") (using the sequence's iteration API to obtain each
chunk), call strings.TrimSpace on each chunk, and append non-empty results to
scopes so the function behavior remains the same while avoiding the temporary
slice allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3216030c-c755-4531-ac80-b5537aad36fa
📒 Files selected for processing (5)
server/plugin/api.goserver/plugin/api_test.goserver/plugin/command.goserver/plugin/plugin.goserver/plugin/utils_test.go
edgarbellot
requested review from
hmohammed-prodsec
and removed request for
edgarbellot
hmohammed-prodsec
approved these changes
Mar 18, 2026
hmohammed-prodsec
removed
the
3: Security Review
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
3 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
This PR refactors part of the oauth callback flows to better match plugin configurations
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-67866
Change Impact: 🔴 High
Reasoning: The PR alters OAuth callback handling and adds OAuth scope validation in the authentication flow, and updates the GitHubUserInfo struct — changes that affect core authentication/authorization and public data structures used across the plugin.
Regression Risk: High — introducing stricter scope validation can block legitimate connections; modifications to a public struct increase risk of incompatibilities and ripple effects. While unit tests were added, broader end-to-end coverage with real GitHub OAuth responses is limited.
QA Recommendation: Perform thorough manual QA of end-to-end OAuth flows (public-only and private repo scenarios), validate behavior with existing tokens of varied scopes, and test plugin upgrade paths for stored user info. Manual QA is strongly recommended and skipping it is not advised.
Generated by CodeRabbitAI