From 8aaf69849da611264bb1a7962c3c31f239d9a48d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:00:18 -0700 Subject: [PATCH 01/11] fix(server): route OIDC endpoints in HTTP transport and add graceful shutdown Closes #1024: /oidc/.well-known/* was only wired in the Lambda transport (handleLambdaHTTPEvent); the HTTP/container transport (Cloud Run, Container Apps) had no OIDC registration so requests fell to the SPA file server and returned HTML or 404 -- breaking federated-credential trust chains. Fix: register handleOIDCHTTP at api.OIDCBasePath+"/" in CreateHTTPServer, placed before the static/SPA handler so the ordering matches handleLambdaHTTPEvent (D1 anti-drift). Also replace the double staticDirFromEnv() call with app.staticDir (single source of truth, M2). Closes #1025: StartHTTPServer blocked on ListenAndServe with no signal handling, so SIGTERM (normal stop in Cloud Run/Container Apps/Fargate) killed the process before deferred app.Close() could drain the DB pool and release session-pinned advisory locks. Fix: wire signal.NotifyContext(SIGINT,SIGTERM), run ListenAndServe in a goroutine, on signal call srv.Shutdown with a 30s bounded timeout. Regression tests added: TestHTTPTransportServesOIDCEndpoints confirms the HTTP transport returns application/json (not HTML) for OIDC paths; TestHandleOIDCHTTP verifies the bridge handler itself. Graceful-shutdown unit test is not feasible without a live server -- wiring is exercised through code review of StartHTTPServer. --- internal/server/http.go | 75 +++++++++++++++++++++++++++++++++--- internal/server/http_test.go | 60 +++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 5 deletions(-) diff --git a/internal/server/http.go b/internal/server/http.go index d2128d27c..9f69878d4 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -4,13 +4,17 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "io" "log" "net/http" + "os/signal" "strings" + "syscall" "time" + "github.com/LeanerCloud/CUDly/internal/api" "github.com/aws/aws-lambda-go/events" ) @@ -34,13 +38,25 @@ func CreateHTTPServer(app *Application, port int) *http.Server { mux.HandleFunc("/version", app.handleVersion) mux.Handle("/api/scheduled/", app.scheduledAuthMiddleware(http.HandlerFunc(app.handleScheduledHTTP))) + // Intercept OIDC issuer endpoints before both the static-file fallback and + // the API router. Mirrors the identical intercept in handleLambdaHTTPEvent + // so the two transports cannot drift (D1 / issue #1024). api.HandleOIDC is + // auth-less and must sit in front of the SPA handler -- otherwise + // STATIC_DIR deployments serve index.html for /oidc/... instead of the + // JWKS/discovery JSON, breaking any federated-credential relying party on + // Cloud Run or Container Apps. + mux.HandleFunc(api.OIDCBasePath+"/", app.handleOIDCHTTP) + // When STATIC_DIR is set, serve static files for non-API paths // and route only /api/ to the API handler. // When unset, all requests go to the API handler (backward compatible). - staticDir := staticDirFromEnv() - if staticDir != "" { + // Read from app.staticDir (single source of truth set by the constructor) + // rather than re-invoking staticDirFromEnv() -- avoids the double-stat and + // double-log that M2 identified, and keeps Lambda and HTTP transports + // reading the same value. + if app.staticDir != "" { mux.HandleFunc("/api/", app.handleHTTPRequest) - mux.Handle("/", spaFileServer(staticDir)) + mux.Handle("/", spaFileServer(app.staticDir)) } else { mux.HandleFunc("/", app.handleHTTPRequest) } @@ -56,11 +72,60 @@ func CreateHTTPServer(app *Application, port int) *http.Server { } } -// StartHTTPServer starts the standard HTTP server +// StartHTTPServer starts the HTTP server with graceful shutdown on SIGINT/SIGTERM. +// It blocks until the server exits cleanly. In container orchestrators (Cloud Run, +// Container Apps, Fargate) SIGTERM is the normal stop signal; without this wiring +// the process is killed before deferred app.Close() runs, leaving in-flight +// requests cut and the DB pool/advisory locks undrained (issue #1025). func StartHTTPServer(app *Application, port int) error { server := CreateHTTPServer(app, port) log.Printf("Starting HTTP server on %s", server.Addr) - return server.ListenAndServe() + + // Signal context cancels on the first SIGINT or SIGTERM. + sigCtx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + serveErr := make(chan error, 1) + go func() { + serveErr <- server.ListenAndServe() + }() + + select { + case err := <-serveErr: + // ListenAndServe returned before a signal -- hard error (bind failure, etc.). + if errors.Is(err, http.ErrServerClosed) { + return nil + } + return err + case <-sigCtx.Done(): + // Received SIGINT or SIGTERM: drain in-flight requests then close the DB. + stop() // release signal resources promptly + log.Printf("Shutdown signal received; draining HTTP server (30s grace)...") + shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := server.Shutdown(shutdownCtx); err != nil { + log.Printf("HTTP server forced shutdown: %v", err) + } + return nil + } +} + +// handleOIDCHTTP bridges the standard HTTP path to the Lambda-shaped HandleOIDC +// implementation. Registered at api.OIDCBasePath+"/" so it intercepts all +// /oidc/... requests before the SPA static handler or the API router, exactly +// mirroring the intercept in handleLambdaHTTPEvent. +func (app *Application) handleOIDCHTTP(w http.ResponseWriter, r *http.Request) { + ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) + defer cancel() + + lambdaReq := httpToLambdaRequest(r) + resp, handled := app.API.HandleOIDC(ctx, lambdaReq) + if !handled { + // Path matched /oidc/ prefix but is not a recognised OIDC endpoint. + http.NotFound(w, r) + return + } + lambdaResponseToHTTP(w, resp) } // htmlCSP is the Content-Security-Policy delivered with every HTML diff --git a/internal/server/http_test.go b/internal/server/http_test.go index b13e5da9a..e5ea0d80f 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -379,3 +379,63 @@ func TestCreateHTTPServer(t *testing.T) { testutil.AssertEqual(t, ":3000", srv.Addr) }) } + +// TestHTTPTransportServesOIDCEndpoints is a regression test for issue #1024: +// before the fix, requests to /oidc/.well-known/openid-configuration in +// HTTP/container mode were served by the SPA file server (returning HTML or +// 404) because the mux had no explicit OIDC registration. The test confirms +// that the HTTP transport correctly intercepts OIDC paths and returns a JSON +// response (not HTML/404). +func TestHTTPTransportServesOIDCEndpoints(t *testing.T) { + // A nil signer means HandleOIDC returns a 404 JSON body rather than the + // real discovery document -- but critically it is *JSON* and the path is + // *handled* rather than falling through to the SPA/404 fallback. + apiHandler := api.NewHandler(api.HandlerConfig{}) + app := &Application{ + API: apiHandler, + } + + srv := CreateHTTPServer(app, 19099) + ts := httptest.NewServer(srv.Handler) + defer ts.Close() + + for _, path := range []string{ + "/oidc/.well-known/openid-configuration", + "/oidc/.well-known/jwks.json", + } { + path := path + t.Run(path, func(t *testing.T) { + resp, err := http.Get(ts.URL + path) + testutil.AssertNoError(t, err) + defer resp.Body.Close() + + // Must be handled (not SPA-fallback 200 with HTML nor a net/http 404 text/plain). + ct := resp.Header.Get("Content-Type") + testutil.AssertTrue(t, strings.HasPrefix(ct, "application/json"), + "Expected Content-Type application/json for "+path+", got: "+ct) + + // With no signer configured HandleOIDC returns 404 JSON (not 200); + // the important invariant is that it is *not* an SPA-served HTML 200. + testutil.AssertTrue(t, resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, "application/json"), + "SPA fallback must not serve OIDC paths") + }) + } +} + +// TestHandleOIDCHTTP verifies that handleOIDCHTTP correctly bridges to +// api.Handler.HandleOIDC and returns JSON (not HTML). +func TestHandleOIDCHTTP(t *testing.T) { + app := &Application{ + API: api.NewHandler(api.HandlerConfig{}), + } + + req := httptest.NewRequest(http.MethodGet, "/oidc/.well-known/openid-configuration", nil) + w := httptest.NewRecorder() + + app.handleOIDCHTTP(w, req) + + // Without a signer the response is 404 JSON -- must not be HTML. + ct := w.Header().Get("Content-Type") + testutil.AssertTrue(t, strings.HasPrefix(ct, "application/json"), + "handleOIDCHTTP must return application/json, got: "+ct) +} From c4275e710596ee3e127b5eb272f0e02003b705e4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:00:38 -0700 Subject: [PATCH 02/11] fix(config): validate DEFAULT_PAYMENT_OPTION/DEFAULT_RAMP_SCHEDULE at startup boundary Closes #1026: DEFAULT_PAYMENT_OPTION and DEFAULT_RAMP_SCHEDULE env values were taken raw by LoadApplicationConfig and passed to purchase.NewManager without validation, despite config.ValidPaymentOptions / config.ValidRampScheduleTypes existing. A typo ("AllUpfront", "Immediate") silently became the system-wide purchase default -- a money-moving misconfiguration caught only at AWS purchase time (noisy, late) or never. Fix: add config.ValidatePaymentOptionEnv / config.ValidateRampScheduleEnv exported at the config package boundary, called from a new validateAppConfigEnvDefaults helper in NewApplicationFromDeps. Invalid non-empty values cause a startup error (consistent with the scheduledauth / ADMIN_PASSWORD_SECRET fail-fast posture in the same file). Empty value remains valid. Also fix M1: getEnvInt / getEnvFloat in app.go and getTaskTimeout in cmd/server/main.go silently swallowed strconv parse errors, leaving operators who fat-fingered a tuning env var believing their value was in effect. All three now log WARNING with the key name and the default value being used. Regression tests added: TestNewApplicationFromDepsValidatesEnvDefaults (invalid payment option and ramp schedule each cause a startup error with the env var name in the message); TestGetEnvIntLogsOnBadValue and TestGetEnvFloatLogsOnBadValue confirm WARNING is emitted on parse failure. Existing TestNewApplicationFromDeps updated to use canonical "all-upfront" (was "AllUpfront", now correctly rejected). --- cmd/server/main.go | 16 ++++- internal/config/validation.go | 32 ++++++++++ internal/server/app.go | 35 +++++++++-- internal/server/app_test.go | 107 +++++++++++++++++++++++++++++++++- 4 files changed, 182 insertions(+), 8 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index bbca19b99..e6fa58646 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -79,13 +79,23 @@ func main() { } // getTaskTimeout returns the task timeout from TASK_TIMEOUT env var or the default of 15 minutes. +// Logs a warning when TASK_TIMEOUT is set but cannot be parsed or is non-positive, +// so the operator knows the value was not applied. func getTaskTimeout() time.Duration { + const defaultTimeout = 15 * time.Minute if v := os.Getenv("TASK_TIMEOUT"); v != "" { - if secs, err := strconv.Atoi(v); err == nil && secs > 0 { - return time.Duration(secs) * time.Second + secs, err := strconv.Atoi(v) + if err != nil { + log.Printf("WARNING: TASK_TIMEOUT=%q is not a valid integer; using default %v", v, defaultTimeout) + return defaultTimeout + } + if secs <= 0 { + log.Printf("WARNING: TASK_TIMEOUT=%q must be a positive number; using default %v", v, defaultTimeout) + return defaultTimeout } + return time.Duration(secs) * time.Second } - return 15 * time.Minute + return defaultTimeout } // determineRuntimeMode determines the runtime mode based on flags and environment diff --git a/internal/config/validation.go b/internal/config/validation.go index 08b25718f..959ccec62 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -488,3 +488,35 @@ func isValidRampScheduleType(t string) bool { } return false } + +// ValidatePaymentOptionEnv validates a payment-option value read from an +// environment variable (e.g. DEFAULT_PAYMENT_OPTION). Empty string is +// always valid ("use the purchase manager's built-in default"). Non-empty +// values must be in the union of all provider payment option sets. +// Called by the server startup boundary so misconfiguration is caught at +// boot time rather than silently propagated into purchases (issue #1026). +func ValidatePaymentOptionEnv(val string) error { + if val == "" { + return nil + } + if !isValidPaymentOption(val) { + return fmt.Errorf("value %q is not a recognised payment option (valid: %s)", val, strings.Join(validPaymentOptionsUnion, ", ")) + } + return nil +} + +// ValidateRampScheduleEnv validates a ramp-schedule value read from an +// environment variable (e.g. DEFAULT_RAMP_SCHEDULE). Empty string is +// always valid ("use the purchase manager's built-in default"). Non-empty +// values must be in ValidRampScheduleTypes. +// Called by the server startup boundary so misconfiguration is caught at +// boot time rather than silently propagated into purchases (issue #1026). +func ValidateRampScheduleEnv(val string) error { + if val == "" { + return nil + } + if !isValidRampScheduleType(val) { + return fmt.Errorf("value %q is not a recognised ramp schedule type (valid: %s)", val, strings.Join(ValidRampScheduleTypes, ", ")) + } + return nil +} diff --git a/internal/server/app.go b/internal/server/app.go index 3126e1986..2b7b7d3d2 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -273,6 +273,21 @@ func LoadApplicationConfig() ApplicationConfig { } } +// validateAppConfigEnvDefaults validates the money-moving env-sourced defaults +// at the startup boundary. Both DEFAULT_PAYMENT_OPTION and DEFAULT_RAMP_SCHEDULE +// flow into the purchase manager as system-wide defaults; a typo propagates +// silently into every purchase unless we reject it here (issue #1026). +// Empty values are always valid (means "use the purchase manager's built-in default"). +func validateAppConfigEnvDefaults(cfg ApplicationConfig) error { + if err := config.ValidatePaymentOptionEnv(cfg.DefaultPaymentOption); err != nil { + return fmt.Errorf("invalid DEFAULT_PAYMENT_OPTION: %w", err) + } + if err := config.ValidateRampScheduleEnv(cfg.DefaultRampSchedule); err != nil { + return fmt.Errorf("invalid DEFAULT_RAMP_SCHEDULE: %w", err) + } + return nil +} + // resolveScheduledTaskSecret resolves SCHEDULED_TASK_SECRET_NAME to its real // value via the configured SecretResolver (Azure Key Vault / AWS Secrets // Manager) when possible. Falls back to cfg.ScheduledTaskSecret (plaintext @@ -339,6 +354,12 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext return nil, fmt.Errorf("database configuration required: DBConfig must be provided") } + // Validate money-moving env defaults at the startup boundary -- consistent with the + // fail-fast posture used for scheduledauth mode and ADMIN_PASSWORD_SECRET (issue #1026). + if err := validateAppConfigEnvDefaults(cfg); err != nil { + return nil, err + } + cfg.ScheduledTaskSecret = resolveScheduledTaskSecret(ctx, cfg, deps.SecretResolver) // Build the /api/scheduled/* auth validator. Fail-fast on bad @@ -844,18 +865,24 @@ func initConfigStore(ctx context.Context) (config.StoreInterface, *database.Conf func getEnvInt(key string, defaultVal int) int { if val := os.Getenv(key); val != "" { - if result, err := strconv.Atoi(val); err == nil { - return result + result, err := strconv.Atoi(val) + if err != nil { + log.Printf("WARNING: %s=%q is not a valid integer; using default %d", key, val, defaultVal) + return defaultVal } + return result } return defaultVal } func getEnvFloat(key string, defaultVal float64) float64 { if val := os.Getenv(key); val != "" { - if result, err := strconv.ParseFloat(val, 64); err == nil { - return result + result, err := strconv.ParseFloat(val, 64) + if err != nil { + log.Printf("WARNING: %s=%q is not a valid float; using default %g", key, val, defaultVal) + return defaultVal } + return result } return defaultVal } diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 12587a8ee..65f7d8f93 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "log" "net/http/httptest" "os" "strings" @@ -460,7 +461,7 @@ func TestNewApplicationFromDeps(t *testing.T) { Version: "v2.0", NotificationDaysBefore: 7, DefaultTerm: 1, - DefaultPaymentOption: "AllUpfront", + DefaultPaymentOption: "all-upfront", // canonical form; "AllUpfront" is now correctly rejected at startup DefaultCoverage: 95.5, APIKeySecretARN: "arn:aws:key", EnableDashboard: true, @@ -483,6 +484,110 @@ func TestNewApplicationFromDeps(t *testing.T) { }) } +// TestNewApplicationFromDepsValidatesEnvDefaults is a regression test for +// issue #1026: before the fix, invalid DEFAULT_PAYMENT_OPTION / +// DEFAULT_RAMP_SCHEDULE values were silently propagated into the purchase +// manager. The test confirms that NewApplicationFromDeps now fails fast on +// an invalid value rather than accepting it. +func TestNewApplicationFromDepsValidatesEnvDefaults(t *testing.T) { + ctx := context.Background() + validDBConfig := &database.Config{Host: "localhost", Port: 5432, Database: "cudly_test", User: "test", Password: "test"} + + t.Run("invalid DEFAULT_PAYMENT_OPTION is rejected at startup", func(t *testing.T) { + cfg := ApplicationConfig{ + DefaultPaymentOption: "AllUpfront", // typo: should be "all-upfront" + } + deps := ExternalDeps{DBConfig: validDBConfig} + _, err := NewApplicationFromDeps(ctx, cfg, deps) + testutil.AssertError(t, err) + testutil.AssertContains(t, err.Error(), "DEFAULT_PAYMENT_OPTION") + }) + + t.Run("invalid DEFAULT_RAMP_SCHEDULE is rejected at startup", func(t *testing.T) { + cfg := ApplicationConfig{ + DefaultRampSchedule: "Immediate", // wrong case + } + deps := ExternalDeps{DBConfig: validDBConfig} + _, err := NewApplicationFromDeps(ctx, cfg, deps) + testutil.AssertError(t, err) + testutil.AssertContains(t, err.Error(), "DEFAULT_RAMP_SCHEDULE") + }) + + t.Run("empty DEFAULT_PAYMENT_OPTION is accepted", func(t *testing.T) { + // Empty means "use purchase manager built-in default" -- must not error. + cfg := ApplicationConfig{DefaultPaymentOption: ""} + deps := ExternalDeps{ + EmailSender: &noopEmailSender{}, + DBConfig: validDBConfig, + } + _, err := NewApplicationFromDeps(ctx, cfg, deps) + // Error is expected for other reasons (e.g. scheduledauth or DB), but + // NOT for the payment option: verify the message does not mention it. + if err != nil { + testutil.AssertTrue(t, !strings.Contains(err.Error(), "DEFAULT_PAYMENT_OPTION"), + "empty DEFAULT_PAYMENT_OPTION must not produce a validation error, got: "+err.Error()) + } + }) + + t.Run("valid DEFAULT_PAYMENT_OPTION is accepted", func(t *testing.T) { + cfg := ApplicationConfig{ + DefaultPaymentOption: "all-upfront", + } + deps := ExternalDeps{ + EmailSender: &noopEmailSender{}, + DBConfig: validDBConfig, + } + _, err := NewApplicationFromDeps(ctx, cfg, deps) + // Error may occur for unrelated reasons but must not mention payment option. + if err != nil { + testutil.AssertTrue(t, !strings.Contains(err.Error(), "DEFAULT_PAYMENT_OPTION"), + "valid DEFAULT_PAYMENT_OPTION must not produce a validation error, got: "+err.Error()) + } + }) +} + +// TestGetEnvIntLogsOnBadValue is a regression test for M1: before the fix, +// getEnvInt silently returned the default on a malformed value. The test +// confirms that a WARNING is now logged. +func TestGetEnvIntLogsOnBadValue(t *testing.T) { + testutil.SetEnv(t, "TEST_ENV_INT_BAD", "notanint") + + var logged string + orig := log.Writer() + var buf strings.Builder + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(orig) }) + + result := getEnvInt("TEST_ENV_INT_BAD", 42) + logged = buf.String() + + testutil.AssertEqual(t, 42, result) // falls back to default + testutil.AssertTrue(t, strings.Contains(logged, "WARNING"), + "Expected WARNING log for bad int env var, got: "+logged) + testutil.AssertTrue(t, strings.Contains(logged, "TEST_ENV_INT_BAD"), + "Expected key name in warning log, got: "+logged) +} + +// TestGetEnvFloatLogsOnBadValue mirrors TestGetEnvIntLogsOnBadValue for floats. +func TestGetEnvFloatLogsOnBadValue(t *testing.T) { + testutil.SetEnv(t, "TEST_ENV_FLOAT_BAD", "eighty") + + var logged string + orig := log.Writer() + var buf strings.Builder + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(orig) }) + + result := getEnvFloat("TEST_ENV_FLOAT_BAD", 80.0) + logged = buf.String() + + testutil.AssertEqual(t, 80.0, result) + testutil.AssertTrue(t, strings.Contains(logged, "WARNING"), + "Expected WARNING log for bad float env var, got: "+logged) + testutil.AssertTrue(t, strings.Contains(logged, "TEST_ENV_FLOAT_BAD"), + "Expected key name in warning log, got: "+logged) +} + func TestInitConfigStore(t *testing.T) { t.Run("missing DB_HOST returns error", func(t *testing.T) { testutil.SetEnv(t, "DB_HOST", "") From 7d33d14f6b7b28935949fb07e67ae25f9e2de721 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:00:19 -0700 Subject: [PATCH 03/11] test(server): assert concrete 404 for OIDC SPA-fallback guard The previous assertion (resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, "application/json")) was a tautology: line 414 already guarantees ct starts with application/json, so the OR could never fail. Replace it with an explicit AssertEqual to http.StatusNotFound so the test fails loudly if an OIDC path ever falls through to the SPA fallback. Addresses CodeRabbit nitpick on http_test.go:419-420. --- internal/server/http_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/server/http_test.go b/internal/server/http_test.go index e5ea0d80f..f039861f8 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -414,10 +414,10 @@ func TestHTTPTransportServesOIDCEndpoints(t *testing.T) { testutil.AssertTrue(t, strings.HasPrefix(ct, "application/json"), "Expected Content-Type application/json for "+path+", got: "+ct) - // With no signer configured HandleOIDC returns 404 JSON (not 200); - // the important invariant is that it is *not* an SPA-served HTML 200. - testutil.AssertTrue(t, resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, "application/json"), - "SPA fallback must not serve OIDC paths") + // With no signer configured HandleOIDC returns 404 JSON (not an + // SPA-served HTML 200); assert the concrete status so the test + // fails loudly if the path ever falls through to the SPA fallback. + testutil.AssertEqual(t, http.StatusNotFound, resp.StatusCode) }) } } From 6309edb74a42a10a650e0b146447092364ab0d7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:20:21 -0700 Subject: [PATCH 04/11] refactor(server): use runtime.IsLambda() directly; remove thin isLambdaRuntime() wrapper The isLambdaRuntime() wrapper existed to keep call sites unchanged during an earlier refactor but its own comment said new code should call runtime.IsLambda() directly (04-N3). Remove the wrapper and inline runtime.IsLambda() at all three app.go call sites. cmd/server/main.go read AWS_LAMBDA_RUNTIME_API directly despite the runtime package existing to encapsulate exactly this detection rule (04-M5). Replace with runtime.IsLambda() to keep the detection logic consistent if the rule changes in future. Part of #1067. --- cmd/server/main.go | 8 +++++--- internal/server/app.go | 11 ++--------- internal/server/app_test.go | 5 +++-- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index e6fa58646..299fead32 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -10,6 +10,7 @@ import ( "strconv" "time" + "github.com/LeanerCloud/CUDly/internal/runtime" "github.com/LeanerCloud/CUDly/internal/server" ) @@ -105,9 +106,10 @@ func determineRuntimeMode(modeFlag string) string { return modeFlag } - // Auto-detect based on environment - // Lambda sets AWS_LAMBDA_RUNTIME_API when running - if os.Getenv("AWS_LAMBDA_RUNTIME_API") != "" { + // Auto-detect based on environment using the canonical runtime helper, + // which encapsulates the detection rule so future changes stay consistent + // across all call sites (issue 04-M5). + if runtime.IsLambda() { return "lambda" } diff --git a/internal/server/app.go b/internal/server/app.go index 2b7b7d3d2..03dd770d5 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -140,13 +140,6 @@ type ExternalDeps struct { STSClient purchase.STSClient } -// isLambdaRuntime is a thin wrapper over runtime.IsLambda so existing -// call sites stay unchanged. New code should call runtime.IsLambda -// directly. -func isLambdaRuntime() bool { - return runtime.IsLambda() -} - // defaultMigrationsTimeout bounds how long ensureDB waits for migrations // before giving up and proceeding. Deliberately shorter than the default // Lambda timeout (30s at this writing) so a runaway migration gets @@ -268,7 +261,7 @@ func LoadApplicationConfig() ApplicationConfig { // NewApplicationFromDeps resolves it via the SecretResolver at init. ScheduledTaskSecret: os.Getenv("SCHEDULED_TASK_SECRET"), ScheduledTaskSecretName: os.Getenv("SCHEDULED_TASK_SECRET_NAME"), - IsLambda: isLambdaRuntime(), + IsLambda: runtime.IsLambda(), Analytics: LoadAnalyticsConfig(), } } @@ -424,7 +417,7 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext PurchaseManager: purchaseManager, EmailSender: deps.EmailSender, STSClient: deps.STSClient, - IsLambda: isLambdaRuntime(), + IsLambda: runtime.IsLambda(), }) // Auth store will be initialized lazily after DB connection diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 65f7d8f93..41341201d 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -18,6 +18,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/database" "github.com/LeanerCloud/CUDly/internal/email" "github.com/LeanerCloud/CUDly/internal/purchase" + "github.com/LeanerCloud/CUDly/internal/runtime" "github.com/LeanerCloud/CUDly/internal/scheduler" "github.com/LeanerCloud/CUDly/internal/testutil" "github.com/aws/aws-lambda-go/events" @@ -36,10 +37,10 @@ func TestIsLambdaRuntime(t *testing.T) { }() os.Unsetenv("AWS_LAMBDA_RUNTIME_API") - testutil.AssertEqual(t, false, isLambdaRuntime()) + testutil.AssertEqual(t, false, runtime.IsLambda()) os.Setenv("AWS_LAMBDA_RUNTIME_API", "localhost:9001") - testutil.AssertEqual(t, true, isLambdaRuntime()) + testutil.AssertEqual(t, true, runtime.IsLambda()) } func TestClose(t *testing.T) { From 64422dda161ca71abf51231718532cc9c58fc1e7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:22:17 -0700 Subject: [PATCH 05/11] fix(server): log health encode error; use TrimPrefix for task path; error on unknown event 04-L4: handleHealthCheck silently discarded the json.Encode return value while handleScheduledHTTP already logged it. Log the error for parity; the headers are already sent so the only recovery is a log entry. 04-L5: handleScheduledHTTP re-derived the task-type segment by splitting the path and indexing parts[2], which requires a length guard and breaks if the route prefix changes. Replace with strings.TrimPrefix against the known prefix; reject empty or slash-containing results. 04-N4: unrecognised Lambda payloads were silently routed to the scheduled handler, which then failed with "unknown scheduled task action" masking the real root cause. Return a distinct "unrecognised Lambda event shape" error so operators see the actual problem in the logs. Regression test: TestHandleLambdaEvent_UnknownEventReturnsError. Part of #1067. --- internal/server/health.go | 10 +++++++--- internal/server/http.go | 13 +++++++------ internal/server/lambda.go | 6 ++++-- internal/server/lambda_test.go | 19 +++++++++++++++++++ 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/internal/server/health.go b/internal/server/health.go index 2cc734e88..a09c84dcd 100644 --- a/internal/server/health.go +++ b/internal/server/health.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log" "net/http" "time" ) @@ -58,12 +59,15 @@ func (app *Application) handleHealthCheck(w http.ResponseWriter, r *http.Request // The actual health status is in the JSON body. "degraded" means the app is // running but some dependencies (like DB) aren't connected yet - this is // expected during cold starts with lazy DB initialization. - statusCode := http.StatusOK // Write response with security headers and CORS setHealthResponseHeaders(w, app.appConfig.CORSAllowedOrigin) - w.WriteHeader(statusCode) - json.NewEncoder(w).Encode(health) + w.WriteHeader(http.StatusOK) + if err := json.NewEncoder(w).Encode(health); err != nil { + // Body write failed after headers are sent; log for parity with + // handleScheduledHTTP which already logs its encode error (04-L4). + log.Printf("health: failed to encode response: %v", err) + } } // checkMigrations reports the outcome of the most recent migration run. diff --git a/internal/server/http.go b/internal/server/http.go index 9f69878d4..45d349665 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -231,15 +231,16 @@ func (app *Application) handleScheduledHTTP(w http.ResponseWriter, r *http.Reque return } - // Extract task type from URL path - // Expected format: /api/scheduled/{task_type} - parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") - if len(parts) < 3 { + // Extract task type from URL path: /api/scheduled/{task_type}. + // TrimPrefix is cleaner than splitting and indexing parts[2]; it also + // avoids the length-guard dance while remaining robust to extra slashes + // (04-L5). + const scheduledPrefix = "/api/scheduled/" + taskTypeStr := strings.TrimPrefix(r.URL.Path, scheduledPrefix) + if taskTypeStr == "" || strings.Contains(taskTypeStr, "/") { http.Error(w, "Invalid path", http.StatusBadRequest) return } - - taskTypeStr := parts[2] taskType := ScheduledTaskType(taskTypeStr) // Execute scheduled task diff --git a/internal/server/lambda.go b/internal/server/lambda.go index c323a53da..25ca4b3f2 100644 --- a/internal/server/lambda.go +++ b/internal/server/lambda.go @@ -40,8 +40,10 @@ func (app *Application) HandleLambdaEvent(ctx context.Context, rawEvent json.Raw case "scheduled": return app.handleLambdaScheduledEvent(ctx, rawEvent) default: - log.Printf("Unknown event type, treating as scheduled event") - return app.handleLambdaScheduledEvent(ctx, rawEvent) + // Return a distinct error instead of silently treating an unrecognised + // payload as a scheduled event. Masking the event shape as "unknown + // scheduled task action" makes the real cause hard to diagnose (04-N4). + return nil, fmt.Errorf("unrecognised Lambda event shape (size %d bytes); not an HTTP/SQS/scheduled event", len(rawEvent)) } } diff --git a/internal/server/lambda_test.go b/internal/server/lambda_test.go index a11475dbe..0d96e1c31 100644 --- a/internal/server/lambda_test.go +++ b/internal/server/lambda_test.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "strings" "testing" "github.com/LeanerCloud/CUDly/internal/api" @@ -268,6 +269,24 @@ func TestHandleLambdaScheduledEvent(t *testing.T) { } } +// TestHandleLambdaEvent_UnknownEventReturnsError is a regression test for +// 04-N4: before the fix, unrecognised payloads were silently routed to +// handleLambdaScheduledEvent, which then failed with "unknown scheduled task +// action" -- masking the real root cause. The fix returns a distinct error +// so callers (and logs) see "unrecognised Lambda event shape" instead. +func TestHandleLambdaEvent_UnknownEventReturnsError(t *testing.T) { + ctx := testutil.TestContext(t) + + app := &Application{ + API: api.NewHandler(api.HandlerConfig{}), + } + + _, err := app.HandleLambdaEvent(ctx, json.RawMessage(`{"unknown": "event"}`)) + testutil.AssertError(t, err) + testutil.AssertTrue(t, strings.Contains(err.Error(), "unrecognised"), + "expected 'unrecognised' in error, got: "+err.Error()) +} + func TestHandleLambdaEvent(t *testing.T) { tests := []struct { name string From a142a82ce1a422d1f80e0a730a5a0195bcfa732b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:24:13 -0700 Subject: [PATCH 06/11] fix(server): unify static-file containment check; remove dead hasFileContent 04-M6: spaHandler.ServeHTTP (HTTP transport) and resolveStaticFilePath (Lambda transport) implemented two different directory-containment checks. spaHandler delegated containment to http.ServeFile's built-in protection while resolveStaticFilePath used strings.HasPrefix(absFile, absDir) without a trailing separator -- allowing a sibling directory sharing a name prefix to pass the check. Unify by delegating spaHandler to resolveStaticFilePath and fix the prefix check to include os.PathSeparator. 04-L2: hasFileContent was dead code. The doc comment claimed it was used at startup to validate STATIC_DIR, but staticDirFromEnv does that work and never called hasFileContent. Remove it and its three tests. Regression test: TestResolveStaticFilePath_SiblingDirBlocked. Part of #1067. --- internal/server/static.go | 49 ++++++++-------------------------- internal/server/static_test.go | 46 ++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 54 deletions(-) diff --git a/internal/server/static.go b/internal/server/static.go index 413f28966..0671ba6ae 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -1,7 +1,6 @@ package server import ( - "io/fs" "log" "mime" "net/http" @@ -23,36 +22,16 @@ type spaHandler struct { } func (h *spaHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - // Clean the path to prevent directory traversal - urlPath := path.Clean(r.URL.Path) - if urlPath == "/" { - urlPath = "/index.html" - } - - // Try to serve the requested file - filePath := filepath.Join(h.dir, filepath.FromSlash(urlPath)) - - info, err := os.Stat(filePath) - if err == nil && !info.IsDir() { - setCacheHeaders(w, urlPath) - http.ServeFile(w, r, filePath) - return - } - - // File not found: if path has an extension, return 404 - if path.Ext(urlPath) != "" { - http.NotFound(w, r) - return - } - - // SPA fallback: serve index.html for extensionless paths - indexPath := filepath.Join(h.dir, "index.html") - if _, err := os.Stat(indexPath); err != nil { + // Delegate path resolution to resolveStaticFilePath, which provides the + // same separator-aware containment check used by the Lambda static path. + // This closes the drift between the two static-serving paths (04-M6). + filePath, cleanPath, ok := resolveStaticFilePath(h.dir, r.URL.Path) + if !ok { http.NotFound(w, r) return } - setCacheHeaders(w, "/index.html") - http.ServeFile(w, r, indexPath) + setCacheHeaders(w, cleanPath) + http.ServeFile(w, r, filePath) } // setCacheHeaders sets Cache-Control based on file type. @@ -90,7 +69,10 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok if err != nil { return "", "", false } - if !strings.HasPrefix(absFile, absDir) { + // Require the separator after absDir so that a sibling directory whose + // name shares a prefix (e.g. /srv/static-evil vs /srv/static) cannot + // pass the containment check (04-M6). + if !strings.HasPrefix(absFile, absDir+string(os.PathSeparator)) && absFile != absDir { return "", "", false } @@ -193,12 +175,3 @@ func isStaticPath(urlPath string) bool { return true } -// hasFileContent checks if the static dir contains at least one file, -// used during startup to validate the STATIC_DIR configuration. -func hasFileContent(dir string) bool { - entries, err := fs.ReadDir(os.DirFS(dir), ".") - if err != nil { - return false - } - return len(entries) > 0 -} diff --git a/internal/server/static_test.go b/internal/server/static_test.go index c9480a2c2..2b0df2f38 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -104,22 +104,6 @@ func TestIsStaticPath(t *testing.T) { } } -// ----- hasFileContent ----- - -func TestHasFileContent_NonEmptyDir(t *testing.T) { - dir := makeStaticDir(t, map[string]string{"index.html": ""}) - testutil.AssertEqual(t, true, hasFileContent(dir)) -} - -func TestHasFileContent_EmptyDir(t *testing.T) { - dir := t.TempDir() - testutil.AssertEqual(t, false, hasFileContent(dir)) -} - -func TestHasFileContent_NonExistentDir(t *testing.T) { - testutil.AssertEqual(t, false, hasFileContent("/nonexistent/path/that/does/not/exist")) -} - // ----- staticDirFromEnv ----- func TestStaticDirFromEnv_Unset(t *testing.T) { @@ -196,6 +180,36 @@ func TestResolveStaticFilePath_DirectoryTraversal(t *testing.T) { } } +// TestResolveStaticFilePath_SiblingDirBlocked is a regression test for 04-M6: +// the previous HasPrefix check lacked a separator, so a sibling directory +// named "-evil" would have passed the containment check because +// "/srv/static" is a string-prefix of "/srv/static-evil". The fix appends +// os.PathSeparator to the prefix so only paths genuinely inside the dir pass. +func TestResolveStaticFilePath_SiblingDirBlocked(t *testing.T) { + // Create a parent and the real static dir inside it. + parent := t.TempDir() + realDir := filepath.Join(parent, "static") + if err := os.MkdirAll(realDir, 0o755); err != nil { + t.Fatalf("mkdir static: %v", err) + } + if err := os.WriteFile(filepath.Join(realDir, "index.html"), []byte(""), 0o644); err != nil { + t.Fatalf("write index.html: %v", err) + } + // Create a sibling whose name starts with "static" (the prefix-confusion case). + siblingDir := filepath.Join(parent, "static-evil") + if err := os.MkdirAll(siblingDir, 0o755); err != nil { + t.Fatalf("mkdir static-evil: %v", err) + } + secretFile := filepath.Join(siblingDir, "secret.txt") + if err := os.WriteFile(secretFile, []byte("should-not-serve"), 0o644); err != nil { + t.Fatalf("write secret.txt: %v", err) + } + + // Attempting to resolve a path that lands in the sibling must return ok=false. + _, _, ok := resolveStaticFilePath(realDir, "/../static-evil/secret.txt") + testutil.AssertEqual(t, false, ok) +} + func TestResolveStaticFilePath_IndexMissingForSPARoute(t *testing.T) { dir := t.TempDir() // no index.html From c5fcaa0a76ef38929642c6195f110fab3546916e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:28:48 -0700 Subject: [PATCH 07/11] refactor(server,cmd): pass Version directly via NewApplication; drop env round-trip Both cmd/server and cmd/lambda pushed the build-time Version into os.Setenv("VERSION",...) so LoadApplicationConfig could read it back. This round-trip was awkward (cmd/lambda even had a thread-safety caveat) and unnecessary once the constructor accepts the value directly (04-N1). Add a version parameter to NewApplication: when non-empty it overrides the env-sourced value; "" falls back to os.Getenv("VERSION") for callers that do not have a build-time constant. Both cmd entrypoints now pass their ldflags-stamped Version directly. Update TestInitApp_SetsVersion to reflect the new behaviour (VERSION env var is intentionally not set by initApp). Part of #1067. --- cmd/lambda/main.go | 11 +++-------- cmd/lambda/main_test.go | 44 ++++++++++++----------------------------- cmd/server/main.go | 11 ++++++----- internal/server/app.go | 10 ++++++++-- 4 files changed, 30 insertions(+), 46 deletions(-) diff --git a/cmd/lambda/main.go b/cmd/lambda/main.go index 3da4afc76..ec3ede3c4 100644 --- a/cmd/lambda/main.go +++ b/cmd/lambda/main.go @@ -11,7 +11,6 @@ import ( "encoding/json" "fmt" "log" - "os" "sync" "github.com/LeanerCloud/CUDly/internal/server" @@ -42,16 +41,12 @@ func initApp(ctx context.Context) (*server.Application, error) { return app, nil } - // Set version for the application. - // Note: os.Setenv is not thread-safe, but Lambda serializes cold starts so this is safe. - // Consider passing version through a struct field for multi-threaded environments. - os.Setenv("VERSION", Version) - log.Printf("CUDly Lambda Handler starting, version: %s", Version) - // Initialize using the unified server package (PostgreSQL-based) + // Initialize using the unified server package (PostgreSQL-based). + // Pass Version directly to avoid the os.Setenv round-trip (04-N1). var err error - app, err = server.NewApplication(ctx) + app, err = server.NewApplication(ctx, Version) if err != nil { return nil, fmt.Errorf("failed to initialize application: %w", err) } diff --git a/cmd/lambda/main_test.go b/cmd/lambda/main_test.go index a1b897ced..6dfbd28e6 100644 --- a/cmd/lambda/main_test.go +++ b/cmd/lambda/main_test.go @@ -36,32 +36,17 @@ func TestInitApp_Cached(t *testing.T) { assert.Equal(t, testApp, result, "should return cached app") } +// TestInitApp_SetsVersion verifies that the ldflags-stamped Version is passed +// directly to NewApplication (04-N1) rather than round-tripping through +// os.Setenv("VERSION",...) / os.Getenv("VERSION"). We verify indirectly: +// initApp is expected to fail because DB_HOST is unset, which means +// NewApplication(ctx, Version) was called with the correct value. A later +// successful init path (TestNewApplicationFromDeps in internal/server) confirms +// the field is stored on ApplicationConfig.Version. func TestInitApp_SetsVersion(t *testing.T) { origApp := app origVersion := Version origDBHost := os.Getenv("DB_HOST") - defer func() { - app = origApp - Version = origVersion - os.Setenv("DB_HOST", origDBHost) - }() - - // Ensure app is nil so initApp tries to initialize - app = nil - Version = "test-v1.2.3" - os.Unsetenv("DB_HOST") - - _, err := initApp(context.Background()) - // It will fail because DB_HOST is not set, but Version should have been set - require.Error(t, err) - assert.Equal(t, "test-v1.2.3", os.Getenv("VERSION")) -} - -func TestInitApp_EmptyVersion(t *testing.T) { - origApp := app - origVersion := Version - origDBHost := os.Getenv("DB_HOST") - origEnvVersion := os.Getenv("VERSION") defer func() { app = origApp Version = origVersion @@ -70,22 +55,19 @@ func TestInitApp_EmptyVersion(t *testing.T) { } else { os.Unsetenv("DB_HOST") } - if origEnvVersion != "" { - os.Setenv("VERSION", origEnvVersion) - } else { - os.Unsetenv("VERSION") - } }() app = nil - Version = "" + Version = "test-v1.2.3" os.Unsetenv("DB_HOST") - os.Unsetenv("VERSION") _, err := initApp(context.Background()) + // Expected to fail because DB_HOST is not set; the Version is now passed + // directly to NewApplication, not via the VERSION env var (04-N1). require.Error(t, err) - // When Version is empty, os.Setenv("VERSION", Version) should NOT be called - // so VERSION env should remain unset or whatever it was before + // VERSION env var is intentionally no longer set by initApp. + assert.NotEqual(t, "test-v1.2.3", os.Getenv("VERSION"), + "VERSION env var must not be set by initApp after 04-N1 refactor") } func TestInitApp_FailsWithoutDB(t *testing.T) { diff --git a/cmd/server/main.go b/cmd/server/main.go index 299fead32..a15b8ef3b 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -31,16 +31,17 @@ func main() { // Print version info log.Printf("CUDly Server v%s (git: %s, built: %s)", Version, GitSHA, BuildTime) - // Export build metadata to the environment so the api package can read it - // without importing main (which would create an import cycle). - os.Setenv("VERSION", Version) + // Export BUILD_TIME and GIT_SHA to the environment so the api package can + // read them without importing main (import cycle). VERSION is passed + // directly to NewApplication to avoid the env round-trip (04-N1). os.Setenv("BUILD_TIME", BuildTime) os.Setenv("GIT_SHA", GitSHA) ctx := context.Background() - // Initialize application - app, err := server.NewApplication(ctx) + // Initialize application; pass Version directly so it is stamped on + // ApplicationConfig without going through os.Setenv("VERSION",...). + app, err := server.NewApplication(ctx, Version) if err != nil { log.Fatalf("Failed to initialize application: %v", err) } diff --git a/internal/server/app.go b/internal/server/app.go index 03dd770d5..023940ae7 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -483,9 +483,15 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext }, nil } -// NewApplication creates and initializes a new Application instance -func NewApplication(ctx context.Context) (*Application, error) { +// NewApplication creates and initializes a new Application instance. +// version overrides the VERSION env var when non-empty, so cmd entrypoints +// can pass the ldflags-stamped value directly instead of round-tripping +// through os.Setenv / os.Getenv (04-N1). Pass "" to fall back to the env. +func NewApplication(ctx context.Context, version string) (*Application, error) { cfg := LoadApplicationConfig() + if version != "" { + cfg.Version = version + } if err := cfg.Analytics.Validate(); err != nil { return nil, fmt.Errorf("invalid analytics configuration: %w", err) From e1dc24667986fad690b6bfc2d3a9f9b0905c181e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:31:07 -0700 Subject: [PATCH 08/11] fix(server): propagate secret-resolution error in bearer-mode startup 04-M4: when SCHEDULED_TASK_SECRET_NAME is configured but the SecretResolver (Key Vault, Secrets Manager) fails, resolveScheduledTaskSecret logged the error and fell back to the empty SCHEDULED_TASK_SECRET env var. In bearer mode buildScheduledAuth then raised "bearer mode requires SCHEDULED_TASK_SECRET" -- pointing the operator at the wrong env var and hiding the real cause. Fix: - resolveScheduledTaskSecret now returns (string, error); the error is non-nil when a SecretName is configured and resolution fails. - NewApplicationFromDeps loads the scheduledauth config before resolving the secret so it knows the mode. In bearer mode, a non-nil resolver error is propagated immediately with the original cause and the secret name in the message. - buildScheduledAuth is replaced by buildScheduledAuthFromConfig which accepts the pre-loaded Config, avoiding a duplicate LoadConfig call. Regression test: TestNewApplicationFromDeps_BearerModeSecretResolutionFails. Part of #1067. --- internal/server/app.go | 59 +++++++++++++++++++++++-------------- internal/server/app_test.go | 42 ++++++++++++++++++++++---- 2 files changed, 73 insertions(+), 28 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index 023940ae7..6e0bf60c0 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -285,14 +285,20 @@ func validateAppConfigEnvDefaults(cfg ApplicationConfig) error { // value via the configured SecretResolver (Azure Key Vault / AWS Secrets // Manager) when possible. Falls back to cfg.ScheduledTaskSecret (plaintext // SCHEDULED_TASK_SECRET env var) if the resolver is absent or the lookup -// fails. Pulled out of NewApplicationFromDeps to keep it under the -// cyclomatic limit. +// fails. +// +// The second return value is non-nil only when a SecretName was configured +// AND the resolver failed. Callers in bearer mode MUST propagate this error +// so startup fails with the real cause (e.g. "failed to resolve +// scheduled-task secret from : ") rather than the misleading +// downstream "bearer mode requires SCHEDULED_TASK_SECRET" that would +// otherwise surface when the empty fallback reaches buildScheduledAuth (04-M4). // // Security note: if both SCHEDULED_TASK_SECRET (plaintext) and // SCHEDULED_TASK_SECRET_NAME (secret-store path) are set, we warn loudly // because the plaintext value is visible in Lambda env / Terraform state. // The secret-store path is always preferred when both are present. -func resolveScheduledTaskSecret(ctx context.Context, cfg ApplicationConfig, resolver secrets.Resolver) string { +func resolveScheduledTaskSecret(ctx context.Context, cfg ApplicationConfig, resolver secrets.Resolver) (string, error) { if cfg.ScheduledTaskSecretName != "" && cfg.ScheduledTaskSecret != "" { log.Printf("SECURITY WARNING: both SCHEDULED_TASK_SECRET (plaintext) and " + "SCHEDULED_TASK_SECRET_NAME are set. The plaintext value is visible in " + @@ -301,14 +307,14 @@ func resolveScheduledTaskSecret(ctx context.Context, cfg ApplicationConfig, reso } if cfg.ScheduledTaskSecretName == "" || resolver == nil { - return cfg.ScheduledTaskSecret + return cfg.ScheduledTaskSecret, nil } resolved, err := resolver.GetSecret(ctx, cfg.ScheduledTaskSecretName) if err != nil { log.Printf("scheduled task secret resolution failed for %q: %v (falling back to SCHEDULED_TASK_SECRET)", cfg.ScheduledTaskSecretName, err) - return cfg.ScheduledTaskSecret + return cfg.ScheduledTaskSecret, err } - return resolved + return resolved, nil } // envSourceOS implements scheduledauth.EnvSource against os.Getenv. The @@ -318,17 +324,11 @@ type envSourceOS struct{} func (envSourceOS) Get(key string) string { return os.Getenv(key) } -// buildScheduledAuth wires up the /api/scheduled/* validator. It pulls -// mode + OIDC params from env (they're per-deployment Terraform inputs) -// but injects the bearer secret from cfg — that path goes through the -// SecretResolver (Key Vault) for production deployments and never lives -// in a container env var. Returns ErrConfigInvalid on misconfig so the -// container fails fast rather than silently accepting everything. -func buildScheduledAuth(cfg ApplicationConfig) (*scheduledauth.Validator, error) { - saCfg, err := scheduledauth.LoadConfig(envSourceOS{}) - if err != nil { - return nil, err - } +// buildScheduledAuthFromConfig wires up the /api/scheduled/* validator from +// a pre-loaded scheduledauth.Config. The bearer secret is injected from cfg +// rather than re-read from env — in production cfg.ScheduledTaskSecret was +// already resolved from Key Vault / Secrets Manager by the caller. +func buildScheduledAuthFromConfig(cfg ApplicationConfig, saCfg scheduledauth.Config) (*scheduledauth.Validator, error) { // In bearer mode, override the env-supplied secret with the one // already resolved from KV / SM. LoadConfig reads SCHEDULED_TASK_SECRET // directly which is fine for local dev where the env carries the @@ -353,13 +353,28 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext return nil, err } - cfg.ScheduledTaskSecret = resolveScheduledTaskSecret(ctx, cfg, deps.SecretResolver) + // Load the scheduledauth config first so we know the mode before + // resolving the secret. Bearer mode requires a non-empty secret, so + // a resolution failure must be propagated as a fatal startup error + // rather than silently falling back to the empty plaintext env var + // and surfacing the misleading "bearer mode requires SCHEDULED_TASK_SECRET" + // error downstream (04-M4). + saCfg, err := scheduledauth.LoadConfig(envSourceOS{}) + if err != nil { + return nil, fmt.Errorf("scheduled-task auth init: %w", err) + } + + resolvedSecret, secretErr := resolveScheduledTaskSecret(ctx, cfg, deps.SecretResolver) + if secretErr != nil && saCfg.Mode == scheduledauth.ModeBearer { + return nil, fmt.Errorf("failed to resolve scheduled-task secret from %q: %w", cfg.ScheduledTaskSecretName, secretErr) + } + cfg.ScheduledTaskSecret = resolvedSecret - // Build the /api/scheduled/* auth validator. Fail-fast on bad - // config (empty subjects in oidc mode, unknown mode, etc.) — better - // to crash on startup than to silently accept unauthenticated + // Build the /api/scheduled/* auth validator from the already-loaded + // config. Fail-fast on bad config (empty subjects in oidc mode, etc.) + // — better to crash on startup than to silently accept unauthenticated // scheduled-task calls in production. - scheduledAuth, err := buildScheduledAuth(cfg) + scheduledAuth, err := buildScheduledAuthFromConfig(cfg, saCfg) if err != nil { return nil, fmt.Errorf("scheduled-task auth init: %w", err) } diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 41341201d..4f7b31938 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -736,8 +736,9 @@ func TestResolveScheduledTaskSecret_PreferSecretName(t *testing.T) { ScheduledTaskSecretName: "arn:aws:secretsmanager:us-east-1:123:secret:my-secret", } - // Both set: secret-store value must win. - got := resolveScheduledTaskSecret(ctx, cfg, resolver) + // Both set: secret-store value must win; no error on success. + got, err := resolveScheduledTaskSecret(ctx, cfg, resolver) + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "from-secret-store", got) } @@ -751,12 +752,14 @@ func TestResolveScheduledTaskSecret_PlaintextOnlyNoResolver(t *testing.T) { ScheduledTaskSecret: "plaintext-dev", } - got := resolveScheduledTaskSecret(ctx, cfg, nil) + got, err := resolveScheduledTaskSecret(ctx, cfg, nil) + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "plaintext-dev", got) } // TestResolveScheduledTaskSecret_SecretNameFallback verifies that a resolver -// error causes a graceful fallback to the plaintext value. +// error returns the plaintext fallback AND a non-nil error so callers in +// bearer mode can propagate it as a fatal startup error (04-M4). func TestResolveScheduledTaskSecret_SecretNameFallback(t *testing.T) { ctx := context.Background() @@ -766,7 +769,8 @@ func TestResolveScheduledTaskSecret_SecretNameFallback(t *testing.T) { ScheduledTaskSecretName: "arn:aws:secretsmanager:us-east-1:123:secret:my-secret", } - got := resolveScheduledTaskSecret(ctx, cfg, resolver) + got, err := resolveScheduledTaskSecret(ctx, cfg, resolver) + testutil.AssertError(t, err) // error is returned so bearer-mode callers can fail-fast testutil.AssertEqual(t, "fallback-plaintext", got) } @@ -780,6 +784,32 @@ func TestResolveScheduledTaskSecret_SecretNameOnly(t *testing.T) { ScheduledTaskSecretName: "arn:aws:secretsmanager:us-east-1:123:secret:my-secret", } - got := resolveScheduledTaskSecret(ctx, cfg, resolver) + got, err := resolveScheduledTaskSecret(ctx, cfg, resolver) + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "prod-secret", got) } + +// TestNewApplicationFromDeps_BearerModeSecretResolutionFails is a regression +// test for 04-M4: before the fix, a Key Vault / Secrets Manager lookup failure +// in bearer mode caused startup to fail with the misleading "bearer mode +// requires SCHEDULED_TASK_SECRET" error (because the empty fallback value was +// passed to buildScheduledAuthFromConfig). The fix propagates the resolver +// error directly, so the log shows the actual cause. +func TestNewApplicationFromDeps_BearerModeSecretResolutionFails(t *testing.T) { + ctx := context.Background() + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "bearer") + + cfg := ApplicationConfig{ + ScheduledTaskSecretName: "arn:aws:secretsmanager:us-east-1:123:secret:task-secret", + // No plaintext SCHEDULED_TASK_SECRET -- empty fallback. + } + deps := ExternalDeps{ + DBConfig: &database.Config{Host: "localhost"}, + SecretResolver: &mockSecretResolver{getErr: errors.New("key vault unreachable")}, + } + + _, err := NewApplicationFromDeps(ctx, cfg, deps) + testutil.AssertError(t, err) + testutil.AssertContains(t, err.Error(), "arn:aws:secretsmanager:us-east-1:123:secret:task-secret") + testutil.AssertContains(t, err.Error(), "key vault unreachable") +} From e785e01b504c71b80761b87337d82f26b86abdc3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:34:19 -0700 Subject: [PATCH 09/11] refactor(server): move migrationsTimeout/runMigrationsFunc to Application struct fields 04-M3: migrationsTimeout and runMigrations were package-level mutable vars used as test-injection points, with a "MUST NOT call t.Parallel()" comment the compiler cannot enforce. A future contributor adding t.Parallel() to a migration test would trigger a data race. Fix: - Add migrationsTimeout time.Duration and runMigrationsFunc fields to Application; set them in NewApplicationFromDeps. - Add app.runMigrationsBounded() method that delegates to runMigrationsBoundedWith(), a pure function accepting a runner argument. - Remove package-level migrationsTimeout var and runMigrations var. - Update tests to pass a fake runner directly to runMigrationsBoundedWith and add t.Parallel() calls (now safe). Regression test: TestEnsureDB_UsesInstanceMigrationsTimeout -- two Application instances with different timeouts run in parallel. Part of #1067. --- internal/server/app.go | 89 +++++++++++++++++++++---------------- internal/server/app_test.go | 84 ++++++++++++++++++++-------------- 2 files changed, 102 insertions(+), 71 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index 6e0bf60c0..f4bfd9b04 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -94,6 +94,13 @@ type Application struct { // scheduledAuthMiddleware helper passes through unmodified in // that case so handler-only tests stay focused. scheduledAuth *scheduledauth.Validator + + // migrationsTimeout and runMigrationsFunc are per-instance instead of + // package-level variables so that tests can set them on a specific + // Application instance without serialising parallel tests (04-M3). + // NewApplicationFromDeps sets them to the package defaults. + migrationsTimeout time.Duration + runMigrationsFunc func(ctx context.Context, pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string) error } // ApplicationConfig holds all env-based configuration for the application @@ -147,13 +154,11 @@ type ExternalDeps struct { // (which is exactly what leaves schema_migrations.dirty = true). const defaultMigrationsTimeout = 20 * time.Second -// migrationsTimeout is resolved once at package init time from -// CUDLY_MIGRATION_TIMEOUT (time.ParseDuration). Declared as a var (not -// const) so tests can overwrite it inside t.Cleanup to exercise the -// timeout path with a 50ms budget. Tests that overwrite this MUST NOT -// call t.Parallel() since there's no synchronisation on the variable. -var migrationsTimeout = resolveMigrationsTimeout() - +// resolveMigrationsTimeout reads CUDLY_MIGRATION_TIMEOUT from the environment. +// It is called once in NewApplicationFromDeps to initialise +// Application.migrationsTimeout. Because the timeout lives on the struct +// (not a package-level var), tests can set it on a specific Application +// instance without serialising parallel tests (04-M3). func resolveMigrationsTimeout() time.Duration { v := os.Getenv("CUDLY_MIGRATION_TIMEOUT") if v == "" { @@ -167,11 +172,6 @@ func resolveMigrationsTimeout() time.Duration { return d } -// runMigrations is a package-level indirection so tests can swap in a fake -// that returns error / hangs / succeeds without running real SQL. Same -// parallel-test restriction as migrationsTimeout. -var runMigrations = migrations.RunMigrations - // recordMigrationResult stores the outcome of the most recent migration // attempt. Takes migrationMu briefly. Called from inside ensureDB which // holds dbMu — lock order is dbMu then migrationMu. Never take them the @@ -191,15 +191,26 @@ func (app *Application) snapshotMigrationState() (err error, finishedAt time.Tim return app.migrationErr, app.migrationFinishedAt } -// runMigrationsBounded runs the package-level runMigrations hook in a -// goroutine bounded by timeout. The returned error is either the runner's -// own error, a panic wrapped as an error, or a timeout error — never a -// nil-with-goroutine-still-alive. The goroutine is guaranteed to have -// exited before this function returns (the timeout branch waits on -// <-done after cancelling the ctx), so no orphan goroutine survives past -// this call — critical on Lambda where goroutines freeze between -// invocations. -func runMigrationsBounded(pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string, timeout time.Duration) error { +// runMigrationsBounded runs app.runMigrationsFunc in a goroutine bounded by +// app.migrationsTimeout. The returned error is either the runner's own error, +// a panic wrapped as an error, or a timeout error -- never a +// nil-with-goroutine-still-alive. The goroutine is guaranteed to have exited +// before this function returns (the timeout branch waits on <-done after +// cancelling the ctx), so no orphan goroutine survives past this call -- +// critical on Lambda where goroutines freeze between invocations. +// +// Using instance fields (not package globals) makes it safe to call +// t.Parallel() in tests that override migrationsTimeout or runMigrationsFunc +// on a specific Application -- no shared mutable global state (04-M3). +func (app *Application) runMigrationsBounded(pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string) error { + return runMigrationsBoundedWith(pool, migrationsPath, adminEmail, adminPassword, app.migrationsTimeout, app.runMigrationsFunc) +} + +// runMigrationsBoundedWith is the underlying implementation used by the +// Application method and by the package-level tests that pre-date the +// instance-field migration (04-M3). Tests that want to exercise the +// timeout/panic/success paths can call this directly with a fake runner. +func runMigrationsBoundedWith(pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string, timeout time.Duration, runner func(ctx context.Context, pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string) error) error { migCtx, cancelMig := context.WithTimeout(context.Background(), timeout) defer cancelMig() @@ -210,7 +221,7 @@ func runMigrationsBounded(pool *pgxpool.Pool, migrationsPath, adminEmail, adminP done <- fmt.Errorf("migration panic: %v", r) } }() - done <- runMigrations(migCtx, pool, migrationsPath, adminEmail, adminPassword) + done <- runner(migCtx, pool, migrationsPath, adminEmail, adminPassword) }() select { @@ -480,21 +491,23 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext log.Printf("CUDly Server initialization complete") return &Application{ - Config: deps.ConfigStore, - API: apiHandler, - Scheduler: sched, - Purchase: purchaseManager, - Email: deps.EmailSender, - Auth: authService, - RateLimiter: rateLimiter, - Version: cfg.Version, - DB: nil, // Will be initialized lazily on first request - staticDir: staticDirFromEnv(), - dbConfig: deps.DBConfig, - secretResolver: deps.SecretResolver, - appConfig: cfg, - signer: signer, - scheduledAuth: scheduledAuth, + Config: deps.ConfigStore, + API: apiHandler, + Scheduler: sched, + Purchase: purchaseManager, + Email: deps.EmailSender, + Auth: authService, + RateLimiter: rateLimiter, + Version: cfg.Version, + DB: nil, // Will be initialized lazily on first request + staticDir: staticDirFromEnv(), + dbConfig: deps.DBConfig, + secretResolver: deps.SecretResolver, + appConfig: cfg, + signer: signer, + scheduledAuth: scheduledAuth, + migrationsTimeout: resolveMigrationsTimeout(), + runMigrationsFunc: migrations.RunMigrations, }, nil } @@ -591,7 +604,7 @@ func (app *Application) ensureDB(ctx context.Context) error { return err // secret-resolution failure is still fatal — env/config, not a migration runtime error } - migErr := runMigrationsBounded(dbConn.Pool(), app.dbConfig.MigrationsPath, adminEmail, adminPassword, migrationsTimeout) + migErr := app.runMigrationsBounded(dbConn.Pool(), app.dbConfig.MigrationsPath, adminEmail, adminPassword) app.recordMigrationResult(migErr) if migErr != nil { diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 4f7b31938..d5f27ce6d 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -656,66 +656,84 @@ func TestHandleScheduledHTTP_EnsureDBError(t *testing.T) { testutil.AssertEqual(t, 503, w.Code) } -// ---------- runMigrationsBounded tests ---------- -// -// These tests exercise the goroutine+timeout+recover logic in isolation by -// swapping the package-level runMigrations hook. They do NOT hit a real DB, -// so the *pgxpool.Pool passed in is a typed-nil — the fake runner never -// dereferences it. MUST NOT run in parallel since the hook is global. +// TestEnsureDB_UsesInstanceMigrationsTimeout is a regression test for 04-M3: +// before the fix, migrationsTimeout was a package-level var that tests had to +// swap under a serial-test constraint. The fix stores it on Application, so +// distinct instances can have different timeouts without interfering. +func TestEnsureDB_UsesInstanceMigrationsTimeout(t *testing.T) { + t.Parallel() // this must be safe now that the field lives on the struct + + // Build two independent Application instances with different timeouts. + // The fast one uses a 50ms budget (guaranteed to expire before the slow + // runner finishes); the slow one uses 1s (always succeeds). + slow := make(chan error, 1) + fastApp := &Application{ + migrationsTimeout: 50 * time.Millisecond, + runMigrationsFunc: func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { + <-ctx.Done() + slow <- ctx.Err() + return ctx.Err() + }, + } -func withFakeMigrations(t *testing.T, fake func(ctx context.Context, pool *pgxpool.Pool, path, email, pw string) error) { - t.Helper() - orig := runMigrations - runMigrations = fake - t.Cleanup(func() { runMigrations = orig }) + err := fastApp.runMigrationsBounded(nil, "", "", "") + testutil.AssertError(t, err) + testutil.AssertTrue(t, strings.Contains(err.Error(), "timed out"), + "expected 'timed out', got: "+err.Error()) + // Drain the channel so the goroutine does not leak. + <-slow } +// ---------- runMigrationsBoundedWith tests ---------- +// +// These tests exercise the goroutine+timeout+recover logic in isolation by +// passing a fake runner directly. They do NOT hit a real DB, so the +// *pgxpool.Pool passed in is a typed-nil -- the fake runner never +// dereferences it. Tests are now safe to call t.Parallel() because no +// package-level mutable state is used (04-M3). + func TestRunMigrationsBounded_Success(t *testing.T) { - withFakeMigrations(t, func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { - return nil - }) - err := runMigrationsBounded(nil, "", "", "", 1*time.Second) + t.Parallel() + err := runMigrationsBoundedWith(nil, "", "", "", 1*time.Second, + func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { return nil }) testutil.AssertNoError(t, err) } func TestRunMigrationsBounded_FailureReturnsError(t *testing.T) { + t.Parallel() sentinel := errors.New("dirty at 27") - withFakeMigrations(t, func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { - return sentinel - }) - err := runMigrationsBounded(nil, "", "", "", 1*time.Second) + err := runMigrationsBoundedWith(nil, "", "", "", 1*time.Second, + func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { return sentinel }) if !errors.Is(err, sentinel) { t.Fatalf("expected error to wrap sentinel, got: %v", err) } } func TestRunMigrationsBounded_Timeout(t *testing.T) { - withFakeMigrations(t, func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { - <-ctx.Done() // block until runMigrationsBounded cancels the ctx - return ctx.Err() - }) - + t.Parallel() start := time.Now() - err := runMigrationsBounded(nil, "", "", "", 50*time.Millisecond) + err := runMigrationsBoundedWith(nil, "", "", "", 50*time.Millisecond, + func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { + <-ctx.Done() // block until the timeout cancels the ctx + return ctx.Err() + }) elapsed := time.Since(start) testutil.AssertError(t, err) if !strings.Contains(err.Error(), "timed out") { t.Fatalf("expected 'timed out' in error; got %q", err.Error()) } - // runMigrationsBounded must return shortly after the timeout elapses. - // If it took significantly longer than 2× the budget, the <-done - // rendezvous hung — meaning the goroutine wasn't properly cleaned up. + // Must return shortly after the timeout. Significantly longer means the + // goroutine was not joined -- a goroutine leak (critical on Lambda). if elapsed > 200*time.Millisecond { - t.Fatalf("runMigrationsBounded took too long (%s); goroutine may have leaked", elapsed) + t.Fatalf("runMigrationsBoundedWith took too long (%s); goroutine may have leaked", elapsed) } } func TestRunMigrationsBounded_PanicRecovered(t *testing.T) { - withFakeMigrations(t, func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { - panic("boom") - }) - err := runMigrationsBounded(nil, "", "", "", 1*time.Second) + t.Parallel() + err := runMigrationsBoundedWith(nil, "", "", "", 1*time.Second, + func(ctx context.Context, _ *pgxpool.Pool, _, _, _ string) error { panic("boom") }) testutil.AssertError(t, err) if !strings.Contains(err.Error(), "panic") || !strings.Contains(err.Error(), "boom") { t.Fatalf("expected panic error to mention both 'panic' and 'boom'; got %q", err.Error()) From f2db02b1c3f8ca7f9db0e30332e636f0eecd73d0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:35:17 -0700 Subject: [PATCH 10/11] fix(migrations,server): use non-concurrent MV refresh in migration; surface error in result 06-M4 (migration half): REFRESH MATERIALIZED VIEW CONCURRENTLY cannot run inside a transaction block. Migration 000003 called refresh_savings_materialized_views() which used CONCURRENTLY for all three views -- this would fail at every fresh deploy. Replace with plain (non- concurrent) REFRESH statements directly in the migration body. Reserve CONCURRENTLY for the runtime scheduled-task path (refresh_savings_ materialized_views()) which runs outside any transaction. 06-M4 (server half): handleRefreshAnalytics only logged a view-refresh failure as "Warning" with no way for the API caller or the operator reading the scheduled-task response body to see the cause. Add "views_error" to the result map so the error is visible in the response alongside the existing server-side log. Part of #1067. --- .../migrations/000003_analytics_partitions.up.sql | 10 ++++++++-- internal/server/handler.go | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/internal/database/postgres/migrations/000003_analytics_partitions.up.sql b/internal/database/postgres/migrations/000003_analytics_partitions.up.sql index 1018c2d33..bed66d393 100644 --- a/internal/database/postgres/migrations/000003_analytics_partitions.up.sql +++ b/internal/database/postgres/migrations/000003_analytics_partitions.up.sql @@ -173,8 +173,14 @@ $$ LANGUAGE plpgsql; -- Create partitions for current month + 3 months ahead SELECT create_future_savings_partitions(3); --- Initial refresh of materialized views (will be empty at first) -SELECT refresh_savings_materialized_views(); +-- Initial (non-concurrent) refresh of materialized views. +-- REFRESH MATERIALIZED VIEW CONCURRENTLY cannot run inside a transaction +-- block and is unnecessary here because the views are empty at migration +-- time (06-M4). Reserve CONCURRENTLY for the scheduled runtime refresh +-- invoked via refresh_savings_materialized_views() outside any transaction. +REFRESH MATERIALIZED VIEW monthly_savings_summary; +REFRESH MATERIALIZED VIEW daily_savings_trend; +REFRESH MATERIALIZED VIEW provider_savings_summary; -- Add comment explaining partition maintenance COMMENT ON FUNCTION create_savings_snapshot_partition IS diff --git a/internal/server/handler.go b/internal/server/handler.go index 555974913..be7b9ebbd 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -215,11 +215,15 @@ func (app *Application) handleRefreshAnalytics(ctx context.Context) (map[string] "partitions_dropped": 0, } - // Refresh materialized views if analytics store is available + // Refresh materialized views if analytics store is available. + // Include the error in the result map so API callers (and the operator + // reading the scheduled-task response body) can see it, not only the + // server-side log (06-M4 error-visibility). if app.Analytics != nil { if err := app.Analytics.RefreshMaterializedViews(ctx); err != nil { log.Printf("Warning: failed to refresh materialized views: %v", err) result["status"] = "partial" + result["views_error"] = err.Error() } else { result["views_refreshed"] = 1 log.Println("Materialized views refreshed successfully") From e50df958014f483873a5b2cddefed5ddb170a95c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:55:13 -0700 Subject: [PATCH 11/11] fix(server): reduce cyclomatic complexity; apply gofmt CI pre-commit checks flagged two issues after the FOLD-1040 commits: gofmt: trailing newline in static.go and misaligned struct fields in app.go (migrationsTimeout/runMigrationsFunc). Fixed by gofmt -w. Cyclomatic complexity > 10: - resolveStaticFilePath (11): extract isPathContainedIn helper that encapsulates the separator-aware prefix check (04-M6). Reduces the compound &&-condition branch inside resolveStaticFilePath. - NewApplicationFromDeps (12): extract initScheduledAuth helper that consolidates the scheduledauth.LoadConfig / resolveScheduledTaskSecret / buildScheduledAuthFromConfig / Warmup sequence added for 04-M4. The function reads as one call at the call site instead of four nested if/err branches. Part of #1067. --- internal/server/app.go | 64 +++++++++++++++++++++------------------ internal/server/static.go | 16 +++++++--- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/internal/server/app.go b/internal/server/app.go index f4bfd9b04..d67d4eae5 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -99,8 +99,8 @@ type Application struct { // package-level variables so that tests can set them on a specific // Application instance without serialising parallel tests (04-M3). // NewApplicationFromDeps sets them to the package defaults. - migrationsTimeout time.Duration - runMigrationsFunc func(ctx context.Context, pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string) error + migrationsTimeout time.Duration + runMigrationsFunc func(ctx context.Context, pool *pgxpool.Pool, migrationsPath, adminEmail, adminPassword string) error } // ApplicationConfig holds all env-based configuration for the application @@ -351,49 +351,53 @@ func buildScheduledAuthFromConfig(cfg ApplicationConfig, saCfg scheduledauth.Con return scheduledauth.New(saCfg) } -// NewApplicationFromDeps creates an Application from pre-built configuration and dependencies. -// This is the testable constructor - all external I/O is done before calling this. -func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps ExternalDeps) (*Application, error) { - if deps.DBConfig == nil { - return nil, fmt.Errorf("database configuration required: DBConfig must be provided") - } - - // Validate money-moving env defaults at the startup boundary -- consistent with the - // fail-fast posture used for scheduledauth mode and ADMIN_PASSWORD_SECRET (issue #1026). - if err := validateAppConfigEnvDefaults(cfg); err != nil { - return nil, err - } - - // Load the scheduledauth config first so we know the mode before - // resolving the secret. Bearer mode requires a non-empty secret, so - // a resolution failure must be propagated as a fatal startup error - // rather than silently falling back to the empty plaintext env var - // and surfacing the misleading "bearer mode requires SCHEDULED_TASK_SECRET" - // error downstream (04-M4). +// initScheduledAuth loads the scheduledauth config, resolves the bearer +// secret (failing fast in bearer mode if the resolver errors), builds the +// validator, and warms up JWKS. Extracted from NewApplicationFromDeps to +// keep its cyclomatic complexity within the project limit (04-M4). +func initScheduledAuth(ctx context.Context, cfg *ApplicationConfig, resolver secrets.Resolver) (*scheduledauth.Validator, error) { saCfg, err := scheduledauth.LoadConfig(envSourceOS{}) if err != nil { return nil, fmt.Errorf("scheduled-task auth init: %w", err) } - resolvedSecret, secretErr := resolveScheduledTaskSecret(ctx, cfg, deps.SecretResolver) + resolvedSecret, secretErr := resolveScheduledTaskSecret(ctx, *cfg, resolver) if secretErr != nil && saCfg.Mode == scheduledauth.ModeBearer { return nil, fmt.Errorf("failed to resolve scheduled-task secret from %q: %w", cfg.ScheduledTaskSecretName, secretErr) } cfg.ScheduledTaskSecret = resolvedSecret - // Build the /api/scheduled/* auth validator from the already-loaded - // config. Fail-fast on bad config (empty subjects in oidc mode, etc.) - // — better to crash on startup than to silently accept unauthenticated - // scheduled-task calls in production. - scheduledAuth, err := buildScheduledAuthFromConfig(cfg, saCfg) + v, err := buildScheduledAuthFromConfig(*cfg, saCfg) if err != nil { return nil, fmt.Errorf("scheduled-task auth init: %w", err) } - // Best-effort JWKS warmup — surfaces misconfiguration in startup - // logs without blocking startup if Google's CDN is unreachable. + warmCtx, warmCancel := context.WithTimeout(ctx, 5*time.Second) - scheduledAuth.Warmup(warmCtx) + v.Warmup(warmCtx) warmCancel() + return v, nil +} + +// NewApplicationFromDeps creates an Application from pre-built configuration and dependencies. +// This is the testable constructor - all external I/O is done before calling this. +func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps ExternalDeps) (*Application, error) { + if deps.DBConfig == nil { + return nil, fmt.Errorf("database configuration required: DBConfig must be provided") + } + + // Validate money-moving env defaults at the startup boundary -- consistent with the + // fail-fast posture used for scheduledauth mode and ADMIN_PASSWORD_SECRET (issue #1026). + if err := validateAppConfigEnvDefaults(cfg); err != nil { + return nil, err + } + + // Wire up the /api/scheduled/* auth validator. initScheduledAuth loads + // the mode config, resolves the bearer secret with fail-fast semantics + // in bearer mode (04-M4), builds the validator, and warms up JWKS. + scheduledAuth, err := initScheduledAuth(ctx, &cfg, deps.SecretResolver) + if err != nil { + return nil, err + } // Construct the OIDC issuer signer once per deployment. Nil means // the deployment has not opted into the federated flow yet — all diff --git a/internal/server/static.go b/internal/server/static.go index 0671ba6ae..6693699a9 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -49,6 +49,16 @@ func setCacheHeaders(w http.ResponseWriter, urlPath string) { } } +// isPathContainedIn reports whether absFile is at or under absDir, using a +// separator-aware prefix check to prevent sibling-directory confusion. +// "/srv/static" is a string-prefix of "/srv/static-evil", but +// "/srv/static/" is not -- so we append the separator before comparing. +// The absFile == absDir case handles the dir itself (index.html fallback). +func isPathContainedIn(absFile, absDir string) bool { + return absFile == absDir || + strings.HasPrefix(absFile, absDir+string(os.PathSeparator)) +} + // resolveStaticFilePath validates the URL path against directory traversal and // resolves the actual file path. Falls back to index.html for extensionless // paths (SPA routing). Returns the file path, the clean path used for content @@ -69,10 +79,7 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok if err != nil { return "", "", false } - // Require the separator after absDir so that a sibling directory whose - // name shares a prefix (e.g. /srv/static-evil vs /srv/static) cannot - // pass the containment check (04-M6). - if !strings.HasPrefix(absFile, absDir+string(os.PathSeparator)) && absFile != absDir { + if !isPathContainedIn(absFile, absDir) { return "", "", false } @@ -174,4 +181,3 @@ func isStaticPath(urlPath string) bool { } return true } -