diff --git a/web/router.go b/web/router.go index 9091803dfa873228a495f01cdc36dd75124bc856..46b9210503d6bfb2a7aa9f1e5af2bb1496700a47 100644 --- a/web/router.go +++ b/web/router.go @@ -6,21 +6,40 @@ "net/url" "strings" "github.com/go-chi/chi/v5" + chimiddleware "github.com/go-chi/chi/v5/middleware" "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" ) // Handler returns a router with everything this package needs already -// installed: panic recovery, the private cache policy, the authn principal -// middleware and the same-origin guard, then the routes. The daemon mounts it -// at "/". +// installed: the request line, panic recovery, the private cache policy, the +// authn principal middleware and the same-origin guard, then the routes, then +// the two refusals chi answers when routing fails. The daemon mounts it at "/". // -// The order is the one the shared packages ask for. RecoverPanics is outermost -// so it covers the later middleware as well as the handlers, and it is -// sr-ht-ecore's rather than chi's or our own for one behaviour: a panic that -// arrives *after* the response has started aborts the connection instead of +// The order is the one the shared packages ask for. RequestID and RealIP first, +// because chimw.RequestLogger reads both and can only carry an id and the +// viewer's address if they have already run. The logger is next and — the one +// piece of ordering that is not cosmetic — *outside* RecoverPanics rather than +// under it: it has to observe the status that actually went out, and for a +// panicking handler that status is the 500 RecoverPanics renders. Installed the +// other way round it would see an unwinding stack and nothing written, and would +// log the request that produced an error page as if it had produced nothing. +// +// Nothing is left uncovered by putting it outside: this middleware builds a +// record and calls slog, and a panic in slog is not a failure any error page was +// going to survive either. +// +// Until now this surface logged no request lines at all. core-go installs chi's +// own Logger, but only under -d and only on the groups it registers itself, +// which the router mounted at "/" is not one of — so a 500 here left a stack in +// the journal with no line saying which URL produced it. +// +// RecoverPanics then covers every later middleware as well as the handlers, and +// it is sr-ht-ecore's rather than chi's or our own for one behaviour: a panic +// that arrives *after* the response has started aborts the connection instead of // appending an error page to a truncated one. PrivateCache sits inside it so // that every answer — including the two refusals below — carries the same // private, no-store a page rendered behind a login cookie needs. @@ -40,6 +59,9 @@ // authn.Resolver.Middleware to it — uses Register instead, and owes its router // this guard: Register installs no middleware of its own. func (s *Server) Handler() http.Handler { r := chi.NewRouter() + r.Use(chimiddleware.RequestID) + r.Use(chimiddleware.RealIP) + r.Use(chimw.RequestLogger(chimw.SlogFormatter{Skip: chimw.SkipPaths("/healthz")})) r.Use(middleware.RecoverPanics(func(w http.ResponseWriter, r *http.Request, _ any) { s.renderError(w, r, http.StatusInternalServerError, "") })) @@ -49,6 +71,15 @@ r.Use(csrf.Require(s.chromeSvc.SelfOrigin(), func(w http.ResponseWriter, r *http.Request) { s.renderError(w, r, http.StatusForbidden, csrf.Message) })) s.Register(r) + + // The two refusals chi answers when routing fails, pointed at this service's + // error page. Without them a mistyped URL — the one refusal a viewer is most + // likely to meet — came back as net/http's plain text, with no nav to get out + // of and nothing to say which service it came from, while every refusal a + // handler produced was a rendered page. It is registered here rather than in + // Register because it is not a route: a caller that owns its own router gets + // its own answer for an address it does not serve. + chimw.RenderRefusals(r, s.renderError) return r } @@ -64,32 +95,43 @@ // ".json" are stripped from the tail by the handler, never routed on, so a // document called "notes/2026.json.md" is still reachable and a request for // "notes/2026.json" still means "the JSON of notes/2026". func (s *Server) Register(r chi.Router) { - r.Get("/", s.handleIndex) - r.Get("/healthz", s.handleHealthz) + // Every read route is registered for GET and HEAD both, through + // chimw.GetHead. chi registers GET alone and net/http synthesizes nothing, so + // each of these answered `curl -I` — and every uptime probe, and every cache + // revalidating what it holds — with a 405 naming GET as the only method it + // takes, plus a page of rendered chrome from a path whose whole job is to be + // cheap. The pair shares one handler, so a HEAD produces exactly the status + // its GET would: there is no second path that could answer 200 where the GET + // answers 404, which on these pages would be a visibility leak. + // + // The mutating routes are deliberately not in it. A HEAD that writes is not a + // HEAD, so each POST stays a POST and a HEAD on that path resolves to the page. + chimw.GetHead(r, "/", s.handleIndex) + chimw.GetHead(r, "/healthz", s.handleHealthz) r.Mount(assets.DefaultPrefix, s.static) - r.Get("/search", s.handleSearch) - r.Get("/inbox", s.handleInbox) + chimw.GetHead(r, "/search", s.handleSearch) + chimw.GetHead(r, "/inbox", s.handleInbox) r.Post("/inbox/seen", s.handleInboxSeen) // /tokens redirects to tokens.sr.ht, which issues every agent credential on // the instance. The POST routes that minted and revoked here went with the // table behind them; the GET stays so that a bookmark, the dashboard button // and every doc that ever said "see /tokens" still land somewhere useful. - r.Get("/tokens", s.handleTokens) + chimw.GetHead(r, "/tokens", s.handleTokens) // The proposal routes are registered before the document wildcard. chi gives // the static "p" segment priority over the "*" catch-all regardless, but // keeping them adjacent makes the "/p/ is the proposal namespace" decision // visible in one place. - r.Get("/~{owner}/{space}/p/{id}", s.handleProposal) + chimw.GetHead(r, "/~{owner}/{space}/p/{id}", s.handleProposal) r.Post("/~{owner}/{space}/p/{id}/approve", s.handleProposalApprove) r.Post("/~{owner}/{space}/p/{id}/reject", s.handleProposalReject) r.Post("/~{owner}/{space}/p/{id}/comment", s.handleProposalComment) r.Post("/~{owner}/{space}/p/{id}/reply", s.handleProposalReply) r.Post("/~{owner}/{space}/p/{id}/resolve", s.handleProposalResolve) - r.Get("/~{owner}/{space}", s.handleSpace) - r.Get("/~{owner}/{space}/*", s.handleDocument) + chimw.GetHead(r, "/~{owner}/{space}", s.handleSpace) + chimw.GetHead(r, "/~{owner}/{space}/*", s.handleDocument) } // handleHealthz is a dependency-free liveness probe. diff --git a/web/server.go b/web/server.go index 0dbe43734a1fed8de7171283f6360cf85e917292..6eef2f0ff78289f1079b9501f0802230d4a82c8f 100644 --- a/web/server.go +++ b/web/server.go @@ -14,20 +14,25 @@ // here rebuilds the switcher or re-derives a login URL: this service's copy of // that code — inherited from compare.sr.ht, which had inherited it from // somewhere else — is what ecore exists to have deleted. // -// Four more of ecore's packages carry what used to be local copies of the same -// idea, and the pattern is the same every time — the rule lives in one place -// and this package supplies only what is genuinely spec.sr.ht's: +// Five more of ecore's packages carry what used to be local copies of the same +// idea, or what no copy here ever had, and the pattern is the same every time — +// the rule lives in one place and this package supplies only what is genuinely +// spec.sr.ht's: // // - pages discovers the page templates, refuses at startup a page that // defines no "content", renders into a buffer before touching the response // and ships the shared error body. What stays here is renderError, which // wraps that body in this service's view struct, and fail, which maps this -// service's own sentinels onto statuses. -// - assets finds the hashed stylesheet and serves the static tree with the -// cache policy each name implies. +// service's own sentinels onto statuses. FormValues is its other half: a +// bounded read that answers r.PostForm and never r.Form. +// - assets finds the hashed stylesheet and the favicon and serves the static +// tree with the cache policy each name implies. // - csrf is the same-origin guard, installed on the router rather than called // by three handlers — see [Server.Handler]. // - middleware is the private-cache policy and the panic guard. +// - chimw is the chi-shaped half: the request line as a slog record, the read +// routes registered for GET and HEAD both, and the two refusals chi answers +// when routing fails, which this surface previously did not answer at all. // // pages.Render answers the response itself and returns an error only for the // log. It must never be handed to fail: that would either write a second diff --git a/web/web_test.go b/web/web_test.go index ee25c11488d8cecc806779fb7e53ab488631c3e9..803aeab527971e025b045bc9a16acfb97e7966c4 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -707,6 +707,49 @@ t.Fatalf("status = %d, want 404", rec.Code) } } +// The router's own two refusals are this service's error page and not net/http's +// plain text. They are the ones a viewer is most likely to meet — a mistyped URL +// and a stale bookmark — and until chimw.RenderRefusals was installed they were +// the only refusals on this surface with no nav to get out of. +func TestRoutingFailuresRenderTheChromePage(t *testing.T) { + h, _ := testServer(t) + + rec := get(t, h, "/no/such/route", "bigbes") + assert.Equal(t, http.StatusNotFound, rec.Code) + assert.Contains(t, rec.Body.String(), "navbar-brand", "the 404 carries the chrome") + + // A method the router does not allow on a path it does serve: /inbox/seen is + // a POST, so a GET of it is chi's 405 rather than a miss. + rec = get(t, h, "/inbox/seen", "bigbes") + assert.Equal(t, http.StatusMethodNotAllowed, rec.Code) + assert.Contains(t, rec.Body.String(), "navbar-brand", "the 405 carries the chrome") +} + +// Every read route answers HEAD, because RFC 9110 §9.3.2 makes it mandatory for +// anything serving GET and because a monitor and a revalidating cache both reach +// for it. The pair shares a handler, so the status is the GET's — including the +// 404 of a document that is not there, which must not become a 200 on a method +// that returns no body to contradict it. +func TestReadRoutesAnswerHead(t *testing.T) { + h, _ := testServer(t) + for target, want := range map[string]int{ + "/": http.StatusOK, + "/healthz": http.StatusOK, + "/inbox": http.StatusOK, + "/~bigbes/rfcs": http.StatusOK, + "/~bigbes/rfcs/specs/0007-storage": http.StatusOK, + "/~bigbes/rfcs/specs/nope": http.StatusNotFound, + } { + t.Run(target, func(t *testing.T) { + req := httptest.NewRequest(http.MethodHead, target, nil) + login(req, "bigbes") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.Equal(t, want, rec.Code, "HEAD %s", target) + }) + } +} + func TestTrailingSlashRedirectsToTheSpace(t *testing.T) { h, _ := testServer(t) rec := get(t, h, "/~bigbes/rfcs/", "bigbes")