From d9f5cd27403ffdde7b226f796fde727f2339b8bb Mon Sep 17 00:00:00 2001 From: Pantelis Vratsalis Date: Thu, 9 Nov 2023 20:18:24 +0200 Subject: [PATCH] [MM-52445] Fix double url encoding of oauth redirect URI params (#23176) * [MM-52445] Fix double url encoding of oauth redirect URI params * Additional test based on code review --------- Co-authored-by: Mattermost Build --- server/channels/app/oauth.go | 4 ++-- server/channels/web/oauth_test.go | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/server/channels/app/oauth.go b/server/channels/app/oauth.go index 18903f987f..39320a5f5f 100644 --- a/server/channels/app/oauth.go +++ b/server/channels/app/oauth.go @@ -178,8 +178,8 @@ func (a *App) GetOAuthCodeRedirect(userID string, authRequest *model.AuthorizeRe uri.RawQuery = queryParams.Encode() return uri.String(), nil } - queryParams.Set("code", url.QueryEscape(authData.Code)) - queryParams.Set("state", url.QueryEscape(authData.State)) + queryParams.Set("code", authData.Code) + queryParams.Set("state", authData.State) uri.RawQuery = queryParams.Encode() return uri.String(), nil } diff --git a/server/channels/web/oauth_test.go b/server/channels/web/oauth_test.go index 4e60fecac8..d8307b4545 100644 --- a/server/channels/web/oauth_test.go +++ b/server/channels/web/oauth_test.go @@ -154,7 +154,7 @@ func TestAuthorizeOAuthApp(t *testing.T) { ClientId: rapp.Id, RedirectURI: rapp.CallbackUrls[0], Scope: "", - State: "123", + State: "/oauthcallback?sesskey=abcd&other=123", } uriResponse, _, err := apiClient.AuthorizeOAuthApp(context.Background(), authRequest) require.NoError(t, err) @@ -164,7 +164,11 @@ func TestAuthorizeOAuthApp(t *testing.T) { // 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") + + // test state is not encoded multiple times require.Equal(t, ru.Query().Get("state"), authRequest.State, "returned state doesn't match") + // test state is URL encoded at least once + require.Empty(t, ru.Query().Get("other"), "state's query parameters should not leak") } func TestDeauthorizeOAuthApp(t *testing.T) {