fix(plugin-auth): the auth catch-all yields only a 404 that disclaims ownership - #15918
Conversation
… ownership `registerAuthRoutes` mounts one catch-all over the auth namespace and, since #4088, deliberately yields to the rest of the Hono chain when better-auth answers 404 — that is what keeps `plugin-hono-server`'s `/auth/me/permissions` and `/auth/me/localization` reachable in either registration order. The yield had only the status to go on, so it could not tell "I do not serve this path" from "I serve it and the answer is 404". Measured with the shipped handler on a real Hono app: with one broad downstream mount in the chain — `app.all('/api/v1/*', c => c.json({}))`, the shape a composition adds — `POST /api/v1/auth/delete-user` came back `200 {}` where better-auth had answered 404 because `user.deleteUser` is unconfigured. `auth-route-ledger.ts` carries that route under the `disabled` disposition precisely because it is published and refused, and the same held for every 404 a routed endpoint produces for a bad token, an unknown id, or an admin family the deployment does mount. All of those answers were up for grabs. The catch-all now asks better-auth's live instance whether it owns the path before it yields. The seam is `auth.api` — the same one the route ledger's conformance test reads and the `/admin/` dogfood sweep derives from — and the matching mirrors better-call's own `createRouter` walk: its `SERVER_ONLY` skip, its `:param` syntax, its per-method registration. The skip is load-bearing, not cosmetic: measured on the stock boot, all nine `/admin/oauth2/*` endpoints are in `auth.api` carrying `SERVER_ONLY: true`, so better-call never routes them and their 404 stays yieldable. Ownership is "does better-call route this", not "is it in `auth.api`". A table that cannot be built answers "not owned", so an enumeration failure degrades to the previous behaviour instead of taking the #4088 surface down with it. The mount is untouched: it still claims exactly `${basePath}/*` and still forwards every request under it. What narrowed is which 404 may be handed on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift CheckThis PR changes 1 package(s): 4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 11 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin af784d80873ffad2bb2459eaa2ca726c3cd85634 && git checkout af784d80873ffad2bb2459eaa2ca726c3cd85634
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cee3961759160ed72aacb407f15288cbc018d2eb 81226d5ba5ac186a8adaae34176597fb1e7dbb02 && git checkout -B drift-repro cee3961759160ed72aacb407f15288cbc018d2eb && git merge --no-ff 81226d5ba5ac186a8adaae34176597fb1e7dbb02
node scripts/docs-audit/affected-docs.mjs --json cee3961759160ed72aacb407f15288cbc018d2eb
|
Clause-② contract review — PR #15918 (card #15417) — verdict: PASSTier: Independence pair (C4, machine spelling):
The dev was a Subject: head 1 · The falsification (blocking bar — met)Booted the real stack myself — Every row of the dev's Step-1 table reproduces byte-for-byte, including the store readback. The boot is the comparable one in the sense that matters: the auth surface is live (positive control lands), the ObjectStack raw mounts refuse (403 rows), and the nonexistent path answers 404, not Is the comparison sound against the card's stack? Two checks beyond the rows. (i) The #4088 yield ( 2 · The mechanism and the fix, end-to-end on the real stack (blocking bar — met)Second boot, same stack,
The leg-1 column is a boot-level ablation, not the unit one: 3 · #4088 intact, and
|
| leg | mutation | blob | suite | result |
|---|---|---|---|---|
| 1 | yield condition → status === 404 |
5cab6b85… |
auth-catchall-yield.test.ts |
3 failed / 5 passed — the three "does NOT yield an owned 404" tests; #4088 file stays 6/6 |
| 2 | owns() → true (my shape keeps the empty-table guard) |
88bbc62e… |
same | 2 failed / 6 passed — the two yield-must-still-happen tests (the dev's unconditional true also reds the SERVER_ONLY pin, hence its 3) |
| 3 | :param → prefix match |
56836810… |
better-auth-route-ownership.test.ts |
1 failed / 9 passed — "does not let a :param swallow a longer or shorter path"; the e2e suite stays 8/8 |
Leg 3 is as thin as flagged — one assertion in one unit file. So I ran the live-table differential of §3 under the same mutation: 102 / 140 "other" divergences (every …/extra spelling under a parameterised or literal route reads as owned). The leg discriminates strongly; the in-tree pin for it is just narrow. Non-blocking suggestion: a conformance-style pin over the live auth.api ("every ledgered route with /extra appended is not owned") would make that leg red on measurement rather than on a hand-written slice.
6 · The #4088 fixture's JSON 404 — the dev's unfiled observation is correct, and not card-worthy
Measured: better-call's unrouted 404 is new Response(null, {status: 404, statusText: 'Not Found'}) (router.mjs:33–51) — on the wire len=0 ct=(none), in both my direct handleRequest probe and the boot. So the fixture's {message:'Not Found', code:'NOT_FOUND'} with application/json is indeed a convenience of that file. It does not weaken the pin: "returned verbatim" is body-agnostic pass-through, and a distinguishable body makes the verbatim assertion stronger. The new suite pins the measured shape. A one-line comment correction can ride on the next touch of that file; no card. (Bonus fact the wire gave up: the routed 404 — delete-user with a session — is len=0 ct=application/json, so the two 404s are distinguishable by content-type alone, which corroborates the ownership split from the outside.)
7 · Half 2 — split, not fixed; ledger untouched; reasoning holds
auth-route-ledger.ts blob e48823bab… at merge-base, 62fce1ac5 and HEAD — untouched. Measured, not re-read: stock auth.api has 9 /admin/ endpoints (all oauth2, all SERVER_ONLY) and the three routes answer the unrouted 404; at the maximal config all three are routed (owns() true, GET /admin/list-users → 401 UNAUTHENTICATED enveloped — a real answer). The ledger's own header pins publication at LEDGERED_PLUGIN_CONFIG and says a subset deployment is correct. Triage's "be prepared for fix-the-ledger" was answered by measurement in the other direction, and the split criterion triage pre-recorded is the one applied. Correct — with the §4 caveat about the nine SERVER_ONLY rows, which #15920's triage should see before it leans on "the ledger is not wrong".
8 · The two exit-3 gates — CI measured both on this head
Both jobs ran (not path-skipped) and both gate steps printed their own pass line on ee26bc613:
- Build Core (job
101320683364, success) at 14:27:34Z:✓ check:dual-build-cjs-loads — 103 published require entry point(s) across 66 package(s) load; 619 emitted CommonJS file(s) parse; … - Type Check · consumer gates (job
101320642184, success) at 14:27:31Z:✓ check:published-readme-exports — 60 published document(s) across 79 workspace package(s); 215 import statement(s), … 200/200 @objectstack/ specifier(s) naming a workspace member.(the step lives intypecheck-consumers, not in Lint & Repo Gates — worth knowing for the next NOT MEASURED placement).
The dev's argument (no package.json/exports/build config/README moved; new module package-internal — confirmed, index.ts does not re-export it) was right, and it was correctly labelled an argument. All 33 check runs on this head are green or path-skipped; none failed.
9 · Other measurements
pnpm --filter @objectstack/plugin-auth test— 100 files / 2087 tests green, exit 0 (the dev's 99/2085 plus my scratch differential file, since deleted).pnpm --filter @objectstack/plugin-auth typecheck— exit 0, includingcheck:test-typecheck("test layer compiles").- The three subject suites at head: 3 files / 24 tests green.
10 · The open question — stated fairly; costs to add (⛔ not ruling)
- A fair. B under-costed in one respect: Hono's
app.routeslists registrations but cannot say which one will answer without re-running the matcher, and the@objectstack/honoadapter has no such seam at all — so B is a plugin-auth-only answer. C is correctly marked a reversal ofvendor-admin-refusal-envelope.ts's narrowing 2; one cost to add: it also changes what plugin-auth 的终结式 catch-all 吞掉 /api/v1/auth/* 下别人的路由 —— console 权限层目前靠 kernel.use() 顺序才活着 #4088's own text guarantees ("the wire shape for a genuinely unclaimed auth path is unchanged") and would move that fixture's verbatim-404 pin. - ⭐ Orthogonal to A/B/C and not on the dev's list: the
@objectstack/honoadapter's/auth/*mount yields every 404 too (adapters/hono 的 ${prefix}/auth/* 与 ${prefix}/storage/* 是终结式通配 —— 与 #4088 同一缺陷,只是这个包在仓内没有消费者 #4117,packages/adapters/hono/src/index.ts:395–405), untouched here. Its downstream is the terminal dispatcher, so an owned 404 handed on there comes back as the dispatcher's 404 envelope — status kept, better-auth's answer replaced. Same defect class, narrower effect, different package; the adapter's own header says the cloud control plane annotates the app it returns. Deserves a follow-up card with the same ownership check, whatever is ruled on A/B/C. Non-blocking for this PR: different package, the plugin-auth changeset namesregisterAuthRoutesexplicitly, and nothing there produces a 200.
Checklist (derived judgments · semver · flags)
① Public surface: AuthManager.ownsRoute(request) is a new public method on an exported class (index.ts:12 export * from './auth-manager.js') — additive; AuthService (spec) is untouched (still handleRequest only) and the catch-all calls the concrete manager, so no contract widening; buildBetterAuthRouteOwnership is package-internal. Accept-set change: the catch-all no longer yields a 404 on a (method, path) better-call routes — exactly as the changeset states, with the one spelling carve-out of §3. ② Semver: patch is consistent with precedent for a plugin-auth wire-behaviour fix carrying an upgrade note (plugin-auth-find-envelope-limbs.md), and ADR-0087 sees no breaking marker. ③ Flags: one open_questions entry, correctly escalated not decided; two out_of_scope_findings, one filed (#15920) and one correctly unfiled (§6). Findings I add: the adapter's identical yield (§10), the nine SERVER_ONLY rows in the mounted-surface ledger (§4), the spelling divergence (§3), the thin leg-3 pin (§5).
Verdict — PASS
The card's headline is falsified on the real framework boot with the comparison rows reproducing exactly; the yield mechanism is demonstrated through the shipped handler in both directions, including the boot-level ablation that makes POST /auth/delete-user answer a wildcard's 200 {} under the pre-fix condition and 404 at head; owns() agrees with better-call's own router construction on the live table at both configurations with zero divergences in the yield direction; the nine SERVER_ONLY endpoints are correctly out of scope; #4088's surface is intact on every real route; the ledger is untouched and its reading holds; the two exit-3 gates are CI-measured green on this head. Requested before undraft, PM's call and text-only: the changeset's "yielded exactly as before" sentence per §3. Non-blocking follow-ups: the ownsRoute trailing-slash/// alignment (§3), the adapter's yield (§10), the nine ledger rows (§4), a live-table pin for leg 3 (§5). ⛔ Nothing pushed, undrafted or merged; no git stash. Worktree left provably clean at ee26bc613: git status --short 0 lines, git diff HEAD 0 lines, all seven files' blobs equal HEAD's. Scratch scripts and logs suffixed -15918-review (24 files); both scratch test files deleted before the typecheck and the final proof.
Generated by Claude Code — reviewer for the domain:services PM seat (PM session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909)
🤖 Generated with Claude Code
https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
Generated by Claude Code
… out of the "yielded as before" claim
Text-only; no source, pins or behaviour move on this commit.
The changeset said a path better-auth does not own is "yielded exactly as
before". A live differential built for the clause-② review — better-call
1.4.0's `createRouter` plus `processRequest`'s pre-checks, re-run over rou3
0.9.2 against the real `auth.api` at the stock and the maximal configuration,
4004 + 5278 (method, path) pairs — found 0 divergences in the yield direction,
so that half of the claim is now measured rather than asserted. It also found
the sentence is not unconditional in the other direction.
A TRAILING-SLASH OR DOUBLED-SLASH SPELLING OF A PATH BETTER-AUTH DOES OWN —
`/api/v1/auth/delete-user/`, `/api/v1/auth//sign-in/social` — is claimed by
this ownership table rather than yielded. better-call refuses those spellings
as unrouted: it returns its 404 on a `//` and on trailing-slash parity before
it ever looks the route up, while `betterAuthEndpointPath` strips the trailing
slash and `splitPath` drops empty segments, so the table counts them as owned.
On a composition with a broad downstream mount, such a spelling now answers
better-auth's 404 instead of that mount's response — measured on the wire:
`POST /auth/delete-user/` answers 404 at this head where the pre-fix yield gave
the wildcard's `200 {}`. Bounded at 91 + 153 pairs (stock) and 121 + 212
(maximal).
Left as it is, deliberately: no route in this repo registers a spelling of that
shape, nothing under `/auth/me/*` or any genuinely unowned path is touched, and
the effect where it does show is that a near-miss spelling stops answering a
foreign mount's vacuous 200 — the direction this change argues for. Aligning
`ownsRoute` with better-call's own pre-checks, with a pin, is a follow-up card
rather than a source change on this head.
The changeset feeds release notes, which is why the sentence had to stop being
unqualified even though the divergence is graded non-blocking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
PM verification of the carve-out — undrafted and armedVerified at head
|
Part of #15417
Part of, not a closing keyword, and deliberately. This lands the framework-side defect the card's mechanism turned out to be. The card's headline claim does not reproduce framework-side, and its second observation is a composition question that is being split out — both measured below. #15417 remains open for the maintainer to re-triage on this evidence.Step 1 first: the card's own control, on a framework-side boot
The card asked for this before anything else, and it is the row that gives every other row meaning. Booted the real stack (
@objectstack/verify+ the showcase app,packages/qa/dogfood), member and platform-admin sessions:The reported
200 {}does not reproduce on a framework-side boot — the nonexistent path answers404. The two403 PERMISSION_DENIEDrows and the positive control reproduce exactly as the card recorded them, so the boot is comparable; it is specifically the vacuous 200 that is absent.What does produce it — the mechanism, reproduced framework-side
The catch-all is involved, but not through its width. Since #4088 it is deliberately not terminal: when better-auth answers 404 it calls
next()and lets whatever else matched answer instead. That yield is load-bearing —plugin-hono-servermounts/auth/me/permissionsand/auth/me/localizationfrom its ownkernel:readyhook, and without it they are reachable only when HonoServerPlugin happens to register first.The yield had only the status to go on, so it yielded every 404. Add one broad downstream mount —
app.all('/api/v1/*', c => c.json({})), the shape a composition adds — and on the same real stack:delete-useris a route better-auth serves; it answers 404 becauseuser.deleteUseris deliberately unconfigured, andauth-route-ledger.tscarries it under thedisableddisposition for exactly that reason. Its answer was being replaced by a downstream route's. The same held for every 404 a routed endpoint produces — a bad token, an unknown id, an admin family the deployment does mount.The fix: narrow the yield, never the mount
⛔ The mount is untouched — still exactly
rawApp.all(\${basePath}/*`)`, still forwarding every request under it. What narrowed is which 404 may be handed on: the catch-all now asks better-auth's live instance whether it owns the path, and yields only when it does not.The seam is
auth.api— the same oneauth-route-ledger.conformance.test.tsreads and the/admin/dogfood sweep derives from, because there is no route table to enumerate by hand. Matching mirrors better-call's owncreateRouterwalk./admin/oauth2/*endpoints were protected examples. Measured on the stock boot, all nine are inauth.apiand all nine carrySERVER_ONLY: true— better-call never routes them, so their 404 genuinely disclaims ownership and they stay yieldable. Ownership is "does better-call route this", not "is it inauth.api". The prose and a dedicated pin now say so; the e2e caught the overclaim, not review.A table that cannot be built answers "not owned", so an enumeration failure degrades to the pre-#4088-era behaviour rather than taking that surface down with it.
One carve-out, measured and bounded
The changeset used to say a path better-auth does not own is "yielded exactly as before". The clause-② review built a live differential — better-call 1.4.0's
createRouterplusprocessRequest's pre-checks, re-run over rou3 0.9.2 against the realauth.apiat the stock and the maximal configuration, 4004 + 5278 = 9282 (method, path) pairs over 572 + 754 paths — and found 0 divergences in the yield direction. It also found the sentence is not unconditional in the other direction, and the changeset now carries the carve-out in the same words:A trailing-slash or doubled-slash spelling of a path better-auth DOES own —
/api/v1/auth/delete-user/,/api/v1/auth//sign-in/social— is now claimed rather than yielded. better-call treats those spellings as unrouted (it refuses on a//and on trailing-slash parity before it looks the route up), while this ownership table strips the trailing slash and drops empty segments and so counts them as owned. On a composition with a broad downstream mount, such a spelling therefore answers better-auth's 404 instead of that mount's response. Bounded at 91 + 153 pairs (stock) and 121 + 212 (maximal), and confirmed on the wire:POST /auth/delete-user/answers 404 at this head where the pre-fix yield gave the wildcard's200 {}.Left as it is, deliberately: no route in this repo registers a spelling of that shape, nothing under
/auth/me/*or any genuinely unowned path is touched, and where it does show the effect is that a near-miss spelling stops answering a foreign mount's vacuous 200 — the direction this change argues for. AligningownsRoutewith better-call's own pre-checks, with a pin, is a follow-up card rather than a source change on this head.Both directions, after the fix, on the real stack
POST /auth/delete-user(owned, answers 404)GET /auth/admin/oauth2/resources(SERVER_ONLY, unrouted){}— still yielded, correctlyPOST /auth/admin/definitely-not-a-route-1989(unowned){}— still yielded (see split, below)GET /auth/me/permissions(#4088 surface)The full post-fix table is byte-identical to the pre-fix table above:
ban-userstill answers the member403 PERMISSION_DENIED,POST /auth/update-userstill answers200 {"status":true}and the write still lands (read back from the store).SERVER_ONLYwas independently confirmed by review at exactly 9/admin/oauth2/*endpoints at both configurations, with the unrouted-404 signature on the wire (len=0 ct=(none), the same signature as the nonexistent path) — andadmin-route-nonadmin-refusal.dogfood.test.tsalready classifies those same nine asnot-mounted.Pins, and each one mutated
Every pin was mutated, the mutation proven on disk by a
git hash-objectdelta plus marker counts, restored withgit checkout HEAD -- ABSOLUTE_PATHunder atrap ... EXIT INT TERM, and the restore proven by an emptygit diff HEADplus the blob back at its HEAD value.status === 404auth-catchall-yield.test.tsowns()forced to always return trueauth-catchall-yield.test.ts:parammatching relaxed to a prefix matchbetter-auth-route-ownership.test.tsThe ablation ran at
62fce1ac5; its subject files are byte-identical atee26bc613. Review checked that carry-over argument independently and found all seven files identical at both commits. Review also re-ran the legs itself, including a boot-level leg 1 with a rebuiltdist/and anablation-dist-preflightmarker check in both directions.auth-catchall-fallthrough.test.ts(#4088) is green throughout. Its fakeAuthManagergained the one method the catch-all now calls, derived from that file's ownownedtable so the file keeps meaning what its title says.Half 2 — measured, and it is a composition question
admin/list-users·admin/set-role·admin/update-user, the three the card measured absent:auth.api— 9/admin/endpoints, all oauth2 — and they answer 404.OS_SCIM_ENABLED=true, which forces it): all three present — 24/admin/endpoints — answering real vendor refusals (403 YOU_ARE_NOT_ALLOWED_TO_LIST_USERSand siblings).So the three routes are configuration-dependent, and the ledger is not wrong.
BETTER_AUTH_MOUNTED_SURFACEis pinned at the maximalLEDGERED_PLUGIN_CONFIGand its own header already states that a deployment running fewer plugins serves a subset and that this is correct — it is publication, not liveness, in as many words. ⛔ Deleting those rows would both break the exact-equality conformance test and misreport the mounted surface. Triage recorded the split criterion in advance; this is that case, so it is filed separately (#15920) rather than blocking this PR.Verification
At head
81226d5ba. Every exit code captured immediately after a single redirected command, never off a multi-command line and never through a pipe.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackfrom the actual changed files — no stale-tree warning, and the family is identical to the previous round's 50. 50 run, 47 exit 0.node scripts/check-adr-0087-registration.mjs --self-test— exit 0 (325 assertions over real temp git repos), as the control for the run below.node scripts/check-adr-0087-registration.mjs --base origin/main --head 81226d5ba5ac186a8adaae34176597fb1e7dbb02— exit 0.check:dual-build-cjs-loads,check:published-readme-exports,check:dts-closure. All three read built output and refuse untilpnpm buildhas populateddist/; this round ran in a fresh worktree with nothing built, which is whycheck:dts-closurejoins the two from last round. Review confirmed the first two CI-green on the previous head —check:dual-build-cjs-loadsin Build Core,check:published-readme-exportsin Type Check · consumer gates (not Lint) — and this round moved only the changeset plus an unrelatedauth-plugin.tschange that arrived with theorigin/mainmerge.Earlier rounds, retained:
pnpm --filter @objectstack/plugin-auth testexit 0 (99 files / 2085 tests; review measured 100 / 2087 with its own scratch file),pnpm --filter @objectstack/plugin-auth typecheckexit 0 includingcheck:test-typecheck, which compiles the test layer.