From aad059ab6e05a6fff53983b747ec6e6dd81eeff5 Mon Sep 17 00:00:00 2001 From: ryan Date: Tue, 22 Sep 2026 14:16:21 +0800 Subject: [PATCH] fix(objectstore): decouple WebDAV targetPath from logical storage key and fix basePath duplication - WebDAV Put now returns PutResult with driver-agnostic logical relative key (relKey), keeping database records decoupled from mount basePath. - targetPath mounts logical keys to the remote WebDAV server path, and transparently handles legacy database records containing basePath or duplicate basePath prefixes. - Make localBackend path resolution resilient to keys with leading slashes or legacy absolute paths outside local root by safely mounting them as relative paths. - Add comprehensive unit tests for WebDAV targetPath, relKey, end-to-end roundtrip with in-memory WebDAV server, and local storage leading slash handling. --- internal/infra/objectstore/local.go | 30 +-- internal/infra/objectstore/local_test.go | 15 ++ internal/infra/objectstore/webdav.go | 54 ++++- internal/infra/objectstore/webdav_test.go | 257 ++++++++++++++++++++++ 4 files changed, 331 insertions(+), 25 deletions(-) create mode 100644 internal/infra/objectstore/webdav_test.go diff --git a/internal/infra/objectstore/local.go b/internal/infra/objectstore/local.go index 0f02e5c8..f525e79b 100644 --- a/internal/infra/objectstore/local.go +++ b/internal/infra/objectstore/local.go @@ -91,30 +91,32 @@ func (b *localBackend) Test(_ context.Context) error { return os.MkdirAll(b.root, storageDirPerm) } +func isWithinRoot(root, target string) bool { + absRoot, err := filepath.Abs(root) + if err != nil { + return false + } + absTarget, err := filepath.Abs(target) + if err != nil { + return false + } + rel, err := filepath.Rel(absRoot, absTarget) + return err == nil && !strings.HasPrefix(rel, "..") +} + func (b *localBackend) path(key string) (string, error) { if filepath.IsAbs(key) { cleanPath := filepath.Clean(key) - absRoot, err := filepath.Abs(b.root) - if err != nil { - return "", err + if isWithinRoot(b.root, cleanPath) { + return cleanPath, nil } - absPath, err := filepath.Abs(cleanPath) - if err != nil { - return "", err - } - rel, err := filepath.Rel(absRoot, absPath) - if err != nil || strings.HasPrefix(rel, "..") { - return "", errors.New("storage key escapes local root") - } - return cleanPath, nil } cleanKey := filepath.Clean(filepath.FromSlash(strings.TrimPrefix(key, "/"))) if cleanKey == "." || cleanKey == "" || strings.HasPrefix(cleanKey, "..") { return "", fmt.Errorf("invalid local storage key %q", key) } path := filepath.Join(b.root, cleanKey) - rel, err := filepath.Rel(b.root, path) - if err != nil || strings.HasPrefix(rel, "..") { + if !isWithinRoot(b.root, path) { return "", errors.New("storage key escapes local root") } return path, nil diff --git a/internal/infra/objectstore/local_test.go b/internal/infra/objectstore/local_test.go index 8525f078..72bbafd3 100644 --- a/internal/infra/objectstore/local_test.go +++ b/internal/infra/objectstore/local_test.go @@ -48,4 +48,19 @@ func TestLocalBackendRoundTrip(t *testing.T) { if _, err := backend.Get(ctx, key); err == nil { t.Errorf("Get(%q) after Delete() returned nil error", key) } + + // Test key with leading slash + slashKey := "/uploads/2026/06/13/slash_test.txt" + putSlashRes, err := backend.Put(ctx, slashKey, bytes.NewBufferString(content), int64(len(content)), "text/plain") + if err != nil { + t.Fatalf("Put(%q) returned error: %v", slashKey, err) + } + if putSlashRes.Key != slashKey { + t.Errorf("Put(%q) key = %q, want %q", slashKey, putSlashRes.Key, slashKey) + } + objSlash, err := backend.Get(ctx, slashKey) + if err != nil { + t.Fatalf("Get(%q) returned error: %v", slashKey, err) + } + _ = objSlash.Body.Close() } diff --git a/internal/infra/objectstore/webdav.go b/internal/infra/objectstore/webdav.go index 5d8e3e65..8a27806e 100644 --- a/internal/infra/objectstore/webdav.go +++ b/internal/infra/objectstore/webdav.go @@ -32,11 +32,15 @@ type webDAVBackend struct { } func newWebDAVBackend(cfg WebDAVConfig) (*webDAVBackend, error) { + basePath := strings.Trim(path.Clean("/"+cfg.BasePath), "/") + if basePath == "." { + basePath = "" + } return &webDAVBackend{ endpoint: strings.TrimRight(cfg.Endpoint, "/"), username: cfg.Username, password: cfg.Password, - basePath: strings.Trim(cfg.BasePath, "/"), + basePath: basePath, }, nil } @@ -50,27 +54,27 @@ func (b *webDAVBackend) newClient(ctx context.Context) *gowebdav.Client { } func (b *webDAVBackend) Put(ctx context.Context, key string, body io.Reader, size int64, _ string) (PutResult, error) { - key = b.key(key) + target := b.targetPath(key) client := b.newClient(ctx) - if dir := path.Dir(key); dir != "." && dir != "/" { + if dir := path.Dir(target); dir != "." && dir != "/" { if err := client.MkdirAll(dir, storageDirPerm); err != nil { return PutResult{}, fmt.Errorf("create WebDAV directory: %w", err) } } - if err := client.WriteStreamWithLength(key, body, size, storageFilePerm); err != nil { + if err := client.WriteStreamWithLength(target, body, size, storageFilePerm); err != nil { return PutResult{}, fmt.Errorf("put WebDAV object: %w", err) } - return PutResult{Key: key}, nil + return PutResult{Key: b.relKey(key)}, nil } func (b *webDAVBackend) Get(ctx context.Context, key string) (*Object, error) { - key = b.key(key) + target := b.targetPath(key) client := b.newClient(ctx) - info, err := client.Stat(key) + info, err := client.Stat(target) if err != nil { return nil, fmt.Errorf("stat WebDAV object: %w", err) } - body, err := client.ReadStream(key) + body, err := client.ReadStream(target) if err != nil { return nil, fmt.Errorf("get WebDAV object: %w", err) } @@ -83,7 +87,7 @@ func (b *webDAVBackend) Get(ctx context.Context, key string) (*Object, error) { func (b *webDAVBackend) Delete(ctx context.Context, key string) error { client := b.newClient(ctx) - if err := client.Remove(b.key(key)); err != nil { + if err := client.Remove(b.targetPath(key)); err != nil { return fmt.Errorf("delete WebDAV object: %w", err) } return nil @@ -97,6 +101,34 @@ func (b *webDAVBackend) Test(ctx context.Context) error { return nil } -func (b *webDAVBackend) key(key string) string { - return "/" + path.Join(b.basePath, strings.TrimLeft(key, "/")) +// relKey extracts the clean, normalized, relative logical key (e.g. "uploads/2026/09/17/xxx.jpg") +// to be persisted in the database, stripping any driver-specific basePath and leading slashes. +func (b *webDAVBackend) relKey(key string) string { + cleanKey := strings.Trim(path.Clean("/"+strings.ReplaceAll(key, "\\", "/")), "/") + if cleanKey == "." { + return "" + } + if b.basePath != "" { + for cleanKey == b.basePath || strings.HasPrefix(cleanKey, b.basePath+"/") { + cleanKey = strings.TrimPrefix(cleanKey, b.basePath) + cleanKey = strings.TrimPrefix(cleanKey, "/") + } + } + return cleanKey +} + +// targetPath resolves any key (relative, legacy with basePath, or corrupted with duplicate basePath) +// into the absolute path used to access the object on the WebDAV server. +func (b *webDAVBackend) targetPath(key string) string { + rel := b.relKey(key) + if b.basePath == "" { + if rel == "" { + return "/" + } + return "/" + rel + } + if rel == "" { + return "/" + b.basePath + } + return "/" + b.basePath + "/" + rel } diff --git a/internal/infra/objectstore/webdav_test.go b/internal/infra/objectstore/webdav_test.go new file mode 100644 index 00000000..b12710f6 --- /dev/null +++ b/internal/infra/objectstore/webdav_test.go @@ -0,0 +1,257 @@ +// Copyright 2026 Arctel.net +// SPDX-License-Identifier: Apache-2.0 + +package objectstore + +import ( + "bytes" + "context" + "io" + "net/http/httptest" + "testing" + + "golang.org/x/net/webdav" +) + +func TestWebDAVTargetPath(t *testing.T) { + tests := []struct { + name string + basePath string + key string + expected string + }{ + { + name: "no base path, relative key", + basePath: "", + key: "uploads/2026/09/17/1.jpg", + expected: "/uploads/2026/09/17/1.jpg", + }, + { + name: "no base path, leading slash key", + basePath: "", + key: "/uploads/2026/09/17/1.jpg", + expected: "/uploads/2026/09/17/1.jpg", + }, + { + name: "with base path, relative key (uploading)", + basePath: "/DockerData/openflare_data_webdav", + key: "uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "with base path, key with leading slash (uploading)", + basePath: "/DockerData/openflare_data_webdav", + key: "/uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "with base path, key already has base path (reading from legacy DB)", + basePath: "/DockerData/openflare_data_webdav", + key: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "with base path without leading slash, key already has base path", + basePath: "DockerData/openflare_data_webdav", + key: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "with base path with trailing slash, key already has base path", + basePath: "/DockerData/openflare_data_webdav/", + key: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "with base path, key already has double base path from previous bug", + basePath: "/DockerData/openflare_data_webdav", + key: "/DockerData/openflare_data_webdav/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "key with similar prefix name that is not a directory match", + basePath: "/data", + key: "/data_backup/uploads/1.jpg", + expected: "/data/data_backup/uploads/1.jpg", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend, err := newWebDAVBackend(WebDAVConfig{ + Endpoint: "http://127.0.0.1:5005", + BasePath: tt.basePath, + }) + if err != nil { + t.Fatalf("newWebDAVBackend failed: %v", err) + } + actual := backend.targetPath(tt.key) + if actual != tt.expected { + t.Errorf("targetPath(%q) = %q, want %q", tt.key, actual, tt.expected) + } + }) + } +} + +func TestWebDAVRelKey(t *testing.T) { + tests := []struct { + name string + basePath string + key string + expected string + }{ + { + name: "relative key remains relative", + basePath: "/DockerData/openflare_data_webdav", + key: "uploads/2026/09/17/105102954490499072.jpg", + expected: "uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "leading slash key is stripped to relative", + basePath: "/DockerData/openflare_data_webdav", + key: "/uploads/2026/09/17/105102954490499072.jpg", + expected: "uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "legacy key with basePath is stripped to relative", + basePath: "/DockerData/openflare_data_webdav", + key: "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "corrupted key with duplicate basePath is stripped to relative", + basePath: "/DockerData/openflare_data_webdav", + key: "/DockerData/openflare_data_webdav/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg", + expected: "uploads/2026/09/17/105102954490499072.jpg", + }, + { + name: "empty basePath preserves relative key", + basePath: "", + key: "/uploads/2026/09/17/105102954490499072.jpg", + expected: "uploads/2026/09/17/105102954490499072.jpg", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend, err := newWebDAVBackend(WebDAVConfig{ + Endpoint: "http://127.0.0.1:5005", + BasePath: tt.basePath, + }) + if err != nil { + t.Fatalf("newWebDAVBackend failed: %v", err) + } + actual := backend.relKey(tt.key) + if actual != tt.expected { + t.Errorf("relKey(%q) = %q, want %q", tt.key, actual, tt.expected) + } + }) + } +} + +func TestWebDAVBackendRoundTrip(t *testing.T) { + wdHandler := &webdav.Handler{ + FileSystem: webdav.NewMemFS(), + LockSystem: webdav.NewMemLS(), + } + server := httptest.NewServer(wdHandler) + defer server.Close() + + ctx := context.Background() + basePath := "/DockerData/openflare_data_webdav" + backend, err := newWebDAVBackend(WebDAVConfig{ + Endpoint: server.URL, + BasePath: basePath, + }) + if err != nil { + t.Fatalf("newWebDAVBackend failed: %v", err) + } + + // 1. Test connection + if err := backend.Test(ctx); err != nil { + t.Fatalf("backend.Test failed: %v", err) + } + + // 2. Put object using relative key (typical upload flow) + origContent := []byte("test image content 12345") + objectKey := "uploads/2026/09/17/105102954490499072.jpg" + putRes, err := backend.Put(ctx, objectKey, bytes.NewReader(origContent), int64(len(origContent)), "image/jpeg") + if err != nil { + t.Fatalf("backend.Put failed: %v", err) + } + + // PutResult.Key MUST be the pure logical relative key, decoupled from basePath + expectedLogicalKey := "uploads/2026/09/17/105102954490499072.jpg" + if putRes.Key != expectedLogicalKey { + t.Errorf("putRes.Key = %q, want %q", putRes.Key, expectedLogicalKey) + } + + // 3. Get object using logical key (new standard upload flow) + obj, err := backend.Get(ctx, putRes.Key) + if err != nil { + t.Fatalf("backend.Get with putRes.Key failed: %v", err) + } + defer obj.Body.Close() + + bodyBytes, err := io.ReadAll(obj.Body) + if err != nil { + t.Fatalf("read body failed: %v", err) + } + if !bytes.Equal(bodyBytes, origContent) { + t.Errorf("read content = %q, want %q", string(bodyBytes), string(origContent)) + } + + // 4. Get object using legacy key containing basePath (existing DB records from before fix) + legacyKey := "/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg" + obj2, err := backend.Get(ctx, legacyKey) + if err != nil { + t.Fatalf("backend.Get with legacyKey failed: %v", err) + } + defer obj2.Body.Close() + bodyBytes2, err := io.ReadAll(obj2.Body) + if err != nil { + t.Fatalf("read body failed: %v", err) + } + if !bytes.Equal(bodyBytes2, origContent) { + t.Errorf("read content = %q, want %q", string(bodyBytes2), string(origContent)) + } + + // 5. Get object using accidental double basePath (defensive recovery for corrupted DB records) + doubledKey := "/DockerData/openflare_data_webdav/DockerData/openflare_data_webdav/uploads/2026/09/17/105102954490499072.jpg" + obj3, err := backend.Get(ctx, doubledKey) + if err != nil { + t.Fatalf("backend.Get with doubledKey failed: %v", err) + } + defer obj3.Body.Close() + bodyBytes3, err := io.ReadAll(obj3.Body) + if err != nil { + t.Fatalf("read body failed: %v", err) + } + if !bytes.Equal(bodyBytes3, origContent) { + t.Errorf("read content = %q, want %q", string(bodyBytes3), string(origContent)) + } + + // 6. Delete object using logical key + if err := backend.Delete(ctx, putRes.Key); err != nil { + t.Fatalf("backend.Delete failed: %v", err) + } + + // 7. Verify object is deleted + _, err = backend.Get(ctx, putRes.Key) + if err == nil { + t.Fatalf("expected error after delete, got nil") + } + + // 8. Put again, and delete using legacy key format + _, err = backend.Put(ctx, objectKey, bytes.NewReader(origContent), int64(len(origContent)), "image/jpeg") + if err != nil { + t.Fatalf("second backend.Put failed: %v", err) + } + if err := backend.Delete(ctx, legacyKey); err != nil { + t.Fatalf("backend.Delete with legacyKey failed: %v", err) + } + _, err = backend.Get(ctx, objectKey) + if err == nil { + t.Fatalf("expected error after delete with legacyKey, got nil") + } +}