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
5 changes: 5 additions & 0 deletions .changeset/spotty-planes-repeat.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@objectstack/plugin-auth': patch
---

`POST /api/v1/auth/admin/revoke-user-session` no longer reports success when it revoked nothing. When the supplied `sessionToken` does not identify any live session — including a session that was already revoked — the endpoint now answers `404` with error code `RESOURCE_NOT_FOUND` (ADR-0112 envelope) instead of `200 { "success": true }` over a delete that removed no record. The refusal is only ever given to callers who pass the admin plugin's own `session: ["revoke"]` permission check; unauthenticated and unauthorized callers keep the previous `401`/`403` answers byte-for-byte, so no session-existence information is exposed below the permission line. A revoke that does identify a live session still answers `200 { "success": true }` and tombstones the session with reason `admin`, unchanged.

Large diffs are not rendered by default.

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.

/**
* [#10069] `POST /admin/revoke-user-session` — a revoke that identifies NO
* record must not report success.
*
* ## The defect, and where it is minted
*
* Not here: the wrong answer comes out of the pinned vendor. better-auth
* `1.7.1` (the installed line, re-read for this card),
* `dist/plugins/admin/routes.mjs`, `revokeUserSession` runs, after its
* `session: ["revoke"]` permission check:
*
* ```js
* await ctx.context.internalAdapter.deleteSession(ctx.body.sessionToken);
* return ctx.json({ success: true });
* ```
*
* There is no match check at all — `deleteSession` on a token matching zero
* rows deletes nothing and the endpoint answers `200 { success: true }`
* unconditionally. Measured behaviourally on this repo's real pipeline
* (AuthManager.handleRequest → better-auth 1.7.1 → ObjectQL adapter) before
* the guard existed: a zero-match token answered `200 {"success":true}`, and
* an ALREADY-REVOKED (tombstoned, #7732) token answered `200 {"success":true}`
* as well. Same "security control no-ops while reporting success" class as the
* `/revoke-session` guard (`revoke-session-match-guard.ts`), on the surface
* where a false success matters most: an administrator revoking someone
* else's session and being told it worked.
*
* ## NOT the sibling's predicate
*
* `/revoke-session` skips on an ownership mismatch, so its guard reproduces an
* ownership predicate ("a session of YOURS carries this token"). This admin
* route deletes blindly — the caller is an admin acting on arbitrary users, so
* there is no ownership dimension. The admission predicate here is simply:
* **does any session carry this token** ({@link anySessionCarriesToken}),
* asked through the same `ctx.context.internalAdapter.findSession` seam the
* sibling uses. Copying the sibling's ownership predicate would wrongly narrow
* an admin's legitimate reach to their own sessions.
*
* ## Permission is graded BEFORE existence — the vendor's own question first
*
* The guard runs in the global `hooks.before`, AHEAD of the vendor's
* `adminMiddleware` and its `hasPermission` check. Refusing 404 on a
* zero-match token without first grading the caller would hand every
* authenticated NON-admin an existence oracle the vendor never gave them
* (guard-404 for a missing token vs vendor-403 for a live one). So the guard
* asks the vendor's own permission question first, with the vendor's own
* inputs, and falls through (to the vendor's 401/403) for any caller the
* vendor would refuse:
*
* - the caller's session is resolved via `getAuthoritativeSessionFromCtx` —
* the exact call the vendor's `adminMiddleware` makes (authoritative on
* purpose: never grade a role off the cookie cache);
* - the permission predicate ({@link adminMayRevokeUserSessions}) is
* better-auth's `has-permission.mjs` `hasPermission`, reproduced line for
* line because the vendor does not export it — but asked with the LIVE
* admin-plugin options read off `ctx.context.options.plugins` (the object
* the vendor itself retains) and with the vendor's own exported
* `defaultRoles` from `better-auth/plugins/admin/access` as the fallback,
* so `adminUserIds` / `defaultRole` / custom `roles` configured on the
* plugin are honoured without a second source of truth. (This repo's
* composition passes none of the three — `auth-manager.ts` constructs
* `admin({ schema })` only — and the integration tests pin both drift
* directions: a mirror gone loose answers this guard's 404 where the
* vendor's 403 belongs, a mirror gone strict resurfaces the vendor's false
* success.)
*
* ## The refusal shape
*
* **404 `RESOURCE_NOT_FOUND`** (ADR-0112: code AND status), for the same three
* reasons recorded in `revoke-session-match-guard.ts`: `res.ok` callers,
* DELETE-like semantics on an unidentifiable resource, and the standard
* catalog member over a synonym extension.
*
* ## The existence-oracle question, RE-DECIDED for this surface
*
* The sibling made zero-match and foreign-token answers byte-identical to
* avoid an existence oracle, because its caller is an arbitrary user. Here the
* refusal is deliberately allowed to reveal "no session carries this token" —
* but ONLY to callers who pass the vendor's own `session: ["revoke"]`
* permission check. That caller class is already entitled to session
* existence knowledge: the same default `admin` role statement grants
* `session: ["list"]`, i.e. `/admin/list-user-sessions` over arbitrary users,
* so the 404 tells an entitled admin nothing they cannot already query
* directly. For everyone else the guard is silent by construction (permission
* graded first), so the unauthenticated and unauthorized surfaces keep the
* vendor's exact refusals (401 / 403) with no existence dimension. There is
* no foreign-vs-missing pair to collapse on this route — any live session is
* legitimately deletable by an entitled admin; only "no session at all" is
* refused.
*
* ## What the guard deliberately does NOT decide
*
* - **Unauthenticated / unresolvable callers fall through** — the vendor's
* `adminMiddleware` owns the 401.
* - **Callers its permission mirror refuses fall through** — the vendor's
* `hasPermission` owns the 403 (including the seeded platform admin whose
* `role` scalar is not `admin`, #9482 — this guard must not change that
* surface's recorded behaviour).
* - **A non-string `sessionToken` falls through** — the endpoint's zod body
* schema answers the 400.
* - **An adapter read failure falls through** — never convert "I could not
* look" into "it does not exist".
*
* ## Interaction with session tombstones (#7732)
*
* This route is in `INTERACTIVE_REVOKE_REASON` with reason `admin`, and
* `hideRevokedSessionRow` makes a tombstoned row invisible to
* `internalAdapter.findSession`. So revoking an ALREADY-REVOKED token now
* answers 404 — consistent with the tombstone module's doctrine ("a revoked
* session is not a session") and with the sibling guard; previously it was a
* silent `{ success: true }` no-op (measured, see above). The admitted path
* still tombstones with reason `admin`, untouched.
*
* ## Scope
*
* The SINGULAR route only. `/admin/revoke-user-sessions` (plural) matches by
* user id and cannot mis-identify a single record — zero sessions to sweep is
* genuinely "nothing to do", exactly the reason the sibling left its own
* plural routes untouched.
*/

