-
Notifications
You must be signed in to change notification settings - Fork 1
try again - fix google mcp oauth #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -807,6 +807,10 @@ export type TransportFactoryOptions = { | |||||
| url?: string | ||||||
| } | ||||||
|
|
||||||
| type BuildTransportOptionsInput = { | ||||||
| includeAuthProvider?: boolean | ||||||
| } | ||||||
|
|
||||||
| export const createTransport = (server: MCPServer, options: TransportFactoryOptions = {}): Transport => { | ||||||
| if (server.transportType === 'streamable_http') { | ||||||
| const requestInit = options.requestInit ?? buildRequestInit(server.headers) | ||||||
|
|
@@ -1613,7 +1617,11 @@ export class MCPService implements Resource { | |||||
| return this.migrateServerTransport(withRuntimeOauthTemplate) | ||||||
| } | ||||||
|
|
||||||
| private async buildTransportOptions(server: MCPServer, username?: string): Promise<TransportFactoryOptions> { | ||||||
| private async buildTransportOptions( | ||||||
| server: MCPServer, | ||||||
| username?: string, | ||||||
| input: BuildTransportOptionsInput = {} | ||||||
| ): Promise<TransportFactoryOptions> { | ||||||
| if (server.transportType !== 'streamable_http') { | ||||||
| return {} | ||||||
| } | ||||||
|
|
@@ -1630,6 +1638,16 @@ export class MCPService implements Resource { | |||||
| return runtimeUrl ? { url: runtimeUrl } : {} | ||||||
| } | ||||||
|
|
||||||
| const sanitizedHeaders = stripAuthorizationHeaders(server.headers) | ||||||
|
|
||||||
| if (input.includeAuthProvider === false) { | ||||||
| oauthLogInfo(`[oauth:${username}:${server.name}] Building transport without auth provider so persisted session resume is attempted first`) | ||||||
| return { | ||||||
| requestInit: buildRequestInit(sanitizedHeaders ?? {}), | ||||||
| ...(runtimeUrl ? { url: runtimeUrl } : {}) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const record = await this.oauthTokensService.getTokenRecord(server.name, username) | ||||||
| if (!record) { | ||||||
| return runtimeUrl ? { url: runtimeUrl } : {} | ||||||
|
|
@@ -1665,7 +1683,6 @@ export class MCPService implements Resource { | |||||
| tokenEndpointAuthMethodsSupported: server.oauthTemplate?.tokenEndpointAuthMethodsSupported, | ||||||
| dcrClients: this.dcrClients | ||||||
| }) | ||||||
| const sanitizedHeaders = stripAuthorizationHeaders(server.headers) | ||||||
| return { | ||||||
| authProvider, | ||||||
| requestInit: buildRequestInit(sanitizedHeaders ?? {}), | ||||||
|
|
@@ -2398,7 +2415,8 @@ export class MCPService implements Resource { | |||||
| private async connectUserToServerInternal( | ||||||
| username: string, | ||||||
| server: MCPServer, | ||||||
| allowSessionRetry = true | ||||||
| allowSessionRetry = true, | ||||||
| forceAuthProvider = false | ||||||
| ): Promise<UserConnection> { | ||||||
| if (server.transportType !== 'streamable_http') { | ||||||
| throw new Error(`connectUserToServer is only for streamable_http servers, got ${server.transportType}`) | ||||||
|
|
@@ -2420,6 +2438,7 @@ export class MCPService implements Resource { | |||||
| const sessionRecord = await this.userSessionsService.getSession(server.name, username) | ||||||
| sessionId = sessionRecord?.sessionId ?? undefined | ||||||
| } | ||||||
| const shouldPreferSessionResume = !!sessionId && !forceAuthProvider | ||||||
|
|
||||||
| const transportErrorHandler = async (error: Error) => { | ||||||
| log({ level: 'error', msg: `[${username}:${server.name}] transport error: ${error.message}`, error }) | ||||||
|
|
@@ -2442,7 +2461,9 @@ export class MCPService implements Resource { | |||||
| { name: 'MSQStdioClient', version: '1.0.0' }, | ||||||
| { capabilities: { prompts: {}, resources: {}, tools: {} } } | ||||||
| ) | ||||||
| const transportOptions = await this.buildTransportOptions(server, username) | ||||||
| const transportOptions = await this.buildTransportOptions(server, username, { | ||||||
| includeAuthProvider: !shouldPreferSessionResume | ||||||
| }) | ||||||
| const transport = createTransport(server, { ...transportOptions, sessionId }) | ||||||
|
|
||||||
| const userConn: UserConnection = { | ||||||
|
|
@@ -2478,11 +2499,22 @@ export class MCPService implements Resource { | |||||
|
|
||||||
| return userConn | ||||||
| } catch (error) { | ||||||
| const httpStatus = extractHttpStatusFromError(error) | ||||||
|
|
||||||
| if (sessionId && shouldPreferSessionResume && allowSessionRetry && httpStatus === 401) { | ||||||
| log({ | ||||||
| level: 'warn', | ||||||
| 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 bearer auth.` | |
| msg: `[${username}:${server.name}] Session resume returned 401 without auth provider. Retrying with auth provider enabled.` |
Copilot
AI
Apr 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
includeAuthProvideris derived solely fromshouldPreferSessionResume(presence of a persistedsessionId). SincebuildTransportOptions(..., { includeAuthProvider: false })also strips anyAuthorizationheader fromserver.headers, this changes connection behavior for all streamable_http servers that have a persisted session, including non-OAuth servers that may rely on a staticAuthorizationheader (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/orserver.oauthTemplateexists) and/or avoiding header-stripping when the server is not OAuth-driven.