Overview
TransactionListParams.page/per_page (src/models/transaction.rs:118-124) are plain Option<u32> with no upper bound on page, and TransactionService::list_for_user's offset computation performs unchecked u32 multiplication:
// src/services/transaction.rs:72-74
let page = params.page.unwrap_or(1).max(1);
let per_page = params.per_page.unwrap_or(20).clamp(1, 100);
let offset = (page - 1) * per_page;
per_page is correctly clamped to [1, 100], but page has no comparable ceiling — a client can send ?page=4294967295 (u32::MAX) or any large value, and (page - 1) * per_page (e.g. (4294967294) * 100) overflows u32's range (4294967295 max). Rust's overflow behavior depends on build profile: in a debug build, this arithmetic panics (attempt to multiply with overflow); in a release build (Rust's default Cargo.toml profile unless overflow-checks = true is explicitly set, which this project's Cargo.toml does not set), it silently wraps, producing an arbitrary, incorrect offset value that gets bound straight into the SQL query (transaction.rs:124-127) — meaning the response silently returns the wrong page of data (or an empty page) rather than erroring, with the client given no indication anything went wrong.
Either outcome is a real bug: a panic is an unhandled-panic-per-request issue (compounding the general absence of CatchPanicLayer already noted elsewhere), and a silent wraparound is a correctness bug that misleads a client into believing they've paged past all their data when they've actually received a query result computed from garbage math.
Requirements
- Clamp or reject
page to a sane range before use, the same way per_page already is — either cap it at a reasonable maximum (informed by what total_pages could plausibly be, given per_page's own cap of 100), or use checked/saturating arithmetic (page.saturating_sub(1).saturating_mul(per_page) at minimum, though an explicit upper-bound validation returning AppError::Validation for a clearly-nonsensical page value is the more honest fix, since a wildly out-of-range page silently saturating to "last possible offset" is still surprising behavior for a client to debug).
- Apply the same treatment anywhere else in the codebase that performs arithmetic directly on client-supplied pagination parameters, if any other list endpoint gains offset-based pagination in the future (currently
transaction.rs is the only implementation, per the companion issue on EscrowService/SubscriptionService lacking pagination entirely).
Acceptance Criteria
Additional Notes
Edge cases
total_pages (transaction.rs:131, itself computed via f64 division/ceil()) has its own, separate class of potential imprecision for very large total values, worth a quick look while this is being fixed but not the primary subject of this issue.- The fix should be verified under both debug and release builds, since the two profiles currently exhibit different (both wrong) behaviors for this bug — a fix validated only in a debug-mode test run could still leave the release-mode silent-wraparound behavior unaddressed if the fix relies on the debug-only overflow panic to "catch" the case rather than an explicit, profile-independent check.
Testing strategy
- A parameterized unit test directly on the offset-computation logic (extracted into a small pure function if not already easily testable in isolation) covering
page values from 1 through u32::MAX, run under both cargo test (debug) and cargo test --release to confirm consistent, correct behavior in both profiles.
Cross-references
- Distinct from the companion
interval_seconds-near-i64::MAX chrono panic issue — same general bug class (unbounded client input reaching unchecked arithmetic) but a completely different file, field, and numeric type (u32 multiplication overflow here vs. i64-backed chrono::Duration internal overflow there).
Overview
TransactionListParams.page/per_page(src/models/transaction.rs:118-124) are plainOption<u32>with no upper bound onpage, andTransactionService::list_for_user's offset computation performs uncheckedu32multiplication:per_pageis correctly clamped to[1, 100], butpagehas no comparable ceiling — a client can send?page=4294967295(u32::MAX) or any large value, and(page - 1) * per_page(e.g.(4294967294) * 100) overflowsu32's range (4294967295max). Rust's overflow behavior depends on build profile: in adebugbuild, this arithmetic panics (attempt to multiply with overflow); in areleasebuild (Rust's defaultCargo.tomlprofile unlessoverflow-checks = trueis explicitly set, which this project'sCargo.tomldoes not set), it silently wraps, producing an arbitrary, incorrectoffsetvalue that gets bound straight into the SQL query (transaction.rs:124-127) — meaning the response silently returns the wrong page of data (or an empty page) rather than erroring, with the client given no indication anything went wrong.Either outcome is a real bug: a panic is an unhandled-panic-per-request issue (compounding the general absence of
CatchPanicLayeralready noted elsewhere), and a silent wraparound is a correctness bug that misleads a client into believing they've paged past all their data when they've actually received a query result computed from garbage math.Requirements
pageto a sane range before use, the same wayper_pagealready is — either cap it at a reasonable maximum (informed by whattotal_pagescould plausibly be, givenper_page's own cap of 100), or use checked/saturating arithmetic (page.saturating_sub(1).saturating_mul(per_page)at minimum, though an explicit upper-bound validation returningAppError::Validationfor a clearly-nonsensicalpagevalue is the more honest fix, since a wildly out-of-range page silently saturating to "last possible offset" is still surprising behavior for a client to debug).transaction.rsis the only implementation, per the companion issue onEscrowService/SubscriptionServicelacking pagination entirely).Acceptance Criteria
GET /api/transactions?page=<u32::MAX>(and other extremepagevalues) returns a clean4xxresponse or a correctly-computed (via checked/saturating arithmetic) empty result, never a panic and never a silently-wrapped, incorrectoffset.pagevalues at and beyond the overflow boundary for a givenper_page, asserting no panic occurs and the returned data (or empty-page response) is computed correctly rather than from wrapped arithmetic.overflow-checks = trueto the release profile inCargo.tomlas defense in depth against this entire class of bug across the codebase — noting this has a small runtime performance cost and is a broader decision than this issue alone, but is directly relevant since this bug's release-mode behavior (silent wraparound) is strictly worse than its debug-mode behavior (at least a panic is loud).Additional Notes
Edge cases
total_pages(transaction.rs:131, itself computed viaf64division/ceil()) has its own, separate class of potential imprecision for very largetotalvalues, worth a quick look while this is being fixed but not the primary subject of this issue.Testing strategy
pagevalues from1throughu32::MAX, run under bothcargo test(debug) andcargo test --releaseto confirm consistent, correct behavior in both profiles.Cross-references
interval_seconds-near-i64::MAXchrono panic issue — same general bug class (unbounded client input reaching unchecked arithmetic) but a completely different file, field, and numeric type (u32multiplication overflow here vs.i64-backedchrono::Durationinternal overflow there).