Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Provisioning API rate limiting #458

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

justinbot
Copy link
Contributor

Improves the Provisioning API rate limiting implementation:

  • Updates express-rate-limit
  • Corrects the parameter type to Partial rate limit options, so that bridges can pass overrides if necessary
  • Changes the default from 100req/6 minutes to 30req/1 minute
    • I think this is more balanced for a default rate limit, allowing a lower burst while having a shorter period before the rate limit resets. In any case, bridges can override this if necessary.

And finally, fixes the rate limit response.
Previously, response headers looked like this:

X-RateLimit-Limit: 20
X-RateLimit-Remaining: 18
X-RateLimit-Reset: 1675712280

This is the legacy format, and the reset number is obviously not correct.
Now the response headers look like this:

RateLimit-Limit: 20
RateLimit-Remaining: 7
RateLimit-Reset: 21

Using the new format and correct reset value.
And if a request has been rate limited, includes a Retry-After:

RateLimit-Limit: 20
RateLimit-Remaining: 0
RateLimit-Reset: 18
Retry-After: 60

@justinbot justinbot self-assigned this Feb 6, 2023
@justinbot justinbot requested a review from a team as a code owner February 6, 2023 19:51
@@ -146,13 +146,13 @@ export class ProvisioningApi {

const limiter = this.opts.ratelimit && ratelimiter({
handler: (req, _res, next) => {
const info = (req as AugmentedRequest).ratelimit as RateLimitInfo;
const retryAfterMs = info?.resetTime ? info.resetTime.getTime() - Date.now() : null;
next(new ApiError("Too many requests", ErrCode.Ratelimited, 429, { retry_after_ms: retryAfterMs }));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a departure from what we do in the Matrix spec, although I am happy to go along with what the HTTP standard is. We aren't beholden to Matrix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed so it has both :)

@justinbot justinbot merged commit 7fbbb21 into develop Feb 7, 2023
@justinbot justinbot deleted the justnbot/provisioning-rate-limiting branch February 7, 2023 15:11
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