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 bec8a5fc..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" @@ -11,6 +12,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 +20,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{ @@ -100,17 +105,73 @@ 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/") +} + +// 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) return } path := strings.TrimPrefix(c.Param("path"), "/") - if path == "" || strings.Contains(path, "..") || !strings.HasPrefix(path, "uploads/") { + if !isServableObjectPath(path) { c.Status(http.StatusBadRequest) 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 { + if errors.Is(err, service.ErrAttachmentNotFound) || errors.Is(err, service.ErrProjectForbidden) { + c.Status(http.StatusNotFound) + } else { + c.Status(http.StatusInternalServerError) + } + 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 new file mode 100644 index 00000000..1178a8b7 --- /dev/null +++ b/apps/api/internal/handler/upload_path_test.go @@ -0,0 +1,46 @@ +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 + 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) + } + } +} 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..807eb3a4 100644 --- a/apps/api/internal/service/attachment.go +++ b/apps/api/internal/service/attachment.go @@ -84,6 +84,28 @@ 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 errors.Is(err, gorm.ErrRecordNotFound) || (err == nil && att == nil) { + return ErrAttachmentNotFound + } + 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 + } + 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 {