Conversation
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
There was a problem hiding this comment.
Pull request overview
Updates the external MCP OAuth flow to better handle provider-specific resource URI quirks, reduce duplicate concurrent work (refresh/connect), and adjust how OAuth tokens/resources are surfaced to the MCP SDK.
Changes:
- Add a compatibility fallback mechanism for external OAuth
resourceUriresolution (notably Webflow/mcp→/sse) and apply it across validation/runtime normalization/backfill. - Introduce single-flight behavior for OAuth refreshes (per server/user) and for per-user server connection establishment.
- Adjust OAuth provider behavior to avoid returning refresh tokens to the SDK helper (preventing SDK-owned refresh loops) and add extensive OAuth-related logging.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/mcp-external-auth.spec.ts | Adds/updates tests for compatibility resource fallback, provider snapshot behavior, and single-flight refresh/connect behavior. |
| src/services/oauthTokens.ts | Adds token record logging, single-flight refresh implementation, resource URL validation, and provider snapshot changes. |
| src/services/mcp.ts | Adds compatibility fallback resource resolution, resource-uri backfill invalidation logic, and single-flight user connection establishment. |
| package.json | Bumps package version to 1.11.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log({ | ||
| level: 'info', | ||
| msg: `[oauth:${input.username}:${input.serverName}] Starting OAuth refresh ${JSON.stringify({ | ||
| tokenEndpoint: summarizeUrlForLog(input.tokenEndpoint), | ||
| resource: summarizeUrlForLog(input.resource), | ||
| clientId: truncateSensitiveValue(existing.clientId), | ||
| refreshToken: truncateSensitiveValue(existing.refreshToken), | ||
| tokenEndpointAuthMethod: existing.tokenEndpointAuthMethod, | ||
| registrationMode: existing.registrationMode, | ||
| expiresAt: existing.expiresAt?.toISOString(), | ||
| scopes: existing.scopes | ||
| })}` | ||
| }) |
There was a problem hiding this comment.
Several new info logs include truncated refreshToken values. Even partial refresh tokens are sensitive credentials and can become a security liability since log() prints to stdout in production. Prefer logging only booleans/metadata (e.g., hasRefreshToken, token endpoint origin/path) and omit refresh token material entirely.
| log({ | ||
| level: 'info', | ||
| msg: `[oauth:${username}:${serverName}] Saved PKCE code verifier ${JSON.stringify({ | ||
| codeVerifier: truncateSensitiveValue(codeVerifier) |
There was a problem hiding this comment.
The new logs emit truncated PKCE codeVerifier values. Code verifiers are secrets for active OAuth sessions; logging them (even truncated) can enable replay/credential stuffing if logs are accessed. Recommend removing the verifier from logs and only logging non-sensitive flags (e.g., hasCodeVerifier: true).
| codeVerifier: truncateSensitiveValue(codeVerifier) | |
| hasCodeVerifier: true |
| const parsed = new URL(value) | ||
| return `${parsed.origin}${parsed.pathname}` | ||
| } catch { | ||
| return value |
There was a problem hiding this comment.
summarizeUrlForLog() returns the original string on URL parse failure, which can re-introduce query strings/fragments (e.g., ?token=...) into logs. Since these values may come from external configuration, consider returning a safe placeholder (like <invalid-url>) or stripping everything after ?/# even in the fallback path.
| return value | |
| // Fallback: strip query strings and fragments to avoid logging sensitive data | |
| const queryIndex = value.indexOf('?') | |
| const fragmentIndex = value.indexOf('#') | |
| let endIndex = value.length | |
| if (queryIndex !== -1) { | |
| endIndex = Math.min(endIndex, queryIndex) | |
| } | |
| if (fragmentIndex !== -1) { | |
| endIndex = Math.min(endIndex, fragmentIndex) | |
| } | |
| const sanitized = value.slice(0, endIndex) | |
| return sanitized || '<invalid-url>' |
| if (oauthTemplate.resourceUri !== resolveCompatibilityFallbackExternalOAuthResourceUri(transportUrl)) { | ||
| throw new McpValidationError( | ||
| 'oauthTemplate.resourceUri must equal the canonicalized transport url in issuer-override discovery mode' | ||
| 'oauthTemplate.resourceUri must equal the resolved fallback resource uri in issuer-override discovery mode' | ||
| ) |
There was a problem hiding this comment.
The updated validation error text references a "resolved fallback resource uri", but doesn’t tell the caller what value was expected vs what was provided. Including the expected resolved URI (and the received one) in the error message would make misconfigurations much easier to diagnose, especially since canonicalization/compat rules can change the expected value.
| global.fetch = jest.fn(async () => | ||
| new Response( | ||
| JSON.stringify({ | ||
| access_token: 'fresh-access', | ||
| refresh_token: 'fresh-refresh', | ||
| token_type: 'Bearer', | ||
| expires_in: 3600 | ||
| }), | ||
| { | ||
| status: 200, | ||
| headers: { 'content-type': 'application/json' } | ||
| } | ||
| ) | ||
| ) as typeof global.fetch | ||
|
|
There was a problem hiding this comment.
This test replaces global.fetch but never restores it afterward, which can leak the mocked fetch into later test blocks in this file (and make future additions flaky). Consider restoring global.fetch in an afterEach for this describe (or use jest.spyOn(global, 'fetch') and mockRestore() / finally cleanup in the test).
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
No description provided.