[MM-60676] Fix incorrect callback URL in setup flow (#827)

* Fix incorrect callback URL in setup flow * Use url.JoinPath to build URL paths * Fix plugin URL
mattermost · Oct 17, 2024 · af478ac · af478ac
1 parent 6ba0bcd
commit af478ac
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 54 deletions.
diff --git a/server/plugin/api.go b/server/plugin/api.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 	"runtime/debug"
 	"strconv"
 	"strings"
@@ -289,15 +290,20 @@ func (p *Plugin) connectUserToGitHub(c *Context, w http.ResponseWriter, r *http.
 		privateAllowed = true
 	}
 
-	conf := p.getOAuthConfig(privateAllowed)
+	conf, err := p.getOAuthConfig(privateAllowed)
+	if err != nil {
+		c.Log.WithError(err).Warnf("Failed to generate OAuthConfig")
+		http.Error(w, "error generating OAuthConfig", http.StatusBadRequest)
+		return
+	}
 
 	state := OAuthState{
 		UserID:         c.UserID,
 		Token:          model.NewId()[:15],
 		PrivateAllowed: privateAllowed,
 	}
 
-	_, err := p.store.Set(githubOauthKey+state.Token, state, pluginapi.SetExpiry(TokenTTL))
+	_, err = p.store.Set(githubOauthKey+state.Token, state, pluginapi.SetExpiry(TokenTTL))
 	if err != nil {
 		c.Log.WithError(err).Warnf("error occurred while trying to store oauth state into KV store")
 		p.writeAPIError(w, &APIErrorResponse{Message: "error saving the oauth state", StatusCode: http.StatusInternalServerError})
@@ -379,7 +385,12 @@ func (p *Plugin) completeConnectUserToGitHub(c *Context, w http.ResponseWriter,
 		return
 	}
 
-	conf := p.getOAuthConfig(state.PrivateAllowed)
+	conf, err := p.getOAuthConfig(state.PrivateAllowed)
+	if err != nil {
+		c.Log.WithError(err).Warnf("Failed to generate OAuthConfig")
+		http.Error(w, "error generating OAuthConfig", http.StatusBadRequest)
+		return
+	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), oauthCompleteTimeout)
 	defer cancel()
@@ -819,10 +830,18 @@ func (p *Plugin) searchIssues(c *UserContext, w http.ResponseWriter, r *http.Req
 	p.writeJSON(w, allIssues)
 }
 
-func (p *Plugin) getPermaLink(postID string) string {
-	siteURL := *p.client.Configuration.GetConfig().ServiceSettings.SiteURL
+func (p *Plugin) getPermaLink(postID string) (string, error) {
+	siteURL, err := getSiteURL(p.client)
+	if err != nil {
+		return "", err
+	}
 
-	return fmt.Sprintf("%v/_redirect/pl/%v", siteURL, postID)
+	redirectURL, err := url.JoinPath(siteURL, "_redirect", "pl", postID)
+	if err != nil {
+		return "", errors.Wrap(err, "failed to build pluginURL")
+	}
+
+	return redirectURL, nil
 }
 
 func getFailReason(code int, repo string, username string) string {
@@ -904,7 +923,11 @@ func (p *Plugin) createIssueComment(c *UserContext, w http.ResponseWriter, r *ht
 	}
 
 	currentUsername := c.GHInfo.GitHubUsername
-	permalink := p.getPermaLink(req.PostID)
+	permalink, err := p.getPermaLink(req.PostID)
+	if err != nil {
+		p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "failed to generate permalink", StatusCode: http.StatusInternalServerError})
+		return
+	}
 	permalinkMessage := fmt.Sprintf("*@%s attached a* [message](%s) *from %s*\n\n", currentUsername, permalink, commentUsername)
 
 	req.Comment = permalinkMessage + req.Comment
@@ -1347,7 +1370,11 @@ func (p *Plugin) createIssue(c *UserContext, w http.ResponseWriter, r *http.Requ
 			return
 		}
 
-		permalink = p.getPermaLink(issue.PostID)
+		permalink, err = p.getPermaLink(issue.PostID)
+		if err != nil {
+			p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "failed to generate permalink", StatusCode: http.StatusInternalServerError})
+			return
+		}
 
 		mmMessage = fmt.Sprintf("_Issue created from a [Mattermost message](%v) *by %s*._", permalink, username)
 	}

