Skip to content

Commit 5046afe

Browse files
os-zhuangclaude
andauthored
fix(plugin-auth): retain the OTP send history for as long as the cooldown it measures (#4808) (#4869)
`OtpSendGuard` enforces two dimensions with two different windows — a per-number cooldown (`cooldownSeconds`) and a per-number rolling-hour cap (`maxPerHour`) — but both were pruned, and stored with a TTL, at a flat one hour: the cap's window, borrowed for the cooldown. So `phoneOtp.cooldownSeconds` above 3600 was accepted, with no validation error and no warning, and then served as one hour, because the record the cooldown is measured from had already been dropped. A declared 2-hour cooldown was really 1 hour — half the declared anti-abuse strength on a PAID channel, silently (ADR-0049, declared != enforced). Same guard as #4790, a different defect; behaviour identical before and after #4806. History is now retained for `max(1 hour, cooldownSeconds)` — the longer of the two windows — with the TTL following it, so the entry outlives what it measures. The hourly cap keeps counting over its own rolling hour, so a long cooldown cannot make `maxPerHour` stricter than declared either. The bound is a rejection, not a higher truncation point: `cooldownSeconds` over MAX_COOLDOWN_SECONDS (86400 / 24h), negative or non-finite throws from `assertOtpCooldownSeconds()`, called from the `AuthManager` constructor so a bad config fails at boot rather than as a 500 on the first `/phone-number/send-otp`. Moving the truncation further out would only be the same defect one order of magnitude away. Default config is unchanged and pinned by a test: 60s cooldown, 5 per rolling hour, 3600s retention and TTL. Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny Co-authored-by: Claude <noreply@anthropic.com>
1 parent 61cc079 commit 5046afe

5 files changed

Lines changed: 342 additions & 9 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
fix(plugin-auth): OTP 冷却按声明值真正生效 —— 发送历史的保留时长不再被硬编码的 1 小时截断 (#4808)
6+
7+
`OtpSendGuard`**两个**维度:每号码「距上次发送至少 N 秒」的冷却(`cooldownSeconds`),
8+
和每号码「滚动一小时内至多 M 条」的上限(`maxPerHour`)。它们需要**不同**的时间窗,而此前
9+
两者共用了同一个硬编码的一小时:发送历史按 1 小时剪枝、也按 1 小时写 TTL。
10+
11+
于是把 `phoneOtp.cooldownSeconds` 配成**大于 3600** 时:配置被接受,没有校验错误,没有 warn,
12+
但冷却所依据的那条历史记录在 1 小时处就被丢掉了 —— 声明「两次发送间隔 2 小时」,实际最多
13+
只有 1 小时,**反滥用强度是声明值的一半,而且没有任何信号**(ADR-0049 声明 ≠ 强制)。
14+
计价单位仍然是真金白银的短信。这与 #4790 是同一个 guard 上的**不同**缺陷,且改动前后行为
15+
一致 —— 不是 #4806 引入的。
16+
17+
**修法(issue 的方向 1):保留时长跟随配置。** 历史保留 `max(1 小时, cooldownSeconds)`,
18+
即「两个维度里还用得着它的那个更长的窗」;TTL 同步跟随,记录因此活得比它所度量的冷却更久。
19+
每小时上限仍在**它自己的滚动一小时**内计数,所以超长冷却不会反过来把 `maxPerHour` 收得比
20+
声明的更严。
21+
22+
**上限是拒绝,不是又一次截断。** `cooldownSeconds` 超过 `MAX_COOLDOWN_SECONDS`(86400,
23+
即 24 小时)会在**启动时**抛错(`AuthPlugin.init()` 构造 `AuthManager` 处),错误信息给出
24+
值、上限和改法。把截断点挪到更高的数字只是把同一个缺陷往外推一个量级;设上限的理由是:
25+
一条号码的历史会在共享缓存里驻留整个冷却期,而超过一天的封锁已经不是发送节流而是账号锁定
26+
(另一套机制、另一套管控)。这条边界同时把「`cooldownSeconds` 误填成毫秒」这类笔误变成
27+
一次响亮的拒绝(5 分钟以上的意图都会被挡下)。校验放在**配置处**而不是首次发送处:guard
28+
是惰性构造的,只在那里校验的话,一个配置错误会表现为 `/phone-number/send-otp` 的 500。
29+
30+
**默认配置行为完全未变**,并有测试锁定:未配置 `phoneOtp` 时仍是 60 秒冷却 + 每小时 5 条,
31+
历史保留与 TTL 仍是 3600 秒。
32+
33+
对使用者的影响:
34+
35+
- `phoneOtp.cooldownSeconds` 现在在 1 小时以上也真正生效(上限 24 小时);
36+
- 超过 24 小时、负数或非有限值的配置**开始被拒绝**——这些值此前从未按声明工作过(要么被
37+
静默截断到 1 小时,要么被静默钳成 0 即关闭冷却),因此不存在依赖其旧行为的部署;
38+
- 新增导出:常量 `MAX_COOLDOWN_SECONDS` 与校验函数 `assertOtpCooldownSeconds()`

packages/plugins/plugin-auth/src/auth-manager.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,6 +1734,30 @@ describe('AuthManager', () => {
17341734
await manager.assertPhoneOtpSendAllowed(PHONE);
17351735
});
17361736

1737+
// ── #4808 — a cooldown above one hour is enforced, or rejected ─────────
1738+
it('accepts a cooldown above one hour and enforces it (#4808)', async () => {
1739+
// Used to be accepted and then served as 1h, because the send history
1740+
// was pruned at a flat hour. The retention now follows the cooldown —
1741+
// the past-the-hour proof lives in otp-send-guard.test.ts, where the
1742+
// clock is injectable; here the point is that the config boots at all.
1743+
const { manager } = await bootOtp({ phoneOtp: { cooldownSeconds: 7_200 } });
1744+
manager.setSmsService(fakeSms().service);
1745+
await manager.assertPhoneOtpSendAllowed(PHONE);
1746+
await expect(manager.assertPhoneOtpSendAllowed(PHONE))
1747+
.rejects.toThrow(/Too many verification codes/);
1748+
});
1749+
1750+
it('rejects an unenforceable cooldown at BOOT, not at the first send (#4808)', () => {
1751+
// The guard is built lazily on first send, so validating only there
1752+
// would report a config error as a 500 on /phone-number/send-otp.
1753+
expect(() => new AuthManager({
1754+
secret: 'test-secret-at-least-32-chars-long',
1755+
baseUrl: 'http://localhost:3000',
1756+
plugins: { phoneNumber: true },
1757+
phoneOtp: { cooldownSeconds: 90_000 },
1758+
} as any)).toThrow(/exceeds the supported maximum of 86400 seconds/);
1759+
});
1760+
17371761
// ── #4790 — the budget is only global if its STORE is ─────────────────
17381762
describe('where the per-number budget is counted (#4790)', () => {
17391763
const makeCache = () => {

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import { invitationRoleCapFailure, isPlainMemberInvitation } from './invitation-
2525
import { isPlaceholderEmail } from './placeholder-email.js';
2626
import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js';
2727
import type { TenancyService } from './tenancy-service.js';
28-
import { OtpSendGuard } from './otp-send-guard.js';
28+
import { OtpSendGuard, assertOtpCooldownSeconds } from './otp-send-guard.js';
2929
import type { CounterStore } from './rate-limit-storage.js';
3030
import {
3131
PHONE_SMS_TOPICS,
@@ -441,7 +441,11 @@ export interface AuthManagerOptions extends Partial<AuthConfig> {
441441
* real money (SMS pumping abuse — see otp-send-guard.ts).
442442
*/
443443
phoneOtp?: {
444-
/** Per-number cooldown between sends, seconds. Default 60. `0` disables. */
444+
/**
445+
* Per-number cooldown between sends, seconds. Default 60. `0` disables.
446+
* Enforced at the declared length up to 24 hours (`MAX_COOLDOWN_SECONDS`);
447+
* a longer value is REJECTED at construction, never truncated (#4808).
448+
*/
445449
cooldownSeconds?: number;
446450
/** Per-number rolling-hour send cap. Default 5. `0` disables. */
447451
maxPerHour?: number;
@@ -714,6 +718,14 @@ export class AuthManager {
714718
constructor(config: AuthManagerOptions) {
715719
this.config = config;
716720

721+
// #4808 — reject an unenforceable OTP cooldown HERE, at boot
722+
// (`AuthPlugin.init()` constructs this manager), rather than at the first
723+
// send: the guard itself is built lazily, so without this the operator
724+
// would learn about a bad throttle from a 500 on `/phone-number/send-otp`.
725+
// Values within the bound are enforced at their declared length — the
726+
// history retention follows the cooldown; see otp-send-guard.ts.
727+
assertOtpCooldownSeconds(config.phoneOtp?.cooldownSeconds);
728+
717729
// WebContainer (StackBlitz) compatibility — install a synchronous
718730
// AsyncLocalStorage polyfill for better-auth's request-state global
719731
// BEFORE better-auth ever instantiates its own. See the helper for the

packages/plugins/plugin-auth/src/otp-send-guard.test.ts

Lines changed: 166 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { describe, it, expect, vi } from 'vitest';
4-
import { OtpSendGuard, type OtpGuardStorage } from './otp-send-guard.js';
4+
import {
5+
OtpSendGuard,
6+
MAX_COOLDOWN_SECONDS,
7+
assertOtpCooldownSeconds,
8+
type OtpGuardStorage,
9+
} from './otp-send-guard.js';
510
import { createLazyCounterStore } from './rate-limit-storage.js';
611

712
const PHONE = '+8613800000000';
@@ -228,3 +233,163 @@ describe('OtpSendGuard — where the budget is counted (#4790)', () => {
228233
expect((await guard.checkAndRecord(PHONE)).ok).toBe(false);
229234
});
230235
});
236+
237+
// ── #4808 — HOW LONG the history is kept ────────────────────────────────────
238+
//
239+
// The history was pruned (and stored) at a flat one hour — the HOURLY CAP's
240+
// window, borrowed for the cooldown. A `cooldownSeconds` above 3600 was
241+
// therefore accepted and then served as one hour, because the record the
242+
// cooldown measures from had already been dropped. Retention now follows
243+
// `max(1h, cooldownSeconds)`; beyond MAX_COOLDOWN_SECONDS the config is
244+
// rejected instead of truncated.
245+
describe('OtpSendGuard — the cooldown is enforced at its declared length (#4808)', () => {
246+
/** Store that records the TTL each write asked for. */
247+
const makeTtlStore = () => {
248+
const store = new Map<string, unknown>();
249+
const ttls: (number | undefined)[] = [];
250+
return {
251+
ttls,
252+
store,
253+
get: async (k: string) => (store.has(k) ? store.get(k) : undefined),
254+
set: async (k: string, v: unknown, ttl?: number) => {
255+
ttls.push(ttl);
256+
store.set(k, v);
257+
},
258+
};
259+
};
260+
261+
it('a 2-hour cooldown STILL rejects after the 1-hour mark — the defect itself', async () => {
262+
const c = clock();
263+
const guard = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, now: c.now });
264+
265+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(true);
266+
267+
// 59 minutes in: denied by anyone's reading.
268+
c.advance(59 * 60_000);
269+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(false);
270+
271+
// Across the old hard-coded 1h boundary — where the history used to
272+
// disappear and the "2 hour" cooldown quietly became one hour.
273+
c.advance(2 * 60_000); // t = 61 min
274+
const justPastTheHour = await guard.checkAndRecord(PHONE);
275+
expect(justPastTheHour.ok).toBe(false);
276+
// …and the retry window is honest about the remaining ~59 minutes.
277+
expect(justPastTheHour.retryAfterSeconds).toBeGreaterThan(58 * 60);
278+
expect(justPastTheHour.retryAfterSeconds).toBeLessThanOrEqual(59 * 60);
279+
280+
// Still denied deep into the second hour.
281+
c.advance(58 * 60_000); // t = 119 min
282+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(false);
283+
284+
// Only the declared 2 hours frees the number.
285+
c.advance(2 * 60_000); // t = 121 min
286+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(true);
287+
});
288+
289+
it('holds across nodes too — the long cooldown lives in the shared store', async () => {
290+
const c = clock();
291+
const kv = new Map<string, string>();
292+
const storage: OtpGuardStorage = {
293+
get: (k) => kv.get(k) ?? null,
294+
set: (k, v) => { kv.set(k, v); },
295+
};
296+
const nodeA = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, storage, now: c.now });
297+
const nodeB = new OtpSendGuard({ cooldownSeconds: 7_200, maxPerHour: 0, storage, now: c.now });
298+
299+
expect((await nodeA.checkAndRecord(PHONE)).ok).toBe(true);
300+
c.advance(70 * 60_000); // past one hour
301+
expect((await nodeB.checkAndRecord(PHONE)).ok).toBe(false);
302+
});
303+
304+
it('the stored TTL follows the cooldown, so the record outlives what it measures', async () => {
305+
const c = clock();
306+
const long = makeTtlStore();
307+
const longGuard = new OtpSendGuard({
308+
cooldownSeconds: 7_200,
309+
now: c.now,
310+
resolveStore: async () => long as any,
311+
});
312+
await longGuard.checkAndRecord(PHONE);
313+
expect(long.ttls).toEqual([7_200]); // NOT 3600 — that was the truncation
314+
315+
// A cooldown under an hour keeps the rolling-hour retention the cap needs.
316+
const short = makeTtlStore();
317+
const shortGuard = new OtpSendGuard({
318+
cooldownSeconds: 60,
319+
now: c.now,
320+
resolveStore: async () => short as any,
321+
});
322+
await shortGuard.checkAndRecord(PHONE);
323+
expect(short.ttls).toEqual([3_600]);
324+
});
325+
326+
it('a long cooldown does not tighten the hourly cap (its window stays one hour)', async () => {
327+
const c = clock();
328+
// Cooldown 90 min, cap 2/hour. The cap must keep counting over its OWN
329+
// hour: sends are ≥90 min apart, so it must never be the reason for a deny.
330+
const guard = new OtpSendGuard({ cooldownSeconds: 5_400, maxPerHour: 2, now: c.now });
331+
for (let i = 0; i < 4; i++) {
332+
const d = await guard.checkAndRecord(PHONE);
333+
expect(d.ok).toBe(true);
334+
c.advance(91 * 60_000);
335+
}
336+
});
337+
338+
// ── the bound is a rejection, not a higher truncation point ───────────────
339+
340+
it('rejects a cooldown above the supported maximum instead of truncating it', async () => {
341+
expect(MAX_COOLDOWN_SECONDS).toBe(86_400);
342+
expect(() => new OtpSendGuard({ cooldownSeconds: MAX_COOLDOWN_SECONDS + 1 })).toThrow(
343+
/exceeds the supported maximum of 86400 seconds/,
344+
);
345+
// `cooldownSeconds` handed over in milliseconds (here: "5 minutes").
346+
expect(() => new OtpSendGuard({ cooldownSeconds: 300_000 })).toThrow(
347+
/If the value is in milliseconds, divide by 1000/,
348+
);
349+
// Exactly at the bound is fine.
350+
expect(() => new OtpSendGuard({ cooldownSeconds: MAX_COOLDOWN_SECONDS })).not.toThrow();
351+
});
352+
353+
it('rejects a cooldown that is not a usable number of seconds', () => {
354+
for (const bad of [-1, Number.NaN, Number.POSITIVE_INFINITY]) {
355+
expect(() => assertOtpCooldownSeconds(bad)).toThrow(
356+
/finite, non-negative number of seconds/,
357+
);
358+
}
359+
// `undefined` (use the default) and `0` (documented: disables) stay valid.
360+
expect(() => assertOtpCooldownSeconds(undefined)).not.toThrow();
361+
expect(() => assertOtpCooldownSeconds(0)).not.toThrow();
362+
});
363+
364+
// ── acceptance #2: the default path is untouched ──────────────────────────
365+
366+
it('DEFAULT config is unchanged: 60s cooldown, 5 per rolling hour, 1h retention', async () => {
367+
const c = clock();
368+
const store = makeTtlStore();
369+
// No cooldownSeconds / maxPerHour at all — exactly what a host that never
370+
// configures `phoneOtp` gets.
371+
const guard = new OtpSendGuard({ now: c.now, resolveStore: async () => store as any });
372+
373+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(true);
374+
const denied = await guard.checkAndRecord(PHONE);
375+
expect(denied.ok).toBe(false);
376+
expect(denied.retryAfterSeconds).toBeLessThanOrEqual(60); // the 60s default
377+
// Retention is still the rolling hour the hourly cap needs.
378+
expect(store.ttls[0]).toBe(3_600);
379+
380+
// 4 more sends, one per minute → the 5/hour default is reached, not 6.
381+
for (let i = 0; i < 4; i++) {
382+
c.advance(61_000);
383+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(true);
384+
}
385+
c.advance(61_000);
386+
const capped = await guard.checkAndRecord(PHONE);
387+
expect(capped.ok).toBe(false);
388+
expect(capped.retryAfterSeconds).toBeGreaterThan(60); // the hour, not the cooldown
389+
390+
// An hour after the first send the window rolls and a slot frees up.
391+
c.advance(3_600_000 - 5 * 61_000);
392+
expect((await guard.checkAndRecord(PHONE)).ok).toBe(true);
393+
expect(store.ttls.every((t) => t === 3_600)).toBe(true);
394+
});
395+
});

0 commit comments

Comments
 (0)