diff --git a/api4/user.go b/api4/user.go index 8694e887b1..1b647d088f 100644 --- a/api4/user.go +++ b/api4/user.go @@ -14,6 +14,7 @@ import ( "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/utils" ) func (api *API) InitUser() { @@ -1430,11 +1431,13 @@ func attachDeviceId(c *Context, w http.ResponseWriter, r *http.Request) { secure = true } + subpath, _ := utils.GetSubpathFromConfig(c.App.Config()) + expiresAt := time.Unix(model.GetMillis()/1000+int64(maxAge), 0) sessionCookie := &http.Cookie{ Name: model.SESSION_COOKIE_TOKEN, Value: c.App.Session.Token, - Path: "/", + Path: subpath, MaxAge: maxAge, Expires: expiresAt, HttpOnly: true, diff --git a/api4/user_test.go b/api4/user_test.go index 5672ca8505..283c18c24a 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -2427,28 +2427,49 @@ func TestAttachDeviceId(t *testing.T) { defer th.TearDown() deviceId := model.PUSH_NOTIFY_APPLE + ":1234567890" - pass, resp := th.Client.AttachDeviceId(deviceId) - CheckNoError(t, resp) - if !pass { - t.Fatal("should have passed") - } - - if sessions, err := th.App.GetSessions(th.BasicUser.Id); err != nil { - t.Fatal(err) - } else { - if sessions[0].DeviceId != deviceId { - t.Fatal("Missing device Id") + t.Run("success", func(t *testing.T) { + testCases := []struct { + Description string + SiteURL string + ExpectedSetCookieHeaderRegexp string + }{ + {"no subpath", "http://localhost:8065", "^MMAUTHTOKEN=[a-z0-9]+; Path=/"}, + {"subpath", "http://localhost:8065/subpath", "^MMAUTHTOKEN=[a-z0-9]+; Path=/subpath"}, } - } - _, resp = th.Client.AttachDeviceId("") - CheckBadRequestStatus(t, resp) + for _, tc := range testCases { + t.Run(tc.Description, func(t *testing.T) { - th.Client.Logout() + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.SiteURL + }) - _, resp = th.Client.AttachDeviceId("") - CheckUnauthorizedStatus(t, resp) + pass, resp := th.Client.AttachDeviceId(deviceId) + CheckNoError(t, resp) + + cookies := resp.Header.Get("Set-Cookie") + assert.Regexp(t, tc.ExpectedSetCookieHeaderRegexp, cookies) + assert.True(t, pass) + + sessions, err := th.App.GetSessions(th.BasicUser.Id) + require.Nil(t, err) + assert.Equal(t, deviceId, sessions[0].DeviceId, "Missing device Id") + }) + } + }) + + t.Run("invalid device id", func(t *testing.T) { + _, resp := th.Client.AttachDeviceId("") + CheckBadRequestStatus(t, resp) + }) + + t.Run("not logged in", func(t *testing.T) { + th.Client.Logout() + + _, resp := th.Client.AttachDeviceId("") + CheckUnauthorizedStatus(t, resp) + }) } func TestGetUserAudits(t *testing.T) { @@ -2673,6 +2694,36 @@ func TestLogin(t *testing.T) { }) } +func TestLoginCookies(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + th.Client.Logout() + + testCases := []struct { + Description string + SiteURL string + ExpectedSetCookieHeaderRegexp string + }{ + {"no subpath", "http://localhost:8065", "^MMAUTHTOKEN=[a-z0-9]+; Path=/"}, + {"subpath", "http://localhost:8065/subpath", "^MMAUTHTOKEN=[a-z0-9]+; Path=/subpath"}, + } + + for _, tc := range testCases { + t.Run(tc.Description, func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.SiteURL + }) + + user, resp := th.Client.Login(th.BasicUser.Email, th.BasicUser.Password) + CheckNoError(t, resp) + assert.Equal(t, user.Id, th.BasicUser.Id) + + cookies := resp.Header.Get("Set-Cookie") + assert.Regexp(t, tc.ExpectedSetCookieHeaderRegexp, cookies) + }) + } +} + func TestCBALogin(t *testing.T) { t.Run("primary", func(t *testing.T) { th := Setup().InitBasic() diff --git a/app/login.go b/app/login.go index 6c153d5395..5554f7ddf2 100644 --- a/app/login.go +++ b/app/login.go @@ -13,6 +13,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" "github.com/mattermost/mattermost-server/store" + "github.com/mattermost/mattermost-server/utils" ) func (a *App) CheckForClientSideCert(r *http.Request) (string, string, string) { @@ -164,11 +165,13 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, } domain := a.GetCookieDomain() + subpath, _ := utils.GetSubpathFromConfig(a.Config()) + expiresAt := time.Unix(model.GetMillis()/1000+int64(maxAge), 0) sessionCookie := &http.Cookie{ Name: model.SESSION_COOKIE_TOKEN, Value: session.Token, - Path: "/", + Path: subpath, MaxAge: maxAge, Expires: expiresAt, HttpOnly: true, @@ -179,7 +182,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, userCookie := &http.Cookie{ Name: model.SESSION_COOKIE_USER, Value: user.Id, - Path: "/", + Path: subpath, MaxAge: maxAge, Expires: expiresAt, Domain: domain, @@ -189,7 +192,7 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, csrfCookie := &http.Cookie{ Name: model.SESSION_COOKIE_CSRF, Value: session.GetCSRF(), - Path: "/", + Path: subpath, MaxAge: maxAge, Expires: expiresAt, Domain: domain, diff --git a/app/oauth.go b/app/oauth.go index 624022a4f7..a78dd570c8 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -654,11 +654,13 @@ func (a *App) GetAuthorizationCode(w http.ResponseWriter, r *http.Request, servi } cookieValue := model.NewId() + subpath, _ := utils.GetSubpathFromConfig(a.Config()) + expiresAt := time.Unix(model.GetMillis()/1000+int64(OAUTH_COOKIE_MAX_AGE_SECONDS), 0) oauthCookie := &http.Cookie{ Name: COOKIE_OAUTH, Value: cookieValue, - Path: "/", + Path: subpath, MaxAge: OAUTH_COOKIE_MAX_AGE_SECONDS, Expires: expiresAt, HttpOnly: true, @@ -741,10 +743,12 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service mlog.Error(appErr.Error()) } + subpath, _ := utils.GetSubpathFromConfig(a.Config()) + httpCookie := &http.Cookie{ Name: COOKIE_OAUTH, Value: "", - Path: "/", + Path: subpath, MaxAge: -1, HttpOnly: true, } diff --git a/app/oauth_test.go b/app/oauth_test.go index 9155cdbcf6..f80caca37e 100644 --- a/app/oauth_test.go +++ b/app/oauth_test.go @@ -468,43 +468,120 @@ func TestAuthorizeOAuthUser(t *testing.T) { }) t.Run("enabled and properly configured", func(t *testing.T) { - userData := "Hello, World!" - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case "/token": - json.NewEncoder(w).Encode(&model.AccessResponse{ - AccessToken: model.NewId(), - TokenType: model.ACCESS_TOKEN_TYPE, - }) - case "/user": - w.WriteHeader(http.StatusOK) - w.Write([]byte(userData)) - } - })) - defer server.Close() - - th := setup(true, true, true, server.URL) - defer th.TearDown() - - cookie := model.NewId() - request := makeRequest(t, cookie) - - stateProps := map[string]string{ - "team_id": model.NewId(), - "token": makeToken(th, cookie).Token, + testCases := []struct { + Description string + SiteURL string + ExpectedSetCookieHeaderRegexp string + }{ + {"no subpath", "http://localhost:8065", "^MMOAUTH=; Path=/"}, + {"subpath", "http://localhost:8065/subpath", "^MMOAUTH=; Path=/subpath"}, } - state := base64.StdEncoding.EncodeToString([]byte(model.MapToJson(stateProps))) - body, receivedTeamId, receivedStateProps, err := th.App.AuthorizeOAuthUser(&httptest.ResponseRecorder{}, request, model.SERVICE_GITLAB, "", state, "") + for _, tc := range testCases { + t.Run(tc.Description, func(t *testing.T) { + userData := "Hello, World!" - require.NotNil(t, body) - bodyBytes, bodyErr := ioutil.ReadAll(body) - require.Nil(t, bodyErr) - assert.Equal(t, userData, string(bodyBytes)) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/token": + json.NewEncoder(w).Encode(&model.AccessResponse{ + AccessToken: model.NewId(), + TokenType: model.ACCESS_TOKEN_TYPE, + }) + case "/user": + w.WriteHeader(http.StatusOK) + w.Write([]byte(userData)) + } + })) + defer server.Close() - assert.Equal(t, stateProps["team_id"], receivedTeamId) - assert.Equal(t, stateProps, receivedStateProps) - assert.Nil(t, err) + th := setup(true, true, true, server.URL) + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.SiteURL + }) + + cookie := model.NewId() + request := makeRequest(t, cookie) + + stateProps := map[string]string{ + "team_id": model.NewId(), + "token": makeToken(th, cookie).Token, + } + state := base64.StdEncoding.EncodeToString([]byte(model.MapToJson(stateProps))) + + recorder := httptest.ResponseRecorder{} + body, receivedTeamId, receivedStateProps, err := th.App.AuthorizeOAuthUser(&recorder, request, model.SERVICE_GITLAB, "", state, "") + + require.NotNil(t, body) + bodyBytes, bodyErr := ioutil.ReadAll(body) + require.Nil(t, bodyErr) + assert.Equal(t, userData, string(bodyBytes)) + + assert.Equal(t, stateProps["team_id"], receivedTeamId) + assert.Equal(t, stateProps, receivedStateProps) + assert.Nil(t, err) + + cookies := recorder.Header().Get("Set-Cookie") + assert.Regexp(t, tc.ExpectedSetCookieHeaderRegexp, cookies) + }) + } + }) +} + +func TestGetAuthorizationCode(t *testing.T) { + t.Run("not enabled", func(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.GitLabSettings.Enable = false + }) + + _, err := th.App.GetAuthorizationCode(nil, nil, model.SERVICE_GITLAB, map[string]string{}, "") + require.NotNil(t, err) + assert.Equal(t, "api.user.get_authorization_code.unsupported.app_error", err.Id) + }) + + t.Run("enabled and properly configured", func(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.GitLabSettings.Enable = true + }) + + testCases := []struct { + Description string + SiteURL string + ExpectedSetCookieHeaderRegexp string + }{ + {"no subpath", "http://localhost:8065", "^MMOAUTH=[a-z0-9]+; Path=/"}, + {"subpath", "http://localhost:8065/subpath", "^MMOAUTH=[a-z0-9]+; Path=/subpath"}, + } + + for _, tc := range testCases { + t.Run(tc.Description, func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.SiteURL + }) + + request, _ := http.NewRequest(http.MethodGet, "https://mattermost.example.com", nil) + + stateProps := map[string]string{ + "email": "email@example.com", + "action": "action", + } + + recorder := httptest.ResponseRecorder{} + url, err := th.App.GetAuthorizationCode(&recorder, request, model.SERVICE_GITLAB, stateProps, "") + require.Nil(t, err) + assert.NotEmpty(t, url) + + cookies := recorder.Header().Get("Set-Cookie") + assert.Regexp(t, tc.ExpectedSetCookieHeaderRegexp, cookies) + }) + } }) } diff --git a/web/context.go b/web/context.go index 1d56eb5547..0b21d05bc9 100644 --- a/web/context.go +++ b/web/context.go @@ -138,10 +138,12 @@ func (c *Context) MfaRequired() { } func (c *Context) RemoveSessionCookie(w http.ResponseWriter, r *http.Request) { + subpath, _ := utils.GetSubpathFromConfig(c.App.Config()) + cookie := &http.Cookie{ Name: model.SESSION_COOKIE_TOKEN, Value: "", - Path: "/", + Path: subpath, MaxAge: -1, HttpOnly: true, } diff --git a/web/handlers_test.go b/web/handlers_test.go index ac3bf7e37c..054909d0ce 100644 --- a/web/handlers_test.go +++ b/web/handlers_test.go @@ -10,6 +10,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func handlerForHTTPErrors(c *Context, w http.ResponseWriter, r *http.Request) { @@ -289,3 +290,50 @@ func TestHandlerServeCSPHeader(t *testing.T) { // assert.Contains(t, response.Header()["Content-Security-Policy"], "frame-ancestors 'self'; script-src 'self' cdn.segment.com/analytics.js/ 'sha256-tPOjw+tkVs9axL78ZwGtYl975dtyPHB6LYKAO2R3gR4='", "csp header incorrectly changed after subpath changed") }) } + +func TestHandlerServeInvalidToken(t *testing.T) { + testCases := []struct { + Description string + SiteURL string + ExpectedSetCookieHeaderRegexp string + }{ + {"no subpath", "http://localhost:8065", "^MMAUTHTOKEN=; Path=/"}, + {"subpath", "http://localhost:8065/subpath", "^MMAUTHTOKEN=; Path=/subpath"}, + } + + for _, tc := range testCases { + t.Run(tc.Description, func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.SiteURL + }) + + web := New(th.Server, th.Server.AppOptions, th.Server.Router) + + handler := Handler{ + GetGlobalAppOptions: web.GetGlobalAppOptions, + HandleFunc: handlerForCSRFToken, + RequireSession: true, + TrustRequester: false, + RequireMfa: false, + IsStatic: false, + } + + cookie := &http.Cookie{ + Name: model.SESSION_COOKIE_TOKEN, + Value: "invalid", + } + + request := httptest.NewRequest("POST", "/api/v4/test", nil) + request.AddCookie(cookie) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + require.Equal(t, http.StatusUnauthorized, response.Code) + + cookies := response.Header().Get("Set-Cookie") + assert.Regexp(t, tc.ExpectedSetCookieHeaderRegexp, cookies) + }) + } +}