fix(plugin-auth): OTP 冷却按声明值真正生效 —— 发送历史保留时长不再被硬编码 1 小时截断 (#4808)

.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()`。

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 = () => {

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<AuthConfig> {
    * 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

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<string, unknown>();
+    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<string, string>();
+    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);
+  });
+});