@@ 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)
+}
@@ 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("<html>Something went wrong.</html>"))
+}
+
+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("<html>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, "<html>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")
+}