Skip to content

Commit c92b887

Browse files
fix(plugin-auth): the phone-number password-reset request answers without waiting for the SMS send, as the emailed reset does (#29973)
Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:手机号重置密码请求(`POST /api/v1/auth/phone-number/request-password-reset`)不再在请求内等待短信发送;重置短信在响应之后构建并发送,与邮件重置一致。 - 为什么改:让手机号重置这扇门与邮件重置遵守同一条规则(请求即答、不等发送),沿用同一个 helper `sendAfterResponseOnPublicDoor`,不新增机制。 - 风险与回滚:答复的状态与正文不变;发送失败改由服务端 logger 以 error 级记录(与邮件门同形),不再经 better-auth 自己的日志。回滚 = 撤销本 PR,无数据或配置迁移。 - 先看:① `packages/plugins/plugin-auth/src/auth-manager-plugin-list.ts` 的 `sendPasswordResetOTP` 一行(取 `ctx?.request` 交给 helper);② `run-after-response.ts` 的日志措辞(`what` 自带介质,邮件那行字节不变);③ `reset-request-mail-after-response.test.ts` 新增的 phone `describe`。 - 两分钟验证:`pnpm --filter @objectstack/plugin-auth exec vitest run src/reset-request-mail-after-response.test.ts`,预期 9/9 通过,其中 phone `describe` 三条全绿。 - 需要你做的:无,席位落地。 ## What changes The phone-number password-reset request answers without waiting for the SMS send, as the emailed reset does. - `sendPasswordResetOTP` (`auth-manager-plugin-list.ts`) now goes through `sendAfterResponseOnPublicDoor`, the helper the emailed reset's `sendResetPassword` already uses. A call that carries a `Request` (the public door) resolves at once; the text is built and sent after the response; a failed send goes to `logAfterResponseSendFailure` at error. - better-auth 1.7.12 calls this callback as `sendPasswordResetOTP(data, ctx)` — the endpoint context, not the `Request` the emailed reset is handed (`plugins/phone-number/routes.mjs`). The callback therefore passes `ctx?.request`, the very value `sendResetPassword` receives (`routes/password.mjs`). The helper's rule is unchanged: a `Request` present means the public door; none means a server-side `auth.api.*` call, which still awaits its send. - `logAfterResponseSendFailure` takes the medium inside `what`: the email door passes `password-reset email` and prints the same bytes as before; the phone door prints `phone password-reset text was not delivered: …`. The line carries the transport's error, never the one-time code. - Unchanged: the answer's status and body (`200 {"status":true}`), the per-number cooldown and hourly cap in the before-hook, the per-IP limit on the route, the text itself, and `sendOTP` (sign-in OTP), which still awaits its send so its `429`/`400` refusals reach the caller. ## Tests New top-level `describe` in the existing harness file `src/reset-request-mail-after-response.test.ts` (real `AuthManager.handleRequest` over ObjectQL + better-sqlite3): - `answers a number with an account and a number without one alike, while the reset text is still being sent`: the SMS stub's send waits on a deferred the case resolves by hand. Both answers arrive, equal, while that send is still open; then the case resolves it and the text goes out to the number (control). No wall-clock threshold. - `logs a reset text whose send fails on the server, without its code, and answers as for a text that is sent`. - `the server-side phone reset call (no Request) still awaits its send`. `src/phone-otp-no-sms-service-refusal.test.ts`: the existing pin that the refusing send is reached now waits for it with `vi.waitFor`, since it runs after the response. The two phone pins in `password-reset-ends-sessions.test.ts` (which already use `vi.waitFor`) pass unchanged. <details><summary>Results at af6b561</summary> ```text pnpm --filter @objectstack/plugin-auth test (under os-verify-lock) Test Files 219 passed (219) Tests 3769 passed | 10 skipped (3779) os-verify-lock: VERDICT command-exit 0 pnpm --filter @objectstack/plugin-auth typecheck (after building plugin-auth) check:test-typecheck: OK — 10 file(s) / 88 error(s) / 23 pinned signature(s) held in test-typecheck-debt.json os-verify-lock: VERDICT command-exit 0 ``` </details> <details><summary>One-shot ablation (committed fix first; mutation through scripts/ablation-replace.mjs; restore proven blob == HEAD, git diff HEAD empty)</summary> ```text Leg A — wrapper removed (callback back to an inline awaited deliverPhoneOtp): anchor x1 -> x0, replacement x0 -> x1, blob 811bf7c -> c1d2e587542b x answers a number with an account and a number without one alike, ... — Test timed out in 60000ms (the answer never arrives while the send is open) x logs a reset text whose send fails on the server, ... — expected [] to have a length of 1 ✓ the server-side phone reset call (no Request) still awaits its send (control) ✓ all four emailed-reset cases, and the no-SMS-service file restore: blob 811bf7c == HEAD, git diff HEAD empty Leg B — the whole ctx handed to the helper instead of ctx?.request: x the server-side phone reset call (no Request) still awaits its send — expected [] to deeply equal [ '+15550161' ] ✓ the two public-door phone cases restore: blob 811bf7c == HEAD, git diff HEAD empty ``` </details> ## Acceptance notes - No server-side caller of the phone reset exists in this repository: `git grep -n requestPasswordResetPhoneNumber` hits only the new test, and the identity import calls the emailed `requestPasswordReset`. The no-`Request` branch is kept anyway and pinned (leg B above). - The rejected alternative, better-auth's global `advanced.backgroundTasks.handler`, stays unset: it would also stop the identity import's server-side reset from awaiting the delivery receipt it reads. - The deferred-gate case fails in the ablated world by the harness's per-case timeout (60 s): with the wrapper gone the answer cannot arrive while the send is open, so the case can never pass; it asserts no duration. ## Verification All readings at `af6b561239` (the head of this PR); `origin/main` had not moved from the merge base `3b8c724cf2` when they were taken. - Derived gate list: `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` named 59 commands for this diff; each was run with its exit code captured before any pipe, then reconciled; in the final record every one answered exit 0 (the list and its codes are in the seat report). - `node scripts/pm/dispatch-gates.mjs --ran` → `Run reconciliation — 59 derived, 59 run, 0 NOT-MEASURED, 0 UNRUN.` - `pnpm check:dual-build-cjs-loads` first answered exit 3 (PREREQUISITE NOT MET: unbuilt packages, nothing measured); after a full workspace build (`turbo run build --filter=!@objectstack/docs`, 119/119 tasks) it answered exit 0: `116 published require entry point(s) across 71 package(s) load`. - Full `@objectstack/plugin-auth` suite and typecheck: see Tests above. - Not run locally: `test:repo` (the vitest `repo` project, which CI's `Test Core` runs) and the full CI farm; CI's check-runs on this PR are the reading for those. ## CI note (seat) - `Build Core` on `af6b561239` first failed before compiling anything: `@objectstack/spec`'s build could not fetch the deletion-check anchor commit `7eb613c39fb8`, and the remote did not serve it during that run. The commit exists on the remote, and this PR touches no file under `packages/spec`. Treated as a transient fetch flake; re-run once through the relay, and the re-run is `success`. - `Check Changeset` on this head was cancelled without a newer run superseding it. This body edit re-triggers it. <!-- signed: security#1 · os-justin · session_01QSeKYn5CmmdShCJifm6WYj --> Co-authored-by: Claude <noreply@anthropic.com>
1 parent b379262 commit c92b887

6 files changed

Lines changed: 171 additions & 17 deletions
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-auth': patch
3+
---
4+
5+
fix(plugin-auth): the phone-number password-reset request answers without waiting for the SMS send, as the emailed reset does
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
- **The rule.** `POST /api/v1/auth/phone-number/request-password-reset` answers at once, the same way for any number: the reset text is built and sent after the response, as the emailed reset (`POST /api/v1/auth/request-password-reset`) already sends its mail.
11+
- **A send that fails is logged on the server**, at error (`phone password-reset text was not delivered`), and never reaches the caller, who is answered `{ status: true }` either way, as before. The one-time code is in the text only, never in that line.
12+
- **Unchanged.** The answer's status and body; the per-number cooldown and hourly cap and the per-IP limit on the route; the text itself; and a server-side `auth.api.requestPasswordResetPhoneNumber` call, which carries no request and still awaits its send.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ export async function createAuthInstance(this: AuthInstanceHost): Promise<Auth<a
500500
`${invitation ? 'Account-invitation' : 'Password-reset'} email could not be sent to ${user.email}: ${result.error ?? 'delivery failed'}`,
501501
);
502502
}
503-
}), (error) => logAfterResponseSendFailure(this.config.logger, 'password-reset', error)),
503+
}), (error) => logAfterResponseSendFailure(this.config.logger, 'password-reset email', error)),
504504
// [#25060, ruling 1B on #25040] The mail half of "a sign-up for an
505505
// address that already has an account answers like a success". The
506506
// vendor calls this only when the address already has a row AND its

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

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ import {
8181
buildTwoFactorPluginSchema,
8282
} from './auth-schema-config.js';
8383
import { isPlaceholderEmail } from './placeholder-email.js';
84+
import { logAfterResponseSendFailure, sendAfterResponseOnPublicDoor } from './run-after-response.js';
8485
import {
8586
ONE_TIME_TOKEN_EXPIRES_IN_MINUTES,
8687
oneTimeTokenStoreHasher,
@@ -354,6 +355,15 @@ export async function buildPluginList(this: PluginListHost): Promise<any[]> {
354355
// by the admin create-user/import routes with a placeholder email
355356
// (see placeholder-email.ts), never by OTP self-signup.
356357
const otpCfg = this.config.phoneOtp ?? {};
358+
// The reset text, sent the way the emailed reset sends its mail
359+
// (`sendResetPassword`, `auth-manager-auth-instance.ts`): on the public
360+
// door after the response, its failure logged on the server.
361+
const sendPasswordResetText = sendAfterResponseOnPublicDoor(
362+
async ({ phoneNumber: phone, code }: { phoneNumber: string; code: string }) => {
363+
await this.deliverPhoneOtp(phone, code);
364+
},
365+
(error) => logAfterResponseSendFailure(this.config.logger, 'phone password-reset text', error),
366+
);
357367
return phoneNumber({
358368
schema: buildPhoneNumberPluginSchema(),
359369
// Wrong-code attempts before the stored OTP is invalidated. Explicit
@@ -373,13 +383,19 @@ export async function buildPluginList(this: PluginListHost): Promise<any[]> {
373383
await this.deliverPhoneOtp(phone, code);
374384
},
375385
// Self-service password reset OTP (`/phone-number/request-password-
376-
// reset`). better-auth invokes this via runInBackgroundOrAwait —
377-
// errors are logged, the route still answers {status:true} — so a
378-
// throw here can neither 500 the request nor leak whether the number
379-
// is registered.
380-
sendPasswordResetOTP: async ({ phoneNumber: phone, code }) => {
381-
await this.deliverPhoneOtp(phone, code);
382-
},
386+
// reset`). The request answers the same way for any number, answers
387+
// at once and does not wait on the SMS send, as the emailed reset
388+
// does: a call that carries a `Request` — the public door — resolves
389+
// immediately and the text is built and sent after the response
390+
// (`sendAfterResponseOnPublicDoor`); what the send does is logged on
391+
// the server and never reaches the caller, who is answered
392+
// `{ status: true }` either way. better-auth hands this callback the
393+
// endpoint context, not the `Request` the emailed reset is handed
394+
// (`sendPasswordResetOTP(data, ctx)`, better-auth 1.7.12
395+
// `plugins/phone-number/routes.mjs`), so the context's own `request`
396+
// is what decides: a server-side `auth.api.*` call carries none and
397+
// still awaits its send.
398+
sendPasswordResetOTP: (data, ctx) => sendPasswordResetText(data, ctx?.request),
383399
});
384400
});
385401
}

