fix: harden oauth headless flow + add scope override/tests (#72) (thanks @mgonto)
This commit is contained in:
parent
b908c4bbe3
commit
98c300b3dc
@ -4,6 +4,8 @@
|
||||
|
||||
### CLI
|
||||
- Preserve default imports when `mcporter config add` writes a config file, instead of forcing `"imports": []`.
|
||||
- OAuth: avoid crashing on headless Linux when `xdg-open` is unavailable; clear stale dynamic-port client registrations; close callback server if stale-client persistence reads fail. (PR #72, thanks @mgonto)
|
||||
- Added optional `oauthScope`/`oauth_scope` config override as an escape hatch for providers that require explicit scopes.
|
||||
|
||||
### Tooling / Dependencies
|
||||
- Updated dependencies to latest releases (including MCP SDK, Rolldown RC, Zod, Biome, Oxlint, Vitest, Bun types).
|
||||
|
||||
@ -156,6 +156,7 @@ Server definition fields (subset of what `RawEntrySchema` accepts):
|
||||
| `tokenCacheDir` | Directory for OAuth tokens; still honored, but mcporter now keeps a centralized vault in `~/.mcporter/credentials.json` (legacy per-server caches are auto-migrated). Supports `~` expansion. |
|
||||
| `clientName` | Optional identifier some servers use for telemetry/audience segmentation. |
|
||||
| `oauthRedirectUrl` | Override the default localhost callback. Useful when tunneling OAuth through Codespaces or remote dev boxes. |
|
||||
| `oauthScope` | Optional explicit OAuth scope string. If omitted, mcporter lets the MCP SDK derive scope from server/auth metadata. Use this as an escape hatch for providers that require explicit scopes but don’t publish `scopes_supported`. |
|
||||
| `oauthCommand.args` | For STDIO servers that ship a custom auth subcommand (e.g., Gmail MCP). mcporter will spawn the stdio command with these args when you run `mcporter auth <name>`, so you don’t need to call `npx ... auth` manually. |
|
||||
|
||||
mcporter normalizes headers to include `Accept: application/json, text/event-stream` automatically, matching the runtime’s streaming expectations.
|
||||
|
||||
@ -24,6 +24,7 @@ export interface SerializedServerDefinition {
|
||||
readonly tokenCacheDir?: string;
|
||||
readonly clientName?: string;
|
||||
readonly oauthRedirectUrl?: string;
|
||||
readonly oauthScope?: string;
|
||||
}
|
||||
|
||||
export interface CliArtifactMetadata {
|
||||
@ -141,6 +142,7 @@ export function serializeDefinition(definition: ServerDefinition): SerializedSer
|
||||
tokenCacheDir: definition.tokenCacheDir,
|
||||
clientName: definition.clientName,
|
||||
oauthRedirectUrl: definition.oauthRedirectUrl,
|
||||
oauthScope: definition.oauthScope,
|
||||
};
|
||||
}
|
||||
return {
|
||||
@ -157,5 +159,6 @@ export function serializeDefinition(definition: ServerDefinition): SerializedSer
|
||||
tokenCacheDir: definition.tokenCacheDir,
|
||||
clientName: definition.clientName,
|
||||
oauthRedirectUrl: definition.oauthRedirectUrl,
|
||||
oauthScope: definition.oauthScope,
|
||||
};
|
||||
}
|
||||
|
||||
@ -10,6 +10,7 @@ export type SerializedServerDefinition = {
|
||||
tokenCacheDir?: string;
|
||||
clientName?: string;
|
||||
oauthRedirectUrl?: string;
|
||||
oauthScope?: string;
|
||||
env?: Record<string, string>;
|
||||
transport: 'http' | 'stdio';
|
||||
baseUrl?: string;
|
||||
@ -30,6 +31,7 @@ export function serializeDefinition(definition: ServerDefinition): SerializedSer
|
||||
tokenCacheDir: definition.tokenCacheDir,
|
||||
clientName: definition.clientName,
|
||||
oauthRedirectUrl: definition.oauthRedirectUrl,
|
||||
oauthScope: definition.oauthScope,
|
||||
env: definition.env,
|
||||
transport: 'http',
|
||||
baseUrl: definition.command.url.href,
|
||||
@ -44,6 +46,7 @@ export function serializeDefinition(definition: ServerDefinition): SerializedSer
|
||||
tokenCacheDir: definition.tokenCacheDir,
|
||||
clientName: definition.clientName,
|
||||
oauthRedirectUrl: definition.oauthRedirectUrl,
|
||||
oauthScope: definition.oauthScope,
|
||||
env: definition.env,
|
||||
transport: 'stdio',
|
||||
command: definition.command.command,
|
||||
|
||||
@ -15,6 +15,7 @@ export function normalizeServerEntry(
|
||||
const tokenCacheDir = normalizePath(raw.tokenCacheDir ?? raw.token_cache_dir);
|
||||
const clientName = raw.clientName ?? raw.client_name;
|
||||
const oauthRedirectUrl = raw.oauthRedirectUrl ?? raw.oauth_redirect_url ?? undefined;
|
||||
const oauthScope = raw.oauthScope ?? raw.oauth_scope ?? undefined;
|
||||
const oauthCommandRaw = raw.oauthCommand ?? raw.oauth_command;
|
||||
const oauthCommand = oauthCommandRaw ? { args: [...oauthCommandRaw.args] } : undefined;
|
||||
const headers = buildHeaders(raw);
|
||||
@ -58,6 +59,7 @@ export function normalizeServerEntry(
|
||||
tokenCacheDir,
|
||||
clientName,
|
||||
oauthRedirectUrl,
|
||||
oauthScope,
|
||||
oauthCommand: defaultedOauthCommand,
|
||||
source,
|
||||
sources,
|
||||
|
||||
@ -62,6 +62,8 @@ export const RawEntrySchema = z.object({
|
||||
client_name: z.string().optional(),
|
||||
oauthRedirectUrl: z.string().optional(),
|
||||
oauth_redirect_url: z.string().optional(),
|
||||
oauthScope: z.string().optional(),
|
||||
oauth_scope: z.string().optional(),
|
||||
oauthCommand: z
|
||||
.object({
|
||||
args: z.array(z.string()),
|
||||
@ -133,6 +135,7 @@ export interface ServerDefinition {
|
||||
readonly tokenCacheDir?: string;
|
||||
readonly clientName?: string;
|
||||
readonly oauthRedirectUrl?: string;
|
||||
readonly oauthScope?: string;
|
||||
readonly oauthCommand?: {
|
||||
readonly args: string[];
|
||||
};
|
||||
|
||||
45
src/oauth.ts
45
src/oauth.ts
@ -33,22 +33,21 @@ function createDeferred<T>(): Deferred<T> {
|
||||
}
|
||||
|
||||
// openExternal attempts to launch the system browser cross-platform.
|
||||
function openExternal(url: string) {
|
||||
const platform = process.platform;
|
||||
function openExternal(url: string, platform: NodeJS.Platform = process.platform, launch: typeof spawn = spawn) {
|
||||
const stdio = 'ignore';
|
||||
try {
|
||||
if (platform === 'darwin') {
|
||||
const child = spawn('open', [url], { stdio, detached: true });
|
||||
const child = launch('open', [url], { stdio, detached: true });
|
||||
child.unref();
|
||||
} else if (platform === 'win32') {
|
||||
const child = spawn('cmd', ['/c', 'start', '""', url], {
|
||||
const child = launch('cmd', ['/c', 'start', '""', url], {
|
||||
stdio,
|
||||
detached: true,
|
||||
});
|
||||
child.unref();
|
||||
} else {
|
||||
try {
|
||||
const child = spawn('xdg-open', [url], { stdio, detached: true });
|
||||
const child = launch('xdg-open', [url], { stdio, detached: true });
|
||||
child.on('error', () => {}); // swallow ENOENT on headless servers
|
||||
child.unref();
|
||||
} catch {
|
||||
@ -88,6 +87,8 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
|
||||
// (resource metadata scopes_supported or auth server scopes_supported).
|
||||
// Hardcoding 'mcp:tools' breaks providers like Granola whose auth server
|
||||
// does not recognise that scope value.
|
||||
// If oauthScope is explicitly configured, prefer that exact value.
|
||||
...(definition.oauthScope !== undefined ? { scope: definition.oauthScope || undefined } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
@ -139,17 +140,17 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
|
||||
if (usesDynamicPort) {
|
||||
try {
|
||||
const cachedClient = await persistence.readClientInfo();
|
||||
if (cachedClient && Array.isArray((cachedClient as Record<string, unknown>).redirect_uris)) {
|
||||
const cachedRedirect = ((cachedClient as Record<string, unknown>).redirect_uris as string[])[0];
|
||||
if (cachedRedirect && cachedRedirect !== redirectUrl.toString()) {
|
||||
logger.info(
|
||||
`Redirect URI changed (${cachedRedirect} → ${redirectUrl.toString()}); clearing stale client registration.`
|
||||
);
|
||||
await persistence.clear('client');
|
||||
}
|
||||
const cachedRedirect = firstRedirectUri(cachedClient);
|
||||
if (cachedRedirect && cachedRedirect !== redirectUrl.toString()) {
|
||||
logger.info(
|
||||
`Redirect URI changed (${cachedRedirect} → ${redirectUrl.toString()}); clearing stale client registration.`
|
||||
);
|
||||
await persistence.clear('client');
|
||||
}
|
||||
} catch (error) {
|
||||
server.close();
|
||||
await new Promise<void>((resolve) => {
|
||||
server.close(() => resolve());
|
||||
});
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
@ -322,3 +323,19 @@ export interface OAuthLogger {
|
||||
warn(message: string): void;
|
||||
error(message: string, error?: unknown): void;
|
||||
}
|
||||
|
||||
function firstRedirectUri(client: OAuthClientInformationMixed | undefined): string | undefined {
|
||||
if (!client || typeof client !== 'object') {
|
||||
return undefined;
|
||||
}
|
||||
const redirectUris = (client as Record<string, unknown>).redirect_uris;
|
||||
if (!Array.isArray(redirectUris)) {
|
||||
return undefined;
|
||||
}
|
||||
const [first] = redirectUris;
|
||||
return typeof first === 'string' ? first : undefined;
|
||||
}
|
||||
|
||||
export const __oauthInternals = {
|
||||
openExternal,
|
||||
};
|
||||
|
||||
@ -35,4 +35,37 @@ describe('config normalization', () => {
|
||||
expect(headers?.accept?.toLowerCase()).toContain('application/json');
|
||||
expect(headers?.accept?.toLowerCase()).toContain('text/event-stream');
|
||||
});
|
||||
|
||||
it('normalizes oauthScope from camelCase and snake_case keys', async () => {
|
||||
await fs.mkdir(TEMP_DIR, { recursive: true });
|
||||
const configPath = path.join(TEMP_DIR, 'mcporter-oauth-scope.json');
|
||||
await fs.writeFile(
|
||||
configPath,
|
||||
JSON.stringify(
|
||||
{
|
||||
mcpServers: {
|
||||
camel: {
|
||||
baseUrl: 'https://example.com/mcp',
|
||||
auth: 'oauth',
|
||||
oauthScope: 'openid profile',
|
||||
},
|
||||
snake: {
|
||||
baseUrl: 'https://example.com/mcp',
|
||||
auth: 'oauth',
|
||||
oauth_scope: 'email',
|
||||
},
|
||||
},
|
||||
},
|
||||
null,
|
||||
2
|
||||
),
|
||||
'utf8'
|
||||
);
|
||||
|
||||
const servers = await loadServerDefinitions({ configPath });
|
||||
const camel = servers.find((entry) => entry.name === 'camel');
|
||||
const snake = servers.find((entry) => entry.name === 'snake');
|
||||
expect(camel?.oauthScope).toBe('openid profile');
|
||||
expect(snake?.oauthScope).toBe('email');
|
||||
});
|
||||
});
|
||||
|
||||
@ -17,6 +17,7 @@ describe('config render helpers', () => {
|
||||
tokenCacheDir: '/tmp/cache',
|
||||
clientName: 'mcporter',
|
||||
oauthRedirectUrl: 'https://example.com/callback',
|
||||
oauthScope: 'openid profile',
|
||||
env: { FOO: 'bar' },
|
||||
};
|
||||
|
||||
@ -30,6 +31,7 @@ describe('config render helpers', () => {
|
||||
tokenCacheDir: '/tmp/cache',
|
||||
clientName: 'mcporter',
|
||||
oauthRedirectUrl: 'https://example.com/callback',
|
||||
oauthScope: 'openid profile',
|
||||
env: { FOO: 'bar' },
|
||||
source: { kind: 'import', path: '/tmp/source.json' },
|
||||
});
|
||||
|
||||
29
tests/oauth-open-external.test.ts
Normal file
29
tests/oauth-open-external.test.ts
Normal file
@ -0,0 +1,29 @@
|
||||
import { EventEmitter } from 'node:events';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
import { __oauthInternals } from '../src/oauth.js';
|
||||
|
||||
describe('openExternal', () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it('swallows xdg-open error events on linux', () => {
|
||||
const child = new EventEmitter() as EventEmitter & { unref: () => void };
|
||||
child.unref = vi.fn();
|
||||
const launch = vi.fn(() => child as unknown as ReturnType<typeof import('node:child_process').spawn>);
|
||||
|
||||
expect(() =>
|
||||
__oauthInternals.openExternal(
|
||||
'https://example.com/auth',
|
||||
'linux',
|
||||
launch as unknown as typeof import('node:child_process').spawn
|
||||
)
|
||||
).not.toThrow();
|
||||
expect(launch).toHaveBeenCalledWith('xdg-open', ['https://example.com/auth'], {
|
||||
stdio: 'ignore',
|
||||
detached: true,
|
||||
});
|
||||
expect(() => child.emit('error', Object.assign(new Error('ENOENT'), { code: 'ENOENT' }))).not.toThrow();
|
||||
expect(child.unref).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@ -1,4 +1,5 @@
|
||||
import fs from 'node:fs/promises';
|
||||
import http from 'node:http';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
@ -33,4 +34,93 @@ describe('FileOAuthClientProvider session lifecycle', () => {
|
||||
await session.close();
|
||||
await expect(waitPromise).rejects.toThrow(/closed before receiving authorization code/i);
|
||||
});
|
||||
|
||||
it('uses oauthScope when explicitly configured', async () => {
|
||||
const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
|
||||
tempDirs.push(tokenCacheDir);
|
||||
const definition: ServerDefinition = {
|
||||
name: 'test-oauth-scope',
|
||||
description: 'Test OAuth server',
|
||||
command: { kind: 'http', url: new URL('https://example.com/mcp') },
|
||||
auth: 'oauth',
|
||||
tokenCacheDir,
|
||||
oauthScope: 'openid email profile',
|
||||
};
|
||||
const logger = {
|
||||
info: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
error: vi.fn(),
|
||||
};
|
||||
|
||||
const session = await createOAuthSession(definition, logger);
|
||||
expect((session.provider as { clientMetadata: { scope?: string } }).clientMetadata.scope).toBe(
|
||||
'openid email profile'
|
||||
);
|
||||
await session.close();
|
||||
});
|
||||
|
||||
it('clears stale client registrations when redirect URI changes with dynamic ports', async () => {
|
||||
const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
|
||||
tempDirs.push(tokenCacheDir);
|
||||
await fs.writeFile(
|
||||
path.join(tokenCacheDir, 'client.json'),
|
||||
JSON.stringify({ redirect_uris: ['http://127.0.0.1:9999/callback'] }, null, 2),
|
||||
'utf8'
|
||||
);
|
||||
const definition: ServerDefinition = {
|
||||
name: 'test-oauth-stale-client',
|
||||
description: 'Test OAuth server',
|
||||
command: { kind: 'http', url: new URL('https://example.com/mcp') },
|
||||
auth: 'oauth',
|
||||
tokenCacheDir,
|
||||
};
|
||||
const logger = {
|
||||
info: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
error: vi.fn(),
|
||||
};
|
||||
|
||||
const session = await createOAuthSession(definition, logger);
|
||||
await session.close();
|
||||
|
||||
await expect(fs.readFile(path.join(tokenCacheDir, 'client.json'), 'utf8')).rejects.toMatchObject({
|
||||
code: 'ENOENT',
|
||||
});
|
||||
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('clearing stale client registration'));
|
||||
});
|
||||
|
||||
it('closes the callback server when stale-client reads throw', async () => {
|
||||
const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
|
||||
tempDirs.push(tokenCacheDir);
|
||||
await fs.writeFile(path.join(tokenCacheDir, 'client.json'), '{not-valid-json', 'utf8');
|
||||
const definition: ServerDefinition = {
|
||||
name: 'test-oauth-read-failure',
|
||||
description: 'Test OAuth server',
|
||||
command: { kind: 'http', url: new URL('https://example.com/mcp') },
|
||||
auth: 'oauth',
|
||||
tokenCacheDir,
|
||||
};
|
||||
const logger = {
|
||||
info: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
error: vi.fn(),
|
||||
};
|
||||
|
||||
const originalCreateServer = http.createServer.bind(http);
|
||||
const createdServers: http.Server[] = [];
|
||||
const createServerSpy = vi.spyOn(http, 'createServer').mockImplementation((...args) => {
|
||||
const server = originalCreateServer(...args);
|
||||
createdServers.push(server);
|
||||
return server;
|
||||
});
|
||||
|
||||
try {
|
||||
await expect(createOAuthSession(definition, logger)).rejects.toThrow(SyntaxError);
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
expect(createdServers).toHaveLength(1);
|
||||
expect(createdServers[0]?.listening).toBe(false);
|
||||
} finally {
|
||||
createServerSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Loading…
Reference in New Issue
Block a user