diff --git a/.changeset/auth-otp-cooldown-retention-follows-config.md b/.changeset/auth-otp-cooldown-retention-follows-config.md new file mode 100644 index 0000000000..517707b08c --- /dev/null +++ b/.changeset/auth-otp-cooldown-retention-follows-config.md @@ -0,0 +1,38 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): OTP 冷却按声明值真正生效 —— 发送历史的保留时长不再被硬编码的 1 小时截断 (#4808) + +`OtpSendGuard` 有**两个**维度:每号码「距上次发送至少 N 秒」的冷却(`cooldownSeconds`), +和每号码「滚动一小时内至多 M 条」的上限(`maxPerHour`)。它们需要**不同**的时间窗,而此前 +两者共用了同一个硬编码的一小时:发送历史按 1 小时剪枝、也按 1 小时写 TTL。 + +于是把 `phoneOtp.cooldownSeconds` 配成**大于 3600** 时:配置被接受,没有校验错误,没有 warn, +但冷却所依据的那条历史记录在 1 小时处就被丢掉了 —— 声明「两次发送间隔 2 小时」,实际最多 +只有 1 小时,**反滥用强度是声明值的一半,而且没有任何信号**(ADR-0049 声明 ≠ 强制)。 +计价单位仍然是真金白银的短信。这与 #4790 是同一个 guard 上的**不同**缺陷,且改动前后行为 +一致 —— 不是 #4806 引入的。 + +**修法(issue 的方向 1):保留时长跟随配置。** 历史保留 `max(1 小时, cooldownSeconds)`, +即「两个维度里还用得着它的那个更长的窗」;TTL 同步跟随,记录因此活得比它所度量的冷却更久。 +每小时上限仍在**它自己的滚动一小时**内计数,所以超长冷却不会反过来把 `maxPerHour` 收得比 +声明的更严。 + +**上限是拒绝,不是又一次截断。** `cooldownSeconds` 超过 `MAX_COOLDOWN_SECONDS`(86400, +即 24 小时)会在**启动时**抛错(`AuthPlugin.init()` 构造 `AuthManager` 处),错误信息给出 +值、上限和改法。把截断点挪到更高的数字只是把同一个缺陷往外推一个量级;设上限的理由是: +一条号码的历史会在共享缓存里驻留整个冷却期,而超过一天的封锁已经不是发送节流而是账号锁定 +(另一套机制、另一套管控)。这条边界同时把「`cooldownSeconds` 误填成毫秒」这类笔误变成 +一次响亮的拒绝(5 分钟以上的意图都会被挡下)。校验放在**配置处**而不是首次发送处:guard +是惰性构造的,只在那里校验的话,一个配置错误会表现为 `/phone-number/send-otp` 的 500。 + +**默认配置行为完全未变**,并有测试锁定:未配置 `phoneOtp` 时仍是 60 秒冷却 + 每小时 5 条, +历史保留与 TTL 仍是 3600 秒。 + +对使用者的影响: + +- `phoneOtp.cooldownSeconds` 现在在 1 小时以上也真正生效(上限 24 小时); +- 超过 24 小时、负数或非有限值的配置**开始被拒绝**——这些值此前从未按声明工作过(要么被 + 静默截断到 1 小时,要么被静默钳成 0 即关闭冷却),因此不存在依赖其旧行为的部署; +- 新增导出:常量 `MAX_COOLDOWN_SECONDS` 与校验函数 `assertOtpCooldownSeconds()`。 diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 04c8484550..5f9738dfa7 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1734,6 +1734,30 @@ describe('AuthManager', () => { await manager.assertPhoneOtpSendAllowed(PHONE); }); + // ── #4808 — a cooldown above one hour is enforced, or rejected ───────── + it('accepts a cooldown above one hour and enforces it (#4808)', async () => { + // Used to be accepted and then served as 1h, because the send history + // was pruned at a flat hour. The retention now follows the cooldown — + // the past-the-hour proof lives in otp-send-guard.test.ts, where the + // clock is injectable; here the point is that the config boots at all. + const { manager } = await bootOtp({ phoneOtp: { cooldownSeconds: 7_200 } }); + manager.setSmsService(fakeSms().service); + await manager.assertPhoneOtpSendAllowed(PHONE); + await expect(manager.assertPhoneOtpSendAllowed(PHONE)) + .rejects.toThrow(/Too many verification codes/); + }); + + it('rejects an unenforceable cooldown at BOOT, not at the first send (#4808)', () => { + // The guard is built lazily on first send, so validating only there + // would report a config error as a 500 on /phone-number/send-otp. + expect(() => new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + plugins: { phoneNumber: true }, + phoneOtp: { cooldownSeconds: 90_000 }, + } as any)).toThrow(/exceeds the supported maximum of 86400 seconds/); + }); + // ── #4790 — the budget is only global if its STORE is ───────────────── describe('where the per-number budget is counted (#4790)', () => { const makeCache = () => { diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 48ab9706f1..eec59b46b5 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -25,7 +25,7 @@ import { invitationRoleCapFailure, isPlainMemberInvitation } from './invitation- 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 { OtpSendGuard, assertOtpCooldownSeconds } from './otp-send-guard.js'; import type { CounterStore } from './rate-limit-storage.js'; import { PHONE_SMS_TOPICS, @@ -441,7 +441,11 @@ export interface AuthManagerOptions extends Partial { * real money (SMS pumping abuse — see otp-send-guard.ts). */ phoneOtp?: { - /** Per-number cooldown between sends, seconds. Default 60. `0` disables. */ + /** + * Per-number cooldown between sends, seconds. Default 60. `0` disables. + * Enforced at the declared length up to 24 hours (`MAX_COOLDOWN_SECONDS`); + * a longer value is REJECTED at construction, never truncated (#4808). + */ cooldownSeconds?: number; /** Per-number rolling-hour send cap. Default 5. `0` disables. */ maxPerHour?: number; @@ -714,6 +718,14 @@ export class AuthManager { constructor(config: AuthManagerOptions) { this.config = config; + // #4808 — reject an unenforceable OTP cooldown HERE, at boot + // (`AuthPlugin.init()` constructs this manager), rather than at the first + // send: the guard itself is built lazily, so without this the operator + // would learn about a bad throttle from a 500 on `/phone-number/send-otp`. + // Values within the bound are enforced at their declared length — the + // history retention follows the cooldown; see otp-send-guard.ts. + assertOtpCooldownSeconds(config.phoneOtp?.cooldownSeconds); + // WebContainer (StackBlitz) compatibility — install a synchronous // AsyncLocalStorage polyfill for better-auth's request-state global // BEFORE better-auth ever instantiates its own. See the helper for the 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 70490dfc70..ae9e01960a 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,12 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; -import { OtpSendGuard, type OtpGuardStorage } from './otp-send-guard.js'; +import { + OtpSendGuard, + MAX_COOLDOWN_SECONDS, + assertOtpCooldownSeconds, + type OtpGuardStorage, +} from './otp-send-guard.js'; import { createLazyCounterStore } from './rate-limit-storage.js'; const PHONE = '+8613800000000'; @@ -228,3 +233,163 @@ describe('OtpSendGuard — where the budget is counted (#4790)', () => { expect((await guard.checkAndRecord(PHONE)).ok).toBe(false); }); }); + +// ── #4808 — HOW LONG the history is kept ──────────────────────────────────── +// +// The history was pruned (and stored) at a flat one hour — the HOURLY CAP's +// window, borrowed for the cooldown. A `cooldownSeconds` above 3600 was +// therefore accepted and then served as one hour, because the record the +// cooldown measures from had already been dropped. Retention now follows +// `max(1h, cooldownSeconds)`; beyond MAX_COOLDOWN_SECONDS the config is +// rejected instead of truncated. +describe('OtpSendGuard — the cooldown is enforced at its declared length (#4808)', () => { + /** Store that records the TTL each write asked for. */ + const makeTtlStore = () => { + const store = new Map(); + const ttls: (number | undefined)[] = []; + return { + ttls, + store, + get: async (k: string) => (store.has(k) ? store.get(k) : undefined), + set: async (k: string, v: unknown, ttl?: number) => { + ttls.push(ttl); + store.set(k, v); + }, + }; + }; + + it('a 2-hour cooldown STILL rejects after the 1-hour mark — the defect itself', async () => { + const c = clock(); + const guard = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, now: c.now }); + + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + + // 59 minutes in: denied by anyone's reading. + c.advance(59 * 60_000); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(false); + + // Across the old hard-coded 1h boundary — where the history used to + // disappear and the "2 hour" cooldown quietly became one hour. + c.advance(2 * 60_000); // t = 61 min + const justPastTheHour = await guard.checkAndRecord(PHONE); + expect(justPastTheHour.ok).toBe(false); + // …and the retry window is honest about the remaining ~59 minutes. + expect(justPastTheHour.retryAfterSeconds).toBeGreaterThan(58 * 60); + expect(justPastTheHour.retryAfterSeconds).toBeLessThanOrEqual(59 * 60); + + // Still denied deep into the second hour. + c.advance(58 * 60_000); // t = 119 min + expect((await guard.checkAndRecord(PHONE)).ok).toBe(false); + + // Only the declared 2 hours frees the number. + c.advance(2 * 60_000); // t = 121 min + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + }); + + it('holds across nodes too — the long cooldown lives in the shared store', async () => { + const c = clock(); + const kv = new Map(); + const storage: OtpGuardStorage = { + get: (k) => kv.get(k) ?? null, + set: (k, v) => { kv.set(k, v); }, + }; + const nodeA = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, storage, now: c.now }); + const nodeB = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, storage, now: c.now }); + + expect((await nodeA.checkAndRecord(PHONE)).ok).toBe(true); + c.advance(70 * 60_000); // past one hour + expect((await nodeB.checkAndRecord(PHONE)).ok).toBe(false); + }); + + it('the stored TTL follows the cooldown, so the record outlives what it measures', async () => { + const c = clock(); + const long = makeTtlStore(); + const longGuard = new OtpSendGuard({ + cooldownSeconds: 7_200, + now: c.now, + resolveStore: async () => long as any, + }); + await longGuard.checkAndRecord(PHONE); + expect(long.ttls).toEqual([7_200]); // NOT 3600 — that was the truncation + + // A cooldown under an hour keeps the rolling-hour retention the cap needs. + const short = makeTtlStore(); + const shortGuard = new OtpSendGuard({ + cooldownSeconds: 60, + now: c.now, + resolveStore: async () => short as any, + }); + await shortGuard.checkAndRecord(PHONE); + expect(short.ttls).toEqual([3_600]); + }); + + it('a long cooldown does not tighten the hourly cap (its window stays one hour)', async () => { + const c = clock(); + // Cooldown 90 min, cap 2/hour. The cap must keep counting over its OWN + // hour: sends are ≥90 min apart, so it must never be the reason for a deny. + const guard = new OtpSendGuard({ cooldownSeconds: 5_400, maxPerHour: 2, now: c.now }); + for (let i = 0; i < 4; i++) { + const d = await guard.checkAndRecord(PHONE); + expect(d.ok).toBe(true); + c.advance(91 * 60_000); + } + }); + + // ── the bound is a rejection, not a higher truncation point ─────────────── + + it('rejects a cooldown above the supported maximum instead of truncating it', async () => { + expect(MAX_COOLDOWN_SECONDS).toBe(86_400); + expect(() => new OtpSendGuard({ cooldownSeconds: MAX_COOLDOWN_SECONDS + 1 })).toThrow( + /exceeds the supported maximum of 86400 seconds/, + ); + // `cooldownSeconds` handed over in milliseconds (here: "5 minutes"). + expect(() => new OtpSendGuard({ cooldownSeconds: 300_000 })).toThrow( + /If the value is in milliseconds, divide by 1000/, + ); + // Exactly at the bound is fine. + expect(() => new OtpSendGuard({ cooldownSeconds: MAX_COOLDOWN_SECONDS })).not.toThrow(); + }); + + it('rejects a cooldown that is not a usable number of seconds', () => { + for (const bad of [-1, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(() => assertOtpCooldownSeconds(bad)).toThrow( + /finite, non-negative number of seconds/, + ); + } + // `undefined` (use the default) and `0` (documented: disables) stay valid. + expect(() => assertOtpCooldownSeconds(undefined)).not.toThrow(); + expect(() => assertOtpCooldownSeconds(0)).not.toThrow(); + }); + + // ── acceptance #2: the default path is untouched ────────────────────────── + + it('DEFAULT config is unchanged: 60s cooldown, 5 per rolling hour, 1h retention', async () => { + const c = clock(); + const store = makeTtlStore(); + // No cooldownSeconds / maxPerHour at all — exactly what a host that never + // configures `phoneOtp` gets. + const guard = new OtpSendGuard({ now: c.now, resolveStore: async () => store as any }); + + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + const denied = await guard.checkAndRecord(PHONE); + expect(denied.ok).toBe(false); + expect(denied.retryAfterSeconds).toBeLessThanOrEqual(60); // the 60s default + // Retention is still the rolling hour the hourly cap needs. + expect(store.ttls[0]).toBe(3_600); + + // 4 more sends, one per minute → the 5/hour default is reached, not 6. + for (let i = 0; i < 4; i++) { + c.advance(61_000); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + } + c.advance(61_000); + const capped = await guard.checkAndRecord(PHONE); + expect(capped.ok).toBe(false); + expect(capped.retryAfterSeconds).toBeGreaterThan(60); // the hour, not the cooldown + + // An hour after the first send the window rolls and a slot frees up. + c.advance(3_600_000 - 5 * 61_000); + expect((await guard.checkAndRecord(PHONE)).ok).toBe(true); + expect(store.ttls.every((t) => t === 3_600)).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-auth/src/otp-send-guard.ts b/packages/plugins/plugin-auth/src/otp-send-guard.ts index 4e943b9f41..be87b6122f 100644 --- a/packages/plugins/plugin-auth/src/otp-send-guard.ts +++ b/packages/plugins/plugin-auth/src/otp-send-guard.ts @@ -13,6 +13,28 @@ * - **Cooldown**: at most one send per number per `cooldownSeconds`. * - **Hourly cap**: at most `maxPerHour` sends per number per rolling hour. * + * ## How long the history is kept (#4808) + * + * The two dimensions above need DIFFERENT windows, and conflating them is how + * a declared cooldown stopped being the enforced one: the history was pruned — + * and stored with a TTL — at a flat one hour, which is the hourly cap's window, + * not the cooldown's. A `cooldownSeconds` above 3600 was therefore accepted, + * then silently served as one hour: the record the cooldown is measured from + * had already been dropped. Declared ≠ enforced, with no signal at all + * (ADR-0049). + * + * History is now retained for `max(1 hour, cooldownSeconds)` — long enough for + * whichever dimension still needs it — while the hourly cap keeps counting over + * its own rolling hour ({@link HOUR_MS}), so a long cooldown does not quietly + * make `maxPerHour` stricter than declared either. + * + * The retention that buys is bounded by {@link MAX_COOLDOWN_SECONDS}: a + * cooldown above it is **rejected** at config time + * ({@link assertOtpCooldownSeconds}), never truncated. A ceiling matters more + * once the TTL follows the config than it did before: a nonsense value used to + * degrade quietly to one hour, and would now be honoured for as long as it + * says. Rejecting it says so at boot instead. + * * ## Where the budget is counted (#4790) * * A budget is only worth what its STORE is worth: counted per process, a @@ -71,7 +93,13 @@ export function counterStoreFromKv(kv: OtpGuardStorage): CounterStore { } export interface OtpSendGuardOptions { - /** Seconds a number must wait between two sends. Default 60. `0` disables. */ + /** + * Seconds a number must wait between two sends. Default 60. `0` disables. + * Enforced at the declared value — including above one hour, which the + * history retention now follows (#4808). Must be a finite, non-negative + * number no greater than {@link MAX_COOLDOWN_SECONDS}; anything else throws + * from the constructor rather than being clamped into something else. + */ cooldownSeconds?: number; /** Max sends per number per rolling hour. Default 5. `0` disables. */ maxPerHour?: number; @@ -100,8 +128,55 @@ export interface OtpSendDecision { } const KEY_PREFIX = 'phone-otp-sends:'; +/** The hourly cap's window. Belongs to `maxPerHour` ONLY — see the file doc. */ const HOUR_MS = 3_600_000; +/** + * Upper bound on `cooldownSeconds`, in seconds (24 hours). + * + * Not a truncation point — a configured cooldown above this is **rejected** + * ({@link assertOtpCooldownSeconds}); moving the silent truncation to a higher + * number would just be the #4808 defect one order of magnitude further out. + * + * Why a ceiling exists at all, now that retention follows the cooldown: the + * history for one number is pinned in the shared cache for the whole cooldown, + * and a cooldown beyond a day is not an anti-abuse throttle any more — it is an + * account-level lockout, which is a different mechanism with different controls. + * It also catches `cooldownSeconds` handed over in MILLISECONDS — the common + * authoring slip — for any intended cooldown from five minutes up (`300000` + * → rejected), which is where the mistake stops being self-evident. + */ +export const MAX_COOLDOWN_SECONDS = 86_400; + +/** + * Validate a configured OTP send cooldown, throwing with the offending value, + * the bound and the fix. Exported so the value is rejected where it is + * CONFIGURED (`AuthManager`'s constructor, i.e. `AuthPlugin.init()` → boot) + * as well as where it becomes behaviour (the guard constructor) — one message, + * both seams, no second copy of the rule. + * + * Accepts `undefined` (use the default) and `0` (documented: disables the + * cooldown). Fractional seconds are floored by the guard — sub-second + * precision on an SMS throttle is not a discrepancy worth a boot failure. + */ +export function assertOtpCooldownSeconds(seconds: number | undefined): void { + if (seconds === undefined) return; + if (typeof seconds !== 'number' || !Number.isFinite(seconds) || seconds < 0) { + throw new RangeError( + `phoneOtp.cooldownSeconds must be a finite, non-negative number of seconds (got ${String(seconds)}). ` + + 'Use 0 to disable the per-number cooldown.', + ); + } + if (seconds > MAX_COOLDOWN_SECONDS) { + throw new RangeError( + `phoneOtp.cooldownSeconds ${seconds} exceeds the supported maximum of ${MAX_COOLDOWN_SECONDS} seconds (24 hours). ` + + 'The per-number OTP send history is retained for the whole cooldown, so the cooldown is bounded rather than ' + + 'silently truncated (#4808). If the value is in milliseconds, divide by 1000; a longer block than 24 hours is ' + + 'an account lockout, not a send throttle.', + ); + } +} + /** * 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 @@ -126,6 +201,13 @@ function parseHistory(raw: unknown): number[] { export class OtpSendGuard { private readonly cooldownMs: number; private readonly maxPerHour: number; + /** + * How long a send stays in the history — `max(1 hour, cooldownMs)` (#4808). + * The hourly cap still counts over {@link HOUR_MS}; this is only about how + * long the record has to SURVIVE for the longer of the two dimensions to be + * measurable at all. + */ + private readonly retentionMs: number; private readonly now: () => number; /** * Per-process fallback, used only when no store was supplied at all (the @@ -138,8 +220,10 @@ export class OtpSendGuard { private readonly resolveStore: () => Promise; constructor(options: OtpSendGuardOptions = {}) { + assertOtpCooldownSeconds(options.cooldownSeconds); this.cooldownMs = Math.max(0, Math.floor(options.cooldownSeconds ?? 60)) * 1000; this.maxPerHour = Math.max(0, Math.floor(options.maxPerHour ?? 5)); + this.retentionMs = Math.max(HOUR_MS, this.cooldownMs); this.now = options.now ?? Date.now; const hostKv = options.storage ? counterStoreFromKv(options.storage) : undefined; this.resolveStore = @@ -158,20 +242,30 @@ export class OtpSendGuard { try { const store = await this.resolveStore(); const key = KEY_PREFIX + phoneNumber; - const history = parseHistory(await store.get(key)).filter((t) => now - t < HOUR_MS); + // Retained for the LONGER of the two windows (#4808) … + const history = parseHistory(await store.get(key)).filter((t) => now - t < this.retentionMs); + // … but the hourly cap counts only its own rolling hour, so a cooldown + // above an hour can never make `maxPerHour` stricter than it was + // declared. Belt and braces today (a history entry inside the wider + // retention window is also inside the cooldown, which returns below + // before the cap is consulted) — stated explicitly so the cap's window + // does not silently become "whatever the cooldown retains" the next time + // either window moves. + const withinHour = history.filter((t) => now - t < HOUR_MS); const last = history.length ? Math.max(...history) : undefined; if (this.cooldownMs > 0 && last !== undefined && now - last < this.cooldownMs) { return { ok: false, retryAfterSeconds: Math.ceil((this.cooldownMs - (now - last)) / 1000) }; } - if (this.maxPerHour > 0 && history.length >= this.maxPerHour) { - const oldest = Math.min(...history); + if (this.maxPerHour > 0 && withinHour.length >= this.maxPerHour) { + const oldest = Math.min(...withinHour); return { ok: false, retryAfterSeconds: Math.ceil((HOUR_MS - (now - oldest)) / 1000) }; } history.push(now); - // TTL = the rolling window; the entry self-expires once irrelevant. - await store.set(key, JSON.stringify(history), Math.ceil(HOUR_MS / 1000)); + // TTL = the retention window, so the entry outlives the cooldown it is + // measured against and self-expires once no dimension can still need it. + await store.set(key, JSON.stringify(history), Math.ceil(this.retentionMs / 1000)); return { ok: true }; } catch { return { ok: true }; // fail open — see doc comment