/** The refusal's wire code — standard catalog (ADR-0112), see header. */
export const ADMIN_REVOKE_USER_SESSION_NOT_FOUND_CODE = 'RESOURCE_NOT_FOUND';

/**
* The refusal's message. Deliberately NOT the sibling's "of yours" wording —
* this route has no ownership dimension (see header).
*/
export const ADMIN_REVOKE_USER_SESSION_NOT_FOUND_MESSAGE =
'No session matches the supplied token.';

/**
* The permission the vendor's handler demands — `revokeUserSession` calls
* `hasPermission({ …, permissions: { session: ["revoke"] } })`.
*/
export const ADMIN_REVOKE_USER_SESSION_PERMISSION: Readonly<Record<string, readonly string[]>> = {
session: ['revoke'],
};

/** The vendor's `AccessControl` role shape, as much of it as the mirror reads. */
type AuthorizingRole = {
authorize?: (permissions: unknown) => { success?: boolean } | undefined;
};

/**
* better-auth's admin-plugin `hasPermission` (dist/plugins/admin/
* has-permission.mjs), reproduced because the vendor does not export it. The
* inputs are the vendor's own: `user` is the authoritative session's user,
* `adminOptions` is the LIVE options object retained on the mounted admin
* plugin, `fallbackRoles` is the vendor's exported `defaultRoles`. Anything
* unreadable grades as not-permitted, which only ever means "fall through to
* the vendor's own refusal" — never a refusal minted here.
*/
export function adminMayRevokeUserSessions(
user: unknown,
adminOptions: unknown,
fallbackRoles: Record<string, AuthorizingRole | undefined>,
): boolean {
const u = (user ?? {}) as { id?: unknown; role?: unknown };
const opts = (adminOptions ?? {}) as {
adminUserIds?: unknown;
defaultRole?: unknown;
roles?: unknown;
};
const userId = u.id == null ? '' : String(u.id);
if (
userId &&
Array.isArray(opts.adminUserIds) &&
opts.adminUserIds.some((x) => String(x) === userId)
) {
return true;
}
const roleSource =
(typeof u.role === 'string' && u.role) ||
(typeof opts.defaultRole === 'string' && opts.defaultRole) ||
'user';
const acRoles: Record<string, AuthorizingRole | undefined> =
opts.roles && typeof opts.roles === 'object'
? (opts.roles as Record<string, AuthorizingRole | undefined>)
: fallbackRoles;
for (const role of roleSource.split(',')) {
try {
if (acRoles[role]?.authorize?.(ADMIN_REVOKE_USER_SESSION_PERMISSION)?.success) return true;
} catch {
// an authorizer that throws grades as not-permitted — the vendor's own
// call would throw the same way one moment later, on its own path.
}
}
return false;
}

