Skip to content

docs(plugin-auth): correct the has-permission header's fail-direction claim - #12362

Merged
os-trump merged 1 commit into
mainfrom
claude/issue-12270-has-permission-fail-direction-header
Aug 25, 2026
Merged

docs(plugin-auth): correct the has-permission header's fail-direction claim#12362
os-trump merged 1 commit into
mainfrom
claude/issue-12270-has-permission-fail-direction-header

Conversation

@os-trump

Copy link
Copy Markdown
Collaborator

Fixes#12270

Comment-only correction in packages/plugins/plugin-auth/src/admin-has-permission-endpoint.tszero executable lines change, no dist/ byte moves. Scope is disposition (a) exactly as triage pinned it; (b) is untouched and remains a maintainer-floor call (see "The fork" below, which measures whether it is required — it is not, on the evidence in this tree).

The two replaced passages, quoted so review need not fetch the diff twice

1. Header, ## Fail direction

Before

Every uncertainty delegates: an unreadable body, a shape outside the set the vendor evaluates, an unreadable live-options object. Delegation can only reproduce the vendor's measured native behaviour — it can never mint a true for a caller the predicate did not admit. The only path to success: true runs through isPlatformAdminUser (or the vendor's own adminUserIds short-circuit, mirrored below for option fidelity; this repo configures none).

After

Two of the three uncertainties delegate: an unreadable body and a shape outside the set the vendor evaluates. Delegation can only reproduce the vendor's measured native behaviour — it can never mint a true for a caller the predicate did not admit. The only path to success: true runs through isPlatformAdminUser (or the vendor's own adminUserIds short-circuit, mirrored below for option fidelity; this repo configures none).

The third — an unreadable live-options object — does NOT delegate. It is caught in answerPermissionQueryAsAdmin, adminOptions becomes undefined, and the evaluation answers from the vendor's exported defaultRoles with adminRoles = ['admin']: on that path the deployment's own roles / adminRoles are not the ones read. It is reached only after isPlatformAdminUser has already admitted the caller, so it still cannot answer for a caller the predicate refused.

The middle sentence — delegation can never mint a true for a caller the predicate did not admit — is kept verbatim. It was and remains true: the fallback sits after the predicate has already admitted the caller. Only the enumeration around it moved.

2. Site comment at the catch in answerPermissionQueryAsAdmin

Before

// Unreadable live options → run on the vendor's own defaults, exactly as
// the vendor itself would with an unconfigured plugin.

After

// Unreadable live options → evaluate on the vendor's exported defaults
// (`defaultRoles`, `adminRoles = ['admin']`) instead of this deployment's
// configured ones. This path answers; it does not delegate (see the
// header's "Fail direction").

Why each was wrong

  1. The enumeration. The first two uncertainties do delegate — readEvaluatedPermissionQuery returns undefined and the mount falls through to AuthManager.handleRequest. The third does not: the catch sets adminOptions = undefined and execution continues into the same evaluation, on defaultRoles with adminRoles = ['admin']. It answers. On a file whose whole subject is which authority answers, a header sentence that reads as a security invariant and is not one is the defect; the next reader to cite it would cite it as true.
  2. The comparison. The mechanism half of the site comment was honest. The comparison was not: the vendor never runs unconfigured on this path — it reads its own live options off its own plugin instance and cannot fail to. So the fallback is not "what the vendor would do", it is "what the vendor would do in a different deployment". The replacement keeps the mechanism, drops the comparison, and points at the header rather than restating it.

Neither replacement asserts anything the code does not do. In particular the new text does not claim the fallback is unreachable, does not claim it is safe, and does not claim a direction for how a configured deployment's answer would differ — only that the configured roles / adminRoles are not what that path reads.

The fork, measured rather than reasoned about

Whether (a) is sufficient turns on one question: is the fallback reachable in a supported deployment shape? Both conditions must hold.

Condition 1 — can AuthManager.getAuthContext() throw? Plausibly yes. It is auth-manager.ts:4485: await this.getOrCreateAuth() then .$context, and getOrCreateAuth calls createAuthInstance(), which dynamically imports better-auth, builds the plugin list and the database config. A boot failure there propagates.

Condition 2 — does any deployment configure custom roles / adminRoles?Zero hits, and there is no surface through which a deployment could.

$ grep -rn --include='*.ts' -E '\badmin\(\s*\{?' . | grep -v node_modules

