From 9660c7204a2b9500e56c7bb2ff47316d50d5fc99 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 9 Aug 2026 00:35:27 +0300 Subject: [PATCH] pages: read forms through FormValues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every mutation on this surface read its fields with r.PostFormValue after a bare r.ParseForm. PostFormValue was already the right half — r.Form would merge the query string into the body, which would let a mutation be driven from a URL somebody was linked to, and that request is exactly the one the same-origin guard sees nothing wrong with — but the body was bounded only by net/http's 10 MiB default, on pages anyone can reach. FormValues is both properties in one call, and the values are passed down explicitly instead of each sub-handler reaching back into the request. --- web/handlers_keys.go | 15 +++++++++------ web/handlers_repo.go | 10 ++++++---- web/handlers_settings.go | 35 ++++++++++++++++++---------------- web/web_test.go | 41 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 26 deletions(-) diff --git a/web/handlers_keys.go b/web/handlers_keys.go index 82f13d8c091cb53d597295cb338107987ae9e3cc..03adf01e082abb90ed924282e895e536ff42387a 100644 --- a/web/handlers_keys.go +++ b/web/handlers_keys.go @@ -3,10 +3,12 @@ package web import ( "errors" "net/http" + "net/url" "strconv" "strings" "sourcecraft.dev/bigbes/sr-ht-ecore/chrome" + "sourcecraft.dev/bigbes/sr-ht-ecore/pages" "sourcecraft.dev/bigbes/sr-ht-dolt/db" ) @@ -55,22 +57,23 @@ func (a *app) handleKeysPost(w http.ResponseWriter, r *http.Request) { if ac == nil { return } - if err := r.ParseForm(); err != nil { + form, err := pages.FormValues(w, r, 0) + if err != nil { a.renderKeys(w, r, ac, http.StatusBadRequest, "Malformed form submission.", "") return } - if idStr := r.PostFormValue("delete_id"); idStr != "" { + if idStr := form.Get("delete_id"); idStr != "" { a.keysDelete(w, r, ac, idStr) return } - a.keysAdd(w, r, ac) + a.keysAdd(w, r, ac, form) } // keysAdd decodes and registers a dolt public key for the caller. -func (a *app) keysAdd(w http.ResponseWriter, r *http.Request, ac *authContext) { - pubStr := strings.TrimSpace(r.PostFormValue("pubkey")) - comment := strings.TrimSpace(r.PostFormValue("comment")) +func (a *app) keysAdd(w http.ResponseWriter, r *http.Request, ac *authContext, form url.Values) { + pubStr := strings.TrimSpace(form.Get("pubkey")) + comment := strings.TrimSpace(form.Get("comment")) pubkey, kid, err := decodeDoltPubKey(pubStr) if err != nil { diff --git a/web/handlers_repo.go b/web/handlers_repo.go index 5e22d7ba0df4d27a78aefc8982af32cbfd1fe7c8..42bb65a25b4f14ceba3b5541f65c362b6ee52d79 100644 --- a/web/handlers_repo.go +++ b/web/handlers_repo.go @@ -9,6 +9,7 @@ import ( "sourcecraft.dev/bigbes/sr-ht-core/config" "sourcecraft.dev/bigbes/sr-ht-ecore/chrome" + "sourcecraft.dev/bigbes/sr-ht-ecore/pages" "sourcecraft.dev/bigbes/sr-ht-dolt/browse" "sourcecraft.dev/bigbes/sr-ht-dolt/core" @@ -76,15 +77,16 @@ func (a *app) handleCreate(w http.ResponseWriter, r *http.Request) { if ac == nil { return } - if err := r.ParseForm(); err != nil { + values, err := pages.FormValues(w, r, 0) + if err != nil { a.renderCreate(w, r, http.StatusBadRequest, createForm{}, "Malformed form submission.") return } form := createForm{ - Name: strings.TrimSpace(r.PostFormValue("name")), - Description: strings.TrimSpace(r.PostFormValue("description")), - Visibility: r.PostFormValue("visibility"), + Name: strings.TrimSpace(values.Get("name")), + Description: strings.TrimSpace(values.Get("description")), + Visibility: values.Get("visibility"), } visibility, ok := parseVisibility(form.Visibility) diff --git a/web/handlers_settings.go b/web/handlers_settings.go index c66e092a30809cafad97b51282e4af98f871a462..846d90c441a7c6e5cfe3070c85ed3a970b913ae2 100644 --- a/web/handlers_settings.go +++ b/web/handlers_settings.go @@ -3,12 +3,14 @@ package web import ( "errors" "net/http" + "net/url" "strconv" "strings" "github.com/go-chi/chi/v5" "sourcecraft.dev/bigbes/sr-ht-ecore/chrome" + "sourcecraft.dev/bigbes/sr-ht-ecore/pages" "sourcecraft.dev/bigbes/sr-ht-dolt/core" "sourcecraft.dev/bigbes/sr-ht-dolt/db" @@ -90,29 +92,30 @@ func (a *app) handleSettingsPost(w http.ResponseWriter, r *http.Request) { if !ok { return } - if err := r.ParseForm(); err != nil { + form, err := pages.FormValues(w, r, 0) + if err != nil { a.renderSettings(w, r, http.StatusBadRequest, repo, "Malformed form submission.", "") return } - switch r.PostFormValue("action") { + switch form.Get("action") { case "update": - a.settingsUpdate(w, r, repo) + a.settingsUpdate(w, r, repo, form) case "acl_add": - a.settingsACLAdd(w, r, repo) + a.settingsACLAdd(w, r, repo, form) case "acl_remove": - a.settingsACLRemove(w, r, repo) + a.settingsACLRemove(w, r, repo, form) case "delete": - a.settingsDelete(w, r, repo) + a.settingsDelete(w, r, repo, form) default: a.renderSettings(w, r, http.StatusBadRequest, repo, "Unknown action.", "") } } // settingsUpdate applies the description + visibility change. -func (a *app) settingsUpdate(w http.ResponseWriter, r *http.Request, repo *core.Repo) { - description := strings.TrimSpace(r.PostFormValue("description")) - visibility, ok := parseVisibility(r.PostFormValue("visibility")) +func (a *app) settingsUpdate(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { + description := strings.TrimSpace(form.Get("description")) + visibility, ok := parseVisibility(form.Get("visibility")) if !ok { a.renderSettings(w, r, http.StatusBadRequest, repo, "Invalid visibility.", "") return @@ -128,9 +131,9 @@ func (a *app) settingsUpdate(w http.ResponseWriter, r *http.Request, repo *core. // settingsACLAdd grants (or updates) an ACL entry for a username. The grantee is // resolved via the user resolver, which mirrors the meta profile on first sight. -func (a *app) settingsACLAdd(w http.ResponseWriter, r *http.Request, repo *core.Repo) { - username := strings.TrimPrefix(strings.TrimSpace(r.PostFormValue("username")), "~") - mode, ok := parseAccessMode(r.PostFormValue("mode")) +func (a *app) settingsACLAdd(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { + username := strings.TrimPrefix(strings.TrimSpace(form.Get("username")), "~") + mode, ok := parseAccessMode(form.Get("mode")) if !ok { a.renderSettings(w, r, http.StatusBadRequest, repo, "Invalid access mode.", "") return @@ -159,8 +162,8 @@ func (a *app) settingsACLAdd(w http.ResponseWriter, r *http.Request, repo *core. } // settingsACLRemove revokes an ACL entry by user id. -func (a *app) settingsACLRemove(w http.ResponseWriter, r *http.Request, repo *core.Repo) { - userID, err := strconv.Atoi(r.PostFormValue("user_id")) +func (a *app) settingsACLRemove(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { + userID, err := strconv.Atoi(form.Get("user_id")) if err != nil { a.renderSettings(w, r, http.StatusBadRequest, repo, "Invalid user.", "") return @@ -179,8 +182,8 @@ func (a *app) settingsACLRemove(w http.ResponseWriter, r *http.Request, repo *co // settingsDelete deletes the database after a name-confirmation check: the row, // then the on-disk store, then the served-cache handle. The confirmation guards // against accidental deletion. -func (a *app) settingsDelete(w http.ResponseWriter, r *http.Request, repo *core.Repo) { - if r.PostFormValue("confirm_name") != repo.Name { +func (a *app) settingsDelete(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { + if form.Get("confirm_name") != repo.Name { a.renderSettings(w, r, http.StatusBadRequest, repo, "Type the database name exactly to confirm deletion.", "") return diff --git a/web/web_test.go b/web/web_test.go index d5b77954291a7834a382da1d363db36763e83482..f1902a7e4ce4bc00c2ff8f4387bde747efa47b5a 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -505,6 +506,46 @@ func TestMutationsAreRefusedWithoutSameOriginEvidence(t *testing.T) { } } +// TestFormsReadTheBodyAndOnlyTheBody pins the two properties every mutation on +// this surface now gets from pages.FormValues. +// +// The query string is not the form. r.Form would merge it into the body's +// values, which would let a mutation be driven entirely from a URL somebody was +// linked to — and that is exactly the request the same-origin guard sees nothing +// wrong with, because it really did come from our own page. +// +// And the body is bounded. net/http's own ceiling is 10 MiB per request, three +// orders of magnitude more than any form here sends. +func TestFormsReadTheBodyAndOnlyTheBody(t *testing.T) { + post := func(t *testing.T, target string, body io.Reader) *httptest.ResponseRecorder { + t.Helper() + h := newHarness(t) + h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", Path: "/d", Visibility: core.VisibilityPublic}) + req := httptest.NewRequest("POST", target, body) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set("Origin", selfOrigin) + req = req.WithContext(authn.WithCaller(req.Context(), testCaller(10, "owner"))) + rec := httptest.NewRecorder() + h.router.ServeHTTP(rec, req) + return rec + } + + t.Run("the query string cannot supply a field", func(t *testing.T) { + // A delete driven from the URL: the confirmation the body does not carry + // is offered in the query string instead. It must not be read. + rec := post(t, "/~owner/db/settings?action=delete&confirm_name=db", strings.NewReader("")) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "Unknown action.") + }) + + t.Run("an oversized body is refused", func(t *testing.T) { + huge := "description=" + strings.Repeat("x", pages.DefaultMaxFormBytes+1) + rec := post(t, "/~owner/db/settings", strings.NewReader(huge)) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "Malformed form submission.") + }) +} + // 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) {