try again - fix google mcp oauth

[j4ys0n]

No description provided.

[j4ys0n]

Automated review 🤖

Summary of Changes
Bumps version to 1.11.4 and refactors buildTransportOptions to support skipping OAuth auth provider initialization when includeAuthProvider: false is passed, enabling fallback to persisted session resume. Updates connectUserToServerInternal to conditionally prefer session resume over auth provider based on existing session ID and forceAuthProvider, and adds 401-specific retry logic with auth provider fallback after session failure.

Key Changes & Positives

• Introduces BuildTransportOptionsInput type to control auth provider inclusion, improving flexibility in transport construction (src/services/mcp.ts, lines +810–812).
• Moves stripAuthorizationHeaders call earlier in buildTransportOptions, ensuring sanitized headers are used consistently regardless of auth path (src/services/mcp.ts, lines +1641, +1668 → removed duplicate at +1665).
• Adds explicit 401 retry path with session teardown and forced auth provider (src/services/mcp.ts, lines +2502–2511), enhancing resilience against stale sessions during OAuth flows.
• New test validates includeAuthProvider: false behavior and confirms getTokenRecord is skipped (test/mcp-external-auth.spec.ts, lines +558–604). 🟢

Potential Issues & Recommendations

1. Issue / Risk: shouldPreferSessionResume is computed before sessionId is confirmed non-null in all paths; if sessionId is falsy, !forceAuthProvider still allows auth provider usage, but the logic may mislead future maintainers.
   Impact: Could obscure intent or cause subtle misbehavior if sessionId is set conditionally elsewhere.
   Recommendation: Explicitly guard shouldPreferSessionResume with !!sessionId before evaluating !forceAuthProvider.
   Status: 🟡 Needs review

2. Issue / Risk: Recursive call this.connectUserToServerInternal(..., true) inside the 401 handler passes allowSessionRetry = false but reuses forceAuthProvider = true; if the second attempt also fails with 401, no further retries occur.
   Impact: May exhaust retry attempts prematurely for transient auth issues.
   Recommendation: Consider limiting recursion depth or adding a retry counter to avoid unbounded recursion.
   Status: 🟡 Needs review

Language/Framework Checks

• TypeScript: New BuildTransportOptionsInput type correctly typed and defaulted (src/services/mcp.ts, +810–812).
• Async/await: buildTransportOptions remains async; input parameter default ({}) avoids undefined errors (src/services/mcp.ts, +1620–1623).
• Jest: Test mocks oauthTokensService precisely and asserts getTokenRecord not called when includeAuthProvider: false (test/mcp-external-auth.spec.ts, +582–583).

Security & Privacy

• stripAuthorizationHeaders is now applied before deciding whether to include authProvider, preventing accidental leakage of shared tokens into OAuth flows when session resume is preferred (src/services/mcp.ts, +1641).

Tests

• New test covers includeAuthProvider: false path; consider adding test for 401 retry behavior (e.g., mock fetch to return 401, verify recursive call with forceAuthProvider=true).

Approval Recommendation
Approve with caveats

• Address potential recursion risk in 401 retry handler (add depth limit or counter).
• Clarify shouldPreferSessionResume logic with explicit !!sessionId guard.

[Copilot]

Pull request overview

This PR adjusts the streamable HTTP connection flow for external OAuth MCP servers to prefer resuming persisted sessions (via sessionId) before attaching an OAuth auth provider, with a fallback retry when the resume attempt is unauthorized.

Changes:

• Add includeAuthProvider input to buildTransportOptions to optionally omit the OAuth auth provider (and sanitize headers) for session-resume-first behavior.
• Update connectUserToServerInternal to attempt session resume without an auth provider when a persisted sessionId exists, and retry with auth enabled on HTTP 401.
• Bump package version to 1.11.4.

Reviewed changes

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

File	Description
test/mcp-external-auth.spec.ts	Adds a unit test ensuring buildTransportOptions can skip auth provider and avoids calling the token store.
src/services/mcp.ts	Implements auth-provider skipping, session-resume preference, and 401 retry logic for streamable HTTP connections.
package.json	Patch version bump.

---

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

[Copilot]

includeAuthProvider is derived solely from shouldPreferSessionResume (presence of a persisted sessionId). Since buildTransportOptions(..., { includeAuthProvider: false }) also strips any Authorization header from server.headers, this changes connection behavior for all streamable_http servers that have a persisted session, including non-OAuth servers that may rely on a static Authorization header (and may fail with a non-401 status, so no retry happens). Consider scoping the “prefer session resume” path to OAuth2 servers (e.g., server.authMode === 'oauth2' and/or server.oauthTemplate exists) and/or avoiding header-stripping when the server is not OAuth-driven.

Suggested change

	const transportOptions = await this.buildTransportOptions(server, username, {
	includeAuthProvider: !shouldPreferSessionResume
	const oauthServerConfig = server as {
	authMode?: string
	oauthTemplate?: unknown
	}
	const shouldResumeWithOAuthSession =
	shouldPreferSessionResume &&
	(oauthServerConfig.authMode === 'oauth2' || !!oauthServerConfig.oauthTemplate)
	const transportOptions = await this.buildTransportOptions(server, username, {
	includeAuthProvider: !shouldResumeWithOAuthSession

[Copilot]

The warn log says “Retrying with bearer auth”, but the retry behavior is “forceAuthProvider=true”, which may still fall back to server.headers (and not necessarily a bearer auth provider if there’s no token record). Consider updating the message to reflect what is actually being retried (e.g., retrying with auth provider / auth headers enabled) to avoid misleading operational logs.

Suggested change

	msg: `[${username}:${server.name}] Session resume returned 401 without auth provider. Retrying with bearer auth.`
	msg: `[${username}:${server.name}] Session resume returned 401 without auth provider. Retrying with auth provider enabled.`

[Copilot]

New 401 retry behavior (session-resume attempt without auth provider, then retry with auth enabled) isn’t covered by tests. Adding a unit test that simulates a persisted sessionId causing the first connect attempt to throw a 401 and asserting that a second attempt is made with includeAuthProvider: true (and no infinite retry) would help prevent regressions.