Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .changeset/admin-remove-user-gate-ordering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
---
"@objectstack/plugin-auth": patch
---

fix(auth): authorize before the break-glass guard on `POST /api/v1/auth/admin/remove-user` (#11477)

The break-glass last-local-credential guard is registered as a global better-auth
`hooks.before`, which runs ahead of an endpoint's own middleware. On
`/admin/remove-user` — served directly by better-auth's router, whose
`adminMiddleware` establishes only a session, with the role decision landing
later inside the vendor's handler — that ordering let the guard's lookup and its
distinctive refusal be reached by any **authenticated** caller before either
authorization layer had run. Because that refusal is target-dependent, the
refusal itself carried a per-record fact about a user the caller was not
entitled to ask about.

`/admin/ban-user` already ran the same guard **after** authorization: #9652
shades that path with an ObjectStack raw mount whose platform-admin gate fires
first. One guard, two routes, opposite orders, and nothing asserting either.

`/admin/remove-user` now carries the same shading, converging the whole
`/admin/*` family on **authorization before the guard**. The mount reuses the
landed #9652 / #9653 pattern and introduces no new mechanism.

What changes is **when** the guard decides, never **what** it decides:

- an anonymous caller still gets `401 UNAUTHENTICATED`;
- an authenticated non-admin now gets `403 PERMISSION_DENIED` for every target,
so the guard is unreachable before authorization and its answer no longer
varies with the named user;
- a platform admin is unaffected in every respect — the mount **delegates** into
better-auth rather than re-implementing removal, so the path-keyed hook still
fires and the guard still refuses the removal of the last local password
login, and admission remains the vendor's own decision (#9969).

An ordering pin ships with the fix so the sequence is mechanically checkable
rather than re-argued: it asserts that one authenticated non-admin naming two
different targets receives **indistinguishable** responses, and — so the pin
cannot be satisfied by deleting the guard — that an admitted platform admin
still hits the guard's refusal, and still succeeds on an ordinary user.

Large diffs are not rendered by default.

13 changes: 13 additions & 0 deletions packages/plugins/plugin-auth/src/auth-manager.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1694,6 +1694,19 @@ export class AuthManager {
// the mount is conditional on the admin plugin, and because the
// guard itself now lives in ONE module both call sites share —
// `last-local-credential.ts`, whose header records this trap.
//
// ⚠️ `/admin/remove-user` IS ALSO SHADED NOW (#11477) — and it DOES
// still reach this hook, which is the opposite of the line above
// and is the point. Its mount only runs `gateAdmin` and then
// RE-DISPATCHES the request through `handleRequest`, so it re-enters
// better-auth's router and this hook fires exactly as before —
// just AFTER authorization instead of before it. That is the whole
// fix: an authenticated non-admin is now refused by the mount and
// never reaches the lookup below, while an admitted platform admin
// reaches the identical lookup and the identical `CONFLICT`.
// ⛔ Do not "reconcile" the two notes by deleting this path from
// the list — that would silently drop the guard on the one route
// that still depends on this hook to run it.
// ── [#10776] AUTHENTICATE FIRST ────────────────────────────
// A `hooks.before` runs AHEAD of the endpoint's own
// `use: [adminMiddleware]`, and that middleware is the only layer
Expand Down
67 changes: 67 additions & 0 deletions packages/plugins/plugin-auth/src/auth-plugin.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -2185,6 +2185,73 @@ export class AuthPlugin implements Plugin {
}
});

