diff --git a/.changeset/auth-lazy-cache-rate-limit-store.md b/.changeset/auth-lazy-cache-rate-limit-store.md new file mode 100644 index 0000000000..9600beb4ae --- /dev/null +++ b/.changeset/auth-lazy-cache-rate-limit-store.md @@ -0,0 +1,43 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): 限流计数器改为惰性解析 kernel cache —— 修掉误报的告警，也修掉「共享限流从未生效」的功能洞 (#4772) + +`pnpm dev`（showcase）每次冷启都会打一条： + +``` +WARN [auth] no cache service registered — rate-limit counters use a per-process in-memory + store; a multi-node deployment needs a shared cache (Redis) to enforce limits globally +``` + +而 `CacheServicePlugin` 就在 **21ms 后**注册好了，它本来就在已加载插件列表里。这条告警把运维引向「你需要 Redis」，接完 Redis 还是同一条告警 —— 因为缺的不是 Redis。 + +**这不只是日志误报。** `AuthPlugin.init()` 里那次 `getServiceAsync('cache')` +探测的结论会被**冻结整个进程生命周期**：better-auth 实例是懒创建的，但它读的是 init +时定下的 config。所以标准组合下 auth 这一侧永远拿着「没有 cache」这个结论，限流计数器 +**从未**用上共享存储 —— 多节点部署的限额从来没有被全局强制过，每个节点各算各的，轮换 +节点即可绕过。ADR-0069 D2 声明的能力与运行时不一致。 + +**修法：把「取 cache 服务」放回真正用到它的那一刻。** 新增 +`createLazyCacheRateLimitStorage()`，实现 better-auth 的 `rateLimit.customStorage`： +计数器被消费时才解析 `cache` 服务（这一刻必然在 `kernel:ready` 之后，因此与插件启动 +顺序无关），解析到就一直用它。告警保留，但只在**计数器真的要用共享存储、而此刻确实 +一个 cache 服务都没有**时才打一次 —— 那时它才是真信号，「加一个共享缓存」也才是对的 +建议。真没有 cache 的部署仍然限流，只是退化成进程内计数（降级，不是关闭）。 + +**刻意走 `rateLimit.customStorage` 而不是 `secondaryStorage`。** 后者会连带把**会话 +的记录之处**搬进缓存：better-auth 的 `createSession` 不再写 `sys_session` 行， +`findSession` 直接从缓存快照作答、根本不查库；而 ADR-0069 D4 的空闲超时 / 绝对时长 +上限 / 并发上限**全部靠写那一行来撤销会话**。所以自动把 cache 绑成 `secondaryStorage` +会静默废掉 D4 的三个管控。本次因此不再从 cache 服务自动派生 `secondaryStorage`： +它回归「宿主显式提供才生效」，`cacheSecondaryStorage()` 改为从包根导出，供知情的宿主 +自行选用。会话到底该存哪，是一个需要维护者裁定的架构问题，记录在 #4785。 + +对使用者的影响： + +- 配了 cache 插件的部署不再出现那条 warn，改为一条 info（计数器已绑定到 cache 服务）； +- 多节点 + Redis cache 的部署，限流计数**现在真的**是全局的； +- 新增 `AuthManagerOptions.rateLimitStorage`（counters-only，不迁移会话）；宿主自己 + 提供的 `secondaryStorage` 行为不变，仍然优先并继续走 + `rateLimit.storage: 'secondary-storage'`。 diff --git a/docs/adr/0069-enterprise-authentication-hardening.md b/docs/adr/0069-enterprise-authentication-hardening.md index 9c87b9e511..dfd426cca5 100644 --- a/docs/adr/0069-enterprise-authentication-hardening.md +++ b/docs/adr/0069-enterprise-authentication-hardening.md @@ -139,7 +139,7 @@ Each row in D1-D6 names exactly one of these seams. No setting is introduced wit | Phase | Status | Notes | |---|---|---| | **P1** (D1/D2/D3 + D7 fields) | ✅ **implemented** | Password complexity/history/expiry (`assertPasswordComplexity`/`assertPasswordNotReused`/`stampPasswordChangedAt`), HIBP (`haveIBeenPwned` plugin), account lockout (`assertAccountNotLocked`/`recordSignInOutcome` + `unlock_user` action), enforced MFA + grace (`computeAuthGate` → `MFA_REQUIRED`, per-org `require_mfa`), rate-limit tuning (`customRules`). All settings in `auth.manifest.ts`, bound via `bindAuthSettings`. Login-audit fields `last_login_at`/`last_login_ip` stamped on sign-in (`stampLastLogin`). | -| **P2** (D4/D5) | 🟡 **mostly implemented** | Session idle/absolute/concurrent (`enforceSessionControls`/`enforceConcurrentCap`), the **global** IP allow-list (`isClientIpAllowed`, `auth.allowed_ip_ranges`), and the **shared multi-node rate-limit + session store** (better-auth `secondaryStorage` bound to the kernel cache service via `cacheSecondaryStorage`; shared iff the cache is — Redis adapter in a cluster) are landed. **Remaining:** per-org `sys_organization.allowed_ip_ranges` (+ optional `sys_user.allowed_ip_ranges` override) — tracked in #2571. | +| **P2** (D4/D5) | 🟡 **mostly implemented** | Session idle/absolute/concurrent (`enforceSessionControls`/`enforceConcurrentCap`), the **global** IP allow-list (`isClientIpAllowed`, `auth.allowed_ip_ranges`), and the **shared multi-node rate-limit counters** (better-auth `rateLimit.customStorage` fed by the kernel cache service through `createLazyCacheRateLimitStorage`; shared iff the cache is — Redis adapter in a cluster) are landed. **Correction (#4772):** this row previously claimed a shared **session** store via `secondaryStorage` as landed. It was not: the binding was taken in `AuthPlugin.init()`, which runs *before* `CacheServicePlugin` registers `cache`, so it never fired in the standard composition — and the counters it was supposed to share never reached the cache either. The counters now ride `rateLimit.customStorage`, resolved at counting time. The **session** half is deliberately NOT auto-wired: better-auth answers `findSession` from a `secondaryStorage` snapshot without reading the database, while D4 above revokes by writing the `sys_session` row, so a cache-backed session store silently disables idle-timeout / absolute-max / concurrent-cap enforcement. `cacheSecondaryStorage` remains exported for a host that opts into that trade knowingly. **Remaining:** per-org `sys_organization.allowed_ip_ranges` (+ optional `sys_user.allowed_ip_ranges` override) — tracked in #2571; the session-store question — tracked in #4785. | | **P2/P3** (D6) | 🟡 partial | Generic OIDC RP wired (`genericOAuth`/`sso`); admin OIDC **trust-list settings UI** still env/`sys_sso_provider`-only. | | **P3** (SAML, broader social) | 🟡 partial | `@better-auth/sso` present (SAML now better-auth-native — see Addendum); broader settings-driven social providers pending. | diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 5d8ae488c8..717d0d9c19 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -2782,6 +2782,82 @@ describe('AuthManager', () => { expect(captured.secondaryStorage).toBe(ss); expect(captured.rateLimit.storage).toBe('secondary-storage'); }); + + // ── #4772 — counters-only store, no session relocation ──────────────── + it('passes rateLimitStorage through as rateLimit.customStorage, without a secondaryStorage', async () => { + let captured: any; + (betterAuth as any).mockImplementation((cfg: any) => { captured = cfg; return { handler: vi.fn(), api: {} }; }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const rls = { consume: vi.fn(async () => ({ allowed: true, retryAfter: null })) }; + const m = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + rateLimitStorage: rls as any, + }); + await m.getAuthInstance(); + warn.mockRestore(); + expect(captured.rateLimit.customStorage).toBe(rls); + // The session store is untouched: ADR-0069 D4 revokes by writing the + // `sys_session` row, which better-auth stops reading once it has a + // secondaryStorage snapshot to answer from. + expect(captured).not.toHaveProperty('secondaryStorage'); + }); + + it('keeps the operator-tuned rateLimit rules alongside the custom storage', async () => { + let captured: any; + (betterAuth as any).mockImplementation((cfg: any) => { captured = cfg; return { handler: vi.fn(), api: {} }; }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const rls = { consume: vi.fn(async () => ({ allowed: true, retryAfter: null })) }; + const m = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + rateLimit: { enabled: true, window: 60, max: 10 } as any, + rateLimitStorage: rls as any, + }); + await m.getAuthInstance(); + warn.mockRestore(); + expect(captured.rateLimit).toMatchObject({ enabled: true, window: 60, max: 10 }); + expect(captured.rateLimit.customStorage).toBe(rls); + }); + + it('a host-supplied secondaryStorage wins — no customStorage is added', async () => { + let captured: any; + (betterAuth as any).mockImplementation((cfg: any) => { captured = cfg; return { handler: vi.fn(), api: {} }; }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const ss2 = { get: vi.fn(), set: vi.fn(), delete: vi.fn() }; + const rls = { consume: vi.fn(async () => ({ allowed: true, retryAfter: null })) }; + const m = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + secondaryStorage: ss2 as any, + rateLimitStorage: rls as any, + }); + await m.getAuthInstance(); + warn.mockRestore(); + expect(captured.rateLimit.storage).toBe('secondary-storage'); + expect(captured.rateLimit.customStorage).toBeUndefined(); + }); + + // A settings patch replaces `rateLimit` wholesale (bindAuthSettings builds + // a fresh object); the counter store must survive that, or tuning the + // limits in Setup would silently un-share them again. + it('survives an applyConfigPatch that replaces rateLimit wholesale', async () => { + let captured: any; + (betterAuth as any).mockImplementation((cfg: any) => { captured = cfg; return { handler: vi.fn(), api: {} }; }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const rls = { consume: vi.fn(async () => ({ allowed: true, retryAfter: null })) }; + const m = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + rateLimitStorage: rls as any, + }); + await m.getAuthInstance(); + m.applyConfigPatch({ rateLimit: { enabled: true, window: 30, max: 5 } as any }); + await m.getAuthInstance(); + warn.mockRestore(); + expect(captured.rateLimit).toMatchObject({ enabled: true, window: 30, max: 5 }); + expect(captured.rateLimit.customStorage).toBe(rls); + }); }); // ADR-0069 D1: password complexity validator (custom; better-auth only does diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 146b39f814..5ce1afb27d 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -594,11 +594,32 @@ export interface AuthManagerOptions extends Partial { * uses it for **rate-limit counters** (the manager also flips * `rateLimit.storage` to `'secondary-storage'`) and session caching, so both * are enforced against ONE store across every node — closing the multi-node - * rate-limit-bypass hole (each node otherwise counts independently). Wired by - * `AuthPlugin` from the kernel `cache` service (memory single-node, Redis in - * a cluster). Absent → better-auth keeps its per-process in-memory store. + * rate-limit-bypass hole (each node otherwise counts independently). + * + * HOST-SUPPLIED ONLY (#4772). `AuthPlugin` no longer derives this from the + * kernel `cache` service: better-auth also makes `secondaryStorage` the + * session store (`createSession` skips the `sys_session` row, `findSession` + * answers from the snapshot without reading the database), which silently + * disables the ADR-0069 D4 session controls that revoke by writing that row. + * The counters ride {@link rateLimitStorage} instead. Absent → better-auth + * keeps its own session + rate-limit defaults. */ secondaryStorage?: BetterAuthOptions['secondaryStorage']; + + /** + * ADR-0069 D2 (#4772) — better-auth `rateLimit.customStorage`: the store the + * per-IP counters are consumed from, and NOTHING else (no session + * relocation, unlike {@link secondaryStorage}). `AuthPlugin` supplies a + * lazily-cache-resolving implementation (`rate-limit-storage.ts`) so the + * counters reach the kernel `cache` service regardless of plugin start + * order. Ignored when `secondaryStorage` is set — better-auth's own + * `storage: 'secondary-storage'` already routes counters there. + * + * Kept as its own option rather than inside {@link rateLimit} so a settings + * patch that replaces `rateLimit` wholesale (`bindAuthSettings`) can never + * drop the store. + */ + rateLimitStorage?: NonNullable['customStorage']; } /** @@ -919,14 +940,24 @@ export class AuthManager { // ADR-0069 D2 — per-IP rate limiting (native). Only set when configured // so better-auth keeps its own defaults otherwise. The settings bind - // supplies stricter `customRules` for the auth endpoints. When a shared - // secondaryStorage is wired, flip the rate-limit store to it so counters - // are enforced across nodes (default 'memory' is per-process). - ...(this.config.rateLimit || this.config.secondaryStorage + // supplies stricter `customRules` for the auth endpoints. + // + // Where the counters live, in precedence order: + // 1. a host-supplied `secondaryStorage` → better-auth's own + // `storage: 'secondary-storage'` (unchanged behaviour); + // 2. otherwise `rateLimitStorage` → `customStorage`, the counters-only + // seam AuthPlugin fills with the lazily-resolved kernel cache + // (#4772). better-auth ignores `storage` when `customStorage` is set. + // Neither → better-auth's per-process 'memory' default. + ...(this.config.rateLimit || this.config.secondaryStorage || this.config.rateLimitStorage ? { rateLimit: { ...(this.config.rateLimit ?? {}), - ...(this.config.secondaryStorage ? { storage: 'secondary-storage' as const } : {}), + ...(this.config.secondaryStorage + ? { storage: 'secondary-storage' as const } + : this.config.rateLimitStorage + ? { customStorage: this.config.rateLimitStorage } + : {}), }, } : {}), diff --git a/packages/plugins/plugin-auth/src/auth-plugin.test.ts b/packages/plugins/plugin-auth/src/auth-plugin.test.ts index 8ea624ae69..2d6d60f6fa 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.test.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.test.ts @@ -1209,4 +1209,115 @@ describe('AuthPlugin', () => { expect(ql.tables.sys_member.find((m: any) => m.user_id === 'seeded_u2')).toBeUndefined(); }); }); + + // ── #4772 — the kernel `cache` service is resolved at COUNTING time ───────── + // + // AuthPlugin.init() runs before CacheServicePlugin registers `cache` (21ms + // earlier in a showcase cold start). The eager probe that used to live in + // init() therefore concluded "no cache" in deployments that had one, printed + // a warning telling the operator to provision Redis, and — the part that was + // not just noise — froze that conclusion, so the rate-limit counters never + // reached the shared store even after it came up. + describe('rate-limit counter store (ADR-0069 D2 / #4772)', () => { + const makeCache = () => { + const store = new Map(); + return { + store, + get: vi.fn(async (k: string) => (store.has(k) ? store.get(k) : undefined)), + set: vi.fn(async (k: string, v: unknown, _ttl?: number) => { store.set(k, v); }), + delete: vi.fn(async (k: string) => store.delete(k)), + has: vi.fn(async (k: string) => store.has(k)), + clear: vi.fn(async () => store.clear()), + stats: vi.fn(async () => ({ hits: 0, misses: 0, keys: store.size })), + }; + }; + + /** Boot the plugin against a context whose `cache` resolution is under test control. */ + const bootWith = async (resolveCache: () => Promise) => { + const ctx = { + ...mockContext, + getServiceAsync: vi.fn(async (name: string) => { + if (name === 'cache') return await resolveCache(); + return undefined; + }), + } as unknown as PluginContext; + const plugin = new AuthPlugin({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + }); + await plugin.init(ctx); + const manager = (ctx.registerService as any).mock.calls.find( + ([name]: [string]) => name === 'auth', + )?.[1] as AuthManager; + return { ctx, manager, storage: (manager as any).config.rateLimitStorage }; + }; + + it('does not warn during init when the cache has not registered yet', async () => { + // Exactly the showcase cold start: `cache` is not in the registry when + // plugin-auth initializes. + const { ctx } = await bootWith(async () => undefined); + const warned = (ctx.logger.warn as any).mock.calls.map((c: any[]) => String(c[0])); + expect(warned.some((m: string) => m.includes('no cache service registered'))).toBe(false); + expect(warned.some((m: string) => m.includes('rate-limit'))).toBe(false); + }); + + it('counts in the cache registered AFTER init — the counters really are shared', async () => { + const cache = makeCache(); + let registered = false; + const { ctx, storage } = await bootWith(async () => (registered ? cache : undefined)); + expect(storage).toBeDefined(); + + // CacheServicePlugin comes up 21ms later. + registered = true; + + const decision = await storage.consume('ip:203.0.113.7', { window: 60, max: 3 }); + expect(decision).toEqual({ allowed: true, retryAfter: null }); + expect(cache.store.size).toBe(1); + const info = (ctx.logger.info as any).mock.calls.map((c: any[]) => String(c[0])); + expect(info.some((m: string) => m.includes('rate-limit counters bound to the kernel cache service'))).toBe(true); + }); + + it('warns at counting time — and only then — when there is genuinely no cache service', async () => { + const { ctx, storage } = await bootWith(async () => undefined); + const rateLimitWarnings = () => + (ctx.logger.warn as any).mock.calls + .map((c: any[]) => String(c[0])) + .filter((m: string) => m.includes('rate-limit counters')); + expect(rateLimitWarnings()).toEqual([]); + + await storage.consume('ip:198.51.100.9', { window: 60, max: 3 }); + expect(rateLimitWarnings()).toHaveLength(1); + + const warned = (ctx.logger.warn as any).mock.calls.map((c: any[]) => String(c[0])); + expect(warned.some((m: string) => m.includes('no `cache` service registered at all'))).toBe(true); + }); + + it('leaves the counters to better-auth when the host supplies its own secondaryStorage', async () => { + const ctx = { + ...mockContext, + getServiceAsync: vi.fn(async () => undefined), + } as unknown as PluginContext; + const plugin = new AuthPlugin({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + secondaryStorage: { get: vi.fn(), set: vi.fn(), delete: vi.fn() }, + } as any); + await plugin.init(ctx); + const manager = (ctx.registerService as any).mock.calls.find( + ([name]: [string]) => name === 'auth', + )?.[1] as AuthManager; + expect((manager as any).config.rateLimitStorage).toBeUndefined(); + expect((manager as any).config.secondaryStorage).toBeDefined(); + }); + + it('never binds the kernel cache as better-auth secondaryStorage (sessions stay in sys_session)', async () => { + // ADR-0069 D4's session controls revoke by writing the `sys_session` row; + // better-auth answers `findSession` from a secondaryStorage snapshot + // without reading the database, so a cache-backed session store would + // silently disable them. The cache reaches the COUNTERS only. + const cache = makeCache(); + const { manager } = await bootWith(async () => cache); + expect((manager as any).config.secondaryStorage).toBeUndefined(); + }); + }); }); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 9bba9fbf61..9c7a9e0360 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -331,33 +331,50 @@ export class AuthPlugin implements Plugin { }, }; - // ADR-0069 D2 — wire the kernel `cache` service as better-auth's shared - // secondaryStorage (rate-limit counters + session cache). Shared across - // nodes iff the cache service is (Redis adapter in a cluster; memory - // single-node). An explicit `secondaryStorage` on the options wins. Skipped - // when no cache service is registered — with a warning, because a multi-node - // deployment then silently rate-limits per-process (ADR-0069 D2 honesty). + // ADR-0069 D2 — cross-node rate-limit counters, backed by the kernel + // `cache` service and resolved LAZILY (#4772). + // + // This used to be an eager `getServiceAsync('cache')` probe right here, + // whose answer was frozen for the life of the process. `init()` runs before + // CacheServicePlugin registers the service (21ms earlier in a showcase cold + // start), so the probe resolved `undefined` in deployments that HAVE a cache + // configured, and the warning it printed sent operators to provision Redis + // for a problem they did not have. Worse than the misdiagnosis: the + // degradation was real and permanent — the counters never reached the shared + // store even once it came up, so a multi-node deployment's limits were never + // enforced globally. `createLazyCacheRateLimitStorage` resolves the service + // at the moment a counter is consumed, which is strictly after `kernel:ready` + // and therefore after ANY plugin ordering, and warns only when a counter + // genuinely has nowhere shared to count. + // + // Deliberately `rateLimit.customStorage`, NOT `secondaryStorage`: a + // `secondaryStorage` also relocates the SESSION of record into the cache + // (better-auth's `createSession` skips the `sys_session` row and + // `findSession` answers from the snapshot without reading the database), + // which would silently disable the ADR-0069 D4 session controls — idle + // timeout, absolute max and concurrent cap all revoke by writing that row. + // Whether the session of record should live in the cache at all is #4785. + // A host that supplies `secondaryStorage` itself still wins and keeps + // better-auth's own `storage: 'secondary-storage'` counters. if (!authConfig.secondaryStorage) { - // The `cache` service is registered ASYNC — `getService` throws for it, - // so resolve via `getServiceAsync` and treat any failure (not registered, - // or not yet ready) as "no shared cache". - let cache: any; - try { - cache = await (ctx as { getServiceAsync?: (n: string) => Promise }).getServiceAsync?.('cache'); - } catch { - cache = undefined; - } - if (cache && typeof cache.get === 'function' && typeof cache.set === 'function') { - const { cacheSecondaryStorage } = await import('./secondary-storage.js'); - authConfig.secondaryStorage = cacheSecondaryStorage(cache); - ctx.logger.info( - '[auth] rate-limit + session store bound to the kernel cache service — shared across nodes iff the cache is (ADR-0069 D2)', - ); - } else { - ctx.logger.warn( - '[auth] no cache service registered — rate-limit counters use a per-process in-memory store; a multi-node deployment needs a shared cache (Redis) to enforce limits globally (ADR-0069 D2)', - ); - } + const { createLazyCacheRateLimitStorage } = await import('./rate-limit-storage.js'); + authConfig.rateLimitStorage = createLazyCacheRateLimitStorage({ + // The `cache` service is registered ASYNC — `getService` throws for it, + // so resolve via `getServiceAsync` and treat any failure (not + // registered, or not yet ready) as "no shared cache, ask again later". + resolveCache: async () => { + let cache: any; + try { + cache = await (ctx as { getServiceAsync?: (n: string) => Promise }) + .getServiceAsync?.('cache'); + } catch { + return undefined; + } + if (cache && typeof cache.get === 'function' && typeof cache.set === 'function') return cache; + return undefined; + }, + logger: ctx.logger, + }); } this.applyEnvSocialProviderFallbacks(authConfig); diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index 354e9b0462..965023ba6c 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -22,6 +22,13 @@ export * from './admin-import-users.js'; export * from './identity-write-guard.js'; export * from './sys-user-writable-fields.js'; export * from './otp-send-guard.js'; +// ADR-0069 D2 / #4772 — the cross-node rate-limit counter store (kernel cache, +// resolved lazily) and the `cache` → better-auth `secondaryStorage` adapter. +// The adapter is exported rather than auto-wired: it also relocates the SESSION +// of record out of `sys_session`, which disables the ADR-0069 D4 session +// controls, so a host opts into that trade explicitly or not at all. +export * from './rate-limit-storage.js'; +export * from './secondary-storage.js'; export * from './register-sso-provider.js'; export * from './send-verification-email.js'; export * from './objectql-adapter.js'; diff --git a/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts b/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts new file mode 100644 index 0000000000..71287cabfe --- /dev/null +++ b/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts @@ -0,0 +1,241 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi } from 'vitest'; +import { + createLazyCacheRateLimitStorage, + incrementFixedWindow, + InProcessCounterStore, +} from './rate-limit-storage.js'; + +/** Minimal in-memory ICacheService stand-in (no TTL eviction — tests drive the clock). */ +function makeCache() { + const store = new Map(); + return { + store, + get: vi.fn(async (k: string) => (store.has(k) ? store.get(k) : undefined)), + set: vi.fn(async (k: string, v: unknown, _ttl?: number) => { store.set(k, v); }), + delete: vi.fn(async (k: string) => store.delete(k)), + has: vi.fn(async (k: string) => store.has(k)), + clear: vi.fn(async () => store.clear()), + stats: vi.fn(async () => ({ hits: 0, misses: 0, keys: store.size })), + }; +} + +const makeLogger = () => ({ info: vi.fn(), warn: vi.fn() }); + +describe('incrementFixedWindow (ADR-0069 D2 — one counting algorithm, any store)', () => { + it('creates the window at 1 and reports when it resets', async () => { + const cache = makeCache(); + const t0 = 1_000_000; + const r = await incrementFixedWindow(cache as any, 'ip:1', 60, t0); + expect(r.count).toBe(1); + expect(r.resetAt).toBe(t0 + 60_000); + expect(cache.set).toHaveBeenCalledWith('ip:1', JSON.stringify({ n: 1, exp: t0 + 60_000 }), 60); + }); + + it('does NOT slide the window forward on later increments', async () => { + const cache = makeCache(); + const t0 = 1_000_000; + await incrementFixedWindow(cache as any, 'ip:1', 60, t0); + const second = await incrementFixedWindow(cache as any, 'ip:1', 60, t0 + 30_000); + expect(second.count).toBe(2); + // Window end unchanged; only the REMAINING seconds are handed to the store. + expect(second.resetAt).toBe(t0 + 60_000); + expect(cache.set).toHaveBeenLastCalledWith('ip:1', JSON.stringify({ n: 2, exp: t0 + 60_000 }), 30); + }); + + it('restarts the window once it has elapsed', async () => { + const cache = makeCache(); + const t0 = 1_000_000; + await incrementFixedWindow(cache as any, 'ip:1', 60, t0); + const later = await incrementFixedWindow(cache as any, 'ip:1', 60, t0 + 60_001); + expect(later.count).toBe(1); + }); + + it('treats a foreign / unparseable value under the key as absent', async () => { + const cache = makeCache(); + cache.store.set('ip:1', 'not json'); + const r = await incrementFixedWindow(cache as any, 'ip:1', 60, 1_000_000); + expect(r.count).toBe(1); + }); + + it('accepts an object-valued read back (memory adapter) as well as a string (Redis)', async () => { + const cache = makeCache(); + const t0 = 1_000_000; + cache.store.set('ip:1', { n: 4, exp: t0 + 60_000 }); + const r = await incrementFixedWindow(cache as any, 'ip:1', 60, t0); + expect(r.count).toBe(5); + }); +}); + +describe('createLazyCacheRateLimitStorage — cache present (#4772 acceptance 1)', () => { + it('counts in the kernel cache and never warns', async () => { + const cache = makeCache(); + const logger = makeLogger(); + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => cache as any, + logger, + }); + + const first = await storage.consume('ip:203.0.113.7', { window: 60, max: 2 }); + expect(first).toEqual({ allowed: true, retryAfter: null }); + // The counter really lives in the shared store, not in this process. + expect(cache.set).toHaveBeenCalledTimes(1); + expect(cache.store.size).toBe(1); + + await storage.consume('ip:203.0.113.7', { window: 60, max: 2 }); + const third = await storage.consume('ip:203.0.113.7', { window: 60, max: 2 }); + expect(third.allowed).toBe(false); + expect(third.retryAfter).toBeGreaterThan(0); + + expect(logger.warn).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledTimes(1); + expect(logger.info.mock.calls[0][0]).toContain('rate-limit counters bound to the kernel cache service'); + }); + + it('resolves the cache once and reuses the handle', async () => { + const cache = makeCache(); + const resolveCache = vi.fn(async () => cache as any); + const storage = createLazyCacheRateLimitStorage({ resolveCache }); + await storage.consume('k', { window: 60, max: 10 }); + await storage.consume('k', { window: 60, max: 10 }); + expect(resolveCache).toHaveBeenCalledTimes(1); + }); +}); + +// The bug this module exists for: AuthPlugin.init() runs BEFORE +// CacheServicePlugin registers `cache`. The old eager probe froze the "no +// cache" answer for the process, so the counters never reached the shared +// store even after it came up. +describe('createLazyCacheRateLimitStorage — cache registered AFTER auth (#4772 acceptance 3)', () => { + it('picks the shared cache up on the first consume that follows registration', async () => { + const cache = makeCache(); + let registered = false; + const logger = makeLogger(); + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => (registered ? (cache as any) : undefined), + logger, + }); + + // Counter consumed while `cache` is still unregistered → per-process fallback. + await storage.consume('ip:198.51.100.9', { window: 60, max: 5 }); + expect(cache.store.size).toBe(0); + expect(logger.warn).toHaveBeenCalledTimes(1); + + // CacheServicePlugin registers the service. + registered = true; + + // The very next counter is consumed from the SHARED store — this is the + // assertion the eager probe could never satisfy. + const after = await storage.consume('ip:198.51.100.9', { window: 60, max: 5 }); + expect(after.allowed).toBe(true); + expect(cache.set).toHaveBeenCalledTimes(1); + expect(cache.store.size).toBe(1); + expect(logger.info).toHaveBeenCalledTimes(1); + expect(logger.info.mock.calls[0][0]).toContain('bound to the kernel cache service'); + + // …and it stays there. + await storage.consume('ip:198.51.100.9', { window: 60, max: 5 }); + expect(JSON.parse(cache.store.get('ip:198.51.100.9') as string).n).toBe(2); + }); + + it('stops warning once the cache is there (the warn is a live signal, not a boot artifact)', async () => { + const cache = makeCache(); + let registered = false; + const logger = makeLogger(); + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => (registered ? (cache as any) : undefined), + logger, + }); + await storage.consume('k', { window: 60, max: 10 }); + registered = true; + await storage.consume('k', { window: 60, max: 10 }); + await storage.consume('k', { window: 60, max: 10 }); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); +}); + +describe('createLazyCacheRateLimitStorage — no cache service at all (#4772 acceptance 2)', () => { + it('warns exactly once, at counting time, and says what is actually wrong', async () => { + const logger = makeLogger(); + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => undefined, + logger, + }); + + // Nothing is logged until a counter is actually consumed — a deployment + // that never rate-limits is not warned about a store it never needs. + expect(logger.warn).not.toHaveBeenCalled(); + + await storage.consume('k', { window: 60, max: 10 }); + await storage.consume('k', { window: 60, max: 10 }); + await storage.consume('other', { window: 60, max: 10 }); + + expect(logger.warn).toHaveBeenCalledTimes(1); + const msg = logger.warn.mock.calls[0][0] as string; + expect(msg).toContain('no `cache` service registered at all'); + expect(msg).toContain('per-process store'); + expect(logger.info).not.toHaveBeenCalled(); + }); + + it('still enforces the limit in-process — degraded, never disabled', async () => { + const storage = createLazyCacheRateLimitStorage({ resolveCache: async () => undefined }); + expect((await storage.consume('ip:a', { window: 60, max: 2 })).allowed).toBe(true); + expect((await storage.consume('ip:a', { window: 60, max: 2 })).allowed).toBe(true); + const third = await storage.consume('ip:a', { window: 60, max: 2 }); + expect(third.allowed).toBe(false); + expect(third.retryAfter).toBeGreaterThan(0); + // Independent keys keep independent windows. + expect((await storage.consume('ip:b', { window: 60, max: 2 })).allowed).toBe(true); + }); + + it('treats a throwing resolver as "no cache right now" rather than failing the request', async () => { + const logger = makeLogger(); + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => { throw new Error('service registry exploded'); }, + logger, + }); + await expect(storage.consume('k', { window: 60, max: 10 })).resolves.toEqual({ + allowed: true, + retryAfter: null, + }); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + + it('survives a logger with no warn/info methods', async () => { + const storage = createLazyCacheRateLimitStorage({ + resolveCache: async () => undefined, + logger: {}, + }); + await expect(storage.consume('k', { window: 1, max: 1 })).resolves.toBeTruthy(); + }); +}); + +describe('InProcessCounterStore (the degraded path)', () => { + it('expires entries on read', async () => { + const store = new InProcessCounterStore(); + await store.set('k', 'v', 1); + expect(await store.get('k')).toBe('v'); + vi.useFakeTimers(); + try { + vi.setSystemTime(Date.now() + 2000); + expect(await store.get('k')).toBeUndefined(); + } finally { + vi.useRealTimers(); + } + }); + + it('prunes expired entries instead of growing without bound', async () => { + const store = new InProcessCounterStore(); + await store.set('a', 1, 1); + expect(store.size).toBe(1); + vi.useFakeTimers(); + try { + vi.setSystemTime(Date.now() + 2000); + await store.set('b', 2, 60); + expect(store.size).toBe(1); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/packages/plugins/plugin-auth/src/rate-limit-storage.ts b/packages/plugins/plugin-auth/src/rate-limit-storage.ts new file mode 100644 index 0000000000..cfca359f8f --- /dev/null +++ b/packages/plugins/plugin-auth/src/rate-limit-storage.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { BetterAuthRateLimitStorage } from '@better-auth/core'; + +/** + * The slice of `ICacheService` a fixed-window counter needs. Declared + * structurally so the in-process fallback below satisfies the SAME contract + * the kernel cache does — one counting algorithm, two backing stores, no + * second code path that can drift. + */ +export interface CounterStore { + get(key: string): Promise; + set(key: string, value: T, ttl?: number): Promise; +} + +/** The window envelope a counter stores: `n` requests so far, window ends at `exp` (epoch ms). */ +interface Counter { + n: number; + exp: number; +} + +/** Outcome of one increment: the POST-increment count and when the window frees up. */ +export interface FixedWindowCount { + count: number; + /** Epoch ms at which the current window expires. */ + resetAt: number; +} + +/** + * Read back a counter envelope. Cache adapters differ on whether they hand + * values back as the stored string (Redis) or as the original object (memory), + * so both are accepted. Anything else — a legacy or foreign value under the + * key — is treated as absent, which restarts the window rather than throwing + * on an auth request. + */ +function parseCounter(raw: unknown): Counter | null { + if (raw === undefined || raw === null) return null; + let value: unknown = raw; + if (typeof raw === 'string') { + try { + value = JSON.parse(raw); + } catch { + return null; + } + } + if (typeof value !== 'object' || value === null) return null; + const { n, exp } = value as Partial; + if (typeof n !== 'number' || !Number.isFinite(n)) return null; + if (typeof exp !== 'number' || !Number.isFinite(exp)) return null; + return { n, exp }; +} + +/** + * Fixed-window counter over any {@link CounterStore}. + * + * Contract: return the POST-increment count; create the key at 1 with + * `windowSeconds` when absent or expired; never extend the window on later + * increments, so the counter expires a fixed window after it was FIRST + * created rather than sliding forward on every request. + * + * `set` always (re)sets the TTL, so the window end travels with the value and + * each re-set is given only the REMAINING seconds. The envelope is private to + * this module — every reader goes through {@link parseCounter}. + * + * Without an atomic INCR this stays read-modify-write: two nodes can read the + * same count and both admit a request, so a limit can over-admit slightly + * under concurrency. That is strictly better than per-node independent + * counters (ADR-0069 D2); a cache adapter that grows an atomic INCR should be + * plumbed through `ICacheService` and used here. + */ +export async function incrementFixedWindow( + store: CounterStore, + key: string, + windowSeconds: number, + now: number = Date.now(), +): Promise { + const ttl = Math.max(1, Math.floor(windowSeconds) || 1); + const current = parseCounter(await store.get(key)); + + if (!current || current.exp <= now) { + const exp = now + ttl * 1000; + await store.set(key, JSON.stringify({ n: 1, exp }), ttl); + return { count: 1, resetAt: exp }; + } + + const n = current.n + 1; + const remaining = Math.max(1, Math.ceil((current.exp - now) / 1000)); + await store.set(key, JSON.stringify({ n, exp: current.exp }), remaining); + return { count: n, resetAt: current.exp }; +} + +/** + * The per-process fallback store. Deliberately NOT better-auth's own memory + * storage: `customStorage` replaces better-auth's storage selection wholesale, + * so the degraded path has to count somewhere, and counting here keeps ONE + * algorithm across both stores (a divergent fallback is how "it works in + * tests, not in prod" happens). + * + * Bounded: expired entries are pruned on write, and the map is cleared if it + * ever exceeds {@link MAX_FALLBACK_KEYS} live keys — an unbounded per-IP map is + * a memory-exhaustion vector on exactly the endpoint being attacked. + */ +const MAX_FALLBACK_KEYS = 10_000; + +export class InProcessCounterStore implements CounterStore { + private readonly entries = new Map(); + + async get(key: string): Promise { + const hit = this.entries.get(key); + if (!hit) return undefined; + if (hit.exp <= Date.now()) { + this.entries.delete(key); + return undefined; + } + return hit.value as T; + } + + async set(key: string, value: T, ttl?: number): Promise { + this.prune(); + this.entries.set(key, { value, exp: Date.now() + Math.max(1, Math.floor(ttl ?? 60)) * 1000 }); + } + + private prune(): void { + const now = Date.now(); + for (const [k, v] of this.entries) if (v.exp <= now) this.entries.delete(k); + if (this.entries.size > MAX_FALLBACK_KEYS) this.entries.clear(); + } + + /** @internal test seam */ + get size(): number { + return this.entries.size; + } +} + +type LoggerLike = { + info?(msg: string): void; + warn?(msg: string): void; +}; + +export interface LazyCacheRateLimitStorageOptions { + /** + * Resolve the kernel `cache` service. Called at COUNTING time, not at plugin + * init — see the class comment on {@link createLazyCacheRateLimitStorage}. + * Returning `undefined` (or throwing) means "no shared cache right now". + */ + resolveCache: () => Promise; + logger?: LoggerLike; +} + +/** + * ADR-0069 D2 — better-auth `rateLimit.customStorage` backed by the kernel + * `cache` service, resolved **lazily, at the moment a counter is consumed**. + * + * Why lazy (#4772): `AuthPlugin.init()` runs BEFORE `CacheServicePlugin` + * registers the `cache` service (21ms earlier in a showcase cold start), so a + * probe taken at init resolves `undefined` in a deployment that HAS a cache + * configured. The old code cached that answer for the life of the process: + * the warning it printed was a misdiagnosis ("you need Redis" — the operator + * already had a cache plugin), and the degradation was real — rate-limit + * counters never reached the shared store even after it came up, so a + * multi-node deployment counted per node and its limits were never enforced + * globally. Resolving at consume time removes the ordering dependency + * entirely: whichever plugin registers `cache` and whenever, the first counter + * consumed after that point uses it. + * + * The warning is kept, but it now fires only when a counter ACTUALLY needs a + * shared store and there genuinely is none — at which point it is a true + * signal, and "add a shared cache" is the right advice. It is emitted once per + * process. + * + * Scope note: this wires the RATE-LIMIT COUNTERS only. better-auth's + * `secondaryStorage` (session snapshots) is a separate, deliberately untouched + * decision — see the comment in `auth-plugin.ts` and `secondary-storage.ts`. + */ +export function createLazyCacheRateLimitStorage( + opts: LazyCacheRateLimitStorageOptions, +): BetterAuthRateLimitStorage { + const fallback = new InProcessCounterStore(); + let cache: CounterStore | undefined; + let boundAnnounced = false; + let degradedWarned = false; + + const resolveStore = async (): Promise => { + if (!cache) { + try { + cache = (await opts.resolveCache()) ?? undefined; + } catch { + cache = undefined; + } + if (cache && !boundAnnounced) { + boundAnnounced = true; + opts.logger?.info?.( + '[auth] rate-limit counters bound to the kernel cache service — enforced against ONE store across nodes iff the cache is shared (ADR-0069 D2)', + ); + } + } + if (cache) return cache; + if (!degradedWarned) { + degradedWarned = true; + opts.logger?.warn?.( + '[auth] rate-limit counters have no cache service to count in — falling back to a per-process store. ' + + 'This deployment has no `cache` service registered at all; a multi-node deployment needs a shared cache ' + + '(Redis via @objectstack/service-cache) or each node enforces the limit independently (ADR-0069 D2)', + ); + } + return fallback; + }; + + return { + consume: async (key, rule) => { + const store = await resolveStore(); + const now = Date.now(); + const { count, resetAt } = await incrementFixedWindow(store, key, rule.window, now); + if (count <= rule.max) return { allowed: true, retryAfter: null }; + return { allowed: false, retryAfter: Math.max(1, Math.ceil((resetAt - now) / 1000)) }; + }, + }; +} diff --git a/packages/plugins/plugin-auth/src/secondary-storage.ts b/packages/plugins/plugin-auth/src/secondary-storage.ts index dc0983f793..982a318b9b 100644 --- a/packages/plugins/plugin-auth/src/secondary-storage.ts +++ b/packages/plugins/plugin-auth/src/secondary-storage.ts @@ -2,6 +2,7 @@ import type { BetterAuthOptions } from 'better-auth'; import type { ICacheService } from '@objectstack/spec/contracts'; +import { incrementFixedWindow } from './rate-limit-storage.js'; type SecondaryStorage = NonNullable; @@ -18,6 +19,23 @@ type SecondaryStorage = NonNullable; * single shared counter — closing the "each node counts independently, so an * attacker rotates nodes to bypass the limit" hole (ADR-0069 D2). * + * ⚠️ `secondaryStorage` is NOT the seam the shared rate-limit counter uses any + * more (#4772). It cannot be: handing better-auth a `secondaryStorage` also + * moves the SESSION of record into it — `internalAdapter.createSession` skips + * the `sys_session` row unless `session.storeSessionInDatabase` is set, and + * `findSession` answers from the cached snapshot without consulting the + * database at all. ObjectStack revokes sessions by writing the row + * (`enforceSessionControls` / `enforceConcurrentCap` stamp `revoked_at` + + * a past `expires_at`, ADR-0069 D4), so a cache-backed session store makes + * idle-timeout, absolute-max and concurrent-cap revocation silently stop + * taking effect. The counters therefore ride `rateLimit.customStorage` + * instead (see `rate-limit-storage.ts`), which touches counters only. This + * adapter stays for a host that supplies `secondaryStorage` deliberately and + * accepts that trade; it is no longer wired automatically from the cache + * service. Whether ObjectStack should move the session of record into the + * cache at all (and rewrite D4's revocation to match) is #4785 — a decision, + * not a bug fix. + * * better-auth's `secondaryStorage` contract is string-valued: `get` returns the * stored string (or null), `set` takes a string value + optional TTL (seconds), * `delete` removes it. We map straight onto `ICacheService`, translating @@ -65,8 +83,12 @@ export function cacheSecondaryStorage(cache: ICacheService): SecondaryStorage { * * `ICacheService.set` always (re)sets the TTL, so the window end is stored * alongside the count and each re-set is given only the REMAINING seconds. - * The envelope is private to this function — better-auth reads rate-limit - * counters exclusively through `increment`. + * The envelope is private to `incrementFixedWindow` — better-auth reads + * rate-limit counters exclusively through `increment`. + * + * Shares ONE implementation with `rateLimit.customStorage` + * ({@link incrementFixedWindow}) so the two seams that count auth requests + * cannot drift apart in window semantics. * * Without an atomic INCR this stays read-modify-write: two nodes can read * the same count and both admit a request, so the limit can over-admit @@ -75,19 +97,8 @@ export function cacheSecondaryStorage(cache: ICacheService): SecondaryStorage { * independent counters it replaces (ADR-0069 D2). */ increment: async (key: string, ttl: number): Promise => { - const now = Date.now(); - const current = parseCounter(await cache.get(key)); - - if (!current || current.exp <= now) { - const exp = now + Math.max(1, ttl) * 1000; - await cache.set(key, JSON.stringify({ n: 1, exp }), Math.max(1, ttl)); - return 1; - } - - const n = current.n + 1; - const remaining = Math.max(1, Math.ceil((current.exp - now) / 1000)); - await cache.set(key, JSON.stringify({ n, exp: current.exp }), remaining); - return n; + const { count } = await incrementFixedWindow(cache, key, ttl); + return count; }, set: async (key: string, value: string, ttl?: number): Promise => { await cache.set(key, value, ttl); @@ -97,33 +108,3 @@ export function cacheSecondaryStorage(cache: ICacheService): SecondaryStorage { }, }; } - -/** The window envelope `increment` stores: `n` requests so far, window ends at `exp` (epoch ms). */ -interface Counter { - n: number; - exp: number; -} - -/** - * Read back an `increment` envelope. Cache adapters differ on whether they - * hand values back as the stored string (Redis) or as the original object - * (memory), so both are accepted. Anything else — a legacy or foreign value - * under the key — is treated as absent, which restarts the window rather than - * throwing on an auth request. - */ -function parseCounter(raw: unknown): Counter | null { - if (raw === undefined || raw === null) return null; - let value: unknown = raw; - if (typeof raw === 'string') { - try { - value = JSON.parse(raw); - } catch { - return null; - } - } - if (typeof value !== 'object' || value === null) return null; - const { n, exp } = value as Partial; - if (typeof n !== 'number' || !Number.isFinite(n)) return null; - if (typeof exp !== 'number' || !Number.isFinite(exp)) return null; - return { n, exp }; -}