Skip to content

Commit e7cf3b8

Browse files
committed
fix(plugin-auth): SMS daily-quota refusal answers 429, not 500 (#6039)
`SmsService.send()` reports the deployment's daily SMS quota being hit as a failed result carrying the service's `CODE: message` envelope (`TOO_MANY_REQUESTS: daily SMS quota exhausted`, #2814) — it is a kernel service and cannot throw an HTTP-shaped error. `AuthManager` never turned that back into one: `deliverPhoneOtp()` / `sendPhoneInviteSms()` rethrew every `status === 'failed'` as a plain `Error`, and better-call maps only `APIError` to a real status (`isAPIError`, better-call@1.3.7 dist/utils.mjs:57 → dist/router.mjs:93), so the caller got 500 with a null body while the per-number wall on the same endpoint answered 429. Both failure branches now recognise the `TOO_MANY_REQUESTS:` prefix and throw `APIError('TOO_MANY_REQUESTS')`. Only the ADR-0112 code crosses the package boundary — restated locally, because `@objectstack/service-sms` already depends on plugin-auth (mirror of `normalizeSmsRecipient` there). Transport failures keep their plain `Error` / 500 semantics, and the 429 message carries no ceiling, remaining count or reset clock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv
1 parent e3ef52b commit e7cf3b8

3 files changed

Lines changed: 280 additions & 2 deletions

File tree

.changeset/otp-sms-quota-429.md

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
fix(plugin-auth): 短信日配额拒发时,OTP / 邀请短信按 429 TOO_MANY_REQUESTS 作答,不再是 500 (#6039)
6+
7+
#2814 把短信总量成本闸落在 `SmsService.send()` —— 它是内核服务,不知道调用方是谁,
8+
所以超限时**返回**一条失败结果,把码写在服务层既有的 `CODE: message` 信封上:
9+
`TOO_MANY_REQUESTS: daily SMS quota exhausted`。把 HTTP 语义还原回去是 auth 端点的
10+
职责,而 `AuthManager` 此前没有做:`deliverPhoneOtp()` / `sendPhoneInviteSms()`
11+
任何 `status === 'failed'` 一律抛普通 `Error`
12+
13+
better-auth 的路由层 better-call 只把 `APIError` 映射成真实状态码
14+
(`isAPIError = err instanceof APIError || err?.name === 'APIError'`,
15+
better-call@1.3.7 `dist/utils.mjs:57`,消费点在 `dist/router.mjs:93`),其余一律走
16+
`console.error` + **500、响应体 `null`** 的分支。于是配额拒发对外是 500,
17+
`TOO_MANY_REQUESTS` 只留在服务端日志里;而**同一个端点**上按号码冷却闸
18+
(`assertPhoneOtpSendAllowed`,在 admission hook 里)抛的是
19+
`APIError('TOO_MANY_REQUESTS')`,正常回 429 —— 一个端点两种口径,正是 #2814
20+
「两道墙从外面看应当一样」的反面。
21+
22+
现在两处失败分支都先识别信封上的 `TOO_MANY_REQUESTS:` **前缀**,改抛
23+
`APIError('TOO_MANY_REQUESTS')`:
24+
25+
- **只有码跨包**。识别用的 `TOO_MANY_REQUESTS` 在 plugin-auth 本地写死并注明出处
26+
(`SMS_QUOTA_EXCEEDED_CODE`,`packages/services/service-sms/src/sms-daily-quota.ts`)——
27+
`@objectstack/service-sms` 已经依赖本包(它的日计数器从这里 import
28+
`InProcessCounterStore`),反向 import 会成环;这与 service-sms 里
29+
`normalizeSmsRecipient` 就地重述 plugin-auth 形状规则是同一个取舍的另一半。
30+
跨包重述的只是一个 ADR-0112 闭集错误码,冒号后的措辞归服务层所有,可以自由改写。
31+
- **不泄露预算**。429 文案沿按号码闸的措辞形状,不含上限、剩余量与重置时刻
32+
(按号码闸报自己的重试窗口,是因为它算得出;配额闸不承诺它给不出的时间)。
33+
- **不顺手收紧**。传输故障(provider 宕机等)仍抛普通 `Error`,500 语义原样不变;
34+
仅仅在文中提到该码而不以之开头的 provider 报错同样保持 500。
35+
36+
对外可见的变化:`POST /phone-number/send-otp`
37+
`POST /phone-number/request-password-reset` 在部署日配额耗尽时,由
38+
**500 + 空响应体**变为 **429 TOO_MANY_REQUESTS**,与按号码冷却闸同形。
39+
邀请短信路径同样返回 `APIError`;仓内唯一调用方(admin import-users)按行捕获它并
40+
记为 `INVITE_SMS_FAILED`,该路径的变化是行内报错不再携带服务层原始信封。

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

Lines changed: 154 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1681,15 +1681,15 @@ describe('AuthManager', () => {
16811681
describe('phone-number OTP over SMS (#2780)', () => {
16821682
const PHONE = '+8613800000000';
16831683

1684-
const fakeSms = (opts: { failed?: boolean } = {}) => {
1684+
const fakeSms = (opts: { failed?: boolean; error?: string } = {}) => {
16851685
const sent: any[] = [];
16861686
return {
16871687
sent,
16881688
service: {
16891689
async send(input: any) {
16901690
sent.push(input);
16911691
return opts.failed
1692-
? { id: 'sms_1', status: 'failed', error: 'provider down' }
1692+
? { id: 'sms_1', status: 'failed', error: opts.error ?? 'provider down' }
16931693
: { id: 'sms_1', status: 'sent', messageId: 'prov_1' };
16941694
},
16951695
isConfigured: () => true,
@@ -1785,6 +1785,158 @@ describe('AuthManager', () => {
17851785
.rejects.toSatisfy((e: Error) => /provider down/.test(e.message) && !e.message.includes('555555'));
17861786
});
17871787

1788+
// ── #6039 / #2814 — the deployment-wide daily SMS quota wall ───────────
1789+
//
1790+
// `SmsService.send()` refuses a send past the deployment's daily cost
1791+
// ceiling by RETURNING a failed result whose `error` carries the
1792+
// `TOO_MANY_REQUESTS:` code prefix (#2814) — it cannot throw an HTTP-shaped
1793+
// error, because it is a kernel service with no idea who is calling.
1794+
// Rethrowing that envelope as a plain `Error` made better-call answer
1795+
// **500 with a null body**: its router maps only `APIError`
1796+
// (`isAPIError = err instanceof APIError || err?.name === 'APIError'`,
1797+
// better-call@1.3.7 `dist/utils.mjs:57`, consumed at `dist/router.mjs:93`),
1798+
// and everything else takes the `console.error` + 500 branch. Meanwhile the
1799+
// per-number wall on the SAME endpoint (`assertPhoneOtpSendAllowed`, in the
1800+
// admission hook) throws a real `APIError('TOO_MANY_REQUESTS')` and answers
1801+
// 429 — so one endpoint spoke with two voices, which is the reverse of what
1802+
// #2814 asked for.
1803+
describe('daily SMS quota refusal reaches the caller as 429 (#6039)', () => {
1804+
/**
1805+
* The refusal envelope an `SmsService` hands back on a quota refusal.
1806+
* Written out here rather than imported: `@objectstack/service-sms`
1807+
* already depends on THIS package (its day counter imports
1808+
* `InProcessCounterStore` / `incrementFixedWindow` from plugin-auth), so
1809+
* importing its `SMS_QUOTA_EXCEEDED_ERROR` back would close a dependency
1810+
* cycle. Source of truth: `SMS_QUOTA_EXCEEDED_CODE` /
1811+
* `SMS_QUOTA_EXCEEDED_ERROR` in
1812+
* `packages/services/service-sms/src/sms-daily-quota.ts`.
1813+
*/
1814+
const QUOTA_REFUSAL = 'TOO_MANY_REQUESTS: daily SMS quota exhausted';
1815+
1816+
/**
1817+
* The outward shape a client — and better-call's router — actually
1818+
* branches on. Message text is deliberately NOT part of it: the two walls
1819+
* must be indistinguishable in code and status, while each may still say
1820+
* something true (only the per-number wall can name a retry window).
1821+
*/
1822+
const outwardShape = (e: any) => ({
1823+
name: e?.name,
1824+
status: e?.status,
1825+
statusCode: e?.statusCode,
1826+
bodyCode: e?.body?.code,
1827+
});
1828+
1829+
const rejection = async (run: () => Promise<unknown>): Promise<any> => {
1830+
try {
1831+
await run();
1832+
} catch (e) {
1833+
return e;
1834+
}
1835+
throw new Error('expected the call to reject, but it resolved');
1836+
};
1837+
1838+
it('OTP send: rejects with an APIError carrying 429 / TOO_MANY_REQUESTS', async () => {
1839+
const { manager, opts } = await bootOtp();
1840+
manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service);
1841+
1842+
const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '424242' }));
1843+
// Exactly what better-call reads to choose 429 over 500.
1844+
const { isAPIError } = await import('better-auth/api');
1845+
expect(isAPIError(err)).toBe(true);
1846+
expect(err.name).toBe('APIError');
1847+
expect(err.status).toBe('TOO_MANY_REQUESTS');
1848+
expect(err.statusCode).toBe(429);
1849+
// #2780 standing requirement: the code never travels in an error.
1850+
expect(String(err.message)).not.toContain('424242');
1851+
});
1852+
1853+
it('invitation SMS: same APIError / 429 at the AuthManager boundary', async () => {
1854+
const { manager } = await bootOtp();
1855+
manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service);
1856+
1857+
const err = await rejection(() => manager.sendPhoneInviteSms(PHONE));
1858+
const { isAPIError } = await import('better-auth/api');
1859+
expect(isAPIError(err)).toBe(true);
1860+
expect(err.status).toBe('TOO_MANY_REQUESTS');
1861+
expect(err.statusCode).toBe(429);
1862+
});
1863+
1864+
it('both walls on the endpoint present the SAME outward shape', async () => {
1865+
const { manager, opts } = await bootOtp();
1866+
manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service);
1867+
1868+
// Wall A — the per-number cooldown, refused in the admission hook (#2780).
1869+
await manager.assertPhoneOtpSendAllowed(PHONE);
1870+
const perNumber = await rejection(() => manager.assertPhoneOtpSendAllowed(PHONE));
1871+
// Wall B — the deployment's daily quota, refused inside the send (#2814).
1872+
const quota = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '111111' }));
1873+
1874+
expect(outwardShape(quota)).toEqual(outwardShape(perNumber));
1875+
expect(outwardShape(quota)).toEqual({
1876+
name: 'APIError',
1877+
status: 'TOO_MANY_REQUESTS',
1878+
statusCode: 429,
1879+
bodyCode: undefined,
1880+
});
1881+
});
1882+
1883+
it('matches the CODE prefix, so the service may reword the message half', async () => {
1884+
const { manager, opts } = await bootOtp();
1885+
// Only `TOO_MANY_REQUESTS` — an ADR-0112 error code — is restated across
1886+
// the package boundary; the prose after the colon is service-owned and
1887+
// free to change without breaking this mapping.
1888+
manager.setSmsService(
1889+
fakeSms({ failed: true, error: 'TOO_MANY_REQUESTS: budget spent for today' }).service,
1890+
);
1891+
const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '333333' }));
1892+
expect(err.statusCode).toBe(429);
1893+
});
1894+
1895+
it('does NOT over-tighten: a transport failure keeps its 500 semantics', async () => {
1896+
const { manager, opts } = await bootOtp();
1897+
manager.setSmsService(fakeSms({ failed: true }).service); // 'provider down'
1898+
const { isAPIError } = await import('better-auth/api');
1899+
1900+
const otpErr = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '555555' }));
1901+
expect(isAPIError(otpErr)).toBe(false);
1902+
expect(otpErr.name).toBe('Error');
1903+
expect(String(otpErr.message)).toContain('provider down');
1904+
1905+
const inviteErr = await rejection(() => manager.sendPhoneInviteSms(PHONE));
1906+
expect(isAPIError(inviteErr)).toBe(false);
1907+
expect(inviteErr.name).toBe('Error');
1908+
expect(String(inviteErr.message)).toContain('provider down');
1909+
});
1910+
1911+
it('the code must PREFIX the envelope — a provider merely mentioning it stays 500', async () => {
1912+
const { manager, opts } = await bootOtp();
1913+
manager.setSmsService(
1914+
fakeSms({ failed: true, error: 'upstream rejected: TOO_MANY_REQUESTS at carrier' }).service,
1915+
);
1916+
const { isAPIError } = await import('better-auth/api');
1917+
const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '777777' }));
1918+
expect(isAPIError(err)).toBe(false);
1919+
expect(err.name).toBe('Error');
1920+
});
1921+
1922+
it('the 429 message carries no quota ceiling, remaining count or reset clock', async () => {
1923+
const { manager, opts } = await bootOtp();
1924+
manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service);
1925+
1926+
for (const run of [
1927+
() => opts.sendOTP({ phoneNumber: PHONE, code: '999999' }),
1928+
() => manager.sendPhoneInviteSms(PHONE),
1929+
]) {
1930+
const message = String((await rejection(run)).message);
1931+
// No digits at all ⇒ no ceiling, no remaining count, no reset clock.
1932+
expect(message).not.toMatch(/\d/);
1933+
// …and not the raw service envelope, which names the budget that was hit.
1934+
expect(message).not.toContain(QUOTA_REFUSAL);
1935+
expect(message.toLowerCase()).not.toContain('quota');
1936+
}
1937+
});
1938+
});
1939+
17881940
it('honours phoneOtp knobs (cooldown off ⇒ back-to-back admissions allowed)', async () => {
17891941
const { manager } = await bootOtp({ phoneOtp: { cooldownSeconds: 0, maxPerHour: 0 } });
17901942
manager.setSmsService(fakeSms().service);

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

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -728,6 +728,70 @@ export function ipMatchesRange(ip: string, range: string): boolean {
728728
return ip.trim() === r;
729729
}
730730

731+
/**
732+
* #6039 / #2814 — the error code an SMS service reports a QUOTA refusal with.
733+
*
734+
* `SmsService.send()` cannot throw an HTTP-shaped error: it is a kernel service
735+
* with no idea who is calling. It reports the deployment's daily cost ceiling
736+
* being hit as a normal failed result whose `error` carries the service's
737+
* `CODE: message` envelope — here `TOO_MANY_REQUESTS: daily SMS quota
738+
* exhausted`. Turning that back into an HTTP answer is the auth endpoint's job,
739+
* which is why the mapping lives here (see {@link isSmsQuotaRefusal}).
740+
*
741+
* **Source of truth**: `SMS_QUOTA_EXCEEDED_CODE` in
742+
* `packages/services/service-sms/src/sms-daily-quota.ts` (exported alongside
743+
* `SMS_QUOTA_EXCEEDED_ERROR`).
744+
*
745+
* **Why it is restated instead of imported**: `@objectstack/service-sms` already
746+
* depends on THIS package — its day counter imports `InProcessCounterStore` /
747+
* `incrementFixedWindow` from `@objectstack/plugin-auth` — so importing the
748+
* constant back would close a dependency cycle. This is the same call, in the
749+
* other direction, that `normalizeSmsRecipient` makes in
750+
* `packages/services/service-sms/src/sms-service.ts` ("Same shape rule as
751+
* plugin-auth's `normalizePhoneNumber` … kept local: the two packages must not
752+
* depend on each other").
753+
*
754+
* Only the CODE crosses the boundary — a closed-vocabulary ADR-0112 error code,
755+
* not prose. The message half of the envelope is service-owned and may be
756+
* reworded without touching this file.
757+
*/
758+
const SMS_QUOTA_EXCEEDED_CODE = 'TOO_MANY_REQUESTS';
759+
760+
/**
761+
* #6039 — is this `SendSmsResult.error` the quota wall's refusal?
762+
*
763+
* Matched as a PREFIX of the service's `CODE: message` envelope, never as a
764+
* substring: a transport failure puts the provider's raw text in `error`
765+
* (`SmsService` truncates it and reports it verbatim), and one that happens to
766+
* mention the code mid-sentence is still a transport failure — 500, not 429.
767+
*/
768+
function isSmsQuotaRefusal(error: string | undefined): boolean {
769+
return typeof error === 'string' && error.startsWith(`${SMS_QUOTA_EXCEEDED_CODE}:`);
770+
}
771+
772+
/**
773+
* #6039 — the 429 an SMS quota refusal must reach the caller as.
774+
*
775+
* better-call (better-auth's router) maps ONLY `APIError` to a real HTTP status:
776+
* `isAPIError(err) = err instanceof APIError || err?.name === 'APIError'`
777+
* (better-call@1.3.7 `dist/utils.mjs:57`), consumed at `dist/router.mjs:93`,
778+
* where everything else takes the `console.error` + `500 / null body` branch.
779+
* A plain `Error` therefore buried `TOO_MANY_REQUESTS` in a server log while the
780+
* per-number wall on the same endpoint ({@link AuthManager.assertPhoneOtpSendAllowed})
781+
* answered 429 — one endpoint, two voices.
782+
*
783+
* The message follows the per-number wall's shape and carries NO budget detail:
784+
* no ceiling, no remaining count, no reset clock. #2814's requirement is that
785+
* the two walls be indistinguishable from outside — an attacker must not learn
786+
* which budget they hit, and a legitimate caller needs no more than "not now".
787+
* (The per-number wall names its own retry window because it can compute one;
788+
* this wall states no time it cannot honestly promise.)
789+
*/
790+
async function smsQuotaExceededApiError(message: string): Promise<Error> {
791+
const { APIError } = await import('better-auth/api');
792+
return new APIError(SMS_QUOTA_EXCEEDED_CODE, { message });
793+
}
794+
731795
export class AuthManager {
732796
private auth: Auth<any> | null = null;
733797
private config: AuthManagerOptions;
@@ -2840,6 +2904,16 @@ export class AuthManager {
28402904
templateParams: { code },
28412905
});
28422906
if (result.status === 'failed') {
2907+
// #6039 — the deployment's daily SMS quota refused this send. Answer it
2908+
// the way the per-number wall on this same endpoint answers: a real
2909+
// `APIError`, hence 429 TOO_MANY_REQUESTS instead of a 500 with a null
2910+
// body. Everything else stays a plain Error — a transport outage IS a
2911+
// server-side failure and 500 is the honest answer for it.
2912+
if (isSmsQuotaRefusal(result.error)) {
2913+
throw await smsQuotaExceededApiError(
2914+
'Too many verification codes requested. Please try again later.',
2915+
);
2916+
}
28432917
// `result.error` is transport detail (never the code) — safe to surface.
28442918
throw new Error(`Phone OTP could not be sent: ${result.error ?? 'SMS delivery failed'}`);
28452919
}
@@ -2870,6 +2944,18 @@ export class AuthManager {
28702944
});
28712945
const result = await sms.send({ to: phone, body, templateParams: { content: body } });
28722946
if (result.status === 'failed') {
2947+
// #6039 — same quota wall, same outward shape as the OTP path above.
2948+
// (The one in-repo caller, admin import-users, catches this per row and
2949+
// records INVITE_SMS_FAILED rather than failing the request — so what
2950+
// changes there is that the row's message no longer carries the raw
2951+
// service envelope. The 429 matters for any caller that surfaces this
2952+
// rejection directly, which is what a public AuthManager method must be
2953+
// correct for.)
2954+
if (isSmsQuotaRefusal(result.error)) {
2955+
throw await smsQuotaExceededApiError(
2956+
'Too many SMS messages requested. Please try again later.',
2957+
);
2958+
}
28732959
throw new Error(`Invitation SMS failed: ${result.error ?? 'SMS delivery failed'}`);
28742960
}
28752961
}

0 commit comments

Comments
 (0)