diff --git a/.scripts/todo-guard/guard.go b/.scripts/todo-guard/guard.go index 36323e90..1023a971 100644 --- a/.scripts/todo-guard/guard.go +++ b/.scripts/todo-guard/guard.go @@ -184,6 +184,14 @@ func onlyQuery(q url.Values, keys ...string) bool { return true } +// maxAPIResponse bounds every JSON evidence body the guard buffers. maxDiffResponse bounds a diff +// body instead: a push that regenerates large generated files (licence notices, lockfiles) yields a +// diff far above the JSON bound, and refusing it fails the scan on every such push. A body above +// either bound is still refused, so oversized evidence never reaches the scanner truncated. +const maxAPIResponse int64 = 16 << 20 + +var maxDiffResponse int64 = 256 << 20 + type response struct { code int header http.Header @@ -191,6 +199,12 @@ type response struct { } func (g *guard) fetch(r *http.Request, target *url.URL, body []byte) (response, error) { + return g.fetchBounded(r, target, body, maxAPIResponse) +} + +// fetchBounded buffers at most limit bytes of the upstream body. Only the bound diff route passes +// maxDiffResponse; every other route keeps the JSON bound whatever Accept header it sends. +func (g *guard) fetchBounded(r *http.Request, target *url.URL, body []byte, limit int64) (response, error) { u := *g.cfg.API if target.IsAbs() { if target.Scheme != u.Scheme || target.Host != u.Host || !strings.HasPrefix(target.Path, strings.TrimSuffix(u.Path, "/")+"/") { @@ -215,8 +229,8 @@ func (g *guard) fetch(r *http.Request, target *url.URL, body []byte) (response, return response{}, errors.New("API transport failed") } defer p.Body.Close() - b, e := io.ReadAll(io.LimitReader(p.Body, (16<<20)+1)) - if e != nil || len(b) > 16<<20 { + b, e := io.ReadAll(io.LimitReader(p.Body, limit+1)) + if e != nil || int64(len(b)) > limit { return response{}, errors.New("API response incomplete") } return response{p.StatusCode, p.Header.Clone(), b}, nil @@ -518,7 +532,7 @@ func (g *guard) diff(w http.ResponseWriter, r *http.Request, prefix string) { return } } - p, e := g.fetch(r, r.URL, nil) + p, e := g.fetchBounded(r, r.URL, nil, maxDiffResponse) if e != nil || p.code != 200 { if strings.HasPrefix(r.URL.Path, compare) && !g.mutationStarted { g.diffPending = true diff --git a/.scripts/todo-guard/guard_test.go b/.scripts/todo-guard/guard_test.go index 811822d3..ba20003e 100644 --- a/.scripts/todo-guard/guard_test.go +++ b/.scripts/todo-guard/guard_test.go @@ -470,3 +470,58 @@ func TestIntegerRequiresRepresentableIdentity(t *testing.T) { } } } + +// A push that regenerates large generated files (licence notices, lockfiles) yields a diff far +// above the 16 MiB bound on JSON evidence: ksail#8182 produced a 54 MB compare diff. The production +// diff bound must admit that with headroom, and must apply only on the bound diff route. +func TestProductionDiffBoundAdmitsRegeneratedNotices(t *testing.T) { + if maxDiffResponse < 128<<20 || maxAPIResponse != 16<<20 { + t.Fatalf("bounds changed: diff=%d json=%d", maxDiffResponse, maxAPIResponse) + } +} + +func TestLargeDiffWithinDiffBoundIsServed(t *testing.T) { + saved := maxDiffResponse + t.Cleanup(func() { maxDiffResponse = saved }) + diff := "diff --git a/n.txt b/n.txt\n" + strings.Repeat("+x\n", (17<<20)/3) + // Larger than the 54 MB diff that motivated the bound, read under the production bound. + notices := "diff --git a/n.txt b/n.txt\n" + strings.Repeat("+y\n", (60<<20)/3) + json := "[" + strings.Repeat(" ", 17<<20) + "]" + compare := "/repos/offline/fixture/compare/fixture-base...1111111111111111111111111111111111111111" + issues := "/repos/offline/fixture/issues?per_page=100&page=1&state=open" + for _, tc := range []struct { + name, path, accept, body string + bound int64 + served bool + }{ + {"diff above JSON bound", compare, "application/vnd.github.v3.diff", diff, 24 << 20, true}, + {"60 MiB diff under the production bound", compare, "application/vnd.github.v3.diff", notices, saved, true}, + {"diff above diff bound", compare, "application/vnd.github.v3.diff", diff, 16 << 20, false}, + {"JSON above JSON bound", issues, "application/vnd.github+json", json, 24 << 20, false}, + {"diff Accept on a JSON route", issues, "application/vnd.github.v3.diff", json, 24 << 20, false}, + } { + t.Run(tc.name, func(t *testing.T) { + maxDiffResponse = tc.bound + calls := 0 + g := fixtureGuard(t, func(w http.ResponseWriter, r *http.Request) { calls++; io.WriteString(w, tc.body) }) + g.cfg.Before = "fixture-base" + r := httptest.NewRequest("GET", tc.path, nil) + r.Header.Set("Authorization", "token fixture-token") + r.Header.Set("Accept", tc.accept) + w := httptest.NewRecorder() + g.ServeHTTP(w, r) + if calls == 0 { + t.Fatal("request never reached the upstream read") + } + if tc.served { + if w.Code != 200 || g.verdict() != nil || w.Body.String() != tc.body { + t.Fatalf("large diff rejected: status=%d len=%d verdict=%v", w.Code, w.Body.Len(), g.verdict()) + } + return + } + if w.Code == 200 || g.verdict() == nil { + t.Fatalf("oversized body accepted: status=%d verdict=%v", w.Code, g.verdict()) + } + }) + } +}