Skip to content

MCP OAuth Stage 5: withMCPAuth bearer-token guard (#95) - #134

Merged
heskew merged 5 commits into
mainfrom
feat/mcp-stage5-withmcpauth
Jun 30, 2026
Merged

MCP OAuth Stage 5: withMCPAuth bearer-token guard (#95)#134
heskew merged 5 commits into
mainfrom
feat/mcp-stage5-withmcpauth

Conversation

@heskew

@heskew heskew commented Jun 24, 2026

Copy link
Copy Markdown
Member

Stage 5 of the MCP OAuth epic (#86). A plugin-provided guard for app-owned MCP routes: it validates the bearer access token and returns the spec-mandated 401 + WWW-Authenticate: Bearer resource_metadata="..." so apps don't reimplement the (security-sensitive) RFC 9728 discovery contract. Bearer-token counterpart to withOAuthValidation.

Closes #95.

What it does

withMCPAuth(handler, options?) wraps an app MCP handler so every request must present a valid RS256 access token (minted by Stage 4's issuer) before the handler runs:

  • Verifies the Authorization: Bearer token: RS256 signature against the published JWKS (key selected by kid), exp/nbf, and aud bound to mcp.resource (RFC 8707).
  • On any failure returns 401 with WWW-Authenticate: Bearer resource_metadata="<issuer>/.well-known/oauth-protected-resource" (RFC 9728).
  • On success attaches request.mcp = { sub, client_id, aud, scope } and invokes the handler, returning its response verbatim.
  • Fails closed (MCP disabled / no token / no keys / malformed / bad claims). Header-only token extraction per RFC 6750 — query-string tokens are ignored.

Registration — the load-bearing detail (and a correction to the #95 example)

Harper core auth is a default-group middleware that consumes Authorization: Bearer and 401s any non-Harper token with WWW-Authenticate: Basic, which breaks MCP discovery. The wrapper supports both registration models so it always owns its route's response; docs lead with the first:

  • urlPath subroute (recommended): server.http(withMCPAuth(handler), { urlPath: '/mcp' }). In harper 5.1.x, routed dispatch (server/middlewareChain.ts buildRoutedChain) runs only the matched subroute's chain — core auth (default group) never runs for it, the same isolation /.well-known/* relies on. No runFirst needed.
  • Default-group fallback: server.http(withMCPAuth(handler, { path: '/mcp' }), { before: 'authentication' }). When the route shares the default chain with auth, path scopes the guard (other paths fall through) and before: 'authentication' runs it ahead of core auth — the static.ts precedent.

Note: the issue #95 example server.http('/mcp', withMCPAuth(h)) is incorrect for this Harper version — server.http is (listener, options) and needs the urlPath/ordering hint above. The plan's original runFirst: true framing was modeled on an older middleware chain; runFirst is not the mechanism here.

Changes

  • NEW src/lib/mcp/withMCPAuth.ts — the guard.
  • src/lib/mcp/tokenIssuer.tsverifyAccessTokenWithKeySet (kid selection: unknown kid throws, no kid uses the sole key; RS256-pinned to block alg confusion; aud+iss enforced).
  • src/lib/mcp/wellKnown.ts — export PRM_PATH (challenge URL stays in sync by construction).
  • src/types.tsMCPRequestClaims + Request.mcp.
  • Exports wired in src/lib/mcp/index.ts and the top-level src/index.ts.
  • Unit tests (withMCPAuth.test.js + verifyAccessTokenWithKeySet cases) and an integration test + fixture (mcp-auth.test.ts / mcp-auth-app) asserting the Bearer challenge survives core auth for both registration models (runs on CI — needs loopback aliases).
  • README + docs/configuration.md.

Testing

  • Unit: 686 pass, 0 fail. Build + lint + format clean.
  • Integration: added; runs on CI (loopback aliases aren't configured locally).

Cross-model review (pre-PR, HEG step 10)

Codex + Harper-domain reviewer (Gemini/agy leg skipped — exfiltration-blocked for this repo; CI's gemini bot covers it). No blockers. One fix applied before this PR: onAuthError's fallback used ??, but the documented contract promises any falsy return fails closed — changed to || and added a falsy-return regression test. No-action notes: the deny-path resource_metadata URL derives from the Host header when mcp.issuer is unset (already documented as "pin mcp.issuer in production"; the fixture pins it); the undefined-pathname default-group fall-through is benign.

🤖 Generated with Claude Code

@heskew
heskew requested a review from kriszyp June 24, 2026 03:01
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Suggestions (non-blocking)

  • src/lib/mcp/withMCPAuth.ts:251 — consider extracting the deny response construction to a helper if more rejection branches are added in the future, though it's currently clean enough as a local closure.
  • src/lib/mcp/withMCPAuth.ts:167 — verify if request.pathname is always segment-normalized in Harper v5 (e.g., whether /mcp/ and /mcp both reach this point). The pathOwned helper handles trailing slashes, but consistent normalization at the entry would be even more robust.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@jcohen-hdb

Copy link
Copy Markdown
Member

Real-world validation: withMCPAuth works end-to-end against a live Claude.ai remote connector (Harper 5.1.x) 🎉

Pulled this branch into a staging deploy and connected it as a custom remote MCP connector from Claude.ai (full browser OAuth: discovery → DCR → Google → consent → token → tool list). The connector authenticated and listed all 17 of the app's tools — the bearer guard validated a real Claude-issued token cleanly.

Registration that worked: the urlPath subroute model, exactly as your module header recommends:

server.http(withMCPAuth(handler, { getConfig, logger, onAuthError }), { urlPath: '/mcp' });

Confirmed the key behavior: POST /mcp with no/invalid token returns 401 WWW-Authenticate: Bearer resource_metadata="…" (not core auth's Basic) — the routed-dispatch isolation holds, and the RFC 9728 loop closes. The onAuthError deny reasons were spot-on ("missing or malformed Authorization: Bearer header" during discovery; "…not issued for this resource" for a stale cached token).

One scenario worth a doc note — cross-component usage. Our setup differs from the integration fixture: the component that declares @harperfast/oauth (and owns the host-global routes / token issuance) is separate from the app component that exposes the MCP tools. To make withMCPAuth work from the consumer component, two things mattered:

  • getConfig had to be injected. OAuthResource.mcpConfig is a module-local static, and the consumer resolves its own node_modules copy of the package, so mcpConfig is undefined there. We injected getConfig returning { enabled: true, issuer, resource } with the issuer pinned to the provider's mcp.issuer so the iss/aud checks match the minted tokens.
  • The default MCPKeyStore worked as-is — it reads databases.oauth.harper_oauth_mcp_keys, which is cluster-global, so the consumer verifies against the same JWKS the provider mints with. 👍
  • Importing withMCPAuth as a function (no config.yaml declaration in the consumer) correctly does not spin up a second plugin instance.

Might be worth a short "consuming withMCPAuth from a different component than the plugin" section in the docs (inject getConfig; keys come from the shared oauth DB). Or, if you'd prefer consumers not hardcode the issuer, withMCPAuth could optionally resolve config from the published AS metadata as a fallback.

Test caveat: we ran this combined with the Harper-5.1.x fixes in #133 (well-known relative-urlPath + {status,body} envelope) plus a path-aware RFC 9728 PRM (Claude fetches /.well-known/oauth-protected-resource/mcp, the resource-path-appended form). On a 5.1.x base without those, discovery 404s before the token step — so #134 lands cleanest on top of #133. Happy to share our combined branch or more logs if useful.

@heskew

heskew commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

Updated now that #133 is merged. Two changes:

  1. Rebased onto main (picks up fix: MCP OAuth endpoints broken on Harper 5.1.x (well-known 404 + response envelope) #133's Harper-5.1.x discovery fixes + path-aware PRM, and the chore(ci): bump ai-review-prompts to 1bbc562 (week-of-06-15 calibration + log-count fix) #135 AI-review-prompts bump). Clean rebase — wellKnown.ts auto-merged (MCP OAuth Stage 5: withMCPAuth bearer-token guard (#95) #134's export PRM_PATH + fix: MCP OAuth endpoints broken on Harper 5.1.x (well-known 404 + response envelope) #133's path-aware handler).
  2. Closed the challenge-emit half (d899d8a): withMCPAuth's WWW-Authenticate: Bearer resource_metadata="…" now points at the RFC 9728 §3.1 path-appended PRM (<resource-origin>/.well-known/oauth-protected-resource/<resource-path>) that fix: MCP OAuth endpoints broken on Harper 5.1.x (well-known 404 + response envelope) #133 serves — via a new exported protectedResourceMetadataUrl() shared as the single source of truth with the serving side. Also added a "using withMCPAuth from a different component than the plugin" docs section (inject getConfig; keys come from the shared oauth DB) per @jcohen-hdb's field report.

Cross-model review of the emit delta (Codex + Harper-domain pass; Gemini skipped — exfiltration-blocked, CI bot covers): no blockers, no significant concerns. Confirmed emit==serve across edge cases (trailing slash, multi-segment, root, query/fragment), fail-closed preserved, no audience-binding drift. One pre-existing config-hygiene note (schemeless mcp.issuer — already documented as "pin to a full origin in prod"), not introduced here.

Tests: 701 total, 699 pass, 2 skipped. Build/lint/format clean.

With #133 merged, the discovery loop now closes end-to-end (serve + emit). Ready to flip out of draft once CI is green.

@heskew
heskew marked this pull request as ready for review June 30, 2026 00:09
heskew added a commit that referenced this pull request Jun 30, 2026
)

Harper populates `pathname` on the runtime request, but the plugin's
`Request` interface (extends IncomingMessage) didn't declare it, so
middleware reads it via `(request as any).pathname`. Declare the optional
field so middleware can use `request.pathname` without the cast.

Type-only, no behavior change. The cast removal itself lives in
withMCPAuth (#134, still open) — this field lets that (and future
middleware) drop the `as any`.

Closes #137.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
heskew and others added 3 commits June 29, 2026 20:31
Plugin-provided guard for app-owned MCP routes. Validates the
`Authorization: Bearer` access token (RS256 signature against the
published JWKS selected by `kid`, `exp`/`nbf`, and audience binding to
`mcp.resource` per RFC 8707) and, on any failure, returns the
spec-mandated `401 + WWW-Authenticate: Bearer resource_metadata="..."`
(RFC 9728) that closes the MCP discovery loop. On success it attaches
`request.mcp = { sub, client_id, aud, scope }` and invokes the wrapped
handler verbatim. Fails closed (MCP disabled / no token / no keys /
malformed). Header-only token extraction (RFC 6750); query-string tokens
are ignored.

Registration model (the load-bearing detail): Harper core auth is a
default-group middleware that 401s a non-Harper bearer token with
`WWW-Authenticate: Basic`, breaking discovery. The wrapper supports both
registrations, documented with urlPath as primary:
  - urlPath subroute (recommended) — routed dispatch isolates the chain,
    so core auth never runs for the route (same as /.well-known/*).
  - default group with `{ path }` + `before: 'authentication'` — the
    wrapper scopes to its path and runs ahead of core auth (static.ts
    precedent). Note: the #95 example registration is wrong for this
    Harper version (server.http takes (listener, options)); docs correct
    it.

- tokenIssuer.ts: add verifyAccessTokenWithKeySet (kid selection; unknown
  kid fails; no-kid uses the sole key; RS256-pinned; aud+iss enforced).
- wellKnown.ts: export PRM_PATH so the challenge URL stays in sync.
- types.ts: add MCPRequestClaims + Request.mcp.
- Exports from src/lib/mcp/index.ts and the top-level src/index.ts.
- Unit tests (withMCPAuth + verifyAccessTokenWithKeySet) and an
  integration test asserting the Bearer challenge survives core auth for
  both registration models.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that #133 serves the RFC 9728 §3.1 path-appended PRM
(/.well-known/oauth-protected-resource/<resource-path>), close the emit
half: withMCPAuth's WWW-Authenticate challenge must point at that same
URL, not the bare host-root form, so a client honoring resource_metadata
verbatim fetches a document the server actually answers.

- wellKnown.ts: add+export protectedResourceMetadataUrl(req, cfg) — the
  canonical PRM URL for the configured resource (origin + well-known +
  resource path; bare for a root resource). Single source of truth shared
  with the serving side.
- withMCPAuth.ts: build the challenge from protectedResourceMetadataUrl
  (guarded so the deny path never throws). Drop the PRM_PATH import.
- Tests: expected challenge is now the resource-derived path-appended URL;
  add a root-resource (bare) case. Integration test asserts the appended
  form.
- Docs: challenge-URL description updated to the path-aware form; add a
  "using withMCPAuth from a different component than the plugin" section
  (inject getConfig; keys come from the shared oauth DB) per jcohen's
  field report.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`request.pathname` is declared on the Request interface as of #138 (merged),
and `request.mcp` is declared in this PR — so the `(request as any)` casts
reading/writing them are no longer needed. Use the typed fields directly.

(The remaining `request as any` casts at the resolveResource/resolveIssuer/
protectedResourceMetadataUrl call sites bridge to wellKnown's HarperRequest
shape — a separate type-unification concern, left as-is.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@heskew
heskew force-pushed the feat/mcp-stage5-withmcpauth branch from d899d8a to 330a418 Compare June 30, 2026 02:33
…CORS

Resolves the two blockers + two suggestions from the gemini-review bot on
this PR:

- Header-parameter injection (blocker): `protectedResourceMetadataUrl`'s
  fallback could interpolate a raw, client-controlled Host into the quoted
  `resource_metadata="..."` challenge param. Every branch now normalizes
  through `new URL().origin` (which rejects/encodes a `"`/control char), with
  a host-less relative `PRM_PATH` as the final always-safe fallback.
- Path-length limit (blocker): enforce the repo's ≤2048-char path invariant
  (CLAUDE.md / OAuthResource.parseRoute) in withMCPAuth too, since it can be
  registered outermost and bypasses parseRoute. Fail closed before token work.
- CORS on the deny 401 (suggestion): add Access-Control-Allow-Origin + expose
  WWW-Authenticate so browser MCP clients can read the challenge cross-origin
  (parity with the well-known discovery docs).
- Array Host header (suggestion): resolveIssuer takes the first value when
  `headers.host` is an array.

Tests added for each. Full suite 703 pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the withMCPAuth bearer guard fails closed throughout (missing / malformed / expired / wrong-scheme all denied), verification is pinned to RS256 (no alg-confusion), and the deny paths are well covered by tests. A few minor suggestions in the thread, none blocking. Nice work @heskew!

— 🤖 KrAIs (Kris's review assistant)

@heskew heskew left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One fail-closed edge worth closing:

withMCPAuth() defaults to OAuthResource.mcpConfig as its config source, but updateConfiguration() only refreshes that static through OAuthResource.configure() when at least one provider is valid. If a live config reload removes/invalidates all providers, the plugin swaps in the "No valid OAuth providers" resource but leaves the previous OAuthResource.mcpConfig in place. I reproduced that by starting with mcp.enabled: true, reloading to providers: {}, and observing OAuthResource.mcpConfig still report the old enabled config.

That means a same-component withMCPAuth(handler) route can keep issuing Bearer challenges and verifying tokens against stale MCP config after the plugin config no longer has a valid provider set. Please clear/update OAuthResource.mcpConfig on every config update, including the zero-provider branch, or make the default getter fail closed when provider configuration is invalid.


🤖 Posted by Codex (gpt-5.5) on Nathan's behalf

…providers

withMCPAuth's default config getter (and the well-known handlers) read the
OAuthResource.mcpConfig static. That static is written only by configure(),
which runs only in updateConfiguration()'s >=1-provider branch. The
zero-provider branch swapped in the 503 error resource but left the previous
mcpConfig in place — so a live reload that removed all providers left the MCP
surface verifying tokens / serving discovery against stale, no-longer-valid
config. Fail closed by clearing the static in the zero-provider branch.

Adds a regression test that reproduces the repro (enabled + provider, then
reload to providers: {}) and asserts mcpConfig is cleared.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@heskew
heskew merged commit 13f46e5 into main Jun 30, 2026
10 checks passed
@heskew
heskew deleted the feat/mcp-stage5-withmcpauth branch June 30, 2026 16:27
heskew added a commit that referenced this pull request Jun 30, 2026
Adds the app-author documentation for MCP OAuth, with docs/mcp-oauth.md as
the single deep guide (flow diagram, endpoint reference, the withMCPAuth
wrapper + both registration models + options + cross-component use, the
onMCPTokenIssued hook, audit events, production-deployment checklist,
troubleshooting, and a hand-rolled-server migration guide). Links the MCP
spec (2025-06-18) and RFCs 6749/6750/7591/7636/8252/8414/8707/9728.

Rebased onto main after #134 merged. #134 had already added withMCPAuth
sections to README and configuration.md; those are trimmed here to short
pointers into docs/mcp-oauth.md so the wrapper isn't documented in three
places. configuration.md also: drops the "(work in progress)" marker,
documents signingKeyPem/signingAlgorithm/accessTokenTtl/refreshTokenTtl,
and fixes the JWKS note. docs/lifecycle-hooks.md documents onMCPTokenIssued.

Documents onMCPTokenIssued (#141), so merge this after #141.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the new commit closes a real fail-open hole: a live config reload to zero providers previously left the stale enabled:true mcpConfig in place, so the MCP surface kept verifying tokens / serving discovery. Nulling the static in the zero-provider branch makes the guard's first gate deny everything. Re-checked the auth invariants: still fails closed at every branch, RS256 pinning untouched, no deny path weakened, and the regression test exercises the real config-listener path. The 3 earlier suggestions (key-store hoist, default-group next doc, payload: any cast) are cosmetic — fine to defer. Nice catch @heskew!

— 🤖 KrAIs (Kris's review assistant)

heskew added a commit that referenced this pull request Jun 30, 2026
This branch changed signAccessToken to return { token, jti } (Stage 6 needs the
jti for audit/hook). #134's withMCPAuth + verifyAccessTokenWithKeySet tests, now
on the branch via the merge of main, still treated the return as a token string —
passing the whole { token, jti } object where a string was expected → the JWT
verifier saw "malformed token" (8 failures, surfaced only in CI's PR-merge build).

Destructure { token } in the verifyAccessTokenWithKeySet tests and return .token
from withMCPAuth.test.js's mint(). Production callers (token.ts) already use the
new shape. Full unit suite green on Node and Bun; lint + format clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
heskew added a commit that referenced this pull request Jun 30, 2026
* feat(mcp): Stage 6 — audit logging + onMCPTokenIssued hook

Adds two audit events and a lifecycle hook for MCP token issuance,
completing the audit trail for the MCP OAuth flow (issue #96, parts 1–2).

- src/lib/mcp/audit.ts: new shared helper emitting structured audit records
  via Harper's global logger (harperLogger.info) with a `MCP audit:` prefix.
  Emits oauth.mcp.token.issued and oauth.mcp.token.refreshed; general enough
  for the deferred oauth.mcp.token.rejected event (Stage 5 / PR #134) to
  call it later. Never includes token strings — only jti, client_id, sub,
  aud, scope, timestamp.

- src/lib/mcp/tokenIssuer.ts: signAccessToken now returns { token, jti }
  instead of a bare string so callers capture the jti for audit/hook events
  without decoding the signed JWT. Accepts an optional caller-supplied jti
  param; generates a fresh UUID when omitted (preserving existing behaviour
  for callers that don't need the id).

- src/lib/mcp/token.ts: mintTokenPair and handleRefreshTokenGrant emit the
  audit event and call onMCPTokenIssued after the JWT is signed, before the
  response is returned. Both accept an optional HookManager; failures are
  fire-and-forget (caught by HookManager.callOnMCPTokenIssued, not re-thrown).
  handleToken signature gains an optional hookManager param.

- src/lib/mcp/index.ts: handleMCPPost threads the HookManager through to
  handleToken so the live request path delivers the hook.

- src/lib/resource.ts: passes OAuthResource.hookManager to handleMCPPost.

- src/types.ts: OAuthHooks gains onMCPTokenIssued. Signature mirrors the
  issue spec: { type: 'access' | 'refresh', client_id, sub, aud, scope, jti }.

- src/lib/hookManager.ts: callOnMCPTokenIssued mirrors callOnLogin /
  callOnTokenRefresh — try/catch logs via this.logger?.error, never re-throws.

Part 3 finding: onLogin already fires on MCP-initiated auth. handlers.ts
calls hookManager.callOnLogin before the MCP branch decision (line 248),
so the hook runs for both human-session and MCP-bridged flows. Added a
regression test asserting callOnLogin is called on the MCP path.

Deferred: oauth.mcp.token.rejected is NOT implemented here — it lives in
withMCPAuth (Stage 5, unmerged PR #134). The audit helper is ready for it.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(test): make JWKS tests order-independent under Bun's shared process

Bun runs all test files in one process (Node isolates per file). Stage 6's
new tokenAuditHook.test.js mints a signing key into the MCPKeyStore module
cache; wellKnown's JWKS 'empty key set' tests, which never reset that cache,
then saw the leaked key and failed under Bun's file order (passed under Node
and under the local Bun's different order — order-dependent flake).

- wellKnown.test.js: resetMCPKeysTableCache() in a top-level beforeEach so the
  JWKS assertions are order-independent regardless of any prior file's key.
- tokenAuditHook.test.js: reset the cached table refs in after() so the file
  cleans up after itself.

Reproduced by forcing the file order (tokenAuditHook then wellKnown → 2 fails);
green after the fix. Full bun test + node --test both pass (694).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): callOnMCPTokenIssued catch handles non-Error throws

A hook that throws a non-Error (null/undefined/string) made the catch's
`(error as Error).message` throw inside the catch and escape — breaking the
documented fire-and-forget contract (a throwing hook must never block token
issuance). Use `error instanceof Error ? error.message : String(error)`, with
a test that throws null. The sibling HookManager methods share the pattern —
tracked repo-wide in #142.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): handleToken returns server_error on unexpected failure

Per the gemini review: handleToken lacked a top-level try/catch, so an
unexpected throw (jwt.sign failure, a store timeout, etc.) propagated to the
framework — violating RFC 6749 §5.2 (the token endpoint must return a
structured JSON error) and risking leaking internal detail via the default
500 handler. Wrap the dispatch in try/catch returning a 500 server_error;
`return await` the grant handlers so their rejections are caught.

Interacts correctly with the Stage 6 audit/hook reorder: a refresh-family
persistence failure (auth-code path) and a signing failure (refresh path) now
surface as a clean server_error with NO phantom audit event / hook fire and no
family rotation. Updated the two affected tests to assert the 500 response
instead of a propagated throw. Hook-execution timeout tracked separately in
#143; the sibling HookManager catch blocks in #142.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): make onMCPTokenIssued fire-and-forget (not awaited); harden sibling catches

Address @kriszyp's review on #141: onMCPTokenIssued was awaited on the issuance
path, adding latency. It now runs detached (Promise.resolve().then(...).catch(...))
so it never delays or blocks token issuance. This subsumes #143 — a hook-execution
timeout only existed to bound how long issuance waits on the hook; not awaiting
removes that wait entirely. Drop the now-redundant await at both token.ts call sites.

Also folds in #142 (same file): apply the `error instanceof Error ? error.message :
String(error)` guard to the older catch blocks (callOnLogin / callOnLogout /
callOnTokenRefresh / callResolveProvider) so a non-Error throw can't crash the catch.

Tests: the hook is now detached, so tests flush microtasks before asserting; adds a
test proving a slow hook never blocks issuance; removes the now-unused
eslint-disable in the throw-null test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(mcp): reconcile token-signing API after merging #134

This branch changed signAccessToken to return { token, jti } (Stage 6 needs the
jti for audit/hook). #134's withMCPAuth + verifyAccessTokenWithKeySet tests, now
on the branch via the merge of main, still treated the return as a token string —
passing the whole { token, jti } object where a string was expected → the JWT
verifier saw "malformed token" (8 failures, surfaced only in CI's PR-merge build).

Destructure { token } in the verifyAccessTokenWithKeySet tests and return .token
from withMCPAuth.test.js's mint(). Production callers (token.ts) already use the
new shape. Full unit suite green on Node and Bun; lint + format clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): shield the detached onMCPTokenIssued catch from a throwing logger

Addresses the claude-review blocker on #141: callOnMCPTokenIssued now runs the
hook on a detached (void) promise chain, so if the `.catch` body itself throws —
a logging-subsystem I/O error, or a malicious `error.toString()` — it becomes an
unhandled rejection (process crash on Node >=15). Wrap the catch body in try/catch
(same best-effort posture as emitMCPAuditEvent). Adds a regression test (throwing
hook + throwing logger → no escape).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(mcp): emit oauth.mcp.token.rejected on withMCPAuth denial paths (#96)

Stage 6 acceptance requires audit coverage for all three event types; the
rejected event was declared but never emitted (the #96 gap @heskew flagged on
#141). withMCPAuth now emits oauth.mcp.token.rejected when a *presented* bearer
token fails validation (bad signature / expired / wrong aud / no keys / malformed
claims) — but NOT for missing-token probes or the pre-token guards (MCP disabled,
path-length), which are unauthenticated noise / DoS surface, not rejected tokens.

A rejected token has no verified claims, so MCPAuditPayload is now a discriminated
union: the rejected variant carries only { reason, aud, timestamp } — never an
unverified client_id/sub/jti (those would be attacker-controlled). Emission goes
through an injectable emitAudit option (defaults to emitMCPAuditEvent), mirroring
the existing getConfig/keyStore/logger DI.

Tests: rejected payload shape (audit.test.js); emitted on invalid + wrong-aud
tokens, NOT on missing-token / disabled / oversized-path (withMCPAuth.test.js).
Node 739/737 pass, Bun 737, lint + format clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): shield the emitAudit call in withMCPAuth's deny path

Codex review on #141: emitAudit is part of the exported WithMCPAuthOptions, so a
custom audit sink that throws would turn an invalid-token denial into a rejected
promise / framework 500 — but deny() must never throw (fail closed → always 401).
The hook's catch was shielded in fa19302; this sibling call site (added in
b487aeb, with a comment that wrongly claimed it couldn't break the 401 path) was
missed. Wrap emitAudit in a best-effort try/catch, matching emitMCPAuditEvent.

Regression test: a throwing emitAudit sink still yields 401 + the Bearer challenge.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(mcp): use instanceof Error in token.ts + withMCPAuth.ts catch blocks

gemini review on #141 flagged (error as Error).message in withMCPAuth's key-load +
verify-failure paths (and token.ts's code-consume catch): a non-Error throw would
crash the catch via .message. Apply the same instanceof-Error guard already used in
hookManager.ts (#142) to the four catch sites in the two files this PR already
modifies. The same pattern remains in untouched MCP files (dcr/authorize/wellKnown/
callback) — tracked as a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
heskew added a commit that referenced this pull request Jun 30, 2026
Adds the app-author documentation for MCP OAuth, with docs/mcp-oauth.md as
the single deep guide (flow diagram, endpoint reference, the withMCPAuth
wrapper + both registration models + options + cross-component use, the
onMCPTokenIssued hook, audit events, production-deployment checklist,
troubleshooting, and a hand-rolled-server migration guide). Links the MCP
spec (2025-06-18) and RFCs 6749/6750/7591/7636/8252/8414/8707/9728.

Rebased onto main after #134 merged. #134 had already added withMCPAuth
sections to README and configuration.md; those are trimmed here to short
pointers into docs/mcp-oauth.md so the wrapper isn't documented in three
places. configuration.md also: drops the "(work in progress)" marker,
documents signingKeyPem/signingAlgorithm/accessTokenTtl/refreshTokenTtl,
and fixes the JWKS note. docs/lifecycle-hooks.md documents onMCPTokenIssued.

Documents onMCPTokenIssued (#141), so merge this after #141.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
heskew added a commit that referenced this pull request Jul 1, 2026
Adds the app-author documentation for MCP OAuth, with docs/mcp-oauth.md as
the single deep guide (flow diagram, endpoint reference, the withMCPAuth
wrapper + both registration models + options + cross-component use, the
onMCPTokenIssued hook, audit events, production-deployment checklist,
troubleshooting, and a hand-rolled-server migration guide). Links the MCP
spec (2025-06-18) and RFCs 6749/6750/7591/7636/8252/8414/8707/9728.

Rebased onto main after #134 merged. #134 had already added withMCPAuth
sections to README and configuration.md; those are trimmed here to short
pointers into docs/mcp-oauth.md so the wrapper isn't documented in three
places. configuration.md also: drops the "(work in progress)" marker,
documents signingKeyPem/signingAlgorithm/accessTokenTtl/refreshTokenTtl,
and fixes the JWKS note. docs/lifecycle-hooks.md documents onMCPTokenIssued.

Documents onMCPTokenIssued (#141), so merge this after #141.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
heskew added a commit that referenced this pull request Jul 1, 2026
* docs: MCP OAuth user-facing docs (Stage 8, #98)

Adds the app-author documentation for MCP OAuth, with docs/mcp-oauth.md as
the single deep guide (flow diagram, endpoint reference, the withMCPAuth
wrapper + both registration models + options + cross-component use, the
onMCPTokenIssued hook, audit events, production-deployment checklist,
troubleshooting, and a hand-rolled-server migration guide). Links the MCP
spec (2025-06-18) and RFCs 6749/6750/7591/7636/8252/8414/8707/9728.

Rebased onto main after #134 merged. #134 had already added withMCPAuth
sections to README and configuration.md; those are trimmed here to short
pointers into docs/mcp-oauth.md so the wrapper isn't documented in three
places. configuration.md also: drops the "(work in progress)" marker,
documents signingKeyPem/signingAlgorithm/accessTokenTtl/refreshTokenTtl,
and fixes the JWKS note. docs/lifecycle-hooks.md documents onMCPTokenIssued.

Documents onMCPTokenIssued (#141), so merge this after #141.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): onMCPTokenIssued is fire-and-forget (not awaited)

Match the hook's behavior after #141: it runs detached and is not awaited, so it
never delays/blocks the token response — and its side effects may complete after
the client already has the token.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs: include package: in the README MCP quickstart config example

gemini review on #144: add `package: '@harperfast/oauth'` to the MCP quickstart's
config.yaml so it matches the main Quick Start / configuration.md examples and
loads correctly when copy-pasted.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): polish quickstart + harmonize onMCPTokenIssued timing language

DevExp review on #144 (Antigravity/Gemini), non-blocking:
1. lifecycle-hooks.md said the hook fires "before the response returns" —
   contradicting mcp-oauth.md's not-awaited/detached wording. Harmonized to the
   fire-and-forget language (it runs detached; side effects may complete after the
   client has the token).
2. Dropped the unused `next` param from the mcp-oauth.md quickstart handler (a
   leaf urlPath route doesn't use it).
3. Added a comment noting MCP messages are JSON-RPC 2.0.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): quickstart handler returns the Harper { status, body } shape

gemini review on #144: the quickstart handler returned a bare { jsonrpc, result }
object, but Harper HTTP listeners return { status, body, headers? } (as the README
and the integration fixture do). Wrap the JSON-RPC response as the body.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): import server in the mcp-oauth.md quickstart

claude review on #144: the quickstart block (labeled resources.ts) calls
server.http() but imported only withMCPAuth — copy-pasting it verbatim would throw
ReferenceError: server is not defined. Add `import { server } from 'harper'`,
matching the README quickstart.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): note request.mcp is guaranteed present inside the guarded handler

gemini review on #144 (non-blocking): make explicit that request.mcp is defined
inside a withMCPAuth-guarded handler (the guard rejects missing/invalid tokens
first), so strict-TS users don't need optional chaining. Clarified the quickstart
comments in mcp-oauth.md + README.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): fix backwards path-appended PRM URL in mcp-oauth.md

Codex review on #144: the RFC 9728 path-appended Protected Resource Metadata URL
was written `/mcp/.well-known/oauth-protected-resource`, but the implementation
(wellKnown.ts `protectedResourceMetadataUrl` = origin + PRM_PATH + resourcePath)
serves `/.well-known/oauth-protected-resource/mcp` — the well-known segment sits
between the origin and the resource path. Flipped both occurrences (the discovery
note + the WWW-Authenticate example) to the correct form.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(mcp): clarify `tables` global + app-owned table in the hook examples

gemini review on #144 (non-blocking, devexp): the onMCPTokenIssued examples use
`tables.McpClient` — note that `tables` is a Harper global (so it isn't imported)
and `McpClient` is an illustrative app-owned table the plugin doesn't provide.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP OAuth Stage 5: withMCPAuth wrapper

3 participants