Add optional Streamable HTTP transport - #35
Conversation
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>
|
Thanks for the submissions @caoimghgin! Taking a look |
conorbronsdon
left a comment
There was a problem hiding this comment.
Requesting changes at 4b24904ef5c098df16bc7b0fed33249c261f40a5 for two correctness/security blockers in the new HTTP transport:
-
The server accepts MCP requests without validating
OriginorHost. This is especially risky with the default0.0.0.0bind: 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. -
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.
|
Concrete regression coverage I would use for this blocker in
The implementation can expose policy/limit options through |
Summary
MCP_TRANSPORT=http) alongside the existing stdio default, for deployments that run the server as a persistent networked service (e.g. behindmcp-remote) instead of a per-session subprocess.node:httpdirectly (no new runtime dependencies) rather than the SDK's Express helper, since Express isn't otherwise a dependency here and ships no bundled types.McpServerinstance, so there's no cross-request session state to lose on a restart.GET /healthfor container healthchecks alongsidePOST /mcp.Test plan
npm run lint— cleannpm run build— cleannpm test— 169/169 passing, including newsrc/__tests__/http.test.tscovering health check, MCPinitializeover HTTP, 405 on GET /mcp, 404 on unknown pathsMCP_TRANSPORT=http, confirmed/health, a fullinitializehandshake, and method-not-allowed behavior🤖 Generated with Claude Code