Skip to content

Commit 5884148

Browse files
committed
fix(plugin-auth): the organization-delete admission refuses a failed session read too
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 <noreply@anthropic.com>
1 parent 46c61da commit 5884148

3 files changed

Lines changed: 122 additions & 39 deletions

File tree

‎.changeset/25059-org-delete-read-fault-refuses.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,14 @@
44

55
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
66

7-
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)
7+
Clause-②: no
88
Metadata-protocol: none
99

10+
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.
11+
1012
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.
1113

12-
- **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.
14+
- **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`.
1315
- **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`.
1416
- **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.
1517

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

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8150,9 +8150,18 @@ export class AuthManager {
81508150
* part) — the bearer plugin may convert `Authorization: Bearer` to a session
81518151
* AFTER this global before-hook runs. Returns `null` when no valid session
81528152
* can be resolved (→ caller lets `sessionMiddleware` issue the 401).
8153+
*
8154+
* [#25059] By default a session read that FAILED answers `null` too, the
8155+
* same value as no session. `failedReadThrows` tells the two apart for a
8156+
* caller whose fall-through would let writes run uncounted: when no actor
8157+
* was resolved and the explicit token read itself threw, that throw
8158+
* propagates. A request with no credential, a credential the store does not
8159+
* hold, an expired session, a credential that does not parse or one
8160+
* carrying a control character still answers `null`.
81538161
*/
81548162
private async resolveActor(
81558163
ctx: any,
8164+
options?: { failedReadThrows?: boolean },
81568165
): Promise<{ userId: string; activeOrgId?: string } | null> {
81578166
try {
81588167
const { getSessionFromCtx } = await import('better-auth/api');
@@ -8166,6 +8175,7 @@ export class AuthManager {
81668175
};
81678176
}
81688177
} catch { /* fall through to explicit token resolution */ }
8178+
let failedRead: { cause: unknown } | undefined;
81698179
try {
81708180
const hdr = (k: string): string =>
81718181
((ctx?.headers?.get?.(k) ?? ctx?.request?.headers?.get?.(k)) as string) || '';
@@ -8185,10 +8195,20 @@ export class AuthManager {
81858195
if (cm?.[1]) token = unsigned(decodeURIComponent(cm[1]));
81868196
}
81878197
if (token) {
8188-
const sess: any = await (ctx as any).context.adapter.findOne({
8189-
model: 'session',
8190-
where: [{ field: 'token', value: token }],
8191-
});
8198+
let sess: any;
8199+
try {
8200+
sess = await (ctx as any).context.adapter.findOne({
8201+
model: 'session',
8202+
where: [{ field: 'token', value: token }],
8203+
});
8204+
} catch (cause) {
8205+
// A token carrying a control character is none the store holds, and
8206+
// a read of it can fail on the request's own bytes (PostgreSQL
8207+
// refuses U+0000 in a text parameter): that failure stays no session.
8208+
const storable = !Array.from(token).some((c) => c.charCodeAt(0) < 0x20 || c.charCodeAt(0) === 0x7f);
8209+
if (options?.failedReadThrows && storable) failedRead = { cause };
8210+
throw cause;
8211+
}
81928212
const exp = sess?.expiresAt ?? sess?.expires_at;
81938213
if (sess && (!exp || new Date(exp).getTime() > Date.now())) {
81948214
const userId = String(sess.userId ?? sess.user_id ?? '');
@@ -8202,6 +8222,7 @@ export class AuthManager {
82028222
}
82038223
}
82048224
} catch { /* unresolved → null */ }
8225+
if (failedRead) throw failedRead.cause;
82058226
return null;
82068227
}
82078228

@@ -8319,7 +8340,8 @@ export class AuthManager {
83198340
* refusal, and no count. What the vendor itself answers falls through to it:
83208341
* no session, no organization id, no membership, no permission, no
83218342
* organization row. A READ this decides on that fails does not fall
8322-
* through [#25059]: the vendor's own reads could then succeed and its writes
8343+
* through [#25059], the session read included (`resolveActor` with
8344+
* `failedReadThrows`): the vendor's own reads could then succeed and its writes
83238345
* run with nothing counted, so it refuses here with 503
83248346
* `SERVICE_UNAVAILABLE` ({@link organizationDeleteReadFaultBody}), and so
83258347
* does a failure deciding whether the target is the Default Organization.
@@ -8350,10 +8372,10 @@ export class AuthManager {
83508372
}
83518373
};
83528374

8353-
// `resolveActor` never throws: a session read that fails answers null, as
8354-
// no session does, so that one reaches the vendor, whose own session read
8355-
// decides it.
8356-
const actor = await this.resolveActor(ctx);
8375+
// The session read refuses on failure too, so a session the store could
8376+
// not read is never taken for no session; no session at all still falls
8377+
// through.
8378+
const actor = await read('sys_session', () => this.resolveActor(ctx, { failedReadThrows: true }));
83578379
if (!actor?.userId) return; // the vendor answers 401
83588380
const callerRow: any = await read('sys_member', () =>
83598381
sys.findOne('sys_member', { where: { organization_id: organizationId, user_id: actor.userId } }),

‎packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts‎

Lines changed: 87 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,26 @@
1717
* Pinned here, each with its CONTROL (the same delete, the read back, goes
1818
* ahead):
1919
*
20+
* - the caller's session read fails: 503, nothing attempted. `resolveActor`
21+
* answers a failed session read with `null` by default, the value no
22+
* session gives; the admission asks it with `failedReadThrows`;
2023
* - the caller's `sys_member` read fails: 503, nothing attempted;
2124
* - the `sys_organization` read fails: 503, nothing attempted;
2225
* - the Default Organization decision's count fails: 503, nothing attempted.
2326
*
24-
* And the case that keeps falling through: the vendor's own `hasPermission`
25-
* reads nothing here, so it throws only on the member row itself, and the
26-
* vendor's identical call throws first too, before its first write.
27+
* And the cases that keep falling through: no session at all, and a
28+
* credential carrying a control character even when its read fails (both the
29+
* vendor's 401); and the vendor's own `hasPermission`, which reads nothing
30+
* here, so it throws only on the member row itself, and the vendor's identical
31+
* call throws first too, before its first write.
2732
*
2833
* Real stack: `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3, the
2934
* objects from this plugin's manifest and a real `AuthManager` with the
3035
* organization plugin (the `session-read-failure-answer.test.ts` harness).
31-
* Each fault is scoped to the admission: the read throws once, and only when
32-
* the admission itself makes it, so better-auth's own reads of the same table
33-
* are the real ones. That is what lets a request that falls through reach
36+
* Each fault is scoped to the admission: the read throws only when the
37+
* admission itself makes it (the session read: only inside the admission's
38+
* `resolveActor` call), so better-auth's own reads of the same table are the
39+
* real ones. That is what lets a request that falls through reach
3440
* better-auth's writes, which the engine hooks below record whether or not the
3541
* request's transaction later rolls them back.
3642
*/
@@ -159,26 +165,41 @@ function deepStack(): string {
159165
}
160166
}
161167

168+
interface AdmissionRead {
169+
/** The engine methods the read goes through. */
170+
methods: ReadonlyArray<'find' | 'findOne' | 'count'>;
171+
object: string;
172+
/** A frame the call must be made inside of. */
173+
frame?: string;
174+
/** The first `AuthManager` method on the stack: the one that made the call. */
175+
caller?: string;
176+
/** How many of those calls throw; the session read makes two (both of `resolveActor`'s paths). */
177+
times?: number;
178+
}
179+
162180
/**
163-
* The admission's own `method` call on `object` throws, once: the call must be
164-
* made from inside `frame` and by the admission itself (its first
165-
* `AuthManager` frame is `assertOrganizationDeletable`, so a session read made
166-
* inside `resolveActor` does not qualify). Every other call is the real one.
181+
* The admission's own read of `object` throws: the call must be made from
182+
* inside `assertOrganizationDeletable` (and `frame`), and by `caller` (by
183+
* default the admission itself, so a session read made inside `resolveActor`
184+
* does not qualify unless asked for). Every other call is the real one.
167185
*/
168-
function failAdmissionRead(engine: ObjectQL, method: 'findOne' | 'count', object: string, frame = 'assertOrganizationDeletable') {
186+
function failAdmissionRead(engine: ObjectQL, read: AdmissionRead) {
187+
const { methods, object, frame = 'assertOrganizationDeletable', caller = 'assertOrganizationDeletable', times = 1 } = read;
169188
let fired = 0;
170189
const target = engine as unknown as Record<string, (object: string, ...rest: unknown[]) => Promise<unknown>>;
171-
const real = target[method].bind(engine);
172-
const spy = vi.spyOn(target, method).mockImplementation(async (name: string, ...rest: unknown[]) => {
173-
const stack = deepStack();
174-
const caller = /AuthManager\.(\w+)/.exec(stack)?.[1];
175-
if (fired === 0 && name === object && stack.includes(frame) && caller === 'assertOrganizationDeletable') {
176-
fired += 1;
177-
throw new Error(`${object} store unavailable (injected)`);
178-
}
179-
return real(name, ...rest);
190+
const spies = methods.map((method) => {
191+
const real = target[method].bind(engine);
192+
return vi.spyOn(target, method).mockImplementation(async (name: string, ...rest: unknown[]) => {
193+
const stack = deepStack();
194+
const first = /AuthManager\.(\w+)/.exec(stack)?.[1];
195+
if (fired < times && name === object && stack.includes('assertOrganizationDeletable') && stack.includes(frame) && first === caller) {
196+
fired += 1;
197+
throw new Error(`${object} store unavailable (injected)`);
198+
}
199+
return real(name, ...rest);
200+
});
180201
});
181-
return { fired: () => fired, restore: () => spy.mockRestore() };
202+
return { fired: () => fired, restore: () => spies.forEach((spy) => spy.mockRestore()) };
182203
}
183204

184205
/** 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
210231
}
211232

212233
describe('[#25059] an organization-delete read that fails refuses 503 before better-auth\'s first write', () => {
213-
const cases: Array<{ name: string; method: 'findOne' | 'count'; object: string; frame?: string }> = [
214-
{ name: 'the caller\'s sys_member read', method: 'findOne', object: 'sys_member' },
215-
{ name: 'the sys_organization read', method: 'findOne', object: 'sys_organization' },
216-
{ name: 'the Default Organization decision\'s count', method: 'count', object: 'sys_organization', frame: 'isDefaultOrganizationTarget' },
234+
const cases: Array<{ name: string; read: AdmissionRead; fires: number }> = [
235+
{
236+
name: 'the caller\'s session read',
237+
read: { methods: ['find', 'findOne'], object: 'sys_session', caller: 'resolveActor', times: Number.POSITIVE_INFINITY },
238+
fires: 2,
239+
},
240+
{ name: 'the caller\'s sys_member read', read: { methods: ['findOne'], object: 'sys_member' }, fires: 1 },
241+
{ name: 'the sys_organization read', read: { methods: ['findOne'], object: 'sys_organization' }, fires: 1 },
242+
{
243+
name: 'the Default Organization decision\'s count',
244+
read: { methods: ['count'], object: 'sys_organization', frame: 'isDefaultOrganizationTarget' },
245+
fires: 1,
246+
},
217247
];
218248

219249
for (const c of cases) {
220250
it(`${c.name} fails: 503 SERVICE_UNAVAILABLE, nothing attempted; CONTROL: with it back, the delete goes ahead`, async () => {
221251
vi.spyOn(console, 'error').mockImplementation(() => {});
222252
const { engine, manager, cookie } = await ownerOfTarget();
223-
const fault = failAdmissionRead(engine, c.method, c.object, c.frame);
253+
const fault = failAdmissionRead(engine, c.read);
224254
const attempted = recordAttemptedWrites(engine);
225255

226256
const res = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie);
227257
engine.unregisterHooksByPackage(RECORDER);
228258
fault.restore();
229259

230-
expect(fault.fired(), 'PREMISE: the admission\'s read really failed').toBe(1);
231-
await expectReadFault(res, c.object);
260+
expect(fault.fired(), 'PREMISE: the admission\'s read really failed').toBe(c.fires);
261+
await expectReadFault(res, c.read.object);
232262
// better-auth's handler never ran: not even its first write, the
233263
// session's active organization, was attempted.
234264
await expectNothingChanged(engine, attempted);
@@ -237,6 +267,35 @@ describe('[#25059] an organization-delete read that fails refuses 503 before bet
237267
});
238268
}
239269

270+
it('no session at all still falls through: the vendor\'s 401, nothing attempted', async () => {
271+
const { engine, manager } = await ownerOfTarget();
272+
const attempted = recordAttemptedWrites(engine);
273+
274+
const res = await post(manager, '/organization/delete', { organizationId: TARGET }, '');
275+
engine.unregisterHooksByPackage(RECORDER);
276+
277+
expect(res.status, await res.clone().text()).toBe(401);
278+
await expectNothingChanged(engine, attempted);
279+
});
280+
281+
it('a credential carrying a control character is no session, even when its read fails: the vendor\'s 401, nothing attempted', async () => {
282+
vi.spyOn(console, 'error').mockImplementation(() => {});
283+
const { engine, manager } = await ownerOfTarget();
284+
const fault = failAdmissionRead(engine, {
285+
methods: ['find', 'findOne'], object: 'sys_session', caller: 'resolveActor', times: Number.POSITIVE_INFINITY,
286+
});
287+
const attempted = recordAttemptedWrites(engine);
288+
289+
// `%00` decodes to U+0000, a token no store holds and one PostgreSQL refuses to bind.
290+
const res = await post(manager, '/organization/delete', { organizationId: TARGET }, 'better-auth.session_token=%00forged.sig');
291+
engine.unregisterHooksByPackage(RECORDER);
292+
fault.restore();
293+
294+
expect(fault.fired(), 'PREMISE: the admission read the session for that token, and the read failed').toBeGreaterThan(0);
295+
expect(res.status, await res.clone().text()).toBe(401);
296+
await expectNothingChanged(engine, attempted);
297+
});
298+
240299
it('a member row the vendor\'s own predicate throws on still falls through: the vendor answers, before its first write', async () => {
241300
vi.spyOn(console, 'error').mockImplementation(() => {});
242301
const { engine, manager, cookie } = await ownerOfTarget(null);

0 commit comments

Comments
 (0)