fix(cleanup): pagination offset overflow, constant-time code check, module docs (PMS-684) #463

Merged

nrupard merged 1 commit from fix/PMS-684-cleanup into main

2026-07-24 17:53:44 +02:00

nrupard commented

2026-07-24 16:32:50 +02:00

Owner

What

Three small, independent correctness/quality fixes from a codebase review, grouped into one cleanup PR (PMS-684).

Why

A request like ?page=99999999 with the default per_page=25 produces an offset of 2,499,999,950, which exceeds i32::MAX; the as i32 cast wrapped it negative and Postgres rejected the OFFSET, returning HTTP 500 on the contacts and tickets list endpoints.
The login-approval code check used a non-constant-time == on two SHA-256 hex strings, inconsistent with the codebase's own constant-time convention for equivalent secret checks (practical risk is low given the SHA-256 preimage and the attempt cap, but it is a needless inconsistency).
CLAUDE.md and docs/dev-docs/codebase-state.md still claimed only auth/contacts/tenants/tickets have real handlers and that most route groups return HTTP 501, which contradicts the current api/router.rs.

Changes

Widen the five pagination.offset() as i32 casts (three in contacts/service.rs, two in tickets/service.rs) to as i64, matching the ~30 other call sites; limit stays i32 because per_page is bounded.
Replace stored_hash == code_hash in verify_login_approval with constant_time_eq::constant_time_eq(stored_hash.as_bytes(), code_hash.as_bytes()), consistent with the TOTP, RMM webhook, and bunyip webhook secret checks.
Correct the module-status text in CLAUDE.md and docs/dev-docs/codebase-state.md: ~30 modules now have real handlers, the stub_routes() 501 mechanism is gone, and the only remaining HTTP 501 is the PDF report-export format (CSV is implemented).

Tests

just pre-commit (Docker mirror of CI check.yml, since there is no host cargo): green. cargo fmt --check, clippy --all-targets -D warnings, cargo check --all-targets, 293 unit tests passed / 0 failed, doc tests passed.
Postgres-backed integration suite: the two binaries that exercise these changes pass: list_filter_pagination (12 passed) and login_approval (5 passed), alongside the rest of the suite. The RLS-isolation binaries (per_user_isolation, rls_isolation) additionally pass (4 and 1) when #[sqlx::test] runs as the superuser, matching CI's DATABASE_URL; with the local dev stack's migrator DATABASE_URL they cannot create the per-test unprivileged role (CREATEROLE), which is a pre-existing environment limitation unrelated to this change.

## What Three small, independent correctness/quality fixes from a codebase review, grouped into one cleanup PR (PMS-684). ## Why - A request like `?page=99999999` with the default `per_page=25` produces an offset of 2,499,999,950, which exceeds i32::MAX; the `as i32` cast wrapped it negative and Postgres rejected the OFFSET, returning HTTP 500 on the contacts and tickets list endpoints. - The login-approval code check used a non-constant-time `==` on two SHA-256 hex strings, inconsistent with the codebase's own constant-time convention for equivalent secret checks (practical risk is low given the SHA-256 preimage and the attempt cap, but it is a needless inconsistency). - `CLAUDE.md` and `docs/dev-docs/codebase-state.md` still claimed only auth/contacts/tenants/tickets have real handlers and that most route groups return HTTP 501, which contradicts the current `api/router.rs`. ## Changes - Widen the five `pagination.offset() as i32` casts (three in `contacts/service.rs`, two in `tickets/service.rs`) to `as i64`, matching the ~30 other call sites; `limit` stays i32 because per_page is bounded. - Replace `stored_hash == code_hash` in `verify_login_approval` with `constant_time_eq::constant_time_eq(stored_hash.as_bytes(), code_hash.as_bytes())`, consistent with the TOTP, RMM webhook, and bunyip webhook secret checks. - Correct the module-status text in `CLAUDE.md` and `docs/dev-docs/codebase-state.md`: ~30 modules now have real handlers, the `stub_routes()` 501 mechanism is gone, and the only remaining HTTP 501 is the PDF report-export format (CSV is implemented). ## Tests - `just pre-commit` (Docker mirror of CI check.yml, since there is no host cargo): green. cargo fmt --check, clippy --all-targets -D warnings, cargo check --all-targets, 293 unit tests passed / 0 failed, doc tests passed. - Postgres-backed integration suite: the two binaries that exercise these changes pass: `list_filter_pagination` (12 passed) and `login_approval` (5 passed), alongside the rest of the suite. The RLS-isolation binaries (`per_user_isolation`, `rls_isolation`) additionally pass (4 and 1) when `#[sqlx::test]` runs as the superuser, matching CI's DATABASE_URL; with the local dev stack's migrator DATABASE_URL they cannot create the per-test unprivileged role (CREATEROLE), which is a pre-existing environment limitation unrelated to this change.

nrupard added 1 commit

2026-07-24 16:32:51 +02:00

fix(cleanup): offset overflow, constant-time code check, module docs

E2E / Playwright against staging (pull_request) Successful in 45s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 1m43s

Details

Integration / integration tests (pull_request) Successful in 9m7s

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 0s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

93811303dd

Three independent correctness/quality fixes from a codebase review, grouped into one cleanup PR.

- Pagination offset truncation: the contacts and tickets list services cast `pagination.offset() as i32`, so a large `?page=` (offset above i32::MAX) wrapped to a negative i32 and Postgres rejected the OFFSET, returning HTTP 500 on the list endpoints. Widen the five casts to `as i64` to match the ~30 other call sites; `limit` stays i32 because per_page is bounded.
- Non-constant-time hash comparison: `verify_login_approval` compared the login-approval code hashes with `==`. Switch to `constant_time_eq::constant_time_eq` over the byte slices, consistent with the TOTP, RMM webhook, and bunyip webhook secret checks.
- Stale module-status docs: `CLAUDE.md` and `docs/dev-docs/codebase-state.md` claimed only auth/contacts/tenants/tickets have real handlers and most route groups return HTTP 501. `api/router.rs` now nests/merges ~30 implemented modules and the `stub_routes()` 501 mechanism is gone; the sole remaining HTTP 501 is the PDF report-export format. Correct both docs.

#PMS-684

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>