6 files changed, 140 insertions(+), 98 deletions(-)

D web/csrf.go
M web/handlers_keys.go
M web/handlers_repo.go
M web/handlers_settings.go
M web/router.go
M web/web_test.go

@@ 1,47 0,0 @@
-package web
-
-import (
-	"net/http"
-	"net/url"
-)
-
-// checkSameOrigin is dolt.sr.ht's CSRF defence for state-changing POSTs. core-go
-// ships no CSRF helper, so we keep it simple and explicit: a mutating request
-// must carry an Origin (or, failing that, a Referer) header whose scheme+host
-// matches our own configured origin. Cross-site form posts from a browser always
-// send an Origin that differs from ours, so this blocks them; same-origin form
-// submissions from our own pages always match.
-//
-// Rationale and limits (documented deliberately): we trust the Origin/Referer
-// header, which browsers set and script cannot forge cross-origin. A request
-// with NEITHER header is rejected — our own forms are same-origin and browsers
-// send Origin on form POSTs, so a missing header signals a non-browser or
-// stripped request, which we decline rather than wave through. This is a
-// header-check, not a token scheme; it is sufficient because dolt.sr.ht uses the
-// shared unified-login cookie (SameSite handling lives in meta) and has no
-// cross-origin embedding.
-func (a *app) checkSameOrigin(r *http.Request) bool {
-	self, err := url.Parse(a.chrome.SelfOrigin())
-	if err != nil || self.Host == "" {
-		return false
-	}
-
-	if origin := r.Header.Get("Origin"); origin != "" {
-		return originMatches(origin, self)
-	}
-	if referer := r.Header.Get("Referer"); referer != "" {
-		return originMatches(referer, self)
-	}
-	// No Origin and no Referer: refuse rather than assume same-origin.
-	return false
-}
-
-// originMatches reports whether raw (a full URL from an Origin or Referer
-// header) has the same scheme and host as self.
-func originMatches(raw string, self *url.URL) bool {
-	u, err := url.Parse(raw)
-	if err != nil {
-		return false
-	}
-	return u.Scheme == self.Scheme && u.Host == self.Host
-}

