feat(mcp): harden HTTP transport (help text, auth logging, timeouts, bind warning) (§2)#2607
Merged
Merged
Conversation
…bind warning) Closes the H-1..H-4 must-fix gaps identified in docs/design-documents/20260712-mcp-http-transport.md before the --http transport ships in v0.17 as experimental: - H-1 (AC-11): --http's usage string and Docs().Long said HTTP is served "in addition to stdio" — false, since Execute serves HTTP-only in --http mode (a daemon has no attached stdin). Text now matches behavior and marks the transport EXPERIMENTAL. - H-2 (AC-10/AC-8): every rejected (401) HTTP MCP request now logs method/path/remote_addr/outcome at warn level, and a startup line logs the bound address + auth mode — without ever logging the configured or presented token. - H-3: added IdleTimeout (120s) to the --http server, bounding idle keep-alive connections without a blanket WriteTimeout (which would break streamable HTTP's long-lived responses). - H-4: warn at startup when --http binds a non-loopback address. Adds the missing-coverage tests the design doc calls out: AC-1 (no --http selects stdio, never builds an http.Server), AC-5 (unauthenticated tools/list is 401, not an empty catalog), AC-6 (empty/whitespace token file refused), AC-7 (--allow-mutations gates write tools identically over HTTP, driven end-to-end with a real *sdkmcp.Client), AC-8/AC-10 (auth rejections are logged, token never appears in log output), and AC-12 (graceful shutdown drains via http.Server.Shutdown). Explicitly out of scope (deferred to v0.18 per the design doc): rate limiting, per-agent tokens, token rotation, mTLS. Advances Phase-1 execution plan §2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3cLokwNJYLLBpdyt3cgGr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the H-1..H-4 must-fix hardening items and the missing-coverage tests identified in
docs/design-documents/20260712-mcp-http-transport.mdso the already-merged
conduit mcp --httptransport (#2591) can ship in v0.17 labeledexperimental per that doc's recommendation. This is a hardening pass on existing, already-tested
code (
cmd/conduit/root/mcp/http.go,mcp.go) — no re-implementation, no new transport behavior,no rate limiting/mTLS/per-agent tokens (explicitly deferred to v0.18 by the design doc).
Roadmap: Phase-1 execution plan §2, "HTTP transport security: authn (token) + TLS for the HTTP MCP
surface; documented."
Must-fixes implemented
--http's usage string andDocs().Longclaimed HTTP is served "inaddition to stdio" — false;
Executeserves HTTP-only in--httpmode (a daemon has no attachedstdin to co-serve stdio from). Text now says "INSTEAD OF stdio" and marks the transport
EXPERIMENTAL, pointing at
docs/operations/mcp-server.md.401) HTTP MCP request now logsmethod/path/remote_addr/outcome=unauthorizedatwarn, and a startup line logs the boundaddress + auth mode at
info. Verified by test and by a manual smoke run (see below) that neitherthe configured token nor the presented (wrong) one ever appears in log output.
IdleTimeout: 120sto the--httpserver. NoWriteTimeout— streamable HTTP canlegitimately hold a response open while streaming a tool result, so a blanket write deadline would
be wrong.
warn-level, non-blocking) when--httpresolves to a non-loopbackaddress (including the all-interfaces
:portform). TLS + the bearer token already make thissafe; the warning exists so a typo'd or defaulted-wide bind doesn't go unnoticed.
Missing-coverage tests added (design doc's AC checklist)
cmd/conduit/root/mcp/mcp_test.go): no--httptakes the stdio path and neverconstructs an
http.Server— proven by cancellingctxbeforeExecuteand asserting it returnscontext.Canceledpromptly (the HTTP path would instead failvalidateHTTPConfigwith a distincterror).
tools/listJSON-RPC call is401, not an empty (or any) catalog.--token-fileis refused.--allow-mutationsgatesapply/etc. identically over HTTP as over stdio — drivenend-to-end with a real
*sdkmcp.Clientoverhttptest.NewServer, not just an internal registrycheck.
scans captured log output and fails if the real or presented token ever appears in it.
--httplistener drains cleanly viahttp.Server.Shutdownon cancellation(same call
Execute's errgroup makes) and returnshttp.ErrServerClosed.IdleTimeoutand H-4 non-loopback-warning additions (both thenewHTTPServercall site and thewarnIfNonLoopbackhelper across loopback/IPv6/hostname/all-interfaces address shapes).
Docs
docs/operations/mcp-server.md's "HTTP transport" section rewritten: fixes the same"in addition to stdio" claim, marks the transport EXPERIMENTAL with a link to the design doc,
documents the new startup/warning/auth-failure log lines and timeouts, and lists what's explicitly
not built yet (rate limiting, per-agent tokens, rotation, cipher pinning) so an operator isn't
surprised.
markdownlint-cli2clean.No
CHANGELOG.mdexists in this repo — changelog is generated from the conventional-commit titleper
docs/releases.md.Risk tier
Tier 2 (Features/CLI) — this touches a command/transport surface, not the data path. It can
carry Tier-1
applycalls, but the three independent non-agent-passable gates protecting writes(
--allow-mutations,--api.allow-live-restart-apply, the diff-hash check) are unchanged; this PRonly adds logging/timeouts/text around the existing fail-closed auth gate, it does not touch
mutation authorization logic.
Adversarial self-review
requireBearerToken's constant-time compare and the 401-before-handler orderingare untouched — I only added a
logger.Warn(...)call inside the existingif !ok || !constantTimeEqual(...)branch, before the401is written. Verified the log call references onlyr.Method/r.URL.Path/r.RemoteAddr— neverpresentedortoken. Grepped the diff for bothidentifiers to confirm neither reaches a log call.
validateHTTPConfig/loadBearerToken/tls.LoadX509KeyPaircalls andtheir ordering in
newHTTPServerare unchanged; I only addedwarnIfNonLoopback(a log call, nocontrol flow effect) and the startup
logger.Infocall after those checks already passed. Manuallyverified
--http :0with no--token-file/--tls-cert/--tls-keystill refuses to start(
go run ./cmd/conduit mcp --http :0→--http requires both --token-file and --tls-cert/--tls-key; refusing to serve HTTP with no auth and no TLS, exit 2) before opening thisPR.
--http, hit it withcurlpresenting no token and a wrong token, and inspected the captured log output directly — confirmed
rejected MCP HTTP request: missing or invalid bearer tokenlines carry onlymethod/outcome/path/remote_addr, never the token file's contents or theAuthorizationheader value.
TestRequireBearerToken_RejectionsAreLoggedWithoutLeakingTokenencodes this as astanding regression test (scans captured log output for both the real and the wrong-guessed token
string).
httpLogger) duplicatespkg/conduit/runtime.go'snewLoggerrather thanimporting it —
MCPCommanddoesn't build aconduit.Runtime, and the ecdysis config-parsingpipeline does not populate
Config.Log.NewLogger(verified:ecdysis.ParseConfigunmarshals viaviper from flag/env/file values, and
NewLoggerhas nolongstruct tag, so it's never set on theparsed config — confirmed by reading
ecdysis@v0.6.0/config.go'ssetDefaults/ParseConfig).Duplicating two lines of
zerolog.ParseLevel/log.ParseFormatwas the smaller coupling; flaggingthis as a judgment call in case a reviewer prefers exporting
pkg/conduit.NewLoggerFromConfigorsimilar instead.
Executewith a realsdkmcp.StdioTransport{}and analready-cancelled
context.Context, relying onServer.Run's documented behavior ("blocks until... the provided context is cancelled") to return promptly via the
ctx.Done()select branchwithout needing to actually read from
os.Stdin. The SDK's own cancellation test(
cmd_test.go:TestServerRunContextCancel) uses in-memory transports instead of real stdio to avoidexactly this hazard; I judged the real-stdio version acceptable here because the test asserts a
bounded 10s timeout (never hangs CI, worst case is a deterministic failure) and it passed
repeatedly locally including under
-race. Flagging this as the one test in the PR with a(low, bounded) environment-dependency risk.
it does not change the fail-closed gate; a misconfigured-but-token/TLS-complete
--http :8443stillstarts, just with a warning. This matches the design doc's explicit scope (TLS+token already make
it safe; the warning is about surprising the operator, not blocking a valid config).
Verification
go build ./...— clean.go test -race -count=1 ./cmd/conduit/root/mcp/... ./cmd/conduit/internal/mcp/...— all pass,including the new AC-1/5/6/7/8/10/12 tests and the H-3/H-4 tests.
golangci-lint run ./cmd/conduit/root/mcp/...(v2.12.2, matchingtools/go.mod) — 0 issues.npx --yes markdownlint-cli2@0.18.1 docs/operations/mcp-server.md— 0 errors.conduit mcp --http :18443 --token-file ... --tls-cert ... --tls-key ..., confirmed the non-loopback warning + startup log lines, thencurl'd itunauthenticated and with a wrong token and confirmed
401+ the auth-failure log lines with notoken leakage; confirmed
--httpwith no token/TLS still refuses to start.Process-maturity note
Per CLAUDE.md's honesty rule: this PR's own review is the adversarial self-review pass above
(live now), not a second-session human review. Tier-2 review (one reviewer approval, CI green)
is the applicable bar. Not merging this PR — leaving it open for review.