Skip to content

Commit e5f194b

Browse files
fix(plugin-auth): an organization-delete read that fails refuses 503 before better-auth's first write (#25184)
Fixes #25059 Clause-②: no Metadata-protocol: none Why `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. ## What changed `AuthManager.assertOrganizationDeletable`, the `POST /organization/delete` admission in better-auth's global `hooks.before`: - **A failed session read refuses as well.** `resolveActor` answered a failed session read with `null`, the value "no session" gives, so the admission fell through. It takes an opt-in `failedReadThrows` option now. With it, when no actor was resolved and the explicit token read itself threw, that throw propagates, and the admission answers it with the same typed `503`. No credential, a credential the store does not hold, an expired session, a credential that does not parse, and one carrying a control character all still answer `null`, so they reach better-auth's `401`. Only the admission passes the option. The other nine `this.resolveActor(ctx)` call sites are byte-identical in the diff. - **A read it decides on that fails now refuses.** A failed read of the caller's `sys_member` row or of the `sys_organization` row used to land in `catch { return; }` and hand the request to better-auth, which re-reads, then clears the session's active organization, then deletes the members and invitations, and only then meets the engine's refusal at the organization row. It now answers `503` with the standard ADR-0112 code `SERVICE_UNAVAILABLE`, before better-auth's first write. - **A failed Default Organization decision answers the same typed `503`.** It already refused before any write, but with a bare `Error`, which better-auth serves as an untyped 500. - **The vendor-decided cases still fall through, unchanged:** no actor, no member row, not permitted, no organization row. - The wire pieces sit beside the 409's in `organization-delete.ts`: `ORGANIZATION_DELETE_READ_FAULT_CODE` and `organizationDeleteReadFaultBody(object)`. The body names the table it could not read. The driver's own error rides `cause` and is not relayed. Two names were added to the existing `organization-delete.js` import list in `auth-manager.ts`; nothing else outside the method moved. ## The four mechanism assumptions, re-measured on this branch 1. **The lookup block.** Measured: it holds four lookups, but `resolveActor` cannot throw. Both of its paths catch, and it answers `null`, the same value as "no session". So of the four, three can throw into the catch: the `sys_member` read, `hasPermission` and the `sys_organization` read. 2. **The Default Organization decision answers 500.** Measured by ablation A2 below: with this change reverted to main's bare propagation, the pin reads `expected 500 to be 503`. 3. **Which status and code.** `503 SERVICE_UNAVAILABLE`: the vocabulary this file already uses for a read fault (the organization slug guard, the invitation role cap and the invitation placement check), a `StandardErrorCode`, so the error-code ledger needs no row. `check:dispatcher-error-vocabulary` ran green (Verification below). 4. **Can `hasPermission` throw for a decidable vendor answer?** Yes, and only for that. better-auth 1.7.12 `plugins/organization/has-permission.mjs` reaches the adapter only behind `options.dynamicAccessControl?.enabled`, and the admission hands it the roles map alone, so the call reads nothing. It throws only on the member row itself: a role that is not a string (`role.split`), or a role name that resolves to an inherited object member (`authorize` is not a function). better-auth's `deleteOrganization` calls the identical predicate (the same roles object: `organizationOptions.roles` is the `customOrgRoles` the admission holds as `orgRolesMap`) on the same row, before `setActiveOrganization`. So that throw keeps falling through, and a pin holds that nothing is attempted when it does. ## The session read The card names `resolveActor` among the lookups that throw. Measured: it never threw. Both of its paths catch and answer `null`, so a failed session read read as "no actor" and fell through. An uncommitted scratch test failed only the admission's own session reads while better-auth's stayed real; it got `200`, and better-auth's session update and member and organization deletes ran with nothing counted. `resolveActor(ctx, { failedReadThrows: true })` now tells the two apart. It is used by the admission alone, and the default reading is unchanged for the other nine callers. Only the explicit token read counts as a failed read. Two things are deliberately not counted: - the first path's throw, when the second path finds no token; - a failure reading a token that carries a control character. PostgreSQL refuses U+0000 in a text parameter, and a crafted cookie can carry `%00`. Counting it would turn that request's `401` into a `503`. ## Pins `packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts`. Real `ObjectQL` over `@objectstack/driver-sql` and better-sqlite3, the manifest's objects, a real `AuthManager` with the organization plugin (the `session-read-failure-answer.test.ts` harness). - **One case per read that can fail:** the caller's session read (both of `resolveActor`'s paths fail inside the admission's call, fired 2), the caller's `sys_member` read, the `sys_organization` read, and the Default Organization decision's count. The admission's own read fails once, scoped to the admission's frame, so better-auth's reads of the same table stay real. Each case asserts `503`, `SERVICE_UNAVAILABLE` and the table named. It also asserts that no write was attempted: the session update and the member, invitation and organization deletes are recorded by engine hooks when attempted, not when committed. Then a CONTROL: with the read back, the same delete answers `200` and the organization is gone. - **What still falls through:** a request with no session gets better-auth's `401`, with nothing attempted. A credential carrying a control character gets the same `401`, with nothing attempted, even while the admission's read of it fails. - **The `hasPermission` fall-through:** a stored member row whose role is null reaches better-auth, which refuses before its first write. Nothing is attempted, and the answer is not the admission's `503`. ### Ablations A1 to A3 ran at `947ab6c25` (before the merge of `main`); A4 and A5 at `5884148db`, the final head (baseline there `Tests 7 passed (7)`). The test imports `./auth-manager.js` relatively, so the subject resolves to `src/` and no build sits between a mutation and its run. Each mutation was applied with `scripts/ablation-replace.mjs` (the anchor hit once, landed on disk, blob changed), and then restored. | | mutation | result | |---|---|---| | baseline | none | `Tests 4 passed (4)` | | A1 | the read helper swallows the fault (main's fall-through put back) | the three read pins red, each `expected 200 to be 503`: the request reached better-auth and deleted the organization | | A2 | the Default Organization decision propagates bare (main's R3) | its pin red, `expected 500 to be 503`; the other three green | | A3 | a `hasPermission` throw refuses `503` instead of falling through | the fall-through pin red, `expected '{"code":"SERVICE_UNAVAILABLE",…' not to contain 'SERVICE_UNAVAILABLE'` | | A4 | `resolveActor` swallows the failed session read again | the session-read pin red, `expected 200 to be 503`: the request reached better-auth and deleted the organization | | A5 | the control-character guard removed | the control-character pin red, `expected 503 to be 401` | Restore after each mutation: blob `f3015ed6f7b0` == HEAD, `git diff HEAD` empty, zero markers on disk, `git status --porcelain` empty. ## Verification - At the final head `5884148db`, under the lock: `pnpm --filter @objectstack/plugin-auth build` exit 0; `pnpm --filter @objectstack/plugin-auth exec vitest run` gave `Test Files 185 passed (185)`, `Tests 3485 passed | 10 skipped (3495)`; `pnpm --filter @objectstack/plugin-auth typecheck` exit 0 (`check:test-typecheck: OK`, no debt row added); `pnpm check:type-check-debt` exit 0 (`re-measure: OK — 1 ledger entr(ies) re-measured in 190.9s, 26 raw tsc error(s) total, none above its recorded number`). The lock printed `VERDICT command-exit 0`. - Gate union at `5884148db`: the 70 commands derived by `node scripts/pm/dispatch-gates.mjs --commands` (the same 70 at `947ab6c25`, `46c61dade` and `5884148db`), plus `check-changeset-fixed` and `check:authz-resolver`, whose rosters sit in a directory this diff touches. Every exit code was captured before any pipe, and all are 0. `--ran`: `70 derived · 70 run · 0 NOT-MEASURED · 0 UNRUN`. - The other nine callers: in the diff, the one changed `resolveActor(` line is the admission's; `git grep` counts nine `this.resolveActor(ctx)` call sites, and `failedReadThrows: true` occurs once (`auth-manager.ts:8378`, inside `assertOrganizationDeletable`). - Not run locally: the full-farm `check:*` sweep, `pnpm lint`, the dogfood battery (the title carries no `!` and there is no `(narrowing)` arm), and the `packages/qa` dogfood pins. CI runs those; every check-run on `5884148db` succeeded or was skipped. ## Acceptance notes - The #24948 dogfood pin for a failed Default Organization lookup (`packages/qa/dogfood/test/organization-delete-federated-fixture.dogfood.test.ts:282`) asserts a status of 400 or above. It stays green with `503`. It could be tightened to `503` plus `SERVICE_UNAVAILABLE`. That file is in the `domain:cli` lane and was not edited here. - The record count (`organizationDeleteRefusal` / `defaultOrganizationDeleteRefusal`) still propagates a failure bare, as an untyped 500. That is the same family as the Default Organization decision. It is not changed here because what reaches it is not only a store fault: an engine refusal carrying its own code lands there too, so a blanket `503` would mislabel it. - A client sees `503 SERVICE_UNAVAILABLE` from an organization delete only when a store read failed. Nothing was deleted then, and a retry is the remedy. --- _Generated by [Claude Code](https://claude.ai/code/session_01UnHcTADjeg3VwxF21QHj7W)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9c431ba commit e5f194b

