Weknora/Enterprise-Knowledge-Base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(session): map ErrSessionNotFound to 404 across all handlers

Browse Source 
PR #1309 plumbed user-scope into the session/message service layer so
non-owner / wrong-tenant lookups now surface as ErrSessionNotFound at
every entry point that goes through sessionRepo.Get. The handler-side
mapping, however, only existed in 4 of the 8 affected sites:

  - handler/session/handler.go:GetSession        ✓ already mapped (with `==`)
  - handler/session/handler.go:UpdateSession     ✓ already mapped (with `==`)
  - handler/session/handler.go:DeleteSession     ✓ already mapped (with `==`)
  - handler/session/handler.go:BatchDeleteSessions ✓ already mapped (with `==`)
  - handler/session/handler.go:ClearSessionMessages ✗ always 500
  - handler/session/stream.go:ContinueStream session lookup ✓ (with `==`)
  - handler/session/stream.go:ContinueStream message lookup ✗ always 500
  - handler/message.go:LoadMessages (recent + before-time) ✗ always 500
  - handler/message.go:DeleteMessage              ✗ always 500

Result: a Contributor in tenant A asking for a Contributor B's session
messages got a 500 instead of 404 — the wire response leaked
"something is broken" rather than "you can't see this URL", which
breaks SDK / frontend axios interceptor error handling that keys on
HTTP status.

Two changes:

1. Add ErrSessionNotFound → 404 mapping to the 4 sites that lacked it
   (ClearSessionMessages, ContinueStream's GetMessage, LoadMessages's
   two paths, DeleteMessage).

2. Replace `err == errors.ErrSessionNotFound` with
   `stderrors.Is(err, errors.ErrSessionNotFound)` everywhere. Sentinel
   error comparison must use errors.Is so wrapped errors
   (`fmt.Errorf("...: %w", ErrSessionNotFound)`) still match. Today no
   service-layer caller wraps, but a future refactor that does would
   silently turn 404s into 500s with no test catching it.

Tests:

  - internal/handler/message_session_not_found_test.go covers the four
    new mapping sites in handler/message.go (LoadMessages-recent,
    LoadMessages-before-time, DeleteMessage) and pins the wrapped-error
    behaviour with a dedicated regression test that fmt.Errorf("%w") a
    sentinel must still be detected as ErrSessionNotFound. The bare
    handler is hit through gin.Engine + middleware.ErrorHandler so the
    response shape matches production exactly.

  - PR #1309's existing service-level tests (session_user_scope_test,
    session_test on the repository) still pass — this PR is a strictly
    handler-layer follow-up.
