From 5bfee589648e48bc3ed30688e2cda9298d22ebcb Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sun, 28 Jun 2026 22:15:04 +0700 Subject: [PATCH] refactor(server): drop HTTP /cron route; run crons in-process only The in-process scheduler is the sole cron trigger on self-host, so the /cron/{name} HTTP endpoint and its CRON_SHARED_SECRET are dead surface. Reduce the HTTP server to GET / (health) and remove the cron secret config, SSM binding, and now-unused timeout. --- cmd/server/main.go | 23 +----- internal/cron/scheduler.go | 10 +-- internal/modules/cron_dispatcher.go | 8 +- internal/server/router.go | 113 ++------------------------ internal/server/router_test.go | 118 ---------------------------- internal/server/timeouts.go | 9 --- 6 files changed, 18 insertions(+), 263 deletions(-) delete mode 100644 internal/server/timeouts.go diff --git a/cmd/server/main.go b/cmd/server/main.go index 911fec6..4b70537 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -143,9 +143,6 @@ func main() { if cfg.BotOwnerID == 0 { log.Warn("OWNER_ID unset; all Private + Protected commands will be denied") } - if cfg.CronSecret == "" { - log.Warn("CRON_SHARED_SECRET unset; /cron/{name} disabled (404 to all)") - } deploynotify.Run(rootCtx, deploynotify.Config{ Bot: b, @@ -154,23 +151,16 @@ func main() { GitSHA: gitSHA, }) - handler := server.New(server.Config{ - Bot: b, - Registry: reg, - CronSecret: cfg.CronSecret, - }) + handler := server.New() srv := &http.Server{ Addr: ":" + cfg.Port, Handler: handler, ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, - // 75s = cron handler cap (60s, internal/server/timeouts.go) plus a - // 15s margin for response serialization. On Lambda the 30s function - // timeout supersedes this; the tighter ceiling matters only for - // local non-Lambda runs where a 6-minute slow-loris write was the - // previous (over-generous) bound. - WriteTimeout: 75 * time.Second, + // The only route is GET / (health). It responds instantly, so a tight + // write deadline is ample and bounds any slow-loris write. + WriteTimeout: 30 * time.Second, IdleTimeout: 120 * time.Second, } @@ -267,7 +257,6 @@ func buildProvider(ctx context.Context, cfg config) (storage.Provider, func(), e type config struct { Port string TelegramBotToken string - CronSecret string GeminiAPIKey string GoldPriceAPIURL string GoldFXAPIURL string @@ -283,7 +272,6 @@ type config struct { MongoURL string // required when KVProvider=mongodb (Atlas SRV connection string; SECRET — never log) MongoDatabase string // required when KVProvider=mongodb TelegramBotTokenParam string - CronSecretParam string GeminiAPIKeyParam string GoldVNAppAPIKeyParam string } @@ -308,7 +296,6 @@ func loadConfig() config { return config{ Port: port, TelegramBotToken: envMap["TELEGRAM_BOT_TOKEN"], - CronSecret: envMap["CRON_SHARED_SECRET"], GeminiAPIKey: envMap["GEMINI_API_KEY"], GoldPriceAPIURL: envMap["GOLD_PRICE_API_URL"], GoldFXAPIURL: envMap["GOLD_FX_API_URL"], @@ -324,7 +311,6 @@ func loadConfig() config { MongoURL: envMap["MONGO_URL"], MongoDatabase: envMap["MONGO_DATABASE"], TelegramBotTokenParam: strings.TrimSpace(envMap["TELEGRAM_BOT_TOKEN_PARAMETER_NAME"]), - CronSecretParam: strings.TrimSpace(envMap["CRON_SHARED_SECRET_PARAMETER_NAME"]), GeminiAPIKeyParam: strings.TrimSpace(envMap["GEMINI_API_KEY_PARAMETER_NAME"]), GoldVNAppAPIKeyParam: strings.TrimSpace(envMap["GOLD_VNAPP_API_KEY_PARAMETER_NAME"]), } @@ -336,7 +322,6 @@ func resolveSSMSecrets(ctx context.Context, cfg *config) error { target *string }{ {name: cfg.TelegramBotTokenParam, target: &cfg.TelegramBotToken}, - {name: cfg.CronSecretParam, target: &cfg.CronSecret}, {name: cfg.GeminiAPIKeyParam, target: &cfg.GeminiAPIKey}, {name: cfg.GoldVNAppAPIKeyParam, target: &cfg.GoldVNAppAPIKey}, } diff --git a/internal/cron/scheduler.go b/internal/cron/scheduler.go index fc6086c..30f72c4 100644 --- a/internal/cron/scheduler.go +++ b/internal/cron/scheduler.go @@ -16,17 +16,15 @@ import ( "github.com/tiennm99/miti99bot/internal/modules" ) -// cronTimeout caps a single in-process cron fire. Defined locally (rather than -// imported from internal/server) to avoid a server→cron layering inversion; -// it intentionally matches internal/server's defaultCronTimeout so the -// in-process path and the HTTP /cron path share the same 60s budget. +// cronTimeout caps a single in-process cron fire so a slow handler cannot +// wedge the scheduler. 60s is the budget; long crons must offload and return +// fast. const cronTimeout = 60 * time.Second // Run starts an in-process scheduler that fires every registered cron with a // non-empty Schedule. Each fire is wrapped with a timeout, panic recovery, and // structured logging — modules.DispatchScheduled provides none of these (it is -// only a registry lookup + handler call), so the wrapping lives here, mirroring -// the server-package cronHandler. +// only a registry lookup + handler call), so the wrapping lives here. // // A malformed schedule string is fatal (returned as an error) so a config typo // fails fast at startup rather than silently never firing. The returned stop diff --git a/internal/modules/cron_dispatcher.go b/internal/modules/cron_dispatcher.go index f228486..7cef0b0 100644 --- a/internal/modules/cron_dispatcher.go +++ b/internal/modules/cron_dispatcher.go @@ -6,16 +6,16 @@ import ( "fmt" ) -// ErrCronNotFound is returned when /cron/{name} addresses an unregistered cron. +// ErrCronNotFound is returned when name addresses an unregistered cron. var ErrCronNotFound = errors.New("cron not found") // DispatchScheduled runs the cron registered under name with the per-module // prefixed Deps the registry stored at Build time. Returns ErrCronNotFound if -// no module owns that name — EventBridge Scheduler hitting an unknown route is a -// configuration bug worth surfacing as a 404 at the HTTP layer. +// no module owns that name — a scheduler firing an unknown name is a +// configuration bug worth surfacing to the caller. // // The handler runs synchronously in the calling goroutine; ctx propagates -// cancellation/timeout from the HTTP request. +// the scheduler's per-fire cancellation/timeout. func DispatchScheduled(ctx context.Context, name string, reg *Registry) error { cron, ok := reg.Cron(name) if !ok { diff --git a/internal/server/router.go b/internal/server/router.go index 1f82e85..2a77172 100644 --- a/internal/server/router.go +++ b/internal/server/router.go @@ -1,120 +1,19 @@ package server -import ( - "context" - "crypto/subtle" - "errors" - "net/http" - "regexp" - "runtime/debug" - "strings" +import "net/http" - "github.com/go-telegram/bot" - - "github.com/tiennm99/miti99bot/internal/log" - "github.com/tiennm99/miti99bot/internal/modules" -) - -// cronNameRe limits cron path segments to a safe alphabet so log injection via -// the route is impossible (newlines, ANSI escapes, etc. are rejected at the -// router boundary). Same shape as Telegram command names. -var cronNameRe = regexp.MustCompile(`^[a-z0-9_]{1,32}$`) - -// cronAuthHeader is the shared-secret header a caller attaches when invoking -// /cron/{name} for a manual trigger. -const cronAuthHeader = "X-Cron-Token" - -// Config wires the router's runtime dependencies. -type Config struct { - Bot *bot.Bot - Registry *modules.Registry - - // CronSecret protects /cron/{name} against unauthenticated calls; a caller - // attaches it as the X-Cron-Token header. Empty means /cron/{name} is fully - // disabled (404) — the default on self-host, where the in-process scheduler - // (internal/cron) is the sole cron trigger. - CronSecret string -} - -// New builds the application's HTTP handler. Routes: +// New builds the application's HTTP handler. The only route is: // -// GET / → health (Coolify container monitor; not publicly routed) -// POST /cron/{name} → optional manual cron trigger (shared-secret check) +// GET / → health (Coolify container monitor; not publicly routed) // // There is no /webhook route: Telegram updates arrive via long polling // (cmd/server runs b.Start), so the bot needs no public inbound ingress. +// There is no /cron route either: crons fire from the in-process scheduler +// (internal/cron), so cron has no HTTP surface to expose or protect. // Anything else is 404. All routes pass through LogRequests so every request // emits a structured `req` log line. -func New(cfg Config) http.Handler { +func New() http.Handler { mux := http.NewServeMux() mux.Handle("/", HealthHandler()) - mux.Handle("/cron/", cronHandler(cfg.Registry, cfg.CronSecret)) return LogRequests(mux) } - -func cronHandler(reg *modules.Registry, secret string) http.HandlerFunc { - secretBytes := []byte(secret) - cronDisabled := secret == "" - return func(w http.ResponseWriter, r *http.Request) { - // Rejection paths use bare status codes (no response body) so a - // scanner hitting /cron/ can't fingerprint the route from the - // response text. Status codes remain distinct for CloudWatch - // metric filters; structured log lines carry the reason for - // operator triage. - if cronDisabled { - w.WriteHeader(http.StatusNotFound) - return - } - if r.Method != http.MethodPost { - log.Warn("cron rejected", "reason", "method", "method", r.Method) - w.WriteHeader(http.StatusMethodNotAllowed) - return - } - got := []byte(r.Header.Get(cronAuthHeader)) - if subtle.ConstantTimeCompare(got, secretBytes) != 1 { - log.Warn("cron rejected", "reason", "secret_mismatch") - w.WriteHeader(http.StatusUnauthorized) - return - } - - name := strings.TrimPrefix(r.URL.Path, "/cron/") - if !cronNameRe.MatchString(name) { - log.Warn("cron rejected", "reason", "bad_name", "name", name) - w.WriteHeader(http.StatusNotFound) - return - } - - log.Info("cron triggered", "route", "/cron", "name", name) - ctx, cancel := context.WithTimeout(r.Context(), defaultCronTimeout) - defer cancel() - - // Recover panics with cron-name context BEFORE the LogRequests - // middleware's safety-net recover sees them — otherwise CloudWatch - // would just show "middleware recovered panic" with no clue which - // scheduled job blew up. EventBridge sees 500 either way. - var dispatchErr error - func() { - defer func() { - if rec := recover(); rec != nil { - log.Error("cron handler panic", - "route", "/cron", - "name", name, - "panic", rec, - "stack", string(debug.Stack())) - dispatchErr = errors.New("cron handler panicked") - } - }() - dispatchErr = modules.DispatchScheduled(ctx, name, reg) - }() - if dispatchErr != nil { - if errors.Is(dispatchErr, modules.ErrCronNotFound) { - w.WriteHeader(http.StatusNotFound) - return - } - log.Error("cron failed", "route", "/cron", "name", name, "err", dispatchErr) - w.WriteHeader(http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusOK) - } -} diff --git a/internal/server/router_test.go b/internal/server/router_test.go index 3fa0495..65998ba 100644 --- a/internal/server/router_test.go +++ b/internal/server/router_test.go @@ -1,27 +1,12 @@ package server import ( - "context" "net/http" "net/http/httptest" "strings" "testing" - - "github.com/tiennm99/miti99bot/internal/modules" - "github.com/tiennm99/miti99bot/internal/storage" ) -const testCronSecret = "shared-cron-secret" - -func buildRegistry(t *testing.T, factories map[string]modules.Factory, names ...string) *modules.Registry { - t.Helper() - reg, err := modules.Build(names, factories, storage.NewMemoryProvider(), modules.BuildOptions{}) - if err != nil { - t.Fatalf("modules.Build: %v", err) - } - return reg -} - func TestHealthHandler_OK(t *testing.T) { rec := httptest.NewRecorder() HealthHandler()(rec, httptest.NewRequest(http.MethodGet, "/", nil)) @@ -32,106 +17,3 @@ func TestHealthHandler_OK(t *testing.T) { t.Errorf("body = %q, want contains 'ok'", rec.Body.String()) } } - -func TestCronHandler_DisabledWhenSecretEmpty(t *testing.T) { - reg := buildRegistry(t, nil) - h := cronHandler(reg, "") - - rec := httptest.NewRecorder() - h(rec, httptest.NewRequest(http.MethodPost, "/cron/anything", nil)) - if rec.Code != http.StatusNotFound { - t.Errorf("status = %d, want 404 (disabled)", rec.Code) - } -} - -func TestCronHandler_RejectsNonPost(t *testing.T) { - reg := buildRegistry(t, nil) - h := cronHandler(reg, testCronSecret) - - rec := httptest.NewRecorder() - h(rec, httptest.NewRequest(http.MethodGet, "/cron/anything", nil)) - if rec.Code != http.StatusMethodNotAllowed { - t.Errorf("status = %d, want 405", rec.Code) - } -} - -func TestCronHandler_RejectsMissingAuth(t *testing.T) { - reg := buildRegistry(t, nil) - h := cronHandler(reg, testCronSecret) - - rec := httptest.NewRecorder() - h(rec, httptest.NewRequest(http.MethodPost, "/cron/anything", nil)) - if rec.Code != http.StatusUnauthorized { - t.Errorf("status = %d, want 401", rec.Code) - } -} - -func TestCronHandler_RejectsInvalidName(t *testing.T) { - reg := buildRegistry(t, nil) - h := cronHandler(reg, testCronSecret) - - // Use bare names that fail the regex; net/http already %-decodes the - // path, so a smuggled \n on the wire would arrive here as a literal byte. - cases := map[string]string{ - "uppercase": "/cron/BadName", - "hyphen": "/cron/with-dash", - "newline": "/cron/with\nnewline", - "empty": "/cron/", - "too long": "/cron/abcdefghijklmnopqrstuvwxyzabcdefg", // 33 chars - "path nested": "/cron/foo/bar", - } - for label, path := range cases { - t.Run(label, func(t *testing.T) { - req := httptest.NewRequest(http.MethodPost, "/cron/x", nil) - req.URL.Path = path // bypass NewRequest's URL parser - req.Header.Set(cronAuthHeader, testCronSecret) - rec := httptest.NewRecorder() - h(rec, req) - if rec.Code != http.StatusNotFound { - t.Errorf("path %q: status = %d, want 404", path, rec.Code) - } - }) - } -} - -func TestCronHandler_UnknownNameReturns404(t *testing.T) { - reg := buildRegistry(t, nil) - h := cronHandler(reg, testCronSecret) - - req := httptest.NewRequest(http.MethodPost, "/cron/missing", nil) - req.Header.Set(cronAuthHeader, testCronSecret) - rec := httptest.NewRecorder() - h(rec, req) - if rec.Code != http.StatusNotFound { - t.Errorf("status = %d, want 404", rec.Code) - } -} - -func TestCronHandler_RunsRegisteredCron(t *testing.T) { - called := false - factories := map[string]modules.Factory{ - "alpha": func(_ modules.Deps) modules.Module { - return modules.Module{Crons: []modules.Cron{{ - Name: "tick", - Handler: func(_ context.Context, _ modules.Deps) error { - called = true - return nil - }, - }}} - }, - } - reg := buildRegistry(t, factories, "alpha") - h := cronHandler(reg, testCronSecret) - - req := httptest.NewRequest(http.MethodPost, "/cron/tick", nil) - req.Header.Set(cronAuthHeader, testCronSecret) - rec := httptest.NewRecorder() - h(rec, req) - - if rec.Code != http.StatusOK { - t.Errorf("status = %d, want 200", rec.Code) - } - if !called { - t.Error("cron handler not invoked") - } -} diff --git a/internal/server/timeouts.go b/internal/server/timeouts.go deleted file mode 100644 index 00fdfc6..0000000 --- a/internal/server/timeouts.go +++ /dev/null @@ -1,9 +0,0 @@ -package server - -import "time" - -// defaultCronTimeout caps a single /cron/{name} invocation. Lambda free -// tier runs at most 1 instance, so a long cron serializes all other crons -// behind it and amplifies any DoS via the cron route. 60s is the budget; long -// crons must publish to PubSub and exit fast. -const defaultCronTimeout = 60 * time.Second