Skip to content

fix(plugin-auth): register the MCP resource so RFC 8707 authorize can succeed - #16780

Merged
os-zhuang merged 8 commits into
mainfrom
claude/issue-16530-mcp-oauth-resource-registration
Sep 8, 2026
Merged

fix(plugin-auth): register the MCP resource so RFC 8707 authorize can succeed#16780
os-zhuang merged 8 commits into
mainfrom
claude/issue-16530-mcp-oauth-resource-registration

Conversation

@os-trump

@os-trump os-trump commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16530

MCP OAuth could not complete at all on 17.3.0. plugin-auth configured @better-auth/oauth-provider with validAudiences, an option the pinned 1.7.2 does not read. In 1.7.2 an RFC 8707 resource is resolved from the oauthResource table and enforcePerClientResources defaults to true, so a client must also be linked in oauthClientResource. Neither row was ever written, so every resource= request was refused at /oauth2/authorize. Six Connect attempts over three days, zero tokens minted.

Route taken: (a), the per-client check stays ON

resources: [mcpResourceUrl] seeds the sys_oauth_resource row from the provider's own init; clientRegistrationDefaultResources: [mcpResourceUrl] links each newly registered client inside the DCR transaction — the only place the link can be made, since an MCP client registers anonymously about a second before the browser login and no administrator can insert a row in between.

enforcePerClientResources is deliberately not passed, so it stays at its true default. Route (b) — switching that check off — would have made the same flow pass by relaxing "which client may hold this audience". It was not needed: (a) is achievable on 1.7.2, measured. A test asserts the check is still live by behaviour: a client whose link row is removed is still refused.

What that does and does not claim. The per-client check is untouched. The set of clients that end up holding the MCP audience is not, and those are two different sentences — the earlier wording, "the security boundary is untouched", ran them together and overstated the first. clientRegistrationDefaultResources links every dynamically registered client, and allowUnauthenticatedClientRegistration is on and unchanged, so after this PR "which client may hold the MCP audience" is "any client that registers". The per-client check goes on discriminating only for clients that did not arrive through DCR — administrator-created and trusted ones. That is the designed outcome for public MCP clients and it is what route (a) is: the user still consents per client, the minted token is bound to user, scopes and audience, and DCR is rate-limited. Route (b) would have been a different act — removing the check that still governs the non-DCR population.

The vendor reading, taken here rather than inherited

