From ee82522b7cb567be83449ace2c63ada4f131525d Mon Sep 17 00:00:00 2001 From: Ivan Godwin Date: Sat, 18 Jul 2026 09:15:57 -0700 Subject: [PATCH] feat(rest,config): wire CORS, real readiness, TLS options, error hygiene - CORS config is now actually applied to the router (the middleware existed but was never wired); preflight returns 204 for allowed origins and 403 with no CORS headers for disallowed ones. - /readyz runs real dependency checks (queue, auth database) and returns 503 with per-component detail when not ready; exported handlers support dedicated health listeners. - Optional server.tls (cert_file/key_file) for REST and gRPC, validated at config load. - 5xx responses no longer echo internal error details; not-found and already-sent map to 404/409 on cancel/retry. Co-Authored-By: Claude Fable 5 --- api/rest/cors_test.go | 7 ++- api/rest/handlers.go | 24 ++++++-- api/rest/keys.go | 8 +-- api/rest/router.go | 116 ++++++++++++++++++++++++++++++++------ config.yaml | 6 ++ internal/config/config.go | 24 ++++++-- 6 files changed, 154 insertions(+), 31 deletions(-) diff --git a/api/rest/cors_test.go b/api/rest/cors_test.go index d32b844..de10ef3 100644 --- a/api/rest/cors_test.go +++ b/api/rest/cors_test.go @@ -171,13 +171,13 @@ func TestCORSMiddleware_PreflightRequest(t *testing.T) { { name: "preflight from allowed origin", origin: "https://example.com", - expectStatus: http.StatusOK, + expectStatus: http.StatusNoContent, expectHeaders: true, }, { name: "preflight from blocked origin", origin: "https://malicious.com", - expectStatus: http.StatusOK, + expectStatus: http.StatusForbidden, expectHeaders: false, }, } @@ -191,7 +191,8 @@ func TestCORSMiddleware_PreflightRequest(t *testing.T) { rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) - // Preflight should always return 200 OK + // Allowed preflights succeed with 204; blocked ones get 403 + // with no CORS headers so the browser rejects the request. if rec.Code != tt.expectStatus { t.Errorf("status = %v, want %v", rec.Code, tt.expectStatus) } diff --git a/api/rest/handlers.go b/api/rest/handlers.go index f8d61b5..2c00ad0 100644 --- a/api/rest/handlers.go +++ b/api/rest/handlers.go @@ -2,6 +2,7 @@ package rest import ( "encoding/json" + "errors" "net/http" "strconv" "time" @@ -160,7 +161,7 @@ func (h *Handler) CancelNotification(w http.ResponseWriter, r *http.Request) { id := vars["id"] if err := h.service.CancelNotification(r.Context(), id); err != nil { - respondError(w, http.StatusInternalServerError, "failed to cancel notification", err) + respondError(w, statusForServiceError(err), "failed to cancel notification", err) return } @@ -177,7 +178,7 @@ func (h *Handler) RetryNotification(w http.ResponseWriter, r *http.Request) { result, err := h.service.RetryNotification(r.Context(), id) if err != nil { - respondError(w, http.StatusInternalServerError, "failed to retry notification", err) + respondError(w, statusForServiceError(err), "failed to retry notification", err) return } @@ -186,6 +187,19 @@ func (h *Handler) RetryNotification(w http.ResponseWriter, r *http.Request) { }) } +// statusForServiceError maps service-layer sentinel errors to HTTP status +// codes; anything unrecognized is an internal error. +func statusForServiceError(err error) int { + switch { + case errors.Is(err, domain.ErrNotificationNotFound): + return http.StatusNotFound + case errors.Is(err, domain.ErrNotificationAlreadySent): + return http.StatusConflict + default: + return http.StatusInternalServerError + } +} + // GetStats handles GET /api/v1/stats func (h *Handler) GetStats(w http.ResponseWriter, r *http.Request) { stats, err := h.service.GetStats(r.Context()) @@ -270,10 +284,12 @@ func respondJSON(w http.ResponseWriter, status int, data interface{}) { } } -// respondError sends an error response +// respondError sends an error response. Client errors (4xx) include the +// underlying detail to help callers fix their request; server errors (5xx) +// deliberately do not echo internals — those belong in the server log. func respondError(w http.ResponseWriter, status int, message string, err error) { errMsg := message - if err != nil { + if err != nil && status < http.StatusInternalServerError { errMsg = message + ": " + err.Error() } diff --git a/api/rest/keys.go b/api/rest/keys.go index 17c94ca..d116699 100644 --- a/api/rest/keys.go +++ b/api/rest/keys.go @@ -120,7 +120,7 @@ func (h *KeyManagementHandler) CreateKey(w http.ResponseWriter, r *http.Request) apiKey, err := h.keyStore.CreateKey(ctx, req.ClientID, req.Roles, req.RateLimit, expiresInDuration, authCtx.ClientID) if err != nil { h.logger.Errorf("Failed to create API key: %v", err) - h.respondError(w, http.StatusInternalServerError, "Failed to create API key", err.Error()) + h.respondError(w, http.StatusInternalServerError, "Failed to create API key", "") return } @@ -165,7 +165,7 @@ func (h *KeyManagementHandler) ListKeys(w http.ResponseWriter, r *http.Request) keys, err := h.keyStore.ListKeys(ctx, clientID) if err != nil { h.logger.Errorf("Failed to list API keys: %v", err) - h.respondError(w, http.StatusInternalServerError, "Failed to list API keys", err.Error()) + h.respondError(w, http.StatusInternalServerError, "Failed to list API keys", "") return } @@ -218,7 +218,7 @@ func (h *KeyManagementHandler) RevokeKey(w http.ResponseWriter, r *http.Request) h.respondError(w, http.StatusNotFound, "Key not found", "") } else { h.logger.Errorf("Failed to revoke API key: %v", err) - h.respondError(w, http.StatusInternalServerError, "Failed to revoke API key", err.Error()) + h.respondError(w, http.StatusInternalServerError, "Failed to revoke API key", "") } return } @@ -291,7 +291,7 @@ func (h *KeyManagementHandler) GetAuditLog(w http.ResponseWriter, r *http.Reques logs, err := h.keyStore.GetAuditLogByName(ctx, keyName, limit) if err != nil { h.logger.Errorf("Failed to get audit log: %v", err) - h.respondError(w, http.StatusInternalServerError, "Failed to get audit log", err.Error()) + h.respondError(w, http.StatusInternalServerError, "Failed to get audit log", "") return } diff --git a/api/rest/router.go b/api/rest/router.go index 8c8ffc5..753d2ab 100644 --- a/api/rest/router.go +++ b/api/rest/router.go @@ -1,9 +1,12 @@ package rest import ( + "context" + "encoding/json" "net/http" "strconv" "strings" + "time" "github.com/gorilla/mux" "github.com/igodwin/notifier/internal/auth" @@ -43,27 +46,50 @@ func DefaultCORSConfig() *CORSConfig { } } -// NewRouter creates a new HTTP router with all routes configured -func NewRouter(service domain.NotificationService, logger *logging.Logger) *mux.Router { - return NewRouterWithAuth(service, logger, nil) +// ReadinessCheck reports whether a named dependency is ready. Implementations +// should be cheap; they run on every /readyz request. +type ReadinessCheck func(ctx context.Context) error + +// RouterOptions configures the REST router. +type RouterOptions struct { + Service domain.NotificationService + Logger *logging.Logger + AuthStore *auth.APIKeyStore // nil disables authentication + KeyStore *auth.HybridKeyStore // nil disables key-management routes + CORS *CORSConfig // nil disables CORS headers entirely + // Readiness maps a component name (e.g. "queue", "database") to its check. + Readiness map[string]ReadinessCheck + // Instrument, when set, wraps the router for request instrumentation + // (e.g. Prometheus HTTP metrics). + Instrument func(http.Handler) http.Handler } -// NewRouterWithAuth creates a new HTTP router with optional authentication and CORS configuration +// NewRouter creates a new HTTP router with all routes configured +func NewRouter(service domain.NotificationService, logger *logging.Logger) *mux.Router { + return NewRouterWithOptions(RouterOptions{Service: service, Logger: logger}) +} + +// NewRouterWithAuth creates a new HTTP router with optional authentication func NewRouterWithAuth(service domain.NotificationService, logger *logging.Logger, authStore *auth.APIKeyStore) *mux.Router { - return NewRouterWithAuthAndKeyStore(service, logger, authStore, nil) + return NewRouterWithOptions(RouterOptions{Service: service, Logger: logger, AuthStore: authStore}) } // NewRouterWithAuthAndKeyStore creates a new HTTP router with authentication and key management func NewRouterWithAuthAndKeyStore(service domain.NotificationService, logger *logging.Logger, authStore *auth.APIKeyStore, keyStore *auth.HybridKeyStore) *mux.Router { - handler := NewHandler(service, logger) + return NewRouterWithOptions(RouterOptions{Service: service, Logger: logger, AuthStore: authStore, KeyStore: keyStore}) +} + +// NewRouterWithOptions creates the HTTP router from RouterOptions. +func NewRouterWithOptions(opts RouterOptions) *mux.Router { + handler := NewHandler(opts.Service, opts.Logger) router := mux.NewRouter() // API v1 routes v1 := router.PathPrefix("/api/v1").Subrouter() // Apply authentication middleware if auth store is provided - if authStore != nil { - authMiddleware := auth.NewRESTAuthMiddleware(authStore, logger) + if opts.AuthStore != nil { + authMiddleware := auth.NewRESTAuthMiddleware(opts.AuthStore, opts.Logger) v1.Use(authMiddleware.Middleware) } @@ -82,8 +108,8 @@ func NewRouterWithAuthAndKeyStore(service domain.NotificationService, logger *lo v1.HandleFunc("/notifiers", handler.GetNotifiers).Methods(http.MethodGet) // Key management routes (requires auth and keystore) - if authStore != nil && keyStore != nil { - keyHandler := NewKeyManagementHandler(keyStore, logger) + if opts.AuthStore != nil && opts.KeyStore != nil { + keyHandler := NewKeyManagementHandler(opts.KeyStore, opts.Logger) v1.HandleFunc("/admin/keys", keyHandler.CreateKey).Methods(http.MethodPost) v1.HandleFunc("/admin/keys", keyHandler.ListKeys).Methods(http.MethodGet) v1.HandleFunc("/admin/keys/{name}", keyHandler.RevokeKey).Methods(http.MethodDelete) @@ -91,16 +117,69 @@ func NewRouterWithAuthAndKeyStore(service domain.NotificationService, logger *lo v1.HandleFunc("/admin/keys/{name}/audit", keyHandler.GetAuditLog).Methods(http.MethodGet) } - // Health check route (no auth required) + // Liveness and readiness routes (no auth required). /health stays a pure + // liveness signal; /readyz fails when a dependency is unavailable. router.HandleFunc("/health", handler.HealthCheck).Methods(http.MethodGet) + router.HandleFunc("/readyz", readinessHandler(opts.Readiness)).Methods(http.MethodGet) - // Middleware - logging, request size limit, and CORS + // Middleware - CORS (when configured), logging, and request size limits + if opts.CORS != nil && len(opts.CORS.AllowedOrigins) > 0 { + router.Use(newCORSMiddleware(opts.CORS)) + } + if opts.Instrument != nil { + router.Use(mux.MiddlewareFunc(opts.Instrument)) + } router.Use(loggingMiddleware) v1.Use(maxBodySizeMiddleware(1 << 20)) // 1 MB limit on API request bodies return router } +// LivenessHandler returns a minimal liveness handler for dedicated health +// listeners (the REST router serves the same signal at /health). +func LivenessHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]interface{}{ + "status": "healthy", + "service": "notifier", + "time": time.Now().UTC(), + }) + }) +} + +// ReadinessHandler exposes the readiness checks for dedicated health listeners. +func ReadinessHandler(checks map[string]ReadinessCheck) http.Handler { + return readinessHandler(checks) +} + +// readinessHandler runs each dependency check and reports 503 if any fail. +func readinessHandler(checks map[string]ReadinessCheck) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) + defer cancel() + + status := http.StatusOK + components := make(map[string]string, len(checks)) + for name, check := range checks { + if err := check(ctx); err != nil { + status = http.StatusServiceUnavailable + components[name] = "unavailable" + } else { + components[name] = "ok" + } + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + ready := status == http.StatusOK + json.NewEncoder(w).Encode(map[string]interface{}{ + "ready": ready, + "components": components, + }) + } +} + // maxBodySizeMiddleware limits the size of incoming request bodies to prevent DoS. func maxBodySizeMiddleware(maxBytes int64) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { @@ -162,10 +241,15 @@ func newCORSMiddleware(config *CORSConfig) func(http.Handler) http.Handler { } } - // Handle preflight OPTIONS requests - if r.Method == http.MethodOptions { - // Return 200 OK for preflight requests - w.WriteHeader(http.StatusOK) + // Handle preflight OPTIONS requests: succeed only for allowed + // origins; disallowed cross-origin preflights get 403 with no + // CORS headers so browsers block the actual request. + if r.Method == http.MethodOptions && origin != "" { + if allowed { + w.WriteHeader(http.StatusNoContent) + } else { + w.WriteHeader(http.StatusForbidden) + } return } diff --git a/config.yaml b/config.yaml index 1212afc..56bd646 100644 --- a/config.yaml +++ b/config.yaml @@ -5,6 +5,12 @@ server: rest_port: 8080 host: "0.0.0.0" mode: "both" # Options: both, grpc, rest + # Optional TLS for both listeners. Leave disabled when a TLS-terminating + # gateway or service mesh fronts the service. + tls: + enabled: false + # cert_file: "/etc/notifier/tls/tls.crt" + # key_file: "/etc/notifier/tls/tls.key" queue: type: "local" # Options: local, kafka diff --git a/internal/config/config.go b/internal/config/config.go index 2a82c10..09e4c9b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,10 +27,20 @@ type Config struct { // ServerConfig contains server configuration type ServerConfig struct { - GRPCPort int `mapstructure:"grpc_port"` - RESTPort int `mapstructure:"rest_port"` - Host string `mapstructure:"host"` - Mode string `mapstructure:"mode"` // "both", "grpc", "rest" + GRPCPort int `mapstructure:"grpc_port"` + RESTPort int `mapstructure:"rest_port"` + Host string `mapstructure:"host"` + Mode string `mapstructure:"mode"` // "both", "grpc", "rest" + TLS TLSConfig `mapstructure:"tls"` +} + +// TLSConfig enables TLS on the REST and gRPC listeners. When disabled the +// servers speak plaintext, which is only appropriate behind a TLS-terminating +// gateway or service mesh. +type TLSConfig struct { + Enabled bool `mapstructure:"enabled"` + CertFile string `mapstructure:"cert_file"` + KeyFile string `mapstructure:"key_file"` } // NotifiersConfig contains configuration for all notifier types @@ -258,6 +268,12 @@ func (c *Config) Validate() error { return fmt.Errorf("invalid server mode: %s (must be both, grpc, or rest)", c.Server.Mode) } + if c.Server.TLS.Enabled { + if c.Server.TLS.CertFile == "" || c.Server.TLS.KeyFile == "" { + return fmt.Errorf("server.tls.enabled requires both cert_file and key_file") + } + } + // Validate queue config validQueueTypes := map[string]bool{"local": true, "kafka": true} if !validQueueTypes[c.Queue.Type] {