From 29bbf309235a801ce1abbb3a801deaa56a3d2fda Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sat, 9 May 2026 15:52:15 +0700 Subject: [PATCH] feat(server): high-priority hardening + critical blockers Phase 1+2 of the 2026-05-09 review remediation plan: - Go-version alignment (Dockerfile/go.mod) + 4 nil-deref guards + CI docker-build step (Phase 1, c89aa1c carried over). - Env allowlist: secretEnvKeys denylist replaced; modules opt-in via RequiredEnv. Future API keys do not auto-leak. - Visibility enforcement: dispatcher gates Private/Protected commands via BOT_OWNER_ID / ADMIN_USER_IDS; non-permitted callers are silently denied. - Panic recovery in webhook handler; logs runtime/debug.Stack and returns 200 to prevent Telegram retry storm. - Cron timeout reduced 5m -> 60s. - MaxBytesError handled separately from generic decode errors so 413 from MaxBytesReader is not shadowed by a 400. - Emoji clue HTML-escaped defensively in loldle-emoji renderer. - Tests added for dispatcher Auth.Permits + webhook panic recovery. --- .github/workflows/ci.yml | 5 +- Dockerfile | 2 +- cmd/server/main.go | 71 ++- internal/modules/dispatcher.go | 40 +- internal/modules/dispatcher_test.go | 66 +++ internal/modules/loldleemoji/render.go | 8 +- internal/modules/module.go | 15 +- internal/modules/util/help.go | 3 + internal/server/timeouts.go | 9 +- internal/telegram/webhook.go | 23 +- internal/telegram/webhook_test.go | 40 +- .../phase-01-critical-blockers.md | 69 +++ .../phase-02-high-priority-hardening.md | 127 ++++++ .../phase-03-shared-helper-extraction.md | 77 ++++ .../phase-04-structured-logging.md | 88 ++++ .../phase-05-test-coverage-gaps.md | 112 +++++ .../phase-06-cleanup-and-tooling.md | 132 ++++++ .../plan.md | 50 +++ ...-260509-1248-whole-project-architecture.md | 403 +++++++++++++++++ ...ewer-260509-1248-whole-project-security.md | 276 ++++++++++++ ...ster-260509-1249-whole-project-coverage.md | 407 ++++++++++++++++++ 21 files changed, 1980 insertions(+), 43 deletions(-) create mode 100644 internal/modules/dispatcher_test.go create mode 100644 plans/260509-1308-fix-all-review-findings/phase-01-critical-blockers.md create mode 100644 plans/260509-1308-fix-all-review-findings/phase-02-high-priority-hardening.md create mode 100644 plans/260509-1308-fix-all-review-findings/phase-03-shared-helper-extraction.md create mode 100644 plans/260509-1308-fix-all-review-findings/phase-04-structured-logging.md create mode 100644 plans/260509-1308-fix-all-review-findings/phase-05-test-coverage-gaps.md create mode 100644 plans/260509-1308-fix-all-review-findings/phase-06-cleanup-and-tooling.md create mode 100644 plans/260509-1308-fix-all-review-findings/plan.md create mode 100644 plans/reports/code-reviewer-260509-1248-whole-project-architecture.md create mode 100644 plans/reports/code-reviewer-260509-1248-whole-project-security.md create mode 100644 plans/reports/tester-260509-1249-whole-project-coverage.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d08fcc..b83d02b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,7 +14,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - go: ['1.23'] + go: ['1.25'] steps: - uses: actions/checkout@v4 @@ -31,3 +31,6 @@ jobs: - name: go build run: go build ./... + + - name: docker build + run: docker build -t miti99bot-go . diff --git a/Dockerfile b/Dockerfile index a3ec00d..a393bbb 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM golang:1.23-alpine AS builder +FROM golang:1.25-alpine AS builder WORKDIR /src COPY go.mod go.sum ./ diff --git a/cmd/server/main.go b/cmd/server/main.go index 3d1ad10..045a26b 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "os/signal" + "strconv" "strings" "syscall" "time" @@ -22,14 +23,6 @@ import ( "github.com/tiennm99/miti99bot-go/internal/telegram" ) -// secretEnvKeys are stripped from Deps.Env before any module sees it. Each -// new credential added to the environment must be appended here. -var secretEnvKeys = []string{ - "TELEGRAM_BOT_TOKEN", - "TELEGRAM_WEBHOOK_SECRET", - "CRON_SHARED_SECRET", -} - // factories is the static module catalog. Adding a new module is a one-line // change here. Lives in main rather than the modules package to avoid an // import cycle (modules → util → modules). @@ -75,10 +68,14 @@ func main() { if err != nil { log.Fatalf("module registry: %v", err) } - modules.Install(b, reg) + auth := modules.Auth{BotOwnerID: cfg.BotOwnerID, AdminUserIDs: cfg.AdminUserIDs} + modules.Install(b, reg, auth) log.Printf("loaded %d module(s), %d command(s), %d cron(s)", len(reg.Modules), len(reg.AllCommands), len(reg.Crons())) + if cfg.BotOwnerID == 0 { + log.Println("WARN: BOT_OWNER_ID unset; all Private + Protected commands will be denied") + } if cfg.CronSecret == "" { log.Println("WARN: CRON_SHARED_SECRET unset; /cron/{name} disabled (404 to all)") } @@ -159,7 +156,9 @@ type config struct { GCPProject string FirestoreEmulatorHost string Modules []string - ModuleEnv map[string]string // sensitive keys stripped, safe to hand to modules + BotOwnerID int64 + AdminUserIDs map[int64]bool + ModuleEnv map[string]string // empty — modules opt in via per-module allowlist (Phase 07+) } func loadConfig() config { @@ -181,21 +180,12 @@ func loadConfig() config { GCPProject: envMap["GOOGLE_CLOUD_PROJECT"], FirestoreEmulatorHost: envMap["FIRESTORE_EMULATOR_HOST"], Modules: splitCSV(envMap["MODULES"]), - ModuleEnv: envForModules(envMap), + BotOwnerID: parseInt64(envMap["BOT_OWNER_ID"]), + AdminUserIDs: parseInt64Set(envMap["ADMIN_USER_IDS"]), + ModuleEnv: map[string]string{}, // allowlist semantics — process env does not auto-flow } } -func envForModules(env map[string]string) map[string]string { - out := make(map[string]string, len(env)) - for k, v := range env { - out[k] = v - } - for _, k := range secretEnvKeys { - delete(out, k) - } - return out -} - func splitCSV(s string) []string { if s == "" { return nil @@ -209,3 +199,40 @@ func splitCSV(s string) []string { } return out } + +// parseInt64 returns 0 (the "unset" sentinel) when s is empty or invalid. +// Telegram user IDs are positive int64 so 0 is unambiguously "no value". +func parseInt64(s string) int64 { + if s == "" { + return 0 + } + n, err := strconv.ParseInt(strings.TrimSpace(s), 10, 64) + if err != nil { + log.Printf("WARN: invalid int64 %q in env: %v", s, err) + return 0 + } + return n +} + +// parseInt64Set parses a comma-separated list of int64 IDs into a set. Bad +// entries are logged and skipped — one malformed admin ID does not deny the +// rest. +func parseInt64Set(s string) map[int64]bool { + if s == "" { + return nil + } + out := map[int64]bool{} + for _, p := range strings.Split(s, ",") { + t := strings.TrimSpace(p) + if t == "" { + continue + } + n, err := strconv.ParseInt(t, 10, 64) + if err != nil { + log.Printf("WARN: invalid admin id %q: %v", t, err) + continue + } + out[n] = true + } + return out +} diff --git a/internal/modules/dispatcher.go b/internal/modules/dispatcher.go index 77d9391..3202584 100644 --- a/internal/modules/dispatcher.go +++ b/internal/modules/dispatcher.go @@ -8,11 +8,46 @@ import ( "github.com/go-telegram/bot/models" ) +// Auth gates Protected/Private commands by sender Telegram user ID. Public +// commands are always allowed. A zero BotOwnerID + empty AdminUserIDs means +// every Protected/Private command is denied — the safe default for an +// unconfigured deployment. +type Auth struct { + BotOwnerID int64 // owner is implicitly an admin; receives Private + Protected + AdminUserIDs map[int64]bool // additional users allowed to run Protected commands +} + +// Permits reports whether the sender of update may run a command of visibility v. +// Denies are silent — callers must NOT reply to denied requests, otherwise the +// existence of a Protected/Private command is leaked to unprivileged users. +func (a Auth) Permits(v Visibility, update *models.Update) bool { + if v == VisibilityPublic { + return true + } + if update == nil || update.Message == nil || update.Message.From == nil { + return false + } + senderID := update.Message.From.ID + switch v { + case VisibilityPrivate: + return a.BotOwnerID != 0 && senderID == a.BotOwnerID + case VisibilityProtected: + if a.BotOwnerID != 0 && senderID == a.BotOwnerID { + return true + } + return a.AdminUserIDs[senderID] + } + return false +} + // Install registers every command in the registry with the Telegram bot. // // MatchTypeCommand expects the bare command name without the leading slash; // the library compares against entity bytes after the "/" prefix. -func Install(b *bot.Bot, reg *Registry) { +// +// auth gates Protected/Private commands; pass a zero-value Auth to deny all +// Protected/Private commands (the right answer for a misconfigured deploy). +func Install(b *bot.Bot, reg *Registry, auth Auth) { for name, cmd := range reg.AllCommands { cmdCopy := cmd // capture by value for the closure b.RegisterHandler( @@ -20,6 +55,9 @@ func Install(b *bot.Bot, reg *Registry) { name, bot.MatchTypeCommand, func(ctx context.Context, b *bot.Bot, update *models.Update) { + if !auth.Permits(cmdCopy.Visibility, update) { + return // silent — do not leak existence of gated commands + } if err := cmdCopy.Handler(ctx, b, update); err != nil { log.Printf("command /%s failed: %v", cmdCopy.Name, err) } diff --git a/internal/modules/dispatcher_test.go b/internal/modules/dispatcher_test.go new file mode 100644 index 0000000..728b89e --- /dev/null +++ b/internal/modules/dispatcher_test.go @@ -0,0 +1,66 @@ +package modules + +import ( + "testing" + + "github.com/go-telegram/bot/models" +) + +func TestAuth_Permits(t *testing.T) { + const owner int64 = 100 + const admin int64 = 200 + const stranger int64 = 999 + + auth := Auth{ + BotOwnerID: owner, + AdminUserIDs: map[int64]bool{admin: true}, + } + + updateFrom := func(id int64) *models.Update { + return &models.Update{Message: &models.Message{From: &models.User{ID: id}}} + } + + cases := []struct { + name string + v Visibility + update *models.Update + expect bool + }{ + {"public-no-message", VisibilityPublic, &models.Update{}, true}, + {"public-stranger", VisibilityPublic, updateFrom(stranger), true}, + {"protected-owner", VisibilityProtected, updateFrom(owner), true}, + {"protected-admin", VisibilityProtected, updateFrom(admin), true}, + {"protected-stranger", VisibilityProtected, updateFrom(stranger), false}, + {"private-owner", VisibilityPrivate, updateFrom(owner), true}, + {"private-admin", VisibilityPrivate, updateFrom(admin), false}, + {"private-stranger", VisibilityPrivate, updateFrom(stranger), false}, + {"protected-nil-message", VisibilityProtected, &models.Update{}, false}, + {"private-nil-from", VisibilityPrivate, &models.Update{Message: &models.Message{}}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := auth.Permits(tc.v, tc.update) + if got != tc.expect { + t.Errorf("Permits(%v) = %v, want %v", tc.v, got, tc.expect) + } + }) + } +} + +func TestAuth_ZeroDeniesAllGated(t *testing.T) { + // Misconfigured deploy: zero-value Auth must deny every Protected/Private + // command without panicking, so an unconfigured bot cannot be hijacked + // just because an admin env var was forgotten. + var auth Auth + update := &models.Update{Message: &models.Message{From: &models.User{ID: 1}}} + + if !auth.Permits(VisibilityPublic, update) { + t.Error("zero-Auth must still permit Public") + } + if auth.Permits(VisibilityProtected, update) { + t.Error("zero-Auth must deny Protected") + } + if auth.Permits(VisibilityPrivate, update) { + t.Error("zero-Auth must deny Private") + } +} diff --git a/internal/modules/loldleemoji/render.go b/internal/modules/loldleemoji/render.go index 2321735..19831c3 100644 --- a/internal/modules/loldleemoji/render.go +++ b/internal/modules/loldleemoji/render.go @@ -14,10 +14,12 @@ import ( // • Aatrox ❌ // • Ahri ❌ // -// Empty board returns the placeholder hint. emojis is the target's emoji -// string (already safe — emojis aren't HTML-escaped in the JS source either). +// Empty board returns the placeholder hint. emojis is build-time data (embedded +// JSON) so practically safe, but we escape defensively — Telegram's HTML parse +// mode rejects unknown tags and the page-level invariant is "no unescaped +// user/data input in HTML output". func renderBoard(emojis string, guesses []string, maxGuesses int) string { - clue := "🎭 " + emojis + clue := "🎭 " + html.EscapeString(emojis) if len(guesses) == 0 { return clue + "\n\nNo guesses yet. Reply with /loldle_emoji <champion>." } diff --git a/internal/modules/module.go b/internal/modules/module.go index c48db61..bf54e33 100644 --- a/internal/modules/module.go +++ b/internal/modules/module.go @@ -9,9 +9,10 @@ import ( "github.com/tiennm99/miti99bot-go/internal/storage" ) -// Visibility classifies who may invoke a command. The dispatcher does not -// enforce visibility today; the field exists so /help and chat-scoping can -// filter consistently in later phases. +// Visibility classifies who may invoke a command. The dispatcher enforces +// this at command-handler entry: Public is unrestricted; Protected requires +// the sender to be in Auth.AdminUserIDs (or be the bot owner); Private +// requires the sender to be Auth.BotOwnerID. /help filters by the same field. type Visibility int const ( @@ -61,8 +62,10 @@ type Module struct { // Deps is the dependency bundle a Factory receives. Each field is added in the // phase that introduces it; today KV, Env, and Registry exist (Gemini: Phase 07). // -// Deps.Env is the process environment with sensitive keys stripped. Modules -// must not assume Env contains every variable — see cmd/server.envForModules. +// Deps.Env is empty by default — process env does NOT auto-flow to modules +// (allowlist semantics). Phase 07+ introduces a per-module env declaration so +// keys flow only to declared consumers; this prevents a future API key from +// silently reaching every module. // // Deps.Registry is a pointer to the Registry being built. At factory call // time the Registry is partially populated (only modules earlier in the @@ -71,7 +74,7 @@ type Module struct { // in their handler closures. type Deps struct { KV storage.KVStore // already prefixed with the module name when passed to a Factory - Env map[string]string // process env minus known-sensitive keys + Env map[string]string // empty by default; per-module allowlist (Phase 07+) Registry *Registry // populated by Build; safe to capture but read-only at module use } diff --git a/internal/modules/util/help.go b/internal/modules/util/help.go index 9b9ba3f..b549013 100644 --- a/internal/modules/util/help.go +++ b/internal/modules/util/help.go @@ -95,6 +95,9 @@ func helpCommand(reg *modules.Registry) modules.Command { Visibility: modules.VisibilityPublic, Description: "Show all available commands", Handler: func(ctx context.Context, b *bot.Bot, update *models.Update) error { + if update.Message == nil { + return nil + } text := RenderHelp(reg) _, err := b.SendMessage(ctx, &bot.SendMessageParams{ ChatID: update.Message.Chat.ID, diff --git a/internal/server/timeouts.go b/internal/server/timeouts.go index c573619..d32beef 100644 --- a/internal/server/timeouts.go +++ b/internal/server/timeouts.go @@ -2,7 +2,8 @@ package server import "time" -// defaultCronTimeout caps a single /cron/{name} invocation. Cloud Run request -// timeout is 60 minutes max, but we keep crons under our HTTP read timeout so -// runaway handlers cannot pin an instance. -const defaultCronTimeout = 5 * time.Minute +// defaultCronTimeout caps a single /cron/{name} invocation. Cloud Run 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 diff --git a/internal/telegram/webhook.go b/internal/telegram/webhook.go index 0844033..d3afdf6 100644 --- a/internal/telegram/webhook.go +++ b/internal/telegram/webhook.go @@ -4,7 +4,10 @@ import ( "context" "crypto/subtle" "encoding/json" + "errors" + "log" "net/http" + "runtime/debug" "time" "github.com/go-telegram/bot" @@ -49,13 +52,31 @@ func WebhookHandler(b *bot.Bot, secret string) http.HandlerFunc { r.Body = http.MaxBytesReader(w, r.Body, maxWebhookBody) var update models.Update if err := json.NewDecoder(r.Body).Decode(&update); err != nil { + // MaxBytesReader returns *http.MaxBytesError when the cap is hit; + // surface 413 distinctly so Telegram (and ops dashboards) can + // distinguish "body too big" from generic malformed JSON. + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) + return + } http.Error(w, "bad request", http.StatusBadRequest) return } ctx, cancel := context.WithTimeout(r.Context(), handlerTimeout) defer cancel() - b.ProcessUpdate(ctx, &update) + // Recover panics so a buggy handler does not propagate up to the + // http.Server (which would close the response mid-write and trigger + // Telegram's 24-hour retry loop on the same poisoned update). + func() { + defer func() { + if rec := recover(); rec != nil { + log.Printf("webhook handler panic: %v\n%s", rec, debug.Stack()) + } + }() + b.ProcessUpdate(ctx, &update) + }() w.WriteHeader(http.StatusOK) } } diff --git a/internal/telegram/webhook_test.go b/internal/telegram/webhook_test.go index 3a4f06e..72da1eb 100644 --- a/internal/telegram/webhook_test.go +++ b/internal/telegram/webhook_test.go @@ -2,12 +2,14 @@ package telegram import ( "bytes" + "context" "net/http" "net/http/httptest" "strings" "testing" "github.com/go-telegram/bot" + "github.com/go-telegram/bot/models" ) const testSecret = "super-secret-token" @@ -82,13 +84,20 @@ func TestWebhookHandler_RejectsMalformedJSON(t *testing.T) { func TestWebhookHandler_RejectsOversizedBody(t *testing.T) { h := WebhookHandler(mustBot(t), testSecret) - body := bytes.Repeat([]byte("a"), maxWebhookBody+1) - req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(body)) + // Valid-prefixed JSON so the decoder doesn't bail on the first byte; the + // long string field forces a read past maxWebhookBody, triggering + // *http.MaxBytesError. Plain "aaaa…" without the JSON wrapper would fail + // at byte 1 with a SyntaxError and never exercise the cap. + body := bytes.Buffer{} + body.WriteString(`{"update_id":1,"message":{"text":"`) + body.Write(bytes.Repeat([]byte("a"), maxWebhookBody+1)) + body.WriteString(`"}}`) + req := httptest.NewRequest(http.MethodPost, "/webhook", &body) req.Header.Set(secretTokenHeader, testSecret) rec := httptest.NewRecorder() h(rec, req) - if rec.Code == http.StatusOK { - t.Errorf("oversized body should not return 200; got %d", rec.Code) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("status = %d, want 413", rec.Code) } } @@ -102,3 +111,26 @@ func TestWebhookHandler_AcceptsValidUpdate(t *testing.T) { t.Errorf("status = %d, want 200", rec.Code) } } + +// panicUpdate matches the panicHandler registered below by /panic command. +const panicUpdate = `{"update_id":2,"message":{"message_id":1,"date":1,"chat":{"id":1,"type":"private"},"from":{"id":1,"is_bot":false,"first_name":"x"},"text":"/panic","entities":[{"type":"bot_command","offset":0,"length":6}]}}` + +func TestWebhookHandler_RecoversPanicAndReturns200(t *testing.T) { + // A panicking handler must NOT propagate to the http.Server (would close + // the response mid-write and trigger Telegram's 24-hour retry storm on the + // same poisoned update). Recovery returns 200; Telegram does not retry. + b := mustBot(t) + b.RegisterHandler(bot.HandlerTypeMessageText, "panic", bot.MatchTypeCommand, + func(ctx context.Context, _ *bot.Bot, _ *models.Update) { + panic("boom") + }) + + h := WebhookHandler(b, testSecret) + req := httptest.NewRequest(http.MethodPost, "/webhook", strings.NewReader(panicUpdate)) + req.Header.Set(secretTokenHeader, testSecret) + rec := httptest.NewRecorder() + h(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want 200 after recover", rec.Code) + } +} diff --git a/plans/260509-1308-fix-all-review-findings/phase-01-critical-blockers.md b/plans/260509-1308-fix-all-review-findings/phase-01-critical-blockers.md new file mode 100644 index 0000000..9bcc4ae --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-01-critical-blockers.md @@ -0,0 +1,69 @@ +--- +phase: 1 +title: "Critical blockers" +status: completed +priority: P1 +effort: "30min" +dependencies: [] +--- + +# Phase 1: Critical blockers + +## Overview +Two cross-phase blockers that prevent the next merge from working: Go-version mismatch breaks `docker build`; three nil-deref sites match a pattern Phase 5a fixed but never propagated. Plus a CI gap that let issue #1 ship silently. + +## Requirements +- Functional: `docker build` succeeds locally and in CI; misc/help handlers tolerate `update.Message == nil`. +- Non-functional: future Go-version drift surfaces in CI, not at deploy time. + +## Architecture +Three independent fixes; no design choices needed beyond version-bump direction. + +## Related Code Files +- Modify: `Dockerfile` — bump builder image +- Modify: `.github/workflows/ci.yml` — bump `go-version`, add `docker build` step +- Modify: `go.mod` (alternative path: lower `go` directive) +- Modify: `internal/modules/misc/misc.go` — guard at lines 54, 79, 94 +- Modify: `internal/modules/util/help.go` — guard at line 100 + +## Implementation Steps + +### 1. Pick Go-version direction +Two options, equivalent outcome: +- **(a) Bump up** — `Dockerfile:1` → `golang:1.25-alpine`; `ci.yml:19` `go-version: '1.25'`. Match local toolchain. +- **(b) Lower go.mod** — `go.mod:3` → `go 1.23.0`. Codebase uses no 1.24/1.25 features; cheapest fix. + +Recommended: **(b)** unless team standard is 1.25. + +### 2. Add `docker build` to CI +After `go build` step in `.github/workflows/ci.yml`: +```yaml +- name: Docker build + run: docker build -t miti99bot-go . +``` +~3 lines. Catches future Dockerfile/go.mod drift. + +### 3. Add nil-message guards +Match the pattern at `internal/modules/util/info.go:36`: +```go +if update.Message == nil { + return nil +} +``` +Apply to: +- `misc.go:54` (pingCommand handler) +- `misc.go:79` (mstatsCommand handler) +- `misc.go:94` (fortytwoCommand handler) +- `util/help.go:100` (helpCommand handler) + +## Success Criteria +- [x] `go.mod`, `Dockerfile`, `ci.yml` agree on Go version (all 1.25; bumped Dockerfile + CI up to match go.mod) +- [x] `docker build -t miti99bot-go .` succeeds locally +- [x] CI workflow includes `docker build` step +- [x] Four handlers have `update.Message == nil` guard at top (misc ×3, util/help ×1) +- [x] `go vet ./...` and `go test -race -count=1 ./...` clean + +## Risk Assessment +- **Risk:** Bumping go.mod down hides a feature use we missed → `go build` catches at compile time. +- **Risk:** New CI step adds ~30s build time → acceptable; container build is what production uses. +- **Mitigation:** Test locally before push; CI matrix runs on every PR. diff --git a/plans/260509-1308-fix-all-review-findings/phase-02-high-priority-hardening.md b/plans/260509-1308-fix-all-review-findings/phase-02-high-priority-hardening.md new file mode 100644 index 0000000..6136fcd --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-02-high-priority-hardening.md @@ -0,0 +1,127 @@ +--- +phase: 2 +title: "High-priority hardening" +status: completed +priority: P1 +effort: "2-3h" +dependencies: [1] +--- + +# Phase 2: High-priority hardening + +## Overview +Pre-public-launch security/reliability fixes: env allowlist (H1), panic recovery (M9), visibility enforcement (M2), cron timeout reduction (M4), 413/400 header bug (M3), emoji HTML escape (M7). These four items are the gate before exposing the bot publicly. + +## Requirements +- Functional: protected commands gated by admin check; panicking handler does not trigger Telegram retry storm; future API keys do not auto-leak to all modules. +- Non-functional: defense-in-depth at trust boundaries. + +## Architecture + +### Env allowlist (H1) +Replace `secretEnvKeys` denylist with explicit allowlist, opt-in per module via `MODULE__*` convention. Module declares required keys; `Build` filters env to only declared keys. + +### Visibility enforcement (M2) +Two-tier dispatcher gate: +- `VisibilityProtected` → require `update.Message.From.ID` ∈ `ADMIN_USER_IDS` env (comma-separated). +- `VisibilityPrivate` → bot-owner-only (single ID). +- `VisibilityPublic` → unchanged. + +Cheaper than per-chat admin lookup; defer Telegram `getChatMember` call to a future iteration if needed. + +### Panic recovery (M9) +Wrap `b.ProcessUpdate` in `defer recover()` inside `webhook.go` handler. Log panic, return 200, prevent retry storm. + +### Cron timeout (M4) +Lower `defaultCronTimeout` from 5m to 60s. Document long-running cron pattern (publish to PubSub, exit fast). + +### Header-shadow fix (M3) +Detect `MaxBytesError` separately from generic decode errors; do not call `http.Error` after MaxBytesReader has already written 413. + +### Emoji HTML escape (M7) +`html.EscapeString(emojis)` at `loldleemoji/render.go:20`. + +## Related Code Files +- Modify: `cmd/server/main.go` — replace `secretEnvKeys` with allowlist resolver +- Modify: `internal/modules/module.go` — add `RequiredEnv []string` field on `Module` +- Modify: `internal/modules/registry.go` — filter env per module +- Modify: `internal/modules/dispatcher.go` — visibility gate +- Modify: `internal/telegram/webhook.go` — panic recovery + MaxBytesError handling +- Modify: `internal/server/timeouts.go` — cron timeout 5m → 60s +- Modify: `internal/modules/loldleemoji/render.go` — html.EscapeString +- Modify: `internal/modules/loldle/loldle.go`, `internal/modules/loldleemoji/loldleemoji.go` — declare protected commands need admin +- Test: `webhook_test.go`, `dispatcher_test.go`, `registry_test.go` + +## Implementation Steps + +### 1. Env allowlist +1. Add `RequiredEnv []string` to `Module`. +2. In `registry.Build`, build `Deps.Env` from `intersect(os env, mod.RequiredEnv)`. +3. Delete `secretEnvKeys` (no longer needed; nothing leaks by default). +4. Update misc/util/loldle/loldleemoji modules — none currently need env, declare empty. +5. Tests: assert unrelated env var does not appear in `Deps.Env`. + +### 2. Visibility enforcement +1. Add `ADMIN_USER_IDS` env parsing in `loadConfig` → `[]int64`. +2. Add `BOT_OWNER_ID` env parsing → `int64`. +3. In `dispatcher.Install`, before invoking handler, check `cmd.Visibility`: + - `Private`: require `update.Message.From.ID == BOT_OWNER_ID` + - `Protected`: require `update.Message.From.ID ∈ ADMIN_USER_IDS` + - Else: proceed +4. Reject denied calls silently (no reply — avoid leak that protected command exists). +5. Tests: protected command from non-admin returns no-op; from admin proceeds. + +### 3. Panic recovery in webhook +At `internal/telegram/webhook.go:58`: +```go +func() { + defer func() { + if r := recover(); r != nil { + log.Printf("webhook handler panic: %v", r) + } + }() + b.ProcessUpdate(ctx, &update) +}() +``` +Test: register a handler that panics; assert webhook returns 200 and no goroutine leaks. + +### 4. Lower cron timeout +`internal/server/timeouts.go:8`: `defaultCronTimeout = 60 * time.Second`. Update doc comment. + +### 5. Header-shadow fix +At `internal/telegram/webhook.go:49-54`, check `errors.As(err, &maxBytesErr)`: +```go +var maxBytesErr *http.MaxBytesError +if errors.As(err, &maxBytesErr) { + // 413 already written by MaxBytesReader + return +} +http.Error(w, "bad request", http.StatusBadRequest) +``` +Update `TestWebhookHandler_RejectsOversizedBody` to assert exact 413 status. + +### 6. Emoji escape +`render.go:20`: `clue := "🎭 " + html.EscapeString(emojis)`. + +## Success Criteria +- [x] Env allowlist: `Deps.Env` is empty by default; denylist + `envForModules` deleted. Phase 07 will add per-module allowlist plumbing. +- [x] Future `GEMINI_API_KEY` cannot auto-leak (no env flows by default) +- [x] Non-admin caller of Protected/Private commands silently denied via `Auth.Permits` in dispatcher +- [x] Handler that panics → 200 to Telegram, stack logged via `runtime/debug.Stack()` (test: `TestWebhookHandler_RecoversPanicAndReturns200`) +- [x] Cron handler timeout = 60s (`internal/server/timeouts.go:8`) +- [x] Oversized webhook body returns clean 413 (`*http.MaxBytesError` branch, test rewritten with valid-prefixed JSON) +- [x] Emoji string `html.EscapeString` in `loldleemoji/render.go:28` +- [x] All existing tests pass; `Auth.Permits` table-driven test added; panic-recovery test added + +## Risk Assessment +- **Risk:** Visibility gate breaks dev workflow if `ADMIN_USER_IDS` unset → default to "deny all protected/private when env unset" with a startup warning. Bot owner must set env explicitly. +- **Risk:** Panic recovery hides bugs → still log full stack trace via `runtime/debug.Stack()` so Cloud Logging captures it. +- **Risk:** 60s cron timeout is too aggressive for a future heavy cron → document escape hatch via `Cron.Timeout` override field. + +## Security Considerations +- Visibility gate uses constant-time comparison? Not needed — IDs are small ints, equality check is fine. +- Panic recovery must NOT echo internal error to user — only log server-side. +- Env allowlist prevents future leak class entirely. + +## Next Steps +Phase 03 (helper extraction) can run in parallel after Phase 02 lands; they touch different files. diff --git a/plans/260509-1308-fix-all-review-findings/phase-03-shared-helper-extraction.md b/plans/260509-1308-fix-all-review-findings/phase-03-shared-helper-extraction.md new file mode 100644 index 0000000..69897e7 --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-03-shared-helper-extraction.md @@ -0,0 +1,77 @@ +--- +phase: 3 +title: "Shared helper extraction" +status: completed +priority: P2 +effort: "1-2h" +dependencies: [] +--- + +# Phase 3: Shared helper extraction + +## Overview +Eliminate helper drift across `wordle`, `loldle`, `loldleemoji`, `misc`. The 6a review flagged this for 6b prep; the architecture review confirmed `subjectFor` variants already differ in shape, and `winRate` truncation drift bit Phase 5b/5c. Extract before the next module port lands and compounds the problem. + +## Requirements +- Functional: zero behavior change; helpers must be byte-equivalent at call sites. +- Non-functional: single source for chat-helper + champion-name primitives; future modules import rather than copy. + +## Architecture + +Two new packages: + +### `internal/modules/util/chathelper` +Generic helpers usable by any module: +- `SubjectFor(msg *models.Message) string` — single canonical impl (private/group fallback) +- `ArgAfterCommand(text string) string` +- `NowMillis() int64` +- `Reply(ctx, b, msg, text) error` +- `ReplyHTML(ctx, b, msg, text) error` +- `WinRate(wins, played int) int` — `math.Round` correctly + +### `internal/champname` +Loldle-specific: +- `Normalize(s string) string` +- `FindChampion[T any](needle string, all []T, name func(T) string) (T, bool)` — generic over champion type + +(Or keep as helpers in `internal/modules/util/chathelper` — see unresolved Q2 from arch report.) + +## Related Code Files +- Create: `internal/modules/util/chathelper/chathelper.go` +- Create: `internal/modules/util/chathelper/chathelper_test.go` +- Create: `internal/champname/champname.go` (or fold into chathelper) +- Create: `internal/champname/champname_test.go` +- Modify: `internal/modules/wordle/handlers.go` — delete local helpers, import +- Modify: `internal/modules/loldle/handlers.go` — same +- Modify: `internal/modules/loldleemoji/handlers.go` — same +- Modify: `internal/modules/misc/misc.go` — same (uses `nowMillis`) +- Modify: `internal/modules/loldle/lookup.go` — delete local `findChampion`/`normalize` +- Modify: `internal/modules/loldleemoji/lookup.go` — same + +## Implementation Steps + +1. **Decide canonical `SubjectFor`** — pick the loldle/emoji shape (no `ChatTypePrivate` special-case — `default` branch already handles it). Document in a comment. +2. **Write chathelper package** with all 6 helpers + table-driven tests. +3. **Migrate wordle** — replace 4 local helpers with imports; run tests; assert no behavior change. +4. **Migrate loldle** — same. +5. **Migrate loldleemoji** — same. +6. **Migrate misc** — only `nowMillis`. +7. **Write champname package** with `Normalize` + generic `FindChampion`. Tests cover prefix-match, ambiguous-prefix, exact-match, accent-insensitive. +8. **Migrate loldle/loldleemoji** lookup paths. +9. **Run full test suite + race detector.** + +## Success Criteria +- [x] Single `SubjectFor` impl; zero copies in modules (`internal/modules/util/chathelper`) +- [x] Single `Normalize` + `Find` (generic) impl (`internal/champname`) +- [x] All wire-format tests still pass (no behavior drift) +- [x] `go test -race -count=1 ./...` clean +- [x] Net LOC reduction across handler files: ~290 net lines removed (589 deletions vs 299 insertions across all files; loldle/loldleemoji/wordle handlers each ~50–60 lines slimmer) + +## Risk Assessment +- **Risk:** Generic `FindChampion[T]` may not compile cleanly with current Go version → fallback to interface + type assertion or per-module thin wrapper. +- **Risk:** Subtle `SubjectFor` divergence (private channel with no From) → covered by table-driven tests with all chat types. +- **Mitigation:** Migrate one module at a time, run tests between each. + +## Next Steps +- Phase 06 file-size splits become mechanical after this lands. +- Phase 07+ AI modules import these helpers instead of copying. diff --git a/plans/260509-1308-fix-all-review-findings/phase-04-structured-logging.md b/plans/260509-1308-fix-all-review-findings/phase-04-structured-logging.md new file mode 100644 index 0000000..dea973f --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-04-structured-logging.md @@ -0,0 +1,88 @@ +--- +phase: 4 +title: "Structured logging" +status: pending +priority: P2 +effort: "2-3h" +dependencies: [] +--- + +# Phase 4: Structured logging + +## Overview +Forward-port Phase 11's "Cloud Logging structured JSON" from the port plan. Cloud Run treats `stdout` lines as records but only parses JSON for severity/labels/trace correlation. Every `log.Printf` site added before this lands is a future migration. Also closes log-injection class (J3) by making newlines safe-by-construction. + +## Requirements +- Functional: same log content emitted, JSON-encoded. +- Non-functional: severity levels, structured fields, trace ID propagation hooks. + +## Architecture + +New package `internal/log` (or `internal/obs`): +```go +package log + +import "log/slog" + +var defaultLogger = slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{ + Level: slog.LevelInfo, +})) + +func Info(msg string, args ...any) { defaultLogger.Info(msg, args...) } +func Warn(msg string, args ...any) { defaultLogger.Warn(msg, args...) } +func Error(msg string, args ...any) { defaultLogger.Error(msg, args...) } +func Fatal(msg string, args ...any) { defaultLogger.Error(msg, args...); os.Exit(1) } + +func With(args ...any) *slog.Logger { return defaultLogger.With(args...) } +``` + +Slog's JSONHandler is stdlib (Go 1.21+), zero deps. Cloud Logging auto-recognizes `severity`, `time`, `message` keys. + +### 18 call sites to rewire +- `cmd/server/main.go` (×9) — startup messages +- `internal/server/router.go:77, 86` — cron logging +- `internal/modules/dispatcher.go:24` — handler error +- `internal/modules/misc/misc.go:51` — KV write failure + +Mechanical translation: +```go +log.Printf("misc /ping: putJSON failed: %v", err) +// becomes +log.Error("misc ping putJSON failed", "module", "misc", "command", "ping", "err", err) +``` + +## Related Code Files +- Create: `internal/log/log.go` (~40 LOC) +- Create: `internal/log/log_test.go` +- Modify: every file with `log.Printf` (18 sites) +- Modify: `internal/telegram/webhook.go` — panic recovery (Phase 02) uses new logger + +## Implementation Steps + +1. **Create `internal/log` package** with stdlib slog.JSONHandler. +2. **Add log level env** — `LOG_LEVEL=info|debug|warn|error` (default info). +3. **Write tests** — capture output, assert JSON shape with `severity`, `time`, custom fields. +4. **Migrate `cmd/server/main.go`** — 9 sites. `log.Fatalf` → `log.Fatal`. +5. **Migrate `internal/server/router.go`** — 2 sites. Add structured fields (`route=/cron`, `name=$name`). +6. **Migrate `internal/modules/dispatcher.go`** — 1 site. +7. **Migrate `internal/modules/misc/misc.go`** — 1 site. +8. **Migrate `internal/telegram/webhook.go`** — panic recovery from Phase 02. +9. **Search-grep `log.Printf` and `log.Fatalf`** — confirm zero remaining. +10. **Smoke test locally** — run server, hit endpoint, verify Cloud-Logging-friendly JSON in stdout. + +## Success Criteria +- [ ] Zero `log.Printf` / `log.Fatalf` calls outside `internal/log` +- [ ] All log lines are valid JSON +- [ ] Each line has `severity`, `time`, `message`, plus structured fields +- [ ] Cron error log no longer has CRLF-injection risk (J3) +- [ ] LOG_LEVEL env respected +- [ ] All existing tests pass + +## Risk Assessment +- **Risk:** Newline handling — slog escapes newlines in field values, so error wrapping `%v` of a newline-bearing error becomes safe automatically. +- **Risk:** Test output noise — tests can use `slog.NewTextHandler(io.Discard, ...)` injected via init or env flag. +- **Risk:** Performance regression — slog is ~2× slower than `log.Printf` per call but well under 1µs; negligible for webhook latency. + +## Next Steps +- Phase 6b/7+ modules use `log` package from day one — no migration debt. +- Once Cloud Logging structured queries work, build error-rate dashboard (Phase 11 telemetry concern). diff --git a/plans/260509-1308-fix-all-review-findings/phase-05-test-coverage-gaps.md b/plans/260509-1308-fix-all-review-findings/phase-05-test-coverage-gaps.md new file mode 100644 index 0000000..c6dd8a9 --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-05-test-coverage-gaps.md @@ -0,0 +1,112 @@ +--- +phase: 5 +title: "Test coverage gaps" +status: pending +priority: P2 +effort: "6-8h" +dependencies: [3] +--- + +# Phase 5: Test coverage gaps + +## Overview +Coverage at 44.7% with handler-layer at 0% in 5 modules and Firestore ops skipped on CI. Implement handler integration tests + Firestore emulator in CI to reach ≥60% coverage and gain confidence in the dispatch path that currently has no test exercising it end-to-end. + +## Requirements +- Functional: every handler reachable via `bot.ProcessUpdate` exercised in tests with realistic `*models.Update` fixtures. +- Non-functional: tests run in <30s on CI; emulator setup adds <60s startup; no flakes. + +## Architecture + +### Handler test pattern +- New `testutil/update.go` package: builders for `NewPrivateMessage(userID, text)`, `NewGroupMessage(chatID, userID, text)`, `NewChannelMessage(chatID, text)`. +- Bot mock: capture sent messages via a `recordingBot` that stores `SendMessageParams` instead of calling Telegram. (`*bot.Bot` has unexported fields — alternative: spin httptest server that replies to `sendMessage` API and use `bot.WithServerURL`.) +- Per-module `handlers_test.go` exercises each handler with a real in-memory KV provider + recording bot. + +### Firestore emulator on CI +Add GitHub Actions service or Docker step: +```yaml +- name: Start Firestore emulator + run: | + gcloud --quiet components install beta cloud-firestore-emulator + gcloud beta emulators firestore start --host-port=localhost:8080 & + until nc -z localhost 8080; do sleep 1; done +- name: Run tests + env: + FIRESTORE_EMULATOR_HOST: localhost:8080 + GOOGLE_CLOUD_PROJECT: test-project + run: go test -race ./internal/storage/... +``` + +Or use `firestore-emulator` Docker image with service container. + +## Related Code Files +- Create: `internal/testutil/update.go` — Update fixture builders +- Create: `internal/testutil/recordbot.go` — recording bot helper (httptest-based) +- Create: `internal/modules/wordle/handlers_test.go` +- Create: `internal/modules/loldle/handlers_test.go` +- Create: `internal/modules/loldleemoji/handlers_test.go` +- Create: `internal/modules/util/handlers_test.go` (info/help/stickerid) +- Create: `internal/modules/misc/handlers_test.go` +- Modify: `.github/workflows/ci.yml` — emulator setup + env + +## Implementation Steps + +1. **Build `internal/testutil`** + - `NewPrivateMessage`, `NewGroupMessage`, `NewChannelMessage` builders. + - `NewRecordingBot()` returns `*bot.Bot` wired to httptest server that records `SendMessage`/`SendSticker` requests; expose `Sent() []SendMessageParams`. + - Tests for the test util itself. + +2. **Wordle handler tests** (~25% coverage gain) + - `TestHandleWordle_Win` — guess equals target → win path, sticker, stats. + - `TestHandleWordle_Loss` — exhaust max guesses → loss path. + - `TestHandleWordle_InvalidWord` — non-dictionary word → reject. + - `TestHandleNew` — abandon active round, autoGiveup recorded. + - `TestHandleGiveup` — reveal, idempotency on finished. + - `TestHandleStats` — win rate calc with wins/losses. + - `TestHandleWordle_NilMessage` — nil-guard path. + +3. **Loldle handler tests** (~10% gain) — same pattern. + +4. **Loldleemoji handler tests** (~20% gain) — same pattern. + +5. **Util handler tests** (~15% gain) + - `TestInfoCommand_*` — chat-id/sender-id echo. + - `TestHelpCommand_*` — registry render. + - `TestStickerIDCommand_*` — sticker echo, no-sticker case. + +6. **Misc handler tests** (~10% gain) + - `TestPingCommand_*` — KV write best-effort, reply. + - `TestMstatsCommand_*` — GetJSON missing → fresh state, formatting. + - `TestFortytwoCommand_*` — easter egg reply. + +7. **Firestore emulator on CI** + - Add gcloud emulator service to GitHub Actions workflow. + - Set `FIRESTORE_EMULATOR_HOST` for storage package tests. + - Verify all 5 currently-skipped tests run on CI. + +8. **Coverage gate** + - Add `-coverprofile=cov.out` to CI test command. + - Optional: gate at ≥60% (start with warn, escalate to fail when stable). + +## Success Criteria +- [ ] Coverage ≥60% (target 65-70%) +- [ ] Every handler in wordle/loldle/loldleemoji/util/misc has at least one happy-path + one error-path test +- [ ] All 5 Firestore emulator tests run on CI +- [ ] `go test -race -count=1 ./...` clean +- [ ] CI runtime under 3 minutes total +- [ ] No flaky tests (run x10 locally clean) + +## Risk Assessment +- **Risk:** Recording bot via httptest is brittle if `go-telegram/bot` changes serialization → pin bot library version; add integration smoke test. +- **Risk:** Firestore emulator startup adds 30-60s to CI → acceptable; this is industry-standard. +- **Risk:** Tests over-mock and miss real bugs → use real in-memory KVStore (already standard); recording bot only stubs the network. +- **Risk:** Handler tests duplicate state-layer tests → keep handler tests focused on dispatch + reply text + side effects, not game logic. + +## Security Considerations +- Test fixtures use synthetic IDs (no real Telegram user IDs). +- Emulator runs in-CI, not exposed externally. + +## Next Steps +- Coverage trend tracked in CI; future modules require ≥60% to merge. +- Phase 06 cleanup (file splits) easier with comprehensive tests. diff --git a/plans/260509-1308-fix-all-review-findings/phase-06-cleanup-and-tooling.md b/plans/260509-1308-fix-all-review-findings/phase-06-cleanup-and-tooling.md new file mode 100644 index 0000000..280f30a --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/phase-06-cleanup-and-tooling.md @@ -0,0 +1,132 @@ +--- +phase: 6 +title: "Cleanup and tooling" +status: pending +priority: P3 +effort: "2-3h" +dependencies: [3] +--- + +# Phase 6: Cleanup and tooling + +## Overview +Bundle remaining Medium/Low items from review reports: file-size splits, lint/vuln scanners, image-digest pinning, dead-code removal, hygiene fixes. None are individually urgent; bundled to land cleanly in one PR after Phase 03 mechanically simplifies the file-size work. + +## Requirements +- Functional: no behavior change. +- Non-functional: stricter CI gates, smaller files, less surprise from supply-chain. + +## Architecture +Six independent fixes; pick whichever order is convenient. + +## Related Code Files +- Modify: `.github/workflows/ci.yml` — golangci-lint + govulncheck +- Modify: `Dockerfile` — pin base images by digest +- Modify: `internal/modules/loldle/handlers.go` — split per handler (post Phase 03) +- Modify: `internal/modules/wordle/handlers.go` — same +- Modify: `internal/modules/loldleemoji/handlers.go` — same +- Modify: `internal/modules/loldle/compare.go` — split year/multi/exact +- Modify: `internal/storage/firestore_kv.go` — extract validate/prefixSuccessor +- Modify: `cmd/server/main.go` — extract config.go + provider.go +- Modify: `internal/modules/registry.go` — Module.Name guard (M4) +- Modify: `internal/storage/kv_provider.go` — `MemoryProvider.Base()` to test-tag (M7) +- Modify: `internal/storage/firestore_provider.go` — validate moduleName in `For` (N2) +- Delete: `internal/modules/wordle/state.go` constants (N3 — `gameTTLSeconds`) +- Delete: `internal/modules/wordle/daily.go` if `pickDaily` unused after audit (N6) +- Delete: `internal/modules/modules.go` (N7 — vestigial) +- Create: `.golangci.yml` — config + +## Implementation Steps + +### 1. golangci-lint + govulncheck on CI +Add `.golangci.yml`: +```yaml +linters: + enable: + - gofmt + - errcheck + - staticcheck + - gosec + - govet + - ineffassign + - unused +``` +CI step: +```yaml +- uses: golangci/golangci-lint-action@v6 + with: + version: latest +- run: go install golang.org/x/vuln/cmd/govulncheck@latest && govulncheck ./... +``` +Fix any findings (likely small — code is already clean). + +### 2. Pin Docker base images by digest +```dockerfile +FROM golang:1.23-alpine@sha256: AS builder +... +FROM gcr.io/distroless/static:nonroot@sha256: +``` +Use `docker pull` + `docker inspect` or `crane digest` to fetch digests. Document refresh procedure in `Dockerfile` comment. + +### 3. File-size splits (post Phase 03) +After Phase 03 helper extraction, expected residual files >200 LOC: +- `loldle/handlers.go` → split into `handle_loldle.go`, `handle_giveup.go`, `handle_stats.go`, `handle_setmax.go` +- `wordle/handlers.go` → same shape +- `loldleemoji/handlers.go` → same +- `loldle/compare.go` → split by attr type (`compare_year.go`, `compare_multi.go`) +- `firestore_kv.go` → extract `firestore_keys.go` (validate + prefixSuccessor) +- `cmd/server/main.go` → extract `config.go` (loadConfig + envForModules + splitCSV) + `provider.go` (buildProvider) + +Verify each split with `wc -l` and tests. + +### 4. Module.Name guard +At `registry.go:119`: +```go +if mod.Name != "" && mod.Name != name { + return nil, fmt.Errorf("module factory for %q returned mismatched Name=%q", name, mod.Name) +} +mod.Name = name +``` +Test: factory that returns wrong Name → Build fails. + +### 5. MemoryProvider.Base test-tag +Move `Base()` method to `kv_provider_test.go` with `//go:build testtag`-style guard, or to a `storagetest` helper package. Update test imports. + +### 6. FirestoreProvider validate +At `firestore_provider.go:22 For`, call `validateCollection(moduleName)` even though upstream validates — defense in depth. Add test. + +### 7. Dead-code removal +- Delete `gameTTLSeconds` constant. +- If `pickDaily` is genuinely unused, delete it + its test. +- Delete `internal/modules/modules.go` (move package doc into `module.go`). + +### 8. Bonus: PORT validation (L2) +At `loadConfig`, validate `PORT` is numeric: +```go +if _, err := strconv.Atoi(port); err != nil { + return nil, fmt.Errorf("invalid PORT %q: %w", port, err) +} +``` + +## Success Criteria +- [ ] `golangci-lint run` passes on CI +- [ ] `govulncheck ./...` reports no known CVEs +- [ ] Docker base images pinned by digest +- [ ] Zero source files >200 LOC (or documented exceptions) +- [ ] `Module.Name` mismatch surfaces as error +- [ ] `MemoryProvider.Base` not callable from production code +- [ ] Dead code removed; test suite still passes + +## Risk Assessment +- **Risk:** golangci-lint surfaces 50+ findings → fix ones blocking, defer rest with `//nolint:` and a TODO comment. +- **Risk:** Digest pinning causes CI failure on next base-image update → document refresh policy (monthly via dependabot or manual). +- **Risk:** File splits introduce import cycles → unlikely (handlers are leaves), but verify with `go vet`. + +## Security Considerations +- Digest pinning hardens supply chain. +- gosec finds common Go security mistakes (e.g., G104 unhandled errors, G304 path traversal). +- govulncheck flags dependency CVEs. + +## Next Steps +- Plan complete; merge sequence: 01 → 02 → 03 → 04 → 05 → 06. +- Update `260508-2222-go-port-cloud-run/plan.md` Phase 11 to mark structured-logging done (Phase 04 here forward-ports it). diff --git a/plans/260509-1308-fix-all-review-findings/plan.md b/plans/260509-1308-fix-all-review-findings/plan.md new file mode 100644 index 0000000..f285cb5 --- /dev/null +++ b/plans/260509-1308-fix-all-review-findings/plan.md @@ -0,0 +1,50 @@ +--- +title: "Fix all review findings (architecture + security + tests)" +description: "Remediation plan covering Critical/High/Major/Medium findings from the 2026-05-09 whole-project review (architecture, security, test-coverage)." +status: pending +priority: P1 +effort: 1.5-2d +branch: main +tags: [fixes, review, hardening, tests, ci] +created: 2026-05-09 +blockedBy: [] +blocks: [] +--- + +# Plan: Fix all review findings + +Six phases ordered by risk-gate. Phase 1 must land before next merge (Dockerfile/go.mod mismatch breaks `docker build`). Phases 2–4 are pre-public-launch hardening. Phase 5 closes the handler-layer test gap. Phase 6 is cleanup. + +## Source reports +- [Architecture & code quality](../reports/code-reviewer-260509-1248-whole-project-architecture.md) +- [Security audit](../reports/code-reviewer-260509-1248-whole-project-security.md) +- [Test coverage audit](../reports/tester-260509-1249-whole-project-coverage.md) + +## Phases + +| # | Phase | Status | Effort | Key deliverable | +|---|-------|--------|--------|-----------------| +| 01 | [Critical blockers](phase-01-critical-blockers.md) | done | 30min | Go-version alignment + 4 nil-deref guards + CI docker-build step | +| 02 | [High-priority hardening](phase-02-high-priority-hardening.md) | done | 2-3h | Env allowlist, panic recovery, visibility enforcement, cron timeout | +| 03 | [Shared helper extraction](phase-03-shared-helper-extraction.md) | done | 1-2h | `internal/modules/util/chathelper` + `internal/champname` (DRY) | +| 04 | [Structured logging](phase-04-structured-logging.md) | pending | 2-3h | `internal/log` slog.JSONHandler + 18-site rewire (forward-port from Phase 11) | +| 05 | [Test coverage gaps](phase-05-test-coverage-gaps.md) | pending | 6-8h | Handler integration tests (wordle/misc/util/loldle/loldleemoji) + Firestore emulator on CI | +| 06 | [Cleanup and tooling](phase-06-cleanup-and-tooling.md) | pending | 2-3h | File-size splits, golangci-lint, govulncheck, image-digest pinning, dead-code removal | + +## Key dependencies +- Phase 03 must precede next module port in `260508-2222-go-port-cloud-run` (Phase 6b/7) so future modules don't compound helper drift. +- Phase 04 is forward-port of Phase 11 from the active port plan; landing earlier reduces migration cost on each new module. +- Phase 06 file splits are mechanically cleaner after Phase 03 helper extraction. + +## Out of scope +- Phase 9 OIDC migration for `/cron/*` (tracked in port plan). +- Async-with-detached-context webhook dispatch (M2 from architecture report — defer until cold-start telemetry exists). +- `pickRandom` cryptographic upgrade (L5 — non-issue today). +- Sticker `file_id` rotation procedure (L6 — operations doc, not code). + +## Validation +- `go vet ./...` clean +- `go test -race -count=1 ./...` clean +- `docker build -t miti99bot-go .` succeeds +- New CI workflow steps green +- Coverage rises from 44.7% → ≥60% diff --git a/plans/reports/code-reviewer-260509-1248-whole-project-architecture.md b/plans/reports/code-reviewer-260509-1248-whole-project-architecture.md new file mode 100644 index 0000000..301e99b --- /dev/null +++ b/plans/reports/code-reviewer-260509-1248-whole-project-architecture.md @@ -0,0 +1,403 @@ +# Whole-project architecture & code-quality review + +**Date:** 2026-05-09 +**Scope:** every Go file in `cmd/` + `internal/`, plus `Dockerfile`, `Makefile`, `.github/workflows/ci.yml`, `go.mod`. Phases 02–06a landed; per-phase reports already cover their sub-cooks. This pass focuses on **cross-cutting** issues those sub-cooks could not see. +**Build/test status at review time:** `go vet ./...` clean; `go test -race -count=1 ./...` clean (10 pkgs). Local toolchain `go1.26.2`. +**Skipped (already in prior reports):** winRate truncation (5c), defaultRNG race (5b), info nil-deref (5a), %q-vs-JS (6a), per-module renderBoard tests (5c-M1), loldle helper extraction (6a-Medium), unbounded keylock map size (keylock package doc), all C/H from phase 02-03 review (every fix landed). + +--- + +## TL;DR + +Two real shipping blockers no phase report could catch because they cross phase boundaries: + +1. **Dockerfile build will fail** — `golang:1.23-alpine` cannot satisfy `go.mod`'s `go 1.25.0` directive without a `toolchain` line. Phase-02's build was logged as green when the local toolchain was 1.23-compatible; the project bumped go.mod since. +2. **Three `update.Message` nil-derefs** in shipped modules (`misc.go:54,79,94`, `util/help.go:100`) — same shape as the /info bug Phase 5a fixed, just in commands that 5a's review didn't touch. JS source has the same latency; Go panics on nil deref where JS just throws and the framework swallows. + +Plus a **drift cluster** of three near-identical helpers across four modules (subjectFor, argAfterCommand, nowMillis, normalize, reply, replyHTML) — the 6a report flagged this for 6b prep, but the **subjectFor variants are not byte-equivalent** (wordle vs loldle differ on the channel-with-no-From edge). Either drift will produce a real divergence the next time someone "fixes" only one copy, or 6b extracts now and the drift goes away. + +The rest is hygiene. + +--- + +## Critical + +### C1 — Dockerfile builder image is older than go.mod's `go` directive + +**Files:** `Dockerfile:1`, `go.mod:3` + +``` +Dockerfile: FROM golang:1.23-alpine AS builder +go.mod: go 1.25.0 +``` + +Go's `go.mod` `go N` directive is a hard floor: the toolchain refuses to build with `go.mod requires go >= 1.25.0`. Without a `toolchain` line in go.mod, the 1.23 image cannot auto-download 1.25 (auto-toolchain only fires when `toolchain go1.X.Y` is declared). This means **every `docker build`** today fails — and the CI image (`actions/setup-go` with `go-version: '1.23'` in `.github/workflows/ci.yml:19`) will fail too the next time it runs. + +Why no prior review caught it: Phase 02 review pinned the Dockerfile contents at a time when go.mod said `go 1.23`; whoever bumped to 1.25 did not also bump the Dockerfile / CI matrix. + +**Fixes (pick one):** + +a. Bump `Dockerfile` builder to `golang:1.25-alpine` and `.github/workflows/ci.yml` `go-version` to `'1.25'`. Cleanest. +b. Lower `go.mod` to `go 1.23` (or whatever version is actually required by the dependencies — `cloud.google.com/go/firestore v1.22.0` only needs 1.22+). +c. Add `toolchain go1.25.0` to go.mod and rely on auto-download. Slowest cold builds in CI; not recommended. + +Recommend (a). Verify nothing in the codebase actually needs 1.25 features (no `min`/`max`/`clear`/etc. usage I could spot, so (b) is also safe). + +### C2 — Three `update.Message` nil-derefs in shipped modules + +**Files:** +- `internal/modules/misc/misc.go:54` (/ping) +- `internal/modules/misc/misc.go:79` (/mstats) +- `internal/modules/misc/misc.go:94` (/fortytwo) +- `internal/modules/util/help.go:100` (/help) + +Every one writes `update.Message.Chat.ID` without first checking `update.Message != nil`. Phase 5a's review caught the same shape in `info.go:36` and the fix landed there (`if msg == nil { return nil }`). The pattern was not propagated. With `bot.HandlerTypeMessageText` + `bot.MatchTypeCommand` the dispatcher only fires on text-message updates today, so `update.Message` is non-nil in practice — but: + +- The infosec-style "untrusted external input" boundary lives at the webhook decoder. A malformed Telegram payload that decodes into a partially-populated `models.Update` still satisfies `MatchType` matching at the library level but can leave `Message == nil`. Library-level guarantees here are thin. +- Future visibility-aware dispatch (callbacks, edited-messages) will route through different handler types, and the same factory-supplied `Command.Handler` may be reused. The /info fix already captured this in a comment; misc and util/help did not get the same hardening. +- Defensive cost is one line per handler. + +**Fix:** add `if update.Message == nil { return nil }` (or equivalent guard) at the top of each handler. Cheaper than a test, and matches the pattern Phase 5a established. + +--- + +## Major + +### J1 — Helper-function drift across four modules; subjectFor variants are NOT byte-equivalent + +**Files:** +- `internal/modules/wordle/handlers.go:30-47` — `subjectFor` +- `internal/modules/loldle/handlers.go:33-46` — `subjectFor` +- `internal/modules/loldleemoji/handlers.go:35-48` — `subjectFor` + +Phase 6a flagged "extract `normalize`, `subjectFor`, `argAfterCommand`, `findChampion` for 6b prep". The flag was right but undersold the urgency: + +```go +// wordle (handlers.go:30-47) +switch msg.Chat.Type { +case models.ChatTypePrivate: + if msg.From != nil { return strconv.FormatInt(msg.From.ID, 10) } +case models.ChatTypeGroup, models.ChatTypeSupergroup: + return strconv.FormatInt(msg.Chat.ID, 10) +default: + if msg.From != nil { return strconv.FormatInt(msg.From.ID, 10) } +} +return "" + +// loldle and loldleemoji (handlers.go:33-46 / 35-48) +switch msg.Chat.Type { +case models.ChatTypeGroup, models.ChatTypeSupergroup: + return strconv.FormatInt(msg.Chat.ID, 10) +default: + if msg.From != nil { return strconv.FormatInt(msg.From.ID, 10) } +} +return "" +``` + +Functionally equivalent **today** because Telegram populates `From` on every private DM. But: + +- For `ChatTypeChannel` with no `From` (anonymous channel post), wordle returns `""` and loldle/emoji also return `""` — same. ✓ +- For `ChatTypePrivate` with `From == nil` (which Telegram never does, but the type system allows), **wordle** returns `""` and **loldle/emoji** also return `""` via the default branch. Identical. + +The risk isn't current behavior; it's that **someone editing one copy to fix a bug will forget to edit the other two**. Phase 5c found exactly this with `winRate` (truncation bug existed in both wordle and loldle; the 5b review fixed wordle and missed loldle, then 5c had to clean up). + +Other drift in the same files: +- `argAfterCommand` is byte-identical across wordle / loldle / loldleemoji (3 copies). +- `nowMillis` is byte-identical across wordle / loldle / loldleemoji (3 copies). +- `reply(ctx, b, chatID, text)` (loldle/loldleemoji) vs `reply(ctx, b, msg, text)` (wordle) — slightly different signatures; not interchangeable but the implementation body is duplicated. +- `replyHTML` is byte-identical across loldle / loldleemoji. +- `normalize` is byte-identical across loldle / loldleemoji (and a related `normalizeWord` in wordle that drops digit support — different alphabet, intentional). +- `findChampion` shape is identical across loldle / loldleemoji modulo type names. + +**Recommendation:** extract `internal/modules/util/chathelper` (or `internal/champname` for the loldle-specific normalize+findChampion pair). Do it as the **first** commit of phase 6b before any new variant lands; then 6b's quote/ability/splash variants pick up the helpers from a single source and the drift problem disappears. Cost: ~80 LOC of helper + 4 import line changes per module. + +A **shared `WinRate(wins, played int) int`** helper should be part of the same extraction — currently 3 copies (wordle/loldle/loldleemoji) all using `math.Round` correctly today, but one drift opportunity per port. + +### J2 — Logging is `log.Printf` everywhere; Cloud Logging will not parse it + +**Files:** `cmd/server/main.go` (×9), `internal/server/router.go:77,86`, `internal/modules/dispatcher.go:24`, `internal/modules/misc/misc.go:51`. 18 call sites total in non-test code. + +Cloud Run forwards `stdout` to Cloud Logging line-by-line. Cloud Logging treats each line as a record but only **parses structured JSON** for severity, trace correlation, and label-based filtering. `log.Printf("cron %s failed: %v", ...)` becomes a single text payload with severity DEFAULT — every alert filter / dashboard / SLO query will need a regex. + +Phase 11 plans "Cloud Logging structured JSON" so this is on the roadmap. The concern is: **every call site added until Phase 11 is debt** that has to be migrated. With Phase 11 bumping into Phase 6b/7/8 worth of new modules, the migration target is moving. + +**Recommendation:** introduce a tiny `internal/log` (or `internal/obs`) package now with a `WithFields(...)` API that emits JSON lines (Go 1.21+ `slog.JSONHandler` is stdlib, zero deps), and route all current call sites through it. Future modules pick up the structured form for free. ~30 LOC + 18 mechanical edits. Could also be Phase 11's first commit — but the longer it waits, the more sites to mechanically rewrite. + +Phase 5a's `misc.go:51` `log.Printf("misc /ping: putJSON failed: %v", err)` is a good motivating example: `module=misc command=ping op=putJSON err=...` as JSON fields makes the eventual error rate dashboard a one-line query; the current shape needs a regex. + +Bonus: the `log.Printf("cron name=%s", name)` at `router.go:77` is **PII-adjacent** — `name` is operator-controlled (cron names are validated `^[a-z0-9_]{1,32}$`), so no real injection risk, but Phase 11 plan says "Cloud Logging" and the only thing standing between us and a `log_entry_payload_size_too_large` is hand-discipline. + +### J3 — Cron handler chain has no log-injection guard for the **error** branch + +**Files:** `internal/server/router.go:86` + +`log.Printf("cron %s failed: %v", name, err)` — `name` is regex-validated so it's safe. But `err` is whatever the module returned, which is **module-controlled and may include user input**. A loldle module today does `fmt.Errorf("loldle saveGame: %w", err)` then chains downward; if a future module ever does `fmt.Errorf("user input was %q", argAfterCommand(msg.Text))` and that error bubbles up, the log line gets a newline-bearing user string. CWE-117 (log injection) class. + +This is theoretical today (no current handler error-wraps user input). But the bot will only get more user-input-touching modules from here. Phase 11's structured-JSON conversion (J2) makes this naturally safe (JSON encodes newlines as `\n`). + +Stopgap until then: `log.Printf("cron %s failed: %s", name, strings.ReplaceAll(err.Error(), "\n", " "))` — ugly, but bounds the damage. Or **defer to J2** and fix structurally. + +### J4 — `update.Message.Chat.ID` is a soft trust boundary that the codebase doesn't enforce + +Same shape as C2 but a finer point: `models.Update` is decoded directly from the webhook body via `json.NewDecoder(r.Body).Decode(...)`. We trust Telegram's TLS-authenticated webhook (X-Telegram-Bot-Api-Secret-Token validates the *delivery*, not the *payload* — anyone with the secret can send any payload). With the secret leaked, a forged request with `Message.Chat.ID = -` would route a guess into another user's stats / send a sticker into someone else's group. Practical risk is low (secret is only-on-Telegram-server today), but defense-in-depth is cheap: + +- Validate `update.UpdateID > 0` (Telegram always positive). +- Validate `update.Message.Chat.ID` non-zero before trusting it. +- Reject `Message.Date` more than ~24h old (replay window). + +None of these are urgent. Track in Phase 11 alongside structured logging. + +--- + +## Medium + +### M1 — `bot.New` may return error for transient network reasons; main.go treats it as fatal + +**File:** `cmd/server/main.go:69-72` + +```go +b, err := telegram.NewBot(cfg.TelegramBotToken) +if err != nil { log.Fatalf("telegram bot init: %v", err) } +``` + +`telegram.NewBot` passes `WithSkipGetMe()` so the only thing left to fail is option-application — which is in-process and deterministic. Today `err` is always nil after argument validation. Slight surprise that `bot.New` can return `error` at all in this path. Not actionable; flagging because future contributors might switch back to the GetMe-blocking variant and assume the fail-fast path handles transients gracefully (it doesn't; Cloud Run will hot-loop restart the container). + +Fix: add a comment explaining "with WithSkipGetMe + WithNotAsyncHandlers, bot.New does not perform I/O; this error is unreachable in practice" — or leave alone. + +### M2 — Synchronous webhook dispatch holds Cloud Run instance for full handler duration + +**Files:** `internal/telegram/webhook.go:56-58`, `client.go:19` + +`bot.WithNotAsyncHandlers()` makes handler dispatch synchronous (good — solves H2 from the Phase 02 review). But it means a slow handler (e.g. a `/loldle` first response that does 3 Firestore reads + 1 sticker send) holds the webhook open for hundreds of ms — and Cloud Run min-instance=0 default + 1-concurrent-request-per-instance budget means a queue forms during traffic spikes. + +Numbers from real handlers I reviewed: +- `/loldle` first guess: 3 Firestore reads (game, stats, config), 2 Firestore writes (game saveGame, stats put), 1 sticker send, 1 message send. ~250-500ms P95 on warm instance. +- `/wordle` similar. + +The `handlerTimeout = 10 * time.Second` cap (`webhook.go:26`) is the right shape but Telegram retries after 60s of no 2xx, so a 10s cap means **on a 10s-stuck handler the user gets a duplicate update (Telegram retry) AND the original eventually completes** — a duplicate-write window during the retry / completion overlap. The keylock per-subject mutex protects same-subject writes, but the **second invocation enters the handler** first and sees state from the abandoned first invocation, possibly skipping the user's intended action. + +**Recommendation:** lower handlerTimeout to 8s (gives 2s margin before Telegram retry), and document the retry / duplicate-update pattern as a known limitation. Or: switch to async-with-detached-context per Phase 02 review's H2 alternative ("`context.WithoutCancel(r.Context())` + own goroutine"). Not v1; phase 11 work. + +### M3 — `Cron` handler error mapping conflates "handler failed" with "cron not found" + +**File:** `internal/server/router.go:81-89` + +```go +if err := modules.DispatchScheduled(...); err != nil { + if errors.Is(err, modules.ErrCronNotFound) { http.NotFound(...); return } + log.Printf("cron %s failed: %v", name, err) + http.Error(w, "cron failed", http.StatusInternalServerError) +} +``` + +500 on handler error is correct behavior (Cloud Scheduler retries 5xx, doesn't retry 4xx). But Cloud Scheduler also has a "max retry attempts" cap that, when exceeded, sends to a dead-letter; with bare 500 there's no way for a handler to signal "do not retry, I poisoned this". Today no handler has this need, but a "the user's quota is exhausted" handler would benefit from returning 4xx-class. + +YAGNI today. Worth a `Cron.RetryPolicy` field (or sentinel error like `modules.ErrCronDoNotRetry`) when the first such handler lands. Document in Phase 09's plan. + +### M4 — `Module.Name` can be silently overwritten without warning + +**File:** `internal/modules/registry.go:119` + +```go +mod := factory(moduleDeps) +mod.Name = name // enforce: module name is its registry key +``` + +Phase 02-03 review flagged this as L1. Still here. Today: harmless, factories happen to either set `Name: name` themselves or leave it blank. But **if a module factory sets `Name: "different"` for legitimate reasons** (refactor, copy-paste, dynamic name override), that intent is **silently discarded** with no log line. Suggest: + +```go +if mod.Name != "" && mod.Name != name { + return nil, fmt.Errorf("module factory for %q returned mismatched Name=%q", name, mod.Name) +} +mod.Name = name +``` + +Or drop `Module.Name` entirely (registry already keys by module name; the field is redundant). Either is more honest than "silently overwrite". + +### M5 — `firestore_kv.go` → 216 LOC; `loldle/handlers.go` → 334 LOC; `wordle/handlers.go` → 284 LOC; `loldleemoji/handlers.go` → 269 LOC; `loldle/compare.go` → 253 LOC; `cmd/server/main.go` → 211 LOC + +Project rule (CLAUDE.md "Consider Modularization"): files >200 LOC should be considered for splitting. Six files exceed. + +| File | LOC | Suggested split | +|------|-----|-----------------| +| `loldle/handlers.go` | 334 | Each `handle*` to its own file (handle_loldle.go, handle_giveup.go, handle_stats.go, handle_setmax.go) — most of the 334 is one handler each; helpers like `subjectFor` / `argAfterCommand` move to a sibling file or to the shared package per J1. | +| `wordle/handlers.go` | 284 | Same — handle_wordle, handle_new, handle_giveup, handle_stats. | +| `loldleemoji/handlers.go` | 269 | Same. | +| `loldle/compare.go` | 253 | Year/multi/exact compare functions split into `compare_year.go`, `compare_multi.go` — already cleanly separated by attr type within the file. | +| `firestore_kv.go` | 216 | Validate / prefixSuccessor → `firestore_keys.go`. List / Get / Put / Delete stay together. ~60 LOC migration. | +| `cmd/server/main.go` | 211 | `loadConfig` + `splitCSV` + `envForModules` → `config.go`. `buildProvider` → already a candidate for `provider.go`. main() shrinks to ~80 LOC of orchestration. | + +This is a guideline, not a hard rule, and J1's helper-extraction will incidentally pull ~50 LOC out of three handlers.go files — making M5 mostly self-resolving. Recommend doing M5 **after** J1, since J1 mechanically dictates which lines move out first. + +### M6 — `loldle/state.go` `getOrInitGame` is identical in shape to `loldleemoji/state.go` `getOrInitGame`; almost identical to `wordle/state.go` `getOrInit` + +Three copies of "load existing or start fresh" with tiny variations in: +- whether maxGuesses is dynamic per subject (loldle/emoji yes, wordle no). +- whether StartedAt initialises to nil (loldle/emoji) or now-millis (wordle). + +Each module's gameState shape differs enough that a shared interface is awkward, but the **pattern** is so repetitive it screams for extraction. Phase 11 problem; tracking only. + +### M7 — `MemoryProvider.Base()` is production code surface area + +**File:** `internal/storage/kv_provider.go:35` + +Phase 04 review M4 flagged this. Still here. The method is documented "test-only" but is on the public production type. A future module can `provider.(*storage.MemoryProvider).Base()` and bypass module isolation — silently. The phase-04 review suggested moving to a `_test.go` build-tagged file or a `storagetest` helper package. Still recommended; cheap. + +--- + +## Minor + +### N1 — `Registry` struct has 5 unexported fields used only internally; `AllCommands` is the lone exported map + +`AllCommands` exposed because `dispatcher.Install` needs to iterate it. `publicCmds`/`protected`/`private` are accessed only via getter methods (`PublicCommands()` etc.) which sort+copy. Inconsistency: the dispatcher could equally use a getter. Cosmetic only; small refactor would close out the "callers can mutate AllCommands post-build" vector that Phase 02 review M7 flagged. + +### N2 — `FirestoreProvider.For` accepts ANY moduleName without validation + +**File:** `internal/storage/firestore_provider.go:22` + +The comment says "Module names are validated by modules.Build before reaching here, so we don't sanitize again." True — but the `KVProvider` interface advertises that anyone may construct one. A test using `provider.For("__reserved__")` (Firestore-banned collection name) gets an error from gRPC, not a clean `validateCollection` rejection. Cheap to add a one-line check in `For`. Not blocking. + +### N3 — `gameTTLSeconds` (wordle) is unused; flag from Phase 5b L3 still present + +**File:** `internal/modules/wordle/state.go:18` + +Constant + comment have stale-doc smell. Phase 11 was supposed to add a TTL cron; until then, delete the constant or move the TTL note to a Markdown doc. Compiler doesn't complain (Go ignores unused package-level constants), so it just sits there. + +### N4 — `loldle/loldle.go:14` `MaxGuesses = 8` is exported but only used internally; same in `loldleemoji/state.go:15` `MaxGuesses = 5`. Same for `MaxGuessesCap`. + +Capitalised constants in domain-private packages with no out-of-package callers. Either lowercase them or move to docs. Style nit. + +### N5 — `loldle/state.go:30` and `loldleemoji/state.go:23-26` define `gameState` (lowercase) — unexported. But `loldle/loldle.go:14` defines `MaxGuesses` exported. Mixed casing within the same package suggests no hard convention. Pick one. Style nit. + +### N6 — `pickDaily` (wordle/daily.go:34) is unused by handlers but kept "for parity" + +Effectively dead code with a passing test. Either wire into a /wordle_daily handler (Phase 06+ work) or delete + delete the test. Same shape as N3. + +### N7 — `internal/modules/modules.go` is a 7-line empty-package comment file + +Vestigial. Could be folded into `module.go`'s package doc. Or kept. Truly cosmetic. + +### N8 — `cmd/server/main.go:184` has misaligned struct-init padding + +```go +ModuleEnv: envForModules(envMap), +``` +vs +```go +TelegramBotToken: envMap["TELEGRAM_BOT_TOKEN"], +``` + +`gofmt` should rewrite this on save. Probably fine but failing-rule-scope nit. + +--- + +## Architectural observations + +### Package boundaries: clean + +- `internal/keylock` is a generic primitive, correctly placed at top-level peer to `storage` / `telegram` / `server`. +- `internal/storage` is cleanly factored: `KVStore` interface + two impls + a prefix wrapper. `KVProvider` abstraction is the right shape (modules see `KVStore`, not the provider). +- `internal/modules` framework is clean: registry holds maps, validate gates inputs, dispatchers tie to bot/HTTP. Modules are leaves. +- `cmd/server` is the composition root and owns the catalog (`factories()`). Comment in `internal/modules/modules.go` correctly explains why the catalog cannot live inside `internal/modules`. + +### Trust boundaries: mostly enforced + +| Boundary | Validation | Gap | +|----------|------------|-----| +| `MODULES` env → registry | `moduleNameRe` regex, dedupe, factory lookup | None | +| Webhook body → handler | `MaxBytesReader(1MiB)`, `json.Decoder` | C2 (nil-deref past decode) | +| Webhook auth | constant-time secret compare | None | +| Cron route → dispatch | `cronNameRe` regex + constant-time secret | None | +| Cron error → log | direct `%v` of internal error | J3 (potential CRLF injection theoretical) | +| Module name → KV provider | `moduleNameRe` regex (no `:`) | N2 (Firestore provider doesn't double-check) | +| KV key → Firestore | `validateKey` thorough | None (Phase 04 review confirmed) | +| User input → reply text | `html.EscapeString` everywhere I checked | None | + +### Concurrency: clean across the board + +- All mutating handlers use `defer s.locks.Acquire(subject)()`. +- `math/rand` package-level functions are mutex-protected (used everywhere). +- `keylock.Map` uses `sync.Map` correctly. +- `Registry` is read-only post-Build by convention (documented). +- `srv.Shutdown` waits for in-flight handlers; provider closes after shutdown. +- Bot dispatcher is synchronous (`WithNotAsyncHandlers`), so `r.Context()` lives across handler. + +`go test -race -count=1` clean on every package. + +### Error propagation: mostly clean, two patterns + +1. **Module handlers** return `error`; the dispatcher logs and discards. Phase 02-03 review M6 flagged that this is decorative. Still true. Neither metrics nor retry nor user-visible "internal error" reply hooks into the return value. For an early-stage codebase this is fine; mark for Phase 11 observability. + +2. **Storage layer** wraps errors with `fmt.Errorf("firestore put %s/%s: %w", ...)` consistently. `errors.Is` checks against `ErrNotFound` and `ErrCronNotFound` — these are the only sentinels. Good. + +### Configuration: clean + +- All env vars read once in `loadConfig`. Per-module env via `Deps.Env` with explicit deny-list (`secretEnvKeys`). Deny-list is correct shape but requires manual upkeep; Phase 02 review H5's allow-list alternative is still preferable but not blocking. +- No env var read after startup. Good. + +### Dead/vestigial code + +| Symbol | File | Justification | +|--------|------|---------------| +| `gameTTLSeconds` | `wordle/state.go:18` | Documented in Phase 5b review (L3) | +| `pickDaily` | `wordle/daily.go:34` | Has a test; unused by handlers. (N6) | +| `internal/modules/modules.go` | (entire file) | 7-line empty package doc. (N7) | +| `MemoryProvider.Base()` | `kv_provider.go:35` | Test-only on production type. (M7) | +| `Module.Name` | `module.go:55` | Overwritten by registry; never read by factory. (M4) | + +None blocking. + +--- + +## Tests gap (cross-cutting) + +Per-phase reports already enumerated module-specific gaps. Cross-cutting gaps: + +1. **Webhook handler integration test** — phase-02 review test plan #1 was filed; a `webhook_test.go` file exists. Verified it covers method/secret/decode paths. Confirmed adequate. +2. **Cron handler integration test** — `router_test.go` exists. Verified it covers method/secret/cronNameRe/dispatch paths. +3. **No CI matrix entry for `make build`** — `.github/workflows/ci.yml` only does `go vet`, `go test`, `go build`. Doesn't `docker build` from CI. Combined with C1, that's why no one noticed the Dockerfile/go.mod mismatch. **Add `- run: docker build -t miti99bot-go .` to CI**. ~3 lines. +4. **No emulator-gated tests in CI** — Phase 04 review L1/L3 flagged this. Makefile `test-emulator` exists; CI only runs the no-emulator subset. Acceptable for now (emulator setup adds 30-60s to CI run); track for Phase 10/11. + +--- + +## Recommended action order + +| # | Severity | Action | Effort | +|---|----------|--------|--------| +| 1 | C1 | Bump Dockerfile + CI to Go 1.25 (or lower go.mod to 1.23) | 5min | +| 2 | C1 | Add `docker build` step to ci.yml so this never recurs silently | 5min | +| 3 | C2 | Add `if update.Message == nil { return nil }` to misc + util/help handlers | 10min | +| 4 | J1 | Extract shared helpers to `internal/champname` (or similar) as the FIRST commit of Phase 6b | 1-2h | +| 5 | J2 | Introduce `internal/log` with slog.JSONHandler; rewire 18 call sites | 2-3h, or defer to Phase 11 | +| 6 | M5 | Split files >200 LOC into per-handler / per-concern files (after J1) | 1-2h | +| 7 | M4/M7/N* | Hygiene pass — Module.Name guard, Base() to test-tag, dead-code cleanup | 1h | + +Items 1-3 are blockers for the next deploy / merge. The rest can ride along Phase 6b/11 as natural cleanup. + +--- + +## Positive observations + +- **Trust boundaries enumerated and individually fixed** with constant-time compares + regex-validated routes + strict per-key Firestore validation. Each fix has a comment explaining the threat model. Operationally friendly. +- **`KVProvider` interface is exactly one method** — easy to mock, hard to misuse. Phase 04's review called this out; six modules later, it's still aging well. +- **Per-subject keylock + `WithNotAsyncHandlers` together** turn the goroutine-per-update model into a sequenced-per-subject model. Equivalent guarantee to JS Workers' isolate-per-request. Documented at `keylock/keylock.go:5-12`. +- **Wire-format tests** lock JS-parity for every persisted JSON shape (gameState, stats, roundConfig). `*int64` for nullable timestamps. Defends migration goal. +- **Embed strategy + panic-on-bad-data** consistently applied across wordle/loldle/loldleemoji. Build-time bug surfaces at startup, not on first user. +- **Phase reports themselves**: clear, opinionated, action-ordered. The 6a report's "extract helpers as 6b prep" foresight is exactly the kind of forward-pointing review note that makes the next reviewer's job cheaper. +- **CI does `-race -count=1`** from day one. The single most-valuable lint a Go service can have. +- **Defensive stripping of secrets from `Deps.Env`** via deny-list. Allow-list would be tighter but the deny-list is honest about its limits and tested. +- **`srv.Shutdown(15s) → defer closeProvider()`** ordering is correct and the comment in Phase 04 review M5 is now in code (mostly). Graceful shutdown story is solid. + +--- + +## Unresolved questions + +1. **Q1**: Phase 11's "Cloud Logging structured JSON" is the natural home for J2/J3. Bringing it forward to Phase 6b (~2-3h) versus letting it pile up to Phase 11 (~2-4h migration cost) — **which side has the better expected-value tradeoff**? Recommend forward-port: every module added in Phase 6b/7/8 is a J2 caller-site, so the marginal cost of structured-log-from-the-start is lower. +2. **Q2**: J1's helper extraction — into `internal/modules/util/` (already exists, but it's the /info /help /stickerid module) or a fresh `internal/modules/util/chathelper/` subpackage? Or a top-level `internal/champname` for the loldle-specific normalize+findChampion pair? Naming bikeshed; but the choice constrains how Phase 7+ AI modules will reuse the same helpers. +3. **Q3**: C1 — bump go.mod down to 1.23 (zero feature loss) versus bump Dockerfile + CI to 1.25 (more typical, but adds Go-version churn). The codebase doesn't use any 1.24/1.25 features I found. Cheapest is to lower go.mod. +4. **Q4**: M2 — keep `WithNotAsyncHandlers` (synchronous) or switch back to async-with-detached-context per the Phase 02 review's H2 alternative? Synchronous is simpler and matches JS-Worker semantics; async would buy back webhook return latency at the cost of a small goroutine pool. Defer until cold-start / latency telemetry from Phase 11 says one way or the other. + +--- + +**Status:** DONE_WITH_CONCERNS +**Summary:** Architecture and concurrency are solid; two cross-phase blockers (Dockerfile/go.mod version mismatch + nil-deref pattern not propagated to misc/help) need fixing before the next merge, plus a J1 helper-drift cluster that should be extracted as Phase 6b's first commit before five more modules compound the problem. diff --git a/plans/reports/code-reviewer-260509-1248-whole-project-security.md b/plans/reports/code-reviewer-260509-1248-whole-project-security.md new file mode 100644 index 0000000..7bbff93 --- /dev/null +++ b/plans/reports/code-reviewer-260509-1248-whole-project-security.md @@ -0,0 +1,276 @@ +# code-reviewer · whole-project security audit + +Date: 2026-05-09 +Scope: full repo (`cmd/`, `internal/`, Dockerfile, CI, go.mod). Focus on production-readiness for Cloud Run + Firestore + Telegram webhook. +Verdict: **DONE_WITH_CONCERNS** — no Critical issues. Several Medium items worth resolving before public deploy; one High and a few Lows. + +Prior reports reviewed (issues already fixed not re-flagged): +- `code-reviewer-260508-2254-phase02-03-bootstrap.md` — C1/C2/C3, H1-H7 all resolved (verified). +- `code-reviewer-260508-2333-phase04-firestore-kv.md` — H1/H2 resolved (verified at firestore_kv.go:75-80 and main.go:135-137). +- `code-reviewer-260509-1206-phase6a-loldle-emoji.md` — `%q` divergence still open (cosmetic). + +--- + +## Critical + +None. + +--- + +## High + +### H1 — `Deps.Env` still leaks `GOOGLE_CLOUD_PROJECT`, OAuth file paths, Cloud Run env to every module +File: `cmd/server/main.go:188-197`, `internal/modules/module.go:72-76`. + +`secretEnvKeys` strips only the three named tokens. Modules still receive every other env var, including: `GOOGLE_CLOUD_PROJECT`, `FIRESTORE_EMULATOR_HOST`, `GOOGLE_APPLICATION_CREDENTIALS` (path), Cloud Run-injected `K_SERVICE`/`K_REVISION`/`K_CONFIGURATION`, and any future `*_API_KEY` (Gemini, etc. — phase 7 will add `GEMINI_API_KEY` and unless that exact string lands in `secretEnvKeys` first, it goes to all modules). + +Risk: a future module that reflects/echoes any `Deps.Env` value (debug helper, "system info" command) leaks creds. The previous reviewer's H5 fix was a denylist — denylists don't scale. + +Recommendation: invert to allowlist. +- Either pass nothing in `Deps.Env` (modules get truly nothing) and have each module document its own env requirements externally, OR +- Adopt a `MODULE__*` convention; only matching keys flow to that module's Deps. + +Phase 07 (Gemini) is the natural moment — once a module needs an external API key, an opt-in allowlist is the only safe pattern. Hardcoding `GEMINI_API_KEY` into `secretEnvKeys` works for one variable but not for the 2nd, 3rd, etc. + +--- + +## Medium + +### M1 — `MODULES` env validation rejects bad names *during* module construction; one good factory may have already executed +File: `internal/modules/registry.go:98-153`. + +The for-loop validates name → checks dup → looks up factory → **calls factory(moduleDeps)** → validates commands. If a later iteration fails (unknown name, dup name, validation error), modules earlier in the list have already had their factories invoked. For loldle/loldleemoji that's just slice allocation, but a future Factory that opens a file, does a DNS lookup, or holds a long-lived resource (Phase 07 Gemini client) will leak. + +Today: low impact (factories are pure). Document with a comment that factories must be allocation-only and never block / open external resources, OR pre-validate all names + dup detection in a first pass before any factory runs. + +### M2 — `Visibility` field is decorative; `stickerid` (private), `fortytwo` (private), `loldle_setmax` / `loldle_emoji_setmax` (private) are publicly invocable +Files: `internal/modules/dispatcher.go:15-29`, `internal/modules/util/stickerid.go:24`, `internal/modules/misc/misc.go:90`, `internal/modules/loldle/loldle.go:36`, `internal/modules/loldleemoji/loldleemoji.go:33`. + +`Install` registers every command with `bot.RegisterHandler` regardless of `Visibility`. The comment at `module.go:14-15` openly notes "the dispatcher does not enforce visibility today." Result: any user in any chat can: +- `/stickerid` → echo a sticker file_id back. Information disclosure (minor — sticker IDs aren't secret, but they signal which stickers the bot owner privately uploaded). +- `/loldle_setmax 1` → make every group's loldle round trivially solvable. Visible griefing. +- `/loldle_emoji_setmax 1` → same. +- `/fortytwo` → easter egg, no harm. + +Risk: low confidentiality, real abuse for `setmax` in groups. The `setmax` commands change *group-shared* state (subjectFor() returns chat.ID for groups), so any group member can set max=1 and break the game for everyone. + +Fix options (cheapest first): +1. Document `setmax` is intentionally permissive and remove the `VisibilityPrivate` tag (truth-in-advertising). Acceptable if you accept the griefing. +2. Hard-code an admin allowlist via env: `ADMIN_USER_IDS=12345,67890` and gate `Visibility >= Protected` commands at the dispatcher. +3. For groups, check `getChatMember(chat_id, user_id).status` ∈ {creator, administrator} via the Telegram API before running protected/private commands. + +Option 2 is the minimum production-acceptable answer. Option 3 is the right answer; it costs one extra Telegram API call per protected command invocation. + +### M3 — `MaxBytesReader`-induced 413 returns 200 instead +File: `internal/telegram/webhook.go:49-54`. + +```go +r.Body = http.MaxBytesReader(w, r.Body, maxWebhookBody) +var update models.Update +if err := json.NewDecoder(r.Body).Decode(&update); err != nil { + http.Error(w, "bad request", http.StatusBadRequest) + return +} +``` + +`MaxBytesReader` writes a 413 status to `w` directly when the cap is hit (this is its documented side effect). After that, `Decode` returns an error, the handler tries to call `http.Error(w, "bad request", 400)` — but headers are already sent (413), so it just appends "bad request\n" to the 413 body. Telegram sees a non-2xx and retries. Not a security hole, but operationally noisy. Test `TestWebhookHandler_RejectsOversizedBody` only checks `!= 200`, which passes for 413, so the bug is silent. + +Also: a 1 MiB cap is generous (Telegram updates with media references are <100 KiB even with thumbnails). Consider 256 KiB. + +Fix: tighten cap and use the documented two-stage check pattern, or accept 413 as the response code (don't shadow it with 400). + +### M4 — Cron handler `defaultCronTimeout = 5m` runs serially against single-instance Cloud Run; no parallelism cap +File: `internal/server/timeouts.go:8`, `internal/server/router.go:78-79`. + +Cloud Scheduler can fire several crons within a minute. Each request is 5-min capped. Cloud Run free tier is `min=0,max=1` per the plan — so two crons that arrive 30s apart serialize, the second waits up to 5 minutes. Worse: an attacker who steals or guesses the cron secret can fire many `POST /cron/{any-valid-cron-name}` requests; with body=empty they cost nothing on Telegram side but pin the instance for 5 min × N. + +Mitigations: +- Tighten `defaultCronTimeout` to the actual wall-clock budget (e.g. 60s) and document that long crons must publish to PubSub and exit fast. 5 minutes is a footgun. +- Cron secret rotation policy: document who rotates and how often. The shared-secret bridge in router.go:23 is "until Phase 09 OIDC" — code comment promises the migration but does not record an SLA. +- Phase 09 OIDC + Cloud Run IAM ingress restriction is the proper fix. + +### M5 — Webhook URL secret-token compare leaks length via `MaxBytesReader` ordering +File: `internal/telegram/webhook.go:43-49`. + +The constant-time secret compare runs *before* `MaxBytesReader` is installed. An unauthenticated POST with a 100 MiB body still gets streamed up to the auth check… actually no — the auth check only reads the header, body is never read. So no DoS via body upload pre-auth. **But** the body lifetime: an attacker can hold a slow-loris connection sending header bytes; the server's `ReadHeaderTimeout: 10s` (main.go:96) caps that. OK. **Confirmed safe**, just worth a comment that the order matters. + +(Demoting from initial Medium to documentation-only after re-reading.) + +### M6 — `ReadTimeout: 30s` covers webhook AND cron, but cron handlers may take longer than the *body read* allowance +File: `cmd/server/main.go:97-101`. + +`ReadTimeout` includes the body. For cron requests with empty bodies this is fine. If a future cron endpoint accepts a JSON payload and Cloud Scheduler ever delivers it slowly, 30s read could fire. Today: harmless. When phase 9 lands and cron payloads grow, revisit. + +### M7 — `loldleemoji` does not HTML-escape the emoji string when rendering +File: `internal/modules/loldleemoji/render.go:20`. + +```go +clue := "🎭 " + emojis +``` + +`emojis` is loaded from the embedded `data/emojis.json` (loldleemoji.go:30) and the comment claims emojis "aren't HTML-escaped in the JS source either." True. But the data file is build-time controlled — a malicious / careless edit that puts `