fix(node): clamp tasks query limit to [1, 200] (#399)

[teddiesloco]

Summary

• Fixes GET /api/v1/tasks: clamp limit to match siblings; negative limit returns 500 with raw DB error #399 (High Severity Security Vulnerability)
• Clamps q.limit in crates/gitlawb-node/src/api/tasks.rs to [1, MAX_TASK_LIMIT] (200) mirroring the pattern in crates/gitlawb-node/src/api/bounties.rs.
• Prevents negative limits from tripping Postgres 2201W (returning 500 with internal DB error details) and prevents unbounded i64::MAX full-table scans.
• Includes unit test verifying boundary clamping for -1, 0, 50, 500, and i64::MAX.

Summary by CodeRabbit

• Bug Fixes

  • Task listing now safely constrains requested result limits to between 1 and 200, preventing invalid or excessively large requests.

• Tests

  • Added coverage for negative, zero, standard, oversized, and maximum integer limit values.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: b2de378f-d894-44d8-8d4d-9902ef8b6bed

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and a026ec6.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/api/tasks.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The task listing API now clamps query limits to 1–200 before database access. Unit tests cover negative, zero, valid, oversized, and maximum integer limits.

Changes

Task list limit clamping

Layer / File(s)	Summary
Clamp and validate task limits crates/gitlawb-node/src/api/tasks.rs	The task handler clamps limits to the inclusive range 1–200. Unit tests cover negative, zero, valid, oversized, and maximum integer inputs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to a026e

Task list requests now bound invalid and excessive limit values before querying, preventing database errors and oversized responses while preserving normal limits and the existing default behavior. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the fix and its security impact, but it omits most required template sections, including motivation/context, kind of change, concrete change bullets, verification steps, revie…	Complete the required template sections. Identify the change as a bug fix or security fix, describe the affected crate and behavior, provide verification commands or test steps, complete the checklist items, and state whether protocol and s…

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: clamping the tasks query limit to the range 1–200.
Linked Issues check	✅ Passed	The implementation clamps task query limits to 1–200 and adds boundary tests for negative, zero, normal, oversized, and maximum integer values. This satisfies the coding objectives in issue [#399], wh…
Out of Scope Changes check	✅ Passed	The changes are limited to task query limit handling and related unit tests in gitlawb-node. No unrelated or out-of-scope changes are identified.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files.

Full details: Description check

Explanation

The description explains the fix and its security impact, but it omits most required template sections, including motivation/context, kind of change, concrete change bullets, verification steps, review checklist status, and protocol impact.

Resolution

Complete the required template sections. Identify the change as a bug fix or security fix, describe the affected crate and behavior, provide verification commands or test steps, complete the checklist items, and state whether protocol and signing impact is applicable.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

Warning

⚠️ This pull request shows signs of AI-generated slop (trivial_assertion). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

[greptile-apps]

Greptile Summary

The PR bounds REST task-list queries to between 1 and 200 records, preventing invalid negative database limits and oversized scans.

• Adds MAX_TASK_LIMIT and clamps the value passed to Db::list_tasks.
• Adds boundary assertions for negative, zero, ordinary, oversized, and maximum i64 values.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking opportunity to make the new limit test exercise production behavior.

The clamp correctly prevents negative SQL limits and caps oversized task queries; the only accepted concern is that its arithmetic-only test would not detect a regression in the handler.

Files Needing Attention: crates/gitlawb-node/src/api/tasks.rs

Important Files Changed

Filename	Overview
crates/gitlawb-node/src/api/tasks.rs	The handler applies the intended safe bounds, but its new test repeats the clamp expression rather than covering the production behavior.

_{Reviews (1): Last reviewed commit: "fix(node): clamp tasks query limit to [1..." | Re-trigger Greptile}

[greptile-apps]

Test duplicates clamp implementation

The test invokes i64::clamp directly instead of the production limit-normalization path, so it remains green if list_tasks stops enforcing the intended bounds and does not provide effective regression coverage.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[beardthelion]

Read the diff against bfc44f9 and compared it to bounties.rs and the GraphQL tasks resolver. The handler change is correct: q.limit.clamp(1, MAX_TASK_LIMIT) closes #399's negative-limit 500 (Postgres 2201W leaked via e.to_string()) and caps i64::MAX scans at 200 rows. Reverted the clamp line in a mutation worktree; test_list_tasks_query_limit_clamping stayed green, so the added test does not guard the production line.

Findings

• [P2] Replace the vacuous clamp unit test with REST handler probes
  crates/gitlawb-node/src/api/tasks.rs:309
test_list_tasks_query_limit_clamping only asserts (-1i64).clamp(1, MAX_TASK_LIMIT) and never calls list_tasks or hits the router. With the handler clamp removed, the test still passes. GraphQL already has load-bearing #[sqlx::test] cases (tasks_negative_limit_clamped, tasks_limit_ceiling_clamped_to_200); REST has none. #399 asked for probes that ?limit=-1 returns 200 (not 500), ?limit=9223372036854775807 is capped at 200 rows, and omitted limit still defaults to 50. Follow the certs pattern in test_support.rs (~15416): seed tasks, anon_get on GET /api/v1/tasks, assert status and count.

Not an ask, recorded only: REST clamps negative/zero to 1 row while GraphQL clamps to 0 rows on the same db.list_tasks sink; pre-existing, and #399 chose the bounties REST floor. tasks.rs still maps DB errors with e.to_string() on all handlers; out of scope for this diff but the same leak class on genuine DB faults.

[jatmn]

The production change correctly implements the [1, 200] limit requested in #399, while preserving the default of 50. I have one P2 finding: the added test does not protect that change against regression. The details below consolidate the evidence and the expected revision so this can be addressed in one focused pass.

Merge readiness

The PR Checks run for head a026ec6e5f8c281c0c978bae7e1764dc6308cc0b is action_required. The successful review and triage statuses do not establish that the normal Rust checks ran. An authorized maintainer needs to unblock the workflow; obtain the normal test/build/lint results on the revised head before merging. This is a validation prerequisite, separate from the finding below.

The reviewed branch is mergeable and its base matches current main at bfc44f926d08c0bf774e2c05dd76b245871294f1. There is no current rebase request.

Finding

[P2] Test the production REST limit boundary

crates/gitlawb-node/src/api/tasks.rs:309-314

What is wrong

test_list_tasks_query_limit_clamping invokes i64::clamp directly on integer literals. It never calls list_tasks, constructs a request that goes through ListTasksQuery deserialization, or observes the limit actually passed to the database. The assertion for literal 50 also does not exercise default_limit() or an omitted query parameter.

Consequently, restoring the handler's old database argument of q.limit leaves every added assertion unchanged and passing. The negative-limit database failure and oversized result set would then return without this regression test detecting either. The existing GraphQL tests do not close this gap: they exercise a different resolver with its own clamp.

The current production handler does apply the bounds correctly. This finding concerns the regression protection introduced by this PR; it does not claim that the fixed runtime failure is occurring on this head.

Root cause and the required outcome

The test duplicates the desired arithmetic instead of observing the application's request-to-database behavior. More literal boundary assertions would retain that same weakness. A standalone normalization helper test would cover the helper, but would not by itself prove that the REST handler uses its result or preserves query defaults.

Please make the tests reach the actual REST handler through HTTP query extraction and the real database boundary. The repository already provides #[sqlx::test] and crate::test_support::test_state(pool) for a migrated, isolated Postgres database. A small router mounting the production list_tasks handler with that state is sufficient; using the existing assembled-router harness is also reasonable. Use the real database-backed state, since the lazy state helper is intended for tests that never query the database.

The three cases below come directly from #399. Seed matching tasks in each isolated test and assert HTTP 200, the returned tasks array length, and count:

HTTP request	Matching tasks to seed	Expected tasks and count	What the fixture distinguishes
GET /api/v1/tasks?limit=-1	2	1	A successful floor of 1, rather than a database error or accidental fallback to 50.
GET /api/v1/tasks?limit=9223372036854775807	201	200	The actual ceiling, rather than an unclamped result or an accidental default of 50.
GET /api/v1/tasks	60	50	The omitted-parameter default, rather than the ceiling or another fallback.

Small fixtures can conceal incorrect limits. For example, a ceiling test with ten tasks returns ten both with and without a 200-row cap. The seed counts above make the specific regressions observable. Keep these tests isolated through the existing database harness so their counts do not depend on another test's rows.

Demonstrate that the tests guard the fix

After the tests pass, temporarily change the production path to pass the raw q.limit to the database again. The negative and oversized request tests should fail. Restore the clamp and confirm they pass. Separately changing the omitted-query default should make the default test fail. These are temporary verification edits; commit the correct production behavior and its tests.

This checks the property missing from the current test: the assertions must depend on the application behavior they are intended to protect. A green test run alone cannot establish that dependency.

Completing this revision

The Greptile comment and beardthelion's changes-requested review describe this same test gap. They are one finding with one corrective outcome, rather than several independent production defects. This head has no subsequent corrective commit, so the evidence does not support attributing the repeated discussion to a series of failed author revisions.

To close the feedback together:

1. Replace the arithmetic-only test with the three production-facing regression cases above.
2. Verify the tests detect the temporary regressions, then restore the correct implementation.
3. Format the changes and run the focused tests and repository checks. If the tests remain in api::tasks::tests, the focused command is shown below; if placed in the shared HTTP test module, use the filter matching their actual names.
4. Include the commands and results in the PR description, and obtain the normal CI results on the revision. Report any environment limitation explicitly.

cargo fmt --all
cargo fmt --all -- --check
cargo test --locked -p gitlawb-node api::tasks::tests --no-fail-fast
cargo clippy --locked --workspace --all-targets -- -D warnings

The database tests require the repository's documented PostgreSQL setup and DATABASE_URL. Let the normal PR workflow run the full test/build checks as well. Existing test infrastructure is sufficient for this revision.

The requested production contract remains [1, 200], default 50, the existing status/assignee filters, and the existing response shape with count equal to returned rows. This finding calls for regression tests of that contract. It does not request changes to GraphQL's separate floor, task authorization, pagination, indexes, or unrelated database-error handling. Describe the production benefit precisely as bounding fetched rows and response item count; a SQL LIMIT alone does not guarantee the database avoids scanning or sorting the table.

Maintainer coordination

#405 implements the same production clamp and includes REST/database tests for these cases. At its captured head 15b715552922f72d4a01841f406b426d99ac1357, its fmt + clippy check fails and it has a changes-requested review. It is useful overlap to coordinate, not a merge-ready replacement to assume. Select one fix for #399 rather than landing duplicate implementations.

#396 separately replaces this handler with task visibility and pagination logic. Coordinate merge ordering and review any resolved overlap when that work is combined. Neither overlapping PR adds feature requirements to the focused test revision requested here.

[beardthelion]

Thanks for the fix. The limit clamp is correct, but this was a race with #405, and I'm closing this one in favor of #405 for two reasons:

1. Fix #399: GET /api/v1/tasks: clamp limit to match siblings; negative limit returns 500 with raw DB error #405's tests drive the real handler through the router (GET /api/v1/tasks?limit=-1 and assert a 200 with clamped count), so they fail if the clamp is removed. The test here asserts i64::clamp directly, which passes regardless of whether the handler clamps.
2. Fix #399: GET /api/v1/tasks: clamp limit to match siblings; negative limit returns 500 with raw DB error #405 ran the full CI workflow (test, build, audit, MSRV, docker all green); this PR's CI didn't run because it's a fork PR.

The issue this targeted (#399) is now closed as a duplicate of #317, which covers the broader class: all eight e.to_string() error paths in tasks.rs, not just the limit clamp. If you want to contribute, #317 is open and a fix covering the remaining seven handlers is what closes it.

[beardthelion]

Closing in favor of #405 (real integration tests, full CI ran). See the comment above.