diff --git a/.changeset/auth-otp-budget-shared-counter-store.md b/.changeset/auth-otp-budget-shared-counter-store.md new file mode 100644 index 0000000000..017a87a115 --- /dev/null +++ b/.changeset/auth-otp-budget-shared-counter-store.md @@ -0,0 +1,41 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): 每号码 OTP 发送预算改用惰性解析的共享计数存储 —— 多节点下不再按节点数倍增 (#4790) + +#2780 的「每号码 OTP 发送预算」(60s 冷却 + 每小时 5 条)此前**只有宿主显式提供 +better-auth `secondaryStorage` 时才跨节点共享**:`AuthManager.getOtpSendGuard()` 唯一的 +存储来源就是 `AuthManagerOptions.secondaryStorage`,而标准 `serve` 组合里没有任何一处 +提供它(#4788 之后 `AuthPlugin` 也明确不再从 cache 服务派生它)。于是预算落在**每个进程 +一份**:N 个节点的部署,一个号码实际能收到的是声明值的 N 倍,而且**没有任何信号**告诉你 +它没兑现(ADR-0049 声明 ≠ 强制)。这里的计价单位是**真金白银的短信**。 + +这是 #4772 那条限流洞的同类,但是独立的一处:#4788 修的是 better-auth 自己的 `rateLimit` +计数器(走 `rateLimit.customStorage`),OTP 预算是 ObjectStack 在 `AuthManager` 里自己实现 +的另一套计数,行为未被 #4788 改变。 + +**修法:复用 #4788 建好的那条路径,而不是再写一份。** `rate-limit-storage.ts` 中把「惰性 +解析 → 绑定即宣告 → 解析不到就降级到有界的进程内存储并响亮告警」抽成 +`createLazyCounterStore()`(`createLazyCacheRateLimitStorage()` 现在就是它的一层薄封装), +OTP 预算经由新的 `AuthManagerOptions.sharedCounterStore` 接同一条路径: + +- **存储在每次发送校验时才解析**,因此 `CacheServicePlugin` 晚于 `AuthPlugin` 注册也照样 + 绑定得上(插件启动顺序不再决定任何事)—— 这正是 #4772 冻结结论造成的那个洞; +- 配了 cache 的多节点部署,每号码预算**现在真的是一份**,换节点不会重新获得冷却额度; +- 没有 cache 服务的部署**仍然限额**,只是降级为进程内计数,并在第一次真正计数时打一条 + 点名代价的 warn(「an N-node deployment can send up to N× the configured number of PAID + SMS to one number」)—— 降级不是关闭,两种情况在日志里可区分(绑定打 info,降级打 warn)。 + +**刻意不引入 `secondaryStorage` 来修它**(#4785):那会把会话的记录之处搬进缓存,静默废掉 +ADR-0069 D4 的三个会话管控。宿主自己提供的 `secondaryStorage` 对这个预算仍然优先且行为不变。 + +冷却与滚动小时窗的语义**未做任何改动**:计数依旧是按号码的时间戳滚动窗口,只是换了它所在的 +存储。(固定窗口计数器无法表达「距上一次发送满 N 秒」,把它改成定窗会在窗口边界放行两倍突发 +——用一种倍增换另一种倍增。) + +对使用者的影响: + +- 新增 `AuthManagerOptions.sharedCounterStore`,`AuthPlugin` 自动填充,一般宿主无需感知; +- 新增导出 `createLazyCounterStore()` 与 `counterStoreFromKv()`; +- `OtpSendGuard` 新增 `resolveStore` 选项,原有的 `storage`(字符串 KV)选项保持可用。 diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 717d0d9c19..04c8484550 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1734,6 +1734,88 @@ describe('AuthManager', () => { await manager.assertPhoneOtpSendAllowed(PHONE); }); + // ── #4790 — the budget is only global if its STORE is ───────────────── + describe('where the per-number budget is counted (#4790)', () => { + 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); }), + }; + }; + const bootNode = async (sharedCounterStore: any) => { + const { manager } = await bootOtp({ sharedCounterStore }); + manager.setSmsService(fakeSms().service); + return manager; + }; + + it('spends ONE budget across nodes when a shared counter store is wired', async () => { + const { createLazyCounterStore } = await import('./rate-limit-storage.js'); + const cache = makeCache(); + // Two nodes: separate managers, separate resolvers, one cache. + const nodeStore = () => + createLazyCounterStore({ resolveCache: async () => cache as any, subject: 'otp-budget' }); + const nodeA = await bootNode(nodeStore()); + const nodeB = await bootNode(nodeStore()); + + await nodeA.assertPhoneOtpSendAllowed(PHONE); + // Rotating nodes no longer buys a fresh cooldown. + await expect(nodeB.assertPhoneOtpSendAllowed(PHONE)) + .rejects.toThrow(/Too many verification codes/); + expect(cache.store.size).toBe(1); + expect([...cache.store.keys()][0]).toContain(PHONE); + }); + + it('resolves the store per check, so a cache registered after boot still binds', async () => { + const { createLazyCounterStore } = await import('./rate-limit-storage.js'); + const cache = makeCache(); + let registered = false; + const manager = await bootNode( + createLazyCounterStore({ + resolveCache: async () => (registered ? (cache as any) : undefined), + subject: 'otp-budget', + }), + ); + registered = true; // CacheServicePlugin comes up after plugin-auth. + await manager.assertPhoneOtpSendAllowed(PHONE); + expect(cache.store.size).toBe(1); + }); + + it('a host-supplied secondaryStorage keeps owning the budget', async () => { + const kv = new Map(); + const secondaryStorage = { + get: async (k: string) => kv.get(k) ?? null, + set: async (k: string, v: string) => { kv.set(k, v); }, + delete: async (k: string) => { kv.delete(k); }, + }; + const cache = makeCache(); + const { manager } = await bootOtp({ + secondaryStorage, + sharedCounterStore: async () => cache as any, + }); + manager.setSmsService(fakeSms().service); + await manager.assertPhoneOtpSendAllowed(PHONE); + expect(kv.size).toBe(1); + expect(cache.store.size).toBe(0); + }); + + it('without any shared store the budget is per-manager — degraded, still enforced', async () => { + const { manager: nodeA } = await bootOtp(); + const { manager: nodeB } = await bootOtp(); + nodeA.setSmsService(fakeSms().service); + nodeB.setSmsService(fakeSms().service); + + await nodeA.assertPhoneOtpSendAllowed(PHONE); + // Enforced on its own node… + await expect(nodeA.assertPhoneOtpSendAllowed(PHONE)) + .rejects.toThrow(/Too many verification codes/); + // …and not on the other one: exactly the N× multiplication #4790 is + // about, which is why AuthPlugin warns loudly when it has to do this. + await nodeB.assertPhoneOtpSendAllowed(PHONE); + }); + }); + it('features.phoneNumberOtp requires plugin + deliverable SMS', async () => { const { manager } = await bootOtp(); expect((manager.getPublicConfig() as any).features.phoneNumberOtp).toBe(false); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 5ce1afb27d..48ab9706f1 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -26,6 +26,7 @@ import { isPlaceholderEmail } from './placeholder-email.js'; import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; import type { TenancyService } from './tenancy-service.js'; import { OtpSendGuard } from './otp-send-guard.js'; +import type { CounterStore } from './rate-limit-storage.js'; import { PHONE_SMS_TOPICS, builtinPhoneSmsBody, @@ -620,6 +621,26 @@ export interface AuthManagerOptions extends Partial { * drop the store. */ rateLimitStorage?: NonNullable['customStorage']; + + /** + * ADR-0069 D2 (#4790) — the store ObjectStack's OWN cross-node counters + * count in, resolved LAZILY (called per count, not at construction). Today + * that is the #2780 per-number OTP send budget; {@link rateLimitStorage} + * covers better-auth's own per-IP counters, which never pass through here. + * + * `AuthPlugin` supplies `createLazyCounterStore(...)` over the kernel `cache` + * service (`rate-limit-storage.ts`) — the same resolution the rate-limit + * counters use, so plugin start order decides nothing and a deployment + * WITHOUT a cache still counts, per process, and is told so. Absent → the + * budget is per-process silently, which is the pre-#4790 behaviour and is + * why this option exists. + * + * NOT `secondaryStorage`: handing better-auth one of those also relocates the + * session of record into the cache and silently disables the ADR-0069 D4 + * session controls (#4785). A host that supplies `secondaryStorage` + * deliberately still wins for this budget — see `getOtpSendGuard`. + */ + sharedCounterStore?: () => Promise; } /** @@ -2511,12 +2532,22 @@ export class AuthManager { private getOtpSendGuard(): OtpSendGuard { if (!this._otpSendGuard) { const otpCfg = this.config.phoneOtp ?? {}; + // WHERE the budget is counted decides whether it is a budget at all + // (#4790): counted per process, a declared "5 per hour" is 5×N across N + // nodes. Precedence, most deliberate first: + // 1. a host-supplied `secondaryStorage` — an explicit cross-node KV; + // 2. `sharedCounterStore` — AuthPlugin's lazily-resolved kernel `cache`, + // which also announces the degraded (no cache) case loudly; + // 3. neither → the guard's own bounded per-process store. + const storeOption = this.config.secondaryStorage + ? { storage: this.config.secondaryStorage } + : this.config.sharedCounterStore + ? { resolveStore: this.config.sharedCounterStore } + : {}; this._otpSendGuard = new OtpSendGuard({ ...(otpCfg.cooldownSeconds != null ? { cooldownSeconds: otpCfg.cooldownSeconds } : {}), ...(otpCfg.maxPerHour != null ? { maxPerHour: otpCfg.maxPerHour } : {}), - // Share better-auth's cross-node KV when wired (ADR-0069 D2) so the - // per-number budget is enforced against ONE store across nodes. - ...(this.config.secondaryStorage ? { storage: this.config.secondaryStorage } : {}), + ...storeOption, }); } return this._otpSendGuard; diff --git a/packages/plugins/plugin-auth/src/auth-plugin.test.ts b/packages/plugins/plugin-auth/src/auth-plugin.test.ts index 2d6d60f6fa..895c73ccfb 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.test.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.test.ts @@ -1319,5 +1319,69 @@ describe('AuthPlugin', () => { const { manager } = await bootWith(async () => cache); expect((manager as any).config.secondaryStorage).toBeUndefined(); }); + + // ── #4790 — the SECOND counter with the same hole: ObjectStack's own + // per-number OTP send budget (#2780). It shared state only through a + // host-supplied `secondaryStorage`, which the standard `serve` composition + // never supplies, so the declared per-number cap was cap×N across N nodes — + // in paid SMS. Same cure, same resolution path, deliberately NOT + // `secondaryStorage` (#4785). + describe('per-number OTP send budget (#2780 / #4790)', () => { + const PHONE = '+8613800000000'; + const deliverableSms = { async send() { return { id: 'x', status: 'sent' }; }, isConfigured: () => true }; + + it('counts the budget in a cache registered AFTER auth init', async () => { + const cache = makeCache(); + let registered = false; + const { ctx, manager } = await bootWith(async () => (registered ? cache : undefined)); + expect((manager as any).config.sharedCounterStore).toBeTypeOf('function'); + + // CacheServicePlugin comes up 21ms later. + registered = true; + manager.setSmsService(deliverableSms as any); + + await manager.assertPhoneOtpSendAllowed(PHONE); + // The send landed in the SHARED store — this is the assertion the + // pre-#4790 wiring could not satisfy in any standard composition. + expect([...cache.store.keys()]).toEqual([`phone-otp-sends:${PHONE}`]); + await expect(manager.assertPhoneOtpSendAllowed(PHONE)) + .rejects.toThrow(/Too many verification codes/); + + // Bound → an info line and NO warning: the operator must be able to + // tell "shared" from "degraded" without reading the code. + const info = (ctx.logger.info as any).mock.calls.map((c: any[]) => String(c[0])); + expect(info.some((m: string) => m.includes('per-number OTP send budget (#2780) bound to the kernel cache service'))).toBe(true); + expect((ctx.logger.warn as any).mock.calls + .map((c: any[]) => String(c[0])) + .filter((m: string) => m.includes('per-number OTP send budget'))).toEqual([]); + }); + + it('warns loudly at counting time when there is no cache — degraded, never disabled', async () => { + const { ctx, manager } = await bootWith(async () => undefined); + manager.setSmsService(deliverableSms as any); + const otpWarnings = () => + (ctx.logger.warn as any).mock.calls + .map((c: any[]) => String(c[0])) + .filter((m: string) => m.includes('per-number OTP send budget')); + + // Nothing is said at boot — a deployment that never sends an OTP is not + // warned about a store it never needs. + expect(otpWarnings()).toEqual([]); + + await manager.assertPhoneOtpSendAllowed(PHONE); + // Still enforced, in-process. + await expect(manager.assertPhoneOtpSendAllowed(PHONE)) + .rejects.toThrow(/Too many verification codes/); + + expect(otpWarnings()).toHaveLength(1); + expect(otpWarnings()[0]).toContain('PAID SMS'); + expect(otpWarnings()[0]).toContain('no `cache` service registered at all'); + // Degraded → a warning and NO "bound" info line; the mirror image of + // the cache-present case above. + expect((ctx.logger.info as any).mock.calls + .map((c: any[]) => String(c[0])) + .filter((m: string) => m.includes('per-number OTP send budget'))).toEqual([]); + }); + }); }); }); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 9c7a9e0360..72b5e1a165 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -41,6 +41,7 @@ import { MANAGED_EXTENSION_EDITABLE_FIELDS } from './managed-extension-fields.js import { runSetInitialPassword } from './set-initial-password.js'; import { runRegisterSsoProviderFromForm, runRegisterSamlProviderFromForm, runRequestDomainVerification, runVerifyDomain } from './register-sso-provider.js'; import { runResendVerificationEmail } from './send-verification-email.js'; +import type { CounterStore } from './rate-limit-storage.js'; import { authIdentityObjects, authPluginManifestHeader, @@ -356,24 +357,51 @@ export class AuthPlugin implements Plugin { // 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) { - 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; - }, + // + // The SAME resolution carries ObjectStack's own per-number OTP send budget + // (#2780) below — that counter had the identical hole (#4790): it counted + // in `secondaryStorage` when a host supplied one and per-process otherwise, + // which under the standard `serve` composition (nobody supplies one) meant + // every node granted the same phone number its own cooldown + hourly cap. + // The currency there is paid SMS, so an N-node deployment was billed for up + // to N× the declared budget. + // + // 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". + const resolveCache = async (): Promise => { + 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; + }; + + { + const { createLazyCacheRateLimitStorage, createLazyCounterStore } = await import( + './rate-limit-storage.js' + ); + if (!authConfig.secondaryStorage) { + authConfig.rateLimitStorage = createLazyCacheRateLimitStorage({ + resolveCache, + logger: ctx.logger, + }); + } + // #4790 — ObjectStack's own counters. Wired even when the host supplied a + // `secondaryStorage` (AuthManager prefers that store for the budget and + // then never calls this resolver), so the two decisions stay independent. + authConfig.sharedCounterStore = createLazyCounterStore({ + resolveCache, logger: ctx.logger, + subject: 'per-number OTP send budget (#2780)', + degradedImpact: + 'The budget is still enforced, but PER NODE: every node grants the same phone number its own ' + + 'cooldown and hourly cap, so an N-node deployment can send up to N× the configured number of ' + + 'PAID SMS to one number (#4790)', }); } diff --git a/packages/plugins/plugin-auth/src/otp-send-guard.test.ts b/packages/plugins/plugin-auth/src/otp-send-guard.test.ts index 6c22b522f4..70490dfc70 100644 --- a/packages/plugins/plugin-auth/src/otp-send-guard.test.ts +++ b/packages/plugins/plugin-auth/src/otp-send-guard.test.ts @@ -1,7 +1,8 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { OtpSendGuard, type OtpGuardStorage } from './otp-send-guard.js'; +import { createLazyCounterStore } from './rate-limit-storage.js'; const PHONE = '+8613800000000'; @@ -79,3 +80,151 @@ describe('OtpSendGuard', () => { expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); }); }); + +// ── #4790 — WHERE the budget is counted ───────────────────────────────────── +// +// The budget was shared across nodes only when a host supplied better-auth's +// `secondaryStorage`, which the standard `serve` composition never does — so +// the declared per-number cap was really cap×N in an N-node deployment, and +// nothing said so. Same defect class as #4772's rate-limit counters, and now +// the same cure: the store is resolved lazily, per check, from the kernel +// `cache` service. +describe('OtpSendGuard — where the budget is counted (#4790)', () => { + /** Minimal in-memory ICacheService stand-in (no TTL eviction — tests drive the clock). */ + 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); }), + }; + }; + const makeLogger = () => ({ info: vi.fn(), warn: vi.fn() }); + const OTP_SUBJECT = 'per-number OTP send budget (#2780)'; + + it('resolves the store at CHECK time — a cache registered AFTER the guard is still used', async () => { + // The exact ordering that made #4772 permanent: plugin-auth builds the + // guard during init(), CacheServicePlugin registers `cache` afterwards. + const c = clock(); + const cache = makeCache(); + let registered = false; + const logger = makeLogger(); + const guard = new OtpSendGuard({ + cooldownSeconds: 60, + now: c.now, + resolveStore: createLazyCounterStore({ + resolveCache: async () => (registered ? (cache as any) : undefined), + logger, + subject: OTP_SUBJECT, + }), + }); + + registered = true; // …21ms later, in a showcase cold start. + + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + // The send really was recorded in the shared store, not in this process. + expect(cache.store.size).toBe(1); + expect([...cache.store.keys()][0]).toBe(`phone-otp-sends:${PHONE}`); + expect(logger.warn).not.toHaveBeenCalled(); + expect(logger.info.mock.calls[0][0]).toContain(`${OTP_SUBJECT} bound to the kernel cache service`); + }); + + it('is ONE budget across nodes when the cache is shared', async () => { + const c = clock(); + const cache = makeCache(); + // Two nodes = two processes = two resolvers over one cache. + const nodeStore = () => + createLazyCounterStore({ resolveCache: async () => cache as any, subject: OTP_SUBJECT }); + const nodeA = new OtpSendGuard({ cooldownSeconds: 60, maxPerHour: 5, now: c.now, resolveStore: nodeStore() }); + const nodeB = new OtpSendGuard({ cooldownSeconds: 60, maxPerHour: 5, now: c.now, resolveStore: nodeStore() }); + + expect((await nodeA.checkAndRecord(PHONE)).ok).toBe(true); + // Rotating to another node does NOT buy a fresh cooldown — the point of #4790. + const denied = await nodeB.checkAndRecord(PHONE); + expect(denied.ok).toBe(false); + expect(denied.retryAfterSeconds).toBeGreaterThan(0); + + // …and the hourly cap is one cap, spent from either node. + c.advance(61_000); + for (const node of [nodeA, nodeB, nodeA, nodeB]) { + expect((await node.checkAndRecord(PHONE)).ok).toBe(true); + c.advance(61_000); + } + expect((await nodeA.checkAndRecord(PHONE)).ok).toBe(false); + expect(cache.store.size).toBe(1); + }); + + it('without any cache service the budget is DEGRADED, not disabled — and said out loud', async () => { + const c = clock(); + const logger = makeLogger(); + const guard = new OtpSendGuard({ + cooldownSeconds: 60, + now: c.now, + resolveStore: createLazyCounterStore({ + resolveCache: async () => undefined, + logger, + subject: OTP_SUBJECT, + degradedImpact: 'up to N× the configured number of PAID SMS (#4790)', + }), + }); + + // Nothing is logged until a send is actually checked. + expect(logger.warn).not.toHaveBeenCalled(); + + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(false); // still enforced, in-process + expect((await guard.checkAndRecord('+15005550006')).ok).toBe(true); + + expect(logger.warn).toHaveBeenCalledTimes(1); + const msg = logger.warn.mock.calls[0][0] as string; + expect(msg).toContain(OTP_SUBJECT); + expect(msg).toContain('per-process store'); + expect(msg).toContain('PAID SMS'); + // The two cases must be distinguishable: bound → info, degraded → warn. + expect(logger.info).not.toHaveBeenCalled(); + }); + + it('an explicit resolveStore wins over a host KV, as the option documents', async () => { + const c = clock(); + const cache = makeCache(); + const kv = new Map(); + const storage: OtpGuardStorage = { + get: (k) => kv.get(k) ?? null, + set: (k, v) => { kv.set(k, v); }, + }; + const guard = new OtpSendGuard({ + cooldownSeconds: 60, + storage, + resolveStore: async () => cache as any, + now: c.now, + }); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + expect(cache.store.size).toBe(1); + expect(kv.size).toBe(0); + }); + + it('fails OPEN when the resolved store breaks mid-flight', async () => { + const guard = new OtpSendGuard({ + cooldownSeconds: 60, + resolveStore: async () => ({ + get: async () => { throw new Error('redis down'); }, + set: async () => { throw new Error('redis down'); }, + }), + }); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + }); + + it('reads a history handed back as an array (memory cache) as well as a string (Redis)', async () => { + const c = clock(); + const cache = makeCache(); + // The memory cache adapter returns the stored value as-is. + cache.store.set(`phone-otp-sends:${PHONE}`, [c.now() - 1_000]); + const guard = new OtpSendGuard({ + cooldownSeconds: 60, + now: c.now, + resolveStore: async () => cache as any, + }); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-auth/src/otp-send-guard.ts b/packages/plugins/plugin-auth/src/otp-send-guard.ts index 49aa585f86..4e943b9f41 100644 --- a/packages/plugins/plugin-auth/src/otp-send-guard.ts +++ b/packages/plugins/plugin-auth/src/otp-send-guard.ts @@ -13,31 +13,81 @@ * - **Cooldown**: at most one send per number per `cooldownSeconds`. * - **Hourly cap**: at most `maxPerHour` sends per number per rolling hour. * - * State lives in better-auth's `secondaryStorage` when wired (one shared - * store across nodes — same reasoning as the rate-limit counters, ADR-0069 - * D2) and falls back to an in-process map otherwise. Counting is - * best-effort (no cross-node atomicity), which is fine for an anti-abuse - * throttle — the budget is small either way. + * ## Where the budget is counted (#4790) + * + * A budget is only worth what its STORE is worth: counted per process, a + * declared "5 sends per number per hour" is really 5×N in an N-node + * deployment, and nothing says so (ADR-0049 — declared ≠ enforced). The store + * is therefore resolved through {@link CounterStore}, lazily, at the moment a + * send is checked: + * + * - the kernel `cache` service when one is registered — shared across nodes + * iff the cache is (Redis via `@objectstack/service-cache`); + * - a host-supplied better-auth `secondaryStorage`, when the host wired one + * deliberately (adapted by {@link counterStoreFromKv}); + * - otherwise a bounded per-process store — still enforced, per node, and + * announced loudly by the resolver (`createLazyCounterStore`). + * + * Lazy on purpose: `AuthPlugin.init()` runs BEFORE `CacheServicePlugin` + * registers `cache`, so anything resolved at init freezes a "no shared store" + * answer for the life of the process — the #4772 defect, of which this guard + * was the second instance. + * + * Counting stays best-effort (read-modify-write, no cross-node atomicity), + * which is fine for an anti-abuse throttle — the budget is small either way, + * and a shared store that occasionally admits one extra is still bounded, + * unlike N independent budgets. * * Keys carry only the phone number and timestamps — never OTP codes. */ +import { InProcessCounterStore, type CounterStore } from './rate-limit-storage.js'; + /** * Subset of better-auth's SecondaryStorage the guard needs. Return types are * deliberately loose (`unknown`) to stay assignable from better-auth's own - * interface across versions; `load()` type-checks what comes back. + * interface across versions; {@link parseHistory} type-checks what comes back. */ export interface OtpGuardStorage { get(key: string): unknown; set(key: string, value: string, ttl?: number): unknown; } +/** + * Adapt a string-valued KV (better-auth `secondaryStorage`) to the + * {@link CounterStore} the guard counts in, so there is ONE store abstraction + * inside the guard instead of a branch per backing store. + */ +export function counterStoreFromKv(kv: OtpGuardStorage): CounterStore { + return { + get: async (key: string): Promise => { + const raw = await kv.get(key); + return (raw ?? undefined) as T | undefined; + }, + set: async (key: string, value: T, ttl?: number): Promise => { + await kv.set(key, typeof value === 'string' ? value : JSON.stringify(value), ttl); + }, + }; +} + export interface OtpSendGuardOptions { /** Seconds a number must wait between two sends. Default 60. `0` disables. */ cooldownSeconds?: number; /** Max sends per number per rolling hour. Default 5. `0` disables. */ maxPerHour?: number; - /** Shared cross-node store (better-auth secondaryStorage). Optional. */ + /** + * Resolve the store the budget is counted in — called on EVERY check, so a + * shared cache that registers after this guard was constructed is picked up + * on the next send rather than never (#4790). `AuthPlugin` supplies + * `createLazyCounterStore(...)` (rate-limit-storage.ts), which memoises the + * handle, falls back to a bounded per-process store and says which of the + * two it got. Omitted → per-process, silently (the guard used standalone). + */ + resolveStore?: () => Promise; + /** + * Host-supplied cross-node KV (better-auth `secondaryStorage`). Adapted to a + * {@link CounterStore}; ignored when {@link resolveStore} is given. + */ storage?: OtpGuardStorage; /** Clock override for tests. */ now?: () => number; @@ -52,19 +102,49 @@ export interface OtpSendDecision { const KEY_PREFIX = 'phone-otp-sends:'; const HOUR_MS = 3_600_000; +/** + * Read back a send history. Cache adapters differ on whether a stored value + * comes back as the JSON string (Redis) or as the original array (memory), so + * both are accepted — the same tolerance `parseCounter` applies to the + * rate-limit envelope. Anything else (absent, foreign, unparseable) counts as + * an empty history: a budget that throws on a junk key would take sign-in down + * with it, which is the opposite of the trade this guard makes. + */ +function parseHistory(raw: unknown): number[] { + let value: unknown = raw; + if (typeof raw === 'string') { + if (raw.length === 0) return []; + try { + value = JSON.parse(raw); + } catch { + return []; + } + } + return Array.isArray(value) ? value.filter((t): t is number => typeof t === 'number') : []; +} + export class OtpSendGuard { private readonly cooldownMs: number; private readonly maxPerHour: number; - private readonly storage?: OtpGuardStorage; private readonly now: () => number; - /** In-process fallback store: phone → send timestamps (ms). */ - private readonly local = new Map(); + /** + * Per-process fallback, used only when no store was supplied at all (the + * guard constructed standalone). The SAME `InProcessCounterStore` the + * rate-limit counters degrade to — one bounded fallback implementation, not + * two (#4790). + */ + private readonly fallback = new InProcessCounterStore(); + /** Resolves the store this guard counts in, per check — see the options doc. */ + private readonly resolveStore: () => Promise; constructor(options: OtpSendGuardOptions = {}) { this.cooldownMs = Math.max(0, Math.floor(options.cooldownSeconds ?? 60)) * 1000; this.maxPerHour = Math.max(0, Math.floor(options.maxPerHour ?? 5)); - this.storage = options.storage; this.now = options.now ?? Date.now; + const hostKv = options.storage ? counterStoreFromKv(options.storage) : undefined; + this.resolveStore = + options.resolveStore ?? + (hostKv ? async () => hostKv : async () => this.fallback); } /** @@ -76,8 +156,9 @@ export class OtpSendGuard { if (this.cooldownMs === 0 && this.maxPerHour === 0) return { ok: true }; const now = this.now(); try { + const store = await this.resolveStore(); const key = KEY_PREFIX + phoneNumber; - const history = (await this.load(key)).filter((t) => now - t < HOUR_MS); + const history = parseHistory(await store.get(key)).filter((t) => now - t < HOUR_MS); const last = history.length ? Math.max(...history) : undefined; if (this.cooldownMs > 0 && last !== undefined && now - last < this.cooldownMs) { @@ -89,42 +170,11 @@ export class OtpSendGuard { } history.push(now); - await this.save(key, history); + // TTL = the rolling window; the entry self-expires once irrelevant. + await store.set(key, JSON.stringify(history), Math.ceil(HOUR_MS / 1000)); return { ok: true }; } catch { return { ok: true }; // fail open — see doc comment } } - - private async load(key: string): Promise { - if (this.storage) { - const raw = await this.storage.get(key); - if (typeof raw !== 'string' || raw.length === 0) return []; - try { - const parsed = JSON.parse(raw); - return Array.isArray(parsed) ? parsed.filter((t) => typeof t === 'number') : []; - } catch { - return []; - } - } - return this.local.get(key) ?? []; - } - - private async save(key: string, history: number[]): Promise { - if (this.storage) { - // TTL = the rolling window; the entry self-expires once irrelevant. - await this.storage.set(key, JSON.stringify(history), Math.ceil(HOUR_MS / 1000)); - return; - } - this.local.set(key, history); - // Opportunistic pruning keeps the fallback map bounded under abuse. - if (this.local.size > 10_000) { - const cutoff = this.now() - HOUR_MS; - for (const [k, v] of this.local) { - const alive = v.filter((t) => t > cutoff); - if (alive.length === 0) this.local.delete(k); - else this.local.set(k, alive); - } - } - } } diff --git a/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts b/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts index 71287cabfe..deffe3e218 100644 --- a/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts +++ b/packages/plugins/plugin-auth/src/rate-limit-storage.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, vi } from 'vitest'; import { createLazyCacheRateLimitStorage, + createLazyCounterStore, incrementFixedWindow, InProcessCounterStore, } from './rate-limit-storage.js'; @@ -211,6 +212,77 @@ describe('createLazyCacheRateLimitStorage — no cache service at all (#4772 acc }); }); +// #4790 — the resolution half, extracted so ObjectStack's OWN counters (the +// #2780 per-number OTP send budget) reach the shared cache through the same +// path as better-auth's, instead of a second copy of it. +describe('createLazyCounterStore (#4790 — one lazy resolution, many counters)', () => { + it('names the subject in both log lines, so two degraded budgets are distinguishable', async () => { + const logger = makeLogger(); + const resolve = createLazyCounterStore({ + resolveCache: async () => undefined, + logger, + subject: 'per-number OTP send budget (#2780)', + degradedImpact: 'an N-node deployment can send N× the configured PAID SMS', + }); + await resolve(); + const warned = logger.warn.mock.calls[0][0] as string; + expect(warned).toContain('per-number OTP send budget (#2780)'); + expect(warned).toContain('no `cache` service registered at all'); + expect(warned).toContain('N× the configured PAID SMS'); + + const bound = createLazyCounterStore({ + resolveCache: async () => makeCache() as any, + logger, + subject: 'per-number OTP send budget (#2780)', + }); + await bound(); + expect(logger.info.mock.calls[0][0]).toContain( + 'per-number OTP send budget (#2780) bound to the kernel cache service', + ); + }); + + it('hands back ONE fallback instance, so the degraded path still counts per node', async () => { + const resolve = createLazyCounterStore({ resolveCache: async () => undefined, subject: 'x' }); + const first = await resolve(); + const second = await resolve(); + expect(first).toBe(second); + expect(first).toBeInstanceOf(InProcessCounterStore); + }); + + it('keeps asking until the cache is there, then memoises it', async () => { + const cache = makeCache(); + let registered = false; + const resolveCache = vi.fn(async () => (registered ? (cache as any) : undefined)); + const resolve = createLazyCounterStore({ resolveCache, subject: 'x' }); + + expect(await resolve()).toBeInstanceOf(InProcessCounterStore); + registered = true; + expect(await resolve()).toBe(cache); + await resolve(); + // One miss + one hit; once resolved the handle is never looked up again. + expect(resolveCache).toHaveBeenCalledTimes(2); + }); + + it('warns once and survives a logger without warn/info', async () => { + const logger = makeLogger(); + const resolve = createLazyCounterStore({ resolveCache: async () => undefined, logger, subject: 'x' }); + await resolve(); + await resolve(); + expect(logger.warn).toHaveBeenCalledTimes(1); + + const bare = createLazyCounterStore({ resolveCache: async () => undefined, logger: {}, subject: 'x' }); + await expect(bare()).resolves.toBeInstanceOf(InProcessCounterStore); + }); + + it('treats a throwing resolver as "no shared store right now"', async () => { + const resolve = createLazyCounterStore({ + resolveCache: async () => { throw new Error('service registry exploded'); }, + subject: 'x', + }); + await expect(resolve()).resolves.toBeInstanceOf(InProcessCounterStore); + }); +}); + describe('InProcessCounterStore (the degraded path)', () => { it('expires entries on read', async () => { const store = new InProcessCounterStore(); diff --git a/packages/plugins/plugin-auth/src/rate-limit-storage.ts b/packages/plugins/plugin-auth/src/rate-limit-storage.ts index cfca359f8f..e17bc933a8 100644 --- a/packages/plugins/plugin-auth/src/rate-limit-storage.ts +++ b/packages/plugins/plugin-auth/src/rate-limit-storage.ts @@ -147,40 +147,46 @@ export interface LazyCacheRateLimitStorageOptions { logger?: LoggerLike; } +export interface LazyCounterStoreOptions extends LazyCacheRateLimitStorageOptions { + /** + * What is being counted, named verbatim in both log lines (e.g. + * `'rate-limit counters'`, `'per-number OTP send budget (#2780)'`). Two + * surfaces that degrade for the same reason must still be distinguishable in + * the log — an operator has to know WHICH budget stopped being global. + */ + subject: string; + /** + * Optional extra sentence appended to the degraded warning, naming what the + * per-process fallback actually costs for this surface. "Degraded" is not + * self-explanatory when the currency is paid SMS. + */ + degradedImpact?: string; +} + /** - * 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. + * ADR-0069 D2 — resolve the store a cross-node counter counts in, **lazily, at + * the moment the counter is consumed**, with a bounded per-process fallback and + * a one-shot announcement of whichever branch was taken. * - * 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. + * This is the shared half of {@link createLazyCacheRateLimitStorage} (#4772), + * extracted in #4790 so the OTP send budget rides the SAME resolution path + * rather than a second copy of it. Everything in the "why lazy" story below + * applies to any ObjectStack counter that wants to be global: plugin start + * order decides nothing, because nothing is resolved at start. * - * 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`. + * The returned function memoises the cache handle once it resolves, keeps + * exactly one in-process fallback per call site (so the degraded path still + * counts, per node), announces the bind once and warns about the degradation + * once. It never throws: a resolver that explodes is "no shared cache right + * now", asked again on the next count. */ -export function createLazyCacheRateLimitStorage( - opts: LazyCacheRateLimitStorageOptions, -): BetterAuthRateLimitStorage { +export function createLazyCounterStore(opts: LazyCounterStoreOptions): () => Promise { const fallback = new InProcessCounterStore(); let cache: CounterStore | undefined; let boundAnnounced = false; let degradedWarned = false; - const resolveStore = async (): Promise => { + return async (): Promise => { if (!cache) { try { cache = (await opts.resolveCache()) ?? undefined; @@ -190,7 +196,7 @@ export function createLazyCacheRateLimitStorage( 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)', + `[auth] ${opts.subject} bound to the kernel cache service — enforced against ONE store across nodes iff the cache is shared (ADR-0069 D2)`, ); } } @@ -198,13 +204,51 @@ export function createLazyCacheRateLimitStorage( 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. ' + + `[auth] ${opts.subject}: 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)', + '(Redis via @objectstack/service-cache) or each node enforces the limit independently (ADR-0069 D2)' + + (opts.degradedImpact ? `. ${opts.degradedImpact}` : ''), ); } return fallback; }; +} + +/** + * 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`. + * ObjectStack's own per-number OTP budget counts through the same + * {@link createLazyCounterStore} resolution, one layer down (#4790). + */ +export function createLazyCacheRateLimitStorage( + opts: LazyCacheRateLimitStorageOptions, +): BetterAuthRateLimitStorage { + const resolveStore = createLazyCounterStore({ + resolveCache: opts.resolveCache, + ...(opts.logger ? { logger: opts.logger } : {}), + subject: 'rate-limit counters', + }); return { consume: async (key, rule) => {