diff --git a/.changeset/otp-sms-quota-429.md b/.changeset/otp-sms-quota-429.md new file mode 100644 index 0000000000..86beaefe3a --- /dev/null +++ b/.changeset/otp-sms-quota-429.md @@ -0,0 +1,40 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): 短信日配额拒发时,OTP / 邀请短信按 429 TOO_MANY_REQUESTS 作答,不再是 500 (#6039) + +#2814 把短信总量成本闸落在 `SmsService.send()` —— 它是内核服务,不知道调用方是谁, +所以超限时**返回**一条失败结果,把码写在服务层既有的 `CODE: message` 信封上: +`TOO_MANY_REQUESTS: daily SMS quota exhausted`。把 HTTP 语义还原回去是 auth 端点的 +职责,而 `AuthManager` 此前没有做:`deliverPhoneOtp()` / `sendPhoneInviteSms()` 对 +任何 `status === 'failed'` 一律抛普通 `Error`。 + +better-auth 的路由层 better-call 只把 `APIError` 映射成真实状态码 +(`isAPIError = err instanceof APIError || err?.name === 'APIError'`, +better-call@1.3.7 `dist/utils.mjs:57`,消费点在 `dist/router.mjs:93`),其余一律走 +`console.error` + **500、响应体 `null`** 的分支。于是配额拒发对外是 500, +`TOO_MANY_REQUESTS` 只留在服务端日志里;而**同一个端点**上按号码冷却闸 +(`assertPhoneOtpSendAllowed`,在 admission hook 里)抛的是 +`APIError('TOO_MANY_REQUESTS')`,正常回 429 —— 一个端点两种口径,正是 #2814 +「两道墙从外面看应当一样」的反面。 + +现在两处失败分支都先识别信封上的 `TOO_MANY_REQUESTS:` **前缀**,改抛 +`APIError('TOO_MANY_REQUESTS')`: + +- **只有码跨包**。识别用的 `TOO_MANY_REQUESTS` 在 plugin-auth 本地写死并注明出处 + (`SMS_QUOTA_EXCEEDED_CODE`,`packages/services/service-sms/src/sms-daily-quota.ts`)—— + `@objectstack/service-sms` 已经依赖本包(它的日计数器从这里 import + `InProcessCounterStore`),反向 import 会成环;这与 service-sms 里 + `normalizeSmsRecipient` 就地重述 plugin-auth 形状规则是同一个取舍的另一半。 + 跨包重述的只是一个 ADR-0112 闭集错误码,冒号后的措辞归服务层所有,可以自由改写。 +- **不泄露预算**。429 文案沿按号码闸的措辞形状,不含上限、剩余量与重置时刻 + (按号码闸报自己的重试窗口,是因为它算得出;配额闸不承诺它给不出的时间)。 +- **不顺手收紧**。传输故障(provider 宕机等)仍抛普通 `Error`,500 语义原样不变; + 仅仅在文中提到该码而不以之开头的 provider 报错同样保持 500。 + +对外可见的变化:`POST /phone-number/send-otp`、 +`POST /phone-number/request-password-reset` 在部署日配额耗尽时,由 +**500 + 空响应体**变为 **429 TOO_MANY_REQUESTS**,与按号码冷却闸同形。 +邀请短信路径同样返回 `APIError`;仓内唯一调用方(admin import-users)按行捕获它并 +记为 `INVITE_SMS_FAILED`,该路径的变化是行内报错不再携带服务层原始信封。 diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index b25e34dcca..d9a288bf43 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1681,7 +1681,7 @@ describe('AuthManager', () => { describe('phone-number OTP over SMS (#2780)', () => { const PHONE = '+8613800000000'; - const fakeSms = (opts: { failed?: boolean } = {}) => { + const fakeSms = (opts: { failed?: boolean; error?: string } = {}) => { const sent: any[] = []; return { sent, @@ -1689,7 +1689,7 @@ describe('AuthManager', () => { async send(input: any) { sent.push(input); return opts.failed - ? { id: 'sms_1', status: 'failed', error: 'provider down' } + ? { id: 'sms_1', status: 'failed', error: opts.error ?? 'provider down' } : { id: 'sms_1', status: 'sent', messageId: 'prov_1' }; }, isConfigured: () => true, @@ -1785,6 +1785,158 @@ describe('AuthManager', () => { .rejects.toSatisfy((e: Error) => /provider down/.test(e.message) && !e.message.includes('555555')); }); + // ── #6039 / #2814 — the deployment-wide daily SMS quota wall ─────────── + // + // `SmsService.send()` refuses a send past the deployment's daily cost + // ceiling by RETURNING a failed result whose `error` carries the + // `TOO_MANY_REQUESTS:` code prefix (#2814) — it cannot throw an HTTP-shaped + // error, because it is a kernel service with no idea who is calling. + // Rethrowing that envelope as a plain `Error` made better-call answer + // **500 with a null body**: its router maps only `APIError` + // (`isAPIError = err instanceof APIError || err?.name === 'APIError'`, + // better-call@1.3.7 `dist/utils.mjs:57`, consumed at `dist/router.mjs:93`), + // and everything else takes the `console.error` + 500 branch. Meanwhile the + // per-number wall on the SAME endpoint (`assertPhoneOtpSendAllowed`, in the + // admission hook) throws a real `APIError('TOO_MANY_REQUESTS')` and answers + // 429 — so one endpoint spoke with two voices, which is the reverse of what + // #2814 asked for. + describe('daily SMS quota refusal reaches the caller as 429 (#6039)', () => { + /** + * The refusal envelope an `SmsService` hands back on a quota refusal. + * Written out here rather than imported: `@objectstack/service-sms` + * already depends on THIS package (its day counter imports + * `InProcessCounterStore` / `incrementFixedWindow` from plugin-auth), so + * importing its `SMS_QUOTA_EXCEEDED_ERROR` back would close a dependency + * cycle. Source of truth: `SMS_QUOTA_EXCEEDED_CODE` / + * `SMS_QUOTA_EXCEEDED_ERROR` in + * `packages/services/service-sms/src/sms-daily-quota.ts`. + */ + const QUOTA_REFUSAL = 'TOO_MANY_REQUESTS: daily SMS quota exhausted'; + + /** + * The outward shape a client — and better-call's router — actually + * branches on. Message text is deliberately NOT part of it: the two walls + * must be indistinguishable in code and status, while each may still say + * something true (only the per-number wall can name a retry window). + */ + const outwardShape = (e: any) => ({ + name: e?.name, + status: e?.status, + statusCode: e?.statusCode, + bodyCode: e?.body?.code, + }); + + const rejection = async (run: () => Promise): Promise => { + try { + await run(); + } catch (e) { + return e; + } + throw new Error('expected the call to reject, but it resolved'); + }; + + it('OTP send: rejects with an APIError carrying 429 / TOO_MANY_REQUESTS', async () => { + const { manager, opts } = await bootOtp(); + manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service); + + const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '424242' })); + // Exactly what better-call reads to choose 429 over 500. + const { isAPIError } = await import('better-auth/api'); + expect(isAPIError(err)).toBe(true); + expect(err.name).toBe('APIError'); + expect(err.status).toBe('TOO_MANY_REQUESTS'); + expect(err.statusCode).toBe(429); + // #2780 standing requirement: the code never travels in an error. + expect(String(err.message)).not.toContain('424242'); + }); + + it('invitation SMS: same APIError / 429 at the AuthManager boundary', async () => { + const { manager } = await bootOtp(); + manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service); + + const err = await rejection(() => manager.sendPhoneInviteSms(PHONE)); + const { isAPIError } = await import('better-auth/api'); + expect(isAPIError(err)).toBe(true); + expect(err.status).toBe('TOO_MANY_REQUESTS'); + expect(err.statusCode).toBe(429); + }); + + it('both walls on the endpoint present the SAME outward shape', async () => { + const { manager, opts } = await bootOtp(); + manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service); + + // Wall A — the per-number cooldown, refused in the admission hook (#2780). + await manager.assertPhoneOtpSendAllowed(PHONE); + const perNumber = await rejection(() => manager.assertPhoneOtpSendAllowed(PHONE)); + // Wall B — the deployment's daily quota, refused inside the send (#2814). + const quota = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '111111' })); + + expect(outwardShape(quota)).toEqual(outwardShape(perNumber)); + expect(outwardShape(quota)).toEqual({ + name: 'APIError', + status: 'TOO_MANY_REQUESTS', + statusCode: 429, + bodyCode: undefined, + }); + }); + + it('matches the CODE prefix, so the service may reword the message half', async () => { + const { manager, opts } = await bootOtp(); + // Only `TOO_MANY_REQUESTS` — an ADR-0112 error code — is restated across + // the package boundary; the prose after the colon is service-owned and + // free to change without breaking this mapping. + manager.setSmsService( + fakeSms({ failed: true, error: 'TOO_MANY_REQUESTS: budget spent for today' }).service, + ); + const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '333333' })); + expect(err.statusCode).toBe(429); + }); + + it('does NOT over-tighten: a transport failure keeps its 500 semantics', async () => { + const { manager, opts } = await bootOtp(); + manager.setSmsService(fakeSms({ failed: true }).service); // 'provider down' + const { isAPIError } = await import('better-auth/api'); + + const otpErr = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '555555' })); + expect(isAPIError(otpErr)).toBe(false); + expect(otpErr.name).toBe('Error'); + expect(String(otpErr.message)).toContain('provider down'); + + const inviteErr = await rejection(() => manager.sendPhoneInviteSms(PHONE)); + expect(isAPIError(inviteErr)).toBe(false); + expect(inviteErr.name).toBe('Error'); + expect(String(inviteErr.message)).toContain('provider down'); + }); + + it('the code must PREFIX the envelope — a provider merely mentioning it stays 500', async () => { + const { manager, opts } = await bootOtp(); + manager.setSmsService( + fakeSms({ failed: true, error: 'upstream rejected: TOO_MANY_REQUESTS at carrier' }).service, + ); + const { isAPIError } = await import('better-auth/api'); + const err = await rejection(() => opts.sendOTP({ phoneNumber: PHONE, code: '777777' })); + expect(isAPIError(err)).toBe(false); + expect(err.name).toBe('Error'); + }); + + it('the 429 message carries no quota ceiling, remaining count or reset clock', async () => { + const { manager, opts } = await bootOtp(); + manager.setSmsService(fakeSms({ failed: true, error: QUOTA_REFUSAL }).service); + + for (const run of [ + () => opts.sendOTP({ phoneNumber: PHONE, code: '999999' }), + () => manager.sendPhoneInviteSms(PHONE), + ]) { + const message = String((await rejection(run)).message); + // No digits at all ⇒ no ceiling, no remaining count, no reset clock. + expect(message).not.toMatch(/\d/); + // …and not the raw service envelope, which names the budget that was hit. + expect(message).not.toContain(QUOTA_REFUSAL); + expect(message.toLowerCase()).not.toContain('quota'); + } + }); + }); + it('honours phoneOtp knobs (cooldown off ⇒ back-to-back admissions allowed)', async () => { const { manager } = await bootOtp({ phoneOtp: { cooldownSeconds: 0, maxPerHour: 0 } }); manager.setSmsService(fakeSms().service); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index b814840feb..b9f63f9e23 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -728,6 +728,70 @@ export function ipMatchesRange(ip: string, range: string): boolean { return ip.trim() === r; } +/** + * #6039 / #2814 — the error code an SMS service reports a QUOTA refusal with. + * + * `SmsService.send()` cannot throw an HTTP-shaped error: it is a kernel service + * with no idea who is calling. It reports the deployment's daily cost ceiling + * being hit as a normal failed result whose `error` carries the service's + * `CODE: message` envelope — here `TOO_MANY_REQUESTS: daily SMS quota + * exhausted`. Turning that back into an HTTP answer is the auth endpoint's job, + * which is why the mapping lives here (see {@link isSmsQuotaRefusal}). + * + * **Source of truth**: `SMS_QUOTA_EXCEEDED_CODE` in + * `packages/services/service-sms/src/sms-daily-quota.ts` (exported alongside + * `SMS_QUOTA_EXCEEDED_ERROR`). + * + * **Why it is restated instead of imported**: `@objectstack/service-sms` already + * depends on THIS package — its day counter imports `InProcessCounterStore` / + * `incrementFixedWindow` from `@objectstack/plugin-auth` — so importing the + * constant back would close a dependency cycle. This is the same call, in the + * other direction, that `normalizeSmsRecipient` makes in + * `packages/services/service-sms/src/sms-service.ts` ("Same shape rule as + * plugin-auth's `normalizePhoneNumber` … kept local: the two packages must not + * depend on each other"). + * + * Only the CODE crosses the boundary — a closed-vocabulary ADR-0112 error code, + * not prose. The message half of the envelope is service-owned and may be + * reworded without touching this file. + */ +const SMS_QUOTA_EXCEEDED_CODE = 'TOO_MANY_REQUESTS'; + +/** + * #6039 — is this `SendSmsResult.error` the quota wall's refusal? + * + * Matched as a PREFIX of the service's `CODE: message` envelope, never as a + * substring: a transport failure puts the provider's raw text in `error` + * (`SmsService` truncates it and reports it verbatim), and one that happens to + * mention the code mid-sentence is still a transport failure — 500, not 429. + */ +function isSmsQuotaRefusal(error: string | undefined): boolean { + return typeof error === 'string' && error.startsWith(`${SMS_QUOTA_EXCEEDED_CODE}:`); +} + +/** + * #6039 — the 429 an SMS quota refusal must reach the caller as. + * + * better-call (better-auth's router) maps ONLY `APIError` to a real HTTP status: + * `isAPIError(err) = err instanceof APIError || err?.name === 'APIError'` + * (better-call@1.3.7 `dist/utils.mjs:57`), consumed at `dist/router.mjs:93`, + * where everything else takes the `console.error` + `500 / null body` branch. + * A plain `Error` therefore buried `TOO_MANY_REQUESTS` in a server log while the + * per-number wall on the same endpoint ({@link AuthManager.assertPhoneOtpSendAllowed}) + * answered 429 — one endpoint, two voices. + * + * The message follows the per-number wall's shape and carries NO budget detail: + * no ceiling, no remaining count, no reset clock. #2814's requirement is that + * the two walls be indistinguishable from outside — an attacker must not learn + * which budget they hit, and a legitimate caller needs no more than "not now". + * (The per-number wall names its own retry window because it can compute one; + * this wall states no time it cannot honestly promise.) + */ +async function smsQuotaExceededApiError(message: string): Promise { + const { APIError } = await import('better-auth/api'); + return new APIError(SMS_QUOTA_EXCEEDED_CODE, { message }); +} + export class AuthManager { private auth: Auth | null = null; private config: AuthManagerOptions; @@ -2840,6 +2904,16 @@ export class AuthManager { templateParams: { code }, }); if (result.status === 'failed') { + // #6039 — the deployment's daily SMS quota refused this send. Answer it + // the way the per-number wall on this same endpoint answers: a real + // `APIError`, hence 429 TOO_MANY_REQUESTS instead of a 500 with a null + // body. Everything else stays a plain Error — a transport outage IS a + // server-side failure and 500 is the honest answer for it. + if (isSmsQuotaRefusal(result.error)) { + throw await smsQuotaExceededApiError( + 'Too many verification codes requested. Please try again later.', + ); + } // `result.error` is transport detail (never the code) — safe to surface. throw new Error(`Phone OTP could not be sent: ${result.error ?? 'SMS delivery failed'}`); } @@ -2870,6 +2944,18 @@ export class AuthManager { }); const result = await sms.send({ to: phone, body, templateParams: { content: body } }); if (result.status === 'failed') { + // #6039 — same quota wall, same outward shape as the OTP path above. + // (The one in-repo caller, admin import-users, catches this per row and + // records INVITE_SMS_FAILED rather than failing the request — so what + // changes there is that the row's message no longer carries the raw + // service envelope. The 429 matters for any caller that surfaces this + // rejection directly, which is what a public AuthManager method must be + // correct for.) + if (isSmsQuotaRefusal(result.error)) { + throw await smsQuotaExceededApiError( + 'Too many SMS messages requested. Please try again later.', + ); + } throw new Error(`Invitation SMS failed: ${result.error ?? 'SMS delivery failed'}`); } }