diff --git a/server/plugin/command.go b/server/plugin/command.go
@@ -337,7 +337,10 @@ func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClien
 	opt := &github.ListOptions{
 		PerPage: PerPageValue,
 	}
-	siteURL := *p.client.Configuration.GetConfig().ServiceSettings.SiteURL
+	siteURL, err := getSiteURL(p.client)
+	if err != nil {
+		return false, err
+	}
 
 	for {
 		var githubHooks []*github.Hook
@@ -804,9 +807,9 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*mo
 	}
 
 	if action == "connect" {
-		siteURL := p.client.Configuration.GetConfig().ServiceSettings.SiteURL
-		if siteURL == nil {
-			p.postCommandResponse(args, "Encountered an error connecting to GitHub.")
+		connectURL, err := buildPluginURL(p.client, "oauth", "connect")
+		if err != nil {
+			p.postCommandResponse(args, fmt.Sprintf("Encountered an error connecting to GitHub: %s", err.Error()))
 			return &model.CommandResponse{}, nil
 		}
 
@@ -834,7 +837,7 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*mo
 			qparams = "?private=true"
 		}
 
-		msg := fmt.Sprintf("[Click here to link your GitHub account.](%s/plugins/%s/oauth/connect%s)", *siteURL, Manifest.Id, qparams)
+		msg := fmt.Sprintf("[Click here to link your GitHub account.](%s%s)", connectURL, qparams)
 		p.postCommandResponse(args, msg)
 		return &model.CommandResponse{}, nil
 	}

