From a716b386a1e46bb54887a8f770c40c3871a8017f Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sun, 25 Oct 2026 11:06:26 -0400 Subject: [PATCH] fix: wire security headers, graceful shutdown, and settings invalidation Lifecycle (internal/app + main.go): - App.OnShutdown hooks run after the HTTP server drains, in registration order: Worker.Shutdown (background jobs now drain instead of dying on SIGTERM), DeviceAuthMiddleware.Stop (releases the rate-limiter ticker), Handler.CloseScanner (fsnotify watcher and scan log files) Security headers: - SecurityHeadersMiddleware (survivor of middleware/security.go, renamed security_headers.go) is now on the echo stack: nosniff, frame-deny, HSTS, CSP (unsafe-inline required by the sanctioned theme flash guard + Alpine), referrer and permissions policy Scanner settings cache: - settings saves (UpdateScanSettings + ApplySetting for the three scan keys) now invalidate MediaScanner's 30s settings cache via SystemSettingsHandler.SetScanSettingsChanged, so changes apply on the next poll instead of after the TTL - MediaScanner.Close closes the scan logger, fixing a file-descriptor leak on every per-item rescan (fresh scanners never closed logs) Client fix: - custom-section-builder's add/remove-selection buttons used inline onclick attributes referencing module-scoped functions, which are not global in a bundled ES module: the buttons threw ReferenceError. Functions restored and exposed on window via declare global Also fixes a pre-existing vet error (saveCoverImage copied echo.Context by value, mutex included) and drops the per-key SettingsCache.InvalidateKey variant left callerless by the full-flush invalidation wiring. --- cmd/server/main.go | 14 +++ internal/app/app.go | 14 +++ internal/handlers/commonhandlers.go | 16 +++ internal/handlers/media.go | 7 +- internal/handlers/system_settings.go | 26 ++++- internal/middleware/device_auth.go | 59 +---------- internal/middleware/security_headers.go | 127 ------------------------ internal/services/cache.go | 7 -- internal/services/media_scanner.go | 12 +++ web/src/custom-section-builder.ts | 11 +- 10 files changed, 98 insertions(+), 195 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index 92b536d7..f01ab502 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -191,6 +191,7 @@ func main() { ExposeHeaders: []string{"Content-Length"}, AllowCredentials: false, })) + e.Use(ratelimit.SecurityHeadersMiddleware()) e.Use(ratelimit.RequestTracingMiddleware(cfg)) // Rate limiter for auth endpoints @@ -254,6 +255,19 @@ func main() { // Create app with lifecycle management application := app.New(e) + // Graceful shutdown: after the HTTP server drains, stop background work โ€” + // drain job workers, release the device rate limiter's ticker, close scan + // log files โ€” in registration order. + application.OnShutdown(func() { worker.Shutdown() }) + application.OnShutdown(func() { deviceAuthMiddleware.Stop() }) + application.OnShutdown(func() { routerConfig.ScannerHandler.CloseScanner() }) + + // Scan settings saves reach the scanner immediately instead of after its + // 30s settings-cache TTL. + systemSettingsHandler.SetScanSettingsChanged(func() { + routerConfig.ScannerHandler.InvalidateScannerSettingsCache() + }) + // ======================================================================== // START SERVER (managed by app lifecycle) // ======================================================================== diff --git a/internal/app/app.go b/internal/app/app.go index 644379ca..fb03006b 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -21,6 +21,7 @@ type App struct { shutdownTimeout time.Duration shutdownMutex sync.Mutex shutdownDone chan struct{} + shutdownHooks []func() } // New creates a new App instance @@ -99,11 +100,24 @@ func (a *App) Shutdown() error { log.Printf("Error stopping HTTP server: %v", err) } } + + // Run shutdown hooks: drain background workers, stop tickers, close files + for _, hook := range a.shutdownHooks { + hook() + } + close(a.shutdownDone) log.Println("Graceful shutdown completed successfully") return nil } +// OnShutdown registers a hook to run during graceful shutdown, after the HTTP +// server has drained. Hooks run in registration order; register background +// workers, tickers, and file handles here. +func (a *App) OnShutdown(fn func()) { + a.shutdownHooks = append(a.shutdownHooks, fn) +} + // SetShutdownTimeout sets the maximum time to wait for graceful shutdown func (a *App) SetShutdownTimeout(timeout time.Duration) { a.shutdownTimeout = timeout diff --git a/internal/handlers/commonhandlers.go b/internal/handlers/commonhandlers.go index 9ad44ecf..930aa77c 100644 --- a/internal/handlers/commonhandlers.go +++ b/internal/handlers/commonhandlers.go @@ -54,6 +54,22 @@ func NewHandler(db *database.Queries, connManager *wsync.ConnectionManager, queu } } +// CloseScanner releases scanner resources (fsnotify watcher, scan log files). +// Call once during graceful shutdown. +func (h *Handler) CloseScanner() { + if h.scanner != nil { + _ = h.scanner.Close() + } +} + +// InvalidateScannerSettingsCache drops the scanner's cached system settings so +// saved scan settings take effect on the next poll instead of after the TTL. +func (h *Handler) InvalidateScannerSettingsCache() { + if h.scanner != nil { + h.scanner.InvalidateSettingsCache() + } +} + // parseDate parses a date string in YYYY-MM-DD format func parseDate(dateStr string) time.Time { if dateStr == "" { diff --git a/internal/handlers/media.go b/internal/handlers/media.go index 5b59bb1e..fe644532 100644 --- a/internal/handlers/media.go +++ b/internal/handlers/media.go @@ -1550,7 +1550,7 @@ func (mh *MediaHandler) UpdateMediaItem(c *echo.Context) error { } else if req.CoverAction == "upload" { file, err := c.FormFile("cover_file") if err == nil { - savedPath, err := mh.saveCoverImage(*c, mediaUUID, file) + savedPath, err := mh.saveCoverImage(c, mediaUUID, file) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to save cover image"}) } @@ -2876,7 +2876,7 @@ func jsonBytesToMap(b []byte) map[string]interface{} { return result } -func (mh *MediaHandler) saveCoverImage(c echo.Context, mediaUUID uuid.UUID, file *multipart.FileHeader) (string, error) { +func (mh *MediaHandler) saveCoverImage(c *echo.Context, mediaUUID uuid.UUID, file *multipart.FileHeader) (string, error) { src, err := file.Open() if err != nil { return "", fmt.Errorf("failed to open uploaded file: %w", err) @@ -3030,7 +3030,8 @@ func (mh *MediaHandler) GetAudioChapters(c *echo.Context) error { // SetAudioSpeed handles PUT /api/media-items/:id/audio-speed โ€” the // per-user per-book playback speed upsert (plan ยง8). The client SPEEDS // list is the real gate; the server sanity-checks the range. -func (mh *MediaHandler) SetAudioSpeed(c *echo.Context) error { mediaID := c.Param("id") +func (mh *MediaHandler) SetAudioSpeed(c *echo.Context) error { + mediaID := c.Param("id") mediaUUID, err := uuid.Parse(mediaID) if err != nil { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid media item id"}) diff --git a/internal/handlers/system_settings.go b/internal/handlers/system_settings.go index 0721b4fe..0928a006 100644 --- a/internal/handlers/system_settings.go +++ b/internal/handlers/system_settings.go @@ -15,8 +15,9 @@ import ( ) type SystemSettingsHandler struct { - db *database.Queries - settings *database.SettingsRegistry + db *database.Queries + settings *database.SettingsRegistry + onScanSettingsChanged func() } func NewSystemSettingsHandler(db *database.Queries) *SystemSettingsHandler { @@ -25,6 +26,19 @@ func NewSystemSettingsHandler(db *database.Queries) *SystemSettingsHandler { } } +// SetScanSettingsChanged registers a callback fired after scan settings are +// successfully saved on any write path, so listeners (the scanner) can drop +// their cached values immediately instead of waiting out the cache TTL. +func (h *SystemSettingsHandler) SetScanSettingsChanged(fn func()) { + h.onScanSettingsChanged = fn +} + +func (h *SystemSettingsHandler) notifyScanSettingsChanged() { + if h.onScanSettingsChanged != nil { + h.onScanSettingsChanged() + } +} + // SetSettings wires the tunable settings registry. Required for the unified // /api/system/settings endpoints and for cache invalidation after writes. func (h *SystemSettingsHandler) SetSettings(s *database.SettingsRegistry) { @@ -121,6 +135,12 @@ func (h *SystemSettingsHandler) ApplySetting(ctx context.Context, key, value str h.settings.Reload(ctx) + // Scan keys are also cached inside MediaScanner (30s TTL); let it drop + // its cache so the change takes effect on the next poll. + if key == "scan_poll_interval_seconds" || key == "auto_scan_enabled" || key == "scan_extractor_version" { + h.notifyScanSettingsChanged() + } + resp := UpdateSettingResponse{ReloadRequired: def.RequiresRestart} for _, e := range h.settings.All() { if e.Key == key { @@ -218,6 +238,8 @@ func (h *SystemSettingsHandler) UpdateScanSettings(c *echo.Context) error { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } + h.notifyScanSettingsChanged() + h.reload(c) return c.JSON(http.StatusOK, ScanSettingsResponse{ diff --git a/internal/middleware/device_auth.go b/internal/middleware/device_auth.go index 8dc68d18..3b6ea0cb 100644 --- a/internal/middleware/device_auth.go +++ b/internal/middleware/device_auth.go @@ -197,61 +197,10 @@ func (m *DeviceAuthMiddleware) getRequestType(path string) string { return "sync" } -func (m *DeviceAuthMiddleware) RequirePermission(permission string) echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - device, ok := c.Get("device").(database.Devices) - if !ok { - return c.JSON(http.StatusUnauthorized, map[string]string{ - "error": "device not authenticated", - }) - } - - if !m.hasPermission(device.DeviceType, permission) { - return c.JSON(http.StatusForbidden, map[string]string{ - "error": "insufficient permissions", - }) - } - - return next(c) - } - } -} - -func (m *DeviceAuthMiddleware) hasPermission(deviceType string, permission string) bool { - permissions := map[string][]string{ - "koreader": {"sync:progress", "sync:annotations", "sync:metadata"}, - "kobo": {"sync:progress", "sync:annotations", "sync:metadata"}, - "web": {"sync:progress", "sync:annotations", "sync:metadata", "device:manage"}, - "mobile": {"sync:progress", "sync:annotations", "sync:metadata"}, - } - - devicePerms, exists := permissions[deviceType] - if !exists { - return false - } - - for _, p := range devicePerms { - if p == permission { - return true - } - } - - return false -} - -func (m *DeviceAuthMiddleware) UpdateLastSeen(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - err := next(c) - - deviceIDBytes, ok := c.Get("device_id").([16]byte) - if ok { - pgDeviceID := pgtype.UUID{Bytes: deviceIDBytes, Valid: true} - m.db.UpdateDeviceLastSeen(c.Request().Context(), pgDeviceID) - } - - return err - } +// Stop releases the device rate limiter's cleanup resources (its ticker +// goroutine). Call once during graceful shutdown. +func (m *DeviceAuthMiddleware) Stop() { + m.rateLimiter.Stop() } func (m *DeviceAuthMiddleware) ValidateDeviceToken(token string) (database.Devices, error) { diff --git a/internal/middleware/security_headers.go b/internal/middleware/security_headers.go index 755ca227..b6c0320c 100644 --- a/internal/middleware/security_headers.go +++ b/internal/middleware/security_headers.go @@ -1,10 +1,6 @@ package middleware import ( - "net/http" - "strconv" - "strings" - "github.com/labstack/echo/v5" ) @@ -25,126 +21,3 @@ func SecurityHeadersMiddleware() echo.MiddlewareFunc { } } } - -// HTTPSRedirectMiddleware redirects HTTP to HTTPS -// NOTE: Disabled for Docker self-hosted deployments - SSL is handled by reverse proxy -func HTTPSRedirectMiddleware(httpsPort string) echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - // SSL is handled by proxy - no redirect needed - return next(c) - } - } -} - -// isTestMode checks if the application is running in test mode -func isTestMode(c *echo.Context) bool { - // Check for test mode header or environment - return c.Request().Header.Get("X-Test-Mode") == "true" || - c.Request().Header.Get("X-Forwarded-Proto") == "http" -} - -// SSLProxyMiddleware handles SSL termination proxy headers -// NOTE: Essential for Docker deployments where SSL is handled by reverse proxy (nginx, traefik, etc.) -// This middleware reads X-Forwarded-Proto and X-Forwarded-Host headers set by the proxy -func SSLProxyMiddleware() echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - // Check for proxy headers - if proto := c.Request().Header.Get("X-Forwarded-Proto"); proto == "https" { - c.Request().URL.Scheme = "https" - } - - if host := c.Request().Header.Get("X-Forwarded-Host"); host != "" { - c.Request().URL.Host = host - } - - return next(c) - } - } -} - -// HTTPSProtectionMiddleware provides HTTPS protection for production -func HTTPSProtectionMiddleware(enableRedirect bool, httpsPort string) []echo.MiddlewareFunc { - return []echo.MiddlewareFunc{ - SSLProxyMiddleware(), - SecurityHeadersMiddleware(), - HTTPSRedirectMiddleware(httpsPort), - } -} - -// CORSSecurityConfig configures CORS with security best practices -type CORSSecurityConfig struct { - AllowedOrigins []string - AllowedMethods []string - AllowedHeaders []string - ExposedHeaders []string - AllowCredentials bool - MaxAge int -} - -// NewSecureCORSConfig creates production-ready CORS config -func NewSecureCORSConfig() CORSSecurityConfig { - return CORSSecurityConfig{ - AllowedOrigins: []string{ - "https://bookhoard.example.com", - "https://*.bookhoard.example.com", - }, - AllowedMethods: []string{"GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"}, - AllowedHeaders: []string{ - "Accept", - "Authorization", - "Content-Type", - "X-Request-ID", - "X-Timestamp", - "X-Signature", - "X-Device-ID", - }, - ExposedHeaders: []string{ - "X-Request-ID", - "X-Sync-Status", - "X-Conflict-Detected", - }, - AllowCredentials: true, - MaxAge: 86400, // 24 hours - } -} - -// SecureCORSMiddleware creates CORS middleware with security -func SecureCORSMiddleware(config CORSSecurityConfig) echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - origin := c.Request().Header.Get("Origin") - - // Check if origin is allowed - allowed := false - for _, allowedOrigin := range config.AllowedOrigins { - if origin == allowedOrigin || allowedOrigin == "*" { - allowed = true - break - } - } - - if !allowed { - return c.JSON(http.StatusForbidden, map[string]string{ - "error": "Origin not allowed", - }) - } - - // Set CORS headers - c.Response().Header().Set("Access-Control-Allow-Origin", origin) - c.Response().Header().Set("Access-Control-Allow-Methods", strings.Join(config.AllowedMethods, ", ")) - c.Response().Header().Set("Access-Control-Allow-Headers", strings.Join(config.AllowedHeaders, ", ")) - c.Response().Header().Set("Access-Control-Expose-Headers", strings.Join(config.ExposedHeaders, ", ")) - c.Response().Header().Set("Access-Control-Allow-Credentials", "true") - c.Response().Header().Set("Access-Control-Max-Age", strconv.Itoa(config.MaxAge)) - - // Handle preflight - if c.Request().Method == "OPTIONS" { - return c.NoContent(http.StatusNoContent) - } - - return next(c) - } - } -} diff --git a/internal/services/cache.go b/internal/services/cache.go index 21fa5416..492fca7f 100644 --- a/internal/services/cache.go +++ b/internal/services/cache.go @@ -48,10 +48,3 @@ func (c *SettingsCache) Invalidate() { c.data = make(map[string]string) c.lastUpdate = time.Time{} } - -func (c *SettingsCache) InvalidateKey(key string) { - c.mu.Lock() - defer c.mu.Unlock() - - delete(c.data, key) -} diff --git a/internal/services/media_scanner.go b/internal/services/media_scanner.go index 4f4bd446..fc7f0991 100644 --- a/internal/services/media_scanner.go +++ b/internal/services/media_scanner.go @@ -343,6 +343,13 @@ func (s *MediaScanner) GetAutoScanEnabled() bool { return strings.ToLower(setting) == "true" } +// InvalidateSettingsCache drops all cached setting values so the next read +// re-queries system_settings. Called when scan settings are saved; without it +// saved values only take effect after the cache TTL (30s). +func (s *MediaScanner) InvalidateSettingsCache() { + s.settingsCache.Invalidate() +} + // GetAutoPlaylistCollections mirrors GetAutoScanEnabled for the A2c // playlist-collection toggle. Defaults ON: the feature only creates // deletable curated collections from explicit "(Reading Order)" markers. @@ -4692,6 +4699,11 @@ func (s *MediaScanner) performInitialScan(ctx context.Context) { func (s *MediaScanner) Close() error { fmt.Printf("Cleaning up scanner resources...\n") + // Close scan log files (no-op-safe if already closed) + if s.logger != nil { + s.logger.Close() + } + // Stop watching if s.watcher != nil { if err := s.watcher.Close(); err != nil { diff --git a/web/src/custom-section-builder.ts b/web/src/custom-section-builder.ts index e9ff6a5a..433db06a 100644 --- a/web/src/custom-section-builder.ts +++ b/web/src/custom-section-builder.ts @@ -626,4 +626,13 @@ function builderEscapeHtml(text: string): string { initCustomSectionBuilder(); -export { addBookToSelection, removeBookFromSelection }; +// Inline onclick handlers in the generated selection HTML resolve against +// globals; bundled module functions need explicit exposure. +declare global { + interface Window { + addBookToSelection: typeof addBookToSelection; + removeBookFromSelection: typeof removeBookFromSelection; + } +} +window.addBookToSelection = addBookToSelection; +window.removeBookFromSelection = removeBookFromSelection;