rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(gitea): harden GetPullRequestDiff against unbounded diff size #109

Merged
aweiker merged 4 commits from review-bot-issue-92 into main 2026-05-13 14:01:22 +00:00
Conversation 30 Commits 4 Files Changed 3
+78 -128
2 changed files with 78 additions and 128 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 6ebf66aefb - Show all commits
gitea/client.go
+3
View File
5
 @@ -71,6 +71,9 @@ type Client struct {
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-13 11:59:24 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Disabling the diff size limit (MaxDiffSize = -1) removes protection against large responses and could allow memory exhaustion if configured from untrusted input. Ensure this setting cannot be influenced by untrusted sources.

**[NIT]** Disabling the diff size limit (MaxDiffSize = -1) removes protection against large responses and could allow memory exhaustion if configured from untrusted input. Ensure this setting cannot be influenced by untrusted sources.
// MaxDiffSize is the maximum number of bytes allowed when fetching a PR diff.
gpt-review-bot commented 2026-05-13 13:42:06 +00:00
Review
Copy Link
Copy Source

[NIT] The MaxDiffSize field comment suggests using math.MaxInt64 to disable the limit. This dual mechanism (negative and MaxInt64) may confuse users; consider documenting only the negative value as the disable switch.

**[NIT]** The MaxDiffSize field comment suggests using math.MaxInt64 to disable the limit. This dual mechanism (negative and MaxInt64) may confuse users; consider documenting only the negative value as the disable switch.
// If zero, defaults to DefaultMaxDiffSize (10 MB). Set to any negative value to disable the limit.
//
// This field must be configured before the first request is made.
// Modifying it while requests are in flight is not safe.
MaxDiffSize int64
}
18
gitea/diff_size_test.go
+75 -128
View File
1
 @@ -10,134 +10,81 @@ import (
"time"
)
func TestGetPullRequestDiff_ExceedsMaxSize(t *testing.T) {
// Create a diff that exceeds a small limit
largeDiff := strings.Repeat("+ added line\n", 1000) // ~13 KB
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(largeDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = 100 // 100 bytes limit
client.RetryBackoff = []time.Duration{} // no delay in tests
_, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for oversized diff, got nil")
func TestGetPullRequestDiff_SizeLimits(t *testing.T) {
tests := []struct {
name string
diff string
maxDiffSize int64
wantErr error
wantDiff string
}{
{
name: "exceeds max size",
sonnet-review-bot commented 2026-05-13 11:58:39 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The w.Write return values are silently discarded in all test handlers. While harmless in httptest servers, the convention in this codebase (and in stdlib examples) is to at least acknowledge the return. Not a correctness issue.

**[NIT]** The `w.Write` return values are silently discarded in all test handlers. While harmless in `httptest` servers, the convention in this codebase (and in stdlib examples) is to at least acknowledge the return. Not a correctness issue.
diff: strings.Repeat("+ added line\n", 1000), // ~13 KB
maxDiffSize: 100,
wantErr: ErrDiffTooLarge,
sonnet-review-bot commented 2026-05-13 12:18:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The test server handler ignores write errors from w.Write([]byte(largeDiff)). This is idiomatic for test handlers and not a bug, but for completeness the existing codebase pattern is the same so this is consistent.

**[NIT]** The test server handler ignores write errors from w.Write([]byte(largeDiff)). This is idiomatic for test handlers and not a bug, but for completeness the existing codebase pattern is the same so this is consistent.
},
{
name: "within max size",
diff: "diff --git a/f.go b/f.go\n--- a/f.go\n+++ b/f.go\n@@ -1 +1 @@\n-old\n+new\n",
maxDiffSize: 1024,
wantDiff: "diff --git a/f.go b/f.go\n--- a/f.go\n+++ b/f.go\n@@ -1 +1 @@\n-old\n+new\n",
},
{
name: "exactly at limit",
diff: strings.Repeat("x", 50),
maxDiffSize: 50,
wantDiff: strings.Repeat("x", 50),
},
{
name: "one byte over limit",
diff: strings.Repeat("x", 51),
maxDiffSize: 50,
wantErr: ErrDiffTooLarge,
},
{
name: "disabled limit",
diff: strings.Repeat("x", 10000),
maxDiffSize: -1,
wantDiff: strings.Repeat("x", 10000),
},
{
name: "default limit",
diff: "diff content",
maxDiffSize: 0, // zero means use DefaultMaxDiffSize
wantDiff: "diff content",
},
}
if !errors.Is(err, ErrDiffTooLarge) {
t.Errorf("expected ErrDiffTooLarge, got: %v", err)
}
}
func TestGetPullRequestDiff_WithinMaxSize(t *testing.T) {
smallDiff := "diff --git a/f.go b/f.go\n--- a/f.go\n+++ b/f.go\n@@ -1 +1 @@\n-old\n+new\n"
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(smallDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = 1024 // 1 KB limit — more than enough
client.RetryBackoff = []time.Duration{}
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if got != smallDiff {
t.Errorf("expected diff %q, got %q", smallDiff, got)
}
}
func TestGetPullRequestDiff_ExactlyAtLimit(t *testing.T) {
// A diff that is exactly at the limit should succeed
exactDiff := strings.Repeat("x", 50)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(exactDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = 50 // exactly the size of the diff
client.RetryBackoff = []time.Duration{}
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error for diff at exact limit: %v", err)
}
if got != exactDiff {
t.Errorf("expected diff to match, got length %d", len(got))
}
}
func TestGetPullRequestDiff_OneByteOverLimit(t *testing.T) {
// A diff that is one byte over the limit should fail
overDiff := strings.Repeat("x", 51)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(overDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = 50
client.RetryBackoff = []time.Duration{}
_, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for diff one byte over limit")
}
if !errors.Is(err, ErrDiffTooLarge) {
t.Errorf("expected ErrDiffTooLarge, got: %v", err)
}
}
func TestGetPullRequestDiff_DisabledLimit(t *testing.T) {
// When MaxDiffSize is -1, no limit is enforced
largeDiff := strings.Repeat("x", 10000)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(largeDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = -1 // disabled
client.RetryBackoff = []time.Duration{}
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error with disabled limit: %v", err)
}
if got != largeDiff {
t.Errorf("expected full diff with disabled limit, got length %d", len(got))
}
}
func TestGetPullRequestDiff_DefaultLimit(t *testing.T) {
// With zero MaxDiffSize (default), should use DefaultMaxDiffSize.
// A small diff should succeed without setting MaxDiffSize.
smallDiff := "diff content"
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(smallDiff))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
// MaxDiffSize is zero (default) — should use DefaultMaxDiffSize (10 MB)
client.RetryBackoff = []time.Duration{}
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error with default limit: %v", err)
}
if got != smallDiff {
t.Errorf("expected diff %q, got %q", smallDiff, got)
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(tt.diff)) //nolint:errcheck // test handler
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
client.MaxDiffSize = tt.maxDiffSize
client.RetryBackoff = []time.Duration{}
got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if tt.wantErr != nil {
sonnet-review-bot commented 2026-05-13 12:42:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The test uses defer server.Close() inside a table-driven t.Run loop. This is fine here since each subtest gets its own server, but it's worth noting that t.Cleanup(server.Close) would be more idiomatic per the testing patterns (ensures cleanup after the subtest completes, not just when the closure returns). Not a bug since both are equivalent in this pattern.

**[NIT]** The test uses `defer server.Close()` inside a table-driven `t.Run` loop. This is fine here since each subtest gets its own `server`, but it's worth noting that `t.Cleanup(server.Close)` would be more idiomatic per the testing patterns (ensures cleanup after the subtest completes, not just when the closure returns). Not a bug since both are equivalent in this pattern.
if err == nil {
t.Fatal("expected error, got nil")
}
sonnet-review-bot commented 2026-05-13 13:40:27 +00:00
Review
Copy Link
Copy Source

[NIT] The test case 'math.MaxInt64 treated as disabled' verifies the bypass works, but the test name 'treated as disabled' implies intended behavior rather than an edge-case workaround. A name like 'math.MaxInt64 bypasses limit via overflow protection' would be more precise. Minor naming issue only.

**[NIT]** The test case 'math.MaxInt64 treated as disabled' verifies the bypass works, but the test name 'treated as disabled' implies intended behavior rather than an edge-case workaround. A name like 'math.MaxInt64 bypasses limit via overflow protection' would be more precise. Minor naming issue only.
if !errors.Is(err, tt.wantErr) {
t.Errorf("expected %v, got: %v", tt.wantErr, err)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if got != tt.wantDiff {
t.Errorf("diff mismatch: got length %d, want length %d", len(got), len(tt.wantDiff))
}
})
}
}