From 7c7d7feed302009a4603e38af2fb690732e7978c Mon Sep 17 00:00:00 2001 From: devv-eve Date: Mon, 13 Apr 2026 21:01:50 -0700 Subject: [PATCH] fix(storage): scope S3 upload keys by workspace (#936) * fix(storage): scope S3 upload keys by workspace Upload keys now use `workspaces/{workspace_id}/{uuid}.{ext}` instead of flat `{uuid}.{ext}`, isolating file storage per workspace. Files uploaded without workspace context (e.g. avatars) keep the flat key structure. Refs: MUL-577 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(storage): scope user uploads under users/{user_id}/ prefix Non-workspace uploads (avatars, profile images) now use `users/{user_id}/{uuid}.{ext}` instead of flat `{uuid}.{ext}`, matching the workspace-scoped pattern from the previous commit. Refs: MUL-577 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(storage): fix LocalStorage for nested key paths - Add MkdirAll before WriteFile to create intermediate directories for workspace/user-scoped keys - Fix KeyFromURL to preserve full path after /uploads/ prefix instead of stripping to just the filename - Update tests to match new behavior Refs: MUL-577 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(upload): validate ownership before writing to storage Move Storage.Upload after issue_id/comment_id ownership validation to prevent orphaned files in S3 when validation fails. Previously, the file was uploaded first and validation happened after, leaving files in workspace-scoped S3 prefixes even on rejected requests. Refs: MUL-577 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(upload): restore workspace membership check before upload The membership check was accidentally removed during the upload reordering refactor. Without it, any authenticated user could upload files to any workspace by setting the X-Workspace-ID header. Also restores the comment explaining the 200-on-DB-error behavior. Refs: MUL-577 Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Devv Co-authored-by: Claude Opus 4.6 (1M context) --- server/internal/handler/file.go | 39 +++++++++++++++++++-------- server/internal/storage/local.go | 11 ++++---- server/internal/storage/local_test.go | 5 ++-- 3 files changed, 36 insertions(+), 19 deletions(-) diff --git a/server/internal/handler/file.go b/server/internal/handler/file.go index d9b9ff11a..3decabc2a 100644 --- a/server/internal/handler/file.go +++ b/server/internal/handler/file.go @@ -160,16 +160,15 @@ func (h *Handler) UploadFile(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, "internal error") return } - key := id.String() + path.Ext(header.Filename) - - link, err := h.Storage.Upload(r.Context(), key, data, contentType, header.Filename) - if err != nil { - slog.Error("file upload failed", "error", err) - writeError(w, http.StatusInternalServerError, "upload failed") - return + filename := id.String() + path.Ext(header.Filename) + var key string + if workspaceID != "" { + key = "workspaces/" + workspaceID + "/" + filename + } else { + key = "users/" + userID + "/" + filename } - // If workspace context is available, create an attachment record. + // If workspace context is available, validate membership before uploading. if workspaceID != "" { if _, err := h.getWorkspaceMember(r.Context(), userID, workspaceID); err != nil { writeError(w, http.StatusForbidden, "not a member of this workspace") @@ -184,12 +183,10 @@ func (h *Handler) UploadFile(w http.ResponseWriter, r *http.Request) { UploaderType: uploaderType, UploaderID: parseUUID(uploaderID), Filename: header.Filename, - Url: link, ContentType: contentType, SizeBytes: int64(len(data)), } - // Optional issue_id / comment_id from form fields — validate ownership. if issueID := r.FormValue("issue_id"); issueID != "" { issue, err := h.Queries.GetIssueInWorkspace(r.Context(), db.GetIssueInWorkspaceParams{ ID: parseUUID(issueID), @@ -210,6 +207,14 @@ func (h *Handler) UploadFile(w http.ResponseWriter, r *http.Request) { params.CommentID = comment.ID } + link, err := h.Storage.Upload(r.Context(), key, data, contentType, header.Filename) + if err != nil { + slog.Error("file upload failed", "error", err) + writeError(w, http.StatusInternalServerError, "upload failed") + return + } + params.Url = link + att, err := h.Queries.CreateAttachment(r.Context(), params) if err != nil { slog.Error("failed to create attachment record", "error", err) @@ -219,9 +224,21 @@ func (h *Handler) UploadFile(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, h.attachmentToResponse(att)) return } + + writeJSON(w, http.StatusOK, map[string]string{ + "filename": header.Filename, + "link": link, + }) + return } - // Fallback response (no workspace context, e.g. avatar upload) + // No workspace context (e.g. avatar upload) — upload directly. + link, err := h.Storage.Upload(r.Context(), key, data, contentType, header.Filename) + if err != nil { + slog.Error("file upload failed", "error", err) + writeError(w, http.StatusInternalServerError, "upload failed") + return + } writeJSON(w, http.StatusOK, map[string]string{ "filename": header.Filename, "link": link, diff --git a/server/internal/storage/local.go b/server/internal/storage/local.go index a2c76c0e9..cc2950207 100644 --- a/server/internal/storage/local.go +++ b/server/internal/storage/local.go @@ -48,12 +48,8 @@ func (s *LocalStorage) KeyFromURL(rawURL string) string { } prefix := "/uploads/" - if strings.HasPrefix(rawURL, prefix) { - filename := strings.TrimPrefix(rawURL, prefix) - if i := strings.LastIndex(filename, "/"); i >= 0 { - return filename[i+1:] - } - return filename + if idx := strings.Index(rawURL, prefix); idx >= 0 { + return rawURL[idx+len(prefix):] } if i := strings.LastIndex(rawURL, "/"); i >= 0 { return rawURL[i+1:] @@ -81,6 +77,9 @@ func (s *LocalStorage) DeleteKeys(ctx context.Context, keys []string) { func (s *LocalStorage) Upload(ctx context.Context, key string, data []byte, contentType string, filename string) (string, error) { dest := filepath.Join(s.uploadDir, key) + if err := os.MkdirAll(filepath.Dir(dest), 0755); err != nil { + return "", fmt.Errorf("local storage MkdirAll: %w", err) + } if err := os.WriteFile(dest, data, 0644); err != nil { return "", fmt.Errorf("local storage WriteFile: %w", err) } diff --git a/server/internal/storage/local_test.go b/server/internal/storage/local_test.go index d3b7ff487..e68bd4287 100644 --- a/server/internal/storage/local_test.go +++ b/server/internal/storage/local_test.go @@ -124,7 +124,8 @@ func TestLocalStorage_KeyFromURL(t *testing.T) { expected string }{ {"local URL format", "/uploads/abc123.png", "abc123.png"}, - {"local URL with subdir", "/uploads/2024/01/image.jpg", "image.jpg"}, + {"local URL with subdir", "/uploads/2024/01/image.jpg", "2024/01/image.jpg"}, + {"local URL with workspace prefix", "/uploads/workspaces/ws-123/abc.png", "workspaces/ws-123/abc.png"}, {"just filename", "abc123.png", "abc123.png"}, {"full path", "/some/path/to/file.pdf", "file.pdf"}, } @@ -155,7 +156,7 @@ func TestLocalStorage_KeyFromURL_WithBaseURL(t *testing.T) { expected string }{ {"full URL format", "http://localhost:8080/uploads/abc123.png", "abc123.png"}, - {"full URL with subdir", "http://localhost:8080/uploads/2024/01/image.jpg", "image.jpg"}, + {"full URL with subdir", "http://localhost:8080/uploads/2024/01/image.jpg", "2024/01/image.jpg"}, {"local URL format still works", "/uploads/abc123.png", "abc123.png"}, }