diff --git a/cmd/doltsrht/main.go b/cmd/doltsrht/main.go index c8a082a94f37ce10d977d18244e501bec7191b51..ce37ba6d7fb84c443b6d8e04d1b282e3f9c75d18 100644 --- a/cmd/doltsrht/main.go +++ b/cmd/doltsrht/main.go @@ -25,7 +25,7 @@ "log/slog" "os" "github.com/go-chi/chi/v5" - chimw "github.com/go-chi/chi/v5/middleware" + chimiddleware "github.com/go-chi/chi/v5/middleware" _ "github.com/lib/pq" // registers the "postgres" database/sql driver "github.com/vaughan0/go-ini" @@ -37,6 +37,7 @@ "sourcecraft.dev/bigbes/sr-ht-core/config" "sourcecraft.dev/bigbes/sr-ht-core/database" "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-dolt/authn" @@ -200,7 +201,17 @@ "component", "web", "keys", instconf.APIOriginKeys()) } srv.AnonRouter().Group(func(r chi.Router) { - r.Use(chimw.RealIP, chimw.Recoverer) + // RequestID and RealIP first: the request line below carries the id and + // the viewer's address, and neither exists until these have run. + r.Use(chimiddleware.RequestID, chimiddleware.RealIP) + // The request line as a slog record rather than chi's colourised line on + // stdout — the one line this daemon emitted that was neither structured + // nor on stderr, so an operator grepping the journal for a request id + // found every panic and none of the requests. It goes outermost, above + // the panic guards, so that the status it reports is the one that + // actually went out. + r.Use(chimw.RequestLogger(chimw.SlogFormatter{})) + r.Use(chimiddleware.Recoverer) r.Use(config.Middleware(conf, serviceName), database.Middleware(db)) r.Use(authn.OptionalCookieMiddleware()) // never 401s; anonymous stays anonymous diff --git a/web/router.go b/web/router.go index 6c908a83434ade9e9ed9bf20b8347f173530240c..dcfa9c69b045ce1d8817d8d9db66e7be87d19a71 100644 --- a/web/router.go +++ b/web/router.go @@ -9,6 +9,7 @@ "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/chrome" "sourcecraft.dev/bigbes/sr-ht-ecore/csrf" "sourcecraft.dev/bigbes/sr-ht-ecore/internalauth" @@ -172,6 +173,13 @@ // web/ renders would only be something for a Go client to discard. r.With(internalauth.Guard(core.InternalClientID, core.InternalNodeID, nil)). Post("/internal/repos", a.handleInternalCreate) + // A URL this router does not serve, and a method it does not allow, are + // answered by the same page every other refusal here is. chi's own pair is + // net/http's plain text — no chrome, no nav, and no way out for a viewer who + // mistyped an address. It is a registration on the tree rather than a link + // in a chain, so it is installed once and inherited by everything below. + chimw.RenderRefusals(r, a.fail) + // Everything a browser reaches. The same-origin guard is the group's, not // each mutating handler's: a predicate spelled per handler is protection // somebody has to remember, and the form added next year is the one that @@ -179,21 +187,29 @@ // goes out unguarded. r.Group(func(r chi.Router) { r.Use(csrf.Require(a.chrome.SelfOrigin(), a.denyCSRF)) - r.Get("/", a.handleIndex) - r.Get("/create", a.handleCreateForm) + // Read routes are registered for GET and HEAD both. Nothing on this + // surface reads r.Method, so a HEAD is the same query answered by the + // same handler and can never say 200 where the GET says 404 — which on + // these pages would be the visibility leak the 404 exists to prevent. + // Registered rather than rewritten per request, so the routing tree + // stays the one record of what this service serves. The mutating half + // of a form page is registered beside it with r.Post: a HEAD that + // writes is not a HEAD. + chimw.GetHead(r, "/", a.handleIndex) + chimw.GetHead(r, "/create", a.handleCreateForm) r.Post("/create", a.handleCreate) - r.Get("/settings/keys", a.handleKeys) + chimw.GetHead(r, "/settings/keys", a.handleKeys) r.Post("/settings/keys", a.handleKeysPost) - r.Get("/~{user}", a.handleUser) - r.Get("/~{user}/{db}", a.handleOverview) - r.Get("/~{user}/{db}/log", a.handleLog) - r.Get("/~{user}/{db}/commit/{hash}", a.handleCommit) - r.Get("/~{user}/{db}/tree/{ref}", a.handleTree) - r.Get("/~{user}/{db}/table/{ref}/{table}", a.handleTable) - r.Get("/~{user}/{db}/view/{view}", a.handleView) - r.Get("/~{user}/{db}/settings", a.handleSettings) + chimw.GetHead(r, "/~{user}", a.handleUser) + chimw.GetHead(r, "/~{user}/{db}", a.handleOverview) + chimw.GetHead(r, "/~{user}/{db}/log", a.handleLog) + chimw.GetHead(r, "/~{user}/{db}/commit/{hash}", a.handleCommit) + chimw.GetHead(r, "/~{user}/{db}/tree/{ref}", a.handleTree) + chimw.GetHead(r, "/~{user}/{db}/table/{ref}/{table}", a.handleTable) + chimw.GetHead(r, "/~{user}/{db}/view/{view}", a.handleView) + chimw.GetHead(r, "/~{user}/{db}/settings", a.handleSettings) r.Post("/~{user}/{db}/settings", a.handleSettingsPost) // The static tree, with the cache policy the hashed names imply and no diff --git a/web/web_test.go b/web/web_test.go index 21b541a1d8487c037f404f3b10ab66b81348cc06..d5b77954291a7834a382da1d363db36763e83482 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -712,6 +712,71 @@ require.Equal(t, http.StatusForbidden, denied.Code) assert.Contains(t, denied.Body.String(), "Only the owner may change database settings.") } +// TestRoutingRefusalsRenderTheSharedErrorPage covers the two refusals that never +// reach a handler at all — a path this router does not serve, and a method it +// does not allow. Both used to fall through to chi's net/http default: plain +// text, no chrome, no nav, and the only refusals on this instance that did not +// look like the service they came from. +func TestRoutingRefusalsRenderTheSharedErrorPage(t *testing.T) { + h := newHarness(t) + + unrouted := h.do("GET", "/no/such/path", nil, nil) + require.Equal(t, http.StatusNotFound, unrouted.Code) + assert.Contains(t, unrouted.Body.String(), pages.NotFoundMessage) + assert.Contains(t, unrouted.Body.String(), `dolt`, + "an unrouted URL is answered through our chrome") + + // POST to a read-only route: routed, but not for this method. + badMethod := h.do("POST", "/~alice/anything/log", nil, url.Values{}) + require.Equal(t, http.StatusMethodNotAllowed, badMethod.Code) + assert.Contains(t, badMethod.Body.String(), pages.MethodMessage) +} + +// TestReadRoutesAnswerHead walks the routing tree and requires every GET route +// to be registered for HEAD as well. +// +// It asks the tree rather than issuing requests because the tree is the record +// that matters: a middleware that rewrote the method per request would answer +// HEAD while chi's own 405 handler, built out of the methods that were +// registered, still said the route accepts GET alone. A read route that answers +// `curl -I` with a 405 and a kilobyte of error page is a route no monitor and no +// cache can revalidate cheaply. +func TestReadRoutesAnswerHead(t *testing.T) { + h := newHarness(t) + + methods := map[string]map[string]bool{} + require.NoError(t, chi.Walk(h.router, + func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { + if methods[route] == nil { + methods[route] = map[string]bool{} + } + methods[route][method] = true + return nil + })) + require.NotEmpty(t, methods) + + for route, served := range methods { + if served[http.MethodGet] { + assert.True(t, served[http.MethodHead], "%s serves GET but not HEAD", route) + } + } +} + +// TestHeadOnAPrivateDatabaseIsStillNotFound: the HEAD twin shares the GET's +// handler, so it cannot answer 200 where the GET answers 404. That equivalence +// is what keeps HEAD from becoming a cheap existence oracle for somebody else's +// private database (SPEC ch. 6.3). +func TestHeadOnAPrivateDatabaseIsStillNotFound(t *testing.T) { + h := newHarness(t) + h.store.add(&core.Repo{Name: "sec", OwnerID: 1, OwnerName: "alice", Path: "/s", Visibility: core.VisibilityPrivate}) + h.store.add(&core.Repo{Name: "pub", OwnerID: 1, OwnerName: "alice", Path: "/p", Visibility: core.VisibilityPublic}) + + assert.Equal(t, http.StatusNotFound, h.do("HEAD", "/~alice/sec", nil, nil).Code) + assert.Equal(t, http.StatusNotFound, h.do("HEAD", "/~alice/nosuch", nil, nil).Code, + "a private database and a missing one must be indistinguishable to HEAD too") + assert.Equal(t, http.StatusOK, h.do("HEAD", "/~alice/pub", nil, nil).Code) +} + // leakyView renders a page whose content block reads a field its envelope does // not carry, so executing it fails halfway. It is the shape of the bug the old // renderer turned into a disclosure.