Every admin(...) construction in the tree passes { schema } and nothing else:

  • packages/plugins/plugin-auth/src/auth-manager.ts:2830 — the one production site: admin({ schema: buildAdminPluginSchema() })
  • last-admin-guard.test.ts:548,879, better-auth-schema-parity.test.ts:227, credential-at-rest-posture.test.ts:471, managed-extension-fields.test.ts — all { schema } only
  • the admin() hits in packages/runtime/src/domains/*.test.ts are a local test helper of the same name, not the better-auth plugin
$ grep -rn 'adminRoles' . | grep -v node_modules # every hit is prose, a mirror of the vendor default, or this file's own reader
$ grep -rn --include='*.ts' -E '^\s*roles:\s' packages/ apps/ examples/ | grep -v node_modules

No roles: hit is a better-auth admin-plugin option (they are ObjectQL positions, spec schemas, a security-plugin fixture, and one test helper argument). And:

$ grep -rn --include='*.ts' -E 'betterAuthOptions|extraPlugins|customPlugins|plugins:\s*\[\s*\.\.\.' packages/plugins/plugin-auth/src/*.ts | grep -v '\.test\.'
(no output)

There is no passthrough by which a deployment supplies its own better-auth plugin options. So condition 2 is not merely unmet today — it has no configuration surface in this repo, which is why this PR does not stop for a ruling: nothing here shows (b) is required. That remains a maintainer call on the merits, not a blocker this change creates. The fork is reported on the card.

Verification

Run against the tree now at 0451231e0 (working tree clean at that commit, so every run below covers exactly HEAD's bytes).

  • pnpm --filter @objectstack/plugin-auth typecheck — exit 0, captured before any pipe. Needs the package's own dist/ for its second program (tsconfig.examples.json), so the dependency closure and the package were built first: pnpm --filter '@objectstack/plugin-auth^...' build and pnpm --filter @objectstack/plugin-auth build, both exit 0.
  • pnpm --filter @objectstack/plugin-auth exec vitest run --maxWorkers=2Test Files 80 passed (80) · Tests 1631 passed (1631). admin-has-permission-endpoint.test.ts on its own: 13 passed (13).
  • Derived gate union, from node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (11 families for this path), each exit code captured before any pipe — all exit 0. Their own verdict lines include:
    • check-nul-bytes: OK (scanned 6834 text file(s) -- 6834 tracked, 0 untracked-not-ignored; skipped 7 binary; no raw ASCII control bytes).
    • OK: 17 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
    • check-type-source-resolution OK — 93 tsc program(s) across 77 packages scanned
    • check-test-source-alias OK — 72 packages with tests scanned
    • plus check:page-declaration-shape, check:published-files, check:slot-lookup, check-ci-filter-parity, check-cross-package-test-inputs, check-plugin-teardown-shape, check-affected-docs, check-drift-comment.
  • Comments-only, proven mechanically, not by eye: git diff -U0 | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | grep -vE '^[+-]\s*(\*|//|/\*\*)' returns empty — every changed line is a comment line.
  • Lint, narrowed and declared as such. The repo-wide pnpm lint is CI's run. Locally the changed file was linted directly: 1 file, 0 errors, 0 warnings (count read from --format json, not estimated), the file present in ESLint's own resolved population with no ignore warning. The narrowing excludes nothing, because eslint.config.mjs states in its own text that this repo "runs one eslint.config.mjs, which never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file, test or not" — with no cross-file type program, a comment edit in one file cannot move any untouched file's verdict.
  • No ablation, deliberately. There is no behaviour to dissolve; a dissolution leg on a comment measures nothing, and claiming one would be worse than skipping it.

Changeset

None, on purpose. Comments do not ship — tsup strips them and no dist/ byte moves — so the skip-changeset label carries the declaration rather than an empty changeset, which stalls the release here. If Check Changeset disagrees, that is reported rather than answered with an invented bump.

Contract review

Not applicable. Comment text changes no accept/reject behaviour and widens no surface, so needs:contract-review is deliberately not hung here.

Refs: #11900 / PR #12210 (where this code landed and where the finding was raised) · ADR-0068 D2 (the predicate) · #12029 (the gate-then-delegate seam this module inverts). None of those is closed by this PR.


Generated by Claude Code

… claim
The `admin-has-permission-endpoint.ts` header's "Fail direction" paragraph
enumerated three uncertainties and said every one of them delegates. Two do:
an unreadable body and a shape outside the set the vendor evaluates both
return `undefined` from `readEvaluatedPermissionQuery` and fall through to
`AuthManager.handleRequest`. The third does not. An options read that throws
is caught, `adminOptions` becomes `undefined`, and the evaluation continues
on better-auth's exported `defaultRoles` with `adminRoles = ['admin']` — it
answers rather than delegating, and on that path the deployment's own
`roles` / `adminRoles` are not the ones read.
On a file whose whole subject is which authority answers, that sentence reads
as a security invariant and is not one. The correction states what the code
does and claims nothing further; the neighbouring sentence — that delegation
can never mint a `true` for a caller the predicate did not admit — is true and
is kept verbatim, since the fallback is reached only after `isPlatformAdminUser`
has already admitted the caller.
The site comment at the `catch` keeps its mechanism half and loses its
comparison: "exactly as the vendor itself would with an unconfigured plugin"
does not hold, because the vendor never runs unconfigured here — it reads its
own live options off its own plugin instance and cannot fail to.
Comments only. Zero executable lines change; no `dist/` byte moves.
Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa
@os-trumpos-trump added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 25, 2026 — with Claude
@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, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

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. Of those 177: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 107 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

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 a6c1e216fcbe39c15e0f31cff4d82275f38085f0packageMentionDocs.

Which tree this was computed on

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

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-trump@claude