Repository navigation
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 intoOct 19, 2026
Conversation
…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>
|
@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! 🚀 |
Cjay-Cyber-2
approved these changes
Oct 19, 2026
Cjay-Cyber-2
left a comment
Collaborator
There was a problem hiding this comment.
Looks good — approving.
Cjay-Cyber-2
approved these changes
Oct 19, 2026
Cjay-Cyber-2
left a comment
Collaborator
There was a problem hiding this comment.
Looks good — approving.
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
mainsrc/lib/pagination.ts, applied to 9 endpoints; expense list filtersvalidateSignedXdrinsrc/services/stellar.ts, used by every submission path#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:
POST /auth/logoutauthLogout(IP){ 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/:idexpenseMutate(user)RATE_LIMIT_EXPENSE_CREATE_*bounded only creation, so the mutations that follow it were unbudgeted.POST /groups/:id/treasury/enablePOST /groups/:id/treasury/validate-signerstreasuryConfig(user)POST /groups/:groupId/webhooks/:webhookId/testwebhookTest(user)webhookCreatebounded 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/:idanchorPoll(reused)/anchors/sessions/:id,/api/v1/sep24/transaction) were already onanchorPoll; this one was missed.No new mechanism: each route names a policy from the table in
src/lib/rate-limit.tsand inherits the registration insrc/plugins/rate-limit.ts, so the 429 envelope (RATE_LIMITED+requestId) and theX-RateLimit-*/Retry-Afterheaders 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.tsgrew 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.tsadds 11 cases that drive the realbuildApp()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 assrc/lib/rate-limit.tsdocuments. 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/deliveriesanswered with an unboundedfindMany. 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/webhookshad the same shape with a smaller blast radius.Both now use the shared
(createdAt, id)cursor contract unchanged and return the standardmeta.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) andresponseBody(text from a third-party server echoed back to a group member).deliverySummaryinsrc/services/webhook-listing.tsis 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
GET /groups/:id/settlementsstatus,asset,startDate,endDate,includeTotalGET /groups/:id/ledgertype(one or more ofexpense,settlement,treasury),startDate,endDateGET /groups/:groupId/webhooks/:webhookId/deliveriesstatus,eventShaped exactly like the existing expense-list filters, and validated with Zod in the same way: an unknown value is a
VALIDATION_ERRORand a reversed date range isINVALID_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 + 1window, so dropping rows after the merge would return pages shorter than the requestedlimitand makehasMoredescribe the unfiltered scan. On the ledger,typetherefore decides which tables are read at all —?type=expenseissues one query instead of three — and the date bound goes into all threewhereclauses.The settlement
statusfilter is written as the exact inverse oftoPublicStatus. Two of the five public statuses are derived rather than stored: a row sitting atpendingorsubmittedpast itsexpiresAtis publiclyexpired. A filter matching the column directly would have disagreed with the status the same row reports when read throughGET /settlements/:id/status— worse than having no filter.publicStatusFilterreproduces the mapping including its clock-skew tolerance, and usesnotInrather thanequals: "pending"so an unrecognised column value lands inawaiting_signatureexactly as the mapping'sdefaultbranch puts it. The invariant: the five values partition the history, so every settlement matches exactly one and none is unreachable.includeTotalis 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 withtoPublicStatus; 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.tsalready had the real thing:validateSignedXdr, used byPOST /settlements/:id/confirm, the expense settle path, the treasury paths, and the worker at submission time.src/utils/stellar-xdr.tsalso exportedvalidateTransactionIntent, a second validator that was weaker than it in five ways:{ valid: true }, so a "validated" transaction could carry nobody's authorization;"10"for an intent of"10.0000000"was reported as an amount mismatch;"Transaction must contain exactly one operation";instanceofblock, three parses of the same envelope, andanycasts 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, fromsrc/validations/stellar-transaction.ts— is untouched.What replaces it, for the callers the issue describes:
matchPaymentIntentinsrc/services/stellar.ts— the intent matcher refactored fromassertMatchesIntentinto a pure function returning{ field, message } | null. Same checks, same order, same messages;assertMatchesIntentis now a three-line throwing wrapper over it, so the submission paths behave identically.src/services/xdr-inspection.ts— the non-throwing inspector.inspectSignedTransactionreturns{ ok: true, tx, hash, source, sequence }or{ ok: false, code, field, message }, wherecodeis one ofxdr_malformed/xdr_mismatch/intent_expired/xdr_unsignedandfieldnames what diverged. It adds no rules of its own — it composesparseSignedPaymentXdr,assertTimeBoundsValid,matchPaymentIntent, andverifySourceSignature, so there is exactly one description of what "matches the intent" means.assertSignedTransactionMatchesIntentis the throwing form, carrying the field in the error'sdetails.verifySourceSignatureis now the single signature check in the codebase, shared withvalidatePaymentTxand 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 samestellar.buildPaymentthe 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.sourceSequencecompares against the envelope's own sequence, whilestellar.buildPayment'ssourceSequenceargument is the account sequence Horizon reported — a transaction built from account sequencencarries sequencen + 1. Nothing populates the expectation's field today (there is no persisted column), so the check never runs; if someone wired it fromaccount.sequenceit 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-webThree response shapes changed and should be mirrored in
mergepay-web/src/lib/types.ts:GET /groups/:groupId/webhooks— addsmeta;webhooksis now one page.GET /groups/:groupId/webhooks/:webhookId/deliveries— addsmeta;deliveriesis now one page, and each row carries thedeliverySummaryfields only (payloadandresponseBodyremoved).GET /groups/:id/settlements,/groups/:id/ledger— unchanged shape; new optional query parameters, andmeta.totalwhenincludeTotal=true.Documented in
docs/api-contract.md(pagination, the three new filter tables, and the inspector under signed-XDR validation) andREADME.md(the rate-limit table rows and a new "Filtering the transaction history" section). The four new env vars are in.env.examplewith their rationale, andCHANGELOG.mdhas Added / Changed / Removed entries.Verification
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-limittiers 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