fix(kb): map ErrKnowledgeBaseNotFound to 404 across handler helpers

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.
This commit is contained in:
wizardchen
2026-05-14 20:10:04 +08:00
committed by lyingbug
parent 3a0a0cff9c
commit 4fb089d4d7
6 changed files with 183 additions and 0 deletions
+10
View File
@@ -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())
}
+15
View File
@@ -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
+7
View File
@@ -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())
}
+8
View File
@@ -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())
}
@@ -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
)
+8
View File
@@ -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())
}