@@ 48,16 48,13 @@ func (a *app) handleKeys(w http.ResponseWriter, r *http.Request) {
 
 // handleKeysPost adds or deletes a dolt key. A form carrying `delete_id` removes
 // that key; otherwise `pubkey` (the base32 string dolt emits) is decoded,
-// validated and registered. Login and a same-origin POST are required.
+// validated and registered. Login is required; the same-origin check is the
+// router's (csrf.Require) and has already run.
 func (a *app) handleKeysPost(w http.ResponseWriter, r *http.Request) {
 	ac := a.requireLogin(w, r)
 	if ac == nil {
 		return
 	}
-	if !a.checkSameOrigin(r) {
-		a.forbidden(w, r, "Cross-origin request rejected.")
-		return
-	}
 	if err := r.ParseForm(); err != nil {
 		a.renderKeys(w, r, ac, http.StatusBadRequest, "Malformed form submission.", "")
 		return

@@ 69,17 69,13 @@ func (a *app) renderCreate(w http.ResponseWriter, r *http.Request, status int, f
 
 // handleCreate processes the new-database form. It validates the name, creates
 // the metadata row, then the on-disk store; on store-init failure it removes the
-// just-created row so no orphan metadata survives. Login and a same-origin POST
-// are required.
+// just-created row so no orphan metadata survives. Login is required; the
+// same-origin check is the router's (csrf.Require) and has already run.
 func (a *app) handleCreate(w http.ResponseWriter, r *http.Request) {
 	ac := a.requireLogin(w, r)
 	if ac == nil {
 		return
 	}
-	if !a.checkSameOrigin(r) {
-		a.forbidden(w, r, "Cross-origin request rejected.")
-		return
-	}
 	if err := r.ParseForm(); err != nil {
 		a.renderCreate(w, r, http.StatusBadRequest, createForm{}, "Malformed form submission.")
 		return

@@ 82,17 82,14 @@ func (a *app) handleSettings(w http.ResponseWriter, r *http.Request) {
 }
 
 // handleSettingsPost dispatches the settings form on its "action" field:
-// update (description + visibility), acl_add, acl_remove, or delete. Owner only,
-// same-origin only.
+// update (description + visibility), acl_add, acl_remove, or delete. Owner
+// only; the same-origin check is the router's (csrf.Require) and has already
+// run.
 func (a *app) handleSettingsPost(w http.ResponseWriter, r *http.Request) {
 	repo, _, ok := a.loadRepoForAdmin(w, r)
 	if !ok {
 		return
 	}
-	if !a.checkSameOrigin(r) {
-		a.forbidden(w, r, "Cross-origin request rejected.")
-		return
-	}
 	if err := r.ParseForm(); err != nil {
 		a.renderSettings(w, r, http.StatusBadRequest, repo, "Malformed form submission.", "")
 		return

@@ 10,6 10,8 @@ import (
 
 	"sourcecraft.dev/bigbes/sr-ht-ecore/assets"
 	"sourcecraft.dev/bigbes/sr-ht-ecore/chrome"
+	"sourcecraft.dev/bigbes/sr-ht-ecore/csrf"
+	"sourcecraft.dev/bigbes/sr-ht-ecore/middleware"
 	"sourcecraft.dev/bigbes/sr-ht-ecore/pages"
 
 	"sourcecraft.dev/bigbes/sr-ht-dolt/authn"


@@ 137,34 139,64 @@ func newApp(cfg Config) (*app, error) {
 // mount installs every dolt.sr.ht web route onto r. Split from Register so tests
 // can mount an *app they retain a handle to.
 func (a *app) mount(r chi.Router) {
-	r.Get("/", a.handleIndex)
-	r.Get("/create", a.handleCreateForm)
-	r.Post("/create", a.handleCreate)
+	// Every page here is rendered for one viewer behind meta's unified-login
+	// cookie at a URL that says nothing about who that is, so nothing this
+	// service answers may be reused for the next viewer. The static handler
+	// overrides this per asset, once it has found the file.
+	r.Use(middleware.PrivateCache)
+	// A panic is a bug like any other and is owed the same page. One that
+	// arrives after the response has started aborts the connection instead,
+	// because there is no status line left to send and half a page with an
+	// error appended to it is neither document.
+	r.Use(middleware.RecoverPanics(func(w http.ResponseWriter, r *http.Request, _ any) {
+		a.fail(w, r, http.StatusInternalServerError, "")
+	}))
 
 	// Service-to-service companion provisioning (git.sr.ht post-update hook).
-	// Guarded by internal-network + network-key auth, not the cookie/CSRF the
-	// browser routes use.
+	// Guarded by internal-network + network-key auth, and deliberately mounted
+	// outside the same-origin group below: it is not a browser route, it carries
+	// no Origin or Referer, and the CSRF guard would refuse every call.
 	r.With(internalAuthGuard).Post("/internal/repos", a.handleInternalCreate)
 
-	r.Get("/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)
-	r.Post("/~{user}/{db}/settings", a.handleSettingsPost)
-
-	// The static tree, with the cache policy the hashed names imply and no
-	// directory listing — a listing of /static/ would publish this build's
-	// stylesheet hash, which nothing else on the surface discloses. An asset URL
-	// typed by hand lands on our own 404 rather than net/http's plain text.
-	r.Handle(assets.DefaultPrefix+"*",
-		assets.Handler(a.static, assets.DefaultPrefix, http.HandlerFunc(a.notFound)))
+	// 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
+	// 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)
+		r.Post("/create", a.handleCreate)
+
+		r.Get("/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)
+		r.Post("/~{user}/{db}/settings", a.handleSettingsPost)
+
+		// The static tree, with the cache policy the hashed names imply and no
+		// directory listing — a listing of /static/ would publish this build's
+		// stylesheet hash, which nothing else on the surface discloses. An asset
+		// URL typed by hand lands on our own 404 rather than net/http's plain
+		// text.
+		r.Handle(assets.DefaultPrefix+"*",
+			assets.Handler(a.static, assets.DefaultPrefix, http.HandlerFunc(a.notFound)))
+	})
+}
+
+// denyCSRF renders the refusal of a mutation that cannot show it came from this
+// site. It is a 403 and never a redirect: a redirect after a POST drops the
+// body and would make a refused mutation look like one that worked.
+func (a *app) denyCSRF(w http.ResponseWriter, r *http.Request) {
+	a.fail(w, r, http.StatusForbidden, csrf.Message)
 }
 
 // staticFS is the asset tree for a configured static directory.

@@ 21,6 21,7 @@ import (
 	"github.com/vaughan0/go-ini"
 	"sourcecraft.dev/bigbes/sr-ht-core/auth"
 
+	"sourcecraft.dev/bigbes/sr-ht-ecore/csrf"
 	"sourcecraft.dev/bigbes/sr-ht-ecore/pages"
 
 	"sourcecraft.dev/bigbes/sr-ht-dolt/authn"


@@ 478,18 479,84 @@ func TestCreateStoreFailureRollsBackRow(t *testing.T) {
 	}
 }
 
-func TestCreateCSRFRejected(t *testing.T) {
+// The same-origin guard is sr-ht-ecore's csrf middleware on the browser group,
+// and no longer three per-handler calls. What is ours to test is that it is
+// mounted over every mutating route — including the one whose handler used to
+// carry the check and now does not — and that a request refusing to say where
+// it came from is refused rather than waved through.
+func TestMutationsAreRefusedWithoutSameOriginEvidence(t *testing.T) {
 	h := newHarness(t)
-	caller := testCaller(5, "carol")
-	req := httptest.NewRequest("POST", "/create", strings.NewReader(url.Values{"name": {"x"}, "visibility": {"PUBLIC"}}.Encode()))
-	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
-	req.Header.Set("Origin", "https://evil.example")
-	req = req.WithContext(authn.WithCaller(req.Context(), caller))
-	rec := httptest.NewRecorder()
-	h.router.ServeHTTP(rec, req)
-	if rec.Code != http.StatusForbidden {
-		t.Fatalf("cross-origin create: got %d, want 403", rec.Code)
+	h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", Path: "/d", Visibility: core.VisibilityPublic})
+	caller := testCaller(10, "owner")
+
+	post := func(target, origin string, form url.Values) *httptest.ResponseRecorder {
+		req := httptest.NewRequest("POST", target, strings.NewReader(form.Encode()))
+		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+		if origin != "" {
+			req.Header.Set("Origin", origin)
+		}
+		req = req.WithContext(authn.WithCaller(req.Context(), caller))
+		rec := httptest.NewRecorder()
+		h.router.ServeHTTP(rec, req)
+		return rec
+	}
+
+	for _, tc := range []struct{ name, target string }{
+		{"create", "/create"},
+		{"keys", "/settings/keys"},
+		{"settings", "/~owner/db/settings"},
+	} {
+		t.Run(tc.name+" cross-origin", func(t *testing.T) {
+			rec := post(tc.target, "https://evil.example", url.Values{"name": {"x"}, "visibility": {"PUBLIC"}})
+			require.Equal(t, http.StatusForbidden, rec.Code)
+			assert.Contains(t, rec.Body.String(), csrf.Message)
+		})
+		t.Run(tc.name+" no headers", func(t *testing.T) {
+			// Neither Origin nor Referer: a request that will not say where it
+			// came from cannot be shown to have come from us.
+			rec := post(tc.target, "", url.Values{"name": {"x"}, "visibility": {"PUBLIC"}})
+			assert.Equal(t, http.StatusForbidden, rec.Code)
+		})
+	}
+}
+
+// Nothing behind the login cookie may be reused for the next viewer: these URLs
+// say nothing about who the page was rendered for.
+func TestPagesAreNotCacheable(t *testing.T) {
+	h := newHarness(t)
+	rec := h.do("GET", "/", testCaller(1, "alice"), 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"))
+}
+
+// panicView is a View whose Build panics, which is the only route into a
+// handler panic this package can reach from a test.
+type panicView struct{}
+
+func (panicView) Name() string                    { return "boom" }
+func (panicView) Label() string                   { return "Boom" }
+func (panicView) Template() string                { return pages.ErrorPage + ".html" }
+func (panicView) Applies([]browse.TableInfo) bool { return true }
+func (panicView) Build(context.Context, BrowseSession, *core.Repo, string, url.Values) (any, error) {
+	panic("the store went away")
+}
+
+// A panic before the response has started is answered with the error page every
+// other bug gets, and the panic value stays in the log.
+func TestAPanicBecomesTheErrorPage(t *testing.T) {
+	h := newHarness(t)
+	h.store.add(&core.Repo{Name: "db", OwnerID: 1, OwnerName: "alice", Path: "/d", Visibility: core.VisibilityPublic})
+	h.browse.sess = &fakeSession{
+		branches: []browse.Branch{{Name: "main", Head: "abcdef1234567890"}},
+		tables:   issueTables(),
 	}
+	setViews(t, h, panicView{})
+
+	rec := h.do("GET", "/~alice/db/view/boom", nil, nil)
+	require.Equal(t, http.StatusInternalServerError, rec.Code)
+	assert.Contains(t, rec.Body.String(), pages.InternalMessage)
+	assert.NotContains(t, rec.Body.String(), "the store went away")
 }
 
 func TestSettingsOwnerGate(t *testing.T) {