Skip to content

fix(plugin-auth): /delete-user resolves its target from the actor, never the body - #11475

Merged
os-sam merged 3 commits into
mainfrom
claude/issue-11074-break-glass-probe-self-service-target
Aug 24, 2026
Merged

fix(plugin-auth): /delete-user resolves its target from the actor, never the body#11475
os-sam merged 3 commits into
mainfrom
claude/issue-11074-break-glass-probe-self-service-target

Conversation

@os-sam

Copy link
Copy Markdown
Collaborator

Fixes#11074

What was wrong

The break-glass last-local-credential guard's target resolution on
/delete-user still preferred a body-supplied userId whenever one was
present:

lettargetId: string|undefined=ctx?.body?.userId??ctx?.body?.user_id;if(!targetId&&ctx.path==='/delete-user')targetId=breakGlassActor.userId;

/delete-user is the vendor's self-service delete: its own contract names no
target, the subject IS the authenticated caller. Because a body value still
won whenever one was supplied, the guard's own refusal became a per-record
answer about a user OTHER than the caller, reachable by any authenticated
caller — no admin role required, since /delete-user sits outside the
/admin/ lane. This is the authenticated residual of #10776's fix (#10776
closed the anonymous half; the pre-auth code had the same body-first
preference, just with no authentication at all in front of it).

Mechanism and fix only, per the card's disclosure discipline. No request
shapes, seeding steps or worked oracle example appear in this PR, in its
commits, or in the card.

What changed

packages/plugins/plugin-auth/src/auth-manager.ts, the same hooks.before
block #10776 touched: on /delete-user the target is now the resolved actor
unconditionallyctx?.body?.userId / ctx?.body?.user_id are never
consulted for that path, not even as a fallback.

consttargetId: string|undefined=ctx.path==='/delete-user'
? breakGlassActor.userId
: (ctx?.body?.userId??ctx?.body?.user_id);

/admin/remove-user and /admin/ban-user are unaffected — target-naming is
their own contract (an admin acting on someone else), per the vendor and the
ADR-0068 admin gate — and neither branch nor test touches them.

For every caller acting on themselves, nothing changes in outcome: the same
lookup runs against the same id and the same LAST_LOCAL_CREDENTIAL
conflict (or the same admission) is returned.

Tests

New file break-glass-guard-self-service-target.test.ts, driven through the
real better-auth pipeline (AuthManager.handleRequest), same precedent as
break-glass-guard-authentication-order.test.ts:

  • The disclosure, pinned as a comparison, not a value change. One
    authenticated non-holder caller; two /delete-user calls that differ only
    in what body.userId claims (the genuine holder vs. an id nobody holds, and
    separately, the holder vs. an omitted userId). Both legs of each pair are
    asserted to land on the same status, the same absence of the guard's code,
    AND on each other — expect(a.status).toBe(b.status) /
    expect(a.text).toBe(b.text). Pinning only "naming the holder no longer
    refuses" would prove nothing about distinguishability by itself; the
    equality assertion is what the finding is actually about.
  • The load-bearing half, both directions. A genuine holder deleting
    themselves via /delete-user is still refused with 409
    LAST_LOCAL_CREDENTIAL — the fix must not disarm the guard. And a holder
    cannot evade the guard by naming a decoy target in body.userId either:
    the target is the actor unconditionally, in both directions.

