fix: Overrides when using hashtag (#84)

When we introduced elevated rate limits, we also implemented the use of Redis Hashtags to ensure that all keys involved in the elevated rate limits operation are allocated in the same Redis hash slot. This hashtagging process involves adding curly braces around the key.

However, this introduced an issue: overrides typically target keys without the hashtag. As a result, if we use a key with hashtags, the overrides will not be applied.

This PR addresses the issue by sanitizing the received key by removing the hashtag before looking for overrides. If any overrides are found, they will be applied accordingly.

Example:

Given the following configuration

buckets: {
  ip: {
    size: 10,
    per_second: 5,
    overrides: {
      '127.0.0.1': {
        per_second: 100
      }
    },
  }
}
It will apply the overrides for:

take('ip', '127.0.0.1')
take('ip', '{127.0.0.1}')
It will not apply the overrides for any other combination.

README.md

@@ -245,6 +245,8 @@ Examples:
 | Curly brace within hashtag | `limitd.takeElevated('bucketName', '{{some-key}')`            | `{some-key`               | `bucketName:{{some-key}`            | `ERLActiveKey:{{some-key}`               | `ERLQuotaKey:{{some-key}`               |
 | Empty hashtag              | `limitd.takeElevated('bucketName', '{}{some-key}')`           | `bucketName:{}{some-key}` | `bucketName:{}{some-key}`           | `ERLActiveKey:{bucketName:{}{some-key}}` | `ERLQuotaKey:{bucketName:{}{some-key}}` |
 
+To address the issue where overrides typically target keys without the hashtag, we sanitize the received key by removing the hashtag before looking for overrides. 
+If any overrides are found, they will be applied accordingly.
 
 ## Breaking changes from `Limitdb`
 

lib/db.js

@@ -7,7 +7,7 @@ const utils = require('./utils');
 const Redis = require('ioredis');
 const { validateParams, validateERLParams } = require('./validation');
 const DBPing = require("./db_ping");
-const { calculateQuotaExpiration, resolveElevatedParams, isFixedWindowEnabled } = require('./utils');
+const { calculateQuotaExpiration, resolveElevatedParams, isFixedWindowEnabled, removeHashtag } = require('./utils');
 const EventEmitter = require('events').EventEmitter;
 
 const TAKE_LUA = fs.readFileSync(`${__dirname}/take.lua`, "utf8");
@@ -161,21 +161,23 @@ class LimitDBRedis extends EventEmitter {
       return utils.normalizeTemporals(params.configOverride);
     }
 
-    const fromOverride = type.overrides[params.key];
+    const key = removeHashtag(params.key);
+
+    const fromOverride = type.overrides[key];
     if (fromOverride) {
       return fromOverride;
     }
 
-    const fromCache = type.overridesCache && type.overridesCache.get(params.key);
+    const fromCache = type.overridesCache && type.overridesCache.get(key);
     if (fromCache) {
       return fromCache;
     }
 
     const fromMatch = _.find(type.overridesMatch, (o) => {
-      return o.match.exec(params.key);
+      return o.match.exec(key);
     });
     if (fromMatch) {
-      type.overridesCache.set(params.key, fromMatch);
+      type.overridesCache.set(key, fromMatch);
       return fromMatch;
     }
 

lib/utils.js

@@ -220,6 +220,13 @@ function replicateHashtag(baseKey, prefix, key) {
   }
 }
 
