feat(mcp): add gitnexus mcp --http server with Streamable HTTP and legacy SSE transports#2141
feat(mcp): add gitnexus mcp --http server with Streamable HTTP and legacy SSE transports#2141blueroseslol wants to merge 3 commits into
Conversation
…mable HTTP transports Adds a dedicated MCP-only HTTP server (gitnexus/src/mcp/http-transport.ts) started via `gitnexus mcp --http [--port 3000] [--host 0.0.0.0] [--auth-token <t>]`. Serves modern Streamable HTTP at POST /mcp and legacy SSE at GET /sse + POST /messages, reusing the transport-agnostic createMCPServer(). stdio remains the default for `gitnexus mcp` (no breaking change). - Reuse the proven StreamableHTTP session machinery by extracting createStreamableHttpHandler() from server/mcp-http.ts; mountMCPEndpoints keeps its signature and /api/mcp behavior. - Add createSseHandlers() implementing the legacy SSEServerTransport round-trip. - Optional bearer-token auth middleware; warn when binding non-loopback without a token. Proper CORS (incl. Private Network Access) headers. - Replace console.log in mcp-http.ts with the core logger. - Tests: auth middleware, /health, Streamable + SSE round-trips, unknown-session 404s, auth enforcement, and mountMCPEndpoints refactor safety (16/16 pass).
|
@blueroseslol is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report❌ Some checks failed Pipeline Status
Test Results
❌ 1 failed / 11011 passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
Tri-review: gitnexus mcp --http server
Reviewed three ways — GitNexus swarm (risk, test/CI, security) and Compound-Engineering personas (correctness, adversarial, maintainability, testing), both Claude; and Codex (the one independent engine, ran live). So six Claude lanes + Codex: security findings with Codex + a Claude lane agreeing are strongly corroborated; Claude-only items are "consistent across personas," weighted accordingly below.
🔴 CI is failing — merge blocker (reproduced)
tests / ubuntu / coverage and CI Gate are red, and this is a regression from the PR, not a flake. The four new mcp options (--http, -p/--port, --host, --auth-token) have no entries in help-i18n.ts OPTION_DESCRIPTION_KEYS and no help.option.* keys in en.ts/zh-CN.ts, so test/unit/cli-index-help.test.ts ("localizes every registered CLI command and option description") fails. I reproduced it locally — the assertion surfaces the short --port description ("HTTP port (only with --http). Default: 3000"); the other three are long enough that Commander line-wraps them past the substring check, but all four still render in English inside zh-CN help. Fix: add the four mcp|… mappings (-p, --port can reuse help.option.port) plus the new keys in both bundles.
Security posture (Codex + security + adversarial + risk)
gitnexus mcp --http with no other flags binds 0.0.0.0 unauthenticated, every response sets Access-Control-Allow-Private-Network: true, and CORS uses origin: true (reflects any Origin). The composition means a web page the user merely visits can fetch() the local MCP server cross-origin and read all indexed-repo intelligence. Two independent fixes:
- (a) Don't emit the PNA header / don't reflect arbitrary origins by default (gate behind an explicit opt-in or an Origin allowlist).
- (b) Default
--hostto127.0.0.1(matchingserveandeval-server, which already do) — the current warn-only on non-loopback is not a control.
Other corroborated findings
- Pre-auth body parsing —
express.json({ limit: '10mb' })is global and runs before the per-routeauthmiddleware, so an unauthenticated client forces a 10 MB parse per request; body-parser errors (malformed/oversized JSON) also bypass the handlers' JSON-RPC error envelope and return Express's default error. (Codex P2 + security + adversarial) - Legacy SSE session leak —
createSseHandlershas no TTL sweep, whilecreateStreamableHttpHandlerin the same file does; abandoned/sseconnections leak aServerper session if the socketclosenever fires. The module's own header comment claims sessions are "cleaned up … after SESSION_TTL_MS of inactivity," but the SSE path never implements it (nolastActivity, nosetInterval). (Codex + adversarial, P2) - Timing-unsafe bearer compare —
header === expectedis not constant-time (the comment even advertises length fast-fail). Usecrypto.timingSafeEqual. Codex + security + correctness + risk agree on the issue; security graded P1, risk P3 for a local-tool threat model — posting as P2.
Maintainability (Claude, single-engine but high-confidence)
The new files comment exclusively in Chinese, and mcp-http.ts translated its pre-existing English doc comments to Chinese (pure churn on a file already English-commented like the rest of gitnexus/src); the new test file uses Chinese it() names, which hurt vitest -t discoverability. Separately, mcp/http-transport.ts imports from server/mcp-http.ts, inverting the established server/ → mcp/ layering (no cycle today, but the arrow now points the wrong way — consider moving the factories into the mcp/ layer).
Lower-priority (body-only; Claude single-lane)
- Streamable session map has no cap → an
initializeflood holds aServerper request for the full 30-min TTL (adversarial, P2). POST /mcpwith no session-id + a non-initializebodyconnect()s aServerthat is neverclose()d and never enters the map, so the TTL sweep can't reclaim it (adversarial, P3).Number(options.port)yieldsNaNon--port abcwith no validation, and the--httpbranch lacks the EADDRINUSE try/catch thatserveCommandhas, so a bad port / busy port surfaces as a raw unhandled rejection (correctness/testing, P3).
Test coverage gaps (testing lane, Claude)
The headline feature startMcpHttpServer (194 lines) has zero tests; the single test touching the Streamable POST branch wraps the call in a silent catch {} with no assertions; and createAuthMiddleware is unit-tested in isolation but never verified as actually wired to the /mcp, /sse, /messages routes. A port-0 smoke test (start → GET /health → assert 200; POST /mcp without token → assert 401) would close most of this. The 16 existing unit tests pass in <1 s with no handle leaks in isolation.
Validated / clean — credit where due
- The refactor of
mountMCPEndpointsintocreateStreamableHttpHandler/createSseHandlersis logic-preserving for the existinggitnexus serve/api/mcproute — correctness verified it against the base file and empirically (GET notification stream, DELETE session eviction, 404/400/401 bodies all match), and Codex + risk concurred. - SDK usage is correct for
@modelcontextprotocol/sdk1.29.0, including thehandlePostMessage3rd-arg parsed-body call. - The stdio sentinel is undisturbed in HTTP mode (the logger writes to stderr;
http-transport.tsis dynamically imported after the sentinel installs). - No bidi / zero-width / homoglyph Unicode in executable code (only Chinese comment/string text); root prettier clean. The Vercel check failure is authorization-only, unrelated to the diff.
Automated multi-tool digest (GitNexus swarm + Compound-Engineering + Codex). Two of the three methods are the same engine (Claude); only Codex is independent — weight accordingly. Verify before acting.
| '(Streamable HTTP at POST /mcp + legacy SSE at GET /sse, POST /messages).', | ||
| ) | ||
| .option('--http', 'Serve MCP over HTTP instead of stdio (for remote clients)') | ||
| .option('-p, --port <port>', 'HTTP port (only with --http). Default: 3000', '3000') |
There was a problem hiding this comment.
P1 - CI blocker (reproduced). This option (and --http, --host, --auth-token) has no entry in help-i18n.ts OPTION_DESCRIPTION_KEYS and no help.option.* key in en.ts/zh-CN.ts, so test/unit/cli-index-help.test.ts ("localizes every registered...") fails - tests / ubuntu / coverage + CI Gate are red. The assertion only surfaces this short --port string; the other three line-wrap past its substring check but are equally untranslated in zh-CN help. Fix: add mcp|-p, --port <port> -> help.option.port (reusable) and new keys for the other three. [reproduced]
|
|
||
| // Chrome 130+ Private Network Access 预检支持 | ||
| app.use((_req: Request, res: Response, next: NextFunction) => { | ||
| res.setHeader('Access-Control-Allow-Private-Network', 'true'); |
There was a problem hiding this comment.
P1 security (Codex + security + adversarial + risk). Emitting Access-Control-Allow-Private-Network: true on every response, combined with cors({ origin: true }) below (reflects any Origin) and the default 0.0.0.0 unauthenticated bind, lets any web page the user visits fetch() this local MCP server cross-origin and read all indexed-repo intelligence. Gate the PNA header behind an explicit opt-in and restrict CORS to an allowlist (or deny cross-origin when no auth token is set). [code-read]
| .option('-p, --port <port>', 'HTTP port (only with --http). Default: 3000', '3000') | ||
| .option( | ||
| '--host <host>', | ||
| 'HTTP bind address (only with --http). Default: 0.0.0.0 (all interfaces). Use 127.0.0.1 for loopback only.', |
There was a problem hiding this comment.
P2 security (Codex + security + risk). Default --host of 0.0.0.0 exposes the unauthenticated MCP server on all interfaces out of the box; the warn-only on non-loopback (http-transport.ts:88) is not a control. serve (line 131 above) and eval-server already default to 127.0.0.1 - match them, and require an explicit --host 0.0.0.0 for remote use. [code-read]
| const expected = `Bearer ${authToken}`; | ||
|
|
||
| // 字符串比较（长度不同则快速失败） | ||
| if (typeof header === 'string' && header === expected) { |
There was a problem hiding this comment.
P2 security (Codex + security + correctness + risk). header === expected is not constant-time - it short-circuits on the first differing byte (the comment even advertises length fast-fail), a timing oracle on the bearer token. Use crypto.timingSafeEqual over equal-length buffers. (security graded P1, risk P3 for a loopback dev-tool; P2 here.) [code-read]
| ); | ||
|
|
||
| // 解析 JSON 请求体（最大 10MB） | ||
| app.use(express.json({ limit: '10mb' })); |
There was a problem hiding this comment.
P2 (Codex + security + adversarial). express.json({ limit: '10mb' }) is global and runs before the per-route auth middleware (line 121), so unauthenticated clients force a 10 MB parse per request (pre-auth DoS amplification). Body-parser errors also throw before the route .catch, so malformed/oversized JSON returns Express's default error instead of the JSON-RPC envelope every other path uses. Apply the parser per-route after auth, and add a terminal JSON-RPC error handler. [code-read]
| const transport = new SSEServerTransport(messagesPath, res); | ||
| const server = createMCPServer(backend); | ||
|
|
||
| sseSessions.set(transport.sessionId, { server, transport }); |
There was a problem hiding this comment.
P2 (Codex + adversarial). sseSessions has no TTL sweep or cap - unlike createStreamableHttpHandler in this same file, which runs a setInterval idle eviction. Abandoned /sse connections (network drop where socket close never fires) leak a Server per session indefinitely. The module header comment claims sessions are evicted "after SESSION_TTL_MS of inactivity," but the SSE path never implements it (no lastActivity, no timer). Add a lastActivity field + a sweep mirroring the Streamable handler. [code-read]
| * stdio 仍是 `gitnexus mcp` 的默认模式（无破坏性变更）。 | ||
| * | ||
| * 安全注意事项： | ||
| * - 默认绑定 0.0.0.0（所有接口），如不提供 --auth-token 会输出警告 |
There was a problem hiding this comment.
P1 maintainability (Claude lanes). This new file (and cli/mcp.ts's new block, and the new test's it() names) comments exclusively in Chinese, and server/mcp-http.ts translated its pre-existing English doc comments to Chinese - the rest of gitnexus/src is English-only, so this is a permanent convention break plus pure churn, and Chinese test names hurt vitest -t discoverability. Recommend English comments/test names to match the codebase. [code-read]
|
@blueroseslol I ran out automated review agents and it left these comments. Let me know what you think. |
2 participants
Closes #2142
Summary
feat(mcp): add gitnexus mcp --http server with Streamable HTTP and legacy SSE transports
Full details and test results: blueroseslol#3
Original PR with CI-ready changes: blueroseslol#2