/**
* The admission predicate: does `found` (the `internalAdapter.findSession`
* result) name ANY session? Shape-tolerant like the sibling's — `findSession`
* answers `{ session, user }` or `null`, and anything without a readable
* `session` is "no match", which is also what the vendor's `deleteSession`
* would have made of it (deletes nothing).
*/
export function anySessionCarriesToken(found: unknown): boolean {
const session = (found as { session?: unknown } | null | undefined)?.session;
return session != null && typeof session === 'object';
}
83 changes: 83 additions & 0 deletions packages/plugins/plugin-auth/src/auth-manager.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,12 @@ import {
REVOKE_SESSION_NOT_FOUND_MESSAGE,
revokeTargetsCallerSession,
} from './revoke-session-match-guard.js';
import {
ADMIN_REVOKE_USER_SESSION_NOT_FOUND_CODE,
ADMIN_REVOKE_USER_SESSION_NOT_FOUND_MESSAGE,
adminMayRevokeUserSessions,
anySessionCarriesToken,
} from './admin-revoke-user-session-match-guard.js';
import {
reconcileMembership,
type MembershipPolicy,
Expand DownExpand Up@@ -1487,6 +1493,24 @@ export class AuthManager {
// fall through — the vendor still performs the revoke itself
}

// ── #10069: the ADMIN revoke that identifies NO record must not ──
// report success either. better-auth 1.7.1's `admin/revoke-user-
// session` handler calls `deleteSession(ctx.body.sessionToken)`
// blindly and answers `200 { success: true }` unconditionally —
// measured on this pipeline: zero-match AND already-revoked tokens
// both answered success. NOT the #9714 predicate: no ownership
// dimension here (the caller is an admin acting on arbitrary
// users) — the question is only "does any session carry this
// token", and it is asked AFTER the vendor's own permission
// question so non-admins keep the vendor's 401/403 and gain no
// existence oracle. Before-hook on purpose: an after-hook cannot
// change the status. `admin-revoke-user-session-match-guard.ts`
// carries the full reading.
if (ctx?.path === '/admin/revoke-user-session') {
await this.assertAdminRevokeUserSessionIdentifiesRecord(ctx);
// fall through — the vendor still performs the revoke itself
}

// ── ADR-0024: admin-gate self-service SSO provider registration ──
// `@better-auth/sso`'s POST /sso/register only checks org-admin when
// `body.organizationId` is present (index.mjs: `if (ctx.body
Expand DownExpand Up@@ -4512,6 +4536,65 @@ export class AuthManager {
}
}