‎packages/plugins/plugin-auth/src/phone-otp-no-sms-service-refusal.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -267,12 +267,12 @@ describe('POST /phone-number/send-otp without a deliverable SMS service', () =>
267267
}),
268268
);
269269

270-
// better-auth runs this send through `runInBackgroundOrAwait`, which logs a
271-
// throw and answers success — so the refusal cannot leak, by status or
272-
// body, whether the number belongs to an account.
270+
// On this public door the send runs after the response
271+
// (`sendAfterResponseOnPublicDoor`) and what it throws is logged on the
272+
// server — so the refusal cannot change the answer, by status or body.
273273
expect(response.status, await response.clone().text()).toBe(200);
274274
expect(await response.json()).toEqual({ status: true });
275-
expect(deliver).toHaveBeenCalledTimes(1);
275+
await vi.waitFor(() => expect(deliver).toHaveBeenCalledTimes(1));
276276
await expect(deliver.mock.results[0]?.value).rejects.toMatchObject({
277277
statusCode: 400,
278278
body: { code: 'SMS_SERVICE_REQUIRED' },

‎packages/plugins/plugin-auth/src/reset-request-mail-after-response.test.ts‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@
1919
* answer is the one a succeeding send gets;
2020
* - the server-side reset call (no `Request` — the identity import's) still
2121
* awaits its send, because it reads the delivery receipt the send settles;
22+
* - the phone-number reset request (`plugins.phoneNumber` with an SMS
23+
* service) answers without waiting for the SMS send, as the emailed reset
24+
* does: a number with an account and a number without one get the same
25+
* answer while the reset text's send is still open, and the text still
26+
* goes out after the response; a text whose send fails is logged on the
27+
* server at error, without its code, and the answer is the one a sent text
28+
* gets; the server-side phone reset call (no `Request`) still awaits its
29+
* send;
2230
* - `runAfterResponse` hands a task's synchronous throw to `onFailure` and
2331
* survives an `onFailure` that throws.
2432
*/
@@ -177,6 +185,118 @@ describe('a reset request answers without waiting on the reset mail', () => {
177185
}, 60_000);
178186
});
179187

188+
describe('the phone-number reset request answers without waiting on the reset text', () => {
189+
const PHONE = '+15550161';
190+
const NO_ACCOUNT_PHONE = '+15550162';
191+
const SYS = { context: { isSystem: true } } as never;
192+
193+
type Text = { to: string; templateParams?: { code?: string } };
194+
195+
/** A promise the case settles by hand. */
196+
function deferred(): { promise: Promise<void>; resolve: () => void } {
197+
let resolve!: () => void;
198+
const promise = new Promise<void>((r) => {
199+
resolve = r;
200+
});
201+
return { promise, resolve };
202+
}
203+
204+
/**
205+
* A deployment with phone-number sign-in and one account holding `PHONE`,
206+
* whose SMS service hands each send to `answer`, and records every text it
207+
* was handed (`texts`) and the number of every send that has settled
208+
* (`settled`).
209+
*/
210+
async function phoneDeployment(answer: () => Promise<unknown>) {
211+
vi.spyOn(console, 'warn').mockImplementation(() => {});
212+
const engine = await engines.take();
213+
const logged: string[] = [];
214+
const manager = new AuthManager({
215+
secret: SECRET,
216+
baseUrl: BASE,
217+
dataEngine: engine as never,
218+
plugins: { phoneNumber: true },
219+
logger: { warn: () => {}, error: (message: string) => logged.push(message) },
220+
} as never);
221+
manager.setEmailService({ sendTemplate: async () => ({ status: 'sent' }) } as never);
222+
const texts: Text[] = [];
223+
const settled: string[] = [];
224+
manager.setSmsService({
225+
async send(input: Text) {
226+
texts.push(input);
227+
try {
228+
return await answer();
229+
} finally {
230+
settled.push(input.to);
231+
}
232+
},
233+
isConfigured: () => true,
234+
} as never);
235+
const signedUp = await post(manager, '/sign-up/email', { email: EMAIL, password: PASSWORD, name: 'Reset' });
236+
expect(signedUp.status, await signedUp.clone().text()).toBe(200);
237+
const [user] = (await engine.find('sys_user', { where: { email: EMAIL }, limit: 1 }, SYS)) as Array<{ id: string }>;
238+
await engine.update('sys_user', { id: user.id, phone_number: PHONE, phone_number_verified: true }, SYS);
239+
return { manager, texts, settled, logged };
240+
}
241+
242+
async function requestPhoneReset(manager: AuthManager, phoneNumber: string): Promise<{ status: number; body: string }> {
243+
const res = await post(manager, '/phone-number/request-password-reset', { phoneNumber });
244+
return { status: res.status, body: await res.text() };
245+
}
246+
247+
it('answers a number with an account and a number without one alike, while the reset text is still being sent', async () => {
248+
const send = deferred();
249+
const d = await phoneDeployment(async () => {
250+
await send.promise;
251+
return { id: 'sms1', status: 'sent' };
252+
});
253+
254+
const forAccount = await requestPhoneReset(d.manager, PHONE);
255+
const forNobody = await requestPhoneReset(d.manager, NO_ACCOUNT_PHONE);
256+
257+
expect(forAccount.status).toBe(200);
258+
expect(forAccount).toEqual(forNobody);
259+
// Both answers arrived while the text's send was still open.
260+
expect(d.settled).toEqual([]);
261+
262+
// The control: the text still goes to the number, after the response.
263+
send.resolve();
264+
await vi.waitFor(() => expect(d.settled).toEqual([PHONE]));
265+
expect(d.texts.map((t) => t.to)).toEqual([PHONE]);
266+
expect(d.logged).toEqual([]);
267+
}, 60_000);
268+
269+
it('logs a reset text whose send fails on the server, without its code, and answers as for a text that is sent', async () => {
270+
const failing = await phoneDeployment(async () => ({ status: 'failed', error: 'carrier unavailable' }));
271+
const sending = await phoneDeployment(async () => ({ id: 'sms1', status: 'sent' }));
272+
273+
const throughFailing = await requestPhoneReset(failing.manager, PHONE);
274+
const throughSending = await requestPhoneReset(sending.manager, PHONE);
275+
276+
expect(throughFailing).toEqual(throughSending);
277+
await vi.waitFor(() => expect(failing.logged).toHaveLength(1));
278+
expect(failing.logged[0]).toContain('phone password-reset text was not delivered');
279+
expect(failing.logged[0]).toContain('carrier unavailable');
280+
const code = String(failing.texts[0]?.templateParams?.code ?? '');
281+
expect(code, 'the text carries its code').not.toBe('');
282+
expect(failing.logged[0]).not.toContain(code);
283+
}, 60_000);
284+
285+
it('the server-side phone reset call (no Request) still awaits its send', async () => {
286+
const d = await phoneDeployment(async () => {
287+
await new Promise((resolve) => setImmediate(resolve));
288+
return { id: 'sms1', status: 'sent' };
289+
});
290+
const api = (await d.manager.getApi()) as unknown as {
291+
requestPasswordResetPhoneNumber(input: { body: { phoneNumber: string } }): Promise<unknown>;
292+
};
293+
294+
await api.requestPasswordResetPhoneNumber({ body: { phoneNumber: PHONE } });
295+
296+
expect(d.settled).toEqual([PHONE]);
297+
}, 60_000);
298+
});
299+
180300
describe('runAfterResponse', () => {
181301
it("hands a task's synchronous throw to onFailure", async () => {
182302
const failures: unknown[] = [];

‎packages/plugins/plugin-auth/src/run-after-response.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
* (`auth-manager-auth-instance.ts`, `sendResetPassword`), and what the send
1313
* does is the server's to log, never the caller's to see — which it already
1414
* never was: better-auth answers `{ status: true }` whatever the send does.
15+
* The phone-number reset request answers the same way, and its reset text is
16+
* sent after the response on the same terms (`auth-manager-plugin-list.ts`,
17+
* `sendPasswordResetOTP`).
1518
*
1619
* ## How
1720
*
@@ -44,12 +47,14 @@ export function runAfterResponse(task: () => Promise<unknown>, onFailure: (error
4447
}
4548

4649
/**
47-
* Wrap a better-auth mail callback — called as `(data, request)` — so that a
50+
* Wrap a better-auth send callback — called as `(data, request)` — so that a
4851
* call carrying its `Request` (every HTTP request: the public door) resolves at
4952
* once and the send runs after the response ({@link runAfterResponse}), its
5053
* failure going to `onFailure`. A call without one (a server-side `auth.api.*`
5154
* call, such as the identity import's) is handed straight to `send` — the same
52-
* promise — and awaited by its caller exactly as before.
55+
* promise — and awaited by its caller exactly as before. A callback better-auth
56+
* hands the endpoint context instead (the phone-number reset's) is called
57+
* with that context's `request`, the value the emailed reset is handed.
5358
*/
5459
export function sendAfterResponseOnPublicDoor<D>(
5560
send: (data: D, request?: unknown) => Promise<unknown>,
@@ -76,11 +81,12 @@ type FailureLogger = {
7681
* `error` when the host logger carries it, else its guaranteed `warn`, else —
7782
* no host logger at all — the console: the level better-auth's own
7883
* background-task log used for the same failure while the send ran inside the
79-
* request. Called through the property, never an extracted reference (a
80-
* class-based logger needs its receiver).
84+
* request. `what` names the send, its medium included (`password-reset
85+
* email`, `phone password-reset text`). Called through the property, never an
86+
* extracted reference (a class-based logger needs its receiver).
8187
*/
8288
export function logAfterResponseSendFailure(logger: unknown, what: string, error: unknown): void {
83-
const message = `[AuthManager] ${what} email was not delivered: ${
89+
const message = `[AuthManager] ${what} was not delivered: ${
8490
error instanceof Error ? error.message : String(error)
8591
}`;
8692
const sink = logger as FailureLogger | undefined;

0 commit comments

Comments
 (0)