From 20bc8662b1299ed5302923368701b318909125cd Mon Sep 17 00:00:00 2001 From: oliverpool Date: Thu, 7 Mar 2024 10:50:04 +0100 Subject: [PATCH 1/2] [BUG] prevent removing session cookie when redirect_uri query contains :// --- tests/integration/oauth_test.go | 58 +++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index fed73696e3..1da1c6f9c0 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -470,6 +470,64 @@ func TestSignInOAuthCallbackSignIn(t *testing.T) { assert.Greater(t, userAfterLogin.LastLoginUnix, userGitLab.LastLoginUnix) } +func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // + // OAuth2 authentication source GitLab + // + gitlabName := "gitlab" + gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName)) + + // + // Create a user as if it had been previously been created by the GitLab + // authentication source. + // + userGitLabUserID := "5678" + userGitLab := &user_model.User{ + Name: "gitlabuser", + Email: "gitlabuser@example.com", + Passwd: "gitlabuserpassword", + Type: user_model.UserTypeIndividual, + LoginType: auth_model.OAuth2, + LoginSource: gitlab.ID, + LoginName: userGitLabUserID, + } + defer createUser(context.Background(), t, userGitLab)() + + // + // A request for user information sent to Goth will return a + // goth.User exactly matching the user created above. + // + defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) { + return goth.User{ + Provider: gitlabName, + UserID: userGitLabUserID, + Email: userGitLab.Email, + }, nil + })() + req := NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", gitlabName)) + req.AddCookie(&http.Cookie{ + Name: "redirect_to", + Value: "/login/oauth/authorize?redirect_uri=https%3A%2F%2Ftranslate.example.org", + Path: "/", + }) + resp := MakeRequest(t, req, http.StatusSeeOther) + + hasNewSessionCookie := false + sessionCookieName := setting.SessionConfig.CookieName + for _, c := range resp.Result().Cookies() { + if c.Name == sessionCookieName { + hasNewSessionCookie = true + break + } + t.Log("Got cookie", c.Name) + } + + assert.True(t, hasNewSessionCookie, "Session cookie %q is missing", sessionCookieName) + assert.Equal(t, "/login/oauth/authorize?redirect_uri=https://translate.example.org", test.RedirectURL(resp)) +} + func TestSignUpViaOAuthWithMissingFields(t *testing.T) { defer tests.PrepareTestEnv(t)() // enable auto-creation of accounts via OAuth2 From 3877a2332b2baf7c9458eceebb859699784e586e Mon Sep 17 00:00:00 2001 From: oliverpool Date: Thu, 7 Mar 2024 10:53:43 +0100 Subject: [PATCH 2/2] implement fix --- modules/context/base.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/context/base.go b/modules/context/base.go index 8df1dde866..511d0ec58c 100644 --- a/modules/context/base.go +++ b/modules/context/base.go @@ -255,7 +255,7 @@ func (b *Base) Redirect(location string, status ...int) { code = status[0] } - if strings.Contains(location, "://") || strings.HasPrefix(location, "//") { + if httplib.IsRiskyRedirectURL(location) { // Some browsers (Safari) have buggy behavior for Cookie + Cache + External Redirection, eg: /my-path => https://other/path // 1. the first request to "/my-path" contains cookie // 2. some time later, the request to "/my-path" doesn't contain cookie (caused by Prevent web tracking)