Skip to content

fix(plugin-auth): /admin/revoke-user-session refuses (404 RESOURCE_NOT_FOUND) when the token identifies no session - #10109

Merged
os-warren merged 4 commits into
mainfrom
claude/issue-10069-admin-revoke-user-session-match-guard
Aug 20, 2026
Merged

fix(plugin-auth): /admin/revoke-user-session refuses (404 RESOURCE_NOT_FOUND) when the token identifies no session#10109
os-warren merged 4 commits into
mainfrom
claude/issue-10069-admin-revoke-user-session-match-guard

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#10069

The measurement first (the card's premise, re-established behaviourally)

The card's reading was from the installed dist only; this PR re-measured it end to end before changing anything. On the installed vendor better-auth 1.7.1 (read from this branch's node_modules, not from the card), through the real pipeline (AuthManager.handleRequest → better-auth → ObjectQL adapter, network-free), POST /admin/revoke-user-session:

Premise valid; the fix proceeded.

The fix — a hooks.before admission gate, NOT the sibling's predicate

admin-revoke-user-session-match-guard.ts + wiring in auth-manager.ts, same seam as the /revoke-session guard from PR #10070, because an after-hook can replace a body but not a status.

The sibling route skips on an ownership mismatch, so its guard reproduces an ownership predicate. This route deletes blindly and the caller is an admin acting on arbitrary users — there is no ownership dimension. The admission predicate here is only "does any session carry this token", asked through the vendor's own internalAdapter.findSession. When no session does, the guard refuses 404 RESOURCE_NOT_FOUND (ADR-0112: code AND status), for the three reasons recorded in the sibling's header (res.ok callers, DELETE-like semantics, standard-catalog member over a synonym extension).

Permission is graded before existence

The guard runs ahead of the vendor's adminMiddleware + hasPermission. Refusing 404 on zero-match without grading the caller first 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 first asks the vendor's own permission question — the caller's session via getAuthoritativeSessionFromCtx (the exact call adminMiddleware makes), then the vendor's hasPermission logic with the vendor's own inputs: the live options object retained on the mounted admin plugin and the vendor's exported defaultRoles as fallback. Any caller the vendor would refuse falls through to the vendor's own 401/403 untouched — pinned byte-identically for missing and live tokens alike, so nothing below the permission line can distinguish them. This also leaves the recorded #9482 surface unchanged (a platform admin without the legacy role scalar still gets the vendor 403).

hasPermission itself is not exported by the vendor, so its ten lines are reproduced with the live options and the exported role map as inputs; both drift directions are pinned (a mirror gone loose answers this guard's 404 where the vendor's 403 belongs — integration test; a mirror gone strict is refused at unit level by the custom-roles pin).

The existence-oracle question, re-decided rather than inherited

