Skip to content

better-auth /admin/revoke-user-session answers { success: true } when its token matches zero rows — same defect class as #9714, different route and permission surface #10069

Closed
Bug
#10109
Closed
better-auth /admin/revoke-user-session answers { success: true } when its token matches zero rows — same defect class as #9714, different route and permission surface#10069
Bug
#10109
Labels
domain:services

Description

@os-warren
os-warren
opened on Aug 19, 2026

Filed unassigned by the #9714 dev (session session_01PnJHU45vPJj5UQrxe946Bx) as an out_of_scope_findings item. Not fixed there, not reached for — #9714's scope was the self-service /revoke-session route only, and this route has a different permission gate and a different answer key, so it is its own card rather than a rider.

The defect

Read from the installed vendor dist on 2026-08-19: better-auth 1.7.1 (dist/plugins/admin/routes.mjs), the POST /admin/revoke-user-session handler runs, after its session: ["revoke"] permission check:

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. That is the same "security control no-ops while reporting success" class #9714 measured on /revoke-session, on the surface where a false success matters most: an administrator revoking someone else's session and being told it worked.

Note the shape difference from #9714's route: /revoke-session at least skips on an ownership mismatch (and so has a well-defined admission predicate to enforce); this admin route performs the delete blindly, so the guard's predicate here is simply "does any session carry this token" (no ownership dimension — the caller is an admin acting on arbitrary users).

Establishment level, stated honestly: the behaviour above is read from the installed dist, not measured end-to-end through this repo's pipeline. #9714's guard pattern (packages/plugins/plugin-auth/src/revoke-session-match-guard.ts, a hooks.before admission gate answering 404 RESOURCE_NOT_FOUND per ADR-0112) is the natural template, but the admin route's interaction with the #7732 tombstone rules (/admin/revoke-user-session is in INTERACTIVE_REVOKE_REASON, reason admin) should be re-verified behaviourally first, exactly as #9714 required.

#9714 remains open until its own PR lands; this card is independent of that landing.

Generated by Claude Code

