-
-
Notifications
You must be signed in to change notification settings - Fork 78
feat: add increment option to createRateLimit #449
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,7 +128,7 @@ async function fastifyRateLimit (fastify, settings) { | |
| if (!fastify.hasDecorator('createRateLimit')) { | ||
| fastify.decorate('createRateLimit', (options) => { | ||
| const args = createLimiterArgs(pluginComponent, globalParams, options) | ||
| return (req) => applyRateLimit.apply(this, args.concat(req)) | ||
| return (req, callOptions) => applyRateLimit.apply(this, args.concat(req, callOptions)) | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -210,7 +210,7 @@ function addRouteRateHook (pluginComponent, params, routeOptions) { | |
| } | ||
| } | ||
|
|
||
| async function applyRateLimit (pluginComponent, params, req) { | ||
| async function applyRateLimit (pluginComponent, params, req, callOptions) { | ||
| const { store } = pluginComponent | ||
|
|
||
| // Retrieve the key from the generator (the global one or the one defined in the endpoint) | ||
|
|
@@ -244,10 +244,18 @@ async function applyRateLimit (pluginComponent, params, req) { | |
| let ttl = 0 | ||
| let ttlInSeconds = 0 | ||
|
|
||
| // We increment the rate limit for the current request | ||
| const storeMethod = callOptions?.increment === false ? 'read' : 'incr' | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, added a fast-fail with a clear error message when |
||
|
|
||
| // `{ increment: false }` requires the store to implement a non-mutating | ||
| // `read` method. Custom stores may not, so fail fast with a clear message | ||
| // instead of a cryptic "store[storeMethod] is not a function" TypeError. | ||
| if (storeMethod === 'read' && typeof store.read !== 'function') { | ||
| throw new Error('The configured rate-limit store does not implement a `read` method, which is required to use `{ increment: false }`') | ||
| } | ||
|
|
||
| try { | ||
| const res = await new Promise((resolve, reject) => { | ||
| store.incr(key, (err, res) => { | ||
| store[storeMethod](key, (err, res) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The signature is now uniform with |
||
| err ? reject(err) : resolve(res) | ||
| }, timeWindow, max) | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,39 @@ LocalStore.prototype.incr = function (ip, cb, timeWindow, max) { | |
| cb(null, current) | ||
| } | ||
|
|
||
| /** | ||
| * Read the current rate-limit state for `ip` without mutating it. | ||
| * | ||
| * Stores expose `read` with the same argument contract as `incr` | ||
| * (`ip, cb, timeWindow, max`) so the two are interchangeable; an | ||
| * implementation may ignore the arguments it does not need (`max` here). | ||
| * | ||
| * `read` is a non-mutating snapshot: it never increments the counter, resets | ||
| * the window, or advances the `continueExceeding`/`exponentialBackoff`/`ban` | ||
| * side effects that `incr` applies. It mirrors `incr`'s window-expiry | ||
| * detection, so a peek and a real request agree on whether the window is | ||
| * still active. | ||
| * | ||
| * @param {string} ip | ||
| * @param {(err: Error | null, res: { current: number, ttl: number }) => void} cb | ||
| * @param {number} timeWindow | ||
| * @param {number} [max] | ||
| */ | ||
| LocalStore.prototype.read = function (ip, cb, timeWindow, max) { | ||
| const nowInMs = Date.now() | ||
| const current = this.lru.get(ip) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Signature inconsistency: this takes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I aligned both stores to |
||
| if (!current || current.iterationStartMs + timeWindow <= nowInMs) { | ||
| // Item doesn't exist or has expired: report a clean state without mutating | ||
| cb(null, { current: 0, ttl: 0 }) | ||
| return | ||
| } | ||
|
|
||
| // Item is alive: report the current state without mutating | ||
| const ttl = timeWindow - (nowInMs - current.iterationStartMs) | ||
| cb(null, { current: current.current, ttl }) | ||
| } | ||
|
|
||
| LocalStore.prototype.child = function (routeOptions) { | ||
| return new LocalStore(routeOptions.continueExceeding, routeOptions.exponentialBackoff, routeOptions.cache) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,30 @@ const lua = ` | |
| return {current, timeWindow} | ||
| ` | ||
|
|
||
| const luaRead = ` | ||
| -- Key to operate on | ||
| local key = KEYS[1] | ||
|
|
||
| -- Read the counter without mutating it | ||
| local current = redis.call('GET', key) | ||
|
|
||
| -- A missing key returns false from redis.call (and nil from redis.pcall); | ||
| -- "not current" covers both, so the clean-state branch is robust either way. | ||
| if not current then | ||
| -- Key doesn't exist: clean state | ||
| return {0, 0} | ||
| end | ||
|
|
||
| -- Read the remaining TTL in milliseconds | ||
| local ttl = redis.call('PTTL', key) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This script reports whatever PTTL says, without clamping to the configured
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into this: the existing |
||
| if ttl < 0 then | ||
| -- -2 (no key) or -1 (no expiry): report no active window | ||
| ttl = 0 | ||
| end | ||
|
|
||
| return {tonumber(current), ttl} | ||
| ` | ||
|
|
||
| function RedisStore (continueExceeding, exponentialBackoff, redis, key = 'fastify-rate-limit-') { | ||
| this.continueExceeding = continueExceeding | ||
| this.exponentialBackoff = exponentialBackoff | ||
|
|
@@ -43,6 +67,13 @@ function RedisStore (continueExceeding, exponentialBackoff, redis, key = 'fastif | |
| lua | ||
| }) | ||
| } | ||
|
|
||
| if (!this.redis.rateLimitRead) { | ||
| this.redis.defineCommand('rateLimitRead', { | ||
| numberOfKeys: 1, | ||
| lua: luaRead | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| RedisStore.prototype.incr = function (ip, cb, timeWindow, max) { | ||
|
|
@@ -51,6 +82,26 @@ RedisStore.prototype.incr = function (ip, cb, timeWindow, max) { | |
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Read the current rate-limit state for `ip` without mutating it. | ||
| * | ||
| * Same argument contract as `incr` (`ip, cb, timeWindow, max`); the Redis | ||
| * implementation only needs the key, so `timeWindow`/`max` are ignored. The | ||
| * reported `ttl` is the raw server `PTTL` — the same source `incr` returns on | ||
| * its alive path — so it may exceed the configured `timeWindow` when | ||
| * `continueExceeding`/`exponentialBackoff` extended it. | ||
| * | ||
| * @param {string} ip | ||
| * @param {(err: Error | null, res: { current: number, ttl: number }) => void} cb | ||
| * @param {number} [timeWindow] | ||
| * @param {number} [max] | ||
| */ | ||
| RedisStore.prototype.read = function (ip, cb, timeWindow, max) { | ||
| this.redis.rateLimitRead(this.key + ip, (err, result) => { | ||
| err ? cb(err, null) : cb(null, { current: result[0], ttl: result[1] }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the comment on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aligned with the above: |
||
| }) | ||
| } | ||
|
|
||
| RedisStore.prototype.child = function (routeOptions) { | ||
| return new RedisStore(routeOptions.continueExceeding, routeOptions.exponentialBackoff, this.redis, `${this.key}${routeOptions.routeInfo.method}${routeOptions.routeInfo.url}-`) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the
readpath the counter is read-only, but the return block further down still computesisBanned: params.ban !== -1 && current - max > params.ban(line ~273). Sincereaddoesn't trigger the ban machinery, a banned IP peeked with{ increment: false }will reportisAllowed: falsewith the current counter — notisBanned: true. This is arguably a deliberate "peek doesn't apply policy" choice, but it's a footgun: a caller peeking to decide whether to consume may incorrectly treat a banned IP as healthy. Either reflect ban state onread(read from a separate ban store / counter) or document the limitation explicitly in the README.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this deserves to be explicit. I treated
{ increment: false }as a deliberate non-mutating snapshot: it reports current counters (soisExceeded/isBannedreflect existing state) but never triggers or advances ban/backoff. Reflecting live ban escalation would need a separate ban store the plugin doesn't keep, so I documented the snapshot semantics in the README rather than half-implementing it. Happy to change the behavior if you'd prefer. 9dc865c.