diff --git a/web/csrf.go b/web/csrf.go deleted file mode 100644 index 487a1e07b21a2eef77f0a96591bde3451147c165..0000000000000000000000000000000000000000 --- a/web/csrf.go +++ /dev/null @@ -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 -} diff --git a/web/handlers_keys.go b/web/handlers_keys.go index 6deb311b1b353e08ec1b7d237876fe02cab9bfea..82f13d8c091cb53d597295cb338107987ae9e3cc 100644 --- a/web/handlers_keys.go +++ b/web/handlers_keys.go @@ -48,14 +48,11 @@ } // 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 { diff --git a/web/handlers_repo.go b/web/handlers_repo.go index 09355a98a07b68c6b017579111b9d08770df30b9..5e22d7ba0df4d27a78aefc8982af32cbfd1fe7c8 100644 --- a/web/handlers_repo.go +++ b/web/handlers_repo.go @@ -69,15 +69,11 @@ } // 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 { diff --git a/web/handlers_settings.go b/web/handlers_settings.go index 21bd49021e9500b0ed0496d1d7d3c4decd520e00..c66e092a30809cafad97b51282e4af98f871a462 100644 --- a/web/handlers_settings.go +++ b/web/handlers_settings.go @@ -82,15 +82,12 @@ a.renderSettings(w, r, http.StatusOK, repo, "", "") } // 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 { diff --git a/web/router.go b/web/router.go index 9bb6ac13d447dfcc4c56c6f7c07655b72fdfcc4b..b4a849e83a69b26922717200af91bd67eced508d 100644 --- a/web/router.go +++ b/web/router.go @@ -10,6 +10,8 @@ "github.com/go-chi/chi/v5" "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 @@ // 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) + // 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("/~{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) + r.Get("/", a.handleIndex) + r.Get("/create", a.handleCreateForm) + r.Post("/create", a.handleCreate) - // 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))) + 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. diff --git a/web/web_test.go b/web/web_test.go index 400e27362904762e1637c88db89622f656805c83..3b2872ebe8c81a7744828551686966da773bbf2a 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -21,6 +21,7 @@ "github.com/stretchr/testify/require" "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 @@ t.Fatalf("row not rolled back: %v", h.store.deletedRepos) } } -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) {