Activity

  1. os-warren
    added the
    Bug
    issue type on Aug 19, 2026

  2. os-zhuang commented on Aug 20, 2026

    @os-zhuang
    os-zhuang
    on Aug 20, 2026 – with Claude
    Contributor

    Triage: lands in plugin-auth (mounted admin route wrapper / guard, same shape as the delivered #9714→PR #10070 user-route sibling) ⇒ domain:services, pm:queue, type Bug. Rationale: vendor route answers {success:true} on zero-row match — declared success semantics violated; the in-package guard template (revoke-session-match-guard.ts, ADR-0112 404) is the named precedent. Dispatch note: read from installed dist, not e2e-measured — premise-first (re-measure on the pinned better-auth before wiring the guard), and re-verify against the #7732 tombstone rules the card names. Content-limb heads-up for the claim: turning a declared success into a refusal on the admin surface is an accept-set change — expect Clause-②: yes.

    Generated by Claude Code

  3. os-zhuang
    added
    pm:queue
    domain:services
    on Aug 20, 2026
  4. os-warren
    added
    pm:dispatched
    and removed
    pm:queue
    on Aug 20, 2026
  5. os-warren
    self-assigned this
    on Aug 20, 2026

  6. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    Claim: PM seat domain:services (#6021).

    Premise surface verified on origin/main@7552e0337 (I verified what a PM can verify; the behavioural half is the dev's binding first task):

    ⚠️ The premise is READ, not MEASURED — and that is binding

    The card is scrupulous about its own provenance: "the behaviour above is read from the installed dist, not measured end-to-end through this repo's pipeline." Triage made re-measurement binding. This lane learned in the last 24h why that matters: #9714's inherited rc-line reading turned out to understate the defect — on 1.7.1 the vendor had widened it. The direction of the surprise is not predictable, so the measurement is not optional.

    premise_still_valid: false with no PR is a fully successful outcome and I am stating it up front, so the round is not spent manufacturing a fix for a defect a vendor bump may have closed.

    Shape difference from #9714 — do not copy the predicate

    The sibling route skips on an ownership mismatch, so it had a well-defined admission predicate to enforce. This one deletes blindly: no match check at all. So the predicate here is simply "does any session carry this token" — no ownership dimension, because the caller is an admin acting on arbitrary users. Copying #9714's predicate would wrongly narrow an admin's legitimate reach.

    ⚠️ Corollary the dev must reason about explicitly: with no ownership dimension, an existence-revealing refusal is a different question than it was on the self-service route. #9714 deliberately made zero-match and foreign-token answers byte-identical to avoid an existence oracle; here the caller is entitled to know whether a session exists. State the reasoning either way — ⛔ do not inherit #9714's answer without re-deciding it.

    Scope

    Generated by Claude Code

  7. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor
    {
  "issue": 10069,
  "status": "done",
  "branch": "claude/issue-10069-admin-revoke-user-session-match-guard",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/10109",
  "premise_still_valid": true,
  "summary": "Installed vendor read on this branch: better-auth 1.7.1. Premise re-measured behaviourally through the real pipeline (AuthManager.handleRequest -> better-auth -> ObjectQL adapter, network-free) BEFORE any fix: a zero-match sessionToken answered 200 {\"success\":true}, an already-revoked (tombstoned) token also answered 200 {\"success\":true}, a live token answered 200 {\"success\":true} with a reason-admin tombstone (#7732 interaction confirmed), a non-admin caller got the vendor 403. Fix: hooks.before admission gate (admin-revoke-user-session-match-guard.ts) answering 404 RESOURCE_NOT_FOUND when no session carries the token — NOT the #9714 ownership predicate (no ownership dimension here), and permission is graded BEFORE existence using the vendor's own inputs (getAuthoritativeSessionFromCtx + live admin-plugin options + exported defaultRoles), so unauthenticated/unauthorized callers keep the vendor 401/403 byte-identically and gain no existence oracle. Existence-oracle question re-decided rather than inherited: the 404 reveals token existence only to callers entitled to session:[revoke], who already hold session:[list] over arbitrary users. Already-revoked tokens now answer 404 (a revoked session is not a session). Plural sibling route untouched. Patch changeset shipped (real behaviour change — skip-changeset not applicable).",
  "tests": "All at b293081c2. pnpm --filter @objectstack/plugin-auth test: 'Test Files 60 passed (60) / Tests 1335 passed (1335)' (os-verify-lock VERDICT command-exit 0). New suite: 15 tests (7 real-pipeline + 8 unit). Ablation: prediction stated first (exactly 3 refusal pins fail 'expected 404, received 200'); observed '3 failed | 12 passed', each 'AssertionError: expected 200 to be 404'; restored byte-identically, git hash-object 8cffac5b... equal both sides; NO rebuild needed for either leg — suite imports ./auth-manager in-package from src under vitest transform, no dist on the resolution path. check:type-check-debt: '33 ledger entr(ies) re-measured ... none above its recorded number' — plugin-auth held at 109, not raised. Gate union re-derived via node scripts/pm/dispatch-gates.mjs after the final ledger commit (it added check:cross-package-test-inputs) and re-run at b293081c2: changeset-gate-self-tests, objectui-changeset, slot-lookup, test-source-alias, type-source-resolution, cross-package-test-inputs, query-options-erasure, type-check-coverage, engine-double-contract ('316 (file, verb) row(s) held by the RETAINED ledger'), where-matcher, nul-bytes, error-code-casing, adr-0087-registration ('this PR adds no declared-breaking changeset'), changeset-no-major, empty-changeset, affected-docs — all exit 0, captured pre-pipe. Repo-wide pin-sweep for old {success:true} assertions on this route: zero found; counter-check passed (same net located the route ledger row, tombstone map, dogfood probe, checklist prose). CI on PR #10109 not awaited per standing clause; gates recorded as in_progress there.",
  "open_questions": [],
  "out_of_scope_findings": []
}

    Generated by Claude Code

    Generated by Claude Code

  8. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    PM 复核：内容 ACCEPT（PR #10109 @ b293081c2）

    按内容逐条核对，不采信报告叙述。

    ① 我事先声明的第一优先级检查项：两个越界测试文件到底改了什么 —— 结论对开发有利。
    better-auth-schema-parity.test.ts 与 managed-extension-fields.test.ts 各 additions: 12 / changes: 12，即 12 增 0 删：不是把 managed-extension-fields.test.ts 改回 findUp 种子（#9694 落地的 const HERE = __dirname; 原封未动），而是新 defaultRoles import 逼出来的扫描器 skip 条目，两处都写明了理由，并靠文件自带的 stale 断言在 import 消失时自动收回豁免。越界成立且必要，PR body 已申报。

    ② 纯增量：auth-manager.ts 83 增 0 删；无既有行为被移除。守卫走 hooks.before（after-hook 改不了 status），与 #10070 同缝。

    ③ 权限先于存在性 —— 这是本卡最容易做错的一步，做对了。守卫在 vendor adminMiddleware 之前跑，却先用 vendor 自己的输入（getAuthoritativeSessionFromCtx + 挂载中 admin 插件的 live options + vendor 导出的 defaultRoles）问 vendor 自己的权限问题；任何 vendor 会拒的调用者原样落回 vendor 的 401/403，missing 与 live token 字节相同（有 pin）。否则每个已认证非管理员都会白得一个存在性 oracle。镜像两个漂移方向都有 pin（松 → 集成测试；紧 → 自定义 roles 单测）。

    ④ 没有照抄 #9714 的谓词：该路由无 ownership 维度，谓词只问"有没有任何 session 带这个 token"。存在性 oracle 的取舍被重新判定而非继承——同一 default admin role 已授 session:['list']，404 不会告诉有权管理员任何它查不到的事。判断成立。

    ⑤ 反证质量：消融先声明预测签名（"恰好 3 个拒绝 pin 报 expected 404, received 200"），实测 3 failed | 12 passed 吻合，git hash-object 证明按字节还原，并明确交代无需重建（vitest 从 src/ 直接吃，dist 不在解析路径上）。零命中扫除带反向对照。债务 109 未抬。

    ⑥ 破坏性面：已撤销 token 再撤销由 200 变 404 —— 这是行为变更，changeset 已用发布说明读者能懂的话写明，且与 #7732"已撤销的 session 不是 session"教义一致。

    唯一未结项：CI 两红，均指向本 diff 够不到的子系统

    base 1800ffac2 在 main 上是绿的，所以"base 也红"这条豁免不成立；我不拿它当挡箭牌。已按规则动用唯一一次 re-run 去确认，因同 run 内 TypeScript Type Check 仍在跑被 GitHub 拒（403 This workflow is already running），等其收尾后立即重试。⛔ 在此期间不改 diff、不放宽 timeout、不 skip 测试。

    红的没转绿之前不 ready、不入队。

    Generated by Claude Code

  9. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    CI 复检结果：一红转绿，一红同签名复现 → 已派补丁轮（复现优先）

    唯一一次 re-run 的两个结果：

    复现 ⇒ 不是抖动，re-run 额度就此用尽，⛔ 不再重跑。

    我上一条里需要修正的部分

    我说该文件把 @objectstack/plugin-auth mock 成抛 ERR_MODULE_NOT_FOUND、因此本 PR 的代码在其中不会被执行 —— 这一条经核对成立，不改。但我由此暗示"所以与本 PR 无关"是跨了一步：plugin-dev/package.json 确实依赖 @objectstack/plugin-auth，所以本 PR 造成 @objectstack/plugin-dev#test turbo 缓存未命中——它让这个测试真的跑了起来，而 main 上它一直吃缓存。"不执行我的代码"与"与我无关"是两件事，我不拿后者当结论。

    现有证据（都是可核对的事实，不是叙述）

    我的假设 —— 是假设，不是结论

    饥饿而非逻辑：本 PR 的 Test Core shard 必须在 job 内重建 plugin-auth（日志里可见一次 22713ms 的 tsup DTS），与 vitest 争 CPU；失败的恰是要付真实 @objectstack/plugin-security 冷 transform 代价的两例。PR run 从不恢复已保存的 turbo 缓存（workflow 步骤是 Save Turbo cache (main only)），所以每次都以同样方式饿死——这能解释"确定性复现"而不需要 diff 是成因。

    ⚠️ 产出这个假设的对照同时变了两件事（构建负载 + diff），因此它不构成因果证据，我不作此主张。另一个我排除不掉的解释：这两例是真的挂死——通过的两例恰好是 security 服务被发布、或安全被显式关闭的两例，而 #10092 的改动正是"在 start() 里向已发布的 security 服务询问是否真的在强制执行"。挂死与饥饿都表现为平坦的 5000ms abort，光看时长分不开。

    已派：复现优先的补丁轮（contract-review tier，CONTRACT_REVIEW_TIER 现位于 scripts/pm/dispatch-gates.mjs:1932，实读非记忆）

    第一交付物是复现尝试，不是修复：(a) HEAD b293081c2；(b) base 1800ffac2 抽掉本 diff、但强制冷构建让该测试真的执行（并须自证不是缓存命中冒充通过）。再用抬高 timeout 的纯测量手段把"挂死"和"只是慢"分开。"无法复现"是被事先允许的答案，以免把人推向假阳性。

    判定规则已写死交给执行位：

    ⛔ 硬禁止：抬 timeout、skip/隔离该测试、为了转绿把 PR 扩到 plugin-dev、空提交踢 CI、改动已验收的守卫行为（404 / RESOURCE_NOT_FOUND 形状、权限先于存在性的次序）。

    红的没转绿之前不 ready、不入队。

    Generated by Claude Code

  10. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor
    {
  "issue": 10069,
  "status": "blocked",
  "branch": "claude/issue-10069-admin-revoke-user-session-match-guard",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/10109",
  "premise_still_valid": true,
  "summary": "CI-red patch round verdict: not-ours-base-also-red (starvation confirmed, not caused by the diff). Reproduced the exact CI failure signature locally on a 4-vCPU container (same size as a CI runner): dev-plugin-security-enforcement-warning.test.ts bail #1 times out at line 79 whenever vitest runs concurrently with two tsup DTS builds, and it does so IDENTICALLY at PR head b293081c2 and at base 1800ffac2 with the diff fully held out (fresh detached worktree, 0-line diff verified). Idle-machine baseline: all 4 tests pass at both shas, bail #1 ~3.5s (head 3428/3581ms, base 3614ms) — i.e. the test intrinsically sits at 70% of its 5000ms budget before any load. Mechanism: bail #1 pays the cold vite transform/import of the deliberately-unmocked real @objectstack/plugin-security chain inside DevPlugin.start()'s dynamic import; once the module graph is warm, bail #2 costs 5ms — but when bail #1 is aborted mid-import, bail #2 inherits the still-cold import (2676/3142ms observed locally; >5000ms on the more loaded CI shard), which is why CI shows BOTH tests red. Hang hypothesis is dead: with --testTimeout=30000 (CLI-only, nothing committed) under the same failing load, bail #1 COMPLETES and PASSES in 7487ms with all assertions green. The defect belongs to #10092/plugin-dev (a test whose in-test import cost saturates the default timeout under CI load); #10109's only causal contribution is the turbo cache miss that makes plugin-dev's tests actually run. Per the decision rule I did NOT touch packages/plugins/plugin-dev/**, did NOT weaken the test, and pushed NOTHING — PM files the plugin-dev card and raises the blocker. Blocked on: PM decision/card for the plugin-dev test defect; PR #10109 itself needs no code change.",
  "tests": "All runs at explicit shas via os-verify-lock.sh. (a) HEAD b293081c2, deps built via pnpm --filter '@objectstack/plugin-dev^...' build (exit 0, 4m24s): idle verbose run — bail #1 3428ms PASS, bail #2 5ms PASS, 4/4 green ('Test Files 1 passed', 'Tests 4 passed'); cold node_modules/.vite rerun — bail #1 3581ms PASS; under 1 concurrent DTS build — 4129ms PASS; under 2 concurrent DTS builds (plugin-auth + plugin-security, the dependents this PR dirties) — bail #1 5006ms 'Error: Test timed out in 5000ms.' at line 79 (CI run 32329090453 showed 5018ms, same line). (b) BASE 1800ffac2, diff held out: fresh detached worktree (git worktree add ../objectstack-issue-10069-base 1800ffac2, git diff --stat vs HEAD = 0 lines), fresh pnpm install, no .turbo dir and plugin-security/dist absent before build (cache-defeat proof: builds run through pnpm --filter which invokes tsup directly, no turbo layer; vitest invoked directly with --reporter=verbose, per-test timings prove real execution, not a cache hit) — idle: bail #1 3614ms PASS 4/4; under identical 2-build load: bail #1 5007ms TIMED OUT at line 79, same signature. Hang-vs-slow: base worktree, same load, --testTimeout=30000 — bail #1 completed and PASSED in 7487ms, bail #2 5ms; exit 0. All background load builds reaped with wait inside the same lock-held command; nothing outlived the runs; both worktrees removed cleanly after (git status empty, no force).",
  "open_questions": [],
  "out_of_scope_findings": ["NOT filed per this round's decision rule (PM files the card): dev-plugin-security-enforcement-warning.test.ts bail #1 intrinsically consumes ~70% of the 5000ms default timeout on an idle 4-vCPU machine paying the unmocked plugin-security transform in-test, so any concurrent turbo build starves it; surfaced by any PR that cache-misses @objectstack/plugin-dev#test. Fix belongs to a plugin-dev card (e.g. pre-warm the import outside the test body or a per-test timeout), not to PR #10109."]
}

    Generated by Claude Code

  11. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    补丁轮判定：not-ours-base-also-red —— 我的假设被对照证实，但证实它的不是我那个对照

    执行位完整报告见上一条（comment 5351307431）。关键在于它补上了我明确说过自己缺的那个东西：保持负载不变、只抽掉 diff 的对照。

    条件 bail #1 结果
    HEAD b293081c2，空闲 3428 / 3581 ms 4/4 通过
    base 1800ffac2，diff 完全抽掉（独立 detached worktree，git diff --stat = 0 行），空闲 3614 ms 4/4 通过
    HEAD，2 个并发 DTS 构建 5006 ms ❌ 超时 @ :79
    base，diff 抽掉，同样负载 5007 ms ❌ 同签名 @ :79
    base，同负载，--testTimeout=30000（仅命令行，未提交） 7487 ms ✅ 通过，断言全绿

    ⚠️ 我先前那个"饥饿"假设方向对了，但产出它的对照同时变了构建负载与 diff 两件事，因此当时不构成因果证据——我没拿它当结论是对的。现在成立的是这份新对照，不是那份旧的。

    执行位按判定规则没碰 plugin-dev/**、没削弱测试、什么都没推。

    阻塞登记

    Blocked-by: #10115 —— 已立卡并派出（plugin-dev 测试面，opus，Clause-② no）。PR #10109 的 diff 无法回避该缓存未命中，因此在 #10115 落地前无法转绿。

    ⛔ 不合并红的、不入队、不翻 ready。⛔ 不会为了转绿去抬 timeout、skip 测试或把 #10109 扩面到 plugin-dev —— 那正是这份测量所排除的做法。内容侧 ACCEPT 不变。

    Generated by Claude Code

  12. os-warren
    closed this as completedin #10109on Aug 20, 2026
  13. os-warren
    removed
    pm:dispatched
    on Aug 20, 2026
  14. os-warren
    removed their assignment
    on Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    domain:services

    Type

    Bug

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions