From cdd83f3d662d52e8a1b170a11ab6d2acc5f0e252 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 08:32:08 +0000 Subject: [PATCH 1/6] wip: internal:true on sys_session.token --- .../src/identity/sys-session.object.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/platform-objects/src/identity/sys-session.object.ts b/packages/platform-objects/src/identity/sys-session.object.ts index 67a92acc5a..f19d2298fe 100644 --- a/packages/platform-objects/src/identity/sys-session.object.ts +++ b/packages/platform-objects/src/identity/sys-session.object.ts @@ -190,11 +190,38 @@ export const SysSession = ObjectSchema.create({ }), // ── Secret (hidden by default) ────────────────────────────── + // + // [#7823] `internal: true` is what makes the description below TRUE. It was + // false on every build before this flag existed: `hidden` is a UI contract + // ("Hidden from default UI"), never a serialization one, and the engine's + // credential read mask collects by field TYPE — so this `text` column was + // collected by nothing and the token came back on the generic data path. + // + // Measured on a real engine: an ADMIN caller got this column on list, on + // get-by-id for ANOTHER user's session, and on an explicit `?select=id,token`. + // The disclosure is admin-cross-user — a member's own reads are self-scoped + // and the `sys_session_self` RLS policy already answers 404 across users. + // + // This column is a LIVE BEARER CREDENTIAL, which is where it differs from + // its `sys_api_key.key` sibling (#7728): that one is a SHA-256 hash, while + // the value here was replay-proven — a member's token, read off the data API + // by an admin, authenticates as that member when sent as `Authorization: + // Bearer `. Disclosure is impersonation, not merely exposure. + // + // Still `text`, deliberately. `Field.secret` would encrypt at rest and + // replace the column with a `sys_secret` ref, destroying the by-token + // session lookup better-auth performs on every authenticated request — i.e. + // it would break authentication to fix a disclosure. `password` is inert + // here: the read mask skips `password` on `managedBy: 'better-auth'` + // objects, and collects by TYPE anyway, so a `text` column is never + // collected regardless. `internal` is read-side only: storage, the unique + // index on `token` and the verifier's filter are all untouched. token: Field.text({ label: 'Session Token', required: true, hidden: true, readonly: true, + internal: true, description: 'Opaque session token — never exposed in UI', group: 'Secret', }), From 796c4fade6f3f815222f140ea2c07b521c3f8e7b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 08:35:48 +0000 Subject: [PATCH 2/6] test+changeset for #7823 --- .changeset/session-token-internal.md | 61 ++++ ...ssion-token-not-serialized.dogfood.test.ts | 279 ++++++++++++++++++ 2 files changed, 340 insertions(+) create mode 100644 .changeset/session-token-internal.md create mode 100644 packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts diff --git a/.changeset/session-token-internal.md b/.changeset/session-token-internal.md new file mode 100644 index 0000000000..fae9925408 --- /dev/null +++ b/.changeset/session-token-internal.md @@ -0,0 +1,61 @@ +--- +"@objectstack/platform-objects": patch +--- + +fix(platform-objects): `sys_session.token` stops serializing on the data API — `internal: true` (#7823) + + + +`sys_session.token` — the **live bearer credential** for an active session — +declared `description: 'Opaque session token — never exposed in UI'` and then +serialized anyway on the generic data path. + +**Scope the persona precisely: this is an ADMIN-CROSS-USER disclosure**, not an +any-authenticated-caller one. Measured on a real engine (`bootStack(showcaseStack)`, +in-process HTTP + sqlite-wasm): + +- **admin**, `GET /data/sys_session` (list) — 200, `token` present on every row, + the admin's own **and every other user's**; +- **admin**, `GET /data/sys_session/{another user's id}` — 200, that member's + token verbatim; +- **admin**, `?select=id,token` — 200, present; +- anonymous — 401, fully denied; +- member — self-scoped reads only, and a cross-user get-by-id still answers + **404**: the `sys_session_self` RLS policy was already holding that line and + is untouched here. + +**Why this is more than exposure.** The sibling column closed by #7728 +(`sys_api_key.key`) is a stored SHA-256 hash. This one is not: the disclosure was +**replay-proven** — a member's token, taken exactly as it came back to the admin +off the data API, authenticates as that member when sent as +`Authorization: Bearer `. So the defect was admin-to-member +**impersonation**, and any admin-adjacent read (an integration, a leaked admin +API response, a support tool) inherited it. + +**The fix is one declaration.** `internal: true` — the opt-in, type-independent +flag minted by #7728 meaning *the declared value is never returned on the generic +data path* — is honoured at `Engine.maskSecretFields`' collector branch and at the +two write-response sites. No spec or engine change was needed. + +`hidden: true` was never the broken contract (spec defines it as "Hidden from +default UI", never as "stripped from serialization"); the broken contract was the +field's own description. + +**Not retyped, deliberately.** `Field.secret` would encrypt at rest and replace +the column with a `sys_secret` ref, destroying the by-token session lookup +better-auth performs on every authenticated request — it would break +authentication in order to fix a disclosure. `Field.password` is inert here: the +read mask skips `password` on `managedBy: 'better-auth'` objects, and it collects +by **TYPE** regardless, which a `text` column never satisfies. Two independent +barriers, so the column stays `text`. + +**Storage, filtering and indexing are untouched** — the strip runs on the rows the +driver has already produced, after the predicate has been evaluated and the unique +index on `token` used. The regression proof drives both directions: sessions still +mint, the minted bearer still authenticates (`GET /auth/get-session` ⇒ 200), and a +`where: { token }` lookup still resolves the row server-side while that same row +comes back with no `token` key. Without those, a change that simply broke +authentication would satisfy every "absent" assertion. diff --git a/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts new file mode 100644 index 0000000000..8ba114fa5f --- /dev/null +++ b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts @@ -0,0 +1,279 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7823 — `sys_session.token` must not come back on the generic read path, + * because its own declaration says it never does: + * + * description: 'Opaque session token — never exposed in UI' + * + * On `origin/main` that sentence was false, for the same structural reason as + * its `sys_api_key.key` sibling (#7728): the engine's read mask collects by + * field TYPE (`collectMaskedReadFields`), and `token` is `text`, so nothing + * collected it. `hidden: true` is not the contract that was broken — spec + * defines it as "Hidden from default UI", not "stripped from serialization". + * The fix is the `internal: true` flag, honoured at the same post-hook choke + * point as the credential mask. + * + * **What makes this card different from its sibling, and worse.** `key` is a + * stored SHA-256 hash; `token` is a LIVE BEARER CREDENTIAL. The measurement on + * the card was replay-proven: a member's token, read off the data API by an + * ADMIN, authenticates as that member when replayed as `Authorization: Bearer`. + * So the disclosure is impersonation, not merely exposure — which is why + * `crossUserTokenIsNotRecoverable` below is the assertion this file exists for. + * + * **Scope the persona precisely.** This is an ADMIN-CROSS-USER disclosure, not + * an any-authenticated-caller one. A member's own reads are self-scoped and the + * `sys_session_self` RLS policy already answers 404 across users — that arm is + * pinned here (`memberCannotReachAnotherSession`) so a future change to the + * strip cannot quietly be credited with holding a line RLS was already holding, + * and so a regression in RLS itself is attributed to RLS. + * + * **This file has to drive BOTH directions**, and the negative one is the + * load-bearing half: a change that broke authentication outright would satisfy + * every "absent" assertion here. So the liveness arms are asserted at the + * moment they would break — + * + * - sessions still MINT (`signIn`/`signUp` still hand back a working bearer); + * - that bearer still AUTHENTICATES (`GET /auth/get-session` ⇒ 200); + * - the by-token session lookup still RESOLVES server-side, i.e. the value is + * still in STORAGE and still filterable — the strip runs on result rows, + * after the driver has evaluated the predicate and used the unique index. + * + * The `?select=` arm is its own test. `select` gates only on whether a field is + * KNOWN (`assertProjectionFieldsExist`) and `token` is known, so a strip that + * only touched the default projection would ship looking complete and still + * leak to any client that spells the column out. + * + * Falsifiability: `id` / `user_id` / `expires_at` are asserted PRESENT + * throughout. Without them a "delete every column" bug reads as a pass. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; + +const MEMBER_EMAIL = 'session-token-member@verify.test'; + +describe('#7823: sys_session.token (a live bearer) never serializes on the generic read path', () => { + let stack: VerifyStack; + let adminToken: string; + let memberToken: string; + let adminUserId: string; + let memberUserId: string; + + /** + * Does this bearer still authenticate? Asked through the real auth surface, + * with the token as the ONLY credential present. This is the negative + * direction — it must stay `true` for a token the fix merely stopped + * serializing. + */ + const stillAuthenticates = async (bearer: string): Promise => { + const res = await stack.apiAs(bearer, 'GET', '/auth/get-session'); + if (res.status !== 200) return false; + const body: any = await res.json(); + return Boolean(body?.user?.id); + }; + + /** Every `sys_session` row the admin can see. */ + const listSessionsAsAdmin = async (query = ''): Promise => { + const res = await stack.apiAs(adminToken, 'GET', `/data/sys_session${query}`); + expect(res.status).toBe(200); + return ((await res.json()) as any).records ?? []; + }; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, {}); + adminToken = await stack.signIn(); + memberToken = await stack.signUp(MEMBER_EMAIL); + + const adminMe: any = await (await stack.apiAs(adminToken, 'GET', '/auth/get-session')).json(); + adminUserId = String(adminMe?.user?.id ?? ''); + const memberMe: any = await (await stack.apiAs(memberToken, 'GET', '/auth/get-session')).json(); + memberUserId = String(memberMe?.user?.id ?? ''); + + expect(adminUserId, 'could not resolve the seeded admin id').toBeTruthy(); + expect(memberUserId, 'could not resolve the member id').toBeTruthy(); + expect(adminUserId).not.toBe(memberUserId); + }, 120_000); + + afterAll(async () => { await stack?.stop?.(); }); + + it('admin list omits `token` on every row, and keeps the other columns', async () => { + const rows = await listSessionsAsAdmin(); + + // The measurement on the card saw the token on THREE rows here — the + // admin's own and every other user's. Assert we still see multiple users' + // rows, so this is the same broad read that leaked, not a narrowed one. + expect(rows.length).toBeGreaterThan(1); + expect(new Set(rows.map((r: any) => String(r.user_id))).size).toBeGreaterThan(1); + + // OMIT, not mask (maintainer ruling 2026-08-12): `token` is + // `required: true`, so a "a value is set" mask carries zero bits while + // still shipping a value under a field whose description promises none. + // `toBeUndefined()` alone would pass on a masked value of `undefined`; the + // key must be ABSENT from the object. + for (const row of rows) expect(Object.keys(row)).not.toContain('token'); + + // Falsifiability: these are real rows, not empty objects. + expect(rows.every((r: any) => typeof r.id === 'string' && r.id.length > 0)).toBe(true); + expect(rows.every((r: any) => Boolean(r.user_id))).toBe(true); + expect(rows.some((r: any) => Boolean(r.expires_at))).toBe(true); + }); + + it("admin get-by-id on ANOTHER user's session omits `token` — the disclosure this card closes", async () => { + // The exact shape that was replay-proven: the admin reads the MEMBER's + // session row by id and, before this fix, got that member's live bearer + // verbatim. Resolve the row through the admin's own list, the way the + // measurement did. + const rows = await listSessionsAsAdmin(); + const memberRow = rows.find((r: any) => String(r.user_id) === memberUserId); + expect(memberRow, "admin must still be able to SEE the member's session row").toBeTruthy(); + + const res = await stack.apiAs(adminToken, 'GET', `/data/sys_session/${memberRow.id}`); + expect(res.status).toBe(200); + const record = ((await res.json()) as any).record ?? {}; + + expect(Object.keys(record)).not.toContain('token'); + + // The read still WORKS and still identifies the session — admin keeps the + // session-management surface (`revoked_at`, expiry, client fingerprint), + // it just stops receiving the credential itself. + expect(record.id).toBe(memberRow.id); + expect(String(record.user_id)).toBe(memberUserId); + expect(record.expires_at).toBeTruthy(); + + // And the credential the admin can no longer read is still the member's + // working credential — the fix removed a disclosure, not a session. + expect(await stillAuthenticates(memberToken)).toBe(true); + }); + + it('an EXPLICIT `?select=id,token` projection does not bypass the strip', async () => { + // `select` only gates on whether a field is KNOWN, and `token` is known, so + // naming it is a LEGAL request that must come back WITHOUT it — stripped, + // not refused, so a client asking for a legal-but-omitted column still gets + // its other columns. + const rows = await listSessionsAsAdmin('?select=id,token'); + expect(rows.length).toBeGreaterThan(0); + for (const row of rows) expect(Object.keys(row)).not.toContain('token'); + // The projection was honoured, not silently downgraded to something that + // never contained `token` anyway. + expect(rows.every((r: any) => typeof r.id === 'string')).toBe(true); + + const withOther = await listSessionsAsAdmin('?select=id,token,user_id'); + expect(withOther.length).toBeGreaterThan(0); + for (const row of withOther) expect(Object.keys(row)).not.toContain('token'); + expect(withOther.every((r: any) => Boolean(r.user_id))).toBe(true); + + const target = withOther[0]; + const byId = await stack.apiAs(adminToken, 'GET', `/data/sys_session/${target.id}?select=id,token`); + expect(byId.status).toBe(200); + const record = ((await byId.json()) as any).record ?? {}; + expect(Object.keys(record)).not.toContain('token'); + expect(record.id).toBe(target.id); + }); + + it('a member self-scoped read omits `token` too', async () => { + const res = await stack.apiAs(memberToken, 'GET', '/data/sys_session'); + expect(res.status).toBe(200); + const rows = ((await res.json()) as any).records ?? []; + + // Self-scoped: the member sees their own session(s) and nobody else's. + expect(rows.length).toBeGreaterThan(0); + expect(rows.every((r: any) => String(r.user_id) === memberUserId)).toBe(true); + for (const row of rows) expect(Object.keys(row)).not.toContain('token'); + + // A member could not read their own token off this path either — worth + // pinning, because "you may only see your own credential" is still a + // serialization the declaration forbids. + const own = await stack.apiAs(memberToken, 'GET', `/data/sys_session/${rows[0].id}`); + expect(own.status).toBe(200); + const record = ((await own.json()) as any).record ?? {}; + expect(Object.keys(record)).not.toContain('token'); + expect(record.id).toBe(rows[0].id); + }); + + it("a member still cannot reach another user's session at all — RLS is untouched", async () => { + // The member arm was never a disclosure: `sys_session_self` answers 404 + // across users. Pinned so a regression HERE is attributed to RLS rather + // than to the strip, and so the strip cannot be credited with a line it + // does not hold. + const adminRows = await listSessionsAsAdmin(); + const adminRow = adminRows.find((r: any) => String(r.user_id) === adminUserId); + expect(adminRow, "could not resolve the admin's own session row").toBeTruthy(); + + const res = await stack.apiAs(memberToken, 'GET', `/data/sys_session/${adminRow.id}`); + expect(res.status).toBe(404); + }); + + it('sessions still MINT and the minted bearer still AUTHENTICATES', async () => { + // The negative direction, at its most direct: a change that broke + // authentication would satisfy every absence assertion above. + expect(await stillAuthenticates(adminToken)).toBe(true); + expect(await stillAuthenticates(memberToken)).toBe(true); + + // A NEW session, minted after the strip is in force, is equally usable — + // proves the mint path still writes a token the auth path can resolve. + const freshMember = await stack.signUp('session-token-fresh@verify.test'); + expect(typeof freshMember).toBe('string'); + expect(freshMember.length).toBeGreaterThan(8); + expect(await stillAuthenticates(freshMember)).toBe(true); + + const freshAdmin = await stack.signIn(); + expect(typeof freshAdmin).toBe('string'); + expect(await stillAuthenticates(freshAdmin)).toBe(true); + }); + + it('the by-token session lookup still resolves server-side — the value is still in STORAGE', async () => { + // The load-bearing negative assertion. `internal` is a SERIALIZATION + // contract, not a storage one: the strip runs on rows the driver has + // already produced, so the predicate has been evaluated and the unique + // index on `token` used before the engine sees anything. If this stopped + // matching, every authenticated request in the product would 401. + const ql = await stack.kernel.getServiceAsync('objectql'); + expect(ql, 'objectql service must be available').toBeTruthy(); + + const rows = (await ql.find('sys_session', { + where: { token: memberToken }, + context: { isSystem: true }, + })) as any[]; + + // The filter MATCHED — proof the plaintext is still stored under `token`. + expect(Array.isArray(rows)).toBe(true); + expect(rows.length).toBe(1); + expect(String(rows[0].user_id)).toBe(memberUserId); + + // …and the row it handed back STILL has no `token` key. Both halves in one + // assertion: filterable in storage, absent from the result. That is exactly + // the line `internal` draws, and the reason authentication keeps working. + expect(Object.keys(rows[0])).not.toContain('token'); + }); + + it('the declaration that makes all of the above required is on the REGISTERED schema', async () => { + // The original defect was a DECLARATION disagreeing with the runtime, so + // pin the declaration from the REGISTERED schema — what the runtime serves, + // not what the source file says. + const ql = await stack.kernel.getServiceAsync('objectql'); + const schema = ql?.getSchema?.('sys_session'); + expect(schema, 'sys_session schema must be registered').toBeTruthy(); + + const token = schema.fields?.token; + expect(token, 'sys_session.token must stay declared').toBeTruthy(); + expect(token.internal).toBe(true); + + // Still a plain `text` column: the fix does NOT retype it. `secret` would + // encrypt at rest and destroy the by-token session lookup asserted above; + // `password` is inert on a `managedBy: 'better-auth'` object and is + // collected by TYPE anyway, which a `text` column never satisfies. + expect(token.type).toBe('text'); + + // The neighbouring flags are unchanged — `internal` is an ADDITION, not a + // re-declaration. `hidden` stays a UI contract, `readonly` the write one. + expect(token.hidden).toBe(true); + expect(token.readonly).toBe(true); + expect(token.required).toBe(true); + + // The description this card exists to make true — unchanged by the fix, so + // the generated translation bundles that mirror it do not churn. + expect(String(token.description)).toContain('never exposed in UI'); + }); +}); From 3b1cb330a5f3f19822b8713b45c1423dae0160a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 08:50:25 +0000 Subject: [PATCH 3/6] test: add session lifecycle arm --- ...ssion-token-not-serialized.dogfood.test.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts index 8ba114fa5f..75ca019f14 100644 --- a/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts +++ b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts @@ -223,6 +223,47 @@ describe('#7823: sys_session.token (a live bearer) never serializes on the gener expect(await stillAuthenticates(freshAdmin)).toBe(true); }); + it('the session LIFECYCLE still works — sign-out and revoke-other-sessions', async () => { + // The deepest negative arm, and the one that decides whether this flag is + // applicable to this column at all. + // + // better-auth's storage adapter is implemented OVER the objectql engine + // (`plugin-auth/src/objectql-adapter.ts` → `dataEngine.findOne`), which is + // the very path `omitInternalFields` runs on, and better-auth then reads + // `session.session.token` back OFF that row — to re-sign the session cookie + // (`api/routes/session.mjs:143`), to delete the session on sign-out (:197), + // to extend it on refresh (:234) and to pick the sessions to drop in + // revoke-other-sessions (:512). A `where`-only analysis misses all four, + // because the token is BOTH the filter and a value read back. + // + // So: does the lifecycle still land? Asked by observing the ROW, not the + // response code — a handler that no-ops on an undefined token still + // answers 200. + const ql = await stack.kernel.getServiceAsync('objectql'); + const victim = await stack.signUp('session-token-lifecycle@verify.test'); + expect(await stillAuthenticates(victim)).toBe(true); + + const rowsFor = async (bearer: string): Promise => + (await ql.find('sys_session', { + where: { token: bearer }, + context: { isSystem: true }, + })) as any[]; + + expect((await rowsFor(victim)).length).toBe(1); + + const signOut = await stack.apiAs(victim, 'POST', '/auth/sign-out', {}); + expect([200, 204]).toContain(signOut.status); + + // The row must actually be gone (or tombstoned) — this is the assertion a + // silently-undefined `deleteSession(session.token)` fails. + const after = await rowsFor(victim); + const stillLive = after.filter((r: any) => r.revoked_at == null); + expect(stillLive.length, 'sign-out must remove/tombstone the session row').toBe(0); + + // …and the bearer must stop working, which is the user-visible half. + expect(await stillAuthenticates(victim)).toBe(false); + }); + it('the by-token session lookup still resolves server-side — the value is still in STORAGE', async () => { // The load-bearing negative assertion. `internal` is a SERIALIZATION // contract, not a storage one: the strip runs on rows the driver has From bac5b4a4fe9e0b5b0b60fcd7eed96d4a2427084d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 14:02:31 +0000 Subject: [PATCH 4/6] fix(security): relocate the internal-field write-response strip to the generic-data-path ingress, and route better-auth session readbacks through resolveInternalField (#7823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A-prime + compose, per the 2026-08-13 maintainer ruling: - objectql: the engine's two omitInternalFields write-response sites are removed — engine write results stay whole, so better-auth's createWithHooks reads the minted sys_session.token back and signIn/signUp work. The generic READ-path strip is unchanged. - metadata-protocol: omitInternalFieldsFromWriteResponse (single exported helper) applied by every *Data write face — createData / cloneData / updateData / createManyData / insertManyData / updateManyData / batchData — plus a tripwire test that enumerates the *Data surface, fails on any face a flagged sentinel reaches, and fails on any new face with no recipe. - rest: the cross-object batch update mouth (direct ql.update) applies the same strip through the protocol instance (dormant today — no flagged object grants bulk — wired so the guarantee does not depend on that). - plugin-auth: session rows read back through the better-auth adapter get token re-attached via Engine.resolveInternalField (#8118's accessor), so revoke-other-sessions / sliding refresh / expired cleanup act again while the generic data API keeps returning token-less rows. Fail-closed when a stripped row meets an engine without the accessor. - dogfood: revoke-other-sessions pinned on the other session's liveness, and expired-session cleanup pinned on the row, not the status code. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- .changeset/session-token-internal.md | 55 ++- packages/metadata-protocol/src/index.ts | 8 + packages/metadata-protocol/src/protocol.ts | 66 ++++ ...-response-internal-fields.tripwire.test.ts | 327 ++++++++++++++++++ .../src/write-response-internal-fields.ts | 112 ++++++ packages/objectql/src/engine.ts | 61 +++- .../plugin-auth/src/objectql-adapter.ts | 28 ++ .../src/session-token-readback.test.ts | 114 ++++++ .../plugin-auth/src/session-token-readback.ts | 138 ++++++++ ...ssion-token-not-serialized.dogfood.test.ts | 67 ++++ packages/rest/src/rest-server.ts | 24 +- 11 files changed, 968 insertions(+), 32 deletions(-) create mode 100644 packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts create mode 100644 packages/metadata-protocol/src/write-response-internal-fields.ts create mode 100644 packages/plugins/plugin-auth/src/session-token-readback.test.ts create mode 100644 packages/plugins/plugin-auth/src/session-token-readback.ts diff --git a/.changeset/session-token-internal.md b/.changeset/session-token-internal.md index fae9925408..61d384d721 100644 --- a/.changeset/session-token-internal.md +++ b/.changeset/session-token-internal.md @@ -1,13 +1,20 @@ --- "@objectstack/platform-objects": patch +"@objectstack/objectql": patch +"@objectstack/metadata-protocol": patch +"@objectstack/plugin-auth": patch +"@objectstack/rest": patch --- -fix(platform-objects): `sys_session.token` stops serializing on the data API — `internal: true` (#7823) +fix(security): `sys_session.token` stops serializing on the data API — `internal: true`, with the write-response strip relocated to the generic-data-path ingress (#7823) +to one existing declaration, plus an internal relocation of where that flag's +write-response half is enforced (engine write sites → the metadata-protocol +ingress). Nothing authorable is renamed, retired or tombstoned, so there is no +conversion to register. The behavioural changes are that a field which already +DECLARED it was never exposed stops being exposed, and that better-auth's +session-lifecycle routes keep working while it does. --> `sys_session.token` — the **live bearer credential** for an active session — declared `description: 'Opaque session token — never exposed in UI'` and then @@ -35,10 +42,34 @@ off the data API, authenticates as that member when sent as **impersonation**, and any admin-adjacent read (an integration, a leaked admin API response, a support tool) inherited it. -**The fix is one declaration.** `internal: true` — the opt-in, type-independent -flag minted by #7728 meaning *the declared value is never returned on the generic -data path* — is honoured at `Engine.maskSecretFields`' collector branch and at the -two write-response sites. No spec or engine change was needed. +**The fix is one declaration plus one relocation** (maintainer ruling +2026-08-13, "A-prime + compose"): + +- `sys_session.token` is declared `internal: true` — the opt-in, + type-independent flag minted by #7728 meaning *the declared value is never + returned on the generic data path*. The engine's READ-path strip is + unchanged and closes the disclosure. +- The flag's **write-response** half moves out of the engine's insert/update + result paths — where it conflated "never on the generic data path" with + "never returned to the engine-level writer" and broke `signIn`/`signUp` + (better-auth reads the minted session row back off the insert result) — + into the **generic-data-path ingress**: every `*Data` write face in + `@objectstack/metadata-protocol` routes its response records through the + single exported helper `omitInternalFieldsFromWriteResponse`, held there by + a tripwire test that enumerates the ingress surface and fails on any face + the sentinel reaches (or any new `*Data` face with no recipe). The + `sys_api_key.key` PATCH-body closure (#7728's fourth surface) is preserved + at the ingress, byte-for-byte for callers. `@objectstack/rest`'s + cross-object batch update — the one write mouth outside the protocol — + applies the same shared strip. +- better-auth's session-lifecycle readbacks (revoke-other-sessions, + sliding-expiry refresh, expired-session cleanup) read `token` back off + adapter find results, which the read strip starves — measured: + `POST /auth/revoke-other-sessions` answered `200 {"status":true}` while the + other session kept authenticating. The adapter now re-attaches the token + through `Engine.resolveInternalField` (#8118's privileged batch accessor) — + no engine carve-out, no second accessor. Plain bearer validation never + needed the readback and is untouched. `hidden: true` was never the broken contract (spec defines it as "Hidden from default UI", never as "stripped from serialization"); the broken contract was the @@ -55,7 +86,9 @@ barriers, so the column stays `text`. **Storage, filtering and indexing are untouched** — the strip runs on the rows the driver has already produced, after the predicate has been evaluated and the unique index on `token` used. The regression proof drives both directions: sessions still -mint, the minted bearer still authenticates (`GET /auth/get-session` ⇒ 200), and a +mint, the minted bearer still authenticates (`GET /auth/get-session` ⇒ 200), a `where: { token }` lookup still resolves the row server-side while that same row -comes back with no `token` key. Without those, a change that simply broke -authentication would satisfy every "absent" assertion. +comes back with no `token` key, and revoke-other-sessions / expired-session +cleanup are pinned on the ROW they act on, not the status code that lied. +Without those, a change that simply broke authentication would satisfy every +"absent" assertion. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 31dfb50729..d8c967e421 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -5,6 +5,14 @@ export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeView // ObjectQL FALLBACK in `@objectstack/runtime`'s `callData` builds the SAME one // instead of minting a second not-found shape. See `recordNotFoundError`. export { recordNotFoundError } from './protocol.js'; +// [#7823] The write-response half of the `internal: true` guarantee — THE +// single helper every generic write ingress routes its response records +// through (A-prime ruling, 2026-08-13). Tripwire-enforced; see the module +// header for why it lives at the ingress and not in the engine. +export { + omitInternalFieldsFromWriteResponse, + collectInternalWriteResponseFields, +} from './write-response-internal-fields.js'; export { createMetadataProtocolPlugin, assembleMetadataProtocol } from './plugin.js'; export type { MetadataProtocolPluginOptions, AssembleMetadataProtocolOptions } from './plugin.js'; // [#6710] The declared authoring channel — the explicit expression of ADR-0005's diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index bd6c6bf14c..301c086cd1 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11,6 +11,7 @@ import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/type // the posture, is what the runtime authoring gate is told. import { postureEnforcesWall } from '@objectstack/spec/security'; import type { MetadataHostEngine } from './host-engine.js'; +import { omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js'; import { evaluateRuntimeAuthoringGate } from './runtime-authoring-gate.js'; // [#7560] ADR-0070's read-only-package rule, shared with the `/packages` // lifecycle gate in `@objectstack/runtime` — see `./package-writability.js`. @@ -7578,6 +7579,14 @@ export class ObjectStackProtocolImplementation implements const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (request.context !== undefined) opts.context = request.context; const result = await this.engine.insert(request.object, data, opts); + // [#7823] The 201 body is a GENERIC-DATA-PATH surface: strip + // `internal: true` fields here, at the ingress, per the A-prime ruling + // (2026-08-13). The engine deliberately no longer strips its own write + // results — better-auth reads a minted `sys_session.token` back off + // them — so this line is what keeps a flagged credential out of the + // external create response. Tripwire-enforced; see + // `write-response-internal-fields.ts`. + omitInternalFieldsFromWriteResponse(this.engine.registry?.getObject(request.object), result); return { object: request.object, id: result.id, @@ -7652,6 +7661,12 @@ export class ObjectStackProtocolImplementation implements const insertData = stripReadonlyForInsert(schema, data, ctx); const result = await this.engine.insert(request.object, insertData, ctxOpt as any); + // [#7823] Same ingress strip as `createData` — a clone's 201 body is + // the same generic-data-path surface. (The SOURCE row was read through + // the engine's find path, which already omits internal fields, so the + // copy never carried one in; this guards the INSERT RESULT, which the + // engine returns whole by design.) + omitInternalFieldsFromWriteResponse(schema, result); return { object: request.object, id: result.id, @@ -7750,6 +7765,16 @@ export class ObjectStackProtocolImplementation implements ? { ...(request.data as Record), id: request.id } : request.data; const result = await this.engine.update(request.object, writeData, opts); + // [#7823] The PATCH 200 body is the surface #7728's fourth measurement + // caught: a client revoking a `sys_api_key` (apiMethods keeps `update` + // open, #7727) got the stored hash back in this response. That closure + // used to live in the engine's by-id update path and RELOCATED here + // under the A-prime ruling (2026-08-13) — the engine's write results + // stay whole for privileged server-side writers, and THIS line is the + // sole closure of that surface. Pinned by + // `api-key-hash-not-serialized.dogfood.test.ts` and the ingress + // tripwire. + omitInternalFieldsFromWriteResponse(this.engine.registry?.getObject(request.object), result); return { object: request.object, id: request.id, @@ -7785,6 +7810,22 @@ export class ObjectStackProtocolImplementation implements }; } + /** + * [#7823] The write-response `internal: true` strip, exposed for generic + * write ingresses that live OUTSIDE this class. The one consumer today is + * the REST cross-object transactional batch (`POST /batch` in + * `@objectstack/rest`), whose UPDATE arm calls `ql.update` directly — a + * deliberate #3835-era choice made when the engine still stripped its own + * write results — and pushes the returned row into the response body. + * `@objectstack/rest` does not depend on this package, so it reaches the + * helper through the protocol instance it already holds (duck-typed, the + * way it probes `createManyData`). In-place, idempotent, non-objects + * skipped — see `write-response-internal-fields.ts`. + */ + omitInternalWriteFields(object: string, records: unknown): void { + omitInternalFieldsFromWriteResponse(this.engine.registry?.getObject(object), records); + } + /** * [#4435] Does this row EXIST? A fact about the database — deliberately * NOT "may this caller see it". @@ -8385,6 +8426,7 @@ export class ObjectStackProtocolImplementation implements const stripped = stripReadonlyForInsert(batchSchema, record.data || record, context); const ev = diffDroppedFields(object, record.data || record, stripped, 'readonly'); const created = await this.engine.insert(object, stripped, insertCtx as any); + omitInternalFieldsFromWriteResponse(batchSchema, created); // [#7823] results.push({ id: created.id, success: true, data: created, index, ...(ev ? { droppedFields: [ev] } : {}) }); succeeded++; break; @@ -8400,6 +8442,7 @@ export class ObjectStackProtocolImplementation implements // [#3455] Collect the engine's LEGAL write strips per row. const dropped: DroppedFieldsEvent[] = []; const updated = await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any); + omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; break; @@ -8431,13 +8474,16 @@ export class ObjectStackProtocolImplementation implements if (existing) { const dropped: DroppedFieldsEvent[] = []; const updated = await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any); + omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); } else { const created = await this.engine.insert(object, { id: record.id, ...(record.data || {}) }, insertCtx as any); + omitInternalFieldsFromWriteResponse(batchSchema, created); // [#7823] results.push({ id: created.id, success: true, data: created, index }); } } else { const created = await this.engine.insert(object, record.data || record, insertCtx as any); + omitInternalFieldsFromWriteResponse(batchSchema, created); // [#7823] results.push({ id: created.id, success: true, data: created, index }); } succeeded++; @@ -8661,6 +8707,12 @@ export class ObjectStackProtocolImplementation implements const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (request.context !== undefined) opts.context = request.context; const records = await this.engine.insert(request.object, rows, opts); + // [#7823] Bulk create is the same generic-data-path surface as the + // single-record 201 — one strip over the returned rows, at the + // ingress. (Today's `internal`-flagged objects grant no `bulk` + // apiMethod, so this face cannot reach one over REST yet; the strip is + // here so the flag's guarantee does not depend on that staying true.) + omitInternalFieldsFromWriteResponse(this.engine.registry?.getObject(request.object), records); const merged = mergeDroppedFieldEvents(dropped); return { object: request.object, @@ -8717,6 +8769,15 @@ export class ObjectStackProtocolImplementation implements rows, opts, ); + // [#7823] Per-outcome ingress strip — the partial-success face hands + // each written row back as `outcomes[i].record`, so each is the same + // generic-data-path surface as a single-record 201 body. + if (Array.isArray(outcomes)) { + const outcomeSchema = this.engine.registry?.getObject(request.object); + for (const o of outcomes) { + if (o?.record) omitInternalFieldsFromWriteResponse(outcomeSchema, o.record); + } + } if (Array.isArray(outcomes)) { for (let i = 0; i < outcomes.length; i++) { if (!outcomes[i]) continue; @@ -8777,6 +8838,10 @@ export class ObjectStackProtocolImplementation implements const results: BatchDataRowResult[] = []; let succeeded = 0; let failed = 0; + // [#7823] Ingress strip over each row's `data` payload — the bulk + // update face is the same generic-data-path surface as the by-id + // PATCH body, one row at a time. Resolved once; the loop reuses it. + const updateManySchema = this.engine.registry?.getObject(object); for (const [index, record] of records.entries()) { try { @@ -8810,6 +8875,7 @@ export class ObjectStackProtocolImplementation implements const opts: any = { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (context !== undefined) opts.context = context; const updated = await this.engine.update(object, record.data || {}, opts); + omitInternalFieldsFromWriteResponse(updateManySchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; } catch (err: any) { diff --git a/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts b/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts new file mode 100644 index 0000000000..0a260773a4 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts @@ -0,0 +1,327 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #7823 — THE TRIPWIRE: every generic data ingress routes its write-response +// records through `omitInternalFieldsFromWriteResponse`, and a NEW ingress +// cannot ship unexamined. +// +// ## Why this file exists (the A-prime ruling's own condition) +// +// The `internal: true` write-response strip lives at the protocol ingress, not +// in the engine — the engine's write results must stay whole (better-auth +// reads a minted `sys_session.token` back off `engine.insert`'s return), while +// `PATCH /data/sys_api_key/{id}`'s 200 body must NOT carry the stored key hash +// (measured: with the strip neutralised it did). The honest cost of that +// placement is that response-body policy became a per-ingress obligation, so a +// FUTURE `*Data` face that forgets the helper leaks silently. The maintainer +// ruling (2026-08-13) refused to defer that risk: this tripwire ships in the +// same PR as the relocation. +// +// ## How the enumeration catches a NEW ingress +// +// The method list is NOT hand-written. It is read off the protocol class's +// prototype chain at runtime — every function whose name ends in `Data`, the +// naming convention every data-plane face in this file has followed since +// `findData`/`createData`. Each enumerated method must have a RECIPE below; +// a `*Data` method with no recipe FAILS the suite with instructions, so the +// author of a new ingress is forced to (1) route its response records through +// the helper and (2) register how to drive it here. A hand-kept list of +// today's faces would silently stay true forever; this one grows by itself. +// +// ## What each recipe proves +// +// The fixture engine returns write results that ALWAYS carry a field declared +// `internal: true` holding SENTINEL (that is exactly what the real engine does +// now — write results are whole). Read results never carry it (the engine's +// read path strips, unchanged by #7823). Each recipe drives its face and the +// suite deep-scans the full response JSON: +// +// - SENTINEL anywhere in the response → the ingress skipped the helper → RED +// - CONTROL missing where a record was promised → the probe went blind → RED +// (falsifiability: proves a real record flowed through the response, so +// "no sentinel" cannot be satisfied by an empty or failed response) +// +// A negative control at the bottom proves the machinery can go red: a subclass +// adds `leakyData` (returning an engine write result verbatim), the +// enumeration is shown to pick it up, and the scan is shown to catch its leak. + +import { describe, it, expect } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; +import { + collectInternalWriteResponseFields, + omitInternalFieldsFromWriteResponse, +} from './write-response-internal-fields.js'; + +/** The value that must NEVER appear in any ingress response. */ +const SENTINEL = 'INTERNAL-SENTINEL-7823-NEVER-SERIALIZED'; +/** The value that MUST appear wherever a record was promised (falsifiability). */ +const CONTROL = 'CONTROL-VALUE-7823-RECORD-FLOWED'; + +const VAULT_SCHEMA = { + name: 'vault', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + vault_secret: { name: 'vault_secret', type: 'text', internal: true }, + }, + enable: { clone: true }, +}; + +/** + * Fixture engine mirroring the post-#7823 engine contract: + * - WRITE results (insert / update / insertMany outcomes) carry the flagged + * column, holding SENTINEL — the engine no longer strips its own write + * results; + * - READ results (find / findOne) do NOT carry it — the engine's read-path + * `omitInternalFields` is unchanged; + * - transaction support so the atomic arms run for real. + */ +function makeSentinelEngine() { + const storedRow = (id: string) => ({ id, name: CONTROL }); + const writtenRow = (id: string, data?: Record) => ({ + id, + name: (data as any)?.name ?? CONTROL, + vault_secret: SENTINEL, + }); + let nextId = 1; + const handle = { id: 'trx-1' }; + + const engine: any = { + registry: { getObject: (n: string) => (n === 'vault' ? VAULT_SCHEMA : undefined) }, + insert: async (_object: string, data: any) => + Array.isArray(data) + ? data.map((d: any) => writtenRow(d?.id ?? `new-${nextId++}`, d)) + : writtenRow(data?.id ?? `new-${nextId++}`, data), + insertMany: async (_object: string, rows: any[]) => + rows.map((r: any) => ({ ok: true, record: writtenRow(r?.id ?? `new-${nextId++}`, r) })), + update: async (_object: string, data: any, options?: any) => + writtenRow(options?.where?.id ?? data?.id ?? 'row-1', data), + // Contract per #4435: `false` is the positive not-found value. + delete: async (_object: string, _options?: any) => ({ deleted: 1 }), + findOne: async (_object: string, options?: any) => storedRow(options?.where?.id ?? 'row-1'), + find: async (_object: string, _options?: any) => [storedRow('row-1')], + count: async () => 1, + validate: async () => ({ valid: true, issues: [] }), + getDefaultDriverName: () => 'default', + getDriverByName: () => ({ beginTransaction: async () => handle }), + transaction: async (callback: (ctx: any) => Promise, baseContext?: any) => + callback({ ...(baseContext ?? {}), transaction: handle }), + }; + return engine; +} + +/** + * Every `*Data` method reachable on `proto`'s prototype chain — the runtime + * enumeration a future author cannot dodge by adding a method without touching + * this file. TypeScript `private` does not hide a method from this walk, which + * is deliberate: private write helpers (e.g. `runAtomicBatchData`) are part of + * the surface and are covered through their public face. + */ +function enumerateDataMethods(proto: object): string[] { + const names = new Set(); + for (let p: any = proto; p && p !== Object.prototype; p = Object.getPrototypeOf(p)) { + for (const name of Object.getOwnPropertyNames(p)) { + if (name.endsWith('Data') && typeof (p as any)[name] === 'function') names.add(name); + } + } + return [...names].sort(); +} + +/** + * One entry per enumerated method. `invocations` drives the face against the + * fixture; `expectRecord` demands CONTROL in the response (write faces that + * promise records). `coveredVia` marks a private helper exercised through the + * named public face — its invocations live there. + */ +type Recipe = + | { invocations: Array<(p: any) => Promise>; expectRecord: boolean } + | { coveredVia: string }; + +const RECIPES: Record = { + // ── read / verdict faces: no write result to strip; enumerated so the map + // stays total and a rename is noticed ────────────────────────────────── + findData: { + invocations: [(p) => p.findData({ object: 'vault', query: {} })], + expectRecord: false, + }, + getData: { + invocations: [(p) => p.getData({ object: 'vault', id: 'row-1' })], + expectRecord: false, + }, + validateData: { + invocations: [(p) => p.validateData({ object: 'vault', data: { name: 'x' } })], + expectRecord: false, + }, + deleteData: { + invocations: [(p) => p.deleteData({ object: 'vault', id: 'row-1' })], + expectRecord: false, + }, + deleteManyData: { + invocations: [(p) => p.deleteManyData({ object: 'vault', ids: ['row-1'] })], + expectRecord: false, + }, + + // ── write faces: engine write results ride the response — the helper is + // what keeps SENTINEL out of each ───────────────────────────────────── + createData: { + invocations: [(p) => p.createData({ object: 'vault', data: { name: CONTROL } })], + expectRecord: true, + }, + cloneData: { + invocations: [(p) => p.cloneData({ object: 'vault', id: 'row-1' })], + expectRecord: true, + }, + updateData: { + invocations: [(p) => p.updateData({ object: 'vault', id: 'row-1', data: { name: CONTROL } })], + expectRecord: true, + }, + createManyData: { + invocations: [ + (p) => p.createManyData({ object: 'vault', records: [{ name: CONTROL }, { name: 'b' }] }), + ], + expectRecord: true, + }, + insertManyData: { + invocations: [(p) => p.insertManyData({ object: 'vault', records: [{ name: CONTROL }] })], + expectRecord: true, + }, + updateManyData: { + invocations: [ + (p) => p.updateManyData({ + object: 'vault', + records: [{ id: 'row-1', data: { name: CONTROL } }], + options: {}, + }), + // The atomic arm shares `runUpdateManyLoop`, but drive it too so the + // transaction wrapper cannot grow its own record echo unexamined. + (p) => p.updateManyData({ + object: 'vault', + records: [{ id: 'row-1', data: { name: CONTROL } }], + options: { atomic: true }, + }), + ], + expectRecord: true, + }, + batchData: { + invocations: [ + (p) => p.batchData({ + object: 'vault', + request: { operation: 'create', records: [{ data: { name: CONTROL } }], options: {} }, + }), + (p) => p.batchData({ + object: 'vault', + request: { operation: 'update', records: [{ id: 'row-1', data: { name: CONTROL } }], options: {} }, + }), + // Upsert, both forks: with an id (probe finds the row → update arm) and + // without one (insert arm). + (p) => p.batchData({ + object: 'vault', + request: { operation: 'upsert', records: [{ id: 'row-1', data: { name: CONTROL } }], options: {} }, + }), + (p) => p.batchData({ + object: 'vault', + request: { operation: 'upsert', records: [{ data: { name: CONTROL } }], options: {} }, + }), + // Atomic — this is what walks `runAtomicBatchData` for real. + (p) => p.batchData({ + object: 'vault', + request: { operation: 'create', records: [{ data: { name: CONTROL } }], options: { atomic: true } }, + }), + ], + expectRecord: true, + }, + runAtomicBatchData: { coveredVia: 'batchData' }, +}; + +describe('#7823 tripwire: every generic data ingress strips `internal: true` from its write response', () => { + const enumerated = enumerateDataMethods(ObjectStackProtocolImplementation.prototype); + + it('the enumeration is real: it sees the three ruling-named ingresses', () => { + expect(enumerated).toEqual(expect.arrayContaining(['createData', 'updateData', 'cloneData'])); + }); + + it('every `*Data` method has a recipe — a NEW ingress must register here', () => { + const missing = enumerated.filter((name) => !(name in RECIPES)); + expect( + missing, + `New generic data ingress(es) with no tripwire recipe: ${missing.join(', ')}. ` + + 'A `*Data` method is a generic-data-path surface (#7823): route every engine ' + + 'write result it returns through `omitInternalFieldsFromWriteResponse` (the ' + + 'single exported helper in write-response-internal-fields.ts), then add a ' + + 'recipe for it in this file so the strip is held by measurement.', + ).toEqual([]); + // …and the map carries no dead entries for methods that no longer exist. + const stale = Object.keys(RECIPES).filter((name) => !enumerated.includes(name)); + expect(stale, `Tripwire recipes for methods that no longer exist: ${stale.join(', ')}`).toEqual([]); + }); + + it('a `coveredVia` entry points at a real recipe, never at another alias', () => { + for (const [name, recipe] of Object.entries(RECIPES)) { + if ('coveredVia' in recipe) { + const target = RECIPES[recipe.coveredVia]; + expect(target, `${name} says coveredVia '${recipe.coveredVia}', which has no recipe`).toBeTruthy(); + expect('invocations' in (target as any), `${name}'s coveredVia target must carry real invocations`).toBe(true); + } + } + }); + + for (const name of enumerated) { + const recipe = RECIPES[name]; + if (!recipe || 'coveredVia' in recipe) continue; + it(`${name}: response never carries the internal sentinel${recipe.expectRecord ? ', and really returned a record' : ''}`, async () => { + for (const invoke of recipe.invocations) { + const p = new ObjectStackProtocolImplementation(makeSentinelEngine()); + const response = await invoke(p); + const wire = JSON.stringify(response ?? null); + expect(wire.includes(SENTINEL), `${name} leaked an internal field: ${wire}`).toBe(false); + if (recipe.expectRecord) { + expect(wire.includes(CONTROL), `${name} returned no record at all — the probe is blind: ${wire}`).toBe(true); + } + } + }); + } + + it('NEGATIVE CONTROL: the machinery goes red on an ingress that skips the helper', async () => { + // A future author adds a write face and forgets the helper. Prove both + // halves of the defence: the enumeration picks the method up, and the + // sentinel scan catches its leak. + class LeakyProtocol extends ObjectStackProtocolImplementation { + async leakyData(request: { object: string; id: string; data: any }) { + const result = await (this as any).engine.update(request.object, request.data, { where: { id: request.id } }); + return { object: request.object, id: request.id, record: result }; + } + } + const names = enumerateDataMethods(LeakyProtocol.prototype); + expect(names).toContain('leakyData'); // half 1: a new `*Data` face cannot hide + expect(names.filter((n) => !(n in RECIPES))).toEqual(['leakyData']); // …and it has no recipe → the completeness arm above would fail + + const p = new LeakyProtocol(makeSentinelEngine()); + const wire = JSON.stringify(await p.leakyData({ object: 'vault', id: 'row-1', data: { name: 'x' } })); + expect(wire.includes(SENTINEL)).toBe(true); // half 2: the scan detects the leak + + // And the helper is exactly what closes it — same response, one call. + const fixed = await p.leakyData({ object: 'vault', id: 'row-1', data: { name: 'x' } }); + omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, (fixed as any).record); + expect(JSON.stringify(fixed).includes(SENTINEL)).toBe(false); + }); + + it('the collector agrees with the engine rule: strict `internal === true` only', () => { + expect(collectInternalWriteResponseFields(VAULT_SCHEMA)).toEqual(['vault_secret']); + expect(collectInternalWriteResponseFields({ + name: 'x', + fields: { a: { internal: 'true' }, b: { internal: 1 }, c: {}, d: { internal: true } }, + })).toEqual(['d']); + expect(collectInternalWriteResponseFields(undefined)).toEqual([]); + expect(collectInternalWriteResponseFields({ name: 'x' })).toEqual([]); + }); + + it('the strip is idempotent and skips non-records', () => { + const row: any = { id: '1', vault_secret: SENTINEL, name: CONTROL }; + omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, row); + expect(row).toEqual({ id: '1', name: CONTROL }); + omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, row); // second pass: no-op + expect(row).toEqual({ id: '1', name: CONTROL }); + expect(() => omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, null)).not.toThrow(); + expect(() => omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, 3)).not.toThrow(); + expect(() => omitInternalFieldsFromWriteResponse(VAULT_SCHEMA, [row, null, 7])).not.toThrow(); + }); +}); diff --git a/packages/metadata-protocol/src/write-response-internal-fields.ts b/packages/metadata-protocol/src/write-response-internal-fields.ts new file mode 100644 index 0000000000..8e88526e45 --- /dev/null +++ b/packages/metadata-protocol/src/write-response-internal-fields.ts @@ -0,0 +1,112 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7823] The write-response half of the `internal: true` guarantee — applied + * at the generic-data-path INGRESS, by maintainer ruling (2026-08-13, A-prime). + * + * ## The contract + * + * A field declared `internal: true` is *never returned on the generic data + * path* (#7728). The READ half lives in the engine (`omitInternalFields` runs + * on every find/findOne result). The WRITE-RESPONSE half lives HERE: every + * protocol `*Data` face that hands an engine write result back to its caller + * passes the record(s) through {@link omitInternalFieldsFromWriteResponse} + * before building its response. + * + * ## Why the ingress and not the engine (the measured history) + * + * The first shape stripped `internal` fields inside the engine's insert and + * by-id-update paths. That conflated two different guarantees: + * + * - "never returned on the generic data path" — the flag's sentence, about + * what an EXTERNAL caller receives; and + * - "never returned to the engine-level caller that performed the write" — + * which no ruling ever asked for, and which is FALSE for credential mint: + * better-auth's `createWithHooks` reads the minted `sys_session` row back + * off the insert result, so the engine-side strip broke `signIn`/`signUp` + * outright (measured: `verify signIn: no token in response`). + * + * Plain removal of the engine limbs was ALSO measured wrong: the by-id-update + * strip was the sole closure of #7728's fourth surface — with it neutralised, + * `PATCH /data/sys_api_key/{id}` answered 200 with the stored 64-hex `key` + * hash in the body. Both measurements are satisfiable at exactly one boundary: + * the ingress that builds the external 201/200 bodies. Engine write results + * keep the stored row whole (mint works); every external write response is + * stripped here (the hash never leaves); the read path is untouched. + * + * ## The residual risk, and what gates it + * + * Response-body policy at the ingress means a FUTURE generic write face that + * forgets this helper leaks silently. The ruling does not accept that as a + * future problem: `protocol.write-response-internal-fields.tripwire.test.ts` + * enumerates every `*Data` method on the protocol class (by name convention, + * walking the prototype), drives each against a fixture engine whose write + * results carry a flagged sentinel, and fails on any response the sentinel + * reaches — AND fails when a `*Data` method exists that the tripwire has no + * recipe for, so a new ingress cannot ship unexamined. Adding a `*Data` face? + * Route its response records through this helper and give the tripwire a + * recipe. + * + * ## Semantics + * + * Mirrors the engine's `collectInternalReadFields` rule exactly — a field + * participates iff its declaration carries `internal === true` (strict + * boolean; truthy strings and numbers do not count, same as the engine). + * `@objectstack/metadata-protocol` cannot import that collector + * (`@objectstack/objectql` depends on this package), so the rule is restated + * here in full; `internal-fields.test.ts` in objectql and the tripwire here + * pin the same spelling from both sides. OMIT, not mask, for the #7728 + * reasons: the flag's columns are `required`, so a mask carries zero bits + * while still shipping a value under a field whose description promises none. + * + * Deletion is IN PLACE and idempotent: records that already lack the field + * (a re-stripped read result, a fake engine that never returned it) pass + * through unchanged, and non-record values (`null`, an affected-row count, a + * driver's boolean delete verdict) are skipped rather than judged. + */ + +/** Minimal view of an object schema this module reads — the field map only. */ +interface SchemaWithFields { + fields?: Record | undefined; +} + +/** + * Collect the names of fields declared `internal: true` on `schema`. + * + * Same verdicts as objectql's `collectInternalReadFields` (see the module + * header for why it is restated rather than imported): strict `=== true`, + * empty result for a missing/field-less schema. + */ +export function collectInternalWriteResponseFields(schema: unknown): string[] { + const fields = (schema as SchemaWithFields | null | undefined)?.fields; + if (!fields || typeof fields !== 'object') return []; + const out: string[] = []; + for (const [name, def] of Object.entries(fields)) { + if (def && def.internal === true) out.push(name); + } + return out; +} + +/** + * Drop every `internal: true` field from a write response's record(s), in + * place. THE single helper every generic write ingress goes through — see the + * module header; the tripwire test enforces the "every". + * + * @param schema The registered object schema (`engine.registry.getObject(...)` + * / the protocol's own registry view). An unknown object (no + * schema) strips nothing — the write itself would have been + * refused upstream by the object-existence gate. + * @param records A single record, an array of records, or anything a write + * face hands back where a record could sit (`null`, a count, a + * boolean): non-objects are skipped, arrays are walked. + */ +export function omitInternalFieldsFromWriteResponse(schema: unknown, records: unknown): void { + if (!records) return; + const internalFields = collectInternalWriteResponseFields(schema); + if (internalFields.length === 0) return; + const list = Array.isArray(records) ? records : [records]; + for (const row of list) { + if (!row || typeof row !== 'object') continue; + for (const field of internalFields) delete (row as Record)[field]; + } +} diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index b390ae2d1c..ec77b7f842 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5003,6 +5003,19 @@ export class ObjectQL implements IObjectQLEngine { * already produced, so the predicate has been evaluated and the index used * before this method sees anything — which is precisely why authentication * keeps working. + * + * **READ path only** (#7823, maintainer ruling 2026-08-13 / A-prime). This + * used to run at the two write-response sites as well (the 201 create body + * and the by-id-update 200 body), which conflated "never returned on the + * generic data path" with "never returned to the engine-level caller that + * performed the write" — and broke authentication for `sys_session.token`, + * whose minted value better-auth reads back off the insert result. The + * write-response half of the guarantee now lives at the generic-data-path + * ingress (`omitInternalFieldsFromWriteResponse` in + * `@objectstack/metadata-protocol`, tripwire-enforced across every `*Data` + * write face); engine write results keep the stored row whole. Lifecycle + * consumers that need a flagged value back off a READ go through + * {@link resolveInternalField} — never a carve-out here. */ private omitInternalFields(object: string, rows: any): void { if (!rows) return; @@ -8179,12 +8192,23 @@ export class ObjectQL implements IObjectQLEngine { // AFTER the hook dispatch, matching the read path: `afterInsert` // handlers still observe the whole stored row. stripSearchCompanion(rowCtx.result); - // [#7728] Same position, same reason, for `internal` fields. A write - // has no projection to consult here either, so the omit is - // unconditional. This does NOT touch the show-once mint path: that - // route reads only `id` off the insert result and returns the - // plaintext it generated itself. - this.omitInternalFields(object, rowCtx.result); + // [#7823] `internal` fields are deliberately NOT stripped here. An + // earlier revision omitted them from this create result (#7728's + // "two write-response sites"), which conflated two different + // guarantees: "never returned on the generic data path" (the flag's + // sentence) and "never returned to the engine-level caller that + // performed the write". For `sys_session.token` those are OPPOSITE + // requirements — better-auth's `createWithHooks` reads the minted + // session row back off exactly this result, so the strip here broke + // `signIn`/`signUp` outright (measured: `verify signIn: no token in + // response`). Maintainer ruling 2026-08-13 (A-prime, #7823): the + // write-response strip lives at the GENERIC-DATA-PATH INGRESS — + // `omitInternalFieldsFromWriteResponse` in + // `@objectstack/metadata-protocol`, applied by every `*Data` write + // face and held there by a tripwire test — while engine-level write + // results keep the stored row whole for the privileged server-side + // caller that just wrote it. The generic READ path is unchanged: + // {@link omitInternalFields} still runs on every find/findOne. } // Roll-up: recompute parent summary fields that aggregate this object. @@ -9259,16 +9283,21 @@ export class ObjectQL implements IObjectQLEngine { // an affected-row COUNT (#4639), which the strip skips as a // non-object. stripSearchCompanion(hookContext.result); - // [#7728] …and the same for `internal` fields, on the identical - // argument. This is not a hypothetical symmetry: `sys_api_key` is - // one of the few identity objects with a write verb open - // (`apiMethods: ['get','list','update']`, #7727) and its declared - // revoke/restore row actions PATCH it, so before this line a client - // revoking a key got the stored hash back in the 200 body — measured, - // and the fourth leaking surface on the object #7728 was filed - // against. A predicate update resolves to an affected-row COUNT - // (#4639), which the omit skips as a non-object. - this.omitInternalFields(object, hookContext.result); + // [#7823] `internal` fields are deliberately NOT stripped here — + // the write-response omit this line used to carry (#7728's fourth + // measured surface: a client revoking a `sys_api_key` got the + // stored hash back in the PATCH 200 body, `apiMethods: + // ['get','list','update']`, #7727) RELOCATED to the + // generic-data-path ingress under the 2026-08-13 A-prime ruling on + // #7823. That surface is still closed — `updateData` in + // `@objectstack/metadata-protocol` builds the PATCH 200 body and + // passes it through `omitInternalFieldsFromWriteResponse`, pinned + // by `api-key-hash-not-serialized.dogfood.test.ts` and the + // ingress tripwire test — while the engine-level result keeps the + // stored row whole for privileged server-side writers (the same + // reason as the insert path: a strip HERE also fired on writes no + // external caller ever sees). The generic READ path is unchanged: + // {@link omitInternalFields} still runs on every find/findOne. // The record IS updated; a summary that could not recompute after // retries must surface, not stay silent (framework#3147). if (summaryFailures.length > 0) throw new SummaryRecomputeError(summaryFailures, hookContext.result); diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index b866059fd8..80ec13d4ec 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -16,6 +16,10 @@ import { liftClientSecretForWrite, type SecretResolvingEngine, } from './sso-client-secret.js'; +import { + reattachSessionTokenOnRead, + type InternalFieldResolvingEngine, +} from './session-token-readback.js'; /** * Mapping from better-auth model names to ObjectStack protocol object names. @@ -700,6 +704,13 @@ 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: + // `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`. + 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, // its camelCase model fields are also converted to snake_case columns on the @@ -779,6 +790,17 @@ 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( + internalFieldEngine, + objectName, + result, + bridged && select ? select.map(camelToSnake) : select, + ); const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; }, @@ -808,6 +830,12 @@ 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 + // 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); 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 new file mode 100644 index 0000000000..a66aa8fd1f --- /dev/null +++ b/packages/plugins/plugin-auth/src/session-token-readback.test.ts @@ -0,0 +1,114 @@ +// 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 new file mode 100644 index 0000000000..936711e321 --- /dev/null +++ b/packages/plugins/plugin-auth/src/session-token-readback.ts @@ -0,0 +1,138 @@ +// 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/session-token-not-serialized.dogfood.test.ts b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts index 75ca019f14..de99d04ef1 100644 --- a/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts +++ b/packages/qa/dogfood/test/session-token-not-serialized.dogfood.test.ts @@ -264,6 +264,73 @@ describe('#7823: sys_session.token (a live bearer) never serializes on the gener expect(await stillAuthenticates(victim)).toBe(false); }); + it('revoke-other-sessions ACTUALLY revokes — the other session stops authenticating (#7823 Q2)', async () => { + // The measured composition defect: with the read strip in place and no + // readback seam, better-auth's revoke-other-sessions filtered + // `listSessions(userId)` rows by a `token` every row had lost — so it + // answered `200 {"status":true}` while the user's OTHER session kept + // authenticating. A security control reporting success while doing + // nothing. The fix routes the adapter's session reads through + // `Engine.resolveInternalField` (#8118); this arm holds it there. + // + // Asserted on the OTHER SESSION'S LIVENESS, not the status code — the + // status code is exactly what lied. + const email = 'session-token-revoke-others@verify.test'; + const olderSession = await stack.signUp(email); + const currentSession = await stack.signIn(email, 'Member-Pass-123'); + expect(olderSession).not.toBe(currentSession); + expect(await stillAuthenticates(olderSession)).toBe(true); + expect(await stillAuthenticates(currentSession)).toBe(true); + + const res = await stack.apiAs(currentSession, 'POST', '/auth/revoke-other-sessions', {}); + expect(res.status).toBe(200); + const body: any = await res.json(); + expect(body?.status).toBe(true); + + // The half that was silently false before: the other session is GONE… + expect(await stillAuthenticates(olderSession)).toBe(false); + // …and the caller kept their own — revoke-OTHER, not revoke-all. + expect(await stillAuthenticates(currentSession)).toBe(true); + }); + + it('expired-session cleanup still lands — the expired row is removed, not just refused (#7823 Q2)', async () => { + // Same seam, second consumer: when bearer validation meets an EXPIRED + // row, better-auth deletes it by `session.token` read back off the row it + // just fetched through the adapter. Token-less rows turn that into a + // refusal that leaves the credential row in storage forever. Observe the + // ROW, not the response — the refusal looks identical either way. + const ql = await stack.kernel.getServiceAsync('objectql'); + const bearer = await stack.signUp('session-token-expired-cleanup@verify.test'); + expect(await stillAuthenticates(bearer)).toBe(true); + + const rowsFor = async (): Promise => + (await ql.find('sys_session', { + where: { token: bearer }, + context: { isSystem: true }, + })) as any[]; + expect((await rowsFor()).length).toBe(1); + + // Expire the row in place (system write — the identity write guard admits + // the platform's own maintenance writes, and this is storage state, not a + // route behaviour). + const [row] = await rowsFor(); + await ql.update( + 'sys_session', + { id: row.id, expires_at: new Date(Date.now() - 60_000).toISOString() }, + { where: { id: row.id }, context: { isSystem: true } }, + ); + + // Bearer validation on the expired session must refuse… + expect(await stillAuthenticates(bearer)).toBe(false); + // …and the cleanup must have LANDED: the row is deleted (or tombstoned), + // which is precisely the write that no-ops when `token` is missing. A row + // still sitting there un-revoked is the silent-no-op state — the refusal + // above looks identical whether or not the delete happened, so the ROW is + // the only honest witness. + const lingering = (await rowsFor()).filter((r: any) => r.revoked_at == null); + expect(lingering.length, 'expired-session cleanup must remove/tombstone the row, not silently no-op').toBe(0); + }); + it('the by-token session lookup still resolves server-side — the value is still in STORAGE', async () => { // The load-bearing negative assertion. `internal` is a SERIALIZATION // contract, not a storage one: the strip runs on rows the driver has diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 4d2e7b54bd..945cb6e1e9 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -10601,13 +10601,27 @@ export class RestServer { } out.push(created?.record); } else if (op.action === 'update') { - // Update needs no ingress detour: the engine enforces - // both static `readonly` (#2948) and `readonlyWhen` - // (#3042) on its own update path, and reports them - // through this listener. + // Update needs no ingress detour for the WRITE half: + // the engine enforces both static `readonly` (#2948) + // and `readonlyWhen` (#3042) on its own update path, + // and reports them through this listener. const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); }; const id = op.id ?? data?.id; - out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped })); + const updated = await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped }); + // [#7823] …but the RESPONSE half moved to the ingress + // (A-prime ruling, 2026-08-13): the engine no longer + // strips `internal: true` fields from its write + // results, so this direct-`ql.update` mouth must + // apply the shared strip itself before the row rides + // `results` out to the caller. Reached through the + // protocol instance because this package does not + // depend on `@objectstack/metadata-protocol` (same + // duck-typing as the `createManyData` probes). + // Dormant today — no `internal`-flagged object grants + // `bulk` — wired so the flag's guarantee does not + // depend on that staying true. + (p as any).omitInternalWriteFields?.(op.object, updated); + out.push(updated); } else { // 'delete' out.push(await ql.delete(op.object, { where: { id: op.id }, context: trxCtx })); } From e2d139e2113b07d868f7925ba079b969d5f7f41b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 14:21:22 +0000 Subject: [PATCH 5/6] test(objectql): re-judge the two engine-level write-response pins under A-prime (#7823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They pinned exactly the limb the ruling relocates — the engine omitting internal fields from its own insert/update results, which is what broke signIn. Replaced wholesale (fixture-triage rule): they now pin the LIVENESS half — engine write results KEEP the flagged value — so re-adding an engine-level strip (the regression that broke authentication) goes red here, while the external-body guarantee is pinned at the ingress by the tripwire and the dogfood suites. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- packages/objectql/src/internal-fields.test.ts | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/packages/objectql/src/internal-fields.test.ts b/packages/objectql/src/internal-fields.test.ts index b28ebdbd9c..26829bf035 100644 --- a/packages/objectql/src/internal-fields.test.ts +++ b/packages/objectql/src/internal-fields.test.ts @@ -232,23 +232,36 @@ describe('#7728: the `internal` field flag omits a value from the generic data p }); }); - describe('the write-response surfaces', () => { - it('omits the flagged field from the create body', async () => { + describe('the write-response surfaces (RELOCATED to the ingress — #7823 A-prime)', () => { + // These two pins used to assert the OPPOSITE: that the engine omitted the + // flagged field from its own insert/update results. That conflated "never + // returned on the generic data path" with "never returned to the + // engine-level caller that performed the write" — and for + // `sys_session.token` those are opposite requirements: better-auth's + // `createWithHooks` reads the minted session row back off the insert + // result, so the engine-side strip broke `signIn`/`signUp` outright + // (measured: `verify signIn: no token in response`). Under the 2026-08-13 + // A-prime ruling the ENGINE keeps write results whole, and the external + // 201/200 bodies are stripped at the generic-data-path ingress + // (`omitInternalFieldsFromWriteResponse` in @objectstack/metadata-protocol, + // held there by its own tripwire test across every `*Data` face — that is + // where #7728's fourth surface, the sys_api_key PATCH body, stays closed). + // + // The assertions below are the LIVENESS half of that ruling: they go RED + // if anyone re-adds an engine-level write-response strip, which is the + // exact regression that broke authentication. + it('the create RESULT keeps the flagged field — mint reads it back off this value', async () => { const created = await seed(); - expect(Object.keys(created)).not.toContain('key'); - // The create still returns a usable record — the mint path reads `id` - // off exactly this value. + expect(created.key).toBe(HASH); expect(created.id).toBeTruthy(); }); - it('omits the flagged field from the by-id update body', async () => { - // The surface measured leaking on `sys_api_key` itself: that object has - // `update` open (#7727) and its revoke/restore row actions PATCH it. + it('the by-id update RESULT keeps the flagged field — engine callers are privileged writers', async () => { const created = await seed(); const updated = await ctx.engine.update('itest_api_key', { id: created.id, revoked: true }, { context: { isSystem: true }, } as any); - expect(Object.keys(updated)).not.toContain('key'); + expect(updated.key).toBe(HASH); expect(updated.revoked).toBe(true); }); }); From 2c86a2227a3bb4afb2a6bde52494531dac00a443 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 15:19:15 +0000 Subject: [PATCH 6/6] fix(metadata-protocol): pin the tripwire fixture's delete/update to the shared dispatch predicate (#7823) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:engine-double-contract flagged makeSentinelEngine()'s delete()/update() as unpinned engine doubles — exactly the toolchain trap AGENTS.md names. Route both through assertEngineDeleteDispatch/assertEngineUpdateDispatch from @objectstack/metadata-core, the same pattern protocol.batch-verb-driver-text.test.ts already uses. No behavioural change to the tripwire itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- ...rite-response-internal-fields.tripwire.test.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts b/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts index 0a260773a4..66b3f7d6fc 100644 --- a/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts +++ b/packages/metadata-protocol/src/protocol.write-response-internal-fields.tripwire.test.ts @@ -45,6 +45,7 @@ // enumeration is shown to pick it up, and the scan is shown to catch its leak. import { describe, it, expect } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { ObjectStackProtocolImplementation } from './protocol.js'; import { collectInternalWriteResponseFields, @@ -93,10 +94,18 @@ function makeSentinelEngine() { : writtenRow(data?.id ?? `new-${nextId++}`, data), insertMany: async (_object: string, rows: any[]) => rows.map((r: any) => ({ ok: true, record: writtenRow(r?.id ?? `new-${nextId++}`, r) })), - update: async (_object: string, data: any, options?: any) => - writtenRow(options?.where?.id ?? data?.id ?? 'row-1', data), + update: async (_object: string, data: any, options?: any) => { + // [#5480] The producer's own update-verb dispatch contract, so this fake + // cannot accept a call `ObjectQL.update` refuses (check:engine-double-contract). + assertEngineUpdateDispatch(data, options); + return writtenRow(options?.where?.id ?? data?.id ?? 'row-1', data); + }, // Contract per #4435: `false` is the positive not-found value. - delete: async (_object: string, _options?: any) => ({ deleted: 1 }), + delete: async (_object: string, options?: any) => { + // [#4550] Likewise for delete. + assertEngineDeleteDispatch(options); + return { deleted: 1 }; + }, findOne: async (_object: string, options?: any) => storedRow(options?.where?.id ?? 'row-1'), find: async (_object: string, _options?: any) => [storedRow('row-1')], count: async () => 1,