Skip to content

Commit 03bdd14

Browse files
os-warrenclaude
andauthored
fix(plugin-auth): run the break-glass last-local-credential guard after authentication (#11038)
* test(plugin-auth): pin the break-glass guard's authentication order at the real seam Lands the hermetic cases before the fix, per the triage execution order on #10776: both anonymous arms, the still-refused leg for an authenticated admin, the admission direction that keeps it non-vacuous, and the self-service path whose timing moves with the guard. Measured on this tree (pre-fix), through AuthManager.handleRequest on the installed better-auth 1.7.1: - the anonymous arm naming the break-glass holder answers 409 LAST_LOCAL_CREDENTIAL, and the same request naming an ordinary user answers 401 UNAUTHENTICATED -- the card's second arm was a reading, and the reading holds - an authenticated admin still gets 409 for the genuine last credential and 200 for an ordinary user - the self-service path shows the same split Part of #10776 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * fix(plugin-auth): run the break-glass guard after authentication, not before it The last-local-credential guard is a better-auth `hooks.before`, which runs ahead of the endpoint's own `use: [adminMiddleware]` -- the only layer that establishes identity on that lane. It therefore read the request body, asked the database a question about the named user, and answered it, for a caller nobody had authenticated. Its refusal is distinctive, and every sibling route on the same lane answers 401, so the refusal itself was a per-record answer. Resolve the acting user first and run the guard only for a caller who has an identity; an unauthenticated caller falls through to the vendor's own session middleware and gets the ordinary refusal, the same shape as every neighbour. Same pattern the /oauth2/authorize gate above already uses. This changes WHEN the guard decides, not WHAT it decides: an authenticated caller reaches the same lookup and the same CONFLICT. The still-refused leg and the admission direction are both pinned so that closing the disclosure by deleting the guard cannot pass. Maintainer ruling 2026-08-22, decision-inbox digest, accepted verbatim 「接受所有」: option A, authentication before the guard. Part of #10776 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * chore(changeset): break-glass guard runs after authentication Part of #10776 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a037f7c commit 03bdd14

4 files changed

Lines changed: 424 additions & 32 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
'@objectstack/plugin-auth': patch
3+
---
4+
5+
Run the break-glass last-local-credential guard after authentication
6+
7+
The guard that refuses removal of the last local-password login was registered
8+
as a better-auth `before` hook, which runs ahead of the endpoint middleware that
9+
establishes identity. It therefore decided — and answered — a question about a
10+
named user for a caller who had not been authenticated, while every neighbouring
11+
route on the same lane answers with the ordinary "please log in" refusal.
12+
13+
The guard now runs only once the acting user is resolved. An unauthenticated
14+
caller falls through to the ordinary refusal and learns nothing about the named
15+
user. For an authenticated caller nothing changes: the same lookup runs and the
16+
same `LAST_LOCAL_CREDENTIAL` conflict is returned, so the lockout protection is
17+
unaffected.

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

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1643,22 +1643,49 @@ export class AuthManager {
16431643
// the mount is conditional on the admin plugin, and because the
16441644
// guard itself now lives in ONE module both call sites share —
16451645
// `last-local-credential.ts`, whose header records this trap.
1646+
// ── [#10776] AUTHENTICATE FIRST ────────────────────────────
1647+
// A `hooks.before` runs AHEAD of the endpoint's own
1648+
// `use: [adminMiddleware]`, and that middleware is the only layer
1649+
// establishing identity on this lane. So this guard used to read an
1650+
// unauthenticated request's body, ask the database a question about
1651+
// the named user, and answer it — a per-record answer to a caller
1652+
// nobody had authenticated, where every sibling `/admin/` route
1653+
// answers 401. Resolving the actor first makes the guard's decision
1654+
// (and its distinctive refusal) reachable only once the caller has
1655+
// an identity; an anonymous caller falls through to the vendor's own
1656+
// `sessionMiddleware`/`adminMiddleware` and hears the ordinary 401.
1657+
//
1658+
// Maintainer ruling 2026-08-22 (decision-inbox digest, accepted
1659+
// verbatim 「接受所有」): option A, authentication before the guard.
1660+
//
1661+
// ⚠️ This changes WHEN the guard decides, never WHAT it decides:
1662+
// an authenticated caller reaches exactly the same lookup and the
1663+
// same `CONFLICT`. `break-glass-guard-authentication-order.test.ts`
1664+
// pins both halves — the disclosure closing AND the invariant
1665+
// surviving — because closing the first by deleting the guard would
1666+
// satisfy a disclosure-only suite.
1667+
//
1668+
// Same shape as the `/oauth2/authorize` gate above: unauthenticated
1669+
// → fall through, never a refusal invented here.
1670+
const breakGlassActor = await this.resolveActor(ctx);
1671+
16461672
let isLastLocalCredential = false;
1647-
try {
1648-
let targetId: string | undefined = ctx?.body?.userId ?? ctx?.body?.user_id;
1649-
if (!targetId && ctx.path === '/delete-user') {
1650-
const { getSessionFromCtx } = await import('better-auth/api');
1651-
const s: any = await getSessionFromCtx(ctx as any).catch(() => null);
1652-
targetId = s?.user?.id ?? s?.session?.userId;
1653-
}
1654-
if (targetId) {
1655-
isLastLocalCredential = await isLastLocalCredentialHolder(
1656-
ctx.context.adapter,
1657-
targetId,
1658-
);
1673+
if (breakGlassActor?.userId) {
1674+
try {
1675+
// `/delete-user` names no target in the vendor's own contract:
1676+
// the subject IS the authenticated caller, which is now already
1677+
// resolved rather than looked up a second time.
1678+
let targetId: string | undefined = ctx?.body?.userId ?? ctx?.body?.user_id;
1679+
if (!targetId && ctx.path === '/delete-user') targetId = breakGlassActor.userId;
1680+
if (targetId) {
1681+
isLastLocalCredential = await isLastLocalCredentialHolder(
1682+
ctx.context.adapter,
1683+
targetId,
1684+
);
1685+
}
1686+
} catch {
1687+
// Fail-open — never block a legitimate op on a lookup error.
16591688
}
1660-
} catch {
1661-
// Fail-open — never block a legitimate op on a lookup error.
16621689
}
16631690
if (isLastLocalCredential) {
16641691
const { APIError } = await import('better-auth/api');
Lines changed: 290 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,290 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// #10776 — the break-glass last-local-credential guard must run AFTER identity
4+
// is established, not before it.
5+
//
6+
// The guard is registered as a better-auth `hooks.before` keyed on `ctx.path`.
7+
// A `before` hook runs ahead of the endpoint's own `use: [adminMiddleware]`,
8+
// which is the only layer that establishes identity on that lane. The guard
9+
// therefore decided — and answered — a per-record question for a caller nobody
10+
// had authenticated. Maintainer ruling 2026-08-22 (decision-inbox digest,
11+
// accepted verbatim 「接受所有」): **Option A, authentication before the
12+
// guard**, so an unauthenticated caller hears only the ordinary refusal that
13+
// every other route on this lane gives. Option B (keep the guard early and
14+
// disguise its answer) was the fallback and is not taken; option C (accept the
15+
// disclosure) is not taken.
16+
//
17+
// ── Why this file drives the REAL seam ──────────────────────────────────────
18+
//
19+
// `break-glass-local-credential.test.ts` drives the before-hook directly with a
20+
// synthetic `ctx`. That is the right shape for the guard's own predicate, and
21+
// it is structurally blind to the defect this file pins: hook ORDER relative to
22+
// endpoint middleware does not exist in a synthetic call. So every assertion
23+
// here goes through `AuthManager.handleRequest` on the installed better-auth
24+
// 1.7.1, where the vendor's own middleware really runs, and reads a status and
25+
// a code off a real `Response`.
26+
//
27+
// ── The load-bearing half ───────────────────────────────────────────────────
28+
//
29+
// ⛔ An implementation that simply DELETED the guard would satisfy every
30+
// disclosure assertion below while destroying the protection the guard exists
31+
// for. Two describe-blocks exist to make that impossible to pass vacuously:
32+
// the still-refused leg (an AUTHENTICATED admin removing the genuine last
33+
// local credential still gets 409 `LAST_LOCAL_CREDENTIAL`) and the admission
34+
// leg (the same admin removing an ordinary user still succeeds, so the
35+
// still-refused leg cannot be satisfied by refusing everyone).
36+
//
37+
// ADR-0112 is `code` AND `status`; every refusal assertion below carries both.
38+
39+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
40+
import { AuthManager } from './auth-manager';
41+
import { createMemoryEngine } from './impersonation-bearer-rotation.test';
42+
import { LAST_LOCAL_CREDENTIAL_CODE } from './last-local-credential';
43+
44+
const SECRET = 'test-secret-at-least-32-chars-long!!';
45+
const PASSWORD = 'S3cure!Passw0rd-10776';
46+
const BASE = 'http://localhost:3000/api/v1/auth';
47+
48+
const makeManager = (engine: any) =>
49+
new AuthManager({
50+
secret: SECRET,
51+
baseUrl: 'http://localhost:3000',
52+
dataEngine: engine,
53+
plugins: { admin: true },
54+
} as any);
55+
56+
const post = (manager: AuthManager, path: string, body: unknown, bearer?: string) =>
57+
manager.handleRequest(
58+
new Request(`${BASE}${path}`, {
59+
method: 'POST',
60+
headers: {
61+
'Content-Type': 'application/json',
62+
...(bearer ? { authorization: `Bearer ${bearer}` } : {}),
63+
},
64+
body: JSON.stringify(body),
65+
}),
66+
);
67+
68+
/** Status + whatever error code the body carries, in either envelope shape. */
69+
async function verdict(res: Response): Promise<{ status: number; code?: string; text: string }> {
70+
const text = await res.text();
71+
let code: string | undefined;
72+
try {
73+
const parsed = JSON.parse(text);
74+
// ObjectStack's ADR-0112 envelope nests it; better-auth's flat shape does not.
75+
code = parsed?.error?.code ?? parsed?.code;
76+
} catch {
77+
/* non-JSON body → no code */
78+
}
79+
return { status: res.status, code, text };
80+
}
81+
82+
/**
83+
* One deployment, staged to the exact posture the guard exists to protect:
84+
*
85+
* - `owner` holds the ONLY local-password (`credential`) account — the
86+
* break-glass escape hatch itself.
87+
* - `admin` is an IdP-managed platform admin holding NO local credential
88+
* (their credential row is removed after they sign in, which is what
89+
* enforced SSO looks like: a managed team with a live session and no
90+
* password). Their session keeps working, so they can act as the
91+
* authenticated caller.
92+
* - `ordinary` is a second credential-less managed user — the removable one,
93+
* so the admission direction is testable on the same fixture.
94+
*
95+
* `/admin/remove-user` is better-auth's own endpoint and still authorizes on
96+
* the legacy `role` scalar (only `/admin/impersonate-user` was re-pointed at
97+
* ObjectStack's predicate, see `auth-manager.ts`), so the scalar is what is
98+
* set here.
99+
*/
100+
async function seedDeployment() {
101+
const engine = createMemoryEngine();
102+
const manager = makeManager(engine);
103+
104+
for (const [email, name] of [
105+
['owner.10776@example.com', 'Break Glass Owner'],
106+
['admin.10776@example.com', 'Managed Admin'],
107+
['ordinary.10776@example.com', 'Ordinary User'],
108+
]) {
109+
const res = await post(manager, '/sign-up/email', { email, password: PASSWORD, name });
110+
expect(res.status, `sign-up ${email}: ${await res.clone().text()}`).toBe(200);
111+
}
112+
113+
const users = (engine.tables.get('sys_user') ?? []) as any[];
114+
const idFor = (email: string) => String(users.find((r) => r.email === email)!.id);
115+
const ownerId = idFor('owner.10776@example.com');
116+
const adminId = idFor('admin.10776@example.com');
117+
const ordinaryId = idFor('ordinary.10776@example.com');
118+
119+
// The vendor `/admin/` lane's own authorization scalar.
120+
users.find((r) => String(r.id) === adminId)!.role = 'admin';
121+
122+
const signIn = await post(manager, '/sign-in/email', {
123+
email: 'admin.10776@example.com',
124+
password: PASSWORD,
125+
});
126+
const bearer = signIn.headers.get('set-auth-token');
127+
expect(bearer, 'sign-in must mint a bearer or the authenticated legs prove nothing').toBeTruthy();
128+
129+
const ownerSignIn = await post(manager, '/sign-in/email', {
130+
email: 'owner.10776@example.com',
131+
password: PASSWORD,
132+
});
133+
const ownerBearer = ownerSignIn.headers.get('set-auth-token');
134+
expect(ownerBearer, 'the self-service leg needs the owner signed in').toBeTruthy();
135+
136+
// Strip the local password from everyone except `owner`, leaving exactly one
137+
// credential holder. This is the state the guard guards.
138+
const accounts = (engine.tables.get('sys_account') ?? []) as any[];
139+
engine.tables.set(
140+
'sys_account',
141+
accounts.filter(
142+
(r) => !(r.provider_id === 'credential' && String(r.user_id ?? '') !== ownerId),
143+
),
144+
);
145+
const remaining = (engine.tables.get('sys_account') ?? []).filter(
146+
(r: any) => r.provider_id === 'credential',
147+
);
148+
expect(
149+
remaining.map((r: any) => String(r.user_id)),
150+
'fixture invariant: `owner` must be the SOLE local-credential holder',
151+
).toEqual([ownerId]);
152+
153+
return { engine, manager, ownerId, adminId, ordinaryId, bearer: bearer!, ownerBearer: ownerBearer! };
154+
}
155+
156+
beforeEach(() => {
157+
vi.spyOn(console, 'warn').mockImplementation(() => {});
158+
vi.spyOn(console, 'error').mockImplementation(() => {});
159+
});
160+
afterEach(() => vi.restoreAllMocks());
161+
162+
// ───────────────────────────────────────────────────────────────────────────
163+
// The disclosure: an unauthenticated caller learns nothing per-record
164+
// ───────────────────────────────────────────────────────────────────────────
165+
166+
describe('#10776 — an anonymous caller gets the ordinary refusal, never a per-record answer', () => {
167+
it('arm 1: naming the break-glass holder answers 401 UNAUTHENTICATED, not the guard‘s 409', async () => {
168+
const { manager, ownerId } = await seedDeployment();
169+
170+
const v = await verdict(await post(manager, '/admin/remove-user', { userId: ownerId }));
171+
172+
// Status AND code (ADR-0112). The status alone was the whole defect: a 409
173+
// where every sibling route answers 401 IS the per-record answer.
174+
expect(v.status, v.text).toBe(401);
175+
expect(v.code, v.text).toBe('UNAUTHENTICATED');
176+
expect(v.code, 'the guard‘s code must not reach an unauthenticated caller').not.toBe(
177+
LAST_LOCAL_CREDENTIAL_CODE,
178+
);
179+
expect(v.status, 'the guard‘s status must not reach an unauthenticated caller').not.toBe(409);
180+
}, 60_000);
181+
182+
it('arm 2: naming an ordinary user answers 401 UNAUTHENTICATED — measured, not read off the source', async () => {
183+
// The card filed this arm as an unmeasured reading. It is measured here so
184+
// the before/after is a real comparison: if this arm did NOT already land
185+
// on 401, the disclosure would be wider than the card states.
186+
const { manager, ordinaryId } = await seedDeployment();
187+
188+
const v = await verdict(await post(manager, '/admin/remove-user', { userId: ordinaryId }));
189+
190+
expect(v.status, v.text).toBe(401);
191+
expect(v.code, v.text).toBe('UNAUTHENTICATED');
192+
}, 60_000);
193+
194+
it('arm 2b: a userId nobody holds answers the same 401 — no existence oracle either', async () => {
195+
const { manager } = await seedDeployment();
196+
197+
const v = await verdict(await post(manager, '/admin/remove-user', { userId: 'usr_no_such_user' }));
198+
199+
expect(v.status, v.text).toBe(401);
200+
expect(v.code, v.text).toBe('UNAUTHENTICATED');
201+
}, 60_000);
202+
203+
it('the two arms are INDISTINGUISHABLE to the anonymous caller', async () => {
204+
// The substance of the card asserted directly rather than inferred from two
205+
// assertions that merely happen to agree today: whatever the platform says,
206+
// it must say the SAME thing for the break-glass holder and for anyone else.
207+
const { manager, ownerId, ordinaryId } = await seedDeployment();
208+
209+
const holder = await verdict(await post(manager, '/admin/remove-user', { userId: ownerId }));
210+
const other = await verdict(await post(manager, '/admin/remove-user', { userId: ordinaryId }));
211+
212+
expect(holder.status).toBe(other.status);
213+
expect(holder.text).toBe(other.text);
214+
}, 60_000);
215+
});
216+
217+
// ───────────────────────────────────────────────────────────────────────────
218+
// The load-bearing half: the invariant survives the move
219+
// ───────────────────────────────────────────────────────────────────────────
220+
221+
describe('#10776 — the break-glass invariant is unchanged for an AUTHENTICATED admin', () => {
222+
it('still-refused: removing the genuine last local credential is still 409 LAST_LOCAL_CREDENTIAL', async () => {
223+
// ⛔ The leg that fails on an implementation that "fixed" the disclosure by
224+
// deleting the guard. Everything in the block above stays green there.
225+
const { manager, ownerId, bearer } = await seedDeployment();
226+
227+
const v = await verdict(
228+
await post(manager, '/admin/remove-user', { userId: ownerId }, bearer),
229+
);
230+
231+
expect(v.status, v.text).toBe(409);
232+
expect(v.code, v.text).toBe(LAST_LOCAL_CREDENTIAL_CODE);
233+
}, 60_000);
234+
235+
it('admission: the same admin removing an ordinary user still succeeds', async () => {
236+
// Without this, the still-refused leg above is satisfiable by refusing
237+
// every caller — the failure mode this lane has already paid for twice.
238+
const { manager, ordinaryId, bearer } = await seedDeployment();
239+
240+
const res = await post(manager, '/admin/remove-user', { userId: ordinaryId }, bearer);
241+
const v = await verdict(res);
242+
243+
expect(v.status, v.text).toBe(200);
244+
expect(v.code, v.text).not.toBe(LAST_LOCAL_CREDENTIAL_CODE);
245+
}, 60_000);
246+
});
247+
248+
// ───────────────────────────────────────────────────────────────────────────
249+
// The self-service path, whose TIMING moves with the guard
250+
// ───────────────────────────────────────────────────────────────────────────
251+
252+
describe('#10776 — /delete-user sits under the same guard and is covered here', () => {
253+
it('anonymous: no per-record answer, and specifically not the guard‘s 409', async () => {
254+
const { manager, ownerId } = await seedDeployment();
255+
256+
const v = await verdict(await post(manager, '/delete-user', { userId: ownerId }));
257+
258+
// Measured: the vendor's own session middleware refuses first, in
259+
// better-auth's flat envelope. That is deliberate and is NOT the #10349
260+
// envelope's business — `/delete-user` is not an `/admin/` path, and that
261+
// card's normalizer is scoped to `/admin/` on purpose. What matters here is
262+
// that the answer is an AUTHENTICATION refusal and carries nothing about
263+
// the named user.
264+
expect(v.status, v.text).toBe(401);
265+
expect(v.code, v.text).toBe('UNAUTHORIZED');
266+
expect(v.code, v.text).not.toBe(LAST_LOCAL_CREDENTIAL_CODE);
267+
expect(v.status, v.text).not.toBe(409);
268+
}, 60_000);
269+
270+
it('anonymous: the self-service path is INDISTINGUISHABLE across the two arms too', async () => {
271+
const { manager, ownerId, ordinaryId } = await seedDeployment();
272+
273+
const holder = await verdict(await post(manager, '/delete-user', { userId: ownerId }));
274+
const other = await verdict(await post(manager, '/delete-user', { userId: ordinaryId }));
275+
276+
expect(holder.status).toBe(other.status);
277+
expect(holder.text).toBe(other.text);
278+
}, 60_000);
279+
280+
it('authenticated: the OUTCOME for the break-glass holder is unchanged — still 409', async () => {
281+
// Triage asked whether the ordering change alters this path's outcome or
282+
// only its timing. This is that question, pinned.
283+
const { manager, ownerId, ownerBearer } = await seedDeployment();
284+
285+
const v = await verdict(await post(manager, '/delete-user', { userId: ownerId }, ownerBearer));
286+
287+
expect(v.status, v.text).toBe(409);
288+
expect(v.code, v.text).toBe(LAST_LOCAL_CREDENTIAL_CODE);
289+
}, 60_000);
290+
});

0 commit comments

Comments
 (0)