/**
* [#10069] `/admin/revoke-user-session` admission gate — refuse (404,
* standard `RESOURCE_NOT_FOUND`) when the supplied token identifies no
* session at all, instead of letting the vendor's unconditional success
* line answer `{ success: true }` over a delete that dispatched nothing.
*
* NOT the #9714 predicate: this route has no ownership dimension — the
* caller is an admin acting on arbitrary users, so the question is only
* "does any session carry this token". And it is asked strictly AFTER the
* vendor's own permission question ({@link adminMayRevokeUserSessions},
* the vendor's `hasPermission` with the LIVE mounted-plugin options), so
* every caller the vendor would refuse falls through to the vendor's own
* 401/403 and never learns whether the token exists. Everything else the
* guard cannot decide falls through too: a non-string token (the vendor's
* zod body schema answers 400), an adapter read failure (never convert
* "could not look" into "does not exist"). Full reading:
* `admin-revoke-user-session-match-guard.ts`.
*/
private async assertAdminRevokeUserSessionIdentifiesRecord(ctx: any): Promise<void> {
const token = ctx?.body?.sessionToken;
if (typeof token !== 'string') return; // vendor's body schema answers 400

let admitted: boolean;
try {
// The vendor's own session resolution — the exact call adminMiddleware
// makes (authoritative: a role graded off the cookie cache could admit
// a demoted caller the vendor is about to refuse).
const { getAuthoritativeSessionFromCtx } = await import('better-auth/api');
const s: any = await getAuthoritativeSessionFromCtx(ctx).catch(() => null);
// No resolvable caller → the vendor's adminMiddleware issues the 401.
if (!s?.user?.id) return;

// The vendor's own permission question, with the vendor's own inputs:
// the LIVE admin-plugin options (the object the mounted plugin retains)
// and its exported defaultRoles as the fallback. A caller this refuses
// gets the vendor's 403 — never this guard's 404.
const adminOptions = ((ctx?.context?.options?.plugins ?? []) as any[]).find(
(p: any) => p?.id === 'admin',
)?.options;
const { defaultRoles } = await import('better-auth/plugins/admin/access');
if (!adminMayRevokeUserSessions(s.user, adminOptions, defaultRoles as any)) return;

const found = await ctx.context.internalAdapter.findSession(token);
admitted = anySessionCarriesToken(found);
} catch {
// A lookup that did not complete hands the request back to the vendor
// unchanged — an engine hiccup must not invent a "not found".
return;
}

if (!admitted) {
const { APIError } = await import('better-auth/api');
throw new APIError('NOT_FOUND', {
message: ADMIN_REVOKE_USER_SESSION_NOT_FOUND_MESSAGE,
code: ADMIN_REVOKE_USER_SESSION_NOT_FOUND_CODE,
});
}
}

/**
* [#3697] The issuer's own better-auth membership role in `orgId` — the
* input to the invitation role cap.
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -270,6 +270,18 @@ const AUTH_MANAGER_PLUGINS: Record<string, { construct: () => unknown } | { skip
hasPermission: {
skip: 'permission predicate exported by the organization plugin — declares no schema',
},
// [#10069] NOT a plugin factory either — same scanner shape as
// `hasPermission` above. `defaultRoles` is the admin plugin's exported
// role→AccessControl map (`better-auth/plugins/admin/access`); it declares no
// schema, so it contributes no model and no column for this gate to compare.
// `assertAdminRevokeUserSessionIdentifiesRecord` reads it so the
// admin-revoke-user-session gate asks the vendor's own permission question
// (its `hasPermission` fallback roles) rather than keeping a second spelling
// of it. The `stale` assertion below removes this entry's licence the moment
// that import goes away.
defaultRoles: {
skip: 'role→AccessControl map exported by the admin plugin — declares no schema',
},
};

/** The plugin set the auth manager actually assembles (`buildPluginList()`). */
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -453,6 +453,18 @@ const AUTH_MANAGER_PLUGINS: Record<string, { construct: () => unknown } | { skip
hasPermission: {
skip: 'permission predicate exported by the organization plugin — declares no schema',
},
// [#10069] NOT a plugin factory either — same scanner shape as
// `hasPermission` above. `defaultRoles` is the admin plugin's exported
// role→AccessControl map (`better-auth/plugins/admin/access`); it declares no
// schema, contributes no model and no column, so there is nothing here for
// the collision loop to compare. `assertAdminRevokeUserSessionIdentifiesRecord`
// reads it so the admin-revoke-user-session gate asks the vendor's own
// permission question (its `hasPermission` fallback roles) rather than keeping
// a second spelling of it. The `stale` assertion below removes this entry's
// licence the moment that import goes away.
defaultRoles: {
skip: 'role→AccessControl map exported by the admin plugin — declares no schema',
},
};

/** The plugin set the auth manager actually assembles (`buildPluginList()`). */
Expand Down
10 changes: 10 additions & 0 deletions scripts/engine-double-contract.pinned.json
Original file line numberDiff line numberDiff line change
Expand Up@@ -1026,6 +1026,16 @@
"verb": "update",
"pinned": 1
},
{
"file": "packages/plugins/plugin-auth/src/admin-revoke-user-session-match-guard.test.ts",
"verb": "delete",
"pinned": 1
},
{
"file": "packages/plugins/plugin-auth/src/admin-revoke-user-session-match-guard.test.ts",
"verb": "update",
"pinned": 1
},
{
"file": "packages/plugins/plugin-auth/src/auth-manager.jwt-eddsa-fallback.test.ts",
"verb": "delete",
Expand Down
Loading