From 6b4d9be14132494a0fb694a2651b5f7be098bb42 Mon Sep 17 00:00:00 2001 From: Rene Nochebuena Guerrero Date: Sat, 8 Aug 2026 00:55:39 -0600 Subject: [PATCH] fix(mcp): CORS-aware scaffold, cors.wildcard rule, server-not-appended FP; v1.1.2 --- CHANGELOG.md | 22 ++++++++++++ README.md | 2 +- cmd/server/main.go | 2 +- internal/index/builtins/README.md | 23 +++++++++--- internal/rules/cors_rules.go | 48 +++++++++++++++++++++++++ internal/rules/cors_rules_test.go | 58 +++++++++++++++++++++++++++++++ internal/rules/rules.go | 18 ++++------ internal/tools/env_tools_test.go | 6 +++- internal/tools/scaffold.go | 58 ++++++++++++++++++++----------- 9 files changed, 198 insertions(+), 39 deletions(-) create mode 100644 internal/rules/cors_rules.go create mode 100644 internal/rules/cors_rules_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bccfd6..742e06a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,28 @@ This module adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html --- +## [1.1.2] — 2026-08-08 + +Patch. Fixes surfaced by an adversarial review plus the CORS wildcard trap. + +### Added + +- **`cors.wildcard-noop` rule** (`validate_snippet`). Flags `mw.CORS(...)` with a `"*"` literal — + which the framework now rejects at boot — and points to `CORSAllowAll()` or explicit origins. + +### Changed + +- **`get_scaffold`**: CORS is now env-gated (`CORSAllowAll` in local, `mw.CORS(origins)` elsewhere); + composes `launcher.Config` and `health.Config` so `EINHERJAR_COMPONENT_STOP_TIMEOUT` and + `EINHERJAR_HEALTH_CHECK_TIMEOUT` are reachable and appear in `.env.example`; the health hook uses + `health.NewHandlerWithConfig(...).ServeHTTP`; `.env.example` no longer emits `APP_CORS_ORIGINS=*`. +- **`wire` builtin**: documents the CORS convention and recommends `check_env`'s `composes` selectors. + +### Fixed + +- **`web.server-not-appended`** no longer false-positives on a correctly-appended server; it fires + only when `server.New` is present and `.Append` is absent. + ## [1.1.1] — 2026-08-07 Patch. `check_env` gains struct-level granularity so it can catch dead vars that module diff --git a/README.md b/README.md index 5c0311f..47c04ec 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # einherjar/mcp -[![version](https://img.shields.io/badge/version-v1.1.1-5C4EE5?style=flat-square)](https://code.nochebuena.dev/einherjar/mcp) +[![version](https://img.shields.io/badge/version-v1.1.2-5C4EE5?style=flat-square)](https://code.nochebuena.dev/einherjar/mcp) [![license](https://img.shields.io/badge/license-AGPL--3.0-22863A?style=flat-square)](LICENSE) [![go](https://img.shields.io/badge/Go-1.26+-00ADD8?style=flat-square&logo=go&logoColor=white)](https://go.dev) diff --git a/cmd/server/main.go b/cmd/server/main.go index 0c64af6..d296a03 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -23,7 +23,7 @@ import ( const ( serverName = "einherjar-mcp" - serverVersion = "v1.1.1" + serverVersion = "v1.1.2" ) func main() { diff --git a/internal/index/builtins/README.md b/internal/index/builtins/README.md index 95c31a3..000af4a 100644 --- a/internal/index/builtins/README.md +++ b/internal/index/builtins/README.md @@ -145,17 +145,21 @@ and then failing at boot because nobody knew which variables to set. - **App-owned vars (`APP_*`)** — like `APP_JWT_SECRET` above: the framework can't know these, so keeping them in `.env.example` is your discipline, not something it can name-check. -After you compose a component, run **`check_env`** with the modules the app composes: it flags +After you compose a component, run **`check_env`** with what the app composes: it flags `EINHERJAR_*` names that don't exist, required vars you forgot to document, and vars set for a -module you don't actually compose (dead vars). `get_scaffold` already emits a `.env.example` -derived from these same tags, so the starting point is correct by construction. +config you don't actually compose (dead vars). Prefer the **`composes`** input (exact struct +selectors like `web/server/Config`) over `modules` — it catches struct-level dead vars (e.g. +`EINHERJAR_SERVER_CORS_ORIGINS` lives on `web.Config`, not `server.Config`). `get_scaffold` already +emits a `.env.example` derived from these same tags, so the starting point is correct by construction. ```bash # .env.example — copy to .env for local dev. Every var the app reads lives here. # ── App ─────────────────────────────────────────────────────────────────── APP_ENV=local -APP_CORS_ORIGINS=* +# APP_CORS_ORIGINS — explicit origins for non-local envs (comma-separated). +# Local uses mw.CORSAllowAll() and ignores this; "*" is rejected by mw.CORS — never use it. +APP_CORS_ORIGINS= APP_JWT_SECRET=change-me APP_JWT_ISSUER=myapp @@ -225,11 +229,20 @@ func Run() error { } db := postgres.New(logger, cfg.PG) + + // CORS convention: allow-all in local dev, explicit origins everywhere else. + // mw.CORS panics on "*" (it matches no real origin) — allow-all is mw.CORSAllowAll, + // never a "*" in APP_CORS_ORIGINS. + corsMW := mw.CORSAllowAll() + if !strings.EqualFold(cfg.AppEnv, "local") { + corsMW = mw.CORS(cfg.CORSOrigins) + } + srv := server.New(logger, cfg.Server, server.WithMiddleware( mw.RequestID(uuid.NewString), mw.Recover(logger), - mw.CORS(cfg.CORSOrigins), + corsMW, mw.RequestLogger(logger), authjwt.AuthMiddleware(logger, signer, publicPaths), authmw.EnrichmentMiddleware(logger, &claimsEnricher{}), diff --git a/internal/rules/cors_rules.go b/internal/rules/cors_rules.go new file mode 100644 index 0000000..6ae8bc5 --- /dev/null +++ b/internal/rules/cors_rules.go @@ -0,0 +1,48 @@ +package rules + +import ( + "go/ast" + "go/token" + "strings" +) + +// The web/mw CORS convention: mw.CORS uses exact-origin matching and rejects "*" +// at construction (panic). Passing "*" is the trap this rule catches before runtime — +// allow-all is done with mw.CORSAllowAll(), gated by env in application code. +func init() { + registered = append(registered, + Rule{ + ID: "cors.wildcard-noop", + Severity: SeverityError, + Module: "web", + Check: checkCORSWildcard, + }, + ) +} + +// checkCORSWildcard flags any call to .CORS(...) whose arguments contain a +// "*" string literal — which mw.CORS rejects (panics) at boot. +func checkCORSWildcard(c *Context) []Finding { + var hits []Finding + ast.Inspect(c.File, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || !strings.HasSuffix(exprName(call.Fun), ".CORS") { + return true + } + for _, arg := range call.Args { + ast.Inspect(arg, func(m ast.Node) bool { + lit, ok := m.(*ast.BasicLit) + if ok && lit.Kind == token.STRING && strings.Trim(lit.Value, `"`) == "*" { + hits = append(hits, Finding{ + Message: `mw.CORS with "*" panics at construction — "*" matches no real origin (exact-match only)`, + Hint: "Use mw.CORSAllowAll() for local development, or list explicit origins.", + Line: c.Fset.Position(call.Pos()).Line, + }) + } + return true + }) + } + return true + }) + return hits +} diff --git a/internal/rules/cors_rules_test.go b/internal/rules/cors_rules_test.go new file mode 100644 index 0000000..a86634c --- /dev/null +++ b/internal/rules/cors_rules_test.go @@ -0,0 +1,58 @@ +package rules + +import "testing" + +const corsWildcardSnippet = `package wire + +import "code.nochebuena.dev/einherjar/web/mw" + +func f() { _ = mw.CORS([]string{"*"}) } +` + +const serverAppendedSnippet = `package wire + +import ( + "code.nochebuena.dev/einherjar/core/launcher" + "code.nochebuena.dev/einherjar/web/server" +) + +func f() { + srv := server.New(logger, cfg) + lc := launcher.New(logger) + lc.Append(srv) +} +` + +const serverNotAppendedSnippet = `package wire + +import ( + "code.nochebuena.dev/einherjar/core/launcher" + "code.nochebuena.dev/einherjar/web/server" +) + +func f() { + srv := server.New(logger, cfg) + lc := launcher.New(logger) + _ = srv + _ = lc +} +` + +func TestCORSWildcardFires(t *testing.T) { + got := findingsFor(Run(corsWildcardSnippet), "cors.wildcard-noop") + if len(got) == 0 { + t.Fatal("cors.wildcard-noop did not fire on mw.CORS([]string{\"*\"})") + } +} + +func TestServerAppendedNoFalsePositive(t *testing.T) { + if got := findingsFor(Run(serverAppendedSnippet), "web.server-not-appended"); len(got) != 0 { + t.Errorf("web.server-not-appended false-positived on an appended server: %+v", got) + } +} + +func TestServerNotAppendedFires(t *testing.T) { + if got := findingsFor(Run(serverNotAppendedSnippet), "web.server-not-appended"); len(got) == 0 { + t.Error("web.server-not-appended should fire when server.New is not appended") + } +} diff --git a/internal/rules/rules.go b/internal/rules/rules.go index ca8bf13..3db1858 100644 --- a/internal/rules/rules.go +++ b/internal/rules/rules.go @@ -268,19 +268,15 @@ var registered = []Rule{ if !c.Importing("einherjar/web/server") || !c.Importing("einherjar/core/launcher") { return nil } - if !c.Called(".Append") { + // Fire only when a server is constructed but never appended — an appended + // server (lc.Append(srv)) is correctly managed, so stay silent (no false positive). + if !c.Called("server.New") || c.Called(".Append") { return nil } - // Heuristic: warn if server.New is constructed but not appended via .Append. - // We can't statically prove the argument was the server, so this is informational. - if c.Called("server.New") { - return []Finding{{ - Severity: SeverityInfo, - Message: "web/server is constructed — ensure it is passed to launcher.Append() so its lifecycle is managed", - Hint: "lc.Append(srv) lets the launcher start and gracefully stop the HTTP server", - }} - } - return nil + return []Finding{{ + Message: "web/server constructed but never appended to the launcher — its lifecycle won't be managed", + Hint: "lc.Append(srv) so the launcher starts and gracefully stops the HTTP server", + }} }, }, } diff --git a/internal/tools/env_tools_test.go b/internal/tools/env_tools_test.go index 863038b..32d4b2a 100644 --- a/internal/tools/env_tools_test.go +++ b/internal/tools/env_tools_test.go @@ -30,11 +30,14 @@ func TestRenderEnvExample(t *testing.T) { // Required, no default → uncommented with a runnable dev value. mustContain := []string{ "APP_ENV=local", - "APP_CORS_ORIGINS=*", + "APP_CORS_ORIGINS=", "EINHERJAR_PG_HOST=localhost", "EINHERJAR_PG_USER=postgres", "EINHERJAR_PG_PASSWORD=postgres", "EINHERJAR_PG_NAME=myapp", + // launcher + health composed → their (defaulted) vars are documented, commented. + "# EINHERJAR_COMPONENT_STOP_TIMEOUT=15s", + "# EINHERJAR_HEALTH_CHECK_TIMEOUT=5s", // logz is composed → its vars (both defaulted) are documented, commented. "# EINHERJAR_LOG_LEVEL=INFO", "# EINHERJAR_LOG_JSON=false", @@ -54,6 +57,7 @@ func TestRenderEnvExample(t *testing.T) { mustNotContain := []string{ "EINHERJAR_PG_DATABASE", "EINHERJAR_SERVER_ADDR", + "APP_CORS_ORIGINS=*", // the wildcard trap must never be emitted // CORS lives on web.Config, which the scaffold does not compose — so it // must not be emitted (it would be a dead var). "EINHERJAR_SERVER_CORS_ORIGINS", diff --git a/internal/tools/scaffold.go b/internal/tools/scaffold.go index 934227a..06cbafe 100644 --- a/internal/tools/scaffold.go +++ b/internal/tools/scaffold.go @@ -100,6 +100,9 @@ func main() { const tplWire = `package wire import ( + "net/http" + "strings" + "github.com/google/uuid" "code.nochebuena.dev/einherjar/core/launcher" @@ -127,19 +130,29 @@ func Run() error { logger := logz.New(logCfg) db := postgres.New(logger, cfg.PG) + + // CORS: allow-all in local dev; explicit origins elsewhere. mw.CORS panics on + // "*", so never pass a wildcard through APP_CORS_ORIGINS outside local. + var corsMW func(http.Handler) http.Handler + if strings.EqualFold(cfg.AppEnv, "local") { + corsMW = mw.CORSAllowAll() + } else { + corsMW = mw.CORS(cfg.CORSOrigins) + } + srv := server.New(logger, cfg.Server, server.WithMiddleware( mw.RequestID(uuid.NewString), mw.Recover(logger), - mw.CORS(cfg.CORSOrigins), + corsMW, mw.RequestLogger(logger), ), ) - lc := launcher.New(logger) + lc := launcher.New(logger, cfg.Launcher) lc.Append(db, srv) - withHealth(lc, srv) + withHealth(lc, srv, logger, cfg.Health, db) // … one withFeature(lc, srv, …) call per feature in your domain. return lc.Run() @@ -149,22 +162,19 @@ func Run() error { const tplHealth = `package wire import ( - "net/http" - + "code.nochebuena.dev/einherjar/contracts/logging" + "code.nochebuena.dev/einherjar/contracts/observability" "code.nochebuena.dev/einherjar/core/launcher" + "code.nochebuena.dev/einherjar/web/health" "code.nochebuena.dev/einherjar/web/server" ) -// withHealth registers a liveness endpoint — the simplest with hook. -// A real feature hook takes its deps (logging.Logger, postgres.Provider, …) and -// wires repo→service→handler inside the closure; see get_example("wire"). -func withHealth(lc launcher.Launcher, srv server.Server) { +// withHealth wires a concurrent health endpoint from the app's Checkable components +// (db, cache, …). health.NewHandlerWithConfig honors EINHERJAR_HEALTH_CHECK_TIMEOUT +// through cfg. Add more with hooks the same way; see get_example("wire"). +func withHealth(lc launcher.Launcher, srv server.Server, logger logging.Logger, cfg health.Config, checks ...observability.Checkable) { lc.BeforeStart(func() error { - srv.Get("/health", func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(` + "`" + `{"status":"ok"}` + "`" + `)) - }) + srv.Get("/health", health.NewHandlerWithConfig(logger, cfg, checks...).ServeHTTP) return nil }) } @@ -181,23 +191,27 @@ package config import ( "github.com/caarlos0/env/v11" + "code.nochebuena.dev/einherjar/core/launcher" "code.nochebuena.dev/einherjar/core/logz" "code.nochebuena.dev/einherjar/db-postgres" + "code.nochebuena.dev/einherjar/web/health" "code.nochebuena.dev/einherjar/web/server" ) // Config is the fully-resolved startup configuration. caarlos0/env recurses into -// the nested framework configs, populating their EINHERJAR_LOG_* / EINHERJAR_SERVER_* / -// EINHERJAR_PG_* tags from the environment next to the app-owned APP_* fields. +// the nested framework configs, populating their EINHERJAR_* tags from the +// environment next to the app-owned APP_* fields. type Config struct { AppEnv string ` + "`" + `env:"APP_ENV" envDefault:"local"` + "`" + ` CORSOrigins []string ` + "`" + `env:"APP_CORS_ORIGINS" envSeparator:","` + "`" + ` // Framework component configs — composed verbatim; their EINHERJAR_* tags // load through this same env.Parse call. - Log logz.Config // EINHERJAR_LOG_* - Server server.Config // EINHERJAR_SERVER_* - PG postgres.Config // EINHERJAR_PG_* + Launcher launcher.Config // EINHERJAR_COMPONENT_STOP_TIMEOUT + Log logz.Config // EINHERJAR_LOG_* + Server server.Config // EINHERJAR_SERVER_* + Health health.Config // EINHERJAR_HEALTH_CHECK_TIMEOUT + PG postgres.Config // EINHERJAR_PG_* } func Load() (Config, error) { @@ -223,10 +237,14 @@ func renderEnvExample(idx *index.Index, app string) string { b.WriteString("# ── App (APP_*) ────────────────────────────────────────────────────────────\n") b.WriteString("APP_ENV=local\n") - b.WriteString("APP_CORS_ORIGINS=*\n\n") + b.WriteString("# APP_CORS_ORIGINS — explicit origins for non-local envs (comma-separated).\n") + b.WriteString("# Local uses mw.CORSAllowAll() and ignores this; \"*\" is rejected by mw.CORS — never use it.\n") + b.WriteString("APP_CORS_ORIGINS=\n\n") + writeEnvSection(&b, "Einherjar: launcher", envspec.FindStruct(idx, "core", "launcher", "Config"), app) writeEnvSection(&b, "Einherjar: logging", envspec.FindStruct(idx, "core", "logz", "Config"), app) writeEnvSection(&b, "Einherjar: HTTP server", envspec.FindStruct(idx, "web", "server", "Config"), app) + writeEnvSection(&b, "Einherjar: health", envspec.FindStruct(idx, "web", "health", "Config"), app) writeEnvSection(&b, "Einherjar: PostgreSQL", envspec.FindStruct(idx, "db-postgres", "", "Config"), app) return b.String() }