2 files changed, 107 insertions(+), 28 deletions(-)

M web/comments.go
M web/comments_test.go

@@ 2,10 2,13 @@ package web
 
 import (
 	"net/http"
+	"net/url"
 	"sort"
 	"strconv"
 	"strings"
 
+	"sourcecraft.dev/bigbes/sr-ht-ecore/pages"
+
 	"sourcecraft.dev/bigbes/sr-ht-spec/authn"
 	"sourcecraft.dev/bigbes/sr-ht-spec/core"
 	"sourcecraft.dev/bigbes/sr-ht-spec/doc"


@@ 212,19 215,19 @@ func docIDFor(path string, src []byte) string {
 // one way two surfaces of the same conversation can disagree without either
 // looking broken.
 func (s *Server) handleProposalComment(w http.ResponseWriter, r *http.Request) {
-	p, ok := s.commentPost(w, r)
+	p, form, ok := s.commentPost(w, r)
 	if !ok {
 		return
 	}
 
-	docPath := r.PostFormValue("doc")
-	body := strings.TrimSpace(r.PostFormValue("body"))
-	ordinal, err := strconv.Atoi(r.PostFormValue("block"))
+	docPath := form.Get("doc")
+	body := strings.TrimSpace(form.Get("body"))
+	ordinal, err := strconv.Atoi(form.Get("block"))
 	if err != nil || ordinal < 0 {
 		s.renderError(w, r, http.StatusBadRequest, "that comment names no block")
 		return
 	}
-	side, err := core.ParseCommentSide(r.PostFormValue("side"))
+	side, err := core.ParseCommentSide(form.Get("side"))
 	if err != nil {
 		s.renderError(w, r, http.StatusBadRequest, err.Error())
 		return


@@ 255,7 258,7 @@ func (s *Server) handleProposalComment(w http.ResponseWriter, r *http.Request) {
 	// dropped the hidden field disabled the staleness check silently, with every
 	// test still passing — the comment would still store a coherent anchor, just
 	// not the block the reviewer was reading.
-	switch want := r.PostFormValue("hash"); {
+	switch want := form.Get("hash"); {
 	case want == "":
 		s.renderError(w, r, http.StatusBadRequest, "that comment names no block revision")
 		return


@@ 284,15 287,15 @@ func (s *Server) handleProposalComment(w http.ResponseWriter, r *http.Request) {
 // reply — that is the loop's turn-taking, the owner critiques and the agent
 // answers — and service.ReplyTo is what says so.
 func (s *Server) handleProposalReply(w http.ResponseWriter, r *http.Request) {
-	p, ok := s.commentPost(w, r)
+	p, form, ok := s.commentPost(w, r)
 	if !ok {
 		return
 	}
-	threadID, ok := s.threadOfProposal(w, r, p.ID)
+	threadID, ok := s.threadOfProposal(w, r, form, p.ID)
 	if !ok {
 		return
 	}
-	body := strings.TrimSpace(r.PostFormValue("body"))
+	body := strings.TrimSpace(form.Get("body"))
 	if body == "" {
 		s.renderError(w, r, http.StatusBadRequest, "a reply needs a body")
 		return


@@ 310,15 313,15 @@ func (s *Server) handleProposalReply(w http.ResponseWriter, r *http.Request) {
 // agent that could resolve the thread opened against its own proposal could
 // clear the auto-merge gate that thread exists to hold shut.
 func (s *Server) handleProposalResolve(w http.ResponseWriter, r *http.Request) {
-	p, ok := s.commentPost(w, r)
+	p, form, ok := s.commentPost(w, r)
 	if !ok {
 		return
 	}
-	threadID, ok := s.threadOfProposal(w, r, p.ID)
+	threadID, ok := s.threadOfProposal(w, r, form, p.ID)
 	if !ok {
 		return
 	}
-	resolved := r.PostFormValue("resolved") == "1"
+	resolved := form.Get("resolved") == "1"
 	who := authn.PrincipalFromContext(r.Context())
 	if err := s.reader.ResolveThread(r.Context(), who, threadID, resolved); err != nil {
 		s.fail(w, r, err)


@@ 327,11 330,21 @@ func (s *Server) handleProposalResolve(w http.ResponseWriter, r *http.Request) {
 	s.backToThread(w, r, p, threadID)
 }
 
-// commentPost is the prologue every comment POST shares: read authority, a
-// parsed form, and the proposal the URL names — refusing one that belongs to
+// commentPost is the prologue every comment POST shares: read authority, the
+// form's values, and the proposal the URL names — refusing one that belongs to
 // another space for the same reason the review page does, that the id is global
 // but the link names its space.
 //
+// The values come back from pages.FormValues and the handlers read *those*
+// rather than the request, which is the point of returning them. FormValues
+// answers r.PostForm and never r.Form, and r.Form is the merge of the body with
+// the query string — so with a bare ParseForm and r.FormValue, every one of
+// these mutations could be driven entirely from a URL somebody was linked to.
+// That request is precisely the one the same-origin guard sees nothing wrong
+// with, because it really did come from our own page. It also bounds the body,
+// which this handler never did: net/http's own ceiling is 10 MiB per request,
+// three orders of magnitude past anything this form sends.
+//
 // The cross-site guard used to open this list and is gone from it: it is
 // csrf.Require on the router now (Handler), where it also covers the POST
 // nobody has written yet.


@@ 342,52 355,57 @@ func (s *Server) handleProposalResolve(w http.ResponseWriter, r *http.Request) {
 // restated: opening and resolving are the owner's alone and replying is not, the
 // service knows both rules, and a second copy here would be a second place for
 // them to be wrong.
-func (s *Server) commentPost(w http.ResponseWriter, r *http.Request) (service.Proposal, bool) {
+func (s *Server) commentPost(w http.ResponseWriter, r *http.Request) (service.Proposal, url.Values, bool) {
 	if !mayRead(r) {
 		s.renderError(w, r, http.StatusForbidden, "you may not read this proposal")
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
 	// The grant half of the same ACL. The review conversation is content, so a
 	// tokens.sr.ht token reaches it on spec:read like every other read here.
 	if err := readGrant(r); err != nil {
 		s.denyGrant(w, r, formatHTML)
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
-	if err := r.ParseForm(); err != nil {
+	form, err := pages.FormValues(w, r, 0)
+	if err != nil {
 		s.renderError(w, r, http.StatusBadRequest, "malformed form submission")
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
 	ref, err := spaceRefFrom(r)
 	if err != nil {
 		s.fail(w, r, err)
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
 	id, ok := proposalIDFrom(r)
 	if !ok {
 		s.renderError(w, r, http.StatusNotFound, "no such proposal")
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
 	p, err := s.reader.GetProposal(r.Context(), id)
 	if err != nil {
 		s.fail(w, r, err)
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
 	if p.Space != ref {
 		s.renderError(w, r, http.StatusNotFound, "no such proposal in this space")
-		return service.Proposal{}, false
+		return service.Proposal{}, nil, false
 	}
-	return p, true
+	return p, form, true
 }
 
-// threadOfProposal reads the "thread" form field and checks that it names a root
-// thread of *this* proposal.
+// threadOfProposal reads the "thread" field out of the submitted body and checks
+// that it names a root thread of *this* proposal.
 //
 // Thread ids are global while the URL names one proposal, so without this a form
 // could carry another proposal's thread id and have the reply land somewhere the
 // reviewer was never looking. Matching against the roots also keeps threading
 // flat: a reply's id is not a root, so it cannot be replied to.
-func (s *Server) threadOfProposal(w http.ResponseWriter, r *http.Request, proposalID int) (int, bool) {
-	threadID, err := strconv.Atoi(r.PostFormValue("thread"))
+//
+// The values are passed in rather than read off the request, so that this field
+// comes from the same body-only set as every other one: a "thread" appended to
+// the URL of a legitimate form post must not be able to redirect the write.
+func (s *Server) threadOfProposal(w http.ResponseWriter, r *http.Request, form url.Values, proposalID int) (int, bool) {
+	threadID, err := strconv.Atoi(form.Get("thread"))
 	if err != nil || threadID <= 0 {
 		s.renderError(w, r, http.StatusBadRequest, "that action names no review thread")
 		return 0, false

@@ 8,6 8,8 @@ import (
 	"strings"
 	"testing"
 
+	"sourcecraft.dev/bigbes/sr-ht-ecore/pages"
+
 	"sourcecraft.dev/bigbes/sr-ht-spec/core"
 	"sourcecraft.dev/bigbes/sr-ht-spec/service"
 )


@@ 264,6 266,65 @@ func TestOwnerOpensAThreadOnABlock(t *testing.T) {
 	}
 }
 
+// A mutation must come from a form's body and never from the URL it was posted
+// to. pages.FormValues answers r.PostForm, so a comment whose fields sit in the
+// query string carries no body and is refused — where r.Form, which merges the
+// two, would have written it.
+//
+// This is the one request the same-origin guard cannot fault: the link is
+// followed from our own page, so the Origin header is ours and the guard is
+// satisfied. The body-only read is what stops it.
+func TestCommentFieldsInTheQueryStringDoNotWrite(t *testing.T) {
+	h, r := commentServer(t)
+	anchor, err := service.AnchorOf(commentDocID, []byte(commentProposed), 1, core.SideNew)
+	if err != nil {
+		t.Fatalf("AnchorOf: %v", err)
+	}
+
+	query := url.Values{
+		"doc":   {"specs/0007-storage.md"},
+		"block": {"1"},
+		"side":  {"new"},
+		"hash":  {anchor.BlockHash},
+		"body":  {"written from a URL"},
+	}.Encode()
+
+	rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment?"+query,
+		"bigbes", "https://spec.example", url.Values{})
+	if rec.Code == http.StatusSeeOther {
+		t.Fatalf("a comment was accepted from the query string alone; body:\n%s", rec.Body)
+	}
+	if n := len(r.threads[7]); n != 0 {
+		t.Fatalf("%d threads stored, want none written from a URL", n)
+	}
+}
+
+// The submitted body is bounded. Nothing here bounded it before: net/http's own
+// ceiling is 10 MiB per request, three orders of magnitude past anything this
+// form sends, and every one of these routes is reachable by anybody who can log
+// in. Over the limit is a 400 and not a stored comment.
+func TestOversizedCommentBodyIsRefused(t *testing.T) {
+	h, r := commentServer(t)
+	anchor, err := service.AnchorOf(commentDocID, []byte(commentProposed), 1, core.SideNew)
+	if err != nil {
+		t.Fatalf("AnchorOf: %v", err)
+	}
+
+	rec := postForm(t, h, "/~bigbes/rfcs/p/7/comment", "bigbes", "https://spec.example", url.Values{
+		"doc":   {"specs/0007-storage.md"},
+		"block": {"1"},
+		"side":  {"new"},
+		"hash":  {anchor.BlockHash},
+		"body":  {strings.Repeat("x", pages.DefaultMaxFormBytes+1)},
+	})
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400 for a body over the limit", rec.Code)
+	}
+	if n := len(r.threads[7]); n != 0 {
+		t.Fatalf("%d threads stored, want none from an over-long body", n)
+	}
+}
+
 // TestCommentOnAStaleBlockIsRefused proves the form's block hash is a guard, not
 // decoration: if the agent revised the document while the page was open, the
 // comment is refused rather than attached to whatever moved into that position.