diff --git a/CHANGELOG.md b/CHANGELOG.md index 325eded..0068061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,26 @@ This module adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html --- +## [1.3.4] — 2026-08-13 + +Patch. Teaches the framework's v1.6.0 request-binding adapters (`httputil.Bind` / `BindEmpty`). + +### Added + +- Wiring builtin documents `Bind` / `BindEmpty`: the adapter table gains both rows, plus a + "Binding path & query parameters" section with `path:` / `query:` / `json:` tag examples, the + `uuid.UUID` / `time.Time` conversion note, and the `default:` + `min` (not `omitempty,min`) + guidance so a defaulted parameter cannot slip past its bound. +- New rule **`httputil.prefer-bind`** (info): flags `chi.URLParam` / `chi.URLParamFromCtx` / + `r.URL.Query()` read by hand inside a handler and points to `Bind` — the escape hatch stays for + genuinely custom responses only. + +### Changed + +- The httputil "Escape hatch" note now scopes `HandlerFunc` to custom *responses* (streaming, + non-JSON), not parameter reading. `Bind` symbols/signatures are picked up automatically by the + indexer from the v1.6.0 `web` source. + ## [1.3.3] — 2026-08-09 Patch. Documents the httputil handler adapters and configurable success status. diff --git a/README.md b/README.md index a138ce7..eb7648b 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # einherjar/mcp -[![version](https://img.shields.io/badge/version-v1.3.3-5C4EE5?style=flat-square)](https://code.nochebuena.dev/einherjar/mcp) +[![version](https://img.shields.io/badge/version-v1.3.4-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 f352d2c..21f19ca 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -23,7 +23,7 @@ import ( const ( serverName = "einherjar-mcp" - serverVersion = "v1.3.3" + serverVersion = "v1.3.4" ) func main() { diff --git a/internal/index/builtins/README.md b/internal/index/builtins/README.md index 83a5ff8..7bd1986 100644 --- a/internal/index/builtins/README.md +++ b/internal/index/builtins/README.md @@ -410,26 +410,78 @@ func (h *Handler) CreateUser(w http.ResponseWriter, r *http.Request) { **Adapters and their default success status:** -| Adapter | Body | Default | Use for | +| Adapter | Fills `Req` from | Default | Use for | |---|---|---|---| -| `httputil.Handle(v, logger, fn, opts…)` | req + res | **200** | POST/PUT returning a body | -| `httputil.HandleNoBody(logger, fn, opts…)` | res only | **200** | GET/HEAD | -| `httputil.HandleEmpty(v, logger, fn, opts…)` | req only | **204** | DELETE / body-less PUT | +| `httputil.Handle(v, logger, fn, opts…)` | JSON body | **200** | POST/PUT returning a body | +| `httputil.HandleNoBody(logger, fn, opts…)` | nothing (no input) | **200** | GET/HEAD with no parameters | +| `httputil.HandleEmpty(v, logger, fn, opts…)` | JSON body | **204** | body-less PUT that returns nothing | +| `httputil.Bind(v, logger, fn, opts…)` | **path + query + body** | **200** | any route with `{id}` or `?filters` (v1.6.0) | +| `httputil.BindEmpty(v, logger, fn, opts…)` | **path + query + body** | **204** | `DELETE /x/{id}` and other body-less routes keyed by a path param (v1.6.0) | **Override the success status with `httputil.WithStatus(code)`** — `WithStatus(http.StatusCreated)` -(201) on a create, `WithStatus(http.StatusAccepted)` (202) on an async `HandleEmpty`. The code **must -be 2xx**: these adapters own only the success path, so a non-2xx code is a routing mistake and -**panics at wiring** (the service fails to boot rather than emit a wrong status at runtime). Never +(201) on a create, `WithStatus(http.StatusAccepted)` (202) on an async `HandleEmpty`/`BindEmpty`. The +code **must be 2xx**: these adapters own only the success path, so a non-2xx code is a routing mistake +and **panics at wiring** (the service fails to boot rather than emit a wrong status at runtime). Never pass a 4xx/5xx to `WithStatus`. +### Binding path & query parameters (`Bind` / `BindEmpty`, v1.6.0) + +`Handle`/`HandleEmpty` fill `Req` from the **JSON body only**. A route with an identifier or a +filter needs the path or query too — and hand-rolling that parse in a raw `http.HandlerFunc` is the +one path that reaches production **with no validation** (unclamped bounds; client mistakes returned +as 500s). `Bind` closes the gap: each field declares its source with a struct tag — `path:`, +`query:` or `json:` — and the assembled struct is validated once, exactly like `Handle`. + +```go +// GET /roles/{roleID}?expand=grants — path + query, validated, 200. +type getRoleReq struct { + RoleID uuid.UUID `path:"roleID" validate:"required"` // from the path, already typed + Expand []string `query:"expand" validate:"omitempty,dive,oneof=grants members"` +} + +func (h *Handler) GetRole(w http.ResponseWriter, r *http.Request) { + httputil.Bind(h.v, h.logger, func(ctx context.Context, req getRoleReq) (RoleRes, error) { + return h.svc.Get(ctx, req) + })(w, r) +} + +// DELETE /roles/{roleID} — path only, no body. BindEmpty writes 204 and does NOT +// fail on the empty body the way HandleEmpty would. +type deleteRoleReq struct { + RoleID uuid.UUID `path:"roleID" validate:"required"` +} + +func (h *Handler) DeleteRole(w http.ResponseWriter, r *http.Request) { + httputil.BindEmpty(h.v, h.logger, func(ctx context.Context, req deleteRoleReq) error { + return h.svc.Delete(ctx, req.RoleID) + })(w, r) +} +``` + +Binding rules to encode when generating these structs: + +- **One source per field** — at most one of `path:` / `query:` / `json:`. Two source tags, an + unsupported field type, or a bad `default:` **panic at wiring** (the service fails to boot). +- **Conversion** covers `string`, integer/unsigned/float, `bool`, and any `encoding.TextUnmarshaler` + — so `uuid.UUID` and `time.Time` bind from a path or query value with no extra code. +- **A malformed value is a 400** naming the parameter — never a 500. +- **`default:` fires only when the parameter is absent.** For a paged list use + `` `query:"per_page" default:"50" validate:"min=1,max=200"` `` — pair `default:` with a plain + `min=…`, **not** `omitempty,min=…`: the default guarantees presence, and `omitempty` would let an + explicit `?per_page=0` slip past the bound. +- **Repeated query params bind to a slice** (`?kind=POS&kind=KDS`); a comma inside one value is not split. + **Error status is automatic — never set it by hand.** Return the right `*xerrors.Err` (`xerrors.NotFound(…)`, `xerrors.InvalidInput(…)`, `xerrors.PermissionDenied(…)`, …) and `httputil.Error` maps it to the HTTP status and logs at the derived level (5xx→Error, 4xx→Warn, 499→Info). Validation failures become 400 automatically. -**Escape hatch:** for a response the adapters don't cover, write a raw `http.HandlerFunc` and call -`httputil.JSON(w, status, v)` or `httputil.NoContent(w)` yourself — that is the only place you pass -a status literal. +**Escape hatch:** for a *response* the adapters don't cover — streaming, file downloads, non-JSON +content types — write a raw `http.HandlerFunc` and call `httputil.JSON(w, status, v)` or +`httputil.NoContent(w)` yourself. That is the only place you pass a status literal. It is **not** the +way to read a path or query parameter — use `Bind`/`BindEmpty` so validation still runs; reaching for +`HandlerFunc` + `chi.URLParam` / `r.URL.Query()` just to read a parameter is the anti-pattern v1.6.0 +removed. ## Authorization diff --git a/internal/rules/bind_rules.go b/internal/rules/bind_rules.go new file mode 100644 index 0000000..77941a9 --- /dev/null +++ b/internal/rules/bind_rules.go @@ -0,0 +1,53 @@ +package rules + +import ( + "go/ast" + "strings" +) + +// web/httputil v1.6.0 added Bind/BindEmpty, which fill a typed Req from path and +// query struct tags and validate it once. Reading a parameter by hand — chi.URLParam +// or r.URL.Query() inside a handler — skips that validation, which is the path by +// which unclamped bounds and 500s-that-should-be-400s reach production. This rule +// nudges toward Bind; it is advisory (info), since a genuinely custom response +// (streaming, non-JSON) may still read parameters directly. +func init() { + registered = append(registered, + Rule{ + ID: "httputil.prefer-bind", + Severity: SeverityInfo, + Module: "web", + Check: checkPreferBind, + }, + ) +} + +func checkPreferBind(c *Context) []Finding { + const ( + msg = "reading a path/query parameter by hand skips validation — httputil.Bind / BindEmpty (v1.6.0) fill a typed Req from path:/query: tags and validate it once" + hint = "Declare the parameter as a struct field (path:\"id\" / query:\"page\") and use httputil.Bind. Keep manual parsing only for genuinely custom responses (streaming, non-JSON)." + ) + var hits []Finding + seen := map[int]bool{} + ast.Inspect(c.File, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + name := exprName(call.Fun) + manual := strings.HasSuffix(name, ".URLParam") || + strings.HasSuffix(name, ".URLParamFromCtx") || + strings.HasSuffix(name, ".URL.Query") + if !manual { + return true + } + line := c.Fset.Position(call.Pos()).Line + if seen[line] { + return true + } + seen[line] = true + hits = append(hits, Finding{Message: msg, Hint: hint, Line: line}) + return true + }) + return hits +} diff --git a/internal/rules/bind_rules_test.go b/internal/rules/bind_rules_test.go new file mode 100644 index 0000000..cb06f5b --- /dev/null +++ b/internal/rules/bind_rules_test.go @@ -0,0 +1,48 @@ +package rules + +import "testing" + +const urlParamSnippet = `package handler + +import "github.com/go-chi/chi/v5" + +func (h *Handler) f(r *http.Request) { + id := chi.URLParam(r, "id") + _ = id +} +` + +const urlQuerySnippet = `package handler + +func (h *Handler) f(r *http.Request) { + page := r.URL.Query().Get("page") + _ = page +} +` + +const boundHandlerSnippet = `package handler + +import "code.nochebuena.dev/einherjar/web/httputil" + +func (h *Handler) f() { + _ = httputil.Bind(h.v, h.logger, h.get) +} +` + +func TestPreferBindFiresOnURLParam(t *testing.T) { + if got := findingsFor(Run(urlParamSnippet), "httputil.prefer-bind"); len(got) == 0 { + t.Fatal("httputil.prefer-bind did not fire on chi.URLParam") + } +} + +func TestPreferBindFiresOnURLQuery(t *testing.T) { + if got := findingsFor(Run(urlQuerySnippet), "httputil.prefer-bind"); len(got) == 0 { + t.Fatal("httputil.prefer-bind did not fire on r.URL.Query()") + } +} + +func TestPreferBindNoFalsePositiveOnBind(t *testing.T) { + if got := findingsFor(Run(boundHandlerSnippet), "httputil.prefer-bind"); len(got) != 0 { + t.Errorf("httputil.prefer-bind false-positived on a Bind handler: %+v", got) + } +}