mcp(oauth): delegate the OAuth engine to internal/oauth (dedup)

[gnanam1990]

Summary

OAuth Phase 2 — MCP OAuth now reuses the shared internal/oauth engine (from #210) instead of carrying its own copy of the transport/identity-agnostic primitives. internal/mcp/oauth.go drops 526 → 418 LOC (−187/+79).

Stacks on #210 — base is feat/oauth-module. Merge #210 first; this then targets main cleanly.

What now delegates to internal/oauth

newPKCE, newState, discoverAuthorizationServer (RFC 8414), the authorize-URL builder, exchangeCode, refreshAccessToken, the token-response parser, and joinWellKnown — all removed from MCP and delegated to the shared engine via thin type conversions.

Behavior-preserving

MCP keeps its own OAuthConfig/StoredToken types, LoginOptions/Login orchestration, loopback handling, registerClient (dynamic client registration — not part of the shared engine), CLI output, and on-disk token format (mcp-oauth-tokens.json, raw server-name keys). The existing internal/mcp/oauth_test.go + oauth_store_test.go + CLI tests pass unchanged.

One intentional hardening inherited from the shared engine: the token endpoint must be https (loopback exempt), so a credential is no longer sent to a plaintext non-loopback endpoint. All existing tests use loopback and are unaffected.

Not changed

The token store (internal/mcp/oauth_store.go) is untouched — unifying it with internal/oauth's namespaced store would change the on-disk format and is a separate, transparent-migration follow-up.

Verification

gofmt · vet · build (host + linux + windows) · go test ./... incl. -race (mcp tests unchanged) · staticcheck (no new) · govulncheck (0) · deadcode (no new vs branch base). Local run check: zero mcp oauth --help / status / status --json produce identical output.

Summary by CodeRabbit

• Refactor
  • Internal OAuth and PKCE authentication handling has been refactored to use shared components across the system. This consolidates duplicate logic and improves code maintainability. All existing user-facing authentication behavior and APIs remain unchanged with no impact on functionality.

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: deffa02322ac
Changed files (1): internal/mcp/oauth.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4727f90e-d613-4fd2-9dcf-6acc0587c969

📥 Commits

Reviewing files that changed from the base of the PR and between 3864909 and bfee4bc.

📒 Files selected for processing (1)

• internal/mcp/oauth.go

---

Walkthrough

internal/mcp/oauth.go removes all local OAuth2/PKCE implementation — PKCE/state generation, RFC 8414 metadata fetch, authorization URL construction, token form POST, and response parsing — delegating each operation to internal/oauth. New local helpers (pkceToParams, pkceToOAuth, configFor, tokenToStored) translate between the two type systems.

Changes

MCP OAuth delegation to shared engine

Layer / File(s)	Summary
Import cleanup and type translation helpers internal/mcp/oauth.go	Removes crypto/base64/hashing imports and introduces defaultLoginTimeout plus four translation helpers (pkceToParams, pkceToOAuth, configFor, tokenToStored) that map MCP-local types to internal/oauth types.
Discovery, auth URL, code exchange, and token refresh delegated internal/mcp/oauth.go	discoverAuthorizationServer calls oauth.DiscoverAuthorizationServer; authorizationURL calls oauth.NewPKCE, oauth.NewState, oauth.BuildAuthorizationURL; exchangeCode calls oauth.ExchangeCode; refreshAccessToken calls oauth.Refresh — removing all inlined HTTP/form/JSON logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: delegating OAuth implementation details from MCP to a shared internal/oauth engine to reduce code duplication.
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/oauth-mcp-dedup

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Vasanthdev2004]

Verdict: APPROVED ✅

Reviewed locally in worktree review-pr212 at 1a9591f (1 commit on top of feat/oauth-module). +79 / −187, single file (internal/mcp/oauth.go), base feat/oauth-module ← feat/oauth-mcp-dedup. Merge order: #210 first, then this rebases cleanly onto main.

CI gates (clean): gofmt -l . empty, go vet ./... clean, go build ./... clean, go test -count=1 ./internal/mcp/... (1.7s), go test -count=1 ./internal/oauth/... (7.1s).

What got delegated to the shared engine:

• newPKCE → oauth.NewPKCE() (32 bytes RawURLEncoding, S256 challenge — byte-for-byte identical)
• newState → oauth.NewState() (32 bytes, base64url)
• discoverAuthorizationServer → oauth.DiscoverAuthorizationServer() (RFC 8414 well-known, with type conversion back to MCP's authServerMetadata since the MCP metadata type omits the device/registration-extension fields the shared engine surfaces)
• authorizationURL → oauth.BuildAuthorizationURL() (now also protected by the reserved-param guard from the shared engine)
• exchangeCode → oauth.ExchangeCode() (form-encoded token POST with the right grant_type and verifier)
• refreshAccessToken → oauth.Refresh() (refresh-token grant, preserves prior refresh token if the response omits one, maps error / error_description)
• joinWellKnown, the local tokenResponse struct, and postTokenRequest — all removed from MCP, replaced by calls into the shared engine

What MCP keeps (correctly):

• Its own OAuthConfig / StoredToken types (different from oauth.Config / oauth.Token — the ServerName and other MCP-specific fields stay local)
• LoginOptions and the full Login orchestration (one deadline bounds discovery + optional client registration + callback wait + token exchange, the loopback listener is bound first so the redirect URI is known before client registration)
• The inline http.Server loopback handler (MCP serves /callback on its own; not using the shared LoopbackListener because the callback flow is different)
• registerClient (RFC 7591 dynamic client registration — explicitly noted as not part of the shared engine)
• The CLI surface
• The on-disk token format (mcp-oauth-tokens.json with raw server-name keys, no shared-engine namespacing)

Conversion helpers are package-private and used only inside the file — pkceToParams, pkceToOAuth, configFor, tokenToStored. No external callers depended on the removed names (grepped the whole module).

Inherited hardening (intentional): the token endpoint must be https or loopback. MCP tests all use loopback, so the behavior is unchanged, but the new path will refuse a credential-leaking plaintext token endpoint that the old path would have silently sent credentials to.

One minor observation (not a blocker):

The MCP loopback in Login (line 320) is net.Listen("tcp", "127.0.0.1:0") + an inline http.Server — this is the MCP-specific redirect path and stays put. It binds 127.0.0.1 only (correct) and is single-use by the resultChan handshake, but it doesn't have the same single-use / idempotent-Close guarantees as the shared LoopbackListener. That's OK because the server.Shutdown is always called in defer, but if a future change ever wants the same security-property guarantee the OAuth provider login has, switching to the shared listener would do it. Cosmetic.

Overall: this is exactly the right shape for "delegate the transport-agnostic core, keep the identity-specific surface." No behavior change for users, no token-store migration (deferred to a separate, transparent PR), the MCP test suite is untouched and still passes, and MCP inherits the engine's hardening. Merging once #210 lands.

🤖 Generated with Claude Code