Skip to content

feat: rate-limit the remaining sensitive routes, filter the history endpoints, consolidate signed-XDR validation (#682, #692, #693, #694) - #856

Merged
Cjay-Cyber-2 merged 1 commit into
mergepay:mainfrom
ciscokwiz:feat/rate-limit-pagination-xdr-hardening
Oct 19, 2026
Merged

Cjay-Cyber-2 merged 1 commit into
mergepay:mainfrom
ciscokwiz:feat/rate-limit-pagination-xdr-hardening

Conversation

@ciscokwiz

Copy link
Copy Markdown
Contributor

Closes #682
Closes #692
Closes #693
Closes #694

Four issues in three areas that already had substantial prior work. Rather than re-implement what is there, each change closes the specific gap the existing implementation left. One PR because #682 and #693 are the same task described twice, and the remaining two share the audit that found them.

Issue Already in main What this PR adds
#682, #693 21 per-route policies; global bucket; 429 envelope + headers; store switch; a wiring audit suite 4 new policies for the sensitive routes still on the global bucket, + 1 reuse
#692 Cursor contract in src/lib/pagination.ts, applied to 9 endpoints; expense list filters Pagination for the 2 endpoints that still had none; filters for the settlement list, the ledger, and the delivery list
#694 validateSignedXdr in src/services/stellar.ts, used by every submission path Removal of a weaker duplicate validator; the matcher refactored so there is one copy of the rules; a non-throwing, field-reporting inspector

#682 / #693 — rate limiting for the routes that were still on the global bucket

The tiers for SEP-10 auth, settlement submission, and anchor initiation were already in place. Auditing the booted route table against the policy table turned up a second group of routes whose cost is not in their response — all of them answering from the blanket global allowance, which no single caller can exhaust:

Route Policy Default Why it needed its own budget
POST /auth/logout authLogout (IP) 20/min Revokes every refresh token a user holds and writes an audit row. The route answers { ok: true } for an absent or invalid token by design — it must not be an oracle for token validity — which also means the write path is reachable anonymously. IP-keyed for the same reason the other pre-session auth routes are.
PATCH / DELETE /expenses/:id expenseMutate (user) 30/min Rewrite what every other participant owes. RATE_LIMIT_EXPENSE_CREATE_* bounded only creation, so the mutations that follow it were unbudgeted.
POST /groups/:id/treasury/enable
POST /groups/:id/treasury/validate-signers
treasuryConfig (user) 10/min Each reads live account state from Horizon — so each call spends upstream budget — and both decide who may move group funds, as opposed to moving it.
POST /groups/:groupId/webhooks/:webhookId/test webhookTest (user) 5/min Turns one inbound request into an outbound HTTP delivery on the same retry schedule as a real event. webhookCreate bounded registering an endpoint but not ringing it — the cheaper half of the same amplification. Tightest write budget in the table: a test fire confirms wiring once, never in a loop.
GET /withdraw/:id anchorPoll (reused) 60/min The anchor-transaction status read a client polls through an interactive withdrawal. Its siblings (/anchors/sessions/:id, /api/v1/sep24/transaction) were already on anchorPoll; this one was missed.

No new mechanism: each route names a policy from the table in src/lib/rate-limit.ts and inherits the registration in src/plugins/rate-limit.ts, so the 429 envelope (RATE_LIMITED + requestId) and the X-RateLimit-* / Retry-After headers are the ones already shipping. Every new policy has its own key prefix, so none of them can draw down the global bucket or another tier.

Tests. tests/rate-limit-wiring.test.ts grew 7 rows in its audited route list — that suite fails if a policy is declared and never applied, if a route hand-rolls its numbers, or if a limit drifts from the table, so the new rows are themselves the regression guard. tests/rate-limit-sensitive-routes.test.ts adds 11 cases that drive the real buildApp() past each new budget and assert the 429 status, the headers reporting the per-route max, the JSON envelope, and bucket independence (expense edits do not spend the creation budget; treasury configuration does not spend the budget that guards moving funds; a bearer token buys no second logout budget).

One thing worth flagging for reviewers: two of these cases originally asserted that routes sharing a policy share a counter. They do under RATE_LIMIT_STORE=database (one row per key) but not under the default in-memory store, which gives each route its own child store — exactly as src/lib/rate-limit.ts documents. The cases now assert per-route enforcement, which is what holds in every deployment.

#692 — pagination and filtering on the group transaction-history endpoints

Two endpoints had no pagination at all

GET /groups/:groupId/webhooks/:webhookId/deliveries answered with an unbounded findMany. One row is written per event per endpoint, and a row is kept for every retry, so it grew without bound in three places at once — the database scan, the JSON serialization, and the response body — and the first request a busy group made after a quiet week was the one that noticed. GET /groups/:groupId/webhooks had the same shape with a smaller blast radius.

Both now use the shared (createdAt, id) cursor contract unchanged and return the standard meta.

The delivery list also stops returning two fields it never should have: payload (the exact signed request body — the group's own data re-served through a different authorization path) and responseBody (text from a third-party server echoed back to a group member). deliverySummary in src/services/webhook-listing.ts is now an allowlist, so adding a column to the model cannot silently widen what the endpoint exposes. The operationally useful fields — status, attempts, responseStatusCode, the attempt timestamps — are all still there.

Filtering was the other half

Endpoint New filters
GET /groups/:id/settlements status, asset, startDate, endDate, includeTotal
GET /groups/:id/ledger type (one or more of expense, settlement, treasury), startDate, endDate
GET /groups/:groupId/webhooks/:webhookId/deliveries status, event

Shaped exactly like the existing expense-list filters, and validated with Zod in the same way: an unknown value is a VALIDATION_ERROR and a reversed date range is INVALID_RANGE, never a silently empty page.

Two details that carry the design:

Filters are pushed into the query, never applied to a fetched page. Each table is read with its own limit + 1 window, so dropping rows after the merge would return pages shorter than the requested limit and make hasMore describe the unfiltered scan. On the ledger, type therefore decides which tables are read at all — ?type=expense issues one query instead of three — and the date bound goes into all three where clauses.

The settlement status filter is written as the exact inverse of toPublicStatus. Two of the five public statuses are derived rather than stored: a row sitting at pending or submitted past its expiresAt is publicly expired. A filter matching the column directly would have disagreed with the status the same row reports when read through GET /settlements/:id/status — worse than having no filter. publicStatusFilter reproduces the mapping including its clock-skew tolerance, and uses notIn rather than equals: "pending" so an unrecognised column value lands in awaiting_signature exactly as the mapping's default branch puts it. The invariant: the five values partition the history, so every settlement matches exactly one and none is unreachable.

includeTotal is opt-in for the same reason it is on the expense list — counting a filtered set is a second query over every matching row, which the cursor contract otherwise never pays for — and counts the filters without the cursor, so the total stays stable while a client pages.

Tests. tests/routes/history-pagination-filtering.test.ts, 38 cases: the reads are bounded and deterministically ordered; filters reach the Prisma query and compose with the cursor rather than replacing it; the boundary conditions the issue asks for (empty result, a full last page, the page before it, a malformed cursor, a limit above the maximum); the status filter's agreement with toPublicStatus; and that the payload, response body, and endpoint secret stay out of the responses.

#694 — one signed-XDR validator instead of two

src/services/stellar.ts already had the real thing: validateSignedXdr, used by POST /settlements/:id/confirm, the expense settle path, the treasury paths, and the worker at submission time.

src/utils/stellar-xdr.ts also exported validateTransactionIntent, a second validator that was weaker than it in five ways:

  • no signature verification at all — an unsigned envelope was { valid: true }, so a "validated" transaction could carry nobody's authorization;
  • no sequence-number check;
  • raw string amount comparison, so a wallet returning "10" for an intent of "10.0000000" was reported as an amount mismatch;
  • a fee-bump wrapper (a genuinely different transaction) reported as "Transaction must contain exactly one operation";
  • no fee-band or time-bounds check, a dead instanceof block, three parses of the same envelope, and any casts throughout.

It had no callers outside its own test file. Keeping a deprecated shim would have preserved exactly the drift the issue is about, so it is removed. isValidXdr — the part of that module production code does use, from src/validations/stellar-transaction.ts — is untouched.

What replaces it, for the callers the issue describes:

  • matchPaymentIntent in src/services/stellar.ts — the intent matcher refactored from assertMatchesIntent into a pure function returning { field, message } | null. Same checks, same order, same messages; assertMatchesIntent is now a three-line throwing wrapper over it, so the submission paths behave identically.
  • src/services/xdr-inspection.ts — the non-throwing inspector. inspectSignedTransaction returns { ok: true, tx, hash, source, sequence } or { ok: false, code, field, message }, where code is one of xdr_malformed / xdr_mismatch / intent_expired / xdr_unsigned and field names what diverged. It adds no rules of its own — it composes parseSignedPaymentXdr, assertTimeBoundsValid, matchPaymentIntent, and verifySourceSignature, so there is exactly one description of what "matches the intent" means. assertSignedTransactionMatchesIntent is the throwing form, carrying the field in the error's details.
  • Signature verification is on by default and can only be waived explicitly, for the paths where authorship is established elsewhere (an unsigned intent read back for display, or a multisig proposal whose shared source account never signs with its own key). verifySourceSignature is now the single signature check in the codebase, shared with validatePaymentTx and the multisig collector.

A rejection names the field and nothing else — never the envelope, a value found in it, or a key — since the envelope is untrusted input and the message is logged and returned.

Tests. tests/services/xdr-inspection.test.ts, 29 cases built from the same stellar.buildPayment the API serves to clients, so a fixture cannot drift from what a real wallet is asked to sign: the accepted case and its hash/source/sequence; unsigned, wrong-signer, and wrong-network signatures; every intent divergence with its own field; unexpected operations (two operations, a non-payment operation, an operation-level source override, an out-of-band fee); malformed input, a fee-bump wrapper, a lapsed window, and an envelope that never expires; precision-insensitive amounts; and that a rejection discloses nothing from the envelope.

Also in this PR

PaymentExpectation.sourceSequence compares against the envelope's own sequence, while stellar.buildPayment's sourceSequence argument is the account sequence Horizon reported — a transaction built from account sequence n carries sequence n + 1. Nothing populates the expectation's field today (there is no persisted column), so the check never runs; if someone wired it from account.sequence it would reject every envelope it was built for. The semantics are load-bearing in an existing test, so this PR documents the trap on the field rather than changing it. Worth a follow-up issue if the column is ever added.

Contract changes for mergepay-web

Three response shapes changed and should be mirrored in mergepay-web/src/lib/types.ts:

  • GET /groups/:groupId/webhooks — adds meta; webhooks is now one page.
  • GET /groups/:groupId/webhooks/:webhookId/deliveries — adds meta; deliveries is now one page, and each row carries the deliverySummary fields only (payload and responseBody removed).
  • GET /groups/:id/settlements, /groups/:id/ledger — unchanged shape; new optional query parameters, and meta.total when includeTotal=true.

Documented in docs/api-contract.md (pagination, the three new filter tables, and the inspector under signed-XDR validation) and README.md (the rate-limit table rows and a new "Filtering the transaction history" section). The four new env vars are in .env.example with their rationale, and CHANGELOG.md has Added / Changed / Removed entries.

Verification

npm run build   # clean, zero TS errors under strict
npm test        # 207 files, 4252 passed | 22 skipped
npx eslint "src/**/*.ts"   # 0 errors, 32 warnings — all pre-existing unused-vars

Baseline before this branch was 205 files / 4170 passed, so this adds 78 tests and changes no existing assertion except the two noted above (the removed validator's four cases, replaced by the inspector's 29; and the wiring audit's route list, which must grow when a route gains a policy).

Tests need no database or network — Prisma, Horizon, and the Stellar SDK's server are mocked throughout, per CONTRIBUTING.

Note on the one-PR-per-issue convention

CONTRIBUTING asks for one PR per issue. #682 and #693 are the same task written twice (both ask for @fastify/rate-limit tiers on auth and payment/anchor routes, both land on the same table and the same two test files), so splitting them would mean two PRs editing the same lines. #692 and #694 are genuinely separate and could have been split; they are here because one audit of the route table and the service layer found all four gaps, and the reviewer reads the same context once. Happy to split into two or three PRs if maintainers prefer.

🤖 Generated with Claude Code

…ints, consolidate XDR validation (mergepay#682, mergepay#692, mergepay#693, mergepay#694)

Four issues, three distinct gaps left by the earlier work in each area.

Rate limiting (mergepay#682, mergepay#693). The tiers for SEP-10 auth, settlement
submission, and anchor initiation were already in place; what the policy
table did not model was a second group of routes whose cost is not in
their response. Four new policies, each with its own key prefix and
env-tunable budget: `authLogout` (the route answers `ok` without a valid
token by design, so the revoke-and-audit path is reachable anonymously),
`expenseMutate` (editing or deleting an expense rewrites every
participant's obligations, while only creation was budgeted),
`treasuryConfig` (enable and validate-signers each read live account
state from Horizon and decide who may move group funds), and
`webhookTest` (one inbound request becomes an outbound delivery with
retries). `GET /withdraw/:id` takes the existing `anchorPoll` budget —
it was the one anchor-transaction read still on the global bucket.

Pagination and filtering (mergepay#692). Two list endpoints answered with
unbounded `findMany` queries; the webhook delivery list in particular
grows by one row per event per endpoint per retry, so it grew without
bound in the database scan, the serialization, and the response body at
once. Both now use the shared `(createdAt, id)` cursor contract and
return standard `meta`, and the delivery list stops returning the stored
request payload and the receiver's response text. Filtering was the
other half: settlements by public status, asset, and date range; the
ledger by entry type and date range; deliveries by status and event.
Every filter is pushed into the query rather than applied to a fetched
page — filtering after the merge would return pages shorter than the
requested limit and make `hasMore` describe the unfiltered scan.

XDR validation (mergepay#694). `src/utils/stellar-xdr.ts` carried a second
intent validator that was weaker than the submission path's: it never
verified signatures or sequence numbers, compared amounts as raw strings
(so "10" did not match "10.0000000"), parsed the envelope three times,
and reported a fee-bump wrapper as "must contain exactly one operation".
It had no callers outside its own test. Removed, and the intent matcher
in src/services/stellar.ts refactored into a pure, field-reporting
`matchPaymentIntent` that both the throwing submission path and the new
non-throwing `src/services/xdr-inspection.ts` compose — so there is one
description of what "matches the intent" means, and signature
verification is on by default rather than absent.

Also documents the `sourceSequence` expectation's off-by-one against
`buildPayment`'s account sequence, which nothing populates today and
would silently reject every envelope if it ever were.

Verification: `npm test` 4252 passed / 22 skipped across 207 files (up
from 4170/205), `npm run build` clean, no new lint warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@drips-wave

drips-wave Bot commented Oct 19, 2026

Copy link
Copy Markdown

@ciscokwiz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Cjay-Cyber-2 Cjay-Cyber-2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good — approving.

@Cjay-Cyber-2 Cjay-Cyber-2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good — approving.

@Cjay-Cyber-2
Cjay-Cyber-2 merged commit 7a0fcaf into mergepay:main Oct 19, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment