From e01e9ed02ec22b7bdebac9ae8327784c4fac2245 Mon Sep 17 00:00:00 2001 From: bigbes Date: Sun, 9 Aug 2026 00:25:48 +0300 Subject: [PATCH] chimw: the request line in the journal, HEAD routes, and a 405 page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things at once, all of them the chi half of the shared middleware. RequestLogger replaces chi's Logger, which wrote an unstructured line to stdout — the highest-volume record this daemon emits and the only one not beside the rest on stderr. RequestID goes above it so the request line and a panic report share an id, Recoverer below it so its own report goes through the entry instead of to stdout, and /healthz is skipped. GetHead registers every read route under HEAD as well. Until now `curl -I` and every uptime probe were answered 405 plus a kilobyte of rendered error page by pages whose whole job is to be cheap to ask about. RenderRefusals installs both routing failures against renderError; this service had the 404 alone and left the 405 to net/http's plain text. The new test that pins it also corrects what TestUnsafeMethodRefused claimed: the group's middleware runs before either refusal, so an unsafe method on a GET-only path is a 403 from the same-origin guard, not a 405. --- README.md | 6 ++-- cmd/comparesrht/main.go | 27 ++++++++++++----- web/router.go | 26 ++++++++++------ web/server.go | 3 +- web/web_test.go | 66 +++++++++++++++++++++++++++++++++++++---- 5 files changed, 103 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 90f78026dbf043185a6e58d2966ecf1bfcba726c..4b67cf98b5a40d38bb536090a0eed5501860212e 100644 --- a/README.md +++ b/README.md @@ -51,8 +51,10 @@ authorizer spares git.sr.ht a GraphQL call on every page load. the machinery around them is not here but in [sr-ht-ecore], wired up in `web/server.go`: `chrome` for the page frame, `pages` for template discovery and the shared error page, `assets` for the hashed artefacts and their cache - policy, `csrf` and `middleware` for the router's guards. What is left is this - service's own: the routes, the handlers, and the templates they render. + policy, `csrf` and `middleware` for the router's guards, `chimw` for the + GET+HEAD route pair, the rendered 404/405 and the request log record. What is + left is this service's own: the routes, the handlers, and the templates they + render. - `frontend/` + `scss/` — build-time TypeScript diff bundle and the SCSS entry. - `cmd/comparesrht/` — the daemon entry point and startup validation. - `contrib/` — nginx server block, systemd unit, and a dev GraphQL stub. diff --git a/cmd/comparesrht/main.go b/cmd/comparesrht/main.go index 6d59b58f43d5647339d1bb2199796a18d91a2997..7238dea5a2f99afd620578625b31ad6e31f82a77 100644 --- a/cmd/comparesrht/main.go +++ b/cmd/comparesrht/main.go @@ -32,11 +32,12 @@ import ( "time" "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" + chimiddleware "github.com/go-chi/chi/v5/middleware" "github.com/vaughan0/go-ini" "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-core/config" coreserver "sourcecraft.dev/bigbes/sr-ht-core/server" + "sourcecraft.dev/bigbes/sr-ht-ecore/chimw" "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" "sourcecraft.dev/bigbes/sr-ht-ecore/logging" "sourcecraft.dev/bigbes/sr-ht-ecore/login" @@ -135,13 +136,25 @@ func main() { // Register installs three more of its own inside a nested group: the // private cache policy, panic recovery through the service's error page, // and the same-origin guard. chi's Recoverer stays here as the outer net - // for a panic in the two middlewares above, which are outside that group — - // it re-panics http.ErrAbortHandler, which is what the inner one raises for - // a panic arriving after the response has already started. + // for a panic in the middlewares above, which are outside that group — it + // re-panics http.ErrAbortHandler, which is what the inner one raises for a + // panic arriving after the response has already started. + // + // The request line is chimw's and no longer chi's. chi's Logger writes an + // unstructured, colourised line to *stdout*, which on this daemon is the + // highest-volume record it emits and the only one not beside the rest in + // the journal: an operator grepping stderr for a path finds every panic and + // none of the requests that caused them. RequestID goes above it so the + // request line and the panic report carry the same id, and Recoverer below + // it so its own report goes through the log entry rather than to stdout. + // /healthz is skipped: it is a probe every second and says nothing. srv.AnonRouter().Group(func(r chi.Router) { - r.Use(middleware.RealIP) - r.Use(middleware.Recoverer) - r.Use(middleware.Logger) + r.Use(chimiddleware.RequestID) + r.Use(chimiddleware.RealIP) + r.Use(chimw.RequestLogger(chimw.SlogFormatter{ + Skip: chimw.SkipPaths("/healthz"), + })) + r.Use(chimiddleware.Recoverer) r.Use(config.Middleware(conf, service)) // The instance's one cookie decode, with the default validator: a name // this service narrowed further would be an account logged out of diff --git a/web/router.go b/web/router.go index 094e12e0ea2cc4fdc606ae351f240be6d588ebea..2d7a9f59a719c8160ecbf3992e5b13f41e94c951 100644 --- a/web/router.go +++ b/web/router.go @@ -6,6 +6,7 @@ import ( "github.com/go-chi/chi/v5" "sourcecraft.dev/bigbes/sr-ht-ecore/assets" + "sourcecraft.dev/bigbes/sr-ht-ecore/chimw" "sourcecraft.dev/bigbes/sr-ht-ecore/csrf" "sourcecraft.dev/bigbes/sr-ht-ecore/middleware" ) @@ -30,8 +31,15 @@ import ( // back on — the session cookie is meta.sr.ht's, set on the parent domain, // with a SameSite no individual service can choose. // -// The router's own 404 is set here too, so a mistyped URL lands on a page with a -// nav rather than on chi's plain-text dead end. +// Both of chi's routing failures are pointed at this service's error page here, +// so a mistyped URL and a method this router does not serve land on a page with +// a nav rather than on chi's plain-text dead end. The 405 is new: until ecore +// grew chimw.RenderRefusals this service installed the 404 alone and left the +// other refusal to net/http. +// +// Every read route is registered under GET and HEAD both. chi's Get registers +// GET alone, so `curl -I` and every uptime probe used to be answered 405 by +// pages whose whole job is to be cheap to ask about. func (s *Server) Register(r chi.Router) { r.Group(func(r chi.Router) { r.Use(middleware.PrivateCache) @@ -40,18 +48,18 @@ func (s *Server) Register(r chi.Router) { })) r.Use(csrf.Require(s.chromeSvc.SelfOrigin(), nil)) - r.NotFound(s.handleNotFound) + chimw.RenderRefusals(r, s.renderError) - r.Get("/", s.handleIndex) - r.Get("/jump", s.handleJump) - r.Get("/healthz", s.handleHealthz) + chimw.GetHead(r, "/", s.handleIndex) + chimw.GetHead(r, "/jump", s.handleJump) + chimw.GetHead(r, "/healthz", s.handleHealthz) r.Handle(assets.DefaultPrefix+"*", s.static) - r.Get("/~{owner}/{repo}", s.handleRepo) + chimw.GetHead(r, "/~{owner}/{repo}", s.handleRepo) // A single wildcard route serves both the form target (empty wildcard ⇒ // redirect to the canonical URL) and the compare view itself. - r.Get("/~{owner}/{repo}/compare/*", s.handleCompare) - r.Get("/~{owner}/{repo}/commit/{rev}", s.handleCommit) + chimw.GetHead(r, "/~{owner}/{repo}/compare/*", s.handleCompare) + chimw.GetHead(r, "/~{owner}/{repo}/commit/{rev}", s.handleCommit) }) } diff --git a/web/server.go b/web/server.go index 2af634133b9d23ec874a28caff1fede37c8830f5..35e55947c4ec9defffd59f3d6ce70ca0cfcfe4ed 100644 --- a/web/server.go +++ b/web/server.go @@ -34,9 +34,10 @@ // guard — and assumes the following is already applied to the router it is // handed, in this order (outermost first): // +// chi middleware.RequestID // chi middleware.RealIP +// chimw.RequestLogger(...) // the request line, as a slog record // chi middleware.Recoverer -// chi middleware.Logger (optional, but recommended) // config.Middleware(conf, "compare.sr.ht") // required: authz + gitx read it // login.Optional() // required: never 401s; sets the viewer // diff --git a/web/web_test.go b/web/web_test.go index 5d5bc69fa68ec51a43c568f8a078f290d440bd58..67ea01b9b9de6e73debe47bf0773396a1e614feb 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -379,6 +379,60 @@ func TestHealthz(t *testing.T) { assert.Contains(t, rec.Body.String(), "ok") } +// TestHeadIsServedWhereGetIs pins what chimw.GetHead registers. chi's Get alone +// left every read route answering `curl -I` — and every uptime probe, and every +// cache revalidating what it holds — with a 405 plus a rendered error page from +// a path whose whole job is to be cheap to ask about. +// +// The status has to be the GET's, on every route: a HEAD that answered 200 where +// the GET answers 404 would be the private-repo leak the 404 exists to prevent. +func TestHeadIsServedWhereGetIs(t *testing.T) { + root, mainSHA := gitFixture(t) + h := testServer(t, root, demoAuthorizer()) + + for _, target := range []string{ + "/", + "/healthz", + "/~alice/demo", + "/~alice/demo/compare/main...feature", + "/~alice/demo/commit/" + mainSHA, + } { + req := httptest.NewRequest(http.MethodHead, target, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.Equalf(t, http.StatusOK, rec.Code, "HEAD %s", target) + } + + // And a hidden repository is a 404 to HEAD exactly as it is to GET. + req := httptest.NewRequest(http.MethodHead, "/~alice/nope", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.Equal(t, http.StatusNotFound, rec.Code, "HEAD on a hidden repo") +} + +// TestMethodNotAllowedIsAPage covers the refusal this service did not render +// before chimw.RenderRefusals: it installed chi's 404 alone, so a method the +// router does not serve produced net/http's plain-text 405 — the one answer on +// the instance that did not look like the service it came from. +// +// The probe is OPTIONS rather than a write method, and the reason is worth +// keeping: the group's middleware does run before chi answers a 405, so an +// unsafe method that carries no Origin is refused 403 by the same-origin guard +// and never reaches this handler at all. OPTIONS is safe, passes the guard, and +// is registered nowhere. +func TestMethodNotAllowedIsAPage(t *testing.T) { + root, _ := gitFixture(t) + h := testServer(t, root, demoAuthorizer()) + + req := httptest.NewRequest(http.MethodOptions, "/healthz", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + require.Equal(t, http.StatusMethodNotAllowed, rec.Code) + assert.Contains(t, rec.Body.String(), pages.MethodMessage) + assert.Contains(t, rec.Body.String(), "navbar-brand", "the 405 has no chrome") +} + func TestStaticBundleAndCSS(t *testing.T) { root, _ := gitFixture(t) h := testServer(t, root, demoAuthorizer()) @@ -509,12 +563,12 @@ func TestBadSpecKeepsItsOwnMessage(t *testing.T) { // today, so what this asserts is that the guard is installed at all — the day a // form arrives it is already covered. // -// The probe is a path with no route, because that is the one unsafe request -// this router hands to the guard: chi resolves a method before a group's -// middleware runs, so a POST to a GET-only path is its own 405, while a POST to -// nothing at all reaches the not-found handler through the whole chain. The -// consequence to know about is in the first arm — such a request is now refused -// 403 rather than answered 404. +// The probe is a path with no route, but any path would do: the group's +// middleware runs before chi answers either of its routing failures, so a POST +// to a GET-only path is refused 403 by the guard exactly as a POST to nothing at +// all is, and neither reaches the 405 or the 404 behind it. The consequence to +// know about is in the first arm — such a request is refused 403 rather than +// answered 404. func TestUnsafeMethodRefused(t *testing.T) { root, _ := gitFixture(t) h := testServer(t, root, demoAuthorizer())