chore(server): S01 middleware chain (Step 3)

Suleimi Ahmedrequested to merge
sahmed/s01-step-3-middleware into main

Summary

Introduces the S01 middleware chain at internal/server/middleware.go: four middlewares occupying spec positions #5/#6 (closed)/#7 (closed) plus the ServeMux response interceptor that rewrites stdlib 404/405 plain-text fallbacks into the S01 JSON envelope.

  • HeaderHardeningMiddleware — sets X-Content-Type-Options: nosniff on every response (AC #17 (closed)).
  • BodySizeMiddleware — wraps the request body with http.MaxBytesReader, early-rejects on Content-Length > MaxBodySize (AC #13 (closed)), and stashes the pre-wrap body under a private context key so upload handlers can call OriginalBody(r) to retrieve it for their own size-limit management.
  • RoutePatternMiddleware — propagates the matched ServeMux pattern via httpserver.SetRoutePattern so LabKit's tracing middleware (chain position #2) uses a low-cardinality span name.
  • ResponseInterceptorMiddleware — captures stdlib mux's plain-text 404/405 and rewrites them as JSON envelopes via transport.WriteError, preserving the Allow header on 405 (ACs #8 (closed)/#9). The interceptor implements Unwrap so http.NewResponseController can reach the underlying writer for Hijack/Flush/SetReadDeadline/SetWriteDeadline (S01 §"ResponseWriter unwrapping").
  • OriginalBody(r io.Reader) io.ReadCloser — exported accessor; returns nil when the middleware chain didn't run.

Leaf code; no callers on this branch. Step 5 (server.New) wires these into the chain on top of LabKit positions #1-#4 (closed).

Acceptance criteria

AC Covered by
#8 (closed) (unknown route → 404 envelope) TestResponseInterceptor_Rewrites404PlainText
#9 (wrong method → 405 envelope + Allow preserved) TestResponseInterceptor_Rewrites405PlainText, TestResponseInterceptor_PreservesAllowHeader
#13 (closed) (oversized body → 413; Content-Length early-reject AND chunked MaxBytesReader) TestBodySizeMiddleware_RejectsOversizedContentLength, TestBodySizeMiddleware_RejectsOversizedChunked
#17 (closed) (X-Content-Type-Options: nosniff on every response) TestHeaderHardeningMiddleware_SetsNosniff (table-driven 200/500)
D9 (handler-set non-text Content-Type passes through interceptor) TestResponseInterceptor_DoesNotRewriteHandlerJSON, TestResponseInterceptor_DoesNotRewriteHandlerNon404OrNon405

Additional behaviors pinned:

  • TestBodySizeMiddleware_OriginalBodyAccessor — OriginalBody(r) returns the pre-wrap body.
  • TestRoutePatternMiddleware_PropagatesPattern — pattern propagates via httpserver.SetRoutePattern.
  • TestResponseInterceptor_UnwrapReachesUnderlying — http.NewResponseController(interceptor).Flush() reaches the underlying writer.

Test plan

  • go test -race -short -count=1 ./internal/server/... — green
  • golangci-lint v2.12.0 — 0 issues
  • goimports -l — clean
  • validate-log-fields v2.6.1 — no deprecated logging fields
  • All 10 plan-listed tests present + 1 polish-added test (UnwrapReachesUnderlying)
  • /validate-step second pass: READY TO PUSH (all 5 categories PASS after /review-branch polish)

Reviewability notes

  • MR size: 698 LOC across 2 files (222 production + 476 test = 68% tests). Exceeds the 500 LOC ceiling in docs/dev/development-model.md, but per the project's test-first contract documented in docs/dev/agentic-development.md, the test surface is authored by a separate subagent before implementation. The production payload (222 LOC) is well within budget; splitting tests into a separate MR would defeat the test-first verification flow. Recommended reading order: test file first (it's the contract), then implementation.
  • Chore framing. This MR introduces no user-visible behavior until Step 5 wires the middleware into server.New. The components sit unwired in internal/server/ after merge — hence chore(server) and type::maintenance rather than feat/type::feature (consistent with !265 (merged)'s posture).
  • responseInterceptor.Unwrap() was added per /review-branch finding B1 (BLOCKING spec drift). S01 §"ResponseWriter unwrapping" mandates Unwrap on every wrapper so http.NewResponseController can reach Hijack/Flush/SetReadDeadline/SetWriteDeadline. Without it, upload handlers (S10+) calling SetReadDeadline per S01 §"Upload timeout extension" would silently receive http.ErrNotSupported. The new TestResponseInterceptor_UnwrapReachesUnderlying pins the contract.
  • Sibling MR (!265 (merged), Step 4). Both this MR and !265 (merged) add files to internal/server/. No conflict — different files (middleware.go vs shutdown.go). Go tolerates the byte-identical package doc-comments.
  • Forward notes (/review-branch OBSERVATIONs deferred):
    • O3: OriginalBody's no-stash branch (returns nil) isn't tested. Three-line follow-up.
    • O4: httpserver.SetRoutePattern's return value is discarded. Benign on the production path where LabKit's tracing seeds a holder.
    • O5: originalBodyCtxKey doc-comment doesn't explain the empty-struct context-key idiom (staticcheck SA1029).
    • O6: chore vs feat commit type — same posture as !254 (merged)/!261 (merged)/!265 (merged) per operator decision.

Related

  • Step 1 (internal/config): !254 (merged) — merged
  • Step 2 (internal/transport): !261 (merged) — merged
  • Step 4 (internal/server/shutdown): !265 (merged) — in review (sibling, parallel)
  • Step 5 (server.New) and Step 6 (composition root) consume these middlewares

Merge request reports