From 4fb089d4d7ee2fc80770237e0709a9620fd0767d Mon Sep 17 00:00:00 2001 From: wizardchen Date: Thu, 14 May 2026 20:10:04 +0800 Subject: [PATCH] fix(kb): map ErrKnowledgeBaseNotFound to 404 across handler helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five handler helpers (validateAndGetKnowledgeBase in knowledgebase.go, validateKnowledgeBaseAccessWithKBID in knowledge.go, the kbService guards in faq.go and tag.go, and getKnowledgeBaseForInitialization + the per-kb config getter in initialization.go) wrapped every GetKnowledgeBaseByID error — including the well-known repository.ErrKnowledgeBaseNotFound sentinel — as NewInternalServerError. The result was that every probe of a stale or cross-tenant kb id surfaced as a 500 instead of the 404 it should have been, both confusing clients ("real 5xx vs. wrong URL") and burning ops attention on monitoring alerts. The mapping pattern is the same as PR #1336 for sessions: detect the sentinel via stderrors.Is and emit NewNotFoundError; everything else still surfaces as a 500 so genuine DB / repo failures keep firing the alerts that matter. Caught during the RBAC e2e smoke run on feat/rbac, where a deliberate cross-tenant kb-id probe produced HTTP 500 + body "knowledge base not found" — the smoking gun. Tests: new internal/handler/knowledgebase_not_found_test.go covers three cases — bare sentinel, fmt.Errorf("%w") wrapped sentinel (regression guard against a future revert to `==`), and a non-sentinel infrastructure error that must still 500. All three pass. The full handler test package is green. --- internal/handler/faq.go | 10 ++ internal/handler/initialization.go | 15 ++ internal/handler/knowledge.go | 7 + internal/handler/knowledgebase.go | 8 ++ .../handler/knowledgebase_not_found_test.go | 135 ++++++++++++++++++ internal/handler/tag.go | 8 ++ 6 files changed, 183 insertions(+) create mode 100644 internal/handler/knowledgebase_not_found_test.go diff --git a/internal/handler/faq.go b/internal/handler/faq.go index 022fcaf31..11a0f6ffb 100644 --- a/internal/handler/faq.go +++ b/internal/handler/faq.go @@ -2,11 +2,13 @@ package handler import ( "context" + stderrors "errors" "net/http" "strconv" "github.com/gin-gonic/gin" + "github.com/Tencent/WeKnora/internal/application/repository" "github.com/Tencent/WeKnora/internal/errors" "github.com/Tencent/WeKnora/internal/logger" "github.com/Tencent/WeKnora/internal/types" @@ -51,6 +53,14 @@ func (h *FAQHandler) effectiveCtxForKB(c *gin.Context, kbID string, requiredPerm } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // ErrKnowledgeBaseNotFound is the expected response for a stale + // or probed kb id; the FAQ endpoints went out of their way to + // surface internal-server-error for those, which both confused + // clients (real 5xx vs. wrong URL) and burned ops attention on + // monitoring alerts. Mirror knowledgebase.go's mapping. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, errors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/initialization.go b/internal/handler/initialization.go index db8635cbe..c4018b152 100644 --- a/internal/handler/initialization.go +++ b/internal/handler/initialization.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + stderrors "errors" "fmt" "io" "math/rand" @@ -13,6 +14,7 @@ import ( "sync" "time" + "github.com/Tencent/WeKnora/internal/application/repository" chatpipeline "github.com/Tencent/WeKnora/internal/application/service/chat_pipeline" "github.com/Tencent/WeKnora/internal/assets" "github.com/Tencent/WeKnora/internal/config" @@ -505,6 +507,13 @@ func (h *InitializationHandler) bindInitializationRequest(ctx context.Context, c func (h *InitializationHandler) getKnowledgeBaseForInitialization(ctx context.Context, kbIdStr string) (*types.KnowledgeBase, error) { kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbIdStr) if err != nil { + // The repo's not-found sentinel must surface as 404, not 500. + // Without this, every probe of a stale kb id from the + // initialization flow burns ops attention with a fake server + // error. See knowledgebase.go:validateAndGetKnowledgeBase. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("知识库不存在") + } logger.ErrorWithFields(ctx, err, map[string]interface{}{"kbId": utils.SanitizeForLog(kbIdStr)}) return nil, errors.NewInternalServerError("获取知识库信息失败: " + err.Error()) } @@ -1303,6 +1312,12 @@ func (h *InitializationHandler) GetCurrentConfigByKB(c *gin.Context) { // 获取指定知识库信息 kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbIdStr) if err != nil { + // Mirror getKnowledgeBaseForInitialization above: missing / + // cross-tenant kb ids are 404, not 500. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + c.Error(errors.NewNotFoundError("知识库不存在")) + return + } logger.Error(ctx, "Failed to get knowledge base", err) c.Error(errors.NewInternalServerError("获取知识库信息失败: " + err.Error())) return diff --git a/internal/handler/knowledge.go b/internal/handler/knowledge.go index d94180f16..0b7d0b10f 100644 --- a/internal/handler/knowledge.go +++ b/internal/handler/knowledge.go @@ -76,6 +76,13 @@ func (h *KnowledgeHandler) validateKnowledgeBaseAccessWithKBID(c *gin.Context, k } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // Same not-found-vs-real-error split as knowledgebase.go's + // validateAndGetKnowledgeBase: ErrKnowledgeBaseNotFound is the + // expected outcome for a probed/stale kb id and must surface as + // 404, not the generic 500 the original code produced. + if goerrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, kbID, 0, "", errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, kbID, 0, "", errors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/knowledgebase.go b/internal/handler/knowledgebase.go index 3abd0447e..2b64a925d 100644 --- a/internal/handler/knowledgebase.go +++ b/internal/handler/knowledgebase.go @@ -181,6 +181,14 @@ func (h *KnowledgeBaseHandler) validateAndGetKnowledgeBase(c *gin.Context) (*typ // Verify tenant has permission to access this knowledge base kb, err := h.service.GetKnowledgeBaseByID(ctx, id) if err != nil { + // repo.GetKnowledgeBaseByID surfaces ErrKnowledgeBaseNotFound for + // missing or cross-tenant rows. Map it to 404 here so the four + // callers (Get / Update / Delete / TogglePin / Copy / Hybrid-search + // path) don't have to wrap NewInternalServerError into a 500 for + // every probe of a non-existent id. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, id, 0, "", apperrors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, id, 0, "", apperrors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/knowledgebase_not_found_test.go b/internal/handler/knowledgebase_not_found_test.go new file mode 100644 index 000000000..eb6f7a281 --- /dev/null +++ b/internal/handler/knowledgebase_not_found_test.go @@ -0,0 +1,135 @@ +package handler + +import ( + "context" + stderrors "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/Tencent/WeKnora/internal/application/repository" + 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" +) + +// validateAndGetKnowledgeBase (knowledgebase.go) and the four sibling +// helpers in faq.go / knowledge.go / tag.go / initialization.go used to +// wrap every Get*ByID error — including the well-known +// repository.ErrKnowledgeBaseNotFound sentinel — as a 500. That turned +// every probe of a stale or cross-tenant KB id into a fake "internal +// server error" envelope. These tests pin the corrected mapping (404) +// at the HTTP boundary so a future refactor can't quietly regress it. +// +// The wrapped-sentinel cases are the actual security boundary: if a +// caller drops the stderrors.Is comparison and reverts to `==`, the +// fmt.Errorf("%w") path silently fails over to 500 and the tests below +// fail before the change ships. + +// stubKBOnlyService implements just enough of KnowledgeBaseService to +// drive validateAndGetKnowledgeBase. Embedding the interface keeps every +// other method nil-panicky on purpose so a future test that reaches +// outside the contract fails loudly. +type stubKBOnlyService struct { + interfaces.KnowledgeBaseService + getByID func(ctx context.Context, id string) (*types.KnowledgeBase, error) + fillKnowledgeBaseCounts func(ctx context.Context, kb *types.KnowledgeBase) error +} + +func (s *stubKBOnlyService) GetKnowledgeBaseByID(ctx context.Context, id string) (*types.KnowledgeBase, error) { + return s.getByID(ctx, id) +} + +func (s *stubKBOnlyService) FillKnowledgeBaseCounts(ctx context.Context, kb *types.KnowledgeBase) error { + if s.fillKnowledgeBaseCounts != nil { + return s.fillKnowledgeBaseCounts(ctx, kb) + } + return nil +} + +// newKBHandlerTestRouter mounts the production ErrorHandler so +// c.Error(NewNotFoundError(...)) renders as the real 404 envelope. +// Tenant id and user id are injected by a tiny middleware so +// validateAndGetKnowledgeBase doesn't bail at the unauthorized branch. +func newKBHandlerTestRouter(svc interfaces.KnowledgeBaseService) *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(middleware.ErrorHandler()) + r.Use(func(c *gin.Context) { + c.Set(types.TenantIDContextKey.String(), uint64(1)) + c.Set(types.UserIDContextKey.String(), "u-test") + c.Next() + }) + h := &KnowledgeBaseHandler{service: svc} + r.GET("/knowledge-bases/:id", h.GetKnowledgeBase) + return r +} + +func TestKBHandlerMapsErrKnowledgeBaseNotFoundToNotFound(t *testing.T) { + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, repository.ErrKnowledgeBaseNotFound + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/missing-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("ErrKnowledgeBaseNotFound must map to 404, got %d body=%s", w.Code, w.Body.String()) + } +} + +func TestKBHandlerHonoursWrappedErrKnowledgeBaseNotFound(t *testing.T) { + // Regression test against a stale `==` sentinel comparison: errors.Is + // unwraps fmt.Errorf("%w", ...); a literal `==` does not. If anyone + // reverts the comparison this test fails before the change ships. + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, fmt.Errorf("loading kb: %w", repository.ErrKnowledgeBaseNotFound) + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/missing-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("wrapped ErrKnowledgeBaseNotFound 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. Code 1003 is ErrNotFound. + if !strings.Contains(w.Body.String(), `"code":1003`) { + t.Fatalf("expected NotFound envelope with code=1003, got body=%s", w.Body.String()) + } +} + +func TestKBHandlerKeeps500ForGenuineInfraErrors(t *testing.T) { + // The mapping is *only* for the not-found sentinel — every other + // error must still surface as a real 5xx so monitoring catches + // genuine DB / repo failures. + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, stderrors.New("connection refused") + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/some-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("non-sentinel errors must remain 500, got %d body=%s", w.Code, w.Body.String()) + } +} + +// guardAgainstStaleSentinelEquality is a compile-time assertion: if a +// future refactor accidentally drops the apperrors / stderrors imports +// from THIS file, the test stops compiling and the human gets a clear +// signal that something tied to the not-found mapping went away. +var ( + _ = stderrors.Is + _ = apperrors.NewNotFoundError +) diff --git a/internal/handler/tag.go b/internal/handler/tag.go index 25b3e7b25..95f4f5bd4 100644 --- a/internal/handler/tag.go +++ b/internal/handler/tag.go @@ -2,11 +2,13 @@ package handler import ( "context" + stderrors "errors" "net/http" "strconv" "github.com/gin-gonic/gin" + "github.com/Tencent/WeKnora/internal/application/repository" "github.com/Tencent/WeKnora/internal/errors" "github.com/Tencent/WeKnora/internal/logger" "github.com/Tencent/WeKnora/internal/types" @@ -55,6 +57,12 @@ func (h *TagHandler) effectiveCtxForKB(c *gin.Context, kbID string) (context.Con } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // Same not-found-vs-real-error split as the other helper sites: + // a missing or cross-tenant kb id should render as 404 so clients + // can tell "wrong URL" from a real 5xx. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, errors.NewInternalServerError(err.Error()) }