Skip to content

[3.2.7 backport] CBG-4694 do not return invalid sessions with GET #7595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions auth/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,24 @@ func (auth *Authenticator) CreateSession(ctx context.Context, user User, ttl tim
return session, nil
}

// GetSession returns a session by ID. Return a not found error if the session is not found, or is invalid.
func (auth *Authenticator) GetSession(sessionID string) (*LoginSession, error) {
var session LoginSession
_, err := auth.datastore.Get(auth.DocIDForSession(sessionID), &session)
if err != nil {
if base.IsDocNotFoundError(err) {
err = nil
}
return nil, err
}
user, err := auth.GetUser(session.Username)
if err != nil {
return nil, err
}
if user == nil {
return nil, base.ErrNotFound
}
if session.SessionUUID != user.GetSessionUUID() {
return nil, base.ErrNotFound
}

return &session, nil
}

Expand Down
3 changes: 1 addition & 2 deletions auth/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,9 @@ func TestDeleteSession(t *testing.T) {
assert.NoError(t, dataStore.Set(auth.DocIDForSession(mockSession.ID), noSessionExpiry, nil, mockSession))
assert.NoError(t, auth.DeleteSession(ctx, mockSession.ID, ""))

// Just to verify the session has been deleted gracefully.
session, err := auth.GetSession(mockSession.ID)
assert.Nil(t, session)
assert.NoError(t, err)
base.RequireDocNotFoundError(t, err)
}

// Coverage for MakeSessionCookie. The MakeSessionCookie should create a cookie
Expand Down
2 changes: 1 addition & 1 deletion docs/api/paths/admin/db-_session.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ post:
description: User name to generate the session for.
type: string
ttl:
description: Time until the session expires. Uses default value of 24 hours if left blank.
description: Time until the session expires. Uses default value of 24 hours if left blank. This value must be greater or equal to 1.
type: integer
responses:
'200':
Expand Down
26 changes: 8 additions & 18 deletions rest/session_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,9 @@ func (h *handler) getUserSession() error {

h.assertAdminOnly()
session, err := h.db.Authenticator(h.ctx()).GetSession(h.PathVar("sessionid"))

if session == nil {
if err == nil {
err = kNotFoundError
}
if err != nil {
return err
}

return h.respondWithSessionInfoForSession(session)
}

Expand Down Expand Up @@ -292,22 +287,17 @@ func (h *handler) deleteUserSessionWithValidation(sessionId string, userName str
// Validate that the session being deleted belongs to the user. This adds some
// overhead - for user-agnostic session deletion should use deleteSession
session, getErr := h.db.Authenticator(h.ctx()).GetSession(sessionId)
if session == nil {
if getErr == nil {
getErr = kNotFoundError
}
if getErr != nil {
return getErr
}

if getErr == nil {
if session.Username == userName {
delErr := h.db.Authenticator(h.ctx()).DeleteSession(h.ctx(), sessionId, userName)
if delErr != nil {
return delErr
}
} else {
return kNotFoundError
if session.Username == userName {
delErr := h.db.Authenticator(h.ctx()).DeleteSession(h.ctx(), sessionId, userName)
if delErr != nil {
return delErr
}
} else {
return kNotFoundError
}
return nil
}
Expand Down
23 changes: 20 additions & 3 deletions rest/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,19 +447,19 @@ func TestSessionAPI(t *testing.T) {

// Attempt to GET the deleted session and make sure it's not found
response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user1sessions[0]), "")
RequireStatus(t, response, 404)
RequireNotFoundError(t, response)

// 2. DELETE a session with user validation
response = rt.SendAdminRequest("DELETE", fmt.Sprintf("/db/_user/%s/_session/%s", "user1", user1sessions[1]), "")
RequireStatus(t, response, 200)

// Attempt to GET the deleted session and make sure it's not found
response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user1sessions[1]), "")
RequireStatus(t, response, 404)
RequireNotFoundError(t, response)

// 3. DELETE a session not belonging to the user (should fail)
response = rt.SendAdminRequest("DELETE", fmt.Sprintf("/db/_user/%s/_session/%s", "user1", user2sessions[0]), "")
RequireStatus(t, response, 404)
RequireNotFoundError(t, response)

// GET the session and make sure it still exists
response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user2sessions[0]), "")
Expand All @@ -469,6 +469,23 @@ func TestSessionAPI(t *testing.T) {
response = rt.SendAdminRequest("DELETE", "/db/_user/user2/_session", "")
RequireStatus(t, response, 200)

// Validate that all sessions were deleted
for i := range 5 {
response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user2sessions[i]), "")
RequireNotFoundError(t, response)
}

// 5. DELETE sessions when password is changed
// Change password for user3
response = rt.SendAdminRequest("PUT", "/db/_user/user3", `{"password":"5678"}`)
RequireStatus(t, response, 200)

// Validate that all sessions were deleted
for i := range 5 {
response = rt.SendAdminRequest("GET", fmt.Sprintf("/db/_session/%s", user3sessions[i]), "")
RequireNotFoundError(t, response)
}

// DELETE the users
RequireStatus(t, rt.SendAdminRequest("DELETE", "/db/_user/user1", ""), 200)
RequireStatus(t, rt.SendAdminRequest("GET", "/db/_user/user1", ""), 404)
Expand Down
7 changes: 7 additions & 0 deletions rest/utilities_testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -2827,3 +2827,10 @@ func (sc *ServerContext) reloadDatabaseWithConfigLoadFromBucket(nonContextStruct
defer sc.lock.Unlock()
return sc._reloadDatabaseWithConfig(nonContextStruct.Ctx, config, true, true)
}

func RequireNotFoundError(t *testing.T, response *TestResponse) {
RequireStatus(t, response, http.StatusNotFound)
var body db.Body
require.NoError(t, base.JSONUnmarshal(response.Body.Bytes(), &body))
require.Equal(t, db.Body{"error": "not_found", "reason": "missing"}, body)
}
Loading