rhinobase / hono-rate-limiter

Rate Limit middleware for Hono Web Server
https://hono-rate-limiter.vercel.app
MIT License
170 stars 8 forks source link

Bug: Error with hono-rate-limiter and Cloudflare KV #23

Open Puliczek opened 1 week ago

Puliczek commented 1 week ago

Description:

I am trying to use hono-rate-limiter with Cloudflare KV, but I consistently encounter the following error:

Error: KV PUT failed: 400 Invalid expiration of 60. Please specify an integer greater than the current number of seconds since the UNIX epoch.

Tested locally and on cloudflare, same error.

Steps to Reproduce:

Follow the code example provided in the hono-rate-limiter Cloudflare Using WorkersKVStore documentation.

Context:

I need to use KV storage because I want to implement a rate limit of 30 requests per day. cloudflareRateLimiter only gives 10 or 60 seconds period of time.

MathurAditya724 commented 6 days ago
MathurAditya724 commented 6 days ago

For some reason the expiration is not working correctly, here is the code -

this.windowMs = 60_000 // 1 minute
async increment(key: string): Promise<ClientRateLimitInfo> {
  const keyWithPrefix = this.prefixKey(key);
  // @ts-ignore
  let payload: Required<ClientRateLimitInfo> = await this.namespace.get<Required<ClientRateLimitInfo>>(
    keyWithPrefix,
    "json",
  );

  const wasCreated = payload != null

  if (wasCreated) {
    payload = {
      totalHits: payload.totalHits + 1,
      resetTime: new Date(payload.resetTime),
    };
  }
  else {
    payload = {
      totalHits: 1,
      resetTime: new Date(Date.now() + this.windowMs),
    };
  }

  console.log("Payload - ", payload, " Expire - ", payload.resetTime.toLocaleString(), " Current - ", new Date().toLocaleString())

  await this.namespace.put(keyWithPrefix, JSON.stringify(payload), {
    expiration: !wasCreated ? Math.floor(payload.resetTime.getTime() / 1000) : undefined,
  });

  return payload;
}

So here are the problems I am facing -

  1. The key is not getting deleted, I am still getting the old value when I fetch it
  2. If I want to update the value for a key but still want to have the same expiration time, what will be the implementation for that? Should I just pass the expiration props again?

image

If you know what's wrong here, do let me know that will be a great help!

Puliczek commented 4 days ago

Yes, I recognized an issue with the expiration. Unfortunately, I haven't had the time to thoroughly investigate and fix it myself. I used assistance from ChatGPT, and after testing, it worked as expected.

import type { ClientRateLimitInfo, ConfigType as RateLimitConfiguration, Store } from "hono-rate-limiter";

export type Options<KVNamespace> = {
  /**
   * The KV namespace to use.
   */
  namespace: KVNamespace;

  /**
   * The text to prepend to the key in Redis.
   */
  readonly prefix?: string;
};

export class WorkersKVStoreMy<KVNamespace> implements Store {
  /**
   * The text to prepend to the key in Redis.
   */
  prefix: string;

  /**
   * The KV namespace to use.
   */
  namespace: KVNamespace;

  /**
   * The number of milliseconds to remember that user's requests.
   */
  windowMs!: number;

  /**
   * @constructor for `RedisStore`.
   *
   * @param options {Options} - The configuration options for the store.
   */
  constructor(options: Options<KVNamespace>) {
    this.namespace = options.namespace;
    this.prefix = options.prefix ?? "hrl:";
  }

  /**
   * Method to prefix the keys with the given text.
   *
   * @param key {string} - The key.
   *
   * @returns {string} - The text + the key.
   */
  prefixKey(key: string): string {
    return `${this.prefix}${key}`;
  }

  /**
   * Method that actually initializes the store.
   *
   * @param options {RateLimitConfiguration} - The options used to setup the middleware.
   */
  init(options: RateLimitConfiguration) {
    this.windowMs = options.windowMs;
  }

