From 77b1a8d29ddcc961be32f05736168edc9ade50d3 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sat, 8 Aug 2026 22:14:50 +0300 Subject: [PATCH] middleware: the shared cache and panic handling --- middleware/middleware.go | 295 ++++++++++++++++++++++++++++++++++ middleware/middleware_test.go | 242 ++++++++++++++++++++++++++++ 2 files changed, 537 insertions(+) create mode 100644 middleware/middleware.go create mode 100644 middleware/middleware_test.go diff --git a/middleware/middleware.go b/middleware/middleware.go new file mode 100644 index 0000000000000000000000000000000000000000..82bf65453fe4e366dcfe4becac2d950857c6df7a --- /dev/null +++ b/middleware/middleware.go @@ -0,0 +1,295 @@ +// Package middleware is the shared HTTP middleware for the custom services of a +// self-hosted SourceHut instance (compare, spec, dolt, cover, bench, ...). +// +// Three things every one of those services needs, and every one of them had +// copied byte for byte into its own web/router.go: the cache policy that keeps a +// page rendered behind a login cookie out of every cache that is not the +// viewer's own, the panic guard that turns a bug in a handler into the service's +// own error page instead of a dropped connection, and the 499 that separates a +// viewer who closed the tab from a server that broke. The copies were identical +// down to their comments, which is the sign that they were never three +// decisions — they were one, made once and pasted twice. This package is the one +// copy. +// +// Usage, in the order the donors install them: +// +// r.Use(middleware.RecoverPanics(func(w http.ResponseWriter, r *http.Request, recovered any) { +// s.renderError(w, r, http.StatusInternalServerError, internalMessage) +// })) +// r.Use(middleware.PrivateCache) +// +// RecoverPanics goes outermost so that it covers every later middleware as well +// as the handlers; PrivateCache goes inside it so that the error page it renders +// carries the same headers as any other answer. +// +// What deliberately did not move here is the donors' getHead helper. It is three +// lines, and all three are chi's — it takes a chi.Router and calls Get and Head +// on it — so hoisting it would put a router dependency in a package whose whole +// point is that it needs nothing but net/http. It stays in each service, where +// the router it talks to already is. +package middleware + +import ( + "bufio" + "errors" + "io" + "log" + "net" + "net/http" + "runtime/debug" +) + +// StatusClientClosedRequest is nginx's 499: the caller went away — hung up, or +// ran out of its own deadline — before the answer was written. +// +// No RFC defines it, and that costs nothing, because the one certain thing about +// this response is that nobody reads it: the context it reports on is the +// request's own, and it ended before there was anything to send. So the code is +// chosen for the operator rather than for the client. What matters is that it is +// not in the 5xx range — that is the rate an alert is written against, and a +// browser navigating away mid-render, or a CI job that pressed ^C, must not page +// anybody — and 499 is the value the log pipelines in front of a SourceHut +// instance already understand, because the nginx that terminates TLS for one has +// been writing it for this exact event since long before any of these services +// existed. +// +// The alternatives are each worse in their own way. 500 is a false statement +// about the instance: nothing broke, and whoever is woken by it finds a healthy +// service. 408 is standard but means the other half of "nobody finished" — the +// server gave up waiting for a body still arriving, which is the server's +// problem and is safe to retry — and several clients do retry it automatically, +// which is the last thing to tell a caller that cancelled on purpose. 504 names +// a gateway timing out upstream, and there is no upstream here. +// +// It belongs in a middleware package rather than next to one service's status +// mapping because both surfaces of every service reach for it: the HTML side +// when a render's context is already cancelled, the API side in its +// error-to-status switch. +const StatusClientClosedRequest = 499 + +// PrivateCache marks every answer of a surface as one no cache may reuse for +// another viewer. +// +// These services render per-viewer documents at URLs that say nothing about the +// viewer: a token list, a repository page that is a page to its owner and a 404 +// to everyone else, a dashboard of somebody's own runs. A cache with no +// instruction treats a 200 to a GET as reusable, so one proxy, one CDN or one +// browser on a shared machine is all it takes for somebody to be served another +// account's page — a disclosure arriving by a route no visibility check can +// stand in front of. +// +// The policy is "private, no-store" and not merely "no-cache" because the two +// answer different questions. no-cache still permits a *stored* copy, and only +// requires it to be revalidated before reuse; the copy sits in the shared proxy +// and on the disk of the shared machine either way, and a revalidation carries +// the next viewer's cookie, not the one the page was rendered for. private bars +// the shared caches from keeping it at all, no-store bars the private ones from +// writing it down, and for a page whose most sensitive form contains a live +// credential in plaintext, "do not write this to disk" is the instruction that +// was actually meant. +// +// Vary names Cookie and Authorization for the same reason: they are the two +// inputs that decide who the page is for, so any cache that does keep something +// must at least not hand it to a request that presented different ones. +// +// It is a middleware and not a line in the render path because render is not the +// only writer: /healthz is text/plain, static assets are bytes, a badge is an +// image, and a header this important must not depend on which write path a +// future page picks. The headers are set before the handler runs, so a handler +// that needs different ones — the static handler, which serves immutable +// hashed-name assets — overrides them with Set. +func PrivateCache(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + SetPrivateCache(w) + next.ServeHTTP(w, r) + }) +} + +// SetPrivateCache writes the two headers PrivateCache exists for. +// +// It is split out because a service also answers from places the router's +// middleware chain does not reach: the deny path called by the authentication +// middleware in front of the whole mount, before the router has seen the request +// at all, is the donors' example. Those answers are as viewer-specific as any +// page and must not be cached either. +func SetPrivateCache(w http.ResponseWriter) { + w.Header().Set("Cache-Control", "private, no-store") + w.Header().Set("Vary", "Cookie, Authorization") +} + +// RecoverPanics turns a panicking handler into the error page the service would +// have written for any other bug. +// +// Without it a panic reaches net/http, which logs it and closes the connection +// without a response: the viewer sees a browser error page, not the service's, +// and an operator sees a stack with no request context around it. It is this and +// not chi's middleware.Recoverer because that one answers with plain text, and +// these surfaces answer with pages. +// +// render is the seam. Each service renders its own 500 — its own chrome, its own +// message, its own template set — so the middleware cannot write the page, only +// decide when one is owed. The callback takes the recovered value so that a +// service which wants to classify the panic can, and a service which does not +// ignores the parameter; a renderer with the donors' (w, r, status, message) +// shape is passed as a one-line closure rather than being reshaped: +// +// middleware.RecoverPanics(func(w http.ResponseWriter, r *http.Request, _ any) { +// s.renderError(w, r, http.StatusInternalServerError, internalMessage) +// }) +// +// A nil render is a wiring mistake and panics here, at construction, rather than +// at 3am inside a deferred function where the only thing left to do about it is +// drop the connection. +// +// The panic value and the stack are logged here and never handed to the viewer — +// an error from below names tables, queries and paths. Logging is the +// middleware's job and not the callback's because the stack is only reachable +// from inside the deferred function that recovered; a service left to log it +// would sooner or later log the value alone, and a panic without a stack is a +// bug report with the address torn off. +// +// Two panics are not this middleware's to answer. +// +// http.ErrAbortHandler is re-panicked, because the standard library defines it +// as "this handler is giving up on this connection on purpose": net/http expects +// to see it, drops the connection silently and logs nothing. Answering it with a +// page would resurrect a response somebody deliberately abandoned. +// +// A panic that happens *after* the response has started is answered by dropping +// the connection, not by rendering. This is where this package parts with its +// donors, which called their error renderer unconditionally: with bytes already +// on the wire that write is a superfluous WriteHeader the standard library logs +// and ignores, followed by an error page appended to the middle of a truncated +// one — a body that is neither document, with a 200 status line in front of it +// claiming both are fine. Panicking with http.ErrAbortHandler instead makes the +// failure legible: the connection dies mid-body, the client sees a short read +// against the Content-Length it was promised (or a chunked stream with no +// terminator) and reports a failed transfer, which is what happened. Detecting +// this is what the response writer is wrapped for. +func RecoverPanics(render func(w http.ResponseWriter, r *http.Request, recovered any)) func(http.Handler) http.Handler { + if render == nil { + panic("middleware: RecoverPanics needs a render callback") + } + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tracked := &startTracker{ResponseWriter: w} + defer func() { + recovered := recover() + if recovered == nil { + return + } + if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) { + panic(recovered) + } + log.Printf("middleware: panic serving %s %s: %v\n%s", + r.Method, r.URL.Path, recovered, debug.Stack()) + if tracked.started { + // Half a page is already out. There is no status line left + // to send and nothing useful to append; abandon the + // connection instead of corrupting the body further. + panic(http.ErrAbortHandler) + } + renderOnce(render, tracked, r, recovered) + }() + next.ServeHTTP(tracked, r) + }) + } +} + +// renderOnce calls render under a guard of its own, so that a panic while +// rendering the error page cannot be fed back into the path that renders error +// pages. +// +// The second failure is logged and the connection dropped. It is deliberately +// not a second attempt at a page: whatever is broken — a template, the chrome, +// the store the chrome reads a username from — is exactly what the retry would +// use, and the third try would be no different from the second. One log line +// naming both panics is what an operator needs; a loop is what they would +// otherwise get. +func renderOnce( + render func(w http.ResponseWriter, r *http.Request, recovered any), + w http.ResponseWriter, + r *http.Request, + recovered any, +) { + defer func() { + second := recover() + if second == nil { + return + } + if err, ok := second.(error); ok && errors.Is(err, http.ErrAbortHandler) { + panic(second) + } + log.Printf("middleware: panic rendering the error page for %s %s: %v (original panic: %v)\n%s", + r.Method, r.URL.Path, second, recovered, debug.Stack()) + panic(http.ErrAbortHandler) + }() + render(w, r, recovered) +} + +// startTracker records whether anything has reached the wire yet. +// +// "The response has started" is the one fact RecoverPanics needs and net/http +// does not expose: by the time a panic is recovered, the only way to know +// whether a status line has already gone out is to have watched for it. Every +// method that commits the response — an explicit WriteHeader, the implicit one +// inside the first Write, a Flush, a Hijack — sets the flag before delegating. +// +// Wrapping a ResponseWriter costs the concrete type behind it, so the methods a +// handler may reasonably reach for are carried across: +// +// - Unwrap is the net/http convention (Go 1.20+) that lets an +// http.ResponseController find the real writer, which is how deadlines and +// flushes are meant to be reached through wrappers like this one. +// - Flush and Hijack are implemented directly as well, because plenty of code +// still type-asserts for http.Flusher and http.Hijacker rather than going +// through the controller. They delegate through the controller, which +// returns http.ErrNotSupported if the writer underneath genuinely cannot do +// it — the same outcome as the assertion having failed, minus the silence. +// - ReadFrom keeps http.ServeContent and io.Copy on the fast path: net/http's +// own writer implements io.ReaderFrom, and losing it would turn every static +// asset into a buffered copy loop. +type startTracker struct { + http.ResponseWriter + started bool +} + +func (t *startTracker) WriteHeader(status int) { + t.started = true + t.ResponseWriter.WriteHeader(status) +} + +func (t *startTracker) Write(b []byte) (int, error) { + t.started = true + return t.ResponseWriter.Write(b) +} + +// Unwrap gives http.ResponseController the writer this one wraps. +func (t *startTracker) Unwrap() http.ResponseWriter { + return t.ResponseWriter +} + +// Flush commits whatever is buffered, which starts the response. +func (t *startTracker) Flush() { + t.started = true + // The error is the writer saying it cannot flush, which is what an + // unsatisfied http.Flusher assertion would have said by not existing. + _ = http.NewResponseController(t.ResponseWriter).Flush() +} + +// Hijack hands the connection to the caller, after which nothing here can write +// a status line — so the response counts as started. +func (t *startTracker) Hijack() (net.Conn, *bufio.ReadWriter, error) { + t.started = true + return http.NewResponseController(t.ResponseWriter).Hijack() +} + +// ReadFrom preserves the io.ReaderFrom fast path of the writer underneath. +func (t *startTracker) ReadFrom(src io.Reader) (int64, error) { + t.started = true + if rf, ok := t.ResponseWriter.(io.ReaderFrom); ok { + return rf.ReadFrom(src) + } + // Copy to the wrapped writer and not to t, or this is a recursion. + return io.Copy(t.ResponseWriter, src) +} diff --git a/middleware/middleware_test.go b/middleware/middleware_test.go new file mode 100644 index 0000000000000000000000000000000000000000..fb0e80827b16f001b8f38b3731d62e8d3434dbb1 --- /dev/null +++ b/middleware/middleware_test.go @@ -0,0 +1,242 @@ +package middleware + +import ( + "bytes" + "log" + "net" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// captureLog redirects the standard logger into a buffer for the duration of one +// test, both to keep the panic stacks out of the test output and so that a test +// can assert on what an operator would have seen. +func captureLog(t *testing.T) *bytes.Buffer { + t.Helper() + + var buf bytes.Buffer + flags := log.Flags() + log.SetOutput(&buf) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(os.Stderr) + log.SetFlags(flags) + }) + + return &buf +} + +// renderInternal is the shape a service passes in: its own error page, at 500, +// ignoring the recovered value. +func renderInternal(w http.ResponseWriter, _ *http.Request, _ any) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Something went wrong.")) +} + +func TestPrivateCacheSetsBothHeadersBeforeTheHandlerRuns(t *testing.T) { + var seen http.Header + h := PrivateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + seen = w.Header().Clone() + w.WriteHeader(http.StatusOK) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/tokens", nil)) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "private, no-store", rec.Header().Get("Cache-Control")) + assert.Equal(t, "Cookie, Authorization", rec.Header().Get("Vary")) + + // Before, not after: a handler that writes its own body must already have + // them, or a flush would put the status line on the wire without them. + assert.Equal(t, "private, no-store", seen.Get("Cache-Control")) + assert.Equal(t, "Cookie, Authorization", seen.Get("Vary")) +} + +func TestPrivateCacheLetsAHandlerOverrideThem(t *testing.T) { + h := PrivateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") + w.WriteHeader(http.StatusOK) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/static/app.abc123.css", nil)) + + assert.Equal(t, "public, max-age=31536000, immutable", rec.Header().Get("Cache-Control")) +} + +func TestSetPrivateCacheWritesTheHeadersWithoutAHandler(t *testing.T) { + rec := httptest.NewRecorder() + SetPrivateCache(rec) + + assert.Equal(t, "private, no-store", rec.Header().Get("Cache-Control")) + assert.Equal(t, "Cookie, Authorization", rec.Header().Get("Vary")) +} + +func TestRecoverPanicsPassesANonPanickingHandlerThrough(t *testing.T) { + rendered := false + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { rendered = true })( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + _, _ = w.Write([]byte("fine")) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) + + assert.Equal(t, http.StatusTeapot, rec.Code) + assert.Equal(t, "fine", rec.Body.String()) + assert.False(t, rendered, "the callback is for panics only") +} + +func TestRecoverPanicsRendersTheErrorPageAndAnswers500(t *testing.T) { + logged := captureLog(t) + + var recovered any + calls := 0 + h := RecoverPanics(func(w http.ResponseWriter, r *http.Request, v any) { + calls++ + recovered = v + renderInternal(w, r, v) + })(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + panic("the store is nil") + })) + + rec := httptest.NewRecorder() + require.NotPanics(t, func() { + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/tokens", nil)) + }) + + assert.Equal(t, 1, calls) + assert.Equal(t, "the store is nil", recovered, "the callback gets the recovered value") + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Contains(t, rec.Body.String(), "Something went wrong.") + + // The detail goes to the log, with the request around it and a stack. + assert.Contains(t, logged.String(), "panic serving GET /tokens: the store is nil") + assert.Contains(t, logged.String(), "runtime/debug.Stack") + assert.NotContains(t, rec.Body.String(), "the store is nil", "never to the viewer") +} + +func TestRecoverPanicsPassesAnErrorValueThroughUnwrapped(t *testing.T) { + captureLog(t) + + boom := &net.AddrError{Err: "boom", Addr: "nowhere"} + var recovered any + h := RecoverPanics(func(w http.ResponseWriter, r *http.Request, v any) { + recovered = v + renderInternal(w, r, v) + })(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + panic(boom) + })) + + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + + assert.Same(t, boom, recovered, "the value arrives as it was thrown, not stringified") +} + +func TestRecoverPanicsAbandonsAResponseThatHasStarted(t *testing.T) { + logged := captureLog(t) + + rendered := false + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { rendered = true })( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("half a p")) + panic("template died mid-page") + })) + + rec := httptest.NewRecorder() + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/repos", nil)) + }, "net/http is told to drop the connection") + + assert.False(t, rendered, "there is no status line left to render a page over") + assert.Equal(t, http.StatusOK, rec.Code, "the status already sent is not rewritten") + assert.Equal(t, "half a p", rec.Body.String(), "nothing is appended to the truncated body") + assert.Contains(t, logged.String(), "panic serving GET /repos: template died mid-page") +} + +func TestRecoverPanicsCountsAFlushAsAStartedResponse(t *testing.T) { + captureLog(t) + + rendered := false + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { rendered = true })( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + flusher, ok := w.(http.Flusher) + require.True(t, ok, "the wrapper keeps http.Flusher") + flusher.Flush() + panic("after the flush") + })) + + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + }) + assert.False(t, rendered) +} + +func TestRecoverPanicsRepanicsErrAbortHandler(t *testing.T) { + rendered := false + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { rendered = true })( + http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + panic(http.ErrAbortHandler) + })) + + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + }) + assert.False(t, rendered, "a deliberately abandoned response is not resurrected") +} + +func TestRecoverPanicsDoesNotLoopWhenTheErrorPagePanics(t *testing.T) { + logged := captureLog(t) + + calls := 0 + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { + calls++ + panic("the chrome is broken too") + })(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + panic("the store is nil") + })) + + rec := httptest.NewRecorder() + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/tokens", nil)) + }) + + assert.Equal(t, 1, calls, "the error page is attempted exactly once") + assert.Contains(t, logged.String(), "panic rendering the error page for GET /tokens: the chrome is broken too") + assert.Contains(t, logged.String(), "original panic: the store is nil") +} + +func TestRecoverPanicsForwardsErrAbortHandlerFromTheErrorPage(t *testing.T) { + captureLog(t) + + h := RecoverPanics(func(http.ResponseWriter, *http.Request, any) { + panic(http.ErrAbortHandler) + })(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + panic("the store is nil") + })) + + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + }) +} + +func TestRecoverPanicsRequiresARenderCallback(t *testing.T) { + assert.PanicsWithValue(t, "middleware: RecoverPanics needs a render callback", func() { + RecoverPanics(nil) + }, "a wiring mistake fails at wiring time") +} + +func TestStatusClientClosedRequestIsNotAServerError(t *testing.T) { + assert.Equal(t, 499, StatusClientClosedRequest) + assert.Less(t, StatusClientClosedRequest, 500, + "a viewer who went away must not land in the rate an alert is written against") +}