refactor(server): standardize handler check order; unify pagination (#89) #109

Merged

icub3d merged 1 commit from feature/89-check-order-pagination into main

2026-07-11 03:46:06 +00:00

icub3d commented

2026-07-11 03:45:38 +00:00

Owner

Finishes #89. Five of its six checklist items (thiserror, new_id fallback, parking_lot, event_json warn, targeted dead_code allows) already landed via #107 — this PR completes the remaining one: standardize check ordering and unify the pagination envelope / after cursor.

Closes #89.

Check ordering

The canonical order is now documented in crate::permissions: feature flag → auth/read-only/membership (extractors) → parent-resource existence → permission → input validation → target existence → object-level guards.

Deviations fixed (found by a full audit of all ~66 handlers):

Threads: only create_thread was gated on the message_threads flag — get_thread, list_thread_messages, create_thread_message, follow/unfollow/mark_read all bypassed it. All thread routes are now gated (new require_feature() helper in crate::error, also adopted by attachments and reactions).
Reactions: handlers computed channel-scoped permissions against a channel they never verified exists. All four now run the shared channel-existence check first, and a bogus channel is a 404 channel not found instead of falling through to 404 message not found.
create_thread likewise never verified the path channel.
Channels: update_channel/delete_channel now return an explicit 404 "channel not found" rather than the generic storage-layer "not found".
One ensure_channel_exists() helper (in channels, re-exported crate-wide) replaces the copies in messages and attachments.

Pagination

after cursor implemented for channel list_messages (it was accepted-then-rejected with a 400, while the thread-messages endpoint honored it via the same ListMessagesQuery). The sqlite query mirrors list_thread_messages: after pages oldest-first, before newest-first, before wins if both are given.
list_reactors: its total field was just the returned page length — misleading, and nothing (client or SDK) consumed it. Replaced with the standard fetch-limit+1 has_more, matching MessagePage. The ListReactorsQuery/ReactorEntry/ListReactorsResponse types were hand-duplicated in server, Rust SDK, and web client; they now live in shared::rest (per the #82 direction) and the SDK re-exports them.
list_thread_messages now uses the batched enrich_messages() (3 queries per page) instead of the per-message loop (3 queries per message) — same fix #84 made for channel messages.

Not changed (noted from the audit, out of scope)

Disabled features return 403 (not 404/501) — kept as the existing convention, now applied uniformly.
create_invite returns 400 for a missing grant_role_id; preview_invite uses 410 — left as-is.
Full-list endpoints (channels, roles, members, bans, …) intentionally remain unpaginated complete lists.

Verification

cargo test --workspace — 209 passed, including 5 new regression tests (after-cursor paging, thread-route feature gating, reactions channel-404, reactors has_more, channel update/delete 404)
cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check — clean
pnpm lint, pnpm test (89 passed) — clean

🤖 Generated with Claude Code

Finishes #89. Five of its six checklist items (thiserror, `new_id` fallback, `parking_lot`, `event_json` warn, targeted `dead_code` allows) already landed via #107 — this PR completes the remaining one: **standardize check ordering and unify the pagination envelope / `after` cursor**. Closes #89. ## Check ordering The canonical order is now documented in `crate::permissions`: feature flag → auth/read-only/membership (extractors) → parent-resource existence → permission → input validation → target existence → object-level guards. Deviations fixed (found by a full audit of all ~66 handlers): - **Threads**: only `create_thread` was gated on the `message_threads` flag — `get_thread`, `list_thread_messages`, `create_thread_message`, `follow`/`unfollow`/`mark_read` all bypassed it. All thread routes are now gated (new `require_feature()` helper in `crate::error`, also adopted by attachments and reactions). - **Reactions**: handlers computed channel-scoped permissions against a channel they never verified exists. All four now run the shared channel-existence check first, and a bogus channel is a `404 channel not found` instead of falling through to `404 message not found`. - **`create_thread`** likewise never verified the path channel. - **Channels**: `update_channel`/`delete_channel` now return an explicit `404 "channel not found"` rather than the generic storage-layer `"not found"`. - One `ensure_channel_exists()` helper (in `channels`, re-exported crate-wide) replaces the copies in `messages` and `attachments`. ## Pagination - **`after` cursor implemented for channel `list_messages`** (it was accepted-then-rejected with a 400, while the thread-messages endpoint honored it via the same `ListMessagesQuery`). The sqlite query mirrors `list_thread_messages`: `after` pages oldest-first, `before` newest-first, `before` wins if both are given. - **`list_reactors`**: its `total` field was just the returned page length — misleading, and nothing (client or SDK) consumed it. Replaced with the standard fetch-limit+1 `has_more`, matching `MessagePage`. The `ListReactorsQuery`/`ReactorEntry`/`ListReactorsResponse` types were hand-duplicated in server, Rust SDK, and web client; they now live in `shared::rest` (per the #82 direction) and the SDK re-exports them. - **`list_thread_messages`** now uses the batched `enrich_messages()` (3 queries per page) instead of the per-message loop (3 queries per message) — same fix #84 made for channel messages. ## Not changed (noted from the audit, out of scope) - Disabled features return 403 (not 404/501) — kept as the existing convention, now applied uniformly. - `create_invite` returns 400 for a missing `grant_role_id`; `preview_invite` uses 410 — left as-is. - Full-list endpoints (channels, roles, members, bans, …) intentionally remain unpaginated complete lists. ## Verification - `cargo test --workspace` — 209 passed, including 5 new regression tests (after-cursor paging, thread-route feature gating, reactions channel-404, reactors `has_more`, channel update/delete 404) - `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --check` — clean - `pnpm lint`, `pnpm test` (89 passed) — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code)

icub3d added 1 commit

2026-07-11 03:45:39 +00:00

refactor(server): standardize handler check order; unify pagination (#89 )

Security Audit / Gitleaks (Secrets) (pull_request) Successful in 8s

Details

Security Audit / Trivy (SCA & IaC) (pull_request) Failing after 22s

Details

Security Audit / PNPM Audit (JS SCA) (pull_request) Failing after 23s

Details

CI / Client — test & lint (pull_request) Successful in 42s

Details

Security Audit / Semgrep (SAST) (pull_request) Failing after 1m17s

Details

CI / Server — test & lint (pull_request) Successful in 2m53s

Details

Security Audit / Cargo Audit (Rust SCA) (pull_request) Failing after 3m49s

Details

f5a40c5ce6

Finishes the last item of #89 (the rest landed via #107).

Check ordering — canonical order documented in crate::permissions:
feature flag -> auth/read-only/membership (extractors) -> parent existence ->
permission -> input validation -> target existence -> object guards.
- error: add require_feature(); attachments/reactions/threads use it.
- threads: gate ALL thread routes on message_threads (previously only
  create_thread was gated); create_thread now verifies the channel exists.
- reactions: verify the path channel exists before computing channel-scoped
  permissions (previously a bogus channel silently resolved to base perms).
- channels: share one ensure_channel_exists() helper crate-wide (was
  duplicated in messages + attachments); update/delete_channel return an
  explicit "channel not found" 404.

Pagination:
- Implement the `after` cursor for channel list_messages (storage trait +
  sqlite, mirroring list_thread_messages) instead of rejecting it with 400.
  Pages with `after` are oldest-first, `before` newest-first, `before` wins.
- list_reactors: promote ListReactorsQuery/ReactorEntry/ListReactorsResponse
  to shared::rest (were hand-duplicated in server, SDK, and client); replace
  the misleading `total` (page length, not a global count — nothing consumed
  it) with the standard limit+1 `has_more`.
- list_thread_messages: enrich via the batched enrich_messages() (3 queries
  per page instead of 3 per message).

Tests: after-cursor paging, thread-route feature gating, reactions
channel-404, reactors has_more, channel update/delete 404.

Closes #89

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>