The sibling (#9714) made zero-match and foreign-token answers byte-identical because its caller is an arbitrary user. Here the refusal deliberately reveals "no session carries this token" — but only to callers who pass the vendor's own session: ["revoke"] check. That caller class is already entitled to session-existence knowledge: the same default admin role grants session: ["list"] (/admin/list-user-sessions over arbitrary users), so the 404 tells an entitled admin nothing they cannot already query. 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" refuses.

Tombstone interaction (issue #7732), re-verified behaviourally

This route carries reason admin in INTERACTIVE_REVOKE_REASON. hideRevokedSessionRow makes a tombstoned row invisible to findSession, so revoking an already-revoked token now answers 404 — consistent with the tombstone module's "a revoked session is not a session" doctrine and with the sibling guard; previously it was a silent false success (measured above). The admitted path still tombstones with reason admin, and the refused second revoke leaves the tombstone untouched (pinned).

Scope

  • The singular route only. /admin/revoke-user-sessions (plural) matches by user id and cannot mis-identify a single record — left untouched, same reasoning as the sibling's plural routes.
  • Two parity-scanner test ledgers (better-auth-schema-parity.test.ts, managed-extension-fields.test.ts) gained a defaultRoles skip entry — it is a role map, not a plugin factory; it declares no schema. Same shape as their existing hasPermission entry.
  • Repo-wide pin-sweep for assertions of the old { success: true } shape on this route: zero found; counter-check — the same search located the route's ledger row, tombstone map entry, dogfood refusal probe, and checklist prose, so the net demonstrably works. The dogfood non-admin refusal suite (admin-route-nonadmin-refusal.dogfood.test.ts) probes this route only below the permission line, which this guard leaves byte-identical.

Verification (all at b293081c2)

  • pnpm --filter @objectstack/plugin-auth testTest Files 60 passed (60) · Tests 1335 passed (1335).
  • New suite: 15 tests (7 pipeline + 8 unit) covering zero-match, admitted revoke, already-revoked, empty-string token, non-admin byte-identical 403s, anonymous 401, non-string 400 fall-through, and the permission-mirror and admission predicates.
  • Ablation (guard neutered by an early return, prediction stated first): predicted exactly the three refusal pins failing "expected 404, received 200"; observed 3 failed | 12 passed, each AssertionError: expected 200 to be 404. Restored from the commit and proven byte-identical (git hash-object = 8cffac5b… both sides). No rebuild was needed for the mutation to be observable — the suite imports ./auth-manager in-package from src/ under vitest's transform; no dist is on the resolution path.
  • pnpm check:type-check-debt — "33 ledger entr(ies) re-measured … none above its recorded number"; plugin-auth stays at its frozen count (109), not raised.
  • Gate union from node scripts/pm/dispatch-gates.mjs (re-derived after the ledger commit, which added check:cross-package-test-inputs): check:changeset-gate-self-tests, check:objectui-changeset, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check:cross-package-test-inputs, check:query-options-erasure, check:type-check-coverage, check:engine-double-contract ("316 (file, verb) row(s) held by the RETAINED ledger"), check:where-matcher, check:nul-bytes, check:error-code-casing, check-adr-0087-registration ("this PR adds no declared-breaking changeset"), check-changeset-no-major, check-empty-changeset, check-affected-docs — all exit 0 at b293081c2.

Changeset: patch for @objectstack/plugin-auth, stating the new answer plainly for a release-notes reader.


Generated by Claude Code

…entifies no record
better-auth 1.7.1's handler deletes blindly and answers 200 { success: true }
unconditionally; a hooks.before admission gate now answers 404
RESOURCE_NOT_FOUND (ADR-0112) when no session carries the supplied token,
after grading the vendor's own permission question first so non-admin callers
keep the vendor's 401/403 unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

github-actionsBot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth, touching 9 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/kernel/contracts/auth-service.mdx(via AuthManager (symbol))
  • content/docs/kernel/services-checklist.mdx(via AuthManager (symbol))
  • content/docs/permissions/authentication.mdx(via AuthManager (symbol))
What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e61ee68320da9136fedbfe2609b0d21f97b587b4packageMentionDocs.

Which tree this was computed on

This run read content/docs from a250f283b51ba24467526aae407550c7e658d276 — the merge of head 9884c8ba64fb89b44f66de820e142ebfe4403e24 into base e61ee68320da9136fedbfe2609b0d21f97b587b4, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin a250f283b51ba24467526aae407550c7e658d276 && git checkout a250f283b51ba24467526aae407550c7e658d276
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e61ee68320da9136fedbfe2609b0d21f97b587b4 9884c8ba64fb89b44f66de820e142ebfe4403e24 && git checkout -B drift-repro e61ee68320da9136fedbfe2609b0d21f97b587b4 && git merge --no-ff 9884c8ba64fb89b44f66de820e142ebfe4403e24
node scripts/docs-audit/affected-docs.mjs --json e61ee68320da9136fedbfe2609b0d21f97b587b4

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e61ee68320da9136fedbfe2609b0d21f97b587b4 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-warren
os-warren marked this pull request as ready for review August 20, 2026 07:15
@os-warren
os-warren added this pull request to the merge queueAug 20, 2026
Merged via the queue into main with commit b5f6b26Aug 20, 2026
38 of 40 checks passed
@os-warren
os-warren deleted the claude/issue-10069-admin-revoke-user-session-match-guard branch August 20, 2026 07:40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-warren@claude