break-glass-local-credential.test.ts (#5892) pinned the pre-#11074 shape of
/delete-user directly against the synthetic before-hook: its BAN_PATHS
loop drove all three guarded paths with one caller identity and a
body-supplied target, which is exactly the shape this PR removes for
/delete-user. Updated so that leg authenticates AS the credential holder
instead of merely naming one in the body — the only way the guard's refusal
is reachable on that route now. /admin/ban-user and /admin/remove-user
keep the original body-target shape, unchanged.

Ran together (vitest run on all four files): 4 test files, 52 tests, all
passing
, on commit 33b16b5f61.

/admin/* same-pass measurement (reported, not acted on)

Traced, not fixed here, per the card: does the guard's refusal fire before
the admin-authorization refusal for an authenticated non-admin on an
/admin/ route?

  • /admin/remove-user is served directly by better-auth's own router. The
    guard is a global hooks.before, which better-auth runs ahead of every
    endpoint's own use: [...] middleware — including that route's
    adminMiddleware (session-only; the real role check runs even later,
    inside the endpoint handler itself). So yes: for an authenticated
    non-admin, the guard's own lookup and possible refusal happen before either
    of those checks ever run. This is a second, narrower face of the same class
    of issue as this card, out of scope here.
  • /admin/ban-user is different: #9652 shadows it with a raw mount
    (auth-plugin.ts) that authorizes the caller (gateAdmin(c)) BEFORE
    re-running the guard, so no such ordering issue exists on that path.

Not extended into this PR — reported to the maintainer for a separate card,
per the dispatch brief.

Gates

Union derived via node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on
commit 33b16b5f61 (clean tree). Every exit code captured before any pipe;
every verdict below is the gate's own printed line.

Path-derived (12): check:changeset-gate-self-tests, check:objectui-changeset,
check:published-files, check:slot-lookup, check:test-source-alias,
check:type-source-resolution, check-adr-0087-registration,
check-changeset-no-major, check-ci-filter-parity, check-empty-changeset,
check-plugin-teardown-shape, check-affected-docs — all green.

Convention-triggered by the new/edited test files (6):
check:query-options-erasure, check:engine-double-contract,
check:cross-package-test-inputs, check:where-matcher,
check:type-check-coverage, check:type-check-debt — all green. The ratchet
ran on a built workspace closure and reached a real verdict:
check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured in 355.2s, 1897 raw tsc error(s) total, none above its recorded number.
(@objectstack/plugin-auth's own TEST_DEBT entry moved from 109 to 97 raw
errors — an improvement, left as reported rather than lowered, per the
gate's own guidance that an improvement need not pay a bookkeeping toll to
land.)

Also check:nul-bytes: OK (scanned 6432 text file(s) ... no raw ASCII control bytes).

Package suite and typecheck: the four directly-affected test files above are
green (52/52). The package's full 71-file suite was run once, pre-fix, and
correctly caught the one fixture this PR's second commit updates
(break-glass-local-credential.test.ts's /delete-user leg, which pinned
the exact cross-target shape this PR removes); it was not re-run in full
post-fix given shared verify-lock contention (#11363) — narrowing declared,
not silent: the four directly-affected files were re-run and are green, and
CI runs the full farm regardless. pnpm --filter @objectstack/plugin-auth typecheck (both tsc invocations) is green on the built package.

Posture

Clause-② is yes — an authenticated caller's accept/reject
distinguishability changes. This PR stays draft,
needs:contract-review stays on the card, and this seat does not clear it,
mark it ready, arm auto-merge or merge.

Scope

No packages/spec edit; no content/docs/releases/** touched. Changeset
included (patch, @objectstack/plugin-auth). /admin/remove-user and
/admin/ban-user target resolution is untouched.


Generated by Claude Code

…ver the body
The break-glass last-local-credential guard's target resolution on
/delete-user still preferred a body-supplied `userId` whenever one was
present. /delete-user is the vendor's self-service delete — its contract
names no target, the subject IS the authenticated caller — so that
preference let an authenticated caller steer the guard's own refusal at a
user other than themselves.
The guard's target on /delete-user is now the resolved actor
unconditionally; body.userId is never consulted for that route.
/admin/remove-user and /admin/ban-user are unaffected.
Updates the #5892 fixture that pinned the old cross-target behavior on
/delete-user to the new invariant: the refusal there is only reachable by
authenticating AS the credential holder, never by naming one in the body.
Part of #11074
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 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 905019b1bf2f291e6a1a4fb87bf7f667f2af9088packageMentionDocs.

Which tree this was computed on

This run read content/docs from 09e0990140b355a1be5106a314481bedc44152cc — the merge of head 33b16b5f611c7dfaa25255bccdc3694c6808312b into base 905019b1bf2f291e6a1a4fb87bf7f667f2af9088, 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 09e0990140b355a1be5106a314481bedc44152cc && git checkout 09e0990140b355a1be5106a314481bedc44152cc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 905019b1bf2f291e6a1a4fb87bf7f667f2af9088 33b16b5f611c7dfaa25255bccdc3694c6808312b && git checkout -B drift-repro 905019b1bf2f291e6a1a4fb87bf7f667f2af9088 && git merge --no-ff 33b16b5f611c7dfaa25255bccdc3694c6808312b
node scripts/docs-audit/affected-docs.mjs --json 905019b1bf2f291e6a1a4fb87bf7f667f2af9088

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 23, 2026
@os-sam
os-sam marked this pull request as ready for review August 24, 2026 00:33
@os-sam
os-sam added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 56d3c7aAug 24, 2026
32 checks passed
@os-sam
os-sam deleted the claude/issue-11074-break-glass-probe-self-service-target branch August 24, 2026 00:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-sam@claude