Skip to content

Add optional Streamable HTTP transport - #35

Open
caoimghgin wants to merge 1 commit into
conorbronsdon:mainfrom
caoimghgin:feat/http-transport
Open

Add optional Streamable HTTP transport#35
caoimghgin wants to merge 1 commit into
conorbronsdon:mainfrom
caoimghgin:feat/http-transport

Conversation

@caoimghgin

Copy link
Copy Markdown

Summary

  • Adds an opt-in Streamable HTTP transport (MCP_TRANSPORT=http) alongside the existing stdio default, for deployments that run the server as a persistent networked service (e.g. behind mcp-remote) instead of a per-session subprocess.
  • Implemented with node:http directly (no new runtime dependencies) rather than the SDK's Express helper, since Express isn't otherwise a dependency here and ships no bundled types.
  • Stateless mode: each request gets its own McpServer instance, so there's no cross-request session state to lose on a restart.
  • Adds GET /health for container healthchecks alongside POST /mcp.
  • stdio remains the default — no behavior change for existing configs.

Test plan

  • npm run lint — clean
  • npm run build — clean
  • npm test — 169/169 passing, including new src/__tests__/http.test.ts covering health check, MCP initialize over HTTP, 405 on GET /mcp, 404 on unknown paths
  • Manual smoke test: ran the built server with MCP_TRANSPORT=http, confirmed /health, a full initialize handshake, and method-not-allowed behavior

🤖 Generated with Claude Code

Enables running substack-mcp as a persistent networked service (behind
mcp-remote, for example) instead of only as a per-session stdio
subprocess, matching how other MCP servers here are deployed. Opt in
with MCP_TRANSPORT=http; stdio stays the default.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@conorbronsdon

Copy link
Copy Markdown
Owner

Thanks for the submissions @caoimghgin! Taking a look

@conorbronsdon conorbronsdon left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes at 4b24904ef5c098df16bc7b0fed33249c261f40a5 for two correctness/security blockers in the new HTTP transport:

  1. The server accepts MCP requests without validating Origin or Host. This is especially risky with the default 0.0.0.0 bind: a malicious webpage or another reachable host can send requests to the transport. The Streamable HTTP transport specification requires Origin validation to prevent DNS-rebinding attacks. Please reject untrusted origins/hosts (or use the SDK's DNS-rebinding protection with an explicit allowlist) and add tests proving allowed and rejected requests.

  2. readJsonBody() buffers request chunks without a byte limit before SDK validation. A client can therefore consume unbounded process memory with one oversized request. Please enforce a documented request-body limit while streaming, return an appropriate client error when exceeded, and cover the boundary in tests.

I verified these against the exact head with authenticated Claude Opus 5 and thinkingmachines/inkling:free. The implementation builds, lints, and its focused HTTP tests pass; these paths are simply not covered yet.

@conorbronsdon

Copy link
Copy Markdown
Owner

Concrete regression coverage I would use for this blocker in src/__tests__/http.test.ts:

  • Send a valid MCP initialize request with Origin: https://attacker.example; expect the configured policy to reject it (typically 403) and assert the MCP server factory was not invoked.
  • Send the same request with an explicitly allowed origin/host; expect 200. A missing Origin should remain compatible with non-browser MCP clients unless the chosen policy intentionally requires it.
  • Send a request body exactly at the configured byte limit; it should reach normal JSON/MCP handling.
  • Send a body one byte over the limit; expect a client error such as 413 and assert the MCP server factory was not invoked.
  • Exercise the over-limit case with chunked input rather than relying only on Content-Length, so the test proves the streaming accumulator itself is bounded.

The implementation can expose policy/limit options through startHttpServer() or central configuration; the important invariants are fail-closed validation before server creation and a byte counter enforced while chunks arrive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants