From 53e3e23c13a42f50c9eb9f2eb4e0b66c4a7a2e39 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 17 Sep 2026 07:03:09 +0700 Subject: [PATCH] fix(api): stop DISABLE_SQLITE_AUTO_BACKUP from turning off Redis rate limiting (#13329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A backup flag was being used as a proxy for test mode, so `DISABLE_SQLITE_AUTO_BACKUP` also disabled Redis rate limiting — two unrelated concerns riding one variable. Probe on your head: 3/3 + 13/13 pass across the new test and the existing rate-limiter suite. Thanks, @datrixlab — catching that the existing rate-limiter tests still pass is what shows this untangled the two without changing the intended behavior of either. **Batch validation** — boarded with the other 10 PRs of your batch into one worktree cut from `release/v3.8.51`; every PR verified as an ancestor of the combined HEAD before validating. - Focused tests across all 11 PRs: **104/104 pass** on the combined tree. - Gates on the combined tree: `check-changelog-integrity` PASS, `check-complexity` PASS, `check-cognitive-complexity` PASS, `typecheck:core` PASS, `check:open-sse-typecheck` PASS. - `check-file-size` is red, but reproduces with byte-identical line counts on the pure `release/v3.8.51` tip (`open-sse/handlers/imageGeneration.ts` 3304, `open-sse/services/combo/roundRobinCombo.ts` 1221, `open-sse/utils/stream.ts` 3115). Inherited base-red, nothing added by this batch — it is also why this PR's "Fast Quality Gates" check was red. --- .../fixes/13329-rate-limiter-backup-flag.md | 1 + src/lib/db/apiKeys.ts | 6 +- src/shared/utils/rateLimiter.ts | 7 +- ...-limiter-backup-flag-not-test-mode.test.ts | 71 +++++++++++++++++++ 4 files changed, 76 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixes/13329-rate-limiter-backup-flag.md create mode 100644 tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts diff --git a/changelog.d/fixes/13329-rate-limiter-backup-flag.md b/changelog.d/fixes/13329-rate-limiter-backup-flag.md new file mode 100644 index 0000000000..fa3330d1e1 --- /dev/null +++ b/changelog.d/fixes/13329-rate-limiter-backup-flag.md @@ -0,0 +1 @@ +- **fix(api):** Setting `DISABLE_SQLITE_AUTO_BACKUP=true` no longer makes the API-key rate limiter and auth cache skip Redis, which let every replica enforce the full per-key limit on its own ([#13329](https://github.com/diegosouzapw/OmniRoute/pull/13329)) diff --git a/src/lib/db/apiKeys.ts b/src/lib/db/apiKeys.ts index 9ffa78159d..7668945814 100644 --- a/src/lib/db/apiKeys.ts +++ b/src/lib/db/apiKeys.ts @@ -272,11 +272,7 @@ function isConfiguredEnvApiKey(key: string): boolean { } function isRedisAuthCacheEnabled(): boolean { - return ( - process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE !== "1" && - process.env.NODE_ENV !== "test" && - process.env.DISABLE_SQLITE_AUTO_BACKUP !== "true" - ); + return process.env.OMNIROUTE_DISABLE_REDIS_AUTH_CACHE !== "1" && process.env.NODE_ENV !== "test"; } async function deleteRedisAuthCacheEntry(keyHash: unknown): Promise { diff --git a/src/shared/utils/rateLimiter.ts b/src/shared/utils/rateLimiter.ts index 7b36a21736..b1121f63c4 100644 --- a/src/shared/utils/rateLimiter.ts +++ b/src/shared/utils/rateLimiter.ts @@ -230,10 +230,9 @@ export async function checkRateLimit( if (!rules || rules.length === 0) return { allowed: true }; // ── In-memory mock for unit tests ── - const isTestMode = - explicitTestMode || - process.env.NODE_ENV === "test" || - process.env.DISABLE_SQLITE_AUTO_BACKUP === "true"; + // Not DISABLE_SQLITE_AUTO_BACKUP: that is a production setting (backups managed + // externally), and treating it as test mode skipped Redis on every replica. + const isTestMode = explicitTestMode || process.env.NODE_ENV === "test"; if (isTestMode) { return checkInMemoryRateLimit(TEST_MEMORY_STORE, keyId, rules); diff --git a/tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts b/tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts new file mode 100644 index 0000000000..db49f730e7 --- /dev/null +++ b/tests/unit/rate-limiter-backup-flag-not-test-mode.test.ts @@ -0,0 +1,71 @@ +/** + * DISABLE_SQLITE_AUTO_BACKUP is a production setting (.env.example: "Set true only when + * those backups are managed externally"), but checkRateLimit() and the Redis auth cache + * also read it as "running under tests". A deployment with REDIS_URL and externally + * managed backups therefore rate-limited every key in process memory, so each replica + * enforced the full per-key limit on its own. + * + * The REDIS_URL below points at a closed port. Taking the Redis path shows up as the + * limiter's fail-open { allowed: true }; the in-memory test store would reject the + * second request against a limit of 1. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import { pathToFileURL } from "node:url"; + +const ENV_KEYS = ["REDIS_URL", "NODE_ENV", "DISABLE_SQLITE_AUTO_BACKUP"] as const; + +async function twoRequestsAgainstLimitOne(env: Partial>) { + const saved = Object.fromEntries(ENV_KEYS.map((key) => [key, process.env[key]])); + for (const key of ENV_KEYS) { + if (env[key] === undefined) delete process.env[key]; + else process.env[key] = env[key]; + } + const originalConsoleError = console.error; + console.error = () => {}; + const modulePath = path.join(process.cwd(), "src/shared/utils/rateLimiter.ts"); + const rateLimiter = await import(`${pathToFileURL(modulePath).href}?case=${Math.random()}`); + try { + const rules = [{ limit: 1, window: 60 }]; + return [ + await rateLimiter.checkRateLimit("key-1", rules), + await rateLimiter.checkRateLimit("key-1", rules), + ]; + } finally { + if (rateLimiter.isRedisConfigured()) { + (await rateLimiter.getRedisClient()).disconnect(); + } + console.error = originalConsoleError; + for (const key of ENV_KEYS) { + if (saved[key] === undefined) delete process.env[key]; + else process.env[key] = saved[key]; + } + } +} + +test("DISABLE_SQLITE_AUTO_BACKUP=true does not move a Redis deployment to the in-memory store", async () => { + const results = await twoRequestsAgainstLimitOne({ + REDIS_URL: "redis://127.0.0.1:1", + NODE_ENV: "production", + DISABLE_SQLITE_AUTO_BACKUP: "true", + }); + assert.deepEqual(results, [{ allowed: true }, { allowed: true }]); +}); + +test("NODE_ENV=test still keeps the limiter in memory even with REDIS_URL set", async () => { + const results = await twoRequestsAgainstLimitOne({ + REDIS_URL: "redis://127.0.0.1:1", + NODE_ENV: "test", + }); + assert.deepEqual(results, [{ allowed: true }, { allowed: false, failedWindow: 60 }]); +}); + +test("the Redis auth cache is not switched off by DISABLE_SQLITE_AUTO_BACKUP", () => { + const source = fs.readFileSync(path.join(process.cwd(), "src/lib/db/apiKeys.ts"), "utf8"); + const gate = source.match(/function isRedisAuthCacheEnabled\(\)[^{]*\{([\s\S]*?)\n\}/); + assert.ok(gate, "isRedisAuthCacheEnabled() not found"); + assert.doesNotMatch(gate[1], /DISABLE_SQLITE_AUTO_BACKUP/); + assert.match(gate[1], /NODE_ENV !== "test"/); +});