Commit 728f109ae5

728f109ae536cd434757278dd6e929720685a27d

parent: 1cb771a8f3

Verified · cmc ci/build: success ci/test: success

cmc <hello@cleberg.net> · 2026-09-29 05:38 UTC

web: authorise release asset uploads before reading the body

The upload handler checks read and write access and the archived flag
before parsing the multipart body, refuses an announced length over the
cap without reading, and allows one upload per user at a time. Flag
values that parseFlags reads take their positionals after "--".

Closes #296

Layout: unified · split

internal/httpd/milestoneactions.go +37 −8
@@ -7,6 +7,7 @@ import (
77 "path/filepath"
88 "strings"
99
10 "gitbay.org/gitbay/internal/policy"
1011 "gitbay.org/gitbay/internal/store"
1112)
1213
@@ -14,17 +15,17 @@ import (
1415// command the CLI runs; who may do it is the command's decision, and the
1516// pages only show the forms to those it will accept.
1617
17// milestoneCreateArgs builds the argv tail for create: the title, and the
18// description and due date when given.
19func milestoneCreateArgs(r *http.Request, head []string) []string {
20 argv := append(head, strings.TrimSpace(r.FormValue("title")))
18// milestoneCreateArgs builds the create argv: the flags, then the
19// positionals after "--" so a title starting with "-" is not a flag.
20func milestoneCreateArgs(r *http.Request, head []string, target string) []string {
21 argv := head
2122 if d := strings.TrimSpace(r.FormValue("description")); d != "" {
2223 argv = append(argv, "--description", d)
2324 }
2425 if d := strings.TrimSpace(r.FormValue("due")); d != "" {
2526 argv = append(argv, "--due", d)
2627 }
27 return argv
28 return append(argv, "--", target, strings.TrimSpace(r.FormValue("title")))
2829}
2930
3031func (s *Server) milestoneSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
@@ -42,7 +43,7 @@ func (s *Server) milestoneSubmit(w http.ResponseWriter, r *http.Request, u store
4243 case "reopen":
4344 argv = []string{"milestone", "reopen", repo, title}
4445 default:
45 argv = milestoneCreateArgs(r, []string{"milestone", "create", repo})
46 argv = milestoneCreateArgs(r, []string{"milestone", "create"}, repo)
4647 }
4748 _, msg, code := s.runControlCode(u, argv)
4849 s.done(w, r, code, msg, back)
@@ -61,7 +62,7 @@ func (s *Server) orgLabelSubmit(w http.ResponseWriter, r *http.Request, u store.
6162 back(w, r, "name the label")
6263 return
6364 }
64 argv := []string{"org", "label", "set", org, name, "--color", strings.TrimSpace(r.FormValue("color"))}
65 argv := []string{"org", "label", "set", "--color", strings.TrimSpace(r.FormValue("color")), "--", org, name}
6566 if r.FormValue("action") == "remove" {
6667 if ok, msg := confirmed(r, name); !ok {
6768 back(w, r, msg)
@@ -88,7 +89,7 @@ func (s *Server) orgMilestoneSubmit(w http.ResponseWriter, r *http.Request, u st
8889 case "reopen":
8990 argv = []string{"org", "milestone", "reopen", org, title}
9091 default:
91 argv = milestoneCreateArgs(r, []string{"org", "milestone", "create", org})
92 argv = milestoneCreateArgs(r, []string{"org", "milestone", "create"}, org)
9293 }
9394 _, msg, code := s.runControlCode(u, argv)
9495 s.done(w, r, code, msg, back)
@@ -103,7 +104,35 @@ func (s *Server) orgMilestoneSubmit(w http.ResponseWriter, r *http.Request, u st
103104func (s *Server) releaseAssetSubmit(w http.ResponseWriter, r *http.Request, u store.User) {
104105 repo := r.PathValue("owner") + "/" + r.PathValue("repo")
105106 back := func(w http.ResponseWriter, r *http.Request, msg string) { s.backTo(w, r, "releases", msg) }
107 // Authorise before reading a byte: a body nobody may upload is never
108 // spooled to disk.
109 rp, err := s.st.RepoByPath(repo)
110 if err == nil {
111 grant, _ := s.st.AccessRole(rp.ID, u.ID)
112 if !policyCanRead(u, rp, grant) {
113 err = store.ErrNotFound
114 } else if !policy.CanWrite(u, rp, grant) {
115 back(w, r, "you need write access to change releases")
116 return
117 } else if rp.Settings.Archived {
118 back(w, r, rp.Path()+" is archived and read-only; unarchive it first")
119 return
120 }
121 }
122 if err != nil {
123 s.notFound(w, r)
124 return
125 }
106126 limit := s.cfg.Limits.MaxAssetBytes
127 if r.ContentLength > limit+1<<20 {
128 back(w, r, fmt.Sprintf("asset exceeds max_asset_bytes (%d)", limit))
129 return
130 }
131 if _, busy := s.uploads.LoadOrStore(u.ID, struct{}{}); busy {
132 back(w, r, "another upload of yours is still running; wait for it to finish")
133 return
134 }
135 defer s.uploads.Delete(u.ID)
107136 r.Body = http.MaxBytesReader(w, r.Body, limit+1<<20)
108137 if err := r.ParseMultipartForm(1 << 20); err != nil && !errors.Is(err, http.ErrNotMultipart) {
109138 var tooBig *http.MaxBytesError
internal/httpd/parity296b_test.go +81
@@ -313,3 +313,84 @@ func TestOrgMilestoneMemberSeesNoFormAndIsRefused(t *testing.T) {
313313 t.Fatal("a member created an org milestone")
314314 }
315315}
316
317// countingBody reports how many bytes a handler read from a request.
318type countingBody struct {
319 r io.Reader
320 n int
321}
322
323func (c *countingBody) Read(b []byte) (int, error) {
324 n, err := c.r.Read(b)
325 c.n += n
326 return n, err
327}
328
329func TestReleaseAssetAuthorisesBeforeReading(t *testing.T) {
330 p := newP296b(t)
331 if _, err := p.st.CreateRepo("user", p.aliceU.ID, "secret", "private"); err != nil {
332 t.Fatal(err)
333 }
334 payload, ct := upload(t, map[string]string{"tag": "v1"}, "tool.bin", bytes.Repeat([]byte("x"), 512))
335 raw, _ := io.ReadAll(payload)
336 send := func(path string, ck *http.Cookie, length int64) (*httptest.ResponseRecorder, *countingBody) {
337 body := &countingBody{r: bytes.NewReader(raw)}
338 req := httptest.NewRequest("POST", path, body)
339 req.ContentLength = length
340 req.Header.Set("Content-Type", ct)
341 req.AddCookie(ck)
342 rr := httptest.NewRecorder()
343 p.h.ServeHTTP(rr, req)
344 return rr, body
345 }
346 for _, path := range []string{"/alice/secret/releases/assets", "/alice/nothing/releases/assets"} {
347 rr, body := send(path, p.bob, int64(len(raw)))
348 if rr.Code != http.StatusNotFound || body.n != 0 {
349 t.Errorf("%s: %d, read %d bytes", path, rr.Code, body.n)
350 }
351 }
352 // A reader of a public repo is refused without reading too.
353 rr, body := send("/alice/app/releases/assets", p.bob, int64(len(raw)))
354 if rr.Code != http.StatusSeeOther || body.n != 0 {
355 t.Errorf("reader: %d, read %d bytes", rr.Code, body.n)
356 }
357 // An announced length over the cap is refused before reading.
358 rr, body = send("/alice/app/releases/assets", p.alice, 3<<20)
359 if rr.Code != http.StatusSeeOther || body.n != 0 || !strings.Contains(p.follow(rr, p.alice), "max_asset_bytes") {
360 t.Errorf("oversized: %d, read %d bytes", rr.Code, body.n)
361 }
362}
363
364func TestReleaseAssetOneUploadAtATime(t *testing.T) {
365 p := newP296b(t)
366 p.s.uploads.Store(p.aliceU.ID, struct{}{})
367 body, ct := upload(t, map[string]string{"tag": "v1"}, "tool.bin", []byte("payload"))
368 rr := p.do("POST", "/alice/app/releases/assets", p.alice, body, ct)
369 if !strings.Contains(p.follow(rr, p.alice), "still running") {
370 t.Fatalf("second upload not refused: %d", rr.Code)
371 }
372 p.s.uploads.Delete(p.aliceU.ID)
373 body, ct = upload(t, map[string]string{"tag": "v1"}, "tool.bin", []byte("payload"))
374 if rr = p.do("POST", "/alice/app/releases/assets", p.alice, body, ct); rr.Code != http.StatusSeeOther {
375 t.Fatal(rr.Code)
376 }
377 if _, busy := p.s.uploads.Load(p.aliceU.ID); busy {
378 t.Fatal("slot not released")
379 }
380}
381
382func TestMilestoneTitleStartingWithDash(t *testing.T) {
383 p := newP296b(t)
384 p.post("/alice/app/milestones", p.alice, url.Values{"title": {"--due"}})
385 if _, err := p.st.MilestoneByTitle(p.repo, "--due"); err != nil {
386 t.Fatalf("repo milestone: %v", err)
387 }
388 p.post("/acme/-/milestones", p.alice, url.Values{"title": {"--description"}})
389 if ms, _ := p.st.ListOrgMilestones(p.orgID, "open", nil); len(ms) != 1 || ms[0].Title != "--description" {
390 t.Fatalf("org milestone: %+v", ms)
391 }
392 p.post("/acme/-/labels", p.alice, url.Values{"name": {"--color"}})
393 if ls, _ := p.st.ListOrgLabels(p.orgID, nil); len(ls) != 1 || ls[0].Name != "--color" {
394 t.Fatalf("org label: %+v", ls)
395 }
396}
internal/httpd/smart.go +1
@@ -35,6 +35,7 @@ type Server struct {
3535 apiLimit *apiLimiter
3636 proxies []*net.IPNet // http.trusted_proxies, parsed once
3737 stopping chan struct{} // closed by Stop
38 uploads sync.Map // user id -> struct{}: release asset uploads in flight
3839 stopOnce sync.Once
3940}
4041