diff --git a/web/comments.go b/web/comments.go index 898c4dd491cacff9929373d5d4934b94b1ccf029..da182fd96e3342db7909168206454c1bdf789b16 100644 --- a/web/comments.go +++ b/web/comments.go @@ -2,9 +2,12 @@ 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" @@ -212,19 +215,19 @@ // the browser's comments on different blocks than the MCP tool's, which is the // 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 @@ // for a submission that omits it would mean a later template refactor that // 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 @@ // handleProposalReply appends a reply to an existing thread. Both principals may // 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 @@ // Owner-only, enforced by service.ResolveThread rather than re-stated here: an // 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 @@ } 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 @@ // is a read like any other. Which principal may *write* what is deliberately not // 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 diff --git a/web/comments_test.go b/web/comments_test.go index 70f305f19c3319d9df9b87b461afc9a675203588..b383ee1dbbe609a9bd3d329562d93932d40ef263 100644 --- a/web/comments_test.go +++ b/web/comments_test.go @@ -8,6 +8,8 @@ "strconv" "strings" "testing" + "sourcecraft.dev/bigbes/sr-ht-ecore/pages" + "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/service" ) @@ -261,6 +263,65 @@ t.Errorf("stored anchor = %+v, want service.AnchorOf's %+v", got.Anchor, anchor) } if loc := rec.Header().Get("Location"); loc != "/~bigbes/rfcs/p/7#thread-1" { t.Errorf("redirect = %q, want the new thread on the proposal page", loc) + } +} + +// 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) } }