diff --git a/.changeset/account-oauth-tokens-internal.md b/.changeset/account-oauth-tokens-internal.md new file mode 100644 index 0000000000..0a0c76fe99 --- /dev/null +++ b/.changeset/account-oauth-tokens-internal.md @@ -0,0 +1,72 @@ +--- +"@objectstack/platform-objects": patch +"@objectstack/plugin-auth": patch +--- + +fix(security): `sys_account`'s OAuth access/refresh/id tokens stop serializing on the data API — `internal: true`, with better-auth's readback seam widened to cover them (#7987) + + + +`sys_account.access_token`, `.refresh_token` and `.id_token` hold each user's +**live third-party OAuth credentials** — the tokens ObjectStack received from +Google, GitHub or an OIDC IdP — in cleartext (better-auth's +`account.encryptOAuthTokens` is not set, so `setTokenUtil` stores them +verbatim). They were plain `Field.textarea` on an object declaring +`apiEnabled: true, apiMethods: ['get','list']`. + +**Both personas were measured leaking, on a real booted stack** (`bootStack(showcaseStack)`, +in-process HTTP + sqlite-wasm), with a planted token on a member's account row: + +- **admin**, `GET /data/sys_account/{another user's account id}` — 200, that + member's `refresh_token` verbatim, plus `access_token` and `id_token`; +- **member**, `GET /data/sys_account` (self-scoped by the `sys_account_self` RLS + policy) — 200, their **own** `refresh_token` verbatim. + +The member arm is the one this object does not share with its `sys_session` +sibling (#7823), and it is the sharper of the two: it converts a short-lived, +revocable ObjectStack session bearer into a **long-lived third-party refresh +token that this platform cannot revoke at all**. Neither collector reached these +columns — the engine's credential mask collects by field TYPE (`textarea` is +neither `secret` nor `password`) *and* exempts objects with +`managedBy: 'better-auth'`, which this object is. + +**The fix is three declarations plus one widening**, inheriting #7823's shape +rather than inventing a second mechanism: + +- the three columns are declared `internal: true` — the opt-in, type-independent + flag minted by #7728 meaning *the declared value is never returned on the + generic data path*. Storage, filtering and indexing are untouched: the strip + runs on rows the driver has already produced. +- better-auth **reads these back off adapter result rows** — measured, and the + risk this card was parked on: `internalAdapter.findAccounts(userId)` issues a + `findMany` with no projection, and `/get-access-token`, `/account-info` and + `/refresh-token` then read `account.refreshToken` / `.accessToken` / + `.idToken` off those rows. The read strip alone would answer + `REFRESH_TOKEN_NOT_FOUND` (400) and hand back an empty access token. So the + existing readback seam in `@objectstack/plugin-auth` — which already recovered + `sys_session.token` through `Engine.resolveInternalField` (#8118's privileged + batch accessor) — is widened to cover these three columns and renamed + accordingly. No engine carve-out, no second accessor. + +**Not retyped, deliberately.** `Field.secret()` would route better-auth's own +writes through the engine's encrypt-on-write path, placing the engine between +better-auth and its own adapter. `Field.password()` is inert here for the two +reasons above. + +**`password` / `previous_password_hashes` are deliberately out of scope** — +they are better-auth one-way hashes (ADR-0100's third channel), not reversible +outbound credentials, and the readback seam refuses to touch them. + +The regression proof drives both directions: the fixture PLANTS real token +values and re-reads them out of storage through the privileged accessor before +asserting anything (so "absent from the response" cannot pass vacuously), then +pins that the values are still on disk, still usable as a server-side predicate, +and that password sign-in — which reads a `sys_account` row back through the +same seam on every request — still works. diff --git a/packages/platform-objects/src/apps/translations/en.objects.generated.ts b/packages/platform-objects/src/apps/translations/en.objects.generated.ts index 859bab8156..bf2fdba20f 100644 --- a/packages/platform-objects/src/apps/translations/en.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/en.objects.generated.ts @@ -398,13 +398,16 @@ export const enObjects: NonNullable = { help: "Link to user table" }, access_token: { - label: "Access Token" + label: "Access Token", + help: "Live OAuth access token issued by the provider — never returned on the data API (#7987); better-auth reads it back through the engine's privileged internal-field accessor" }, refresh_token: { - label: "Refresh Token" + label: "Refresh Token", + help: "Live OAuth refresh token — long-lived and not revoked by revoking an ObjectStack session; never returned on the data API (#7987)" }, id_token: { - label: "ID Token" + label: "ID Token", + help: "OIDC ID token issued by the provider — never returned on the data API (#7987)" }, access_token_expires_at: { label: "Access Token Expires At" diff --git a/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts b/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts index c294ff6373..dc91ac30a5 100644 --- a/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/es-ES.objects.generated.ts @@ -398,13 +398,16 @@ export const esESObjects: NonNullable = { help: "Enlace a la tabla de usuarios." }, access_token: { - label: "Token de acceso" + label: "Token de acceso", + help: "Live OAuth access token issued by the provider — never returned on the data API (#7987); better-auth reads it back through the engine's privileged internal-field accessor" }, refresh_token: { - label: "Token de actualización" + label: "Token de actualización", + help: "Live OAuth refresh token — long-lived and not revoked by revoking an ObjectStack session; never returned on the data API (#7987)" }, id_token: { - label: "Token de ID" + label: "Token de ID", + help: "OIDC ID token issued by the provider — never returned on the data API (#7987)" }, access_token_expires_at: { label: "El token de acceso caduca el" diff --git a/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts b/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts index 50b808898d..9fbeb2b593 100644 --- a/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/ja-JP.objects.generated.ts @@ -398,13 +398,16 @@ export const jaJPObjects: NonNullable = { help: "ユーザーテーブルへのリンク" }, access_token: { - label: "アクセストークン" + label: "アクセストークン", + help: "Live OAuth access token issued by the provider — never returned on the data API (#7987); better-auth reads it back through the engine's privileged internal-field accessor" }, refresh_token: { - label: "リフレッシュトークン" + label: "リフレッシュトークン", + help: "Live OAuth refresh token — long-lived and not revoked by revoking an ObjectStack session; never returned on the data API (#7987)" }, id_token: { - label: "ID トークン" + label: "ID トークン", + help: "OIDC ID token issued by the provider — never returned on the data API (#7987)" }, access_token_expires_at: { label: "アクセストークン有効期限" diff --git a/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts b/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts index c8200a999f..ce644b40e4 100644 --- a/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts +++ b/packages/platform-objects/src/apps/translations/zh-CN.objects.generated.ts @@ -398,13 +398,16 @@ export const zhCNObjects: NonNullable = { help: "关联到用户表" }, access_token: { - label: "访问令牌" + label: "访问令牌", + help: "Live OAuth access token issued by the provider — never returned on the data API (#7987); better-auth reads it back through the engine's privileged internal-field accessor" }, refresh_token: { - label: "刷新令牌" + label: "刷新令牌", + help: "Live OAuth refresh token — long-lived and not revoked by revoking an ObjectStack session; never returned on the data API (#7987)" }, id_token: { - label: "ID 令牌" + label: "ID 令牌", + help: "OIDC ID token issued by the provider — never returned on the data API (#7987)" }, access_token_expires_at: { label: "Access Token 过期时间" diff --git a/packages/platform-objects/src/identity/sys-account.object.ts b/packages/platform-objects/src/identity/sys-account.object.ts index e270b5be26..ef28879f54 100644 --- a/packages/platform-objects/src/identity/sys-account.object.ts +++ b/packages/platform-objects/src/identity/sys-account.object.ts @@ -178,19 +178,72 @@ export const SysAccount = ObjectSchema.create({ description: 'Link to user table', }), + // ── Live third-party credentials (never on the generic data path) ────── + // + // [#7987] These three columns hold the user's LIVE bearer credentials for + // SOMEONE ELSE'S service — the tokens ObjectStack received from Google, + // GitHub, an OIDC IdP — in cleartext. better-auth writes them plain here: + // its `account.encryptOAuthTokens` option is not set (see + // `AUTH_ACCOUNT_CONFIG` in plugin-auth's `auth-schema-config.ts`), so + // `setTokenUtil` stores the value verbatim. + // + // Before `internal: true` they serialized on the generic data path, on an + // object that declares `apiEnabled: true, apiMethods: ['get','list']`: + // + // - an ADMIN read every user's tokens (list, get-by-id, `?select=`); + // - a MEMBER read their own, because the `sys_account_self` RLS policy + // (`plugin-security/objects/default-permission-sets.ts`) grants + // `select` on `user_id == current_user.id`. That arm is the one this + // object does NOT share with `sys_session`: it converts a short-lived, + // revocable ObjectStack session bearer into the user's long-lived + // third-party REFRESH token, which survives revocation here entirely. + // + // Neither collector reached them: `maskSecretFields` collects by field + // TYPE (`textarea` is not `secret`/`password`) *and* exempts objects with + // `managedBy: 'better-auth'` — which this object is — so the one mask that + // could have applied was exempt by construction (#7902's survey result). + // + // `internal: true` is the same flag #7728 minted for `sys_api_key.key` and + // #7823 applied to `sys_session.token`: the engine OMITS the key from + // find/findOne results, on the default projection and when a client names + // the column in `?select=`. Storage, filtering and indexing are untouched. + // + // ⛔ NOT retyped to `Field.secret()`, deliberately. better-auth owns every + // write to this object through its own adapter; routing them through the + // engine's encrypt-on-write path would sit between better-auth and its + // adapter. `Field.password()` is inert for the two reasons above (type-keyed + // collection + the `better-auth` exemption). + // + // ⚠️ better-auth READS these back off adapter result rows — measured, and + // the load-bearing risk this card was parked on: + // `internalAdapter.findAccounts(userId)` (no projection) feeds + // `resolveUserAccount`, and `/get-access-token`, `/account-info` and + // `/refresh-token` then read `account.refreshToken` / `.accessToken` / + // `.idToken` off those rows. The read strip alone would make the refresh + // exchange answer `REFRESH_TOKEN_NOT_FOUND` and hand back an empty access + // token. They are re-attached at better-auth's own storage seam through + // the privileged accessor — `internal-field-readback.ts` in plugin-auth, + // over `Engine.resolveInternalField` (#8118) — exactly as #7823 did for + // `sys_session.token`. ⛔ Do not add an engine-side carve-out. access_token: Field.textarea({ label: 'Access Token', required: false, + internal: true, + description: "Live OAuth access token issued by the provider — never returned on the data API (#7987); better-auth reads it back through the engine's privileged internal-field accessor", }), - + refresh_token: Field.textarea({ label: 'Refresh Token', required: false, + internal: true, + description: 'Live OAuth refresh token — long-lived and not revoked by revoking an ObjectStack session; never returned on the data API (#7987)', }), - + id_token: Field.textarea({ label: 'ID Token', required: false, + internal: true, + description: 'OIDC ID token issued by the provider — never returned on the data API (#7987)', }), access_token_expires_at: Field.datetime({ diff --git a/packages/plugins/plugin-auth/src/internal-field-readback.test.ts b/packages/plugins/plugin-auth/src/internal-field-readback.test.ts new file mode 100644 index 0000000000..743cf893eb --- /dev/null +++ b/packages/plugins/plugin-auth/src/internal-field-readback.test.ts @@ -0,0 +1,256 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #7823 + #7987 — the internal-field READBACK seam, unit-pinned from both +// directions. +// +// The engine's `internal: true` read strip removes the flagged column from +// every find/findOne result; better-auth reads those columns back OFF adapter +// results — `sys_session.token` on the session-lifecycle routes +// (revoke-other-sessions filters by it, sliding refresh and expired cleanup +// delete/update by it), and `sys_account`'s three OAuth columns on the +// token-exchange routes (`findAccounts` → `resolveUserAccount` → +// `getValidAccessToken` / `/refresh-token`). This module re-attaches the values +// through `Engine.resolveInternalField` (#8118's privileged batch accessor). +// The end-to-end proofs live in the dogfood suite; THIS file pins the seam's +// own contract: +// +// - re-attach only for the two declared objects, only for rows the strip +// actually hit, only when the caller's projection did not exclude the +// column; +// - one batched privileged read per COLUMN per page, never one per row; +// - FAIL CLOSED and loud when a stripped row meets an engine with no +// accessor — that state is exactly what turns a security control into a +// silent no-op and an OAuth refresh into a 400, so it must never pass +// quietly. + +import { describe, it, expect, vi } from 'vitest'; +import { reattachInternalFieldsOnRead } from './internal-field-readback.js'; + +const resolver = (map: Record>) => + vi.fn(async (_object: string, ids: readonly string[], field: string) => { + const out = new Map(); + for (const id of ids) if (id in map && field in map[id]!) out.set(id, map[id]![field]); + return out; + }); + +describe('#7823 reattachInternalFieldsOnRead — sys_session.token', () => { + it('re-attaches the token to stripped sys_session rows — one batched call', async () => { + const resolveInternalField = resolver({ s1: { token: 'tok-1' }, s2: { token: 'tok-2' } }); + const rows: any[] = [ + { id: 's1', user_id: 'u1' }, + { id: 's2', user_id: 'u1' }, + ]; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', rows); + expect(rows[0].token).toBe('tok-1'); + expect(rows[1].token).toBe('tok-2'); + expect(resolveInternalField).toHaveBeenCalledTimes(1); + expect(resolveInternalField).toHaveBeenCalledWith('sys_session', ['s1', 's2'], 'token'); + }); + + it('handles the findOne shape (a single row, not an array)', async () => { + const resolveInternalField = resolver({ s1: { token: 'tok-1' } }); + const row: any = { id: 's1', user_id: 'u1' }; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', row); + expect(row.token).toBe('tok-1'); + }); + + it('never touches an object with no readback entry, and issues no privileged read', async () => { + // `sys_api_key.key` is flagged `internal` too — and deliberately NOT in the + // table: its mint route returns the plaintext it generated itself and never + // reads the stored hash back (#7728). A seam that re-attached every flagged + // column everywhere would undo that. + const resolveInternalField = resolver({ k1: { key: 'HASH' } }); + const row: any = { id: 'k1' }; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_api_key', row); + expect(row).toEqual({ id: 'k1' }); + expect(resolveInternalField).not.toHaveBeenCalled(); + }); + + it('rows still carrying `token` are left byte-identical and trigger no privileged read', async () => { + // Fake engines in adapter tests (and any engine without the strip) return + // the row whole — the seam must be inert there. + const resolveInternalField = resolver({ s1: { token: 'REPLACED' } }); + const row: any = { id: 's1', token: 'original' }; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', row); + expect(row.token).toBe('original'); + expect(resolveInternalField).not.toHaveBeenCalled(); + }); + + it('a projection that deliberately excluded `token` keeps its projection', async () => { + const resolveInternalField = resolver({ s1: { token: 'tok-1' } }); + const row: any = { id: 's1' }; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', row, ['id', 'expires_at']); + expect('token' in row).toBe(false); + expect(resolveInternalField).not.toHaveBeenCalled(); + // …but a projection that NAMED the column gets it back. + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', row, ['id', 'token']); + expect(row.token).toBe('tok-1'); + }); + + it('FAILS CLOSED: a stripped session row plus an engine with no accessor throws loudly', async () => { + const row: any = { id: 's1', user_id: 'u1' }; + await expect( + reattachInternalFieldsOnRead({}, 'sys_session', row), + ).rejects.toThrow(/resolveInternalField/); + // The message names the consequence and the remedy — this is the state + // that makes revoke-other-sessions a 200 that revokes nothing. + await expect( + reattachInternalFieldsOnRead({}, 'sys_session', row), + ).rejects.toThrow(/revoke-other-sessions/); + }); + + it('…but an engine with no accessor and NO stripped rows stays quiet (inert seam)', async () => { + const row: any = { id: 's1', token: 'tok' }; + await expect(reattachInternalFieldsOnRead({}, 'sys_session', row)).resolves.toBeUndefined(); + }); + + it('a row deleted between the read and the dereference stays token-less', async () => { + const resolveInternalField = resolver({ s1: { token: 'tok-1' } }); // s2 vanished + const rows: any[] = [{ id: 's1' }, { id: 's2' }]; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', rows); + expect(rows[0].token).toBe('tok-1'); + expect('token' in rows[1]).toBe(false); + }); + + it('non-record members and id-less rows are skipped, not judged', async () => { + const resolveInternalField = resolver({ s1: { token: 'tok-1' } }); + const rows: any[] = [{ id: 's1' }, null, 'noise', { user_id: 'u1' }]; + await expect( + reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_session', rows), + ).resolves.toBeUndefined(); + expect(rows[0].token).toBe('tok-1'); + expect(resolveInternalField).toHaveBeenCalledWith('sys_session', ['s1'], 'token'); + }); +}); + +describe('#7987 reattachInternalFieldsOnRead — sys_account OAuth columns', () => { + const ACCOUNT_ROW = () => ({ id: 'a1', user_id: 'u1', provider_id: 'google' }); + + it('re-attaches ALL THREE columns — one batched call per column, not per row', async () => { + // This is the read `internalAdapter.findAccounts(userId)` issues (no + // projection), which feeds `/get-access-token`, `/account-info` and + // `/refresh-token`. + const resolveInternalField = resolver({ + a1: { access_token: 'at-1', refresh_token: 'rt-1', id_token: 'it-1' }, + a2: { access_token: 'at-2', refresh_token: 'rt-2', id_token: 'it-2' }, + }); + const rows: any[] = [ + { id: 'a1', user_id: 'u1', provider_id: 'google' }, + { id: 'a2', user_id: 'u1', provider_id: 'github' }, + ]; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', rows); + + expect(rows[0].access_token).toBe('at-1'); + expect(rows[0].refresh_token).toBe('rt-1'); + expect(rows[0].id_token).toBe('it-1'); + expect(rows[1].refresh_token).toBe('rt-2'); + // Three columns, two rows ⇒ THREE reads, not six. The accessor resolves one + // field per call by contract (#8118); the batching that matters is per-page. + expect(resolveInternalField).toHaveBeenCalledTimes(3); + expect(resolveInternalField).toHaveBeenCalledWith('sys_account', ['a1', 'a2'], 'access_token'); + expect(resolveInternalField).toHaveBeenCalledWith('sys_account', ['a1', 'a2'], 'refresh_token'); + expect(resolveInternalField).toHaveBeenCalledWith('sys_account', ['a1', 'a2'], 'id_token'); + }); + + it('the refresh token specifically survives the round trip (the exchange input)', async () => { + // `/refresh-token` answers REFRESH_TOKEN_NOT_FOUND (400) when this single + // property is undefined, so it gets its own assertion rather than riding + // on the batch one above. + const resolveInternalField = resolver({ a1: { refresh_token: 'rt-live' } }); + const row: any = ACCOUNT_ROW(); + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', row); + expect(row.refresh_token).toBe('rt-live'); + }); + + it('a column with no stored value comes back null, not missing', async () => { + // A password-only account has no OAuth tokens; the accessor answers `null` + // for an unset column. better-auth's `account.refreshToken` check reads + // false either way, but the KEY must exist so the row is not mistaken for + // a stripped one on a later pass. + const resolveInternalField = resolver({ + a1: { access_token: null, refresh_token: null, id_token: null }, + }); + const row: any = ACCOUNT_ROW(); + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', row); + expect(row.access_token).toBeNull(); + expect('refresh_token' in row).toBe(true); + }); + + it('the projection guard is PER COLUMN', async () => { + const resolveInternalField = resolver({ + a1: { access_token: 'at-1', refresh_token: 'rt-1', id_token: 'it-1' }, + }); + const row: any = ACCOUNT_ROW(); + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', row, [ + 'id', + 'access_token', + ]); + expect(row.access_token).toBe('at-1'); + // The two the caller did not name keep their projection. + expect('refresh_token' in row).toBe(false); + expect('id_token' in row).toBe(false); + expect(resolveInternalField).toHaveBeenCalledTimes(1); + }); + + it('a partially stripped row only resolves the columns that are missing', async () => { + const resolveInternalField = resolver({ + a1: { access_token: 'at-1', refresh_token: 'rt-1', id_token: 'it-1' }, + }); + const row: any = { ...ACCOUNT_ROW(), refresh_token: 'already-here' }; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', row); + expect(row.refresh_token).toBe('already-here'); + expect(row.access_token).toBe('at-1'); + expect(resolveInternalField).toHaveBeenCalledTimes(2); + }); + + it('⛔ never re-attaches `password` or `previous_password_hashes`', async () => { + // The card's explicit scope guard: those are one-way hashes (ADR-0100's + // third channel), they are not flagged `internal`, and the accessor would + // refuse them anyway. Pinned here so a future widening of the table has to + // walk past a red test. + const resolveInternalField = resolver({ a1: { password: 'HASH' } }); + const row: any = ACCOUNT_ROW(); + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_account', row); + expect('password' in row).toBe(false); + expect('previous_password_hashes' in row).toBe(false); + for (const call of resolveInternalField.mock.calls) { + expect(call[2]).not.toBe('password'); + expect(call[2]).not.toBe('previous_password_hashes'); + } + }); + + it('an engine with NO accessor is left alone — absence is ordinary on these columns', async () => { + // The asymmetry with `sys_session.token` above, and the reason it exists. + // These three columns are `required: false` and are empty on every + // credential (password) account, so a missing key does NOT prove the strip + // ran. Throwing here broke ordinary sign-in against the in-memory fake + // engines — measured: 16 red tests across session-of-record, + // session-tombstone and impersonation-bearer-rotation, because + // better-auth's `findCredentialAccount` reads exactly such a row on the + // sign-in path. An engine with no `resolveInternalField` does not + // implement the `internal` channel at all, so it never stripped anything. + const row: any = ACCOUNT_ROW(); + await expect(reattachInternalFieldsOnRead({}, 'sys_account', row)).resolves.toBeUndefined(); + expect('access_token' in row).toBe(false); + }); + + it('…while the SESSION column keeps failing closed on the same engine (posture is per column)', async () => { + // Guards the discriminator itself: a future edit that made the account + // columns inert by making the whole seam inert would take #7823's + // fail-closed contract with it, silently. + await expect( + reattachInternalFieldsOnRead({}, 'sys_session', { id: 's1' }), + ).rejects.toThrow(/resolveInternalField/); + }); + + it('…and stays inert for account rows that still carry their columns', async () => { + const row: any = { + ...ACCOUNT_ROW(), + access_token: 'at', + refresh_token: 'rt', + id_token: 'it', + }; + await expect(reattachInternalFieldsOnRead({}, 'sys_account', row)).resolves.toBeUndefined(); + expect(row.access_token).toBe('at'); + }); +}); diff --git a/packages/plugins/plugin-auth/src/internal-field-readback.ts b/packages/plugins/plugin-auth/src/internal-field-readback.ts new file mode 100644 index 0000000000..ecefebdba0 --- /dev/null +++ b/packages/plugins/plugin-auth/src/internal-field-readback.ts @@ -0,0 +1,243 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7823, widened by #7987] The internal-field READBACK seam — better-auth's + * own routes get the `internal`-stripped credential columns back, through the + * engine's privileged accessor, at the one layer we own. + * + * ## What this seam is for + * + * A column declared `internal: true` is omitted by the engine's generic read + * path from every find/findOne result — that is the fix for the disclosures + * #7823 and #7987 exist for, and it stays. But better-auth's storage adapter + * is implemented OVER that same read path (`objectql-adapter.ts` → + * `dataEngine.find`/`findOne`), and several of better-auth's own routes read + * those columns back OFF the rows it returns. The strip alone starves them. + * + * This module re-attaches the stored values to the rows the adapter hands + * better-auth, so every one of those readbacks sees the row whole while the + * generic data API keeps returning rows without the column. + * + * ## The two objects, and why each is here (both MEASURED, not assumed) + * + * **`sys_session.token`** (#7823): + * + * - `revoke-other-sessions` filters `listSessions(userId)` rows by + * `session.token !== ctx.context.session.session.token` and deletes by + * token. With every row's `token` undefined the filter yields nothing — + * measured: `POST /auth/revoke-other-sessions` answered + * `200 {"status":true}` while the user's other session KEPT AUTHENTICATING. + * A security control reporting success while doing nothing. + * - sliding-expiry refresh (`updateSession(session.session.token, …)`) and + * expired-session cleanup (`deleteSession(…)`) read the token off the + * context session, itself hydrated from an adapter read — same silent + * no-op shape. + * + * **`sys_account.access_token` / `.refresh_token` / `.id_token`** (#7987) — + * the OAuth credentials for the user's linked providers. better-auth reads + * these off adapter result rows on the token-exchange paths, traced in + * `better-auth/dist/api/routes/account.mjs`: + * + * - `internalAdapter.findAccounts(userId)` issues `findMany` with **no + * projection**, so the row set is exactly what the strip empties; + * - `resolveUserAccount()` picks a row out of it, and `getValidAccessToken()` + * (behind `/get-access-token` and `/account-info`) then reads + * `account.refreshToken` to decide whether to refresh, `account.accessToken` + * to answer with, and `account.idToken` to carry forward; + * - `POST /refresh-token` reads `account.refreshToken` and answers + * `REFRESH_TOKEN_NOT_FOUND` (400) when it is absent. + * + * So without this seam the strip would not merely hide the OAuth tokens, it + * would make the refresh EXCHANGE fail and `/get-access-token` hand back an + * empty string. That is the risk #7987 was parked on, and the reason the + * mechanism is a readback rather than a bare flag. + * + * ## The shape (maintainer ruling 2026-08-13, Q2: compose) + * + * `Engine.resolveInternalField` — the purpose-built privileged batch accessor + * #8118 landed — recovers the stored value for a batch of row ids. ⛔ NOT a + * second accessor, ⛔ NOT an engine carve-out: the engine's read path stays + * carve-out-free (#7728's design), and the privileged dereference happens + * here, in the identity authority's own storage seam — the same placement as + * `sso-client-secret.ts`'s `injectClientSecretOnRead` (#8009) and the same + * raw-engine access rule: `withSystemContext` deliberately exposes CRUD verbs + * only, so the privileged verb comes off the RAW engine. + * + * The accessor resolves ONE field per call by contract, so an object with + * three flagged columns costs three id-batched driver reads per page. That is + * consumed as-is, deliberately: widening the accessor to a field SET would + * restructure a surface `@objectstack/service-messaging` and other consumers + * share, to save two indexed point-reads on a path already dominated by the + * provider round trip and the password KDF. The batching that matters — + * one read per FIELD per page rather than one per ROW — is already there. + * + * ## Fail-closed, loudly + * + * A row that comes back missing a flagged column from an engine that offers no + * `resolveInternalField` is exactly the state that turns `revoke-other-sessions` + * into a silent no-op and an OAuth refresh into a 400 — so it throws + * (composition error, named remedy) instead of degrading. Rows that still + * carry the column (a fake engine in tests, an engine without the strip) are + * left untouched and trigger no privileged read at all, so the seam is inert + * everywhere the strip is. + */ + +import { SystemObjectName } from '@objectstack/spec/system'; + +/** + * Engine surface this seam needs. The verb is separately named and privileged + * (#8118) precisely so it cannot be reached from a query string; it comes off + * the RAW engine, never the `withSystemContext` wrapper. + */ +export interface InternalFieldResolvingEngine { + resolveInternalField?( + object: string, + recordIds: readonly string[], + field: string, + ): Promise>; +} + +interface ReadbackColumn { + /** The flagged column, by its ObjectStack (snake_case) name. */ + readonly field: string; + /** + * Does the ABSENCE of this key from a result row prove the engine's strip + * ran? Only when the column's own declaration forbids a row from lacking it. + * + * This is the seam's one non-obvious discriminator, and getting it wrong is + * measurable in both directions. Absence has two possible causes — the strip + * removed it, or the row never carried it — and the seam cannot tell them + * apart by looking. What settles it is the DECLARATION: + * + * - `sys_session.token` is `required: true`. A session row without a token + * does not exist, so absence can only be the strip ⇒ absence is a + * reliable signal and this seam fails closed on it. + * - `sys_account`'s three OAuth columns are `required: false`, and are + * genuinely empty on every credential (password) account — the ordinary + * case. Treating absence as proof of the strip there means an engine + * without the accessor throws on ordinary sign-in: measured, it broke 16 + * session/impersonation tests against the in-memory fake engines, because + * better-auth's `findCredentialAccount` reads a password account on the + * sign-in path and those rows never had a token column to begin with. + * + * For a column marked `false`, the seam therefore falls back to the engine's + * own capability: it recovers the value when the engine offers the accessor, + * and stays inert when it does not (an engine with no `resolveInternalField` + * does not implement the `internal` channel, so it never stripped anything). + * The residual risk that buys — a version-skewed engine that strips but + * predates #8118 — degrades LOUDLY for these columns, with the token routes + * answering `REFRESH_TOKEN_NOT_FOUND` (400), where the session column would + * have degraded into a security control silently reporting success. + */ + readonly absenceProvesStrip: boolean; +} + +/** + * The columns this seam re-attaches, per object. + * + * Bounded on purpose — this is NOT "re-attach every `internal` field the + * registry knows". The entries here are the ones whose consumer was traced + * into better-auth's own route code (see the module header); a flagged column + * nobody reads back must stay stripped everywhere, which is the whole point of + * the flag. `sys_api_key.key` is deliberately absent: its mint route returns + * the plaintext it generated itself and never reads the stored hash back + * (#7728). + */ +const READBACK_FIELDS: Readonly> = { + [SystemObjectName.SESSION]: [{ field: 'token', absenceProvesStrip: true }], + // [#7987] All three OAuth credential columns. `password` / + // `previous_password_hashes` are NOT here and must not be: they are + // better-auth one-way hashes (ADR-0100's third channel), they are not + // flagged `internal`, and the accessor refuses any field that is not. + [SystemObjectName.ACCOUNT]: [ + { field: 'access_token', absenceProvesStrip: false }, + { field: 'refresh_token', absenceProvesStrip: false }, + { field: 'id_token', absenceProvesStrip: false }, + ], +}; + +/** What breaks if a stripped row is handed to better-auth un-repaired. */ +const FAIL_CLOSED_CONSEQUENCE: Readonly> = { + [SystemObjectName.SESSION]: + 'better-auth session-lifecycle routes (revoke-other-sessions, sliding-expiry refresh, ' + + 'expired-session cleanup) would silently no-op on such rows', + [SystemObjectName.ACCOUNT]: + 'better-auth OAuth token routes (/get-access-token, /account-info, /refresh-token) would ' + + 'fail to exchange the refresh token — answering REFRESH_TOKEN_NOT_FOUND, or handing back ' + + 'an empty access token — on such rows', +}; + +/** + * Re-attach an object's `internal: true` columns to adapter read results, in + * place. + * + * @param engine The RAW data engine (privileged verb holder). + * @param objectName Protocol object name of the model just read. + * @param rows The row (findOne) or rows (findMany) about to be + * handed to better-auth. Mutated in place. + * @param requestedFields The caller's projection, if it named one — a read + * that deliberately selected columns without a flagged + * one keeps its projection (that column is not + * attached). Applied per column, so a projection may + * name one flagged column and not another. + */ +export async function reattachInternalFieldsOnRead( + engine: InternalFieldResolvingEngine, + objectName: string, + rows: unknown, + requestedFields?: readonly string[], +): Promise { + const fields = READBACK_FIELDS[objectName]; + if (!fields) return; + + const list = (Array.isArray(rows) ? rows : [rows]).filter( + (r): r is Record => Boolean(r) && typeof r === 'object', + ); + if (list.length === 0) return; + const projected = Array.isArray(requestedFields) && requestedFields.length > 0 + ? requestedFields + : null; + + for (const { field, absenceProvesStrip } of fields) { + if (projected && !projected.includes(field)) continue; + // Only rows the engine actually stripped need the privileged read; a row + // still carrying the column (fake engines, a strip-less engine) is left + // alone. + const stripped = list.filter( + (r) => !(field in r) && (typeof r.id === 'string' || typeof r.id === 'number'), + ); + if (stripped.length === 0) continue; + + if (typeof engine.resolveInternalField !== 'function' && !absenceProvesStrip) { + // The engine does not implement the `internal` channel at all — both + // halves of it ship together on the ObjectQL engine — so it cannot have + // stripped anything, and this column is one whose absence is ordinary + // (see `absenceProvesStrip`). Nothing to recover; stay inert. + continue; + } + + if (typeof engine.resolveInternalField !== 'function') { + // Refuse rather than degrade: handing better-auth rows without their + // credential column is what turns a security control into a 200 that + // does nothing (#7823) and a token refresh into a 400 (#7987). This + // state is a composition error, so it must be loud. + throw new Error( + `${objectName} rows were read back without '${field}' (the engine's \`internal: true\` ` + + 'strip ran) but this engine offers no `resolveInternalField` accessor to recover it. ' + + `${FAIL_CLOSED_CONSEQUENCE[objectName] ?? 'better-auth would observe an incomplete row'}. ` + + 'Wire the ObjectQL engine (which provides the accessor, #8118), or remove the ' + + `\`internal\` flag from ${objectName}.${field}.`, + ); + } + + const ids = stripped.map((r) => String(r.id)); + const values = await engine.resolveInternalField(objectName, ids, field); + for (const row of stripped) { + const id = String(row.id); + // An id missing from the map is a row deleted between the read and the + // dereference — leave it stripped; the consuming route treats it as the + // already-gone record it is. + if (values.has(id)) row[field] = values.get(id); + } + } +} diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 80ec13d4ec..237753e1dc 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -17,9 +17,9 @@ import { type SecretResolvingEngine, } from './sso-client-secret.js'; import { - reattachSessionTokenOnRead, + reattachInternalFieldsOnRead, type InternalFieldResolvingEngine, -} from './session-token-readback.js'; +} from './internal-field-readback.js'; /** * Mapping from better-auth model names to ObjectStack protocol object names. @@ -704,12 +704,14 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { // privileged verb for exactly that reason (#7823), so it comes off the raw // engine. See `sso-client-secret.ts` for why the seam sits here at all. const secretEngine = rawDataEngine as unknown as SecretResolvingEngine; - // [#7823] Same access rule for the session-token readback seam: + // [#7823, #7987] Same access rule for the internal-field readback seam: // `resolveInternalField` (#8118) is the privileged batch accessor that - // recovers `sys_session.token` after the engine's `internal: true` read - // strip, so better-auth's lifecycle routes (revoke-other-sessions, - // sliding-expiry refresh, expired-session cleanup) see the row whole while - // the generic data API does not. See `session-token-readback.ts`. + // recovers `sys_session.token` and `sys_account`'s OAuth token columns after + // the engine's `internal: true` read strip, so better-auth's lifecycle + // routes (revoke-other-sessions, sliding-expiry refresh, expired-session + // cleanup) and its token-exchange routes (/get-access-token, /account-info, + // /refresh-token) see the row whole while the generic data API does not. + // See `internal-field-readback.ts`. const internalFieldEngine = rawDataEngine as unknown as InternalFieldResolvingEngine; // Field-name bridging for better-auth plugins that expose NO `schema` option // (e.g. @better-auth/sso): when a model is remapped via AUTH_MODEL_TO_PROTOCOL, @@ -790,12 +792,15 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { // and authenticates to the IdP with the plaintext; encrypt-on-write // without this breaks every federated login. await injectClientSecretOnRead(secretEngine, objectName, result); - // [#7823] Session rows come back token-less off the engine's generic - // read path (`internal: true`); better-auth reads `session.token` back - // off this result to re-sign cookies, refresh, sign out and revoke. - // Re-attach it through the privileged accessor. The projection guard - // uses the CALLER's select, not the tombstone-borrowed one above. - await reattachSessionTokenOnRead( + // [#7823, #7987] Session and account rows come back missing their + // credential columns off the engine's generic read path + // (`internal: true`); better-auth reads `session.token` back off this + // result to re-sign cookies, refresh, sign out and revoke, and reads + // `account.{accessToken,refreshToken,idToken}` back off it to answer + // and refresh OAuth tokens. Re-attach them through the privileged + // accessor. The projection guard uses the CALLER's select, not the + // tombstone-borrowed one above. + await reattachInternalFieldsOnRead( internalFieldEngine, objectName, result, @@ -830,12 +835,15 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { // [#8009] Same read half, per row — better-auth reaches the provider // through findMany as well as findOne. for (const r of results) await injectClientSecretOnRead(secretEngine, objectName, r); - // [#7823] Same token readback, batched over the whole result — this is + // [#7823, #7987] Same readback, batched over the whole result. This is // the read `revoke-other-sessions` filters by `session.token`, where a // token-less row set made it answer `200 {status:true}` while revoking - // nothing (measured). One privileged read serves the page (#8118's - // batch shape); this verb has no projection, so no guard is needed. - await reattachSessionTokenOnRead(internalFieldEngine, objectName, results); + // nothing (measured) — and it is ALSO the read better-auth's + // `internalAdapter.findAccounts(userId)` issues, which feeds every + // OAuth token-exchange route. One privileged read serves the page per + // column (#8118's batch shape); this verb has no projection, so no + // guard is needed. + await reattachInternalFieldsOnRead(internalFieldEngine, objectName, results); return results.map((r) => { const norm = normaliseLegacyDates(model, r as Record); diff --git a/packages/plugins/plugin-auth/src/session-token-readback.test.ts b/packages/plugins/plugin-auth/src/session-token-readback.test.ts deleted file mode 100644 index a66aa8fd1f..0000000000 --- a/packages/plugins/plugin-auth/src/session-token-readback.test.ts +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -// -// #7823 — the session-token READBACK seam, unit-pinned from both directions. -// -// The engine's `internal: true` read strip removes `sys_session.token` from -// every find/findOne result; better-auth's lifecycle routes read that token -// back OFF adapter results (revoke-other-sessions filters by it, sliding -// refresh and expired cleanup delete/update by it). This module re-attaches -// the value through `Engine.resolveInternalField` (#8118's privileged batch -// accessor). The end-to-end proof that revoke-other-sessions actually revokes -// lives in the dogfood suite; THIS file pins the seam's own contract: -// -// - re-attach only for `sys_session`, only for rows the strip actually hit, -// only when the caller's projection did not exclude the column; -// - one batched privileged read per page, never one per row; -// - FAIL CLOSED and loud when a stripped row meets an engine with no -// accessor — that state is exactly what turns a security control into a -// silent no-op, so it must never pass quietly. - -import { describe, it, expect, vi } from 'vitest'; -import { reattachSessionTokenOnRead } from './session-token-readback.js'; - -const resolver = (map: Record) => - vi.fn(async (_object: string, ids: readonly string[], _field: string) => { - const out = new Map(); - for (const id of ids) if (id in map) out.set(id, map[id]); - return out; - }); - -describe('#7823 reattachSessionTokenOnRead', () => { - it('re-attaches the token to stripped sys_session rows — one batched call', async () => { - const resolveInternalField = resolver({ s1: 'tok-1', s2: 'tok-2' }); - const rows: any[] = [ - { id: 's1', user_id: 'u1' }, - { id: 's2', user_id: 'u1' }, - ]; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', rows); - expect(rows[0].token).toBe('tok-1'); - expect(rows[1].token).toBe('tok-2'); - expect(resolveInternalField).toHaveBeenCalledTimes(1); - expect(resolveInternalField).toHaveBeenCalledWith('sys_session', ['s1', 's2'], 'token'); - }); - - it('handles the findOne shape (a single row, not an array)', async () => { - const resolveInternalField = resolver({ s1: 'tok-1' }); - const row: any = { id: 's1', user_id: 'u1' }; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', row); - expect(row.token).toBe('tok-1'); - }); - - it('never touches another object, and issues no privileged read for one', async () => { - const resolveInternalField = resolver({ k1: 'HASH' }); - const row: any = { id: 'k1' }; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_api_key', row); - expect(row).toEqual({ id: 'k1' }); - expect(resolveInternalField).not.toHaveBeenCalled(); - }); - - it('rows still carrying `token` are left byte-identical and trigger no privileged read', async () => { - // Fake engines in adapter tests (and any engine without the strip) return - // the row whole — the seam must be inert there. - const resolveInternalField = resolver({ s1: 'REPLACED' }); - const row: any = { id: 's1', token: 'original' }; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', row); - expect(row.token).toBe('original'); - expect(resolveInternalField).not.toHaveBeenCalled(); - }); - - it('a projection that deliberately excluded `token` keeps its projection', async () => { - const resolveInternalField = resolver({ s1: 'tok-1' }); - const row: any = { id: 's1' }; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', row, ['id', 'expires_at']); - expect('token' in row).toBe(false); - expect(resolveInternalField).not.toHaveBeenCalled(); - // …but a projection that NAMED the column gets it back. - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', row, ['id', 'token']); - expect(row.token).toBe('tok-1'); - }); - - it('FAILS CLOSED: a stripped session row plus an engine with no accessor throws loudly', async () => { - const row: any = { id: 's1', user_id: 'u1' }; - await expect( - reattachSessionTokenOnRead({}, 'sys_session', row), - ).rejects.toThrow(/resolveInternalField/); - // The message names the consequence and the remedy — this is the state - // that makes revoke-other-sessions a 200 that revokes nothing. - await expect( - reattachSessionTokenOnRead({}, 'sys_session', row), - ).rejects.toThrow(/revoke-other-sessions/); - }); - - it('…but an engine with no accessor and NO stripped rows stays quiet (inert seam)', async () => { - const row: any = { id: 's1', token: 'tok' }; - await expect(reattachSessionTokenOnRead({}, 'sys_session', row)).resolves.toBeUndefined(); - }); - - it('a row deleted between the read and the dereference stays token-less', async () => { - const resolveInternalField = resolver({ s1: 'tok-1' }); // s2 vanished - const rows: any[] = [{ id: 's1' }, { id: 's2' }]; - await reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', rows); - expect(rows[0].token).toBe('tok-1'); - expect('token' in rows[1]).toBe(false); - }); - - it('non-record members and id-less rows are skipped, not judged', async () => { - const resolveInternalField = resolver({ s1: 'tok-1' }); - const rows: any[] = [{ id: 's1' }, null, 'noise', { user_id: 'u1' }]; - await expect( - reattachSessionTokenOnRead({ resolveInternalField }, 'sys_session', rows), - ).resolves.toBeUndefined(); - expect(rows[0].token).toBe('tok-1'); - expect(resolveInternalField).toHaveBeenCalledWith('sys_session', ['s1'], 'token'); - }); -}); diff --git a/packages/plugins/plugin-auth/src/session-token-readback.ts b/packages/plugins/plugin-auth/src/session-token-readback.ts deleted file mode 100644 index 936711e321..0000000000 --- a/packages/plugins/plugin-auth/src/session-token-readback.ts +++ /dev/null @@ -1,138 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * [#7823] The session-token READBACK seam — better-auth's lifecycle routes get - * the `internal`-stripped `sys_session.token` back, through the engine's - * privileged accessor, at the one layer we own. - * - * ## The defect this closes (measured, not asserted) - * - * `sys_session.token` is declared `internal: true`, so the engine's generic - * read path omits it from every find/findOne result — that is the fix for the - * replay-proven admin-cross-user disclosure this card exists for, and it stays. - * But better-auth's storage adapter is implemented OVER that same read path - * (`objectql-adapter.ts` → `dataEngine.find`/`findOne`), and several of its - * session-lifecycle routes read `session.token` back OFF the rows it returns: - * - * - `revoke-other-sessions` filters `listSessions(userId)` rows by - * `session.token !== ctx.context.session.session.token` and deletes by - * token. With every row's `token` undefined the filter yields nothing — - * measured: `POST /auth/revoke-other-sessions` answered - * `200 {"status":true}` while the user's other session KEPT AUTHENTICATING. - * A security control reporting success while doing nothing. - * - sliding-expiry refresh (`updateSession(session.session.token, …)`) and - * expired-session cleanup (`deleteSession(…)`) read the token off the - * context session, which was itself hydrated from an adapter read — same - * silent no-op shape, by code trace on the same routes file. - * - * Plain bearer VALIDATION is not affected and is not touched here: the - * verifier uses the token as a `where` FILTER (never a readback), and - * `/auth/get-session` measured 200 throughout the breakage. - * - * ## The shape (maintainer ruling 2026-08-13, Q2: compose) - * - * `Engine.resolveInternalField` — the purpose-built privileged batch accessor - * #8118 landed, whose consumption pattern that card established — recovers the - * stored value for a batch of row ids. This module re-attaches it to session - * rows the adapter hands better-auth, so every lifecycle readback sees the row - * whole while the generic data API keeps returning rows without it. ⛔ NOT a - * second accessor, ⛔ NOT an engine carve-out: the engine's read path stays - * carve-out-free (#7728's design), and the privileged dereference happens - * here, in the identity authority's own storage seam — the same placement as - * `sso-client-secret.ts`'s `injectClientSecretOnRead` (#8009) and the same - * raw-engine access rule: `withSystemContext` deliberately exposes CRUD verbs - * only, so the privileged verb comes off the RAW engine. - * - * ## Fail-closed, loudly - * - * A session row that comes back WITHOUT `token` from an engine that offers no - * `resolveInternalField` is exactly the state that turns `revoke-other-sessions` - * into a silent no-op — so it throws (composition error, named remedy) instead - * of degrading. Rows that still carry `token` (a fake engine in tests, an - * engine without the strip) are left untouched and trigger no privileged read - * at all, so the seam is inert everywhere the strip is. - */ - -import { SystemObjectName } from '@objectstack/spec/system'; - -/** - * Engine surface this seam needs. The verb is separately named and privileged - * (#8118) precisely so it cannot be reached from a query string; it comes off - * the RAW engine, never the `withSystemContext` wrapper. - */ -export interface InternalFieldResolvingEngine { - resolveInternalField?( - object: string, - recordIds: readonly string[], - field: string, - ): Promise>; -} - -/** The one column this seam re-attaches. Bounded on purpose: `sys_account`'s - * OAuth token columns are #7987's call, not a widening here. */ -const SESSION_TOKEN_FIELD = 'token'; - -/** - * Re-attach `sys_session.token` to adapter read results, in place. - * - * @param engine The RAW data engine (privileged verb holder). - * @param objectName Protocol object name of the model just read. - * @param rows The row (findOne) or rows (findMany) about to be - * handed to better-auth. Mutated in place. - * @param requestedFields The caller's projection, if it named one — a read - * that deliberately selected columns without `token` - * keeps its projection (nothing is attached). - */ -export async function reattachSessionTokenOnRead( - engine: InternalFieldResolvingEngine, - objectName: string, - rows: unknown, - requestedFields?: readonly string[], -): Promise { - if (objectName !== SystemObjectName.SESSION) return; - if ( - Array.isArray(requestedFields) - && requestedFields.length > 0 - && !requestedFields.includes(SESSION_TOKEN_FIELD) - ) { - return; - } - const list = (Array.isArray(rows) ? rows : [rows]).filter( - (r): r is Record => Boolean(r) && typeof r === 'object', - ); - // Only rows the engine actually stripped need the privileged read; a row - // still carrying `token` (fake engines, a strip-less engine) is left alone. - const stripped = list.filter( - (r) => !(SESSION_TOKEN_FIELD in r) - && (typeof r.id === 'string' || typeof r.id === 'number'), - ); - if (stripped.length === 0) return; - - if (typeof engine.resolveInternalField !== 'function') { - // Refuse rather than degrade: handing better-auth token-less session rows - // is what turns revoke-other-sessions into a 200 that revokes nothing - // (#7823). This state is a composition error, so it must be loud. - throw new Error( - `sys_session rows were read back without '${SESSION_TOKEN_FIELD}' (the engine's ` - + "`internal: true` strip ran) but this engine offers no `resolveInternalField` " - + 'accessor to recover it. better-auth session-lifecycle routes ' - + '(revoke-other-sessions, sliding-expiry refresh, expired-session cleanup) would ' - + 'silently no-op on such rows. Wire the ObjectQL engine (which provides the ' - + 'accessor, #8118), or remove the `internal` flag from sys_session.token.', - ); - } - - const ids = stripped.map((r) => String(r.id)); - const values = await engine.resolveInternalField( - SystemObjectName.SESSION, - ids, - SESSION_TOKEN_FIELD, - ); - for (const row of stripped) { - const id = String(row.id); - // An id missing from the map is a row deleted between the read and the - // dereference — leave it token-less; the lifecycle routes treat it as the - // already-gone session it is. - if (values.has(id)) row[SESSION_TOKEN_FIELD] = values.get(id); - } -} diff --git a/packages/qa/dogfood/test/account-oauth-tokens-not-serialized.dogfood.test.ts b/packages/qa/dogfood/test/account-oauth-tokens-not-serialized.dogfood.test.ts new file mode 100644 index 0000000000..003bb40cac --- /dev/null +++ b/packages/qa/dogfood/test/account-oauth-tokens-not-serialized.dogfood.test.ts @@ -0,0 +1,327 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7987 — `sys_account`'s three OAuth credential columns must not come back on + * the generic data path. + * + * `access_token`, `refresh_token` and `id_token` hold the user's LIVE bearer + * credentials for someone else's service (Google, GitHub, an OIDC IdP), stored + * in cleartext — better-auth's `account.encryptOAuthTokens` is not set, so + * `setTokenUtil` writes the value verbatim. On `origin/main` they were plain + * `Field.textarea` on an object declaring `apiEnabled: true, + * apiMethods: ['get','list']`, and nothing masked them: the engine's credential + * mask collects by field TYPE, and it also EXEMPTS objects with + * `managedBy: 'better-auth'` — which this object is. Two independent reasons + * the one applicable collector could never have reached these columns. + * + * ## Why this object's persona story is worse than its `sys_session` sibling + * + * #7823's disclosure was admin-cross-user. This one has that arm AND a + * self-service arm: the `sys_account_self` RLS policy grants a MEMBER `select` + * on `user_id == current_user.id`, so an ordinary authenticated user could read + * their own row — and their own row holds a long-lived third-party REFRESH + * token. That converts a short-lived, revocable ObjectStack session bearer into + * a credential this platform cannot revoke at all. Both arms are pinned below; + * `memberReadsOwnRow` is the one that has no analogue in #7823. + * + * ## What makes this fixture non-vacuous (read this before adding to it) + * + * A credential-account row — what `signUp` creates — has all three columns + * EMPTY. So "the API response has no `refresh_token` key" is true on such a row + * whether or not the fix exists, and a fixture built on sign-up rows alone + * would certify nothing while printing green. Two deliberate choices close + * that: + * + * 1. the fixture PLANTS real token values on the member's account row through + * the engine (the write better-auth's OAuth callback would perform), so + * there is something to leak; + * 2. `assertArmed` (#8074) re-reads those values back out of STORAGE through + * the privileged accessor before a single assertion runs. If the plant did + * not land, the file fails in `beforeAll` rather than passing hollowly. + * + * The other direction is pinned too: `storageAndFilteringUntouched` proves the + * values are still on disk and still usable as a server-side predicate, and + * `signInStillWorks` proves the strip did not break authentication. Without + * those, a change that simply deleted the columns would satisfy every + * "absent" assertion here. + * + * ## What this fixture does NOT prove, stated plainly + * + * The end-to-end OAuth token-exchange routes (`/get-access-token`, + * `/account-info`, `/refresh-token`) need a configured social provider and a + * live IdP, which this stack has neither of. Their dependency on the readback + * seam — better-auth reads `account.refreshToken` off an adapter result row, so + * the read strip alone would answer `REFRESH_TOKEN_NOT_FOUND` — is pinned at + * the unit level in `plugin-auth/src/internal-field-readback.test.ts`. What + * IS covered here end-to-end is the sign-in path, which really does read a + * `sys_account` row back through the same seam + * (`internalAdapter.findCredentialAccount`). + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { assertArmed, armedWhen } from './armed.js'; + +const MEMBER_EMAIL = 'account-oauth-member@verify.test'; +const MEMBER_PASSWORD = 'Member-Pass-123'; + +/** The values planted on the member's account row — distinctive on purpose. */ +const PLANTED = { + access_token: 'ya29.PLANTED-ACCESS-TOKEN-7987', + refresh_token: '1//PLANTED-REFRESH-TOKEN-7987', + id_token: 'eyJhbGciOiJSUzI1NiJ9.PLANTED-ID-TOKEN-7987', +} as const; + +const TOKEN_COLUMNS = ['access_token', 'refresh_token', 'id_token'] as const; + +describe('#7987: sys_account OAuth tokens never serialize on the generic data path', () => { + let stack: VerifyStack; + let ql: any; + let adminToken: string; + let memberToken: string; + let memberUserId: string; + let memberAccountId: string; + + /** Every `sys_account` row the admin can see. */ + const listAccountsAsAdmin = async (query = ''): Promise => { + const res = await stack.apiAs(adminToken, 'GET', `/data/sys_account${query}`); + expect(res.status).toBe(200); + return ((await res.json()) as any).records ?? []; + }; + + /** Read a column straight out of storage, below the strip (#8118's accessor). */ + const storedValue = async (field: string): Promise => { + const map = await ql.resolveInternalField('sys_account', [memberAccountId], field); + return map.get(memberAccountId); + }; + + const assertNoTokenColumns = (record: Record): void => { + // OMIT, not mask: the key must be ABSENT. `toBeUndefined()` would also pass + // on a masked value of `undefined`, which still ships a key whose presence + // says "this user has a linked provider credential". + for (const column of TOKEN_COLUMNS) expect(Object.keys(record)).not.toContain(column); + }; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, {}); + ql = await stack.kernel.getServiceAsync('objectql'); + adminToken = await stack.signIn(); + memberToken = await stack.signUp(MEMBER_EMAIL, MEMBER_PASSWORD); + + const memberMe: any = await (await stack.apiAs(memberToken, 'GET', '/auth/get-session')).json(); + memberUserId = String(memberMe?.user?.id ?? ''); + expect(memberUserId, 'could not resolve the member id').toBeTruthy(); + + // The account row better-auth created for the member at sign-up. Read as + // system, because the read path this fixture is about would hide the very + // columns we are planting. + const accounts: any[] = await ql.find('sys_account', { + where: { user_id: memberUserId }, + context: { isSystem: true }, + }); + expect(accounts.length, 'sign-up must create a sys_account row').toBeGreaterThan(0); + memberAccountId = String(accounts[0].id); + + // Plant the credentials an OAuth link would have written. This is the whole + // reason the assertions below mean anything: a credential account's token + // columns are empty, so without this the file would assert the absence of + // values that never existed. + await ql.update( + 'sys_account', + { id: memberAccountId, ...PLANTED }, + { context: { isSystem: true } }, + ); + + await assertArmed([ + armedWhen({ + control: '#7987 — the member account row really holds live OAuth tokens', + disarmedBy: + 'the plant above not landing (a rejected system write, or a renamed column) — the ' + + 'token columns would then be empty and every "absent from the API response" ' + + 'assertion in this file would pass without a credential to leak', + observe: async () => ({ + access_token: await storedValue('access_token'), + refresh_token: await storedValue('refresh_token'), + id_token: await storedValue('id_token'), + }), + armed: (observed) => + observed.access_token === PLANTED.access_token && + observed.refresh_token === PLANTED.refresh_token && + observed.id_token === PLANTED.id_token, + describe: (observed) => + `stored: access=${String(observed.access_token)} refresh=${String(observed.refresh_token)} ` + + `id=${String(observed.id_token)}`, + }), + armedWhen({ + control: '#7987 — sys_account is reachable on the data API at all', + disarmedBy: + "the object's `apiEnabled`/`apiMethods` being narrowed — the columns would stop " + + 'serializing because the OBJECT stopped serializing, which is a different fix and ' + + 'would make this fixture blind to a regression in the field flag itself', + observe: async () => (await stack.apiAs(adminToken, 'GET', '/data/sys_account')).status, + armed: (status) => status === 200, + describe: (status) => `GET /data/sys_account as admin ⇒ ${status}`, + }), + ]); + }, 120_000); + + afterAll(async () => { await stack?.stop?.(); }); + + it('admin list omits all three token columns, and keeps the rest of the row', async () => { + const rows = await listAccountsAsAdmin(); + + // The admin sees other users' account rows — the cross-user arm. + expect(rows.length).toBeGreaterThan(0); + const memberRow = rows.find((r: any) => String(r.id) === memberAccountId); + expect(memberRow, "admin must still SEE the member's account row").toBeTruthy(); + + for (const row of rows) assertNoTokenColumns(row); + + // Falsifiability: these are real, populated rows — not empty objects that + // would satisfy any absence assertion. + expect(rows.every((r: any) => typeof r.id === 'string' && r.id.length > 0)).toBe(true); + expect(memberRow.provider_id).toBeTruthy(); + expect(String(memberRow.user_id)).toBe(memberUserId); + }); + + it("admin get-by-id on another user's account omits them — the cross-user arm", async () => { + const res = await stack.apiAs(adminToken, 'GET', `/data/sys_account/${memberAccountId}`); + expect(res.status).toBe(200); + const record = ((await res.json()) as any).record ?? {}; + + assertNoTokenColumns(record); + + // The row is still readable and still identifies the link — admins keep the + // account-management surface, they just stop receiving the credentials. + expect(record.id).toBe(memberAccountId); + expect(String(record.user_id)).toBe(memberUserId); + expect(record.provider_id).toBeTruthy(); + }); + + it('a MEMBER reading their OWN row does not get their own OAuth tokens back', async () => { + // The arm with no analogue in #7823. `sys_account_self` grants this read, + // and it is a legitimate read — the row is theirs. What must not come with + // it is the long-lived third-party refresh token, because that turns any + // session bearer (or any XSS on the console) into a credential ObjectStack + // cannot revoke. + const res = await stack.apiAs(memberToken, 'GET', '/data/sys_account'); + expect(res.status).toBe(200); + const rows = ((await res.json()) as any).records ?? []; + + expect(rows.length).toBeGreaterThan(0); + expect(rows.every((r: any) => String(r.user_id) === memberUserId)).toBe(true); + for (const row of rows) assertNoTokenColumns(row); + + const own = await stack.apiAs(memberToken, 'GET', `/data/sys_account/${memberAccountId}`); + expect(own.status).toBe(200); + const record = ((await own.json()) as any).record ?? {}; + assertNoTokenColumns(record); + expect(record.id).toBe(memberAccountId); + }); + + it('an EXPLICIT `?select=` naming the columns does not bypass the strip', async () => { + // `select` gates only on whether a field is KNOWN, and all three are known, + // so naming them is a LEGAL request that must come back without them — + // stripped, not refused. A strip that only touched the default projection + // would ship looking complete and still leak to any client that spells the + // column out. + const rows = await listAccountsAsAdmin('?select=id,access_token,refresh_token,id_token'); + expect(rows.length).toBeGreaterThan(0); + for (const row of rows) assertNoTokenColumns(row); + expect(rows.every((r: any) => typeof r.id === 'string')).toBe(true); + + const byId = await stack.apiAs( + adminToken, + 'GET', + `/data/sys_account/${memberAccountId}?select=id,refresh_token`, + ); + expect(byId.status).toBe(200); + const record = ((await byId.json()) as any).record ?? {}; + assertNoTokenColumns(record); + expect(record.id).toBe(memberAccountId); + + // And the member cannot spell their way to their own refresh token either. + const memberSelect = await stack.apiAs( + memberToken, + 'GET', + '/data/sys_account?select=id,refresh_token', + ); + expect(memberSelect.status).toBe(200); + for (const row of ((await memberSelect.json()) as any).records ?? []) { + assertNoTokenColumns(row); + } + }); + + it('the system read path is stripped too — there is no `isSystem` carve-out', async () => { + // #7728's design: the omission has no escape hatch, because an escape + // hatch nobody needs is a hole in a non-exposure guarantee. The legitimate + // system reader uses the privileged accessor instead, which is what the + // next test asserts. + const rows: any[] = await ql.find('sys_account', { + where: { id: memberAccountId }, + context: { isSystem: true }, + }); + expect(rows.length).toBe(1); + assertNoTokenColumns(rows[0]); + expect(rows[0].id).toBe(memberAccountId); + }); + + it('storage, filtering and the privileged accessor are UNTOUCHED', async () => { + // The negative direction. The strip runs on rows the driver has already + // produced, so the value is still on disk, still indexable and still + // usable as a server-side predicate — which is exactly what better-auth's + // own lookups depend on. A change that deleted the columns outright would + // pass every absence assertion above and fail here. + expect(await storedValue('refresh_token')).toBe(PLANTED.refresh_token); + expect(await storedValue('access_token')).toBe(PLANTED.access_token); + expect(await storedValue('id_token')).toBe(PLANTED.id_token); + + // The predicate still resolves the row server-side… + const byToken: any[] = await ql.find('sys_account', { + where: { refresh_token: PLANTED.refresh_token }, + context: { isSystem: true }, + }); + expect(byToken.length).toBe(1); + expect(String(byToken[0].id)).toBe(memberAccountId); + // …while that same row comes back with no token columns on it. + assertNoTokenColumns(byToken[0]); + }); + + it('⛔ the accessor refuses a column that is not flagged — `password` stays unreachable', async () => { + // The card's scope guard, held mechanically. `password` and + // `previous_password_hashes` are better-auth one-way hashes (ADR-0100's + // third channel) and are deliberately NOT `internal`; dereferencing one + // through this accessor would be a mask bypass rather than a read of the + // internal channel, so the engine refuses it (ADR-0112 code + status). + const err: any = await ql + .resolveInternalField('sys_account', [memberAccountId], 'password') + .then(() => null, (e: any) => e); + expect(err, 'resolveInternalField must refuse a non-internal column').toBeTruthy(); + expect(err.code).toBe('INVALID_FIELD'); + expect(err.status).toBe(400); + }); + + it('sign-in still works — the account read path is on the authentication route', async () => { + // The liveness arm that matters for THIS object. better-auth's + // `findCredentialAccount` reads a `sys_account` row back through the same + // adapter seam the strip runs on, on every password sign-in — so a readback + // seam that threw, or returned a mangled row, would take authentication + // down with it (the sibling card measured exactly that shape for + // `sys_session.token`: `verify signIn: no token in response`). + const fresh = await stack.signIn(MEMBER_EMAIL, MEMBER_PASSWORD); + expect(typeof fresh).toBe('string'); + expect(fresh.length).toBeGreaterThan(8); + + const me = await stack.apiAs(fresh, 'GET', '/auth/get-session'); + expect(me.status).toBe(200); + const body: any = await me.json(); + expect(String(body?.user?.id)).toBe(memberUserId); + + // A brand-new user signs up and authenticates as well — the account WRITE + // path is unaffected by a read-side flag. + const newcomer = await stack.signUp('account-oauth-newcomer@verify.test'); + const newcomerMe = await stack.apiAs(newcomer, 'GET', '/auth/get-session'); + expect(newcomerMe.status).toBe(200); + }); +});