fix(#950): bound page parameter on /supabase-discover via schema validation and controller clamp

[hariom888]

Closes #950

Summary

GET /api/match/supabase-discover accepted an unbounded page query parameter. Because Zod strips unknown keys by default, page passed through the validate() middleware silently. An authenticated user could pass page=99999&limit=100, issuing a .range(9999800, 9999899) query against the profiles table — a near-10M row offset on every call.

This PR adds two independent layers of defence.

Defence layers

Layer 1 — Schema allowlist (backend/validation/schemas.js)

Added page to matchSchemas.getSupabaseDiscover, mirroring the identical field already present in matchSchemas.getRecommendedPartners:

page: z
  .string()
  .optional()
  .refine(
    (val) =>
      val === undefined ||
      (/^\d+$/.test(val) && parseInt(val, 10) >= 1 && parseInt(val, 10) <= 1000),
    { message: "page must be an integer between 1 and 1000" }
  ),

Any page value outside 1–1000, or non-numeric, now returns HTTP 400 before the controller is reached.

Layer 2 — Controller clamp (backend/controllers/matchController.js)

// BEFORE
const page = Math.max(1, parseInt(req.query.page, 10) || 1);
// AFTER
const page = Math.min(Math.max(1, parseInt(req.query.page, 10) || 1), 1000);

Maximum possible .range() offset: (1000 - 1) × 100 = 99,900 rows — down from (99999 - 1) × 100 = 9,999,800.

Changes

File	Change
backend/validation/schemas.js	Add page field to matchSchemas.getSupabaseDiscover
backend/controllers/matchController.js	Clamp page to 1000 maximum
backend/tests/supabaseDiscover.test.js	New — 6 rejection cases, 2 boundary passes, 2 controller unit tests

Test coverage

Schema rejection (400): page=99999 · page=0 · page=-1 · page=1.5 · page=abc · limit=101

Schema acceptance: page=1000 (boundary) · absent page param

Controller unit tests: page=2&limit=10 → skip=10, to=19 · page=99999 clamped to max skip of 99900 (never reaches row 9,999,800)

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Strengthened pagination validation for the discover feature by clamping page to 1–1000 and limit to 1–100, preventing invalid pagination requests.

• Tests

  • Added automated coverage for discover pagination validation, boundary behavior, and correct pagination window calculations, including oversize and malformed inputs.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e1f3190c-99c4-455d-be8c-bbfad2daae43

📥 Commits

Reviewing files that changed from the base of the PR and between cc09209 and 6b611ce.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (2)

• backend/controllers/matchController.js
• backend/tests/supabaseDiscover.test.js

🚧 Files skipped from review as they are similar to previous changes (2)

• backend/controllers/matchController.js
• backend/tests/supabaseDiscover.test.js

---

📝 Walkthrough

Walkthrough

Fixes an unbounded page parameter vulnerability in getSupabaseDiscover. The Zod schema for matchSchemas.getSupabaseDiscover.query gains an optional page field constrained to integers 1–1000, and the controller adds a Math.min(..., 1000) clamp as defense-in-depth. A new Vitest suite covers HTTP validation rejections and pagination offset computation.

Changes

Page Parameter Bounding in getSupabaseDiscover

Layer / File(s)	Summary
Schema validation and controller clamp backend/validation/schemas.js, backend/controllers/matchController.js	matchSchemas.getSupabaseDiscover.query gains an optional page field with Zod refine enforcing integer values 1–1000. The controller updates page parsing from Math.max(1, ...) to Math.min(Math.max(1, ...), 1000), bounding the Supabase .range(skip, ...) offset.
Vitest suite: mocks, setup, and validation coverage backend/tests/supabaseDiscover.test.js	Introduces a Supabase query-builder mock via vi.mock, a minimal Express test app with the validate middleware, HTTP-layer tests asserting 400 for out-of-range/non-integer/negative page and over-cap limit, boundary-passing tests for page=1000 and omitted page, and controller unit tests capturing .range(from, to) for page=2,limit=10 and the page=99999 clamp scenario.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• durdana3105/peer-learning#774: Modifies getSupabaseDiscover pagination logic using the same page/limit-driven Supabase .range(skip, ...) window that this PR further constrains with a 1000-page cap.

Suggested labels

type:bug, quality:clean

🐇 A bunny named Page once hopped without end,
Through rows upon rows that would never suspend.
But now there's a fence at one thousand, no more—
The schemas and clamps guard the database's door.
No runaway queries, no full-table sprint,
Just bounded pagination—take that as a hint! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title correctly summarizes the main change: adding bounds to the page parameter via schema validation and controller clamp to fix issue #950.
Linked Issues check	✅ Passed	All requirements from issue #950 are met: page parameter validation added to schema with 1-1000 bounds, controller clamping implemented to limit max offset, and comprehensive tests confirm proper validation and boundary enforcement.
Out of Scope Changes check	✅ Passed	All changes are directly related to addressing the page parameter vulnerability described in issue #950; no unrelated modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/controllers/matchController.js (1)

154-156: ⚡ Quick win

Harden limit parsing at controller level as well.

page now has defense-in-depth clamping, but limit is only capped at the top end. Clamping limit to 1..100 here keeps skip/.range() safe even if this handler is invoked outside the validated route path.

Suggested patch

-    const limit = Math.min(parseInt(req.query.limit, 10) || 100, 100);
+    const limit = Math.min(Math.max(1, parseInt(req.query.limit, 10) || 100), 100);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/controllers/matchController.js` around lines 154 - 156, The limit
variable in the matchController.js file at the limit declaration lacks
lower-bound clamping, which could result in zero or negative values if
unexpected input is passed directly to the handler outside of validated route
paths. Apply the same defense-in-depth clamping strategy used for the page
variable by wrapping the parseInt call with Math.max(1, ...) to ensure limit is
clamped between 1 and 100, protecting skip and .range() calculations from
invalid values.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/tests/supabaseDiscover.test.js`:
- Around line 152-163: The boundary test for page=1000 uses a weak assertion
that only checks the response status is not 400, which allows other failure
statuses like 500 to pass undetected. Replace the weak
`expect(res.status).not.toBe(400)` assertion in the boundary test with a
positive assertion that explicitly checks for the expected success status code
(200). Apply the same fix to the other boundary test mentioned in the comment at
lines 165-173, ensuring both tests now assert the specific expected successful
response status rather than just excluding a single failure case.

---

Nitpick comments:
In `@backend/controllers/matchController.js`:
- Around line 154-156: The limit variable in the matchController.js file at the
limit declaration lacks lower-bound clamping, which could result in zero or
negative values if unexpected input is passed directly to the handler outside of
validated route paths. Apply the same defense-in-depth clamping strategy used
for the page variable by wrapping the parseInt call with Math.max(1, ...) to
ensure limit is clamped between 1 and 100, protecting skip and .range()
calculations from invalid values.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7bf2f243-098d-4820-beb9-4189932e3778

📥 Commits

Reviewing files that changed from the base of the PR and between 7200d81 and cc09209.

📒 Files selected for processing (3)

• backend/controllers/matchController.js
• backend/tests/supabaseDiscover.test.js
• backend/validation/schemas.js

[vercel]

@hariom888 is attempting to deploy a commit to the durdana3105's projects Team on Vercel.

A member of the Team first needs to authorize it.

[hariom888]

@durdana3105 These 6 CI failures are pre-existing on main and not introduced by this PR. The express module resolution error (ERR_MODULE_NOT_FOUND) and missing docs/api.md affect multiple test files that existed before this branch. All 75 previously passing tests continue to pass. This PR introduces no new failures.