From 85db9b9bb0e153698ee7ad4503cb779676734803 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 05:17:22 +0000 Subject: [PATCH 1/4] fix(plugin-auth): an organization-delete read that fails refuses 503 before better-auth's first write The organization-delete admission caught every throw in its lookup block and handed the request to better-auth, so a failed sys_member or sys_organization read let better-auth clear the active organization and delete the members and invitations with nothing counted. A failed read now refuses with a typed 503 SERVICE_UNAVAILABLE, and so does a failed Default Organization decision, which answered a bare 500. The vendor-decided cases still fall through. Claude-Session: https://claude.ai/code/session_01UnHcTADjeg3VwxF21QHj7W Co-authored-by: Claude --- .../plugins/plugin-auth/src/auth-manager.ts | 80 +++++++++++++------ .../plugin-auth/src/organization-delete.ts | 33 +++++++- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index fb0a015c00a..f3015ed6f7b 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -211,9 +211,11 @@ import { } from './signing-key-read-cache.js'; import { ORGANIZATION_DELETE_PATH, + ORGANIZATION_DELETE_READ_FAULT_CODE, ORGANIZATION_DELETE_REFUSAL_CODE, defaultOrganizationDeleteRefusal, isDefaultOrganizationTarget, + organizationDeleteReadFaultBody, organizationDeleteRefusal, organizationNotEmptyBody, removeOrganizationAuthStructure, @@ -8314,12 +8316,16 @@ export class AuthManager { * Only for a caller the vendor itself admits — a member whose role holds * `organization: ['delete']`, asked of the vendor's own `hasPermission`, as * {@link assertRemoveMemberPermitted} does — so a caller it refuses gets its - * refusal, and no count. Everything this cannot decide falls through to the - * vendor: no session, no organization id, no membership, a lookup that - * failed. The COUNT does not fall through: it is the guard, and a guard that - * could not be evaluated must not pass, so its failure propagates, and so + * refusal, and no count. What the vendor itself answers falls through to it: + * no session, no organization id, no membership, no permission, no + * organization row. A READ this decides on that fails does not fall + * through [#25059]: the vendor's own reads could then succeed and its writes + * run with nothing counted, so it refuses here with 503 + * `SERVICE_UNAVAILABLE` ({@link organizationDeleteReadFaultBody}), and so * does a failure deciding whether the target is the Default Organization. - * The Default Organization is asked the last-administrator guard's question + * The COUNT does not fall through either: it is the guard, and a guard that + * could not be evaluated must not pass, so its failure propagates. The + * Default Organization is asked the last-administrator guard's question * first (a 403), then its own record count (the same 409), both here, before * the first write. */ @@ -8329,17 +8335,34 @@ export class AuthManager { const organizationId = typeof ctx?.body?.organizationId === 'string' ? ctx.body.organizationId : ''; if (!organizationId) return; - let organization: any; + const sys = withSystemReadContext(engine); + const read = async (object: string, run: () => Promise): Promise => { + try { + return await run(); + } catch (cause) { + const { APIError } = await import('better-auth/api'); + const err = new APIError('SERVICE_UNAVAILABLE', { + code: ORGANIZATION_DELETE_READ_FAULT_CODE, + ...organizationDeleteReadFaultBody(object), + }); + (err as { cause?: unknown }).cause = cause; + throw err; + } + }; + + // `resolveActor` never throws: a session read that fails answers null, as + // no session does, so that one reaches the vendor, whose own session read + // decides it. + const actor = await this.resolveActor(ctx); + if (!actor?.userId) return; // the vendor answers 401 + const callerRow: any = await read('sys_member', () => + sys.findOne('sys_member', { where: { organization_id: organizationId, user_id: actor.userId } }), + ); + if (!callerRow) return; // the vendor answers USER_IS_NOT_A_MEMBER_OF_THE_ORGANIZATION + let permitted: boolean; try { - const actor = await this.resolveActor(ctx); - if (!actor?.userId) return; // the vendor answers 401 - const sys = withSystemReadContext(engine); - const callerRow: any = await sys.findOne('sys_member', { - where: { organization_id: organizationId, user_id: actor.userId }, - }); - if (!callerRow) return; // the vendor answers USER_IS_NOT_A_MEMBER_OF_THE_ORGANIZATION const { hasPermission } = await import('better-auth/plugins/organization'); - const permitted = await hasPermission( + permitted = await hasPermission( { role: callerRow.role, options: (this.orgRolesMap ? { roles: this.orgRolesMap } : {}) as any, @@ -8348,21 +8371,26 @@ export class AuthManager { } as any, ctx, ); - if (!permitted) return; // the vendor answers YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_ORGANIZATION - organization = await sys.findOne('sys_organization', { where: { id: organizationId } }); - if (!organization) return; // the vendor answers 400 } catch { - // A lookup that did not complete hands the request back to the vendor, - // which re-reads everything it decides; the engine's own refusal still - // stands under the delete. + // Not a read: handed a roles map and no dynamic access control, the + // vendor's predicate reads nothing, so it throws only on the member row + // itself (a role that is no string, or names an inherited object + // member), and the vendor's identical call on that row throws too, + // before its first write. The vendor answers this. return; } - - // Which of the two refusals below applies is part of the guard, so it is - // decided outside the fall-through above: a lookup that fails here - // refuses, never hands the request to the vendor unasked. - const defaultOrganization = await isDefaultOrganizationTarget( - engine, organization, postureEnforcesWall(resolveTenancyPosture()), + if (!permitted) return; // the vendor answers YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_ORGANIZATION + const organization: any = await read('sys_organization', () => + sys.findOne('sys_organization', { where: { id: organizationId } }), + ); + if (!organization) return; // the vendor answers 400 + + // Which of the two refusals below applies is part of the guard, so a + // lookup that fails deciding it refuses too, never hands the request to + // the vendor unasked. + const walled = postureEnforcesWall(resolveTenancyPosture()); + const defaultOrganization = await read('sys_organization', () => + isDefaultOrganizationTarget(engine, organization, walled), ); // The Default Organization is asked the last-administrator guard's diff --git a/packages/plugins/plugin-auth/src/organization-delete.ts b/packages/plugins/plugin-auth/src/organization-delete.ts index 5dd4c47fad9..c46c1e398a9 100644 --- a/packages/plugins/plugin-auth/src/organization-delete.ts +++ b/packages/plugins/plugin-auth/src/organization-delete.ts @@ -48,7 +48,9 @@ * subject-erasure wrapper's posture. On a driver without transactions the * refusals are the guarantee, since every one runs before the first write: * the count above, and the Default Organization's guard question and count - * below. + * below. So does the refusal of a delete the admission could not decide + * because a read it decides on failed (#25059: 503 `SERVICE_UNAVAILABLE`, + * {@link organizationDeleteReadFaultBody}). * 4. **The engine never clears the tenant column.** Under all of this, * `ObjectQL.cascadeDeleteRelations` refuses a `sys_organization` delete * while any row still names it, whatever path reaches it. @@ -202,6 +204,35 @@ export function organizationNotEmptyBody(owned: readonly OrganizationOwnedRows[] }; } +/** + * [#25059] The wire code for a delete the admission could not decide because a + * read it decides on failed: the standard catalog's member for a store that + * cannot answer right now (ADR-0112), the code the organization slug guard's + * and the invitation role cap's read faults answer with too. + */ +export const ORGANIZATION_DELETE_READ_FAULT_CODE: StandardErrorCode = 'SERVICE_UNAVAILABLE'; + +/** + * [#25059] The 503 body for that delete, beside its code + * ({@link ORGANIZATION_DELETE_READ_FAULT_CODE}, spelled at the throw site). + * + * The admission runs before better-auth's first write, so a read it decides on + * that fails refuses there: handing the request on would let better-auth clear + * the caller's active organization and delete the members and invitations with + * nothing counted, and a driver without transactions keeps those deletes when + * the engine's own refusal lands on the organization row. It is no verdict + * about the delete (neither the 403 nor the 409 code), and it names the table, + * never the driver's message. + */ +export function organizationDeleteReadFaultBody(object: string): { message: string } { + return { + message: + `The organization delete could not read \`${object}\`, so whether this organization may be deleted ` + + 'was never decided, and nothing was deleted. This is a server-side outage, not a refusal of the ' + + 'delete: retry once the data store is reachable.', + }; +} + /** * Is `organization` the Default Organization: on a posture with no * organization wall, the one with `slug = 'default'`, else the deployment's From 947ab6c25fd57040ef33ad290888ae799e4a6009 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 05:20:04 +0000 Subject: [PATCH 2/4] test(plugin-auth): pin the organization-delete read-fault refusal per read, with controls Claude-Session: https://claude.ai/code/session_01UnHcTADjeg3VwxF21QHj7W Co-authored-by: Claude --- .../25059-org-delete-read-fault-refuses.md | 16 ++ .../organization-delete-read-fault.test.ts | 255 ++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 .changeset/25059-org-delete-read-fault-refuses.md create mode 100644 packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts diff --git a/.changeset/25059-org-delete-read-fault-refuses.md b/.changeset/25059-org-delete-read-fault-refuses.md new file mode 100644 index 00000000000..089ca2208b9 --- /dev/null +++ b/.changeset/25059-org-delete-read-fault-refuses.md @@ -0,0 +1,16 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): an organization delete whose admission cannot read what it decides on is refused with 503 `SERVICE_UNAVAILABLE`, before better-auth's first write + +Clause-②: no (the refusal replaces an outcome only a failed store read produces, and the typed 503 replaces a bare 500 fault; no request a caller can make is answered differently) +Metadata-protocol: none + +The `POST /organization/delete` admission counts the records an organization still owns before better-auth's handler runs, so a refused delete writes nothing on any driver. + +- **A read the admission decides on that fails now refuses.** When its read of the caller's membership or of the organization row failed, the admission handed the request to better-auth, which could then clear the caller's active organization and delete the members and invitations with nothing counted. On a driver without transactions those deletes stayed while the organization row survived. The admission now answers `503` with the standard `SERVICE_UNAVAILABLE` code, before better-auth's first write. The body names the table it could not read; the driver's own message is not relayed. +- **A failed Default Organization decision answers the same `503`.** It already refused before any write, but as a bare error, which better-auth answers with an untyped `500`. +- **Unchanged.** What better-auth answers itself still goes to it: no session, no membership, no permission, no organization row, and a member row whose role the vendor's own permission check cannot evaluate (its identical check runs before its first write). The `409 DELETE_RESTRICTED` refusal and the Default Organization's `403` are unchanged. + +**What a client does.** Treat `503 SERVICE_UNAVAILABLE` from an organization delete as "retry later": nothing was deleted. diff --git a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts new file mode 100644 index 00000000000..bcda33cdec2 --- /dev/null +++ b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts @@ -0,0 +1,255 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#25059] The organization-delete admission refuses a delete it could not + * decide because a read it decides on failed: 503 `SERVICE_UNAVAILABLE`, + * before better-auth's first write. + * + * The admission (`AuthManager.assertOrganizationDeletable`, in better-auth's + * global `hooks.before`) caught every throw in its lookup block and handed the + * request to better-auth. Better-auth re-reads what it decides and then + * writes: it clears the caller's active organization, then deletes the members + * and the invitations, and only then meets the engine's refusal at the + * organization row. On a driver without transactions those deletes stay. The + * Default Organization decision already refused on a failed lookup (#24948), + * but with a bare `Error`, which better-auth answers as a 500. + * + * Pinned here, each with its CONTROL (the same delete, the read back, goes + * ahead): + * + * - the caller's `sys_member` read fails: 503, nothing attempted; + * - the `sys_organization` read fails: 503, nothing attempted; + * - the Default Organization decision's count fails: 503, nothing attempted. + * + * And the case that keeps falling through: the vendor's own `hasPermission` + * reads nothing here, so it throws only on the member row itself, and the + * vendor's identical call throws first too, before its first write. + * + * Real stack: `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3, the + * objects from this plugin's manifest and a real `AuthManager` with the + * organization plugin (the `session-read-failure-answer.test.ts` harness). + * Each fault is scoped to the admission: the read throws once, and only when + * the admission itself makes it, so better-auth's own reads of the same table + * are the real ones. That is what lets a request that falls through reach + * better-auth's writes, which the engine hooks below record whether or not the + * request's transaction later rolls them back. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { AuthManager } from './auth-manager.js'; +import { authIdentityObjects } from './manifest.js'; + +const BASE = 'http://localhost:3000'; +const AUTH = `${BASE}/api/v1/auth`; +const SECRET = 'test-secret-at-least-32-chars-long-25059'; +const PASSWORD = 'S3cure!Passw0rd-25059'; +const SYSTEM = { context: { isSystem: true } } as never; +const TARGET = 'org_25059'; +const RECORDER = 'test.25059.attempted-writes'; + +/** The grant table the admission's count reads; declared locally, with the columns it reads. */ +const sysUserPermissionSet = { + name: 'sys_user_permission_set', + label: 'User Permission Set', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + user_id: { name: 'user_id', type: 'text' as const }, + permission_set: { name: 'permission_set', type: 'text' as const }, + organization_id: { name: 'organization_id', type: 'text' as const }, + }, +}; + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()!.destroy(); } catch { /* noop */ } + } +}); + +async function bootEngine(): Promise { + const engine = new ObjectQL(); + engines.push(engine); + engine.registerDriver(new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), true); + await engine.init(); + for (const object of authIdentityObjects) engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + engine.registry.registerObject(sysUserPermissionSet as never, '@objectstack/plugin-security'); + await engine.syncSchemas(); + return engine; +} + +const cookieFrom = (response: Response): string => + (response.headers.getSetCookie?.() ?? [response.headers.get('set-cookie') ?? '']) + .map((c) => c.split(';')[0]) + .filter(Boolean) + .join('; '); + +const post = (manager: AuthManager, path: string, body: unknown, cookie: string) => + manager.handleRequest(new Request(`${AUTH}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify(body), + })); + +async function rows(engine: ObjectQL, object: string, where: Record): Promise { + const found = await engine.find(object, { where, limit: 50, context: { isSystem: true } } as never); + return Array.isArray(found) ? found : ((found as { records?: any[] })?.records ?? []); +} + +/** + * An owner of `TARGET`, signed in with `TARGET` active, on a deployment with a + * second organization: so the target is not the Default Organization (its + * slug is not `default` and it is not the only one), and better-auth's first + * write, were the request to reach it, is the session's active organization. + */ +async function ownerOfTarget(role: string | null = 'owner') { + const engine = await bootEngine(); + const manager = new AuthManager({ + secret: SECRET, + baseUrl: BASE, + dataEngine: engine as never, + plugins: { organization: true }, + } as never); + const signedUp = await manager.handleRequest(new Request(`${AUTH}/sign-up/email`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', origin: BASE }, + body: JSON.stringify({ email: 'owner@example.com', password: PASSWORD, name: 'Owner' }), + })); + expect(signedUp.status, await signedUp.clone().text()).toBe(200); + const cookie = cookieFrom(signedUp); + const [user] = await rows(engine, 'sys_user', { email: 'owner@example.com' }); + await engine.insert('sys_organization', { id: TARGET, name: 'Org 25059', slug: 'org-25059' }, SYSTEM); + await engine.insert('sys_organization', { id: 'org_other', name: 'Other', slug: 'org-other' }, SYSTEM); + await engine.insert('sys_member', { id: 'mem_owner', user_id: String(user.id), organization_id: TARGET, role: 'owner' }, SYSTEM); + const active = await post(manager, '/organization/set-active', { organizationId: TARGET }, cookie); + expect(active.status, await active.clone().text()).toBe(200); + if (role !== 'owner') { + // Below the engine, which would apply the field's default to a null: the + // row as a store can hold it. + await (engine as any).getDriver('sys_member').update('sys_member', 'mem_owner', { role }); + } + return { engine, manager, cookie }; +} + +/** + * Every write better-auth's handler makes on the way to the delete, recorded + * at the engine when it is ATTEMPTED: the session's active organization (its + * first write), then the member, invitation and organization deletes. + */ +function recordAttemptedWrites(engine: ObjectQL): string[] { + const attempted: string[] = []; + engine.registerHook('beforeUpdate', async () => { attempted.push('update sys_session'); }, { object: 'sys_session', packageId: RECORDER }); + for (const object of ['sys_member', 'sys_invitation', 'sys_organization']) { + engine.registerHook('beforeDelete', async () => { attempted.push(`delete ${object}`); }, { object, packageId: RECORDER }); + } + return attempted; +} + +/** The current stack, deep enough to reach the request's frames (V8 keeps ten by default). */ +function deepStack(): string { + const limit = Error.stackTraceLimit; + Error.stackTraceLimit = 100; + try { + return new Error().stack ?? ''; + } finally { + Error.stackTraceLimit = limit; + } +} + +/** + * The admission's own `method` call on `object` throws, once: the call must be + * made from inside `frame` and by the admission itself (its first + * `AuthManager` frame is `assertOrganizationDeletable`, so a session read made + * inside `resolveActor` does not qualify). Every other call is the real one. + */ +function failAdmissionRead(engine: ObjectQL, method: 'findOne' | 'count', object: string, frame = 'assertOrganizationDeletable') { + let fired = 0; + const target = engine as unknown as Record Promise>; + const real = target[method].bind(engine); + const spy = vi.spyOn(target, method).mockImplementation(async (name: string, ...rest: unknown[]) => { + const stack = deepStack(); + const caller = /AuthManager\.(\w+)/.exec(stack)?.[1]; + if (fired === 0 && name === object && stack.includes(frame) && caller === 'assertOrganizationDeletable') { + fired += 1; + throw new Error(`${object} store unavailable (injected)`); + } + return real(name, ...rest); + }); + return { fired: () => fired, restore: () => spy.mockRestore() }; +} + +/** The refusal, asserted on its envelope: 503 with the standard code, naming the table it could not read. */ +async function expectReadFault(res: Response, object: string) { + const text = await res.clone().text(); + expect(res.status, text).toBe(503); + const body = JSON.parse(text) as { code?: string; message?: string }; + expect(body.code).toBe('SERVICE_UNAVAILABLE'); + expect(body.message).toContain(`\`${object}\``); + // The driver's own words stay on the server. + expect(text).not.toContain('injected'); +} + +async function expectNothingChanged(engine: ObjectQL, attempted: string[]) { + expect(attempted).toEqual([]); + expect(await rows(engine, 'sys_organization', { id: TARGET })).toHaveLength(1); + expect(await rows(engine, 'sys_member', { organization_id: TARGET })).toHaveLength(1); +} + +/** CONTROL: with the read back, the same delete reaches better-auth and goes ahead. */ +async function expectControlDeletes(engine: ObjectQL, manager: AuthManager, cookie: string) { + const attempted = recordAttemptedWrites(engine); + const ok = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); + engine.unregisterHooksByPackage(RECORDER); + expect(ok.status, await ok.clone().text()).toBe(200); + expect(attempted).toEqual(expect.arrayContaining(['update sys_session', 'delete sys_member', 'delete sys_organization'])); + expect(await rows(engine, 'sys_organization', { id: TARGET })).toHaveLength(0); + expect(await rows(engine, 'sys_member', { organization_id: TARGET })).toHaveLength(0); +} + +describe('[#25059] an organization-delete read that fails refuses 503 before better-auth\'s first write', () => { + const cases: Array<{ name: string; method: 'findOne' | 'count'; object: string; frame?: string }> = [ + { name: 'the caller\'s sys_member read', method: 'findOne', object: 'sys_member' }, + { name: 'the sys_organization read', method: 'findOne', object: 'sys_organization' }, + { name: 'the Default Organization decision\'s count', method: 'count', object: 'sys_organization', frame: 'isDefaultOrganizationTarget' }, + ]; + + for (const c of cases) { + it(`${c.name} fails: 503 SERVICE_UNAVAILABLE, nothing attempted; CONTROL: with it back, the delete goes ahead`, async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { engine, manager, cookie } = await ownerOfTarget(); + const fault = failAdmissionRead(engine, c.method, c.object, c.frame); + const attempted = recordAttemptedWrites(engine); + + const res = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); + engine.unregisterHooksByPackage(RECORDER); + fault.restore(); + + expect(fault.fired(), 'PREMISE: the admission\'s read really failed').toBe(1); + await expectReadFault(res, c.object); + // better-auth's handler never ran: not even its first write, the + // session's active organization, was attempted. + await expectNothingChanged(engine, attempted); + + await expectControlDeletes(engine, manager, cookie); + }); + } + + it('a member row the vendor\'s own predicate throws on still falls through: the vendor answers, before its first write', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { engine, manager, cookie } = await ownerOfTarget(null); + const [member] = await rows(engine, 'sys_member', { organization_id: TARGET }); + expect(member.role, 'PREMISE: the stored role is no string').toBeNull(); + const attempted = recordAttemptedWrites(engine); + + const res = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); + engine.unregisterHooksByPackage(RECORDER); + + const text = await res.clone().text(); + expect(res.status, text).toBeGreaterThanOrEqual(400); + // Not the admission's read fault: no read failed, and the vendor decided. + expect(text).not.toContain('SERVICE_UNAVAILABLE'); + await expectNothingChanged(engine, attempted); + }); +}); From 46c61dade04072f4d53aaf56ae0e0ee1200ff145 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 06:27:00 +0000 Subject: [PATCH 3/4] test(plugin-auth): say plainly why the null-role fixture is written beneath the engine Claude-Session: https://claude.ai/code/session_01UnHcTADjeg3VwxF21QHj7W Co-authored-by: Claude --- .../plugin-auth/src/organization-delete-read-fault.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts index bcda33cdec2..d8720b3f5bc 100644 --- a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts +++ b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts @@ -126,8 +126,9 @@ async function ownerOfTarget(role: string | null = 'owner') { const active = await post(manager, '/organization/set-active', { organizationId: TARGET }, cookie); expect(active.status, await active.clone().text()).toBe(200); if (role !== 'owner') { - // Below the engine, which would apply the field's default to a null: the - // row as a store can hold it. + // Written beneath the engine, so the row is exactly what a store can hold + // (`role` is not a required column), whatever the engine's write path does + // with a null. await (engine as any).getDriver('sys_member').update('sys_member', 'mem_owner', { role }); } return { engine, manager, cookie }; From 5884148db582551b305da826933d2e2ed099da27 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 08:09:26 +0000 Subject: [PATCH 4/4] fix(plugin-auth): the organization-delete admission refuses a failed session read too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveActor answered a failed session read with null, the value no session gives, so the admission fell through and better-auth deleted with nothing counted. An opt-in failedReadThrows mode, used only by the admission, lets the explicit token read's failure propagate, which the admission answers with the same typed 503. No session, an unknown or expired credential, one that does not parse and one carrying a control character still answer null; the other nine callers are unchanged. The changeset's Clause-② line now carries no reasoning. Claude-Session: https://claude.ai/code/session_01UnHcTADjeg3VwxF21QHj7W Co-authored-by: Claude --- .../25059-org-delete-read-fault-refuses.md | 6 +- .../plugins/plugin-auth/src/auth-manager.ts | 40 ++++-- .../organization-delete-read-fault.test.ts | 115 +++++++++++++----- 3 files changed, 122 insertions(+), 39 deletions(-) diff --git a/.changeset/25059-org-delete-read-fault-refuses.md b/.changeset/25059-org-delete-read-fault-refuses.md index 089ca2208b9..1559514a36b 100644 --- a/.changeset/25059-org-delete-read-fault-refuses.md +++ b/.changeset/25059-org-delete-read-fault-refuses.md @@ -4,12 +4,14 @@ fix(plugin-auth): an organization delete whose admission cannot read what it decides on is refused with 503 `SERVICE_UNAVAILABLE`, before better-auth's first write -Clause-②: no (the refusal replaces an outcome only a failed store read produces, and the typed 503 replaces a bare 500 fault; no request a caller can make is answered differently) +Clause-②: no Metadata-protocol: none +The refusal replaces an outcome only a failed store read produces, and the typed 503 replaces a bare 500 fault; no request a caller can make is answered differently. + The `POST /organization/delete` admission counts the records an organization still owns before better-auth's handler runs, so a refused delete writes nothing on any driver. -- **A read the admission decides on that fails now refuses.** When its read of the caller's membership or of the organization row failed, the admission handed the request to better-auth, which could then clear the caller's active organization and delete the members and invitations with nothing counted. On a driver without transactions those deletes stayed while the organization row survived. The admission now answers `503` with the standard `SERVICE_UNAVAILABLE` code, before better-auth's first write. The body names the table it could not read; the driver's own message is not relayed. +- **A read the admission decides on that fails now refuses.** When its read of the caller's session, of the caller's membership or of the organization row failed, the admission handed the request to better-auth, which could then clear the caller's active organization and delete the members and invitations with nothing counted. On a driver without transactions those deletes stayed while the organization row survived. The admission now answers `503` with the standard `SERVICE_UNAVAILABLE` code, before better-auth's first write. The body names the table it could not read; the driver's own message is not relayed. A failed session read used to answer the same as no session at all; the two are now told apart, and only no session still goes to better-auth's `401`. - **A failed Default Organization decision answers the same `503`.** It already refused before any write, but as a bare error, which better-auth answers with an untyped `500`. - **Unchanged.** What better-auth answers itself still goes to it: no session, no membership, no permission, no organization row, and a member row whose role the vendor's own permission check cannot evaluate (its identical check runs before its first write). The `409 DELETE_RESTRICTED` refusal and the Default Organization's `403` are unchanged. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index f3015ed6f7b..ab76fa3c90e 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -8150,9 +8150,18 @@ export class AuthManager { * part) — the bearer plugin may convert `Authorization: Bearer` to a session * AFTER this global before-hook runs. Returns `null` when no valid session * can be resolved (→ caller lets `sessionMiddleware` issue the 401). + * + * [#25059] By default a session read that FAILED answers `null` too, the + * same value as no session. `failedReadThrows` tells the two apart for a + * caller whose fall-through would let writes run uncounted: when no actor + * was resolved and the explicit token read itself threw, that throw + * propagates. A request with no credential, a credential the store does not + * hold, an expired session, a credential that does not parse or one + * carrying a control character still answers `null`. */ private async resolveActor( ctx: any, + options?: { failedReadThrows?: boolean }, ): Promise<{ userId: string; activeOrgId?: string } | null> { try { const { getSessionFromCtx } = await import('better-auth/api'); @@ -8166,6 +8175,7 @@ export class AuthManager { }; } } catch { /* fall through to explicit token resolution */ } + let failedRead: { cause: unknown } | undefined; try { const hdr = (k: string): string => ((ctx?.headers?.get?.(k) ?? ctx?.request?.headers?.get?.(k)) as string) || ''; @@ -8185,10 +8195,20 @@ export class AuthManager { if (cm?.[1]) token = unsigned(decodeURIComponent(cm[1])); } if (token) { - const sess: any = await (ctx as any).context.adapter.findOne({ - model: 'session', - where: [{ field: 'token', value: token }], - }); + let sess: any; + try { + sess = await (ctx as any).context.adapter.findOne({ + model: 'session', + where: [{ field: 'token', value: token }], + }); + } catch (cause) { + // A token carrying a control character is none the store holds, and + // a read of it can fail on the request's own bytes (PostgreSQL + // refuses U+0000 in a text parameter): that failure stays no session. + const storable = !Array.from(token).some((c) => c.charCodeAt(0) < 0x20 || c.charCodeAt(0) === 0x7f); + if (options?.failedReadThrows && storable) failedRead = { cause }; + throw cause; + } const exp = sess?.expiresAt ?? sess?.expires_at; if (sess && (!exp || new Date(exp).getTime() > Date.now())) { const userId = String(sess.userId ?? sess.user_id ?? ''); @@ -8202,6 +8222,7 @@ export class AuthManager { } } } catch { /* unresolved → null */ } + if (failedRead) throw failedRead.cause; return null; } @@ -8319,7 +8340,8 @@ export class AuthManager { * refusal, and no count. What the vendor itself answers falls through to it: * no session, no organization id, no membership, no permission, no * organization row. A READ this decides on that fails does not fall - * through [#25059]: the vendor's own reads could then succeed and its writes + * through [#25059], the session read included (`resolveActor` with + * `failedReadThrows`): the vendor's own reads could then succeed and its writes * run with nothing counted, so it refuses here with 503 * `SERVICE_UNAVAILABLE` ({@link organizationDeleteReadFaultBody}), and so * does a failure deciding whether the target is the Default Organization. @@ -8350,10 +8372,10 @@ export class AuthManager { } }; - // `resolveActor` never throws: a session read that fails answers null, as - // no session does, so that one reaches the vendor, whose own session read - // decides it. - const actor = await this.resolveActor(ctx); + // The session read refuses on failure too, so a session the store could + // not read is never taken for no session; no session at all still falls + // through. + const actor = await read('sys_session', () => this.resolveActor(ctx, { failedReadThrows: true })); if (!actor?.userId) return; // the vendor answers 401 const callerRow: any = await read('sys_member', () => sys.findOne('sys_member', { where: { organization_id: organizationId, user_id: actor.userId } }), diff --git a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts index d8720b3f5bc..50092ba344f 100644 --- a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts +++ b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts @@ -17,20 +17,26 @@ * Pinned here, each with its CONTROL (the same delete, the read back, goes * ahead): * + * - the caller's session read fails: 503, nothing attempted. `resolveActor` + * answers a failed session read with `null` by default, the value no + * session gives; the admission asks it with `failedReadThrows`; * - the caller's `sys_member` read fails: 503, nothing attempted; * - the `sys_organization` read fails: 503, nothing attempted; * - the Default Organization decision's count fails: 503, nothing attempted. * - * And the case that keeps falling through: the vendor's own `hasPermission` - * reads nothing here, so it throws only on the member row itself, and the - * vendor's identical call throws first too, before its first write. + * And the cases that keep falling through: no session at all, and a + * credential carrying a control character even when its read fails (both the + * vendor's 401); and the vendor's own `hasPermission`, which reads nothing + * here, so it throws only on the member row itself, and the vendor's identical + * call throws first too, before its first write. * * Real stack: `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3, the * objects from this plugin's manifest and a real `AuthManager` with the * organization plugin (the `session-read-failure-answer.test.ts` harness). - * Each fault is scoped to the admission: the read throws once, and only when - * the admission itself makes it, so better-auth's own reads of the same table - * are the real ones. That is what lets a request that falls through reach + * Each fault is scoped to the admission: the read throws only when the + * admission itself makes it (the session read: only inside the admission's + * `resolveActor` call), so better-auth's own reads of the same table are the + * real ones. That is what lets a request that falls through reach * better-auth's writes, which the engine hooks below record whether or not the * request's transaction later rolls them back. */ @@ -159,26 +165,41 @@ function deepStack(): string { } } +interface AdmissionRead { + /** The engine methods the read goes through. */ + methods: ReadonlyArray<'find' | 'findOne' | 'count'>; + object: string; + /** A frame the call must be made inside of. */ + frame?: string; + /** The first `AuthManager` method on the stack: the one that made the call. */ + caller?: string; + /** How many of those calls throw; the session read makes two (both of `resolveActor`'s paths). */ + times?: number; +} + /** - * The admission's own `method` call on `object` throws, once: the call must be - * made from inside `frame` and by the admission itself (its first - * `AuthManager` frame is `assertOrganizationDeletable`, so a session read made - * inside `resolveActor` does not qualify). Every other call is the real one. + * The admission's own read of `object` throws: the call must be made from + * inside `assertOrganizationDeletable` (and `frame`), and by `caller` (by + * default the admission itself, so a session read made inside `resolveActor` + * does not qualify unless asked for). Every other call is the real one. */ -function failAdmissionRead(engine: ObjectQL, method: 'findOne' | 'count', object: string, frame = 'assertOrganizationDeletable') { +function failAdmissionRead(engine: ObjectQL, read: AdmissionRead) { + const { methods, object, frame = 'assertOrganizationDeletable', caller = 'assertOrganizationDeletable', times = 1 } = read; let fired = 0; const target = engine as unknown as Record Promise>; - const real = target[method].bind(engine); - const spy = vi.spyOn(target, method).mockImplementation(async (name: string, ...rest: unknown[]) => { - const stack = deepStack(); - const caller = /AuthManager\.(\w+)/.exec(stack)?.[1]; - if (fired === 0 && name === object && stack.includes(frame) && caller === 'assertOrganizationDeletable') { - fired += 1; - throw new Error(`${object} store unavailable (injected)`); - } - return real(name, ...rest); + const spies = methods.map((method) => { + const real = target[method].bind(engine); + return vi.spyOn(target, method).mockImplementation(async (name: string, ...rest: unknown[]) => { + const stack = deepStack(); + const first = /AuthManager\.(\w+)/.exec(stack)?.[1]; + if (fired < times && name === object && stack.includes('assertOrganizationDeletable') && stack.includes(frame) && first === caller) { + fired += 1; + throw new Error(`${object} store unavailable (injected)`); + } + return real(name, ...rest); + }); }); - return { fired: () => fired, restore: () => spy.mockRestore() }; + return { fired: () => fired, restore: () => spies.forEach((spy) => spy.mockRestore()) }; } /** The refusal, asserted on its envelope: 503 with the standard code, naming the table it could not read. */ @@ -210,25 +231,34 @@ async function expectControlDeletes(engine: ObjectQL, manager: AuthManager, cook } describe('[#25059] an organization-delete read that fails refuses 503 before better-auth\'s first write', () => { - const cases: Array<{ name: string; method: 'findOne' | 'count'; object: string; frame?: string }> = [ - { name: 'the caller\'s sys_member read', method: 'findOne', object: 'sys_member' }, - { name: 'the sys_organization read', method: 'findOne', object: 'sys_organization' }, - { name: 'the Default Organization decision\'s count', method: 'count', object: 'sys_organization', frame: 'isDefaultOrganizationTarget' }, + const cases: Array<{ name: string; read: AdmissionRead; fires: number }> = [ + { + name: 'the caller\'s session read', + read: { methods: ['find', 'findOne'], object: 'sys_session', caller: 'resolveActor', times: Number.POSITIVE_INFINITY }, + fires: 2, + }, + { name: 'the caller\'s sys_member read', read: { methods: ['findOne'], object: 'sys_member' }, fires: 1 }, + { name: 'the sys_organization read', read: { methods: ['findOne'], object: 'sys_organization' }, fires: 1 }, + { + name: 'the Default Organization decision\'s count', + read: { methods: ['count'], object: 'sys_organization', frame: 'isDefaultOrganizationTarget' }, + fires: 1, + }, ]; for (const c of cases) { it(`${c.name} fails: 503 SERVICE_UNAVAILABLE, nothing attempted; CONTROL: with it back, the delete goes ahead`, async () => { vi.spyOn(console, 'error').mockImplementation(() => {}); const { engine, manager, cookie } = await ownerOfTarget(); - const fault = failAdmissionRead(engine, c.method, c.object, c.frame); + const fault = failAdmissionRead(engine, c.read); const attempted = recordAttemptedWrites(engine); const res = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); engine.unregisterHooksByPackage(RECORDER); fault.restore(); - expect(fault.fired(), 'PREMISE: the admission\'s read really failed').toBe(1); - await expectReadFault(res, c.object); + expect(fault.fired(), 'PREMISE: the admission\'s read really failed').toBe(c.fires); + await expectReadFault(res, c.read.object); // better-auth's handler never ran: not even its first write, the // session's active organization, was attempted. await expectNothingChanged(engine, attempted); @@ -237,6 +267,35 @@ describe('[#25059] an organization-delete read that fails refuses 503 before bet }); } + it('no session at all still falls through: the vendor\'s 401, nothing attempted', async () => { + const { engine, manager } = await ownerOfTarget(); + const attempted = recordAttemptedWrites(engine); + + const res = await post(manager, '/organization/delete', { organizationId: TARGET }, ''); + engine.unregisterHooksByPackage(RECORDER); + + expect(res.status, await res.clone().text()).toBe(401); + await expectNothingChanged(engine, attempted); + }); + + it('a credential carrying a control character is no session, even when its read fails: the vendor\'s 401, nothing attempted', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { engine, manager } = await ownerOfTarget(); + const fault = failAdmissionRead(engine, { + methods: ['find', 'findOne'], object: 'sys_session', caller: 'resolveActor', times: Number.POSITIVE_INFINITY, + }); + const attempted = recordAttemptedWrites(engine); + + // `%00` decodes to U+0000, a token no store holds and one PostgreSQL refuses to bind. + const res = await post(manager, '/organization/delete', { organizationId: TARGET }, 'better-auth.session_token=%00forged.sig'); + engine.unregisterHooksByPackage(RECORDER); + fault.restore(); + + expect(fault.fired(), 'PREMISE: the admission read the session for that token, and the read failed').toBeGreaterThan(0); + expect(res.status, await res.clone().text()).toBe(401); + await expectNothingChanged(engine, attempted); + }); + it('a member row the vendor\'s own predicate throws on still falls through: the vendor answers, before its first write', async () => { vi.spyOn(console, 'error').mockImplementation(() => {}); const { engine, manager, cookie } = await ownerOfTarget(null);