diff --git a/server/plugin/flows.go b/server/plugin/flows.go
@@ -486,6 +486,11 @@ func (fm *FlowManager) submitEnterpriseConfig(f *flow.Flow, submitted map[string
 }
 
 func (fm *FlowManager) stepOAuthInfo() flow.Step {
+	callbackURL, err := buildPluginURL(fm.client, "oauth", "complete")
+	if err != nil {
+		fm.client.Log.Warn("Failed to build callbackURL", "err", err)
+	}
+
 	oauthPretext := `
 ##### :white_check_mark: Step 1: Register an OAuth Application in GitHub
 You must first register the Mattermost GitHub Plugin as an authorized OAuth app.`
@@ -494,11 +499,11 @@ You must first register the Mattermost GitHub Plugin as an authorized OAuth app.
 		"2. Set the following values:\n"+
 		"	- Application name: `Mattermost GitHub Plugin - <your company name>`\n"+
 		"	- Homepage URL: `https://github.com/mattermost/mattermost-plugin-github`\n"+
-		"	- Authorization callback URL: `%s/oauth/complete`\n"+
+		"	- Authorization callback URL: `%s`\n"+
 		"3. Select **Register application**\n"+
 		"4. Select **Generate a new client secret**.\n"+
 		"5. If prompted, complete 2FA.",
-		fm.pluginID,
+		callbackURL,
 	)
 
 	return flow.NewStep(stepOAuthInfo).
@@ -597,7 +602,11 @@ func (fm *FlowManager) submitOAuthConfig(f *flow.Flow, submitted map[string]inte
 
 func (fm *FlowManager) stepOAuthConnect() flow.Step {
 	connectPretext := "##### :white_check_mark: Step {{ if .UsePreregisteredApplication }}1{{ else }}2{{ end }}: Connect your GitHub account"
-	connectURL := fmt.Sprintf("%s/oauth/connect", fm.pluginID)
+	connectURL, err := buildPluginURL(fm.client, "oauth", "connect")
+	if err != nil {
+		fm.client.Log.Warn("Failed to build connectURL", "err", err)
+	}
+
 	connectText := fmt.Sprintf("Go [here](%s) to connect your account.", connectURL)
 	return flow.NewStep(stepOAuthConnect).
 		WithText(connectText).
@@ -685,11 +694,16 @@ func (fm *FlowManager) submitWebhook(f *flow.Flow, submitted map[string]interfac
 
 	webhookEvents := []string{"create", "delete", "issue_comment", "issues", "pull_request", "pull_request_review", "pull_request_review_comment", "push", "star"}
 
+	webHookURL, err := buildPluginURL(fm.client, "webhook")
+	if err != nil {
+		fm.client.Log.Warn("Failed to build webHookURL", "err", err)
+	}
+
 	webhookConfig := map[string]interface{}{
 		"content_type": "json",
 		"insecure_ssl": "0",
 		"secret":       config.WebhookSecret,
-		"url":          fmt.Sprintf("%s/webhook", fm.pluginID),
+		"url":          webHookURL,
 	}
 
 	hook := &github.Hook{

diff --git a/server/plugin/graphql/client.go b/server/plugin/graphql/client.go
@@ -3,7 +3,6 @@ package graphql
 import (
 	"context"
 	"net/url"
-	"path"
 
 	"github.com/pkg/errors"
 	"github.com/shurcooL/githubv4"
@@ -36,16 +35,14 @@ func NewClient(logger pluginapi.LogService, getOrganizations func() []string, to
 			getOrganizations: getOrganizations,
 		}
 	} else {
-		baseURL, err := url.Parse(enterpriseBaseURL)
+		baseURL, err := url.JoinPath(enterpriseBaseURL, "api", "graphql")
 		if err != nil {
-			logger.Debug("Not able to parse the URL", "error", err.Error())
+			logger.Debug("Not able to parse the enterprise URL", "error", err.Error())
 			return nil
 		}
 
-		baseURL.Path = path.Join(baseURL.Path, "api", "graphql")
-
 		client = Client{
-			client:           githubv4.NewEnterpriseClient(baseURL.String(), httpClient),
+			client:           githubv4.NewEnterpriseClient(baseURL, httpClient),
 			username:         username,
 			org:              orgName,
 			logger:           logger,

diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"path"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -203,14 +202,17 @@ func getGitHubClient(authenticatedClient *http.Client, config *Configuration) (*
 	if config.EnterpriseBaseURL == "" || config.EnterpriseUploadURL == "" {
 		return github.NewClient(authenticatedClient), nil
 	}
+	baseURL, err := url.JoinPath(config.EnterpriseBaseURL, "api", "v3")
+	if err != nil {
+		return nil, err
+	}
 
-	baseURL, _ := url.Parse(config.EnterpriseBaseURL)
-	baseURL.Path = path.Join(baseURL.Path, "api", "v3")
-
-	uploadURL, _ := url.Parse(config.EnterpriseUploadURL)
-	uploadURL.Path = path.Join(uploadURL.Path, "api", "v3")
+	uploadURL, err := url.JoinPath(config.EnterpriseUploadURL, "api", "v3")
+	if err != nil {
+		return nil, err
+	}
 
-	client, err := github.NewEnterpriseClient(baseURL.String(), uploadURL.String(), authenticatedClient)
+	client, err := github.NewEnterpriseClient(baseURL, uploadURL, authenticatedClient)
 	if err != nil {
 		return nil, err
 	}
@@ -244,12 +246,12 @@ func (p *Plugin) setDefaultConfiguration() error {
 func (p *Plugin) OnActivate() error {
 	p.ensurePluginAPIClient()
 
-	siteURL := p.client.Configuration.GetConfig().ServiceSettings.SiteURL
-	if siteURL == nil || *siteURL == "" {
-		return errors.New("siteURL is not set. Please set it and restart the plugin")
+	_, err := getSiteURL(p.client)
+	if err != nil {
+		return err
 	}
 
-	err := p.setDefaultConfiguration()
+	err = p.setDefaultConfiguration()
 	if err != nil {
 		return errors.Wrap(err, "failed to set default configuration")
 	}
@@ -525,7 +527,7 @@ func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*mode
 	return post, ""
 }
 
-func (p *Plugin) getOAuthConfig(privateAllowed bool) *oauth2.Config {
+func (p *Plugin) getOAuthConfig(privateAllowed bool) (*oauth2.Config, error) {
 	config := p.getConfiguration()
 
 	repo := github.ScopePublicRepo
@@ -545,45 +547,54 @@ func (p *Plugin) getOAuthConfig(privateAllowed bool) *oauth2.Config {
 		baseURL = testOAuthServerURL + "/"
 	}
 
-	authURL, _ := url.Parse(baseURL)
-	tokenURL, _ := url.Parse(baseURL)
-
-	authURL.Path = path.Join(authURL.Path, "login", "oauth", "authorize")
-	tokenURL.Path = path.Join(tokenURL.Path, "login", "oauth", "access_token")
+	authURL, err := url.JoinPath(baseURL, "login", "oauth", "authorize")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build AuthURL")
+	}
+	tokenURL, err := url.JoinPath(baseURL, "login", "oauth", "access_token")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build TokenURL")
+	}
 
 	return &oauth2.Config{
 		ClientID:     config.GitHubOAuthClientID,
 		ClientSecret: config.GitHubOAuthClientSecret,
 		Scopes:       scopes,
 		Endpoint: oauth2.Endpoint{
-			AuthURL:   authURL.String(),
-			TokenURL:  tokenURL.String(),
+			AuthURL:   authURL,
+			TokenURL:  tokenURL,
 			AuthStyle: oauth2.AuthStyleInHeader,
 		},
-	}
+	}, nil
 }
 
-func (p *Plugin) getOAuthConfigForChimeraApp(scopes []string) *oauth2.Config {
+func (p *Plugin) getOAuthConfigForChimeraApp(scopes []string) (*oauth2.Config, error) {
 	baseURL := fmt.Sprintf("%s/v1/github/%s", p.chimeraURL, chimeraGitHubAppIdentifier)
-	authURL, _ := url.Parse(baseURL)
-	tokenURL, _ := url.Parse(baseURL)
 
-	authURL.Path = path.Join(authURL.Path, "oauth", "authorize")
-	tokenURL.Path = path.Join(tokenURL.Path, "oauth", "token")
-
-	redirectURL, _ := url.Parse(fmt.Sprintf("%s/plugins/github/oauth/complete", *p.client.Configuration.GetConfig().ServiceSettings.SiteURL))
+	authURL, err := url.JoinPath(baseURL, "oauth", "authorize")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build AuthURL")
+	}
+	tokenURL, err := url.JoinPath(baseURL, "oauth", "token")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build TokenURL")
+	}
+	redirectURL, err := buildPluginURL(p.client, "oauth", "token")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build RedirectURL")
+	}
 
 	return &oauth2.Config{
 		ClientID:     "placeholder",
 		ClientSecret: "placeholder",
 		Scopes:       scopes,
-		RedirectURL:  redirectURL.String(),
+		RedirectURL:  redirectURL,
 		Endpoint: oauth2.Endpoint{
-			AuthURL:   authURL.String(),
-			TokenURL:  tokenURL.String(),
+			AuthURL:   authURL,
+			TokenURL:  tokenURL,
 			AuthStyle: oauth2.AuthStyleInHeader,
 		},
-	}
+	}, nil
 }
 
 type GitHubUserInfo struct {

diff --git a/server/plugin/utils.go b/server/plugin/utils.go
@@ -15,6 +15,8 @@ import (
 	"strings"
 	"unicode"
 
+	"github.com/mattermost/mattermost/server/public/pluginapi"
+
 	"github.com/google/go-github/v54/github"
 	"github.com/pkg/errors"
 )
@@ -370,6 +372,29 @@ func isValidURL(rawURL string) error {
 	return nil
 }
 
+func buildPluginURL(client *pluginapi.Client, elem ...string) (string, error) {
+	siteURL, err := getSiteURL(client)
+	if err != nil {
+		return "", err
+	}
+
+	redirectURL, err := url.JoinPath(siteURL, append([]string{"plugins", Manifest.Id}, elem...)...)
+	if err != nil {
+		return "", errors.Wrap(err, "failed to build pluginURL")
+	}
+
+	return redirectURL, nil
+}
+
+func getSiteURL(client *pluginapi.Client) (string, error) {
+	siteURL := client.Configuration.GetConfig().ServiceSettings.SiteURL
+	if siteURL == nil {
+		return "", errors.New("siteURL is not set. Please set it and restart the plugin")
+	}
+
+	return strings.TrimSuffix(*siteURL, "/"), nil
+}
+
 // lastN returns the last n characters of a string, with the rest replaced by *.
 // At most 3 characters are replaced. The rest is cut off.
 func lastN(s string, n int) string {