fix(smoke): make the /inspect auth probe independent of concurrent traffic
Some checks failed
CI/CD / typecheck (pull_request) Successful in 1m3s
CI/CD / lint (pull_request) Successful in 2m5s
CI/CD / test (pull_request) Successful in 1m17s
CI/CD / build (pull_request) Successful in 2m12s
CI/CD / smoke (pull_request) Failing after 2m42s
CI/CD / publish (pull_request) Has been skipped
Some checks failed
CI/CD / typecheck (pull_request) Successful in 1m3s
CI/CD / lint (pull_request) Successful in 2m5s
CI/CD / test (pull_request) Successful in 1m17s
CI/CD / build (pull_request) Successful in 2m12s
CI/CD / smoke (pull_request) Failing after 2m42s
CI/CD / publish (pull_request) Has been skipped
The probe waited for a 3s socket-inactivity timeout and treated the rejection as proof the endpoint was reachable. That only holds when nothing else is talking to mcplocal — /inspect streams every project's MCP traffic, so any other smoke file running concurrently keeps the socket busy and the timeout never fires. Adding one more traffic-generating smoke file was enough to tip it into a 10s test timeout; it passed in isolation the whole time. Resolve on response headers instead. That removes the dependence on the rest of the suite being quiet, and it is a stronger assertion than before: the test now reads the real status code and content-type, where a timeout-means-success probe would have passed just as happily against a slow 401. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5NAoE7VJA5TWvHsVfEmUr
This commit is contained in:
@@ -32,6 +32,16 @@ function httpRequest(opts: {
|
||||
headers?: Record<string, string>;
|
||||
body?: string;
|
||||
timeout?: number;
|
||||
/**
|
||||
* Resolve as soon as response headers arrive, without waiting for the body
|
||||
* to end. Required for endpoints that never end: an SSE stream's socket only
|
||||
* goes idle when nothing else is talking to mcplocal, so waiting for the
|
||||
* inactivity timeout made this depend on whether other smoke files happened
|
||||
* to be generating traffic concurrently. Resolving on headers is also a
|
||||
* STRONGER assertion — the caller sees the real status instead of inferring
|
||||
* "reachable" from a timeout, which would pass just as happily on a slow 401.
|
||||
*/
|
||||
resolveOnHeaders?: boolean;
|
||||
}): Promise<{ status: number; headers: http.IncomingHttpHeaders; body: string }> {
|
||||
return new Promise((resolve, reject) => {
|
||||
const parsed = new URL(opts.url);
|
||||
@@ -46,6 +56,12 @@ function httpRequest(opts: {
|
||||
timeout: opts.timeout ?? 10_000,
|
||||
},
|
||||
(res) => {
|
||||
if (opts.resolveOnHeaders === true) {
|
||||
resolve({ status: res.statusCode ?? 0, headers: res.headers, body: '' });
|
||||
res.destroy();
|
||||
req.destroy();
|
||||
return;
|
||||
}
|
||||
const chunks: Buffer[] = [];
|
||||
res.on('data', (chunk: Buffer) => chunks.push(chunk));
|
||||
res.on('end', () => {
|
||||
@@ -92,22 +108,19 @@ describe('Smoke: Security — mcplocal unauthenticated endpoints', () => {
|
||||
if (!available) return;
|
||||
|
||||
// /inspect streams ALL MCP traffic (tool calls, arguments, responses)
|
||||
// for ALL projects to any unauthenticated local client
|
||||
// for ALL projects to any unauthenticated local client. The stream never
|
||||
// ends, so take the status off the response headers and hang up.
|
||||
const res = await httpRequest({
|
||||
url: `${MCPLOCAL_URL}/inspect`,
|
||||
method: 'GET',
|
||||
headers: { 'Accept': 'text/event-stream' },
|
||||
timeout: 3_000,
|
||||
}).catch((err) => {
|
||||
// Timeout is expected (SSE keeps connection open) — still means endpoint is accessible
|
||||
if ((err as Error).message.includes('timed out')) {
|
||||
return { status: 200, headers: {} as http.IncomingHttpHeaders, body: '' };
|
||||
}
|
||||
throw err;
|
||||
timeout: 5_000,
|
||||
resolveOnHeaders: true,
|
||||
});
|
||||
|
||||
// Should be accessible without auth (documenting the vulnerability)
|
||||
expect(res.status).toBeLessThan(400);
|
||||
expect(res.headers['content-type']).toContain('text/event-stream');
|
||||
console.log(` ⚠ /inspect accessible without auth (status ${res.status})`);
|
||||
}, 10_000);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user