From 47167905643ef3ef12494f2edf8b96c3be9b7a5b Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Thu, 6 Aug 2026 13:02:35 -0400 Subject: [PATCH] fix: OPDS base_url placeholder bug + setup gate requires base_url Three bugs fixed: 1. Schema seeded base_url with fake placeholder 'bookhoard.example.com'. Removed seed; startup now seeds from BASE_URL env var only if DB row is empty (admin changes persist across restarts). One-time UPDATE clears the placeholder in existing installs. 2. config.GetBaseURL() had a broken type assertion (local SystemConfigRow vs database.SystemConfig) that always failed, returning . Admin panel showed env var fallback instead of actual DB value. Fixed with a function-type getter that properly wraps the DB query. 3. OPDS handler read base_url only from DB with no fallback. When DB had the placeholder, all feed links pointed to an unreachable domain, breaking KOReader search/download. Added deriveBaseURL() helper that falls back to the request Host/scheme when DB value is empty. Setup gate improvements: - isSetupComplete now requires both admin user AND non-empty base_url - Setup middleware no longer exempts all /api/ routes; only allows /api/auth/register, /api/auth/login, /api/system/config before setup is complete. All other API routes get 503. - Cache invalidated when base_url is saved via admin settings Dev workflow: - New bruno/NewDevDBSetup/SetBaseUrl.yml for dev DB setup - NewDB.sh runs SetBaseUrl between RegisterUser and CreateEbookLibrary --- bruno/NewDB.sh | 2 +- bruno/NewDevDBSetup/SetBaseUrl.yml | 38 +++++++++++++++++++++++ cmd/server/main.go | 29 +++++++++++++++++ database/schema/schema.sql | 14 +++++---- internal/config/config.go | 27 +++++----------- internal/handlers/opds.go | 14 ++++----- internal/handlers/sidecar.go | 37 +++++++++++----------- internal/handlers/url.go | 36 +++++++++++++++++++++ internal/router/frontend.go | 16 ++-------- internal/router/router.go | 18 +++++++++++ internal/router/setup.go | 50 ++++++++++++++++++++++++------ internal/setupstatus/status.go | 37 ++++++++++++++++------ 12 files changed, 234 insertions(+), 84 deletions(-) create mode 100644 bruno/NewDevDBSetup/SetBaseUrl.yml create mode 100644 internal/handlers/url.go diff --git a/bruno/NewDB.sh b/bruno/NewDB.sh index 03cc26f..18dd9ec 100755 --- a/bruno/NewDB.sh +++ b/bruno/NewDB.sh @@ -1,3 +1,3 @@ #!/bin/bash -bru run --env Bookhoard --delay 500 "NewDevDBSetup/RegisterUser.yml" "NewDevDBSetup/CreateEbookLibrary.yml" "NewDevDBSetup/CreateComicLibrary.yml" "NewDevDBSetup/CreateMangaLibrary.yml" "NewDevDBSetup/AddEbookLibraryFolder.yml" "NewDevDBSetup/AddComicLibraryFolder.yml" "NewDevDBSetup/AddMangaLibraryFolder.yml" "NewDevDBSetup/ScanAllLibraries.yml" +bru run --env Bookhoard --delay 500 "NewDevDBSetup/RegisterUser.yml" "NewDevDBSetup/SetBaseUrl.yml" "NewDevDBSetup/CreateEbookLibrary.yml" "NewDevDBSetup/CreateComicLibrary.yml" "NewDevDBSetup/CreateMangaLibrary.yml" "NewDevDBSetup/AddEbookLibraryFolder.yml" "NewDevDBSetup/AddComicLibraryFolder.yml" "NewDevDBSetup/AddMangaLibraryFolder.yml" "NewDevDBSetup/ScanAllLibraries.yml" diff --git a/bruno/NewDevDBSetup/SetBaseUrl.yml b/bruno/NewDevDBSetup/SetBaseUrl.yml new file mode 100644 index 0000000..27ba77d --- /dev/null +++ b/bruno/NewDevDBSetup/SetBaseUrl.yml @@ -0,0 +1,38 @@ +info: + name: SetBaseUrl + type: http + seq: 3 + +http: + method: PUT + url: '{{base_url}}/api/system/config' + auth: inherit + body: + type: json + jsonBody: |- + { + "base_url": "http://localhost:8765" + } + headers: + - key: Authorization + value: Bearer {{token}} + - key: Content-Type + value: application/json + +settings: + encodeUrl: true + timeout: 0 + followRedirects: true + maxRedirects: 5 + +docs: |- + ## Set Base URL + + Configures the server's base_url during initial dev database setup. + + Must be run after RegisterUser (which provides the auth token) and before + any library/device creation (which require setup to be complete). + + **Method:** PUT + **Endpoint:** /api/system/config + **Auth:** Bearer token (from RegisterUser) diff --git a/cmd/server/main.go b/cmd/server/main.go index 6ccd173..7586134 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -48,6 +48,35 @@ func main() { } log.Println("✅ Database schema initialized and verified, starting server...") + // Seed base_url from env var if not already configured. Uses conditional + // UPDATE so admin-set values are never overwritten on restart. + if cfg.BaseURL != "" { + _, err = dbPool.Exec(ctx, ` + INSERT INTO system_config (key, value) + VALUES ('base_url', $1) + ON CONFLICT (key) DO UPDATE + SET value = EXCLUDED.value + WHERE system_config.value = '' + `, cfg.BaseURL) + if err != nil { + log.Printf("⚠️ Could not seed base_url: %v", err) + } else { + // Also seed derived URLs + for key, suffix := range map[string]string{ + "opds_base_url": "/opds", + "api_base_url": "/api", + } { + _, _ = dbPool.Exec(ctx, ` + INSERT INTO system_config (key, value) + VALUES ($1, $2) + ON CONFLICT (key) DO UPDATE + SET value = EXCLUDED.value + WHERE system_config.value = '' + `, key, cfg.BaseURL+suffix) + } + } + } + // Create login attempt tracker: 5 failed attempts = 15 minute lockout loginAttemptTracker := ratelimit.NewLoginAttemptTracker(5, 15*time.Minute, 5*time.Minute) diff --git a/database/schema/schema.sql b/database/schema/schema.sql index 9d72688..0629cf6 100644 --- a/database/schema/schema.sql +++ b/database/schema/schema.sql @@ -1149,12 +1149,14 @@ CREATE TABLE IF NOT EXISTS system_config ( updated_by UUID REFERENCES users(id) ); --- Pre-seeded values -INSERT INTO system_config (key, value) VALUES -('base_url', 'https://bookhoard.example.com'), -('opds_base_url', 'https://bookhoard.example.com/opds'), -('api_base_url', 'https://bookhoard.example.com/api') -ON CONFLICT (key) DO NOTHING; +-- One-time cleanup: clear the old placeholder seed so the startup logic +-- can re-seed from the BASE_URL env var (or the setup wizard can set it). +UPDATE system_config SET value = '' +WHERE key = 'base_url' AND value = 'https://bookhoard.example.com'; +UPDATE system_config SET value = '' +WHERE key = 'opds_base_url' AND value = 'https://bookhoard.example.com/opds'; +UPDATE system_config SET value = '' +WHERE key = 'api_base_url' AND value = 'https://bookhoard.example.com/api'; -- Create opds_tokens table (device-specific OPDS access tokens) CREATE TABLE IF NOT EXISTS opds_tokens ( diff --git a/internal/config/config.go b/internal/config/config.go index a68adf9..137334f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -45,30 +45,19 @@ func (c *Config) DatabaseURL() string { c.DatabaseUser, c.DatabasePassword, c.DatabaseHost, c.DatabasePort, c.DatabaseName) } -// GetBaseURL returns the base URL from system configuration database with fallback to config/env var -func GetBaseURL(ctx context.Context, db interface{}) string { - // Try to get from database first - type SystemConfigQuerier interface { - GetSystemConfig(ctx context.Context, key string) (SystemConfigRow, error) - } +// SystemConfigGetter returns the value for a system config key, or an error. +type SystemConfigGetter func(ctx context.Context, key string) (string, error) - if querier, ok := db.(SystemConfigQuerier); ok { - config, err := querier.GetSystemConfig(ctx, "base_url") - if err == nil && config.Value != "" { - return config.Value - } +// GetBaseURL returns the base URL from system configuration database, or empty +// string if not set. The getter abstraction avoids importing the database package. +func GetBaseURL(ctx context.Context, getter SystemConfigGetter) string { + val, err := getter(ctx, "base_url") + if err == nil && val != "" { + return val } - - // Fallback: return empty string - caller should use their own fallback return "" } -// SystemConfigRow represents a system configuration row -type SystemConfigRow struct { - Key string - Value string -} - func getEnv(key, defaultValue string) string { if value := os.Getenv(key); value != "" { return value diff --git a/internal/handlers/opds.go b/internal/handlers/opds.go index 1d23b74..69287d6 100644 --- a/internal/handlers/opds.go +++ b/internal/handlers/opds.go @@ -38,15 +38,15 @@ func NewOPDSHandler(db *database.Queries, libraryService *services.LibraryServic } } -// Helper function to get base URL from system config +// Helper function to get base URL from system config with request-derived fallback func (h *OPDSHandler) getBaseURLs(c *echo.Context) (string, string, error) { - baseURL, err := h.db.GetSystemConfig(c.Request().Context(), "base_url") - if err != nil { - return "", "", fmt.Errorf("failed to get base_url from config: %w", err) + var dbBaseURL string + if config, err := h.db.GetSystemConfig(c.Request().Context(), "base_url"); err == nil { + dbBaseURL = config.Value } - - opdsBaseURL := baseURL.Value + "/opds" - return baseURL.Value, opdsBaseURL, nil + baseURL := deriveBaseURL(c, dbBaseURL) + opdsBaseURL := baseURL + "/opds" + return baseURL, opdsBaseURL, nil } func (h *OPDSHandler) getAuthToken(c *echo.Context) string { diff --git a/internal/handlers/sidecar.go b/internal/handlers/sidecar.go index 678397e..bf36b06 100644 --- a/internal/handlers/sidecar.go +++ b/internal/handlers/sidecar.go @@ -3,6 +3,7 @@ package handlers import ( "bookhoard/internal/config" "bookhoard/internal/database" + "bookhoard/internal/setupstatus" "encoding/json" "fmt" "net/http" @@ -81,15 +82,13 @@ func (h *SidecarHandler) GetSidecarConfig(c *echo.Context) error { userID := device.UserID.Bytes pgUserID := pgtype.UUID{Bytes: userID, Valid: true} - // Get base URL and compute paths - baseURL, _ := h.db.GetSystemConfig(ctx, "base_url") - if baseURL.Value == "" { - baseURL.Value = h.cfg.BaseURL - } + // Get base URL and compute paths (with request-derived fallback) + dbBaseURL, _ := h.db.GetSystemConfig(ctx, "base_url") + baseURL := deriveBaseURL(c, dbBaseURL.Value) // Generate URLs - opdsCatalogURL := fmt.Sprintf("%s/opds/devices/%s/catalog", baseURL.Value, deviceID.String()) - syncAPIURL := fmt.Sprintf("%s/api/sync/kobo", baseURL.Value) + opdsCatalogURL := fmt.Sprintf("%s/opds/devices/%s/catalog", baseURL, deviceID.String()) + syncAPIURL := fmt.Sprintf("%s/api/sync/kobo", baseURL) // Get user's visible libraries with media items mediaItems, err := h.db.GetUserMediaItemsForSync(ctx, pgUserID) @@ -176,8 +175,8 @@ func (h *SidecarHandler) GetSidecarConfig(c *echo.Context) error { Bookhoard: SidecarBookhoardConfig{ OPDSCatalog: opdsCatalogURL, SyncAPI: syncAPIURL, - OPDSBaseURL: baseURL.Value + "/opds", - APIBaseURL: baseURL.Value + "/api", + OPDSBaseURL: baseURL + "/opds", + APIBaseURL: baseURL + "/api", DeviceID: deviceID.String(), DeviceToken: device.AuthToken, }, @@ -219,15 +218,13 @@ func (h *SidecarHandler) DownloadSidecarConfig(c *echo.Context) error { userID := device.UserID.Bytes pgUserID := pgtype.UUID{Bytes: userID, Valid: true} - // Get base URL and compute paths - baseURL, _ := h.db.GetSystemConfig(ctx, "base_url") - if baseURL.Value == "" { - baseURL.Value = h.cfg.BaseURL - } + // Get base URL and compute paths (with request-derived fallback) + dbBaseURL, _ := h.db.GetSystemConfig(ctx, "base_url") + baseURL := deriveBaseURL(c, dbBaseURL.Value) // Generate URLs - opdsCatalogURL := fmt.Sprintf("%s/opds/devices/%s/catalog", baseURL.Value, deviceID.String()) - syncAPIURL := fmt.Sprintf("%s/api/sync/kobo", baseURL.Value) + opdsCatalogURL := fmt.Sprintf("%s/opds/devices/%s/catalog", baseURL, deviceID.String()) + syncAPIURL := fmt.Sprintf("%s/api/sync/kobo", baseURL) // Get user's visible libraries with media items mediaItems, err := h.db.GetUserMediaItemsForSync(ctx, pgUserID) @@ -309,8 +306,8 @@ func (h *SidecarHandler) DownloadSidecarConfig(c *echo.Context) error { Bookhoard: SidecarBookhoardConfig{ OPDSCatalog: opdsCatalogURL, SyncAPI: syncAPIURL, - OPDSBaseURL: baseURL.Value + "/opds", - APIBaseURL: baseURL.Value + "/api", + OPDSBaseURL: baseURL + "/opds", + APIBaseURL: baseURL + "/api", DeviceID: deviceID.String(), DeviceToken: device.AuthToken, }, @@ -434,6 +431,10 @@ func (h *SidecarHandler) UpdateSystemConfiguration(c *echo.Context) error { }) } } + + // Invalidate setup status cache so the middleware picks up the new + // base_url immediately (setup is not complete until base_url is set). + setupstatus.Invalidate() } // Check for HTMX request diff --git a/internal/handlers/url.go b/internal/handlers/url.go new file mode 100644 index 0000000..eaaa21d --- /dev/null +++ b/internal/handlers/url.go @@ -0,0 +1,36 @@ +package handlers + +import ( + "strings" + + "github.com/labstack/echo/v5" +) + +// deriveBaseURL returns the base URL to use for constructing self-referential +// links (OPDS feeds, sidecar config, etc.). It prefers the database-configured +// base_url when available, and falls back to deriving the URL from the incoming +// HTTP request (Host header + scheme), which is always reachable by the client. +// +// Proxy header support: X-Forwarded-Proto and X-Forwarded-Host are respected so +// that deployments behind TLS-terminating reverse proxies advertise the correct +// external URL. +func deriveBaseURL(c *echo.Context, dbBaseURL string) string { + if dbBaseURL != "" { + return strings.TrimRight(dbBaseURL, "/") + } + + scheme := "http" + if c.Request().TLS != nil { + scheme = "https" + } + if proto := c.Request().Header.Get("X-Forwarded-Proto"); proto != "" { + scheme = proto + } + + host := c.Request().Host + if forwarded := c.Request().Header.Get("X-Forwarded-Host"); forwarded != "" { + host = forwarded + } + + return scheme + "://" + host +} diff --git a/internal/router/frontend.go b/internal/router/frontend.go index 91fd900..1ee0774 100644 --- a/internal/router/frontend.go +++ b/internal/router/frontend.go @@ -9,7 +9,6 @@ import ( "strconv" "time" - "bookhoard/internal/config" "bookhoard/internal/database" "bookhoard/internal/handlers" "bookhoard/internal/services" @@ -734,10 +733,7 @@ func registerFrontendRoutes(cfg *Config) { } // Get base URL from database config with fallback to config/env var - baseURL := config.GetBaseURL(c.Request().Context(), cfg.Queries) - if baseURL == "" { - baseURL = cfg.Cfg.BaseURL - } + baseURL := cfg.getBaseURL(c.Request().Context()) var buf bytes.Buffer err = templates.Devices(user, devices, pendingList, errorMsg, baseURL).Render(c.Request().Context(), &buf) @@ -995,10 +991,7 @@ func registerFrontendRoutes(cfg *Config) { } // Fetch current system configuration - just base_url - baseURL := config.GetBaseURL(c.Request().Context(), cfg.Queries) - if baseURL == "" { - baseURL = cfg.Cfg.BaseURL - } + baseURL := cfg.getBaseURL(c.Request().Context()) systemConfig := map[string]string{ "base_url": baseURL, @@ -1086,10 +1079,7 @@ func registerFrontendRoutes(cfg *Config) { } // Get base URL from database config with fallback to config/env var - baseURL := config.GetBaseURL(c.Request().Context(), cfg.Queries) - if baseURL == "" { - baseURL = cfg.Cfg.BaseURL - } + baseURL := cfg.getBaseURL(c.Request().Context()) var buf bytes.Buffer err = templates.Devices(user, devices, pendingList, errorMsg, baseURL).Render(c.Request().Context(), &buf) diff --git a/internal/router/router.go b/internal/router/router.go index 9e2f20d..4c956ab 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -72,6 +72,24 @@ type Config struct { LibraryService *services.LibraryService } +// getBaseURL returns the configured base URL from the database, falling back to +// the env var / config default. Uses a closure to adapt the database query to +// config.SystemConfigGetter. +func (cfg *Config) getBaseURL(ctx context.Context) string { + getter := func(ctx context.Context, key string) (string, error) { + row, err := cfg.Queries.GetSystemConfig(ctx, key) + if err != nil { + return "", err + } + return row.Value, nil + } + baseURL := config.GetBaseURL(ctx, getter) + if baseURL == "" { + baseURL = cfg.Cfg.BaseURL + } + return baseURL +} + // createJWTMiddleware creates a JWT middleware with proper user context setup func createJWTMiddleware(cfg *Config) echo.MiddlewareFunc { return echojwt.WithConfig(echojwt.Config{ diff --git a/internal/router/setup.go b/internal/router/setup.go index d951635..cd9f71a 100644 --- a/internal/router/setup.go +++ b/internal/router/setup.go @@ -14,7 +14,40 @@ import ( ) func isSetupComplete(cfg *Config) bool { - return setupstatus.IsSetupComplete(context.Background(), cfg.Queries) + getter := func(ctx context.Context) (string, error) { + row, err := cfg.Queries.GetSystemConfig(ctx, "base_url") + if err != nil { + return "", err + } + return row.Value, nil + } + return setupstatus.IsSetupComplete(context.Background(), cfg.Queries, getter) +} + +// setupAllowedAPIRoutes lists API endpoints that remain accessible before +// initial setup is complete so the server can be configured via API. +var setupAllowedAPIRoutes = []string{ + "/api/auth/register", + "/api/auth/login", + "/api/system/config", +} + +// isAllowedDuringSetup reports whether a request path should bypass the setup +// gate. This includes the setup page itself, static assets, health checks, and +// the minimal set of API routes needed to perform initial configuration. +func isAllowedDuringSetup(path string) bool { + if path == "/setup" || path == "/setup/" { + return true + } + if strings.HasPrefix(path, "/static/") || path == "/health" || path == "/favicon.ico" { + return true + } + for _, route := range setupAllowedAPIRoutes { + if path == route || strings.HasPrefix(path, route+"/") { + return true + } + } + return false } func setupRedirectMiddleware(cfg *Config) echo.MiddlewareFunc { @@ -22,19 +55,16 @@ func setupRedirectMiddleware(cfg *Config) echo.MiddlewareFunc { return func(c *echo.Context) error { path := c.Request().URL.Path - if path == "/setup" || path == "/setup/" { - return next(c) - } - - if strings.HasPrefix(path, "/api/") { - return next(c) - } - - if strings.HasPrefix(path, "/static/") || path == "/health" || path == "/favicon.ico" { + if isAllowedDuringSetup(path) { return next(c) } if !isSetupComplete(cfg) { + if strings.HasPrefix(path, "/api/") { + return c.JSON(http.StatusServiceUnavailable, map[string]string{ + "error": "Server setup is not complete. Configure an admin account and base_url via the setup wizard or API.", + }) + } return c.Redirect(http.StatusFound, "/setup") } diff --git a/internal/setupstatus/status.go b/internal/setupstatus/status.go index d95fc19..e91b803 100644 --- a/internal/setupstatus/status.go +++ b/internal/setupstatus/status.go @@ -1,8 +1,9 @@ // Package setupstatus reports whether the application's initial setup has been -// completed. Setup is considered complete as soon as at least one admin user -// exists, regardless of how that user was created (setup wizard, API, or a -// future CLI). This keeps the setup gate a derived property of real data -// rather than a manually-flipped flag that can drift out of sync. +// completed. Setup is considered complete when at least one admin user exists +// AND a non-empty base_url has been configured, regardless of how those were +// created (setup wizard, API, or a future CLI). This keeps the setup gate a +// derived property of real data rather than a manually-flipped flag that can +// drift out of sync. package setupstatus import ( @@ -18,6 +19,12 @@ type AdminCounter interface { CountAdmins(ctx context.Context) (int64, error) } +// BaseURLGetter returns the configured base_url value from the database, or an +// error if it cannot be read. Defined as a function type (not an interface) so +// it can be satisfied by a closure wrapping *database.Queries.GetSystemConfig +// without importing the database package. +type BaseURLGetter func(ctx context.Context) (string, error) + var ( cacheMu sync.RWMutex cacheComplete bool = true @@ -26,10 +33,11 @@ var ( ) // IsSetupComplete reports whether setup is complete. Setup is complete when at -// least one admin user exists. A short in-memory cache avoids hammering the -// database on every request. On a database error the function fails open -// (returns true) so a transient outage does not lock users out of the app. -func IsSetupComplete(ctx context.Context, q AdminCounter) bool { +// least one admin user exists AND base_url is configured. A short in-memory +// cache avoids hammering the database on every request. On a database error the +// function fails open (returns true) so a transient outage does not lock users +// out of the app. +func IsSetupComplete(ctx context.Context, q AdminCounter, baseURLGetter BaseURLGetter) bool { cacheMu.RLock() if time.Now().Before(cacheExpiry) { complete := cacheComplete @@ -38,12 +46,20 @@ func IsSetupComplete(ctx context.Context, q AdminCounter) bool { } cacheMu.RUnlock() - count, err := q.CountAdmins(ctx) complete := true + + count, err := q.CountAdmins(ctx) if err == nil { complete = count > 0 } + if complete && baseURLGetter != nil { + baseURL, err := baseURLGetter(ctx) + if err == nil { + complete = baseURL != "" + } + } + cacheMu.Lock() cacheComplete = complete cacheExpiry = time.Now().Add(cacheTTL) @@ -53,7 +69,8 @@ func IsSetupComplete(ctx context.Context, q AdminCounter) bool { // Invalidate clears the cached setup status so the next call to IsSetupComplete // re-reads from the database. Call this after any write that could change the -// admin user count (user creation, role promotion/demotion, user deletion). +// admin user count (user creation, role promotion/demotion, user deletion) or +// the base_url configuration. func Invalidate() { cacheMu.Lock() cacheComplete = true