This commit is contained in:
wizardchen
2026-05-14 16:44:55 +08:00
committed by lyingbug
parent 6f95c75ed2
commit d7b5835519
4 changed files with 191 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/handler/message.go
+24
View File
@@ -1,6 +1,7 @@
package handler
import (
stderrors "errors"
"net/http"
"strconv"
"time"
@@ -70,6 +71,15 @@ func (h *MessageHandler) LoadMessages(c *gin.Context) {
logger.Infof(ctx, "Getting recent messages for session, session ID: %s, limit: %d", sessionID, limitInt)
messages, err := h.MessageService.GetRecentMessagesBySession(ctx, sessionID, limitInt)
if err != nil {
if stderrors.Is(err, errors.ErrSessionNotFound) {
// PR #1309 plumbed user-scope into the message service's
// session existence check; non-owner / wrong-tenant lookups
// surface as ErrSessionNotFound. Map to 404 so clients can
// tell "wrong URL" from a real 5xx.
logger.Warnf(ctx, "Session not found, ID: %s", sessionID)
c.Error(errors.NewNotFoundError(err.Error()))
return
}
logger.ErrorWithFields(ctx, err, nil)
c.Error(errors.NewInternalServerError(err.Error()))
return
@@ -104,6 +114,12 @@ func (h *MessageHandler) LoadMessages(c *gin.Context) {
sessionID, beforeTime.Format(time.RFC3339Nano), limitInt)
messages, err := h.MessageService.GetMessagesBySessionBeforeTime(ctx, sessionID, beforeTime, limitInt)
if err != nil {
if stderrors.Is(err, errors.ErrSessionNotFound) {
// See note on the GetRecentMessagesBySession path above.
logger.Warnf(ctx, "Session not found, ID: %s", sessionID)
c.Error(errors.NewNotFoundError(err.Error()))
return
}
logger.ErrorWithFields(ctx, err, nil)
c.Error(errors.NewInternalServerError(err.Error()))
return
@@ -146,6 +162,14 @@ func (h *MessageHandler) DeleteMessage(c *gin.Context) {
// Delete the message using the message service
if err := h.MessageService.DeleteMessage(ctx, sessionID, messageID); err != nil {
if stderrors.Is(err, errors.ErrSessionNotFound) {
// See note on LoadMessages above — message-service operations
// surface ErrSessionNotFound when the caller can't see the
// owning session (post-#1309 user scope).
logger.Warnf(ctx, "Session not found, ID: %s", sessionID)
c.Error(errors.NewNotFoundError(err.Error()))
return
}
logger.ErrorWithFields(ctx, err, nil)
c.Error(errors.NewInternalServerError(err.Error()))
return
internal/handler/message_session_not_found_test.go
+146
View File
@@ -0,0 +1,146 @@
package handler
import (
"context"
stderrors "errors"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/gin-gonic/gin"
apperrors "github.com/Tencent/WeKnora/internal/errors"
"github.com/Tencent/WeKnora/internal/middleware"
"github.com/Tencent/WeKnora/internal/types"
"github.com/Tencent/WeKnora/internal/types/interfaces"
)
// stubMessageService satisfies just the four MessageService methods this
// file exercises. Embedding the interface keeps the rest nil-panicky on
// purpose: any test that reaches an un-stubbed method should fail loudly
// rather than silently returning zero-values.
type stubMessageService struct {
interfaces.MessageService
getRecent func(ctx context.Context, sessionID string, limit int) ([]*types.Message, error)
getBeforeTime func(ctx context.Context, sessionID string, beforeTime time.Time, limit int) ([]*types.Message, error)
deleteMessage func(ctx context.Context, sessionID, id string) error
}
func (s *stubMessageService) GetRecentMessagesBySession(
ctx context.Context, sessionID string, limit int,
) ([]*types.Message, error) {
return s.getRecent(ctx, sessionID, limit)
}
func (s *stubMessageService) GetMessagesBySessionBeforeTime(
ctx context.Context, sessionID string, beforeTime time.Time, limit int,
) ([]*types.Message, error) {
return s.getBeforeTime(ctx, sessionID, beforeTime, limit)
}
func (s *stubMessageService) DeleteMessage(ctx context.Context, sessionID, id string) error {
return s.deleteMessage(ctx, sessionID, id)
}
// newMessageTestRouter mounts the standard ErrorHandler middleware so
// c.Error(NewNotFoundError(...)) renders as a real 404 envelope —
// matching production routing exactly.
func newMessageTestRouter(svc interfaces.MessageService) *gin.Engine {
gin.SetMode(gin.TestMode)
r := gin.New()
r.Use(middleware.ErrorHandler())
h := &MessageHandler{MessageService: svc}
r.GET("/messages/:session_id/load", h.LoadMessages)
r.DELETE("/messages/:session_id/:id", h.DeleteMessage)
return r
}
// TestLoadMessagesMapsErrSessionNotFoundToNotFound pins the fix for
// PR #1309's surfacing gap: when message-service ops can't see the
// owning session (non-owner / wrong tenant), the handler used to
// surface a 500 because ErrSessionNotFound flowed through the generic
// error branch. We now map it to 404 so clients can tell "wrong URL"
// from a real 5xx.
func TestLoadMessagesMapsErrSessionNotFoundToNotFound(t *testing.T) {
svc := &stubMessageService{
getRecent: func(_ context.Context, _ string, _ int) ([]*types.Message, error) {
return nil, apperrors.ErrSessionNotFound
},
}
w := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodGet, "/messages/sess1/load?limit=20", nil)
newMessageTestRouter(svc).ServeHTTP(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("ErrSessionNotFound must map to 404, got %d body=%s", w.Code, w.Body.String())
}
}
// TestLoadMessagesBeforeTimeMapsErrSessionNotFoundToNotFound covers the
// second branch of LoadMessages (the `before_time` path), which has its
// own error-handling block.
func TestLoadMessagesBeforeTimeMapsErrSessionNotFoundToNotFound(t *testing.T) {
svc := &stubMessageService{
getBeforeTime: func(_ context.Context, _ string, _ time.Time, _ int) ([]*types.Message, error) {
return nil, apperrors.ErrSessionNotFound
},
}
w := httptest.NewRecorder()
url := "/messages/sess1/load?limit=20&before_time=" + time.Now().UTC().Format(time.RFC3339Nano)
req := httptest.NewRequest(http.MethodGet, url, nil)
newMessageTestRouter(svc).ServeHTTP(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("ErrSessionNotFound on before-time path must map to 404, got %d body=%s",
w.Code, w.Body.String())
}
}
// TestDeleteMessageMapsErrSessionNotFoundToNotFound mirrors the
// LoadMessages test for the delete endpoint.
func TestDeleteMessageMapsErrSessionNotFoundToNotFound(t *testing.T) {
svc := &stubMessageService{
deleteMessage: func(_ context.Context, _, _ string) error {
return apperrors.ErrSessionNotFound
},
}
w := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodDelete, "/messages/sess1/msg1", nil)
newMessageTestRouter(svc).ServeHTTP(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("ErrSessionNotFound on DeleteMessage must map to 404, got %d body=%s",
w.Code, w.Body.String())
}
}
// TestDeleteMessageHonoursWrappedErrSessionNotFound is the regression
// test against the stale `==` sentinel comparison the original PR used.
// errors.Is unwraps fmt.Errorf("%w", ...); a literal `==` does not. If
// anyone reverts the comparison, this test fails before the change ships.
func TestDeleteMessageHonoursWrappedErrSessionNotFound(t *testing.T) {
svc := &stubMessageService{
deleteMessage: func(_ context.Context, _, _ string) error {
return fmt.Errorf("delete: %w", apperrors.ErrSessionNotFound)
},
}
w := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodDelete, "/messages/sess1/msg1", nil)
newMessageTestRouter(svc).ServeHTTP(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("wrapped ErrSessionNotFound must still map to 404, got %d body=%s",
w.Code, w.Body.String())
}
// Defence-in-depth: the response body should expose the AppError
// envelope (ErrorHandler renders {success,error:{code,message}}),
// not the bare gin error string.
if !strings.Contains(w.Body.String(), `"code":1003`) {
t.Fatalf("expected NotFound envelope with code=1003, got body=%s", w.Body.String())
}
}
// guardAgainstStaleSentinelEquality is a compile-time assertion: if a
// future refactor accidentally drops the stderrors import, the test file
// stops compiling and the human gets a clear signal that something tied
// to errors.Is wrapping went away.
var _ = stderrors.Is
internal/handler/session/handler.go
+10 -4
View File
@@ -1,6 +1,7 @@
package session
import (
stderrors "errors"
"net/http"
"github.com/Tencent/WeKnora/internal/config"
@@ -160,7 +161,7 @@ func (h *Handler) GetSession(c *gin.Context) {
logger.Infof(ctx, "Retrieving session, ID: %s", id)
session, err := h.sessionService.GetSession(ctx, id)
if err != nil {
if err == errors.ErrSessionNotFound {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "Session not found, ID: %s", id)
c.Error(errors.NewNotFoundError(err.Error()))
return
@@ -275,7 +276,7 @@ func (h *Handler) UpdateSession(c *gin.Context) {
// Call service to update session
if err := h.sessionService.UpdateSession(ctx, &session); err != nil {
if err == errors.ErrSessionNotFound {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "Session not found, ID: %s", id)
c.Error(errors.NewNotFoundError(err.Error()))
return
@@ -326,7 +327,7 @@ func (h *Handler) DeleteSession(c *gin.Context) {
// Call service to delete session
if err := h.sessionService.DeleteSession(ctx, id); err != nil {
if err == errors.ErrSessionNotFound {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "Session not found, ID: %s", id)
c.Error(errors.NewNotFoundError(err.Error()))
return
@@ -369,6 +370,11 @@ func (h *Handler) ClearSessionMessages(c *gin.Context) {
logger.Infof(ctx, "Clearing all messages for session: %s", id)
if err := h.messageService.ClearSessionMessages(ctx, id); err != nil {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "Session not found, ID: %s", id)
c.Error(errors.NewNotFoundError(err.Error()))
return
}
logger.ErrorWithFields(ctx, err, map[string]interface{}{"session_id": id})
c.Error(errors.NewInternalServerError(err.Error()))
return
@@ -442,7 +448,7 @@ func (h *Handler) BatchDeleteSessions(c *gin.Context) {
}
if err := h.sessionService.BatchDeleteSessions(ctx, sanitizedIDs); err != nil {
if err == errors.ErrSessionNotFound {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "No visible sessions found for batch delete")
c.Error(errors.NewNotFoundError(err.Error()))
return
internal/handler/session/stream.go
+11 -1
View File
@@ -2,6 +2,7 @@ package session
import (
"context"
stderrors "errors"
"fmt"
"net/http"
"time"
@@ -54,7 +55,7 @@ func (h *Handler) ContinueStream(c *gin.Context) {
// Verify that the session exists and belongs to this tenant
_, err := h.sessionService.GetSession(ctx, sessionID)
if err != nil {
if err == errors.ErrSessionNotFound {
if stderrors.Is(err, errors.ErrSessionNotFound) {
logger.Warnf(ctx, "Session not found, ID: %s", sessionID)
c.Error(errors.NewNotFoundError(err.Error()))
} else {
@@ -67,6 +68,15 @@ func (h *Handler) ContinueStream(c *gin.Context) {
// Get the incomplete message
message, err := h.messageService.GetMessage(ctx, sessionID, messageID)
if err != nil {
if stderrors.Is(err, errors.ErrSessionNotFound) {
// PR #1309 plumbed user-scope into messageService.GetMessage's
// session existence check; non-owner / wrong-user lookups now
// surface as ErrSessionNotFound. Map to 404 so clients can tell
// "wrong URL" from a real 5xx instead of seeing a generic 500.
logger.Warnf(ctx, "Session not found, ID: %s", sessionID)
c.Error(errors.NewNotFoundError(err.Error()))
return
}
logger.ErrorWithFields(ctx, err, nil)
c.Error(errors.NewInternalServerError(err.Error()))
return