Repository navigation
MCP OAuth Stage 6: audit logging + onMCPTokenIssued hook (#96) - #141
Conversation
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>
|
Reviewed; no blockers found. |
There was a problem hiding this comment.
Code Review
This pull request introduces structured audit logging and an onMCPTokenIssued hook for MCP token lifecycle events (issuance and refresh). It updates the token-signing process to return the JWT ID (jti) alongside the token, integrates the hook and audit emissions into the token-minting flows, and adds comprehensive test coverage. Feedback is provided regarding a potential crash in src/lib/hookManager.ts where an error is unsafely cast to Error to access .message; passing the error object directly to the logger is recommended to prevent crashes and preserve the stack trace.
|
Reviewed; no blockers found. |
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>
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>
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>
kriszyp
left a comment
There was a problem hiding this comment.
Approving — audit records are secret-free (identifiers/hashes, not raw tokens), and the onMCPTokenIssued hook is crash-safe fire-and-forget so a throwing hook can't break token issuance. One suggestion: the hook is awaited on the issuance path, which adds latency — consider not awaiting it. Thanks @heskew!
— 🤖 KrAIs (Kris's review assistant)
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>
… 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>
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>
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
left a comment
There was a problem hiding this comment.
Blocking on the Stage 6 acceptance gap: oauth.mcp.token.rejected is still only declared, not emitted.
This branch now includes withMCPAuth via the merge from main after #134, but rg "token.rejected|emitMCPAuditEvent" shows audit emission only in the token endpoint. #96 says this PR closes Stage 6 and requires rejected audit events on the withMCPAuth denial paths, plus unit coverage for all three event types. As written, merging closes #96 while failed MCP bearer-token validation remains unaudited.
Please either wire rejected-event emission/tests into withMCPAuth in this PR or stop closing #96 and track the remaining Stage 6 item separately.
🤖 Posted by Codex (gpt-5.5) on Nathan's behalf
…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>
…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>
heskew
left a comment
There was a problem hiding this comment.
One remaining issue in the rejected-token audit path: withMCPAuth calls options.emitAudit directly, so a custom audit sink that throws changes an invalid-token denial from the expected 401 into a rejected handler promise / framework 500.
The default emitMCPAuditEvent swallows logger failures, but emitAudit is part of the exported WithMCPAuthOptions surface and the surrounding comment says audit emission cannot break the 401 path. Please wrap the emitAudit({ ... }) call in deny() with a best-effort try/catch, matching emitMCPAuditEvent.
🤖 Posted by Codex (gpt-5.5) on Nathan's behalf
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>
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>
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>
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>
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>
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: 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>
Gemini review on #154: preserve the original error via Error cause when wrapping the ID-token verification failure, per HEG error-handling (cause chaining). Log sites keep the safe message ternary for consistency with the #141 convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: harden catch blocks against non-Error throws (#142, #147) Replace every remaining (error as Error).message inside catch blocks with error instanceof Error ? error.message : String(error). When a non-Error value (null, string, number) is thrown, the cast form crashes the catch itself — (null).message throws TypeError — defeating the catch's swallow-and-log contract. #141 already fixed hookManager.ts (callOnLogin/Logout/TokenRefresh/ callResolveProvider/callOnMCPTokenIssued) and token.ts / withMCPAuth.ts. This sweep fixes all remaining sites found by exhaustive grep: src/index.ts:313 — session validator provider resolution src/lib/sessionValidator.ts:97 — token validation error log src/lib/sessionValidator.ts:181 — token refresh error log src/lib/sessionValidator.ts:186 — Token refresh failed: error message src/lib/resource.ts:381 — dynamic provider resolution src/lib/OAuthProvider.ts:161 — additional user info fetch fallback src/lib/OAuthProvider.ts:225-226 — ID token signature verification log + rethrow src/lib/handlers.ts:239 — ID token verification fallback log src/lib/handlers.ts:332 — OAuth callback error reason derivation src/lib/providers/github.ts:42 — GitHub token validation error src/lib/providers/github.ts:76 — GitHub email fetch failure src/lib/mcp/dcr.ts:296 — client registration storage failure src/lib/mcp/wellKnown.ts:273 — well-known handler failure src/lib/mcp/authorize.ts:203 — client lookup failure src/lib/mcp/authorize.ts:292 — CSRF token generation failure src/lib/mcp/callback.ts:93 — auth code persistence failure Tests: add 4 non-Error throw assertions in hookManager.test.js for callOnLogin (string + null), callOnLogout (null), and callOnTokenRefresh (string) — the async-await methods that are easily exercised without DB mocking. The mcp store catches (dcr/authorize/callback/wellKnown) are mechanical compiler-verified changes over DB-wrapping code. Closes #142 Closes #147 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: chain original error as cause on ID-token wrap (review) Gemini review on #154: preserve the original error via Error cause when wrapping the ID-token verification failure, per HEG error-handling (cause chaining). Log sites keep the safe message ternary for consistency with the #141 convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stage 6 of the MCP OAuth epic (#86): wires MCP token-lifecycle events into Harper's audit trail and adds a lifecycle hook so apps can react to token issuance (rate-limiting, billing, usage attribution).
Closes #96. Draft until CI's claude/gemini review passes — then Ready for @kriszyp.
What this adds
onMCPTokenIssuedhook onOAuthHooks+HookManager.callOnMCPTokenIssued— mirrorsonLogin(fire-and-forget; a throwing hook is caught/logged and never blocks issuance). Fires after the JWT is minted and all token state is persisted, before the response.oauth.mcp.token.issued/oauth.mcp.token.refreshed, emitted from the token-issuance path via a newsrc/lib/mcp/audit.tshelper (emitMCPAuditEvent). StructuredMCP audit: <JSON>record through Harper's global logger at.info, mirroring core'sauthAuditLogpattern (→ hdb.log, alongside the auth audit trail). No token material — payload isclient_id/sub/aud/scope/jti/timestamponly.signAccessTokennow returns{ token, jti }(accepts an optionaljti) so the audit/hook can reference the token id without re-parsing the JWT.onLoginalready fires on MCP-initiated auth (the bridged authorize/callback flow, Stage 3) — verified, with a regression test added.Deferred (depends on unmerged #134)
oauth.mcp.token.rejectedlives inwithMCPAuth(Stage 5 / #134, not yet on main). The audit helper already accepts that event type, so adding the call site once #134 merges is a one-liner. This PR ships the two issuance events + the hook + the onLogin verification.Cross-model review (pre-PR)
Ran Codex + a Harper-domain pass before pushing. Both P2 findings fixed (the bulk of this PR's care):
emitMCPAuditEventnon-throwing — it nowtry/catches the logger call (its doc promised non-throwing but didn't deliver). On the refresh path the emit runs after the family is rotated, so a thrown logger error would otherwise deny the client its new token and strand the family — a data-integrity path, not just observability.Plus: corrected the audit doc (records are filterable by the
MCP audit:prefix /eventfield, not a logger tag — Harper doesn't expose tagging to plugins here); a JSDoc note that the hook'seventis sanitized butrequestis not; and two regression tests for the paths above (failing refresh-family persistence → no phantom audit/hook; throwing logger → token still returned).Testing
Unit suite: 694 pass, 2 skipped (pre-existing), 0 fail. Build/lint/format clean. (Integration tests run on CI.)
🤖 Generated with Claude Code