From 7ddddeb54aa7adde2987c2681ecf37a779bedf56 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 8 Nov 2026 19:39:32 +0100 Subject: [PATCH 1/3] fix(todo-guard): serve push diffs above the 16 MiB JSON bound The guard buffered every upstream API body under one 16 MiB cap and failed closed above it. A push that regenerates large generated files yields a far larger diff (ksail#8182: 54 MB of licence notices), so the TODO scan failed on every retry and turned the consumer's main red. Diff bodies now get their own 256 MiB bound; JSON evidence keeps 16 MiB, and a body above either bound is still refused rather than truncated. Co-Authored-By: Claude Opus 5.5 --- .scripts/todo-guard/guard.go | 16 +++++++++-- .scripts/todo-guard/guard_test.go | 45 +++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/.scripts/todo-guard/guard.go b/.scripts/todo-guard/guard.go index 36323e90..1fdff3e8 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 @@ -215,8 +223,12 @@ 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 { + limit := maxAPIResponse + if req.Header.Get("Accept") == "application/vnd.github.v3.diff" { + limit = maxDiffResponse + } + 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 diff --git a/.scripts/todo-guard/guard_test.go b/.scripts/todo-guard/guard_test.go index 811822d3..4f2feb36 100644 --- a/.scripts/todo-guard/guard_test.go +++ b/.scripts/todo-guard/guard_test.go @@ -470,3 +470,48 @@ func TestIntegerRequiresRepresentableIdentity(t *testing.T) { } } } + +// A push that regenerates large generated files (licence notices, lockfiles) yields a diff far +// above the 16 MiB cap that bounds ordinary JSON evidence. Such a diff must still reach the +// scanner, while an oversized JSON body and a diff above the diff cap stay refused. +func TestLargeDiffWithinDiffCapIsServed(t *testing.T) { + saved := maxDiffResponse + t.Cleanup(func() { maxDiffResponse = saved }) + maxDiffResponse = 24 << 20 + big := "diff --git a/n.txt b/n.txt\n" + strings.Repeat("+x\n", (17<<20)/3) + for _, tc := range []struct { + name string + accept string + body string + cap int64 + want int + }{ + {"diff above JSON cap", "application/vnd.github.v3.diff", big, 24 << 20, 200}, + {"diff above diff cap", "application/vnd.github.v3.diff", big, 16 << 20, 0}, + {"JSON above JSON cap", "application/vnd.github+json", "[" + strings.Repeat(" ", 17<<20) + "]", 24 << 20, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + maxDiffResponse = tc.cap + g := fixtureGuard(t, func(w http.ResponseWriter, r *http.Request) { io.WriteString(w, tc.body) }) + g.cfg.Before = "fixture-base" + path := "/repos/offline/fixture/compare/fixture-base...1111111111111111111111111111111111111111" + if !strings.HasSuffix(tc.accept, "diff") { + path = "/repos/offline/fixture/issues?state=all&per_page=100" + } + r := httptest.NewRequest("GET", path, nil) + r.Header.Set("Authorization", "token fixture-token") + r.Header.Set("Accept", tc.accept) + w := httptest.NewRecorder() + g.ServeHTTP(w, r) + if tc.want == 200 { + if w.Code != 200 || g.verdict() != nil || w.Body.Len() != len(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", w.Code) + } + }) + } +} From 4b14665d4e36eb9ef620984a20d93fd89a15d061 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 8 Nov 2026 19:58:16 +0100 Subject: [PATCH 2/3] fix(todo-guard): bind the diff bound to the diff route Apply maxDiffResponse only where the bound diff route reads, so another route keeps the JSON bound whatever Accept header it sends. Tests now pin the production bounds, send the oversized JSON case through an allowed route and assert the upstream read was reached. Co-Authored-By: Claude Opus 5.5 --- .scripts/todo-guard/guard.go | 12 ++++--- .scripts/todo-guard/guard_test.go | 53 +++++++++++++++++-------------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/.scripts/todo-guard/guard.go b/.scripts/todo-guard/guard.go index 1fdff3e8..1023a971 100644 --- a/.scripts/todo-guard/guard.go +++ b/.scripts/todo-guard/guard.go @@ -199,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, "/")+"/") { @@ -223,10 +229,6 @@ func (g *guard) fetch(r *http.Request, target *url.URL, body []byte) (response, return response{}, errors.New("API transport failed") } defer p.Body.Close() - limit := maxAPIResponse - if req.Header.Get("Accept") == "application/vnd.github.v3.diff" { - limit = maxDiffResponse - } b, e := io.ReadAll(io.LimitReader(p.Body, limit+1)) if e != nil || int64(len(b)) > limit { return response{}, errors.New("API response incomplete") @@ -530,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 4f2feb36..7d7153ee 100644 --- a/.scripts/todo-guard/guard_test.go +++ b/.scripts/todo-guard/guard_test.go @@ -472,45 +472,52 @@ func TestIntegerRequiresRepresentableIdentity(t *testing.T) { } // A push that regenerates large generated files (licence notices, lockfiles) yields a diff far -// above the 16 MiB cap that bounds ordinary JSON evidence. Such a diff must still reach the -// scanner, while an oversized JSON body and a diff above the diff cap stay refused. -func TestLargeDiffWithinDiffCapIsServed(t *testing.T) { +// 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 }) - maxDiffResponse = 24 << 20 - big := "diff --git a/n.txt b/n.txt\n" + strings.Repeat("+x\n", (17<<20)/3) + diff := "diff --git a/n.txt b/n.txt\n" + strings.Repeat("+x\n", (17<<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 string - accept string - body string - cap int64 - want int + name, path, accept, body string + bound int64 + served bool }{ - {"diff above JSON cap", "application/vnd.github.v3.diff", big, 24 << 20, 200}, - {"diff above diff cap", "application/vnd.github.v3.diff", big, 16 << 20, 0}, - {"JSON above JSON cap", "application/vnd.github+json", "[" + strings.Repeat(" ", 17<<20) + "]", 24 << 20, 0}, + {"diff above JSON bound", compare, "application/vnd.github.v3.diff", diff, 24 << 20, 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.cap - g := fixtureGuard(t, func(w http.ResponseWriter, r *http.Request) { io.WriteString(w, tc.body) }) + 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" - path := "/repos/offline/fixture/compare/fixture-base...1111111111111111111111111111111111111111" - if !strings.HasSuffix(tc.accept, "diff") { - path = "/repos/offline/fixture/issues?state=all&per_page=100" - } - r := httptest.NewRequest("GET", path, nil) + 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 tc.want == 200 { + if calls == 0 { + t.Fatal("request never reached the upstream read") + } + if tc.served { if w.Code != 200 || g.verdict() != nil || w.Body.Len() != len(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", w.Code) + if w.Code == 200 || g.verdict() == nil { + t.Fatalf("oversized body accepted: status=%d verdict=%v", w.Code, g.verdict()) } }) } From 69165ef79d3d28ae83d3ec76c0e05ffb3f62e903 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 8 Nov 2026 20:59:33 +0100 Subject: [PATCH 3/3] test(todo-guard): serve a 60 MiB diff under the production bound Co-Authored-By: Claude Opus 5.5 --- .scripts/todo-guard/guard_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.scripts/todo-guard/guard_test.go b/.scripts/todo-guard/guard_test.go index 7d7153ee..ba20003e 100644 --- a/.scripts/todo-guard/guard_test.go +++ b/.scripts/todo-guard/guard_test.go @@ -484,6 +484,8 @@ 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" @@ -493,6 +495,7 @@ func TestLargeDiffWithinDiffBoundIsServed(t *testing.T) { 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}, @@ -511,7 +514,7 @@ func TestLargeDiffWithinDiffBoundIsServed(t *testing.T) { t.Fatal("request never reached the upstream read") } if tc.served { - if w.Code != 200 || g.verdict() != nil || w.Body.Len() != len(tc.body) { + 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