// ── #11477: /admin/remove-user — AUTHORIZATION BEFORE THE GUARD ──────
//
// The break-glass last-local-credential guard is a global
// `hooks.before` in auth-manager.ts keyed on `ctx.path`. A better-auth
// `before` hook runs ahead of the endpoint's own `use: [adminMiddleware]`
// — and on this route that middleware is only a SESSION check, with the
// role decision landing later still, inside the vendor's handler. So on
// the unshaded route the guard's lookup and its distinctive refusal were
// reached by any AUTHENTICATED caller, admin or not, before either
// authorization layer had run.
//
// MEASURED on the installed better-auth 1.7.1 before this mount existed,
// one authenticated non-admin, two targets: naming the break-glass
// holder answered `409 LAST_LOCAL_CREDENTIAL` while naming an ordinary
// user answered `403 YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS`. Two different
// answers to the same caller IS the finding — the refusal itself carried
// a per-record fact about a user the caller was never entitled to ask
// about. The same measurement on `/admin/ban-user` answered
// `403 PERMISSION_DENIED` for BOTH targets, because #9652 already shades
// that path and `gateAdmin` runs first there.
//
// Maintainer ruling 2026-08-25 (decision-inbox batch 5, accepted
// verbatim 「全部同意」): option A — give this route the same raw-mount
// shading `/admin/ban-user` has, converging the whole `/admin/*` family
// on authorization before the guard. No new mechanism.
//
// ⚠️ This mount DELEGATES; it does not re-implement. That is the whole
// difference from the ban/unban mounts above, and it is deliberate:
//
// • #9969 (closed `not_planned`) ruled that the consumer-less vendor
// routes — this one included — are NOT re-implemented; their 403 to
// a platform admin is a recorded, intended state. Re-implementing
// removal here would quietly overturn that ruling.
// • Delegating keeps the request inside better-auth's router, so the
// path-keyed `hooks.before` still fires and the guard KEEPS working.
// Shadowing normally DETACHES such hooks (the trap written up in
// last-local-credential.ts, which is why ban-user must re-run the
// guard by hand); re-dispatching through `handleRequest` is what
// avoids paying that cost twice. The `/admin/sso/*` bridges use this
// exact gate-then-delegate shape (#9653).
//
// ⇒ WHAT CHANGES is only WHEN the guard decides, never WHAT it decides:
// anonymous → 401 UNAUTHENTICATED (unchanged)
// authenticated member → 403 PERMISSION_DENIED for EVERY target — the
// guard is now unreachable before authorization
// platform admin → unchanged in every respect, including the
// vendor's own 403 (#9969) and the guard's 409
// when the target really is the last holder
//
// The vendor Response is returned VERBATIM so the delegated answer —
// status, body and the #10349 ADR-0112 envelope `handleRequest` applies
// to vendor `/admin/` refusals — is byte-identical to the unshaded route.
//
// Pinned by `admin-remove-user-gate-ordering.test.ts`, which fails if
// this mount is removed.
rawApp.post(`${basePath}/admin/remove-user`, async (c: any) => {
try {
const gated = await gateAdmin(c);
if (gated instanceof Response) return gated;
return await this.authManager!.handleRequest(c.req.raw);
} catch (error) {
const err = error instanceof Error ? error : new Error(String(error));
ctx.logger.error('[AuthPlugin] admin/remove-user failed', err);
return c.json({ success: false, error: { code: 'INTERNAL_ERROR', message: err.message } }, 500);
}
});