+function removeHashtag(key) {
+  if (key.startsWith('{') && key.endsWith('}')) {
+    return key.slice(1, -1);
+  }
+  return key;
+}
+
 /** isFixedWindowEnabled
  * | fixed_window bucket config | fixed_window param | Fixed Window Enabled |
  * |----------------------------|--------------------|----------------------|
@@ -252,4 +259,5 @@ module.exports = {
   resolveElevatedParams,
   replicateHashtag,
   isFixedWindowEnabled,
+  removeHashtag,
 };

test/db.tests.js

@@ -1668,6 +1668,8 @@ module.exports.tests = (clientCreator) => {
         });
 
         describe('overrides', () => {
+          const hashtaggedERLIsActiveKey = replicateHashtag(`${bucketName}:${key}`, prefix, erl_is_active_key)
+
           it('should use elevated_limits config override when provided', (done) => {
             const bucketName = 'bucket_with_no_elevated_limits_config';
             const erl_is_active_key = 'some_erl_active_identifier';
@@ -1720,6 +1722,114 @@ module.exports.tests = (clientCreator) => {
                 });
               });
           });
+
+          describe('when base override is greater than erl', () => {
+            const bucketConfig = {
+              size: 10,
+              per_minute: 10,
+              elevated_limits: {
+                size: 20,
+                per_minute: 20,
+              },
+              overrides: {
+                [[key]]: {
+                  size: 700,
+                  per_minute: 700,
+                },
+              },
+            };
+
+            it('should not trigger ERL when bucket is exhausted', (done) => {
+              db.configurateBucket(bucketName, bucketConfig);
+
+              const params = {
+                type: bucketName,
+                key: key,
+                count: 700,
+                elevated_limits: {
+                  erl_is_active_key: erl_is_active_key,
+                  erl_quota_key: erl_quota_key,
+                  erl_activation_period_seconds: 900,
+                  quota_per_calendar_month: 10
+                },
+              };
+
+              takeElevatedPromise(params)
+                .then((result) => {
+                  assert.isTrue(result.conformant);
+                  assert.isFalse(result.elevated_limits.triggered);
+                  assert.isFalse(result.elevated_limits.activated);
+                  assert.isTrue(result.elevated_limits.erl_configured_for_bucket)
+                  assert.equal(result.remaining, 0);
+                })
+                .then(() => takeElevatedPromise({ ...params, count: 1 }))
+                .then((result) => {
+                  assert.isFalse(result.conformant);
+                  assert.isFalse(result.elevated_limits.triggered);
+                  assert.isFalse(result.elevated_limits.activated);
+                  assert.isTrue(result.elevated_limits.erl_configured_for_bucket)
+                  assert.equal(result.remaining, 0);
+                })
+                .then(() => redisExistsPromise(hashtaggedERLIsActiveKey))
+                .then((erlIsActiveExists) => assert.equal(erlIsActiveExists, 0))
+                .then(done);
+            });
+          });
+
+          describe('when elevated limits is greater than the override', () => {
+            const bucketConfig = {
+              size: 10,
+              per_minute: 10,
+              elevated_limits: {
+                size: 20,
+                per_minute: 20,
+              },
+              overrides: {
+                [[key]]: {
+                  size: 15,
+                  per_minute: 15,
+                },
+              },
+            };
+
+            it('should trigger ERL when bucket is exhausted', (done) => {
+              db.configurateBucket(bucketName, bucketConfig);
+
+              const params = {
+                type: bucketName,
+                key: key,
+                count: 15,
+                elevated_limits: {
+                  erl_is_active_key: erl_is_active_key,
+                  erl_quota_key: erl_quota_key,
+                  erl_activation_period_seconds: 900,
+                  quota_per_calendar_month: 10
+                },
+              };
+
+              takeElevatedPromise(params)
+                .then((result) => {
+                  assert.isTrue(result.conformant);
+                  assert.isFalse(result.elevated_limits.triggered);
+                  assert.isFalse(result.elevated_limits.activated);
+                  assert.isTrue(result.elevated_limits.erl_configured_for_bucket)
+                  assert.equal(result.remaining, 0);
+                })
+                .then(() => takeElevatedPromise({ ...params, count: 1 }))
+                .then((result) => {
+                  assert.isTrue(result.conformant);
+                  assert.isTrue(result.elevated_limits.triggered);
+                  assert.isTrue(result.elevated_limits.activated);
+                  assert.isTrue(result.elevated_limits.erl_configured_for_bucket)
+                  assert.equal(result.remaining, 4);
+                })
+                .then(() => db.redis.scan(0, ))
+                .then(() => redisExistsPromise(hashtaggedERLIsActiveKey))
+                .then((erlIsActiveExists) => assert.equal(erlIsActiveExists, 1))
+                .then(done);
+            });
+          });
+
           describe('should use config override when elevated_limits is not provided and erl is active for the given key', () => {
             const tests = [
               {
@@ -2149,6 +2259,45 @@ module.exports.tests = (clientCreator) => {
           });
         });
       });
+
+      describe('when using overrides', () => {
+        const testBuckets = {
+          ip: {
+            size: 10,
+            per_second: 5,
+            overrides: {
+              '127.0.0.1': {
+                per_second: 100
+              }
+            },
+          }
+        }
+
+        const testCases = [
+          { description: 'when the key contains curly braces', key: '{127.0.0.1}', expectedLimit: 100 },
+          { description: 'when the key does not contain curly braces', key: '127.0.0.1', expectedLimit: 100 },
+        ];
+        testCases.forEach(({ description, key, expectedLimit }) => {
+          describe(description, () => {
+            it('should apply the override', (done) => {
+              const takeParams = { type: 'ip', key };
+
+              db.configurateBuckets(testBuckets);
+
+              db.take(takeParams, (err, result) => {
+                if (err) {
+                  return done(err);
+                }
+
+                assert.ok(result.conformant);
+                assert.equal(result.remaining, expectedLimit - 1);
+                assert.equal(result.limit, expectedLimit);
+                done();
+              });
+            });
+          });
+        });
+      });
     });
 
     describe('PUT', () => {
@@ -2636,4 +2785,4 @@ module.exports.tests = (clientCreator) => {
       });
     });
   });
-}
+}

test/utils.tests.js

@@ -5,7 +5,9 @@ const chaiExclude = require('chai-exclude');
 chai.use(chaiExclude);
 const assert = chai.assert;
 
-const { getERLParams, calculateQuotaExpiration, normalizeType, resolveElevatedParams, replicateHashtag, isFixedWindowEnabled } = require('../lib/utils');
+const { getERLParams, calculateQuotaExpiration, normalizeType, resolveElevatedParams, replicateHashtag, isFixedWindowEnabled,
+  removeHashtag
+} = require('../lib/utils');
 const { set, reset } = require('mockdate');
 const { expect } = require('chai');
 
@@ -431,4 +433,36 @@ describe('utils', () => {
       assert.isFalse(result);
     });
   });
+
+  describe('removeHashtag', () => {
+    it('should remove curly braces from a key with both curly braces', () => {
+      const key = '{127.0.0.1}';
+      const result = removeHashtag(key);
+      expect(result).to.equal('127.0.0.1');
+    });
+
+    it('should return the key unchanged if it does not have curly braces', () => {
+      const key = '127.0.0.1';
+      const result = removeHashtag(key);
+      expect(result).to.equal(key);
+    });
+
+    it('should return the key unchanged if it only has the left curly brace', () => {
+      const key = '{127.0.0.1';
+      const result = removeHashtag(key);
+      expect(result).to.equal(key);
+    });
+
+    it('should return the key unchanged if it only has the right curly brace', () => {
+      const key = '127.0.0.1}';
+      const result = removeHashtag(key);
+      expect(result).to.equal(key);
+    });
+
+    it('should remove the outermost curly braces if the key has nested curly braces', () => {
+      const key = '{{127.0.0.1}}';
+      const result = removeHashtag(key);
+      expect(result).to.equal('{127.0.0.1}');
+    });
+  });
 });