From f968496cc6dd9130edd4fce5812d5d7449cae45b Mon Sep 17 00:00:00 2001 From: Paco Cartones <253313177+pacocartones@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:59:57 +0200 Subject: [PATCH] fix(gamification): validate leaderboard limit/offset before the SQLite bind (#11059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⭐5 — LIMIT negativo = "sem limite" no SQLite: ?limit=-1 retornava a leaderboard inteira em endpoint management autenticado; ceil por Math.min só no upper. Duas camadas: route rejeita não-inteiro/fora de range com 400 (mesmo contrato de parseListLimit), getTopN clampeia como backstop defense-in-depth + exporta LEADERBOARD_MAX_LIMIT. TDD red→green, 5 casos novos, 84/84 suíte gamification. Fecha #11058. --- .../federation/leaderboard/route.ts | 9 ++- src/app/api/gamification/leaderboard/route.ts | 12 +++- src/lib/db/gamification.ts | 13 ++++- .../leaderboard-limit-validation.test.ts | 58 +++++++++++++++++++ 4 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 tests/unit/gamification/leaderboard-limit-validation.test.ts diff --git a/src/app/api/gamification/federation/leaderboard/route.ts b/src/app/api/gamification/federation/leaderboard/route.ts index e198ae2aef..26e2846122 100644 --- a/src/app/api/gamification/federation/leaderboard/route.ts +++ b/src/app/api/gamification/federation/leaderboard/route.ts @@ -38,7 +38,14 @@ export async function GET(request: NextRequest) { const url = new URL(request.url); const scope: LeaderboardScope = (url.searchParams.get("scope") || "global") as LeaderboardScope; - const limit = Number(url.searchParams.get("limit") || 100); + const rawLimit = url.searchParams.get("limit"); + const limit = rawLimit === null ? 100 : Number(rawLimit); + if (!Number.isInteger(limit) || limit < 1 || limit > 200) { + return NextResponse.json( + { error: "'limit' must be an integer between 1 and 200" }, + { status: 400, headers: CORS_HEADERS } + ); + } const entries = await getTopN(scope, limit); diff --git a/src/app/api/gamification/leaderboard/route.ts b/src/app/api/gamification/leaderboard/route.ts index 33159af39c..cded801d13 100644 --- a/src/app/api/gamification/leaderboard/route.ts +++ b/src/app/api/gamification/leaderboard/route.ts @@ -18,10 +18,18 @@ export async function GET(request: NextRequest) { const url = new URL(request.url); const scope = (url.searchParams.get("scope") || "global") as LeaderboardScope; - const limit = Number(url.searchParams.get("limit") || 50); + const rawLimit = url.searchParams.get("limit"); + const limit = rawLimit === null ? 50 : Number(rawLimit); const apiKeyId = url.searchParams.get("apiKeyId"); - const entries = await getTopN(scope, Math.min(limit, 200)); + if (!Number.isInteger(limit) || limit < 1 || limit > 200) { + return NextResponse.json( + { error: "'limit' must be an integer between 1 and 200" }, + { status: 400, headers: CORS_HEADERS } + ); + } + + const entries = await getTopN(scope, limit); let myRank: number | null = null; let neighbors = null; diff --git a/src/lib/db/gamification.ts b/src/lib/db/gamification.ts index 731c5d82ad..cd6c907170 100644 --- a/src/lib/db/gamification.ts +++ b/src/lib/db/gamification.ts @@ -131,13 +131,24 @@ export function getRank(apiKeyId: string, scope: string): number { return rankRow.rank; } +export const LEADERBOARD_MAX_LIMIT = 200; + export function getTopN(scope: string, limit: number, offset: number = 0): LeaderboardRow[] { + // Guard the SQLite LIMIT/OFFSET bind. A negative LIMIT means "no limit" in + // SQLite (returns the whole table), and a non-integer throws a datatype + // mismatch, so an unvalidated `?limit` on a caller route (e.g. the leaderboard + // endpoints) could read the entire leaderboard or 500. Clamp to a coherent + // range here as a defense-in-depth backstop, independent of route validation. + const safeLimit = Number.isFinite(limit) + ? Math.min(Math.max(Math.trunc(limit), 0), LEADERBOARD_MAX_LIMIT) + : 0; + const safeOffset = Number.isFinite(offset) ? Math.max(Math.trunc(offset), 0) : 0; const rows = db() .prepare( `SELECT api_key_id, scope, score, updated_at FROM leaderboard WHERE scope = ? ORDER BY score DESC LIMIT ? OFFSET ?` ) - .all(scope, limit, offset) as Array<{ + .all(scope, safeLimit, safeOffset) as Array<{ api_key_id: string; scope: string; score: number; diff --git a/tests/unit/gamification/leaderboard-limit-validation.test.ts b/tests/unit/gamification/leaderboard-limit-validation.test.ts new file mode 100644 index 0000000000..b574e9b438 --- /dev/null +++ b/tests/unit/gamification/leaderboard-limit-validation.test.ts @@ -0,0 +1,58 @@ +import { describe, it, before, after } from "node:test"; +import assert from "node:assert/strict"; + +import { getTopN, LEADERBOARD_MAX_LIMIT } from "../../../src/lib/db/gamification"; +import { getDbInstance } from "../../../src/lib/db/core"; + +// Regression for the unvalidated `?limit` that reached the SQLite LIMIT bind on +// the leaderboard endpoints. In SQLite a negative LIMIT means "no limit", so a +// caller passing limit=-1 would read the entire leaderboard; a non-integer would +// throw a datatype mismatch. getTopN must clamp the bind as a backstop. +describe("getTopN limit/offset clamping", () => { + const scope = "global"; + const keys: string[] = []; + + before(() => { + const db = getDbInstance(); + for (let i = 0; i < 5; i++) { + const k = `test-lb-${Date.now()}-${i}`; + keys.push(k); + db + .prepare( + "INSERT OR REPLACE INTO leaderboard (api_key_id, scope, score, updated_at) VALUES (?, ?, ?, ?)" + ) + .run(k, scope, 100 - i, new Date().toISOString()); + } + }); + + after(() => { + const db = getDbInstance(); + for (const k of keys) { + db.prepare("DELETE FROM leaderboard WHERE api_key_id = ?").run(k); + } + }); + + it("returns at most the requested number of rows", () => { + assert.equal(getTopN(scope, 2).length, 2); + }); + + it("treats a negative limit as empty, never as unbounded", () => { + // Pre-fix this returned every row (SQLite LIMIT -1 == no limit). + assert.equal(getTopN(scope, -1).length, 0); + assert.equal(getTopN(scope, -100).length, 0); + }); + + it("treats a non-integer limit as empty instead of throwing", () => { + assert.equal(getTopN(scope, Number.NaN).length, 0); + }); + + it("caps the limit at LEADERBOARD_MAX_LIMIT", () => { + const rows = getTopN(scope, LEADERBOARD_MAX_LIMIT + 5000); + assert.ok(rows.length <= LEADERBOARD_MAX_LIMIT); + }); + + it("never binds a negative offset", () => { + // Would throw or behave oddly if a negative offset reached SQLite. + assert.doesNotThrow(() => getTopN(scope, 2, -10)); + }); +});