fix(oauth): reuse persisted DCR client instead of re-registering on every connect

[eoigal]

Problem

The OAuth server has been accumulating large numbers of orphaned Dynamic Client Registration (RFC 7591) records, with a near-100% orphan rate (no owner, no matching token, authorization never completed). The pattern is one client identity re-registering a fresh DCR client on essentially every connect instead of reusing a prior registration.

Root cause

MCPOAuthProvider is the active DCR provider (selected for the OAuth 2.1 authorization_code + PKCE defaults in wordpress-api.ts). Its ensureClientRegistration() gated registration only on the in-memory this.config.clientId:

if (this.config.clientId) { /* skip */ return; }
// ...goes straight to DCR /register

this.config.clientId is seeded solely from options.clientId / WP_OAUTH_CLIENT_ID at construction and is never read back from disk. readClientInfo was imported but never called. So any fresh process without an env client ID:

1. mints a brand-new DCR client via /register, and
2. overwrites the persisted client_info.json, orphaning the prior registration server-side.

If the browser flow is never completed (the common unexpected-re-prompt case), the result is a tokenless orphan. This happens on first run, on missing/expired tokens, and on every run for stateless/ephemeral hosts where the config dir doesn't persist.

Fix

Read the persisted client_info.json for this serverUrlHash before hitting /register, and reuse a stored client_id when present. Registration now runs only when no client has ever been registered for the server — making the path reuse-first.

Tests

New tests/unit/mcp-oauth-provider.test.ts (nock-based, matching the repo's existing pattern):

• stored client_id present → no /register call, stored id reused
• no stored client → exactly one /register call, result persisted to client_info.json
• absent/expired tokens with a stored client → reuse (re-auth, not re-register)
• explicit client_id provided → skips both reuse and registration

Full suite green: 212/212.

Note on the client_name fingerprint

This provider registers with client_name = "WordPress MCP Remote" (mcp-oauth-provider.ts:259). If orphaned records on the server show a different client_name, they originate from a different client identity (e.g. another component or an older release), not this code path — this PR fixes the bug pattern present here, which would otherwise keep producing "WordPress MCP Remote" orphans the same way. Any differently-named orphans should be confirmed separately.

Deliberately deferred (follow-up)

Cross-process registration locking (concurrent starts → at most one registration). The MCP path currently has only an in-process authPromise guard; the lockfile coordinator (coordination.ts) is wired only into the legacy provider. Reuse-first eliminates the steady-state re-registration entirely and narrows the remaining race to a single cold start with no stored client. Wiring the lock into the MCP path is a larger change touching the coordinator and is kept out of this PR per the repo's commit-discipline rule (every line traces to one goal).

Known limitations / follow-ups

These came out of review. None block the orphan fix this PR delivers; each is a separate, larger change:

• No self-heal for a stale client_id. After this change, a persisted client_id that the authorization server later rejects (invalid_client-class) is reused on every run and never re-registered — the only recovery is deleting client_info.json. Closing the loop needs invalid_client classification at token exchange (mcp-oauth-utils.ts currently keys only on HTTP status) plus a guarded single re-register (invalidateCredentials('client') → clear clientId → re-run ensureClientRegistration()), which is its own flow + test surface. Deferred to a follow-up alongside the cross-process lock.
• Version-scoped config dir causes once-per-upgrade re-registration. client_info.json lives under wordpress-remote-${VERSION}, so a release bump can't find the prior version's file and re-registers (orphaning the old client once per upgrade). This is pre-existing and not introduced here; this PR still cuts orphan creation from "every connect" to "once per version." If cross-version reuse is intended, the client_info path should drop the version segment — separate follow-up.

[Copilot]

Pull request overview

This PR fixes repeated OAuth Dynamic Client Registration (DCR) by making MCPOAuthProvider.ensureClientRegistration() reuse a previously persisted client_info.json entry (keyed by serverUrlHash) instead of registering a new client on each fresh process run, reducing server-side orphaned DCR records and unnecessary /register calls.

Changes:

• Load persisted client_id via readClientInfo(serverUrlHash) and short-circuit DCR when present.
• Add unit tests to verify reuse-first behavior, correct persistence on first registration, and explicit clientId bypass behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/lib/mcp-oauth-provider.ts	Reuses a persisted client_id before attempting dynamic registration.
tests/unit/mcp-oauth-provider.test.ts	Adds regression tests ensuring reuse-first behavior and correct registration/persistence semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[galatanovidiu]

Solid, well-scoped fix. The reuse-first logic correctly stops the per-connect re-registration, security is clean for a public client (token_endpoint_auth_method: 'none', so restoring only client_id is right), and it follows the repo's test conventions. Full suite green, 212/212.

One thing worth resolving before this becomes load-bearing: there is no self-healing for a stale persisted client_id. After this change, a client_id the authorization server later rejects is reused on every run and never re-registered. invalidateCredentials('client') already exists but has zero callers in src/, and invalid_client is classified nowhere, so no error path clears client_info.json at runtime. The only recovery is manually deleting the file — which is exactly the state the server-side orphan cleanup that motivates this PR will produce for tokenless clients. A guarded single re-register on an invalid_client-class rejection would close the loop; if that is out of scope, listing it as a known limitation next to the deferred cross-process lock is enough.

Two smaller points inline. None of this blocks merge.

Separate observation (not introduced by this PR): the auth config dir is version-scoped (wordpress-remote-${VERSION}). On a release bump, a client_info.json written by the prior version is no longer found, so the provider re-registers and orphans the old client once per upgrade. The fix still cuts orphan creation from "every connect" to "once per version," but if cross-version reuse is intended, the client_info path should not include the package version. Worth a follow-up.

[eoigal]

Thanks @galatanovidiu — all five addressed in 889b2e3:

Fixed in this PR (low-risk hardening):

• Non-string client_id guard (line 241): now typeof client_id === 'string' && client_id.length > 0, so a corrupt-but-JSON-valid file falls through to registration instead of promoting a bad value into the auth URL. Added a regression test ({ client_id: '' } → re-registers).
• Reuse before the OAUTH_DYNAMIC_REGISTRATION guard (line 247): kept intentionally, with a comment explaining why — the flag governs minting a new client, not reusing one already obtained; a persisted client_info.json is only ever the output of a prior successful registration, so reusing it isn't "performing registration."
• Test strength (test line 122): added nock.disableNetConnect() (interceptor mismatch now fails fast instead of hitting example.com), switched "registers once" to an exact call-count assertion, and added the corrupt-file fall-through case.

Deferred to follow-ups (documented in the PR description):

• No self-heal for a stale client_id — agreed this is the real gap. Closing it needs invalid_client classification at token exchange (mcp-oauth-utils.ts keys on HTTP status only today) plus a guarded single re-register, which is its own flow + test surface. Listed as a known limitation alongside the cross-process lock rather than expanding this PR's scope.
• Version-scoped config dir (wordpress-remote-${VERSION}) re-registering once per upgrade — good catch, pre-existing. Noted as a follow-up; this PR still cuts orphan creation from "every connect" to "once per version."

I'll open the follow-up for self-heal + version-independent client_info path next.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.