  /**
   * Method to fetch a client's hit count and reset time.
   *
   * @param key {string} - The identifier for a client.
   *
   * @returns {ClientRateLimitInfo | undefined} - The number of hits and reset time for that client.
   */
  async get(key: string): Promise<ClientRateLimitInfo | undefined> {
    // @ts-ignore
    const result = await this.namespace.get<ClientRateLimitInfo>(this.prefixKey(key), "json");

    if (result) {
      // Ensure resetTime is a Date object
      result.resetTime = new Date(result.resetTime);
      return result;
    }

    return undefined;
  }

  /**
   * Method to increment a client's hit counter.
   *
   * @param key {string} - The identifier for a client
   *
   * @returns {ClientRateLimitInfo} - The number of hits and reset time for that client
   */
  async increment(key: string): Promise<ClientRateLimitInfo> {
    const keyWithPrefix = this.prefixKey(key);
    // @ts-ignore
    let payload = await this.namespace.get<Required<ClientRateLimitInfo>>(keyWithPrefix, "json");

    if (payload) {
      payload.totalHits += 1;
      // Ensure resetTime is a Date object
      payload.resetTime = new Date(payload.resetTime);
    } else {
      payload = {
        totalHits: 1,
        resetTime: new Date(Date.now() + this.windowMs),
      };
    }

    // Ensure expiration is at least 60 seconds in the future
    const expiration = Math.floor(payload.resetTime.getTime() / 1000);
    const minExpiration = Math.floor(Date.now() / 1000) + 60;

    // @ts-ignore
    await this.namespace.put(keyWithPrefix, JSON.stringify(payload), {
      expiration: Math.max(expiration, minExpiration),
    });

    return payload;
  }

  /**
   * Method to decrement a client's hit counter.
   *
   * @param key {string} - The identifier for a client
   */
  async decrement(key: string): Promise<void> {
    const keyWithPrefix = this.prefixKey(key);

    // @ts-ignore
    const payload = await this.namespace.get<Required<ClientRateLimitInfo>>(keyWithPrefix, "json");

    if (!payload) return;

    payload.totalHits -= 1;
    // Ensure resetTime is a Date object
    payload.resetTime = new Date(payload.resetTime);

    // Ensure expiration is at least 60 seconds in the future
    const expiration = Math.floor(payload.resetTime.getTime() / 1000);
    const minExpiration = Math.floor(Date.now() / 1000) + 60;

    // @ts-ignore
    await this.namespace.put(keyWithPrefix, JSON.stringify(payload), {
      expiration: Math.max(expiration, minExpiration),
    });
  }

  /**
   * Method to reset a client's hit counter.
   *
   * @param key {string} - The identifier for a client
   */
  async resetKey(key: string): Promise<void> {
    // @ts-ignore
    await this.namespace.delete(this.prefixKey(key));
  }
}
MathurAditya724 commented 4 days ago

I did some digging and found that in some cases WorkerKV doesn't work as expected, especially if the window is small. The expiration or expirationTtl has to be greater than 60 sec, or it will not expire. So, If you update the value and the expiration is less than 60 seconds, it's going to stay there and not expire, and all the requests are going to be discarded.

All and all I am thinking of shifting from WorkerKV to DurableObjects, as that will be much better. Will release a new version of this lib by the end of this week maybe early if possible.

MathurAditya724 commented 4 days ago

I will release a patch version, so that at least if someone wanna use WorkerKV it will work. And in the next minor release, I will add support for DurableObjects too. So at least there will be no breaking changes

Puliczek commented 4 days ago

Yes, Durable Objects are more suitable for that purpose :) However, they require a $5/month subscription and are a bit slower. Looking forward to your update, thank you very much!

MathurAditya724 commented 4 days ago

I deployed a new version of @hono-rate-limiter/cloudflare@0.1.2, and it seems to be working on my end. Can you also check and let me know, if it's working for you?

I am now working on adding support for DurableObject, let see how fast can I add support for it