From f27a1e083cfbb92f7ab09143ed2c3d091aee88ce Mon Sep 17 00:00:00 2001 From: wizardchen Date: Thu, 30 Apr 2026 10:59:54 +0800 Subject: [PATCH] fix(im): prefer tenant ID from context, shorten presigned URL TTL Addresses review feedback on the IM storage URL rewrite: - localFileService.GetFileURL now reads tenant ID from request context first, falling back to ParseTenantIDFromStoragePath only when context is absent. Fixes ambiguity for cloud providers whose paths embed numeric bucket/region names before the tenant segment, which could mint presigned URLs bound to the wrong tenant ID. - Shorten presigned URL default TTL from 24h to 2h. A leaked HMAC key authorizes cross-tenant file reads, so URLs should expire quickly; IM clients fetch referenced images within seconds anyway. - Document ParseTenantIDFromStoragePath as a best-effort fallback. - Add unit tests covering context-first, path-fallback, and the no-external-URL backward-compat path. --- internal/application/service/file/local.go | 8 ++- .../application/service/file/local_test.go | 57 +++++++++++++++++++ internal/utils/presign.go | 11 +++- 3 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 internal/application/service/file/local_test.go diff --git a/internal/application/service/file/local.go b/internal/application/service/file/local.go index 3c03b9c83..63a3f52a3 100644 --- a/internal/application/service/file/local.go +++ b/internal/application/service/file/local.go @@ -11,6 +11,7 @@ import ( "time" "github.com/Tencent/WeKnora/internal/logger" + "github.com/Tencent/WeKnora/internal/types" "github.com/Tencent/WeKnora/internal/types/interfaces" secutils "github.com/Tencent/WeKnora/internal/utils" ) @@ -204,7 +205,12 @@ func (s *localFileService) GetFileURL(ctx context.Context, filePath string) (str // If external URL is configured, generate a presigned HTTP URL. if s.externalURL != "" { - tenantID := secutils.ParseTenantIDFromStoragePath(normalized) + // Prefer tenant ID from context (authoritative); fall back to parsing + // the storage path for callers that don't propagate tenant context. + tenantID, ok := types.TenantIDFromContext(ctx) + if !ok || tenantID == 0 { + tenantID = secutils.ParseTenantIDFromStoragePath(normalized) + } presignedURL, err := secutils.SignFileURL(s.externalURL, normalized, tenantID, 0) if err != nil { logger.Warnf(ctx, "Failed to generate presigned URL for %s: %v, returning local:// path", normalized, err) diff --git a/internal/application/service/file/local_test.go b/internal/application/service/file/local_test.go new file mode 100644 index 000000000..a72c23b4b --- /dev/null +++ b/internal/application/service/file/local_test.go @@ -0,0 +1,57 @@ +package file + +import ( + "context" + "net/url" + "testing" + + "github.com/Tencent/WeKnora/internal/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// extractTenantIDFromPresignedURL pulls the tenant_id query parameter from a +// signed URL. Returns "" when the URL is not parseable as a presigned URL. +func extractTenantIDFromPresignedURL(t *testing.T, presigned string) string { + t.Helper() + u, err := url.Parse(presigned) + require.NoError(t, err) + return u.Query().Get("tenant_id") +} + +// TestLocalGetFileURL_TenantIDFromContext verifies that tenant context wins +// over path parsing — critical when the first numeric segment of the path is +// a bucket name or region (not the tenant). +func TestLocalGetFileURL_TenantIDFromContext(t *testing.T) { + t.Setenv("SYSTEM_AES_KEY", "weknora-test-aes-key-32bytes!!!") + + svc := NewLocalFileService("/data/files", "https://weknora.example.com") + + ctx := context.WithValue(context.Background(), types.TenantIDContextKey, uint64(42)) + got, err := svc.GetFileURL(ctx, "local://1/abc/img.png") + require.NoError(t, err) + // Context tenant (42) must override the path's first numeric segment (1). + assert.Equal(t, "42", extractTenantIDFromPresignedURL(t, got)) +} + +// TestLocalGetFileURL_FallbackToPathParse verifies that when context is +// missing, the service falls back to parsing the tenant ID from the path. +func TestLocalGetFileURL_FallbackToPathParse(t *testing.T) { + t.Setenv("SYSTEM_AES_KEY", "weknora-test-aes-key-32bytes!!!") + + svc := NewLocalFileService("/data/files", "https://weknora.example.com") + + got, err := svc.GetFileURL(context.Background(), "local://7/abc/img.png") + require.NoError(t, err) + assert.Equal(t, "7", extractTenantIDFromPresignedURL(t, got)) +} + +// TestLocalGetFileURL_NoExternalURL verifies backward compatibility: without +// APP_EXTERNAL_URL, GetFileURL still returns the local:// path unchanged. +func TestLocalGetFileURL_NoExternalURL(t *testing.T) { + svc := NewLocalFileService("/data/files", "") + + got, err := svc.GetFileURL(context.Background(), "local://1/abc/img.png") + require.NoError(t, err) + assert.Equal(t, "local://1/abc/img.png", got) +} diff --git a/internal/utils/presign.go b/internal/utils/presign.go index f9d18cac4..2f5365edb 100644 --- a/internal/utils/presign.go +++ b/internal/utils/presign.go @@ -16,7 +16,10 @@ const ( // presignPath is the URL path for presigned file access. presignPath = "/api/v1/files/presigned" // presignDefaultTTL is the default validity period for presigned URLs. - presignDefaultTTL = 24 * time.Hour + // Kept short because the HMAC key alone authorizes cross-tenant access — + // a leaked URL should expire before it can be widely abused. IM clients + // typically fetch and cache images within seconds of receipt. + presignDefaultTTL = 2 * time.Hour ) // getPresignKey returns the HMAC key derived from SYSTEM_AES_KEY. @@ -41,7 +44,7 @@ func signPayload(key []byte, filePath string, tenantID uint64, expires int64) st // baseURL is the external URL of the WeKnora instance (e.g. "https://weknora.example.com"). // filePath is the provider:// storage path (e.g. "local://1/abc/img.png"). // tenantID identifies the tenant that owns the file. -// ttl is how long the URL remains valid (0 uses the default 24h). +// ttl is how long the URL remains valid (0 uses the default presignDefaultTTL). // // Returns ("", error) if the signing key is not configured. func SignFileURL(baseURL, filePath string, tenantID uint64, ttl time.Duration) (string, error) { @@ -95,6 +98,10 @@ func VerifyFileURLSig(filePath string, tenantID uint64, expiresStr, sig string) // 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. +// +// NOTE: This is a best-effort fallback. Prefer passing the tenant ID from +// request context when available — for cloud providers with numeric bucket +// or region names, the first numeric segment may not be the tenant ID. func ParseTenantIDFromStoragePath(filePath string) uint64 { // Strip scheme: "local://1/abc/img.png" → "1/abc/img.png" _, rest, ok := strings.Cut(filePath, "://")