Skip to content

feat(api): add API rate limiting and abuse protection - #4

Merged
vedant21-ctr merged 3 commits into
vedant21-ctr:mainfrom
hrishu802:feature/api-rate-limiting
Sep 29, 2026
Merged

vedant21-ctr merged 3 commits into
vedant21-ctr:mainfrom
hrishu802:feature/api-rate-limiting

Conversation

@hrishu802

Copy link
Copy Markdown
Contributor

Summary

  • Add API rate limiting and abuse protection
  • Add Redis-backed distributed rate limiting with in-memory fallback
  • Add per-route rate-limit policies
  • Support authenticated-user and IP-based rate-limit keys
  • Add standard rate-limit response headers and Retry-After
  • Add environment configuration for rate limiting
  • Add comprehensive rate-limit tests

Validation

  • 118/118 tests passing
  • TypeScript: 0 errors
  • ESLint: 0 errors
  • Prettier: clean

@vedant21-ctr

vedant21-ctr commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

@hrishu802 Reviewed the complete PR and the current implementation. The rate-limiting implementation is well structured, with Redis support, in-memory fallback, per-route policies, headers, and good test coverage.

A few things need to be addressed before merge:

CI issue — please fix the CI bug: The current GitHub Actions run is failing during the dependency-installation step, before type-checking, linting, build, and tests can even run. This appears to be an issue with the CI workflow/configuration, so please investigate and correct the CI bug first, then verify that the complete pipeline runs successfully.
Route policy matching: resolveRoutePolicy() currently relies on startsWith() for route groups. This can unintentionally match routes such as /api/v1/eventsSomething as an events route. Please make the matching boundary-aware, e.g. exact match or path.startsWith(prefix + '/').
Authenticated-user keying: generateClientKey() uses request.user.id whenever request.user exists. Please verify that authentication is guaranteed to run before the rate-limit hook for all routes using user-based limits, so an unverified request.user cannot be trusted for rate-limit identity.
Tests: The test coverage is comprehensive, but after fixing the CI configuration, please ensure the tests actually execute in CI rather than relying only on the local 118/118 result mentioned in the PR description.

@hrishu802

Copy link
Copy Markdown
Contributor Author

The CI lockfile issue has been fixed in dc52859. The new workflow run is currently awaiting maintainer approval before it can execute.

@vedant21-ctr vedant21-ctr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

resolveRoutePolicy() still uses broad startsWith() matching, so paths like /api/v1/eventsSomething can get the events policy. Please make the prefix matching boundary-aware.
method is passed into resolveRoutePolicy() but isn’t actually used, so the current implementation/comment is slightly misleading.
maxMemoryKeys is exposed in RateLimitPluginOptions but the store is always created with the default 10000, so that option currently has no effect.
Please add a couple of negative/boundary tests for route matching.
Once the workflow gets maintainer approval, please confirm the full CI pipeline passes rather than relying only on the local 118/118 result.

The authenticated-user keying looks fine with the current preValidation → preHandler hook order.

@hrishu802
hrishu802 force-pushed the feature/api-rate-limiting branch from dc52859 to a86478d Compare September 27, 2026 12:16
@hrishu802

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've addressed the requested changes:

  1. Made route-policy matching boundary-aware to prevent false prefix matches.
  2. Wired maxMemoryKeys into the bounded memory store and added eviction tests.
  3. Added positive and negative route-matching tests.
  4. Rebased the branch onto the latest main and resolved the merge conflicts.
  5. Verified TypeScript compilation and the API test suite locally — 86 tests passed, including 36 rate-limit tests.

The PR now shows no conflicts with the base branch. The GitHub Actions workflow is currently awaiting maintainer approval, so I'll wait for the CI run to complete.

@vedant21-ctr
vedant21-ctr merged commit 21c5a3d into vedant21-ctr:main Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants