From 9bd7f89e4cbb4aa61842ddeec4b5a436d9f049b3 Mon Sep 17 00:00:00 2001 From: martian56 Date: Sun, 5 Jul 2026 12:46:39 +0400 Subject: [PATCH 1/3] fix(files): allow downloading issue attachments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file-serving endpoint only accepted object paths under "uploads/", but attachments are stored as "attachments//", so every attachment download was rejected with 400 before any lookup — a core feature was completely broken. ServeFile now serves both "uploads/" and "attachments/" prefixes (still rejecting empty paths and path traversal). The prefix check is extracted into a small isServableObjectPath helper with unit tests. Closes #135 Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/internal/handler/upload.go | 14 ++++++++++- apps/api/internal/handler/upload_path_test.go | 24 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 apps/api/internal/handler/upload_path_test.go diff --git a/apps/api/internal/handler/upload.go b/apps/api/internal/handler/upload.go index bec8a5fc..4fabc703 100644 --- a/apps/api/internal/handler/upload.go +++ b/apps/api/internal/handler/upload.go @@ -100,13 +100,25 @@ func (h *UploadHandler) Upload(c *gin.Context) { // ServeFile streams a file from MinIO by path. // GET /api/files/*path +// isServableObjectPath allows only the object prefixes we intend to serve — +// generic uploads (avatars/covers/logos) and issue attachments — and rejects +// empty paths and path traversal. Attachments live under +// "attachments//", so serving only "uploads/" made every +// attachment download fail with 400. +func isServableObjectPath(path string) bool { + if path == "" || strings.Contains(path, "..") { + return false + } + return strings.HasPrefix(path, "uploads/") || strings.HasPrefix(path, "attachments/") +} + func (h *UploadHandler) ServeFile(c *gin.Context) { if h.Minio == nil { c.Status(http.StatusServiceUnavailable) return } path := strings.TrimPrefix(c.Param("path"), "/") - if path == "" || strings.Contains(path, "..") || !strings.HasPrefix(path, "uploads/") { + if !isServableObjectPath(path) { c.Status(http.StatusBadRequest) return } diff --git a/apps/api/internal/handler/upload_path_test.go b/apps/api/internal/handler/upload_path_test.go new file mode 100644 index 00000000..f8eb3beb --- /dev/null +++ b/apps/api/internal/handler/upload_path_test.go @@ -0,0 +1,24 @@ +package handler + +import "testing" + +func TestIsServableObjectPath(t *testing.T) { + cases := []struct { + path string + want bool + }{ + {"uploads/2026/07/abc.png", true}, + {"attachments/11111111-1111-1111-1111-111111111111/22222222-2222-2222-2222-222222222222", true}, + {"", false}, + {"attachments/../uploads/secret", false}, + {"uploads/../../etc/passwd", false}, + {"secrets/key", false}, + {"attachmentsfoo/x", false}, + {"uploadsfoo/x", false}, + } + for _, c := range cases { + if got := isServableObjectPath(c.path); got != c.want { + t.Errorf("isServableObjectPath(%q) = %v, want %v", c.path, got, c.want) + } + } +} From b81a43328ed89394e6b093e00b10ed1bb48ce053 Mon Sep 17 00:00:00 2001 From: martian56 Date: Sun, 5 Jul 2026 13:00:30 +0400 Subject: [PATCH 2/3] fix(files): authorize attachment downloads by workspace membership Addresses the Strix/CodeRabbit finding on PR #256: serving the attachments/ prefix meant any signed-in user with a leaked object URL could fetch the file, bypassing the issue/workspace authorization the attachment APIs enforce. ServeFile now resolves the attachment record from the object path and requires the caller to be a member of the attachment's workspace before streaming it; missing and forbidden both return 404 so attachment existence isn't leaked. The uploads/ prefix (avatars/covers/logos) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/handler/attachment_authz_test.go | 52 +++++++++++++++++++ apps/api/internal/handler/upload.go | 44 ++++++++++++++++ apps/api/internal/handler/upload_path_test.go | 22 ++++++++ apps/api/internal/router/router.go | 2 +- apps/api/internal/service/attachment.go | 16 ++++++ 5 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 apps/api/internal/handler/attachment_authz_test.go diff --git a/apps/api/internal/handler/attachment_authz_test.go b/apps/api/internal/handler/attachment_authz_test.go new file mode 100644 index 00000000..0c9431cb --- /dev/null +++ b/apps/api/internal/handler/attachment_authz_test.go @@ -0,0 +1,52 @@ +package handler_test + +import ( + "context" + "testing" + + "github.com/Devlaner/devlane/api/internal/model" + "github.com/Devlaner/devlane/api/internal/service" + "github.com/Devlaner/devlane/api/internal/store" + "github.com/Devlaner/devlane/api/internal/testutil" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// A leaked attachment object URL must not be fetchable by a non-member. +func TestAttachment_AuthorizeDownload(t *testing.T) { + ts := testutil.NewTestServer(t) + w := testutil.SeedWorld(t, ts.DB) + issue := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID) + + assetID := uuid.New() + require.NoError(t, ts.DB.Create(&model.FileAsset{ + ID: assetID, + Asset: "attachments/" + issue.ID.String() + "/" + assetID.String(), + IssueID: &issue.ID, + }).Error) + require.NoError(t, ts.DB.Create(&model.IssueAttachment{ + ID: uuid.New(), + IssueID: issue.ID, + AssetID: assetID, + ProjectID: w.Project.ID, + WorkspaceID: w.Workspace.ID, + }).Error) + + svc := service.NewAttachmentService( + store.NewIssueStore(ts.DB), + store.NewProjectStore(ts.DB), + store.NewWorkspaceStore(ts.DB), + nil, + ) + ctx := context.Background() + + // A workspace member (the owner) may download. + require.NoError(t, svc.AuthorizeDownload(ctx, issue.ID, assetID, w.User.ID)) + + // A user who isn't in the workspace is denied. + outsider := testutil.CreateUser(t, ts.DB) + require.Error(t, svc.AuthorizeDownload(ctx, issue.ID, assetID, outsider.ID)) + + // A non-existent attachment (wrong asset id) is not found. + require.Error(t, svc.AuthorizeDownload(ctx, issue.ID, uuid.New(), w.User.ID)) +} diff --git a/apps/api/internal/handler/upload.go b/apps/api/internal/handler/upload.go index 4fabc703..a4d19b8d 100644 --- a/apps/api/internal/handler/upload.go +++ b/apps/api/internal/handler/upload.go @@ -11,6 +11,7 @@ import ( "github.com/Devlaner/devlane/api/internal/middleware" "github.com/Devlaner/devlane/api/internal/minio" + "github.com/Devlaner/devlane/api/internal/service" "github.com/gin-gonic/gin" "github.com/google/uuid" ) @@ -18,6 +19,9 @@ import ( // UploadHandler handles file uploads to MinIO. type UploadHandler struct { Minio *minio.Client + // Attachments authorizes downloads of attachment objects. Optional: when + // nil, attachment paths are refused (they can't be safely served). + Attachments *service.AttachmentService } var allowedImageTypes = map[string]bool{ @@ -112,6 +116,21 @@ func isServableObjectPath(path string) bool { return strings.HasPrefix(path, "uploads/") || strings.HasPrefix(path, "attachments/") } +// parseAttachmentPath extracts the issue and asset ids from an object path of the +// form "attachments//". +func parseAttachmentPath(path string) (issueID, assetID uuid.UUID, ok bool) { + parts := strings.Split(path, "/") + if len(parts) != 3 || parts[0] != "attachments" { + return uuid.Nil, uuid.Nil, false + } + iid, err1 := uuid.Parse(parts[1]) + aid, err2 := uuid.Parse(parts[2]) + if err1 != nil || err2 != nil { + return uuid.Nil, uuid.Nil, false + } + return iid, aid, true +} + func (h *UploadHandler) ServeFile(c *gin.Context) { if h.Minio == nil { c.Status(http.StatusServiceUnavailable) @@ -123,6 +142,31 @@ func (h *UploadHandler) ServeFile(c *gin.Context) { return } + // Attachment objects are per-issue, so authorize the caller against the + // attachment's workspace before streaming — otherwise a leaked object URL + // would be fetchable by anyone signed in. A 404 is returned for both missing + // and forbidden so we don't reveal which attachments exist. + if strings.HasPrefix(path, "attachments/") { + if h.Attachments == nil { + c.Status(http.StatusServiceUnavailable) + return + } + user := middleware.GetUser(c) + if user == nil { + c.Status(http.StatusUnauthorized) + return + } + issueID, assetID, ok := parseAttachmentPath(path) + if !ok { + c.Status(http.StatusBadRequest) + return + } + if err := h.Attachments.AuthorizeDownload(c.Request.Context(), issueID, assetID, user.ID); err != nil { + c.Status(http.StatusNotFound) + return + } + } + obj, err := h.Minio.GetObject(c.Request.Context(), path) if err != nil { c.Status(http.StatusNotFound) diff --git a/apps/api/internal/handler/upload_path_test.go b/apps/api/internal/handler/upload_path_test.go index f8eb3beb..1178a8b7 100644 --- a/apps/api/internal/handler/upload_path_test.go +++ b/apps/api/internal/handler/upload_path_test.go @@ -2,6 +2,28 @@ package handler import "testing" +func TestParseAttachmentPath(t *testing.T) { + iid, aid, ok := parseAttachmentPath("attachments/11111111-1111-1111-1111-111111111111/22222222-2222-2222-2222-222222222222") + if !ok { + t.Fatalf("expected ok for a well-formed attachment path") + } + if iid.String() != "11111111-1111-1111-1111-111111111111" || aid.String() != "22222222-2222-2222-2222-222222222222" { + t.Errorf("parsed ids wrong: issue=%s asset=%s", iid, aid) + } + bad := []string{ + "uploads/2026/07/x.png", + "attachments/not-a-uuid/22222222-2222-2222-2222-222222222222", + "attachments/11111111-1111-1111-1111-111111111111", + "attachments/a/b/c", + "attachments//", + } + for _, p := range bad { + if _, _, ok := parseAttachmentPath(p); ok { + t.Errorf("parseAttachmentPath(%q) = ok, want not-ok", p) + } + } +} + func TestIsServableObjectPath(t *testing.T) { cases := []struct { path string diff --git a/apps/api/internal/router/router.go b/apps/api/internal/router/router.go index d99ce034..6edc0f74 100644 --- a/apps/api/internal/router/router.go +++ b/apps/api/internal/router/router.go @@ -264,7 +264,7 @@ func New(cfg Config) *gin.Engine { api.POST("/instance/admins/", instanceSettingsHandler.AddAdmin) api.DELETE("/instance/admins/:id/", instanceSettingsHandler.RemoveAdmin) - uploadHandler := &handler.UploadHandler{Minio: cfg.Minio} + uploadHandler := &handler.UploadHandler{Minio: cfg.Minio, Attachments: attachmentSvc} api.POST("/upload", uploadHandler.Upload) api.GET("/files/*path", uploadHandler.ServeFile) api.GET("/users/me/workspaces/", workspaceHandler.List) diff --git a/apps/api/internal/service/attachment.go b/apps/api/internal/service/attachment.go index 91dbb934..cccc9a89 100644 --- a/apps/api/internal/service/attachment.go +++ b/apps/api/internal/service/attachment.go @@ -84,6 +84,22 @@ func (s *AttachmentService) ensureIssueAccess(ctx context.Context, workspaceSlug return nil } +// AuthorizeDownload checks that userID may fetch the attachment identified by an +// object path "attachments//". It resolves the attachment +// record (404 if missing) and requires the caller to be a member of the +// attachment's workspace, so a leaked object URL can't be fetched by outsiders. +func (s *AttachmentService) AuthorizeDownload(ctx context.Context, issueID, assetID, userID uuid.UUID) error { + att, err := s.is.GetAttachmentByAssetID(ctx, assetID, issueID) + if err != nil || att == nil { + return ErrAttachmentNotFound + } + ok, _ := s.ws.IsMember(ctx, att.WorkspaceID, userID) + if !ok { + return ErrProjectForbidden + } + return nil +} + // InitiateUpload creates the DB records and returns the presigned upload URL + attachment shape. func (s *AttachmentService) InitiateUpload(ctx context.Context, workspaceSlug string, projectID, issueID uuid.UUID, userID uuid.UUID, name string, size float64, contentType string) (*PresignedUploadResponse, error) { if s.minio == nil { From c20936873414801fab787c795688439ff3d88b20 Mon Sep 17 00:00:00 2001 From: martian56 Date: Sun, 5 Jul 2026 13:08:20 +0400 Subject: [PATCH 3/3] fix(files): surface datastore errors from attachment authorization CodeRabbit follow-up: AuthorizeDownload collapsed unexpected store errors into not-found/forbidden, hiding real failures behind 404/403. It now returns ErrAttachmentNotFound only for a genuinely missing record and propagates any other datastore error, which ServeFile maps to 500 (missing/forbidden stay 404). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/internal/handler/upload.go | 7 ++++++- apps/api/internal/service/attachment.go | 10 ++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/apps/api/internal/handler/upload.go b/apps/api/internal/handler/upload.go index a4d19b8d..9436fc15 100644 --- a/apps/api/internal/handler/upload.go +++ b/apps/api/internal/handler/upload.go @@ -2,6 +2,7 @@ package handler import ( "bytes" + "errors" "fmt" "io" "net/http" @@ -162,7 +163,11 @@ func (h *UploadHandler) ServeFile(c *gin.Context) { return } if err := h.Attachments.AuthorizeDownload(c.Request.Context(), issueID, assetID, user.ID); err != nil { - c.Status(http.StatusNotFound) + if errors.Is(err, service.ErrAttachmentNotFound) || errors.Is(err, service.ErrProjectForbidden) { + c.Status(http.StatusNotFound) + } else { + c.Status(http.StatusInternalServerError) + } return } } diff --git a/apps/api/internal/service/attachment.go b/apps/api/internal/service/attachment.go index cccc9a89..807eb3a4 100644 --- a/apps/api/internal/service/attachment.go +++ b/apps/api/internal/service/attachment.go @@ -90,10 +90,16 @@ func (s *AttachmentService) ensureIssueAccess(ctx context.Context, workspaceSlug // attachment's workspace, so a leaked object URL can't be fetched by outsiders. func (s *AttachmentService) AuthorizeDownload(ctx context.Context, issueID, assetID, userID uuid.UUID) error { att, err := s.is.GetAttachmentByAssetID(ctx, assetID, issueID) - if err != nil || att == nil { + if errors.Is(err, gorm.ErrRecordNotFound) || (err == nil && att == nil) { return ErrAttachmentNotFound } - ok, _ := s.ws.IsMember(ctx, att.WorkspaceID, userID) + if err != nil { + return err // a real datastore failure — surface it as 5xx, not a 404 + } + ok, err := s.ws.IsMember(ctx, att.WorkspaceID, userID) + if err != nil { + return err + } if !ok { return ErrProjectForbidden }