Skip to content

Commit d3d4683

Browse files
author
David Ruzicka
committed
fix(upstream): fail closed on profile lookup miss
- prevent HTTP upstream routing from falling back to this.profile when transport context cannot resolve the requested profileId - report invalid upstream_mcp shapes as upstream config errors instead of misleading missing openapi_spec_path errors - add regression tests for both paths
1 parent e521d7c commit d3d4683

4 files changed

Lines changed: 47 additions & 20 deletions

File tree

src/mcp/mcp-server.test.ts

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3958,7 +3958,7 @@ paths:
39583958

39593959
// -------------------------------------------------------------------------
39603960
describe('getUpstreamMcpConfig HTTP fallback (fix: issue #1)', () => {
3961-
it('falls back to profile.upstream_mcp when httpTransport.getUpstreamMcpConfig returns undefined', async () => {
3961+
it('does not fall back to profile.upstream_mcp when httpTransport.getUpstreamMcpConfig returns undefined', async () => {
39623962
const server = new MCPServer();
39633963
const provider = { name: 'fallback-provider', transport: { type: 'http-streamable', url: 'https://upstream.example.com/mcp' } };
39643964
(server as any).profile = {
@@ -3988,15 +3988,13 @@ paths:
39883988
'fallback-profile',
39893989
) as any;
39903990

3991-
// Should have routed to upstream (not local tools)
3992-
expect(mockClientFn).toHaveBeenCalledWith('session-1', provider, undefined);
3991+
// Fail closed: no upstream routing when the transport context does not own the profile.
3992+
expect(mockClientFn).not.toHaveBeenCalled();
39933993
expect(response.result).toBeDefined();
3994+
expect(response.result.tools).toEqual([]);
39943995
});
39953996

3996-
it('logs warn when fallback fires for profileId not in transport context', async () => {
3997-
// The fallback is safe only in single-profile mode. When it fires for a foreign
3998-
// profileId it returns this.profile.upstream_mcp (potentially the wrong config).
3999-
// The warn log makes this auditable rather than silent.
3997+
it('logs warn when upstream config lookup misses for profileId not in transport context', async () => {
40003998
const logger = new JsonLogger();
40013999
const warnSpy = vi.spyOn(logger, 'warn');
40024000

@@ -4024,9 +4022,10 @@ paths:
40244022
);
40254023

40264024
expect(warnSpy).toHaveBeenCalledWith(
4027-
expect.stringContaining('falling back to this.profile.upstream_mcp'),
4025+
expect.stringContaining('refusing to fall back to this.profile.upstream_mcp'),
40284026
expect.objectContaining({ profileId: 'profile-b' }),
40294027
);
4028+
expect(mockClientFn).not.toHaveBeenCalled();
40304029
});
40314030
});
40324031

src/mcp/mcp-server.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1818,14 +1818,10 @@ export class MCPServer {
18181818
if (this.httpTransport && profileId) {
18191819
const fromTransport = this.httpTransport.getUpstreamMcpConfig(profileId);
18201820
if (fromTransport !== undefined) return fromTransport;
1821-
// Fallback: safe only in single-profile HTTP mode where runHttp() stores the profile
1822-
// under defaultProfileId. If profileId normalization diverges (e.g. startup race),
1823-
// this recovers. In multi-profile mode this would return the wrong profile's config —
1824-
// the warn log makes the mismatch auditable rather than silent.
18251821
if (this.profile?.upstream_mcp) {
1826-
this.logger?.warn('getUpstreamMcpConfig: profileId not found in transport context, falling back to this.profile.upstream_mcp', { profileId });
1822+
this.logger?.warn('getUpstreamMcpConfig: profileId not found in transport context; refusing to fall back to this.profile.upstream_mcp', { profileId });
18271823
}
1828-
return this.profile?.upstream_mcp;
1824+
return undefined;
18291825
}
18301826
// stdio path: upstream_mcp cannot be used without a wired client
18311827
if (!this.upstreamStdioWarnLogged && this.profile?.upstream_mcp && !this.getUpstreamClientFn) {

src/profile/profile-resolver.test.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,7 @@ describe('profile-resolver', () => {
209209
expect(resolved.specPath).toBeUndefined();
210210
});
211211

212-
it('throws missing openapi_spec_path for upstream_mcp with invalid shape (no transport.url)', async () => {
213-
// looksLikeUpstreamMcpProxy rejects objects that don't have transport.type + transport.url,
214-
// so resolveSpecPath treats the profile as non-proxy and throws for missing spec path.
212+
it('throws invalid upstream_mcp for object shape missing transport.url', async () => {
215213
const root = await createTempDir();
216214
const profilesDir = path.join(root, 'profiles');
217215
const profilePath = path.join(profilesDir, 'bad-proxy.json');
@@ -223,7 +221,22 @@ describe('profile-resolver', () => {
223221
upstream_mcp: {},
224222
});
225223

226-
await expect(resolveProfileById('bad-proxy', profilesDir)).rejects.toThrow('openapi_spec_path');
224+
await expect(resolveProfileById('bad-proxy', profilesDir)).rejects.toThrow('invalid upstream_mcp');
225+
});
226+
227+
it('throws invalid upstream_mcp for legacy array shape without openapi_spec_path', async () => {
228+
const root = await createTempDir();
229+
const profilesDir = path.join(root, 'profiles');
230+
const profilePath = path.join(profilesDir, 'legacy-array-proxy.json');
231+
232+
await writeJson(profilePath, {
233+
profile_name: 'legacy-array-proxy',
234+
profile_id: 'legacy-array-proxy',
235+
tools: [],
236+
upstream_mcp: [{ name: 'legacy', transport: { type: 'http-streamable', url: 'https://example.com/mcp' } }],
237+
});
238+
239+
await expect(resolveProfileById('legacy-array-proxy', profilesDir)).rejects.toThrow('invalid upstream_mcp');
227240
});
228241

229242
it('extracts env vars and auth methods for profile index', async () => {

src/profile/profile-resolver.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,7 @@ function resolveSpecPath(
411411
profilePath: string,
412412
specPathRaw?: string,
413413
overrideSpecPath?: string,
414+
hasUpstreamMcp = false,
414415
isUpstreamMcpProxy = false,
415416
): string | undefined {
416417
const trimmed = normalizeSpecPath(specPathRaw);
@@ -422,6 +423,12 @@ function resolveSpecPath(
422423
if (isUpstreamMcpProxy) {
423424
return undefined;
424425
}
426+
if (hasUpstreamMcp) {
427+
throw new ConfigurationError('Profile has invalid upstream_mcp configuration', {
428+
profilePath,
429+
path: 'upstream_mcp',
430+
});
431+
}
425432
throw new ConfigurationError('Profile is missing openapi_spec_path', { profilePath });
426433
}
427434
if (isHttpUrl(trimmed)) {
@@ -574,7 +581,13 @@ export async function resolveProfileById(
574581
}
575582

576583
const match = matches[0];
577-
const specPath = resolveSpecPath(match.profilePath, match.specPathRaw, options?.specPathOverride, match.isUpstreamMcpProxy);
584+
const specPath = resolveSpecPath(
585+
match.profilePath,
586+
match.specPathRaw,
587+
options?.specPathOverride,
588+
match.hasUpstreamMcp,
589+
match.isUpstreamMcpProxy,
590+
);
578591

579592
return {
580593
profileId: match.profileId,
@@ -632,7 +645,13 @@ export async function resolveProfileFromPath(
632645
throw new ConfigurationError('Profile file does not look like a valid profile', { profilePath: resolvedPath });
633646
}
634647

635-
const specPath = resolveSpecPath(resolvedPath, entry.specPathRaw, options?.specPathOverride, entry.isUpstreamMcpProxy);
648+
const specPath = resolveSpecPath(
649+
resolvedPath,
650+
entry.specPathRaw,
651+
options?.specPathOverride,
652+
entry.hasUpstreamMcp,
653+
entry.isUpstreamMcpProxy,
654+
);
636655

637656
return {
638657
profileId: entry.profileId,

0 commit comments

Comments
 (0)