Jiayuan Zhang 580ad5b492 fix(issues): validate and clamp limit/offset in ListIssues (MUL-2847) (#3585) ...

* 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>

2026-06-02 17:17:41 +08:00

10 KiB

Raw Permalink Blame History

View Raw