diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index 660b0ff86..c95543d47 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -811,12 +811,15 @@ func (h *Handler) ListIssues(w http.ResponseWriter, r *http.Request) { limit := 100 offset := 0 if l := r.URL.Query().Get("limit"); l != "" { - if v, err := strconv.Atoi(l); err == nil { + if v, err := strconv.Atoi(l); err == nil && v > 0 { limit = v } } + if limit > 100 { + limit = 100 + } if o := r.URL.Query().Get("offset"); o != "" { - if v, err := strconv.Atoi(o); err == nil { + if v, err := strconv.Atoi(o); err == nil && v >= 0 { offset = v } } diff --git a/server/internal/handler/issue_limit_validation_test.go b/server/internal/handler/issue_limit_validation_test.go new file mode 100644 index 000000000..7f72089b4 --- /dev/null +++ b/server/internal/handler/issue_limit_validation_test.go @@ -0,0 +1,286 @@ +package handler + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// Backs issue #3563 / MUL-2847: ListIssues must validate and clamp the `limit` +// and `offset` query params the same way the sibling endpoints (SearchIssues, +// ListGroupedIssues) already do. Without these guards: +// - limit=-1 → Postgres rejects the negative LIMIT → HTTP 500 +// - limit=0 → returns 0 rows today, not 500; guarded only for +// sibling-consistency (treat as "use default") +// - limit=100000000 → unbounded read in one response +// - offset=-1 → same 500 from Postgres +// - non-numeric limit/offset → silently ignored today (this test pins that) +// +// Default is 100 and clamp is 100, matching the upstream issue's suggestion +// and the current default. All cases below must return 200 and a well-formed +// JSON body, never 500. +func TestListIssues_LimitValidation(t *testing.T) { + ctx := context.Background() + suffix := time.Now().UnixNano() + + // Seed three issues in a dedicated project so the test is hermetic and + // not polluted by other tests' fixtures in the workspace. + var projectID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO project (workspace_id, title) VALUES ($1, $2) RETURNING id + `, testWorkspaceID, fmt.Sprintf("Limit Validation %d", suffix)).Scan(&projectID); err != nil { + t.Fatalf("create project: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE project_id = $1`, projectID) + testPool.Exec(context.Background(), `DELETE FROM project WHERE id = $1`, projectID) + }) + + insertIssue := func(title string) string { + var number int + if err := testPool.QueryRow(ctx, ` + UPDATE workspace + SET issue_counter = GREATEST(issue_counter, (SELECT COALESCE(MAX(number), 0) FROM issue WHERE workspace_id = $1)) + 1 + WHERE id = $1 RETURNING issue_counter + `, testWorkspaceID).Scan(&number); err != nil { + t.Fatalf("next issue number: %v", err) + } + var id string + if err := testPool.QueryRow(ctx, ` + INSERT INTO issue (workspace_id, title, status, priority, creator_type, creator_id, position, number, project_id) + VALUES ($1, $2, 'todo', 'none', 'member', $3, 0, $4, $5) RETURNING id + `, testWorkspaceID, title, testUserID, number, projectID).Scan(&id); err != nil { + t.Fatalf("create issue %q: %v", title, err) + } + return id + } + _ = insertIssue(fmt.Sprintf("limit-val-1-%d", suffix)) + _ = insertIssue(fmt.Sprintf("limit-val-2-%d", suffix)) + _ = insertIssue(fmt.Sprintf("limit-val-3-%d", suffix)) + + type listResp struct { + Issues []IssueResponse `json:"issues"` + Total int64 `json:"total"` + } + + call := func(query string) (int, listResp, string) { + path := fmt.Sprintf("/api/issues?workspace_id=%s&project_id=%s%s", + testWorkspaceID, projectID, query) + w := httptest.NewRecorder() + testHandler.ListIssues(w, newRequest("GET", path, nil)) + var resp listResp + body := w.Body.String() + if w.Code == http.StatusOK { + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode list response (q=%q): %v\nbody: %s", query, err, body) + } + } + return w.Code, resp, body + } + + // Cases that previously 500'd. All must return 200 with a well-formed + // body. With only 3 seeded rows the clamp and default-fallback are + // observably indistinguishable here — those behaviors are covered by + // TestListIssues_LimitClamp below, which seeds 101 rows. + cases := []struct { + name string + query string + }{ + {"negative limit falls back to default", "&limit=-1"}, + {"negative offset falls back to 0", "&offset=-1"}, + {"negative limit and offset", "&limit=-1&offset=-1"}, + {"non-numeric limit falls back to default", "&limit=abc"}, + {"non-numeric offset falls back to default", "&offset=abc"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + code, resp, body := call(tc.query) + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + // We seeded exactly 3 issues, so any well-formed response must + // report 3 of them. + if resp.Total != 3 { + t.Fatalf("total: want 3, got %d", resp.Total) + } + }) + } + + // Sanity: an explicit small limit is honored. + t.Run("explicit limit below clamp is honored", func(t *testing.T) { + code, resp, body := call("&limit=1") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if len(resp.Issues) != 1 { + t.Fatalf("limit=1: want 1 issue, got %d", len(resp.Issues)) + } + if resp.Total != 3 { + t.Fatalf("total: want 3, got %d", resp.Total) + } + }) + + // Sanity: a positive offset is honored and yields the empty tail of the + // page when it runs past the seeded set. + t.Run("positive offset is honored", func(t *testing.T) { + code, resp, body := call("&limit=2&offset=2") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if len(resp.Issues) != 1 { + t.Fatalf("limit=2 offset=2: want 1 issue (the 3rd of 3), got %d", len(resp.Issues)) + } + if resp.Total != 3 { + t.Fatalf("total: want 3, got %d", resp.Total) + } + }) +} + +// TestListIssues_LimitClamp proves the upper-bound clamp on `limit` actually +// fires. TestListIssues_LimitValidation above seeds only 3 rows, so a missing +// clamp would still return 3 rows for `limit=100000000` and the assertion +// would pass. Here we seed 101 rows and pin the clamp at exactly 100, the +// same boundary the production endpoint promises. +func TestListIssues_LimitClamp(t *testing.T) { + const seeded = 101 + ctx := context.Background() + suffix := time.Now().UnixNano() + + var projectID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO project (workspace_id, title) VALUES ($1, $2) RETURNING id + `, testWorkspaceID, fmt.Sprintf("Limit Clamp %d", suffix)).Scan(&projectID); err != nil { + t.Fatalf("create project: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE project_id = $1`, projectID) + testPool.Exec(context.Background(), `DELETE FROM project WHERE id = $1`, projectID) + }) + + // Seed 101 issues. Each row is inserted individually so the workspace's + // `issue_counter` advances correctly via the same path real issues take; + // `LIMIT 101` returning exactly 101 rows is itself a sanity check that + // nothing in the test wiring is off. + insertIssue := func(idx int) { + title := fmt.Sprintf("clamp-%d-%d", suffix, idx) + var number int + if err := testPool.QueryRow(ctx, ` + UPDATE workspace + SET issue_counter = GREATEST(issue_counter, (SELECT COALESCE(MAX(number), 0) FROM issue WHERE workspace_id = $1)) + 1 + WHERE id = $1 RETURNING issue_counter + `, testWorkspaceID).Scan(&number); err != nil { + t.Fatalf("next issue number: %v", err) + } + if _, err := testPool.Exec(ctx, ` + INSERT INTO issue (workspace_id, title, status, priority, creator_type, creator_id, position, number, project_id) + VALUES ($1, $2, 'todo', 'none', 'member', $3, 0, $4, $5) + `, testWorkspaceID, title, testUserID, number, projectID); err != nil { + t.Fatalf("create issue #%d: %v", idx, err) + } + } + for i := 0; i < seeded; i++ { + insertIssue(i) + } + + type listResp struct { + Issues []IssueResponse `json:"issues"` + Total int64 `json:"total"` + } + + call := func(query string) (int, listResp, string) { + path := fmt.Sprintf("/api/issues?workspace_id=%s&project_id=%s%s", + testWorkspaceID, projectID, query) + w := httptest.NewRecorder() + testHandler.ListIssues(w, newRequest("GET", path, nil)) + var resp listResp + body := w.Body.String() + if w.Code == http.StatusOK { + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode list response (q=%q): %v\nbody: %s", query, err, body) + } + } + return w.Code, resp, body + } + + // First sanity-check: with no limit, ListIssues defaults to 100, so + // exactly 100 rows come back. This would NOT pass if the seeded set + // were broken or the default were wrong. + t.Run("no limit returns default page of 100", func(t *testing.T) { + code, resp, body := call("") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if got := len(resp.Issues); got != 100 { + t.Fatalf("default page size: want 100, got %d", got) + } + if resp.Total != seeded { + t.Fatalf("total: want %d, got %d", seeded, resp.Total) + } + }) + + // The actual clamp assertions. Without the `if limit > 100` clamp, the + // huge/above-clamp cases would return 101 rows and these would fail. + clampCases := []struct { + name string + query string + want int + }{ + {"huge limit is clamped to 100", "&limit=100000000", 100}, + {"one above the clamp", "&limit=101", 100}, + {"well above the clamp", "&limit=200", 100}, + {"at the clamp boundary", "&limit=100", 100}, + } + for _, tc := range clampCases { + t.Run(tc.name, func(t *testing.T) { + code, resp, body := call(tc.query) + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if got := len(resp.Issues); got != tc.want { + t.Fatalf("len(issues) for %q: want %d, got %d", tc.query, tc.want, got) + } + if resp.Total != seeded { + t.Fatalf("total: want %d, got %d", seeded, resp.Total) + } + }) + } + + // Limits below the clamp must be honored (clamp is upper-bound only). + t.Run("limit below clamp is honored", func(t *testing.T) { + code, resp, body := call("&limit=50") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if got := len(resp.Issues); got != 50 { + t.Fatalf("limit=50: want 50 issues, got %d", got) + } + if resp.Total != seeded { + t.Fatalf("total: want %d, got %d", seeded, resp.Total) + } + }) + + // Offset and clamp compose against the full result set. The SQL is + // `LIMIT $limit OFFSET $offset` (with the limit already clamped to 100), + // so `limit=200&offset=50` over 101 rows produces `LIMIT 100 OFFSET 50`, + // which is rows 50..100 — 51 rows. This subtest pins the composition: + // it would fail with a 500 if the offset guard were missing, with a + // different count if the clamp were applied to the wrong axis, and with + // `len == 0` if a buggy implementation skipped rows beyond the clamped + // page instead of against the full result set. + t.Run("offset and clamp compose against the full result set", func(t *testing.T) { + code, resp, body := call("&limit=200&offset=50") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", code, body) + } + if got := len(resp.Issues); got != 51 { + t.Fatalf("limit=200 offset=50: want 51 issues, got %d", got) + } + if resp.Total != seeded { + t.Fatalf("total: want %d, got %d", seeded, resp.Total) + } + }) +}