diff --git a/.changeset/17234-signin-signup-session-envelope.md b/.changeset/17234-signin-signup-session-envelope.md new file mode 100644 index 0000000000..ce8d290c5d --- /dev/null +++ b/.changeset/17234-signin-signup-session-envelope.md @@ -0,0 +1,47 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): `/sign-in/email` and `/sign-up/email` now attach the `session` their declared `SessionResponse` envelope requires (#17234) + +Both routes answered `{ token, user }` (`/sign-in/email` also carries +`redirect`) with no `session` member anywhere in the body or the response +headers, so `SessionResponseSchema.safeParse` on `auth.login()` / `auth.register()`'s +return value always reported a `data.session` issue — the second of two +departures measured on #17234 (`success` was closed in the previous round). + +**The fix is a read, never an invention.** better-auth stores sessions in the +database by default and `internalAdapter.createSession` is awaited to +completion — including the write — before either endpoint returns its +`{ token, user }` body (measured against the installed `better-auth@1.7.3`, +`dist/db/internal-adapter.mjs:247-319`). So the row the response's own `token` +names is already committed by the time this repo's global `after` hook runs. +The fix reads it back through `internalAdapter.findSession(token)` — the exact +seam `/get-session` already uses for `data.session` — and attaches it. No id or +expiry is ever fabricated; a read that fails for any reason (no +`internalAdapter`, no row, any error) leaves the response exactly as +better-auth wrote it. + +``` +FROM POST /api/v1/auth/sign-in/email -> 200 { redirect, token, user } +TO POST /api/v1/auth/sign-in/email -> 200 { redirect, token, user, session } + +FROM POST /api/v1/auth/sign-up/email -> 200 { token, user } +TO POST /api/v1/auth/sign-up/email -> 200 { token, user, session } +``` + +`session` is the SAME row a following `/get-session` call reads (same `id`, +same `expiresAt`, same `userId`) — one row read twice, not two arrangements — +and `session.token` is the same UNSIGNED credential the body already carried +at `token` / `data.token`, not a second credential this fix introduces. + +⛔ **No wire byte moves on any other member.** `token`, `user`, `redirect` are +byte-identical; `data.token` and the client's auto-`this.token = data.token` +are unchanged and pinned. `auth.me()` / `auth.refreshToken()` (`/get-session`, +#16760) are untouched — this change is scoped to the two credential-issuing +routes. + +This is additive on an already-declared field — `SessionResponseSchema.data.session` +existed in `@objectstack/spec` before this card; the two routes simply did not +serve it. No schema changes, no new exported symbol, no new key on any +published payload. diff --git a/packages/client/src/auth-login-register-envelope.test.ts b/packages/client/src/auth-login-register-envelope.test.ts index 848a3afd40..49175888ab 100644 --- a/packages/client/src/auth-login-register-envelope.test.ts +++ b/packages/client/src/auth-login-register-envelope.test.ts @@ -2,9 +2,12 @@ // // #17234 — `auth.login` and `auth.register` annotate their return as // `SessionResponse` (`BaseResponseSchema.extend(…)`, so `success` is a REQUIRED -// boolean) and delivered a body that never carried `success` at all. Two -// departures were measured on the card; this suite closes one and PINS the -// other as a measurement rather than letting it be invented away. +// boolean, and `data.session` a required `Session`) and delivered a body that +// carried neither. Two departures were measured on the card; this suite now +// closes BOTH: `success` (the previous round) and `data.session` (this one — +// `/sign-in/email` and `/sign-up/email` now attach the session the request +// itself already committed, read back through `internalAdapter.findSession`; +// see `session-envelope-completion.ts` in `plugin-auth`). // // ## Why the server here is the real one // @@ -27,21 +30,22 @@ // - `① the declared envelope is delivered` — the defect proper. The judge is a // PARSE against the declaration, not a key spot-check. // - `② the residue is exhaustive` — `SessionResponseSchema` still does not -// parse, for two reasons that are NOT this card's `success`. Pinned as the -// complete issue list so a regression on `success` shows up here as an extra -// issue instead of hiding inside "it already failed". +// parse, for one reason that is NOT this card's: `data.user.image` (#17235). +// Pinned as the complete issue list so a regression on `success` OR +// `data.session` shows up here as an extra issue instead of hiding inside +// "it already failed". // - `③ the instrument can still fail` — the negative control. The same parse, // on the same returned value with `success` taken back out, must report -// `success` again. Without it, a green ① could equally mean the assertion -// broke. +// `success` again (and still report the `data.user.image` residue). +// Without it, a green ① could equally mean the assertion broke. // - `④ the credential survives byte-identical` — the regression this fix could // most easily have caused. `data.token` is the body's own token, and // `client.token` is still armed from it. -// - `⑤ data.session is not obtainable on these routes` — the card's second -// departure, left OPEN deliberately. This block is the measurement that says -// why: no session in the body, none in the headers, and the value only -// appears on a SECOND call. If better-auth ever starts serving one, this -// block reddens and #17234 can be closed properly. +// - `⑤ data.session is now obtainable on these routes` — the card's second +// departure, closed. The session attached is the SAME row a following +// `/get-session` would read — not a second one, not an invented one — and a +// fabricated body without a `session` member still fails the parse (the +// instrument's negative control, criterion 3(a)). // - `⑥ the raw keys survive the lift` — callers were pushed onto `.user` / // `.token` by the very misdeclaration this card fixes. @@ -217,27 +221,27 @@ describe('[#17234] auth.login / auth.register deliver the SessionResponse envelo }); }); - describe('② the residue is exhaustive, and `success` is not in it', () => { - // Two issues remain on the FULL declared type, and neither is this card's: + describe('② the residue is exhaustive, and neither `success` nor `data.session` is in it', () => { + // ONE issue remains on the FULL declared type, and it is not this card's: // - // data.session — block ⑤: these routes serve none. #17234 stays open. // data.user.image — `SessionUserSchema.image` is `z.string().optional()`, // which does not admit `null`, and better-auth serves // `"image": null` for a user who never set one. Filed // as #17235, and NOT specific to these two methods. // // Pinned as the EXHAUSTIVE list rather than as "it still fails": if - // `success` ever regresses it reappears here as a third issue and these - // cases redden. It is the residue's tripwire, not an acceptance of it. - const RESIDUE = ['data.session', 'data.user.image']; + // `success` OR `data.session` ever regress they reappear here as extra + // issues and these cases redden. It is the residue's tripwire, not an + // acceptance of it. + const RESIDUE = ['data.user.image']; - it('register() reports exactly the two issues that are not `success`', async () => { + it('register() reports exactly the one issue that is not this card\'s', async () => { const { res } = await registered(); const issues = SessionResponseSchema.safeParse(res).error?.issues ?? []; expect(issues.map((i) => i.path.join('.'))).toEqual(RESIDUE); }); - it('login() reports exactly the two issues that are not `success`', async () => { + it('login() reports exactly the one issue that is not this card\'s', async () => { const { res } = await signedIn(); const issues = SessionResponseSchema.safeParse(res).error?.issues ?? []; expect(issues.map((i) => i.path.join('.'))).toEqual(RESIDUE); @@ -256,11 +260,23 @@ describe('[#17234] auth.login / auth.register deliver the SessionResponse envelo success?: unknown; }; const issues = SessionResponseSchema.safeParse(withoutSuccess).error?.issues ?? []; - expect(issues.map((i) => i.path.join('.'))).toEqual([ - 'success', - 'data.session', - 'data.user.image', - ]); + expect(issues.map((i) => i.path.join('.'))).toEqual(['success', 'data.user.image']); + }); + + it('and reports `data.session` again once the fix is taken back out — a fabricated body without a session still fails', async () => { + const { res } = await signedIn(); + + // The other half of criterion 3(a): the SAME instrument, on a body this + // test fabricates rather than one the server returned, with `session` + // removed. This is what makes ① and ⑤'s green readings a real assertion + // rather than a schema that stopped checking `data.session` at all. + const { data, ...rest } = res as unknown as Record & { + data: Record; + }; + const { session: _droppedSession, ...dataWithoutSession } = data; + const issues = + SessionResponseSchema.safeParse({ ...rest, data: dataWithoutSession }).error?.issues ?? []; + expect(issues.map((i) => i.path.join('.'))).toEqual(['data.session', 'data.user.image']); }); }); @@ -312,50 +328,64 @@ describe('[#17234] auth.login / auth.register deliver the SessionResponse envelo }); }); - describe('⑤ `data.session` is not obtainable on these routes — the open half of #17234', () => { - it('neither route carries a session in its body', async () => { + describe('⑤ `data.session` is now obtainable on these routes — #17234 closes', () => { + it('both routes now carry a session in the body, alongside the pre-existing keys', async () => { const reg = await registered(); - expect(Object.keys(reg.wireBody as object)).toEqual(['token', 'user']); - expect(reg.res.data.session).toBeUndefined(); + expect(Object.keys(reg.wireBody as object)).toEqual(['token', 'user', 'session']); + expect(reg.res.data.session).toBeTruthy(); const log = await signedIn(); - expect(Object.keys(log.wireBody as object)).toEqual(['redirect', 'token', 'user']); - expect(log.res.data.session).toBeUndefined(); + expect(Object.keys(log.wireBody as object)).toEqual(['redirect', 'token', 'user', 'session']); + expect(log.res.data.session).toBeTruthy(); }); - it('nor in any response header — the only credential carrier is a bare token', async () => { - const { wireRes } = await signedIn(); - const names = [...wireRes.headers.keys()]; - // Nothing header-side is named for a session payload… - expect(names.filter((n) => /session/i.test(n))).toEqual([]); - // …and the one header that does carry a credential carries a STRING, not - // a session object: no `id`, no `expiresAt`, nothing `SessionSchema` - // would accept. So deriving `data.session` from the headers is not an - // option that was overlooked. - const signed = wireRes.headers.get('set-auth-token') ?? ''; - expect(signed).toBeTruthy(); - expect(signed.trimStart().startsWith('{')).toBe(false); - expect(SessionSchema.safeParse(signed).success).toBe(false); + it('the session parses as the declared SessionSchema and names the right user', async () => { + const reg = await registered(); + const regSession = SessionSchema.safeParse(reg.res.data.session); + expect( + regSession.success, + `register()'s data.session did not parse: ${JSON.stringify(regSession.error?.issues)}`, + ).toBe(true); + expect(reg.res.data.session?.userId).toBe(reg.res.data.user?.id); + + const log = await signedIn(); + const logSession = SessionSchema.safeParse(log.res.data.session); + expect( + logSession.success, + `login()'s data.session did not parse: ${JSON.stringify(logSession.error?.issues)}`, + ).toBe(true); + expect(log.res.data.session?.userId).toBe(log.res.data.user?.id); }); - it('the session exists only one NETWORK CALL later, via /get-session', async () => { + it('is the SAME row a following /get-session reads — a lookup, not an invention', async () => { const { client, res } = await signedIn(); - // The positive leg, and the whole reason this card stays open: the value - // the declared type names is real and reachable — just not on this route. - // Satisfying `data.session` here would mean either a second round trip - // inside `login()` (a behaviour change no ruling has authorised) or a - // fabricated id and expiry (forbidden outright). + // The decisive proof that this is a READ: a second, independent call + // through better-auth's own `/get-session` route names the identical + // session id and expiry as the one attached to the sign-in response — + // one row, read twice, not two arrangements that happen to agree. const me = await client.auth.me(); - const session = SessionSchema.safeParse(me.data.session); - expect( - session.success, - `/get-session did not serve a parseable session: ${JSON.stringify(session.error?.issues)}`, - ).toBe(true); - expect(me.data.session?.userId).toBe(res.data.user?.id); - // …and it really is absent from the sign-in answer, so the two readings - // above are about one session and not two arrangements. - expect(res.data.session).toBeUndefined(); + expect(me.data.session?.id).toBe(res.data.session?.id); + expect(me.data.session?.expiresAt).toBe(res.data.session?.expiresAt); + expect(me.data.session?.userId).toBe(res.data.session?.userId); + }); + + it('the session token is the SAME unsigned credential the body already carried at `data.token`', async () => { + const { res } = await signedIn(); + // `session.token` is the row's own token column — the same unsigned + // string `data.token` already held before this card, not a second + // credential this fix introduced. + expect(res.data.session?.token).toBe(res.data.token); + }); + + it('criterion 3(a): a fabricated body with no session still fails SessionSchema', async () => { + // The negative control on this instrument specifically: SessionSchema + // itself must still be capable of failing, so a green reading above + // cannot equally mean the schema stopped checking `id` / `expiresAt` / + // `userId` at all. + expect(SessionSchema.safeParse(undefined).success).toBe(false); + expect(SessionSchema.safeParse({}).success).toBe(false); + expect(SessionSchema.safeParse({ id: 'x' }).success).toBe(false); // missing expiresAt, userId }); }); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 775119f9c3..0861d9d909 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -1505,14 +1505,18 @@ const SET_AUTH_TOKEN_HEADER = 'set-auth-token'; * session would not change which credential lands here — it would invent a * `token` on a route that served none, which is a different lie. * - * ⚠️ **Known residue — `data.session` on `/sign-in|sign-up/email` (#17234).** - * Those two routes serve no session object and no session id or expiry - * anywhere, body or header, so `login` and `register` return a `data` with no - * `session` and still do not parse as the full declared `SessionResponse`. The - * only place a session is obtainable is a SECOND call to `/get-session` - * (`auth.me`), and manufacturing one here would put a fabricated id and expiry - * under a declared type — the card stays open for that shape decision rather - * than being closed by an invention. + * ⚠️ **`data.session` on `/sign-in|sign-up/email` is no longer residue — + * closed server-side (#17234).** Those two routes now serve a `session` + * member too: `plugin-auth`'s `after` hook (`session-envelope-completion.ts`) + * reads the row `internalAdapter.createSession` already committed, back by + * the response's OWN token — the same seam `/get-session` uses — and attaches + * it, rather than this lift inventing one. So `login` and `register` now + * parse as the full declared `SessionResponse`, with one gap that is NOT + * this: `data.user.image` served `null` against a declared + * `string | undefined` (#17235, tracked separately). This does not touch the + * `data.token` rule above: `session.token` is the SAME unsigned string the + * body's own `token` already carried, not a second credential, and + * `data.token` is still never synthesized FROM a session. * * The `!body` guard is what carries the anonymous answer: `null` is falsy and * is returned untouched rather than wrapped into a signed-in-looking envelope diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index f0362ccea1..a1c5491b20 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -71,6 +71,7 @@ import { } from './impersonation-bearer-rotation.js'; import { echoInstalledSessionToken } from './two-factor-rotated-token-echo.js'; import { resetVerifiedOnTwoFactorReenrollment } from './two-factor-reenrollment-verified-reset.js'; +import { attachSessionToCredentialResponse } from './session-envelope-completion.js'; import { applyPlatformAdminImpersonation, } from './admin-impersonate-endpoint.js'; @@ -2257,6 +2258,19 @@ export class AuthManager { // corrects the echoed VALUE only; resolver precedence is untouched. await echoInstalledSessionToken(ctx); + // ── #17234: complete the SessionResponse envelope with the session + // the route just committed ──────────────────────────────────────── + // `/sign-in/email` and `/sign-up/email` answer `{ token, user }` + // (plus `redirect` on sign-in) with no `session` member anywhere in + // the body or the headers, so `SessionResponseSchema` never parsed + // either method's return value. The session is not absent — it is + // the row `internalAdapter.createSession` already committed before + // the endpoint returned — so this reads it back by the response's + // own token, the same seam `/get-session` uses, and attaches it. + // See `session-envelope-completion.ts` for the full measurement and + // why this can never fabricate an id or an expiry. + await attachSessionToCredentialResponse(ctx); + // ── #10700: `verified` must describe the secret stored beside it ── // A second `/two-factor/enable` on an already-confirmed account // rewrites the TOTP secret on the one `sys_two_factor` row the diff --git a/packages/plugins/plugin-auth/src/session-envelope-completion.ts b/packages/plugins/plugin-auth/src/session-envelope-completion.ts new file mode 100644 index 0000000000..e701c4763e --- /dev/null +++ b/packages/plugins/plugin-auth/src/session-envelope-completion.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #17234 — `/sign-in/email` and `/sign-up/email` answer `{ token, user }` + * (sign-in also carries `redirect`), and no member of that body, and no + * response header, is a `Session` — `SessionResponseSchema.safeParse` on + * either method's return value therefore always reported a `data.session` + * issue, alongside the `success` gap the previous round closed. + * + * ## The ruling (director seat, decision batch #125 item 4) + * + * > Returning the session on sign-in is the mainstream shape; the server + * > holds the session it just created (the token is its credential), so one + * > request can answer the declared envelope. + * + * The measurement that makes this possible: better-auth stores sessions in + * the database by default (`storeSessionInDatabase`; this deployment wires no + * `secondaryStorage` — see `auth-manager.ts`'s `deleteUser` note), and + * `internalAdapter.createSession` is `await`-ed to completion — including the + * database write — before the sign-in/sign-up ENDPOINT returns + * `{ token, user }` at all (`better-auth@1.7.3` + * `dist/db/internal-adapter.mjs:247-319`). So by the time this repo's global + * `after` hook runs (`ctx.context.returned` already holds the endpoint's + * answer), the row the token names is not merely creatABLE — it is already + * committed. Reading it back is exactly what `/get-session` already does + * (`internalAdapter.findSession`, `dist/db/internal-adapter.mjs:321-358`), so + * this produces the SAME shape `data.session` already carries on that route + * (`auth-get-session-envelope.test.ts`), not a second declaration of it. + * + * ## Why this is a READ, not an invention + * + * The session attached here is never synthesized: `freshSessionForToken` + * looks up the row by the UNSIGNED token the response body already + * published, through the same `internalAdapter.findSession` seam + * `/get-session` uses. If the row is not there — a future better-auth + * version that defers the write, a `secondaryStorage`-only deployment, + * anything this comment did not anticipate — the response is left exactly as + * the vendor wrote it. No id or expiry is ever fabricated, matching the + * `⛔ Do not invent a session` line in the ruling that authorised this file. + * + * ## Scope + * + * `/sign-up/email` only reaches this when the vendor actually minted a + * session for it — i.e. `autoSignIn` is on and the route's own body carries a + * `token` (a sign-up that requires email verification first, or one done with + * `autoSignIn: false`, serves no token and this is a no-op, matching + * `credentialTokenPayload`'s guard). `/sign-in/email` always mints one on + * success. Both are checked by PATH TABLE, mirroring + * `two-factor-rotated-token-echo.ts`'s `ROTATING_TWO_FACTOR_VERIFY_PATHS` + * rather than one bespoke `if` per route. + */ + +/** The two credential-issuing routes this repo can complete a session on. */ +export const CREDENTIAL_RESPONSE_PATHS: readonly string[] = ['/sign-in/email', '/sign-up/email']; + +/** Did the route succeed, and does its payload echo a bare credential token? */ +async function credentialTokenPayload(ctx: any): Promise<{ token: string } | undefined> { + const returned = ctx?.context?.returned; + if (!returned || typeof returned !== 'object') return undefined; + try { + const { isAPIError } = await import('better-auth/api'); + if (isAPIError(returned)) return undefined; + } catch { + if (returned instanceof Error) return undefined; + } + return typeof (returned as any).token === 'string' && (returned as any).token + ? (returned as any) + : undefined; +} + +/** + * The session row better-auth just created for `token`, in the exact shape + * `/get-session` already serves under `data.session` — or `undefined` when it + * cannot be read (no session for the token, no `internalAdapter` on this + * context, or a read failure of any kind). + */ +async function freshSessionForToken( + ctx: any, + token: string, +): Promise | undefined> { + const findSession = ctx?.context?.internalAdapter?.findSession; + if (typeof findSession !== 'function') return undefined; + const found = await findSession(token).catch(() => null); + if (!found || typeof found !== 'object') return undefined; + const session = (found as { session?: unknown }).session; + if (!session || typeof session !== 'object') return undefined; + return session as Record; +} + +/** + * Complete the `SessionResponse` envelope on `/sign-in/email` and + * `/sign-up/email` by attaching the `session` member their declared type + * names — read back from the row the same request already committed, never + * fabricated. + * + * Never throws, and never overwrites a `session` the vendor (or a plugin + * ahead of this hook) already put on the payload: a read that fails for any + * reason leaves the response exactly as it would have been without this file, + * which is the honest fallback the ruling names for the case a synchronous + * read genuinely is not possible. + */ +export async function attachSessionToCredentialResponse(ctx: any): Promise { + try { + if (!CREDENTIAL_RESPONSE_PATHS.includes(ctx?.path)) return; + const payload = await credentialTokenPayload(ctx); + if (!payload) return; + if ((payload as Record).session !== undefined) return; + const session = await freshSessionForToken(ctx, payload.token); + if (!session) return; + (payload as Record).session = session; + } catch { + /* leave the payload exactly as the vendor route wrote it */ + } +}