Triage stated verbatim that it could not verify this half — its environment had no node_modules/@better-auth/oauth-provider/dist/. Measured independently against the installed 1.7.2 (dist/, 17 files, 608,522 bytes — the earlier revision of this body said 18, a miscount; the byte total was right and the review's 17 is the correct file count). Counts below are OCCURRENCES, not matching lines, which is why clientRegistrationDefaultResources reads 5 here and 4 where lines were counted:

needle hits in 1.7.2 dist/
validAudiences 0
silenceWarnings 0
oauthAuthServerConfig 0
enforcePerClientResources 13 (positive control)
clientRegistrationDefaultResources 5 (positive control)
resourceSeedMode 4 (positive control)
oauthResource / oauthClientResource 22 / 19
invalid_target 29

The controls fire in the runtime .mjs, not only in the .d.mts type surface, so a zero is a reading and not a broken search. Widened to the whole pnpm store, validAudiences occurs in 0 files while clientRegistrationDefaultResources occurs in 2 — the same method, both answers.

silenceWarnings was removed on the same evidence: neither the option name, nor its oauthAuthServerConfig key, nor the notice text it claimed to suppress occurs anywhere in @better-auth/oauth-provider or better-auth 1.7.2. It was the identical dead-option shape as the defect this card is about.

Making the check able to fail, before fixing the code

The old assertion mocked oauthProvider, took mock.calls.at(-1)[0], and asserted that the options object we passed in contained validAudiences. The provider never consumed it, so it was green whether or not 1.7.2 read the option. It is gone. auth-manager.mcp-oauth-resource.test.ts boots a real authorization server from the exact options AuthManager produces and drives discovery → DCR → authorize?resource= → consent → token, and the surviving mock-side block is renamed to say what it can and cannot answer.

Ablation, on the committed tree — the fix reverted to its pre-fix shape, mutation proven on disk before the run (git hash-object 7368f501…61643efb…; anchors resources 1→0, clientRegistrationDefaultResources 1→0, validAudiences 0→1), restore proven by state afterwards (bytes match the HEAD blob, git diff HEAD empty). Over auth-manager.mcp-oauth-resource.test.ts:

× every option AuthManager passes occurs in the installed provider
× seeds the MCP resource as an oauthResource row at plugin init
× links a DCR-registered client to the MCP resource without an admin step
× does NOT answer invalid_target for authorize?resource=MCP_URL (the production symptom)
× mints a token whose audience is the MCP resource (discovery → DCR → authorize → consent → token)
× still refuses an UNREGISTERED resource at authorize — the fix registers ONE resource, it does not switch resource checking off
× refuses at /oauth2/token a resource that was not bound at authorize
Tests  7 failed | 2 passed (9)

The refusal it reproduces is the reported one, verbatim:

error=invalid_target&error_description=requested+resource+https%3A%2F%2Facme.example.com%2Fapi%2Fv1%2Fmcp+is+not+configured

The per-client guard stays green under that mutation, which is what distinguishes it from a route-(b) change. It and the two-way dist scan are the two survivors above.

The wrong-resource control is a DIFFERENTIAL, and that is the whole point

Before this PR the AS answered invalid_target for every resource, so a bare "an unregistered resource is refused" assertion is green on both sides of the fix and measures nothing. The control is therefore written as a differential: one booted AS, one DCR client, one session, two /oauth2/authorize requests that differ only in resource. The registered MCP resource must reach consent; https://acme.example.com/api/v1/other, which nothing ever registered, must answer invalid_target. A second control redeems a code bound to the MCP resource against that other identifier at /oauth2/token and requires invalid_target with nothing minted.

It asserts nothing about the options object. The resource inventory it checks at the end is read out of the running AS's own store — "assert the option we passed" is the shape this card exists to retire, and re-introducing it here would have re-introduced the defect one layer up.

Both refutation directions were run on the committed tree. Each leg proves its mutation on disk before the run (anchor grep -c plus git hash-object differing from the HEAD blob) and proves its restore by state afterwards (git checkout HEAD -- ABSPATH under a trap … EXIT INT TERM; blob back to the HEAD blob, git diff HEAD empty):

leg mutation of auth-manager.ts (HEAD blob 7368f501…) what the control does
removal — the pre-fix option shape 61643efb…; resources 1→0, clientRegistrationDefaultResources 1→0, validAudiences 0→1 RED on the granted half: the registered MCP resource must still be granted: expected 'http://localhost:56789/callback?error…' not to contain 'invalid_target' — 7 failed / 2 passed
over-broad seed — the "seeds by wildcard" edit: a second resource seeded and linked to every DCR client 0defb8ab…; resources 1→0, clientRegistrationDefaultResources 1→0, /api/v1/other 0→2 RED on the refused half: the AS handed …/api/v1/other off to /_console/oauth/consent instead of refusing it — 4 failed / 5 passed

The removal leg says the control cannot be vacuous. The over-broad leg says it is the specific registration being measured, not "resource checking is on somewhere" — and only that leg separates "we registered the MCP resource" from "the AS grants whatever it is asked for". Under the over-broad seed the token-leg control stays green, correctly: the code is still bound at authorize to the MCP resource alone, so the subset rule at redemption still refuses the other identifier. The per-client guard stays green there too — over-broad seeding is not the same act as switching the per-client check off, and the two controls separate those as well.

Table counts, before and after

Counted in the harness the same way the report counted them, keyed by the platform table names:

table before (ablated tree) after: at boot after: full flow
sys_oauth_resource 0 1 1
sys_oauth_client_resource 0 0 1
sys_oauth_access_token 0 0 0
sys_oauth_refresh_token 0 0 1

sys_oauth_access_token legitimately stays 0. In 1.7.2 isJwtAccessToken = audienceClaim && !opts.disableJwtPlugin, and only the opaque branch (createOpaqueAccessToken) persists a row. An MCP token carries an audience and the jwt plugin is on, so the access token is a signed JWT — the minted-token evidence is the JWT itself, whose aud the test asserts contains the MCP resource and whose iss is the issuer. The persisted evidence of a completed grant is the refresh row, which offline_access earns.

The end-to-end test starts from the RFC 9728 discovery document and never re-types the resource: it asserts the advertised resource is the same string the AS is seeded with, then drives authorize and token with that value. An AS that seeds one spelling while advertising another reproduces this defect class, and a flow that hard-codes the identifier cannot see it.

Two boot-path defects the seed uncovered

Seeding is the first write this package performs from a plugin init, and it made two latent problems reachable. Both are fixed here because this change is what makes them fire.

  1. betterAuth() returns before its plugin init hooks settle, running them behind auth.$context. Anything a plugin did at init was a promise nobody held: a failure escaped as an unhandled rejection — fatal to the process under Node's default — and, in tests, as a boot write racing its engine's teardown. createAuthInstance now awaits $context, so a boot failure rejects the call that asked for the instance. Measured: 68 unhandled rejections before, 0 after.

  2. The no-dataEngine development fallback passed better-auth no database, which makes it build an in-memory store keyed by the schema key while every read resolves by modelName. Measured on 1.7.2: getAuthTables() returns { oauthResource: { modelName: 'sys_oauth_resource' } } and the store gets the key oauthResource, so the adapter's request for sys_oauth_resource answers Model … not found. Every model this package renames was unreachable on that path — user/sys_user as much as the oauth ones. It stayed invisible only because nothing had ever touched a renamed model during boot. Production never reaches this branch; it returns the ObjectQL adapter factory above it.

Why the seed is safe at boot in production: sys_oauth_resource is registered by plugin-auth itself (authIdentityObjects), and the auth instance is built lazily on the first request, long after plugin registration. If the table has not been provisioned yet, the provider's own MISSING_TABLE_PATTERN matches the driver's "no such table" and defers to its lazy seed on first resource access.

Three existing checks were corrected rather than weakened:

  • The pin asserting database === undefined is replaced, not edited — it pinned exactly the branch removed, and it read the value passed rather than what that value does. Its successor drives the factory and asks the adapter for a renamed model.
  • signup-existing-address-refusal.test.ts drains boot writes before arming its insert recorder, so its "nothing was written, nothing was even attempted" assertion stays total and now really measures the sign-up.
  • The membership-policy-setting.test.ts double stopped filing every insert as a membership regardless of which object it named; a sys_oauth_resource row was arriving in _members as {organization_id: undefined, user_id: undefined}.

Verification

Run through scripts/pm/os-verify-lock.sh; verdicts read off the wrapper's own VERDICT line, exit codes captured before any pipe.

  • pnpm --filter @objectstack/plugin-auth test106 files, 2215 tests passed, VERDICT command-exit 0.
  • pnpm --filter @objectstack/plugin-auth typecheckVERDICT command-exit 0 (all three legs: tsc, the examples project, check:test-typecheck, the last reporting the test layer compiles under tsconfig.test.json with its ledger held).
  • pnpm --filter '@objectstack/plugin-auth^...' build and pnpm --filter @objectstack/plugin-auth build — both exit 0.
  • Gate families re-derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands after merging origin/main — the first derivation reported STALE TREE (50 commits behind, 19 files it derives from changed), so it was discarded rather than reported. Post-merge: 57 derived, 57 run, 0 UNRUN; the command list is byte-identical to the pre-merge one. 56 exit 0.
  • check:type-check-debt first came back exit 3 = PREREQUISITE NOT MET (four workspace dependencies had no built type entry point), so it was not reported as a pass: the four were built and it was re-run. It then OOM'd at the caller's --max-old-space-size=4096, which the gate itself names as the binding ceiling; re-run at 8192 it is exit 0 — 5 ledger entries re-measured in 85.1s, 55 raw tsc errors, none above its recorded number.
  • check:dual-build-cjs-loads remains exit 3 = PREREQUISITE NOT MET — NOT MEASURED, and explicitly not reported as a pass. It reads built output for 40 packages that have no dist/ here; satisfying it is a whole-repo pnpm build, which is CI's run, not a narrowing this PR can make.
  • check:route-envelope does not apply and was not run: it is in dispatch-gates' silent bucket, and this diff adds no routes module and contains zero c.json( / res.json( call sites.

Measurements above were taken at the head of this branch after the origin/main merge.

验收备注

分诊席 5578245700 与派发席 5578514682 的验收口径,逐条:

  1. 先修测试的可失败性,再修代码。 断言对象已从「我们传进去的 options」换成真实 provider 的行为:authorize?resource=MCP_URL 不再答 invalid_target。消融证明它能红(该文件 9 条中 7 条转红,含逐字复现的生产报错);并补一条错误 resource 的差分对照,两个方向各自跑过一腿。
  2. 端到端一次。 discovery → DCR → authorize → 同意页 → token 全程跑通,token 的 aud 指向 MCP resource;三张表修复前后行数见上表(含 sys_oauth_access_token 为何合法地保持 0 的机制说明)。
  3. 路线理由。 走 (a),enforcePerClientResources 保持 true,不放宽安全面;(b) 未被采用,因为实测 (a) 在 1.7.2 上可行。
  4. 清掉死选项。 validAudiences 已删;同一形态的 silenceWarnings 一并删除,依据是同一次带阳性对照的 dist 读数。

⛔ 不在本卡:CLI 启动横幅从监听端口拼 http://localhost:4001/api/v1/mcp —— 分诊席已单独归档,是 packages/cli 的独立面,本 PR 未触碰。

Contract review — patch round

Review: #16780 (comment) (isolated CONTRACT_REVIEW_TIER seat, verdict CHANGES REQUIRED at 5cd3b999a). Its security reading came back clean; two things were owed and one was recorded.

  • F1 — the two self-declarations contradicted. Fixed: the changeset is minor, see the Gate section below for the ruling applied. The review's aside that the LEVEL-axis gate could not see packages/plugins/* (its PUBLISHED_SOURCE_PATH regex) is that gate's card, [finding] The changeset LEVEL axis is blind to every NESTED package: packages/*/src/** matches one segment, so 51 of 74 workspace packages (all drivers/services/adapters) can pair Clause-②: yes with patch and stay green #16713, and is deliberately not touched here.
  • F2 — the claimed wrong-resource control was not tested. Added, as the differential above, with both refutation legs run.
  • F3 — recorded, no code. The route section above no longer says "the security boundary is untouched"; it says which sentence is true (the check is untouched) and which is not (the set of clients that can hold the audience), and states the DCR consequence outright.
  • F4 — informational. The one number the review could not reproduce is corrected above: the dist holds 17 files, not 18. The pnpm-store-wide reading it flagged is left as it was taken; its dist-level half reproduces exactly.

Gate

Clause-②: yes, re-derived from the actual surface rather than inherited. The accept set of /oauth2/authorize strictly grows — a request carrying resource=MCP_URL from a DCR-registered client moves from always-refused to accepted — and it is externally observable at a published protocol endpoint third-party clients consume. It is a pure widening: seeding by the string form sets allowedScopes: null, disabled: false, dpopBoundAccessTokensRequired: false, so nothing previously accepted becomes refused, and a request naming any other resource is still refused exactly as before — that last clause is no longer only a claim: it is the differential control above, red under an over-broad seed. The contract review has since run and concluded (CHANGES REQUIRED at 5cd3b999a, addressed above), so needs:contract-review is currently off both carriers; re-hanging it against this patched head is the PM seat's act, not this branch's. This PR stays in draft.

Changeset: minor on @objectstack/plugin-auth. It was patch, citing AGENTS.md:1029 ("a bug fix in a released package takes patch"), which contradicted this PR's own Clause-②: yes. The maintainer's ruling of 2026-09-04 (decision batch #35 on #15294, the WHICH LEVEL prose in pr-automation.yml) settles the order between those two rules: a purely additive widening of a published package's public surface — "a new accepted key or value" — takes at least minor, and the commit type "may raise a bump but never lower it below what the act requires". The accept set of a published endpoint grew and this body says so, so minor is the level. Not skip-changeset: this publishes. Nothing breaking, so no ADR-0087 marker is owed.

🤖 Generated with Claude Code

https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37


Generated by Claude Code

The predecessor check asserted `opts.validAudiences` on the options object
captured from a mocked `oauthProvider`. The provider never consumes that
object, so the assertion was green whether or not the installed version read
the option -- and 1.7.2 does not read it at all. An assertion that cannot
fail is indistinguishable from one that passed.

Replace it with checks whose subject is what the REAL provider does:

- an option-surface liveness scan over the INSTALLED provider dist, carrying
  a two-way control so a 0-hit reading is a measurement rather than silence;
- an end-to-end block that boots a real authorization server from the exact
  options AuthManager produces and drives discovery -> DCR ->
  `authorize?resource=<mcp url>` -> consent -> token;
- a guard that the per-client resource check stays ON, so satisfying the
  flow by switching a security check off turns this red instead.

This commit is deliberately red: it is the reproduction.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
… succeed

`@better-auth/oauth-provider` 1.7.2 resolves a requested `resource` from the
`oauthResource` table and, with `enforcePerClientResources` at its `true`
default, requires the client to be linked in `oauthClientResource`. Neither
row was ever written, so every MCP client that sends `resource=` was refused
at `/oauth2/authorize` with `invalid_target: requested resource <mcp url> is
not configured`. No token could be minted on 17.3.0.

Route (a): declare the resource rather than relax the check.

- `resources: [mcpResourceUrl]` seeds the sys_oauth_resource row from the
  provider's own `init`, idempotently and `insertOnly`, so an admin's later
  policy edits survive a restart.
- `clientRegistrationDefaultResources: [mcpResourceUrl]` links every newly
  registered client inside the DCR transaction -- the only place the link can
  happen, since a client registers anonymously about a second before login.
- `enforcePerClientResources` stays at its `true` default. A client with no
  link row is still refused, and a test asserts that.

Two dead options removed. Neither `validAudiences` nor `silenceWarnings`
occurs anywhere in the installed `@better-auth/oauth-provider` or
`better-auth` (0 hits each, against positive controls that fire), and the
`oauthAuthServerConfig` notice `silenceWarnings` claimed to suppress no
longer exists in 1.7.2 either. A field that is passed and read by nobody
looks like configuration and enforces nothing -- that is how this defect
survived a version bump, so the new option-surface liveness check refuses
any such field rather than allowlisting these two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…fallback by modelName

Registering the MCP resource made the oauth-provider seed a `sys_oauth_resource`
row from its plugin `init` — the first write this package ever performs during
better-auth construction. Two latent boot-path defects became reachable as soon
as it did, and both are fixed here:

* `betterAuth()` returns synchronously and runs plugin `init` behind
  `auth.$context`, so anything a plugin does at init was a promise nobody held.
  A failure escaped as an UNHANDLED REJECTION (fatal to the process by default)
  and, in tests, as a boot write racing its engine teardown. `createAuthInstance`
  now awaits `$context`, making the seed part of "the instance is ready" and a
  boot failure a rejection of the call that asked for it.

* The no-`dataEngine` fallback handed better-auth no `database` at all, which
  makes it build an in-memory store keyed by the schema KEY while every read
  resolves by `modelName`. Measured on better-auth 1.7.2: every renamed model —
  `user`/`sys_user` included, not just the oauth ones — answered
  "Model <name> not found" on that path. The fallback now builds the store
  itself, keyed the way the adapter reads it. Production is unaffected: it
  returns the ObjectQL adapter factory above this branch.

The pin that asserted `database === undefined` is replaced rather than edited —
it pinned exactly the branch this removes, and it read the value we passed
rather than what that value does. Its successor drives the factory and asks the
adapter for a renamed model.

Two suites had their measurement windows corrected, not their assertions
weakened: the sign-up refusal test now drains boot writes before arming its
insert recorder, and the membership-policy double stops filing every insert as
a membership regardless of which object it named.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/api/index.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/deployment/cli.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/deployment/self-hosting.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/deployment/tenancy-modes.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/getting-started/your-first-project.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/kernel/services-checklist.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/permissions/authentication.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/permissions/sso.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/protocol/kernel/http-protocol.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/protocol/objectui/actions.mdx (via /api/v1/auth (route, a path literal in buildPluginList))

4 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v14.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/releases/v15.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/releases/v16.mdx (via /api/v1/auth (route, a path literal in buildPluginList))
  • content/docs/releases/v17.mdx (via sys_oauth_resource (literal, a string literal in createAuthInstance; a string literal in createDatabaseConfig), /api/v1/auth (route, a path literal in buildPluginList))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 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; 100 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 — 12 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 a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9packageMentionDocs.

Which tree this was computed on

This run read content/docs from da728414bc6d90ae65f4a0d79e598deed4ff9349 — the merge of head 28c7da91ede7c30b04e37d07049873cc5f03297b into base a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9, 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 da728414bc6d90ae65f4a0d79e598deed4ff9349 && git checkout da728414bc6d90ae65f4a0d79e598deed4ff9349
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9 28c7da91ede7c30b04e37d07049873cc5f03297b && git checkout -B drift-repro a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9 && git merge --no-ff 28c7da91ede7c30b04e37d07049873cc5f03297b

node scripts/docs-audit/affected-docs.mjs --json a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9

⚠️ 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 a5d4e286b6dc85a52e9686052b060d7cdb7fbfe9 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Contributor

Contract review (CONTRACT_REVIEW_TIER, isolated seat) — PR #16780 @ 5cd3b999a

Verdict: CHANGES REQUIRED — the security reading is clean (no path accepts a token minted for resource X at resource Y; the per-client check is on and pinned by behaviour). What is required is small: the PR's two declarations about its own act contradict each other (Clause-②: yes and a patch changeset on the package that grew), and the negative control the body claims ("any other resource is still refused exactly as before") has no test.

Ruling implemented: n/a — no ## Ruling recorded exists on #16530 or on this PR (all 5 card comments and the PR thread read). The card is not a decision-box card: triage 5578245700 states verbatim 「此卡不入决策箱……第 3 条的两条路线是承接者带取舍陈述的工程选择,不是需要维护者裁的岔路」. The acceptance criteria the PR cites are a seat's (triage os-zhuang, dispatch os-trump), not a maintainer's. Against those four criteria the PR conforms: (1) the assertion subject moved from "the options we passed" to the real provider's behaviour, (2) end-to-end discovery → DCR → authorize → consent → token with before/after table counts, (3) route (a) with enforcePerClientResources left at its true default and the reason stated, (4) validAudiences removed. The one maintainer ruling that binds here is 2026-09-04 decision batch #35 on #15294 ("WHICH LEVEL") — see F1.

Everything below was read from refs/review/16780 (= 5cd3b999a) vs merge-base 941232040 with origin/main, and from the installed @better-auth/oauth-provider@1.7.2 dist (17 files, 608,522 bytes, in a sibling worktree's pnpm store). Nothing in the PR body was taken on trust.

Verification

  1. Diff vs merge-base — 7 files, +597/−51. .changeset/mcp-oauth-resource-registration.md (A), packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts (A, 417), auth-manager.mcp-oauth.test.ts (M), auth-manager.test.ts (M), auth-manager.ts (M, +99/−27), membership-policy-setting.test.ts (M), signup-existing-address-refusal.test.ts (M). Governed paths: NO — none of docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/** is touched. Non-comment code delta in auth-manager.ts is exactly: database: await this.createDatabaseConfig(); await auth.$context before returning the instance; silenceWarnings and validAudiences removed; resources: [this.getMcpResourceUrl()] + clientRegistrationDefaultResources: [this.getMcpResourceUrl()] added; the no-dataEngine fallback now returns a memoryAdapter factory keyed by modelName.

  2. Security (RFC 8707 authorize path).

    • (a) What is registered, under which identifier: one resource, identifier getMcpResourceUrl() = ${getCanonicalOrigin()}${basePath minus /auth}/mcp (auth-manager.ts:5984) — derived from configuration (config.basePath, canonical origin), not hard-coded. It is the same accessor the RFC 9728 document advertises (:6021) and the same one verifyMcpAccessToken matches aud against (:6069), so the three cannot drift. Vendor string-form seed (buildSeedRow, introspect-*.mjs:705): allowedScopes: null, disabled: false, dpopBoundAccessTokensRequired: false, TTLs null — the body's claim is accurate; resourceSeedMode defaults to "insertOnly" (:808) so an admin's later edits survive restarts.
    • (b) Audience binding downstream — intact, verified at four points. Authorize: resolveResourcePolicy (introspect-*.mjs:449) resolves each requested resource from the oauthResource table, refuses a miss (invalid_target … is not configured) and a disabled row, then — with enforcePerClientResources unset → {value: true, source: "default"} (:618) — calls assertClientLinkedToResources (:538), which refuses any client without an oauthClientResource row. Token (code exchange): requested resource must be ⊆ the resources bound at authorize (index.mjs:77-83 and introspect-*.mjs:1936-1945, invalid_target otherwise); refresh: same subset rule (:2132). Token use: verifyMcpAccessToken verifies iss = getAuthIssuer() and aud = getMcpResourceUrl() via jose.jwtVerify, fail-closed (auth-manager.ts:6067-6069); the only caller is packages/runtime/src/security/resolve-execution-context.ts:140, MCP dispatch path only. Existing negative pin for a token minted for a different audience is kept (auth-manager.mcp-oauth.test.ts:228). No path accepts a token for resource X at resource Y.
    • (c) No scope widening / consent bypass / redirect-URI change. With allowedScopes: null the vendor leaves effectiveScopes equal to the requested scopes (:484-486); the scopes option is unchanged; loginPage/consentPage unchanged and the e2e test still has to POST /oauth2/consent to obtain a code; validateClientRedirectUri is not on any changed path. clientRegistrationDefaultResources only adds the link row inside DCR (authorize-*.mjs:1664-1683) after getResource confirms the row exists and is not disabled.
    • (d) Vendor API exists at the pin. package.json pins @better-auth/oauth-provider 1.7.2 (exact). In that dist: validAudiences 0, silenceWarnings 0, oauthAuthServerConfig 0; enforcePerClientResources 13 (9 in runtime .mjs), clientRegistrationDefaultResources 4 (2 runtime), resourceSeedMode 4 (2 runtime), invalid_target 28. The provider constructor throws clientRegistrationDefaultResources resource <id> not found in resources if the two options disagree (authorize-*.mjs:4188), which the test's real-provider construction exercises. better-auth@1.7.2 exposes $context (dist/auth/base.mjs:46), exports ./adapters/memory, and @better-auth/core/db exports getAuthTables; its no-database fallback does key the memory store by schema key (dist/db/adapter-base.mjs:7-13, Object.keys(getAuthTables(options))), so the PR's second boot-path claim is a correct reading.
  3. Clause-②: derived independently — YES, agrees with the body. Before: every GET /oauth2/authorize?…&resource=<mcp url> was refused (sys_oauth_resource empty → invalid_target). After: the same request from a DCR-registered client is accepted (hands off to consent). Nothing previously accepted becomes refused (string-form seed sets no scope restriction, not disabled). It is externally observable at a published protocol endpoint; the carrier is on the PR and the card. ⚠️ One consequence the body understates — recorded as F3, not blocking.

  4. Changeset. @objectstack/plugin-auth: patch. FROM/TO: none stated and none owed — nothing authorable is removed or renamed; the changeset body says no configuration change is required. check-changeset-no-major (head-tree copy, run offline with the carrier label and the body's Clause-② line as the event payload): no major; LEVEL AXIS reads "declares clause-② yes, and no package whose packages/*/src/** it moves is graded patch" — that green is a blind spot, not a clearance: see F1.

  5. Tests. Pin that reddens on revert: does NOT answer invalid_target for authorize?resource=<mcp url> and mints a token whose audience is the MCP resource boot the real provider from the options AuthManager produces; both necessarily fail with resources/clientRegistrationDefaultResources absent (the resource row never exists). Negative control present: keeps the per-client resource check ON — an unlinked client is still refused (deletes the link row, asserts invalid_target) — this is the route-(a)/route-(b) discriminator and it is real. Negative control missing: wrong resource — see F2. Typecheck: tsconfig.test.json includes src/**/*, and typecheck runs check:test-typecheck --project tsconfig.test.json, so the new file is in the program; CI Type Check · workspace/source/consumer/debt ledger all green. No .skip/.only/.todo in any changed test. The three corrected pre-existing tests were read: the database === undefined pin is replaced by one that resolves a renamed model through the produced adapter; signup-existing-address-refusal now builds the instance before arming its recorder (the assertion stays total); membership-policy-setting's double is scoped to 'sys_member', which is the object name the manager actually uses.

  6. CI on head 5cd3b999a: 49 check runs — 36 success, 13 skipped, 0 failure/pending (skipped = duplicate Auto Label / Check PR Size runs on label events, Packed-tarball opt-in, Build Docs, Console Pin Gate). mergeable_state: clean; 8 commits behind origin/main; draft. No new *-routes.ts is added, so check-route-envelope owes nothing (Lint & Repo Gates green).

Findings

F1 — The PR's two self-declarations contradict, and the gate that exists to catch exactly this could not see the package. The body declares Clause-②: yes on the ground that "the accept set of /oauth2/authorize strictly grows"; the changeset grades the package that grew patch, citing AGENTS.md:1029 ("a bug fix in a released package takes patch"). The maintainer's ruling of 2026-09-04 (batch #35, #15294, the "WHICH LEVEL" prose in pr-automation.yml:667) settles the order between those two rules: a purely additive widening of a published package's public surface — "a new accepted key or value" — takes at least minor, and the commit type "may raise a bump but never lower it below what the act requires"; check-changeset-no-major.mjs (#16055) mechanizes "declared clause-② + patch on the grown package = self-contradiction → refuse". It stayed green here only because its PUBLISHED_SOURCE_PATH = /^packages\/([^/]+)\/src\// does not match packages/plugins/plugin-auth/src/** — the package was never recognised as grown, so the axis had nothing to grade. Expectation: make the two declarations agree. If the act is the widening the body says it is, grade @objectstack/plugin-auth minor. If the author's position is that this is a pure restoration of an already-advertised contract and not a widening, then the Clause-② line must read no and the carrier should not have been hung — but that is the weaker reading: the accept set of a published endpoint did grow, and the body says so. (The regex blind spot for packages/plugins/* and packages/adapters/* is a separate card for the gate, not this PR's to fix.)

F2 — The claimed negative control for a wrong resource is not tested. The body's Clause-② derivation states "a request naming any other resource is still refused exactly as before"; the new test file's authorizeWithResource(auth, clientId, cookie, resource = MCP_RESOURCE) takes a resource parameter that no test varies (calls at lines 306, 340, 410 all pass the MCP resource). Under a future edit that seeds by wildcard or disables the resource lookup, the suite would stay green. Expectation: one it that drives authorize?resource=${BASE_URL}/api/v1/other (unregistered) against the real provider and asserts invalid_target in the redirect; optionally the same at /oauth2/token with a resource not bound at authorize, which the vendor refuses at index.mjs:81.

F3 — Record, no action: for the DCR population, route (a) and route (b) are observationally the same. clientRegistrationDefaultResources links every dynamically registered client, and allowUnauthenticatedClientRegistration: true (unchanged) lets any client self-register. So "which client may hold the MCP audience" is, after this PR, "any client that registers" — the per-client check still discriminates only for non-DCR (admin-created/trusted) clients. This is the designed outcome for public MCP clients (the user still consents per client; the token is bound to user, scopes and audience; DCR is rate-limited), and it is the route the seat ordered — but the body's sentence "the security boundary is untouched" should be read as "the check is untouched", not "the set of clients able to obtain the audience is untouched". The changeset already says it accurately ("DCR-registered clients are linked to it").

F4 — Informational: what this seat could not measure. The body's "whole pnpm store: validAudiences in 0 files" reading was not reproduced (the dist-level zero was). The test-suite run (105 files / 2197 tests) and the ablation run were not re-executed here; the ablation's logic was verified by reading (the two failing pins cannot pass with the options absent).

Maintainer-only merge: not by governed path (none touched). The PR is held from the queue by the needs:contract-review carrier until a maintainer clears it, and it is a draft.


Generated by Claude Code

… changeset minor

Contract review of PR #16780 returned CHANGES REQUIRED on two points.

F1 — the changeset level. The PR declares `Clause-②: yes` (the accept set of
`/oauth2/authorize` grows) while grading `@objectstack/plugin-auth` `patch`.
The maintainer's 2026-09-04 ruling (decision batch #35, the WHICH LEVEL prose
in `pr-automation.yml`) settles the order between that and "a bug fix in a
released package takes `patch`": a purely additive widening of a published
package's public surface — "a new accepted key or value" — takes at least
`minor`, and the commit type never lowers the bump below what the act
requires. Graded `minor`.

F2 — the missing negative control. The body claimed "a request naming any
other resource is still refused exactly as before" and nothing tested it:
`authorizeWithResource`'s `resource` parameter was never varied. Without that
control a green suite cannot tell "the MCP resource is registered" from
"resource checking is off" — the same axis as this card's original defect,
where the assertion read the options we passed rather than what the provider
does.

The control is written as a DIFFERENTIAL against the real provider: one run,
one booted AS, one DCR client, one session, and two authorize requests that
differ only in `resource`. The registered MCP resource must reach consent; an
identifier that was never registered must answer `invalid_target`. That shape
reddens from both sides — remove the registration and the granted half fails,
seed the second resource and link clients to it and the refused half does —
where a bare refusal assertion would stay green under either. It asserts
nothing about the options object; the resource inventory it checks at the end
is read out of the AS's own store.

A second control covers the token leg: a code bound to the MCP resource at
authorize, redeemed with a `resource` the grant never carried, must be refused
`invalid_target` and mint nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026

Copy link
Copy Markdown
Contributor

Contract review (claude-fable-5-1, isolated seat) — PR #16780 @ 28c7da91e

Verdict: PASS. All four items of the first review (comment 5580320985, at 5cd3b999a) are discharged on the tree. The security direction was re-derived from the installed @better-auth/oauth-provider@1.7.2 dist rather than taken from the body, and it agrees with the body's corrected sentence. Nothing new is blocking.

Read from refs/review/16780 (= 28c7da91e) against merge-base c930f8597 with origin/main; the delta since 5cd3b999a is one content commit, 11854b605 (.changeset/mcp-oauth-resource-registration.md 1 line, auth-manager.mcp-oauth-resource.test.ts +120/−14), plus a merge of origin/main that changes nothing under packages/plugins/plugin-auth/ relative to the merge-base (git diff c930f8597 origin/main -- packages/plugins/plugin-auth/ is empty). Vendor dist read from a sibling worktree's pnpm store, read-only (this checkout has no node_modules).

Governed-surface check: NO. The 7-file diff is .changeset/mcp-oauth-resource-registration.md + six files under packages/plugins/plugin-auth/src/. None of docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/** is touched.

F1–F4

item status evidence
F1 — changeset level contradicted Clause-②: yes discharged .changeset/mcp-oauth-resource-registration.md:2 now reads "@objectstack/plugin-auth": minor (was patch; 11854b605). Consistent with pr-automation.yml:667-682 WHICH LEVEL ("a new accepted key or value takes at least minor; the commit type may raise a bump but never lower it"). No BREAKING banner and no ADR-0087 marker owed: the seed sets allowedScopes: null, disabled: false (vendor string-form seed), so nothing previously accepted is refused. check-changeset-no-major.mjs:822 PUBLISHED_SOURCE_PATH = /^packages\/([^/]+)\/src\// still cannot see packages/plugins/plugin-auth/src/**, so judgeLevel (:933-953) has an empty grown set here and its green is the #16713 blind spot, not a clearance — but Check Changeset is not among the seven required status checks on main, so it neither gates nor misgates this merge.
F2 — wrong-resource control missing discharged auth-manager.mcp-oauth-resource.test.ts:441-485 is a real differential: one booted AS (bootRealAuthorizationServer, :145), one DCR client (:448), one session (:450), two /oauth2/authorize requests through the same authorizeWithResource (:248) differing only in resourceMCP_RESOURCE must not contain invalid_target and must reach /oauth/consent (:458-460); UNREGISTERED_RESOURCE (:71, …/api/v1/other) must contain invalid_target and not reach consent (:462-475); the resource inventory is read from the AS's own store, not the options (:481-484). Token leg :487-522: a code bound to the MCP resource redeemed with resource=UNREGISTERED_RESOURCE must be non-200, error === 'invalid_target', no access_token — the vendor rule it exercises is index.mjs:81 (requestedResources.some(r => !boundResourceSet.has(r))invalid_target). Mutation reasoning, by reading: removal of resources/clientRegistrationDefaultResources empties the resource table, so the granted half (:459) reddens; an over-broad seed that also links the second identifier makes resolveResourcePolicy (introspect-*.mjs:449) resolve it and assertClientLinkedToResources (:538) pass, so the refused half (:468) and the inventory (:481) redden, while the token leg stays green because the code was bound at authorize to the MCP resource alone — exactly what the body reports. No .skip/.only/.todo/xit in any of the five changed test files.
F3 — "security boundary untouched" overstated discharged Body now says which sentence is true. Re-derived: resolveEnforcePerClientResources (introspect-*.mjs:619-625) returns {value: true, source: "default"} when the option is undefined; auth-manager.ts:3532-3533 passes resources and clientRegistrationDefaultResources and does not pass enforcePerClientResources (grep of the file at the ref: only comment mentions). resolveClientRegistrationResources (authorize-*.mjs:1664-1685) always merges opts.clientRegistrationDefaultResources into the link set after getResource confirms the row exists and is not disabled; called at :1957 for every client created through the provider's registration path. allowUnauthenticatedClientRegistration: dcr (auth-manager.ts:3541) is byte-identical to origin/main:3501; resolveDcrEnabled (:340) unchanged. So: the per-client check is untouched, and every client the provider registers now holds the MCP audience — the body's corrected sentence is true. Widening is bounded to exactly one identifier, getMcpResourceUrl(), the same accessor the RFC 9728 document (:6069) and verifyMcpAccessToken (:6117) use. This is route (a) as ordered in the dispatch (card comment 5578514682: route (a) preferred, route (b) is the maintainer floor); the card is not a decision-box card (triage 5578245700).
F4 — one number not reproduced discharged Measured here: 17 files, 608,522 bytes. Occurrences: validAudiences 0, silenceWarnings 0, oauthAuthServerConfig 0, enforcePerClientResources 13, clientRegistrationDefaultResources 5, resourceSeedMode 4, invalid_target 29 — the body's table reproduces exactly on the occurrence basis it now states.

Other checks asked of this seat

  • Boot path, await $context (auth-manager.ts:2438): a plugin-init failure previously escaped as an unhandled rejection — process-fatal under Node's default and unattributable; it now rejects the call that asked for the instance. Every caller is getOrCreateAuth() (:1236), which assigns this.auth only on success, so a failed build is retried rather than cached. The two boot-time callers in auth-plugin.ts are already wrapped: registerOidcDiscoveryRoutes is invoked as void …().catch(…) (:3090) and resolveInstantiatedSocialProviders sits in try/catch (:373-380); handleRequest (:5437) surfaces it as a request failure. Unprovisioned-table case: vendor MISSING_TABLE_PATTERN (introspect-*.mjs:757, /no such table|relation.*does not exist|table.*does(?: not|n't) exist/i) defers to the lazy seed at :833/:852. This is consistent with the fail-fast boot posture ADR-0115's amendment records (initPluginWithTimeout does not catch; bootstrap() rethrows) and strictly more attributable than before. Correct.
  • No-dataEngine fallback (auth-manager.ts:3851-3860): getAuthTables(options) keys → db[table.modelName ?? key] = []memoryAdapter(db)(options). The replacement pin (auth-manager.test.ts:344-370) drives the produced factory (capturedConfig.database(capturedConfig)) and asks for the renamed model sys_user, expecting null rather than Model … not found. Both dynamic imports resolve against declared exact deps (package.json:29 @better-auth/core 1.7.2, :40 better-auth 1.7.2, unchanged from main).
  • Test corrections — scoping, not loosening. signup-existing-address-refusal.test.ts:284 builds the instance before instrumentInserts, so the "nothing attempted" assertion stays total over the sign-up window (and only works because of the $context await). membership-policy-setting.test.ts:70-75 files only sys_member inserts and :267-268/:310-311 assert no sys_member insert plus _members empty; see N3.
  • CI on 28c7da91e: 43 check runs, all completed — 37 success, 6 skipped (Packed-tarball ×2, Auto Label, Check PR Size, Build Docs, Console Pin Gate), 0 failure, 0 in progress; combined status success (Vercel). All seven required contexts on main's ruleset are green on this head: TypeScript Type Check, Test Core, Dogfood Regression Gate, Build Core, Temporal Conformance, Lint & Repo Gates, Governed Surface Queue Guard. mergeable_state: blocked is branch protection, not a red or running check: the ruleset carries a merge_queue rule (squash, ALLGREEN), the PR is a draft with zero reviews, and the needs:contract-review carrier holds it out of the queue. Nothing red, nothing running.
  • Diff outside the stated surface: none. content/docs/releases/** untouched; the docs-drift bot's four release-page rows are advisory read-only hits, not edits.

New findings

  1. N1 (record)clientRegistrationDefaultResources links every client created through the provider's registration path, not only RFC 7591 DCR in the narrow sense: authorize-*.mjs:1957 calls resolveClientRegistrationResources for every registrationSource, and defaultResources (:1665) is merged in unconditionally; only the requested resources are gated on registrationSource === "dynamic". The body's "any client that registers" is the accurate sentence; the changeset's "DCR-registered clients" is slightly narrower than the mechanism. No action.
  2. N2 (observation) — one options-object assertion remains in the e2e test (auth-manager.mcp-oauth-resource.test.ts:356-359, opts.resources must contain the advertised identifier). It is a discovery↔seed drift check and is immediately backed by behaviour (:321 store row identifier, :374-375 authorize with the advertised string), so it is not the retired shape. No action.
  3. N3 (observation)membership-policy-setting.test.ts narrowed expect(engine.insert).not.toHaveBeenCalled() to "no sys_member insert + _members empty". The sibling test kept its total assertion by draining boot writes first (await manager.getAuthInstance()); the same move would have kept this one total too. The membership claim the test makes is still fully pinned. No action.
  4. N4 (observation)$context is a better-auth internal (dist/auth/base.mjs), guarded at auth-manager.ts:2438 by optional chaining so a mocked or host-supplied instance without it is a no-op. A future vendor bump that renames it silently reverts to the pre-PR posture; the test at auth-manager.mcp-oauth-resource.test.ts:186 also depends on it, so a rename would go red there. No action.
  5. N5 (observation) — what this seat did not do: re-execute the suite or the two ablation legs (CI's Test Core shards are the execution evidence; the mutation logic was verified by reading the vendor resolution path above). The dist was measured read-only from a sibling worktree's store.

Maintainer-only merge: no by governed path (none touched). The auth-boundary widening is bounded to one resource identifier, is the route the dispatch seat ordered, and the dispatch seat recorded route (b) — not (a) — as the maintainer floor. That said, this is the one paragraph in the PR that changes who can hold an audience (F3), so a maintainer reading that paragraph once before the needs:contract-review carrier is cleared is warranted even though no rule requires it.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review September 8, 2026 14:01
@os-zhuang
os-zhuang enabled auto-merge September 8, 2026 14:01
@os-zhuang
os-zhuang added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 142c01c Sep 8, 2026
48 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-16530-mcp-oauth-resource-registration branch September 8, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

3 participants