From 9ee100e40d7c1b1746e1bb84e051589f0961edeb Mon Sep 17 00:00:00 2001 From: LurigeLars <201361928+LurigeLars@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:27:14 +0200 Subject: [PATCH] fix(local-mcp): reuse backend across short-lived clients --- .github/workflows/local-gateway-security.yml | 3 + deploy/local-gateway/README.md | 9 +-- deploy/local-gateway/local-mcp/bridge.mjs | 46 ++++++++++--- .../local-gateway/local-mcp/stdio-proxy.mjs | 33 +++------ .../test/local-backend-lifecycle.test.mjs | 68 +++++++++++++++++++ .../test/local-stdio-proxy.test.mjs | 62 ++++++++++++++++- 6 files changed, 180 insertions(+), 41 deletions(-) create mode 100644 deploy/local-gateway/test/local-backend-lifecycle.test.mjs diff --git a/.github/workflows/local-gateway-security.yml b/.github/workflows/local-gateway-security.yml index 206a81d..92bbdc9 100644 --- a/.github/workflows/local-gateway-security.yml +++ b/.github/workflows/local-gateway-security.yml @@ -44,6 +44,9 @@ jobs: - name: Check upstream tool inventory against gateway policy run: node --test deploy/local-gateway/test/upstream-tool-inventory.test.mjs + - name: Check shared backend lifecycle with concurrent clients + run: node --test deploy/local-gateway/test/local-backend-lifecycle.test.mjs + - name: Exercise isolated stdin-to-tmpfs runtime injection shell: bash run: | diff --git a/deploy/local-gateway/README.md b/deploy/local-gateway/README.md index 84bb8bf..5e3f4f4 100644 --- a/deploy/local-gateway/README.md +++ b/deploy/local-gateway/README.md @@ -65,13 +65,14 @@ Use `test` after deployment changes or upstream image updates. `local-mcp/stdio-proxy.mjs` serves local MCP clients without touching the gateway, its tmpfs import or the private backend network: ```text -local client --stdio--> stdio-proxy.mjs --HTTP 127.0.0.1--> ephemeral github-mcp container +local clients --stdio--> stdio-proxy.mjs --HTTP 127.0.0.1--> shared local github-mcp container ``` - The proxy reads the PAT with the existing `Read-DpapiSecret` in a child `pwsh` and keeps it in process memory only. -- The ephemeral backend uses the exact image pin from `compose.yaml`, runs in HTTP mode with the same hardening, publishes only on `127.0.0.1` and gets the PAT solely as a per-request `Authorization` header. It is never in a file, command-line argument, Docker `.Config.Env` or log. +- The shared local backend uses the exact image pin from `compose.yaml`, runs in HTTP mode with the same hardening, publishes only on `127.0.0.1` and gets the PAT solely as a per-request `Authorization` header. It is never in a file, command-line argument, Docker `.Config.Env` or log. - Local clients see the full upstream tool surface. The ChatGPT discovery hiding is not applied because local clients have no first-party GitHub connector to deduplicate against. Denied tools, the `LurigeLars` write-owner restriction and Secret Scanning redaction from `public/policy.mjs` still apply. -- Each proxy owns one labelled container, removes it on exit and removes orphans left by dead proxy processes. +- Clients with the same pinned image and non-secret configuration reuse one labelled `github-mcp-local-stdio-*` container, including concurrent clients and background discovery. Closing a client does not remove the backend. The first client after Docker stops starts the same container again; no new daemon or scheduled task is required. +- Dead legacy per-process backends are still removed by owner PID. Shared backends use a separate label so legacy proxies cannot remove them. Image/config changes get a separate backend; retire the previous one explicitly once its clients have exited. Register the runtime copy (after `sync-runtime.ps1`) in each client: @@ -121,4 +122,4 @@ separate policy for capabilities that must actually be blocked. For `get_file_contents` calls targeting repositories owned by `LurigeLars`, the gateway converts upstream MCP embedded **text** resources into ordinary MCP text content before returning the response to ChatGPT. This avoids the per-conversation attachment-materialization prompt for small text files while keeping the official GitHub MCP server unchanged. -The rewrite is intentionally narrow: binary resources, large-file resource links, directory listings and repositories owned by anyone else are left unchanged. \ No newline at end of file +The rewrite is intentionally narrow: binary resources, large-file resource links, directory listings and repositories owned by anyone else are left unchanged. diff --git a/deploy/local-gateway/local-mcp/bridge.mjs b/deploy/local-gateway/local-mcp/bridge.mjs index 586962d..95ba029 100644 --- a/deploy/local-gateway/local-mcp/bridge.mjs +++ b/deploy/local-gateway/local-mcp/bridge.mjs @@ -1,5 +1,7 @@ -// Pure building blocks for the local stdio proxy (Claude Code, Claude Desktop, Codex). -// No process, Docker or secret-store side effects live here so they can be tested in CI. +// Building blocks for the local stdio proxy (Claude Code, Claude Desktop, Codex). +// Docker access is injected; this module never launches processes or reads secrets. + +import { createHash } from 'node:crypto'; import { checkRequestPolicy, @@ -9,6 +11,7 @@ import { export const LOCAL_CONTAINER_LABEL = 'com.lurigelars.github-mcp-local.stdio'; export const LOCAL_OWNER_PID_LABEL = 'com.lurigelars.github-mcp-local.stdio.owner-pid'; +export const SHARED_CONTAINER_LABEL = 'com.lurigelars.github-mcp-local.shared-stdio'; export const BACKEND_PORT = 8082; // The pinned official image is read from compose.yaml so the local proxy can never @@ -21,12 +24,11 @@ export function parsePinnedImage(composeText) { // The GitHub PAT is deliberately absent: the backend runs in HTTP mode and receives // the token only as a per-request Authorization header from this process. -export function buildDockerRunArgs({ image, configFile, name, ownerPid }) { +export function buildDockerRunArgs({ image, configFile, name }) { return [ - 'run', '-d', '--rm', + 'run', '-d', '--name', name, - '--label', `${LOCAL_CONTAINER_LABEL}=1`, - '--label', `${LOCAL_OWNER_PID_LABEL}=${ownerPid}`, + '--label', `${SHARED_CONTAINER_LABEL}=1`, '--publish', `127.0.0.1::${BACKEND_PORT}`, '--user', '65532:65532', '--read-only', @@ -44,11 +46,35 @@ export function buildDockerRunArgs({ image, configFile, name, ownerPid }) { ]; } -export function parseDockerPort(output) { - for (const line of String(output).split(/\r?\n/)) { - const m = /^127\.0\.0\.1:(\d+)\s*$/.exec(line.trim()); - if (m) return Number(m[1]); +// A backend belongs to its pinned image/config, not to a short-lived MCP client. +// shortcut: old image/config backends need explicit retirement after their clients exit. +export function ensureLocalBackend({ image, configFile, configText, docker }) { + const key = createHash('sha256').update(image).update('\0').update(configText).digest('hex').slice(0, 16); + const name = `github-mcp-local-stdio-${key}`; + const inspect = () => docker(['inspect', '--format', + `{"image":{{json .Config.Image}},"shared":{{json (index .Config.Labels "${SHARED_CONTAINER_LABEL}")}},"running":{{json .State.Running}}}`, name]); + let state = inspect(); + if (state.status !== 0) { + docker(buildDockerRunArgs({ image, configFile, name })); + // A concurrent client may have won the name; inspect that same backend. + state = inspect(); + } + if (state.status !== 0) throw new Error('failed to create the shared local GitHub MCP backend'); + const backend = JSON.parse(state.stdout); + if (backend.image !== image || backend.shared !== '1') throw new Error('shared local GitHub MCP backend identity mismatch'); + if (backend.running === false) { + if (docker(['start', name]).status !== 0) throw new Error('failed to start the shared local GitHub MCP backend'); + } else if (backend.running !== true) { + throw new Error('invalid shared local GitHub MCP backend state'); } + const port = docker(['port', name, `${BACKEND_PORT}/tcp`]); + if (port.status !== 0) throw new Error('failed to resolve the shared local GitHub MCP backend port'); + return { name, endpoint: `http://127.0.0.1:${parseDockerPort(port.stdout)}/` }; +} + +export function parseDockerPort(output) { + const m = /^127\.0\.0\.1:(\d+)$/.exec(String(output).trim()); + if (m && Number(m[1]) > 0 && Number(m[1]) <= 65535) return Number(m[1]); throw new Error('local GitHub MCP backend is not published on 127.0.0.1'); } diff --git a/deploy/local-gateway/local-mcp/stdio-proxy.mjs b/deploy/local-gateway/local-mcp/stdio-proxy.mjs index d9fdedc..6806a64 100644 --- a/deploy/local-gateway/local-mcp/stdio-proxy.mjs +++ b/deploy/local-gateway/local-mcp/stdio-proxy.mjs @@ -2,7 +2,7 @@ // // Secret path (unchanged storage, same pattern as github-mcp.ps1): // DPAPI file -> Read-DpapiSecret in a child pwsh -> stdout pipe -> this process memory -// -> per-request Authorization header over 127.0.0.1 to an ephemeral official backend. +// -> per-request Authorization header over 127.0.0.1 to a shared official backend. // The PAT is never written to a file, command-line argument, Docker Config.Env, image // layer or log. The production gateway, its tmpfs import and its private backend network // are not touched. The backend image is the exact pin from compose.yaml. @@ -13,15 +13,12 @@ import { spawnSync } from 'node:child_process'; import { readFileSync, existsSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import readline from 'node:readline'; -import { randomBytes } from 'node:crypto'; import { fileURLToPath } from 'node:url'; import { LOCAL_CONTAINER_LABEL, LOCAL_OWNER_PID_LABEL, - BACKEND_PORT, - buildDockerRunArgs, createForwarder, - parseDockerPort, + ensureLocalBackend, parsePinnedImage, validateRuntimeToken, } from './bridge.mjs'; @@ -46,7 +43,7 @@ for (const path of [configFile, secretStore]) { } const image = parsePinnedImage(readFileSync(join(ROOT, 'compose.yaml'), 'utf8')); -const docker = (args, options = {}) => spawnSync('docker', args, { encoding: 'utf8', windowsHide: true, ...options }); +const docker = args => spawnSync('docker', args, { encoding: 'utf8', windowsHide: true, timeout: 30_000 }); function readPatFromDpapi() { const literal = secretStore.replaceAll("'", "''"); @@ -84,17 +81,7 @@ function removeOrphanedBackends() { } let token = readPatFromDpapi(); -const name = `github-mcp-stdio-${process.pid}-${randomBytes(3).toString('hex')}`; -let started = false; - -function cleanup() { - token = null; - if (started) { - started = false; - docker(['rm', '-f', name]); - } -} -process.on('exit', cleanup); +process.on('exit', () => { token = null; }); for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) process.on(sig, () => process.exit(0)); process.on('uncaughtException', () => { log('unexpected proxy error; shutting down'); @@ -103,13 +90,9 @@ process.on('uncaughtException', () => { removeOrphanedBackends(); -const run = docker(buildDockerRunArgs({ image, configFile, name, ownerPid: process.pid })); -if (run.status !== 0) fail('failed to start the local GitHub MCP backend container'); -started = true; - -const portOut = docker(['port', name, `${BACKEND_PORT}/tcp`]); -if (portOut.status !== 0) fail('failed to resolve the local GitHub MCP backend port'); -const endpoint = `http://127.0.0.1:${parseDockerPort(portOut.stdout)}/`; +const { name, endpoint } = ensureLocalBackend({ + image, configFile, configText: readFileSync(configFile, 'utf8'), docker, +}); async function waitReady() { const deadline = Date.now() + 30_000; @@ -124,7 +107,7 @@ async function waitReady() { fail('local GitHub MCP backend did not become reachable'); } await waitReady(); -log(`ready (${image.split('@')[0]}, loopback backend)`); +log(`ready (${image.split('@')[0]}, shared loopback backend ${name})`); const emit = obj => process.stdout.write(JSON.stringify(obj) + '\n'); const handleLine = createForwarder({ diff --git a/deploy/local-gateway/test/local-backend-lifecycle.test.mjs b/deploy/local-gateway/test/local-backend-lifecycle.test.mjs new file mode 100644 index 0000000..616409c --- /dev/null +++ b/deploy/local-gateway/test/local-backend-lifecycle.test.mjs @@ -0,0 +1,68 @@ +// Real Docker regression: background discovery and concurrent clients must reuse +// one backend. A unique non-secret config keeps this test separate from runtime. +import assert from 'node:assert/strict'; +import { execFile, spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { promisify } from 'node:util'; +import test from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { parsePinnedImage } from '../local-mcp/bridge.mjs'; + +const exec = promisify(execFile); +const root = path.resolve(import.meta.dirname, '..'); +const image = parsePinnedImage(fs.readFileSync(path.join(root, 'compose.yaml'), 'utf8')); +const bridgeUrl = pathToFileURL(path.join(root, 'local-mcp/bridge.mjs')).href; +const docker = args => spawnSync('docker', args, { encoding: 'utf8', windowsHide: true, timeout: 30_000 }); + +test('concurrent and successive client processes reuse the same container, including after stop', { timeout: 90_000 }, async () => { + const temp = fs.mkdtempSync(path.join(os.tmpdir(), 'github-mcp-lifecycle-')); + const configFile = path.join(temp, 'github.env'); + fs.writeFileSync(configFile, `GITHUB_TOOLSETS=repos\n# isolated lifecycle test ${path.basename(temp)}\n`); + const client = ` + import { spawnSync } from 'node:child_process'; + import { readFileSync } from 'node:fs'; + import { ensureLocalBackend } from ${JSON.stringify(bridgeUrl)}; + const configFile = ${JSON.stringify(configFile)}; + const docker = args => spawnSync('docker', args, { encoding: 'utf8', windowsHide: true, timeout: 30000 }); + console.log(JSON.stringify(ensureLocalBackend({ image: ${JSON.stringify(image)}, configFile, + configText: readFileSync(configFile, 'utf8'), docker }))); + `; + const connect = async () => JSON.parse((await exec(process.execPath, ['--input-type=module', '-e', client], { windowsHide: true, timeout: 40_000 })).stdout); + let name; + try { + const clients = await Promise.allSettled(Array.from({ length: 4 }, connect)); + name = clients.find(r => r.status === 'fulfilled')?.value.name; + for (const result of clients) assert.equal(result.status, 'fulfilled', result.reason?.message); + const first = clients[0].value; + assert.ok(clients.every(r => r.value.name === first.name)); + const inspect = () => { + const result = docker(['inspect', '--format', '{{.Id}} {{.RestartCount}} {{.HostConfig.AutoRemove}}', name]); + assert.equal(result.status, 0); + return result.stdout.trim(); + }; + const original = inspect(); + assert.match(original, /^[a-f0-9]{64} 0 false$/); + assert.deepEqual(await connect(), first); + assert.equal(inspect(), original, 'client process exit must leave the same backend running'); + assert.equal(docker(['stop', name]).status, 0); + const restarted = await connect(); + assert.equal(restarted.name, name); + assert.equal(inspect(), original, 'Docker stop/reconnect must start the existing container'); + + const replies = []; + const { createForwarder } = await import('../local-mcp/bridge.mjs'); + const forward = createForwarder({ endpoint: restarted.endpoint, getToken: () => 'ghp_' + '0'.repeat(36), emit: reply => replies.push(reply), timeoutMs: 5000 }); + for (let attempt = 0; attempt < 40; attempt++) { + await forward(JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/list' })); + if (replies.at(-1)?.result?.tools) break; + await new Promise(resolve => setTimeout(resolve, 250)); + } + assert.ok(replies.at(-1)?.result?.tools.length > 0, 'shared backend must answer MCP tools/list'); + } finally { + if (name) assert.equal(docker(['rm', '-f', name]).status, 0); + fs.unlinkSync(configFile); + fs.rmdirSync(temp); + } +}); diff --git a/deploy/local-gateway/test/local-stdio-proxy.test.mjs b/deploy/local-gateway/test/local-stdio-proxy.test.mjs index b94af75..cc622a4 100644 --- a/deploy/local-gateway/test/local-stdio-proxy.test.mjs +++ b/deploy/local-gateway/test/local-stdio-proxy.test.mjs @@ -6,9 +6,11 @@ import test from 'node:test'; import { buildDockerRunArgs, createForwarder, + ensureLocalBackend, parseDockerPort, parsePinnedImage, validateRuntimeToken, + SHARED_CONTAINER_LABEL, } from '../local-mcp/bridge.mjs'; import { CHATGPT_HIDDEN_TOOLS, rewriteResponse } from '../public/policy.mjs'; @@ -26,7 +28,7 @@ test('local backend uses the exact compose image pin', () => { }); test('docker run arguments never carry credentials and bind only to loopback', () => { - const args = buildDockerRunArgs({ image: parsePinnedImage(compose), configFile: 'C:\\x\\config\\github.env', name: 'n', ownerPid: 1 }); + const args = buildDockerRunArgs({ image: parsePinnedImage(compose), configFile: 'C:\\x\\config\\github.env', name: 'n' }); const joined = args.join(' '); assert.equal(args.includes('-e'), false); assert.equal(args.includes('--env'), false); @@ -34,11 +36,65 @@ test('docker run arguments never carry credentials and bind only to loopback', ( assert.equal(args.filter(a => a === '--env-file').length, 1); assert.ok(args[args.indexOf('--env-file') + 1].endsWith('github.env')); assert.ok(args.includes('127.0.0.1::8082')); - for (const flag of ['--read-only', '--rm']) assert.ok(args.includes(flag)); + assert.ok(args.includes('--read-only')); + assert.equal(args.includes('--rm'), false, 'client exit must not remove the shared backend'); + assert.ok(args.includes(`${SHARED_CONTAINER_LABEL}=1`)); + assert.doesNotMatch(joined, /owner-pid/); assert.deepEqual(args.slice(args.indexOf('--cap-drop'), args.indexOf('--cap-drop') + 2), ['--cap-drop', 'ALL']); assert.deepEqual(args.slice(-3), ['http', '--port=8082', '--listen-host=0.0.0.0']); }); +test('successive clients reuse one backend and a stopped backend is started without recreation', () => { + const image = parsePinnedImage(compose); + const calls = []; + let exists = false; + let running = true; + const docker = args => { + calls.push(args); + if (args[0] === 'inspect') return exists + ? { status: 0, stdout: JSON.stringify({ image, shared: '1', running }) } + : { status: 1 }; + if (args[0] === 'run') { exists = true; return { status: 0 }; } + if (args[0] === 'start') { running = true; return { status: 0 }; } + if (args[0] === 'port') return { status: 0, stdout: '127.0.0.1:49152\n' }; + assert.fail('unexpected Docker operation: ' + args[0]); + }; + const connect = () => ensureLocalBackend({ image, configFile: 'github.env', configText: 'GITHUB_TOOLSETS=repos', docker }); + const first = connect(); + assert.deepEqual(connect(), first); + running = false; + assert.deepEqual(connect(), first); + assert.equal(calls.filter(args => args[0] === 'run').length, 1); + assert.equal(calls.filter(args => args[0] === 'start').length, 1); + assert.equal(first.endpoint, 'http://127.0.0.1:49152/'); +}); + +test('a concurrent creator winning the shared name is reused', () => { + const image = parsePinnedImage(compose); + let inspections = 0; + const docker = args => { + if (args[0] === 'inspect') return ++inspections === 1 + ? { status: 1 } + : { status: 0, stdout: JSON.stringify({ image, shared: '1', running: true }) }; + if (args[0] === 'run') return { status: 125 }; + if (args[0] === 'port') return { status: 0, stdout: '127.0.0.1:49152\n' }; + assert.fail('unexpected Docker operation: ' + args[0]); + }; + assert.equal(ensureLocalBackend({ image, configFile: 'github.env', configText: '', docker }).endpoint, 'http://127.0.0.1:49152/'); +}); + +test('wrong backend identity and Docker failure fail closed', () => { + const image = parsePinnedImage(compose); + for (const stdout of [JSON.stringify({ image: 'wrong-image', shared: '1', running: true }), JSON.stringify({ image, shared: '', running: true })]) { + assert.throws(() => ensureLocalBackend({ image, configFile: 'github.env', configText: '', + docker: args => { assert.equal(args[0], 'inspect'); return { status: 0, stdout }; }, + }), /identity mismatch/); + } + assert.throws(() => ensureLocalBackend({ image, configFile: 'github.env', configText: '', + docker: () => ({ status: 1 }), + }), /failed to create/); +}); + test('proxy source keeps the DPAPI path and never persists or logs the PAT', () => { assert.match(proxy, /Read-DpapiSecret/); assert.match(proxy, /github_pat\.dpapi/); @@ -52,6 +108,8 @@ test('proxy source keeps the DPAPI path and never persists or logs the PAT', () test('helpers validate port output and runtime token shape', () => { assert.equal(parseDockerPort('127.0.0.1:49152\n'), 49152); assert.throws(() => parseDockerPort('0.0.0.0:49152\n')); + assert.throws(() => parseDockerPort('127.0.0.1:49152\n0.0.0.0:49152\n')); + assert.throws(() => parseDockerPort('127.0.0.1:65536\n')); assert.equal(validateRuntimeToken(` ${FAKE_TOKEN}\r\n`), FAKE_TOKEN); assert.throws(() => validateRuntimeToken('')); assert.throws(() => validateRuntimeToken('short'));