From def92bb74fcde45dfc1be84cd6d63cf47113aa0b Mon Sep 17 00:00:00 2001 From: wizardchen Date: Mon, 13 Jul 2026 12:12:09 +0800 Subject: [PATCH] fix(router): restrict KB-scoped file proxy to exports/ paths Borrowers with shared-KB read access could previously request any owner-tenant storage path via /knowledge-bases/:id/files. Limit the proxy to exports/ embedded images; raw knowledge uploads continue to use /knowledge/:id/download. --- internal/router/router.go | 4 +-- internal/router/router_files_test.go | 29 ++++++++++++++++ internal/utils/presign.go | 49 +++++++++++++++++++++++++++- internal/utils/presign_test.go | 12 +++++++ 4 files changed, 91 insertions(+), 3 deletions(-) diff --git a/internal/router/router.go b/internal/router/router.go index 1d36aa35..217ad2f2 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -1642,8 +1642,8 @@ func newKBScopedFileServeHandler( return } - if err := secutils.ValidateStoragePathTenant(filePath, ownerTenantID); err != nil { - logger.Warnf(ctx, "[Router] /knowledge-bases/:id/files denied path not owned by KB tenant: owner_tenant_id=%d file_path=%q err=%v", + if err := secutils.ValidateKBScopedStoragePath(filePath, ownerTenantID); err != nil { + logger.Warnf(ctx, "[Router] /knowledge-bases/:id/files denied path not allowed for KB proxy: owner_tenant_id=%d file_path=%q err=%v", ownerTenantID, filePath, err) c.JSON(http.StatusForbidden, gin.H{"error": "forbidden: file path not accessible"}) return diff --git a/internal/router/router_files_test.go b/internal/router/router_files_test.go index aa8a7e9f..f30f263b 100644 --- a/internal/router/router_files_test.go +++ b/internal/router/router_files_test.go @@ -266,6 +266,35 @@ func TestKBScopedFilesRejectsPathNotOwnedByKBTenant(t *testing.T) { } } +// KB-scoped proxy is for embedded exports/ images only; raw knowledge uploads +// must use /knowledge/:id/download even when the tenant matches. +func TestKBScopedFilesRejectsNonExportsPath(t *testing.T) { + gin.SetMode(gin.TestMode) + t.Setenv("STORAGE_TYPE", "local") + + const ownerTenantID = uint64(10008) + engine := newKBScopedFilesTestEngine( + ownerTenantID, + &stubTenantService{get: func(_ context.Context, id uint64) (*types.Tenant, error) { + t.Fatalf("GetTenantByID should not be called for non-exports path, got %d", id) + return nil, nil + }}, + &stubFileService{getFile: func(_ context.Context, filePath string) (io.ReadCloser, error) { + t.Fatalf("GetFile should not be called for non-exports path, got %q", filePath) + return nil, nil + }}, + ) + + req := httptest.NewRequest(http.MethodGet, + "/knowledge-bases/kb-1/files?file_path="+url.QueryEscape("local://10008/knowledge-id/123.pdf"), nil) + recorder := httptest.NewRecorder() + engine.ServeHTTP(recorder, req) + + if got, want := recorder.Code, http.StatusForbidden; got != want { + t.Fatalf("status = %d, want %d", got, want) + } +} + func TestKBScopedFilesRequiresFilePath(t *testing.T) { gin.SetMode(gin.TestMode) diff --git a/internal/utils/presign.go b/internal/utils/presign.go index 5662f352..139f9ab0 100644 --- a/internal/utils/presign.go +++ b/internal/utils/presign.go @@ -95,9 +95,16 @@ func VerifyFileURLSig(filePath string, tenantID uint64, expiresStr, sig string) return hmac.Equal([]byte(expected), []byte(sig)) } +// kbScopedExportsSegment is the only storage prefix served by the KB-scoped +// file proxy. Embedded wiki/chunk images land under exports/; raw knowledge +// uploads use {tenant}/{knowledgeID}/... and are served via +// /knowledge/{id}/download instead. +const kbScopedExportsSegment = "exports" + // ValidateStoragePathTenant ensures the tenant segment embedded in a provider:// // storage path matches the authenticated caller's tenant. Cross-tenant access -// must use /api/v1/files/presigned with an HMAC bound to the resource owner. +// for arbitrary tenant paths uses /api/v1/files/presigned with an HMAC bound to +// the resource owner; KB-scoped shared rendering uses ValidateKBScopedStoragePath. func ValidateStoragePathTenant(filePath string, tenantID uint64) error { pathTenant := ParseTenantIDFromStoragePath(filePath) if pathTenant == 0 { @@ -109,6 +116,46 @@ func ValidateStoragePathTenant(filePath string, tenantID uint64) error { return nil } +// ValidateKBScopedStoragePath is used by GET /knowledge-bases/:id/files. It +// requires the path to belong to the KB owner tenant and to live under the +// exports/ namespace used for embedded images (SaveBytes / multimodal output). +// This prevents borrowers with shared-KB read access from using the proxy to +// fetch arbitrary owner-tenant objects such as raw knowledge uploads. +func ValidateKBScopedStoragePath(filePath string, tenantID uint64) error { + if err := ValidateStoragePathTenant(filePath, tenantID); err != nil { + return err + } + if !storagePathHasExportsScope(filePath, tenantID) { + return fmt.Errorf("storage path is outside KB-scoped exports namespace") + } + return nil +} + +// storagePathHasExportsScope reports whether tenantID appears next to an +// exports segment in either canonical layout: +// - {tenant}/exports/... (local, minio, s3, most cloud backends) +// - exports/{tenant}/... (OSS temp-bucket layout) +func storagePathHasExportsScope(filePath string, tenantID uint64) bool { + _, rest, ok := strings.Cut(filePath, "://") + if !ok { + return false + } + tenantSeg := strconv.FormatUint(tenantID, 10) + parts := strings.Split(rest, "/") + for i, part := range parts { + if part != tenantSeg { + continue + } + if i+1 < len(parts) && parts[i+1] == kbScopedExportsSegment { + return true + } + if i > 0 && parts[i-1] == kbScopedExportsSegment { + return true + } + } + return false +} + // ParseTenantIDFromStoragePath extracts the tenant ID from a provider:// storage path. // Storage paths follow the convention: {scheme}://.../{tenantID}/... // Returns 0 if the path does not contain a valid tenant ID. diff --git a/internal/utils/presign_test.go b/internal/utils/presign_test.go index 5ceb0ea9..4cb24070 100644 --- a/internal/utils/presign_test.go +++ b/internal/utils/presign_test.go @@ -92,6 +92,18 @@ func TestValidateStoragePathTenant(t *testing.T) { assert.Error(t, ValidateStoragePathTenant("local://docs/example.txt", 42)) } +func TestValidateKBScopedStoragePath(t *testing.T) { + const tenantID uint64 = 10008 + + assert.NoError(t, ValidateKBScopedStoragePath("local://10008/exports/img.jpg", tenantID)) + assert.NoError(t, ValidateKBScopedStoragePath("minio://bucket/10008/exports/uuid.png", tenantID)) + assert.NoError(t, ValidateKBScopedStoragePath("oss://bucket/exports/10008/uuid.png", tenantID)) + + assert.Error(t, ValidateKBScopedStoragePath("local://10008/knowledge-id/123.pdf", tenantID)) + assert.Error(t, ValidateKBScopedStoragePath("local://9999/exports/img.jpg", tenantID)) + assert.Error(t, ValidateKBScopedStoragePath("local://10008/other/img.jpg", tenantID)) +} + func TestParseTenantIDFromStoragePath(t *testing.T) { tests := []struct { path string