mirror of
https://github.com/Tencent/WeKnora.git
synced 2026-09-19 02:18:25 +08:00
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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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, "://")
|
||||
|
||||
Reference in New Issue
Block a user