fix(throttler): track authenticated routes per user/merchant at runtime (AC4) - #533
Merged
Cedarich merged 11 commits intoAug 29, 2026
Merged
Conversation
…nforced
- Register ThrottlerGuard as APP_GUARD in CustomThrottlerModule
- Add getTracker for identity-aware tracking (user:{id} for authenticated, IP for unauthenticated)
- Fix @Throttle TTL values from seconds to milliseconds (v6.5.0 expects ms)
- Implement Redis block duration with separate block key and pttl handling
- Add @public() to auth handshake, health, and public invoice endpoints
- Swap import order in app.module.ts for correct guard execution
- Add configurable trust proxy via TRUST_PROXY env var
- Add 14 unit tests for Redis storage and E2E identity isolation tests
Closes ZyntariHQ#485
Pre-existing formatting-only changes (quote style + import collapsing) in files unrelated to ZyntariHQ#528. Required so the repo-wide 'npm run lint' step passes in CI; no logic changed. These files used single quotes while the project's prettier config (no .prettierrc, so prettier defaults to double quotes) flagged them. They are not part of the rate-limiting fix.
…me (AC4) The global ThrottlerGuard runs before the route-level JwtAuthGuard, so req.user is unset when getTracker computes the key. The previous req.user?.id branch never executed for authenticated routes, collapsing every logged-in caller behind the same client-IP bucket (violating AC4 and breaking the per-user isolation e2e case). getTracker now derives the caller identity from the verified bearer token (merchantId -> merchant:<id>, sub -> user:<sub>) when req.user is absent, falling back to req.ip for unauthenticated routes. This keeps the throttle-first ordering (AC3 preserved) and is non-breaking. Closes ZyntariHQ#528
Contributor
|
Please fix unit tes |
… and fix unit-test mock
Contributor
Author
My bro please add me in your 10 contributors to get paid! |
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.
Summary
This PR completes acceptance criterion AC4 for rate-limit enforcement: authenticated requests are now throttled per user/merchant instead of collapsing every logged-in caller behind the same client-IP bucket. The global
ThrottlerGuard(registered ine61d40b, which closed the core of #528) runs before the route-levelJwtAuthGuard, soreq.userwas never populated when the throttle key was computed — the per-user keying code existed but never executed.getTrackernow derives the caller's identity from the verified bearer token directly.Closes #528
Problem
#528 reported rate limits were inert. Guard registration, block persistence, and proxy-aware IP resolution were already merged (
e61d40b), but one criterion was still not met at runtime: AC4 — "Authenticated routes tracked per user or merchant." BecauseThrottlerGuardis the only global guard andJwtAuthGuardis applied per-route via@Auth(), the global guard executes first andreq.useris unset at key-computation time. Thereq.user?.idbranch ingetTrackertherefore never fired, and every authenticated request fell back toreq.ip. Concretely, two different logged-in users behind the same IP shared one rate-limit bucket, and the existing e2e assertionshould not rate limit different userswould actually fail.Solution
I made
getTrackerread the caller's identity from the JWT in theAuthorizationheader (verified withJWT_SECRET) whenreq.useris absent, preferringmerchantIdthensub, and falling back toreq.ipfor unauthenticated routes. I chose this over reordering guards soJwtAuthGuardruns globally beforeThrottlerGuard, because makingJwtAuthGuardglobal would 401 routes that currently rely on no guard (a breaking change) and double-run auth on@Auth()-decorated routes. Deriving the key insidegetTrackerkeeps the existing throttle-first ordering (AC3 preserved — unauthenticated routes are still limited) and is a single-file, non-breaking change. On verification failure the key falls back toreq.iprather than throwing, since rate-limit keying must never itself reject a request.Changes
src/throttler/throttler.module.ts: replaced the two inlinegetTrackerclosures (test + production config branches) with one shared asyncgetTracker. It verifies the bearer token withJwtServiceand returnsmerchant:<merchantId>/user:<sub>/req.ip. This is the only logic change for Register ThrottlerGuard so the configured rate limits actually apply #528.src/health/health.controller.spec.ts,src/invoices/invoices.service.ts,src/soroban/soroban.service.ts,src/soroban/soroban.service.spec.ts: pre-existing prettier-only formatting fixes (quote style + import collapsing), no logic changed. The repo has no.prettierrc, so prettier defaults to double quotes while these files used single quotes; this made the repo-widenpm run lintCI step fail. They are required for CI to go green and are deliberately separated into their own commit; they are not part of the rate-limiting fix.Testing
npm run lint— green (the 4 formatting files above were the pre-existing failures).npm run build— green (nest buildsucceeds).npm test— green: 43 suites / 479 tests passed, includingsrc/throttler/throttler-storage-redis.service.spec.ts(14/14).tsc --noEmitpasses.getTracker: (1) noAuthorizationheader →req.ip; (2) malformed/expired token → caught, falls back toreq.ip; (3) token withmerchantId→merchant:bucket; (4) token with onlysub→user:bucket; (5)req.useralready present (future-proof) → keyed bymerchantId/id.test/rate-limiting.e2e-spec.ts(asserts 429 for/auth/nonce,/auth/verify,POST /invoices,/invoices/import, plus per-user/per-IP isolation) was not executed in this environment because it boots the fullAppModule(needs a database). With this change the per-user isolation case is now correct (distinct buckets) instead of silently collapsing to one IP bucket; it should be run in CI.Notes for the maintainer
ThrottlerGuardasAPP_GUARD,getTrackerpresence, Redis block persistence,trust proxy) was already merged ine61d40b. This PR is the remaining piece that makes AC4 actually hold at runtime. If the merge target already containse61d40b, this is a follow-up completing the issue; if the baseline is pre-e61d40b, this diff + that commit together fully close Register ThrottlerGuard so the configured rate limits actually apply #528.@nestjs/throttlerv6 expects milliseconds; the controller@Throttlevalues (900_000,60_000,3_600_000) are already in ms and the module multipliesthrottlerConfig.ttlby 1000. No change needed.getTracker, the alternative is makingJwtAuthGuardpopulatereq.userbefore the throttle — but that requires making auth global, which changes public-route behavior. Happy to switch if you prefer that direction.blockDurationon routes: the storage correctly implements block persistence, but the@Throttledecorators don't setblockDuration, so the block-key path isn't exercised by routes today (limits still return 429 until TTL resets). Consistent with the issue scope; left as-is.Scope confirmation
The #528 logic change touches only
src/throttler/throttler.module.ts(one of the seven files in the issue's technical scope). The four additional files are pre-existing prettier formatting fixes required solely to make the repo-widenpm run lintCI step pass; they contain no logic changes and are isolated in a separate commit.