From 269a684d07d4c34c9d8c5678ed3471dd3a39149a Mon Sep 17 00:00:00 2001 From: Shantanu Mane Date: Tue, 16 Jun 2026 23:15:07 +0530 Subject: [PATCH] =?UTF-8?q?fix(api):=20hardening=20batch=20=E2=80=94=20rec?= =?UTF-8?q?overy=20order,=20request=20IDs,=20MIME=20allowlist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move RecoveryMiddleware outermost so panics in inner middleware are caught (DEV-36) - Replace time-based request-ID chars with math/rand/v2 (unbiased, no collisions) (DEV-37) - Centralise supported MIME types in repository.SupportedMIMETypes; handler gates against it (DEV-38) - Add tests for request-ID generation and the MIME gate --- internal/handler/asset_handler.go | 11 ++------ internal/middleware/logging.go | 5 +++- internal/middleware/logging_test.go | 42 +++++++++++++++++++++++++++++ internal/repository/asset_repo.go | 17 ++++++++++++ internal/repository/mime_test.go | 30 +++++++++++++++++++++ internal/router/router.go | 6 ++++- 6 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 internal/middleware/logging_test.go create mode 100644 internal/repository/mime_test.go diff --git a/internal/handler/asset_handler.go b/internal/handler/asset_handler.go index 1473230..8c92521 100644 --- a/internal/handler/asset_handler.go +++ b/internal/handler/asset_handler.go @@ -9,6 +9,7 @@ import ( "github.com/rndmcodeguy20/mpiper/internal/config" "github.com/rndmcodeguy20/mpiper/internal/metrics" "github.com/rndmcodeguy20/mpiper/internal/models" + "github.com/rndmcodeguy20/mpiper/internal/repository" "github.com/rndmcodeguy20/mpiper/internal/service" "github.com/rndmcodeguy20/mpiper/pkg/utils" "go.opentelemetry.io/otel" @@ -17,14 +18,6 @@ import ( "go.uber.org/zap" ) -var allowedMIMETypes = map[string]bool{ - "image/jpeg": true, - "image/png": true, - "image/webp": true, - "video/mp4": true, - "video/quicktime": true, -} - func maxAssetSize() int64 { return config.MustGet().MaxAssetSizeBytes } @@ -85,7 +78,7 @@ func (h *AssetHandler) CreateAsset(w http.ResponseWriter, r *http.Request) { return } - if !allowedMIMETypes[req.ContentType] { + if !repository.IsSupportedMIMEType(req.ContentType) { span.SetStatus(codes.Error, "unsupported content type") utils.RespondJSON(w, map[string]string{"status": "error", "message": "unsupported content type"}, http.StatusBadRequest) return diff --git a/internal/middleware/logging.go b/internal/middleware/logging.go index a8cd7bc..c046ab2 100644 --- a/internal/middleware/logging.go +++ b/internal/middleware/logging.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "math/rand/v2" "net/http" "time" @@ -82,7 +83,9 @@ func randomString(length int) string { const charset = "abcdefghijklmnopqrstuvwxyz0123456789" b := make([]byte, length) for i := range b { - b[i] = charset[time.Now().UnixNano()%int64(len(charset))] + // math/rand/v2 is concurrency-safe and unbiased; a log-correlation ID + // needs neither crypto strength nor per-call seeding. + b[i] = charset[rand.IntN(len(charset))] } return string(b) } diff --git a/internal/middleware/logging_test.go b/internal/middleware/logging_test.go new file mode 100644 index 0000000..d1528fa --- /dev/null +++ b/internal/middleware/logging_test.go @@ -0,0 +1,42 @@ +package middleware + +import "testing" + +func TestRandomString(t *testing.T) { + const charset = "abcdefghijklmnopqrstuvwxyz0123456789" + inCharset := func(s string) bool { + for _, c := range s { + found := false + for _, allowed := range charset { + if c == allowed { + found = true + break + } + } + if !found { + return false + } + } + return true + } + + // Length + charset. + s := randomString(8) + if len(s) != 8 { + t.Fatalf("len = %d, want 8", len(s)) + } + if !inCharset(s) { + t.Errorf("%q contains chars outside charset", s) + } + + // Many rapid calls must not collide (the old time-based impl produced + // identical IDs within a nanosecond window). + seen := make(map[string]struct{}, 1000) + for i := 0; i < 1000; i++ { + id := randomString(8) + if _, dup := seen[id]; dup { + t.Fatalf("duplicate id %q on iteration %d", id, i) + } + seen[id] = struct{}{} + } +} diff --git a/internal/repository/asset_repo.go b/internal/repository/asset_repo.go index c81fd08..09a1234 100644 --- a/internal/repository/asset_repo.go +++ b/internal/repository/asset_repo.go @@ -54,6 +54,23 @@ func ToAssetType(fileType string) AssetType { } } +// SupportedMIMETypes is the single source of truth for which content types the +// pipeline accepts for upload and processing, mapped to their asset category. +// The handler gates uploads against this set; do not maintain a second list. +var SupportedMIMETypes = map[string]AssetType{ + "image/jpeg": ImageAsset, + "image/png": ImageAsset, + "image/webp": ImageAsset, + "video/mp4": VideoAsset, + "video/quicktime": VideoAsset, +} + +// IsSupportedMIMEType reports whether the pipeline accepts the given content type. +func IsSupportedMIMEType(mimeType string) bool { + _, ok := SupportedMIMETypes[mimeType] + return ok +} + func ToAssetTypeFromMimeType(mimeType string) AssetType { if len(mimeType) < 5 { return OtherAsset diff --git a/internal/repository/mime_test.go b/internal/repository/mime_test.go new file mode 100644 index 0000000..ba3f138 --- /dev/null +++ b/internal/repository/mime_test.go @@ -0,0 +1,30 @@ +package repository + +import "testing" + +func TestSupportedMIMETypes(t *testing.T) { + supported := map[string]AssetType{ + "image/jpeg": ImageAsset, + "image/png": ImageAsset, + "image/webp": ImageAsset, + "video/mp4": VideoAsset, + "video/quicktime": VideoAsset, + } + + for mime, want := range supported { + if !IsSupportedMIMEType(mime) { + t.Errorf("IsSupportedMIMEType(%q) = false, want true", mime) + } + if got := SupportedMIMETypes[mime]; got != want { + t.Errorf("SupportedMIMETypes[%q] = %v, want %v", mime, got, want) + } + } + + // The gate must reject types the pipeline cannot process, even ones the + // broad classifier would still bucket (e.g. gif, pdf). + for _, mime := range []string{"image/gif", "application/pdf", "audio/mpeg", "text/plain", ""} { + if IsSupportedMIMEType(mime) { + t.Errorf("IsSupportedMIMEType(%q) = true, want false", mime) + } + } +} diff --git a/internal/router/router.go b/internal/router/router.go index 486f54d..d8a90b0 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -93,6 +93,11 @@ func NewRouter(cfg config.EnvConfig, db *sqlx.DB, m *metrics.Metrics) *chi.Mux { // Middleware r.Use(middleware.RequestID) r.Use(middleware.RealIP) + // Recovery must be the outermost app-level middleware so panics in any + // inner middleware (logger, cors, tracing, …) are caught and turned into a + // 500 rather than crashing the process. It takes the base logger directly, + // so it does not depend on LoggerMiddleware running first. + r.Use(appMiddleware.RecoveryMiddleware(logger)) r.Use(cors.Handler(cors.Options{ AllowedOrigins: allowedOrigins, AllowedMethods: []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"}, @@ -105,7 +110,6 @@ func NewRouter(cfg config.EnvConfig, db *sqlx.DB, m *metrics.Metrics) *chi.Mux { r.Use(middleware.Timeout(MiddlewareTimeout)) r.Use(appMiddleware.TracingMiddleware) r.Use(appMiddleware.MetricsMiddleware(m)) - r.Use(appMiddleware.RecoveryMiddleware(logger)) r.Use(middleware.Compress(5)) r.Use(appMiddleware.SlowRequestMiddleware(logger, 2*time.Second))