highperfocused/multica
1
0
Fork 0
mirror of https://github.com/multica-ai/multica.git synced 2026-06-16 19:29:26 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(issues): validate and clamp limit/offset in ListIssues (MUL-2847) (#3585)

Browse Source 
* fix(issues): validate and clamp limit/offset in ListIssues (MUL-2847)

ListIssues parsed the limit and offset query params but never validated
them, so:
  - GET /api/issues?limit=-1  -> HTTP 500 (Postgres rejects negative
    LIMIT with SQLSTATE 2201W)
  - GET /api/issues?limit=100000000 -> unbounded read in a single
    response
  - GET /api/issues?offset=-1 -> same 500

SearchIssues and ListGroupedIssues already apply v > 0 + an upper clamp
on limit and v >= 0 on offset. This brings ListIssues to the same
pattern: ignore non-positive limit (keep default 100), clamp to 100,
ignore negative offset (keep default 0). default == clamp == 100 keeps
existing callers' behavior identical and matches the upstream issue
suggestion.

TestListIssues_LimitValidation seeds 3 issues in a dedicated project
and pins the nine boundary cases (negative/zero/huge/non-numeric
limit, negative/non-numeric offset, the clamp boundary, and explicit
small/positive-offset sanity) plus two sanity checks that an explicit
small limit and a positive offset are honored.

Fixes MUL-2847 / upstream multica-ai/multica#3563.

Co-authored-by: multica-agent <github@multica.ai>

* test(issues): strengthen LimitClamp test and fix comments (MUL-2847)

Address review feedback from @Lambda and @Emacs on PR #3585:

1. The 3-row set in TestListIssues_LimitValidation can't distinguish
   'clamp fired' from 'clamp missing': with only 3 rows, limit=100000000
   returns 3 rows whether or not the clamp exists. Split the clamp
   behavior into a new TestListIssues_LimitClamp that seeds 101 issues
   and asserts len(issues) == 100 for limit=100/101/200/100000000, plus
   limit=50 honored below the clamp. Without the clamp line, the
   huge/above-clamp subtests would fail with len == 101.

2. Fix the misleading comment that claimed 'limit=0 -> same 500'.
   Postgres LIMIT 0 is valid SQL and returns zero rows. The guard
   exists for sibling-consistency (SearchIssues / ListGroupedIssues
   already treat v <= 0 as 'use default'), not to avoid a 500. Move
   the limit=0 case out of TestListIssues_LimitValidation since it's
   not 500-related; TestListIssues_LimitClamp's 'no limit returns
   default page of 100' subtest pins the default behavior anyway.

3. Add a subtest that pins the offset+clamp composition
   (limit=200&offset=50 against 101 rows = 51 rows), proving the
   clamp caps the page size while offset still indexes the full
   result set.

4. Fix gofmt: the original file's leading-bullet comment indentation
   was off by two spaces; gofmt -l now reports clean.

All 14 subtests across both functions pass; full ./internal/handler/
suite still passes (3.2s).

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: Lambda <lambda@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Jiayuan Zhang
2026-06-02 11:17:41 +02:00
committed by GitHub
parent f539fdba83
commit 580ad5b492
2 changed files with 291 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
server/internal/handler/issue.go
View File

@@ -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
}
}

286
server/internal/handler/issue_limit_validation_test.go Normal file
View File

@@ -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)
}
})
}