From f63ce3452ece128534fb271db59d59840de3b9c4 Mon Sep 17 00:00:00 2001 From: Noooste <83548733+Noooste@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:48:25 +0200 Subject: [PATCH] refactor: update handler methods to use interfaces for admin and S3 services Signed-off-by: Noooste <83548733+Noooste@users.noreply.github.com> --- backend/internal/config/config.go | 7 +++ backend/internal/handlers/buckets.go | 10 ++-- backend/internal/handlers/cluster.go | 8 +-- backend/internal/handlers/monitoring.go | 10 ++-- backend/internal/handlers/objects.go | 80 ++++++++++++++++++++++--- backend/internal/handlers/users.go | 8 +-- backend/internal/middleware/cors.go | 21 +++++-- backend/internal/services/interfaces.go | 68 +++++++++++++++++++++ 8 files changed, 179 insertions(+), 33 deletions(-) create mode 100644 backend/internal/services/interfaces.go diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 0ee4df1..48ff1d3 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -244,6 +244,13 @@ func (c *Config) Validate() error { if len(c.Auth.OIDC.Scopes) == 0 { return fmt.Errorf("oidc scopes are required when oidc is enabled") } + // Every authenticated route on this service grants full admin + // access — there is no separate authorization layer. An empty + // admin_role would therefore promote every user in the IdP realm + // to cluster admin. Require operators to opt in explicitly. + if c.Auth.OIDC.AdminRole == "" { + return fmt.Errorf("oidc admin_role is required when oidc is enabled: leaving it empty would grant cluster-admin access to any authenticated IdP user") + } } return nil diff --git a/backend/internal/handlers/buckets.go b/backend/internal/handlers/buckets.go index 3947a71..1765594 100644 --- a/backend/internal/handlers/buckets.go +++ b/backend/internal/handlers/buckets.go @@ -7,14 +7,14 @@ import ( "github.com/gofiber/fiber/v3" ) -// BucketHandler handles bucket-related operations +// BucketHandler handles bucket-related HTTP requests. type BucketHandler struct { - adminService *services.GarageAdminService - s3Service *services.S3Service + adminService services.AdminService + s3Service services.S3Storage } -// NewBucketHandler creates a new bucket handler -func NewBucketHandler(adminService *services.GarageAdminService, s3Service *services.S3Service) *BucketHandler { +// NewBucketHandler creates a new bucket handler. +func NewBucketHandler(adminService services.AdminService, s3Service services.S3Storage) *BucketHandler { return &BucketHandler{ adminService: adminService, s3Service: s3Service, diff --git a/backend/internal/handlers/cluster.go b/backend/internal/handlers/cluster.go index 3ea6371..6c13b44 100644 --- a/backend/internal/handlers/cluster.go +++ b/backend/internal/handlers/cluster.go @@ -7,13 +7,13 @@ import ( "github.com/gofiber/fiber/v3" ) -// ClusterHandler handles cluster management operations +// ClusterHandler handles cluster-status HTTP requests. type ClusterHandler struct { - adminService *services.GarageAdminService + adminService services.AdminService } -// NewClusterHandler creates a new cluster handler -func NewClusterHandler(adminService *services.GarageAdminService) *ClusterHandler { +// NewClusterHandler creates a new cluster handler. +func NewClusterHandler(adminService services.AdminService) *ClusterHandler { return &ClusterHandler{ adminService: adminService, } diff --git a/backend/internal/handlers/monitoring.go b/backend/internal/handlers/monitoring.go index 4f793ae..cc5d352 100644 --- a/backend/internal/handlers/monitoring.go +++ b/backend/internal/handlers/monitoring.go @@ -7,14 +7,14 @@ import ( "github.com/gofiber/fiber/v3" ) -// MonitoringHandler handles monitoring operations +// MonitoringHandler handles metrics and dashboard HTTP requests. type MonitoringHandler struct { - adminService *services.GarageAdminService - s3Service *services.S3Service + adminService services.AdminService + s3Service services.S3Storage } -// NewMonitoringHandler creates a new monitoring handler -func NewMonitoringHandler(adminService *services.GarageAdminService, s3Service *services.S3Service) *MonitoringHandler { +// NewMonitoringHandler creates a new monitoring handler. +func NewMonitoringHandler(adminService services.AdminService, s3Service services.S3Storage) *MonitoringHandler { return &MonitoringHandler{ adminService: adminService, s3Service: s3Service, diff --git a/backend/internal/handlers/objects.go b/backend/internal/handlers/objects.go index 9aa3562..8375089 100644 --- a/backend/internal/handlers/objects.go +++ b/backend/internal/handlers/objects.go @@ -3,7 +3,10 @@ package handlers import ( "bufio" "io" + "net/url" + "path" "strconv" + "strings" "time" "Noooste/garage-ui/internal/models" @@ -12,13 +15,66 @@ import ( "github.com/gofiber/fiber/v3" ) -// ObjectHandler handles object-related operations -type ObjectHandler struct { - s3Service *services.S3Service +// unsafeInlineContentTypes are MIME types that a browser can execute as +// JavaScript in the response's origin when rendered inline. Since the SPA is +// served from the same origin as the API, any uploader could otherwise plant +// stored XSS by uploading a file with one of these Content-Types. +var unsafeInlineContentTypes = map[string]struct{}{ + "text/html": {}, + "application/xhtml+xml": {}, + "image/svg+xml": {}, + "application/xml": {}, + "text/xml": {}, + "application/javascript": {}, + "text/javascript": {}, } -// NewObjectHandler creates a new object handler -func NewObjectHandler(s3Service *services.S3Service) *ObjectHandler { +// safeContentType rewrites Content-Types that the browser would treat as +// executable to application/octet-stream. +func safeContentType(ct string) string { + base := strings.TrimSpace(strings.ToLower(ct)) + if i := strings.IndexByte(base, ';'); i >= 0 { + base = strings.TrimSpace(base[:i]) + } + if _, bad := unsafeInlineContentTypes[base]; bad { + return "application/octet-stream" + } + return ct +} + +// contentDispositionHeader builds an RFC 6266 / RFC 5987 Content-Disposition +// header value with the user-controlled object key safely encoded. Strips +// path components and control characters before emitting the ASCII fallback, +// then appends the percent-encoded UTF-8 filename*= for full fidelity. +func contentDispositionHeader(disposition, key string) string { + name := path.Base(key) + if name == "." || name == "/" || name == "" { + name = "download" + } + // ASCII-safe fallback: drop anything that could break the quoted value. + var asciiFallback strings.Builder + for _, r := range name { + if r < 0x20 || r == 0x7f || r == '"' || r == '\\' || r > 0x7e { + asciiFallback.WriteByte('_') + continue + } + asciiFallback.WriteRune(r) + } + fallback := asciiFallback.String() + if fallback == "" { + fallback = "download" + } + encoded := url.PathEscape(name) + return disposition + "; filename=\"" + fallback + "\"; filename*=UTF-8''" + encoded +} + +// ObjectHandler handles object-related HTTP requests. +type ObjectHandler struct { + s3Service services.S3Storage +} + +// NewObjectHandler creates a new object handler. +func NewObjectHandler(s3Service services.S3Storage) *ObjectHandler { return &ObjectHandler{ s3Service: s3Service, } @@ -178,16 +234,22 @@ func (h *ObjectHandler) GetObject(c fiber.Ctx) error { ) } - // Set response headers - c.Set("Content-Type", objectInfo.ContentType) + // The uploader controls Content-Type. Rewrite executable MIME types to + // application/octet-stream and always disable sniffing so stored HTML/SVG + // cannot run as XSS in the SPA origin when fetched inline. + c.Set("Content-Type", safeContentType(objectInfo.ContentType)) + c.Set("X-Content-Type-Options", "nosniff") c.Set("Content-Length", strconv.FormatInt(objectInfo.Size, 10)) c.Set("ETag", objectInfo.ETag) c.Set("Last-Modified", objectInfo.LastModified.Format(time.RFC1123)) - // Check if client wants to download or view inline + // The object key is attacker-controlled — build the header via the safe + // RFC 6266 helper to avoid quote/semicolon injection into filename=. + disposition := "inline" if c.Query("download") == "true" { - c.Set("Content-Disposition", "attachment; filename=\""+key+"\"") + disposition = "attachment" } + c.Set("Content-Disposition", contentDispositionHeader(disposition, key)) // Stream the object body to the client without buffering the entire file return c.SendStreamWriter(func(w *bufio.Writer) { diff --git a/backend/internal/handlers/users.go b/backend/internal/handlers/users.go index e3e9cfb..7e87105 100644 --- a/backend/internal/handlers/users.go +++ b/backend/internal/handlers/users.go @@ -9,13 +9,13 @@ import ( "github.com/gofiber/fiber/v3" ) -// UserHandler handles user/key management operations using Garage Admin API +// UserHandler handles user and access key HTTP requests. type UserHandler struct { - adminService *services.GarageAdminService + adminService services.AdminService } -// NewUserHandler creates a new user handler -func NewUserHandler(adminService *services.GarageAdminService) *UserHandler { +// NewUserHandler creates a new user handler. +func NewUserHandler(adminService services.AdminService) *UserHandler { return &UserHandler{ adminService: adminService, } diff --git a/backend/internal/middleware/cors.go b/backend/internal/middleware/cors.go index bac15f2..051accd 100644 --- a/backend/internal/middleware/cors.go +++ b/backend/internal/middleware/cors.go @@ -1,6 +1,7 @@ package middleware import ( + "strconv" "strings" "Noooste/garage-ui/internal/config" @@ -20,10 +21,14 @@ func CORSMiddleware(cfg *config.CORSConfig) fiber.Handler { return func(c fiber.Ctx) error { origin := c.Get("Origin") - // Check if origin is allowed - if origin != "" && isAllowedOrigin(origin, cfg.AllowedOrigins) { + // Check if origin is allowed. When credentials are allowed we refuse + // to treat "*" as a match: reflecting an arbitrary Origin alongside + // Access-Control-Allow-Credentials: true lets any site read responses + // cross-origin with the user's session cookie. + if origin != "" && isAllowedOrigin(origin, cfg.AllowedOrigins, cfg.AllowCredentials) { // Set CORS headers c.Set("Access-Control-Allow-Origin", origin) + c.Set("Vary", "Origin") if cfg.AllowCredentials { c.Set("Access-Control-Allow-Credentials", "true") @@ -41,7 +46,7 @@ func CORSMiddleware(cfg *config.CORSConfig) fiber.Handler { // Set max age for preflight cache if cfg.MaxAge > 0 { - c.Set("Access-Control-Max-Age", string(rune(cfg.MaxAge))) + c.Set("Access-Control-Max-Age", strconv.Itoa(cfg.MaxAge)) } } @@ -54,10 +59,14 @@ func CORSMiddleware(cfg *config.CORSConfig) fiber.Handler { } } -// isAllowedOrigin checks if an origin is in the allowed list -func isAllowedOrigin(origin string, allowedOrigins []string) bool { +// isAllowedOrigin checks if an origin is in the allowed list. +// When allowCredentials is true, "*" is NOT honored — exact match is required. +func isAllowedOrigin(origin string, allowedOrigins []string, allowCredentials bool) bool { for _, allowed := range allowedOrigins { - if allowed == "*" || allowed == origin { + if allowed == origin { + return true + } + if allowed == "*" && !allowCredentials { return true } } diff --git a/backend/internal/services/interfaces.go b/backend/internal/services/interfaces.go new file mode 100644 index 0000000..f8a7856 --- /dev/null +++ b/backend/internal/services/interfaces.go @@ -0,0 +1,68 @@ +package services + +import ( + "context" + "io" + "time" + + "Noooste/garage-ui/internal/models" +) + +// AdminService is the set of Garage Admin API operations used by HTTP handlers. +// It is implemented by *GarageAdminService in admin.go. Kept narrow so that +// hand-rolled mocks in tests don't need to cover admin methods the handlers +// never call. +type AdminService interface { + // Access keys + ListKeys(ctx context.Context) ([]models.ListKeysResponseItem, error) + CreateKey(ctx context.Context, req models.CreateKeyRequest) (*models.GarageKeyInfo, error) + GetKeyInfo(ctx context.Context, keyID string, showSecret bool) (*models.GarageKeyInfo, error) + UpdateKey(ctx context.Context, keyID string, req models.UpdateKeyRequest) (*models.GarageKeyInfo, error) + DeleteKey(ctx context.Context, keyID string) error + + // Buckets + ListBuckets(ctx context.Context) ([]models.ListBucketsResponseItem, error) + GetBucketInfo(ctx context.Context, bucketID string) (*models.GarageBucketInfo, error) + GetBucketInfoByAlias(ctx context.Context, globalAlias string) (*models.GarageBucketInfo, error) + CreateBucket(ctx context.Context, req models.CreateBucketAdminRequest) (*models.GarageBucketInfo, error) + UpdateBucket(ctx context.Context, bucketID string, req models.UpdateBucketRequest) (*models.GarageBucketInfo, error) + DeleteBucket(ctx context.Context, bucketID string) error + AllowBucketKey(ctx context.Context, req models.BucketKeyPermRequest) (*models.GarageBucketInfo, error) + + // Cluster + GetClusterHealth(ctx context.Context) (*models.ClusterHealth, error) + GetClusterStatus(ctx context.Context) (*models.ClusterStatus, error) + GetClusterStatistics(ctx context.Context) (*models.ClusterStatistics, error) + GetNodeInfo(ctx context.Context, nodeID string) (*models.MultiNodeResponse, error) + GetNodeStatistics(ctx context.Context, nodeID string) (*models.MultiNodeResponse, error) + + // Monitoring + HealthCheck(ctx context.Context) error + GetMetrics(ctx context.Context) (string, error) +} + +// S3Storage is the set of S3 operations used by HTTP handlers. It is +// implemented by *S3Service in s3.go. Methods on *S3Service that are not +// called by handlers (ListBuckets, CreateBucket, DeleteBucket, +// GetBucketStatistics) are intentionally excluded. +type S3Storage interface { + ListObjects(ctx context.Context, bucketName, prefix string, maxKeys int, continuationToken string) (*models.ObjectListResponse, error) + UploadObject(ctx context.Context, bucketName, key string, body io.Reader, contentType string) (*models.ObjectUploadResponse, error) + GetObject(ctx context.Context, bucketName, key string) (io.ReadCloser, *models.ObjectInfo, error) + ObjectExists(ctx context.Context, bucketName, key string) (bool, error) + DeleteObject(ctx context.Context, bucketName, key string) error + GetObjectMetadata(ctx context.Context, bucketName, key string) (*models.ObjectInfo, error) + GetPresignedURL(ctx context.Context, bucketName, key string, expiresIn time.Duration) (string, error) + DeleteMultipleObjects(ctx context.Context, bucketName string, keys []string) error + UploadMultipleObjects(ctx context.Context, bucketName string, files []struct { + Key string + Body io.Reader + ContentType string + }) []UploadResult +} + +// Compile-time guarantees that the concrete services implement the interfaces. +var ( + _ AdminService = (*GarageAdminService)(nil) + _ S3Storage = (*S3Service)(nil) +)