4 files changed

Lines changed: 445 additions & 31 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/plugin-auth': patch
3+
---
4+
5+
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
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
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+
12+
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.
13+
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`.
15+
- **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`.
16+
- **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.
17+
18+
**What a client does.** Treat `503 SERVICE_UNAVAILABLE` from an organization delete as "retry later": nothing was deleted.

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

Lines changed: 80 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,11 @@ import {
211211
} from './signing-key-read-cache.js';
212212
import {
213213
ORGANIZATION_DELETE_PATH,
214+
ORGANIZATION_DELETE_READ_FAULT_CODE,
214215
ORGANIZATION_DELETE_REFUSAL_CODE,
215216
defaultOrganizationDeleteRefusal,
216217
isDefaultOrganizationTarget,
218+
organizationDeleteReadFaultBody,
217219
organizationDeleteRefusal,
218220
organizationNotEmptyBody,
219221
removeOrganizationAuthStructure,
@@ -8148,9 +8150,18 @@ export class AuthManager {
81488150
* part) — the bearer plugin may convert `Authorization: Bearer` to a session
81498151
* AFTER this global before-hook runs. Returns `null` when no valid session
81508152
* 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`.
81518161
*/
81528162
private async resolveActor(
81538163
ctx: any,
8164+
options?: { failedReadThrows?: boolean },
81548165
): Promise<{ userId: string; activeOrgId?: string } | null> {
81558166
try {
81568167
const { getSessionFromCtx } = await import('better-auth/api');
@@ -8164,6 +8175,7 @@ export class AuthManager {
81648175
};
81658176
}
81668177
} catch { /* fall through to explicit token resolution */ }
8178+
let failedRead: { cause: unknown } | undefined;
81678179
try {
81688180
const hdr = (k: string): string =>
81698181
((ctx?.headers?.get?.(k) ?? ctx?.request?.headers?.get?.(k)) as string) || '';
@@ -8183,10 +8195,20 @@ export class AuthManager {
81838195
if (cm?.[1]) token = unsigned(decodeURIComponent(cm[1]));
81848196
}
81858197
if (token) {
8186-
const sess: any = await (ctx as any).context.adapter.findOne({
8187-
model: 'session',
8188-
where: [{ field: 'token', value: token }],
8189-
});
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+
}
81908212
const exp = sess?.expiresAt ?? sess?.expires_at;
81918213
if (sess && (!exp || new Date(exp).getTime() > Date.now())) {
81928214
const userId = String(sess.userId ?? sess.user_id ?? '');
@@ -8200,6 +8222,7 @@ export class AuthManager {
82008222
}
82018223
}
82028224
} catch { /* unresolved → null */ }
8225+
if (failedRead) throw failedRead.cause;
82038226
return null;
82048227
}
82058228

@@ -8314,12 +8337,17 @@ export class AuthManager {
83148337
* Only for a caller the vendor itself admits — a member whose role holds
83158338
* `organization: ['delete']`, asked of the vendor's own `hasPermission`, as
83168339
* {@link assertRemoveMemberPermitted} does — so a caller it refuses gets its
8317-
* refusal, and no count. Everything this cannot decide falls through to the
8318-
* vendor: no session, no organization id, no membership, a lookup that
8319-
* failed. The COUNT does not fall through: it is the guard, and a guard that
8320-
* could not be evaluated must not pass, so its failure propagates, and so
8340+
* refusal, and no count. What the vendor itself answers falls through to it:
8341+
* no session, no organization id, no membership, no permission, no
8342+
* organization row. A READ this decides on that fails does not fall
8343+
* through [#25059], the session read included (`resolveActor` with
8344+
* `failedReadThrows`): the vendor's own reads could then succeed and its writes
8345+
* run with nothing counted, so it refuses here with 503
8346+
* `SERVICE_UNAVAILABLE` ({@link organizationDeleteReadFaultBody}), and so
83218347
* does a failure deciding whether the target is the Default Organization.
8322-
* The Default Organization is asked the last-administrator guard's question
8348+
* The COUNT does not fall through either: it is the guard, and a guard that
8349+
* could not be evaluated must not pass, so its failure propagates. The
8350+
* Default Organization is asked the last-administrator guard's question
83238351
* first (a 403), then its own record count (the same 409), both here, before
83248352
* the first write.
83258353
*/
@@ -8329,17 +8357,34 @@ export class AuthManager {
83298357
const organizationId = typeof ctx?.body?.organizationId === 'string' ? ctx.body.organizationId : '';
83308358
if (!organizationId) return;
83318359

8332-
let organization: any;
8360+
const sys = withSystemReadContext(engine);
8361+
const read = async <T>(object: string, run: () => Promise<T>): Promise<T> => {
8362+
try {
8363+
return await run();
8364+
} catch (cause) {
8365+
const { APIError } = await import('better-auth/api');
8366+
const err = new APIError('SERVICE_UNAVAILABLE', {
8367+
code: ORGANIZATION_DELETE_READ_FAULT_CODE,
8368+
...organizationDeleteReadFaultBody(object),
8369+
});
8370+
(err as { cause?: unknown }).cause = cause;
8371+
throw err;
8372+
}
8373+
};
8374+
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 }));
8379+
if (!actor?.userId) return; // the vendor answers 401
8380+
const callerRow: any = await read('sys_member', () =>
8381+
sys.findOne('sys_member', { where: { organization_id: organizationId, user_id: actor.userId } }),
8382+
);
8383+
if (!callerRow) return; // the vendor answers USER_IS_NOT_A_MEMBER_OF_THE_ORGANIZATION
8384+
let permitted: boolean;
83338385
try {
8334-
const actor = await this.resolveActor(ctx);
8335-
if (!actor?.userId) return; // the vendor answers 401
8336-
const sys = withSystemReadContext(engine);
8337-
const callerRow: any = await sys.findOne('sys_member', {
8338-
where: { organization_id: organizationId, user_id: actor.userId },
8339-
});
8340-
if (!callerRow) return; // the vendor answers USER_IS_NOT_A_MEMBER_OF_THE_ORGANIZATION
83418386
const { hasPermission } = await import('better-auth/plugins/organization');
8342-
const permitted = await hasPermission(
8387+
permitted = await hasPermission(
83438388
{
83448389
role: callerRow.role,
83458390
options: (this.orgRolesMap ? { roles: this.orgRolesMap } : {}) as any,
@@ -8348,21 +8393,26 @@ export class AuthManager {
83488393
} as any,
83498394
ctx,
83508395
);
8351-
if (!permitted) return; // the vendor answers YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_ORGANIZATION
8352-
organization = await sys.findOne('sys_organization', { where: { id: organizationId } });
8353-
if (!organization) return; // the vendor answers 400
83548396
} catch {
8355-
// A lookup that did not complete hands the request back to the vendor,
8356-
// which re-reads everything it decides; the engine's own refusal still
8357-
// stands under the delete.
8397+
// Not a read: handed a roles map and no dynamic access control, the
8398+
// vendor's predicate reads nothing, so it throws only on the member row
8399+
// itself (a role that is no string, or names an inherited object
8400+
// member), and the vendor's identical call on that row throws too,
8401+
// before its first write. The vendor answers this.
83588402
return;
83598403
}
8360-
8361-
// Which of the two refusals below applies is part of the guard, so it is
8362-
// decided outside the fall-through above: a lookup that fails here
8363-
// refuses, never hands the request to the vendor unasked.
8364-
const defaultOrganization = await isDefaultOrganizationTarget(
8365-
engine, organization, postureEnforcesWall(resolveTenancyPosture()),
8404+
if (!permitted) return; // the vendor answers YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_ORGANIZATION
8405+
const organization: any = await read('sys_organization', () =>
8406+
sys.findOne('sys_organization', { where: { id: organizationId } }),
8407+
);
8408+
if (!organization) return; // the vendor answers 400
8409+
8410+
// Which of the two refusals below applies is part of the guard, so a
8411+
// lookup that fails deciding it refuses too, never hands the request to
8412+
// the vendor unasked.
8413+
const walled = postureEnforcesWall(resolveTenancyPosture());
8414+
const defaultOrganization = await read('sys_organization', () =>
8415+
isDefaultOrganizationTarget(engine, organization, walled),
83668416
);
83678417

83688418
// The Default Organization is asked the last-administrator guard's

0 commit comments

Comments
 (0)