From 2171ad8abf655445af50f4bfdabf74b41677287f Mon Sep 17 00:00:00 2001
From: Shivashis Padhi <shivashispadhi@gmail.com>
Date: Mon, 14 Nov 2022 10:27:47 +0530
Subject: [PATCH] [MM-46216] P3 Bugfix - Oauth redirectURI with query
 parameters generates malformed redirect URL (#20854)

* Parse and reconstruct URI, instead of using string concatenation

* Rename parse error, add test

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
---
 app/oauth.go      | 19 +++++++++++++++----
 web/oauth_test.go | 29 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/app/oauth.go b/app/oauth.go
index c0ca0484b4..7b5b37723d 100644
--- a/app/oauth.go
+++ b/app/oauth.go
@@ -165,11 +165,22 @@ func (a *App) GetOAuthCodeRedirect(userID string, authRequest *model.AuthorizeRe
 	authData := &model.AuthData{UserId: userID, ClientId: authRequest.ClientId, CreateAt: model.GetMillis(), RedirectUri: authRequest.RedirectURI, State: authRequest.State, Scope: authRequest.Scope}
 	authData.Code = model.NewId() + model.NewId()
 
-	if _, err := a.Srv().Store().OAuth().SaveAuthData(authData); err != nil {
-		return authRequest.RedirectURI + "?error=server_error&state=" + authRequest.State, nil
+	// parse authRequest.RedirectURI to handle query parameters see: https://mattermost.atlassian.net/browse/MM-46216
+	uri, err := url.Parse(authRequest.RedirectURI)
+	if err != nil {
+		return authRequest.RedirectURI + "?error=redirect_uri_parse_error&state=" + authRequest.State, nil
 	}
-
-	return authRequest.RedirectURI + "?code=" + url.QueryEscape(authData.Code) + "&state=" + url.QueryEscape(authData.State), nil
+	queryParams := uri.Query()
+	if _, err := a.Srv().Store().OAuth().SaveAuthData(authData); err != nil {
+		queryParams.Set("error", "server_error")
+		queryParams.Set("state", authRequest.State)
+		uri.RawQuery = queryParams.Encode()
+		return uri.String(), nil
+	}
+	queryParams.Set("code", url.QueryEscape(authData.Code))
+	queryParams.Set("state", url.QueryEscape(authData.State))
+	uri.RawQuery = queryParams.Encode()
+	return uri.String(), nil
 }
 
 func (a *App) AllowOAuthAppAccessToUser(userID string, authRequest *model.AuthorizeRequest) (string, *model.AppError) {
diff --git a/web/oauth_test.go b/web/oauth_test.go
index 6531290300..026807dcda 100644
--- a/web/oauth_test.go
+++ b/web/oauth_test.go
@@ -137,6 +137,35 @@ func TestAuthorizeOAuthApp(t *testing.T) {
 	_, resp, err = apiClient.AuthorizeOAuthApp(authRequest)
 	require.Error(t, err)
 	CheckNotFoundStatus(t, resp)
+
+	// test callback URI doesn't have malformed query parameters
+	oappWithQueryParamInCallback := &model.OAuthApp{
+		Name:         GenerateTestAppName(),
+		Homepage:     "https://nowhere.com",
+		Description:  "test",
+		CallbackUrls: []string{"https://nowhere.com?simply=lovely"},
+		CreatorId:    th.SystemAdminUser.Id,
+	}
+
+	rapp, appErr = th.App.CreateOAuthApp(oappWithQueryParamInCallback)
+	require.Nil(t, appErr)
+
+	authRequest = &model.AuthorizeRequest{
+		ResponseType: model.AuthCodeResponseType,
+		ClientId:     rapp.Id,
+		RedirectURI:  rapp.CallbackUrls[0],
+		Scope:        "",
+		State:        "123",
+	}
+	uriResponse, _, err := apiClient.AuthorizeOAuthApp(authRequest)
+	require.NoError(t, err)
+	ru, _ = url.Parse(uriResponse)
+	require.NotEmpty(t, uriResponse, "redirect url should be set")
+	require.NotNil(t, ru, "redirect url unparseable")
+	// require no query parameter to have "?"
+	require.False(t, strings.Contains(ru.RawQuery, "?"), "should not malform query parameters")
+	require.NotEmpty(t, ru.Query().Get("code"), "authorization code not returned")
+	require.Equal(t, ru.Query().Get("state"), authRequest.State, "returned state doesn't match")
 }
 
 func TestDeauthorizeOAuthApp(t *testing.T) {