Support MCP OAuth through the configured OIDC issuer - #63
Conversation
backnotprop
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES. The design fits well: it reuses the existing ExternalMcpBearerVerifier seam, pins the algorithms, and matches issuer and audience exactly. Managed API keys and WorkOS behave as before. Before merge, the ID-token refusal needs to live up to what the ledger and docs promise, the conformance tests need their requirement IDs, and two policy and doc points need a decision.
Blocking
1. The ID-token refusal only works for Keycloak, but the ledger and docs describe it as generic
- Where:
src/identity/oidc-mcp-bearer-verifier.ts:22and:240. - What happens: The only ID-token check is the payload claim
typ === "ID", which only Keycloak emits. Other issuers are refused only because an ID token'saudis normally the client ID, not<origin>/mcp. - Failure case: Some issuers let operators pick any client ID string. If an operator names a client after the resource URL (a common resource-server convention), that client's ID tokens carry
aud = <origin>/mcp. They are then accepted at/mcp. - Reproduced: A token signed by the issuer with
nonce,at_hash,auth_time, headertyp: JWT, no payloadtyp, andaud = <origin>/mcpreturned 200 and admitted the user. With headertyp: logout+jwtit also returned 200. - Why it matters: MCP-013-F,
docs/deployment.md("ID tokens … are refused") and decision 0028 all state this as a general rule. - Suggested fix: Refuse tokens that carry ID-token-only claims (
nonce,at_hash,c_hash). Also refuse a JOSE headertypthat is present and notat+jwtorJWT, with a note that RFC 9068at+jwtis preferred. Add a hostile test for each shape. - Severity: Medium. Exploiting it needs an unusual client-ID setup, but the promise is recorded in the ledger.
2. The conformance tests have no requirement IDs in their titles
- Where:
tests/conformance/oidc-mcp-authorization.test.ts:80, 117, 153, 163, 215, 250, 259. - What happens: None of the seven titles includes
MCP-013-BorMCP-013-F. - Why it matters: AGENTS.md requires conformance tests to be named with their requirement IDs, so this evidence cannot be attached to the ledger.
- Suggested fix: Put
MCP-013-Bon the positive tests (metadata plus successful token, userinfo fallback, several audiences) andMCP-013-Fon the hostile ones.workos-mcp-authorization.test.tshas the same gap already on main, so it is not a precedent.
3. Tokens without email_verified are treated as verified, and they can claim the bootstrap administrator
- Where:
src/identity/oidc-mcp-bearer-verifier.ts:223,emailVerified: claims.email_verified !== false. - Reproduced: On a fresh installation, the first token to arrive carried
email = <bootstrap admin email>, noemail_verified, and a newsub. It returned 200 and was bound as the administrator.resolveExternalMember(src/application/installation-access.ts:445) also links by email to any admitted member who is not yet bound. - This is not new: Browser login uses the same rule (
oidc-identity-provider.ts:177). The PR carries that rule over to access tokens. - Why it still matters here: The rule is riskier on this path. The docs name Entra, whose
emailoptional claim is mutable, is not verified, and comes with noemail_verifiedclaim. - Needs a maintainer decision: Either require
email_verified === trueon the MCP path, or record in 0028 that the browser-login rule is inherited on purpose and explain why.
4. The Entra guidance in docs/deployment.md:60 is probably wrong
- What the doc says: "Entra exposes an API and uses its application ID URI."
- What Microsoft documents: v2.0 access tokens always carry the API's client ID (a GUID) in
aud, not the application ID URI. v1.0 tokens can carry the URI, but theirissishttps://sts.windows.net/{tid}/, which never matches the v2.0 discovery issuer. - Userinfo does not help: Entra's userinfo endpoint only accepts tokens issued for Microsoft Graph, so a token with
aud = /mcpcannot reach it. - Caveat: This was checked against Microsoft's documented claim rules, not a live tenant.
- Suggested fix: Remove Entra from the list, or test it and document a setup that works.
Non-blocking
- Unchecked discovery fields are re-served (
src/identity/oidc-oauth-metadata.ts:100)....documentpasses every discovery field through to/.well-known/oauth-authorization-server, includingend_session_endpoint,device_authorization_endpoint, the rawuserinfo_endpointandmtls_endpoint_aliases. Only the registration and revocation endpoints are checked, so the comment at:79("a client must not be sent anywhere this server would have refused") overstates it. Consider an allowlist of fields. The risk is low because the operator configures the issuer. - Ledger proof gap is not stated fully (MCP-013). The new MCP-013-B text says "connect … through browser approval including a generic OIDC issuer". The Keycloak test gets its tokens through the password grant. No MCP client runs the authorization code, PKCE or dynamic registration flow against Keycloak. The status is
implementingwith empty evidence, which is accurate, butproof_gapshould say this. - Cookie refusal is not tested. MCP-013-F lists browser login cookies. Neither new test sends a real browser session cookie to
/mcp. A bogus cookie returns 401. In the Keycloak test, a cookie from the real browser login is available and would make this a real proof. - Startup wiring and fallback are not tested (
src/cli/lifecycle-commands.ts:434-447). No test coversoidcAuthenticationOrBrowserOnly: the compiled server turning on MCP OAuth when OIDC is configured, or thediscovery_failedwarning and browser-only fallback. After a startup discovery failure, MCP OAuth stays off until restart. That is worth one sentence in the deployment guide. - Trailing-slash issuers fail. An issuer whose tokens carry a trailing slash in
iss(Auth0 does this) is refused, because the configured issuer is normalized without the slash and jose compares exactly (returned 401 in a probe). Browser login already behaves the same way, so this is not a regression, but Auth0 is named as compatible in 0028. - Cosmetic:
resolveIdentityrunsjwtVerifya second time on every first-use request. That is cheap because the key set is cached. It is defensible as defense in depth, but a comment saying so would help.
What was verified
Run against this branch (gh pr checkout 63; no conflicts with current main):
pnpm install --frozen-lockfile,pnpm exec tsc -p tsconfig.json --noEmit, andpnpm lintall pass.pnpm exec vitest run tests/conformance/oidc-mcp-authorization.test.ts tests/conformance/workos-mcp-authorization.test.ts— 10/10 pass; WorkOS path unchanged.pnpm test:oidc(Docker, pinned Keycloak) — 4/4 pass.- Hostile probes over real HTTP against the provider boundary (scratch tests, since deleted):
alg: none, emptyaud, missingexp, futurenbf, trailing-slashiss, garbage segments (generic error, no filesystem paths),email_verified: false, and bogus-cookie-only all correctly return 401. The three 200s are the reproductions in findings 1 and 3.
Not run: pnpm verify:iteration.
Thank you
Thank you @yalait — this is the project's first substantial outside contribution, and it is a careful one: it extends an existing port instead of adding a parallel path, the decision record is honest about reversing 0020, key-set outages are told apart from bad tokens, and a real Keycloak harness backs it. The changes asked for above are small and focused. We would be glad to merge this once they are in.
This review was prepared with Claude under maintainer direction.
Picks up the dual-license change, effect 4.0.0-rc.115, hono 4.13.5, vitest 4.1.11, and the dependency security overrides. No conflicts; the branch type-checks against rc.115 unchanged. Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
The ID-token refusal only recognized Keycloak's payload typ of ID. A JWT the issuer signed with nonce, at_hash, or c_hash, or with a JOSE header typ other than at+jwt or JWT (logout+jwt, secevent+jwt, ID-JAG), is now refused as well, following RFC 9068 section 4 and RFC 8725 explicit typing. at+jwt, application/at+jwt, JWT, and a missing typ stay accepted. On the MCP bearer path an email links a member or claims the bootstrap administrator only when email_verified is true. A subject that is already bound is still recognized by issuer and subject alone. Browser login is unchanged. Conformance tests now carry MCP-013-B and MCP-013-F, and the Keycloak harness proves that the real browser session cookie is refused at /mcp. Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
Decision 0028 now records the access-token type checks and why the MCP path requires email_verified when browser login does not. The deployment guide drops the Entra audience claim, which v2.0 tokens cannot satisfy, and notes that MCP OAuth stays off after a startup discovery failure. The MCP-013 proof gap states that the Keycloak harness uses the password grant. Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
|
Thanks again @yalait — this is a really solid contribution, and we wanted to get it over the line without another round trip, so we pushed the review fixes directly to your branch (hope that's okay!). Summary:
One review item we deliberately did not take: the authorization-server metadata allowlist. The official TypeScript SDK client reads a growing set of metadata fields (several are recent spec additions), so a fixed allowlist would silently break future clients while protecting nothing — spec-following clients fetch the same document directly from the issuer anyway. Everything is green locally: lint, typecheck, the conformance checks, the Keycloak integration run (twice), and the full test suite. Take a look and tell us if anything doesn't match what you intended. |
backnotprop
left a comment
There was a problem hiding this comment.
All four blocking findings from the earlier review are addressed on the branch (maintainer-applied, vetted against the MCP 2026-07-28 authorization spec and the official SDKs). Approving; will merge once CI is green.
Closes #57.
/mcpnow accepts end-user access tokens from the OIDC issuer already configured forbrowser login, through the existing
ExternalMcpBearerVerifierseam and the sameoidc:<issuer>plussubbinding. A token must name the configured issuer and carry<origin>/mcpinaud. ID tokens, tokens for another resource, and browser cookies arerefused. The server publishes RFC 9728 protected-resource metadata and answers an
unauthenticated call with
401andresource_metadata. Managed API keys and the WorkOSpath are untouched.
0028 records the reversal, 0020's exclusion points at it, MCP-013 takes the generic path,
and
docs/deployment.mdexplains the audience mapper and the two client-registrationoptions: the issuer's own RFC 7591 endpoint, or one client an administrator registers.
Artifact Server issues no client credentials, so MCP-014 stays as it is.
resolveIdentitynow also receives the credential. WorkOS ignores it. The OIDC pathneeds it, because userinfo answers the presenter of the token, and a Keycloak realm can
leave
emailout of the access token. The caching alternative is in 0028.Two calls for you
audmust contain<origin>/mcp, but a multi-valuedaudpasses. The Keycloak harnessin this PR mints this token from a client whose only extra configuration is the audience
mapper from the deployment guide:
{"aud": ["http://127.0.0.1:44835/mcp", "account"], "typ": "Bearer"}accountcomes from Keycloak's audience-resolve mapper, so a single-valued check wouldrefuse every Keycloak token. A token without this resource in
audis refused eitherway. Tell me if you meant single-valued and I will tighten it.
A valid token from a person who was never admitted gets
401at/mcp, while thebrowser callback answers
403for the same identity. I kept the existing mapping.Verification
pnpm lint,pnpm typecheck,pnpm build,pnpm conformance:validate,pnpm conformance:tests,pnpm test:oidcwith two new MCP cases against the pinnedKeycloak, and
pnpm testwith 320 passed. Threetests/cliprocess-spawning tests timeout under parallel load here and pass when run alone, the same way they do on an
unmodified checkout. I did not run the Compose, object-storage, Kubernetes, coverage, or
performance legs of
verify:iteration.Hostile cases in
tests/conformance/oidc-mcp-authorization.test.ts: expired, wrongissuer, a second issuer signing with the same key, wrong and multi-valued audiences, ID
token, HS256, unknown
kid, missing subject, missing claims, userinfo outage, userinforejection, key-set outage. That last case turned up a bug worth naming:
josereports anon-200 JWKS response as a generic error, which read as an invalid token and answered
401. The key-set fetch now raises its own failure.