rawApp.post(`${basePath}/admin/set-user-password`, async (c: any) => {
try {
const actor = await gateAdmin(c);
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -151,7 +151,7 @@ function refusedByDesignFor(targetUserId: string): Record<string, RefusalSpec> {
code: 'YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS',
body: { userId: targetUserId },
ruledBy: '#9969 (closed not_planned)',
why: 'no ObjectStack consumer; re-implement on demand. It also still carries better-auth\'s break-glass last-local-credential before-hook precisely BECAUSE it is not shadowed by a raw mount',
why: 'no ObjectStack consumer; re-implement on demand — so ADMISSION is still better-auth\'s adminMiddleware on the legacy `role` scalar and this refusal is intended. ⚠️ #11477 DID shade the path with a raw mount (gateAdmin first, to stop the break-glass before-hook answering an authenticated non-admin ahead of authorization), but that mount DELEGATES through handleRequest instead of re-implementing removal — so the request re-enters better-auth\'s router, the break-glass before-hook still fires, and this vendor refusal is unchanged. A future mount that stopped delegating would break BOTH this row and the guard',
},
'POST /api/v1/auth/admin/revoke-user-session': {
code: 'YOU_ARE_NOT_ALLOWED_TO_REVOKE_USERS_SESSIONS',
Expand Down
109 changes: 105 additions & 4 deletions packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -75,7 +75,25 @@
// bridges while SSO is off). That is what proves the member's 403 is a
// gate verdict and not a payload the server rejects for everyone.
//
// `better-auth-gate` (9 routes) — refusal side only, DELIBERATELY. The
// `shaded-vendor-gate` (1 route: `remove-user`) — the two halves belong to
// DIFFERENT layers, which is why it is neither of its neighbours. #11477
// gave the route the raw-mount shading `ban-user` already had, so an
// ObjectStack gate answers the refusal (member 403 PERMISSION_DENIED, anon
// 401 UNAUTHENTICATED) — but the mount DELEGATES rather than
// re-implementing, so admission is still better-auth's `adminMiddleware` on
// the legacy `role` scalar and a platform admin is still refused
// `403 YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS` (#9969, closed `not_planned`:
// consumer-less vendor routes are not re-implemented).
//
// The both-sides contrast is therefore not a 2xx but a DIFFERENCE: the
// member and the admin hear two different refusals, which is what proves
// the member's 403 is an authorization verdict and not a blanket refusal.
// The bucket also carries the #11477 negative — a member must never again
// see the break-glass guard's `409 LAST_LOCAL_CREDENTIAL`, which before the
// shading was answered ahead of every authorization layer and VARIED WITH
// THE TARGET, disclosing per-record state to a caller entitled to none.
//
// `better-auth-gate` (8 routes) — refusal side only, DELIBERATELY. The
// ANONYMOUS half is now the full ADR-0112 pin, identical to the bucket
// above — `401` and `code: 'UNAUTHENTICATED'`. It was `[401, 403].includes(
// status)` until #10349, because the vendor's `adminMiddleware` answered an
Expand DownExpand Up@@ -113,8 +131,9 @@
// a session-scoped synthesis is discarded before the check runs. It moved
// the two routes `sys_user` actions call — `ban-user` / `unban-user` —
// onto ObjectStack mounts, where the allowed side IS pinned above. The
// nine below still answer the platform admin with the vendor's own
// `YOU_ARE_NOT_ALLOWED_*`; that is a known, filed gap, not drift.
// eight below — and `remove-user`, one bucket up — still answer the
// platform admin with the vendor's own `YOU_ARE_NOT_ALLOWED_*`; that is a
// known, filed state (#9969, closed `not_planned`), not drift.
//
// `self-scoped` (2 routes) — `has-permission` and `stop-impersonating` answer
// a non-admin without a refusal BY DESIGN, and the invariant is asserted in
Expand DownExpand Up@@ -164,6 +183,7 @@ const AUTH_BASE = '/api/v1/auth';
/** How a non-admin must be answered by one derived route. */
type Bucket =
| 'objectstack-gate'
| 'shaded-vendor-gate'
| 'better-auth-gate'
| 'self-scoped'
| 'not-mounted';
Expand DownExpand Up@@ -285,6 +305,36 @@ function expectationsFor(targetUserId: string): Record<string, RouteExpectation>
body: { userId: targetUserId },
},

// ── #11477 — shaded for ORDERING, still admitted by the vendor ─────────
//
// The only member of its bucket, and the bucket exists because this route
// genuinely has a third shape rather than because the other two did not
// fit. Its raw mount runs `gateAdmin` and then RE-DISPATCHES into
// better-auth instead of re-implementing removal, so the two halves are
// owned by different layers:
//
// refusal → ObjectStack's gate (403 PERMISSION_DENIED), because the
// mount answers first. That is #11477's whole point: the
// break-glass `hooks.before` guard used to answer an
// authenticated non-admin BEFORE any authorization ran, and
// its 409 differed per target — a per-record disclosure.
// admission → still better-auth's `adminMiddleware` on the legacy `role`
// scalar, so a platform admin is still refused
// 403 YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS. #9969 closed
// `not_planned`: consumer-less vendor routes are not
// re-implemented, and that refusal is intended.
//
// ⛔ Do not "simplify" this into either neighbouring bucket. In
// `objectstack-gate` the admin-is-not-refused assertion would be red (the
// vendor still refuses); in `better-auth-gate` the member's code assertion
// would be red (`PERMISSION_DENIED` is not `YOU_ARE_NOT_ALLOWED_*`).
// Forcing either one would mean weakening a live security assertion.
'POST /api/v1/auth/admin/remove-user': {
bucket: 'shaded-vendor-gate',
body: { userId: targetUserId },
note: '#11477 — ObjectStack gate answers the refusal, better-auth still owns admission (#9969)',
},

// ── better-auth admin plugin (legacy `role` scalar gate) ────────────────
//
// Still refusal-side only, and still for the reason in the header: the
Expand All@@ -293,7 +343,6 @@ function expectationsFor(targetUserId: string): Record<string, RouteExpectation>
// re-implementable); the rest stay on the vendor's gate pending the
// maintainer's call on the remaining surface.
'POST /api/v1/auth/admin/set-role': { bucket: 'better-auth-gate', body: { userId: targetUserId, role: 'admin' } },
'POST /api/v1/auth/admin/remove-user': { bucket: 'better-auth-gate', body: { userId: targetUserId } },
'POST /api/v1/auth/admin/impersonate-user': { bucket: 'better-auth-gate', body: { userId: targetUserId } },
'POST /api/v1/auth/admin/revoke-user-sessions': { bucket: 'better-auth-gate', body: { userId: targetUserId } },
'POST /api/v1/auth/admin/revoke-user-session': { bucket: 'better-auth-gate', body: { sessionToken: 'probe-session-token' } },
Expand DownExpand Up@@ -553,6 +602,50 @@ describe('#9482 C9: every derived /admin/ route refuses a non-admin', () => {
}
}, 600_000);

it('the shaded vendor route refuses a non-admin from the ObjectStack gate, before the break-glass guard', async () => {
// #11477. The both-sides contrast here is NOT a 2xx — it is that the two
// callers hear DIFFERENT refusals. A member is turned away by ObjectStack's
// gate (`PERMISSION_DENIED`) and a platform admin gets past it only to be
// turned away by the vendor's (`YOU_ARE_NOT_ALLOWED_*`, #9969). Two
// distinct codes on the same route and payload is what proves the member's
// 403 is an authorization verdict rather than a blanket refusal — the same
// job the 2xx does in the `objectstack-gate` bucket above.
const routes = derived.all.filter((r) => expectations[r]?.bucket === 'shaded-vendor-gate');
expect(routes.length, 'no shaded-vendor-gate routes were derived').toBeGreaterThan(0);

for (const route of routes) {
const anon = await fire(route, undefined);
expect(anon.status, `${route} anonymous: ${anon.body}`).toBe(401);
expect(anon.code, `${route} anonymous code: ${anon.body}`).toBe('UNAUTHENTICATED');

const member = await fire(route, memberToken);
expect(member.status, `${route} member: ${member.body}`).toBe(403);
expect(member.code, `${route} member code: ${member.body}`).toBe('PERMISSION_DENIED');

// ⛔ The load-bearing negative. Before #11477 the break-glass
// `hooks.before` guard answered an authenticated non-admin ahead of every
// authorization layer, and its answer varied with the TARGET — a
// per-record disclosure to a caller entitled to nothing. A member must
// never see the guard's verdict on this route again.
expect(member.code, `${route} member must not reach the break-glass guard`).not.toBe(
'LAST_LOCAL_CREDENTIAL',
);
expect(member.status, `${route} member must not reach the break-glass guard`).not.toBe(409);

const admin = await fire(route, adminToken);
expect(
admin.code,
`${route} platform admin: the vendor gate still owns admission (#9969), ` +
`so this must be the vendor's own code, got ${admin.status} ${admin.body}`,
).toMatch(/^YOU_ARE_NOT_ALLOWED/);
expect(
admin.code,
`${route} platform admin was refused by the OBJECTSTACK gate — the member's ` +
`403 above therefore proves nothing about authorization`,
).not.toBe('PERMISSION_DENIED');
}
}, 600_000);

it('the better-auth admin routes refuse a non-admin with a named vendor code', async () => {
const routes = derived.all.filter((r) => expectations[r]?.bucket === 'better-auth-gate');
expect(routes.length, 'no better-auth-gate routes were derived').toBeGreaterThan(0);
Expand DownExpand Up@@ -596,6 +689,14 @@ describe('#9482 C9: every derived /admin/ route refuses a non-admin', () => {
// transaction, so this route answers the authorization question like
// every other member of the bucket and needs no exception.
//
// ⚠️ #11477 moved `remove-user` OUT of this bucket entirely — its raw
// mount now answers a member from ObjectStack's gate
// (`403 PERMISSION_DENIED`) before better-auth is reached at all, so the
// vendor-vocabulary rule below no longer describes it. It lives in
// `shaded-vendor-gate`, which asserts that code exactly. This is the
// reverse of re-widening: the vocabulary here stayed narrow and the route
// that stopped matching it was reclassified.
//
// ⛔ Do not re-widen the vocabulary — for this route or for all of them.
// A route that answers `UNAUTHENTICATED` to a signed-in caller is
// announcing that authentication ran where authorization should have, and
Expand Down
Loading
Loading